ARMmbed / core-util

DEPRECATED: Mbed 3 utilities library
Other
12 stars 17 forks source link

v1.1.5 adds breaking changes to the Silicon Labs platforms #63

Open marcuschangarm opened 8 years ago

marcuschangarm commented 8 years ago

https://github.com/ARMmbed/core-util/commit/1cf46421fd860f688dc8ec5fed5ac28cb416329d

The c++11 extensions should have been a major version increase.

bogdanm commented 8 years ago

We enabled c++11 a while ago. At that time, we didn't find any incompatibilities, and since the API themselves didn't change, it made little sense to releae a major number. What kind of breaking changes are you talking about?

ciarmcom commented 8 years ago

ARM Internal Ref: IOTSFW-1918

marcuschangarm commented 8 years ago

The c++11 enablement was in a major version upgrade.

By making this a patch update you are forcing me to change my code, which is against the semantic versioning meaning of patch.

bogdanm commented 8 years ago

I'm not sure actually. semver.org has this to say:

Major version X (X.y.z | X > 0) MUST be incremented if any backwards incompatible changes are introduced to the public API. It MAY include minor and patch level changes. Patch and minor version MUST be reset to 0 when major version is incremented.

It talks about the API. The API didn't change, this was actually a bugfix release. If your code used some core-util code that was buggy and it was fixed, it follows logically that your code needs to change too. I'm really not sure though how this should be handled at versioning level.

marcuschangarm commented 8 years ago

So you see no problem in forcing changes on our user base like this? Interesting.

bogdanm commented 8 years ago

To quote myself:

I'm really not sure though how this should be handled at versioning level.

marcuschangarm commented 8 years ago

Breaking changes = major version update

bogdanm commented 8 years ago

Did you read the semver.org quote in my previous post carefully? From that perspective, this is not a breaking change, but a bugfix. And again, I don't know how this is should be handled under semver rules. In this particular case, I'd say that if you were using copy constructors/assignment operators, there's a quite high change that your code won't work properly. I strongly suggest changing it.

marcuschangarm commented 8 years ago

The API did change. I can't even compile it.

My point is, I should always be able to update minor and patch version modules and still expect my code to work without changes.

0xc0170 commented 8 years ago

@marcuschangarm Fair point. In this case, using newer standard in our modules, shall be considered as breaking change. We should be more careful with this type of updates, as many of our current modules yet not introduced new only c++1x features.

bogdanm commented 8 years ago

This is an interesting situation indeed. You were using some functions generated automatically by the compiler; these functions were already implicitly in the interface, even though they were not explicitly declared. And then I broke that by explicitly removing them. You're right, I didn't consider this. Notwithstanding that, I still strongly recommend fixing your code if it depended on the default copy constructor/assignment operators defined by the compiler. For any non-trivial type, that code probably doesn't do the right thing. Is there anything we can do to help mitigate this issue?

marcuschangarm commented 8 years ago

@bogdanm Thank you for the offer! I got it from here though.

@PrzemekWirkus I wonder if we could have the automatic testing detect when modules break for different major target versions and parent modules within the mbed-drivers dependency tree?

0xc0170 commented 8 years ago

@marcuschangarm Shall this be closed?

marcuschangarm commented 8 years ago

@0xc0170 why? the major version hasn't been bumped yet and the current version still breaks the efm32gg-stk board.