shafik marked 17 inline comments as done.
shafik added a comment.
@martong I don't follow the discussion on `VisitParmVarDecl` an what seems like
an alternate fix. Can you elaborate?
================
Comment at: lib/AST/ASTImporter.cpp:3042
+ if (FoundByLookup) {
+ if (auto *MD = dyn_cast<CXXMethodDecl>(FoundByLookup)) {
----------------
martong wrote:
> It is not trivial why we add this block to the code, so I think a comment
> would be really appreciated here.
> I was thinking about something like this:
> ```
> + // We do not allow more than one in-class prototype of a function. This is
> + // because AST clients like VTableBuilder asserts on this. VTableBuilder
> + // assumes that only one function can override a function. Building a
> redecl
> + // chain would result there are more than one function which override the
> + // base function (even if they are part of the same redecl chain inside the
> + // derived class.)
> ```
Just for clarification, from a C++ perspective, I would say "in-class
declaration" rather than "in-class prototype" and therefore `VTableBuilder`
assumes only one in class declaration and building a redecal chain would result
in more than one in-class declaration being present.
Does that makes sense?
================
Comment at: unittests/AST/ASTImporterTest.cpp:2344
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceOutOfClassDefTwoTUs) {
+ auto CodeTU0 =
----------------
balazske wrote:
> The previous tests use two TUs too so the name for this is not exact (here
> the code is different, maybe use
> `ImportOverriddenMethodTwiceOutOfClassDefInSeparateCode`?). This test does
> roughly the same as `ImportOverriddenMethodTwiceDefinitionFirst` but with the
> definition last and out-of-class version. (Name can be
> `ImportOverriddenMethodTwiceOutOfClassDefLast` too.) I am not sure why is the
> `foo` used here (`B::F` can be imported instead).
I originally thought I did not need `foo` either but I could get it to import
the definition of `B::f()` any other way.
I also saw similar behavior with `clang-import-test` where I need an expression
that included something like `foo()` in order to obtain the out-of-line
definition. It is not obvious to me why.
So in this case if I switch to using `BFP` instead of `FooDef` then this fails:
auto *ToBFOutOfClass = FirstDeclMatcher<CXXMethodDecl>().match(
ToTU, cxxMethodDecl(hasName("f"), isDefinition()));
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56936/new/
https://reviews.llvm.org/D56936
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
