[PATCH] D41102: Setup clang-doc frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added a comment.

In https://reviews.llvm.org/D41102#1034919, @lebedev.ri wrote:

> Since the commit was reverted, did you mean to either recommit it, or reopen this (with updated diff), so it does not get lost?


Relanded in r327295.



================
Comment at: clang-doc/BitcodeWriter.h:154
+  ClangDocBitcodeWriter(llvm::BitstreamWriter &Stream,
+                        bool OmitFilenames = false)
+      : Stream(Stream), OmitFilenames(OmitFilenames) {
----------------
sammccall wrote:
> Hmm, you spend a lot of effort plumbing this variable around! Why is it so important?
> Filesize? (I'm not that familiar with LLVM bitcode, but surely we'll end up with a string table anyway?)
> 
> If it really is an important option people will want, the command-line arg should probably say why.
It was for testing purposes (so that the tests aren't flaky on filenames), but I replaced it with regex.


================
Comment at: clang-doc/BitcodeWriter.h:241
+/// \param I The info to emit to bitcode.
+template <typename T> void ClangDocBitcodeWriter::emitBlock(const T &I) {
+  StreamSubBlockGuard Block(Stream, MapFromInfoToBlockId<T>::ID);
----------------
lebedev.ri wrote:
> sammccall wrote:
> > OK, I don't get this at all.
> > 
> > We have to declare emitBlockContent(NamespaceInfo) *and* the specialization of MapFromInfoToBlockId<NamespaceInfo>, and deal with the public interface emitBlock being a template function where you can't tell what's legal to pass, instead of writing:
> > 
> > ```void emitBlock(const NamespaceInfo &I) {
> >   SubStreamBlockGuard Block(Stream, BI_NAMESPACE_BLOCK_ID); // <-- this one line
> >   ...
> > }```
> > 
> > This really seems like templates for the sake of templates :(
> If you want to add a new block, in one case you just need to add one
> ```
> template <> struct MapFromInfoToBlockId<???Info> {
>   static const BlockId ID = BI_???_BLOCK_ID;
> };
> ```
> In the other case you need to add whole
> ```
> void ClangDocBitcodeWriter::emitBlock(const ???Info &I) {
>   StreamSubBlockGuard Block(Stream, BI_???_BLOCK_ID);
>   emitBlockContent(I);
> }
> ```
> (and it was even longer initially)
> It seems just templating one static variable is shorter than duplicating `emitBlock()` each time, no?
> 
> Do compare the current diff with the original diff state.
> I *think* these templates helped move much of the duplication to simplify the code overall.
You'd still have to add the appropriate `emitBlock()` function for any new block, since it would have different attributes. 


================
Comment at: clang-doc/Mapper.cpp:33
+  ECtx->reportResult(llvm::toHex(llvm::toStringRef(serialize::hashUSR(USR))),
+                     serialize::emitInfo(D, getComment(D, D->getASTContext()),
+                                         getLine(D, D->getASTContext()),
----------------
sammccall wrote:
> It seems a bit of a poor fit to use a complete bitcode file (header, version, block info) as your value format when you know the format, and know there'll be no version skew.
> Is it easy just to emit the block we care about?
Ideally, yes, but right now in the clang BitstreamWriter there's no way to tell the instance what all the abbreviations are without also emitting the blockinfo to the output stream, though I'm thinking about taking a stab at separating the two. 

Also, this relies on the llvm-bcanalyzer for testing, which requires both the header and the blockinfo in order to read the data :/


Repository:
  rL LLVM

https://reviews.llvm.org/D41102