================ Comment at: test/clang-tidy/redundant-smartptr-get.cpp:76 @@ +75,2 @@ + +// CHECK-NOT: warning ---------------- nit: This one is also redundant.
================ Comment at: clang-tidy/misc/RedundantSmartptrGet.cpp:78 @@ +77,3 @@ + + const StringRef SmartptrText = Lexer::getSourceText( + CharSourceRange::getTokenRange(Smartptr->getSourceRange()), ---------------- Samuel Benzaquen wrote: > 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. >> The same for the Replacement variable. nit: Could you remove const from the Replacement variable below as well? Thanks! ================ Comment at: clang-tidy/misc/RedundantSmartptrGet.cpp:81 @@ +80,3 @@ + *Result.SourceManager, LangOptions()); + // Replace *foo->get() with **foo, and foo.get() with foo + const std::string Replacement = ---------------- nit: Please add "." in the end of the comment. ================ 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: > > 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. Good idea. It would overlap with your fix-it tests, but that doesn't harm, I guess. 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
