👋 Hey,
Feature
This is a follow up to #4667 (comment), where we discussed removing TestName as an option for Function names.
Benefit
The biggest benefit is cleaning up our naming interface. TestNames arguably shouldn't belong to the core cranelift infrastructure. Instead the test suite should have an external table as any other embedder would do.
The other benefit was allowing larger names in test functions, but @jameysharp has fixed that in #4764 (Thanks!)
@jameysharp tested this and while it does have some performance benefits, they are minimal.
This also removes some edges such as cranelift-jit not supporting test names which makes for a better experience using it.
Implementation
I started working on this a few days ago, but I'm putting it on hold to fix some other more prioritary issues. (in progress branch) If anyone wants to pick this up in the mean time feel free!
The branch above starts by changing the parser to produce a side table of test names while assigning UserExernalNames to each function.
Following from that we need to switch the testsuite to use this new side table.
And after that we should be able to remove the TestName from cranelift.
This sounds simple, but I think it will end up being quite a big change.
Alternatives
Do nothing. This has been the state of cranelift for a long time.
I think this would also break the interface for pretty much every downstream user since we would probably remove UserFuncName and replace it with UserExernalName directly.
Edit: Another alternative would be to expand this, and start supporting string function names, which would also be an interesting option.
👋 Hey,
Feature
This is a follow up to #4667 (comment), where we discussed removing TestName as an option for Function names.
Benefit
The biggest benefit is cleaning up our naming interface. TestNames arguably shouldn't belong to the core cranelift infrastructure. Instead the test suite should have an external table as any other embedder would do.
The other benefit was allowing larger names in test functions, but @jameysharp has fixed that in #4764 (Thanks!)
@jameysharp tested this and while it does have some performance benefits, they are minimal.
This also removes some edges such as
cranelift-jitnot supporting test names which makes for a better experience using it.Implementation
I started working on this a few days ago, but I'm putting it on hold to fix some other more prioritary issues. (in progress branch) If anyone wants to pick this up in the mean time feel free!
The branch above starts by changing the parser to produce a side table of test names while assigning UserExernalNames to each function.
Following from that we need to switch the testsuite to use this new side table.
And after that we should be able to remove the TestName from cranelift.
This sounds simple, but I think it will end up being quite a big change.
Alternatives
Do nothing. This has been the state of cranelift for a long time.
I think this would also break the interface for pretty much every downstream user since we would probably remove UserFuncName and replace it with UserExernalName directly.
Edit: Another alternative would be to expand this, and start supporting string function names, which would also be an interesting option.