- 
                Notifications
    
You must be signed in to change notification settings  - Fork 15.1k
 
[MsDemangle] Use LLVM style RTTI for AST nodes #143410
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
Conversation
          
 Do you have an example in mind, where this added RTTI functionality would make the code simpler?  | 
    
          
 Yes, I wanted to add rich function demangling info for MS to LLDB (#143149). For this, I need to keep track where certain parts of the demangled name are. Specifically, the basename and function arguments. Here is my WIP, which still has some unrelated changes. RTTI is used there for checking if a node is an identifier and to be able to access its template parameters. LLDB already does this for the Itanium demangling in DemangledNameInfo. It would be even better if that visitor/output buffer could be reused somehow, as MS-demangle also uses that class. However, it doesn't call   | 
    
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.
Okay, adding this sounds good to me.
| enum class NodeKind { | ||
| Unknown, | ||
| 
               | 
          ||
| SymbolStart, | 
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 think we should introduce new enumerator values for the Start/End values. The guideline is that there should be one enumerator per concrete class.
How about something like:
enum class NodeKind {
  Unknown,
  SymbolStart,
  EncodedStringLiteral = SymbolStart,
  ...
  VariableSymbol,
  SymbolEnd = VariableSymbol,
  ...
86c1777    to
    4e15273      
    Compare
  
    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
| 
           @zmodem I can't merge this on my own. Could you please merge this for me?  | 
    
| 
           LLVM Buildbot has detected a new failure on builder  Full details are available at: https://lab.llvm.org/buildbot/#/builders/207/builds/3367 Here is the relevant piece of the build log for the reference | 
    
          
 The next build (https://lab.llvm.org/buildbot/#/builders/207/builds/3368) is green, so that was a flake.  | 
    
The inheritance hierarchy for
llvm::ms_demangle::Node(doxygen) is a bit more involved. One thing that's missing without RTTI is the ability to determine if a node is a symbol, identifier, or type (or one would need to check for every kind).This PR adds support for
dyn_cast,isa, and friends tollvm::ms_demangle::Node. As the type already has akind(), this mainly addsclassofto the nodes as well as some start and end markers in theNodeKindenum.