Deprecate DUNE_DEPRECATED and DUNE_DEPRECATED_MSG

True. But I would like this change to stand the test of time before using it in a release.

changed milestone to %DUNE 2.7.0

This requires us to also update our deprecation warning placement guides. Playing around with [[deprecated]] some time ago, I noticed that some of our current placements cause compilation errors. That also makes it impossible to just go ahead and switch the macros to use the new attributes.

No hurry. I just noticed it while working on something else.

mentioned in merge request !359 (merged)

mentioned in merge request staging/dune-uggrid!132 (merged)

didn't make 2.7.0, retarget to 2.8.0

So, there is another issue with this. Currently we teach doxygen about DUNE_DEPRECATED and DUNE_DEPRECATED_MSG(), by telling Doxygen's preprocessor to replace those with an appropriate /** \deprecated */ comment, even including the message if available. Ideally, Doxygen would understand [[deprecated]] and [[deprecated("msg")]], but currently is does not.

So I guess the options are:

Develop and upstream a Doxygen patch to teach it about the deprecation attribute
Keep DUNE_DEPRECATED and DUNE_DEPRECATED_MSG() as it is right now
Tell Doxygen to pass the source files through an input filter before processing them, the turns deprecation attributes into Doxygen deprecation comments. I threw such a filter together (filter.cc).
repeat the deprecation message in the deprecation attribute and in the deprecation comment.

There are drawbacks to these.

upstreaming a patch takes effort and it will take time for the benefits to arrive
Keeping the current macros means using non-standard constructs, plus the handling of the message in DUNE_DEPRECATED_MSG() is clunky since the macro stringifies it's argument. That makes it difficult to include a url in the message due to the double-slash introducing a comment.
For the input-filter approach we need to think how to make the filter program available when Doxygen runs. E.g. if it needs compiling, we now actually need to compile something before pulling all the module's sources together to build the docs for the website. And there are more approriate to to write such a thing than handcoding a lexer and a parser, but do we want to introduce dependencies on flex/bison?
and having to repeat the deprecation is something that will surely be forgotten.

Here is the Doxygen feature request: https://github.com/doxygen/doxygen/issues/5940

I don't think we should wait for Doxygen. Lets replace our macro by [[deprecated]] and the additional Doxygen comment.

I think one problem will be, that DUNE_DEPRECATED worked for types, while [[deprecated]] does not.

I checked all post-2.7 compilers currently supported by our build-system:

true: the compiler emits a deprecation warning, though it might be a bit misleading (e.g. with multiple declarations of an entity, and only one of them including [[deprecated]], the warning may point to the declaration not including [[deprecated]])
false: no deprecation warning is emitted

Source for tests here: joe/test-deprecation@5ef0f6ad

Results from this Pipeline: https://gitlab.dune-project.org/joe/test-deprecation/pipelines/24859

The test names correspond to .cc files in that repo. The tests were mostly inspired by the explanation in https://en.cppreference.com/w/cpp/language/attributes/deprecated

	clang 6.0	clang 7	clang 8	gcc 7	gcc 8	gcc 9
alias-typedef	true	true	true	true	true	true
enumeration	true	true	true	true	true	true
enumerator	true	true	true	true	true	true
function	true	true	true	true	true	true
namespace	true	true	true	false	false	false
nonstatic-data-member	true	true	true	true	true	true
specialization-struct	true	true	true	true	true	true
static-data-member	true	true	true	true	true	true
static-template-data-member	false	false	false	true	true	true
struct-typedef	true	true	true	true	true	true
struct-variable	true	true	true	true	true	true
template-alias	false	false	false	true	true	true
template-function	true	true	true	true	true	true
template-nonstatic-data-member	true	true	true	true	true	true
template-nonstatic-data-member-via	true	true	true	true	true	true
template-static-data-member	true	true	true	true	true	true
template-static-data-member-via	true	true	true	false	false	false
template-struct	true	true	true	true	true	true
template-variable	true	true	true	true	true	true
typedef-typedef	true	true	true	true	true	true
variable	true	true	true	true	true	true

