[RFA] Add support for __VA_OPT__

[RFA] Add support for __VA_OPT__

[Tom Tromey]

C++2a adds a "__VA_OPT__" feature that can be used to control the
pesky "," emission when the final (variable) argument of a variadic
macro is empty.  This patch implements this feature for gdb.  (A patch
to implement it for gcc is pending.)

gdb/ChangeLog
2017-09-17  Tom Tromey  <tom@tromey.com>

	* macroexp.c (get_next_token_for_substitution): New function.
	(substitute_args): Call it.  Check for __VA_OPT__.

gdb/testsuite/ChangeLog
2017-09-17  Tom Tromey  <tom@tromey.com>

	* gdb.base/macscp.exp: Add __VA_OPT__ tests.
---
 gdb/ChangeLog                     |  5 +++
 gdb/macroexp.c                    | 73 +++++++++++++++++++++++++++++++++++----
 gdb/testsuite/ChangeLog           |  4 +++
 gdb/testsuite/gdb.base/macscp.exp | 12 +++++++
 4 files changed, 87 insertions(+), 7 deletions(-)

diff --git a/gdb/macroexp.c b/gdb/macroexp.c
index e7a0dad..369fc78 100644
--- a/gdb/macroexp.c
+++ b/gdb/macroexp.c
@@ -946,6 +946,22 @@ find_parameter (const struct macro_buffer *tok,
   return -1;
 }
  
+/* Helper function for substitute_args that gets the next token and
+   updates the passed-in state variables.  */
+
+static int
+get_next_token_for_substitution (struct macro_buffer *replacement_list,
+				 struct macro_buffer *token,
+				 char **start,
+				 struct macro_buffer *lookahead,
+				 char **lookahead_start)
+{
+  *token = *lookahead;
+  *start = *lookahead_start;
+  *lookahead_start = replacement_list->text;
+  return get_token (lookahead, replacement_list);
+}
+
 /* Given the macro definition DEF, being invoked with the actual
    arguments given by ARGC and ARGV, substitute the arguments into the
    replacement list, and store the result in DEST.
@@ -996,8 +1012,57 @@ substitute_args (struct macro_buffer *dest,
   lookahead_rl_start = replacement_list.text;
   lookahead_valid = get_token (&lookahead, &replacement_list);
 
-  for (;;)
+  /* __VA_OPT__ state variable.  The states are:
+     0 - nothing happening
+     1 - saw __VA_OPT__
+     >= 2 in __VA_OPT__, the value encodes the parenthesis depth.  */
+  unsigned vaopt_state = 0;
+
+  for (;;
+       lookahead_valid = get_next_token_for_substitution (&replacement_list,
+							  &tok,
+							  &original_rl_start,
+							  &lookahead,
+							  &lookahead_rl_start))
     {
+      if (vaopt_state > 0)
+	{
+	  if (tok.len == 1)
+	    {
+	      if (tok.text[0] == '(')
+		{
+		  ++vaopt_state;
+		  /* We just entered __VA_OPT__, so don't emit this
+		     token.  */
+		  continue;
+		}
+	      else if (tok.text[0] == ')')
+		{
+		  --vaopt_state;
+		  if (vaopt_state == 1)
+		    {
+		      /* Done with __VA_OPT__.  */
+		      vaopt_state = 0;
+		      /* Don't emit.  */
+		      continue;
+		    }
+		}
+	    }
+
+	  /* If __VA_ARGS__ is empty, then drop the contents of
+	     __VA_OPT__.  */
+	  if (argv[argc - 1].len == 0)
+	    continue;
+	}
+      else if (is_varargs
+	       && tok.len == 10
+	       && strncmp (tok.text, "__VA_OPT__", 10) == 0)
+	{
+	  vaopt_state = 1;
+	  /* Don't emit this token.  */
+	  continue;
+	}
+
       /* Just for aesthetics.  If we skipped some whitespace, copy
          that to DEST.  */
       if (tok.text > original_rl_start)
@@ -1160,12 +1225,6 @@ substitute_args (struct macro_buffer *dest,
 
       if (! lookahead_valid)
 	break;
-
-      tok = lookahead;
-      original_rl_start = lookahead_rl_start;
-
-      lookahead_rl_start = replacement_list.text;
-      lookahead_valid = get_token (&lookahead, &replacement_list);
     }
 }
 
