Forbid case-after-default, tweak error messages #4831
Conversation
There was a problem hiding this comment.
This issue apparently has nothing to do with case-after-default, and it just masks the "real" problem (the one the issue referred to).
vlstill
added
breaking-change
vlstill
requested review from
asl,
grg,
jnfoster,
ChrisDodd,
fruffy and
MichalKekely
|
Would it perhaps be a good idea to add a modified version of default-switch.p4, that puts default last, in case that helps improve test coverage of whatever issue it was originally created for? That modified version would have no errors, and so belong in the testdata/p4_16_samples directory, where default-switch.p4 is before this PR. |
|
Regarding the comment about a breaking change, yes I agree this is a kind of breaking change. Fortunately, it is what I think could be termed a "safe" breaking change: any program that breaks because of it, will have a clear error message and a straightforward path to update the code to work again. As opposed to what I would call an "unsafe" breaking change, where some programs change their behavior with no warning or error messages at compile time. Those are the really nasty ones I would strongly suggest we bump up the major version of the language, if we make a change like that. I can't think of one that has been made in P4_16, or if there was, it was very early on, perhaps before version 1.0 was released. |
I think the semantics for "breaking change" was (as this is in description): "This change may break assumptions of compiler back ends", not about the input source. Yes, it may break malformed input, but I think it is ok, that next version of compiler is more strict wrt the spec. This certainly does not affect backends |
Agree with Anton. Maybe we should add a different label for changes that cause existing input sources to no longer compile, or to have different semantics? |
We could probably broaden the definition of the |
vlstill
added
p4-spec
Yep I think the |
Right, I should have done it that way originally. Added the positive version of the test now. |
vlstill
force-pushed
the
vstill/forbid-case-after-default
branch
from
eb31164
to
e9bf6a8
Compare
Fixes #4290. This forbids compilation of code which contains a switch label after a default label. This behaviour matches the P4 spec ($12.7.3):
See #4290 (comment) for detains why I believe there is no way to allow this and be consistent.
I've also changed the error messages to be more readable. Note that neither
SwitchStatementnorDefaultExpressionhave a reasonable string representation for the error messages, so there is no point in including anything more that the debug info in the message.@fruffy, while this can't break "assumptions in back-end code" I guess it should still be marked as breaking change as it can actually break compilation of P4. Such P4 was not adhering to spec though and it should be simple to fix it.