Skip to content

ISLE: Enable the overlap checker - #5011

Merged
elliottt merged 2 commits into
bytecodealliance:mainfrom
elliottt:trevor/enable-overlap-checking
Oct 4, 2022
Merged

ISLE: Enable the overlap checker#5011
elliottt merged 2 commits into
bytecodealliance:mainfrom
elliottt:trevor/enable-overlap-checking

Conversation

@elliottt

@elliottt elliottt commented Oct 4, 2022

Copy link
Copy Markdown
Member

This PR turns the overlap checker on by default, requiring the use of priorities to resolve overlap between rules.

Additionally, I removed the overlap_errors pragma leaving the rest of the pragma parsing framework in place, and resolved overlaps in the isle tests.

@elliottt
elliottt force-pushed the trevor/enable-overlap-checking branch from 6a84d46 to b7e781c Compare October 4, 2022 20:25
@elliottt
elliottt force-pushed the trevor/enable-overlap-checking branch from b7e781c to cfa6139 Compare October 4, 2022 20:29

@cfallin cfallin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

Comment thread cranelift/isle/isle/src/ast.rs Outdated
/// Enable overlap errors in the source.
OverlapErrors,
}
pub enum Pragma {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we put a comment here stating that currently no pragmas are defined (but the infrastructure remains from past pragmas and we are choosing to retain it to easily support future ones)? Otherwise this might look a bit odd to a new reader with no context.

match ident.0.as_ref() {
"overlap_errors" => Ok(Pragma::OverlapErrors),
_ => Err(self.error(ident.1, format!("Unknown pragma '{}'", ident.0))),
// currently, no pragmas are defined, but the infrastructure is useful to keep around

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

... ah, and I see you have exactly that comment here :-) (Mentioning this in both places is still good, I think.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll copy this one over to the enum definition as well, thanks for catching that!

@elliottt
elliottt enabled auto-merge (squash) October 4, 2022 21:27
@elliottt
elliottt merged commit a209cb6 into bytecodealliance:main Oct 4, 2022
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