-
Notifications
You must be signed in to change notification settings - Fork 130
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
Use new macros in existing surface area, so that classes marked as final don't have virtual methods. #5389
Conversation
…nal don't have virtual methods.
sdk/identity/azure-identity/inc/azure/identity/detail/token_cache.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing - drop macro definition from identity/dll_import_export.hpp
?
It's already done in this PR :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, for the future: I have a dissenting opinion about _azure_VIRTUAL_FOR_TESTS
- I think we should not have preprocessor-conditional virtual
. I am approving to not sabotage the team majority decision while the other team members are AFK.
…ed as final don't have virtual methods. (Azure#5389)" This reverts commit 3d7eadd.
…R_TESTS` (#5416) * Revert "Remove the use of ifdef for TESTING_BUILD in KeyVault clients, where (#5406)" This reverts commit 2d8c940. * Revert "Rename the TESTING_BUILD macro to be _azure_TESTING_BUILD to highlight (#5390)" This reverts commit 256c2df. * Revert "Use new macros in existing surface area, so that classes marked as final don't have virtual methods. (#5389)" This reverts commit 3d7eadd. * Revert "For new surface area, classes marked as final should not have virtual methods. (#5331)" This reverts commit ddd0f4b. --------- Co-authored-by: Anton Kolesnyk <antkmsft@users.noreply.github.com>
Fixes #5253 for existing surface area.
Follow-up from #5331