nateprewitt commented on code in PR #48971:
URL: https://github.com/apache/arrow/pull/48971#discussion_r2728671113
##########
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:
I'm fine doing that if that's the preferred convention. The decision to move
to `{}` instead of `= default` is the other file systems used as reference had
this convention. I was trying to push the implementations closer together.
Looking at all of them though, it does appear to be mixed.
[s3fs
destructor](https://github.com/apache/arrow/blob/8010794116fa9fb1ed020c649053411a7bd7f1bf/cpp/src/arrow/filesystem/s3fs.cc#L3025)
[hdfs
destructor](https://github.com/apache/arrow/blob/main/cpp/src/arrow/filesystem/hdfs.cc#L456)
--
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]
