[clang-tidy][docs][NFC] Refer to the CERT rule in bugprone-shared-ptr-array-mismatch docs

[steakhal]

Summary

Document the connection between this checker and the corresponding CERT rule.

[whisperity]

Is "partial aliasing" a new notion? Or... is the alias not partial, but the coverage?

[steakhal]

Register the checker with the alias name :D

[njames93]

Nit: This is not an NFC change

[whisperity]

I have some concerns about this. While it is now clear to me that the partialness refers to partial coverage of the guideline rule, it is indeed very, very partial. MEM51-CPP as of now lists 9 cases of non-compliant examples, from which std::shared_ptr<T> = new S[8]; is just one. bugprone-shared-ptr-array-mismatch (D117306) in itself is a check that inherits from a "more generic" smart pointer check.

Right now, there is no check for the exact same unique_ptr case, which the linked CERT site explicitly phrases:

As with std::unique_ptr, when the std::shared_ptr is destroyed [...]

Let's suppose a bugprone-unique-ptr-array-mismatch check is also created (even though that would mean immediately that the naming of the shared_ptr one is kind of bad and that we should rename or deprecate the check... which is its own can of worms with regards to support of clients...). What will this alias become, then?

At first re-read my recent comment of

In D121214#3367609, @whisperity wrote:

Is "partial aliasing" a new notion?

sounded weird to me too, but thinking this through again, I think I know what I was thinking yesterday.

Please correct me if I'm wrong, but the "name" of a check (alias or not) is a unique property. We lack the infrastructure support for 1:N aliasing. While there are certainly cases where we only managed to partially cover a rule, and made an alias for it, missing a small chunk of the guideline (e.g. because static analyses can't catch such) looks less intrusive than -- essentially -- "trying to sell 1/9 coverage as the alias".

clang-tools-extra/docs/clang-tidy/checks/cert-mem51-cpp.rst

12 ↗

(On Diff #414065)

Maybe this warning could be emphasised with the appropriate RST entity, but a quick grep of Tidy docs turned up no such similar constructs so maybe it is better if we do not break form here...

[steakhal]

What I wanted is to highlight the relationship between the bugprone-shared-ptr-array-mismatch check and the cert-mem51 rule.
I thought these aliases are the _programmatic_ way of encoding this relationship, but I understand that it might be a false advertisement given that only 1 out of 9 cases are caught by the related check.

What do you recommend?
Drop the alias-related changes and preserve the note in the bugprone-shared-ptr-array-mismatch.rst stating this relationship with the cert rule?
If we do it like that, then this will be again NFC.

How shall I proceed?

[whisperity]

In D121214#3369871, @steakhal wrote:

Drop the alias-related changes and preserve the note in the bugprone-shared-ptr-array-mismatch.rst stating this relationship with the cert rule?
If we do it like that, then this will be again NFC.

I would suggest definitely doing that. Perhaps with a bit more emphasis on this one catching the shared_ptr case only.
If you could create the check for the unique_ptr case, that would also be great.
I think some of MEM51-CPP is covered by the Static Analyser, right? (Things like free()ing a new-created pointer.)

In D121214#3369871, @steakhal wrote:

How shall I proceed?

Let's ask @aaron.ballman or @njames93 with regards to that. The long-term solution would be implementing the 1-to-N check aliasing in the infrastructure, but I understand it might be an out-of-scope work right now.

---

However, I am also in favour of creating a mapping of "partially implemented guidelines" in the documentation somewhere... maybe in the list.rst, maybe on a separate page. There, we could start documenting cases like this one... It's less direct than an alias, but more accessible to users who wish to cover as many rules of thumb as possible than having to iterate through hundreds of distinct documentation pages for the checks and trying to deduce "Hey, so this one makes me catch some of that guideline!".

Consider (in a Markdown example, because I can never remember at the top of my head how the heading level magic characters of RST are in sequence...)

# Guideline coverage

## CppCoreGuidelines

{first the fully covered, I believe the cross-reference links here are enough}
 * X.00 Blah blah rule | cppcoreguidelines-foo-bar
 * Y.01 Blah blah rule | cppcoreguidelines-baz-qux

{then the partially covered}
 * Z.99 Blah blah rule | misc-whatever-something, bugprone-fancy-diagnostics

## CERT
<... same format ...>

<... etc. ...>

Note: I'm not saying you should go ahead and create the ENTIRE collection of these data right now, just to start pinning down a format. A "work in progress" marker could also be added to the top of this new page.

[steakhal]

In D121214#3372091, @whisperity wrote:

In D121214#3369871, @steakhal wrote:

Drop the alias-related changes and preserve the note in the bugprone-shared-ptr-array-mismatch.rst stating this relationship with the cert rule?
If we do it like that, then this will be again NFC.

I would suggest definitely doing that. Perhaps with a bit more emphasis on this one catching the shared_ptr case only.

Okay, I'll pick this direction.

If you could create the check for the unique_ptr case, that would also be great.

We will put it on the roadmap, but no promises :D

I think some of MEM51-CPP is covered by the Static Analyser, right? (Things like free()ing a new-created pointer.)

Exactly. CSA checkers unix.MismatchedDeallocator,cplusplus.NewDelete,cplusplus.NewDeleteLeaks together can catch all but the last two cases.

In D121214#3369871, @steakhal wrote:

How shall I proceed?

Let's ask @aaron.ballman or @njames93 with regards to that. The long-term solution would be implementing the 1-to-N check aliasing in the infrastructure, but I understand it might be an out-of-scope work right now.

This is definitely out of scope for me ATM.

However, I am also in favour of creating a mapping of "partially implemented guidelines" in the documentation somewhere... maybe in the list.rst, maybe on a separate page. There, we could start documenting cases like this one... It's less direct than an alias, but more accessible to users who wish to cover as many rules of thumb as possible than having to iterate through hundreds of distinct documentation pages for the checks and trying to deduce "Hey, so this one makes me catch some of that guideline!".

We should also consider the documentation hell, and carefully design a solution that mitigates the already existing burden.
It's already hard to keep everything in sync.

[steakhal]

Removed all the check alias stuff; preserved only the docs part.
Reworded the docs to put emphasis on the implementation limitations.

[aaron.ballman]

LGTM, I think this is a good incremental improvement until the check covers more of MEM51-CPP.

clang-tools-extra/docs/clang-tidy/checks/bugprone-shared-ptr-array-mismatch.rst

.. code-block:: c++
   //                                   ^ warning: shared pointer to non-array is initialized with array [bugprone-shared-ptr-array-mismatch]
 
   std::shared_ptr<Foo> x3(new Foo[10], [](const Foo *ptr) { delete[] ptr; }); // no warning
 
   struct S {
     std::shared_ptr<Foo> x(new Foo[10]); // no replacement in this case
     //                     ^ warning: shared pointer to non-array is initialized with array [bugprone-shared-ptr-array-mismatch]
   };
+
+This check partially covers the CERT C++ Coding Standard rule
+`MEM51-CPP. Properly deallocate dynamically allocated resources
+<https://wiki.sei.cmu.edu/confluence/display/cplusplus/MEM51-CPP.+Properly+deallocate+dynamically+allocated+resources>`_
+However, only the ``std::shared_ptr`` case is detected by this check.