lebedev.ri added inline comments.
================ Comment at: clang-doc/BitcodeWriter.cpp:149 + +/// \brief Emits a record ID in the BLOCKINFO block. +void ClangDocBitcodeWriter::emitRecordID(RecordId ID) { ---------------- For me, with no prior knowledge of llvm's bitstreams, it was not obvious that the differences with `emitBlockID()` are intentional. Maybe add a comment noting that for blocks, we do output their ID, but for records, we only output their name. ================ Comment at: clang-doc/BitcodeWriter.cpp:178 +void ClangDocBitcodeWriter::emitLocationAbbrev(RecordId ID, BlockId Block) { + auto EmitString = [](std::shared_ptr<BitCodeAbbrev> &Abbrev) { + Abbrev->Add( ---------------- That should be `EmitLocation` ================ Comment at: clang-doc/BitcodeWriter.cpp:191 +void ClangDocBitcodeWriter::emitIntAbbrev(RecordId ID, BlockId Block) { + auto EmitString = [](std::shared_ptr<BitCodeAbbrev> &Abbrev) { + Abbrev->Add( ---------------- `EmitInt` ================ Comment at: clang-doc/BitcodeWriter.cpp:219 + +void ClangDocBitcodeWriter::emitIntRecord(int Value, RecordId ID) { + if (!Value) return; ---------------- 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. ================ Comment at: clang-doc/BitcodeWriter.h:24 +#include "llvm/ADT/SmallVector.h" +#include "llvm/Bitcode/BitstreamReader.h" +#include "llvm/Bitcode/BitstreamWriter.h" ---------------- Is `BitstreamReader.h` include needed here? ================ Comment at: clang-doc/BitcodeWriter.h:142 + AbbreviationMap() {} + void add(RecordId RID, unsigned abbrevID); + unsigned get(RecordId RID) const; ---------------- `void add(RecordId RID, unsigned AbbrevID);` ================ Comment at: clang-doc/BitcodeWriter.h:175 + void emitStringRecord(StringRef Str, RecordId ID); + void emitLocationRecord(int LineNumber, StringRef File, RecordId ID); + void emitIntRecord(int Value, RecordId ID); ---------------- You have already included `"Representation.h"` here. Why don't you just pass `const Location& Loc` into this function? ================ Comment at: clang-doc/BitcodeWriter.h:177 + void emitIntRecord(int Value, RecordId ID); + void emitTypeBlock(const std::unique_ptr<TypeInfo> &N); + void emitMemberTypeBlock(const std::unique_ptr<MemberTypeInfo> &N); ---------------- New line ``` void emitIntRecord(int Value, RecordId ID); void emitTypeBlock(const std::unique_ptr<TypeInfo> &N); ``` ================ 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); ---------------- 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. https://reviews.llvm.org/D41102 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits