[PATCH] D40518: [CodeView] Re-write TypeSerializer and TypeTableBuilder

[Reid Kleckner via Phabricator via llvm-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: llvm/include/llvm/DebugInfo/CodeView/ContinuationRecordBuilder.h:55
+
+  template <typename RecordType> void writeMemberType(RecordType &Record);
+
----------------
zturner wrote:
> rnk wrote:
> > I see how this is supposed to work, but maybe stamping out overloads for all of the member types in the header and redirecting them to use a common template implementation in the .cpp file is a better way to do this.
> > 
> > Compilers sometimes warn when an implicit template instantiation is requested from a function template declaration with no body.
> I had it like you suggested originally, but I actually did this on purpose because it's easier to look at a header file with a single declaration than it is to look at a bunch of template goo and list of 100 method overloads (or alternatively, the macro and `#include` goo that also works).
> 
> I explicitly instantiate every template in the implementation file using the macro / `#include` trick, but at least that way it's hidden in the implementation file.  I thought explicit instantiations were correctly supported by all compilers?
It'll work, but template declarations in headers without inline definitions below are kind of a code smell. At least, I start looking around going "how can this work". At least put a comment explaining that these are instantiated for all member types in the .cpp file. If it's warning free with that, sounds good, ship it.


================
Comment at: llvm/lib/DebugInfo/CodeView/ContinuationRecordBuilder.cpp:8
+struct ContinuationRecord {
+  ulittle16_t Kind{uint16_t(TypeLeafKind::LF_INDEX)};
+  ulittle16_t Size{0};
----------------
zturner wrote:
> rnk wrote:
> > I hate unicorn initialization =P
> If you mean the `{}` syntax, it's actually necessary in this case since it requires an implicit conversion.  I couldn't get it to work with an = syntax.
Yeah, I figured. I'm just complaining.


https://reviews.llvm.org/D40518