[PATCH] D41102: Setup clang-doc frontend framework

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added inline comments.


================
Comment at: clang-doc/BitcodeWriter.cpp:219
+
+void ClangDocBitcodeWriter::emitIntRecord(int Value, RecordId ID) {
+  if (!Value) return;
----------------
lebedev.ri wrote:
> Now, all these three `emit*Record` functions now have the 'same signature':
> ```
> template <typename T>
> void ClangDocBitcodeWriter::emitRecord(const T& Record, RecordId ID);
> 
> template <>
> void ClangDocBitcodeWriter::emitRecord(StringRef Str, RecordId ID) {
> ...
> ```
> 
> **Assuming there are no implicit conversions going on**, i'd make that change.
> It, again, may open the road for further generalizations.
I overloaded the functions -- cleaner, and deals with any implicit conversions nicely.


================
Comment at: clang-doc/BitcodeWriter.h:178
+  void emitTypeBlock(const std::unique_ptr<TypeInfo> &N);
+  void emitMemberTypeBlock(const std::unique_ptr<MemberTypeInfo> &N);
+  void emitFieldTypeBlock(const std::unique_ptr<FieldTypeInfo> &N);
----------------
lebedev.ri wrote:
> Let's continue cracking down on duplication.
> I think these four functions need the same template treatment as `writeBitstreamForInfo()`
> 
> (please feel free to use better names)
> ```
> template<typename T>
> void emitBlock(const std::unique_ptr<T> &B);
> 
> template<typename T>
> void emitTypedBlock(const std::unique_ptr<T> &B) {
>   StreamSubBlockGuard Block(Stream, MapFromInfoToBlockId<T>::ID);
>   emitBlock(B);
> }
> 
> template<>
> void ClangDocBitcodeWriter::emitBlock(const std::unique_ptr<TypeInfo> &T) {
>   emitStringRecord(T->TypeUSR, FIELD_TYPE_TYPE);
>   for (const auto &CI : T->Description) emitCommentBlock(CI);
> }
> ```
> 
> I agree that it seems strange, and seem to actually increase the code size so far,
> but i believe by exposing similar functionality under one function,
> later, it will open the road for more opportunities of further consolidation.
Since it actually ended up duplicating the `writeBitstreamForInfo()` code, I rolled all of this into one `emitBlock()` entry point.


https://reviews.llvm.org/D41102