Warnings in Visual Studio 2008: C4512, C4127, C4244, C4100 (#include <concurrent_hash_map>)

[Boris_Dušek]

Hi,

I downloaded TBB (tbb21_20080605oss binary for Windows)and started using concurrent_hash_map. However using it triggers warnings by Visual Studio (I am compiling on the highest warning level). I have a habit to compile without a single warning, so I clearly want to iron them out. Some of these can be easily fixed, so it would be great if the fixes made it to some future release :-)

All warnings I am seeing (along with comment on how/whether it's possible to fix them)

tbb21_20080605oss\include\tbb\concurrent_hash_map.h(585) : warning C4512: 'tbb::concurrent_hash_map::node' : assignment operator could not be generated

- not sure if this is something because of my use of the template, or a general thing; if general, private: node &operator=(const node &); should solve it (I am using concurrent_hash_map with Key = std::wstring and T = std::vector<:DICT::WORDSTAT> and WordStat is a struct of size_t and another struct of 2 std::wstring: WordStat "=" struct { size_t; struct { std::wstring; std::wstring; enum }; };

tbb\concurrent_hash_map.h(730) : warning C4127: conditional expression is constant

tbb\concurrent_hash_map.h(742) : warning C4127: conditional expression is constant

- I don't think this can be solved :-) unless the warning is explicitly disabled locally for the one line with #pragma warning(push,pop) (and #ifdef'ed for _MSC_VER only)

tbb\atomic.h(162) : warning C4244: 'argument' : conversion from 'tbb::internal::atomic_traits::word' to 'char', possible loss of data

- not sure what this means

tbb\concurrent_hash_map.h(578) : warning C4100: 'size' : unreferenced formal parameter

- this is clearly general problem and solvable by commenting size out: (size_t /* size */, node_...)

Boris

[Dmitry_Vyukov]

Quoting - boris.dusek

I downloaded TBB (tbb21_20080605oss binary for Windows)and started using concurrent_hash_map. However using it triggers warnings by Visual Studio (I am compiling on the highest warning level). I have a habit to compile without a single warning, so I clearly want to iron them out. Some of these can be easily fixed, so it would be great if the fixes made it to some future release :-)

Totally agree, presence of warnings usually lends some distrust to a library.

Quoting - boris.dusek

tbbconcurrent_hash_map.h(730) : warning C4127: conditional expression is constant

tbbconcurrent_hash_map.h(742) : warning C4127: conditional expression is constant

- I don't think this can be solved :-) unless the warning is explicitly disabled locally for the one line with #pragma warning(push,pop) (and #ifdef'ed for _MSC_VER only)

First variant:

if ((void)0, constant_flag) ...

Result of operator , () can't be compile-time constant. And '(void)' part is required to shut down gcc about something like 'senseless expression'.

Second variant:

template

T val(T v) {return v;}

if (val(constant_flag)) ...

[Boris_Dušek]

Quoting - Boris Dusek

Some of these can be easily fixed, so it would be great if the fixes made it to some future release :-)

I submitted discussed fixes for C4127, C4100 and C4512 as 3 patches using the "Make a contribution" link.

C4244 still remains to be tackled.

[Boris_Dušek]

Quoting - Dmitriy V'jukov

Totally agree, presence of warnings usually lends some distrust to a library.

I can also think of a more practical reason - the warnings I mentioned create clutter and I have to search for really my own warnings (warnings for *my* code) among warnings that don't belong to me. Add to that that some of these 8 warnings are template ones = long as hell, so I then hesitate to go through the warning list at all. :-) Easily solved though by disabling the 4 warnings for the concurrent_hash_map header only.

First variant:

if ((void)0, constant_flag) ...

Result of operator , () can't be compile-time constant. And '(void)' part is required to shut down gcc about something like 'senseless expression'.

- so is the result being computed at runtime?

Second variant:

template

T val(T v) {return v;}

if (val(constant_flag)) ...

- again, is it computed at compile-time?

