pitrou commented on code in PR #48971:
URL: https://github.com/apache/arrow/pull/48971#discussion_r2727737955
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -3218,6 +3220,11 @@ class AzureFileSystem::Impl {
std::atomic<LeaseGuard::SteadyClock::time_point>
LeaseGuard::latest_known_expiry_time_ =
SteadyClock::time_point{SteadyClock::duration::zero()};
+// Destructor must be defined here where Impl is a complete type.
+// Defining it in the header (even as = default) causes MSVC to fail
+// because it tries to instantiate std::default_delete<Impl> before Impl is
defined.
Review Comment:
Same here, this comment is not needed IMHO.
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -3218,6 +3220,11 @@ class AzureFileSystem::Impl {
std::atomic<LeaseGuard::SteadyClock::time_point>
LeaseGuard::latest_known_expiry_time_ =
SteadyClock::time_point{SteadyClock::duration::zero()};
+// Destructor must be defined here where Impl is a complete type.
+// Defining it in the header (even as = default) causes MSVC to fail
+// because it tries to instantiate std::default_delete<Impl> before Impl is
defined.
+AzureFileSystem::~AzureFileSystem() {}
Review Comment:
Nit: I think we can still use the "default" idiom here:
```suggestion
AzureFileSystem::~AzureFileSystem() = default;
```
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -1363,6 +1363,7 @@ Result<HNSSupport> CheckIfHierarchicalNamespaceIsEnabled(
directory_client.GetAccessControlList();
return HNSSupport::kEnabled;
} catch (std::out_of_range& exception) {
+ ARROW_UNUSED(exception);
Review Comment:
I think we can just remove the variable name?
```suggestion
} catch (const std::out_of_range&) {
```
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -2500,6 +2501,7 @@ class AzureFileSystem::Impl {
auto delete_result = deferred_response.GetResponse();
success = delete_result.Value.Deleted;
} catch (const Core::RequestFailedException& exception) {
+ ARROW_UNUSED(exception);
Review Comment:
Same here: just remove the variable name?
##########
cpp/src/arrow/filesystem/azurefs.h:
##########
@@ -251,7 +251,9 @@ class ARROW_EXPORT AzureFileSystem : public FileSystem {
void ForceCachedHierarchicalNamespaceSupport(int hns_support);
public:
- ~AzureFileSystem() override = default;
+ // Destructor must be defined in the .cc file where Impl is complete,
+ // otherwise MSVC fails with "use of undefined type" for
std::unique_ptr<Impl>.
Review Comment:
No need for the comment IMHO, this is a reasonable change and will be
exercised by CI anyway.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
