rsmith added a comment.
In D77491#2246948 <https://reviews.llvm.org/D77491#2246948>, @rjmccall wrote:
> I'd still like @rsmith to sign off here as code owner.
Generally, I'm happy with this direction.
What happens for builtins with the "t" (custom typechecking) flag, for which
the signature is intended to have no meaning? Do we always give them builtin
semantics, or never, or ... something else? I think it might be reasonable to
require them to always be declared as taking unspecified arguments -- `()` in C
and `(...)` in C++, or to simply say that the user cannot declare such
functions themselves.
================
Comment at: clang/lib/Headers/intrin.h:148
void __writemsr(unsigned long, unsigned __int64);
-static __inline__
-void *_AddressOfReturnAddress(void);
-static __inline__
-unsigned char _BitScanForward(unsigned long *_Index, unsigned long _Mask);
-static __inline__
-unsigned char _BitScanReverse(unsigned long *_Index, unsigned long _Mask);
+__inline__ void *_AddressOfReturnAddress(void);
+__inline__ unsigned char _BitScanForward(unsigned long *_Index,
----------------
Does the `__inline__` here do anything for a builtin function? Can we remove it
along with the `static`?
================
Comment at: clang/lib/Headers/intrin.h:200
void __incgsword(unsigned long);
-static __inline__
-void __movsq(unsigned long long *, unsigned long long const *, size_t);
+static __inline__ void __movsq(unsigned long long *, unsigned long long const
*,
+ size_t);
----------------
Why is `static` being removed from some of the functions in this header but not
others?
================
Comment at: clang/lib/Sema/SemaDecl.cpp:9668-9694
+ // In C builtins get merged with implicitly lazily created declarations.
+ // In C++ we need to check if it's a builtin and add the BuiltinAttr here.
+ if (getLangOpts().CPlusPlus) {
+ if (IdentifierInfo *II = Previous.getLookupName().getAsIdentifierInfo()) {
+ if (unsigned BuiltinID = II->getBuiltinID()) {
+ FunctionDecl *D = CreateBuiltin(II, BuiltinID, NewFD->getLocation());
+
----------------
I think this needs more refinement:
* You appear to be creating and throwing away a new builtin function
declaration (plus parameter declarations etc) each time you see a declaration
with a matching name, even if one was already created. Given that you don't
actually use `D` for anything other than its type, creating the declaration
seems redundant and using `ASTContext::GetBuiltinType` would be more
appropriate.
* There are no checks of which scope the new function is declared in; this
appears to apply in all scopes, but some builtin names are only reserved in the
global scope (those beginning with an underscore followed by a lowercase letter
such as `_bittest`), so that doesn't seem appropriate. The old code in
`FunctionDecl::getBuiltinID` checked that the declaration is given C language
linkage (except for `_GetExceptionInfo`, which was permitted to have C++
language linkage), and that check still seems appropriate to me.
* The special case permitting `_GetExceptionInfo` to be declared with *any*
type seems suspect; the old code permitted it to have different language
linkage, not the wrong type.
* Using `typesAreCompatible` in C++-specific code is weird, since C++ doesn't
have a notion of "compatible types".
================
Comment at: clang/lib/Serialization/ASTReader.cpp:975
bool HasRevertedTokenIDToIdentifier = readBit(Bits);
- bool HasRevertedBuiltin = readBit(Bits);
+ readBit(Bits); // Previously used to indicate reverted builtin.
bool Poisoned = readBit(Bits);
----------------
We don't have any stability guarantees for our AST bitcode format yet; you can
just remove this bit rather than retaining a placeholder.
================
Comment at: clang/test/Analysis/bstring.cpp:106
Derived d;
- memset(&d.d_mem, 0, sizeof(Derived));
+ memset(&d.d_mem, 0, sizeof(Derived)); // expected-warning {{'memset' will
always overflow; destination buffer has size 4, but size argument is 8}}
clang_analyzer_eval(d.b_mem == 0); // expected-warning{{UNKNOWN}}
----------------
This should not be recognized as a builtin, because the `memset` function is
not `extern "C"`.
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
