JonasToth added inline comments.
================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:149
+ // Deletion of non-owners, with `delete variable;`
+ if (DeletedVariable != nullptr) {
+ assert(DeleteStmt != nullptr &&
----------------
aaron.ballman wrote:
> Can `DeletedVariable` ever be null if `DeleteStmt` is non-null? I don't think
> it can, so perhaps the best approach is to gate on `DeleteStmt` and remove
> the assert.
I think that both will be matched simultaneously, but i was unsure, therefor i
programmed it defensive.
I can change it if you want.
================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:153
+ diag(DeleteStmt->getLocStart(),
+ "deleting unowned memory; use RAII or gsl::owner")
+ << SourceRange(DeletedVariable->getLocStart(),
----------------
aaron.ballman wrote:
> "use RAII" is not particularly helpful. I think this means you should use a
> smart pointer, no? If so, I'd recommend: `"deleting a pointer through a type
> that is not marked 'gsl::owner'; consider using a smart pointer instead"` or
> something along those lines.
in this case RAII means smartpointers, but in general `gsl::owner` could be
file handles, sockets and whatever else.
i will adjust the warning to mention smartpointers.
================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:168
+ diag(ExpectedOwner->getLocStart(),
+ "expected this argument to be an owner or a recognized resource")
+ << SourceRange(ExpectedOwner->getLocStart(),
----------------
aaron.ballman wrote:
> I'm having a bit of a hard time understanding the guidance in this warning.
> How about: `"argument is expected to be of type 'gsl::owner<>'; got %0"` and
> pass in the type of the argument?
>
> I think something needs to be done to better-define "recognized resource"
> before it's used as a term of art in the diagnostics; this applies elsewhere
> as well.
I used the term 'recognized resource' since `FILE*` would be a resource as
well, but the there is no way the C standard function could return an
`gsl::owner<>`.
I plan to enhance this check, to allow of lists of C/C++ functions that are
known to create a resource, but can not be changed.
The effect would be like in the case of `new` - whenever such a function is
called, the result must be assigned to an `gsl::owner`.
Furthermore the CppCoreGuidelines usually talk about 'resources', so the
wording is somewhat similar.
same applies to next comment
https://reviews.llvm.org/D36354
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
