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