[PATCH] D29892: ar: add llvm-dlltool support

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lib/Object/ArchiveWriter.cpp:380
 std::pair<StringRef, std::error_code>
-llvm::writeArchive(StringRef ArcName,
-                   std::vector<NewArchiveMember> &NewMembers,
+llvm::writeArchive(StringRef ArcName, std::vector<NewArchiveMember> &NewMembers,
                    bool WriteSymtab, object::Archive::Kind Kind,
----------------
martell wrote:
> clang-format picked this up previously.
> I just never added it to the diff.
> Adding it here to show that I am actually using clang-format.
Well, what you want to use is actually clang-format-diff, not clang-format, as I said before. clang-format-diff formats only the part you modified while clang-format formats entire file. Pleaser revert this part of change so that you don't change unrelated code.


================
Comment at: lib/Object/COFFImportFile.cpp:484-486
+  static std::vector<uint8_t> Buffer;
+  static const uint32_t NumberOfSections = 1;
+  static const uint32_t NumberOfSymbols = 5;
----------------
martell wrote:
> ruiu wrote:
> > Why static?
> The reason for doing this was to not have to initialise the variables on every call to the function.
> 
> createNullImportDescriptor has
> `static const uint32_t NumberOfSections = 1;`
> `static const uint32_t NumberOfSymbols = 1;`
> 
> createImportDescriptor has
> `static const uint32_t NumberOfSections = 2;`
> `static const uint32_t NumberOfSymbols = 7;`
> `static const uint32_t NumberOfRelocations = 3;`
> 
> createNullThunk has
> `static const uint32_t NumberOfSections = 2;`
> `static const uint32_t NumberOfSymbols = 1;`
> 
> We do not need the variables for the lifetime of the program but if I am to remove `static` here it should also be done for the other functions.
> 
> Which option will I go with?
Compilers can optimize them out, so remove `static`.


================
Comment at: lib/Object/COFFImportFile.cpp:549-550
+        sizeof(uint32_t) + Sym.size() + 1 + 6;
+    writeStringTable(Buffer, {std::string("__imp_").append(Sym),
+                              std::string("__imp_").append(Weak)});
+  } else {
----------------
Are these the same as

  "__imp_" + Sym

and

  "__imp_" + Weak

? If so, write that way.


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:144-146
+  // Do this after the parser because parseCOFFModuleDefinition sets OutputFile.
+  if (auto *Arg = Args.getLastArg(OPT_D))
+    Def->OutputFile = Arg->getValue();
----------------
Is `OutputFile` guaranteed to have some non-empty value at this point?


Repository:
  rL LLVM

https://reviews.llvm.org/D29892