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.

+  /* 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.

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.

+     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; 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.

Jason

Reply via email to