Compiler warning levels

[Blas_Rodriguez_Somoz]

Hello

According to CHANGES file, the warning level was raised to -Wextra in 2.2 update 3 (20100310) but it seems this only happen for tbb tests, not for libraries build or tbbmalloc tests.

Analyzing the warning levels in the last version (4.0 update 2 AKA 20111130) the warning levels are different for each target:

• tbb library -> -Wall -Wno-non-virtual-dtor
• tbbmalloc lib -> -Wno-non-virtual-dtor
• tbbmalloc tests -> -Wall
• tbb tests -> -Wall -Wextra -Wshadow -Wcast-qual -Woverloaded-virtual -Wno-non-virtual-dtor
  or -Wall -Wextra -Wshadow -Wcast-qual -Woverloaded-virtual -Wnon-virtual-dtor

Apparently the differences are not intentional, so I submitted two small patches to raise the levels in tbb library and tbbmalloc tests, to be more similar with the ones in tbb tests.

• Patch to raise to -Wextra, 10 lines modified in 10 source files.
• Patch to raise to -Wshadow, 46 lines in 11 source files.

Those patches are tested to remove all warnings on macos with compiler=gcc/clang and arch=ia32/intel64 with the default defines, of course it can be other warnings with different os/compiler/configuration.

With the patches applied the warning levels (macosx gcc/clang) are:

• tbb library -> -Wall -Wextra -Wshadow -Wno-non-virtual-dtor
• tbbmalloc lib -> -Wno-non-virtual-dtor
• tbbmalloc tests -> -Wall -Wextra -Wshadow
• tbb tests -> -Wall -Wextra -Wshadow -Wcast-qual -Woverloaded-virtual -Wno-non-virtual-dtor
  or -Wall -Wextra -Wshadow -Wcast-qual -Woverloaded-virtual -Wnon-virtual-dtor

The warning level for tbbmalloc library build is intentional according to Makefile.tbbmalloc.

Hope it helps

[RafSchietekat]

I don't know what changes you made (obviously I'm curious), but I like the sound of it. Could you increase the warning levels of tbbmalloc anyway, or did you see something that would make this impossible?

(Edited.)

[Blas_Rodriguez_Somoz]

The changes are always very simple
- (Wextra) mainly "unused parameter" patched with TBB_ASSERT_EX or parameter name comment.
- (Wshadow) variable name changes.

About the levels in tbbmalloc and tbbproxy, it seems someone don't add or even remove the warning levels from those builds, so at least for me, is of a different kind. I think someone of the development team should check it and make the decision to change it or not, I don't know if there is a reason behind.

There is a line in Makefile.tbbmalloc which explicitly removes warnings:

M_CPLUS_FLAGS := $(subst $(WARNING_KEY),,$(M_CPLUS_FLAGS)) $(DEFINE_KEY)__TBB_BUILD=1

Warnings which in fact are not added previously, this seems strange but too clear to doubt about the intention.

And Makefile.tbbproxy simply doesn't add warning options to the compiler flags.

[Blas_Rodriguez_Somoz]

Correction to the previous

I was wrong about warning levels in tbbproxy and M_CPLUS_FLAGS.

I've submitted two patches more (4 total), with which the warning level used for all compilations (macos clang/gcc) will be the same and equal to the level for tests build before the patches (and without supressed warnings).
C := -Wall -Wextra -Wshadow -Wcast-qual
C++ := -Wall -Wextra -Wshadow -Wcast-qual-Woverloaded-virtual -Wnon-virtual-dtor

- (diff_patches_wcastqual) Adds -Wcast-qual -Woverloaded-virtual -Wnon-virtual-dtor to the WARNING_KEY, splits the warnings levels for C and C++ (c++ levels apllied to C throw a warning) and sets WARNING_SUPRESS to empty. patches 5 lines in 3 files
- (diff_same_warnings) Sets the warning levels for tbbmalloc build and deprecated tests build to the general value. patches 32 lines in 12 files

The patches could be valid for any other platform with gcc compiler, although must be reviewed due to code lines which are specific to a platform (f.i. linux).

Hope it helps

[Alexey-Kukanov]

As a general rule, the warning level for tests is higher (and was raised a few times in past, based on user requests) because it applies to the TBB headers, which are included into user code and so compiled by its rules (including warning options). Internal files are not expected to be compiled by most users, and even those who do compile it perhaps could just ignore the warnings since it's not their code.

I personally don't want to raise the bar for internal files to the same level, because in our experience inmost cases the extra warnings serve no good (at least to our team), but fixing/suppressing those adds code clutter, often goes against stylistic preferences of the developers (for example, some *hate* underscores to parameter names just to please -Wshadow),sometimes makes the code harder to read,and somewhat increases development time. As an example, let's take the famous warning against assignments in the condition - heck, it's allowed by the language, Iknow what I am doing,and I want it to be written this way because it reads more naturally! I would not mind in this case to put extra parentheses around the condition, and that helped with some older compilers; but recent compiler versions ignore it. There are other examples, like e.g. keeping the warning about "type punned cast" even though I use reinterpret_cast for it, and etc.

