[PATCH] D125778: [llvm-dva] 03 - Logical elements

[Pavel Samolysov via Phabricator via llvm-commits]

psamolysov added inline comments.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:461
+    QualifiedName.append("::");
+  QualifiedName.append(std::string(getName()));
+}
----------------
CarlosAlbertoEnciso wrote:
> psamolysov wrote:
> > Are you sure the explicitly invoked constructor `std::string(getName())` is required? It looks like this converts a `StringRef` into a temporary `std::string`, then makes a copy of the `std::string` and put the copy into the `QualifiedName.append` method. `QualifiedName.append(getName());` should be enough, shouldn't it?
> Way back the following upstream commits:
> ```
> commit adcd02683856c30ba6f349279509acecd90063df
> Author: Benjamin Kramer <benny.kra at googlemail.com>
> Date:   Tue Jan 28 20:23:46 2020 +0100
> 
>     Make llvm::StringRef to std::string conversions explicit.
>     
>     This is how it should've been and brings it more in line with
>     std::string_view. There should be no functional change here.
>     
>     This is mostly mechanical from a custom clang-tidy check, with a lot of
>     manual fixups. It uncovers a lot of minor inefficiencies.
>     
>     This doesn't actually modify StringRef yet, I'll do that in a follow-up.
> 
> commit 777180a32b61070a10dd330b4f038bf24e916af1
> Author: Benjamin Kramer <benny.kra at googlemail.com>
> Date:   Tue Jan 28 23:30:02 2020 +0100
> 
>     [ADT] Make StringRef's std::string conversion operator explicit
>     
>     This has the same behavior as converting std::string_view to
>     std::string. This is an expensive conversion, so explicit conversions
>     are helpful for avoiding unneccessary string copies.
> ```
> 
> Introduced the need to use the explicit conversion.
Hm... Sorry, I wasn't aware about this explicit conversion. Thank you for the explanation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125778