omarahmed added a comment.
As I understand, the default behavior for when the user didn't use
`SBPO_Custom` is to add a space in placement operators based on this issue
<https://github.com/llvm/llvm-project/issues/54703>. And, at the same time, the
default behavior should be `APO_Never` when we have `SBPO_Custom` so that we
handle other code that depends on that. the existing tests confirm this
understanding. So, the current logic was added based on this understanding.
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3396
spaceRequiredBeforeParens(Right);
+ if (Style.SpaceBeforeParens == FormatStyle::SBPO_Custom &&
+ Left.isOneOf(tok::kw_new, tok::kw_delete) &&
----------------
MyDeveloperDay wrote:
> shouldn't the very first part of this be?
>
>
> ```
> if (Style.SpaceBeforeParensOptions.AfterPlacementOperator !=
> FormatStyle::SpaceBeforeParensCustom::APO_Leave)
> {
>
> ....
>
> }
> ```
>
> i.e. don't we want a zero change if someone says "Leave"
The current code logic, when we do not enter this suggested condition, will
switch to [this
condition](https://github.com/llvm/llvm-project/blob/c2bb2e5973acd24c45c1823e95e8e33003c59484/clang/lib/Format/TokenAnnotator.cpp#L3580).
And will not leave the code as it is, but will change it.
I have thought of getting rid of this condition entirely, but it handles the
default situation suggested here, not only for cpp but for other languages like
js:
>>As I understand, the default behavior for when the user didn't use
>>`SBPO_Custom` is to add a space in placement operators based on [this
>>issue](https://github.com/llvm/llvm-project/issues/54703). And, at the same
>>time, the default behavior should be `APO_Never` when we have `SBPO_Custom`
>>so that we handle other code that depends on that. the existing tests confirm
>>this understanding. So, the current logic was added based on this
>>understanding.
I tried to add an extra condition to [this
condition](https://github.com/llvm/llvm-project/blob/c2bb2e5973acd24c45c1823e95e8e33003c59484/clang/lib/Format/TokenAnnotator.cpp#L3580)
to support the main logic that we are pursuing. so that it would be like that:
```
if (Style.SpaceBeforeParensOptions.AfterPlacementOperator !=
FormatStyle::SpaceBeforeParensCustom::APO_Leave && Left.isOneOf(tok::kw_new,
tok::kw_delete))
```
But that wouldn't fix the leave situation, as it will be also at the end, reach
[this
part](https://github.com/llvm/llvm-project/blob/c2bb2e5973acd24c45c1823e95e8e33003c59484/clang/lib/Format/TokenAnnotator.cpp#L3598)
which will
```
return false
```
and force no space.
I think all of that was raised because all current
"SpaceBeforeParanthesesOptions" are either true or false and this is the only
enum
However, I completely agree with this logic which is to begin our checking for
leave. But I think we should refactor all the "spaceBeforeParenthesesOptions"
conditions to handle this and do that after all the options have been
transformed to enums.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127270/new/
https://reviews.llvm.org/D127270
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