diff --git a/gdb/testsuite/gdb.base/macscp.exp b/gdb/testsuite/gdb.base/macscp.exp
index 54b5ab2..a08ec46 100644
--- a/gdb/testsuite/gdb.base/macscp.exp
+++ b/gdb/testsuite/gdb.base/macscp.exp
@@ -620,6 +620,10 @@ gdb_test_no_output "macro define va_gnu(args...) varfunc (fixedarg, args)" \
 gdb_test_no_output "macro define va2_gnu(args...) varfunc (fixedarg, ## args)" \
   "define fourth varargs helper"
 
+gdb_test_no_output \
+    "macro define va3_cxx2a(x, ...) varfunc (x __VA_OPT__(,) __VA_ARGS__)" \
+    "define fifth varargs helper"
+
 gdb_test "macro expand va_c99(one, two, three)" \
   "expands to: *varfunc \\(fixedarg, *one, two, three\\)" \
   "c99 varargs expansion"
@@ -644,6 +648,14 @@ gdb_test "macro expand va2_gnu()" \
   "expands to: *varfunc \\(fixedarg\\)" \
   "gnu varargs expansion special splicing without an argument"
 
+gdb_test "macro expand va3_cxx2a(23)" \
+    "expands to: *varfunc \\(23 \\)" \
+    "C++2a __VA_OPT__ handling without variable argument"
+
+gdb_test "macro expand va3_cxx2a(23, 24, 25)" \
+    "expands to: *varfunc \\(23, 24, 25\\)" \
+    "C++2a __VA_OPT__ handling with variable argument"
+
 # Stringification tests.
 
 gdb_test_no_output "macro define str(x) #x" \
-- 
2.9.4

Re: [RFA] Add support for __VA_OPT__

[Pedro Alves]

W00t, thanks for doing this (and the gcc side as well!)

On 09/18/2017 03:42 AM, Tom Tromey wrote:

> diff --git a/gdb/testsuite/gdb.base/macscp.exp b/gdb/testsuite/gdb.base/macscp.exp
> index 54b5ab2..a08ec46 100644
> --- a/gdb/testsuite/gdb.base/macscp.exp
> +++ b/gdb/testsuite/gdb.base/macscp.exp
> @@ -620,6 +620,10 @@ gdb_test_no_output "macro define va_gnu(args...) varfunc (fixedarg, args)" \
>  gdb_test_no_output "macro define va2_gnu(args...) varfunc (fixedarg, ## args)" \
>    "define fourth varargs helper"
>  
> +gdb_test_no_output \
> +    "macro define va3_cxx2a(x, ...) varfunc (x __VA_OPT__(,) __VA_ARGS__)" \
> +    "define fifth varargs helper"
> +
>  gdb_test "macro expand va_c99(one, two, three)" \
>    "expands to: *varfunc \\(fixedarg, *one, two, three\\)" \
>    "c99 varargs expansion"
> @@ -644,6 +648,14 @@ gdb_test "macro expand va2_gnu()" \
>    "expands to: *varfunc \\(fixedarg\\)" \
>    "gnu varargs expansion special splicing without an argument"
>  
> +gdb_test "macro expand va3_cxx2a(23)" \
> +    "expands to: *varfunc \\(23 \\)" \
> +    "C++2a __VA_OPT__ handling without variable argument"
> +
> +gdb_test "macro expand va3_cxx2a(23, 24, 25)" \
> +    "expands to: *varfunc \\(23, 24, 25\\)" \
> +    "C++2a __VA_OPT__ handling with variable argument"
> +

