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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to