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

[clang-format] Faulty RemoveParentheses: MultipleParentheses/ReturnStatement #81399

Closed
prj- opened this issue Feb 11, 2024 · 4 comments · Fixed by #81444
Closed

[clang-format] Faulty RemoveParentheses: MultipleParentheses/ReturnStatement #81399

prj- opened this issue Feb 11, 2024 · 4 comments · Fixed by #81444
Assignees

Comments

@prj-
Copy link

prj- commented Feb 11, 2024

$ cat no_return.c 
#define foo(...) bar((__VA_ARGS__))
$ clang-format --version
clang-format version 17.0.6
$ clang-format -style='{RemoveParentheses: ReturnStatement}' no_return.c 
#define foo(...) bar(__VA_ARGS__)
$ clang-format-18 -style='{RemoveParentheses: ReturnStatement}' no_return.c 
#define foo(...) bar(__VA_ARGS__)
$ clang-format-19 -style='{RemoveParentheses: ReturnStatement}' no_return.c
#define foo(...) bar(__VA_ARGS__)
@prj-
Copy link
Author

prj- commented Feb 11, 2024

My bad, I just realized that ReturnStatement applies on top of MultipleParentheses. I would like to just format the return statements, because this generates wrong code otherwise. Or would it be possible to fix MultipleParentheses in the first place?

$ cat foo.c
cat foo.c 
#define PetscDefined_arg_1                                    shift,
#define PetscDefined_arg_                                     shift,
#define PetscDefined__take_second_expanded(ignored, val, ...) val
#define PetscDefined__take_second_expand(args)                PetscDefined__take_second_expanded args
#define PetscDefined__take_second(...)                        PetscDefined__take_second_expand((__VA_ARGS__))
#define PetscDefined__(arg1_or_junk)                          PetscDefined__take_second(arg1_or_junk 1, 0, at_)
#define PetscDefined_(value)                                  PetscDefined__(PetscConcat_(PetscDefined_arg_, value))
#define PetscDefined(def)                                     PetscDefined_(PetscConcat(PETSC_, def))

#if PetscDefined(FOO)

#endif
$ clang -c foo.c 2>/dev/null; echo $?
0
$ clang-format -style='{RemoveParentheses: MultipleParentheses}' foo.c > bar.c
$ clang -c bar.c 2>/dev/null; echo $?
1

@prj- prj- changed the title [clang-format] Faulty RemoveParentheses: ReturnStatement [clang-format] Faulty RemoveParentheses: MultipleParentheses/ReturnStatement Feb 11, 2024
@owenca
Copy link
Contributor

owenca commented Feb 11, 2024

IMO MultipleParentheses does what it's supposed to do in your case as there's no way to tell that the extra pair of parentheses enclosing __VA_ARGS__ is required just by looking at the definition of PetscDefined__take_second. (There is a big warning in the documentation and you must opt in.) Nevertheless, you can turn on SkipMacroDefinitionBody or opt out of RemoveParentheses.

@prj-
Copy link
Author

prj- commented Feb 11, 2024

Yes, I’m aware of the warning. If you don’t think think there is anything to be done on your side, feel free to close the issue.

@owenca owenca self-assigned this Feb 12, 2024
@owenca
Copy link
Contributor

owenca commented Feb 12, 2024

I'll disable removing parentheses in macro definitions.

owenca added a commit to owenca/llvm-project that referenced this issue Feb 12, 2024
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Feb 13, 2024
llvmbot pushed a commit to llvmbot/llvm-project that referenced this issue Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants