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

[logan-5]

Summary

Returning OS.str() is guaranteed to copy the underlying string, whereas OS.flush(); return Underlying; makes Underlying a candidate for NRVO, or at worst, implicit move.

To keep this kind of inefficiency at bay in the future, the fast code should probably be made easier to type than the slow code (as it's currently the opposite). Perhaps this could be solved by:

• making raw_string_ostream guarantee+document that it does not need to be flush()ed (similar to raw_svector_ostream), and/or
• making raw_string_ostream::str() return a StringRef rather than std::string&, to discourage using it to initialize std::strings

Implementing those two ideas would make simply return Underlying; the natural, correct, and efficient choice. They are, however, out of scope for this patch.

[• Quuxplusone]

clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h

36

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

[logan-5]

clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h

36

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.

[dexonsmith]

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().

Have you considered other options? Two below.

One option:

std::string Result;
{
  llvm::raw_string_ostream OS(Str);
  OS << ...;
}
return Result;

which in branch-free cases could shorten to:

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

I personally find the lifetime a bit more obvious than the explicit flush call.

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.

clang/lib/AST/DeclarationName.cpp

236–240 ↗

(On Diff #392864)

For trivial cases like this, maybe it'd be worth creating a helper function that gets reused. Maybe called llvm::raw_string_ostream::toString():

template <class T>
std::string raw_string_ostream::toString(const T &V) {
  std::string Result;
  raw_string_ostream(Result) << V;
  return Result;
}

and the code here would be:

return llvm::raw_string_ostream::toString(*this);

[dexonsmith]

clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h

36

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.

[logan-5]

In 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.)

clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h

36

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

[logan-5]

Eliminate some explicit .flush()es by using temporary raw_string_ostreams where possible.

[dexonsmith]

In D115374#3181491, @logan-5 wrote:

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

I haven't done the git-blame, but I somewhat suspect str() was added specifically as a convenience to make the flush+access/return a one-liner.

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

I think take() would be clearer for use in LLVM code, despite the symmetry. Especially since the two stream libraries have significant differences. (Also, maybe I'm mis-remembering, but I think I've seen places where OS.str() is passed to some function when it's partially written; seems dangerous to silent start selecting a different overload. Could be I'm wrong though.)

clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h

36

I don't think we need to be afraid. raw_ostream seems like a bit of a leaky abstraction, but IMO we need to trust that code doesn't arbitrarily call SetBuffered() on without full control. It's essentially never the right thing to do except from subclasses (where it could be protected) and maybe in unit tests for convenience. As you pointed, out this would also break svector_stream.

What I'd be more worried about is the micro-performance-penalty of std::string null-terminating with every write. But probably that's not particularly important either.

[logan-5]

To be clear, it sounds like we should either add .take() for moving the string out of raw_string_ostream's string reference, or make raw_string_ostream not need to be flushed (after which there won't be as clear a use for .take(), since you can just use the underlying string directly).

I vote for the second option, making raw_string_ostream not need to be flushed, since it allows for simpler code at the call site (return Str;), permits NRVO at the call site, and avoids some possibly weird questions .take() would bring with it (like whether it would ever be surprising that it moves out of a reference that someone else might also have).

If that's the direction that sounds best, I can submit an updated patch sometime tomorrow.

[dexonsmith]

In D115374#3181670, @logan-5 wrote:

To be clear, it sounds like we should either add .take() for moving the string out of raw_string_ostream's string reference, or make raw_string_ostream not need to be flushed (after which there won't be as clear a use for .take(), since you can just use the underlying string directly).

I vote for the second option, making raw_string_ostream not need to be flushed, since it allows for simpler code at the call site (return Str;), permits NRVO at the call site, and avoids some possibly weird questions .take() would bring with it (like whether it would ever be surprising that it moves out of a reference that someone else might also have).

If that's the direction that sounds best, I can submit an updated patch sometime tomorrow.

Yeah, probably one or the other. I'm leaning toward the no-flush approach as well, but I'd suggest making that a separate prep patch to reduce churn/dependencies in case there's some reason it needs to be reverted (e.g., compile time regression). Removing the no-longer-needed .str()s in a follow-up a few days later (rebasing this without the flushes) should be trivial.

