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

[Aaron Ballman via Phabricator via cfe-commits]

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