Archived

This topic is now archived and is closed to further replies.

Dumb compiler or dumb coder?

This topic is 5038 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

Recommended Posts

Take a look at this code..
	const int MaxFrameRate= 0;
const double FramePeriod= MaxFrameRate > 0 ? 1. / MaxFrameRate : 0;

VC 6 (latest patch) gives me a the error C2124: divide or mod by zero. Which is pretty dumb, you would think it could detect that 'MaxFrameRate' is never > 0 and not attempt to compile the expression. Whats up with that? [edited by - willm on May 7, 2004 10:28:36 AM]

Share on other sites
Well, you have maxframrate constantly defined as 0, so in your statement, you will always be dividing 1 by 0. It tests if MaxFramRAte is greater than 0, and since it isnt, then it will execute the second part of the ternary statement, which is dividing 1 by maxframerate.

Share on other sites
Well, perhaps neither ... or both. :-)

Point taken though. It does seem like you''ve got the compiler in a twist there. Perhaps the compiler just doesn''t like constants to be defined using formulae.

So, why are you defining a constant using a conditional operator anway? Makes no sense to me.

Also, setting your Max Frame Rate to 0 seems counter-productive unless you are implying that 0 means that there is no maximum. In which case it''s probably just as good to set it to 1000 which will always look unlimited anyway and you''ll also have no need for your conditional operator.

Ah, I''ve got it now that I''ve looked at it again. It might be your lack of parentheses (sp?). Try this:

const double FramePeriod = (MaxFrameRate > 0) ? (1.0 / (double)MaxFrameRate) : 0;

There could be a mixup between the ''>'' and the ''?''. If not, then my earlier comments stand.

R

--------------------------------------------------------------------------
There is no point in flaming if you''ve merely poured fuel on your own head

Share on other sites
Incidentally, shouldn''t this be a warning? Or are you set to show warnings as errors?

Share on other sites
"Why won''t my compiler compile my broken code?????"

Does it matter? Just fix your broken code and be done with it.

Share on other sites
I was experimenting with a a frame rate limiter, and I used a MaxFrameRate value of 0 to indicate that there was no maximum value.

Tried adding the parenthesis, but no change..

Found some more wierdness, the code in my original post gives an error, but if I explicitly cast MaxFrameRate to a double when doing the divide I get a 'warning C4723: potential divide by 0'.

None of this is a big problem, its easy to work around. Just seems inconsistent..

[edited by - willm on May 7, 2004 11:11:24 AM]

Share on other sites
Code isn''t broken as I stated earlier, sorry about that.

I''m guessing this is a result of the type of parser used by both MSVC and gcc. The parser most likely evaluates the "1. / blah" before it gets to the conditional. Because reducing the division is an optimization it can make (since both are constants) it does make it, even though it results in NaN. If you visualize the parse tree, the division is probably at the lowest branch, and the compiler most likely processes the tree in a bottom-up fashion.

It bit you in this case, but it''s necessary that the compiler works in this way. It should just be a warning though.

Share on other sites
Even VC7.1 generates an error(not warning). If you remove the const it compiles just fine though, not even a warning. A #define gives the same error.

It seems very much like a compiler bug, because the division part of the statement shouldn''t happen when MaxFrameRate is zero.

Share on other sites
This is dumb code IMHO, and a perfectly valid compiler complaint.

If MaxFrameRate was non-const, then i would see the problem, but why are you testing that the value of it is greater than 0 when you already know the value is 0?

Its just the same as writing

0 > 0 ? 1/0 : 0

doesnt make any sense.

Share on other sites
quote:
Original post by Jingo
This is dumb code IMHO, and a perfectly valid compiler complaint.

If MaxFrameRate was non-const, then i would see the problem, but why are you testing that the value of it is greater than 0 when you already know the value is 0?

Its just the same as writing

0 > 0 ? 1/0 : 0

doesnt make any sense.

It does make sense if you want to change the max framerate in one place, without needing to change the FramePeriod too. And both should be constant that doesn''t change during the game.

Share on other sites
Well a max framerate of 0 doesnt make much sense either. If 0 is supposed to mean there is no limit, then set the framerate to std::numeric_limits::max().

Share on other sites
The code does make sense in context, but really whether it does or not is irrelevant. The question is why does the compiler error, the answer is because it uses a bottom up parser which sees the division before the conditional. Compilers don't process things left to right like humans might, which is important because we don't want them pretending that 6 + 6 / 2 is equal to 6, we want it equal to 9.

If there's a compiler bug here, it's that it errors instead of warning. Maybe you can turn that off with a pragma in MSVC?

Edit: Trying to fit everything into one post here:

fredizzimo - It works when MaxFrameRate is not const because then the compiler can no longer make the optimization in the parsing stage, it has no choice but to delay the initialization of FramePeriod until runtime.

[edited by - bobstevens on May 7, 2004 12:05:55 PM]

Share on other sites
Aarrrrgggggghhhhhhhh!!!

That''s it! It may not be el-perfecto but do it this way. Or at least some way that compiles.

#define THE_FREAKIN_FRAME_RATE 0

#if THE_FREAKIN_FRAME_RATE > 0
const int MaxFrameRate = THE_FREAKIN_FRAME_RATE;
const double FramePeriod = 1.0 / (double)THE_FREAKIN_FRAME_RATE;
#else
const int MaxFrameRate = 0;
const double FramePeriod = 0.0;
#endif

#undef THE_FREAKIN_FRAME_RATE

R

--------------------------------------------------------------------------
There is no point in flaming if you''ve merely poured fuel on your own head

