[Lldb-commits] [PATCH] D56229: [PECOFF] Implementation of ObjectFilePECOFF:: GetUUID()

[Pavel Labath via Phabricator via lldb-commits]

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