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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
