Page MenuHomePhabricator

Fixed ValueObject::GetExpressionPath() for paths including anonymous struct/union
ClosedPublic

Authored by mamai on Mar 9 2016, 11:43 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

mamai updated this revision to Diff 50168.Mar 9 2016, 11:43 AM
mamai retitled this revision from to Fixed ValueObject::GetExpressionPath() for paths including anonymous struct/union.
mamai updated this object.
mamai added a reviewer: clayborg.
mamai set the repository for this revision to rL LLVM.
mamai added a subscriber: lldb-commits.
mamai updated this revision to Diff 50172.Mar 9 2016, 11:55 AM

Fixed test header, removed useless include.

The source change looks fine, but Greg's the owner of this code so I'll let him say yes for sure.

But don't call the test case "ExpressionPath". The expression parser is different from frame variable. I understand that your intent was "the expression you feed to frame variable" but since we have an "expression" command, and the expressions you feed to it are different from those you feed to "frame variable" overloading the word is only going to cause confusion.

mamai added a comment.Mar 9 2016, 12:11 PM

Sure. Would you have a suggestion of how to call it? Maybe something like variable_flat since it is the command used? Also, is it the right place to add it in functionalities or should it be elsewhere?

There's actually already a test for handling C anonymous unions in lang/c/anonymous. That tests the expression parser and some direct SBValue queries at present. It would be appropriate to add your test as another test case in here.

The part of frame variable you are testing here - namely the parsing of these "frame variable" paths, can also be directly accessed with SBValue::GetValueForExpressionPath (aargh, shouldn't have called that "expression" should we? Too late to change now.) Anyway, you can also use that to test more directly if you want.

ldrumm added a subscriber: ldrumm.Mar 9 2016, 12:22 PM

Hi all

This differential may also be somewhat relevant / of interest, although it deals only with typedefs of untagged struct types.

Best

Luke

clayborg requested changes to this revision.Mar 9 2016, 12:51 PM
clayborg edited edge metadata.

See inlined comments.

packages/Python/lldbsuite/test/functionalities/expression_path/TestExpressionPath.py
44–46 ↗(On Diff #50172)

Shouldn't we also see "s.r[0] = 1" and "s.r[1] = 2" in response to "frame variable s --flat"?

47 ↗(On Diff #50172)

Add newline at end of file

packages/Python/lldbsuite/test/functionalities/expression_path/main.cpp
27 ↗(On Diff #50172)

Add newline at end of file

This revision now requires changes to proceed.Mar 9 2016, 12:51 PM
mamai updated this revision to Diff 50192.Mar 9 2016, 1:54 PM
mamai edited edge metadata.

Replaced the test by a new case in lang/c/anonymous test, as suggested by Jim. Is this correct?

clayborg accepted this revision.Mar 9 2016, 2:01 PM
clayborg edited edge metadata.

Looks fine to me.

This revision is now accepted and ready to land.Mar 9 2016, 2:01 PM
mamai added a comment.Mar 9 2016, 2:06 PM

Thanks! Could someone commit it for me? I don't have commit access.

This revision was automatically updated to reflect the committed changes.