[dexonsmith]

Returning from https://reviews.llvm.org/D115421, it looks like that raw_string_ostream has unbuffered by default since 65b13610a5226b84889b923bae884ba395ad084d. Seems like all the new flush()s in this patch are no-ops. You can simplify this patch to just mechanically converting return OS.str() to return S.

[logan-5]

Removed .flush()es. Seems like this might be able to land without https://reviews.llvm.org/D115421?

[dexonsmith]

In D115374#3183776, @logan-5 wrote:

Removed .flush()es. Seems like this might be able to land without https://reviews.llvm.org/D115421?

Yup! LGTM if you remove the cleanups that are no longer related to this patch, and/or split them into separate follow-up NFC commits. Might be nice to split even the main commit into a few pieces (e.g., by functional area) to push separately, just in case something needs to be reverted due to a call to SetBuffered() in an unexpected place.

Please make sure the commit message points at 65b13610a5226b84889b923bae884ba395ad084d, which made this correct.

clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h

72

Given there's no scope change needed, these cleanups are no longer related to the patch and are adding noise to the diff. I suggest committing them separately (or not at all).

clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h

 class SValExplainer : public FullSValVisitor<SValExplainer, std::string> {
 private:
   ASTContext &ACtx;
 
   std::string printStmt(const Stmt *S) {
     std::string Str;
     llvm::raw_string_ostream OS(Str);
     S->printPretty(OS, nullptr, PrintingPolicy(ACtx.getLangOpts()));
-    return OS.str();
+    return Str;
   }
 
   bool isThisObject(const SymbolicRegion *R) {
     if (auto S = dyn_cast<SymbolRegionValue>(R->getSymbol()))
       if (isa<CXXThisRegion>(S->getRegion()))
         return true;
     return false;
   }
 
std::string VisitLocMemRegionVal(loc::MemRegionVal V) {
     return "pointer to " + Visit(R);
   }
 
   std::string VisitLocConcreteInt(loc::ConcreteInt V) {
     const llvm::APSInt &I = V.getValue();
     std::string Str;
     llvm::raw_string_ostream OS(Str);
     OS << "concrete memory address '" << I << "'";
-    return OS.str();
+    return Str;
   }
 
   std::string VisitNonLocSymbolVal(nonloc::SymbolVal V) {
     return Visit(V.getSymbol());
   }
 
   std::string VisitNonLocConcreteInt(nonloc::ConcreteInt V) {
     const llvm::APSInt &I = V.getValue();
     std::string Str;
     llvm::raw_string_ostream OS(Str);
     OS << (I.isSigned() ? "signed " : "unsigned ") << I.getBitWidth()
        << "-bit integer '" << I << "'";
-    return OS.str();
+    return Str;
   }
 
   std::string VisitNonLocLazyCompoundVal(nonloc::LazyCompoundVal V) {
     return "lazily frozen compound value of " + Visit(V.getRegion());
   }
 
   std::string VisitSymbolRegionValue(const SymbolRegionValue *S) {
     const MemRegion *R = S->getRegion();
public:
   }
 
   std::string VisitSymIntExpr(const SymIntExpr *S) {
     std::string Str;
     llvm::raw_string_ostream OS(Str);
     OS << "(" << Visit(S->getLHS()) << ") "
        << std::string(BinaryOperator::getOpcodeStr(S->getOpcode())) << " "
        << S->getRHS();
-    return OS.str();
+    return Str;
   }
 
   // TODO: IntSymExpr doesn't appear in practice.
   // Add the relevant code once it does.
 
   std::string VisitSymSymExpr(const SymSymExpr *S) {
     return "(" + Visit(S->getLHS()) + ") " +
            std::string(BinaryOperator::getOpcodeStr(S->getOpcode())) +
std::string VisitElementRegion(const ElementRegion *R) {
     OS << "element of type '" << R->getElementType().getAsString()
        << "' with index ";
     // For concrete index: omit type of the index integer.
     if (auto I = R->getIndex().getAs<nonloc::ConcreteInt>())
       OS << I->getValue();
     else
       OS << "'" << Visit(R->getIndex()) << "'";
     OS << " of " + Visit(R->getSuperRegion());
-    return OS.str();
+    return Str;
   }
 
   std::string VisitNonParamVarRegion(const NonParamVarRegion *R) {
     const VarDecl *VD = R->getDecl();
     std::string Name = VD->getQualifiedNameAsString();
     if (isa<ParmVarDecl>(VD))
       return "parameter '" + Name + "'";
     else if (VD->hasAttr<BlocksAttr>())

clang/lib/Analysis/AnalysisDeclContext.cpp

if (const auto *OID = dyn_cast<ObjCImplementationDecl>(DC)) {
            << OC->getIdentifier()->getNameStart() << ')';
       }
     } else if (const auto *OCD = dyn_cast<ObjCCategoryImplDecl>(DC)) {
       OS << OCD->getClassInterface()->getName() << '(' << OCD->getName() << ')';
     }
     OS << ' ' << OMD->getSelector().getAsString() << ']';
   }
 
-  return OS.str();
+  return Str;
 }
 
 LocationContextManager &AnalysisDeclContext::getLocationContextManager() {
   assert(
       ADCMgr &&
       "Cannot create LocationContexts without an AnalysisDeclContextManager!");
   return ADCMgr->getLocationContextManager();
 }

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

