[PATCH] D30956: Introduce NativeEnumModules and NativeCompilandSymbol

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth marked 4 inline comments as done.
amccarth added inline comments.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeCompilandSymbol.cpp:25
+
+uint32_t NativeCompilandSymbol::getLexicalParentId() const { return 0; }
+
----------------
zturner wrote:
> In the future, this will be the unique (within the context of this PDB session) of the Exe symbol.
Acknowledged.  I'm going to make a specialization next for the Exe, so this should become clearer soon.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumModules.cpp:34-36
+  return std::unique_ptr<PDBSymbol>(new PDBSymbolCompiland(Session,
+      std::unique_ptr<IPDBRawSymbol>(
+          new NativeCompilandSymbol(Session, Modules[Index]))));
----------------
zturner wrote:
> Not as important as it is when you have a `shared_ptr`, but I think as a matter of style we prefer `make_unique` instead of explicitly constructing a `unique_ptr` with the result of a `new` expression.
I generally do use make_unique, but I don't know of another way to have make_unique create a unique_ptr of the base class that actually points to an instance of a subclass.


================
Comment at: llvm/test/DebugInfo/PDB/Native/pdb-native-compilands.test:18
+BIGREAD:---COMPILANDS---
+BIGREAD:  D:\src\llvm\test\tools\llvm-symbolizer\pdb\Inputs\test.obj
+BIGREAD:  f:\dd\vctools\crt\vcstartup\build\md\msvcrt_kernel32\obj1r\i386\_cpu_disp_.obj
----------------
zturner wrote:
> Should this prefix be removed as well? (as per the comment above)?
I missed that one.  It probably doesn't matter, since the path is fixed for whoever built the PDB originally, but I've done it to be consistent.


https://reviews.llvm.org/D30956