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

More inspections #22

Merged
merged 18 commits into from
Dec 17, 2020
Merged

More inspections #22

merged 18 commits into from
Dec 17, 2020

Conversation

dnbln
Copy link
Contributor

@dnbln dnbln commented Nov 27, 2020

This PR adds inspections for the first 3 ideas mentioned in #18 + 3 extra.

@dnbln dnbln marked this pull request as draft November 27, 2020 19:48
@dnbln dnbln marked this pull request as ready for review November 27, 2020 20:09
Copy link
Owner

@AzureMarker AzureMarker left a comment

Choose a reason for hiding this comment

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

This is looking pretty good!

return object : LpVisitor() {
override fun visitNonterminal(nonterminal: LpNonterminal) {
if (nonterminal.nonterminalName.nonterminalParams != null) return
Copy link
Owner

Choose a reason for hiding this comment

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

I think it may be possible to support nonterminals with params by generating unit structs in Rust file. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really sure what you mean. ctx.canCombineTypes() would return true only if both types are (), right? What do you have in mind?

PS: sorry for not answering sooner, I was way too busy this week.

Copy link
Owner

Choose a reason for hiding this comment

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

No worries about responding slower; it's the holiday season now 🙂.

For each type/rule parameter, the generated Rust code could have a RuleN struct which is used in place of the generic type. For example:

MyNonterminal<K, V>: HashMap<K, V> = ...; // alternative type resolves to std::collections::HashMap<K, V>
// imports

struct Rule1;
struct Rule2;

type T1 = HashMap<Rule1, Rule2>;
type T2 = std::collections::HashMap<Rule1, Rule2>;

There might be some issues if the generic type has bounds, like Hash in this case. It may be better to check if the Rust plugin supports comparing types with unbound generics (i.e. type T1<K, V> = HashMap<K, V>;).

But I think this can wait for a future PR, if you think it's worthwhile to pursue.

Copy link
Owner

@AzureMarker AzureMarker left a comment

Choose a reason for hiding this comment

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

I'm ready to merge this, but I'll wait in case you're planning on adding more commits.

@AzureMarker
Copy link
Owner

AzureMarker commented Dec 16, 2020

@dblanovschi Let me know when you're ready for this PR to be merged. I'll probably make a release right afterwards.

@dnbln
Copy link
Contributor Author

dnbln commented Dec 17, 2020

With the last commit, there is only #22 (comment) left. I think that'd be worth looking into, but some other time. There is nothing else I'd like to add here. Edit: I just found out that precedence annotations were merged onto lalrpop, so I'd like to add an inspection to validate them (the rules described in "Prevalidation" here at least)

Copy link
Owner

@AzureMarker AzureMarker left a comment

Choose a reason for hiding this comment

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

Thanks again for contributing! These inspections look great!

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