The patch looks good to me, though I think it'd be nice to see
tests that make sure that ill-formed input doesn't send us
to the weeds.  Like:

 - does the state machine handle "__VA_OPT__)" gracefully?
   I.e., ')' before '('.
 - similarly: "__VA_OPT__)(,)"
 - does the state machine handle multiple occurrences of
   __VA_OPT__ in the same macro expansion?  It looks like
   it, but....

Also, does this handle:

 __VA_OPT__(__VA_ARGS__)

correctly?  I think so, but...

Thanks,
Pedro Alves

Re: [RFA] Add support for __VA_OPT__

[Tom Tromey]

Pedro> The patch looks good to me, though I think it'd be nice to see
Pedro> tests that make sure that ill-formed input doesn't send us
Pedro> to the weeds.  Like:
Pedro>  - does the state machine handle "__VA_OPT__)" gracefully?
Pedro>    I.e., ')' before '('.
Pedro>  - similarly: "__VA_OPT__)(,)"
Pedro>  - does the state machine handle multiple occurrences of
Pedro>    __VA_OPT__ in the same macro expansion?  It looks like
Pedro>    it, but....
Pedro> Also, does this handle:
Pedro>  __VA_OPT__(__VA_ARGS__)
Pedro> correctly?  I think so, but...

I've added all of these.

But I wonder if gdb should just error() on the invalid ones.
My first thought was no, why make life harder -- but at the same time,
the invalid cases really aren't that useful either.

Tom

commit 635411e81e8ce24fcc8887755b1520baf9044205
Author: Tom Tromey <tom@tromey.com>
Date:   Sun Sep 17 20:36:41 2017 -0600

    Add support for __VA_OPT__
    
    C++2a adds a "__VA_OPT__" feature that can be used to control the
    pesky "," emission when the final (variable) argument of a variadic
    macro is empty.  This patch implements this feature for gdb.  (A patch
    to implement it for gcc is pending.)
    
    gdb/ChangeLog
    2017-09-17  Tom Tromey  <tom@tromey.com>
    
            * macroexp.c (get_next_token_for_substitution): New function.
            (substitute_args): Call it.  Check for __VA_OPT__.
    
    gdb/testsuite/ChangeLog
    2017-09-17  Tom Tromey  <tom@tromey.com>
    
            * gdb.base/macscp.exp: Add __VA_OPT__ tests.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 4de6da2..34c9810 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2017-09-17  Tom Tromey  <tom@tromey.com>
+
+	* macroexp.c (get_next_token_for_substitution): New function.
+	(substitute_args): Call it.  Check for __VA_OPT__.
+
 2017-09-20  Pedro Alves  <palves@redhat.com>
 
 	* eval.c (make_params): Delete, refactored as ...
diff --git a/gdb/macroexp.c b/gdb/macroexp.c
index e7a0dad..369fc78 100644
--- a/gdb/macroexp.c
+++ b/gdb/macroexp.c
@@ -946,6 +946,22 @@ find_parameter (const struct macro_buffer *tok,
   return -1;
 }
  
