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

David Blaikie dblaikie at gmail.com
Fri Feb 6 10:10:33 PST 2015

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(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).



More information about the llvm-commits mailing list