[PATCH] D33180: Add functionality to cvtres to parse all entries in res file.

Eric Beckmann via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri May 19 13:43:33 PDT 2017


ecbeckmann added inline comments.


================
Comment at: llvm/lib/Object/WindowsResource.cpp:47-48
+        object_error::invalid_file_type);
+  std::unique_ptr<WindowsResource> Ret(new WindowsResource(Source));
+  return std::move(Ret);
+}
----------------
ruiu wrote:
> `return make_unique<WindowsResource>(Source)`?
Unfortunately, since we made the constructor private it cannot be accessed by make_unique


================
Comment at: llvm/lib/Object/WindowsResource.cpp:64-71
+  uint32_t Offset = DataSize + HeaderSize;
+
+  // Correct for the fact we already read 2 integers.
+  Offset -= 2 * sizeof(uint32_t);
+
+  // Advance and align.
+  RETURN_IF_ERROR(Reader.skip(Offset));
----------------
zturner wrote:
> zturner wrote:
> > Maybe I'm missing something, but I don't think any of these lines are needed?  You're computing `Offset` here by adding together the sizes of the two fields you just wrote.  Then subtracting the size of the two fields you just wrote, at which point `Offset` is going to be 0.  So you're skipping zero bytes, then padding to 4 byte alignment, but since you just read 2 uint32s, it should already be 4 byte aligned.
> > 
> > I think you can just delete all of this?
> Ahh I think I follow now.  But why are you just reading the data and header size, but not the *actual* data or headers?
This patch was just to add functionality for iterating through the entries without reading them just yet.  It will be easy in future patches to add functions to ResourceEntryRef to return specific fields from each entry.


================
Comment at: llvm/tools/llvm-cvtres/ResCOFFWriter.h:1-18
+//===- ResCOFFWriter.h ---------------------------------------- *- C++ --*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
----------------
zturner wrote:
> Do we need a header file just for this?  Maybe just put it in `llvm-cvtres.h`?
When I eventually get to generating the COFF output file, that code will live in this file.  This module is what needs to know what machine type it is generating for, so I thought I should place the enum here.  But for now, I guess we can remove this file.


================
Comment at: llvm/tools/llvm-cvtres/llvm-cvtres.cpp:174-183
+  switch (Machine) {
+    case machine::ARM:
+      outs() << "ARM";
+      break;
+    case machine::X86:
+      outs() << "X86";
+      break;
----------------
zturner wrote:
> `outs() << StringSwitch<machine>(Machine).(machine::ARM, "ARM")...`
Unfortunately StringSwitch only goes from StringRef -> value, not the other way around.  There's no constructor that takes a value parameter.


================
Comment at: llvm/tools/llvm-cvtres/llvm-cvtres.h:15
+
+void error(std::error_code EC);
+
----------------
zturner wrote:
> Can you put this in the same file as the other enum?  That said, it looks like maybe it is only used from 1 file?  In that case make it static inline in the file where you need it.
In future patches this function will surely be used in multiple files.


https://reviews.llvm.org/D33180





More information about the llvm-commits mailing list