[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:202
+
+  // Initializer of class constructors, that initialize owners.
+  if (OwnerInitializer) {
----------------
Can remove the comma.


================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:239
+    diag(BadOwnerInitialization->getLocStart(),
+         "initializing non owner %0 with a newly created 'gsl::owner<>'")
+        << BadOwnerInitialization->getType()
----------------
s/non owner/non-owner


================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:256
+    assert(BadOwnerParameter && "parameter for the problematic argument not found");
+    diag(BadOwnerArgument->getLocStart(), "initializing non owner argument of "
+                                          "type %0 with a newly created "
----------------
s/non owner/non-owner


================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:292
+    assert(DeclaredOwnerMember &&
+           "Match on class with bad destructor, but without an declared owner");
+
----------------
s/an declared/a declared

Also, you should either add punctuation or remove the capital M in Match.


================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:294
+
+    diag(BadClass->getLocStart(),
+         "class with an 'gsl::owner<>' as member but without declared "
----------------
Instead of diagnosing this on the class, why not diagnose the individual offending members (and then you don't need a note, either)? (Note, that may change the suggested diagnostic wording below).


================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:295
+    diag(BadClass->getLocStart(),
+         "class with an 'gsl::owner<>' as member but without declared "
+         "destructor to release the owner");
----------------
This diagnostic isn't grammatically correct; how about: `class with a member variable of type 'gsl::owner<>' but no destructor to release the owned resource`?


https://reviews.llvm.org/D36354