[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

I think this was just fixed by 48677f58b06cfb8715902173c5bc3d1764d7c8c6 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[Jan Svoboda via Phabricator via lldb-commits]

jansvoboda11 added a comment.

This breaks our build: 
https://green.lab.llvm.org/green/job/lldb-cmake/37529/console
Can you take a look? If the fix is not obvious, please revert for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdfd499a61c45: [lldb][NFC] avoid unnecessary roundtrips 
between different string types (authored by xujuntwt95329, committed by Walter 
Erquinigo wall...@fb.com).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

Files:
  lldb/include/lldb/Target/PathMappingList.h
  lldb/source/API/SBTarget.cpp
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Core/Module.cpp
  lldb/source/Core/ModuleList.cpp
  lldb/source/Interpreter/OptionValuePathMappings.cpp
  lldb/source/Target/PathMappingList.cpp
  lldb/unittests/Target/FindFileTest.cpp
  lldb/unittests/Target/PathMappingListTest.cpp

Index: lldb/unittests/Target/PathMappingListTest.cpp
===
--- lldb/unittests/Target/PathMappingListTest.cpp
+++ lldb/unittests/Target/PathMappingListTest.cpp
@@ -66,16 +66,16 @@
 #endif
   };
   PathMappingList map;
-  map.Append(ConstString("."), ConstString("/tmp"), false);
+  map.Append(".", "/tmp", false);
   TestPathMappings(map, matches, fails);
   PathMappingList map2;