+/* Helper function for substitute_args that gets the next token and
+   updates the passed-in state variables.  */
+
+static int
+get_next_token_for_substitution (struct macro_buffer *replacement_list,
+				 struct macro_buffer *token,
+				 char **start,
+				 struct macro_buffer *lookahead,
+				 char **lookahead_start)
+{
+  *token = *lookahead;
+  *start = *lookahead_start;
+  *lookahead_start = replacement_list->text;
+  return get_token (lookahead, replacement_list);
+}
+
 /* Given the macro definition DEF, being invoked with the actual
    arguments given by ARGC and ARGV, substitute the arguments into the
    replacement list, and store the result in DEST.
@@ -996,8 +1012,57 @@ substitute_args (struct macro_buffer *dest,
   lookahead_rl_start = replacement_list.text;
   lookahead_valid = get_token (&lookahead, &replacement_list);
 
-  for (;;)
+  /* __VA_OPT__ state variable.  The states are:
+     0 - nothing happening
+     1 - saw __VA_OPT__
+     >= 2 in __VA_OPT__, the value encodes the parenthesis depth.  */
+  unsigned vaopt_state = 0;
+
+  for (;;
+       lookahead_valid = get_next_token_for_substitution (&replacement_list,
+							  &tok,
+							  &original_rl_start,
+							  &lookahead,
+							  &lookahead_rl_start))
     {
+      if (vaopt_state > 0)
+	{
+	  if (tok.len == 1)
+	    {
+	      if (tok.text[0] == '(')
+		{
+		  ++vaopt_state;
+		  /* We just entered __VA_OPT__, so don't emit this
+		     token.  */
+		  continue;
+		}
+	      else if (tok.text[0] == ')')
+		{
+		  --vaopt_state;
+		  if (vaopt_state == 1)
+		    {
+		      /* Done with __VA_OPT__.  */
+		      vaopt_state = 0;
+		      /* Don't emit.  */
+		      continue;
+		    }
+		}
+	    }
+
+	  /* If __VA_ARGS__ is empty, then drop the contents of
+	     __VA_OPT__.  */
+	  if (argv[argc - 1].len == 0)
+	    continue;
+	}
+      else if (is_varargs
+	       && tok.len == 10
+	       && strncmp (tok.text, "__VA_OPT__", 10) == 0)
+	{
+	  vaopt_state = 1;
+	  /* Don't emit this token.  */
+	  continue;
+	}
+
       /* Just for aesthetics.  If we skipped some whitespace, copy
          that to DEST.  */
       if (tok.text > original_rl_start)
@@ -1160,12 +1225,6 @@ substitute_args (struct macro_buffer *dest,
 
       if (! lookahead_valid)
 	break;
-
-      tok = lookahead;
-      original_rl_start = lookahead_rl_start;
-
-      lookahead_rl_start = replacement_list.text;
-      lookahead_valid = get_token (&lookahead, &replacement_list);
     }
 }
 
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 721bd81..3e16b5a 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2017-09-17  Tom Tromey  <tom@tromey.com>
+
+	* gdb.base/macscp.exp: Add __VA_OPT__ tests.
+
 2017-09-20  Pedro Alves  <palves@redhat.com>
 
 	* gdb.base/list-ambiguous.exp (test_list_ambiguous_symbol): Expect
diff --git a/gdb/testsuite/gdb.base/macscp.exp b/gdb/testsuite/gdb.base/macscp.exp
index 54b5ab2..96674f3 100644
--- a/gdb/testsuite/gdb.base/macscp.exp
+++ b/gdb/testsuite/gdb.base/macscp.exp
@@ -620,6 +620,18 @@ gdb_test_no_output "macro define va_gnu(args...) varfunc (fixedarg, args)" \
 gdb_test_no_output "macro define va2_gnu(args...) varfunc (fixedarg, ## args)" \
   "define fourth varargs helper"
 
+gdb_test_no_output \
+    "macro define va3_cxx2a(x, ...) varfunc (x __VA_OPT__(,) __VA_ARGS__)" \
+    "define fifth varargs helper"
+
+gdb_test_no_output \
+    "macro define va4_cxx2a(x, ...) varfunc (x __VA_OPT__(, __VA_ARGS__))" \
+    "define sixth varargs helper"
+
+gdb_test_no_output \
+    "macro define va5_cxx2a(x, ...) varfunc (x __VA_OPT__(,) __VA_OPT__(__VA_ARGS__))" \
+    "define seventh varargs helper"
+
 gdb_test "macro expand va_c99(one, two, three)" \
   "expands to: *varfunc \\(fixedarg, *one, two, three\\)" \
   "c99 varargs expansion"
@@ -644,6 +656,50 @@ gdb_test "macro expand va2_gnu()" \
   "expands to: *varfunc \\(fixedarg\\)" \
   "gnu varargs expansion special splicing without an argument"
 
