================ Comment at: clang-tidy/misc/RedundantSmartptrGet.cpp:78 @@ +77,3 @@ + + const StringRef SmartptrText = Lexer::getSourceText( + CharSourceRange::getTokenRange(Smartptr->getSourceRange()), ---------------- Alexander Kornienko wrote: > 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. Done.
================ 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] + ---------------- Alexander Kornienko wrote: > 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. Done. Would it make sense to add a little more of the output in the check? Each warning also contains a few more lines that print the before and after of the fix. http://llvm-reviews.chandlerc.com/D3186 BRANCH master ARCANIST PROJECT clang-tools-extra _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
