[Lldb-commits] [PATCH] D138834: [lldb] Fix simple template names interaction with debug info declarations

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:747-749
+  if (ParseTemplateParameterInfos(die, template_param_infos) &&
+      (!template_param_infos.args.empty() ||
+       template_param_infos.packed_args)) {
----------------
Could invert this condition and use an early `return ConstString();` to reduce indentation - but I guess this looks more similar to the definition handling case and so might be worth keeping in the same shape. (maybe `ParseTemplateParameterInfos` should return true only if the result is !empty or has packed args, so that test doesn't need to be repeated at multiple callers?)


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3069-3079
+        if (auto i = test_base_name.find('<'); i != llvm::StringRef::npos) {
+          llvm::StringRef test_template_params =
+              test_base_name.slice(i, test_base_name.size());
+          // Bail if template parameters don't match.
+          if (test_template_params != template_params.GetStringRef())
+            return true;
+        } else {
----------------
might be worth flipping to reduce indentation:
```
auto i = test_base_name.find('<');
if (i == llvm::StringRef::npos)
  return true;
llvm::StringRef test_template_params = ...
if (test_template_params != template_params.GetStringRef())
  return true;
```


================
Comment at: lldb/test/API/lang/cpp/unique-types3/main.cpp:1-14
+#include <atomic>
+#include <vector>
+
+std::atomic<double> a1;
+std::atomic<int> a2;
+std::atomic<float> a3;
+
----------------
maybe good to simplify this a bit - rather than using big/complex templates like std::atomic and std::vector, instead using test-only, simpler templates to keep the test a bit more focussed (less likely to fail for unrelated reasons/other regressions)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138834