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

[Jonas Toth via Phabricator via cfe-commits]

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