[PATCH] D38513: [LLD] [COFF] Add support for GNU binutils import libraries

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Yeah, I agree with you that this patch is lean. It actually looks pretty good.



================
Comment at: COFF/Writer.cpp:378
+    std::map<std::pair<StringRef, uint32_t>, std::vector<Chunk *>> &Map) {
+  const uint32_t RDATA = IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ;
+
----------------
nit: we omit `const` if it is obviously a const.


================
Comment at: COFF/Writer.cpp:383
+  for (auto &Pair : Map) {
+    if (!Pair.first.first.startswith(".idata"))
+      continue;
----------------
Can you assign `Pair.first.first` and `Pair.first.second` to local variables for the sake of documentation?


================
Comment at: COFF/Writer.cpp:397
+  for (auto &Pair : Map) {
+    if (!Pair.first.first.startswith(".idata"))
+      continue;
----------------
Ditto


================
Comment at: COFF/Writer.cpp:413
+      // of the ObjFile.
+      return toString(SC1->File) < toString(SC2->File);
+    });
----------------
`toString` is written for constructing a debug or log message, so it is better to avoid using it as part of the actual logic.


================
Comment at: COFF/Writer.cpp:478
+    // See Microsoft PE/COFF spec 5.4 for details.
+    auto AddChunks = [&](StringRef N, std::vector<Chunk *> &V) {
+      std::vector<Chunk *> &DestVect = Map[{N, DATA | R}];
----------------
nit: AddChunk -> Add since the scope of this variable is very narrow.


================
Comment at: COFF/Writer.cpp:636
+    if (!isa<DefinedImportThunk>(File->ThunkSym))
+      fatal(toString(*File->ThunkSym) + " was replaced");
+    DefinedImportThunk *Thunk = cast<DefinedImportThunk>(File->ThunkSym);
----------------
I wonder if you should use `assert`. If this line is unreachable unless there's a bug in lld, you should use `assert`.


https://reviews.llvm.org/D38513