Skip to content

Conversation

@sbc100
Copy link
Member

@sbc100 sbc100 commented Aug 2, 2024

No description provided.

@sbc100
Copy link
Member Author

sbc100 commented Aug 2, 2024

I wonder if there is a better way to handle fallout from making names not explicit by default?

@sbc100 sbc100 requested a review from kripken August 2, 2024 17:20
@kripken
Copy link
Member

kripken commented Aug 5, 2024

What is the specific fallout here? That is, why is this necessary - does it fix something?

@sbc100
Copy link
Member Author

sbc100 commented Aug 5, 2024

What is the specific fallout here? That is, why is this necessary - does it fix something?

No expected fallout, just improved debugging experience since these functions now have meaningful debug names.

This change allows us to re-enable the test_no_legalize_js_ffi test which is checking explicitly for imports that start with $legalimport.

See #6428 (comment)

@sbc100
Copy link
Member Author

sbc100 commented Aug 5, 2024

What is the specific fallout here? That is, why is this necessary - does it fix something?

No expected fallout, just improved debugging experience since these functions now have meaningful debug names.

This change allows us to re-enable the test_no_legalize_js_ffi test which is checking explicitly for imports that start with $legalimport.

See #6428 (comment)

I think whenever we give something a useful name like this in binaryen it should be preserved in the debug name section.. just dropping the name completely (which is what happens without the ExplicitName flag set) seems like the wrong default. This was changed at some point which has had a fair amount of fallout.

@kripken
Copy link
Member

kripken commented Aug 5, 2024

Oh, I see, thanks. Yes, this makes sense: we do want explicit names here for debugging, and that test is looking at such internals.

@sbc100 sbc100 merged commit 53d54d7 into main Aug 5, 2024
@sbc100 sbc100 deleted the explicit_name branch August 5, 2024 16:21
@kripken
Copy link
Member

kripken commented Aug 5, 2024

This was changed at some point which has had a fair amount of fallout.

Yeah, maybe the other default would be better, since the harm would only be some meaningless names in the name section.

Do you think there are more cases left to handle for it? If so I'm not opposed to changing the default.

@sbc100
Copy link
Member Author

sbc100 commented Aug 5, 2024

This was changed at some point which has had a fair amount of fallout.

Yeah, maybe the other default would be better, since the harm would only be some meaningless names in the name section.

Do you think there are more cases left to handle for it? If so I'm not opposed to changing the default.

I think we would want to draw is clear distinction between names that were explicitly given like this ones and names that binaryen makes upon the fly simply because it needs everything to be named (e.g. function42). The former we always want in the name section and the later we never want (I think).

@sbc100
Copy link
Member Author

sbc100 commented Aug 5, 2024

See #6496.

I think the original issue is from #6466

sbc100 added a commit that referenced this pull request Aug 5, 2024
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
@gkdn gkdn mentioned this pull request Aug 31, 2024
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.

3 participants