tra added a comment.
LGTM in principle, with new nits.
================
Comment at: llvm/include/llvm/Object/OffloadBinary.h:121
const Entry *TheEntry)
- : Buffer(Buffer), TheHeader(TheHeader), TheEntry(TheEntry) {
-
+ : Binary(Binary::ID_Offload, Source), Buffer(Source.getBufferStart()),
+ TheHeader(TheHeader), TheEntry(TheEntry) {
----------------
We're losing information by passing only the start of the buffer.
I'd change the type of `Buffer` field to `MemoryBufferRef` instead.
================
Comment at: llvm/lib/Object/OffloadBinary.cpp:18
using namespace llvm;
-
-namespace llvm {
+using namespace object;
----------------
Nit: `using namespace` seems a bit too heavy-handed when you only need one enum
`using object_error = llvm::object::object_error;` would probably do.
TBH, the use of fully qualified enum in only two instances also looked OK to me.
================
Comment at: llvm/lib/Object/OffloadBinary.cpp:18
-
-namespace llvm {
----------------
tra wrote:
> Nit: `using namespace` seems a bit too heavy-handed when you only need one
> enum
> `using object_error = llvm::object::object_error;` would probably do.
> TBH, the use of fully qualified enum in only two instances also looked OK to
> me.
Nit: Offload binary is declared within `llvm` namespace. Removing namespace
here and relying on `using namespace llvm;` works, but makes things a bit less
obvious. I'd keep the namespace around. Maybe, even remove `using namespace
llvm` above, too as it should not be needed if the code is within the actual
namespace.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126812/new/
https://reviews.llvm.org/D126812
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
