[PATCH] D79279: Add overloaded versions of builtin mem* functions

[JF Bastien via Phabricator via cfe-commits]

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