[Bug c++/101488] New: Implement p1042r1 __VA_OPT__ placemarker changes

[Bug c++/101488] New: Implement p1042r1 __VA_OPT__ placemarker changes

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101488

            Bug ID: 101488
           Summary: Implement p1042r1 __VA_OPT__ placemarker changes
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jakub at gcc dot gnu.org
  Target Milestone: ---

With the https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575355.html
there are still some pending placemarker changes that need to be done for full
C++20 __VA_OPT__ support.

Testcases include besides c-c++-common/cpp/va-opt* also e.g.
#define A(x,...) a ## __VA_OPT__ (a) ## a
#define B(x,...) a ## __VA_OPT__ () ## a
#define C(x,...) a ## __VA_OPT__ (x) ## a
#define D(x,...) a ## __VA_OPT__ (x##x) ## a
#define E(x,...) a ## __VA_OPT__ (x##x 1) ## a
#define F(x,...) a ## __VA_OPT__ (1 x##x) ## a
v1: A(,)
v2: A(,1)
v3: A(2,1)
v4: B(,)
v5: B(,1)
v6: B(2,1)
v7: C(,)
v8: C(,1)
v9: C(2,1)
v10: D(,)
v11: D(,1)
v12: D(2,1)
v13: E(,)
v14: E(,1)
v15: E(2,1)
v16: F(,)
v17: F(,1)
v18: F(2,1)
or clang/test/Preprocessor/macro_vaopt_expand.cpp and
clang/test/Preprocessor/macro_vaopt_p1042r1.cpp
On the above testcase, trunk with the #__VA_OPT__ patch to clang difference is:
-v14: a1a
+v14: a 1a
-v17: a1a
+v17: a1 a
On the macro_vaopt_expand.cpp testcase, the difference between patched trunk
and clang is
-26: B1
-26_1: B1
+26: B 1
+26_1: B 1
-27: B11
+27: B 11
-28: B11
+28: B 11
and on the macro_vaopt_p1042r1.cpp testcase the difference is:
-4: ab
+4: a b
-4B: ab
+4B: a b

