[libc++] Add nodiscard attribute to std::make_unique/std::make_shared #153115

lukasx999 · 2025-08-12T01:12:33Z

I think nodiscard makes a lot of sense as a sensible default for smart pointer factory functions.
Calling make_unique and discarding the return value, will lead to an unnecessary allocation and therefore is a misuse of the function. (see https://libcxx.llvm.org/CodingGuidelines.html#apply-nodiscard-where-relevant)
Im kind of surprised that this isn't the default in libc++, aswell as libstdc++, since the standard says that nodiscard attributes are implementation defined.

github-actions · 2025-08-12T01:12:54Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-08-12T01:13:23Z

@llvm/pr-subscribers-libcxx

Author: Lukas (lukasx999)

Changes

I think nodiscard makes a lot of sense as a sensible default for smart pointer factory functions.
Im kind of surprised that this isn't the default in libc++, aswell as libstdc++, since the standard says that nodiscard attributes are implementation defined.

Full diff: https://github.com/llvm/llvm-project/pull/153115.diff

1 Files Affected:

(modified) libcxx/include/memory (+4-4)

diff --git a/libcxx/include/memory b/libcxx/include/memory
index ca880c83a544d..5a4100155fc94 100644
--- a/libcxx/include/memory
+++ b/libcxx/include/memory
@@ -566,15 +566,15 @@ class bad_weak_ptr
 };
 
 template<class T, class... Args>
-constexpr unique_ptr<T> make_unique(Args&&... args);                            // C++14, constexpr since C++23
+[[nodiscard]] constexpr unique_ptr<T> make_unique(Args&&... args);                            // C++14, constexpr since C++23
 template<class T>
-constexpr unique_ptr<T> make_unique(size_t n);                                  // C++14, constexpr since C++23
+[[nodiscard]] constexpr unique_ptr<T> make_unique(size_t n);                                  // C++14, constexpr since C++23
 template<class T, class... Args> unspecified   make_unique(Args&&...) = delete; // C++14, T == U[N]
 
 template<class T>
-  constexpr unique_ptr<T> make_unique_for_overwrite();                        // T is not array, C++20, constexpr since C++23
+  [[nodiscard]] constexpr unique_ptr<T> make_unique_for_overwrite();                        // T is not array, C++20, constexpr since C++23
 template<class T>
-  constexpr unique_ptr<T> make_unique_for_overwrite(size_t n);                // T is U[], C++20, constexpr since C++23
+  [[nodiscard]] constexpr unique_ptr<T> make_unique_for_overwrite(size_t n);                // T is U[], C++20, constexpr since C++23
 template<class T, class... Args>
   unspecified make_unique_for_overwrite(Args&&...) = delete;                  // T is U[N], C++20

philnik777

This is incorrect. You've modified a comment.

lukasx999 · 2025-08-12T11:26:58Z

Do you mean the whitespace between the function and the comment?

philnik777 · 2025-08-12T11:47:19Z

No, you didn't modify any code. You just modified a comment.

H-G-Hristov · 2025-08-12T13:17:13Z

You will need to also add tests.

lukasx999 · 2025-08-12T13:49:35Z

No, you didn't modify any code. You just modified a comment.

Sorry, my editor was not syntax-highlighting the comment - as it should, I just though that was because of some #if version checking, which is very common in libc++.
I updated the actual make_unique and make_shared implementation accordingly.
Do you think the memory synopsis should still contain the nodiscard attribute?

philnik777

Do you think the memory synopsis should still contain the nodiscard attribute?

No

Please also add tests. You can take a look at libcxx/test/libcxx/mem/mem.res/nodiscard.verify.cpp for an example.

libcxx/include/__config

philnik777

Could you refer in the commit message to https://libcxx.llvm.org/CodingGuidelines.html#apply-nodiscard-where-relevant instead of "I think it makes a lot of sense"?

libcxx/test/libcxx/memory/nodiscard.verify.cpp

philnik777 · 2025-08-13T07:06:38Z

libcxx/test/libcxx/memory/nodiscard.verify.cpp

+        S() = default;
+    };
+
+    std::make_unique<S>(S{}); // expected-warning {{ignoring return value of function}}


It looks like you're missing a few test cases. I count 12 new annotations above, but this tests only 6 functions.

where do you see 12 annotations? I only see 6.

I also counted 12.

I'm sorry but you're not being very helpful, you do mind pointing me to a resource that explains how "annotations" in this test system work?

These .verify.cpp tests use Clang-verify: https://clang.llvm.org/docs/InternalsManual.html#verifying-diagnostics

I think what @philnik777 is referring to is that you added [[__nodiscard__]] to 12 functions or function overloads, yet you are only testing that the diagnostic is firing on 6 functions. This means that you are missing coverage for 6 functions or function overloads to which the attribute was added, but you have nothing testing it.

github-actions · 2025-08-13T23:18:51Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions h,cpp -- libcxx/test/libcxx/memory/nodiscard.verify.cpp libcxx/include/__memory/shared_ptr.h libcxx/include/__memory/unique_ptr.h

View the diff from clang-format here.