+gdb_test "macro expand va3_cxx2a(23)" \
+    "expands to: *varfunc \\(23 \\)" \
+    "C++2a __VA_OPT__ handling without variable argument"
+
+gdb_test "macro expand va3_cxx2a(23, 24, 25)" \
+    "expands to: *varfunc \\(23, 24, 25\\)" \
+    "C++2a __VA_OPT__ handling with variable argument"
+
+gdb_test "macro expand va4_cxx2a(23, 24, 25)" \
+    "expands to: *varfunc \\(23, 24, 25\\)" \
+    "C++2a __VA_OPT__ conditional __VA_ARGS__ handling with variable argument"
+
+gdb_test "macro expand va4_cxx2a(23)" \
+    "expands to: *varfunc \\(23\\)" \
+    "C++2a __VA_OPT__ conditional __VA_ARGS__ handling without variable argument"
+
+gdb_test "macro expand va5_cxx2a(23, 24, 25)" \
+    "expands to: *varfunc \\(23,24, 25\\)" \
+    "C++2a double __VA_OPT__ conditional __VA_ARGS__ handling with variable argument"
+
+gdb_test "macro expand va5_cxx2a(23)" \
+    "expands to: *varfunc \\(23\\)" \
+    "C++2a double __VA_OPT__ conditional __VA_ARGS__ handling without variable argument"
+
+#
+# Invalid __VA_OPT__ tests are not intended to define gdb's output,
+# but rather to ensure that gdb at least doesn't crash when presented
+# with something invalid.
+#
+
+gdb_test_no_output \
+    "macro define badopt1(x, ...) __VA_OPT__) x" \
+    "define first invalid varargs helper"
+gdb_test "macro expand badopt1(5)" \
+    "expands to:  5" \
+    "Test that first invalid __VA_OPT__ doesn't crash"
+
+gdb_test_no_output \
+    "macro define badopt1(x, ...) __VA_OPT__)(,) x" \
+    "define second invalid varargs helper"
+gdb_test "macro expand badopt1(5)" \
+    "expands to: \\(,\\) 5" \
+    "Test that second invalid __VA_OPT__ doesn't crash"
+
 # Stringification tests.
 
 gdb_test_no_output "macro define str(x) #x" \

Re: [RFA] Add support for __VA_OPT__

[Pedro Alves]

On 09/21/2017 04:42 AM, Tom Tromey wrote:
> Pedro> The patch looks good to me, though I think it'd be nice to see
> Pedro> tests that make sure that ill-formed input doesn't send us
> Pedro> to the weeds.  Like:
> Pedro>  - does the state machine handle "__VA_OPT__)" gracefully?
> Pedro>    I.e., ')' before '('.
> Pedro>  - similarly: "__VA_OPT__)(,)"
> Pedro>  - does the state machine handle multiple occurrences of
> Pedro>    __VA_OPT__ in the same macro expansion?  It looks like
> Pedro>    it, but....
> Pedro> Also, does this handle:
> Pedro>  __VA_OPT__(__VA_ARGS__)
> Pedro> correctly?  I think so, but...
> 
> I've added all of these.

Great, thanks!

> 
> But I wonder if gdb should just error() on the invalid ones.
> My first thought was no, why make life harder -- but at the same time,
> the invalid cases really aren't that useful either.

Yeah, error might be better - e.g., for someone trying
to write a "macro define" interactively (without
going via the compiler first), and puzzling about why it
doesn't exactly work [due to some typo].  But we can
decide to do that incrementally.  Fine with me to push
as is if you'd like.

Thanks,
Pedro Alves

Re: [RFA] Add support for __VA_OPT__

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> But I wonder if gdb should just error() on the invalid ones.
>> My first thought was no, why make life harder -- but at the same time,
>> the invalid cases really aren't that useful either.

Pedro> Yeah, error might be better - e.g., for someone trying
Pedro> to write a "macro define" interactively (without
Pedro> going via the compiler first), and puzzling about why it
Pedro> doesn't exactly work [due to some typo].  But we can
Pedro> decide to do that incrementally.  Fine with me to push
Pedro> as is if you'd like.

