[PATCH] D36354: [clang-tidy] Implement type-based check for `gsl::owner`

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

- Adressed some review comments.
- Added testcases, where the `owner<>` annotation might be hidden by a `typedef` or `using`, like int `using heap_int = gsl::owner<int*>;`

The check is currently not able to resolve a `typedef` to `owner<>` correctly, and my knowledge in clang is to limited to find exact reason and/or a workaround.
I tried to match the type with `hasCanonicalType`, but i guess that will remove the `owner<>` alias and therefore be not an option.
The basic testcase for `typedef` should stay in the code with the FIXME, since there is probably a way around.

I am thinking of a matcher like `IsTypedefToOwner`, that looks through one typedef after another recursively until it finds an `owner<>`, but later and probably with a different solution.

The typedef issue is in the following lines, just as shortcut since the patch is quite big:
testcase: 200-225

The issue is mentioned in the docs as well.



================
Comment at: test/clang-tidy/cppcoreguidelines-owning-memory.cpp:39
+  return new int(42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: returning a 'gsl::owner<>' from a function but not declaring it; return type is 'int *'
+}
----------------
aaron.ballman wrote:
> JonasToth wrote:
> > aaron.ballman wrote:
> > > This diagnostic confuses me -- there's no gsl::owner<> involved anywhere; am I missing something?
> > `owner<>` is not involved, but the guidelines say, that `new` must be assigned to an owner.
> > 
> > This is in line with the resource semantics. Everything that creates an resource, that must be released (no RAII available) shall be annotated.
> > 
> > The diagnostic is bad, though.
> > 
> > `Returning a newly created resource from function 'functionname', without declaring it as 'gsl::owner<>'; type is '...'`
> Okay, that makes more sense to me. I don't think the name of the function helps all that much in the diagnostic, however. What about:
> 
> `"returning a newly created resource of type %0 from a function whose return type is not 'gsl::owner<>'"`
There is a minor issue with the diagnostic in general, since it is emitted for values of type `gsl::owner<>` and values that are known to be an owner like `new int(42)`.

There is no easy way to distinguish between a recognized resource or an annotated resource, without complicating the matchers (what i dont want, since there is already a lot happening).

Mixing both cases in the diagnostic would help, but go in the direction of `recognized resource`, that was decided against earlier.

Would the following modification be acceptable as well?
`returning a newly created resource of type %0 or value of type 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>'`
or
`returning a newly created resource of type %0 or value of type 'gsl::owner<>' without annotating the return type of the function as 'gsl::owner<>'`.

This general problem holds true for other cases, since i want to match for `IsConsideredOwner`, which wraps cases like `new`, functions returning `owner<>` and variables of type `owner<>`. 
I want to expand this further to functions that should return `owner<>` but can't, like `malloc`.
Splitting up the matchers instead of using `IsConsideredOwner` would be a burden including a lot of code duplication.


https://reviews.llvm.org/D36354