martong added a comment. > 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 :)
Yes. More specifically, the call of `getBitWidthValue()` caused a segfault with release builds and with assertions enabled it caused the mentioned assert on the given test code. In D88665#2307945 <https://reviews.llvm.org/D88665#2307945>, @teemperor wrote: > 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)); > } > Yes, absolutely, it should. I have updated the patch this way, thanks! However, I was struggling to pass the existing lit test (`ASTMerge/struct/test.c`). Finally, I decided to remove all the BitField diagnostic code because it was already flawed - we called getBitWidthValue unchecked. Currently, we are totally inconsistent about the diagnostics. Either we should emit a diagnostic before all `return false` or we should not ever emit any diags. The diagnostics in their current form are misleading, because there could be many notes missing. I am not sure how much do you guys value these diags in LLDB, but in CTU we simply don't care about them. I'd rather remove these diags from the equivalency check code, because they are causing only confusion. (Or we should properly implement and test all aspects of the diags.) 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
