================ 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 = ---------------- Alexander Kornienko wrote: > nit: Please add "." in the end of the comment. Done.
================ Comment at: test/clang-tidy/redundant-smartptr-get.cpp:76 @@ +75,2 @@ + +// CHECK-NOT: warning ---------------- Alexander Kornienko wrote: > nit: This one is also redundant. 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: > > > 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. Done. Adding those lines will make it hard for errors to match in the wrong place now. 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
