[PATCH] D88665: [ASTImporter][AST] Fix structural equivalency crash on dependent FieldDecl

[Raphael Isemann via Phabricator via cfe-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

Shouldn't this compare the actual width expressions? In other words, this patch wouldn't pass the test below:

  TEST_F(StructuralEquivalenceTemplateTest, DependentFieldDeclDifferentVal) {
    const char *Code1 = "template <class A, class B> class foo { int a : sizeof(A); };";
    const char *Code2 = "template <class A, class B> class foo { int a : sizeof(B); };";
    auto t = makeNamedDecls(Code1, Code2, Lang_CXX03);
    EXPECT_FALSE(testStructuralMatch(t));
  }

I don't want to derail the simple crash fix here, but I'm tempted to say that we might as well just simplify all this to `return IsStructurallyEquivalent(Context, Field1->getBitWidth(), Field2->getBitWidth());`. The nice diagnostic could live behind behind a check that ensures that both widths are not value-dependent.

If you want to keep this simple because you want to backport it I would also be fine with the current patch (not crashing is clearly an improvement)

In D88665#2307562 <https://reviews.llvm.org/D88665#2307562>, @shafik wrote:

> So was the bug we were saying there were falsely equivalent or falsely not equivalent?

I think the bug is the crash :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88665