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