dexonsmith added inline comments.
================ Comment at: clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h:36 + OS.flush(); + return Str; } ---------------- 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. 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