If yes, I have a strange feeling of changing the resulting binary code that will execute based on compiler warnings that can be solved statically (i.e. with #pragma warning). Does g++ as well produce the "conditional expression constant" warning?

PS.: How do I reply to more than 1 part of a quote? Hitting enter when cursor is at some point of the quote does not split the quote at that place, rather it adds the newline to the quote itself.

[Dmitry_Vyukov]

Quoting - Boris Dusek

First variant:

if ((void)0, constant_flag) ...

Result of operator , () can't be compile-time constant. And '(void)' part is required to shut down gcc about something like 'senseless expression'.

- so is the result being computed at runtime?

'compile-time constant' vs. 'run-time value' is more like formal thing here. Every sane modern compiler is able to resolve it in compile-time anyway.

Quoting - Boris Dusek

Second variant:

template

T val(T v) {return v;}

if (val(constant_flag)) ...

- again, is it computed at compile-time?

Again, every sane modern compiler is able to resolve it in compile-time anyway.

Quoting - Boris Dusek

If yes, I have a strange feeling of changing the resulting binary code that will execute based on compiler warnings that can be solved statically (i.e. with #pragma warning).

I don't think that it will affect binary code. However, second variant in *debug* build probably will contain additional function call.

Quoting - Boris Dusek

Does g++ as well produce the "conditional expression constant" warning?

My g++ 3.4.4 with "-Wall" doesn't produce any warnings wrt constant conditional expressions.

[Dmitry_Vyukov]

Quoting - Boris Dusek

PS.: How do I reply to more than 1 part of a quote? Hitting enter when cursor is at some point of the quote does not split the quote at that place, rather it adds the newline to the quote itself.

It's easy.

Quoting - Boris Dusek

PS.: How do I reply to more than 1 part of a quote? Hitting enter when cursor is at some point of the quote does not split the quote at that place, rather it adds the newline to the quote itself.

Press Ctrl+A to select whole post. Press Ctrl+C to copy it.

Quoting - Boris Dusek

PS.: How do I reply to more than 1 part of a quote? Hitting enter when cursor is at some point of the quote does not split the quote at that place, rather it adds the newline to the quote itself.

Then place cursor at bottom, and press Ctrl+V to paste second copy of the post. Now you can add your comments, and delete some parts of quotations as well.

[Boris_Dušek]

Quoting - Dmitriy V'jukov

It's easy.

Press Ctrl+A to select whole post. Press Ctrl+C to copy it.

Thank you,

Quoting -Dmitriy V'jukov

Then place cursor at bottom, and press Ctrl+V to paste second copy of the post. Now you can add your comments, and delete some parts of quotations as well.

As you see, I got it :-)

[Alexey-Kukanov]

Thank youBoris for reporting the issues, and even more for proposing patches - I appreciate it much, asthe right approach to make things happen (as opposed to mere complaints :))

The decision with regard to the warnings in TBB is that for Microsoft Visual C++ our code should have no warnings of level 3. For level 4 warnings, we do not care much because they are usually benign. We have to draw the line somewhere; if you only know how much clutter - otherwise unnecessary braces,different kind of casts, etc- was added to the code to shut down /Wall for all gcc versions we support (starting from 3.2.3 or so); and some these casts for GCC caused MSVC to yell, and|or vice versa.

So for MSVC, /W3seems most reasonable level of warnings, and it is the default one as far as Iremember.However if someof higher level warnings are reported and fixing those doesnot add unnecessary clutter to the code, we fix.

Quoting - Boris Dusek

tbb21_20080605ossincludetbbconcurrent_hash_map.h(585) : warning C4512: 'tbb::concurrent_hash_map::node' : assignment operator could not be generated

tbbconcurrent_hash_map.h(730) : warning C4127: conditional expression is constant

tbbconcurrent_hash_map.h(742) : warning C4127: conditional expression is constant

tbbatomic.h(162) : warning C4244: 'argument' : conversion from 'tbb::internal::atomic_traits::word' to 'char', possible loss of data

tbbconcurrent_hash_map.h(578) : warning C4100: 'size' : unreferenced formal parameter

- this is clearly general problem and solvable by commenting size out: (size_t /* size */, node_...)

For 4512, I think the right way toovercome is to derive the struct node from tbb::no_copy. The warning is there I believe because the mutex variable inside the node is noncopyable already.

