[PATCH] D60376: [llvm-objdump] Align instructions to a tab stop in disassembly output

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 2 inline comments as done.
MaskRay added inline comments.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:622
+      unsigned Column = FOS.getColumn();
+      FOS.indent(Column < TabStop - 1 ? TabStop - 1 - Column : 7 - Column % 8);
+
----------------
sidneym wrote:
> jhenderson wrote:
> > MaskRay wrote:
> > > jhenderson wrote:
> > > > MaskRay wrote:
> > > > > jhenderson wrote:
> > > > > > I can never remember the precedence of `%`, so would you mind adding some brackets to the LHS.
> > > > > > 
> > > > > > `7 - Column` seems a bit weird given that `Column` is always going to be bigger than that. Could you rewrite this bit so that it doesn't rely on underflowing?
> > > > > Multiplication, division and remainder have the same precedence. Just as `1 - 2 / 3` doesn't need parentheses, the remainder expression doesn't need parentheses.
> > > > > 
> > > > > `7 - Column % 8` computes to the range 0~7, there is no underflow.
> > > > Oh, right, that's me misremembering things. Even more of a reason to add brackets around `Column % 8` then. They aren't needed for correctness, but are for readability.
> > > I don't agree. For me, the redundant pair of parentheses harms readability.
> > Hmm... okay. I know others I work with have the same issue as I do in interpreting it quickly when reading.
> I find parentheses very useful for instant understanding of expressions.
> I find parentheses very useful for instant understanding of expressions.

I also know a group of people who can't stand superfluous parentheses.

This passes -Wparentheses. Note, some rules in -Wparentheses itself is debatable.

This is not the only instance in the code base.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60376