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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