For the first of 4127, the way to overcome is probably in declaring grow as non-const automatic variable. I am not sure why it is even declared const now; it won't help any compiler optimization most probably.

The second 4127 is just silly. Yes op_insert is constant; it's the bool template parameter, and it issobecause we want two different implementations to be instantiated from the same code; so dear compiler, instead of issuing a stupid warning, please eliminate the unnecesary code when instantiating the function! Adding 7 lines of code clutter just to workaround this idiocy does not sound right to me. Is this warning issued in debug or in release mode, I wonder?

The 4100 one is reasonable to overcome by avoiding the name for the unusedargument, I agree.

The 4244 in atomic.h would also not be worth the clutter to overcome, but we already have the clutter in place. All you probably need to do is to remove (or comment out) __TBB_x86_64 in the preprocessor condition at line 91.

Could you please check if my proposals work for you?

[uj]

Boostmanages to remove alllevel 4 warnings for the MS C++ compiler so I see no reason why TBB couldn't. It's possible to avoid the warnings like this, but it's annoying,

#pragma warning(push,3)

#include "tbb/task.h"

#pragma warning(pop)

[Alexey-Kukanov]

I did not say that it is impossible, did I? But if adding two pragmas is annoying for you, you might probably understand that adding 7 lines here and there is annoying for me.

Satisfying every warning of every nanny compiler on every level (including such exotics as e.g. -Weffc++, which also could be used by some customers, right?) has some cost in development, and as we already know it adds significantly to the code clutter, so it also costs in support. This is why I said we had to draw the line somewhere; and we selected /W3 for MSVC (the default, as far as I remember) and /Wall for GCC. The rest is considered on case by case.

See also that for five level 4 warnings reported above I agreed that 4 can be fixed rather easily, and it will be done, and only one is so silly that in my opinion it does not deserve the cost of 7 added lines impacting code readability just to suppress it. If there is another, moreaffordable solution, nobody will object.

Or might be we could just decrease the warning level in every TBB header. I will suggest this to the team.

[Boris_Dušek]

Hello Alexey,

Quoting - Alexey Kukanov (Intel)

The decision with regard to the warnings in TBB is that for Microsoft Visual C++ our code should have no warnings of level 3. For level 4 warnings, we do not care much because they are usually benign. We have to draw the line somewhere; if you only know how much clutter - otherwise unnecessary braces,different kind of casts, etc- was added to the code to shut down /Wall for all gcc versions we support (starting from 3.2.3 or so); and some these casts for GCC caused MSVC to yell, and|or vice versa.

So for MSVC, /W3seems most reasonable level of warnings, and it is the default one as far as Iremember.However if someof higher level warnings are reported and fixing those doesnot add unnecessary clutter to the code, we fix.

OK, I see. In my code, I just test MSVC 9, gcc 4.0.1 Apple and gcc 4.1 Linux, so I can imagine that for multiple versions of multiple compilers things are probably different. I confirm /W3 is the default on MSVC, and /W4 is an explicit opt-in by the user.

Quoting -Alexey Kukanov (Intel)

For 4512, I think the right way toovercome is to derive the struct node from tbb::no_copy. The warning is there I believe because the mutex variable inside the node is noncopyable already.

adding #include "tbb_stddef.h" and struct node : public internal::no_copy worked.

Quoting -Alexey Kukanov (Intel)

For the first of 4127, the way to overcome is probably in declaring grow as non-const automatic variable. I am not sure why it is even declared const now; it won't help any compiler optimization most probably.

After looking more closely at it and looking at Microsoft's page about C4127, it's probably improper warning. I guess this warning is meant for cases like "if (false)" or "if (2 + 1 < 4)", i.e. when the expression can be evaluated at compile-time, not when it's constant. I will bug Microsoft VS connect with this (no chance to send patches there :D)

Quoting -Alexey Kukanov (Intel)

The second 4127 is just silly. Yes op_insert is constant; it's the bool template parameter, and it issobecause we want two different implementations to be instantiated from the same code; so dear compiler, instead of issuing a stupid warning, please eliminate the unnecesary code when instantiating the function! Adding 7 lines of code clutter just to workaround this idiocy does not sound right to me. Is this warning issued in debug or in release mode, I wonder?

