[PATCH] D29212: InferAddressSpaces: Handle a few intrinsics

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
Comment at: lib/Transforms/Scalar/InferAddressSpaces.cpp:235
+    Function *NewDecl
+      = Intrinsic::getDeclaration(M, II->getIntrinsicID(), { DestTy, SrcTy });
+    II->setArgOperand(0, NewV);
----------------
Nit, idiom is to wrap `=` on the first line, I think.  Also idiom is not to have spaces around the curly braces, I believe.  (I care because I just want to clang-format my code, and so if this adheres to a different format, then when I go and edit this, I'm going to have spurious formatting changes, which ends up costing reviewer time.  Same applies if I'm not the author but the reviewer.)


================
Comment at: lib/Transforms/Scalar/InferAddressSpaces.cpp:249
+  std::vector<std::pair<Value *, bool>> *PostorderStack,
+  DenseSet<Value *> *Visited) const {
+  switch (II->getIntrinsicID()) {
----------------
Really prefer clang-format's formatting of this function header:

    void InferAddressSpaces::collectRewritableIntrinsicOperands(
        IntrinsicInst *II, std::vector<std::pair<Value *, bool>> *PostorderStack,
        DenseSet<Value *> *Visited) const {
      switch (II->getIntrinsicID()) {
      case Intrinsic::objectsize:

By indenting four spaces rather than two, it becomes unambiguous which lines are part of the function header vs the body.  As-is I had to read it multiple times to parse it correctly.

If you're amenable, we could save ourselves a lot of energy by simply agreeing to use clang-format (and to file bugs when it's ugly -- djasper is pretty responsive).

For myself, I've wrapped `arc diff` so it runs git-clang-format over all the lines I touch, so there's no additional cognitive load: https://github.com/jlebar/conf/blob/master/bin/arc.  The `git-clang-format` script is in clang/tools/clang-format/git-clang-format.


================
Comment at: lib/Transforms/Scalar/InferAddressSpaces.cpp:621
+bool InferAddressSpaces::handleComplexPtrUse(User &U,
+                                             Value *OldV, Value *NewV) const {
   if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(&U)) {
----------------
Nit, suggest

    bool InferAddressSpaces::handleComplexPtrUse(User &U, Value *OldV,
                                                 Value *NewV) const {

This is more aesthetically pleasing (and so, marginally easier to read) because the second line isn't a lot longer than the first.  (And yes, this is clang-format's output.  :)


https://reviews.llvm.org/D29212