[PATCH] D130324: [ODRHash] Hash `ObjCProtocolDecl` and diagnose discovered mismatches.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In D130324#3843934 <https://reviews.llvm.org/D130324#3843934>, @jansvoboda11 wrote:

> Should we also update `ODRHash::isDeclToBeProcessed`?

No, as `isDeclToBeProcessed` is only for sub-decls and protocols cannot be nested inside other entities. Looks like nested protocols are rejected at the parsing stage※, so I won't test such case.

Your comment is valuable as it reveals bad naming and I plan to rename `isDeclToBeProcessed` to `isSubDeclToBeProcessed` in a separate commit (suggestions for alternative names are welcome).

Footnotes
---------

※ - code like

  @protocol Foo
  @protocol Bar
  @end
  @end

is rejected with errors

  test.m:2:1: error: illegal interface qualifier
  @protocol Bar
  ^
  test.m:3:2: error: unknown type name 'end'
  @end
   ^
  test.m:4:2: error: prefix attribute must be followed by an interface, protocol, or implementation
  @end
   ^
  test.m:4:5: error: missing '@end'
  @end
      ^
  test.m:1:1: note: protocol started here
  @protocol Foo
  ^



================
Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:313
+    DeclarationName SecondProtocolName = SecondProtocol->getDeclName();
+    if (FirstProtocolName != SecondProtocolName) {
+      SourceLocation FirstLoc = *(FirstProtocols.loc_begin() + I);
----------------
jansvoboda11 wrote:
> Are we okay with simply comparing protocol //names// here instead of the full declarations? I guess we're relying on the top-level diagnostic for those, and avoiding recursing, correct?
The main reason to compare just names is so that we don't fail with forward-declared protocols that don't have full declarations (covered by tests). And yes, we are relying on the top-level diagnostic to find mismatches in the referenced protocols.

Note that we have the same behavior in `ODRHash::AddObjCProtocolDecl`.


================
Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:1660
+  case TypeDef:
+  case Var:
+  // C++ only, invalid in this context.
----------------
jansvoboda11 wrote:
> This will fall-through and call `llvm_unreachable`, will it not? I assume that's not intended.
Good question. Protocols cannot have fields or vars (properties are covered by D130326). Typedefs are top-level and not nested inside protocols, so that case cannot happen too. `Other` is covered by `FirstDiffType == Other || SecondDiffType == Other` and `EndOfClass` by `FirstDiffType != SecondDiffType`. So it is correct that in all of these cases we fall-through to `llvm_unreachable`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130324