Already have a commit adapting to kdevplatform changes in kdevelop.git (not intrusive)
Details
Details
- Reviewers
mwolff - Group Reviewers
KDevelop - Commits
- R32:52cd6937057e: Pimpl DUChain navigation classes
R33:52cd6937057e: Pimpl DUChain navigation classes
Diff Detail
Diff Detail
- Repository
- R33 KDevPlatform
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
big +1 from my side in general, but there are some minor issues with this patch as-is.
also note: only commit to master, not to 5.0, as this breaks the ABI.
| language/duchain/navigation/abstractdeclarationnavigationcontext.h | ||
|---|---|---|
| 38–40 | while at it: please const& the DeclarationPointer and the TopDUContextPointer. | |
| 39 | override | |
| language/duchain/navigation/abstractnavigationcontext.cpp | ||
| 43 | QVector? | |
| 55 | QHash, also below? | |
| language/duchain/navigation/abstractnavigationcontext.h | ||
| 64 | const TopDUContextPointer& topContext = {} | |
| 86 | is that really nowhere overwritten, like in kdevphp or ruby or such? | |
| 105 | const& | |
| 107 | const& | |
| language/duchain/navigation/abstractnavigationwidget.h | ||
| 79 | const& | |
Comment Actions
That's *why* I'd like to have it in 5.0, let's break the ABI *before* the 5.x cycle.
Will fix the other remarks.
Comment Actions
Thing is, we already published a first beta release and in all previous releases that marked the ABI freeze, thus we should not break the ABI now.
Comment Actions
We're not really strict about our ABI guarantees anyway, are we...
TBH, it'll cause more harm than good when I commit this to master instead of 5.0. More difficulties to switch between 5.0 / master versions. I'd rather like to keep 5.x series almost, or, if possible, completely API/ABI-break-free.
I'd also fix all plugins using this API in their resp. 5.0 branches, if that's the issue (I have patches for kdev-php, kdev-python, etc. already).
Comment Actions
Maybe it'd be a good time to start guaranteeing "no API/ABI breaks during major releases"?
Comment Actions
Imo, we had good success the last years with our promise to not break the ABI between 4.x and 4.y. I'd like to continue with this, as our API is still far from clean. If we'd decide now to not break betweeen 5.x and 5.y, then we'd need a 6.x really soon... I'd rather not do that and stick with what we did all these years before.
| language/duchain/navigation/abstractnavigationcontext.cpp | ||
|---|---|---|
| 55 | Will fix in follow-up commit. This might change semantics. | |
Comment Actions
as a follow-up, we may want to implement sharing the d pointer across a parent/child chain, similar to what QObject and QWidget are doing.
| language/duchain/navigation/abstractdeclarationnavigationcontext.h | ||
|---|---|---|
| 52 | de-indent | |
| language/duchain/navigation/abstractdeclarationnavigationcontext.cpp | ||
|---|---|---|
| 530–531 | Note to myself: Unnecessary change | |