rsmith added inline comments.
================
Comment at: clang/lib/CodeGen/CGBuilder.h:53
+ : CGBuilderBaseTy(C), TypeCache(TypeCache) {}
+ CGBuilderTy(const CodeGenTypeCache &TypeCache, llvm::LLVMContext &C,
+ const llvm::ConstantFolder &F,
----------------
There are a lot of unrelated whitespace changes (`clang-format`, I assume) in
this file. Can you produce a separate patch with just the whitespace changes
and go ahead and check that in first?
================
Comment at: clang/lib/Sema/SemaChecking.cpp:2293-2315
+ case Builtin::BI__builtin_memset_inline: {
+ if (checkArgCount(*this, TheCall, 3))
+ return ExprError();
+ auto ArgArrayConversionFailed = [&](unsigned Arg) {
+ ExprResult ArgExpr =
+ DefaultFunctionArrayLvalueConversion(TheCall->getArg(Arg));
+ if (ArgExpr.isInvalid())
----------------
This custom checking seems unnecessary and also insufficient: you're not
enforcing that the arguments have the right types. We should convert the
arguments to this builtin in the same way we'd convert arguments for a
normally-declared function, which you can achieve by removing the "t" flag from
the builtin definition and removing all of this logic (except for the nonnull
checking, which I don't think we have a way to model in Builtins.def yet).
Please do the same to `__builtin_memcpy_inline`, which is wrong in the same
way. (For example, `__builtin_memcpy_inline(1, 2, 3)` crashes in code
generation because it fails to perform proper conversions and type-checking on
its arguments.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126903/new/
https://reviews.llvm.org/D126903
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
