xazax.hun marked 4 inline comments as done. xazax.hun added inline comments.
================ Comment at: test/clang-tidy/cert-dcl21-cpp.cpp:1 +// RUN: %check_clang_tidy %s cert-dcl21-cpp %t + ---------------- aaron.ballman wrote: > xazax.hun wrote: > > aaron.ballman wrote: > > > alexfh wrote: > > > > As usual, please add tests with macros and templates with multiple > > > > instantiations. When diagnostics in macros are ignored, the tests > > > > should demonstrate this as well. > > > In addition to these tests, I'd like to see a test for the following: > > > ``` > > > struct S {}; > > > typedef S& SRef; > > > > > > SRef operator++(SRef, int); > > > > > > struct T { > > > typedef T& TRef; > > > > > > TRef operator++(int); > > > }; > > > > > > struct U { > > > typedef const U& ConstURef; > > > > > > ConstURef& operator++(int); // There's an extra ref shoved on here for > > > fun. > > > }; > > > > > > struct V { > > > V *operator++(int); > > > V *const operator--(int); > > > }; > > > ``` > > I added those test cases. > > > > I choose not to warn about primitive and pointer types since it makes no > > sense to make them const. Do you agree? > > > > Unfortunately, the current fixits might use the underlying type instead of > > the typedef. Is that a blocker for this to be commited? > > In same cases it is not possible to preserve the typedef, e.g. when the > > underlying type is a reference. (Or we need to insert a > > `std::remove_reference` meta call). > > > > In one case the const is not added. This is because the underlying type is > > already const. Rewriting the fixit is not trivial, because it is not easy > > to get the source range for the whole return type. Unfortunately, > > `getReturnTypeSourceRange` ignores the qualifiers. > > I choose not to warn about primitive and pointer types since it makes no > > sense to make them const. Do you agree? > > I think that's reasonable behavior, yes. > > > Unfortunately, the current fixits might use the underlying type instead of > > the typedef. Is that a blocker for this to be commited? > > That's what I was wondering about. You may want to simply disable the fixit > if the underlying type is a typedef (or alias, etc). > > > In same cases it is not possible to preserve the typedef, e.g. when the > > underlying type is a reference. (Or we need to insert a > > std::remove_reference meta call). > > Yes, I think it's a complex problem that doesn't need to be solved right now. > > > In one case the const is not added. This is because the underlying type is > > already const. Rewriting the fixit is not trivial, because it is not easy > > to get the source range for the whole return type. Unfortunately, > > getReturnTypeSourceRange ignores the qualifiers. > > That is unfortunate, but I don't think it's a deal-breaker, either. Good idea, done. https://reviews.llvm.org/D32743 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits