Skip to content

Expressions should not appear twice in the ast#1191

Merged
kripken merged 2 commits intomasterfrom
twice
Sep 18, 2017
Merged

Expressions should not appear twice in the ast#1191
kripken merged 2 commits intomasterfrom
twice

Conversation

@kripken
Copy link
Copy Markdown
Member

@kripken kripken commented Sep 17, 2017

This adds validation for the tree structure in our IR, that each node must have exactly one parent, and so each node must appear exactly once and no more. Turns out we had some errors in that regard, one actual bug and one test bug, both fixed. Also added some docs to the readme.

@kripken kripken mentioned this pull request Sep 17, 2017
Comment thread src/wasm/wasm-validator.cpp Outdated
Walker(std::unordered_set<Expression*>& seen) : seen(seen) {}

void visitExpression(Expression* curr) {
if (seen.find(curr) == seen.end()) {
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 you just do
if (seen.insert(curr).second) dupes.push_back(curr);

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 suppose, but I find the .second notation hard to read myself (a shame it's not .inserted). Is it just me?

Copy link
Copy Markdown
Member

@dschuff dschuff Sep 18, 2017

Choose a reason for hiding this comment

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

No, I agree the API is ugly.
Could always do something like

bool inserted;
std::tie<std::ignore, inserted> = seen.insert(curr);
if (!inserted) dupes.push_back(curr);

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.

Thanks, I like that way, updated.

@kripken kripken merged commit 0532093 into master Sep 18, 2017
@kripken kripken deleted the twice branch September 18, 2017 23:33
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