Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for __VA_OPT__ expansion #329

Merged
merged 2 commits into from
Nov 6, 2023
Merged

Conversation

JohnSiegel
Copy link
Contributor

This change adds support for __VA_OPT__ in variadic macros. This should close #191

@firewave
Copy link
Collaborator

Not having check the actual implementation. This should be behind a check if the standard is >= c++20 - possibly similar to what I did in #286.

@JohnSiegel
Copy link
Contributor Author

JohnSiegel commented Oct 25, 2023

Here is an example from the unit tests I added:

From define_va_opt_1:

#define p1(fmt, args...) printf(fmt __VA_OPT__(,) args)

p1("hello");
p1("%s", "hello");

Output before the change:

printf ( "hello" __VA_OPT__ ( , ) ) ;
printf ( "%s" __VA_OPT__ ( , ) "hello" ) ;

Output after the change:

printf ( "hello" ) ;
printf ( "%s" , "hello" ) ;

@danmar
Copy link
Owner

danmar commented Nov 4, 2023

@JohnSiegel Sorry for delay..

If we look in the standards this is the proper output:

C++11 => this output is correct:

printf ( "hello" __VA_OPT__ ( , ) ) ;
printf ( "%s" __VA_OPT__ ( , ) "hello" ) ;

C++20 => this output is correct:

printf ( "hello" ) ;
printf ( "%s" , "hello" ) ;

However:

  • gcc seems to handle __VA_OPT__ even if standard is changed to C++11.
  • as far as I see you don't have access to the standard in Macro::expandToken.. so somehow we would need to pass that info.

In general I think @firewave has a good point we should follow the standards. However in this case gcc doesn't follow it exactly. What do you think?

@JohnSiegel
Copy link
Contributor Author

Sorry, I initially misunderstood @firewave 's comment. Thank you for clarifying @danmar . I can update this PR with a change passing a simplecpp::DUI& to expandToken and its callers if you think that is the best way to handle this.

@danmar
Copy link
Owner

danmar commented Nov 6, 2023

@JohnSiegel I'd suggest we keep it as it is for now. In general it's a good idea to respect the standard carefully but in this case __VA_OPT__ should be relatively safe to handle. Such identifier should be avoided in older code.

There is a clang-tidy warning. Please fix that.

/home/runner/work/simplecpp/simplecpp/simplecpp.cpp:1980:17: error: do not use 'else' after 'throw' [readability-else-after-return,-warnings-as-errors]

@firewave
Copy link
Collaborator

firewave commented Nov 6, 2023

@JohnSiegel I'd suggest we keep it as it is for now. In general it's a good idea to respect the standard carefully but in this case __VA_OPT__ should be relatively safe to handle. Such identifier should be avoided in older code.

I can address that in the PR I linked.

@danmar danmar merged commit e941a2e into danmar:master Nov 6, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

C++20 __VA_OPT__
3 participants