Mark deprecated functions through macro?

[jgvictores]

Use case: https://github.com/roboticslab-uc3m/kinematics-dynamics/issues/98 suggests an API break, implemented in https://github.com/roboticslab-uc3m/kinematics-dynamics/commit/21ec3ea0c32a7811461d55264592a3aa7dbe9486

Should the old function be left around for a while, while marking it as deprecated (as in YARP and OpenRAVE)?

Here are my favourite to least favourite options:

1. Yes, use YARP's mechanism.
2. Re-implement something like YARP's mechanism.
3. No, just kill and go on.
4. Use some non-portable gcc-specific marker.

More options/opinions?

[PeterBowman]

I might sound a bit selfish, but let's consider that we are the only consumers of our own code. That is, we are in full control of development, shipment and usage of roboticslab-uc3m software, which in turn means that any API breakage would just impact specific repos at our GitHub organization and maybe a few user forks/independent projects around the same ecosystem. I'd expand on point 3 and say:

3b. No. Open an issue wherever the offending function is located at. Label it as announcement. Use the search bar to localize any call to said function across roboticslab-uc3m and list all affected repos. If necessary, elaborate a removal plan and detail any steps that need to be taken to perform a seamless migration to the new API. Ping the corresponding team or whoever could need more action on their end. Proceed gradually and, finally, kill the function.

We may take advantage of the fact that superbuild-like repos usually pull upstream code at its master branch. On the other hand, git submodules point at a specific point in the upstream's commit history. Both dependency management mechanisms enable us to proceed in stages and defer the final removal until everything is tied up. Within the past week, we've seen a similar situation at #15.

[jgvictores]

In addition to what you say, maybe add a CD_WARNING inside the function? I'm also thinking about a #warning, but would have to look into how this is implemented in YARP and/or OpenRAVE.

[PeterBowman]

In addition to what you say, maybe add a CD_WARNING inside the function?

You mean inside the old function, while it's still marked as deprecated? I don't see the point in adding it to its replacement function, that kind of warnings should strike the developer in the eyes during the compilation phase - too easy to overlook on runtime.

I'm also thinking about a #warning, but would have to look into how this is implemented in YARP and/or OpenRAVE.

See robotology/yarp/cmake/template/yarp_conf_system.h.in#L106-L130. We could then add a return false in ICartesianSolver::getNumLinks, perhaps preceded by CD_ERROR with the corresponding notice.

[jgvictores]

If we did something like robotology/yarp/cmake/template/yarp_conf_system.h.in#L106-L130, where would it go? ColorDebug? Some new macro repository?

[PeterBowman]

Some new macro repository?

Almost like killing a fly with an elephant gun :smile: (or is it a BFG?). It would feel out of place inside ColorDebug, though. I don't have an answer for the general case (yet), but I recall seeing preprocessor directives in there that maybe could benefit from replicating YARP's mechanism (something along the lines of kinematics-dynamics-conf.h.in?). More precisely, I'd see _USE_LMA (ref) getting handled by the #cmakedefine magic. The problem is that, in order to properly mark deprecations in the public API, we'd have to distribute (i.e. install) the configured header file as well. If we set aside the _USE_LMA thing for a moment, another option is copy-pasting those #defines inside ICartesianSolver.h. Nasty.

CMake seems to offer an automated way of generating export-like macros, including deprecation attributes: GenerateExportHeader. Mentioning for completeness, I haven't tested this yet.

[PeterBowman]

The following comments arose from a quick chat with @jgvictores:

• GenerateExportHeader looks nice, we could generate one such header file per repo, name it after the repo itself and distribute along with the API headers. This is an example of invocation at amor-api adjusted for its particular needs:

  generate_export_header(amor_api
                       BASE_NAME amor
                       EXPORT_FILE_NAME ${CMAKE_BINARY_DIR}/generated_include/amor_export.h)

• We'd rather put a CD_WARNING prior to returning from the deprecated function (compare with my previous comment). The CD macro would point users at the correct function on runtime.

PS GenerateExportHeader is available since CMake 2.8.6 (compatibility matrix).

[jgvictores]

In the context of the original issue:

• GenerateExportHeader looks nice, we could generate one such header file per repo, name it after the repo itself and distribute along with the API headers.

This has been implemented in https://github.com/roboticslab-uc3m/kinematics-dynamics/commit/e05909832863329bbe09b436d32302210ef25072 and https://github.com/roboticslab-uc3m/kinematics-dynamics/commit/fb7480ad423e60bb48c719f7ca7e88bd376edaf0. Note that the macro in the abstract base class. Accessing with some code like:

    int i;
    ICartesianSolver* ii;
    ii->getNumLinks(&i);

