[clang-tools-extra] Enforce SL.con.3: Add check to replace operator[] with at() [Cont.] (PR #95220)

[Julian Schmidt via cfe-commits]

Paul =?utf-8?q?Heidekrüger?= <paul.heidekrueger at tum.de>,
Paul =?utf-8?q?Heidekrüger?= <paul.heidekrueger at tum.de>,
Paul =?utf-8?q?Heidekrüger?= <paul.heidekrueger at tum.de>,
Paul =?utf-8?q?Heidekrüger?= <paul.heidekrueger at tum.de>,
Paul =?utf-8?q?Heidekrüger?= <paul.heidekrueger at tum.de>,
Paul =?utf-8?q?Heidekrüger?= <paul.heidekrueger at tum.de>,
Paul =?utf-8?q?Heidekrüger?= <paul.heidekrueger at tum.de>,
Paul =?utf-8?q?Heidekrüger?= <paul.heidekrueger at tum.de>,
Paul =?utf-8?q?Heidekrüger?= <paul.heidekrueger at tum.de>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/95220 at github.com>


https://github.com/5chmidti requested changes to this pull request.

I also don't think the check should propose using `at()`. The rule suggests using `span`, `at()`, `range-for`, `begin(),end()` or algorithms instead of accessing elements in a container with an unchecked-bounds function.

@carlosgalvezp with `AvoidSubscriptOperator` we constrain the check to be about subscript access only, which, admittedly, the check currently is. However, the rule is not constrained to subscript access and the rule has an example with `memset` which does not use the subscript operator.
`AvoidBoundErrors` sounds a little bit too broad IMO, because the rule talks about containers and is in the SL containers section.
What about: `cppcoreguidelines-(avoid-)container-unchecked-bounds(-access)` or
`cppcoreguidelines-(avoid-)container-bounds-errors`?
I specifically added `container` to both names because the check is in `SL.con`.

---

> We are not sure what the best behaviour for templates is

> keep the warning and add the name of the class of the object / the template parameters that lead to the message

This may result in a lot of warnings because every diagnostic to the same location will no longer be combined like it is now, and instead we get `N` diagnostics (given `N` instantiations). On the other hand, these warnings would provide context on the type being used, so I think it is better to have that instead of fewer warnings but without the type named.

We could ignore instantiations and diagnose any use of `x[]` inside template declarations. We can then check, if possible, if the type of `x` is known to be excluded (e.g., normal type if it is not a dependent type, or `TemplateSpecializationType`).

---

> I think clang-tidy could offer an enum option for the fix

+1 
That would be a good way to accommodate projects that prefer `at()`, because it requires the project to choose to go with this way of solving it (off-by-default).

---

> disabling the check when exceptions are disabled

That would only apply if the check (is enabled to) proposes to use `at()`.

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