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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
