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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I just took a look at SemaChecking, the rest I'm not sure I get the intent of the patch sufficiently to understand.



================
Comment at: clang/lib/Sema/SemaChecking.cpp:1442
 
+  enum class MemCheckType { Full, Basic };
+
----------------
Oh boy... all these lambdas are making me squeamish. 


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1444
+
+  auto CheckArityIs = [&](unsigned ExpectedArity) {
+    if (TheCall->getNumArgs() == ExpectedArity)
----------------
What is wrong with CheckArgCount (static function at the top of the file)?  It seems to do some nice additions here too.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1452
+  };
+  auto getPointeeOrArrayType = [&](clang::Expr *E) {
+    if (E->getType()->isArrayType())
----------------
What is this doing that ->getType()->getPointeeOrArrayElementType() doesn't do?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1457
+  };
+  auto CheckIsObjectPointerOrArrayType = [&](clang::Expr *E) {
+    if (E->getType()->isObjectPointerType())
----------------
Typically 'check' functions have the bool logic reversed.


================
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))
----------------
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.




================
Comment at: clang/lib/Sema/SemaChecking.cpp:1472
+        << InitializedEntity::EK_Parameter << Context.VoidPtrTy << E->isLValue()
+        << E->getType() << 0 << E->getSourceRange();
+    return false;
----------------
Put a comment in the message with some sort of hint as to what the '0' does.  Typically a << /*Whatever*/ 0<<


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1494
+  auto CheckIsTriviallyCopyablePointeeType = [&](clang::Expr *E) {
+    if (!(E->getType()->isPointerType() || E->getType()->isArrayType()))
+      return true;
----------------
Can you invert this logic?  !Ptr && !Array?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1509
+                                                clang::Expr *E1) {
+    if (!E0->getType()->isObjectPointerType() && !E0->getType()->isArrayType())
+      return true;
----------------
What if 1 of them is of these types?  Is that OK?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1527
+                                               clang::Expr *E1) {
+    if (!E0->getType()->isObjectPointerType() && !E0->getType()->isArrayType())
+      return true;
----------------
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?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:1533
+    QualType El1Ty = getPointeeOrArrayType(E1);
+    if (!(El0Ty->isAtomicType() || El1Ty->isAtomicType()))
+      return true;
----------------
invert these please.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79279/new/

https://reviews.llvm.org/D79279