-  map2.Append(ConstString(""), ConstString("/tmp"), false);
+  map2.Append("", "/tmp", false);
   TestPathMappings(map, matches, fails);
 }
 
 TEST(PathMappingListTest, AbsoluteTests) {
   PathMappingList map;
-  map.Append(ConstString("/old"), ConstString("/new"), false);
+  map.Append("/old", "/new", false);
   Matches matches[] = {
 {"/old", "/new"},
 {"/old/", "/new"},
@@ -97,7 +97,7 @@
 
 TEST(PathMappingListTest, RemapRoot) {
   PathMappingList map;
-  map.Append(ConstString("/"), ConstString("/new"), false);
+  map.Append("/", "/new", false);
   Matches matches[] = {
 {"/old", "/new/old"},
 {"/old/", "/new/old"},
@@ -118,7 +118,7 @@
 #ifndef _WIN32
 TEST(PathMappingListTest, CrossPlatformTests) {
   PathMappingList map;
-  map.Append(ConstString(R"(C:\old)"), ConstString("/new"), false);
+  map.Append(R"(C:\old)", "/new", false);
   Matches matches[] = {
 {R"(C:\old)", llvm::sys::path::Style::windows, "/new"},
 {R"(C:\old\)", llvm::sys::path::Style::windows, "/new"},
Index: lldb/unittests/Target/FindFileTest.cpp
===
--- lldb/unittests/Target/FindFileTest.cpp
+++ lldb/unittests/Target/FindFileTest.cpp
@@ -75,8 +75,8 @@
   llvm::FileRemover file_remover(FileName);
   PathMappingList map;
 
-  map.Append(ConstString("/old"), ConstString(DirName.str()), false);
-  map.Append(ConstString(R"(C:\foo)"), ConstString(DirName.str()), false);
+  map.Append("/old", DirName.str(), false);
+  map.Append(R"(C:\foo)", DirName.str(), false);
 
   Matches matches[] = {
   {"/old", llvm::sys::path::Style::posix, DirName.c_str()},
Index: lldb/source/Target/PathMappingList.cpp
===
--- lldb/source/Target/PathMappingList.cpp
+++ lldb/source/Target/PathMappingList.cpp
@@ -30,11 +30,11 @@
   // with the raw path pair, which doesn't work anymore because the paths have
   // been normalized when the debug info was loaded. So we need to store
   // nomalized path pairs to ensure things match up.
-  ConstString NormalizePath(ConstString path) {
-// If we use "path" to construct a FileSpec, it will normalize the path for
-// us. We then grab the string and turn it back into a ConstString.
-return ConstString(FileSpec(path.GetStringRef()).GetPath());
-  }
+std::string NormalizePath(llvm::StringRef path) {
+  // If we use "path" to construct a FileSpec, it will normalize the path for
+  // us. We then grab the string.
+  return FileSpec(path).GetPath();
+}
 }
 // PathMappingList constructor
 PathMappingList::PathMappingList() : m_pairs() {}
@@ -59,8 +59,8 @@
 
 PathMappingList::~PathMappingList() = default;
 
-void PathMappingList::Append(ConstString path,
- ConstString replacement, bool notify) {
+void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement,
+ bool notify) {
   ++m_mod_id;
   m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement)));
   if (notify && m_callback)
@@ -78,9 +78,8 @@
   }
 }
 
-void PathMappingList::Insert(ConstString path,
- ConstString replacement, uint32_t index,
- bool notify) {
+void PathMappingList::Insert(llvm::StringRef path, llvm::StringRef replacement,
+ uint32_t index, bool notify) {
   ++m_mod_id;
   iterator insert_iter;
   if (index >= m_pairs.size())
@@ -93,9 +92,8 @@
 m_callback(*this, m_callback_baton);
 }
 
-bool PathMappingList::Replace(ConstString path,
-  ConstString replacement, uint32_t index,
-  bool notify) {
+bool 

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 added a comment.

@wallace rebased

But I guess you should apply this patch firstly?
https://reviews.llvm.org/D112439


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 updated this revision to Diff 383965.
xujuntwt95329 added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

Files:
  lldb/include/lldb/Target/PathMappingList.h
  lldb/source/API/SBTarget.cpp
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Core/Module.cpp
  lldb/source/Core/ModuleList.cpp
  lldb/source/Interpreter/OptionValuePathMappings.cpp
  lldb/source/Target/PathMappingList.cpp
  lldb/unittests/Target/FindFileTest.cpp
  lldb/unittests/Target/PathMappingListTest.cpp

Index: lldb/unittests/Target/PathMappingListTest.cpp
===
--- lldb/unittests/Target/PathMappingListTest.cpp
+++ lldb/unittests/Target/PathMappingListTest.cpp
@@ -66,16 +66,16 @@
 #endif
   };
   PathMappingList map;
-  map.Append(ConstString("."), ConstString("/tmp"), false);
+  map.Append(".", "/tmp", false);
   TestPathMappings(map, matches, fails);
   PathMappingList map2;
-  map2.Append(ConstString(""), ConstString("/tmp"), false);
+  map2.Append("", "/tmp", false);
   TestPathMappings(map, matches, fails);
 }
 
 TEST(PathMappingListTest, AbsoluteTests) {
   PathMappingList map;
-  map.Append(ConstString("/old"), ConstString("/new"), false);
+  map.Append("/old", "/new", false);
   Matches matches[] = {
 {"/old", "/new"},
 {"/old/", "/new"},
@@ -97,7 +97,7 @@
 
 TEST(PathMappingListTest, RemapRoot) {
   PathMappingList map;
-  map.Append(ConstString("/"), ConstString("/new"), false);
+  map.Append("/", "/new", false);
   Matches matches[] = {
 {"/old", "/new/old"},
 {"/old/", "/new/old"},
@@ -118,7 +118,7 @@
 #ifndef _WIN32
 TEST(PathMappingListTest, CrossPlatformTests) {
   PathMappingList map;
-  map.Append(ConstString(R"(C:\old)"), ConstString("/new"), false);
+  map.Append(R"(C:\old)", "/new", false);
   Matches matches[] = {
 {R"(C:\old)", llvm::sys::path::Style::windows, "/new"},
 {R"(C:\old\)", llvm::sys::path::Style::windows, "/new"},
Index: lldb/unittests/Target/FindFileTest.cpp
===
--- lldb/unittests/Target/FindFileTest.cpp
+++ lldb/unittests/Target/FindFileTest.cpp
@@ -75,8 +75,8 @@
   llvm::FileRemover file_remover(FileName);
   PathMappingList map;
 
-  map.Append(ConstString("/old"), ConstString(DirName.str()), false);
-  map.Append(ConstString(R"(C:\foo)"), ConstString(DirName.str()), false);
+  map.Append("/old", DirName.str(), false);
+  map.Append(R"(C:\foo)", DirName.str(), false);
 
   Matches matches[] = {
   {"/old", llvm::sys::path::Style::posix, DirName.c_str()},
Index: lldb/source/Target/PathMappingList.cpp
===
--- lldb/source/Target/PathMappingList.cpp
+++ lldb/source/Target/PathMappingList.cpp
@@ -30,11 +30,11 @@
   // with the raw path pair, which doesn't work anymore because the paths have
   // been normalized when the debug info was loaded. So we need to store
   // nomalized path pairs to ensure things match up.
-  ConstString NormalizePath(ConstString path) {
-// If we use "path" to construct a FileSpec, it will normalize the path for
-// us. We then grab the string and turn it back into a ConstString.
-return ConstString(FileSpec(path.GetStringRef()).GetPath());
-  }
+std::string NormalizePath(llvm::StringRef path) {
+  // If we use "path" to construct a FileSpec, it will normalize the path for
+  // us. We then grab the string.
+  return FileSpec(path).GetPath();
+}
 }
 // PathMappingList constructor
 PathMappingList::PathMappingList() : m_pairs() {}
@@ -59,8 +59,8 @@
 
 PathMappingList::~PathMappingList() = default;
 
-void PathMappingList::Append(ConstString path,
- ConstString replacement, bool notify) {
+void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement,
+ bool notify) {
   ++m_mod_id;
   m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement)));
   if (notify && m_callback)
@@ -78,9 +78,8 @@
   }
 }
 
-void PathMappingList::Insert(ConstString path,
- ConstString replacement, uint32_t index,
- bool notify) {
+void PathMappingList::Insert(llvm::StringRef path, llvm::StringRef replacement,
+ uint32_t index, bool notify) {
   ++m_mod_id;
   iterator insert_iter;
   if (index >= m_pairs.size())
@@ -93,9 +92,8 @@
 m_callback(*this, m_callback_baton);
 }
 
-bool PathMappingList::Replace(ConstString path,
-  ConstString replacement, uint32_t index,
-  bool notify) {
+bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef replacement,
+  uint32_t index, bool notify) {
   if (index >= m_pairs.size())
 return false;
   ++m_mod_id;
@@ -221,20 +219,19 @@
   // 

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

could you rebase this on top of the main branch? I tried to 'arc patch' it 
locally and there are a few merge issues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[Adrian Prantl via Phabricator via lldb-commits]

aprantl accepted this revision.
aprantl added a comment.

Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 added a comment.

In D112863#3098706 , @wallace wrote:

> @xujuntwt95329 do you have push permissions or do you need help to land this 
> diff?

I don't have push permissions, could you please help me to land this?

Thanks a lot!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

@xujuntwt95329 do you have push permissions or do you need help to land this 
diff?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 marked 2 inline comments as done.
xujuntwt95329 added a comment.

@dblaikie Thanks for your detailed comments, I've uploaded the new patch-set


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 updated this revision to Diff 383559.
xujuntwt95329 added a comment.

address dblaikie's comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

Files:
  lldb/include/lldb/Target/PathMappingList.h
  lldb/source/API/SBTarget.cpp
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Core/Module.cpp
  lldb/source/Core/ModuleList.cpp
  lldb/source/Interpreter/OptionValuePathMappings.cpp
  lldb/source/Target/PathMappingList.cpp
  lldb/unittests/Target/FindFileTest.cpp
  lldb/unittests/Target/PathMappingListTest.cpp

Index: lldb/unittests/Target/PathMappingListTest.cpp
===
--- lldb/unittests/Target/PathMappingListTest.cpp
+++ lldb/unittests/Target/PathMappingListTest.cpp
@@ -66,16 +66,16 @@
 #endif
   };
   PathMappingList map;
-  map.Append(ConstString("."), ConstString("/tmp"), false);
+  map.Append(".", "/tmp", false);
   TestPathMappings(map, matches, fails);
   PathMappingList map2;
-  map2.Append(ConstString(""), ConstString("/tmp"), false);
+  map2.Append("", "/tmp", false);
   TestPathMappings(map, matches, fails);
 }
 
 TEST(PathMappingListTest, AbsoluteTests) {
   PathMappingList map;
-  map.Append(ConstString("/old"), ConstString("/new"), false);
+  map.Append("/old", "/new", false);
   Matches matches[] = {
 {"/old", "/new"},
 {"/old/", "/new"},
@@ -97,7 +97,7 @@
 
 TEST(PathMappingListTest, RemapRoot) {
   PathMappingList map;
-  map.Append(ConstString("/"), ConstString("/new"), false);
+  map.Append("/", "/new", false);
   Matches matches[] = {
 {"/old", "/new/old"},
 {"/old/", "/new/old"},
@@ -118,7 +118,7 @@
 #ifndef _WIN32
 TEST(PathMappingListTest, CrossPlatformTests) {
   PathMappingList map;
-  map.Append(ConstString(R"(C:\old)"), ConstString("/new"), false);
+  map.Append(R"(C:\old)", "/new", false);
   Matches matches[] = {
 {R"(C:\old)", llvm::sys::path::Style::windows, "/new"},
 {R"(C:\old\)", llvm::sys::path::Style::windows, "/new"},
Index: lldb/unittests/Target/FindFileTest.cpp
===
--- lldb/unittests/Target/FindFileTest.cpp
+++ lldb/unittests/Target/FindFileTest.cpp
@@ -75,8 +75,8 @@
   llvm::FileRemover file_remover(FileName);
   PathMappingList map;
 
-  map.Append(ConstString("/old"), ConstString(DirName.str()), false);
-  map.Append(ConstString(R"(C:\foo)"), ConstString(DirName.str()), false);
+  map.Append("/old", DirName.str(), false);
+  map.Append(R"(C:\foo)", DirName.str(), false);
 
   Matches matches[] = {
   {"/old", llvm::sys::path::Style::posix, DirName.c_str()},
Index: lldb/source/Target/PathMappingList.cpp
===
--- lldb/source/Target/PathMappingList.cpp
+++ lldb/source/Target/PathMappingList.cpp
@@ -30,11 +30,11 @@
   // with the raw path pair, which doesn't work anymore because the paths have
   // been normalized when the debug info was loaded. So we need to store
   // nomalized path pairs to ensure things match up.
-  ConstString NormalizePath(ConstString path) {
-// If we use "path" to construct a FileSpec, it will normalize the path for
-// us. We then grab the string and turn it back into a ConstString.
-return ConstString(FileSpec(path.GetStringRef()).GetPath());
-  }
+std::string NormalizePath(llvm::StringRef path) {
+  // If we use "path" to construct a FileSpec, it will normalize the path for
+  // us. We then grab the string.
+  return FileSpec(path).GetPath();
+}
 }
 // PathMappingList constructor
 PathMappingList::PathMappingList() : m_pairs() {}
@@ -59,8 +59,8 @@
 
 PathMappingList::~PathMappingList() = default;
 
-void PathMappingList::Append(ConstString path,
- ConstString replacement, bool notify) {
+void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement,
+ bool notify) {
   ++m_mod_id;
   m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement)));
   if (notify && m_callback)
@@ -78,9 +78,8 @@
   }
 }
 
-void PathMappingList::Insert(ConstString path,
- ConstString replacement, uint32_t index,
- bool notify) {
+void PathMappingList::Insert(llvm::StringRef path, llvm::StringRef replacement,
+ uint32_t index, bool notify) {
   ++m_mod_id;
   iterator insert_iter;
   if (index >= m_pairs.size())
@@ -93,9 +92,8 @@
 m_callback(*this, m_callback_baton);
 }
 
-bool PathMappingList::Replace(ConstString path,
-  ConstString replacement, uint32_t index,
-  bool notify) {
+bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef replacement,
+  uint32_t index, bool notify) {
   if (index >= m_pairs.size())
 return false;
   ++m_mod_id;
@@ 

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.



Comment at: lldb/source/API/SBTarget.cpp:1589
 
-  const ConstString csFrom(from), csTo(to);
-  if (!csFrom)
+  llvm::StringRef srFrom(from), srTo(to);
+  if (srFrom.empty())

Aside (since the old code did this too): Generally code should prefer `=` 
initialization over `()` initialization - because the former can't invoke 
explicit conversions, so is easier to read because it limits the possible 
conversion to simpler/safer ones (for instance, with unique ptrs 
"std::unique_ptr p = x();" tells me `x()` returns a unique_ptr or 
equivalent, but `std::unique_ptr p(x());` could be that `x()` returns a 
raw pointer and then I have to wonder if it meant to transfer ownership or not 
- but I don't have to wonder that in the first example because the type system 
checks that for me).

Admittedly StringRef has no explicit ctors - but it's a good general style to 
use imho (would be happy to include this in the LLVM style guide if this seems 
contentious in any way & is worth some discussion/formalization - but I think 
it's just generally good C++ practice & hopefully can be accepted as such)



Comment at: lldb/source/Target/PathMappingList.cpp:35
+  // If we use "path" to construct a FileSpec, it will normalize the path for
+  // us. We then grab the string and turn it back into a ConstString.
+  return FileSpec(path).GetPath();

out of date comment - maybe this comment isn't pulling its weight? The content 
seems fairly simple/probably self explanatory?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[walter erquinigo via Phabricator via lldb-commits]

wallace accepted this revision.
wallace added a comment.
This revision is now accepted and ready to land.

thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112863

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

[Xu Jun via Phabricator via lldb-commits]

xujuntwt95329 created this revision.
xujuntwt95329 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The amount of roundtrips between StringRefs, ConstStrings and std::strings is 
getting a bit out of hand, this patch avoid the unnecessary roundtrips.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112863

Files:
  lldb/include/lldb/Target/PathMappingList.h
  lldb/source/API/SBTarget.cpp
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Core/Module.cpp
  lldb/source/Core/ModuleList.cpp
  lldb/source/Interpreter/OptionValuePathMappings.cpp
  lldb/source/Target/PathMappingList.cpp
  lldb/unittests/Target/FindFileTest.cpp
  lldb/unittests/Target/PathMappingListTest.cpp

Index: lldb/unittests/Target/PathMappingListTest.cpp
===
--- lldb/unittests/Target/PathMappingListTest.cpp
+++ lldb/unittests/Target/PathMappingListTest.cpp
@@ -66,16 +66,16 @@
 #endif
   };
   PathMappingList map;
-  map.Append(ConstString("."), ConstString("/tmp"), false);
+  map.Append(".", "/tmp", false);
   TestPathMappings(map, matches, fails);
   PathMappingList map2;
-  map2.Append(ConstString(""), ConstString("/tmp"), false);
+  map2.Append("", "/tmp", false);
   TestPathMappings(map, matches, fails);
 }
 
 TEST(PathMappingListTest, AbsoluteTests) {
   PathMappingList map;
-  map.Append(ConstString("/old"), ConstString("/new"), false);
+  map.Append("/old", "/new", false);
   Matches matches[] = {
 {"/old", "/new"},
 {"/old/", "/new"},
@@ -97,7 +97,7 @@
 
 TEST(PathMappingListTest, RemapRoot) {
   PathMappingList map;
-  map.Append(ConstString("/"), ConstString("/new"), false);
+  map.Append("/", "/new", false);
   Matches matches[] = {
 {"/old", "/new/old"},
 {"/old/", "/new/old"},
@@ -118,7 +118,7 @@
 #ifndef _WIN32
 TEST(PathMappingListTest, CrossPlatformTests) {
   PathMappingList map;
-  map.Append(ConstString(R"(C:\old)"), ConstString("/new"), false);
+  map.Append(R"(C:\old)", "/new", false);
   Matches matches[] = {
 {R"(C:\old)", llvm::sys::path::Style::windows, "/new"},
 {R"(C:\old\)", llvm::sys::path::Style::windows, "/new"},
Index: lldb/unittests/Target/FindFileTest.cpp
===
--- lldb/unittests/Target/FindFileTest.cpp
+++ lldb/unittests/Target/FindFileTest.cpp
@@ -75,8 +75,8 @@
   llvm::FileRemover file_remover(FileName);
   PathMappingList map;
 
-  map.Append(ConstString("/old"), ConstString(DirName.str()), false);
-  map.Append(ConstString(R"(C:\foo)"), ConstString(DirName.str()), false);
+  map.Append("/old", DirName.str(), false);
+  map.Append(R"(C:\foo)", DirName.str(), false);
 
   Matches matches[] = {
   {"/old", llvm::sys::path::Style::posix, DirName.c_str()},
Index: lldb/source/Target/PathMappingList.cpp
===
--- lldb/source/Target/PathMappingList.cpp
+++ lldb/source/Target/PathMappingList.cpp
@@ -30,11 +30,11 @@
   // with the raw path pair, which doesn't work anymore because the paths have
   // been normalized when the debug info was loaded. So we need to store
   // nomalized path pairs to ensure things match up.
-  ConstString NormalizePath(ConstString path) {
-// If we use "path" to construct a FileSpec, it will normalize the path for
-// us. We then grab the string and turn it back into a ConstString.
-return ConstString(FileSpec(path.GetStringRef()).GetPath());
-  }
+std::string NormalizePath(llvm::StringRef path) {
+  // If we use "path" to construct a FileSpec, it will normalize the path for
+  // us. We then grab the string and turn it back into a ConstString.
+  return FileSpec(path).GetPath();
+}
 }
 // PathMappingList constructor
 PathMappingList::PathMappingList() : m_pairs() {}
@@ -59,8 +59,8 @@
 
 PathMappingList::~PathMappingList() = default;
 
-void PathMappingList::Append(ConstString path,
- ConstString replacement, bool notify) {
+void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement,
+ bool notify) {
   ++m_mod_id;
   m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement)));
   if (notify && m_callback)
@@ -78,9 +78,8 @@
   }
 }
 
-void PathMappingList::Insert(ConstString path,
- ConstString replacement, uint32_t index,
- bool notify) {
+void PathMappingList::Insert(llvm::StringRef path, llvm::StringRef replacement,
+ uint32_t index, bool notify) {
   ++m_mod_id;
   iterator insert_iter;
   if (index >= m_pairs.size())
@@ -93,9 +92,8 @@
 m_callback(*this, m_callback_baton);
 }
 
-bool PathMappingList::Replace(ConstString path,
-  ConstString replacement, uint32_t index,
-  bool notify) {
+bool PathMappingList::Replace(llvm::StringRef path,