aaron.ballman added inline comments.
================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:149
+ // Deletion of non-owners, with `delete variable;`
+ if (DeletedVariable != nullptr) {
+ assert(DeleteStmt != nullptr &&
----------------
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.
================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:153
+ diag(DeleteStmt->getLocStart(),
+ "deleting unowned memory; use RAII or gsl::owner")
+ << SourceRange(DeletedVariable->getLocStart(),
----------------
"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.
================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:154-155
+ "deleting unowned memory; use RAII or gsl::owner")
+ << SourceRange(DeletedVariable->getLocStart(),
+ DeletedVariable->getLocEnd());
+ return true;
----------------
Can call `DeletedVariable->getSourceRange()` instead (same applies elsewhere).
================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:157
+ return true;
+ } else
+ return false;
----------------
No `else` after a return (same applies elsewhere).
================
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(),
----------------
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.
================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:188
+ diag(OwnerAssignment->getLocStart(),
+ "assigning neither an owner nor a recognized resource")
+ << SourceRange(OwnerAssignment->getLocStart(),
----------------
Same comments here as above about "owner" and "recognized resource". I think
you want to talk about `gsl::owner<>` where you use "owner" and drop
"recognized resource" (because such a resource returns a `gsl::owner<>`-flagged
type, if I understand properly).
https://reviews.llvm.org/D36354
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
