[libcxx-commits] [PATCH] D108080: [libcxx] simplifies nodiscard by permanently enabling the attribute...

Mon Aug 30 14:11:36 PDT 2021

cjdb added a comment.

In D108080#2973100 <https://reviews.llvm.org/D108080#2973100>, @ldionne wrote:

> IMO, this approach makes sense. However, from 99% of the people's perspective (who didn't enable the extensions), this is equivalent to suddenly adding `[[nodiscard]]` on a bunch of algorithms, period. This has widespread potential for breakage (I'm not saying it does, but it has the potential to).
>
> @cjdb Can you share the results of running this on Google's code base? I'll do the same at Apple and report when that's done. If we don't have significant deployment issues to report, I'd be fine with doing this.
>
> Requesting changes for now.

I will, but it's going to take me some time to get accurate results. I'm also compiling it into a table per codebase, with the following information:

| Function | Total estimated usage | Build failures due to `[[nodiscard]]` | Owners agreed failure was a bug |
| -------- | --------------------- | ------------------------------------- | ------------------------------- |
| fn       | ~K hits               | Not mentioned == zero hits            | x/failures owners agreed        |
|

Gathering such data is a lot slower but gives a **lot** more insight (e.g. `::operator new` has zero hits in two of the codebases, but that'd be obfuscated if I ran it alongside `std::remove`). I also need to run this on codebases where I can't directly apply this patch too, which will take more time than I'd first thought.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108080/new/

https://reviews.llvm.org/D108080