[RFA] Add support for __VA_OPT__
Commit Message
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.
Comments
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
@@ -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 ...
@@ -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);
}
}
@@ -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
@@ -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" \