I tested with Debug, and now with Release, in both configs the warning happens. Release even has one more warning I did not see in Debug:

tbbspin_rw_mutex.h(130) : warning C4245: 'argument' : conversion from 'const tbb::spin_rw_mutex_v3::state_t' to 'unsigned int', signed/unsigned mismatch

I did verify that Dmitriy's proposal which includes much less clutter - "if ((void)0, op_insert)" - works for MSVC 9.

Quoting -Alexey Kukanov (Intel)

The 4244 in atomic.h would also not be worth the clutter to overcome, but we already have the clutter in place. All you probably need to do is to remove (or comment out) __TBB_x86_64 in the preprocessor condition at line 91.

This worked.

Quoting -Alexey Kukanov (Intel)

Could you please check if my proposals work for you?

All in all, when I combined all the suggestions as you approved them, really the only warning I got was the "silly" instance of C4127. That's a very nice improvement - now I need to disable only 1 warning when including the header, opposed to 4 warnings.

Thanks for your response and looking into this. If I find some easily fixable /W4 stuff in other headers, I will let the forum know (and send patches again).

Boris

[uj]

Quoting - Alexey Kukanov (Intel)

I did not say that it is impossible, did I? But if adding two pragmas is annoying for you, you might probably understand that adding 7 lines here and there is annoying for me.

Satisfying every warning of every nanny compiler on every level (including such exotics as e.g. -Weffc++, which also could be used by some customers, right?) has some cost in development, and as we already know it adds significantly to the code clutter, so it also costs in support. This is why I said we had to draw the line somewhere; and we selected /W3 for MSVC (the default, as far as I remember) and /Wall for GCC. The rest is considered on case by case.

See also that for five level 4 warnings reported above I agreed that 4 can be fixed rather easily, and it will be done, and only one is so silly that in my opinion it does not deserve the cost of 7 added lines impacting code readability just to suppress it. If there is another, moreaffordable solution, nobody will object.

Or might be we could just decrease the warning level in every TBB header. I will suggest this to the team.

Well, quality is about attention to detail and going the extra mile. If you feel it's finer that lots of TBB usershave to clutter their code with silly work-arounds just to save the TBB team some work then so be it.

[uj]

I don't seem to be able to edit the above post but it should be "If you feel it's fine"

[ARCH_R_Intel]

We will see what we can do about the /W4 warnings. However, our first priority is delivering reliable code. Some of the suggested work arounds amount to the tail wagging the dog. The intent of warnings is to improve code quality, not make it worse.If the warnings cause us to ship worse code, then it is the compiler that is broken. To me, complexity is worse if simplicity suffices. I don't care whether a compiler can optimize away "if((void)0,constant)" or identity functions. To me it is completely unacceptable to introduce such obfuscations to placate a miguided nanny. We will make a best effort to remove as many warnings as we can without becoming self destructive about it.

I'm speaking as a former compiler optimizer writer and (more importantly) multiple award winner for obfuscation :-)

- Arch

[Alexey-Kukanov]

I could add the following to Arch's words: to overcome a warning in a code that is otherwise perfectly compliant, usually you change the code to tell the compiler what is your intent, sometimes with veryartificial syntax. If the change allows a human reader to better understand your intent as well, the quality is improved. However if the change distracts a human reader from understanding the intent, code quality is reduced.

For example, let's consider C4512 above. The compiler couldn't generate an assignment, so if one would tryto usean assignment somewhere in the code, the compiler would issue an error. Now a programmer explicitly coded operator= to be private, or derived the class from e.g. boost::noncopyable. An attempt to use an assignment still leads to an error, as before - nothing changed except no warning. So did code quality improve? Yes! It improved, because the intent is now clear for a human looking at the class declaration.

On the contrary, overcoming C4127 with the suggested workarounds would decrease code readability, and so decrease, not improve, the quality. If there is a way to shut the warning down while increasing or at leastkeepingreadability, we would be glad to add it.

