This is an archive of the discontinued LLVM Phabricator instance.

[lldb][NFCI] Change return type of PersistentExpressionState::GetNextPersistentVariableName
AcceptedPublic

Authored by bulbazord on Jun 6 2023, 5:40 PM.

Details

Summary

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.

Diff Detail

Event Timeline

bulbazord created this revision.Jun 6 2023, 5:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2023, 5:40 PM
bulbazord requested review of this revision.Jun 6 2023, 5:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2023, 5:40 PM

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

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! :)

JDevlieghere accepted this revision.Jun 7 2023, 10:57 PM

LGTM

lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp
129–134

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

This revision is now accepted and ready to land.Jun 7 2023, 10:57 PM
fdeazeve accepted this revision.Jun 8 2023, 3:40 AM
fdeazeve added inline comments.
lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp
129–134

+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)

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.

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...