[Lldb-commits] [PATCH] D61146: Set a CXXRecordDecl to not be passed in registers if DW_CC_pass_by_reference when loading from DWARF

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.


================
Comment at: packages/Python/lldbsuite/test/expression_command/argument_passing_restrictions/TestArgumentPassingRestrictions.py:26
+    lldbutil.run_to_source_breakpoint(self, '// break here',
+            lldb.SBFileSpec("main.cpp", False))
+
----------------
What does False do if it isn't the default argument? And if it is, can we remove it?


================
Comment at: packages/Python/lldbsuite/test/expression_command/argument_passing_restrictions/TestArgumentPassingRestrictions.py:29
+    self.expect("expr Shape::empty_shape()->bounds()",
+            substrs=['(Bounds) $0 = (x = 1, y = 2)'])
----------------
it's probably more future-proof to write this as `substrs=['(Bounds)', 'x = 1', 'y = 2'])`


================
Comment at: packages/Python/lldbsuite/test/expression_command/argument_passing_restrictions/main.cpp:1
+struct Bounds {
+  Bounds() = default;
----------------
Any comment at all to explain what's special here?


================
Comment at: packages/Python/lldbsuite/test/expression_command/argument_passing_restrictions/main.cpp:11
+class Shape {
+    static Shape *sp;
+public:
----------------
clang-format please


================
Comment at: packages/Python/lldbsuite/test/expression_command/argument_passing_restrictions/main.cpp:22
+      bounds.y = 2;
+      return;
+    }
----------------
what's the point of the return?


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:965
+              m_ast.GetAsCXXRecordDecl(clang_type.GetOpaqueQualType());
+          if (record_decl) {
+            record_decl->setArgPassingRestrictions(
----------------
at least in LLVM style we elide single-statement braces.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:967
+            record_decl->setArgPassingRestrictions(
+                clang::RecordDecl::APK_CannotPassInRegs);
+          }
----------------
This part LGTM.


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

https://reviews.llvm.org/D61146