[PATCH] D148181: [Demangle] make llvm::demangle take a StringRef; NFC

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/Demangle/Demangle.cpp:31
   std::string Result;
-  const char *S = MangledName.c_str();
+  std::string Copy = MangledName.str();
+  const char *S = Copy.data();
----------------
erichkeane wrote:
> nickdesaulniers wrote:
> > nickdesaulniers wrote:
> > > erichkeane wrote:
> > > > `std::string` is implicitly constructible from a `std::string_view`, so I think on line 37 we can just do: `MangledName[0]` instead of `S[0]`, and just do `return MangledName` below, rather than constructing it every time, despite it being just destructed in every other branch.
> > > > 
> > > > Though, I guess the 'gotcha' here is that despite everything else not modifying the `S` here, that we're no longer able to use strlen for the length.   Perhaps this change should be more viral, in that the other demangle functions should take a `string_view` instead of a `const char *` as well.
> > > > Perhaps this change should be more viral, in that the other demangle functions should take a string_view instead of a const char * as well.
> > > 
> > > Sigh, include/llvm/Demangle/StringView.h has a comment
> > > 
> > >   9 // FIXME: Use std::string_view instead when we support C++17.                   
> > > 
> > > and is used throughout llvm/lib/Demangle/. This is potentially a large cleanup.  Let me start on that...
> > Not having starts_with and ends_with until C++20 is going to be a PITA.
> Ouch, yeah, i could see that being a problem.  Perhaps we need an `llvm::starts_with`/`llvm::ends_with` that implements in terms of substr?
Yeah, I guess llvm/ADT/STLExtras.h might be an appropriate place to add them.

So far in the conversion, another pain point has been llvm::StringView's constructor that takes two char*'s.  I think to break up this conversion, I might remove that constructor and modify the callsites.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148181