You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/10/28 00:47:34 UTC
[GitHub] [geode-native] moleske opened a new pull request #680: GEODE-8666: Enforce no-non-virtual-dtor
moleske opened a new pull request #680:
URL: https://github.com/apache/geode-native/pull/680
Authored-by: M. Oleske <mi...@oleske.engineer>
tagging @gaussianrecurrence & @mreddington if you'd like to review also
Passing tests
[Ubuntu - clang, units, integration, acceptance, ipv6](https://github.com/moleske/geode-native/actions/runs/332211481)
[Mac - units, integration, acceptance](https://github.com/moleske/geode-native/actions/runs/332211479)
[Windows units, integration, acceptance (cli and cpp)](https://github.com/moleske/geode-native/actions/runs/332211482)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on pull request #680: GEODE-8666: Enforce no-non-virtual-dtor
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #680:
URL: https://github.com/apache/geode-native/pull/680#issuecomment-718257918
> I am running an [ABI compliance tool](https://lvc.github.io/abi-compliance-checker/) having compiled both the develop branch and the PR's branch and oddly it states that changes introduced are ABI backward compatible. I will try to run the same check with Clang-6.0 and onwards so as well as with MSVC.
>
> As stated by @pivotal-jbarrett, exposing base class destructors into vtable should be an ABI breaking change.
For some reason in both GCC/Clang seems to have no impact. I've opened an [stackoverflow question](https://stackoverflow.com/questions/64576696/exposing-a-base-class-destructor-as-virtual-means-breaking-abi-backward-compatib) just to clarify it, but in the case of MSVC it does. For example for the case of CqQuery vtable is shifted by one position...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] moleske commented on a change in pull request #680: GEODE-8666: Enforce no-non-virtual-dtor
Posted by GitBox <gi...@apache.org>.
moleske commented on a change in pull request #680:
URL: https://github.com/apache/geode-native/pull/680#discussion_r513832424
##########
File path: cppcache/include/geode/AuthenticatedView.hpp
##########
@@ -90,7 +90,7 @@ class APACHE_GEODE_EXPORT AuthenticatedView : public RegionService {
/**
* @brief destructor
*/
- virtual ~AuthenticatedView();
+ virtual ~AuthenticatedView() override;
Review comment:
whoops, missed that on my pass. That's what reviews are for
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #680: GEODE-8666: Enforce no-non-virtual-dtor
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #680:
URL: https://github.com/apache/geode-native/pull/680#discussion_r513466788
##########
File path: cppcache/include/geode/AuthenticatedView.hpp
##########
@@ -90,7 +90,7 @@ class APACHE_GEODE_EXPORT AuthenticatedView : public RegionService {
/**
* @brief destructor
*/
- virtual ~AuthenticatedView();
+ virtual ~AuthenticatedView() override;
Review comment:
In this case the change is not breaking ABI BC as override is just a keyword introduced in the standard in order to have the compiler checked that the virtual function you are overriding has the same signature as the one on the base class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #680: GEODE-8666: Enforce no-non-virtual-dtor
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #680:
URL: https://github.com/apache/geode-native/pull/680#discussion_r513209098
##########
File path: cppcache/include/geode/CqQuery.hpp
##########
@@ -53,6 +53,8 @@ class Query;
*/
class APACHE_GEODE_EXPORT CqQuery {
public:
+ virtual ~CqQuery() = default;
Review comment:
This will for sure change the ABI. I am not sure we can do this on the public headers until the next major.
##########
File path: cppcache/include/geode/AuthenticatedView.hpp
##########
@@ -90,7 +90,7 @@ class APACHE_GEODE_EXPORT AuthenticatedView : public RegionService {
/**
* @brief destructor
*/
- virtual ~AuthenticatedView();
+ virtual ~AuthenticatedView() override;
Review comment:
`virtual` is redundant when `override` is specified. Under some analyzers it will warn or error about it.
##########
File path: cppcache/include/geode/AuthenticatedView.hpp
##########
@@ -90,7 +90,7 @@ class APACHE_GEODE_EXPORT AuthenticatedView : public RegionService {
/**
* @brief destructor
*/
- virtual ~AuthenticatedView();
+ virtual ~AuthenticatedView() override;
Review comment:
Given that this is a public header, what impact is there to the ABI?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence edited a comment on pull request #680: GEODE-8666: Enforce no-non-virtual-dtor
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence edited a comment on pull request #680:
URL: https://github.com/apache/geode-native/pull/680#issuecomment-717956010
I am running an [ABI compliance tool](https://lvc.github.io/abi-compliance-checker/) having compiled both the develop branch and the PR's branch and oddly it states that changes introduced are ABI backward compatible. I will try to run the same check with Clang-6.0 and onwards so as well as win MSVC.
As stated by @pivotal-jbarrett, exposing base class destructors into vtable should be an ABI breaking change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] moleske closed pull request #680: GEODE-8666: Enforce no-non-virtual-dtor
Posted by GitBox <gi...@apache.org>.
moleske closed pull request #680:
URL: https://github.com/apache/geode-native/pull/680
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] moleske commented on pull request #680: GEODE-8666: Enforce no-non-virtual-dtor
Posted by GitBox <gi...@apache.org>.
moleske commented on pull request #680:
URL: https://github.com/apache/geode-native/pull/680#issuecomment-724207520
Closing this now. Shame there wasn't a better answer to why the tool wasn't working. I don't necessarily still agree with why ABI needs to be maintained til a major, but I also don't care enough since I'm currently not paid to work on this (apparently I just really like removing skipped compiler warnings 🤷♀️.) Same for making a 2.0 geode native. I'll look to set up automation on my branch so that when a decision to make a major version happens, this can be contributed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request #680: GEODE-8666: Enforce no-non-virtual-dtor
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #680:
URL: https://github.com/apache/geode-native/pull/680#issuecomment-718851470
> Tangential question, what version is native client now? Geode server is at 1.13 but is that what native client is? So to release a major version of native client we need to wait til Geode makes a 2.0 release? That seems not great. I've never understood why geode native and geode are release with the same versioning scheme...
This is an ASF thing. The geode-native repository is not a separate project or sub-project of Geode. As such its part of the Geode project and is released with the Geode project, currently only as source, at each Geode release. So yes, for a breaking change Geode project needs to release a major. Now this is strict interpretation of the ASF project rules, blah blah blah... Once could probably interpret them differently and version each module produces under the project independently. One could define a semantic version independent of the project version. All great discussion topics for the dev list probably.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on pull request #680: GEODE-8666: Enforce no-non-virtual-dtor
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #680:
URL: https://github.com/apache/geode-native/pull/680#issuecomment-717956010
I am running an [ABI compliance tool](https://lvc.github.io/abi-compliance-checker/) having compile both the develop branch and the PR's branch and oddly it states that changes introduced are ABI backward compatible. I will try to run the same check with Clang-6.0 and onwards so as well as win MSVC.
As stated by @pivotal-jbarrett, exposing base class destructors into vtable should be an ABI breaking change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence edited a comment on pull request #680: GEODE-8666: Enforce no-non-virtual-dtor
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence edited a comment on pull request #680:
URL: https://github.com/apache/geode-native/pull/680#issuecomment-717956010
I am running an [ABI compliance tool](https://lvc.github.io/abi-compliance-checker/) having compiled both the develop branch and the PR's branch and oddly it states that changes introduced are ABI backward compatible. I will try to run the same check with Clang-6.0 and onwards so as well as with MSVC.
As stated by @pivotal-jbarrett, exposing base class destructors into vtable should be an ABI breaking change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request #680: GEODE-8666: Enforce no-non-virtual-dtor
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #680:
URL: https://github.com/apache/geode-native/pull/680#issuecomment-718074796
> I am running an [ABI compliance tool](https://lvc.github.io/abi-compliance-checker/)
This is awesome. Perhaps we can add this tooling to the Travis checks we run on PRs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #680: GEODE-8666: Enforce no-non-virtual-dtor
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #680:
URL: https://github.com/apache/geode-native/pull/680#discussion_r513466788
##########
File path: cppcache/include/geode/AuthenticatedView.hpp
##########
@@ -90,7 +90,7 @@ class APACHE_GEODE_EXPORT AuthenticatedView : public RegionService {
/**
* @brief destructor
*/
- virtual ~AuthenticatedView();
+ virtual ~AuthenticatedView() override;
Review comment:
In this case this is not an ABI breaking change, as override is just a keyword introduced in the standard in order to have the compiler checked that the virtual function you are overriding has the same signature as the one on the base class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on pull request #680: GEODE-8666: Enforce no-non-virtual-dtor
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #680:
URL: https://github.com/apache/geode-native/pull/680#issuecomment-722570740
> I'll close this after @gaussianrecurrence reports back with any learnings from why the ABI compliance tool says it is fine (when I also agree it is not). Will also update the JIRA story to reflect that it can't be done yet
Sadly nobody seems to know the reason. I guess at least from my side will have to remain a mistery. If you happen to ever find out why, please let me know :S
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request #680: GEODE-8666: Enforce no-non-virtual-dtor
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #680:
URL: https://github.com/apache/geode-native/pull/680#issuecomment-718848764
> Also, another tangential question, why are we saying ABI breakage requires a major version change? Might be worth writing that down somewhere (unless it is already in the wiki, which I never check). Assuming because it requires consumers of geode native to recompile, but are there other reasons?
This is a semantic versioning thing. An ABI breaking change would require a recompilation of the downstream application. Normally I can drop a minor updated dependency library in place and just restart the downstream application, no recompilation required. An ABI change requires a recompilation, even if the API is unchanged, because the behavior will now be unknown between the downstream application and the library. In this particular case which side of the library boundary generated the destructor and is it in a VTable or not. It is possible that the compiler did the right thing, in seeing that there were virtual methods on class it behind the scenes produced a virtual destructor and exported it as such in the ABI.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] moleske commented on pull request #680: GEODE-8666: Enforce no-non-virtual-dtor
Posted by GitBox <gi...@apache.org>.
moleske commented on pull request #680:
URL: https://github.com/apache/geode-native/pull/680#issuecomment-718276637
Ugh ABI. I never think about it cause it's not in my head part of the contract for breaking changes (just API changes). I'll close this after @gaussianrecurrence reports back with any learnings from why the ABI compliance tool says it is fine (when I also agree it is not). Will also update the JIRA story to reflect that it can't be done yet
Tangential question, what version is native client now? Geode server is at 1.13 but is that what native client is? So to release a major version of native client we need to wait til Geode makes a 2.0 release? That seems not great. I've never understood why geode native and geode are release with the same versioning scheme...
Also, another tangential question, why are we saying ABI breakage requires a major version change? Might be worth writing that down somewhere (unless it is already in the wiki, which I never check). Assuming because it requires consumers of geode native to recompile, but are there other reasons?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org