[PATCH] D70720: [llvm-objdump] Display locations of variables alongside disassembly

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/Support/FormattedStream.cpp:42
+      MultiByte = true;
+    }
+
----------------
Perhaps move `MultiByte` inside the loop and then do:

```
bool MultiByte = true;
if ((*Ptr & 0b11100000) == 0b11000000)
  Ptr += 1;
else if ((*Ptr & 0b11110000) == 0b11100000)
  Ptr += 2;
else if ((*Ptr & 0b11111000) == 0b11110000)
  Ptr += 3;
else
  MultiByte = false;

...

if (MultiByte)
  continue;
```


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:568
+  void AddParens(unsigned NewPrecedence) {
+    if (NewPrecedence < Precedence) {
+      SmallString<20> NewString;
----------------
Use an early return?

```
if (NewPrecedence >= Precedence)
  return;
```


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:872
+
+    auto Locs = VarDie.getLocations(dwarf::DW_AT_location);
+    if (Locs) {
----------------
Please avoid using `auto` when return type is not obvious.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:906
+        AddFunction(Child);
+      }
+    }
----------------
You do not need to use curly bracers here. Because both branches contain only single lines.

```
      if (Child.getTag() == dwarf::DW_TAG_variable ||
          Child.getTag() == dwarf::DW_TAG_formal_parameter)
        AddVariable(D, Child);
      else
        AddFunction(Child);
```


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:933
+        return ColIdx;
+    }
+    size_t OldSize = ActiveCols.size();
----------------
The same: no need to use curly braces here.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:981
+      ActiveCols[ColIdx].LiveOut = LV.liveAtAddress(NextAddr);
+      LLVM_DEBUG(dbgs() << "pass 1, " << ThisAddr.Address << "-"
+                        << NextAddr.Address << ", " << LV.VarName << ", Col "
----------------
Interesting to hear from others, I am not sure, do we allow `LLVM_DEBUG` in `llvm-objdump` code?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1098
+      printAfterOtherLine(OS, false);
+    }
+  }
----------------
No need to have curly bracers.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1108
+         ++ColIdx) {
+      if (ActiveCols[ColIdx].isActive()) {
+        if (ActiveCols[ColIdx].LiveIn && ActiveCols[ColIdx].LiveOut)
----------------
I'd suggest to reduce nesting here


```
if (ActiveCols[ColIdx].isActive()) {
  OS << "  ";
  continue;
}
```


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1803
+      LVP.AddCompileUnit(CU->getUnitDIE(false));
+    }
+  }
----------------
Remove bracers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70720