aaron.ballman 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 {
+};
----------------
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
}
```
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
