[PATCH] D32743: [clang-tidy] Add new cert-dcl21-cpp check.

[Gábor Horváth via Phabricator via cfe-commits]

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