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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
