[PATCH] D41102: Setup clang-doc frontend framework

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Thank you for working on this!
Some more nitpicking.

//Please// consider adding even more tests (ideally, all this code should have 100% test coverage)



================
Comment at: clang-doc/BitcodeWriter.cpp:139
+          {COMMENT_NAME, {"Name", &StringAbbrev}},
+          {COMMENT_POSITION, {"Position", &IntAbbrev}},
+          {COMMENT_DIRECTION, {"Direction", &StringAbbrev}},
----------------
This change is not covered by tests.
(I've actually found out that the hard way, by trying to find why it didn't trigger any asssertions, oh well)


================
Comment at: clang-doc/BitcodeWriter.cpp:325
+  emitHeader();
+  Stream.EnterBlockInfoBlock();
+
----------------
I think it would be cleaner to move it (at least the enterblock, it might make sense to leave the header at the very top) after the static variable


================
Comment at: clang-doc/BitcodeWriter.cpp:363
+
+  for (const auto &Block : TheBlocks) {
+    assert(Block.second.size() < (1U << BitCodeConstants::SubblockIDSize));
----------------
I.e.
```
...
, FUNCTION_IS_METHOD}}};

  Stream.EnterBlockInfoBlock();
  for (const auto &Block : TheBlocks) {
    assert(Block.second.size() < (1U << BitCodeConstants::SubblockIDSize));
    emitBlockInfo(Block.first, Block.second);
  }
  Stream.ExitBlock();

  emitVersion();
}
```


================
Comment at: clang-doc/BitcodeWriter.h:19
+
+#include <initializer_list>
+#include <vector>
----------------
Please sort includes, clang-tidy complains.


================
Comment at: clang-doc/BitcodeWriter.h:32
+// BitCodeConstants, though they can be added without breaking it.
+static const unsigned VERSION_NUMBER = 1;
+
----------------
```
/build/clang-tools-extra/clang-doc/BitcodeWriter.h:32:23: warning: invalid case style for variable 'VERSION_NUMBER' [readability-identifier-naming]
static const unsigned VERSION_NUMBER = 1;
                      ^~~~~~~~~~~~~~
                      VersionNumber

```


================
Comment at: clang-doc/BitcodeWriter.h:163
+ public:
+  ClangDocBitcodeWriter(llvm::BitstreamWriter &Stream,
+                        bool OmitFilenames = false)
----------------
The simplest solution would be
```
#ifndef NDEBUG // Don't want explicit dtor unless needed
~ClangDocBitcodeWriter() {
  // Check that the static size is large-enough.
  assert(Record.capacity() == BitCodeConstants::RecordSize);
}
#endif
```


================
Comment at: clang-doc/BitcodeWriter.h:228
+  // Static size is the maximum length of the block/record names we're pushing
+  // to this + 1. Longest is currently `MemberTypeBlock` at 15 chars.
+  SmallVector<uint32_t, BitCodeConstants::RecordSize> Record;
----------------
So you want to be really definitive with this. I wanted to avoid that, actually..
Then i'm afraid one more assert is needed, to make sure this is *actually* true.

I'm not seeing any way to make `SmallVector` completely static,
so you could either add one more wrapper around it (rather ugly),
or check the final size in the `ClangDocBitcodeWriter` destructor (will not pinpoint when the size has 'overflowed')


================
Comment at: clang-doc/BitcodeWriter.h:246
+void ClangDocBitcodeWriter::writeBitstream(const T &I, bool WriteBlockInfo) {
+  if (WriteBlockInfo) emitBlockInfoBlock();
+  StreamSubBlockGuard Block(Stream, MapFromInfoToBlockId<T>::ID);
----------------
Does it *ever* make sense to output `BlockInfoBlock` anywhere else other than once at the very beginning?
I'd think you should drop the boolean param, and unconditinally call the `emitBlockInfoBlock();` from `ClangDocBitcodeWriter::ClangDocBitcodeWriter()` ctor.


================
Comment at: clang-doc/BitcodeWriter.h:248
+  StreamSubBlockGuard Block(Stream, MapFromInfoToBlockId<T>::ID);
+  emitBlock(I);
+}
----------------
The naming choices confuse me.
There is `writeBitstream()` and `emitBlock()`, which is called from `writeBitstream()` to write the actual contents of the block.

Why one is `write` and another is `emit`?
To match the `BitstreamWriter` naming choices? (which uses `Emit` prefix)?
To avoid the confusion of which one outputs the actual content, and which one outputs the whole block?

I think it should be:
*
```
- void emitBlock(const NamespaceInfo &I);
+ void emitBlockContent(const NamespaceInfo &I);
```
*
```
- void ClangDocBitcodeWriter::writeBitstream(const T &I, bool WriteBlockInfo);
+ void ClangDocBitcodeWriter::emitBlock(const T &I, bool EmitBlockInfo);
```

This way, *i think* their names would clearner-er state what they do, and won't be weirdly different.
What do you think?


================
Comment at: clang-doc/Representation.h:18
+
+#include <string>
+#include "clang/AST/Type.h"
----------------
Please sort includes, clang-tidy complains.


================
Comment at: clang-doc/Serialize.cpp:88
+  CurrentCI.Name = getCommandName(C->getCommandID());
+  for (unsigned i = 0, e = C->getNumArgs(); i < e; ++i)
+    CurrentCI.Args.push_back(C->getArgText(i));
----------------
```
/build/clang-tools-extra/clang-doc/Serialize.cpp:88:17: warning: invalid case style for variable 'i' [readability-identifier-naming]
  for (unsigned i = 0, e = C->getNumArgs(); i < e; ++i)
                ^                           ~        ~~
                I                           I        I
/build/clang-tools-extra/clang-doc/Serialize.cpp:88:24: warning: invalid case style for variable 'e' [readability-identifier-naming]
  for (unsigned i = 0, e = C->getNumArgs(); i < e; ++i)
                       ^                        ~~
                       E                        E

```


================
Comment at: clang-doc/Serialize.cpp:107
+  if (C->isPositionValid()) {
+    for (unsigned i = 0, e = C->getDepth(); i < e; ++i)
+      CurrentCI.Position.push_back(C->getIndex(i));
----------------
```
/build/clang-tools-extra/clang-doc/Serialize.cpp:107:19: warning: invalid case style for variable 'i' [readability-identifier-naming]
    for (unsigned i = 0, e = C->getDepth(); i < e; ++i)
                  ^                         ~        ~~
                  I                         I        I
/build/clang-tools-extra/clang-doc/Serialize.cpp:107:26: warning: invalid case style for variable 'e' [readability-identifier-naming]
    for (unsigned i = 0, e = C->getDepth(); i < e; ++i)
                         ^                      ~~
                         E                      E

```


================
Comment at: clang-doc/Serialize.h:19
+
+#include <string>
+#include <vector>
----------------
Please sort includes, clang-tidy complains.


================
Comment at: clang-doc/tool/ClangDocMain.cpp:80
+        getInsertArgumentAdjuster("-fparse-all-comments",
+                                  tooling::ArgumentInsertPosition::BEGIN),
+        ArgAdjuster);
----------------
Why at the beginning though?
Couldn't the user pass `-extra-arg=-fno-parse-all-comments`, which could override this?


https://reviews.llvm.org/D41102