[PATCH] D17811: [clang-tidy] Add check to detect dangling references in value handlers.

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at: clang-tidy/misc/DanglingHandleCheck.cpp:24
@@ +23,3 @@
+
+std::vector<std::string> parseClasses(StringRef Option) {
+  SmallVector<StringRef, 4> Classes;
----------------
A very similar code has been added recently to clang-tidy/utils/HeaderFileExtensionsUtils.*. Maybe make that code generic enough and reuse it?

================
Comment at: clang-tidy/misc/DanglingHandleCheck.cpp:82
@@ +81,3 @@
+      anyOf(
+          // For sequences:
+          // assign, push_back, resize
----------------
nit: The comment can well fit on a single line without making it less readable. Also, please add a trailing period.

================
Comment at: clang-tidy/misc/DanglingHandleCheck.cpp:116
@@ +115,3 @@
+  return cxxRecordDecl(internal::Matcher<NamedDecl>(
+                           new internal::HasNameMatcher(HandleClasses)))
+      .bind("handle");
----------------
This use of internal classes doesn't look nice. Can you add a `hasAnyName` overload taking a `set` or an `ArrayRef` of names (or maybe a template version that'll take a range of something convertible to StringRef)?

================
Comment at: clang-tidy/misc/DanglingHandleCheck.cpp:162
@@ +161,3 @@
+                                        hasAutomaticStorageDuration(),
+                                        // and it is a local array or Value
+                                        anyOf(hasType(arrayType()),
----------------
nit: Please add a trailing period (and optionally an ellipsis at the start of the comment and at the end of the previous comment).

================
Comment at: clang-tidy/misc/DanglingHandleCheck.cpp:175
@@ +174,3 @@
+                     IsAHandle, handleFromTemporaryValue(IsAHandle))))))
+          .bind("bad"),
+      this);
----------------
`handleFromTemporaryValue` already binds something to `bad`, which looks suspicious. Should we try to place all .binds() on the upper-most level, when it's possible, to make the whole picture easier to see?

================
Comment at: test/clang-tidy/misc-dangling-handle.cpp:85
@@ +84,3 @@
+  std::string_view view_2 = ReturnsAString();
+  // CHECK-MESSAGES: [[@LINE-1]]:29: warning: std::basic_string_view outlives
+
----------------
jbcoe wrote:
> This (and other) check-messages line looks truncated.
This is intentional. It makes sense to specify the whole message once and remove repeated static text from other CHECK-MESSAGES lines to make them shorter and the whole test more readable.


http://reviews.llvm.org/D17811