HazardyKnusperkeks added a comment.
Nice work adding the capability to the dump script.
================
Comment at: clang/docs/ClangFormatStyleOptions.rst:4264
+ and opening parentheses.
+ 14
+
----------------
This seems to be not entirely correct yet. :)
================
Comment at: clang/include/clang/Format/Format.h:3504
+ enum AfterPlacementOperatorStyle : int8_t {
+ /// Never add space after ``new/delete`` operators and before ``(``.
+ /// \code
----------------
================
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:
> 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`.
================
Comment at: clang/unittests/Format/FormatTest.cpp:10145
+ " int *c = new(p) int(3);\n"
+ " int *d = delete(p) int(3);\n"
+ " }\n"
----------------
Can this be valid code? If not we don't need to assert on how it is formatted,
only that it doesn't crash.
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
