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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
