[PATCH] D91693: [Support] Add reserve() method to the raw_ostream.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/include/llvm/Support/raw_ostream.h:634
+
+  void reserve(size_t Capacity) override { OS.reserve(Capacity); }
 };
----------------
dblaikie wrote:
> avl wrote:
> > dblaikie wrote:
> > > Would it make sense for the reserve operation to be relative to where the stream has reached so far?
> > > 
> > > ie: for it to be implemented as:
> > > ```
> > > void reserve(size_t ExtraCapacity) override { OS.reserve(OS.size() + ExtraCapacity); }
> > > ```
> > > (similarly for other implementations)
> > > Because the average caller probably shouldn't be thinking about how many bytes have already been written to the stream when deciding how much capacity they want for future write operations?
> > I think - yes, it makes sense. In that case, it would probably be better to name that method reserveExtraSpace()(to look differently than std::vector::reserve()). 
> > 
> > Also, I think it should not be calculated against OS.size(). It looks better to calculate it against current_pos()(Because the callee can receive stream with the current position in the middle of the stream) 
> > 
> > Would update the review accordingly(if there are no objections).
> Happy to use current_pos, though it looks like it's == size anyway, right?
> ```
> uint64_t raw_svector_ostream::current_pos() const { return OS.size(); }
> ```
> & raw_string_ostream's member:
> ```
>   uint64_t current_pos() const override { return OS.size(); }
> ```
>Happy to use current_pos, though it looks like it's == size anyway, right?

right. I was thinking on general case when we might seek from the current position(like in  raw_fd_ostream). Thus using current_pos() might be more general. But, since we have specialized implementation in raw_svector_ostream/raw_string_ostream and do not have seek methods there - it would be fine to use OS.size().


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91693/new/

https://reviews.llvm.org/D91693