Skip to content
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

buffer: add advance(unsigned) back #11993

Merged
merged 2 commits into from Nov 21, 2016
Merged

buffer: add advance(unsigned) back #11993

merged 2 commits into from Nov 21, 2016

Conversation

tchaikov
Copy link
Contributor

advance(unsigned) and seek(int) were replaced with advance(size_t> and
seek(ssize_t) in 053bfa6. this change broke the backward compatibility
of librados/librbd's ABI.

  • deprecates advance(unsigned) and seek(int)
  • update the callers to use the prefered versions.

Fixes: http://tracker.ceph.com/issues/17809
Signed-off-by: Kefu Chai kchai@redhat.com

advance(static_cast<ssize_t>(o));
}

template<bool is_const>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have to make the [s]size_t variants advance2. IIRC @jdurgin told me that adding an overloaded function name changes the ABI even for the original call...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liewegas i doubt it, because when C++ mangles the method signature it takes its arguments into consideration. let's see if the test passes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue would be if a user integrated w/ bufferlist in such a way where they are attempting to take the address of the function so that they could invoke it via function pointer -- by adding an overload it now becomes ambiguous and breaks the compilation until it is resolved. Of course, this overloaded parameter change is also ambiguous just by invoking it and would break the compilation of anyone using bufferlist::advance (but linked apps would still work).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dillaman yes and yes.

but is the address of a method really a part of ABI? and i think we should force the user to use the new version, under most circumstances, "the ambiguity" is a bad thing, but in this very case, i think it is helpful.

Copy link
Contributor Author

@tchaikov tchaikov Nov 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, the layout changes with this fix. i will revert 053bfa6 then.

quote from https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B#The_Do.27s_and_Don.27ts

add an overload (BC, but not SC: makes &func ambiguous), adding overloads to already overloaded functions is ok (any use of &func already needed a cast).

okay then. seems we need to keep source compatibility. i didn't even take this into consideration actually.

This reverts commit 3caafc4.

Fixes: http://tracker.ceph.com/issues/17809
Signed-off-by: Kefu Chai <kchai@redhat.com>
advance(unsigned) and seek(int) were replaced with advance(size_t> and
seek(ssize_t) in 053bfa6. this change broke the backward compatibility
of librados/librbd's ABI.

- deprecates advance(unsigned) and seek(int)
- update the callers to use the prefered versions.

Fixes: http://tracker.ceph.com/issues/17809
Signed-off-by: Kefu Chai <kchai@redhat.com>
@yuriw yuriw merged commit f4ecdc5 into master Nov 21, 2016
@tchaikov tchaikov deleted the wip-17809 branch November 22, 2016 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants