[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);
   }
----------------
juliehockett wrote:
> lebedev.ri wrote:
> > 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?
> Ah, yes. I see what you're trying to do, but the issue with that is it assumes that the record must be a certain size (or smaller). However, there is no guarantee of the size of the record (i.e. there may and will be records of variable size getting written), so it's not unreasonable to think that the record may need to be resized to accommodate that.
> However, there is no guarantee of the size of the record (i.e. there may and will be records of variable size getting written), so it's not unreasonable to think that the record may need to be resized to accommodate that.

Yes, but *currently* that was correct assumption.
All variable-length records (well, blobs) are (were?) emitted directly, without using this intermediate `Record` container.


https://reviews.llvm.org/D43341