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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
