search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
27.02k stars 11.06k forks source link

Opting out of a check should also opt out of its aliases #54319

Open cjdb opened 2 years ago

cjdb commented 2 years ago

It's fairly annoying to need to write something like this:

// NOLINT(modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays)

or this

Checks: '
  -modernize-avoid-c-arrays,
  -cppcoreguidelines-avoid-c-arrays,
  -hicpp-avoid-c-arrays
  '

Instead, it would be nice if aliases weren't just for opting in, but also opting out.

llvmbot commented 2 years ago

@llvm/issue-subscribers-clang-tidy

njames93 commented 2 years ago

This is a very touchy subject, and the community still hasn't reached a consensus on the best way to resolve some edge cases with this. For example, what to do if 2 alias checks are both enabled but with a different configuration

For your specific case -*-avoid-c-arrays would disable all those checks in one go

Trass3r commented 2 years ago

For your specific case -*-avoid-c-arrays would disable all those checks in one go

Been using this as a workaround already. But wildcards don't work in NOLINT.

i-ky commented 1 year ago

For example, what to do if 2 alias checks are both enabled but with a different configuration

What is the use case for such configuration? It's like using the same check multiple times with different options, which does not really make sense, does it?

njames93 commented 1 year ago

Some modules set their own options for aliases. So running some aliases together, may have them containing different options.

i-ky commented 1 year ago

Some modules set their own options for aliases.

So, these are not users, who configure different options for aliased checks, but aliases themselves, right?

In the list of checks I managed to find few that mention aliasing with differing parameters:

  1. uppercase-literal-suffix

    cert-dcl16-c redirects here as an alias for this check. By default, only the suffixes that begin with l (l, ll, lu, llu, but not u, ul, ull) are diagnosed by that alias.

  2. unhandled-self-assignment

    cert-oop54-cpp redirects here as an alias for this check. For the CERT alias, the WarnOnlyIfThisHasSuspiciousField option is set to false.

  3. signed-char-misuse

    cert-str34-c redirects here as an alias for this check. For the CERT alias, the DiagnoseSignedUnsignedCharComparisons option is set to false.

These "aliases, but with different options" seem to be a minority. Maybe it makes sense to use different words for "true aliases" (same check, same options, same option defaults) and other kinds of aliases (different options and/or different defaults)? Then the original request will be less controversial.

njames93 commented 1 year ago

The complexity in handling "true aliases" and other aliases is just not worth the trouble, when it's usually just a case of editing the check glob to remove those aliases.

cjdb commented 2 months ago

Has there been any progression on reaching consensus for this issue? It looks like NOLINT comments support globs these days, but not all aliases have an appropriate glob (e.g. google-build-namespaces and cert-dcl59-cpp).