ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
lgtm with just a few more nits.
================
Comment at: clang-doc/BitcodeWriter.cpp:484
#undef EMITINFO
----------------
Nit: `EMITINFO` is a bit confusing with `writeInfo`. Are they doing the same
thing (`emit` sounds like `write`)? You might want to rename either of them.
While you are here, it might also make sense to clear up the MACRO :)
================
Comment at: clang-doc/Representation.cpp:57
+ return llvm::make_error<llvm::StringError>("Unexpected info type.\n",
+ std::error_code());
+ }
----------------
nit: `std::error_code()` is usually used to indicate no error. You could use
`llvm::inconvertibleErrorCode()`.
================
Comment at: clang-doc/tool/ClangDocMain.cpp:181
+ doc::writeInfo(I.get(), Buffer);
+ if (DumpResultToFile("bc", Group.getKey() + ".bc", Buffer))
+ return 1;
----------------
juliehockett wrote:
> ioeric wrote:
> > juliehockett wrote:
> > > ioeric wrote:
> > > > (Sorry that I might be missing context here.)
> > > >
> > > > Could you please explain the incentive for dumping each info group to
> > > > one bc file? If the key identifies a symbol (e.g. USR), wouldn't this
> > > > result in a bitcode file being created for each symbol? This doesn't
> > > > seem very scalable.
> > > Yes, it would. This is mostly for debugging, since there's not really any
> > > tools outside the clang system that would actually want/be able to use
> > > this information.
> > Ok, is there any plan to convert intermediate result to final result and
> > output in a more accessible format? If so, please add a `FIXME` somewhere
> > just to be clearer.
> That's what the next patch set is (to generate YAML).
Sure. I think a `FIXME` should be in place if the feature is not already there.
================
Comment at: clang-doc/tool/ClangDocMain.cpp:68
+ "format",
+ llvm::cl::desc("Format for outputted docs (Current options are yaml)."),
+ llvm::cl::init("yaml"), llvm::cl::cat(ClangDocCategory));
----------------
nit: s/current option/default option/
================
Comment at: clang-doc/tool/ClangDocMain.cpp:68
+ "format",
+ llvm::cl::desc("Format for outputted docs (Current options are yaml)."),
+ llvm::cl::init("yaml"), llvm::cl::cat(ClangDocCategory));
----------------
ioeric wrote:
> nit: s/current option/default option/
consider using enum type options. Example:
https://github.com/llvm-mirror/clang-tools-extra/blob/master/include-fixer/tool/ClangIncludeFixer.cpp#L85
================
Comment at: clang-doc/tool/ClangDocMain.cpp:178
+ if (!Reduced)
+ llvm::errs() << llvm::toString(Reduced.takeError());
+
----------------
Shouldn't we `continue` to the next group if reduction goes wrong for the
current group?
https://reviews.llvm.org/D43341
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
