This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add [[nodiscard]] extensions to ranges algorithms
ClosedPublic

Authored by philnik on Nov 1 2022, 12:07 PM.

Details

Summary

This mirrors what we have done in the classic algorithms

Diff Detail

Event Timeline

philnik created this revision.Nov 1 2022, 12:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2022, 12:07 PM
philnik requested review of this revision.Nov 1 2022, 12:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2022, 12:07 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 472369.Nov 1 2022, 12:12 PM

List functions in the documentation

LGTM

libcxx/include/__algorithm/ranges_remove_if.h
68

not an issue, but I think we can make use of the optional nodiscard string literal to suggest the user to use erase-remove (same of erase-unique)

ldionne accepted this revision.Nov 3 2022, 9:03 AM

FWIW I got positive feedback from some folks on our nodiscard extensions. I resisted them for a while but I now definitely think they're an improvement.

libcxx/test/libcxx/diagnostics/ranges.nodiscard.compile.pass.cpp
1 ↗(On Diff #472369)

Filename ranges.nodiscard_extensions.compile.pass.cpp maybe? Not as cute but it explains better what it's about.

12–14 ↗(On Diff #472369)

I don't think we should try to guard this test against potential future changes in Clang like that. Clang should never implement ranges algorithms as builtins, that sounds like a weird thing to do.

libcxx/test/libcxx/diagnostics/ranges.nodiscard.verify.cpp
9–10 ↗(On Diff #472369)

This is an incorrect copy-paste.

12–14 ↗(On Diff #472369)

Same comment, let's remove.

This revision is now accepted and ready to land.Nov 3 2022, 9:03 AM
philnik updated this revision to Diff 473252.Nov 4 2022, 8:33 AM
philnik marked 5 inline comments as done.
  • Address comments
  • Try to fix CI
philnik added inline comments.Nov 5 2022, 8:38 AM
libcxx/include/__algorithm/ranges_remove_if.h
68

I'm not sure what you mean exactly. Either way this should probably be it's own PR, in case you feel like it.

This revision was automatically updated to reflect the committed changes.