[PATCH] Add clang-tidy check to remove redundant .get() calls on smart pointers.

[Alexander Kornienko]

  Looks good provided that you fix the noted issues in the test.

  Thanks for the work!


================
Comment at: clang-tidy/misc/RedundantSmartptrGet.cpp:78
@@ +77,3 @@
+
+  const StringRef SmartptrText = Lexer::getSourceText(
+      CharSourceRange::getTokenRange(Smartptr->getSourceRange()),
----------------
nit: StringRef is a reference to a constant string, so I'd call this use of const redundant. If the intent was to make sure the variable isn't assigned a different value after initialization, I'd argue that it's hard to lose an assignment to SmartptrText in the following five lines of code. And why not make all other local variables const then? The same for the Replacement variable.

================
Comment at: test/clang-tidy/redundant-smartptr-get.cpp:44
@@ +43,3 @@
+  std::unique_ptr<Bar>().get()->Do();
+  // CHECK: warning: Redundant get() call on smart pointer. [misc-redundant-smartptr-get]
+
----------------
Samuel Benzaquen wrote:
> Alexander Kornienko wrote:
> > Maybe add line and column numbers to make the tests more precise?
> > // CHECK: :[[@LINE-1]]:<column number>: warning: ...
> > 
> > I'd also leave the whole error message only once, and abbreviate all other occasions so that they don't exceed the column limit and thus are easier to read.
> Done.
I meant adding the line numbers for all checks. This way we can ensure the warnings are issued on correct lines. Currently, for example, a warning may be issued on the line 66 instead of line 60, but the test will still pass (or even all 5 warning checks without line numbers may match to warnings generated from your negative tests, and the test will still pass).

The lit+FileCheck system is rather primitive. CHECK lines are processed in order, but without any reference to the surrounding text, so it's important to make CHECK lines unique (e.g. by specifying line numbers).

Also, specifying many CHECK-NOT lines one after another makes little sense. It may seem to be good for documentation purposes, but it's not how FileCheck works, so it's misleading. If you have all your negative test cases in the end of the file, it's better to use one "// CHECK-NOT: warning" to assert that there are no warnings expected in this part (the part of the output after the line matched by the last CHECK: that is). You can put CHECK-NOT before this section for documentation purposes, but it doesn't matter for FileCheck.

I know, it's rather confusing, so there are plans to implement an analogue of clang's -verify option for clang-tidy. But while it's not here, we should deal with FileCheck's quirks.


http://llvm-reviews.chandlerc.com/D3186