rjmccall requested changes to this revision.
rjmccall added inline comments.
This revision now requires changes to proceed.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:3759
unsigned BuiltinID;
- if (Old->isImplicit() && (BuiltinID = Old->getBuiltinID())) {
+ bool RevertedBuiltin{Old->getIdentifier()->hasRevertedBuiltin()};
+ if (Old->isImplicit() &&
----------------
The old code didn't check this eagerly. The `Old->isImplicit()` check is
unlikely to fire; please make sure that we don't do any extra work unless it
does.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:3765
+ // Also warn if it's a redeclaration of a builtin redeclaration. We know if
+ // it is if the previous declaration is a reverted builtin.
+ if (RevertedBuiltin ||
----------------
`hasRevertedBuiltin()` is global state for the identifier, so this check isn't
specifically checking anything about the previous declaration, it's checking
whether the identifier was ever used for a builtin. Now, the implicit-ness of
the previous declaration *is* declaration-specific, and there are very few ways
that we end up with an implicit function declaration. It's quite possible that
implicit + identifier-is-or-was-a-builtin is sufficient to say that it's an
implicit builtin declaration today. However, I do worry about the fact that
this change is losing the is-predefined-library-function part of the existing
check, and I think we should continue to check that somehow. If that means
changing how we revert builtin-ness, e.g. by continuing to store the old
builtinID but just recording that it's been reverted, that seems reasonable.
That said, I think there's a larger problem, which is that you're ignoring half
of the point of the comment you've deleted:
> // Doing this for local extern declarations is problematic. If
> // the builtin declaration remains visible, a second invalid
> // local declaration will produce a hard error; if it doesn't
> // remain visible, a single bogus local redeclaration (which is
> // actually only a warning) could break all the downstream code.
The problem with reverting the builtin-ness of the identifier after seeing a
bad local declaration is that, after we leave the scope of that function, the
global function doesn't go back to being a builtin, which can break the
behavior of all the rest of the code.
If we want to handle this kind of bogus local declaration correctly, I think we
need to stop relying primarily on the builtin-ness of the identifier — which is
global state and therefore inherently problematic — and instead start recording
whether a specific declaration has builtin semantics. The most obvious way to
do that is to record an implicit `BuiltinAttr` when implicitly declaring a
builtin, inherit that attribute only on compatible redeclarations, and then
make builtin queries look for that attribute instead of checking the identifier.
Richard, do you agree?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77491/new/
https://reviews.llvm.org/D77491
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
