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

[Martell Malone via Phabricator via llvm-commits]

martell marked 5 inline comments as done.
martell added a comment.

Will follow with a diff update shortly.
Here is an example failure with the new diff using `std::string` without manual memory management.

  in/FileCheck -check-prefix=CHECK7 /llvm/tools/lld/test/COFF/export32.test
  --
  Exit Code: 1
  Command Output (stderr):
  --
  /llvm/tools/lld/test/COFF/export32.test:65:16: error: expected string not found in input
  # CHECK5-NEXT: 2 0x1010 fn2
                 ^
  <stdin>:10:2: note: scanning from here
   2 0x1010 t.t
   ^
  --
  ********************
  Testing Time: 16.79s
  ********************
  Failing Tests (2):
      lld :: COFF/export.test
      lld :: COFF/export32.test
    Expected Passes    : 982
    Unsupported Tests  : 139
    Unexpected Failures: 2



================
Comment at: include/llvm/Object/DEFParser.h:49
+parseModuleDefs(MemoryBufferRef MB,
+                COFFModuleDefinition *CMD = nullptr);
+
----------------
ruiu wrote:
> 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.
It makes sense to pass in an existing object because the function doesn't necessarily update the defaults.
This way we can set the defaults from what it in `Config.h` from LLD before copying the full contents back in. I can do a conditional `if == -1` instead in `Driver.cpp` if that suits better?


================
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();
----------------
ruiu wrote:
> `std::vector<uint8_t>::size_type` is, simply put, `size_t`, no?
This is from the original file in LLD.
I will change it to `size_t`


================
Comment at: lib/Object/DEFParser.cpp:218
+  Error parseExport() {
+    COFFShortExport *E = new COFFShortExport();
+    E->Name = Tok.Value;
----------------
ruiu wrote:
> 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.
I will update it back to the `std::string` without the raw pointer and give an example of how it is failing the test suite. 


Repository:
  rL LLVM

https://reviews.llvm.org/D32689