[PATCH] Initial pass at API design for DebugInfo/PDB

David Blaikie dblaikie at gmail.com
Tue Feb 3 15:05:09 PST 2015

Comment at: include/llvm/DebugInfo/PDB/IPDBEnumChildren.h:24
@@ +23,3 @@
+  virtual uint32_t getChildCount() const = 0;
+  virtual ChildTypePtr getChildAtIndex(uint32_t Index) const = 0;
Name's OK, but "count" still seems like it might be a bit ambiguous with index, or number remaining. Another option (which seems more idiomatic in LLVM) might be "getNumChildren" (while I would like "size()" it's annoying that the count is attached to the iterator and not to some separate structure that produces iterators and I don't know how to properly communicate the separation of these concepts, unfortunately)

Comment at: include/llvm/DebugInfo/PDB/IPDBRawSymbol.h:193
@@ +192,3 @@
+  virtual bool isVirtualInheritance() const = 0;
+  virtual bool isVolatileType() const = 0;
How're these functions going to communicate failure?

Comment at: include/llvm/DebugInfo/PDB/PDBSymbol.h:33
@@ +32,3 @@
+  IPDBRawSymbol *getRawSymbol();
+  const IPDBRawSymbol *getRawSymbol() const;
return by reference rather than pointer here, perhaps?

Comment at: include/llvm/DebugInfo/PDB/PDBSymbol.h:46
@@ +45,3 @@
+  std::unique_ptr<IPDBRawSymbol> RawSymbol;
could probably make this private and the derived classes can just call "getRawSymbol" - the derived classes don't need the ability to mutate (null, repoint, etc) this variable. (optional, not a big deal either way, just a little more stringent encapsulation)

The member could actually be const, too/instead.

Comment at: lib/DebugInfo/PDB/PDB.cpp:20
@@ +19,3 @@
+  // Create the correct concrete instance type based on the value of Type.
+  return std::unique_ptr<IPDBSession>();
"return nullptr" will work here



More information about the llvm-commits mailing list