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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Memory management here is not that complicated. If you need a string that owns string contents, use std::string. If you need a string-ish object that doesn't own string contents, use StringRef. You can make this code more efficient by using std::move, but that's not mandatory, so you probably don't want to do that now.



================
Comment at: include/llvm/Object/DEFParser.h:1
+//===--- DEFParser.h - Simple YAML parser --------------------------------===//
+//
----------------
You want to rename this to match with the class name, i.e. COFFModuleDefinition.h.


================
Comment at: include/llvm/Object/DEFParser.h:10
+//
+// This is a DEF parser.
+//
----------------
`DEF parser` is not a common term. Please remove.


================
Comment at: include/llvm/Object/DEFParser.h:32
+struct COFFModuleDefinition {
+  std::vector<COFFShortExport*> Exports;
+  COFF::MachineTypes Machine = COFF::IMAGE_FILE_MACHINE_UNKNOWN;
----------------
Change this type to `std::vector<COFFShortExport>` to avoid manual memory management.


================
Comment at: include/llvm/Object/DEFParser.h:48
+Expected<COFFModuleDefinition>
+parseModuleDefs(MemoryBufferRef MB,
+                COFFModuleDefinition *CMD = nullptr);
----------------
Rename parseCOFFModuleDefinition.


================
Comment at: include/llvm/Object/DEFParser.h:49
+parseModuleDefs(MemoryBufferRef MB,
+                COFFModuleDefinition *CMD = nullptr);
+
----------------
Why do you have to pass an object? I think this function is to read a module definition from MB, so it doesn't seem to make sense to pass in an existing object.


================
Comment at: lib/Object/COFFImportFile.cpp:10
+//
+// This file declares the writeImportLibrary function.
+//
----------------
declares -> defines


================
Comment at: lib/Object/COFFImportFile.cpp:77
+
+  std::vector<uint8_t>::size_type Pos = B.size();
+  std::vector<uint8_t>::size_type Offset = B.size();
----------------
`std::vector<uint8_t>::size_type` is, simply put, `size_t`, no?


================
Comment at: lib/Object/DEFParser.cpp:10
+//
+// This is a DEF parser.
+//
----------------
Ditto


================
Comment at: lib/Object/DEFParser.cpp:218
+  Error parseExport() {
+    COFFShortExport *E = new COFFShortExport();
+    E->Name = Tok.Value;
----------------
In general, you want to avoid using the raw `new`. If you do that, you'll need to manage that memory yourself, which is error-prone.


================
Comment at: tools/lld/COFF/Driver.cpp:437
+  for (Export &E : Config->Exports) {
+    COFFShortExport shortExport;
+    shortExport.Name = E.Name;
----------------
shortExport -> ShotExport

But probably just E1 and E2 suffice.


================
Comment at: tools/lld/COFF/Driver.cpp:452
+static COFFModuleDefinition setupCOFFModFromConfig() {
+    COFFModuleDefinition outputCOFFMod;
+    outputCOFFMod.OutputFile = Config->OutputFile;
----------------
All variable names should start with uppercase letter.


================
Comment at: tools/lld/COFF/Driver.cpp:467-472
+static COFFModuleDefinition importCOFFModToConfig(COFFModuleDefinition &importCOFFMod) {
+
+    Config->OutputFile = importCOFFMod.OutputFile;
+    Config->Machine = importCOFFMod.Machine;
+    Config->ImageBase = importCOFFMod.ImageBase;
+    Config->StackReserve = importCOFFMod.StackReserve;
----------------
Please format everything in the LLVM coding style and look the same as other code. I'll review new code after you reformat.


================
Comment at: tools/lld/COFF/Driver.cpp:510-514
+static void cleanupModuleDefs(COFFModuleDefinition defHandle) {
+  for (COFFShortExport *shortExport : defHandle.Exports) {
+      delete shortExport;
+  }
+}
----------------
I'd really want to avoid this kind of non-automatic memory management. Please use std::unique_ptr if you need memory ownership management.


================
Comment at: tools/lld/COFF/Driver.cpp:1016
                          Twine("could not open ") + Arg->getValue())));
+    if(!V) {
+      fatal(errorToErrorCode(V.takeError()).message());
----------------
Omit {}


Repository:
  rL LLVM

https://reviews.llvm.org/D32689