jfb added inline comments.
================
Comment at: clang/include/clang/Basic/Builtins.def:489
BUILTIN(__builtin_memcpy_inline, "vv*vC*Iz", "nt")
+BUILTIN(__builtin_overloaded_memcpy, "v*v*vC*z", "nt")
BUILTIN(__builtin_memmove, "v*v*vC*z", "nF")
----------------
gchatelet wrote:
> `overloaded` doesn't bring much semantic (says the one who added
> `__builtin_memcpy_inline`...). Can you come up with something that describes
> more precisely what the intends are?
>
> Also `memset`, `memcmp`, `memcpy`, `memmove` will have their `inline` and
> `overloaded` versions. This is becoming a crowded space. It may be confusing
> in the long run. If we want to go in that direction maybe we should come up
> with a consistent pattern: `__builtin_<memfun>_<feature>`. WDYT?
Flipping it around is fine with me, see update (done with `sed`).
What's your approach on choosing what gets an `inline` variant and what
doesn't? `memcmp` is easy to add, but I wonder how far it's useful to go... I
can just wait for requests as well (as I imagine you're doing?).
================
Comment at: clang/lib/Sema/SemaChecking.cpp:1442
+ enum class MemCheckType { Full, Basic };
+
----------------
erichkeane wrote:
> Oh boy... all these lambdas are making me squeamish.
C++14 🎉
================
Comment at: clang/lib/Sema/SemaChecking.cpp:1444
+
+ auto CheckArityIs = [&](unsigned ExpectedArity) {
+ if (TheCall->getNumArgs() == ExpectedArity)
----------------
erichkeane wrote:
> What is wrong with CheckArgCount (static function at the top of the file)?
> It seems to do some nice additions here too.
It is most wonderful and has now taken over for valiant `CheckArityIs`. I'd
somehow not seen that! I had gripped for another error message and figured this
was what I needed.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:1452
+ };
+ auto getPointeeOrArrayType = [&](clang::Expr *E) {
+ if (E->getType()->isArrayType())
----------------
erichkeane wrote:
> What is this doing that ->getType()->getPointeeOrArrayElementType() doesn't
> do?
It keeps the qualifiers 🙂
Maybe I can make a separate `QualType` helper that does this?
================
Comment at: clang/lib/Sema/SemaChecking.cpp:1465
+ Expr::EvalResult Result;
+ if (E->getType()->isIntegerType() && !E->isValueDependent() &&
+ E->EvaluateAsInt(Result, Context) && (Result.Val.getInt() == 0))
----------------
erichkeane wrote:
> Why is a value-dependent expression not OK? Typically you'd want to not
> bother with dependence in the checking, and just check it during
> instantiation (the 2nd time this is called).
>
> Because it seems to me that this will error during phase 1 when an integer
> template parameter (or 'auto' parameter?) would be fine later.
>
>
```
bool Expr::EvaluateAsInt(EvalResult &Result, const ASTContext &Ctx,
SideEffectsKind AllowSideEffects,
bool InConstantContext) const {
assert(!isValueDependent() &&
"Expression evaluator can't be called on a dependent expression.");
EvalInfo Info(Ctx, Result, EvalInfo::EM_IgnoreSideEffects);
Info.InConstantContext = InConstantContext;
return ::EvaluateAsInt(this, Result, Ctx, AllowSideEffects, Info);
}
```
😊
It seems pretty common to use that check when trying to get a value out.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:1509
+ clang::Expr *E1) {
+ if (!E0->getType()->isObjectPointerType() && !E0->getType()->isArrayType())
+ return true;
----------------
erichkeane wrote:
> What if 1 of them is of these types? Is that OK?
It's to avoid weird corner cases where this check isn't super relevant, but
subsequent ones are. It avoids making `isVolatileQualified` below sad because
e.g. `void` makes the `QualType` null. That one can't be `_Atomic`, and it can
be `volatile` but then the size won't match the `_Atomic`'s size.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:1527
+ clang::Expr *E1) {
+ if (!E0->getType()->isObjectPointerType() && !E0->getType()->isArrayType())
+ return true;
----------------
erichkeane wrote:
> Same question as above. Is there other checks that need to happen here?
> Also, is there any ability to reuse some of the logic between these funcitons?
I don't think so here either.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79279/new/
https://reviews.llvm.org/D79279
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
