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

Saleem Abdulrasool via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Dec 18 15:18:41 PST 2017


compnerd added inline comments.


================
Comment at: lib/CodeGen/TargetLoweringObjectFileImpl.cpp:97
+
+    llvm::SmallString<128> Descriptor;
+    llvm::raw_svector_ostream DOS(Descriptor);
----------------
ruiu wrote:
> nit: delete the blank line.
UGH, I swear I had deleted this.  I hate this blank line, it keeps coming back :-(.


================
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)
----------------
ruiu wrote:
> 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();
> 
It wouldn't format it the same IIRC.  This is also something which is effectively the same pattern as the rest of the file.  If it does, then doing that in a follow up seems better.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3558-3560
+  default: return;
+  case ELF::NT_LLVM_LINKER_OPTIONS:
+    OS << "    Linker Options:\n"
----------------
ruiu wrote:
> Using a switch for one `case` and one `default` seems odd. Regular `if` statement would be better.
It allows for adding new items in the future.  It also is maintaining symmetry with the rest of the print[Owner]Note functions.


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


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


Repository:
  rL LLVM

https://reviews.llvm.org/D40849





More information about the llvm-commits mailing list