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

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Sep 12 08:18:38 PDT 2017


aaron.ballman added inline comments.


================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:141
+  assert(CheckExecuted &&
+         "Non of the subroutines executed, logic error in matcher!");
+}
----------------
Non -> None


================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:294
+
+  // Classes, that contains owners, but do not declare destructors
+  if (BadClass) {
----------------
Missing a full stop at the end of the sentence.


================
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 *'
+}
----------------
JonasToth wrote:
> 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.
> 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<>'`

I would rephrase slightly to:

`returning a newly created resource of type %0 or 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>'`


https://reviews.llvm.org/D36354





More information about the cfe-commits mailing list