On Sep 23, 2014, at 11:44 AM, Borislav Petkov <bp@xxxxxxxxx> wrote: > On Tue, Sep 23, 2014 at 05:24:22PM +0000, Rustad, Mark D wrote: >> Yes, but I think there are a few cases where it could be helpful. When >> there is something exceptional that will throw a warning. In one of the >> patches that Jeff sent, I used the DIAG_CLANG_IGNORE macro to suppress >> the warning that is thrown for every entry of the syscall table when >> compiled with clang. The code is right and doing exactly what is wanted, >> so note the exception and make it shut up. > > So we're doing some dancing around solely to shut up the compiler? Even > if the code is correct?!? Sorry, this is just ass backwards. Well, please consider the specifics. The entire syscall table is initialized with a constant pattern to be sure that every item is initialized. Then each syscall is initialized into its proper place. The compiler is complaining that entries are being initialized twice. Most of the time, that is not done, and so it may catch a patch foulup or something. In this particular case, it is normal and intended. There is nothing wrong, so there is no reason to throw a warning for every single entry in the table. Which is what happens with clang today. So the code is correct, but in general the warning can reveal certain issues. Just not in this particular usage. This happens to be a warning specific to clang at the moment. > If it were me, I'd say even one is too much. Because the very thing > of adding code just to shut up the compiler which generates otherwise > correct code is simply very very wrong in my book. > > Bear in mind that even if initially you have a low number of macro > invocations, that number will grow because people will start using it in > other places too. And all of a sudden, the thing has spread like weed > and there's no removing it anymore. So we better not start in the first > place. That is why it would be more than reasonable for checkpatch to warn on the macro introductions. It is certainly a more significant thing than a line > 80 characters. > Again, we should take compiler warnings with a grain of salt and judge > them only by the quality of the generated code. IMO. I think more than the nature of the executable code matters. The namespace issues revealed by -Wshadow can certainly create nasty surprises over time. But we can't get that value from them when they are lost in an ocean of warnings that are always there and are not the source of any trouble. The problem is that when so many warnings are generated, particularly by include files, even useful warnings will not be seen. Specifically silencing ones that are deemed to be "ok" will help in seeing ones that are not. The silencing has the greatest effect in the include files, since there is such a multiplier effect. Warnings that no one looks at, or bother to generate at all, have absolutely no value. Even a silenced warning continues to wear its shame attribute (macro). Hmm. Maybe instead of DIAG_* they should be named SHAME_*. Most of the time, it is new instances of warnings that are most likely to reveal a problem. Hiding them in a flood of "normal" warnings prevents them from ever being seen. And that is a shame. -- Mark Rustad, Networking Division, Intel Corporation
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail
