[PATCH] [LLD] Add a new PE/COFF port

Rui Ueyama ruiu at google.com
Thu May 28 11:56:30 PDT 2015


Davide,

Thank you for reviewing!


================
Comment at: COFF/Chunks.cpp:46
@@ +45,3 @@
+bool SectionChunk::isRoot() {
+  if (isCOMDAT())
+    return false;
----------------
davide wrote:
> Can you add a small comment here explaining why COMDATs are GC roots?
Done.

================
Comment at: COFF/Chunks.cpp:72
@@ +71,3 @@
+  // Mark associative sections if any.
+  for (Chunk *C : AssocChildren)
+    C->markLive();
----------------
davide wrote:
> using 'auto' here? Or do you think it makes the code less readable?
I think explicit type is more readable this case, although we are using C as a variable for Chunk in almost all cases.

================
Comment at: COFF/Chunks.cpp:131
@@ +130,3 @@
+  uint32_t E = File->getCOFFObj()->getNumberOfSymbols();
+  for (uint32_t I = 0; I < E; ++I) {
+    auto SrefOrErr = File->getCOFFObj()->getSymbol(I);
----------------
davide wrote:
> In other places you assign E in the for() initialization, any reason you don't do that here as well? Maybe there are few other places where you do this that I missed. 
I think I wrote E before for if E's initializer is too long (I just don't want to line-warp for conditionals).

================
Comment at: COFF/InputFiles.cpp:111
@@ +110,3 @@
+  } else {
+    return make_dynamic_error_code(Twine(Name) + " is not a COFF file.");
+  }
----------------
davide wrote:
> No braces needed, I guess.
I omit {} only when it doesn't have else consistently, so keeping it is probably better.

================
Comment at: COFF/Options.td:21
@@ +20,3 @@
+def defaultlib : P<"defaultlib", "Add the library to the list of input files">;
+def nodefaultlib : P<"nodefaultlib", "Remove a default library">;
+def disallowlib : Joined<["/", "-", "-?"], "disallowlib:">, Alias<nodefaultlib>;
----------------
davide wrote:
> Can you sort these (alphabetically?)
Done.

================
Comment at: COFF/Writer.cpp:30
@@ +29,3 @@
+static const int PageSize = 4096;
+static const int FileAlignment = 512;
+static const int SectionAlignment = 4096;
----------------
davide wrote:
> I personally prefer to express power-of-two constant(s) as 1 << SOMETHING; but it's not a strong preference.
Interesting. I always think in the other way just like this, so page size is 4096 to me than 1<<12 and disk sector is 512 than 1<<9.

http://reviews.llvm.org/D10036

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list