[PATCH] D32689: DEF: migrate def parser from LLD to LLVM

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lib/Object/COFFImportFile.cpp:60
+
+static inline Error createError(const Twine &Err) {
+  return make_error<StringError>(Err.getSingleStringRef(),
----------------
Remove this function and inline it (you are using this function only once in this file.)


================
Comment at: lib/Object/COFFModuleDefinition.cpp:62
+
+static inline Error createError(const Twine &Err) {
+  return make_error<StringError>(Err.getSingleStringRef(),
----------------
Remove `inline` as it seems premature optimization.


================
Comment at: tools/lld/COFF/Driver.cpp:1002
+    // mutateModuleDefs mutates Config object.
+    auto V = mutateModuleDefs(
         takeBuffer(check(MemoryBuffer::getFile(Arg->getValue()),
----------------
Use its actual type instead of `auto`.


================
Comment at: tools/lld/COFF/Driver.cpp:1002
+    // mutateModuleDefs mutates Config object.
+    auto V = mutateModuleDefs(
         takeBuffer(check(MemoryBuffer::getFile(Arg->getValue()),
----------------
Ping?


================
Comment at: tools/lld/COFF/Driver.cpp:452
+static COFFModuleDefinition
+importCOFFModToConfig(COFFModuleDefinition &ImportCOFFMod) {
+  // Set the output file, but don't override /out if it was already passed.
----------------
importCOFFModToConfig doesn't sound like a good name, but you don't really need this function, I think. This function is called only once at a tail-call location, so separating this as a different function doesn't reduce complexity. Please just remove this and inline it.


================
Comment at: tools/lld/COFF/Driver.cpp:499
+static Expected<COFFModuleDefinition> mutateModuleDefs(MemoryBufferRef MB) {
+  Expected<COFFModuleDefinition> ImportCOFFMod =
+      parseCOFFModuleDefinition(MB, Config->Machine);
----------------
Please use shorter name. ImportCOFFMod -> Def


================
Comment at: tools/lld/COFF/Driver.cpp:503
+    return ImportCOFFMod.takeError();
+  return importCOFFModToConfig(*ImportCOFFMod);
+}
----------------
Use a short name -- something like

  COFFModuleDefinition &M = *ImportCOFFMod;


Repository:
  rL LLVM

https://reviews.llvm.org/D32689