[PATCH] D30956: Introduce NativeEnumModules and NativeCompilandSymbol

[Adrian McCarthy via Phabricator via llvm-commits]

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


================
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
----------------
inglorion wrote:
> 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.
Zach and I were having a partially offline discussion about this.  We might rename these methods in a future patch, but I'll take a crack at re-wording this comment in the mean time.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeEnumModules.cpp:32
+NativeEnumModules::getChildAtIndex(uint32_t Index) const {
+  if (Index >= Modules.size())
+    return nullptr;
----------------
inglorion wrote:
> 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?
I'm following the lead set by the DIA versions of the enumerators I've looked at.  For example, DIAEnumDebugStreams.cpp returns nullptr if anything goes wrong querying DIA for the desired item, and calling with an invalid index is one possibility.

I'd be happy to assert instead if you think it's important.


https://reviews.llvm.org/D30956