[PATCH] D41102: Setup clang-doc frontend framework

[Roman Lebedev via Phabricator via cfe-commits]

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