[Lldb-commits] [PATCH] D75761: Fix to get the AST we generate for function templates to be closer to what clang generates and expects

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

The general mechanics look good, I have a few comments about the implementation.



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:821
 
+static clang::Optional<llvm::StringRef>
+StripTemplateParameters(llvm::StringRef Name) {
----------------
Nit: llvm::Optional

However, usually just a plain StringRef is enough, since it can be empty().


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:823
+StripTemplateParameters(llvm::StringRef Name) {
+  // We are looking for template parameters to strip from Name. e.g.
+  //
----------------
Nit: This would be nicer as the doxygen comment of the function.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:829
+  // have something like operator>>. We check for the operator<=> case.
+  if (!Name.endswith(">") || Name.count("<") == 0 || Name.endswith("<=>"))
+    return {};
----------------
Should we scan for the complete "operator<" "operator<=>" instead, to be clearer and more future-proof?
Should we assert(Name.count("<")), too?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:839
+  size_t RightAngleCount = Name.count('>');
+  size_t LeftAngleCount = Name.count('<');
+
----------------
We are scanning the entire string multiple times here and that seems unnecessarily expensive since C++ template function names get looong. I think we could do this as a single for-loop with a state machine instead, without being too difficult to read?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:850
+
+  return Name.substr(0, StartOfTemplate - 1);
+}
----------------
This function would probably benefit from a unit test even if that means it can't be static any more. I just can't prove to myself that there isn't an off-by-one error lurking in it somewhere.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1202
       if (!function_decl) {
+        llvm::StringRef name_without_template_params_ref =
+            attrs.name.GetStringRef();
----------------
Can you add a comment here explaining why the template parameters are being stripped? I don't think that's obvious to the casual reader.


================
Comment at: lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py:63
+          self.expect("expr c1 == c2",
+           substrs=['(bool)', '= true'])
 
----------------
Are all of these strings exercising the stripping function? Perhaps that's just as good as a unit test then? But the unit test would still be better to document the expectations of the function.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75761/new/

https://reviews.llvm.org/D75761