[PATCH] D33180: Add functionality to cvtres to parse all entries in res file.
Zachary Turner via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri May 19 11:23:26 PDT 2017
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
More information about the llvm-commits
mailing list