[Lldb-commits] [PATCH] D152324: [lldb][NFCI] Change return type of PersistentExpressionState::GetNextPersistentVariableName

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D152324#4415324 , @jingham wrote:

> I wonder about this one.  In every instance where the API is used, its result 
> is turned into a ConstString first.  That's because this variable name lives 
> in the same slot as normal variable names, which come from the debug 
> information and so tend to be in the ConstString pool for better reasons.  Do 
> you project being able to get rid of that latter requirement?  If not, it 
> seems a bit odd to go to the trouble to avoid this value starting life as a 
> ConstString when the first thing everybody does with it is to turn it into a 
> ConstString.

That's the major reason I'm sitting on this right now. I want to gather some 
more evidence and test things more before deciding this is the direction to go 
in. I'm pretty sure `ConstString` in its current form is the wrong abstraction 
for this, but perhaps some kind of variation of `ConstString` is the right 
call? We'll see...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152324

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

[Lldb-commits] [PATCH] D152324: [lldb][NFCI] Change return type of PersistentExpressionState::GetNextPersistentVariableName

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I wonder about this one.  In every instance where the API is used, its result 
is turned into a ConstString first.  That's because this variable name lives in 
the same slot as normal variable names, which come from the debug information 
and so tend to be in the ConstString pool for better reasons.  Do you project 
being able to get rid of that latter requirement?  If not, it seems a bit odd 
to go to the trouble to avoid this value starting life as a ConstString when 
the first thing everybody does with it is to turn it into a ConstString.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152324

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

[Lldb-commits] [PATCH] D152324: [lldb][NFCI] Change return type of PersistentExpressionState::GetNextPersistentVariableName

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve accepted this revision.
fdeazeve added inline comments.



Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp:129-134
   llvm::SmallString<64> name;
   {
 llvm::raw_svector_ostream os(name);
 os << GetPersistentVariablePrefix(is_error)
<< m_next_persistent_variable_id++;
   }

JDevlieghere wrote:
> If we're going to return a `std::string`, we might as well use a 
> `raw_string_ostream` and simplify this function. 
+1, we can avoid the conversion of SmallString->std::string (and enable copy 
elision) by declaring `name` as std::string

(we don't necessarily need to do it in this patch though, either way is fine 
IMO)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152324

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

[Lldb-commits] [PATCH] D152324: [lldb][NFCI] Change return type of PersistentExpressionState::GetNextPersistentVariableName

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM




Comment at: 
lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp:129-134
   llvm::SmallString<64> name;
   {
 llvm::raw_svector_ostream os(name);
 os << GetPersistentVariablePrefix(is_error)
<< m_next_persistent_variable_id++;
   }

If we're going to return a `std::string`, we might as well use a 
`raw_string_ostream` and simplify this function. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152324

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

[Lldb-commits] [PATCH] D152324: [lldb][NFCI] Change return type of PersistentExpressionState::GetNextPersistentVariableName

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D152324#4403368 , @aprantl wrote:

> Is the idea to remove the ConstString from these APIs too? If yes, this LGTM, 
> otherwise it seems to needlessly complicate things.

Yes, I was going to remove ConstString from these other APIs too. I can let 
this patch sit here until I can do more work to make sure that happens though. 
I'm somewhat confident that removing ConstString from this area of LLDB is 
going to be better long-term, but I've been wrong about this before! :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152324

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

[Lldb-commits] [PATCH] D152324: [lldb][NFCI] Change return type of PersistentExpressionState::GetNextPersistentVariableName

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Is the idea to remove the ConstString from these APIs too? If yes, this LGTM, 
otherwise it seems to needlessly complicate things.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152324

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

[Lldb-commits] [PATCH] D152324: [lldb][NFCI] Change return type of PersistentExpressionState::GetNextPersistentVariableName

[Alex Langford via Phabricator via lldb-commits]

bulbazord created this revision.
bulbazord added reviewers: JDevlieghere, aprantl, Michael137, fdeazeve.
Herald added a project: All.
bulbazord requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

These don't really need to be in the ConstString StringPool. Note that
they still end up there because we need to wrap them in ConstStrings to
use them with certain APIs, but we can stop creating them as
ConstStrings to start.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152324

Files:
  lldb/include/lldb/Expression/ExpressionVariable.h
  lldb/source/Core/ValueObject.cpp
  lldb/source/Expression/Materializer.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
  lldb/source/Target/ABI.cpp

Index: lldb/source/Target/ABI.cpp
===
--- lldb/source/Target/ABI.cpp
+++ lldb/source/Target/ABI.cpp
@@ -93,8 +93,8 @@
 if (!persistent_expression_state)
   return {};
 
-ConstString persistent_variable_name =
-persistent_expression_state->GetNextPersistentVariableName();
+ConstString persistent_variable_name = ConstString(
+persistent_expression_state->GetNextPersistentVariableName());
 
 lldb::ValueObjectSP const_valobj_sp;
 
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
@@ -1015,7 +1015,7 @@
 }
 
 ConstString ClangUserExpression::ResultDelegate::GetName() {
-  return m_persistent_state->GetNextPersistentVariableName(false);
+  return ConstString(m_persistent_state->GetNextPersistentVariableName(false));
 }
 
 void ClangUserExpression::ResultDelegate::DidDematerialize(
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
@@ -55,7 +55,7 @@
 
   void RemovePersistentVariable(lldb::ExpressionVariableSP variable) override;
 
-  ConstString GetNextPersistentVariableName(bool is_error = false) override;
+  std::string GetNextPersistentVariableName(bool is_error = false) override;
 
   /// Returns the next file name that should be used for user expressions.
   std::string GetNextExprFileName() {
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp
@@ -124,7 +124,7 @@
   return m_modules_decl_vendor_sp;
 }
 
-ConstString
+std::string
 ClangPersistentVariables::GetNextPersistentVariableName(bool is_error) {
   llvm::SmallString<64> name;
   {
@@ -132,5 +132,5 @@
 os << GetPersistentVariablePrefix(is_error)
<< m_next_persistent_variable_id++;
   }
-  return ConstString(name);
+  return std::string(name);
 }
Index: lldb/source/Expression/Materializer.cpp
===
--- lldb/source/Expression/Materializer.cpp
+++ lldb/source/Expression/Materializer.cpp
@@ -1041,9 +1041,10 @@
   return;
 }
 
-ConstString name = m_delegate
-   ? m_delegate->GetName()
-   : persistent_state->GetNextPersistentVariableName();
+ConstString name =
+m_delegate
+? m_delegate->GetName()
+: ConstString(persistent_state->GetNextPersistentVariableName());
 
 lldb::ExpressionVariableSP ret = persistent_state->CreatePersistentVariable(
 exe_scope, name, m_type, map.GetByteOrder(), map.GetAddressByteSize());
Index: lldb/source/Core/ValueObject.cpp
===
--- lldb/source/Core/ValueObject.cpp
+++ lldb/source/Core/ValueObject.cpp
@@ -3143,10 +3143,10 @@
   if (!persistent_state)
 return nullptr;
 
-  ConstString name = persistent_state->GetNextPersistentVariableName();
+  const std::string name = persistent_state->GetNextPersistentVariableName();
 
-  ValueObjectSP const_result_sp =
-  ValueObjectConstResult::Create(target_sp.get(), GetValue(), name);
+  ValueObjectSP const_result_sp = ValueObjectConstResult::Create(
+  target_sp.get(), GetValue(), ConstString(name));
 
   ExpressionVariableSP persistent_var_sp =
   persistent_state->CreatePersistentVariable(const_result_sp);
Index: lldb/include/lldb/Expression/ExpressionVariable.h