[Lldb-commits] [PATCH] D92643: [lldb] Lookup static const members in FindGlobalVariables

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2162
+      llvm::StringRef parent_type_name = GetDWARFDeclContext(parent)
+                                             .GetQualifiedNameAsConstString()
+                                             .GetStringRef();
----------------
ConstString here is needlessly expensive to construct and it is then used only once. Use plain `const char *` or `std::string` is also much cheaper.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2162
 
 void SymbolFileDWARF::FindGlobalVariables(const RegularExpression &regex,
                                           uint32_t max_matches,
----------------
jankratochvil wrote:
> ConstString here is needlessly expensive to construct and it is then used only once. Use plain `const char *` or `std::string` is also much cheaper.
This function also needs to be patched (with a testcase) as this command works:
```
(lldb) target variable Vars::inline_static
(double) Vars::inline_static = 1.5
```
But this one does not (and it should work):
```
(lldb) target variable -r Vars::inline_static
error: can't find global variable 'Vars::inline_static'
```


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2166
+      // This type is from another scope, skip it.
+      if (!parent_type_name.endswith(context))
+        return true;
----------------
Could this case have a testcase so that it cannot find such false positives?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3129
 
+VariableSP SymbolFileDWARF::ParseStaticConstMemberDIE(
+    const lldb_private::SymbolContext &sc, const DWARFDIE &die) {
----------------
Could you try to merge this functionality into `SymbolFileDWARF::ParseVariableDIE`? There is now some code duplication. I hope the merge will not become too ugly. (@labath was also suggesting this.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92643