Page MenuHomePhabricator

InferAddressSpaces: Handle a few intrinsics
AbandonedPublic

Authored by arsenm on Jan 26 2017, 8:16 PM.

Details

Diff Detail

Event Timeline

arsenm created this revision.Jan 26 2017, 8:16 PM
jlebar added inline comments.Jan 27 2017, 11:26 AM
lib/Transforms/Scalar/InferAddressSpaces.cpp
235

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

249

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.

621

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

arsenm abandoned this revision.Feb 1 2017, 4:40 PM
arsenm marked 3 inline comments as done.

Looks like I accidentally merged this with the other memintrinsic patch when I committed it. I pushed a format fix patch in r293843 to fix these