[PATCH] D40849: CodeGen: support an extension to pass linker options on ELF

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lib/CodeGen/TargetLoweringObjectFileImpl.cpp:97
+
+    llvm::SmallString<128> Descriptor;
+    llvm::raw_svector_ostream DOS(Descriptor);
----------------
nit: delete the blank line.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3464-3478
+    uint32_t ID;
+    const char *Name;
+  } Notes[] = {
+    {ELF::NT_LLVM_LINKER_OPTIONS, "NT_LLVM_LINKER_OPTIONS (linker options)"},
+  };
+
+  for (const auto &Note : Notes)
----------------
Since this can be written in three lines like this, this code seems a bit too over-designed to me.

  if (NT == ELF::NT_LLVM_LINKER_OPTIONS)
    return "NT_LLVM_LINKER_OPTIONS (linker options)";
  return ("unknown note type (0x" + Twine::utohexstr(NT) + ")").str();



================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3558-3560
+  default: return;
+  case ELF::NT_LLVM_LINKER_OPTIONS:
+    OS << "    Linker Options:\n"
----------------
Using a switch for one `case` and one `default` seems odd. Regular `if` statement would be better.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3563
+       << "      Options: \n";
+    const char *option = reinterpret_cast<const char *>(&Words[1]);
+    while (*option) {
----------------
Remove the trailing space in the output.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3565
+    while (*option) {
+      StringRef OR(option);
+      OS << "        " << OR << '\n';
----------------
option -> Option


Repository:
  rL LLVM

https://reviews.llvm.org/D40849