Details
- Reviewers
dfaure mart apol - Group Reviewers
Plasma Frameworks - Commits
- R237:5fa1a3312ab2: Make kconfig_compiler generate ctors with the optional parent arg
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.
+1 in spirit.
autotests/kconfig_compiler/test_notifiers.cpp.ref | ||
---|---|---|
7 | Maybe it would be better to use the parent argument in KConfigSkeleton? |
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). |
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?
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.
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
It could make sense to tweak it so in KF6 ParentInConstructor=true it's true by default.
#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.
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.