[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: clang-doc/BitcodeWriter.h:129
     // Check that the static size is large-enough.
     assert(Record.capacity() > BitCodeConstants::RecordSize);
   }
----------------
lebedev.ri wrote:
> juliehockett wrote:
> > lebedev.ri wrote:
> > > Isn't that the opposite of what was that assert supposed to do?
> > > It would have been better to just `// disable` it, and add a `FIXME` note.
> > I'm not sure I'm understanding you -- my understanding was that it existed to check that the record size was large enough.
> https://reviews.llvm.org/D41102?id=136520#inline-384087
> ```
> #ifndef NDEBUG // Don't want explicit dtor unless needed
> ~ClangDocBitcodeWriter() {
>   // Check that the static size is large-enough.
>   assert(Record.capacity() == BitCodeConstants::RecordSize);
> }
> #endif
> ```
> I.e. it checked that it still only had the static size, and did not transform into normal `vector` with data stored on heap-allocated buffer.
Ok, let me use more words this time.
There are several storage container types with contiguous storage:
* `array` - fixed-size stack storage, size == capacity == compile-time constant
* `vector` - dynamic heap storage, size <= capacity
* `smallvector` - fixed-size stack storage, and if the data does not fit, it degrades into the `vector`.

But there is also one more option:
* `fixedvector` - which is a `array` - has only fixed-size stack storage (capacity == compile-time constant), but with vector-like interface, i.e. size <= capacity, and size can be changed at run-time.

In llvm, i'm not aware of existence of `llvm::fixedvector`, so that assert was ensuring that the `smallvector` behaved like the `fixedvector` - it only had the fixed-size stack storage, and 'did not have' the heap buffer.

This is a kinda ugly hack, but i think the current assert clearly does something different...

Does that make any sense?


================
Comment at: clang-doc/Representation.cpp:28
+  if (L.Namespace.empty())
+    L.Namespace = std::move(R.Namespace);
+  std::move(R.Description.begin(), R.Description.end(),
----------------
This was changed, but no test changes followed.
Needs more tests. The test coverage is clearly bad.


https://reviews.llvm.org/D43341