[PATCH] D35278: Support: Add llvm::center_justify.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: include/llvm/Support/Format.h:132
   unsigned Width;
-  bool RightJustify;
+  unsigned Justify : 2;
   friend class raw_ostream;
----------------
Why don't you use `Justification` as a type? I think you don't need to save a few bits here.


================
Comment at: lib/Support/raw_ostream.cpp:334-343
+  int PadAmount = FS.Justify == FormattedString::JustifyCenter ? Difference / 2
+                                                               : Difference;
+  if (FS.Justify & FormattedString::JustifyRight) {
     this->indent(PadAmount);
+    if (FS.Justify == FormattedString::JustifyCenter)
+      PadAmount += Difference - (PadAmount * 2);
+  }
----------------
Using an enum value as a bitmask looks too tricky. It is probably better to write in three-way branches like this.

  switch (FS.Justify) {
  case JustifyRight:
    this->indent(FS.Width - Len);
    this->operator<<(FS.Str);
    break;
  case JustifyCenter: {
    int PadAmount = (FS.Width - Len) / 2;
    this->indent(PadAmount);
    this->operator<<(FS.Str);  
    this->indent(Difference - PadAmount * 2);
  }
  case JustifyLeft:
    this->operator<<(FS.Str);
    this->indent(FS.Width - Len);
    break;
  }



https://reviews.llvm.org/D35278