Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix on missing warning on some cases of integer literal coercion #5728 #6510

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zlatinski
Copy link
Contributor

Fix missing warnings on:

int x3 = 18446744073709551615;
int x4 = 0xFFFFFFFFFFFFFFFF;

fixes #5728

@zlatinski zlatinski requested a review from a team as a code owner March 3, 2025 18:32
@zlatinski zlatinski added the pr: non-breaking PRs without breaking changes label Mar 3, 2025
@zlatinski zlatinski self-assigned this Mar 3, 2025
@zlatinski zlatinski force-pushed the slang/warning-on-some-cases-of-integer-literal-coercion branch from 0546a8d to ba80845 Compare March 3, 2025 19:22
Copy link
Collaborator

@csyonghe csyonghe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test?

I am not sure I understand what is being fixed here. We don’t want a warning for int64_t v = -9223372036854775808 and I am not seeing how the patch fixes that.

@zlatinski
Copy link
Contributor Author

It is fixing the issues reported:
int x3 = 18446744073709551615;
int x4 = 0xFFFFFFFFFFFFFFFF;

We don’t want a warning for int64_t v = -9223372036854775808 and I am not seeing how the patch fixes that.
Are you saying that we have an issue with the above assignment? Let me check ...

@csyonghe
Copy link
Collaborator

csyonghe commented Mar 4, 2025

I am just seeing the comment being deleted is discussing this case, so it is best to verify things are working as expected with tests.

@zlatinski zlatinski force-pushed the slang/warning-on-some-cases-of-integer-literal-coercion branch from ba80845 to 96c3384 Compare March 4, 2025 23:47
@zlatinski
Copy link
Contributor Author

I have the test added for the integer literal coercion.

jkwak-work
jkwak-work previously approved these changes Mar 5, 2025
Copy link
Collaborator

@jkwak-work jkwak-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good to me.

Fix misisng warnings on:

int x3 = 18446744073709551615;
int x4 = 0xFFFFFFFFFFFFFFFF;

Add test cases for the integer literal coercion
@zlatinski zlatinski force-pushed the slang/warning-on-some-cases-of-integer-literal-coercion branch from 96c3384 to c557cc2 Compare March 5, 2025 23:23
@zlatinski zlatinski enabled auto-merge (squash) March 5, 2025 23:26
@csyonghe
Copy link
Collaborator

csyonghe commented Mar 7, 2025

The fundamental problem of this issue is that we are trying to determine the type of a literal or issue a warning too early.
We should wait until the semantic checking stage to issue this kind of warnings.

When we parse the int literal, we just store the token into the AST, and parse the int into a uint64_t field. Let's not determine the type of the literal expr yet.

Then when we are checking the expr in SemanticExprVisitor, we can always fold the OperatorExpr(IntLiteralExpr) into a single IntLiteralExpr, and then do the checking on the IntLiteralExpr to see if it is out of bounds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing warning on some cases of integer literal coercion
3 participants