labath added a comment.
Thanks. I have a couple of small comments, but I think this is basically done.
================
Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:43
+namespace {
+using namespace llvm;
----------------
llvm style is to only use the anonymous namespaces for class declarations (and
use the `static` keyword for functions). What you've done here is particularly
confusing, as you've combined this with `using namespace llvm`, which gives off
the impression that the `using` declaration is somehow local to the anonymous
namespace (which it isn't).
In this case, I'd probably just get rid of the anonymous namespace and move
everything (the struct definition and using declarations, if you really want
it) into the now-static GetCoffUUID function.
================
Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:45
+using namespace llvm;
+typedef struct CVInfoPdb70 {
+ // 16-byte GUID
----------------
`typedef struct` is very C-like. Just use plain `struct`.
================
Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:117
- if (ObjectFilePECOFF::MagicBytesMatch(data_sp)) {
- DataExtractor data;
----------------
You should keep the MagicBytesMatch call (if you want to llvm-ize it, you could
replace that with a call to `llvm::identify_magic`).
All of the GetModuleSpecifications will be called each time lldb does anything
with an object file (any object file), and the idea is to avoid reading the
whole file until we are reasonably certain that it is the file we are looking
for. That's the reason this function gets the initial bytes of the file in the
data_sp member. This way, all of the object file plugins can quickly inspect
that data to see if they care about the file, and only one of them will attempt
an actual parse.
================
Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:891-892
+
+ if (!CreateBinary())
+ return UUID();
+ auto COFFObj =
----------------
I don't think this is necessary as `CreateInstance` will refuse to return the
ObjectFile instance if the creation of the coff binary object failed. (You
could theoretically assert that the binary is really there if you want extra
security).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56229/new/
https://reviews.llvm.org/D56229
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
