-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
treewide: Use deprecations in code rather than comments #18650
base: master
Are you sure you want to change the base?
Conversation
Contributes-To: RIOT-OS#18561
Can we also add |
I think this is @chrysn's plan anyway. But for the CI runs it will be an error (except for some apps explicitly testing for backward compatibility), so that no new in-tree users sneak in. |
Yes, that's the plan. Docs now look as good as it gets: flashpage_first_free looks like that Reviewers, is that good enough for you? (I'd like to know before I do the treewide edits). |
This keeps deprecations at the warning level during regular development, but enforces that no deprecated items are used in internal code, unless the test explicitly allows that.
I've made the intended handling of deprecations explicit in the latest commit. It introduces a The new behavior is as follows, with the strongest rules on top:
Do you have any preferences as to whether the treewide edit (moving as many deprecations as possible into C attributes) would be in here or a follow-up (with this one merely setting the stage)? |
Add missing kernel_defines header
This change also serves as an example of how an item can still be used in code that needs to use it for some time after the deprecation.
While I'm adding selected examples, I've started looking into how to get The Big List of things that should be deprecated in C rather than in documentation. Right now my best expression is
because doc.txt files and vendor headers often include Doxygen deprecations that can't (practically) be moved to C. I also found that there's no straightforward way to deprecated enums as a whole -- a One more case of "code needs to declare when it uses deprecated stuff" I've found is that when a deprecated enum value (such as In the end, replacing the existing documentation deprecations with C ones is likely to be a highly manual process, with many case-by-case decisions; so I'm leaning toward moving that to a later commit (except for the few items done so far, which'd serve as examples). |
Thanks @bergzand for clarifying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. If possible, I would prefer to split the lines to match the maximum char limit (e.g. see suggestions), so that the CI is happy. But I do not insist.
I'll need to rename this due to collisions -- Anyway, I'm not fully convinced that the deprecation with attributes works well around enum items, so my next change series will change this to:
and apply line breaking. |
Really nice, this is a good solution for C code, I wonder if we can expand this or at least reuse the marco semantics to handle deprecation of tools or build system info? We would also need to publicise it to all maintainers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also handle compiler abstractions? Maybe check for LLVM?
I suppose everything is gcc first and only portions of llvm are supported so maybe that would be OK since it is more about warnings rather then code execution.
Note that clang also defines Since we don't support any compiler but GCC and clang/LLVM and there are no plans to do so, IMO this is just fine as it is. |
@@ -280,6 +280,61 @@ extern "C" { | |||
|
|||
/* end{code-style-ignore} */ | |||
|
|||
/** @brief Declare a function as deprecated | |||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* | |
* | |
* @param reason Reason for removal | |
* @param eol Expected end of life | |
* @param migration Migration path (e.g. API to use instead) | |
* |
Maybe enforce consistency and completeness of the doc by having explicit arguments for the important details. That would also decrease the itch to use fancy formatting.
Beware: Indent levels are not correct, did this on a phone.
@chrysn Ping? |
This is a replacement PR for #18565 because that had been made to the wrong branch and GitHub closed it irrevertably when I tried to clean up.
The relevant parts of that PR are replicated here:
Contribution description
This executes #18561.
It already has the macros and Doxygen stuff, it lacks
@deprecated
toDEPRECATED
-Wno-error=deprecated-declarations
if we still want to test the deprecated functions.Testing procedure
Right now:
[edit: or the more forgiving gnu2x, given that native has some trouble with being precisely conformant]
Long term:
@maribu said:
If you throw in an
-Wno-error=deprecated-declarations
, the warning should not be escalated to an error. We may however want to still escalate it to an error withRIOT_CI_BUILD=1
to avoid in-tree users sneaking back in.--
@chrysn responded:
On the severity of errors, I think a good path would be to have Makefiles of
ALLOW_DEPRECATED ?= 1
, make Murdock setALLOW_DEPRECATED = 0
, and have some tests (those that explicitly include deprecated functions because we don't want these to accidentally break) setALLOW_DEPRECATED = 1
.On the CI side, Murdock went well because it duly rejected the tests that used the deprecated function (even though, as above, these should later be exempt).
Sadly, doxygen output breaks completely with the current doxygen version -- deprecation text gets moved into functions list, but the function itself is not present any more at all, and the next function is somehow also missing ... putting this in as "waiting for CI update" because the next Ubuntu hopefully has a better Doxygen. (Things looked good with the 1.9.4 I have here).
This is now blocked by RIOT-OS/riotdocker#189