On 3/26/20 6:51 PM, Martin Sebor wrote:
On 3/26/20 4:16 PM, Jason Merrill wrote:
On 3/26/20 2:58 PM, Martin Sebor wrote:
On 3/25/20 11:36 PM, Jason Merrill wrote:
On 3/23/20 12:50 PM, Martin Sebor wrote:
On 3/23/20 8:49 AM, Jason Merrill wrote:
On 3/21/20 5:59 PM, Martin Sebor wrote:
+ /* Diagnose class/struct/union mismatches. IS_DECLARATION
is false
+ for alias definition. */
+ bool decl_class = (is_declaration
+ && cp_parser_declares_only_class_p (parser));
cp_parser_check_class_key (parser, key_loc, tag_type,
type, false,
cp_parser_declares_only_class_p (parser));
Don't you need to use the new variable?
Don't your testcases exercise this?
Of course they do. This was a leftover from an experiment after I put
the initial updated patch together. On final review I decided to
adjust
some comments and forgot to restore the original use of the variable.
+ /* When TYPE is the use of an implicit specialization of a
previously
+ declared template set TYPE_DECL to the type of the primary
template
+ for the specialization and look it up in CLASS2LOC below.
For uses
+ of explicit or partial specializations TYPE_DECL already
points to
+ the declaration of the specialization.
+ IS_USE is clear so that the type of an implicit
instantiation rather
+ than that of a partial specialization is determined. */
+ type_decl = TREE_TYPE (type_decl);
+ if (TREE_CODE (type_decl) != TEMPLATE_DECL)
+ type_decl = TYPE_MAIN_DECL (type_decl);
The comment is no longer relevant to the code. The remaining code
also seems like it would have no effect; we already know type_decl
is TYPE_MAIN_DECL (type).
I removed the block of code.
Martin
PS I would have preferred to resolve just the reported problem in this
patch and deal with the template specializations more fully (and with
aliases) in a followup. As it is, it has grown bigger and more
complex
than I'm comfortable with, especially with the template
specializations,
harder for me to follow, and obviously a lot more time-consuming not
just to put together but also to review. Although this revision
handles
many more template specialization cases correctly, there still are
other
(arguably corner) cases that it doesn't. I suspect getting those
right
might even require a design change, which I see as out of scope at
this
time (not to mention my ability).
Sure, at this point in the cycle there's always a tradeoff between
better functionality and risk from ballooning changes. It looks
like the improved template handling could still be split out into a
separate patch, if you'd prefer.
I would prefer to get this patch committed as is now. I appreciate
there are improvements that can be made to the code (there always
are) but, unlike the bugs it fixes, they are invisible to users and
so don't seem essential at this point.
+ /* Number of usesn of the class. */
Typo.
+ definintion if one exists or the first declaration
otherwise. */
typo.
+ if (CLASSTYPE_USE_TEMPLATE (type) == 1 && !is_decl (0))
...
+ the first reference to the instantiation. The primary must
+ be (and inevitably is) at index zero. */
I think CLASSTYPE_IMPLICIT_INSTANTIATION is more readable than
testing the value 1.
Okay.
What is the !is_decl (0) intended to do? Changing it to an assert
inside the 'if' doesn't seem to affect any of the testcases.
Looks like the test is an unnecessary remnant and can be removed.
In fact, both is_decl() and decl_p member don't appear necessary
anymore so I've removed them too.
+ For implicit instantiations of a primary template it's
+ the class-key used to declare the primary with. The primary
+ must be at index zero. */
+ const tag_types xpect_key
+ = cdlguide->class_key (cdlguide == this ? idxguide : 0);
A template can also be declared before it's defined;
Obviously, just like a class. Is there something you expect me to
change in response to this point?
You're hardcoding index zero for the template case, which seems to
assume that the definition of a template is always at index zero.
Sounds like you're concerned about something like:
template <class> class A;
template <class T> class A<T*>; // expect -Wmismatched-tags
template <class T> struct A<T*> { }; // ...because of this
class A<int*> a; // expect -Wmismatched-tags
The definition should be at index zero because once it's seen, entries
for prior declarations are purged (after diagnosing mismatches). But
I'm sure the tests for these things could stand to beefed up so there
could easily be cases that aren't handled correctly.
I think you want to move the def_p/idxdef/idxguide logic into
another member function that you invoke on cdlguide to perhaps get
the class_key_loc_t that you want to use as the pattern.
I'm not quite sure what you have in mind here. I agree the cdlcode
code looks a little cumbersome and perhaps could be restructured but
it's not obvious to me how. Nothing I tried looked like a clear win
so unless you consider changing how this is done a prerequisite for
accepting the whole patch I'd rather not spend any more time at this
stage iterating over refinements of it. Please let me know soon.
I mean that
+ const unsigned ndecls = locvec.length (); > + const bool def_p =
idxdef < ndecls;
+ const unsigned idxguide = def_p ? idxdef : 0;
are based on whether the instantiation, rather than the template, is
defined.
I's probably enough to update ndecls to cdlguide->locvec.length() and
change the uses of idxdef to cdlguide->idxdef. And then idxguide will
be set properly for cdlguide, so the code farther above can become
+ const tag_types xpect_key
+ = cdlguide->class_key (idxguide);
If you'd prefer, I can make these changes and commit the patch myself.
Please go ahead. That will make it a lot quicker.
Done. Here are the changes I made.
Jason
commit 882a90dbfdd776ce33749ae4a44dc89588417a63
Author: Jason Merrill <ja...@redhat.com>
Date: Fri Mar 27 09:56:13 2020 -0400
set def_p and idxguide based on cdlguide
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 2f539570aae..3ca8eb9baf8 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -31024,6 +31024,12 @@ private:
return locvec[i].class_key;
}
+ /* True if a definition for the class has been seen. */
+ bool def_p () const
+ {
+ return idxdef < locvec.length ();
+ }
+
/* The location of a single mention of a class type with the given
class-key. */
struct class_key_loc_t
@@ -31273,11 +31279,6 @@ class_decl_loc_t::diag_mismatched_tags (tree type_decl)
/* Number of uses of the class. */
const unsigned ndecls = locvec.length ();
- if (ndecls < 2)
- return;
-
- /* Set if a definition for the class has been seen. */
- const bool def_p = idxdef < ndecls;
/* The class (or template) declaration guiding the decisions about
the diagnostic. For ordinary classes it's the same as THIS. For
@@ -31305,17 +31306,13 @@ class_decl_loc_t::diag_mismatched_tags (tree type_decl)
return;
}
+ /* Set if a definition for the class has been seen. */
+ const bool def_p = cdlguide->def_p ();
+
/* The index of the declaration whose class-key this declaration
is expected to match. It's either the class-key of the class
definition if one exists or the first declaration otherwise. */
- const unsigned idxguide = def_p ? idxdef : 0;
- unsigned idx = 0;
- /* Advance IDX to the first declaration that either is not
- a definition or that doesn't match the first declaration
- if no definition is provided. */
- while (class_key (idx) == cdlguide->class_key (idxguide))
- if (++idx == ndecls)
- return;
+ const unsigned idxguide = def_p ? cdlguide->idxdef : 0;
/* The class-key the class is expected to be declared with: it's
either the key used in its definition or the first declaration
@@ -31323,10 +31320,15 @@ class_decl_loc_t::diag_mismatched_tags (tree type_decl)
For implicit instantiations of a primary template it's
the class-key used to declare the primary with. The primary
must be at index zero. */
- const tag_types xpect_key
- = cdlguide->class_key (cdlguide == this ? idxguide : 0);
- if (cdlguide != this && xpect_key == class_key (idx))
- return;
+ const tag_types xpect_key = cdlguide->class_key (idxguide);
+
+ unsigned idx = 0;
+ /* Advance IDX to the first declaration that either is not
+ a definition or that doesn't match the first declaration
+ if no definition is provided. */
+ while (class_key (idx) == xpect_key)
+ if (++idx == ndecls)
+ return;
/* Save the current function before changing it below. */
tree save_func = current_function_decl;
