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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: include/llvm/Object/COFFImportFile.h:96
+std::error_code writeImportLibrary(StringRef DLLName,
+                                   std::string &Path,
+                                   std::vector<COFFShortExport> *Exports,
----------------
Use `StringRef` instead of `std::string &`


================
Comment at: include/llvm/Object/COFFImportFile.h:97
+                                   std::string &Path,
+                                   std::vector<COFFShortExport> *Exports,
+                                   COFF::MachineTypes Machine);
----------------
I think you are not mutating `Exports`. Can you change this to ArrayRef<COFFShortExport>?


================
Comment at: lib/Object/COFFImportFile.cpp:32
+namespace llvm {
+namespace object{
+
----------------
Please run clang-format.


================
Comment at: lib/Object/COFFImportFile.cpp:60
+static inline Error createError(const Twine &Err) {
+  return make_error<StringError>(Err.getSingleStringRef(), object_error::parse_failed);
+}
----------------
80 cols?


================
Comment at: lib/Object/COFFImportFile.cpp:95
+
+static ImportNameType getNameType(StringRef Sym, StringRef ExtName, MachineTypes Machine) {
+  if (Sym != ExtName)
----------------
80 cols. (Run clang-format.)


================
Comment at: lib/Object/COFFModuleDefinition.cpp:1
+//===--- COFFModuleDefinition.cpp - Simple DEF parser ------------------------------===//
+//
----------------
Format.


================
Comment at: lib/Object/COFFModuleDefinition.cpp:63
+static inline Error createError(const Twine &Err) {
+  return make_error<StringError>(Err.getSingleStringRef(), object_error::parse_failed);
+}
----------------
Format.


================
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.
----------------
Remove empty line


================
Comment at: tools/lld/COFF/Driver.cpp:454
+    // Set the output file, but don't override /out if it was already passed.
+    if(!Config->OutputFile.empty()) Config->OutputFile = ImportCOFFMod.OutputFile;
+
----------------
Please run clang-format.


================
Comment at: tools/lld/COFF/Driver.cpp:482
+static void createImportLibrary() {
+    std::vector<COFFShortExport> Exports = createCOFFShortExportFromConfig();
+    std::string DLLName = sys::path::filename(Config->OutputFile);
----------------
Format.


================
Comment at: tools/lld/COFF/Driver.cpp:489
+static Expected<COFFModuleDefinition> mutateModuleDefs(MemoryBufferRef MB) {
+    Expected<COFFModuleDefinition> ImportCOFFMod =
+      parseCOFFModuleDefinition(MB, Config->Machine);
----------------
Format.


Repository:
  rL LLVM

https://reviews.llvm.org/D32689