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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere 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;
----------------
What's the purpose of this? Do we expect to see the type attribute more than once? 


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:900-901
+  const auto *ft = qt->getAs<clang::FunctionType>();
+  TypeSystemClang *ts =
+      llvm::dyn_cast_or_null<TypeSystemClang>(function_type.GetTypeSystem());
+  ast.adjustDeducedFunctionResultType(
----------------
You're doing `dyn_cast_or_null` but then below you're dereferencing `ts` unconditionally? 


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1337-1339
+    TypeSP ret_type = dwarf->FindTypeForAutoReturnForDIE(
+        die, ConstString(attrs.mangled_name));
+    if (ret_type) {
----------------
LLVM likes to put these variables in the if-clause, which I personally really like because it conveys the scope without hurting readability. 


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1341-1344
+      auto *function_decl = llvm::dyn_cast_or_null<clang::FunctionDecl>(
+          GetCachedClangDeclContextForDIE(die));
+
+      if (function_decl)
----------------



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2672-2675
+  TypeSP type_sp;
+
+  if (!name)
+    return type_sp;
----------------
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. 


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2718
+
+      type_sp = func_type->shared_from_this();
+    }
----------------



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

https://reviews.llvm.org/D105564