RFC: Add ECMGenerateExportHeaders, for improved handling of deprecated API
Needs ReviewPublic

Authored by kossebau on Sun, Sep 8, 3:24 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Generates additional macros in the export header which can be used for
fine-grained disabling of warnings & visibility as well as excluding from
the build.

Diff Detail

Repository
R240 Extra CMake Modules
Branch
addgenerateexportheader
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16385
Build 16403: arc lint + arc unit
kossebau created this revision.Sun, Sep 8, 3:24 PM
Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptSun, Sep 8, 3:24 PM
Restricted Application added subscribers: kde-buildsystem, kde-frameworks-devel. · View Herald Transcript
kossebau requested review of this revision.Sun, Sep 8, 3:24 PM
kossebau added a comment.EditedSun, Sep 8, 3:39 PM

Implementation prototype for T11490.

Current version works already quite well, developed against applying it to a few tier-1 and tier-2 KDE Frameworks modules.
A few corner casess still need care, but otherwise already pretty much a prototype which is not too far from a production version.

So feel invited to investigate by reading at least the API dox and check the examples given there.

Tests needed :)

Sure :) Will be done, once I know investment makes sense. For now interested in feedback whether the principle approach makes sense and is welcome/wanted, and what people think about the API of the macro as well as the generated API/macros for use in the code.

kossebau updated this revision to Diff 65678.Mon, Sep 9, 3:27 PM
  • change BEFORE to BEFORE_AND_AT, to match actual Qt behaviour (though do not copy confusing name, but be more precise using AND_AT
  • add NO_DEFINITION_EXPORT_TO_BUILD_INTERFACE option
kossebau retitled this revision from WIP: Add ECMGenerateExportHeaders, for improved handling of deprecated API to RFC: Add ECMGenerateExportHeaders, for improved handling of deprecated API.Mon, Sep 9, 5:07 PM
dfaure added a subscriber: dfaure.Tue, Sep 10, 8:11 PM

Great work. Not really easy to grasp at first sight (because it handles BC for no-compat builds of the lib itself, which we never did before) but this is certainly quite comprehensive.

Since we don't yet have any source compat to worry about for these macros (unlike Qt, which needed _X variants for that), how about simply adding a third argument to FOO_DEPRECATED_VERSION right away, to allow specifying a hint in the compiler warning, provided the compiler supports C++14?

kio's src/core/job_base.h:77 would become

KIOCORE_DEPRECATED_VERSION(5, 0, "use uiDelegate()") KJobUiDelegate *ui() const;

If your first version just ignores the argument, at least we'll start writing those hints and we can work on making cmake's GenerateExportHeaders support that, or fork it here to support it.

modules/ECMGenerateExportHeader.cmake
76

typo: pther -> other

86

(minor) small inconsistency, here you use '&' as separator between #if and #endif, while on line 80 you use '/'.

88

I'm having trouble parsing the "only" in this sentence.

deprecated API only... that seems redundant, it's only API that gets deprecated :-)

Maybe you mean "to disable only at build time"?
Or "declaration-only code" ?

and it's always about build time... the choice is between building the library and building the users of the library, isn't it? It's all "build time" of someone...

108

"The tricks [....] does not work" is invalid grammar: either "The trick" (singular) or "do not work" (plural).

kossebau added a comment.EditedWed, Sep 11, 1:33 PM

@dfaure Thanks for first in-detail feedback, good to get a feeling this is not totally insane over-engineered stuff to other people'e eyes :)

Great work. Not really easy to grasp at first sight (because it handles BC for no-compat builds of the lib itself, which we never did before) but this is certainly quite comprehensive.

It's basically trying to solve 2 problems at the same time, as they both would be solved by the same macro techniques:
a) allow control over warnings and API visibility to people building against the library (incl. 2 level settings by additional library group level)
b) allow building of the library itself with legacy code stripped

If there are suggestions for better wording of variables/arguments, or improved ordering/threading of the documentation, all eager to hear. Myself still too much into thinking about details to be able to simulate a first-contact experience again, where ideas are taken from docs and term semantics :)

Since we don't yet have any source compat to worry about for these macros (unlike Qt, which needed _X variants for that), how about simply adding a third argument to FOO_DEPRECATED_VERSION right away, to allow specifying a hint in the compiler warning, provided the compiler supports C++14?

kio's src/core/job_base.h:77 would become

KIOCORE_DEPRECATED_VERSION(5, 0, "use uiDelegate()") KJobUiDelegate *ui() const;

If your first version just ignores the argument, at least we'll start writing those hints and we can work on making cmake's GenerateExportHeaders support that, or fork it here to support it.

As there might not be always a simple hint text, I would still like to also keep the just-version variant of that macro. So if we overload the macro name to support both 2 (major, minor) and 3 (major, minor, message) arguments, any simple way to do default argument value with C++ preprocessor macros? Never wrote such code myself, all things I just found do complicated dances around __VA_ARGS__ of which I none could resolve to a simple application for our single-parameter default value yet.

So, any C++ macro magicians around to be able to tell how to support both

KIOCORE_DEPRECATED_VERSION(5, 0)
KIOCORE_DEPRECATED_VERSION(5, 0, "use uiDelegate()")

while also making sure there is only one argument, and not many? (at least that would be my ambition here to catch this ourselves, instead of just taking __VA_ARGS__ and pass it on, to fail later if >1 args are used.

kossebau updated this revision to Diff 65854.Wed, Sep 11, 3:57 PM
kossebau marked 3 inline comments as done.
  • reword documentation based on dfaure's comments
  • bump since-version to 5.64.0, as currently targetted introduction version

Hope is review can be finished at 5.63 tagging time, so this macro would be
added right after that and all the ideally prepared patches for the KF
modules, so there is a full KF release cycle of testing in git master before
things get rolled out to users.

kossebau added inline comments.Wed, Sep 11, 3:58 PM
modules/ECMGenerateExportHeader.cmake
88

I tried to reword things a bit, also changed all other places talking about build time to be explicit.

I wouldn't offer both variants, but rather recommend always adding a hint. At worse that hint can be "See method documentation" or an empty string. I think we're just over-complicating things otherwise, for no actual gain, since providing a hint is good practice anyway.

Quick update:
Currently still busy trying to get unit tests done, half-way through that. ETA begin of upcoming week.
Next plan: see how only having the 3-arg-FOO_DEPRECATED_VERSION(major. minor, message) would work by using that in the experimental patches done for some KF repos.