Skip to content

Fix atomics refinalization (we were missing some glue)#1241

Merged
kripken merged 4 commits intomasterfrom
atomics
Oct 24, 2017
Merged

Fix atomics refinalization (we were missing some glue)#1241
kripken merged 4 commits intomasterfrom
atomics

Conversation

@kripken
Copy link
Copy Markdown
Member

@kripken kripken commented Oct 24, 2017

And add a visitor which must override all its elements, so this never happens again.

If the visitors were virtual methods, they could be = 0. We use the CRTP pattern here instead, so it's at compile time, and it seems like there should be a way to get the compiler to error at compile time if a method isn't overridden (currently the PR aborts at runtime). Is there a way?

…itor which must override all its elements, so this never happens again
@dschuff
Copy link
Copy Markdown
Member

dschuff commented Oct 24, 2017

Can we do something like
static_assert(&SubType::visitFoo != &OverridenVisitor<SubType>::visitFoo, "Derived class must implement visitFoo");

@kripken
Copy link
Copy Markdown
Member Author

kripken commented Oct 24, 2017

Thanks, added, and verified it works.

Comment thread src/wasm-traversal.h Outdated
template<typename SubType, typename ReturnType = void>
struct OverriddenVisitor {
// Expression visitors
ReturnType visitBlock(Block* curr) {
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.

Hm, this is a bit verbose. I wonder if we could turn it into a macro a la DELEGATE?

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.

Done.

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.

Hmm, but it stopped working (throwing an error).

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.

Weird, it looks exactly equivalent except for the extra semicolon (although since I'm nitpicking, it probably shouldn't actually be called DELEGATE since it's not delegating, it's just asserting).

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.

Looks like static asserts only happen if the code is called. Which is good enough.

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.

Yeah, it is the same as before. I just happened to test a method that wasn't called now and not earlier ;)

Maybe delegate isn't the best name, but it's also writing a stub for the function with an abort in it, so it's more than the static assert. Maybe "handle"? or "implement"?

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.

how about unhandled/unimplemented?

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.

Sure, done.

Copy link
Copy Markdown
Member

@dschuff dschuff left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for putting up with me :D

@kripken
Copy link
Copy Markdown
Member Author

kripken commented Oct 24, 2017

The CI failures here are the known issues currently being fixed, merging.

@kripken kripken merged commit 059e6e3 into master Oct 24, 2017
@kripken kripken deleted the atomics branch October 24, 2017 23:48
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