This is an archive of the discontinued LLVM Phabricator instance.

Separate ClangExpressionVariable from ExpressionVariable
Needs ReviewPublic

Authored by spyffe on Sep 3 2015, 9:45 AM.

Details

Reviewers
jingham
Summary

This patch separates the generic portion of ClangExpressionVariable, which stores information about a variable that different parts of LLDB use, from the compiler-specific portion that only the expression parser cares about.

Diff Detail

Event Timeline

spyffe updated this revision to Diff 33954.Sep 3 2015, 9:45 AM
spyffe retitled this revision from to Separate ClangExpressionVariable from ExpressionVariable.
spyffe updated this object.
spyffe added a reviewer: jingham.
spyffe set the repository for this revision to rL LLVM.
spyffe added a subscriber: lldb-commits.
spyffe updated this revision to Diff 33986.Sep 3 2015, 4:02 PM
spyffe removed rL LLVM as the repository for this revision.

Removed ClangExpressionVariable from the friend list for ValueObject.

jingham requested changes to this revision.Sep 3 2015, 5:39 PM
jingham edited edge metadata.

In general this looks good. It seems to me that in almost all the uses of ClangExpressionVariable::CreateVariableInList, you actually only want the ClangExpressionVariable *, since you pretty much always turn around an get that out of the ExpressionVariableSP anyway. So it might be more convenient to have CreateVariableInList return a ClangExpressionVariable *, and if you find yourself needing the ExpressionVariableSP, then just have ExpressionVariable implement shared_from_this, and you can use that to resurrect the shared pointer.

This revision now requires changes to proceed.Sep 3 2015, 5:39 PM
spyffe updated this revision to Diff 34004.Sep 3 2015, 6:15 PM
spyffe edited edge metadata.

According to Jim's suggestions, made CreateVariableInList return a ClangExpressionVariable * rather than a ExpressionVariableSP. Also made ExpressionVariable be able to get a shared pointer from itself, so that the few cases that want to create a variable and get a shared pointer can do that correctly.

jingham requested changes to this revision.Sep 3 2015, 6:30 PM
jingham edited edge metadata.

Is there any reason ClangExpression::FindVariableInList doesn't work the same way as CreateVariableInList. It seems like for that one you also usually want the ClangExpressionVariable when you call it?

This revision now requires changes to proceed.Sep 3 2015, 6:30 PM

Working on that now. I have to be careful not just to stuff ClangExpressionVariable* into ExpressionVariableSP, but to use shared_from_this, otherwise I get crashes in the testsuite. I'll update this patch once I get that cleaned up.

spyffe updated this revision to Diff 34055.Sep 4 2015, 12:24 PM
spyffe edited edge metadata.

Applied Jim's suggestion that FindVariableInList return a ClangExpressionVariable *. Also fixed some places where we should have called shared_from_this instead of creating a new shared pointer.

That look good. There's one place (noted inline) where you are missing a newline at the end of a file. Fix that and it's good to go.

source/Expression/ExpressionVariable.cpp
32–33

Add missing newline.

spyffe updated this revision to Diff 34061.Sep 4 2015, 12:53 PM
spyffe edited edge metadata.

Added a newline to the end of ExpressionVariable.cpp