diff --git a/libcxx/include/__memory/unique_ptr.h b/libcxx/include/__memory/unique_ptr.h
index 7a2b4a507..87c46ecc9 100644
--- a/libcxx/include/__memory/unique_ptr.h
+++ b/libcxx/include/__memory/unique_ptr.h
@@ -755,7 +755,8 @@ operator<=>(const unique_ptr<_T1, _D1>& __x, nullptr_t) {
 #if _LIBCPP_STD_VER >= 14
 
 template <class _Tp, class... _Args, enable_if_t<!is_array<_Tp>::value, int> = 0>
-[[__nodiscard__]] inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 unique_ptr<_Tp> make_unique(_Args&&... __args) {
+[[__nodiscard__]] inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 unique_ptr<_Tp>
+make_unique(_Args&&... __args) {
   return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...));
 }
 
@@ -778,7 +779,8 @@ template <class _Tp, enable_if_t<!is_array_v<_Tp>, int> = 0>
 }
 
 template <class _Tp, enable_if_t<is_unbounded_array_v<_Tp>, int> = 0>
-[[nodiscard]] _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 unique_ptr<_Tp> make_unique_for_overwrite(size_t __n) {
+[[nodiscard]] _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 unique_ptr<_Tp>
+make_unique_for_overwrite(size_t __n) {
   return unique_ptr<_Tp>(__private_constructor_tag(), new __remove_extent_t<_Tp>[__n], __n);
 }
 
diff --git a/libcxx/test/libcxx/memory/nodiscard.verify.cpp b/libcxx/test/libcxx/memory/nodiscard.verify.cpp
index 316ce0b47..34f751fb5 100644
--- a/libcxx/test/libcxx/memory/nodiscard.verify.cpp
+++ b/libcxx/test/libcxx/memory/nodiscard.verify.cpp
@@ -20,14 +20,12 @@
 #include <memory>
 
 void f() {
+  std::make_unique<int>(1); // expected-warning {{ignoring return value of function}}
+  std::make_shared<int>(1); // expected-warning {{ignoring return value of function}}
 
-    std::make_unique<int>(1); // expected-warning {{ignoring return value of function}}
-    std::make_shared<int>(1); // expected-warning {{ignoring return value of function}}
-
-    std::make_unique<int[]>(5); // expected-warning {{ignoring return value of function}}
-    std::make_shared<int[]>(5); // expected-warning {{ignoring return value of function}}
-
-    std::make_unique_for_overwrite<int>(); // expected-warning {{ignoring return value of function}}
-    std::make_shared_for_overwrite<int>(); // expected-warning {{ignoring return value of function}}
+  std::make_unique<int[]>(5); // expected-warning {{ignoring return value of function}}
+  std::make_shared<int[]>(5); // expected-warning {{ignoring return value of function}}
 
+  std::make_unique_for_overwrite<int>(); // expected-warning {{ignoring return value of function}}
+  std::make_shared_for_overwrite<int>(); // expected-warning {{ignoring return value of function}}
 }

added nodiscard attribute to std::make_unique()

935d246

lukasx999 requested a review from a team as a code owner August 12, 2025 01:12

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 12, 2025

added nodiscard attribute to std::make_shared()

30e0103

philnik777 changed the title ~~added nodiscard attribute to std::make_unique()~~ [libc++] Add nodiscard attribute to std::make_unique() Aug 12, 2025

philnik777 requested changes Aug 12, 2025

View reviewed changes

Merge branch 'main' into dev

cf7c589

lukasx999 added 4 commits August 12, 2025 15:30

added nodiscard attribute to actual make_unique implementation

2a26cb8

added nodiscard attribute to make_unique for C++17

e13ed16

added nodiscard for make_shared

b229542

Merge remote-tracking branch 'origin/dev' into dev

1ae47c2

philnik777 requested changes Aug 12, 2025

View reviewed changes

libcxx/include/__config Outdated Show resolved Hide resolved

lukasx999 added 4 commits August 12, 2025 16:16

removed _LIBCPP_NODISCARD_SINCE_CXX17 in favor of [[__nodiscard__]]

cb4d239

removed nodiscard from memory synopsis

710f97c

removed _LIBCPP_NODISCARD_SINCE_CXX17 from make_shared

d793dae

added tests for make_unique/make_shared nodiscard attribute

d6ac2b2

philnik777 requested changes Aug 13, 2025

View reviewed changes

lukasx999 added 2 commits August 13, 2025 14:11

using int instead of S for make_unique/make_shared nodiscard test

23356ef

removed version constraint for make_unique/make_shared nodiscard tests

2e14fa1

lukasx999 changed the title ~~[libc++] Add nodiscard attribute to std::make_unique()~~ [libc++] Add nodiscard attribute to std::make_unique/std::make_shared Aug 13, 2025

[libc++] Add nodiscard attribute to std::make_unique/std::make_shared #153115

Are you sure you want to change the base?

[libc++] Add nodiscard attribute to std::make_unique/std::make_shared #153115

Conversation

lukasx999 commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Aug 12, 2025

Uh oh!

llvmbot commented Aug 12, 2025

Uh oh!

philnik777 left a comment

Choose a reason for hiding this comment

Uh oh!

lukasx999 commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philnik777 commented Aug 12, 2025

Uh oh!

H-G-Hristov commented Aug 12, 2025

Uh oh!

lukasx999 commented Aug 12, 2025

Uh oh!

philnik777 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

philnik777 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

philnik777 Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

lukasx999 Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

Zingam Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

lukasx999 Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

ldionne Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 13, 2025

Uh oh!

Uh oh!

lukasx999 commented Aug 12, 2025 •

edited

Loading

lukasx999 commented Aug 12, 2025 •

edited

Loading