[PATCH] [Layout] Assign ordinals in resolution order

Rui Ueyama ruiu at google.com
Tue Oct 8 22:09:52 PDT 2013


  Overall this looks pretty good! :) After submitting this patch, the file ordinal will be assigned as the same order as LinkingContext::nextFile() returns file objects. I believe currently it's the same order as the command line file order in all flavors, but it's not really enforced. We probably should add a comment to nextFile() about the fact.


================
Comment at: include/lld/Core/File.h:80
@@ -82,1 +79,3 @@
+  /// Sets the command line order of the file.
+  virtual void setOrdinal(uint64_t ordinal) const { _ordinal = ordinal; }
 
----------------
Why virtual?

================
Comment at: lib/Core/Resolver.cpp:306
@@ +305,3 @@
+    if (nextFile->kind() == File::kindArchiveLibrary) {
+      if (nextFile->getOrdinal() != (uint64_t)(-1))
+        nextFile->setOrdinal(_context.getNextOrdinalAndIncrement());
----------------
I'd define File::hasOrdinal() and move this comparison into that function.

================
Comment at: lib/Core/Resolver.cpp:302
@@ -299,1 +301,3 @@
+    if (nextFile->kind() == File::kindObject) {
+      nextFile->setOrdinal(_context.getNextOrdinalAndIncrement());
       handleFile(*nextFile);
----------------
Don't you need to check if an ordinal has already been assigned?


http://llvm-reviews.chandlerc.com/D1865



More information about the llvm-commits mailing list