[PATCH] D36304: [lld] [COFF] Add support for aligncomm directives

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: COFF/Config.h:142
 
+  std::map<std::string, int> AlignComm;
+
----------------
Please add a comment saying that this is for /aligncomm.


================
Comment at: COFF/Driver.cpp:238
       break;
+    case OPT_aligncomm:
+      parseAligncomm(Arg->getValue());
----------------
Move to the top of this `switch` as `case`s are sorted alphabetically.


================
Comment at: COFF/Driver.cpp:1177
+    }
+    if (auto *DC = dyn_cast<DefinedCommon>(Sym->body())) {
+      if (auto *CC = DC->getChunk())
----------------
Since you are using early continue, I'd prefer early continue to handle the following warning too.

  auto *DC = dyn_cast<DefinedCommon>(Sym->body());
  if (!DC) {
    warn(("/aligncomm symbol " + Name + " of wrong kind");
    continue;
  }



================
Comment at: COFF/Driver.cpp:1178
+    if (auto *DC = dyn_cast<DefinedCommon>(Sym->body())) {
+      if (auto *CC = DC->getChunk())
+        CC->setAlign(1 << Align);
----------------
Is it possible that getChunk() returns a null?


================
Comment at: COFF/Driver.cpp:1179
+      if (auto *CC = DC->getChunk())
+        CC->setAlign(1 << Align);
+    } else {
----------------
Throughout the codebase, we store a power of two instead of log2 to `Align` variables. So please do shift when you parse a value instead of when you use a value.


================
Comment at: COFF/DriverUtils.cpp:230
+    fatal("/aligncomm: invalid argument: " + S);
+  int AlignValue;
+  if (Align.getAsInteger(0, AlignValue))
----------------
Since the scope is very narrow, use a shorter name for `AlignValue`. Probably I or V are enough.


https://reviews.llvm.org/D36304