refactor: Fix deprecated errors when using shared_ptr::unique() #11318

czentgr · 2024-10-22T05:07:20Z

After the macOS CommandLineTools SDK15 (with Clang16), the new shared_ptr.h now issues the deprecated warnings for C++17 when using the shared_ptr::unique function. This function is removed for C++20.

The solution is to replace the function with the actual implementation using use_count.

Example error:

/Users/czentgr/gitspace/velox/velox/vector/BaseVector.cpp:604:14: error: 'unique' is deprecated [-Werror,-Wdeprecated-declarations]
  if (result.unique() && !isUnknownType) {
             ^
/Library/Developer/CommandLineTools/SDKs/MacOSX15.0.sdk/usr/include/c++/v1/__memory/shared_ptr.h:730:3: note: 'unique' has been explicitly marked deprecated here
  _LIBCPP_DEPRECATED_IN_CXX17 _LIBCPP_HIDE_FROM_ABI bool unique() const _NOEXCEPT { return use_count() == 1; }
  ^
/Library/Developer/CommandLineTools/SDKs/MacOSX15.0.sdk/usr/include/c++/v1/__config:1022:41: note: expanded from macro '_LIBCPP_DEPRECATED_IN_CXX17'
#    define _LIBCPP_DEPRECATED_IN_CXX17 _LIBCPP_DEPRECATED

Resolves: #11342

netlify · 2024-10-22T05:07:37Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`c52ceb4`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/672a8c819f15120008163bde

kgpai · 2024-10-24T17:28:57Z

velox/dwio/common/SelectiveRepeatedColumnReader.cpp

@@ -63,7 +63,7 @@ void prepareResult(
          result->encoding() == VectorEncoding::Simple::ARRAY) ||
         (type->kind() == TypeKind::MAP &&
          result->encoding() == VectorEncoding::Simple::MAP)) &&
-        result.unique())) {
+        result.use_count() == 1)) {


I am wondering whether we should even rely on use_count as its unreliable , see for e.g : https://stackoverflow.com/a/77286276 .

Correct, it is unreliable. This change is a 1-1 replacement for now. Using unique was already unreliable and I guess that is why it is being removed. It will need some change eventually.

Understood , I will create another issue to remove the use of shared_ptr::use_count == 1 from the code base as its unreliable and not meant for use across threads.

Ahh, was about to create this issue :)

czentgr · 2024-10-24T22:08:41Z

@majetideepak FYI.

majetideepak · 2024-10-29T16:26:36Z

velox/dwio/common/SelectiveStructColumnReader.cpp

