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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
