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.
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.
Jason