Despite factual reduce in code readability in some cases, in the headers we do overcome (or, rarely, suppress) warnings on standard levels recommended by compiler documentation, i.e. /W3 for MSVC and -Wall for GCC, and also -w1 for Intel C++ compiler (ICC), which means "all but remarks"; and we are testing with multiple versions of each compiler. Might be we are not as cool as Boost, butstill reasonably good:).

If someone opts for satisfying every warning beyond the standard level, he/she factually agrees to take and manage some additional pain, for whatever sake. Sometimes, overcoming warnings in 3rd party headers is part of the pain. As an example, we compile all TBB code and tests with -strict_ansi option for ICC, which enforces stricter standard-compliant syntax rules. Unfortunately, standard headers (such as ) do not comply to these rules (as seen by ICC) on some Linux systems. So we had to complicate our makefiles to overcome this, by degrading the rule to -ansi.

While we are interested and committed to make TBB easy to use for as many customers as possible, other forces and principles,including those outlined above,impact our decisions. I hope this makes sense to our users.

[uj]

Quoting - Arch Robison (Intel)

We will see what we can do about the /W4 warnings. However, our first priority is delivering reliable code. Some of the suggested work arounds amount to the tail wagging the dog. The intent of warnings is to improve code quality, not make it worse.If the warnings cause us to ship worse code, then it is the compiler that is broken. To me, complexity is worse if simplicity suffices. I don't care whether a compiler can optimize away "if((void)0,constant)" or identity functions. To me it is completely unacceptable to introduce such obfuscations to placate a miguided nanny. We will make a best effort to remove as many warnings as we can without becoming self destructive about it.

I'm speaking as a former compiler optimizer writer and (more importantly) multiple award winner for obfuscation :-)

- Arch

Well to me it's very simple. You delivera high quality product and it shouldn't haveblemishes. If Boost can produce /W4 warning free code, so can you.

It's not adeep philosophical question. It's about costumer perceived quality. If you buy a brand new car you expect the painting to be shining, not covered withsmall scratches.

[uj]

Quoting - Alexey Kukanov (Intel)

I could add the following to Arch's words: to overcome a warning in a code that is otherwise perfectly compliant, usually you change the code to tell the compiler what is your intent, sometimes with veryartificial syntax. If the change allows a human reader to better understand your intent as well, the quality is improved. However if the change distracts a human reader from understanding the intent, code quality is reduced.

For example, let's consider C4512 above. The compiler couldn't generate an assignment, so if one would tryto usean assignment somewhere in the code, the compiler would issue an error. Now a programmer explicitly coded operator= to be private, or derived the class from e.g. boost::noncopyable. An attempt to use an assignment still leads to an error, as before - nothing changed except no warning. So did code quality improve? Yes! It improved, because the intent is now clear for a human looking at the class declaration.

On the contrary, overcoming C4127 with the suggested workarounds would decrease code readability, and so decrease, not improve, the quality. If there is a way to shut the warning down while increasing or at leastkeepingreadability, we would be glad to add it.

Despite factual reduce in code readability in some cases, in the headers we do overcome (or, rarely, suppress) warnings on standard levels recommended by compiler documentation, i.e. /W3 for MSVC and -Wall for GCC, and also -w1 for Intel C++ compiler (ICC), which means "all but remarks"; and we are testing with multiple versions of each compiler. Might be we are not as cool as Boost, butstill reasonably good:).

If someone opts for satisfying every warning beyond the standard level, he/she factually agrees to take and manage some additional pain, for whatever sake. Sometimes, overcoming warnings in 3rd party headers is part of the pain. As an example, we compile all TBB code and tests with -strict_ansi option for ICC, which enforces stricter standard-compliant syntax rules. Unfortunately, standard headers (such as ) do not comply to these rules (as seen by ICC) on some Linux systems. So we had to complicate our makefiles to overcome this, by degrading the rule to -ansi.

While we are interested and committed to make TBB easy to use for as many customers as possible, other forces and principles,including those outlined above,impact our decisions. I hope this makes sense to our users.

I think you're confusing the real issue here.To put it bluntly. You'repassingyour***** on to the consumer of your code. Nobody is interested in why you're doing it, just stop doing it.

[ARCH_R_Intel]

Quoting - uj

