vsapsai added a comment. In D151523#4396010 <https://reviews.llvm.org/D151523#4396010>, @ahatanak wrote:
> In D151523#4374808 <https://reviews.llvm.org/D151523#4374808>, @vsapsai wrote: > >> Kinda a follow-up to D121176 <https://reviews.llvm.org/D121176>. >> >> One big alternative that I've considered is to store interface name in >> `ObjCCategoryDecl` because when we parse `@interface >> InterfaceName(CategoryName)` we know the interface name. The intention was >> to allow >> >> @interface A(X) @end >> >> and >> >> @interface A @end >> @interface A(X) @end >> >> as equivalent. But I'm not convinced it is the right call and that's why it >> doesn't justify the extra effort. > > Are there any cases you are aware of where this change would fix a bug? In > any case, it sounds like that is something that can be done as a follow-up > patch after fixing the crash. No, I don't know about any such cases. My conclusion is that a category without an interface is invalid code, so trying to do something smart in this situation is a questionable approach. That's why I've decided not to do anything smart for a missing interface. ================ Comment at: clang/lib/AST/ASTStructuralEquivalence.cpp:2062 + *Intf2 = D2->getClassInterface(); + if ((Intf1 != nullptr) != (Intf2 != nullptr)) + return false; ---------------- ahatanak wrote: > vsapsai wrote: > > shafik wrote: > > > I think this would be easier to read if you checked `Intf1 != Intf2` and > > > then checked for `nullptr` > > I am totally up to the style that is more readable and consistent. I was > > just trying to mimic the check for `Template1` and `Template2`. I agree > > that 1 (**one**) datapoint isn't representative, so I can check this file > > more thoroughly for the prevalent style. Do you have any other places in > > mind that are worth checking? I'll look for something more representative > > but it would help if you have something in mind already. > `!Intf1 != !Intf2` should work too. So after checking for the common patterns I've found the following examples ```lang=c++ if (!Child1 || !Child2) return false; ``` ```lang=c++ if (!S1 || !S2) return S1 == S2; ``` ```lang=c++ if (T1.isNull() || T2.isNull()) return T1.isNull() && T2.isNull(); ``` ```lang=c++ if (TemplateDeclN1 && TemplateDeclN2) { ... } else if (TemplateDeclN1 || TemplateDeclN2) return false; ``` So I went for `(!Intf1 || !Intf2)` check and added `(Intf1 != Intf2)` because we can check further if both interfaces are nullptr. The boolean expression is more complex than before but it captures my thought process better. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151523/new/ https://reviews.llvm.org/D151523 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
