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

[lukasx999]

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]

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]

@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]

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

[philnik777]

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

[H-G-Hristov]

You will need to also add tests.

[lukasx999]

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.

[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"?

[philnik777]

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

[lukasx999]

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