Fix warnings on signed integer initialization - #4309
Conversation
d4d648e to
f0e84c2
Compare
| big_int mask = Util::mask(width); | ||
|
|
||
| if (tb->isSigned) { | ||
| auto masked_value = value; |
There was a problem hiding this comment.
The logic here looks fairly complex. Possible to add some tests to https://github.com/p4lang/p4c/blob/main/test/gtest/constant_expr_test.cpp which exercise this code?
| masked_value = value & mask; | ||
| } | ||
| } | ||
| if (is_overflow) { |
There was a problem hiding this comment.
This big block of code seems to be equivalent to just changing one line
big_int max = (one << width) -1;
There was a problem hiding this comment.
Wouldn't this need to be adjusted based on signedness (value >= 0, value <0)?
@asawulaINTC Can you comment?
eec5c29 to
16a7248
Compare
16a7248 to
6b94bd7
Compare
| EXPECT_EQ(c5.asUnsigned(), 5461); | ||
|
|
||
| // overflow, but this should not even generate a warning under regular settings | ||
| auto c6 = IR::Constant(&t13s, 0x3555); // 0b1_0101_0101_0101 |
There was a problem hiding this comment.
It should be possible to get the number of warnings triggered with diagnosticCount().
|
It looks like this didn't come up yet in the corresponding spec change, but, based on discussions in the LDWG, I think we'd prefer to keep the warnings on signed integer initialization unless you have a use-case that doesn't fit our imagination in mind. Let me add a comment to the specification issue: github.com/p4lang/p4-spec/issues/1269 |
|
I've been experimenting a bit with this and another implementation issue I see is that it does not appear to be possible to stop specific warnings with explicit casts -- that is, something like |
fruffy
added
the
p4-spec
4 participants
Remove misleading overflow warning for correct-width signed integer literals with MSBit set, i.e. representing negative numbers. This is actually the only way integer literals of explicitly-declared width can be specified (as per language specs, Section 6.3.3.2).
As these warnings are mentioned in the language specs, Section 7.1.6.6, this change is accompanied with one to the specs content, PR #1270.