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

[David Blaikie]

Few optional bits of feedback but otherwise seems fine.

If you want some more help/details on the .def stuff, let me know - happy to go through it.


================
Comment at: lib/DebugInfo/PDB/PDBInterfaceAnchors.cpp:64
@@ +63,3 @@
+
+// All of the concrete symbol types have their methods declared inline through
+// the use of a forwarding macro, so the constructor should be declared out of
----------------
This still means the vtable is emitted weakly, though it will only be emitted in one place. So this doesn't quite meet the style guide's requirement & I'd rather use the virtual dtor or an explicit anchor function, but I'm not so fussed. I do think the rule is a bit silly, but I'm not sure this particular approach is worthwhile (I suppose my thought is that the rule is silly enough that it's not worth partly/tangentially/sort-of adhering to, might as well just not bother adhering to it at all, perhaps - I dunno)

================
Comment at: lib/DebugInfo/PDB/PDBSymbol.cpp:56
@@ +55,3 @@
+
+#define FACTORY_SYMTAG_CASE(Tag, Type)                                         \
+  case PDB_SymType::Tag:                                                       \
----------------
Could possibly move the #define closer to the use (even inside the function) and maybe undef it afterwards.

================
Comment at: lib/DebugInfo/PDB/PDBSymbol.cpp:63
@@ +62,3 @@
+  switch (Symbol->getSymTag()) {
+    FACTORY_SYMTAG_CASE(Exe, PDBSymbolExe)
+    FACTORY_SYMTAG_CASE(Compiland, PDBSymbolCompiland)
----------------
Would it be useful to do this more as a .def file style - Take a look at Duncan's recent use of include/llvm/IR/Metadata.def for an example (essentially have the big long list of all the PDBSymbols written once, in one file - then #define a macro for what you want to do with those names, then include the file - thus stamping out all sorts of things you want, for all PDBSymbols)

================
Comment at: unittests/DebugInfo/PDB/PDBApiTest.cpp:53
@@ +52,3 @@
+#define MOCK_SYMBOL_ACCESSOR(Func)                                             \
+  auto Func() const->decltype(((IPDBRawSymbol *)nullptr)->Func()) override {   \
+    typedef decltype(IPDBRawSymbol::Func()) ReturnType;                        \
----------------
Probably prefer the LHS return type. The only reason to use the -> return type formulation is if you want to refer to function parameters, generally.

Also, rather than casting nullptr, the usual way to "give me a thing of some particular kind in an unevaluated context" is a utility called declval:

  decltype(declval<IPDBRawSymbol>().Func()) Func() const override {



================
Comment at: unittests/DebugInfo/PDB/PDBApiTest.cpp:62
@@ +61,3 @@
+
+  virtual void dump(llvm::raw_ostream &OS) const {}
+
----------------
Use override rather than virtual (for all of these).

http://reviews.llvm.org/D7356

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