I've switched it; and good thing, too, because this caught a bug in the
previous patch that could cause an infinite loop.

Tom

commit dbaa7cadd373972d362c10cedc8f34f5cf1d6e2a
Author: Tom Tromey <tom@tromey.com>
Date:   Sun Sep 17 20:36:41 2017 -0600

    Add support for __VA_OPT__
    
    C++2a adds a "__VA_OPT__" feature that can be used to control the
    pesky "," emission when the final (variable) argument of a variadic
    macro is empty.  This patch implements this feature for gdb.  (A patch
    to implement it for gcc is pending.)
    
    ChangeLog
    2017-09-17  Tom Tromey  <tom@tromey.com>
    
            * macroexp.c (get_next_token_for_substitution): New function.
            (substitute_args): Call it.  Check for __VA_OPT__.
    
    testsuite/ChangeLog
    2017-09-17  Tom Tromey  <tom@tromey.com>
    
            * gdb.base/macscp.exp: Add __VA_OPT__ tests.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index aa50dc9..d660fb0 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2017-09-17  Tom Tromey  <tom@tromey.com>
+
+	* macroexp.c (get_next_token_for_substitution): New function.
+	(substitute_args): Call it.  Check for __VA_OPT__.
+
 2017-09-22  Tom Tromey  <tom@tromey.com>
 
 	* utils.c (class scoped_input_handler) <m_quit_handler>: Change
diff --git a/gdb/macroexp.c b/gdb/macroexp.c
index e7a0dad..7d5b876 100644
--- a/gdb/macroexp.c
+++ b/gdb/macroexp.c
@@ -946,6 +946,30 @@ find_parameter (const struct macro_buffer *tok,
   return -1;
 }
  
+/* Helper function for substitute_args that gets the next token and
+   updates the passed-in state variables.  */
+
+static void
+get_next_token_for_substitution (struct macro_buffer *replacement_list,
+				 struct macro_buffer *token,
+				 char **start,
+				 struct macro_buffer *lookahead,
+				 char **lookahead_start,
+				 int *lookahead_valid,
+				 bool *keep_going)
+{
+  if (!*lookahead_valid)
+    *keep_going = false;
+  else
+    {
+      *keep_going = true;
+      *token = *lookahead;
+      *start = *lookahead_start;
+      *lookahead_start = replacement_list->text;
+      *lookahead_valid = get_token (lookahead, replacement_list);
+    }
+}
+
 /* Given the macro definition DEF, being invoked with the actual
    arguments given by ARGC and ARGV, substitute the arguments into the
    replacement list, and store the result in DEST.
@@ -996,8 +1020,67 @@ substitute_args (struct macro_buffer *dest,
   lookahead_rl_start = replacement_list.text;
   lookahead_valid = get_token (&lookahead, &replacement_list);
 
-  for (;;)
+  /* __VA_OPT__ state variable.  The states are:
+     0 - nothing happening
+     1 - saw __VA_OPT__
+     >= 2 in __VA_OPT__, the value encodes the parenthesis depth.  */
+  unsigned vaopt_state = 0;
+
+  /* Whether the loop should keep going.  */
+  bool keep_going = true;
+
+  for (;
+       keep_going;
+       get_next_token_for_substitution (&replacement_list,
+					&tok,
+					&original_rl_start,
+					&lookahead,
+					&lookahead_rl_start,
+					&lookahead_valid,
+					&keep_going))
     {
+      bool token_is_vaopt = (tok.len == 10
+			     && strncmp (tok.text, "__VA_OPT__", 10) == 0);
+
+      if (vaopt_state > 0)
+	{
+	  if (token_is_vaopt)
+	    error (_("__VA_OPT__ cannot appear inside __VA_OPT__"));
+	  else if (tok.len == 1 && tok.text[0] == '(')
+	    {
+	      ++vaopt_state;
+	      /* We just entered __VA_OPT__, so don't emit this
+		 token.  */
+	      continue;
+	    }
+	  else if (vaopt_state == 1)
+	    error (_("__VA_OPT__ must be followed by an open parenthesis"));
+	  else if (tok.len == 1 && tok.text[0] == ')')
+	    {
+	      --vaopt_state;
+	      if (vaopt_state == 1)
+		{
+		  /* Done with __VA_OPT__.  */
+		  vaopt_state = 0;
+		  /* Don't emit.  */
+		  continue;
+		}
+	    }
+
+	  /* If __VA_ARGS__ is empty, then drop the contents of
+	     __VA_OPT__.  */
+	  if (argv[argc - 1].len == 0)
+	    continue;
+	}
+      else if (token_is_vaopt)
+	{
+	  if (!is_varargs)
+	    error (_("__VA_OPT__ is only valid in a variadic macro"));
+	  vaopt_state = 1;
+	  /* Don't emit this token.  */
+	  continue;
+	}
+
       /* Just for aesthetics.  If we skipped some whitespace, copy
          that to DEST.  */
       if (tok.text > original_rl_start)
@@ -1157,16 +1240,10 @@ substitute_args (struct macro_buffer *dest,
 	  if (! substituted)
 	    append_tokens_without_splicing (dest, &tok);
 	}
-
-      if (! lookahead_valid)
-	break;
-
-      tok = lookahead;
-      original_rl_start = lookahead_rl_start;
-
-      lookahead_rl_start = replacement_list.text;
-      lookahead_valid = get_token (&lookahead, &replacement_list);
     }
