omarahmed marked 3 inline comments as done and an inline comment as not done.
omarahmed added inline comments.
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3396
spaceRequiredBeforeParens(Right);
+ if (Style.SpaceBeforeParens == FormatStyle::SBPO_Custom &&
+ Left.isOneOf(tok::kw_new, tok::kw_delete) &&
----------------
HazardyKnusperkeks wrote:
> omarahmed wrote:
> > 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.
> >
> `SpaceBeforeParensOptions` should always be expanded, regardless of
> `SpaceBeforeParens`, as far as I remember. `SpaceBeforeParens` is only for
> the one configuring clang-format, the code that formats should always and
> only check `SpaceBeforeParensOptions`.
But in general, doesn't that could create some kind of conflict like if someone
used a specific option in `SpaceBeforeParens ` and then defined inside
`SpaceBeforeParensOptions ` some rules that could conflict with
`SpaceBeforeParens `, what should be the priority between them?
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3396
spaceRequiredBeforeParens(Right);
+ if (Style.SpaceBeforeParens == FormatStyle::SBPO_Custom &&
+ Left.isOneOf(tok::kw_new, tok::kw_delete) &&
----------------
omarahmed wrote:
> HazardyKnusperkeks wrote:
> > omarahmed wrote:
> > > 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.
> > >
> > `SpaceBeforeParensOptions` should always be expanded, regardless of
> > `SpaceBeforeParens`, as far as I remember. `SpaceBeforeParens` is only for
> > the one configuring clang-format, the code that formats should always and
> > only check `SpaceBeforeParensOptions`.
> But in general, doesn't that could create some kind of conflict like if
> someone used a specific option in `SpaceBeforeParens ` and then defined
> inside `SpaceBeforeParensOptions ` some rules that could conflict with
> `SpaceBeforeParens `, what should be the priority between them?
>
In this specific case, for `AfterPlacementOperator` if someone doesn't define
it, it should keep the default behavior in the past which is to add a space,
and we defined the default to be `APO_Never`, so that's why I have conditioned
it with `Custom` so that if someone doesn't want to define it, then we keep the
old behavior.
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
