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

[David Blaikie]

Few random comments provided.


================
Comment at: include/llvm/DebugInfo/PDB/IPDBDataStream.h:23
@@ +22,3 @@
+
+  virtual ~IPDBDataStream() {}
+
----------------
In theory the LLVM coding convention requires that types with vtables have an anchor function (one non-inline virtual function).

I'm not sure if the type /only/ has pure virtual functions whether that matters (but it probably does - even a vtable of only pure virtual functions still has to exist, I think)

================
Comment at: include/llvm/DebugInfo/PDB/IPDBDataStream.h:25
@@ +24,3 @@
+
+  virtual uint32 getRecordCount() = 0;
+  virtual std::string getName() = 0;
----------------
This is the total number of things this stream will visit? (or the number remaining? or the current index?)

================
Comment at: include/llvm/DebugInfo/PDB/IPDBDataStream.h:26
@@ +25,3 @@
+  virtual uint32 getRecordCount() = 0;
+  virtual std::string getName() = 0;
+  virtual bool getItemAtIndex(RecordType &Record) = 0;
----------------
What's this the name of?

================
Comment at: include/llvm/DebugInfo/PDB/IPDBDataStream.h:27
@@ +26,3 @@
+  virtual std::string getName() = 0;
+  virtual bool getItemAtIndex(RecordType &Record) = 0;
+  virtual bool getNext(RecordType &Record) = 0;
----------------
I take it this returns the current item but doesn't move the cursor? Maybe it should be const?

================
Comment at: include/llvm/DebugInfo/PDB/IPDBDataStream.h:28
@@ +27,3 @@
+  virtual bool getItemAtIndex(RecordType &Record) = 0;
+  virtual bool getNext(RecordType &Record) = 0;
+  virtual void reset() = 0;
----------------
return Optional<RecordType> rather than an out parameter?

(I'd probably call this "next" but that might be a bit vague (& collides with std::next))

================
Comment at: include/llvm/DebugInfo/PDB/IPDBSourceFile.h:32
@@ +31,1 @@
+#endif
\ No newline at end of file

----------------
missing newline at end of file

================
Comment at: include/llvm/DebugInfo/PDB/IPDBSymbolBase.h:25
@@ +24,3 @@
+
+  virtual IPDBSymbolEnumerator *findChildren(PDB_SymType Type, StringRef Name,
+                                             PDB_NameSearchFlags Flags) = 0;
----------------
Wouldn't this return value be passing ownership (via unique_ptr or similar)? and similarly on the next two functions.

================
Comment at: include/llvm/DebugInfo/PDB/PDBTypes.h:27
@@ +26,3 @@
+
+typedef std::unique_ptr<IPDBDataStream> PDBDataStreamPtr;
+typedef std::unique_ptr<IPDBSession> PDBSessionPtr;
----------------
Not sure if these typedefs help readability - it feels like they hinder it by hiding a common construct (unique_ptr) in something opaque when reading the code. But I tend to be a bit verbose...

http://reviews.llvm.org/D7356

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/