[Bug c++/101488] Implement p1042r1 __VA_OPT__ placemarker changes

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101488

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I've tried a WIP (with just 0 && or commenting out parts of current code
instead of removing/cleaning up) that attempts to treat __VA_OPT__ ( ... ) with
## before __VA_OPT__ and/or after ) more similarly to the case where the
__VA_OPT__, ( and ) tokens are missing:
--- libcpp/macro.c.jj   2021-07-16 11:10:08.512925510 +0200
+++ libcpp/macro.c      2021-07-17 15:55:03.178568895 +0200
@@ -2059,7 +2059,7 @@ replace_args (cpp_reader *pfile, cpp_has

              /* Remove any tail padding from inside the __VA_OPT__.  */
              paste_flag = tokens_buff_last_token_ptr (buff);
-             while (paste_flag && paste_flag != start
+             while (0 && paste_flag && paste_flag != start
                     && (*paste_flag)->type == CPP_PADDING)
                {
                  tokens_buff_remove_last_token (buff);
@@ -2184,7 +2184,7 @@ replace_args (cpp_reader *pfile, cpp_has
                 previous emitted token is at the beginning of __VA_OPT__;
                 placemarkers within __VA_OPT__ are ignored in that case.  */
              else if (arg_tokens_count == 0
-                      && tmp_token_ptr != vaopt_start)
+/*                    && tmp_token_ptr != vaopt_start */)
                paste_flag = tmp_token_ptr;
            }
        }
@@ -2197,7 +2197,7 @@ replace_args (cpp_reader *pfile, cpp_has
                                     MACRO_ARG_TOKEN_EXPANDED,
                                     arg, arg->expanded);

-         if (last_token_is (buff, vaopt_start))
+         if (0 && last_token_is (buff, vaopt_start))
            {
              /* We're expanding an arg at the beginning of __VA_OPT__.
                 Skip padding. */
@@ -2215,7 +2215,7 @@ replace_args (cpp_reader *pfile, cpp_has
       /* Padding on the left of an argument (unless RHS of ##).  */
       if ((!pfile->state.in_directive || pfile->state.directive_wants_padding)
          && src != macro->exp.tokens && !(src[-1].flags & PASTE_LEFT)
-         && !last_token_is (buff, vaopt_start))
+         /* && !last_token_is (buff, vaopt_start) */)
        {
          const cpp_token *t = padding_token (pfile, src);
          unsigned index = expanded_token_index (pfile, macro, src, i);
@@ -2301,7 +2301,7 @@ replace_args (cpp_reader *pfile, cpp_has

       /* Avoid paste on RHS (even case count == 0).  */
       if (!pfile->state.in_directive && !(src->flags & PASTE_LEFT)
-         && !last_token_is (buff, vaopt_start))
+         /*&& !last_token_is (buff, vaopt_start)*/)
        {
          const cpp_token *t = &pfile->avoid_paste;
          tokens_buff_add_token (buff, virt_locs,
@@ -3544,6 +3544,8 @@ create_iso_definition (cpp_reader *pfile
   bool varadic = false;
   bool ok = false;
   cpp_macro *macro = NULL;
+  bool vaopt_after_paste = false;
+  unsigned int vaopt_end_idx = 0;

   /* Look at the first token, to see if this is a function-like
      macro.   */
@@ -3700,14 +3702,33 @@ create_iso_definition (cpp_reader *pfile
                token[-1].flags |= SP_DIGRAPH;
              if (token->flags & PREV_WHITE)
                token[-1].flags |= SP_PREV_WHITE;
+             if (macro->count == vaopt_end_idx)
+               token[-2].flags |= PASTE_LEFT;
            }
          following_paste_op = true;
        }
       else
        following_paste_op = false;

-      if (vaopt_tracker.update (token) == vaopt_state::ERROR)
+      vaopt_state::update_type vostate = vaopt_tracker.update (token);
+      if (vostate == vaopt_state::INCLUDE)
+       continue;
+      if (vostate == vaopt_state::ERROR)
        goto out;
+      if (vostate == vaopt_state::BEGIN
+         && macro->count > 1
+         && (token[-1].flags & PASTE_LEFT))
+       vaopt_after_paste = true;
+      if (vostate == vaopt_state::DROP && vaopt_after_paste)
+       {
+         vaopt_after_paste = false;
+         /* Duplicate the PASTE_LEFT flag on ( after __VA_OPT__
+            if ## is before __VA_OPT__.  */
+         if (!vaopt_tracker.stringify ())
+           token->flags |= PASTE_LEFT;
+       }
+      if (vostate == vaopt_state::END)
+       vaopt_end_idx = macro->count;
     }

   /* We're committed to winning now.  */

and that results into identical output on the #c0 testcase above as clang++,
and ditto macro_vaopt_p1042r1.cpp testcase, but macro_vaopt_expand.cpp differs
slightly:
-9: HT_A() C ( ) B ( ) "A()"
+9: TONG C ( ) B ( ) "A()"
-27_1: BA0 11
+27_1: BexpandedA0 11
(- is gcc with #__VA_OPT__ and the above patch, + is clang++).  That actually
matches how the F0 case in
#define LPAREN ( 
#define A0 expandedA0
#define A1  expandedA1 A0
#define A2  expandedA2 A1
#define A3  expandedA3 A2
#define A() B LPAREN )
#define B() C LPAREN )
#define C() D LPAREN )
#define HT_B() TONG
#define F0(x, ...) HT_ ## x x A()  #x
#define F(x, ...) HT_ ## __VA_OPT__(x x A()  #x)
8: F0(A(),1)
9: F(A(),1)
is handled, so I'm getting lost on what is supposed to be special about
__VA_OPT__ here.  In any case, the unpatched libcpp handled that F0 and F cases
the same as clang++.  And we probably need more extensive testsuite coverage...

[Bug c++/101488] Implement p1042r1 __VA_OPT__ placemarker changes

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101488

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Different patch:
--- libcpp/macro.c.jj   2021-07-16 11:10:08.512925510 +0200
+++ libcpp/macro.c      2021-07-19 14:19:48.217122675 +0200
@@ -236,10 +236,20 @@ class vaopt_state {
     return m_state == 0;
   }

+  /* Return true for # __VA_OPT__.  */
   bool stringify () const
   {
     return m_stringify;
-  }  
+  }
+
+  /* Return true if inside of __VA_OPT__ args but not inside of extra ()s
+     nested in it, i.e. when if next token is ), it will be END.
+     Next token is guaranteed to appear, as otherwise it would be ERROR
+     earlier.  */
+  bool maybe_before_end () const
+  {
+    return m_state == 3;
+  }

  private:

@@ -2025,6 +2035,7 @@ replace_args (cpp_reader *pfile, cpp_has
   i = 0;
   vaopt_state vaopt_tracker (pfile, macro->variadic, &args[macro->paramc -
1]);
   const cpp_token **vaopt_start = NULL;
+  bool vaopt_paste_left = false;
   for (src = macro->exp.tokens; src < limit; src++)
     {
       unsigned int arg_tokens_count;
@@ -2050,6 +2061,8 @@ replace_args (cpp_reader *pfile, cpp_has
                                         t->src_loc, t->src_loc,
                                         map, index);
                }
+              vaopt_paste_left = (src != macro->exp.tokens
+                                 && (src[-1].flags & PASTE_LEFT));
              vaopt_start = tokens_buff_last_token_ptr (buff);
            }
          else if (vostate == vaopt_state::END)
@@ -2059,7 +2072,7 @@ replace_args (cpp_reader *pfile, cpp_has

              /* Remove any tail padding from inside the __VA_OPT__.  */
              paste_flag = tokens_buff_last_token_ptr (buff);
-             while (paste_flag && paste_flag != start
+             while (0 && paste_flag && paste_flag != start
                     && (*paste_flag)->type == CPP_PADDING)
                {
                  tokens_buff_remove_last_token (buff);
@@ -2184,7 +2197,7 @@ replace_args (cpp_reader *pfile, cpp_has
                 previous emitted token is at the beginning of __VA_OPT__;
                 placemarkers within __VA_OPT__ are ignored in that case.  */
              else if (arg_tokens_count == 0
-                      && tmp_token_ptr != vaopt_start)
+/*                    && tmp_token_ptr != vaopt_start */)
                paste_flag = tmp_token_ptr;
            }
        }
@@ -2214,8 +2227,9 @@ replace_args (cpp_reader *pfile, cpp_has

       /* Padding on the left of an argument (unless RHS of ##).  */
       if ((!pfile->state.in_directive || pfile->state.directive_wants_padding)
-         && src != macro->exp.tokens && !(src[-1].flags & PASTE_LEFT)
-         && !last_token_is (buff, vaopt_start))
+         && src != macro->exp.tokens
+         && !(src[-1].flags & PASTE_LEFT)
+         && (!last_token_is (buff, vaopt_start) || !vaopt_paste_left))
        {
          const cpp_token *t = padding_token (pfile, src);
          unsigned index = expanded_token_index (pfile, macro, src, i);
@@ -2300,8 +2314,12 @@ replace_args (cpp_reader *pfile, cpp_has
                     NODE_NAME (node), src->val.macro_arg.arg_no);

       /* Avoid paste on RHS (even case count == 0).  */
-      if (!pfile->state.in_directive && !(src->flags & PASTE_LEFT)
-         && !last_token_is (buff, vaopt_start))
+      if (!pfile->state.in_directive
+         && !(src->flags & PASTE_LEFT)
+         && (!vaopt_tracker.maybe_before_end ()
+             || src[1].type != CPP_CLOSE_PAREN
+             || (src[1].flags & PASTE_LEFT) == 0)
+         /*&& !last_token_is (buff, vaopt_start)*/)
        {
          const cpp_token *t = &pfile->avoid_paste;
          tokens_buff_add_token (buff, virt_locs,
With this one, clang's macro_vaopt_expand.cpp and macro_vaopt_p1042r1.cpp and
the #c0 testcase preprocess the same between g++ and clang++, but
c-c++-common/cpp/va-opt-3.c has one difference:
t26 f20 (f21 (), 2);
/* { dg-final { scan-file va-opt-3.i "t26 f17 h;" } } */
which yields with the patched cc1plus into
t26 f17 f17 m4();

I guess I understand why for the cases of x ## __VA_OPT__ (y z) and __VA_OPT__
(a b) ## c we want
for y and b macros use MACRO_ARG_TOKEN_EXPANDED instead of
MACRO_ARG_TOKEN_NORMAL when it would be
x ## y z or a b ## c, but already in the above patch I had to uncomment again
the
          if (last_token_is (buff, vaopt_start))
            {
              /* We're expanding an arg at the beginning of __VA_OPT__.
                 Skip padding. */
              while (arg_tokens_count)
                {
                  const cpp_token *t = macro_arg_token_iter_get_token (&from);
                  if (t->type != CPP_PADDING)
                    break;
                  macro_arg_token_iter_forward (&from);
                  --arg_tokens_count;
                }
            }
padding token removal because otherwise e.g. the #c1 testcase (and
macro_vaopt_expand.cpp) misbehaves - MACRO_ARG_TOKEN_EXPANDED
tokens for the x macro start with CPP_PADDING and at least if clang++ is
correct we don't want it there.
The question is if we should optimize away a single CPP_PADDING tokens or
unlimited number of them, whether it is possible
that somehow the macro expanded definition would start with multiple
CPP_PADDING tokens and whether some of them shouldn't be kept.
For the t26 case, we need to remove some trailing CPP_PADDING tokens too, but
as various testcases show, not all of them.
So, perhaps remember in the loop in some integer variable how many CPP_PADDING
tokens should be removed if next token would be vaopt_state::END
with PASTE_LEFT on it?

[Bug c++/101488] Implement p1042r1 __VA_OPT__ placemarker changes

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101488

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 51172
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51172&action=edit
gcc12-pr101488.patch

Another version of the patch, this one passes the clang macro_vaopt_* tests and
preprocesses the #c0 testcase (now extended into va-opt-7.c) and other tests in
gcc testsuite the same.

[Bug c++/101488] Implement p1042r1 __VA_OPT__ placemarker changes

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101488

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:e928cf47f350e46eacb48ed954112e603ef3800a

commit r12-3298-ge928cf47f350e46eacb48ed954112e603ef3800a
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Sep 1 21:31:25 2021 +0200

    libcpp: __VA_OPT__ p1042r1 placemarker changes [PR101488]

    So, besides missing #__VA_OPT__ patch for which I've posted patch last
week,
    P1042R1 introduced some placemarker changes for __VA_OPT__, most notably
    the addition of before "removal of placemarker tokens," rescanning ...
    and the
     #define H4(X, ...) __VA_OPT__(a X ## X) ## b
    H4(, 1)  // replaced by a b
    example mentioned there where we replace it currently with ab

    The following patch are the minimum changes (except for the
    __builtin_expect) that achieve the same preprocessing between current
    clang++ and patched gcc on all the testcases I've tried (i.e. gcc
__VA_OPT__
    testsuite in c-c++-common/cpp/va-opt* including the new test and the clang
    clang/test/Preprocessor/macro_va_opt* testcases).

    At one point I was trying to implement the __VA_OPT__(args) case as if
    for non-empty __VA_ARGS__ it expanded as if __VA_OPT__( and ) were missing,
    but from the tests it seems that is not how it should work, in particular
    if after (or before) we have some macro argument and it is not followed
    (or preceded) by ##, then it should be macro expanded even when __VA_OPT__
    is after ## or ) is followed by ##.  And it seems that not removing any
    padding tokens isn't possible either, because the expansion of the
arguments
    typically has a padding token at the start and end and those at least
    according to the testsuite need to go.  It is unclear if it would be enough
    to remove just one or if all padding tokens should be removed.
    Anyway, e.g. the previous removal of all padding tokens at the end of
    __VA_OPT__ is undesirable, as it e.g. eats also the padding tokens needed
    for the H4 example from the paper.

    2021-09-01  Jakub Jelinek  <jakub@redhat.com>

            PR preprocessor/101488
            * macro.c (replace_args): Fix up handling of CPP_PADDING tokens at
the
            start or end of __VA_OPT__ arguments when preceeded or followed by
##.

            * c-c++-common/cpp/va-opt-3.c: Adjust expected output.
            * c-c++-common/cpp/va-opt-7.c: New test.

[Bug c++/101488] Implement p1042r1 __VA_OPT__ placemarker changes

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101488

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.