Fix syntax support for dynamic member access.
ClosedPublic

Authored by pprkut on Oct 27 2018, 3:43 PM.

Details

Summary

Fix syntax support for dynamic member access.

Covers syntax like:

  • A::${$foo}
  • A::$var::${$foo}
  • A::$var->${$foo}

BUG: 400294
FIXED-IN: 5.3.1

Diff Detail

Repository
R52 KDevelop: PHP Support
Branch
variableExprMember_v5.3
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 4282
Build 4300: arc lint + arc unit
pprkut created this revision.Oct 27 2018, 3:43 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptOct 27 2018, 3:43 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
pprkut requested review of this revision.Oct 27 2018, 3:43 PM
kfunk requested changes to this revision.Oct 28 2018, 5:39 PM
kfunk added a subscriber: kfunk.
kfunk added inline comments.
duchain/expressionvisitor.cpp
725

You already dereference node->staticProperty->staticProperty before in line 700; so this if-stmt would be always true(?). Doesn't make sense to me.

This revision now requires changes to proceed.Oct 28 2018, 5:39 PM
pprkut updated this revision to Diff 44377.Oct 28 2018, 5:58 PM

Restructure if-statements in visitStaticMember.
Remove commented code from unit tests.

pprkut marked an inline comment as done.Oct 28 2018, 6:00 PM
pprkut added inline comments.
duchain/expressionvisitor.cpp
725

You're right. I restructured it to make things clearer.

kfunk added inline comments.Oct 28 2018, 6:09 PM
duchain/expressionvisitor.cpp
737

Same here

790

And probably needs a check against ...->staticProperty->staticProperty, too?

820–821

Same here(?)

pprkut updated this revision to Diff 44379.Oct 28 2018, 7:02 PM
pprkut marked an inline comment as done.

Add more checks to the if-statements

pprkut marked 3 inline comments as done.Oct 28 2018, 7:04 PM
pprkut added inline comments.
duchain/expressionvisitor.cpp
790

With the current grammer, if there's staticProperty, there's also a staticProperty->staticProperty, so conceptually we don't need to check for both right now. However, the grammar can change, so it's probably good to be safe.

kfunk added a comment.Nov 16 2018, 2:14 PM

I wonder if a couple of auto* element = it->element, auto* staticProperty = it->element->staticProperty sprayed throughout the code base would help making all of this a bit more readable...

duchain/tests/uses.cpp
1423

Minor, for future: you can use C++11-style initialization here: QList<RangeInRevision>{...}

More TODO: Should be a QVector<RangeInRevision> for performance reasons.

kfunk accepted this revision.Nov 16 2018, 2:14 PM

LGTM as-is

This revision is now accepted and ready to land.Nov 16 2018, 2:14 PM
This revision was automatically updated to reflect the committed changes.
pprkut marked an inline comment as done.