[libcxx-commits] [libcxx] [libc++] Diagnose unused variables of container types (PR #203084)

[Nikolas Klauser via libcxx-commits]

philnik777 wrote:

> I think this would be more if C++ added similar attribute and we wanted to use that instead or conditionally.

In that case we can just search-and-replace all of them. If it becomes standard we'd definitely remove the macro, so that wouldn't change much.

> In my experience, if something isn't guaranteed to not change there's always a chance it will. It isn't always about the spelling. If newer GCC introduces a behaviour about this attribute that we no longer like any more, the PR to unapply/substitute it would have a large blast radius.

Diagnostics change all the time, sure. We also wouldn't want to regress on our current diagnostics though, and a macro wouldn't help us in the end either. That seems more like something we'd want to report back and get fixed in the compiler rather than just dropping the attribute.



> One is regarding the goal of this patch, that is to enable unused variable warnings. Replacing this with a `_LIBCPP_WARN_UNUSED` is not going to affect the effectiveness of this PR.

Actually, no. There are lots of diagnostics which print where some attribute was applied, and macros directly make these diagnostics worse, since you have a list of macro expansions in that case. Arguably `[[gnu::warn_unused]]` is missing such a diagnostic, so I wouldn't be surprised if Clang decided to add it. (CC @AaronBallman)



> Another is about diagnosing syntax errors within the macro itself (or the syntax constructs the macro introduced) which quite frankly is our due diligence to keep it error free. There's also widespread precedence in libc++ (e.g. `_LIBCPP_HIDE_FROM_ABI`)

I'm not quite sure what widespread precedence you're talking about here. That we have attribute behind macros? Sure, there is precedence for having a macro if there is a technical reason for it. Mostly, if one of our supported compilers doesn't support a particular macro (or if that was the case at some point, and we simply didn't remove the macro). `_LIBCPP_HIDE_FROM_ABI` is actually quite special in that it represents a concept, not a particular attribute. Its implementation has changed before, and may change again in the future. Notably, there is also significant precedence of not having a macro, like `[[nodiscard]]` (with > 3k instances).

> Also please provide a description for your PR.

What would you like to see in that description?

https://github.com/llvm/llvm-project/pull/203084