actually does the trick and gives a nice compiler deprecated warning:

/home/yo/repos/kinematics-dynamics/libraries/TeoYarp/KdlSolver/ICartesianSolverImpl.cpp: In member function ‘virtual bool roboticslab::KdlSolver::getNumJoints(int*)’:
/home/yo/repos/kinematics-dynamics/libraries/TeoYarp/KdlSolver/ICartesianSolverImpl.cpp:18:23: warning: ‘virtual bool roboticslab::ICartesianSolver::getNumLinks(int*)’ is deprecated (declared at /home/yo/repos/kinematics-dynamics/libraries/TeoYarp/ICartesianSolver.h:27) [-Wdeprecated-declarations]
     ii->getNumLinks(&i);
                       ^

• We'd rather put a CD_WARNING prior to returning from the deprecated function (compare with my previous comment). The CD macro would point users at the correct function on runtime.

This has been implemented in https://github.com/roboticslab-uc3m/kinematics-dynamics/commit/a167107d1edee81dc25e310c66c7a072920b1e59.

[jgvictores]

Yet another issue to be closed once documented at https://github.com/roboticslab-uc3m/best-practices :smile:

[PeterBowman]

• CMakeLists.txt: please install(FILES) the new header; see first use case at GenerateExportHeader docs and libraries/TeoYarp/CMakeLists.txt.
• libraries/TeoYarp/ICartesianSolver.h: I'd go the YARP way and provide a common implementation such as:

  ROBOTICSLAB_KINEMATICS_DYNAMICS_DEPRECATED virtual bool getNumLinks(int* numLinks)
  {
    CD_WARNING("getNumLinks deprecated: use getNumJoints instead.\n");
    return false;
  }

  This is mainly aimed at respecting the DRY rule. Keep in mind that these lines would be needlessly replicated in every ICartesianSolver implementation.

[jgvictores]

1. Done at https://github.com/roboticslab-uc3m/kinematics-dynamics/commit/7d221ec3bc0e0f35b3366d8415a3c94ac5fe8214.

[jgvictores]

2. Done at https://github.com/roboticslab-uc3m/kinematics-dynamics/commit/2f49edee876a39ee8992cd0485fe837f6e6a50e9 and https://github.com/roboticslab-uc3m/kinematics-dynamics/commit/1bb84a7dad3ab12c9b4d05e5c872ac76f20bb6b2.

[jgvictores]

Documented on best-practices at https://github.com/roboticslab-uc3m/best-practices/commit/73dc3c2f9f435e425371a2009dba3d13c0223c06.

@PeterBowman @olayasturias Closing issue. Please comment if I've done anything terribly wrong. A pat on the back or some :beers: will suffice elsewise.

[PeterBowman]

Nice! In addition of merely marking a function as deprecated, I'd still recommend devs to open an announcement-like issue and actively assist in the migration to the new API.

Open an issue wherever the offending function is located at. Label it as announcement. Use the search bar to localize any call to said function across roboticslab-uc3m and list all affected repos. If necessary, elaborate a removal plan and detail any steps that need to be taken to perform a seamless migration to the new API. Ping the corresponding team or whoever could need more action on their end. Proceed gradually and, finally, kill the function.

A month-long removal stage is not bad, but it's not unusual to spot deprecated functions hanging around for years in popular APIs. I'd not be surprised if developers in our ecosystem, especially maintainers/users of those less common components, will eventually miss such occasional events; hence my proposal.

[jgvictores]

Ok https://github.com/roboticslab-uc3m/best-practices/commit/1feae0b65a0c13232197750a46ceabf03e2aecd1 (and https://github.com/roboticslab-uc3m/best-practices/commit/26767cd8dfc5e5cbf0347dd2915465a4c6438504)!

[PeterBowman]

Google Translate FTW! :metal:

Just kidding. BTW it's curious to see how GitBook messes up the formatting (numbering + indentation, even if we take into account that github/es is missing a space before each 1.). Compare: github/en, github/es, gitbook/en, gitbook/es.

[jgvictores]

Hey, Thanks for pointing that out! I totally missed how Google Translator removed those spaces LOL. gitbook/es looking better after https://github.com/roboticslab-uc3m/best-practices/commit/7bc92937f2988b59d56c0968757819e21839ac2a.

[PeterBowman]

Regarding what a deprecated function should return (how should it behave), see my comment at roboticslab-uc3m/kinematics-dynamics@106aff2.

[PeterBowman]

Marking a function as deprecated does not work quite well with SWIG. See roboticslab-uc3m/kinematics-dynamics#132 and #43.