[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:188
+    diag(OwnerAssignment->getLocStart(),
+         "assigning neither an owner nor a recognized resource")
+        << SourceRange(OwnerAssignment->getLocStart(),
----------------
JonasToth wrote:
> aaron.ballman wrote:
> > 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).
> ideally every created resource would be flagged as `gsl::owner<>`, but for example `fopen`, `malloc` and friends can't, but would benefit the most from such a check (+ flow analysis if the owner gets consumed on every path)
The use case certainly makes sense, but that doesn't make the diagnostic any more clear to the user. Perhaps: "expected assignment source to be of type 'gsl::owner<>'; got %0"? It's not perfect because it doesn't mention "recognized resource", but that can be improved in the follow-up patch that defines those resources.

The wording should be similar for the below cases as well.


================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:167
+         "expected argument of type 'gsl::owner<>'; got %0")
+        << ExpectedOwner->getType().getAsString()
+        << ExpectedOwner->getSourceRange();
----------------
You can pass the `QualType` directly instead of calling `getAsString()` on it.


================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:184
+  // Assignments to owners.
+  if (OwnerAssignment != nullptr) {
+    diag(OwnerAssignment->getLocStart(),
----------------
No need to check against `nullptr` explicitly (here and elsewhere).


================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:191
+  // Initialization of owners.
+  else if (OwnerInitialization != nullptr) {
+    diag(OwnerInitialization->getLocStart(),
----------------
No else after a return. Same applies elsewhere.


================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:208
+
+bool OwningMemoryCheck::handleBadAssignmentAndInit(const BoundNodes &Nodes) {
+  // Problematic assignment and initializations, since the assigned value is a
----------------
It's hard to understand how this differs from `handleAssignmentAndInit()`; might need a more descriptive name.


================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:230
+        << BadOwnerInitialization->getSourceRange();
+    // FIXME FixitHint to rewrite the type if possible
+
----------------
Are you intending to address this in this patch?

Also, it should probably be "FIXME: " (with the colon).


https://reviews.llvm.org/D36354