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

Improve diagnostics for BaseListExpression, StructExpression, and AssignmentStatement #4353

Conversation

kfcripps
Copy link
Contributor

supersedes #3232

Copy link
Collaborator

@fruffy fruffy left a comment

Choose a reason for hiding this comment

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

The P4Testgen code seems to fail because of a nullptr access. Unclear why.

y = 32w1; // Error: no implicit cast to enum
^
enumCast.p4(3)
enum bit<32> X {
^
enumCast.p4(33): [--Werror=type-error] error: AssignmentStatement: values of type 'E2' cannot be implicitly cast to type 'E1'
enumCast.p4(33): [--Werror=type-error] error: a = b: values of type 'E2' cannot be implicitly cast to type 'E1'
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like the assignment statement here is missing quotes, which makes it harder to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean like "a = b"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor Author

@kfcripps kfcripps Jan 24, 2024

Choose a reason for hiding this comment

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

Are you requesting to add quotes only to the error messages validated in the tests that this PR affects, or to all error messages that might possibly print an IR::AssignmentStatement? The latter would take a lot of time so if that's the case then I'll probably just leave the AssignmentStatement::toString() method as it is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is probably prudent to only add quotes to the error messages that are changed in this PR for now. Changing all error messages might be too much too ask for.

@kfcripps
Copy link
Contributor Author

The P4Testgen code seems to fail because of a nullptr access. Unclear why.

I'm going to open separate PRs for each of the IR classes to help isolate the cause.

@kfcripps
Copy link
Contributor Author

I split this PR into #4357, #4358, and #4360.

@kfcripps kfcripps closed this Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants