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.

Sign up for GitHub

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

Jump to bottom

When creating dynCall and legalstub function mark them as hasExplicitName. NFC #6496

Merged
merged 1 commit into from
Apr 12, 2024
Merged

When creating dynCall and legalstub function mark them as hasExplicitName. NFC #6496

merged 1 commit into from
Apr 12, 2024

Conversation

sbc100
Copy link
Member

@sbc100 sbc100 commented Apr 12, 2024 •
edited
Loading

This is temporary workaround for the current test failures on the emscripten waterfall. The real fix I believe will be to make hasExplicitName the default in these kinds of cases.

See: #6466

@sbc100
When creating dynCall and legalstub function mark them as hasExpl…
55d035c 
…icitName. NFC

This is temporary workaround for the current test failures on the
emscripten waterfall.  The real fix I believe will be to make
`hasExplicitName` the default in these kinds of cases.

See: #6466
@sbc100 sbc100 changed the title When creating dynCall and legalstub function mark them as hasExpl… When creating dynCall and legalstub function mark them as hasExplicitName. NFC Apr 12, 2024
@sbc100 sbc100 requested review from tlively and kripken April 12, 2024 16:18
tlively
tlively reviewed Apr 12, 2024
View reviewed changes
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

Can you add a test? FWIW I took a stab at setting this in makeFunction like @kripken suggested, but I couldn't see any effect on a roundtripping test, so I must have been missing something.

@sbc100
Copy link
Member Author

sbc100 commented Apr 12, 2024 •
edited
Loading

Can you add a test? FWIW I took a stab at setting this in makeFunction like @kripken suggested, but I couldn't see any effect on a roundtripping test, so I must have been missing something.

What do you think about just landing this now to make the emscripten CI green again and then followup with test as part of the makeFunction fix? Clearly we lack some coverage here ..

@tlively
Copy link
Member

tlively commented Apr 12, 2024

Did you at least verify locally that it does fix the Emscripten issue? If so, I guess landing this would be ok.

@sbc100
Copy link
Member Author

sbc100 commented Apr 12, 2024

Did you at least verify locally that it does fix the Emscripten issue? If so, I guess landing this would be ok.

Yes I verified that this fixes the emscripten tests.

@sbc100
Copy link
Member Author

sbc100 commented Apr 12, 2024

I'm happy to start looking into tests and a better fix right away.

tlively
tlively approved these changes Apr 12, 2024
View reviewed changes
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

Sounds good, thanks!

@sbc100 sbc100 merged commit 8c834e8 into main Apr 12, 2024
13 checks passed
@sbc100 sbc100 deleted the explicit_name branch April 12, 2024 16:51
@sbc100 sbc100 mentioned this pull request Aug 5, 2024
Merged
sbc100 added a commit that referenced this pull request Aug 5, 2024
@sbc100
Use explicit names for elements creating using Builder::make_ methods
ca2f115 
In all the cases I could find the `make_xx` helper method were
called with meaningful names that are useful in the name section.

See #6496 and #6806
@sbc100 sbc100 mentioned this pull request Aug 5, 2024
Draft
@gkdn gkdn mentioned this pull request Aug 31, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tlively tlively tlively approved these changes

@kripken kripken Awaiting requested review from kripken

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sbc100 @tlively