[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

Looks good, apart from some stylistic comments inline. Thank you for taking the time to do this right.



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3545
+
+DIEVector MergeBlockAbstractParameters(const DWARFDIE &block_die,
+                                       DIEVector &&variable_dies) {
----------------
In llvm, we prefer `static` functions over anonymous namespaces. Theoretically, you could keep the anonymous namespace around the using declaration (per <https://llvm.org/docs/CodingStandards.html#anonymous-namespaces>, as those can't use `static`), though I would actually probably prefer  DIEArray type defined in DIERef.h over a custom type.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3588
+          concrete_it->GetReferencedDIE(DW_AT_abstract_origin);
+      if (origin_of_concrete && origin_of_concrete == abstract_child) {
+        // The current abstract paramater is the origin of the current
----------------
I would assume this is redundant, as an invalid DIE will never match `abstract_child`.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:402
+      const lldb_private::SymbolContext &sc,
+      const llvm::SmallVectorImpl<DWARFDIE> &variable_dies,
+      const lldb::addr_t func_low_pc);
----------------
When you're not mutating the vector, the usual argument type is `ArrayRef<T>`


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:403
+      const llvm::SmallVectorImpl<DWARFDIE> &variable_dies,
+      const lldb::addr_t func_low_pc);
 
----------------
We generally do not put a const qualifier on by-value arguments (it's pretty useless).

(I see it's present on other functions too, but I don't know if they were introduced by you or you're just propagating them.)


================
Comment at: lldb/test/API/functionalities/unused-inlined-parameters/TestUnusedInlinedParameters.py:17
+
+        # For the unused parameters, only check the types.
+        self.assertEqual("void *", self.frame().FindVariable("unused1").GetType().GetName())
----------------
jarin wrote:
> labath wrote:
> > Maybe we could check something else as well... Do `GetDescription` or `str(value)` return something reasonable here?
> Actually, the most important thing is the type here, so this was quite deliberate.
> 
> GetDescription returns `(void *) unused1 = <no location, value may have been optimized out>\n\n`, but I am not sure if we can safely require that all future versions/platforms optimize the parameter out.
I don't feel strongly about it, but I would say that this function is so simple than any optimizer worthy of that name should be able to optimize those arguments away. I might replace printf with a `noinline`/`optnone` function though, to avoid any libc shenanigans.


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

https://reviews.llvm.org/D110571