So gcc has problems with

namespaces:

namespace [[deprecated]] NS {}

using namespace NS;

static data members of template classes when referenced indirectly:

template<int>
struct S {
  [[deprecated]] static int x;
};
template<int i>
int S<i>::x;
template<int i>
int &ref = S<i>::x;

int &ref0 = ref<0>;

On the other hand, clang has problems with

static data member templates:

struct S {
  template<int>
  [[deprecated]] static int x;
};
template<int i>
int &ref = S::x<i>;

alias template

template<int>
using S [[deprecated]] = int;
template<int i>
using Alias = S<i>;

@gruenich I'm assuming you were referring to clang's problem with alias templates? Or were you referring to something else? At least I can't get deprecation of alias templates to work with __attribute__((deprecated)) (used by DUNE_DEPRECATED) either.

Thanks for your analysis! Using !777 (merged) Clang 10 (couple of weeks old Git version) spills out:

/home/gruech/dune/complete/dune-grid/dune/grid/io/file/dgfparser/dgfgridfactory.hh:48:89: error: 'deprecated' attribute cannot be applied to types
                              MPICommunicatorType comm = MPIHelper::getCommunicator() ) DUNE_DEPRECATED
                                                                                        ^
/home/gruech/dune/complete/dune-common/dune/common/deprecated.hh:14:27: note: expanded from macro 'DUNE_DEPRECATED'
#define DUNE_DEPRECATED [[deprecated]]

I cross-checked with GCC 9 and it just worked.

Others came across the same issue: https://github.com/Haivision/srt/pull/1027/files I don't have a GCC 10 at hand, might be interesting to check how GCC dev's are handling this now.

Would __attribute__((deprecated)) still work in the same spot?

I mean, if we're gonna use [[deprecated]] now it's kind of pointless to hide it behind a macro. It's probably best to just say "Don't use DUNE_DEPRECATED anymore, use [[deprecated]] directly. And as others pointed out, that just needs to go in different spots in the declaration.

Jö, you are right. I wanted to get rid of the CMake check for attribute(deprecated). I am going to rework my MR to what you suggested. I might add a #warning to indicate thedeprecation of the macro.

See !777 (merged), the deprecation of the current macros is only in the documentation. Might still do the trick.

mentioned in merge request !777 (merged)

mentioned in merge request extensions/dune-grid-glue!44 (merged)

I don't see a solution for the automatically added Doxygen documentation. We will lose this feature until Doxygen picks it up. Not sure when or wether it will happen at all.

There is a good description on where to put the [[deprecated]] for several cases (functions, namespaces etc.), cf. https://en.cppreference.com/w/cpp/language/attributes/deprecated

I think deprecation warnings in Doxygen are important and I honestly object to merging before this issue is settled.

I think currently the filter is the most promising approach.

