[PATCH] D115374: [NFC][clang] Return std::strings built from raw_string_ostreams more efficiently

[Logan Smith via Phabricator via cfe-commits]

logan-5 added a comment.

In D115374#3181383 <https://reviews.llvm.org/D115374#3181383>, @dexonsmith wrote:

> I don't feel strongly, but IMO the code might be a bit harder to read/maintain with the explicit flush. I worry that it'd be easy to move the `flush()` away from the `return`. Not sure I'm right; could just be familiarity with `str()`.

I definitely hear you. I don't really mind it personally, and I did it this way because I saw precedent in a couple spots (there's one on CompilerInvocation.cpp:653, several in clangd, etc.). I definitely see how it could be a little bit spooky though.

>   std::string Str;
>   llvm::raw_string_ostream(Str) << ...;
>   return Str;

I like this idea. I'd be happy to go back through and change the simple ones to this pattern.

> Another option:
>
>   std::string Result;
>   llvm::raw_string_ostream OS(Str);
>   OS << ...;
>   return OS.take();
>
> Where `raw_string_ostream::take()` is just `return std::move(str())`. It doesn't get NRVO, but I'm not sure that really matters in most of these places. Benefit is that it's a minimal change and the name is clear / matches other LLVM things.

I suppose the question then becomes whether to name it `take()` or `str() &&` for symmetry with C++20's `std::ostringstream`. (Also for the record, I agree that NRVOing some std::strings isn't going to make a giant difference; my opinion is simply that if we can get it, we might as well.)



================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h:36
+    OS.flush();
+    return Str;
   }
----------------
dexonsmith wrote:
> logan-5 wrote:
> > Quuxplusone wrote:
> > > FWIW, it appears to me that in most (all?) of these cases, what's really wanted is not "a string //and// a stream" but rather "a stream that owns a string" (`std::ostringstream` or the LLVM-codebase equivalent thereof). Then the return can be `return std::move(OS).str();` — for `std::ostringstream`, this Does The Right Thing since C++20, and if LLVM had its own stringstream it could make it Do The Right Thing today.
> > > https://en.cppreference.com/w/cpp/io/basic_ostringstream/str
> > > 
> > True enough. Although `return std::move(OS).str();` is still much harder to type than the less efficient `return OS.str();`, and it requires at minimum a move of the underlying string--whereas `return Str;` is the easiest of all to type, and it opens things up for NRVO. If (as I said in the patch summary) `raw_string_ostream` were changed to be guaranteed to not need flushing, `return Str;` would IMHO be cemented as the clear winner.
> > 
> > That said, you're clearly right that all these cases are semantically trying to do "a stream that owns a string", and it's clunky to execute with the existing APIs.
> >  If (as I said in the patch summary) raw_string_ostream were changed to be guaranteed to not need flushing
> 
> This sounds like a one-line patch; might be better to just do it rather than having to churn all these things twice.
> >  If (as I said in the patch summary) raw_string_ostream were changed to be guaranteed to not need flushing
> 
> This sounds like a one-line patch; might be better to just do it rather than having to churn all these things twice.

I guess this change kind of freaks me out. Currently you can call `SetBuffered()` on `raw_string_ostream` (though I don't know why you would...), which creates an intermediate buffer and then `flush()` syncs the buffer with the underlying `std::string&`. Removing that ability would be a breaking change, and I'm not sure how we could make it while being confident we're not breaking anything downstream. 

(On the other hand, you can call `SetBuffered()` on `raw_svector_ostream` too, whose documentation more or less says it doesn't support buffering. If I'm reading right, you get an assert failure in `~raw_ostream()` if you do.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115374