Well to me it's very simple. You delivera high quality product and it shouldn't haveblemishes. If Boost can produce /W4 warning free code, so can you.

It's not adeep philosophical question. It's about costumer perceived quality. If you buy a brand new car you expect the painting to be shining, not covered withsmall scratches.

I've modified my private copy to eliminate the /W4 warnings from VS2005. I still need to run it by VS2008. VS2003 turns out to be impossible to reasonably appease in some cases.

Used car dealers are famous for putting making an internally damaged car shiney. Or evenrepainting a wreck and selling it. A shiney paint job does not mean the car is any good. /W4 is checking the paint.

/W4 can't even check the paint right. The quality issue is with the compiler, not the library. The compiler is issuing unreasonable warnings because it is being sloppy about analysis and written for C programs, not modern templated C++. Of hundreds of /W4 warnings I eliminated, only one said anything reasonable, forthe new class thread_specific_enumerable. I find real issues by code review much more efficiently than wading through these misguided warnings.

Some of the more obnoxious /W4 warnings that I ran into with VS2005.

• += applied to two short operands generates a warning. There is no work-around except to give up on using += or use a pragma.
• Use of compiler-time constants in conditional expressions, particularly in templates.
• Just plain wrong warnings about parameters not being used when in fact they were as arguments to explicit destructor calls.
• A just plain wrong warning about a local variable possibly being used before it is defined.
• A warning about defining a volatile variable but not using it. That's crazy: the whole point of the volatile keyword is to say that there might be hiddenreads/writes for the variable.
• The compiler saying it could not generate operator= for a class, even though I never needed it. This warning goes against Stroustrup's design point that C++ should not prevent potential errors, only actual errors.
• Assignments in the second arm of a || expression. The alternatives ways to write that sort of thing involve gotos, which I consider to be worse.
• A warning about "unreachable code" after a throw expression. There was only whitespace after the throw, so any code generated there was the compiler's fault.

As I said, we'll put an end to the warnings. But it won't improve quality one bit, and clutters the sources with pragmas specific to one vendor's compiler.

- Arch

[Alexey-Kukanov]

Quoting - Arch Robison (Intel)

As I said, we'll put an end to the warnings. But it won't improve quality one bit, and clutters the sources with pragmas specific to one vendor's compiler.

We might now start waiting for GCC users to come and require us fixing warnings beyond -Wall.Since GCC does not support warning suppression by pragmas, the code clutter will theninclude GCC-specific attributes (hidden by macros, for portability) and warning suppression options in makefiles. The latter will be particularly bad because the suppression is made globally this way.

Quoting - uj

I think you're confusing the real issue here.To put it bluntly. You'repassingyour***** on to the consumer of your code. Nobody is interested in why you're doing it, just stop doing it.

See the above warning analysis by Arch, then go and say that sentense to your compiler vendor. Whatever I wrote abovewas not for excuse but to educate others that using /W4 in productionis silly and does not add to quality.

[Alexey-Kukanov]

PossiblyI might sound bad in my last sentense; if someone was offended by my words I apologize for that. What I really should have said is:

"Whatever I wrote abovewas not for excuse but to express my opinion about correlation between code quality and warnings, as well my feeling of /W4 as not quite appropriate to use in productionbecause it does not add much to quality and may even reduce it if every warning of this level is worked around.

[robert_jay_gould]

Quoting - Alexey Kukanov (Intel)

PossiblyI might sound bad in my last sentense; if someone was offended by my words I apologize for that. What I really should have said is:

"Whatever I wrote abovewas not for excuse but to express my opinion about correlation between code quality and warnings, as well my feeling of /W4 as not quite appropriate to use in productionbecause it does not add much to quality and may even reduce it if every warning of this level is worked around.

I agree on your general idea, I too use moderntemplatedC++, and I compile my code on 3 compilers right now, and try to keep everyone happy, but anything beyond /w3 on VS2005 or -Wall reduces code quality in my perspective. Most of these warnings are more like moral advice given to young programmers than actual C++ warnings. And the obfuscation required to silence these warnings just ruins themaintainability/literacy/value of code. But that's because of my philosophy for spartan and literate coding.