Care with disabling warnings

[martin_watt]

I'd like to put in a request that any warnings that are disabled in a TBB header are re-enabled at the end of that header. Currently I only see one example - atomic.h disables warning 4146 and does not re-enable it.

This is potentially alarming for us because we carefully select which warnings we want to see in our code, and then treat all occurrences of such warnings as errors in our build. If we include a header that disables one of these warnings, then we will no longer see that warning in any of our code that includes that header, directly or indirectly. Some Microsoft headers have the same behaviour, which has caused us significant pain in the past, so I'd like to be sure we can avoid it in TBB if at all possible.

For this particular case it would just want a

#pragma warning(default:4146)

near the end of the file. However doing this does open you up to other problems, for example if you disable warning 4146 in another TBB header and then include atomic.h, the warning will be left in an enabled state after the include, meaning you still see the warning even though it looks like you disabled it. For that reason I'd also like to request that disable/default warning regions are as localized as possible, and as a policy warnings should not be disabled until all headers have been included.

And of course ideally, as in the thread below, all warnings would be fixed. :-)

A couple of additional warning we see in our builds with TBB and VC8 are C4244 (conversion from 'uintptr_t' to 'int', possible loss of data) in atomic.h and tbb_machine.h, and C4189 (local variable "count" is initialised but not referenced.)in tbb_machine.h. Might be worth enabling these particular warnings by default in your builds? Looks like they are still present in the latest dev drop.

Thanks!
Martin

[ARCH_R_Intel]

I agree that we should more carefully scope any warnings that we turn off.

I'm not sure rewriting the code to remove all warnings makes sense. We're finding that we need to write weird, obscure, and more complicated code to appease the warnings on various compilers. Warnings are not governed by any standards bodies; compiler writers make up whatever they want. For example, one version of the Microsoft compiler warns about "unary minus operator applied to unsigned type, result still unsigned". That's the whole point of the unsigned type in C. So now we're considering rewriting the offending expression using ~ and two-complement identities, the sort of thing people do in the Obfuscated C Code Contest.

[Alexey-Kukanov]

Martin.Watt:

A couple of additional warning we see in our builds with TBB and VC8 are C4244 (conversion from 'uintptr_t' to 'int', possible loss of data) in atomic.h and tbb_machine.h, and C4189 (local variable "count" is initialised but not referenced.)in tbb_machine.h. Might be worth enabling these particular warnings by default in your builds? Looks like they are still present in the latest dev drop.

Martin, the most recent developer drop from August 15 should contain fixes to avoid C4244 and C4189; please double check. As far as I remember, "warnings are errors" mode is used for test compilation starting from this update; obviously it also covers the headers,and we work further to extend it to the library source files as well. In particular,we are making changes to overcome C4146 in the headers and localize its suppression in source files.

Thanks for the feedback!

[martin_watt]

MADakukanov:

Martin, the most recent developer drop from August 15 should contain fixes to avoid C4244 and C4189; please double check. As far as I remember, "warnings are errors" mode is used for test compilation starting from this update; obviously it also covers the headers,and we work further to extend it to the library source files as well. In particular,we are making changes to overcome C4146 in the headers and localize its suppression in source files.

Yes, it looks like the warnings are fixed in that release. Thanks for the update and for the fixes that are done and planned.

Martin

[Alexey-Kukanov]

In the recent development update, suppression of C4146 is localized in TBB source files and not used in the public headers anymore.