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.

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

Fix demangling of namespaced type arguments #220

Merged
merged 1 commit into from
Feb 27, 2024
Merged

Conversation

DutChen18
Copy link
Member

The Demangler breaks up a mangled identifier into a list of "scopes":

namespace foo {
	// mangled name: _ZN3foo4funcEv
	// demangled name: foo::func
	// scopes: ["foo", "func"]
	void func() {}
}

This does not work when the idenfitier is for a template with a namespaced type argument:

namespace bar {
	class Arg;
}

namespace foo {
	template<class T>
	void func() {}

	// mangled name: _ZN3foo4funcIN3bar3ArgIiEEEEvv
	// demangled name: foo::func<bar::Arg>
	// scopes: ["foo", "func<bar", "Arg>"]
	template<>
	void func<bar::Arg>() {}
}

The problem is that, when splitting the demangled name into scopes, it ignores the meaning of < and > characters, instead treating them as part of the name. The most hygienic solution is to replace the list of scopes with a more complex structure that can also hold type arguments.

As this would require significant changes to the Demangler, and the Demangler is used in a few places where the effect of such changes would be unclear. I've opted for a quick and dirty solution for now.

The list of scopes will still contain broken names, exactly as shown in the example above. When converting this list of scopes into a javascript name, the templates are removed after joining the scopes together instead of before.

cleanupTemplates used to assume that the template parameters, if any, are at the end of the string passed to cleanupTemplates. This was true before because > is always at the end of a type name, or it is followed by ::. But now, because cleanupTemplates is only called after joining the scopes back together, it can also be passed strings with multiple template parameters, or where the template parameters are not at the end of the string. cleanupTemplates is updated to handle these cases as well, splicing out template parameters from the middle of the string instead of just at the end.

Side note: this whole class is a mess, half of the functions aren't even used anywhere.

@DutChen18 DutChen18 requested a review from yuri91 February 27, 2024 13:48
@DutChen18
Copy link
Member Author

Some tests seem to be failing, that's what I get for making a PR before running tests.

@DutChen18 DutChen18 marked this pull request as draft February 27, 2024 13:51
@DutChen18 DutChen18 force-pushed the fix-template-demangling branch from d5f0d25 to df07c54 Compare February 27, 2024 14:07
The `Demangler` breaks up a mangled identifier into a list of "scopes":
```cpp
namespace foo {
	// mangled name: _ZN3foo4funcEv
	// demangled name: foo::func
	// scopes: ["foo", "func"]
	void func() {}
}
```

This does not work when the idenfitier is for a template with a
namespaced type argument:
```cpp
namespace bar {
	class Arg;
}

namespace foo {
	template<class T>
	void func() {}

	// mangled name: _ZN3foo4funcIN3bar3ArgIiEEEEvv
	// demangled name: foo::func<bar::Arg>
	// scopes: ["foo", "func<bar", "Arg>"]
	template<>
	void func<bar::Arg>() {}
}
```

The problem is that, when splitting the demangled name into scopes, it
ignores the meaning of `<` and `>` characters, instead treating them as
part of the name. The most hygienic solution is to replace the list of
scopes with a more complex structure that can also hold type arguments.

As this would require significant changes to the `Demangler`, and the
`Demangler` is used in a few places where the effect of such changes
would be unclear. I've opted for a quick and dirty solution for now.

The list of scopes will still contain broken names, exactly as shown in
the example above. When converting this list of scopes into a javascript
name, the templates are removed *after* joining the scopes together
instead of before.

`cleanupTemplates` used to assume that the template parameters, if any,
are at the end of the string passed to `cleanupTemplates`. This was true
before because `>` is always at the end of a type name, or it is
followed by `::`. But now, because `cleanupTemplates` is only called
*after* joining the scopes back together, it can also be passed strings
with multiple template parameters, or where the template parameters are
not at the end of the string. `cleanupTemplates` is updated to handle
these cases as well, splicing out template parameters from the middle of
the string instead of just at the end.
@DutChen18 DutChen18 force-pushed the fix-template-demangling branch from df07c54 to 008ac35 Compare February 27, 2024 14:09
@DutChen18 DutChen18 marked this pull request as ready for review February 27, 2024 14:09
@DutChen18
Copy link
Member Author

Some tests seem to be failing, that's what I get for making a PR before running tests.

Was a stupid typo, now fixed.

@yuri91
Copy link
Member

yuri91 commented Feb 27, 2024

@DutChen18 add a task to Asana to review this whole class. I agree that it's a mess and when we have time we should rework it properly.
The llvm demangler was bugged at the time, but I think that it works better now. And since we are using more features in client types now it makes sense to make it more robust

@yuri91 yuri91 merged commit 1ff8b18 into master Feb 27, 2024
1 check passed
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