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

[David Blaikie]

================
Comment at: include/llvm/DebugInfo/PDB/IPDBDataStream.h:25
@@ +24,3 @@
+
+  virtual uint32 getRecordCount() = 0;
+  virtual std::string getName() = 0;
----------------
zturner wrote:
> dblaikie wrote:
> > This is the total number of things this stream will visit? (or the number remaining? or the current index?)
> The former.  It's the total number of times it's valid to call getNext() if you're at the beginning of the stream (when it's newly constructed, or you've just called reset).
Not sure if there might be a better name for this. (size()?)

================
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;
----------------
zturner wrote:
> dblaikie wrote:
> > What's this the name of?
> The name of the data stream (which in turn identifies what kind of data it has).  The stream named "FPO" for example, contains fpo data.
Is that useful to clients? Would there be a better API like just having different types (templated, perhaps) of streams?

================
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;
----------------
zturner wrote:
> dblaikie wrote:
> > I take it this returns the current item but doesn't move the cursor? Maybe it should be const?
> In theory it probably could be, but the underlying DIA interface doesn't have any const methods.  I could probably use const on almost every single method of IPDBSymbol if we're ok const_casting.  I didn't make anything const though since the underlying DIA interface has no const methods.
> 
> Since you poitned out this method though, I also noticed that I left off the index.
Chances are you won't need to const_cast much since you'll have indirection to the underlying API anyway, I assume (the concrete implementation of this interface will have a member pointer to some DIA structure, right? And you call functions on the thing pointed to - since const isn't "deep" that'll just work without const_cast)

(open question about the threading guarantees of the underlying DIA APIs, though - if those operations that are conceptually const but not marked const in the DIA API are not thread safe, that might surprise users of this API to call a non-thread-safe const function, but if that's the case, a comment that this whole API is thread incompatible would be fine I think)

================
Comment at: include/llvm/DebugInfo/PDB/PDBTypes.h:27
@@ +26,3 @@
+
+typedef std::unique_ptr<IPDBDataStream> PDBDataStreamPtr;
+typedef std::unique_ptr<IPDBSession> PDBSessionPtr;
----------------
zturner wrote:
> dblaikie wrote:
> > 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... 
> Does llvm tend to write out unique_ptr<T> everywhere?  It leads to a lot of line wrapping.
I think so, at least in my experience - but I don't claim to have lots of data.

(I tend to be much more conservative about using typedefs (rarely use them) than most people, so feel free to treat this feedback with a grain of salt)

http://reviews.llvm.org/D7356

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