tinyxml2 icon indicating copy to clipboard operation
tinyxml2 copied to clipboard

Migration challenges from API changes in 6.0.0

Open scpeters opened this issue 8 years ago • 4 comments

I'm just writing here to document some of the challenges I've had as a downstream user of tinyxml2 due to the API changes in major version 6. I'm grateful for the work you do and don't want this to register as a complaint or a demand for fixes, but some ideas for making it easier for downstream users to migrate to new versions.

We have some code that attempts to print both GetErrorStr1 and GetErrorStr2 for diagnostic purposes. These functions were replaced by the single function ErrorStr in major version 6.

  1. The old functions were removed without a deprecation tick-tock cycle, so instead of getting compiler warnings telling me that I should update my code, it fails to compile. I noticed this because homebrew recently updated to major version 6, so our code is broken for now on homebrew, while I work on a fix.

  2. It's great that you define the API version numbers in your header file, but they are static const int variables, which means that I can't evaluate them in a compiler directive like #if TIXML2_MAJOR_VERSION >= 6, since only macros can be evaluated in compiler directives (I just learned this actually that for compiler directives Identifiers that are not macros ... are all considered to be the number zero). I believe I can work around this with some cmake code to detect the version and define a symbol if the version is 6 or greater, but it's a little more of a hassle.

Again, thanks for your work maintaining tinyxml2.

scpeters avatar Dec 11 '17 23:12 scpeters

I have a kinda brittle workaround for migration in the following PR:

  • https://bitbucket.org/ignitionrobotics/ign-common/pull-requests/87/support-new-errorstr-api-in-tinyxml2-600/diff

It only works if I use pkg_check_modules in cmake to find tinyxml2, since that provides the version string. I don't think the cmake config file has version numbers defined.

scpeters avatar Dec 12 '17 00:12 scpeters

It was a pretty sweeping change (although long overdue.) I don't generally do the deprecation tick-tock, but...possibly if there is another API consolidation like that.

Macro's would be easy to add. The only trick is modifying the python script. Marking as feature enhancement to remind me.

leethomason avatar Dec 20 '17 22:12 leethomason

Thanks. You can rename the issue to focus on adding version number macros if you want.

scpeters avatar Dec 20 '17 22:12 scpeters

The define macros are in master; leaving this issue open for a bit. It's good discussion. I'll close if there aren't any additional comments.

leethomason avatar Dec 29 '17 17:12 leethomason