Skip to content

Conversation

@pfaffe
Copy link
Contributor

@pfaffe pfaffe commented Nov 22, 2022

Drive-by: Fix pointer comparison on 32bit targets

@pfaffe pfaffe changed the title Fix for an upstream llvm API change Fixes required for targeting wasm32 Dec 6, 2022
Comment on lines +2278 to +2283
#ifndef __EMSCRIPTEN__
EXPECT_THAT(Scope("bf").Eval("a"), IsEqual("1023"));
EXPECT_THAT(Scope("bf").Eval("b"), IsEqual("9"));
EXPECT_THAT(Scope("bf").Eval("c"), IsEqual("false"));
EXPECT_THAT(Scope("bf").Eval("d"), IsEqual("true"));
#endif
Copy link
Member

Choose a reason for hiding this comment

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

What's wrong with these? Is scoped evaluation not supported for wasm32?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly.

Comment on lines +2921 to 2922
#ifndef __EMSCRIPTEN__
TEST_F(EvalTest, TestDereferencedType) {
Copy link
Member

Choose a reason for hiding this comment

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

What's wrong with this test? It seems to me this should work for wasm32 too...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the unconditional comparison with lldb's output here. wasm32 can't run lldb, so we can't do this comparison.

Comment on lines +3044 to 3045
#ifndef __EMSCRIPTEN__
TEST_F(EvalTest, TestCompositeAssignmentAdd) {
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered doing something like this when disabling the whole test?

#ifdef __EMSCRIPTEN__
  // Doesn't work on wasm32 because of X
  GTEST_SKIP() << "not supported on wasm32";
#else

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 did, but ifdef'ing out the entire test simplifies the wasm wrapper, so that it doesn't have to provide noop implementations of things like EvalWithContext() or Scope(). This seemed cleaner than implementing methods that aren't supposed to be called.

@werat werat merged commit e87123a into google:master Dec 8, 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