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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

For `raw_fd_ostream`, I wonder if we should actually just have a separate class e.g. `raw_mmap_ostream`, rather than trying to make `raw_fd_ostream` represent two possible different things. For other streams, I'm not sure the reserving behaviour is useful - strings and vectors could be reserved from outside the wrapping stream, for example.



================
Comment at: llvm/include/llvm/Support/raw_ostream.h:138
+  /// If possible, preallocate space for stream data when the final size
+  /// is already known. Attempt to put more data into the stream
+  /// than it was reserved is not allowed. Set the stream to be unbuffered
----------------



================
Comment at: llvm/include/llvm/Support/raw_ostream.h:139-140
+  /// is already known. Attempt to put more data into the stream
+  /// than it was reserved is not allowed. Set the stream to be unbuffered
+  /// if the space was successfully reserved.
+  virtual void reserve(uint64_t Size) {
----------------
What's the reason for setting the stream to be unbuffered on reserving space? It seems like a potentially unexpected side effect that isn't strictly needed.


================
Comment at: llvm/include/llvm/Support/raw_ostream.h:144
+    assert(tell() == 0 &&
+           "Can`t reserve space if any data is already written.");
+    ReservedSize = Size;
----------------



================
Comment at: llvm/include/llvm/Support/raw_ostream.h:575-578
+  /// If possible, preallocate space for stream data when the final size
+  /// is already known. Attempt to put more data into the stream
+  /// than it was reserved is not allowed. Set the stream to be unbuffered
+  /// if the space was successfully reserved. raw_fd_ostream uses a
----------------
Same comments as above. I'm not convinced duplicating the comment is worthwhile, but I suppose these are separate classes, so it's probably acceptable.


================
Comment at: llvm/include/llvm/Support/raw_ostream.h:582
+  /// buffer.
+  virtual void reserve(uint64_t Size) override;
 };
----------------
No need for `virtual` when the base method is `virtual` and the sub class method has `override`.


================
Comment at: llvm/include/llvm/Support/raw_ostream.h:656-660
+  /// If possible, preallocate space for stream data when the final size
+  /// is already known. Attempt to put more data into the stream
+  /// than it was reserved is not allowed. Set the stream to be unbuffered
+  /// if the space was successfully reserved.
+  virtual void reserve(uint64_t Size) override {
----------------
Ditto.


================
Comment at: llvm/include/llvm/Support/raw_ostream.h:698-702
+  /// If possible, preallocate space for stream data when the final size
+  /// is already known. Attempt to put more data into the stream
+  /// than it was reserved is not allowed. Set the stream to be unbuffered
+  /// if the space was successfully reserved.
+  virtual void reserve(uint64_t Size) override {
----------------
Ditto.


================
Comment at: llvm/lib/Support/raw_ostream.cpp:742-743
   assert(FD >= 0 && "File already closed.");
-  pos += Size;
+  assert(!ReservedSize ||
+         pos + Size <= *ReservedSize && "Writing over reserved area.");
+
----------------
It's definitely a programmer error if this case is hit, but I think it might be worth also bailing out of the function in that case, to avoid writing past the end of the file, which could cause all sorts of horridness. What do you think?


================
Comment at: llvm/lib/Support/raw_ostream.cpp:819
+  if (MMapFile) {
+    assert(off <= *ReservedSize);
+    pos = off;
----------------
Same as above. Perhaps seek to EOF here in that case?


================
Comment at: llvm/lib/Support/raw_ostream.cpp:920-923
+  if (EC) {
+    MMapFile.reset();
+    return;
+  }
----------------
This should report some sort of error up the tree if this happens, right? Maybe `reserve` should return `Error`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91693