+
+  if (vaopt_state > 0)
+    error (_("Unterminated __VA_OPT__"));
 }
 
 
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 6584a23..ddc17d2 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2017-09-17  Tom Tromey  <tom@tromey.com>
+
+	* gdb.base/macscp.exp: Add __VA_OPT__ tests.
+
 2017-09-21  Kevin Buettner  <kevinb@redhat.com>
 
 	* gdb.python/py-thrhandle.c, gdb.python/py-thrhandle.exp: New
diff --git a/gdb/testsuite/gdb.base/macscp.exp b/gdb/testsuite/gdb.base/macscp.exp
index 54b5ab2..c5cd899 100644
--- a/gdb/testsuite/gdb.base/macscp.exp
+++ b/gdb/testsuite/gdb.base/macscp.exp
@@ -620,6 +620,18 @@ gdb_test_no_output "macro define va_gnu(args...) varfunc (fixedarg, args)" \
 gdb_test_no_output "macro define va2_gnu(args...) varfunc (fixedarg, ## args)" \
   "define fourth varargs helper"
 
+gdb_test_no_output \
+    "macro define va3_cxx2a(x, ...) varfunc (x __VA_OPT__(,) __VA_ARGS__)" \
+    "define fifth varargs helper"
+
+gdb_test_no_output \
+    "macro define va4_cxx2a(x, ...) varfunc (x __VA_OPT__(, __VA_ARGS__))" \
+    "define sixth varargs helper"
+
+gdb_test_no_output \
+    "macro define va5_cxx2a(x, ...) varfunc (x __VA_OPT__(,) __VA_OPT__(__VA_ARGS__))" \
+    "define seventh varargs helper"
+
 gdb_test "macro expand va_c99(one, two, three)" \
   "expands to: *varfunc \\(fixedarg, *one, two, three\\)" \
   "c99 varargs expansion"
@@ -644,6 +656,58 @@ gdb_test "macro expand va2_gnu()" \
   "expands to: *varfunc \\(fixedarg\\)" \
   "gnu varargs expansion special splicing without an argument"
 
