[PATCH] D30956: Introduce NativeEnumModules and NativeCompilandSymbol

[Bob Haarman via Phabricator via llvm-commits]

inglorion added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/PDB/Native/NativeCompilandSymbol.h:2
+//===- NativeCompilandSymbol.h - Native impl of PDBSymbolCompiland - C++
+//-*-===//
+//
----------------
This comment seems broken. It's spread over multiple lines and the opening "-*-" is missing. Is the problem here that this really didn't fit on one line? If so, since Emacs likes its magic markers on the first line, perhaps we can put the description on the second line?


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeCompilandSymbol.cpp:1
+//===- NativeCompilandSymbol.h - Native impl of PDBCompilandSymbol -C++ -*-===//
+//
----------------
s/\.h/.cpp/


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeCompilandSymbol.cpp:27
+
+// DIA, which this API was modeled after, uses counter-intuitive meanings for
+// IDiaSymbol::get_name and IDiaSymbol::get_libraryName, which is why these
----------------
Instead of "counter-intuitive", can you specify what they actually mean? That would make it easier to verify that these methods are indeed doing the right thing.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumModules.cpp:32
+NativeEnumModules::getChildAtIndex(uint32_t Index) const {
+  if (Index >= Modules.size())
+    return nullptr;
----------------
You're choosing to return nullptr here, instead of something else, e.g. crashing or asserting. I'm curious why you prefer to return nullptr. Could you explain your rationale?


https://reviews.llvm.org/D30956