This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Consolidate the different [[nodiscard]] configuration options into a single one
ClosedPublic

Authored by philnik on Jul 3 2022, 5:52 PM.

Diff Detail

Event Timeline

philnik created this revision.Jul 3 2022, 5:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2022, 5:52 PM
philnik requested review of this revision.Jul 3 2022, 5:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2022, 5:52 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I wonder whether we want to take this approach. We've had patches in the past to remove this feature and make all [[nodiscard]] extensions always enabled. MSVC STL already does this and there have been done some tests on Google and Apple codebases. (I don't recall whether the results have been posted.)

How about going in that direction instead? probably we should then do this shortly after LLVM 15 is branched.

libcxx/docs/ReleaseNotes.rst
55

I feel this wording can use a bit of polish. I have a hard time to understand what this means.

libcxx/include/__config
869

Do we need two different macros or can they be combined? It doesn't need to be done in this patch.

I wonder whether we want to take this approach. We've had patches in the past to remove this feature and make all [[nodiscard]] extensions always enabled. MSVC STL already does this and there have been done some tests on Google and Apple codebases. (I don't recall whether the results have been posted.)

How about going in that direction instead? probably we should then do this shortly after LLVM 15 is branched.

That's what I want to do in D128267, but there @ldionne requested that the different configuration macros should be consolidated into one, which I'm doing in this patch. I think these are two separate issues, so I wanted to land them separately.

I wonder whether we want to take this approach. We've had patches in the past to remove this feature and make all [[nodiscard]] extensions always enabled. MSVC STL already does this and there have been done some tests on Google and Apple codebases. (I don't recall whether the results have been posted.)

How about going in that direction instead? probably we should then do this shortly after LLVM 15 is branched.

That's what I want to do in D128267, but there @ldionne requested that the different configuration macros should be consolidated into one, which I'm doing in this patch. I think these are two separate issues, so I wanted to land them separately.

I missed that patch, thanks for the information. In that case consider my comment above as resolved.

ldionne requested changes to this revision.Aug 4 2022, 9:03 AM

This change will require some updates to the documentation, please grep for _LIBCPP_ENABLE_NODISCARD and _LIBCPP_DISABLE_NODISCARD_EXT.

I think we are on the same page in essence, but this patch does not 100% reflect that. I think we should remove all of our various nodiscard-related settings in favour of a single _LIBCPP_DISABLE_NODISCARD_EXT, which would be enabled by default. Then, in D128267, we can flip the default to _LIBCPP_DISABLE_NODISCARD_EXT being disabled by default (i.e. nodiscard extensions being enabled by default).

libcxx/include/__config
862
This revision now requires changes to proceed.Aug 4 2022, 9:03 AM
philnik updated this revision to Diff 452497.Aug 14 2022, 2:14 AM
philnik marked an inline comment as done.
  • Address comments
libcxx/include/__config
869

IIUC we still want to distinguish between standards-mandated applications and extensions.

ldionne accepted this revision.Aug 18 2022, 8:58 AM

This LGTM, but can you please ping me on Discord for a quick look once CI is passing?

IMO this is a nice simplification, and I can hardly imagine users wanting to apply some extended applications of nodiscard, but not all of them. So the flexibility we provided didn't pull its weight, IMO.

This revision is now accepted and ready to land.Aug 18 2022, 8:58 AM
philnik updated this revision to Diff 453958.Aug 19 2022, 4:52 AM
  • Try to fix CI
ldionne accepted this revision.Aug 25 2022, 8:06 AM
thakis added a subscriber: thakis.Aug 31 2022, 12:30 PM

We (used to) use _LIBCPP_ENABLE_NODISCARD. I only happened to see this change here by coincidence.

It would be nice if there was a

#ifdef _LIBCPP_ENABLE_NODISCARD
#error _LIBCPP_ENABLE_NODISCARD no longer has an effect, define _LIBCPP_ENABLE_NODISCARD_EXT
#endif

in some central header to inform people in a similar situation.

(Also, libcxx/test/libcxx/diagnostics/nodiscard.pass.cpp still mentions _LIBCPP_ENABLE_NODISCARD in a comment.)

@thakis I think your comments should be addressed by D128267.

thakis added a comment.Sep 1 2022, 7:25 AM

@thakis I think your comments should be addressed by D128267.

That's cool, but there still was (is) a Window where [[nodiscard]] disappeared for people who define _LIBCPP_ENABLE_NODISCARD.

In practice, it looks like nothing regressed for us in the 1 week it took us to notice this, but if some hard-to-update dependency had added code that ignored a nodiscard, this would've been pretty painful to deal with.

So I still think it'd be nice if that #error was there until D128267 lands.

libcxx/test/libcxx/diagnostics/enable_nodiscard_ext.verify.cpp