[PATCH] D41997: Build virtual override tables in DWARFASTParserClang::CompleteTypeFromDWARF

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2103
 
+static bool isOverload(clang::CXXMethodDecl *m1, clang::CXXMethodDecl *m2) {
+  // FIXME: This should detect covariant return types, but currently doesn't.
----------------
aprantl wrote:
> Could you add some doxygen comments explaining what the new function do and why doing this is necessary?
Will do.


================
Comment at: Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2105-2106
+  // FIXME: This should detect covariant return types, but currently doesn't.
+  assert(&m1->getASTContext() == &m2->getASTContext() &&
+         "Methods should have the same AST context");
+  clang::ASTContext &context = m1->getASTContext();
----------------
clayborg wrote:
> Use lldb_assert and possibly return false afterwards in case the asserts are compiled out
We can switch to lldb_assert to give us more control (at compile time) about when we turn it on or off, but I don't think we should bail out: this is an invariant, rather than a potential error case.


================
Comment at: Python/lldbsuite/test/expression_command/call-overridden-method/main.cpp:15
+  Base *b = &d;
+  return 0; // Please test these expressions while stopped at this line:
+}
----------------
jingham wrote:
> aprantl wrote:
> > the expressions are missing :-)
> > Perhaps convert this into an inline testcase?
> The expressions are in the test .py file.  It isn't necessary to put them here, I'd just fix the comment.
> 
> Please don't convert regular test cases into inline ones, however.  The benefit of inline test cases is mostly that they are easier to write, but they are harder to debug when they go wrong.  So if the are already in regular form I'd rather not convert them.
The text of the comment was just cribbed from one of the other expression tests. Is there a preferred phraseology?

  return 0; // run expressions here

?


Repository:
  rL LLVM

https://reviews.llvm.org/D41997