On Tue, Jun 15, 2010 at 4:56 AM, Jeff Flinn <TriumphSprint2000@hotmail.com> wrote:

Stewart, Robert wrote:

...

Jürgen Hunold wrote:

...

I start using Boost.Units for a new project and found some annyoing warnings when compiling with "-Wall -Wextra" and "-Wconversion"

Don't do that and you won't get those warnings!  (Just kidding.)

...

/home/hunold/src/devel/boost/boost/units/io.hpp: In function 'int boost::units::get_flags(std::ios_base&, int)': /home/hunold/src/devel/boost/boost/units/io.hpp:163: warning: conversion to 'int' from 'long int' may alter its value [-Wconversion]

Patch using an explicit static_cast<int> for the return value attached.

Do you know that the long int value can never exceed the range of int?  If not, the static_cast will simply mask the problem.  It would be better to check against std::numeric_limits<int>::max() before casting, and resort to an exception if the range is exceeded.

IIRC, that's boost::numeric_cast's purpose.

I would not suggest incurring a dependency on another file only to avoid a warning. Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode

AMDG Stewart, Robert wrote:

Jürgen Hunold wrote:

...

/home/hunold/src/devel/boost/boost/units/io.hpp: In function 'int boost::units::get_flags(std::ios_base&, int)': /home/hunold/src/devel/boost/boost/units/io.hpp:163: warning: conversion to 'int' from 'long int' may alter its value [-Wconversion]

Patch using an explicit static_cast<int> for the return value attached.

Do you know that the long int value can never exceed the range of int? If not, the static_cast will simply mask the problem. It would be better to check against std::numeric_limits<int>::max() before casting, and resort to an exception if the range is exceeded.

The maximum value it can have is currently 15. Anyway, I'll change it to use long consistently. In Christ, Steven Watanabe

Emil Dotchevski wrote:

On Tue, Jun 15, 2010 at 3:55 AM, Stewart, Robert <Robert.Stewart@sig.com> wrote:

...

Jürgen Hunold wrote:

...

I start using Boost.Units for a new project and found some annyoing warnings when compiling with "-Wall -Wextra" and "-Wconversion"

...

/home/hunold/src/devel/boost/boost/units/io.hpp: In function 'int boost::units::get_flags(std::ios_base&, int)': /home/hunold/src/devel/boost/boost/units/io.hpp:163: warning: conversion to 'int' from 'long int' may alter its value [-Wconversion]

Patch using an explicit static_cast<int> for the return value attached.

Do you know that the long int value can never exceed the range of int? If not, the static_cast will simply mask the problem.

With or without the static_cast, if the long int value exceeds the range of an int you're screwed. The only difference the cast makes is that you won't get the warning.

You snipped the part in which I suggested an exception to indicate the overflow. That leaves the error handling to the calling code.

The problem with using casts to silence warnings is that silencing the warning is a side effect of altering of the behavior of what is usually a correct program.

I can't quite parse that, but I was advocating against the OP's plan to simply mask the conversion with a cast. I have to wonder if you actually meant to reply to the OP and not me. _____ Rob Stewart robert.stewart@sig.com Software Engineer, Core Software using std::disclaimer; Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.

Steven Watanabe wrote:

Stewart, Robert wrote:

...

Emil Dotchevski wrote:

...

On Tue, Jun 15, 2010 at 3:55 AM, Stewart, Robert <Robert.Stewart@sig.com> wrote:

...

Jürgen Hunold wrote:

...

I start using Boost.Units for a new project and found some annyoing warnings when compiling with "-Wall -Wextra" and "-Wconversion"

Patch using an explicit static_cast<int> for the return value attached.

Do you know that the long int value can never exceed the range of int? If not, the static_cast will simply mask the problem. [snip more discussion]

Humph. Has anyone in this discussion actually looked at the code in question?

I didn't, but note that my reply merely calls on the OP to check whether the range of int will be exceeded.

a) There can be no overflow because the possible values are enumerated.

That means the answer to my query would be affirmative, so the rest of the advice doesn't apply.

b) With the current code, the compiler will implicitly use a static_cast.

Sure, but it does so noisily. _____ Rob Stewart robert.stewart@sig.com Software Engineer, Core Software using std::disclaimer; Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.

Emil Dotchevski wrote:

On Tue, Jun 15, 2010 at 10:03 AM, Steven Watanabe <watanabesj@gmail.com> wrote:

...

Humph. Has anyone in this discussion actually looked at the code in question? a) There can be no overflow because the possible values are enumerated. b) With the current code, the compiler will implicitly use a static_cast.

With the current code, the compiler will use an implicit conversion which is not the same as static_cast in general. Casts are designed to punch holes in the C/C++ type system, which by definition is a dangerous thing to do. In contrast, one has to assume that implicit conversions are a necessary, reasonably safe part of the language.

A static_cast in this context explicitly calls for the otherwise implicit conversion, with the intent of silencing the warning. I think Steven was suggesting that the compiler was applying the same conversion(s) in either case.

Integer implicit conversions specifically are very common in C and C++. For example, incrementing a short int results in two implicit conversions, one of which might lead to truncating the value. That is about as safe (or unsafe) as the code in question, so why nobody cares about that case? Answer: because the compiler doesn't warn.

