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

[Alexey Lapshin via Phabricator via llvm-commits]

avl 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.

The idea behind usage of reserve() method is that the size which should be reserved could be unknown. 
Thus we do not know how to reserve strings and vectors at creation point. The same for memory mapped file: it could be unknown which class to create raw_fd_ostream or raw_mmap_ostream:

  void WriteToStreamLibrayFunc(raw_ostream& Out) {
     if ( HasVariableData()) {
        while (HasData()) {
           Out.Write()
        }
     } else {
        size_t Size = calculateSize();
        
        Out.reserve(Size);
        Out.Write();
     }
  }

Usage 1:

  std::string string;
  string.reserve(?????);
  raw_string_ostream Out(string);
  WriteToStreamLibrayFunc (Out);

Usage 2:

  raw_fd_ostream?raw_mmap_ostream? Out();
  WriteToStreamLibrayFunc (Out);

So, using reserve() method allows us to create effective storage if resulting size could be calculated under some conditions.



================
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) {
----------------
jhenderson wrote:
> 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.
The reason is when some space is allocated by reserve() method then other buffers become useless in most cases. We do not need to copy data through internal buffers - we could write them directly into memory mapped file.


================
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.");
+
----------------
jhenderson wrote:
> 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?
It seems to me that we do not need to bail out. That is a programmer error as you said. But we need to stay correct. i.e. I think I need to add check for sizes(to ignore writing out of the file), like in raw_fd_stream::read().


================
Comment at: llvm/lib/Support/raw_ostream.cpp:819
+  if (MMapFile) {
+    assert(off <= *ReservedSize);
+    pos = off;
----------------
jhenderson wrote:
> Same as above. Perhaps seek to EOF here in that case?
yes. I think seeking to EOF is a good idea. But I think it is better to have assertion here, not to bail out with error.


================
Comment at: llvm/lib/Support/raw_ostream.cpp:920-923
+  if (EC) {
+    MMapFile.reset();
+    return;
+  }
----------------
jhenderson wrote:
> This should report some sort of error up the tree if this happens, right? Maybe `reserve` should return `Error`.
The idea of that method is when it could not use memory-mapped file then we continue with usual processing. F.e. if file is opened without read access then we will have a error "permission denied". But we would not like to report this error. We would like to continue without memory mapped file. So the idea, is that if reserve() method is not able to reserve data then we continue the same as without calling to reserve(). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91693