[PATCH] D154701: [clang] Overridden CXXMethodDecl::isVirtual() assertion failed before fully imported.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

It is possible to reproduce the same problem with this test added to ASTImporterTest.cpp:

  TEST_P(ASTImporterOptionSpecificTestBase, ImportVirtualOverriddenMethodTest) {
    const char *Code =
        R"(
        void f1();
        class A {
          virtual void f(){}
        };
        class B: public A {
          void f() override {
            f1();
          }
        };
        class C: public B {
          void f() override {}
        };
        void f1() { C c; }
        )";
    Decl *FromTU = getTuDecl(Code, Lang_CXX11);
  
    auto *FromF = FirstDeclMatcher<CXXMethodDecl>().match(
        FromTU, cxxMethodDecl(hasName("B::f")));
  
    auto *ToBF = Import(FromF, Lang_CXX11);
    EXPECT_TRUE(ToBF->isVirtual());
  
    auto *ToCF = FirstDeclMatcher<CXXMethodDecl>().match(
        ToBF->getTranslationUnitDecl(), cxxMethodDecl(hasName("C::f")));
    EXPECT_TRUE(ToCF->isVirtual());
  }

I am not opposed to removal of the assertion as fix because it looks not very important (probably it can be replaced by another assertion for example to not allow constructors here, see this <https://github.com/llvm/llvm-project/commit/913590247898627144f62c0e48d0fe25f9d34e76> commit) but another person in the AST area should check this.



================
Comment at: clang/test/Analysis/ctu-astimport-virtual-assertion/main.cpp:22
+
+#include "Inputs/input.h"
----------------
Such tests are not in the //Analysis// folder but in the //ASTMerge// folder instead. I would say that this test is not necessary if the other test (in my added note) is inserted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154701