-
Notifications
You must be signed in to change notification settings - Fork 36
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
Handle conversion operators correctly. #149
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, although I do have a couple questions inline below.
Thanks @khuey !
@@ -155,49 +155,74 @@ macro_rules! log_demangle_as_inner { | |||
} | |||
} | |||
|
|||
#[derive(Debug, Default, Clone, Copy)] | |||
struct ParseContextState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why you pulled this out instead of introducing a new in_conversion: Cell<bool>
member to ParseContext
? I think the latter would be easier to work with, since you wouldn't need to copy in/out the whole ParseContextState
struct just to change one member.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seemed nicer than having a bunch of Cell
s but I don't feel strongly about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly either, so let's leave it.
src/ast.rs
Outdated
Prefix::DataMember(ref prefix, ref member) => { | ||
prefix.demangle(ctx, scope)?; | ||
member.demangle(ctx, scope) | ||
}*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's up with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops that's just left over crud from copying/pasting around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me when you remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
Because these now parse as templates instead of template-templates, we need to special case the conversion operator case when printing function return types, as described in the CXX ABI text. We can do ctors and dtors here as well, which requires the change to the existing test. See https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=libiberty/cp-demangle.c;h=3f2a097e7f2075e5750e40a31ce46589d4ab83d5;hb=e6d079c2fb797e84d7d234f9c8277fbe4558213b#l2464 for an explanation of the grammar involved.
Because these now parse as templates instead of template-templates, we need to special case the conversion operator case when printing function return types, as described in the CXX ABI text. We can do ctors and dtors here as well, which requires the change to the existing test.
See https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=libiberty/cp-demangle.c;h=3f2a097e7f2075e5750e40a31ce46589d4ab83d5;hb=e6d079c2fb797e84d7d234f9c8277fbe4558213b#l2464 for an explanation of the grammar involved.