Share on other sites
Having a value of 0 for the second operand of the operator/ is ''undefined behavior''. The compiler can issue an error message if it wishes, or do anything else that it wants to do. So its not really a bug.

Share on other sites
quote:
Original post by Jingo
Having a value of 0 for the second operand of the operator/ is 'undefined behavior'. The compiler can issue an error message if it wishes, or do anything else that it wants to do. So its not really a bug.

Fair enough, although this seems in conflict with the IEEE 754 standard for floating point numbers, which says that a positive number divided by zero results in infinity. I mistakenly said that it was NaN earlier.

A useful alternative, although still mildly horrible, could be:
int MaxFrameRate_tmp = 0;const double FramePeriod= MaxFrameRate_tmp > 0 ? 1. / MaxFrameRate_tmp : 0;const int MaxFrameRate = MaxFrameRate_tmp;

[edited by - bobstevens on May 7, 2004 1:18:13 PM]

Share on other sites
There's no need to fall back to macro programming here. You can keep using constants and calculate the double at compile time using templates:

// Some template magic heretemplate<size_t MaxFrameRate>struct FramePeriodCalculator { static const double value; }; template<size_t MaxFrameRate>const double FramePeriodCalculator<MaxFrameRate>::value = 1. / MaxFrameRate; template<> const double FramePeriodCalculator<0>::value = 0.; // Now it works ;-)const size_t MaxFrameRate = 0;const double FramePeriod = FramePeriodCalculator<MaxFrameRate>::value;

-Markus-

[edited by - Cygon on May 7, 2004 1:20:35 PM]

Share on other sites
Cygon beat me to the template solution, that''s the way to do it =)

Share on other sites
quote:
Original post by bobstevens

fredizzimo - It works when MaxFrameRate is not const because then the compiler can no longer make the optimization in the parsing stage, it has no choice but to delay the initialization of FramePeriod until runtime.

It doesn't leave anything to runtime when using full optimizations. It generates the following code
	00		 fld	 QWORD PTR __real@0000000000000000  00006	83 ec 08	 sub	 esp, 8  00009	b9 00 00 00 00	 mov	 ecx, OFFSET FLAT:?cout@std@@3V?$basic_ostream@DU?$char_traits@D@std@@@1@A  0000e	dd 1c 24	 fstp	 QWORD PTR [esp]  00011	e8 00 00 00 00	 call	 ??6?$basic_ostream@DU?$char_traits@D@std@@@std@@QAEAAV01@N@Z ; std::basic_ostream >::operator<<

So obviously the compiler is able to parse the statement. Note there's no comparsions and that I added a std::cout at the end to output the actual value

Why it doesn't use its full parse capabilities when using const is a mystery.

Edit actually that's not even full optimizations, it's just the default optimize for speed, that doesn't do any global optimizations or things like that.

[edited by - fredizzimo on May 7, 2004 2:55:26 PM]

Share on other sites
quote:
Original post by fredizzimo
Why it doesn''t use its full parse capabilities when using const is a mystery.

No, I don''t think it is. When you declare something as a constant it can make the optimization in the parse tree itself. The compiler is trying to do this with the constant, but since it is zero, there''s an error. But when something isn''t constant it has to make the optimization in a later pass, if at all. MSVC probably has a later pass which discovered that the MaxFrameRate variable couldn''t be anything but 0 given the context, and it made the optimization based on that. Since the optimization was made in a later pass, it took the result of the conditional into consideration and ignored the divide by zero.

Share on other sites
quote:
Original post by bobstevens
quote:
Original post by fredizzimo
Why it doesn''t use its full parse capabilities when using const is a mystery.

No, I don''t think it is. When you declare something as a constant it can make the optimization in the parse tree itself. The compiler is trying to do this with the constant, but since it is zero, there''s an error. But when something isn''t constant it has to make the optimization in a later pass, if at all. MSVC probably has a later pass which discovered that the MaxFrameRate variable couldn''t be anything but 0 given the context, and it made the optimization based on that. Since the optimization was made in a later pass, it took the result of the conditional into consideration and ignored the divide by zero.

You don''t need to explain why it does it. The point is that it detects a divide by zero in an early phase and flags it as an error too early. If the compiler just flagged it as a potential error, and parsed further the error would not happen. Mystery was maybe a too strong word, but since the compiler has a much more advanced parser than that it should make the error happen only when it really is an error, not before.

The compiler is able to detect errors and warning that comes only after full parsing too, like "all code paths doesn''t return a value", and similars. I''m also pretty sure it have given me errors/warnings about divide by zero for non constant values, but only after taking the full codepath into account, which means it''s able to detect the exact same error at a later stage.

Don''t try to defend the compiler writers here, it''s a clear bug. I understand they took it the easy way though, or possibly they didn''t even think about the whole case, but it''s not an unfixable bug. A warning instead of error would be better in this case, but generally I would prefer an error if I make a division by zero. And if it clearly detects that the division by zero never can occour, why give a message at all?

Share on other sites
I agree with Jingo in that the fact that division by zero is undefined means it can''t be ruled a compiler bug. However I think that it would be much more useful for the compiler to warn in this situation rather than error, since the result of the constant folding may exist in a path that''s never executed. I didn''t mean to sound like I was talking down to you. It just seemed like you may not have known about how parse trees and constant folding work, and you never know who you''re talking to on these boards.

Share on other sites
The true part of that statement is completely constant. The compiler will try to substitute it with the result before it even has a look at the statement and realises that it doesn''t need it anyway.