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

[Julie Hockett via Phabricator via cfe-commits]

juliehockett marked 5 inline comments as done.
juliehockett added inline comments.


================
Comment at: clang-doc/BitcodeReader.h:59
+
+  void storeData(llvm::SmallVectorImpl<char> &Field, llvm::StringRef Blob);
+  void storeData(bool &Field, llvm::StringRef Blob);
----------------
sammccall wrote:
> Comment for these like "decodes the blob part of a record" - or just rename these `decodeBlob` or similar?
In general they decode the record abbrev of the field, which is a single blob in many, but not all, cases. I've changed it to `decodeRecord`, but is too similar to the above?


================
Comment at: clang-doc/BitcodeWriter.h:129
     // Check that the static size is large-enough.
     assert(Record.capacity() > BitCodeConstants::RecordSize);
   }
----------------
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.


https://reviews.llvm.org/D43341