+gdb_test "macro expand va3_cxx2a(23)" \
+    "expands to: *varfunc \\(23 \\)" \
+    "C++2a __VA_OPT__ handling without variable argument"
+
+gdb_test "macro expand va3_cxx2a(23, 24, 25)" \
+    "expands to: *varfunc \\(23, 24, 25\\)" \
+    "C++2a __VA_OPT__ handling with variable argument"
+
+gdb_test "macro expand va4_cxx2a(23, 24, 25)" \
+    "expands to: *varfunc \\(23, 24, 25\\)" \
+    "C++2a __VA_OPT__ conditional __VA_ARGS__ handling with variable argument"
+
+gdb_test "macro expand va4_cxx2a(23)" \
+    "expands to: *varfunc \\(23\\)" \
+    "C++2a __VA_OPT__ conditional __VA_ARGS__ handling without variable argument"
+
+gdb_test "macro expand va5_cxx2a(23, 24, 25)" \
+    "expands to: *varfunc \\(23,24, 25\\)" \
+    "C++2a double __VA_OPT__ conditional __VA_ARGS__ handling with variable argument"
+
+gdb_test "macro expand va5_cxx2a(23)" \
+    "expands to: *varfunc \\(23\\)" \
+    "C++2a double __VA_OPT__ conditional __VA_ARGS__ handling without variable argument"
+
+gdb_test_no_output \
+    "macro define badopt1(x, ...) __VA_OPT__) x" \
+    "define first invalid varargs helper"
+gdb_test "macro expand badopt1(5)" \
+    "__VA_OPT__ must be followed by an open parenthesis" \
+    "__VA_OPT__ without open paren"
+
+gdb_test_no_output \
+    "macro define badopt2(x, ...) __VA_OPT__(__VA_OPT__(,)) x" \
+    "define second invalid varargs helper"
+gdb_test "macro expand badopt2(5)" \
+    "__VA_OPT__ cannot appear inside __VA_OPT__" \
+    "__VA_OPT__ inside __VA_OPT__"
+
+gdb_test_no_output \
+    "macro define badopt3(x) __VA_OPT__" \
+    "define third invalid varargs helper"
+gdb_test "macro expand badopt3(5)" \
+    "__VA_OPT__ is only valid in a variadic macro" \
+    "__VA_OPT__ not in variadic macro"
+
+gdb_test_no_output \
+    "macro define badopt4(x, ...) __VA_OPT__(x" \
+    "define fourth invalid varargs helper"
+gdb_test "macro expand badopt4(5)" \
+    "Unterminated __VA_OPT__" \
+    "__VA_OPT__ without closing paren"
+
 # Stringification tests.
 
 gdb_test_no_output "macro define str(x) #x" \

Re: [RFA] Add support for __VA_OPT__

[Pedro Alves]

On 09/23/2017 04:03 AM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
>>> But I wonder if gdb should just error() on the invalid ones.
>>> My first thought was no, why make life harder -- but at the same time,
>>> the invalid cases really aren't that useful either.
> 
> Pedro> Yeah, error might be better - e.g., for someone trying
> Pedro> to write a "macro define" interactively (without
> Pedro> going via the compiler first), and puzzling about why it
> Pedro> doesn't exactly work [due to some typo].  But we can
> Pedro> decide to do that incrementally.  Fine with me to push
> Pedro> as is if you'd like.
> 
> I've switched it; and good thing, too, because this caught a bug in the
> previous patch that could cause an infinite loop.
> 

Nice.  Patch looks good.  Please push.


> +  /* Whether the loop should keep going.  */
> +  bool keep_going = true;

'keep_going' doesn't appear used after the loop.  Could it be
declared in the for's init statement ?

  for (bool keep_going = true;
       keep_going;
       ...

> +
> +  for (;
> +       keep_going;
> +       get_next_token_for_substitution (&replacement_list,
> +					&tok,
> +					&original_rl_start,
> +					&lookahead,
> +					&lookahead_rl_start,
> +					&lookahead_valid,
> +					&keep_going))
>      {

Thanks,
Pedro Alves

Re: [RFA] Add support for __VA_OPT__

[Tom Tromey]

Pedro> 'keep_going' doesn't appear used after the loop.  Could it be
Pedro> declared in the for's init statement ?

Yes.  I made this change, and I'm going to push it in now.

thanks,
Tom