[PATCH] D151523: [ASTStructuralEquivalence] Fix crash when ObjCCategoryDecl doesn't have corresponding ObjCInterfaceDecl.

[Volodymyr Sapsai via Phabricator via cfe-commits]

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