[PATCH] D127270: [clang-format] Add space in placement new expression

[omar ahmed via Phabricator via cfe-commits]

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