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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

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.



================
Comment at: clang/lib/AST/DeclarationName.cpp:236-240
   std::string Result;
   llvm::raw_string_ostream OS(Result);
   OS << *this;
-  return OS.str();
+  OS.flush();
+  return Result;
----------------
For trivial cases like this, maybe it'd be worth creating a helper function that gets reused. Maybe called `llvm::raw_string_ostream::toString()`:
```
lang=c++
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:
```
lang=c++
  return llvm::raw_string_ostream::toString(*this);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115374