[PATCH] D50152: [CodeGen] Merge equivalent block copy/helper functions

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/AST/ASTContext.h:248
+  /// Mapping from __block VarDecls to their copy initialization expr. The
+  /// boolean flag indicates whether the expression can throw.
+  typedef llvm::DenseMap<const VarDecl *,
----------------
rjmccall wrote:
> Maybe you should just make a type for this pairing.  You can put this documentation there, and the access functions can take and return it.
I don't think the `typedef` is needed here.


================
Comment at: include/clang/AST/ASTContext.h:158
+    Expr *CopyExpr;
+    bool CanThrow;
+  };
----------------
Using a PointerIntPair in the implementation of this is still a reasonable choice.  Just make it a proper abstraction, with a constructor with the two arguments and getters for the two fields.


================
Comment at: include/clang/AST/ASTContext.h:2680
+  /// indicates whether the copy expression can throw or not.
+  void setBlockVarCopyInits(const VarDecl* VD, Expr *CopyExpr, bool CanThrow);
 
----------------
I know this is longstanding, but since you're changing all the call sites anyway, please remove the trailing `s` from these two method names.


================
Comment at: lib/AST/ASTContext.cpp:2511
          "getBlockVarCopyInits - not __block var");
-  llvm::DenseMap<const VarDecl*, Expr*>::iterator
-    I = BlockVarCopyInits.find(VD);
-  return (I != BlockVarCopyInits.end()) ? I->second : nullptr;
+  BlockVarCopyInitMap::const_iterator I = BlockVarCopyInits.find(VD);
+  if (I != BlockVarCopyInits.end())
----------------
I think `auto` is okay for things like this.


================
Comment at: lib/CodeGen/CGBlocks.cpp:1682
+          if (IsCopyHelper && Ctx.getBlockVarCopyInits(Var).CanThrow)
+            Name += "c";
+        }
----------------
I don't think you need to add `d` to the name of a copy helper.  It's a bit weird, but while copying a `__block` variable can cause its copy helper to run, destroying it immediately afterwards can never cause its destroy helper to run.  That's because a newly-copied `__block` variable always has a reference count of 2: the new reference in the copy and the forwarding reference from the original.

I think that means you can just add a single letter which specifies whether the corresponding `__block` variable operation is known to be able to throw.


================
Comment at: lib/CodeGen/CGBlocks.cpp:1688
+        if (F == BLOCK_FIELD_IS_BLOCK)
+          Name += "b";
+      }
----------------
Why `rb` for a captured block instead of some single-letter thing?  You don't need to emulate the structure of the flags here.


================
Comment at: lib/CodeGen/CGBlocks.cpp:1705
+                                                          IsVolatile, Ctx);
+      Name += llvm::to_string(Str.size()) + "_" + Str;
+      break;
----------------
The underscore is necessary here because non-trivial destructor strings can start with a number?  Worth a comment.


Repository:
  rC Clang

https://reviews.llvm.org/D50152