dlang-community / Pegged

A Parsing Expression Grammar (PEG) module, using the D programming language.
534 stars 66 forks source link

Fix warning about statement not accessible building pegged #188

Closed Temtaime closed 8 years ago

veelo commented 8 years ago

Hi, thanks for pointing out the cause of the issue.

I am wondering whether the assert on the previous line is correct. According to http://dlang.org/spec/expression.html#AssertExpression, this signifies unreachable code. But in reality this is not always the case, the code is reachable if the grammar is left-recursive and the parser is generated at compile time. It is just that this is a condition that is not supported, and the effect I am after is that compilation halts (which it does).

Since we are halting compilation, it shouldn't matter whether statements follow, so I guess we could accept this PR. We would get a confusing error if a compiler implementation were to strip assertions, but if I am not mistaken, assert(0) is supposedly never stripped by any compiler implementation.

Maybe we should change assert(false, ...) into static assert(0, ...), as that better captures the situation: it is a compile-time trap, and does not imply unreachable code I guess. I am wondering, when assert is changed into static assert, would it still generate the unreachable code warning?

Temtaime commented 8 years ago

Hi ! Yes, it seems to be an error in older compiler versions that didn't show up the message about unreachable code. And yes, compiler never removes assert(false). I think this PR should be accepted as it is, changing assert to static assert is not neccessary IMO.

PhilippeSigaud commented 8 years ago

I'll merge it. We can always change it back (or using static assert()) later if necessary.

Temtaime commented 8 years ago

Thanks !