search

Azure / azure-sdk-for-cpp

This repository is for active development of the Azure SDK for C++. For consumers of the SDK we recommend visiting our versioned developer docs at https://azure.github.io/azure-sdk-for-cpp.
MIT License
177 stars 126 forks source link

Minor code cleanups (3) #6179

Closed antkmsft closed 2 weeks ago

azure-sdk commented 3 weeks ago

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-identity-cpp azure-security-attestation-cpp azure-data-tables-cpp azure-messaging-eventhubs-cpp azure-security-keyvault-keys-cpp

ahsonkhan commented 2 weeks ago

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-identity-cpp azure-security-attestation-cpp azure-data-tables-cpp azure-messaging-eventhubs-cpp azure-security-keyvault-keys-cpp

It looks like there are some false positives detected when it comes to "API changes" that are considered breaking here (around const-ness). At least in the case of marking a method param that was previously passed in by-value to const&, which isn't a breaking change. That seemed to be the only highlighted change for azure-identity-cpp.

Larry, is it possible to exclude that in the detection, to improve signal-to-noise ratio, or do we see value in keeping that diff highlighted in APIView?

LarryOsterman commented 2 weeks ago

API change check APIView has identified API level changes in this PR and created following API reviews. azure-identity-cpp azure-security-attestation-cpp azure-data-tables-cpp azure-messaging-eventhubs-cpp azure-security-keyvault-keys-cpp

It looks like there are some false positives detected when it comes to "API changes" that are considered breaking here (around const-ness). At least in the case of marking a method param that was previously passed in by-value to const&, which isn't a breaking change. That seemed to be the only highlighted change for azure-identity-cpp.

Larry, is it possible to exclude that in the detection, to improve signal-to-noise ratio, or do we see value in keeping that diff highlighted in APIView?

Api changes are not intended to detect breaking changes. They are intended to detect changes in the shape of the API.

That change is a change in the API surface, so it is correctly detected.

Having said that, there ARE changes that are false positives - for instance, when we switched from clang-15 to clang-18, the code which emits type names went from emitting the fully qualified name of the type to an unqualified type name - that is flagged as an API surface change even though it's a consequence of a tooling change.