[PATCH] D44668: [demangler] WIP: Add a "partial" demangling API for LLDB

[Erik Pilkington via Phabricator via llvm-commits]

erik.pilkington added a reviewer: espindola.
erik.pilkington removed a subscriber: rafael.
erik.pilkington added a comment.

Use Rafael's Phab account.



================
Comment at: llvm/include/llvm/Demangle/Demangle.h:42
+  /// \return true on error, false otherwise
+  bool partialDemangle(const char *MangledName);
+
----------------
clayborg wrote:
> Use "llvm::StringRef MangledName" instead of "const char *"?
On second thought, I don't think we can use LLVM data structures for this. libSupport depends on libDemangle, so using any LLVM stuff would create a circular dependency. (I'm basing this off the commit message for r328072)


================
Comment at: llvm/include/llvm/Demangle/Demangle.h:46
+  /// second and third parameters to itaniumDemangle.
+  char *finishDemangle(char *Buf, size_t *N) const;
+
----------------
clayborg wrote:
> Might be better to use a "llvm::raw_ostream &out" instead of just a plain buffer? Seems like llvm::raw_ostream can be backed by what ever kind of buffer you want it to be backed (llvm::raw_string_ostream is a nice thing to use with this that uses a std::string as the destination buffer).
This has the same problem as above, but also raw_ostream isn't compatible with our how the AST is printed (Sometimes the printer look back at the last char).


================
Comment at: llvm/include/llvm/Demangle/Demangle.h:54
+  /// "a::b".
+  char *getFunctionContextName(char *Buf, size_t *N) const;
+
----------------
clayborg wrote:
> Maybe rename to getFunctionDeclContext? or getFunctionDeclContextName?
Sure, I called this getFunctionDeclContextName() in the new patch.


================
Comment at: llvm/include/llvm/Demangle/Demangle.h:64
+  /// know this if the function has a cv or reference qualifier.
+  bool isProvablyMemberFunction() const;
+
----------------
labath wrote:
> clayborg wrote:
> > erik.pilkington wrote:
> > > clayborg wrote:
> > > > s/provably/probably/ in comment and function name. Maybe "couldBeMemberFunction" might be a better name?
> > > That wasn't a typo, this function checks if the name is for sure a non-static member function, so it conservatively returns false for `a::b()`. I suppose `isDefinitelyMemberFunction()` might be more clear.
> > Maybe just "isMemberFunction()" would be clear enough? Don't think Definitely needs to be in the name.
> I think that having something in the name which indicates this is just a best-effort guess is a good idea.
> 
> Alternatively, we could replace this function with something more well-defined (getFunctionQualifiers ? hasFunctionQualifiers?) and leave the business of guessing the member-ness of the function to lldb.
I agree it would be better to indicate that this is only a best effort guess. hasFunctionQualifiers sounds good, I renamed this function to that in the new patch.


https://reviews.llvm.org/D44668