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
