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

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:1-2
+//===-- WindowsResource.h -----------------------------------------------*- C++
+//-*-===//
+//
----------------
Can you fix this so that it's exactly 80 characters so it doesn't wrap?


================
Comment at: llvm/lib/Object/WindowsResource.cpp:33-36
+  ArrayRef<uint8_t> Ref(
+      reinterpret_cast<const uint8_t*>(Data.getBufferStart() + LeadingSize),
+      Data.getBufferSize() - LeadingSize);
+  BBS = BinaryByteStream(Ref, support::little);
----------------
`BBS = BinaryByteStream(Data.getBuffer(), support::little).drop_front(LeadingSize);`


================
Comment at: llvm/lib/Object/WindowsResource.cpp:47
+        object_error::invalid_file_type);
+  std::unique_ptr<WindowsResource> Ret(new WindowsResource(Source));
+  return std::move(Ret);
----------------
`return llvm::make_unique<WindowsResource>(Source);`


================
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));
----------------
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?


================
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.
 //
----------------
Do we need a header file just for this?  Maybe just put it in `llvm-cvtres.h`?


================
Comment at: llvm/tools/llvm-cvtres/llvm-cvtres.cpp:15
 
+#include <locale>
+
----------------
Remove this include.  In the future though, stadard includes go after llvm includes.


================
Comment at: llvm/tools/llvm-cvtres/llvm-cvtres.cpp:121
+  if (InputArgs.hasArg(OPT_MACHINE)) {
+    std::locale Loc;
+    std::string MachineString = InputArgs.getLastArgValue(OPT_MACHINE).str();
----------------
Remove


================
Comment at: llvm/tools/llvm-cvtres/llvm-cvtres.cpp:122-126
+    std::string MachineString = InputArgs.getLastArgValue(OPT_MACHINE).str();
+    for (unsigned int i = 0; i < MachineString.length(); i++) {
+      MachineString[i] = std::toupper(MachineString[i], Loc);
+    }
+    Machine = StringSwitch<llvm::machine>(MachineString)
----------------
`std::string MachineString = InputArgs.getLastArgValue(OPT_MACHINE).upper();`


================
Comment at: llvm/tools/llvm-cvtres/llvm-cvtres.cpp:153
+
+  for (auto File : InputFiles) {
+    Expected<object::OwningBinary<object::Binary>> BinaryOrErr =
----------------
`const auto &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;
----------------
`outs() << StringSwitch<machine>(Machine).(machine::ARM, "ARM")...`


================
Comment at: llvm/tools/llvm-cvtres/llvm-cvtres.h:15
+
+void error(std::error_code EC);
+
----------------
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.


https://reviews.llvm.org/D33180