This extends the existing support for the allowed syntax of ClassNameReferences.
As a bonus, we get rid of one first/first conflict.
Details
Details
- Reviewers
brauch - Commits
- R52:cb5bf5f425c0: Fix ClassNameReferences.
Diff Detail
Diff Detail
- Repository
- R52 KDevelop: PHP Support
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Good amount of tests :) For the actual implementation, I would suggest trying to make it a bit more readable by a) splitting it up into several functions and b) trying to reduce indent depth a bit by using continue instead of nested ifs.
| duchain/expressionvisitor.cpp | ||
|---|---|---|
| 717 | This is a really long function. Can you break it up a bit to make it clearer what's going on? | |
| 732 | Is it intended that you use a null type pointer here? | |
| 776 | here with a bit of if (!declaration) { continue; } I think the code could be much more readable. This has too much indent for my taste. | |
| duchain/expressionvisitor.cpp | ||
|---|---|---|
| 732 | No, the entire else statement seems redundant. Unit tests still pass without it and my test files render fine with it too. Probably a left-over artifact from an earlier draft. Removed. | |
Comment Actions
I'm not really capable of doing an in-depth review here, since I don't know enough details of PHP nowadays. Style-wise I think it could still profit from reducing the block size of un-named (i.e. not a function with a name), uncommented complex code a bit, since it often requires quite some thinking to grasp what a certain conditional actually checks for. A simple comment like "// not a function" or whatever can do wonders there.
Feel free to merge this as-is or with a bit more comments, better get it in before the beta and still have a fix window before the release than merge it after the beta. Thanks for keeping kdev-php alive!