Make kconfig_compiler generate ctors with the optional parent arg
ClosedPublic

Authored by ervin on Oct 8 2019, 11:20 AM.

Diff Detail

Repository
R237 KConfig
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ervin created this revision.Oct 8 2019, 11:20 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptOct 8 2019, 11:20 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ervin requested review of this revision.Oct 8 2019, 11:20 AM
apol added a subscriber: apol.Oct 8 2019, 12:29 PM

+1 in spirit.

autotests/kconfig_compiler/test_notifiers.cpp.ref
7

Maybe it would be better to use the parent argument in KConfigSkeleton?
explicit KConfigSkeleton(const QString &configname = QString(), QObject *parent = nullptr);

ervin added inline comments.Oct 8 2019, 12:38 PM
autotests/kconfig_compiler/test_notifiers.cpp.ref
7

It's what I attempted first, the problem was that it will create compatibility issues with existing code. From the generator perspective KConfigSkeleton might not be the base class and since we never enforced the parent chaining constructor before they might not have it. The tests using MyPrefs as base class exhibit that exact problem.

That's why I ended up falling back on setParent... it doesn't have my preference but at least now we won't force the use of setParent in user code (even though we use it internally).

apol accepted this revision.Oct 8 2019, 1:38 PM

Right, makes sense.

This revision is now accepted and ready to land.Oct 8 2019, 1:38 PM

How does this effect the BIC of the generated classes? Consumers might have exposed the generated classes in public API. Any chance this could become a opt-in change for KF5 times?

ervin added a comment.Oct 8 2019, 2:20 PM

How does this effect the BIC of the generated classes? Consumers might have exposed the generated classes in public API. Any chance this could become a opt-in change for KF5 times?

To my knowledge it shouldn't, it's like adding an extra constructor really.

How does this effect the BIC of the generated classes? Consumers might have exposed the generated classes in public API. Any chance this could become a opt-in change for KF5 times?

To my knowledge it shouldn't, it's like adding an extra constructor really.

On the source-compatibility layer I would agree it is, but what about the API symbol layer (which is what I meant with public API, as in, exported visible symbols).

ervin added a comment.Oct 8 2019, 3:12 PM

How does this effect the BIC of the generated classes? Consumers might have exposed the generated classes in public API. Any chance this could become a opt-in change for KF5 times?

To my knowledge it shouldn't, it's like adding an extra constructor really.

On the source-compatibility layer I would agree it is, but what about the API symbol layer (which is what I meant with public API, as in, exported visible symbols).

Right, the type info will change the symbol indeed. Other option is to add an extra ctor instead.

ervin updated this revision to Diff 67516.Oct 8 2019, 4:13 PM

Address Friedrich's comments, I went for hiding the feature behind a flag after all, it was just less complexity added to the compiler in the end

ervin added a comment.Oct 10 2019, 5:44 AM

Any chance to get another round of reviews now that this patch changed quite a bit?

apol accepted this revision.Oct 10 2019, 10:39 AM

It could make sense to tweak it so in KF6 ParentInConstructor=true it's true by default.

In D24490#544599, @apol wrote:

It could make sense to tweak it so in KF6 ParentInConstructor=true it's true by default.

What do you mean by tweak?

apol added a comment.Oct 10 2019, 2:00 PM

#if FRAMEWORK_VERSION < 6
defaultValue = false
#else
defaultValue = true
#endif

Or at least
#if FRAMEWORK_VERSION >= 6
#pragma message Consider enabling ParentInConstructor by default
#endif

It's just a thought, not necessary.

ervin added a comment.Oct 11 2019, 6:53 AM

Ah I see what you meant now. I'd rather not do this, it's IMO better if we remove the setting completely when KF6 comes.

apol added a comment.Oct 11 2019, 9:48 AM

ok, let's just land this for now.

This revision was automatically updated to reflect the committed changes.