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

[#4656] Revert PR #3633 so that mux expressions will be constant folded #4660

Closed
wants to merge 4 commits into from

Conversation

kfcripps
Copy link
Contributor

@kfcripps kfcripps commented May 9, 2024

Reverts 4a74084, as discussed as an alternate fix for #4656 in #4657.

@kfcripps kfcripps marked this pull request as ready for review May 9, 2024 17:15
@kfcripps kfcripps requested review from asl and mihaibudiu May 9, 2024 18:20
@fruffy fruffy added core Topics concerning the core segments of the compiler (frontend, midend, parser) p4-spec Topics related to the P4 specification (https://github.com/p4lang/p4-spec/). labels May 9, 2024
@ChrisDodd
Copy link
Contributor

Looks like this is supceded by #4657, so this can be closed?

@kfcripps
Copy link
Contributor Author

@ChrisDodd It is an alternate approach to #4657. I plan to close one after the other is merged.

@kfcripps
Copy link
Contributor Author

As mentioned in #4657, I'm actually preferring this PR at this point.

@kfcripps kfcripps requested a review from ChrisDodd May 10, 2024 02:45
vlstill
vlstill previously approved these changes May 10, 2024
}
apply {
switch(baz()) {
1 + 2 == 3 ? 1 : 2 : { foo(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

I am just surprised this is not syntactically ambiguous, the 2 : look quite weird :-). (This is not an issue with the PR/test, I'm just surprised the grammar allows it).

Copy link
Contributor

Choose a reason for hiding this comment

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

A single token of lookahead in Bison is apparently enough for it to figure it out, especially from the context that this is inside of a switch statement.

@fruffy
Copy link
Collaborator

fruffy commented May 15, 2024

p4lang/p4-spec#1281 determines whether we may merge this particular PR, too.

@kfcripps kfcripps marked this pull request as draft May 15, 2024 16:34
@vlstill
Copy link
Contributor

vlstill commented May 16, 2024

superseded by #4657

OK, so should this one be closed now?

@kfcripps
Copy link
Contributor Author

Sure, it will need to be rebased even if the outcome of p4lang/p4-spec#1281 is to remove the error.

@kfcripps kfcripps closed this May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser) p4-spec Topics related to the P4 specification (https://github.com/p4lang/p4-spec/).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants