[[nodiscard]] for constructors

[AdamBucior]

Fixes #63.

If there are any more places where it would make sense to add [[nodiscard]] please let me know.

[miscco]

These are definitely the most important places, but if I remember correctly we were leaning on putting it on everywhere it is not wrong, which is essentially everywhere.

[AdamBucior]

These are definitely the most important places, but if I remember correctly we were leaning on putting it on everywhere it is not wrong, which is essentially everywhere.

I don't think using it everywhere was the intent of this feature, because creating an unused temporary of literally every class makes no sense. Putting it everywhere would just be a code bloat.

[StephanTLavavej]

I suspect that there's user code out there that's constructing unused strings and so forth - it's probably very rare, but always a bug. We've already marked pure observer member functions as [[nodiscard]] even though it's very rare for the warnings to be emitted (aside from vector::empty()). It does make our sources more verbose, but they're already incredibly verbose and ugly, and that cost is acceptable in exchange for providing value to users.

That said, I think that marking the Standard "guard" classes is sufficient to declare this feature to be complete (the feature is already "extra credit" in the sense that the Library isn't required to do anything). We have a tracking issue to use [[nodiscard]] even more widely throughout the libraries, and we can always investigate applying it to string etc. in the future.

[StephanTLavavej]

This looks good to me! The questions I thought about were:

• For the lock_guard family, is it too aggressive to make them nodiscard types? For example, this would affect a user-defined function returning scoped_lock by value. While I can imagine contrived scenarios, they seem to be very contrived (like "what if the predicate passed to find() has side effects"), and it's extremely hazardous to mistakenly destroy a guard too early. So for guards, I think that making them nodiscard types is fine - we can always revisit this decision if we get customer feedback.

• For thread and jthread, are there valid cases for constructing and destroying a temporary? I can't imagine any. thread needs to be joined. jthread will join in its destructor, but it wouldn't make sense to spin up a new thread, and then block until it's done (why not just call the callable object directly, aside from thread-local variables having fresh values). Both seem extremely likely to be bugs, making them proper candidates for [[nodiscard]].

[StephanTLavavej]

@CaseyCarter @AdamBucior In addition to using plain _NODISCARD on the classes, I've added this to all of our internal guard classes (found by searching for guard).

I wondered if _NODISCARD_CTOR was necessary and answered my own question - old Clang emitted warning: attribute 'nodiscard' cannot be applied to functions without return value [-Wignored-attributes]. While we don't need to worry about that (modern MSVC, Clang, and EDG support this), it's probably safest to keep this distinct macro for CUDA.

[CaseyCarter]

@CaseyCarter @AdamBucior In addition to using plain _NODISCARD on the classes, I've added this to all of our internal guard classes (found by searching for guard).

Searching for guard won't find all of our internal RAII guards; at least _Uninitialized_backout and _Uninitialized_backout_al come to mind, I suspect there are more. That said I'm not really concerned about auditing every single class declaration - or even about adding coverage for these two guard classes - and I've approved. We can always fixup internal machinery when we touch it next.

[StephanTLavavej]

I knew I was forgetting something! I've updated the backout guards and removed their TRANSITION comments. I agree that we can add more later, but this was easy.

[StephanTLavavej]

Thanks for improving the STL's diagnostics! Whenever these [[nodiscard]]s activate, you're going to save programmers an incredible amount of time debugging bizarre multithreading issues. 😸 🎉