We add the filter to dune-common (perhaps `dune-common/bin/doxyfilter-deprecations) and (possibly) install it
We will need a program to do the filtering, perhaps a python program would be best, as most often python will be available anyway. (requires porting Jö's filter.cc to python)
We add an filter entry to dune-common/do/doxygen/Doxystyle/ that uses cmake substitution
INPUT_FILTER = @DUNE_COMMON_DOXYFILTER_DEPRECATIONS@
cmake takes are to find the correct path, depending on whether dune-common is installed or not.
if python is not found, the INPUT_FILTER will simply be empty. But on our build machines we can make sure that python is available.

Thanks for sharing your view, I wasn't aware that there is interest in keeping the deprecation documentation mechanism. I won't merge !777 (merged) until this is settled.

I don't like your approach. For me, it looks overly complicated and again Dune-specific. I would prefer to ask people to explicitly document the deprecation with \deprecated in the Doxygen comment.

@christi I agree with Christoph here: What you propose seems overly complicated to me. Instead I propose to file a feature request for doxygen to auto-detect the deprecated attributes, and meanwhile tell our own devs to just put in the extra \deprecated line manually.

@oliver.sander do you honestly think people will keep track of this? The reason to do his via the macro was specifically because people are too lazy.

regarding patches to Doxygen... I don't think this will be fixed very soon, unless we also add a patch. There is already a corresponding Issue... open since 2018, without any reaction.

Not people are lazy, it is us who is lazy. Who is deprecating beside the core developers?
We are lazy. I confess, in the past it was handy to have the macro. But it will be much less work to fix the incomplete deprecations compared to introduce and maintain the Python magic. So overall I think, not having the macro would suite my laziness more.
I wouldn't bet on Doxygen neither.

do you honestly think people will keep track of this?

Not 100%. But I don't remember ever meeting anyone who actually used the doxygen documentation anyway.

I'm using it and the people in the our DUNE courses always used it extensively.

I just found an even simpler solution... we can use cmake to do the filtering, at least for any reasonable deprecation message.

calling the following script

file(READ "${FILE}" data)
# mark escaped double quotes '\"'
string(REPLACE "\\\"" "@filter_backslash_doublequote@" data "${data}")
# filter empty deprecations
string(REPLACE "[[deprecated]]" "/** @deprecated */" data "${data}")
string(REGEX REPLACE "\\[\\[deprecated\\(\"([^\"]*)\"\\)\\]\\]" "/** @deprecated { \1 } */" data "${data}")
# bring back the escaped double quotes
string(REPLACE "@filter_backslash_doublequote@" "\\\"" data "${data}")
message("${data}")

like

cmake -DFILE=test.cc -P filter.cmake

should do the trick.

I don't know how cmake finds scripts, but I'm sure @gruenich knows how to do this.

Hmm... I think also the sript of Jö will not work, as the position of [[depreated]] is not working wor doxygen.

Admittedly, the cmake approach is simpler. And admittedly most developers deprecating stuff will simply forget the doxygen deprecation mark. But if you care about it so much, why don't you simply review all deprecating merge requests and complain if the \deprecated is missing? Things don't get deprecated all that frequently, so this is not a large burden.

What do you mean by "who cmake finds scripts"? I don't get the question.

Nevertheless, I agree with Oliver, not worth the work.

How can we proceed? Should I invite people from the mailing list to chip in? Should I start a formal vote?

Summary for Dune Developer meeting

Deprecate DUNE_DEPRECATED and DUNE_DEPRECATED_MSG as C++14 introduce [[deprecated]]/[[deprecated("message")]] and GCC 4.9, which had some issues as the last compiler, is no longer supported.
Pro: It's the C++ way, nobody will be surprised, no code to maintain for Dune.
1. Why keeping a home-grown solution when the standard provides a solution?
2. Which way should be used? Is it allowed to change it one way or the other?
3. It's not a drop-in replacement, sometimes it has put at a different spot.
4. If has two CMake checks, that must be run.
Contra: We loose automatic documentation in Doxygen. Ways to handle this:
1. Doxygen has an open bug, but nobody seems to be willing to fix it.
2. We can ask people to add \deprecated message when adding the C++ deprecated attribute.
3. Christian suggested a script to do so automatically.

Concerning the pro can someone tell me if our DUNE_DEPRECATION is broken in some way, I don't know about or if it needs updating? Checking the git log messages shows that the last change to deprecation.hh was in Jan 2018 so 3 years ago, then there is one log message in 2017, 2016, 2015, 2013. So it doesn't look like a massive maintenance burden at the moment. So shouldn't we wait until something is broken with it or the C++ version is demonstrably superior in some useful way instead of possible even inferior as discussed above? I don't have a strong opinion here since I never had to maintain the deprecation.hh and I haven't investigate the advantages/disadvantages of the C++ version but I'm unsure that I can actually see any 'pro' at the moment.

It is not only having the code. I expanded the above pro list a little bit. Which way to prefer? Is it allowed to change the code either way? We have CMake code, too. It is executed for every configure of every Dune module. I don't see how we don't want to drop it. Christian's use-case is the only point keeping it and to me it is only minor.

closed with merge request !777 (merged)

mentioned in commit 2c560425

Deprecate DUNE_DEPRECATED and DUNE_DEPRECATED_MSG

Designs

Child items ...

Activity