[PATCH] D110614: [clang-tidy] Fix false positives in cppcoreguidelines-virtual-class-destructor

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp:216-218
+template <typename T>
+struct TemplatedDerived : PublicVirtualBaseStruct {
+};
----------------
aaron.ballman wrote:
> whisperity wrote:
> > carlosgalvezp wrote:
> > > aaron.ballman wrote:
> > > > I think there are more interesting template test cases that should be checked.
> > > > ```
> > > > // What to do when the derived type is definitely polymorphic
> > > > // but the base class may or may not be?
> > > > template <typename Ty>
> > > > struct Derived : Ty {
> > > >   virtual void func();
> > > > };
> > > > 
> > > > struct S {};
> > > > struct  T { virtual ~T(); };
> > > > 
> > > > void instantiate() {
> > > >   Derived<S> d1; // Diagnose
> > > >   Derived<T> d2; // Don't diagnose
> > > > }
> > > > ```
> > > > 
> > > Very interesting example.
> > > 
> > > The problem is that the diagnostic is shown where `Derived` is, not where the template is instantiated. How to go about that?
> > > 
> > > Seems like more testing and better diagnostics are needed to lower the amount of false positives, I wonder if we should disable the test meanwhile?
> > First, let's try to see if printing, to the user, the matched record, prints `Derived<S>` instead of just `Derived`, irrespective of the location. If the matcher matched the instantiation, the printing/formatting logic should **really** pick that up.
> > 
> > It would be very hard to get to the `VarDecl` with the specific type if your matcher logic starts the top-level matching from the type definitions (`cxxRecordDecl(...)`).
> > 
> > If we **really** want to put some sort of a diagnostic at the location at the places where the type is used, it could be done with another pass over the AST. However, that has an associated runtime cost, and also could create bazillions of `note: used here`-esque messages... But certainly doable. I believe this is `typeLoc`, but `typeLoc` is always a thing I never understand and every time I may end up using it it takes me a lot of reading to understand it for a short time to use it.
> > 
> > ----
> > 
> > If the previous step failed, you could still go around in a much more convoluted way:
> > 
> > However, there is something called a `ClassTemplateSpecializationDecl` (see the AST for @aaron.ballman's example here: http://godbolt.org/z/9qd7d1rs6), which surely should have an associated matcher.
> > 
> > ```
> > | |-ClassTemplateSpecializationDecl <line:1:1, line:4:1> line:2:8 struct Derived definition
> > | | |-DefinitionData polymorphic literal has_constexpr_non_copy_move_ctor can_const_default_init
> > | | | |-DefaultConstructor exists non_trivial constexpr defaulted_is_constexpr
> > | | | |-CopyConstructor simple non_trivial has_const_param implicit_has_const_param
> > | | | |-MoveConstructor exists simple non_trivial
> > | | | |-CopyAssignment simple non_trivial has_const_param implicit_has_const_param
> > | | | |-MoveAssignment exists simple non_trivial
> > | | | `-Destructor simple irrelevant trivial
> > | | |-public 'S':'S'
> > | | |-TemplateArgument type 'S'
> > | | | `-RecordType 'S'
> > | | |   `-CXXRecord 'S'
> > | | |-CXXRecordDecl prev 0x55ebcec4e600 <col:1, col:8> col:8 implicit struct Derived
> > ```
> > 
> > The location for the "template specialisation" is still the location of the primary template (as it is not an //explicit specialisation//), but at least somehow in the AST the information of //"Which type was this class template instantiated with?"// (`S`) is stored, so it is very likely that you can either match on this directly, or if you can't match that way, you could match all of these `ClassTemplateSpecializationDecl`s and check if their type parameter matches a `ProblematicClassOrStruct`.
> > 
> > Of course, this becomes extra nasty the moment we have multiple template parameters, or nested stuff, `template template`s (brrrr...).
> > 
> > This will still not give you the location of the variable definition, but at least you would have in your hand the template specialisation/instantiation instance properly.
> Oh, I may have caused some confusion with the comments in my example, sorry for that! I was imagining the diagnostics would be emitted on the line with the class declaration, but I commented on which instantiation I thought should diagnose. Being more explicit with what I was thinking:
> ```
> // What to do when the derived type is definitely polymorphic
> // but the base class may or may not be?
> template <typename Ty>
> struct Derived : Ty { // Diagnostic emitted here
>   virtual void func();
> };
> 
> struct S {};
> struct  T { virtual ~T(); };
> 
> void instantiate() {
>   Derived<S> d1; // Instantiation causes a diagnostic above
>   Derived<T> d2; // Instantiation does not cause a diagnostic above
> }
> ```
No worries, that was my understanding as well :) I was more thinking about how to tag the expected error/no-error with `CHECK-MESSAGES`. I would need to say that the same line both should get an error and not get an error.

Maybe I will solve it with some duplication creating another class template.

Thanks for the comments, will update asap!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110614/new/

https://reviews.llvm.org/D110614