@@ -99,7 +99,7 @@ void setConstantField(
    const VectorPtr& constant,
    vector_size_t size,
    VectorPtr& field) {
-  if (field && field->isConstantEncoding() && field.unique() &&
+  if (field && field->isConstantEncoding() && field.use_count() == 0 &&


Should be field.use_count() == 1? lets add () (field.use_count() == 1)

We don't seem to use () around boolean expressions like these within if statements. So I didn't add it. I just added it when using macros where the argument is expanded to avoid any potential issues.

majetideepak · 2024-10-29T16:26:48Z

velox/dwio/common/SelectiveStructColumnReader.cpp

@@ -112,7 +112,7 @@ void setNullField(
    VectorPtr& field,
    const TypePtr& type,
    memory::MemoryPool* pool) {
-  if (field && field->isConstantEncoding() && field.unique() &&
+  if (field && field->isConstantEncoding() && field.use_count() == 0 &&


(field.use_count() == 1)?

I only use the quotes when they are expression arguments into macros that could not be replaced e.g. with ASSERT_EQ.

Yes, it is == 1. mistake when fixing a merge conflict.

velox/dwio/common/SelectiveStructColumnReader.cpp

majetideepak · 2024-10-29T17:53:20Z

velox/vector/ComplexVector.cpp

@@ -673,7 +673,7 @@ void RowVector::resize(vector_size_t newSize, bool setNotNull) {
      // to skip uniqueness check since effectively we are just changing
      // the length.
      if (newSize > oldSize) {
-        VELOX_CHECK(child.unique(), "Resizing shared child vector");
+        VELOX_CHECK(child.use_count() == 1, "Resizing shared child vector");


use VELOX_CHECK_EQ

facebook-github-bot · 2024-11-04T13:37:45Z

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

mbasmanova · 2024-11-04T13:37:46Z

@czentgr Would you rebase to resolve merge conflicts?

After the macOS CommandLineTools SDK15 (with Clang16), the new shared_ptr.h now issues the deprecated warnings for C++17 when using the shared_ptr::unique function. This function is removed for C++20. The solution is to replace the function with the actual implementation using use_count.

czentgr · 2024-11-05T21:23:16Z

@mbasmanova I've completed the rebase and resolved the conflict.

facebook-github-bot · 2024-11-05T21:25:05Z

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-11-06T00:55:47Z

@mbasmanova merged this pull request in ef4614d.

conbench-facebook · 2024-11-06T01:46:25Z

Conbench analyzed the 1 benchmark run on commit ef4614d0.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 22, 2024

czentgr force-pushed the fix_shared_ptr_unique_dependency branch from b0b166b to 27fbeec Compare October 22, 2024 16:41

kgpai reviewed Oct 24, 2024

View reviewed changes

kgpai mentioned this pull request Oct 24, 2024

Remove uses of shared_ptr::use_count == 1 ? #11345

Open

czentgr requested a review from majetideepak October 24, 2024 22:08

czentgr marked this pull request as ready for review October 24, 2024 22:08

czentgr mentioned this pull request Oct 28, 2024

feat(build): Switch to use C++20 standard #10866

Open

czentgr force-pushed the fix_shared_ptr_unique_dependency branch from 27fbeec to 83eb2af Compare October 29, 2024 16:19

majetideepak reviewed Oct 29, 2024

View reviewed changes

czentgr force-pushed the fix_shared_ptr_unique_dependency branch from 83eb2af to ee3f458 Compare October 29, 2024 18:18

majetideepak approved these changes Oct 29, 2024

View reviewed changes

majetideepak added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Oct 29, 2024

czentgr force-pushed the fix_shared_ptr_unique_dependency branch from ee3f458 to c52ceb4 Compare November 5, 2024 21:22

czentgr changed the title ~~Fix deprecated errors when using shared_ptr::unique()~~ refactor: Fix deprecated errors when using shared_ptr::unique() Nov 5, 2024

facebook-github-bot closed this in ef4614d Nov 6, 2024

facebook-github-bot added the Merged label Nov 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Fix deprecated errors when using shared_ptr::unique() #11318

refactor: Fix deprecated errors when using shared_ptr::unique() #11318

czentgr commented Oct 22, 2024 •

edited

Loading

netlify bot commented Oct 22, 2024 •

edited

Loading

kgpai Oct 24, 2024

czentgr Oct 24, 2024 •

edited

Loading

kgpai Oct 24, 2024

czentgr Oct 24, 2024

czentgr Oct 24, 2024

czentgr commented Oct 24, 2024

majetideepak Oct 29, 2024

czentgr Oct 29, 2024

majetideepak Oct 29, 2024

czentgr Oct 29, 2024

czentgr Oct 29, 2024

majetideepak Oct 29, 2024

facebook-github-bot commented Nov 4, 2024

mbasmanova commented Nov 4, 2024

czentgr commented Nov 5, 2024

facebook-github-bot commented Nov 5, 2024

facebook-github-bot commented Nov 6, 2024

conbench-facebook bot commented Nov 6, 2024

refactor: Fix deprecated errors when using shared_ptr::unique() #11318

refactor: Fix deprecated errors when using shared_ptr::unique() #11318

Conversation

czentgr commented Oct 22, 2024 • edited Loading

netlify bot commented Oct 22, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

Choose a reason for hiding this comment

czentgr Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czentgr commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 4, 2024

mbasmanova commented Nov 4, 2024

czentgr commented Nov 5, 2024

facebook-github-bot commented Nov 5, 2024

facebook-github-bot commented Nov 6, 2024

conbench-facebook bot commented Nov 6, 2024

czentgr commented Oct 22, 2024 •

edited

Loading

netlify bot commented Oct 22, 2024 •

edited

Loading

czentgr Oct 24, 2024 •

edited

Loading