[Lldb-commits] [PATCH] D105564: Fix for DWARF parsing to better handle auto return type for member functions

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:402-403
     case DW_AT_type:
-      type = form_value;
+      if (!type.IsValid())
+        type = form_value;
       break;
----------------
JDevlieghere wrote:
> What's the purpose of this? Do we expect to see the type attribute more than once? 
Good question, the first iteration was done by @teemperor and then I modified it heavily but I did not dig  into this change to deeply but I was pretty sure when we first talked about there was a good reason for it.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1337-1339
+    TypeSP ret_type = dwarf->FindTypeForAutoReturnForDIE(
+        die, ConstString(attrs.mangled_name));
+    if (ret_type) {
----------------
JDevlieghere wrote:
> LLVM likes to put these variables in the if-clause, which I personally really like because it conveys the scope without hurting readability. 
Good catch, I was changing this code around a lot and forgot to go back and clean this up.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2672-2675
+  TypeSP type_sp;
+
+  if (!name)
+    return type_sp;
----------------
JDevlieghere wrote:
> I know this pattern is common in LLDB, but I really dislike it because you have to backtrack all the way to the beginning of the function to know if `type_sp` has been modified in any way. When I write code like, this I tend to use `return {};` to make it clear I'm returning a default constructed instance of the return type. That also makes it clear where we're actually returning a non-default instance by just looking at the `return`s. 
Good catch, I originally based this on `FindCompleteObjCDefinitionTypeForDIE` and stuck with the idiom w/o thinking about it.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:406
 
+  virtual lldb::TypeSP
+  FindTypeForAutoReturnForDIE(const DWARFDIE &die,
----------------
shafik wrote:
> teemperor wrote:
> > If this is virtual then I guess `SymbolFileDWARFDwo` should overload it?
> I am actually not sure if this has to be virtual or not, let me dig into this and see if there would be any difference for `SymbolFileDWARFDwo` or not.
Since this is based on the same approach in `GetObjCClassSymbol` and that is not `virtual` I am going to drop it.


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

https://reviews.llvm.org/D105564