KConfig: stop exporting and installing KConfigBackend.
ClosedPublic

Authored by dfaure on Feb 14 2017, 7:17 AM.

Details

Summary

It can't possibly have been used anywhere, because it's not in the KConfig
API anywhere. The intended way to use this API was by providing a plugin
that would derive from KConfigBackend but the plugin loading code
in KConfigBackend::create has been disabled since before KF 5.0.

The reason I want to stop exporting this class is to be able to
optimize it (e.g. the QDateTime in it is completely unused but leads
to data races due to tzset)

Test Plan

Note, I'll rename it to _p.h if we agree, it would just have made this diff too big.

Diff Detail

Repository
R237 KConfig
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
dfaure updated this revision to Diff 11320.Feb 14 2017, 7:17 AM
dfaure retitled this revision from to KConfig: stop exporting and installing KConfigBackend..
dfaure updated this object.
dfaure edited the test plan for this revision. (Show Details)
dfaure added a reviewer: mdawson.
dfaure added a subscriber: Frameworks.
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 14 2017, 7:17 AM
mdawson requested changes to this revision.Feb 20 2017, 10:56 PM

I agree, this can't be used so removing it from the API can't hurt anything. Just to be sure, let's take this change for the next release, but not change KConfigBackend' API/ABI until after the next release. If anyone complains about KConfigBackend missing before then, we can just re-export it easily. Otherwise this can always come back in a (ABI) different form if this ever becomes worthwhile. Does that sound good to you?

I do have one small comment before committing.

src/core/kconfigbackend.h
212

I don't think this is worth saving as a comment, just remove it completely. If we decide to undo this, it's in git.

This revision now requires changes to proceed.Feb 20 2017, 10:56 PM

The plan sounds good.

src/core/kconfigbackend.h
212

In general I agree that leaving dead code is bad.

In this particular case, though, the other side of the plugin code is still there
("#if 0 TODO port to Qt5 plugin loading" in kconfigbackend.cpp), and actually this is the bit that *was* ported to Qt5 plugin loading, so it will be useful if anyone decides to finally implement plugin based backends. Maybe I can make it
#if 0
TODO re-enable if the plugin loading code is re-enabled
instead of just "//" which is indeed uglier.

Alternatively, we decide that plugin-based backends are not even on the far-future radar and we clean up both sides completely. Works for me too. The whole idea was crazy anyway (apps make assumptions about config files being files, if one day we change that for e.g. something like gconf then it won't be as transparent as just switching to a different backend).

mdawson added inline comments.Feb 20 2017, 11:09 PM
src/core/kconfigbackend.h
212

Sure, #if 0 is best since it has the code, and they will at least match.

I'd actually like to try and bring this back sometime, but as you say it would be very difficult. I don't know if it will ever come to pass, so I don't want to hold up actual improvements for a nice to have. I definitely don't know if it would ever be plugin based, or just built into KConfig.

dfaure updated this revision to Diff 11570.Feb 20 2017, 11:11 PM
dfaure edited edge metadata.

Use "#if 0"

mdawson accepted this revision.Feb 20 2017, 11:13 PM

Ship it!

This revision is now accepted and ready to land.Feb 20 2017, 11:13 PM
dfaure closed this revision.Feb 20 2017, 11:17 PM