googleapis
Add override keyword to MockAsyncResponseReader::Destroy when available
Future versions of gRPC will include Destroy() in grpc::ClientAsyncResponseReaderInterface<Response>, when that happens we need to add the override keyword to any Destroy() member functions in our mocks. We may need to do this with some #ifdef based on gRPC version.
For context, this is waiting on the approval and implementation of https://github.com/grpc/proposal/pull/238.
@coryan When is it safe to do this? Do you use gRPC from HEAD in the open source release?
@coryan When is it safe to do this?
gRPC releases every 6 weeks or so, the robots send us PRs to update our builds the first Monday after each gRPC release. So after your changes make it to gRPC, wait anywhere form 0 to 6 weeks, then the next Monday. However ...
Do you use gRPC from
HEADin the open source release?
No, we do not hate ourselves that much. Our builds are (mostly) pinned to the latest gRPC release. We have builds that are pinned to the oldest release we claim to support (1.33.1). We have some builds that are pinned to whatever release is included with some package managers.
I am not sure if that answers your question about "when is it safe"...
You mentioned off-thread the possibility of conditionally adding the override keyword based on version; is there some symbol that can be ifdef'd? (I'm not sure it's worth it, but if you think it's important.)
Well, this makes me sad: I expected gRPC to follow, oh, I don't know, 30 years of software development conventions and include a macro declaring its version... I cannot find any such macro.
(I'm not sure it's worth it, but if you think it's important.)
I am 99.4% certain clang-tidy will generate a warning if you redefine a function as virtual and already is in the base class:
https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html
When the time comes, we can probably silence clang-tidy with // NOLINT(modernize-use-override) (or // NOLINTNEXTLINE(modernize-use-override) if it does not fit.
Oh, and I forgot to mention, at least one of our builds compiles with warnings-as-errors and with clang-tidy turned on.
Discussed this. Still want it, but we're waiting until our min gRPC version is >= ???
Strike 2. Let's see if we have increased the minimum gRPC version to > 1.38 the next time (we are currently at 1.35).
We no longer use grpc::ClientAsyncResponseReaderInterface<> in our mocks, this is now obsolete.