if (hasFuchsiaAttr<AcquireHandleAttr>(FuncDecl)) {
     SymbolRef RetSym = Call.getReturnValue().getAsSymbol();
     Notes.push_back([RetSym, FuncDecl](BugReport &BR) -> std::string {
       auto *PathBR = static_cast<PathSensitiveBugReport *>(&BR);
       if (auto IsInteresting = PathBR->getInterestingnessKind(RetSym)) {
         std::string SBuf;
         llvm::raw_string_ostream OS(SBuf);
         OS << "Function '" << FuncDecl->getDeclName()
            << "' returns an open handle";
-        return OS.str();
+        return SBuf;
       } else
         return "";
     });
     State =
         State->set<HStateMap>(RetSym, HandleState::getMaybeAllocated(nullptr));
   } else if (hasFuchsiaUnownedAttr<AcquireHandleAttr>(FuncDecl)) {
     // Function returns an unowned handle
     SymbolRef RetSym = Call.getReturnValue().getAsSymbol();
     Notes.push_back([RetSym, FuncDecl](BugReport &BR) -> std::string {
       auto *PathBR = static_cast<PathSensitiveBugReport *>(&BR);
       if (auto IsInteresting = PathBR->getInterestingnessKind(RetSym)) {
         std::string SBuf;
         llvm::raw_string_ostream OS(SBuf);
         OS << "Function '" << FuncDecl->getDeclName()
            << "' returns an unowned handle";
-        return OS.str();
+        return SBuf;
       } else
         return "";
     });
     State = State->set<HStateMap>(RetSym, HandleState::getUnowned());
   }
 
   for (unsigned Arg = 0; Arg < Call.getNumArgs(); ++Arg) {
     if (Arg >= FuncDecl->getNumParams())
for (SymbolRef Handle : Handles) {
         } else {
           Notes.push_back([Handle, ParamDiagIdx](BugReport &BR) -> std::string {
             auto *PathBR = static_cast<PathSensitiveBugReport *>(&BR);
             if (auto IsInteresting = PathBR->getInterestingnessKind(Handle)) {
               std::string SBuf;
               llvm::raw_string_ostream OS(SBuf);
               OS << "Handle released through " << ParamDiagIdx
                  << llvm::getOrdinalSuffix(ParamDiagIdx) << " parameter";
-              return OS.str();
+              return SBuf;
             } else
               return "";
           });
           State = State->set<HStateMap>(Handle, HandleState::getReleased());
         }
       } else if (hasFuchsiaAttr<AcquireHandleAttr>(PVD)) {
         Notes.push_back([Handle, ParamDiagIdx](BugReport &BR) -> std::string {
           auto *PathBR = static_cast<PathSensitiveBugReport *>(&BR);
           if (auto IsInteresting = PathBR->getInterestingnessKind(Handle)) {
             std::string SBuf;
             llvm::raw_string_ostream OS(SBuf);
             OS << "Handle allocated through " << ParamDiagIdx
                << llvm::getOrdinalSuffix(ParamDiagIdx) << " parameter";
-            return OS.str();
+            return SBuf;
           } else
             return "";
         });
         State = State->set<HStateMap>(
             Handle, HandleState::getMaybeAllocated(ResultSymbol));
       } else if (hasFuchsiaUnownedAttr<AcquireHandleAttr>(PVD)) {
         Notes.push_back([Handle, ParamDiagIdx](BugReport &BR) -> std::string {
           auto *PathBR = static_cast<PathSensitiveBugReport *>(&BR);
           if (auto IsInteresting = PathBR->getInterestingnessKind(Handle)) {
             std::string SBuf;
             llvm::raw_string_ostream OS(SBuf);
             OS << "Unowned handle allocated through " << ParamDiagIdx
                << llvm::getOrdinalSuffix(ParamDiagIdx) << " parameter";
-            return OS.str();
+            return SBuf;
           } else
             return "";
         });
         State = State->set<HStateMap>(Handle, HandleState::getUnowned());
       } else if (!hasFuchsiaAttr<UseHandleAttr>(PVD) &&
                  PVD->getType()->isIntegerType()) {
         // Working around integer by-value escapes.
         // The by-value escape would not be captured in checkPointerEscape.

clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp

std::string HTMLDiagnostics::GenerateHTML(const PathDiagnostic& D, Rewriter &R,
   const FileEntry* Entry = SMgr.getFileEntryForID(FID);
   FinalizeHTML(D, R, SMgr, path, FileIDs[0], Entry, declName);
 
   std::string file;
   llvm::raw_string_ostream os(file);
   for (auto BI : *Buf)
     os << BI;
 
-  return os.str();
+  return file;
 }
 
 void HTMLDiagnostics::dumpCoverageData(
     const PathDiagnostic &D,
     const PathPieces &path,
     llvm::raw_string_ostream &os) {
 
   const FilesToLineNumsMap &ExecutedLines = D.getExecutedLines();
<form>
     <input type="checkbox" name="showArrows"
            id="showArrows" style="margin-left: 10px" />
     <label for="showArrows">
        Show control flow arrows
     </label>
 </form>
 )<<<";
 
-  return os.str();
+  return s;
 }
 
 void HTMLDiagnostics::FinalizeHTML(const PathDiagnostic& D, Rewriter &R,
     const SourceManager& SMgr, const PathPieces& path, FileID FID,
     const FileEntry *Entry, const char *declName) {
   // This is a cludge; basically we want to append either the full
   // working directory if we have no directory information.  This is
   // a work in progress.
const arrowIndices = )<<<";
   R.InsertTextBefore(R.getSourceMgr().getLocForStartOfFile(BugFileID),
                      OS.str());
 }
 
 std::string getSpanBeginForControl(const char *ClassName, unsigned Index) {
   std::string Result;
   llvm::raw_string_ostream OS(Result);
   OS << "<span id=\"" << ClassName << Index << "\">";
-  return OS.str();
+  return Result;
 }
 
 std::string getSpanBeginForControlStart(unsigned Index) {
   return getSpanBeginForControl("start", Index);
 }
 
 std::string getSpanBeginForControlEnd(unsigned Index) {
   return getSpanBeginForControl("end", Index);

clang/lib/StaticAnalyzer/Core/MemRegion.cpp

 LLVM_DUMP_METHOD void MemRegion::dump() const {
   dumpToStream(llvm::errs());
 }
 
 std::string MemRegion::getString() const {
   std::string s;
   llvm::raw_string_ostream os(s);
   dumpToStream(os);
-  return os.str();
+  return s;
 }
 
 void MemRegion::dumpToStream(raw_ostream &os) const {
   os << "<Unknown Region>";
 }
 
 void AllocaRegion::dumpToStream(raw_ostream &os) const {
   os << "alloca{S" << Ex->getID(getContext()) << ',' << Cnt << '}';