[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