Needless to say, both warnings and the way to suppress it differ from one compiler to another, andsometimes even from one to another version of the same compiler, making development of portable code harder and adding more of "#ifdef" clutter (by the way, another very "useful" warning is "unrecognized pragma" - so I cannot just put #pragma warnings(disable) or #pragma GCC diagnostic ignored, but need to put it into #if/#endif directives). Even the limited number of compilers and platforms that we officially support is enough to drive me crazy sometimes by the warnings that I have to overcome or suppress without a real problem underneath. I wish warnings were standardized (at least to some extent) and there was a standard mechanism to suppress a certain warning in a certain experssion; but I'm afraid it will never happen :(.

Nevertheless, as another general rule, we accept and integrate most of the contributions that reduce the number of warnings, at the very least to acknowledge people for the job done, even if it causes certain discomfort to us. Also we (and me personally) get more tolerance to this stuff over time, so maybe at some point we will apply -Wextra etc. to the library build, but we aren't there yet.

[RafSchietekat]

"some *hate* underscores to parameter names just to please -Wshadow"
If you get rid if all the y's in the my_ prefixes for member variables (which only really makes sense when also using our_ for static member variables, but only conceptually, before you experience the full effect), keeping just m_ (short for "member"), you save enough bytes to recycle them into a_ prefixes for problematic parameter names. When I now look at TBB code, I always start feeling like it was written by those seagulls in Finding Nemo, as the "mine, mine" starts haunting my thoughts...

"let's take the famous warning against assignments in the condition"
A very useful warning indeed, and one you should always suppress with an actual comparison with 0 or NULL or whatever applies rather than redundant parentheses. A lesser known feature of C++ is that you can often do the same thing as in a for loop header, e.g., "while (const size_t n = f()) { [...] }", so many of those assignments in a condition can actually be transformed into this kind of accident-resistant syntax instead, and you get const protection for free if you want it.

"type punned cast"
Easily avoided in a general and conspicuous manner.

[Alexey-Kukanov]

Raf, thanks for your opinion, but let's not go into a discussion of stylistic preferences and coding guidelines :)

The only thing that I would like to clarify is avoiding of punned cast warning. As you may know, in TBB we devised a function that hides ugliness of several casts that in our experience are necessary to suppress the warning. If you think there is a more elegant way to do it, would you mind giving an example? If not, fine. Anyway, my point was not that it cannot be worked around but that in some cases it should not be issued at all, particularly when reinterpret_cast is used.

[RafSchietekat]

"Raf, thanks for your opinion, but let's not go into a discussion of stylistic preferences and coding guidelines :)"
Oops, excuse me for entering the opened door... :-)

"As you may know, in TBB we devised a function that hides ugliness of several casts that in our experience are necessary to suppress the warning."
Using punned_cast instead of reinterpret_cast seems easy enough.

[Alexey-Kukanov]

Quoting Raf Schietekat

Oops, excuse me for entering the opened door... :-)

Yeah, sorry for that :) I really just wanted to illustrate my point of view, and not open a discussion that could easy turn into a holy war :)

It's all about personal preferences and habits; and we know these are not easy to change. I recently read a book about language evolution (Russian, in that case), and there was a viewpoint expressed that any language changes, and more so official ones, first of all make literate people feel not confortable, and often annoyed, because they have to spent more mental effort to read and write in accordance to the new rules. By the way, it is no different from feeling uncomfortable by somebody's illiteracy :). And soon after, I realized that compiler warnings, especiallysome very misguided ones,make me annoyed for very much the same reason - I need to change my habits of reading and writing the code in order to meet the new language rules. The fact that essentially each compiler insists on its own language dialect, and I need to please them all, only exacerbates all that. Time and practice is needed to become comfortable with thechanged rules, be it reading/writing or programming. As a "spelling&grammar fiend", perhaps you understand what I mean :)

[RafSchietekat]

I don't perceive the specific rules you mentioned above as arbitrary, useless or misguided. A mistake between = and == is easily made, very difficult to spot, and has big consequences; I have often been saved by the compiler, myself, although more often before I started to adopt the personal rule to put the const side on the left, as in "if (1==n)". Shadowing a name often results in bugs, etc. They demand more discipline, which is not the same as making actual changes.

It's very different from when a language is being changed by ignorance, fads, etc. Why would it be a good thing to continuously change a language when the result is that people wouldn't be able to read a book written a century earlier? Oh yes, that's right, books are not hip enough to be a concern...

Then again, I'm not generally trying to please a lot of different compilers at the same time, myself.

[IDZ_A_Intel]

[Alexey K.] It's my message, but I somehow managed to submit it "anonymously".

Interestingly, I don't disagree about the warnings being useful - in cases where they find real problems :) But I seem to be less tolerant to situations where they don't. And the more code changes are required to suppress it andthe more readability suffers, the less I am tolerant :)

Perhaps I want compilers to be smarter and don't produce so many false positives. E.g. I think in many cases it can be proved that shadowing does not cause problems, the case of constructor parameter names matching class member names in initialization list being one of such; or when a variable shadows a function, and the usage context matches the variable but does not match the function, etc. It's not much different than when customers ask to reduce the number of false positives reported by Intel Inspector XE - but somehow customers are more tolerant to false positives in compiler warnings than in tool diagnostics.

And, of course, I want to have simple, non-intrusive, elegant ways to switch this or that warning off for a particular expression or a particular line of code. E.g. a pragma that only applies to the next line (and that does not cause warnings by itself, for any compiler!), or a special comment at the end of the line, or some such.

> It's very different from when a language is being changed by ignorance, fads, etc.
(A side note)Actually, I did not mean changes due to ignorance etc. when talked about (non-programming) languages in the previous post. The reasons why a language is changed affect the feelings somewhat, but, interestingly, not much; evenif the intent is good and the changes would be useful, people still feel uncomfortable.

Perhaps it's time for me to stop :) Thank you for being the opponent in this discussion.