-
Notifications
You must be signed in to change notification settings - Fork 373
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
samples(storagecontrol): add storagecontrol folder samples #14332
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14332 +/- ##
=======================================
Coverage 93.22% 93.22%
=======================================
Files 2181 2181
Lines 191173 191173
=======================================
+ Hits 178213 178222 +9
+ Misses 12960 12951 -9 ☔ View full report in Codecov by Sentry. |
google/cloud/storagecontrol/v2/samples/storage_control_folder_samples.cc
Show resolved
Hide resolved
google/cloud/storagecontrol/v2/samples/storage_control_folder_samples.cc
Outdated
Show resolved
Hide resolved
google/cloud/storagecontrol/v2/samples/storage_control_folder_samples.cc
Outdated
Show resolved
Hide resolved
google/cloud/storagecontrol/v2/samples/storage_control_folder_samples.cc
Outdated
Show resolved
Hide resolved
google/cloud/storagecontrol/v2/samples/storage_control_folder_samples.cc
Outdated
Show resolved
Hide resolved
google/cloud/storagecontrol/v2/samples/storage_control_folder_samples.cc
Outdated
Show resolved
Hide resolved
google/cloud/storagecontrol/v2/samples/storage_control_folder_samples.cc
Outdated
Show resolved
Hide resolved
google/cloud/storagecontrol/v2/samples/storage_control_folder_samples.cc
Outdated
Show resolved
Hide resolved
google/cloud/storagecontrol/v2/samples/storage_control_folder_samples.cc
Outdated
Show resolved
Hide resolved
(void)client.DeleteFolder(folder->name()); | ||
} | ||
return {}; | ||
}(std::move(client), bucket_name, prefix, create_time_limit); |
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.
Calling std::move(client)
here invalidates the client and causes the crash.
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.
Hmm... Normally clang-tidy
should give you a warning about that. I wonder why it didn't... Ah, because generated libraries do not get a clang-tidy
build:
google-cloud-cpp/ci/lib/shard.sh
Lines 89 to 90 in 54274c0
# If we decided to compile all libraries with clang-tidy we would need | |
# to enable shards such as: |
After we merge this PR, we should discuss with @scotthart and @dbolduc if they want to enable clang-tidy
for any generated libraries with manual examples.
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.
Removed. But out of curiosity why does this crash?
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.
You are using the move-constructor: note that std::move()
by itself just says "please use the move constructor if available". There is a move constructor available: the compiler generated one, which performance member-by-member move construction. Oh, and because your lambda (now a function) consumes the client
parameter by-value the constructor is called.
Move constructors (as the name implies) move or steal the contents of a the source object into the destination object 1. That leaves the source object in an undetermined state 2. Calling member functions that have pre-conditions may result in UB (undefined behavior). In this case the contents are a std::shared_ptr<>
, the shared pointer move constructor leaves the source with a nullptr
and dereferencing a nullptr
causes a crash.
Footnotes
-
Well, move constructors can do whatever they want. They can behave like copy constructors. But by convention they move / steal the contents. ↩
-
Again, technically move constructors are required to leave the source object in a "consistent but undetermined state". It could be that the source object is left in a state where it is usable, but in general one should assume move constructors leave you with an object where only functions without preconditions (e.g. assigning to the object) work. ↩
google/cloud/storagecontrol/v2/samples/storage_control_folder_samples.cc
Outdated
Show resolved
Hide resolved
google/cloud/storagecontrol/v2/samples/storage_control_folder_samples.cc
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.
A few nits left for you to consider. Otherwise it looks good.
This change is