No, it is because increment has well defined behavior for short that doesn't require a warning. The code in question could have generated -- but doesn't presently -- a value that wouldn't fit in the target type leading to a misinterpretation of the result.

If you don't want your compiler to issue a particular warning, a type cast should be the last solution to reach for, IMO.

It certainly appears that the types can be adjusted to alleviate the problem in this case, but that isn't always possible as impedance mismatches occur that cannot be altered. In such cases, a static_cast, which *should* indicate that the developer examined the code in question and has decided that the potential for truncation is not an issue, is the appropriate recourse to eliminate the warning. Even so, assertions are a wise addition against future maintenance effects. _____ Rob Stewart robert.stewart@sig.com Software Engineer, Core Software using std::disclaimer; Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.

Emil Dotchevski wrote:

On Wed, Jun 16, 2010 at 4:00 AM, Stewart, Robert <Robert.Stewart@sig.com> wrote:

...

Emil Dotchevski wrote:

...

Integer implicit conversions specifically are very common in C and C++. For example, incrementing a short int results in two implicit conversions, one of which might lead to truncating the value. That is about as safe (or unsafe) as the code in question, so why nobody cares about that case? Answer: because the compiler doesn't warn.

No, it is because increment has well defined behavior for short that doesn't require a warning. The code in question could have generated -- but doesn't presently -- a value that wouldn't fit in the target type leading to a misinterpretation of the result.

AFAIK the semantics of incrementing a short are that its value is first implicitly converted to int, which is then incremented (as an int), and then the int result is truncated (assuming sizeof(short)<sizeof(int)) back to short.

That's roughly how it works, yes. Overflow is well defined for unsigned types (3.9.1/4 and 4.7/2), but not signed (4.7/3), so it is possible to overflow s and not know the result portably (because 4.7/3 says the result is implementation defined). Since signed overflow is implementation defined, even without the int-to-short conversion, incrementing a short is a problem.

As far as warnings are concerned, shouldn't this be treated identically to the case when the truncated value was an int to begin with?

Yes, that could just as easily warrant a warning. Practically speaking, it would be most annoying, though it would encourage using int rather than short!

...

...

If you don't want your compiler to issue a particular warning, a type cast should be the last solution to reach for, IMO.

It certainly appears that the types can be adjusted to alleviate the problem in this case, but that isn't always possible as impedance mismatches occur that cannot be altered. In such cases, a static_cast, which *should* indicate that the developer examined the code in question and has decided that the potential for truncation is not an issue

Disabling the warning locally would indicate that the developer examined the code, but (importantly) the program remains unchanged.

That isn't possible for all compilers and isn't portable for those that support the feature. That makes it much less than ideal unless only those compilers complain.

Using static_cast is not the same as disabling the warning locally; for example, if a future remote edit changes the type of the object

I suggested an assertion in addition to the static_cast to verify the behavior as a defense against maintenance effects. Assuming the type is always short, then asserting the current value less-than std::numeric_limits<short>::max() would suffice. To account for your change of type scenario, a compile-time assertion that the type is short would also be needed.

being converted, static_cast may inhibit a compile error.

I don't see how static_cast would be able to hide an error.

Even more ill-advised is instead of static_cast<T>(x) to use the functional notation syntax T(x) which in some cases has the semantics of a C-style cast which is even more likely to inhibit a compile error.

Did we allow for C-style casts in this discussion? I don't think so. I almost never use them. _____ Rob Stewart robert.stewart@sig.com Software Engineer, Core Software using std::disclaimer; Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.

Emil Dotchevski wrote:

On Wed, Jun 16, 2010 at 10:16 AM, Stewart, Robert <Robert.Stewart@sig.com> wrote:

...

I don't see how static_cast would be able to hide an error.

The general point is that casts do more than simply silence warnings. Example:

void foo( short x ); int value(); .... foo(static_cast<short>(value())); //static_cast used to //suppress a warning

Later under maintenance we introduce an additional foo overload:

void foo( long x );

Now the call to foo with the static_cast<short> will continue to compile while without the static_cast you'd get a compile error.

That makes sense. However, in order for that code to be useful after the maintenance change, the result of calling value() would need to be cast to either short or long to avoid the ambiguity and casting to short could be justifiable. Doing so, before or after the maintenance, falls within the discussion below.

...

I suggested an assertion in addition to the static_cast to verify the behavior as a defense against maintenance effects. Assuming the type is always short, then asserting the current value less-than std::numeric_limits<short>::max() would suffice. To account for your change of type scenario, a compile-time assertion that the type is short would also be needed.

void foo( short a, short b ) { short result=a+b; .... }

In general, I find it difficult to accept this instead:

void foo( short a, short b ) { int tmp=a+b; short result=static_cast<short>(tmp); assert(result==tmp); .... }

This would be more palatable: short const result(a + b); assert(result == a + b); (That might warrant a comment, though, as it looks odd.)

In some specific case -- maybe, but I can't justify asserts every time I deal with shorts (which admittedly isn't very often.)

If you don't do something like that, when using shorts, then the code is unsafe, right? Why wouldn't you want to help to ensure that your code does the right thing? The result of this is to recognize that doing math like this with shorts is dangerous because so many things like to be int. (There are problems with all signed types, but those smaller than int are clearly worse.) _____ Rob Stewart robert.stewart@sig.com Software Engineer, Core Software using std::disclaimer; Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.