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.

Sign up for GitHub

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

For existing surface area, classes marked as final should not have virtual methods. #5332

Closed
wants to merge 2 commits into from
Closed

For existing surface area, classes marked as final should not have virtual methods. #5332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2b4705c
Classes marked as final should not have virtual methods.
ahsonkhan Feb 8, 2024
2a626b9
Update to use the new macro.
ahsonkhan Feb 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion sdk/identity/azure-identity/inc/azure/identity/azure_cli_credential.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,9 @@ namespace Azure { namespace Identity {
private:
#else
protected:
virtual
#endif
virtual std::string GetAzCommand(std::string const& scopes, std::string const& tenantId) const;
std::string GetAzCommand(std::string const& scopes, std::string const& tenantId) const;
virtual int GetLocalTimeToUtcDiffSeconds() const;
};

Expand Down
21 changes: 8 additions & 13 deletions sdk/identity/azure-identity/inc/azure/identity/detail/token_cache.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@

#pragma once

#include "azure/identity/dll_import_export.hpp"

#include <azure/core/credentials/credentials.hpp>

#include <chrono>
Expand All @@ -24,21 +26,15 @@ namespace Azure { namespace Identity { namespace _detail {
* @brief Access token cache.
*
*/
class TokenCache
#if !defined(TESTING_BUILD)
final
#endif
{
#if !defined(TESTING_BUILD)
class TokenCache _azure_NON_FINAL_FOR_TESTS {
private:
#else
protected:
#endif
Comment on lines -34 to -36
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LarryOsterman A good thing from this effort is we caught this protected: keyword with side effects, which was making unrelated structs like CacheKey etc. below protected, instead of private.

We can fix that now.

// A test hook that gets invoked before cache write lock gets acquired.
virtual void OnBeforeCacheWriteLock() const {};
_azure_VIRTUAL_FOR_TESTS
// A test hook that gets invoked before cache write lock gets acquired.
void
OnBeforeCacheWriteLock() const {};

// A test hook that gets invoked before item write lock gets acquired.
virtual void OnBeforeItemWriteLock() const {};
_azure_VIRTUAL_FOR_TESTS void OnBeforeItemWriteLock() const {};

struct CacheKey
{
Expand All @@ -63,7 +59,6 @@ namespace Azure { namespace Identity { namespace _detail {
mutable std::map<CacheKey, std::shared_ptr<CacheValue>, CacheKeyComparator> m_cache;
mutable std::shared_timed_mutex m_cacheMutex;

private:
TokenCache(TokenCache const&) = delete;
TokenCache& operator=(TokenCache const&) = delete;

Expand Down
8 changes: 8 additions & 0 deletions sdk/identity/azure-identity/inc/azure/identity/dll_import_export.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,14 @@

#undef AZ_IDENTITY_BUILT_AS_DLL

#if defined(_azure_TESTING_BUILD)
#define _azure_NON_FINAL_FOR_TESTS
#define _azure_VIRTUAL_FOR_TESTS virtual
#else
#define _azure_NON_FINAL_FOR_TESTS final
#define _azure_VIRTUAL_FOR_TESTS
#endif

/**
* @brief Azure SDK abstractions.
*
Expand Down
Loading