[clang-format] Clean up code looking for if statements

[sstwcw]

Summary

Previously several places had code to determine whether the line is an
if / while / for statement. This commit replaces them with a function
call.

About the removal of the check in parseAngle. At first there was this
bug https://bugs.llvm.org/show_bug.cgi?id=46969. Right-shift
operators would sometimes get split because they got regarded as
template closers. They fixed the problem by adding the check in
question, telling the program to assume two consecutive >'s in a
condition is not a template. Then they found this bug
https://bugs.llvm.org/show_bug.cgi?id=46969. It turned out a
right-shift can also occur outside of parentheses and get regarded as a
template closer. So they added the next line to fix it. That check
would succeed whether or not the right shift was in an if condition.
Thus the first check has been unnecessary since we had the second one.

[HazardyKnusperkeks]

Please add tests in TokenAnnotatorTests for TT_ConditionLParen.

clang/lib/Format/FormatToken.h

526

Please document what this means.

clang/lib/Format/TokenAnnotator.cpp

133

Any reason why one doesn't need this check anymore?

1446–1450

Unrelated Change.

clang/lib/Format/UnwrappedLineParser.cpp

2426

Please use setFixedType.
Then you don't need to add TT_ConditionLParen to the exclude list in the continuation indenter.

[sstwcw]

clang/lib/Format/TokenAnnotator.cpp

133

The next check already covers what the first is supposed to cover, as explained in the commit message.

1446–1450

Sorry. This part has changed several times in the main line since I started working on this patch. After a few merge conflicts I got tired and started running sort uniq on this part.

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

1446–1450

Nothing against that, but in a separate patch.
Although my goal is the get this list smaller again with the finalized type.

[MyDeveloperDay]

Sorry I don't believe we've covered the changes with unit tests.

clang/lib/Format/FormatToken.h

530

IncludeSpecial seems confusing at best if not obtuse.

540

What about MacroIf

clang/lib/Format/TokenAnnotator.cpp

3018

There has to be a missed unit test here..this condition before only handled if and for

Did you run the regression suite as well as the unit tests?

[sstwcw]

It turned out this patch does change behavior.

-  while (
-      FormatTok->isOneOf(tok::identifier, tok::kw_requires, tok::coloncolon)) {
+  while (FormatTok->isOneOf(tok::identifier, tok::kw_requires,
+                            tok::coloncolon)) {

So what do I do?

And what's the regression suite?

[MyDeveloperDay]

In D121756#3398165, @sstwcw wrote:

It turned out this patch does change behavior.

-  while (
-      FormatTok->isOneOf(tok::identifier, tok::kw_requires, tok::coloncolon)) {
+  while (FormatTok->isOneOf(tok::identifier, tok::kw_requires,
+                            tok::coloncolon)) {

So what do I do?

And what's the regression suite?

I like to run clang-format over all the files in LLVM that were previously clang-formatted, (now be careful because some people don't keep their files formatted), but you can run this over the ~8000 files in LLVM that are clang-format clean, this should give you a good idea of if you change is breaking stuff.

clang-format -verbose -n -files ./clang/docs/tools/clang-formatted-files.txt

[MyDeveloperDay]

It turned out this patch does change behavior.

i.e. now your function is considering more things, the formatted code is different, do you think the new formatting is more correct? (I mean it feels like it should be right? assuming we want to consider whiles in the same way we consider if and for (which seems fairly reasonable, i guess)

You have to capture that in the unit tests for sure, but also can you work out what it was not doing? what it that it wasn't considering the while? should it have?

This is kind of the problem about refactoring without a github issue to justify why you are making the changes in the first place. If you had/can find a bug saying "While loop doesn't format correctly" then it would be easier to justify.

[MyDeveloperDay]

FWIW I'm not a fan of the while( \n case, so assuming this fix, fixes that then that would be good I think.

-  while (
-      FormatTok->isOneOf(tok::identifier, tok::kw_requires, tok::coloncolon)) {
+  while (FormatTok->isOneOf(tok::identifier, tok::kw_requires,
+                            tok::coloncolon)) {

[owenpan]

clang/lib/Format/ContinuationIndenter.cpp

749 ↗

(On Diff #416052)

We only checked for and if before. Now you are also checking while and switch?

clang/lib/Format/FormatToken.h

531

536–541

We prefer early returns and shorter conditionals.

clang/lib/Format/UnwrappedLineParser.cpp

2425–2426

2719–2721

You can move the comment to above line 2729 below if you like.

2727

[HazardyKnusperkeks]

Just a formality.

[sstwcw]

clang/lib/Format/ContinuationIndenter.cpp

749 ↗

(On Diff #416052)

Yes, I am. Please see the diff and tell me whether while and switch should be checked here.

clang/lib/Format/FormatToken.h

540

What's that?

clang/lib/Format/TokenAnnotator.cpp

3018

I added a test.

[sstwcw]

This is how checking for while changes behavior.

diff --git a/flang/lib/Evaluate/tools.cpp b/flang/lib/Evaluate/tools.cpp
index 68b2a40d48c5..3f811fee9bad 100644
--- a/flang/lib/Evaluate/tools.cpp
+++ b/flang/lib/Evaluate/tools.cpp
@@ -868,8 +868,8 @@ bool HasVectorSubscript(const Expr<SomeType> &expr) {
 parser::Message *AttachDeclaration(
     parser::Message &message, const Symbol &symbol) {
   const Symbol *unhosted{&symbol};
-  while (
-      const auto *assoc{unhosted->detailsIf<semantics::HostAssocDetails>()}) {
+  while (const auto *assoc{
+             unhosted->detailsIf<semantics::HostAssocDetails>()}) {
     unhosted = &assoc->symbol();
   }
   if (const auto *binding{
diff --git a/flang/lib/Semantics/data-to-inits.cpp b/flang/lib/Semantics/data-to-inits.cpp
index 6bdbf5f6549f..aa8188656bcb 100644
--- a/flang/lib/Semantics/data-to-inits.cpp
+++ b/flang/lib/Semantics/data-to-inits.cpp
@@ -407,8 +407,8 @@ bool DataInitializationCompiler<DSV>::InitElement(
             DescribeElement(), designatorType->AsFortran());
       }
       auto folded{evaluate::Fold(context, std::move(converted->first))};
-      switch (GetImage().Add(
-          offsetSymbol.offset(), offsetSymbol.size(), folded, context)) {
+      switch (GetImage().Add(offsetSymbol.offset(), offsetSymbol.size(), folded,
+                  context)) {
       case evaluate::InitialImage::Ok:
         return true;
       case evaluate::InitialImage::NotAConstant:
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index 3391665786d5..f1b86a133f1c 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -526,7 +526,7 @@ void SBDebugger::HandleCommand(const char *command) {
         EventSP event_sp;
         ListenerSP lldb_listener_sp = m_opaque_sp->GetListener();
         while (lldb_listener_sp->GetEventForBroadcaster(
-            process_sp.get(), event_sp, std::chrono::seconds(0))) {
+                   process_sp.get(), event_sp, std::chrono::seconds(0))) {
           SBEvent event(event_sp);
           HandleProcessEvent(process, event, GetOutputFile(), GetErrorFile());
         }
diff --git a/openmp/runtime/src/kmp_alloc.cpp b/openmp/runtime/src/kmp_alloc.cpp
index 0f76906714b1..319c3e779fda 100644
--- a/openmp/runtime/src/kmp_alloc.cpp
+++ b/openmp/runtime/src/kmp_alloc.cpp
@@ -2051,7 +2051,8 @@ void *___kmp_fast_allocate(kmp_info_t *this_thr, size_t size KMP_SRC_LOC_DECL) {
     // threads only)
     // pop the head of the sync free list, push NULL instead
     while (!KMP_COMPARE_AND_STORE_PTR(
-        &this_thr->th.th_free_lists[index].th_free_list_sync, ptr, nullptr)) {
+               &this_thr->th.th_free_lists[index].th_free_list_sync, ptr,
+               nullptr)) {
       KMP_CPU_PAUSE();
       ptr = TCR_SYNC_PTR(this_thr->th.th_free_lists[index].th_free_list_sync);
     }
@@ -2178,7 +2179,8 @@ void ___kmp_fast_free(kmp_info_t *this_thr, void *ptr KMP_SRC_LOC_DECL) {
         *((void **)tail) = old_ptr;
 
         while (!KMP_COMPARE_AND_STORE_PTR(
-            &q_th->th.th_free_lists[index].th_free_list_sync, old_ptr, head)) {
+                   &q_th->th.th_free_lists[index].th_free_list_sync, old_ptr,
+                   head)) {
           KMP_CPU_PAUSE();
           old_ptr = TCR_PTR(q_th->th.th_free_lists[index].th_free_list_sync);
           *((void **)tail) = old_ptr;
diff --git a/openmp/runtime/src/kmp_atomic.cpp b/openmp/runtime/src/kmp_atomic.cpp
index 21c2c60bfb60..79b8373319e4 100644
--- a/openmp/runtime/src/kmp_atomic.cpp
+++ b/openmp/runtime/src/kmp_atomic.cpp
@@ -793,8 +793,9 @@ static inline kmp_cmplx128_a16_t operator/(kmp_cmplx128_a16_t &lhs,
     old_value = *(TYPE volatile *)lhs;                                         \
     new_value = (TYPE)(old_value OP rhs);                                      \
     while (!KMP_COMPARE_AND_STORE_ACQ##BITS(                                   \
-        (kmp_int##BITS *)lhs, *VOLATILE_CAST(kmp_int##BITS *) & old_value,     \
-        *VOLATILE_CAST(kmp_int##BITS *) & new_value)) {                        \
+               (kmp_int##BITS *)lhs,                                           \
+               *VOLATILE_CAST(kmp_int##BITS *) & old_value,                    \
+               *VOLATILE_CAST(kmp_int##BITS *) & new_value)) {                 \
       KMP_DO_PAUSE;                                                            \
                                                                                \
       old_value = *(TYPE volatile *)lhs;                                       \
@@ -821,8 +822,9 @@ static inline kmp_cmplx128_a16_t operator/(kmp_cmplx128_a16_t &lhs,
     *old_value.vvv = *(volatile kmp_int##BITS *)lhs;                           \
     new_value.cmp = (TYPE)(old_value.cmp OP rhs);                              \
     while (!KMP_COMPARE_AND_STORE_ACQ##BITS(                                   \
-        (kmp_int##BITS *)lhs, *VOLATILE_CAST(kmp_int##BITS *) old_value.vvv,   \
-        *VOLATILE_CAST(kmp_int##BITS *) new_value.vvv)) {                      \
+               (kmp_int##BITS *)lhs,                                           \
+               *VOLATILE_CAST(kmp_int##BITS *) old_value.vvv,                  \
+               *VOLATILE_CAST(kmp_int##BITS *) new_value.vvv)) {               \
       KMP_DO_PAUSE;                                                            \
                                                                                \
       *old_value.vvv = *(volatile kmp_int##BITS *)lhs;                         \
@@ -848,8 +850,9 @@ static inline kmp_cmplx128_a16_t operator/(kmp_cmplx128_a16_t &lhs,
     *old_value.vvv = *(volatile kmp_int##BITS *)lhs;                           \
     new_value.cmp = old_value.cmp OP rhs;                                      \
     while (!KMP_COMPARE_AND_STORE_ACQ##BITS(                                   \
-        (kmp_int##BITS *)lhs, *VOLATILE_CAST(kmp_int##BITS *) old_value.vvv,   \
-        *VOLATILE_CAST(kmp_int##BITS *) new_value.vvv)) {                      \
+               (kmp_int##BITS *)lhs,                                           \
+               *VOLATILE_CAST(kmp_int##BITS *) old_value.vvv,                  \
+               *VOLATILE_CAST(kmp_int##BITS *) new_value.vvv)) {               \
       KMP_DO_PAUSE;                                                            \
                                                                                \
       *old_value.vvv = *(volatile kmp_int##BITS *)lhs;                         \
@@ -1461,8 +1464,9 @@ ATOMIC_CRITICAL(cmplx16, div_a16, kmp_cmplx128_a16_t, /, 32c,
     old_value = temp_val;                                                      \
     new_value = (TYPE)(rhs OP old_value);                                      \
     while (!KMP_COMPARE_AND_STORE_ACQ##BITS(                                   \
-        (kmp_int##BITS *)lhs, *VOLATILE_CAST(kmp_int##BITS *) & old_value,     \
-        *VOLATILE_CAST(kmp_int##BITS *) & new_value)) {                        \
+               (kmp_int##BITS *)lhs,                                           \
+               *VOLATILE_CAST(kmp_int##BITS *) & old_value,                    \
+               *VOLATILE_CAST(kmp_int##BITS *) & new_value)) {                 \
       KMP_DO_PAUSE;                                                            \
                                                                                \
       temp_val = *lhs;                                                         \
@@ -2127,8 +2131,9 @@ ATOMIC_CRITICAL_READ(cmplx16, a16_rd, kmp_cmplx128_a16_t, +, 32c,
     old_value = temp_val;                                                      \
     new_value = rhs;                                                           \
     while (!KMP_COMPARE_AND_STORE_ACQ##BITS(                                   \
-        (kmp_int##BITS *)lhs, *VOLATILE_CAST(kmp_int##BITS *) & old_value,     \
-        *VOLATILE_CAST(kmp_int##BITS *) & new_value)) {                        \
+               (kmp_int##BITS *)lhs,                                           \
+               *VOLATILE_CAST(kmp_int##BITS *) & old_value,                    \
+               *VOLATILE_CAST(kmp_int##BITS *) & new_value)) {                 \
       temp_val = *lhs;                                                         \
       old_value = temp_val;                                                    \
       new_value = rhs;                                                         \
@@ -2275,8 +2280,9 @@ ATOMIC_CRITICAL_WR(cmplx16, a16_wr, kmp_cmplx128_a16_t, =, 32c,
     old_value = temp_val;                                                      \
     new_value = (TYPE)(old_value OP rhs);                                      \
     while (!KMP_COMPARE_AND_STORE_ACQ##BITS(                                   \
-        (kmp_int##BITS *)lhs, *VOLATILE_CAST(kmp_int##BITS *) & old_value,     \
-        *VOLATILE_CAST(kmp_int##BITS *) & new_value)) {                        \
+               (kmp_int##BITS *)lhs,                                           \
+               *VOLATILE_CAST(kmp_int##BITS *) & old_value,                    \
+               *VOLATILE_CAST(kmp_int##BITS *) & new_value)) {                 \
       temp_val = *lhs;                                                         \
       old_value = temp_val;                                                    \
       new_value = (TYPE)(old_value OP rhs);                                    \
@@ -2971,8 +2977,9 @@ ATOMIC_CRITICAL_CPT(cmplx16, div_a16_cpt, kmp_cmplx128_a16_t, /, 32c,
     old_value = temp_val;                                                      \
     new_value = (TYPE)(rhs OP old_value);                                      \
     while (!KMP_COMPARE_AND_STORE_ACQ##BITS(                                   \
-        (kmp_int##BITS *)lhs, *VOLATILE_CAST(kmp_int##BITS *) & old_value,     \
-        *VOLATILE_CAST(kmp_int##BITS *) & new_value)) {                        \
+               (kmp_int##BITS *)lhs,                                           \
+               *VOLATILE_CAST(kmp_int##BITS *) & old_value,                    \
+               *VOLATILE_CAST(kmp_int##BITS *) & new_value)) {                 \
       temp_val = *lhs;                                                         \
       old_value = temp_val;                                                    \
       new_value = (TYPE)(rhs OP old_value);                                    \
@@ -3295,8 +3302,9 @@ ATOMIC_CRITICAL_CPT_REV_MIX(float10, long double, div_cpt_rev, /, fp, _Quad,
     old_value = temp_val;                                                      \
     new_value = rhs;                                                           \
     while (!KMP_COMPARE_AND_STORE_ACQ##BITS(                                   \
-        (kmp_int##BITS *)lhs, *VOLATILE_CAST(kmp_int##BITS *) & old_value,     \
-        *VOLATILE_CAST(kmp_int##BITS *) & new_value)) {                        \
+               (kmp_int##BITS *)lhs,                                           \
+               *VOLATILE_CAST(kmp_int##BITS *) & old_value,                    \
+               *VOLATILE_CAST(kmp_int##BITS *) & new_value)) {                 \
       temp_val = *lhs;                                                         \
       old_value = temp_val;                                                    \
       new_value = rhs;                                                         \
@@ -3476,8 +3484,9 @@ void __kmpc_atomic_2(ident_t *id_ref, int gtid, void *lhs, void *rhs,
     (*f)(&new_value, &old_value, rhs);
 
     /* TODO: Should this be acquire or release? */
-    while (!KMP_COMPARE_AND_STORE_ACQ16(
-        (kmp_int16 *)lhs, *(kmp_int16 *)&old_value, *(kmp_int16 *)&new_value)) {
+    while (!KMP_COMPARE_AND_STORE_ACQ16((kmp_int16 *)lhs,
+                                        *(kmp_int16 *)&old_value,
+                                        *(kmp_int16 *)&new_value)) {
       KMP_CPU_PAUSE();
 
       old_value = *(kmp_int16 *)lhs;
@@ -3525,8 +3534,9 @@ void __kmpc_atomic_4(ident_t *id_ref, int gtid, void *lhs, void *rhs,
     (*f)(&new_value, &old_value, rhs);
 
     /* TODO: Should this be acquire or release? */
-    while (!KMP_COMPARE_AND_STORE_ACQ32(
-        (kmp_int32 *)lhs, *(kmp_int32 *)&old_value, *(kmp_int32 *)&new_value)) {
+    while (!KMP_COMPARE_AND_STORE_ACQ32((kmp_int32 *)lhs,
+                                        *(kmp_int32 *)&old_value,
+                                        *(kmp_int32 *)&new_value)) {
       KMP_CPU_PAUSE();
 
       old_value = *(kmp_int32 *)lhs;
@@ -3574,8 +3584,9 @@ void __kmpc_atomic_8(ident_t *id_ref, int gtid, void *lhs, void *rhs,
     old_value = *(kmp_int64 *)lhs;
     (*f)(&new_value, &old_value, rhs);
     /* TODO: Should this be acquire or release? */
-    while (!KMP_COMPARE_AND_STORE_ACQ64(
-        (kmp_int64 *)lhs, *(kmp_int64 *)&old_value, *(kmp_int64 *)&new_value)) {
+    while (!KMP_COMPARE_AND_STORE_ACQ64((kmp_int64 *)lhs,
+                                        *(kmp_int64 *)&old_value,
+                                        *(kmp_int64 *)&new_value)) {
       KMP_CPU_PAUSE();
 
       old_value = *(kmp_int64 *)lhs;
diff --git a/openmp/runtime/src/kmp_dispatch.cpp b/openmp/runtime/src/kmp_dispatch.cpp
index 648332109dbb..d3dd63f57401 100644
--- a/openmp/runtime/src/kmp_dispatch.cpp
+++ b/openmp/runtime/src/kmp_dispatch.cpp
@@ -1359,9 +1359,9 @@ int __kmp_dispatch_next_algorithm(int gtid,
         vnew.b = vold.b;
         vnew.p.count++; // get chunk from head of self range
         while (!KMP_COMPARE_AND_STORE_REL64(
-            (volatile kmp_int64 *)&pr->u.p.count,
-            *VOLATILE_CAST(kmp_int64 *) & vold.b,
-            *VOLATILE_CAST(kmp_int64 *) & vnew.b)) {
+                   (volatile kmp_int64 *)&pr->u.p.count,
+                   *VOLATILE_CAST(kmp_int64 *) & vold.b,
+                   *VOLATILE_CAST(kmp_int64 *) & vnew.b)) {
           KMP_CPU_PAUSE();
           vold.b = *(volatile kmp_int64 *)(&pr->u.p.count);
           vnew.b = vold.b;

[owenpan]

In D121756#3398165, @sstwcw wrote:

It turned out this patch does change behavior.

-  while (
-      FormatTok->isOneOf(tok::identifier, tok::kw_requires, tok::coloncolon)) {
+  while (FormatTok->isOneOf(tok::identifier, tok::kw_requires,
+                            tok::coloncolon)) {

So what do I do?

IMO, refactoring and cleaning up code should be NFC. You can first add while, switch, etc (if it makes sense) in one patch and then refactor in another patch (or vice versa). It would be much easier to review.

[HazardyKnusperkeks]

In D121756#3407320, @owenpan wrote:

In D121756#3398165, @sstwcw wrote:

It turned out this patch does change behavior.

-  while (
-      FormatTok->isOneOf(tok::identifier, tok::kw_requires, tok::coloncolon)) {
+  while (FormatTok->isOneOf(tok::identifier, tok::kw_requires,
+                            tok::coloncolon)) {

So what do I do?

IMO, refactoring and cleaning up code should be NFC. You can first add while, switch, etc (if it makes sense) in one patch and then refactor in another patch (or vice versa). It would be much easier to review.

+1

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

252–257

I don't think this is NFC.
Before:

} else if (OpeningParen.Previous &&
           (OpeningParen.Previous->isOneOf(tok::kw_static_assert,
                                           tok::kw_while, tok::l_paren,
                                           tok::comma, tok::kw_if,
                                           TT_BinaryOperator) ||
            OpeningParen.Previous->endsSequence(tok::kw_constexpr,
                                                tok::kw_if) ||
            OpeningParen.Previous->endsSequence(tok::identifier,
                                                tok::kw_if))) {

After:

} else if ((OpeningParen.is(tok::l_paren) &&
            OpeningParen.is(TT_ConditionLParen)) ||
           // PreviousNonComment = OpeningParen.getPreviousNonComment()
           (PreviousNonComment &&
            PreviousNonComment->isOneOf(tok::kw_if, tok::kw_while,
                                        tok::kw_switch, tok::kw_case,
                                        tok::kw_constexpr)) ||
           (OpeningParen.Previous &&
            OpeningParen.Previous->isOneOf(tok::kw_static_assert,
                                           tok::l_paren, tok::comma,
                                           TT_BinaryOperator))) {

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

252–257

After:

} else if ((OpeningParen.is(tok::l_paren) &&
            OpeningParen.is(TT_ConditionLParen)) ||
            ...
                                        tok::kw_constexpr)) ||
           ...

After:

} else if ((OpeningParen.is(tok::l_paren) &&
            (OpeningParen.is(TT_ConditionLParen) ||
            ...
                                         tok::kw_constexpr)))) ||
           ...

[sstwcw]

clang/lib/Format/TokenAnnotator.cpp

252–257

I removed NFC from the title. It would affect things like this:

new:
switch (x * x)
old:
switch (x *x)

However the entire llvm codebase doesn't seem to have such things.

[owenpan]

Please see D123741. It should make it easier to see if you are making NFC changes at other places as well.

clang/lib/Format/TokenAnnotator.cpp

252–257

Nevertheless, your example is valid C++ and may exists in other codebases.

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

133–136

Can you split it to a separate patch? Unlike the rest of this patch, it's likely to get accepted.

clang/lib/Format/FormatToken.h

#define LIST_TOKEN_TYPES                                                       \
   TYPE(AttributeSquare)                                                        \
   TYPE(BinaryOperator)                                                         \
   TYPE(BitFieldColon)                                                          \
   TYPE(BlockComment)                                                           \
   TYPE(BracedListLBrace)                                                       \
   TYPE(CastRParen)                                                             \
   TYPE(ClassLBrace)                                                            \
   TYPE(CompoundRequirementLBrace)                                              \
-  TYPE(ConditionalExpr)                                                        \
+  TYPE(ConditionLParen) /* The condition in an if, while, or switch statement. \
+                           The parenthesis in a for statement is not labeled   \
+                           this type, because inside is not simply an          \
+                           expression and thus special treatment is needed. */ \
+  TYPE(ConditionalExpr) /* ternary ?: expression */                            \
   TYPE(ConflictAlternative)                                                    \
   TYPE(ConflictEnd)                                                            \
   TYPE(ConflictStart)                                                          \
   TYPE(CppCastLParen)                                                          \
   TYPE(CtorInitializerColon)                                                   \
   TYPE(CtorInitializerComma)                                                   \
   TYPE(DesignatedInitializerLSquare)                                           \
   TYPE(DesignatedInitializerPeriod)                                            \
bool isOneOf(A K1, B K2, Ts... Ks) const {
     return is(K1) || isOneOf(K2, Ks...);
   }
   template <typename T> bool isNot(T Kind) const { return !is(Kind); }
 
   bool isIf(bool AllowConstexprMacro = true) const {
     return is(tok::kw_if) || endsSequence(tok::kw_constexpr, tok::kw_if) ||
            (endsSequence(tok::identifier, tok::kw_if) && AllowConstexprMacro);
   }
 
+  /// Returns \c true if the token is the open parenthesis of a
+  /// statement's condition like if or while.
+  /// @param IncludeFor - Whether to return true if the parenthesis is in some
+  /// special construct including for, catch, and foreach. Because inside the
+  /// parentheses of these constructs is not simply an expression, they need to
+  /// be handled separately.
+  bool isConditionLParen(bool IncludeFor) const {
+    if (isNot(tok::l_paren))
+      return false;
+    if (is(TT_ConditionLParen))
+      return true;
+    const FormatToken *Prev = getPreviousNonComment();
+    if (!Prev)
+      return false;
+    if (Prev->isOneOf(tok::kw_if, tok::kw_while, tok::kw_switch, tok::kw_case,
+                      tok::kw_constexpr))
+      return true;
+    // `for` and `catch` special handling.
+    if (IncludeFor && Prev->isOneOf(TT_ForEachMacro, TT_ObjCForIn, tok::kw_for,
+                                    tok::kw_catch))
+      return true;
+    return false;
+  }
+
   bool closesScopeAfterBlock() const {
     if (getBlockKind() == BK_Block)
       return true;
     if (closesScope())
       return Previous->closesScopeAfterBlock();
     return false;
   }
 

clang/lib/Format/TokenAnnotator.cpp

if (Left->Previous && Left->Previous->is(tok::greater) &&
     Left = Left->Previous->MatchingParen;
 
   // Check for `[...]`.
   return Left->Previous && Left->Previous->is(tok::r_square) &&
          Left->Previous->MatchingParen &&
          Left->Previous->MatchingParen->is(TT_LambdaLSquare);
 }
 
-/// Returns \c true if the token is followed by a boolean condition, \c false
-/// otherwise.
-static bool isKeywordWithCondition(const FormatToken &Tok) {
-  return Tok.isOneOf(tok::kw_if, tok::kw_for, tok::kw_while, tok::kw_switch,
-                     tok::kw_constexpr, tok::kw_catch);
-}
-
 /// A parser that gathers additional information about tokens.
 ///
 /// The \c TokenAnnotator tries to match parenthesis and square brakets and
 /// store a parenthesis levels. It also tries to resolve matching "<" and ">"
 /// into template parameter lists.
 class AnnotatingParser {
 public:
   AnnotatingParser(const FormatStyle &Style, AnnotatedLine &Line,
while (CurrentToken) {
         // Try to do a better job at looking for ">>" within the condition of
         // a statement. Conservatively insert spaces between consecutive ">"
         // tokens to prevent splitting right bitshift operators and potentially
         // altering program semantics. This check is overly conservative and
         // will prevent spaces from being inserted in select nested template
         // parameter cases, but should not alter program semantics.
         if (CurrentToken->Next && CurrentToken->Next->is(tok::greater) &&
             Left->ParentBracket != tok::less &&
-            (isKeywordWithCondition(*Line.First) ||
-             CurrentToken->getStartOfNonWhitespace() ==
-                 CurrentToken->Next->getStartOfNonWhitespace().getLocWithOffset(
-                     -1)))
+                    -1))
           return false;
         Left->MatchingParen = CurrentToken;
         CurrentToken->MatchingParen = Left;
         // In TT_Proto, we must distignuish between:
         //   map<key, value>
         //   msg < item: data >
         //   msg: < item: data >
         // In TT_TextProto, map<key, value> does not occur.
if (OpeningParen.is(TT_OverloadedOperatorLParen)) {
       Contexts.back().IsExpression = OperatorCalledAsMemberFunction;
     } else if (Style.isJavaScript() &&
                (Line.startsWith(Keywords.kw_type, tok::identifier) ||
                 Line.startsWith(tok::kw_export, Keywords.kw_type,
                                 tok::identifier))) {
       // type X = (...);
       // export type X = (...);
       Contexts.back().IsExpression = false;
-    } else if (OpeningParen.Previous &&
-               (OpeningParen.Previous->isOneOf(tok::kw_static_assert,
-                                               tok::kw_while, tok::l_paren,
-                                               tok::comma) ||
-                OpeningParen.Previous->isIf() ||
-                OpeningParen.Previous->is(TT_BinaryOperator))) {
+    } else if (OpeningParen.isConditionLParen(/*IncludeFor=*/false) ||
+               (OpeningParen.Previous &&
+                OpeningParen.Previous->isOneOf(TT_BinaryOperator, tok::l_paren,
+                                               tok::comma,
+                                               tok::kw_static_assert))) {
       // static_assert, if and while usually contain expressions.
       Contexts.back().IsExpression = true;
     } else if (Style.isJavaScript() && OpeningParen.Previous &&
                (OpeningParen.Previous->is(Keywords.kw_function) ||
                 (OpeningParen.Previous->endsSequence(tok::identifier,
                                                      Keywords.kw_function)))) {
       // function(...) or function f(...)
       Contexts.back().IsExpression = false;
     } else if (Style.isJavaScript() && OpeningParen.Previous &&
if (!CurrentToken->isTypeFinalized() &&
         !CurrentToken->isOneOf(
             TT_LambdaLSquare, TT_LambdaLBrace, TT_AttributeMacro, TT_IfMacro,
             TT_ForEachMacro, TT_TypenameMacro, TT_FunctionLBrace,
             TT_ImplicitStringLiteral, TT_InlineASMBrace, TT_FatArrow,
             TT_LambdaArrow, TT_NamespaceMacro, TT_OverloadedOperator,
             TT_RegexLiteral, TT_TemplateString, TT_ObjCStringLiteral,
             TT_UntouchableMacroFunc, TT_StatementAttributeLikeMacro,
             TT_FunctionLikeOrFreestandingMacro, TT_ClassLBrace, TT_EnumLBrace,
             TT_RecordLBrace, TT_StructLBrace, TT_UnionLBrace, TT_RequiresClause,
             TT_RequiresClauseInARequiresExpression, TT_RequiresExpression,
             TT_RequiresExpressionLParen, TT_RequiresExpressionLBrace,
             TT_CompoundRequirementLBrace, TT_BracedListLBrace))
       CurrentToken->setType(TT_Unknown);
     CurrentToken->Role.reset();
     CurrentToken->MatchingParen = nullptr;
     CurrentToken->FakeLParens.clear();
     CurrentToken->FakeRParens = 0;
   }
 
   void next() {
     if (!CurrentToken)
if (Line.Type == LT_ObjCDecl && Left.is(tok::l_paren) && Left.Previous &&
     return 500;
 
   if (Left.is(tok::l_paren) && Style.PenaltyBreakOpenParenthesis != 0)
     return Style.PenaltyBreakOpenParenthesis;
   if (Left.is(tok::l_paren) && InFunctionDecl &&
       Style.AlignAfterOpenBracket != FormatStyle::BAS_DontAlign)
     return 100;
   if (Left.is(tok::l_paren) && Left.Previous &&
       (Left.Previous->is(tok::kw_for) || Left.Previous->isIf()))
     return 1000;
   if (Left.is(tok::equal) && InFunctionDecl)
     return 110;
   if (Right.is(tok::r_brace))
     return 1;
   if (Left.is(TT_TemplateOpener))
     return 100;
   if (Left.opensScope()) {
bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
   if (Right.is(tok::hashhash))
     return Left.is(tok::hash);
   if (Left.isOneOf(tok::hashhash, tok::hash))
     return Right.is(tok::hash);
   if ((Left.is(tok::l_paren) && Right.is(tok::r_paren)) ||
       (Left.is(tok::l_brace) && Left.isNot(BK_Block) &&
        Right.is(tok::r_brace) && Right.isNot(BK_Block)))
     return Style.SpaceInEmptyParentheses;
-  if (Style.SpacesInConditionalStatement) {
-    if (Left.is(tok::l_paren) && Left.Previous &&
-        isKeywordWithCondition(*Left.Previous))
-      return true;
-    if (Right.is(tok::r_paren) && Right.MatchingParen &&
-        Right.MatchingParen->Previous &&
-        isKeywordWithCondition(*Right.MatchingParen->Previous))
-      return true;
-  }
+  if (Style.SpacesInConditionalStatement &&
+      (Left.isConditionLParen(/*IncludeFor=*/true) ||
+       (Right.is(tok::r_paren) && Right.MatchingParen &&
+        Right.MatchingParen->isConditionLParen(/*IncludeFor=*/true))))
 
   // auto{x} auto(x)
   if (Left.is(tok::kw_auto) && Right.isOneOf(tok::l_paren, tok::l_brace))
     return false;
 
   // operator co_await(x)
   if (Right.is(tok::l_paren) && Left.is(tok::kw_co_await) && Left.Previous &&
       Left.Previous->is(tok::kw_operator))
if (Left.is(TT_ForEachMacro))
              spaceRequiredBeforeParens(Right);
     if (Left.is(TT_IfMacro))
       return Style.SpaceBeforeParensOptions.AfterIfMacros ||
              spaceRequiredBeforeParens(Right);
     if (Line.Type == LT_ObjCDecl)
       return true;
     if (Left.is(tok::semi))
       return true;
-    if (Left.isOneOf(tok::pp_elif, tok::kw_for, tok::kw_while, tok::kw_switch,
-                     tok::kw_case, TT_ForEachMacro, TT_ObjCForIn))
-      return Style.SpaceBeforeParensOptions.AfterControlStatements ||
-             spaceRequiredBeforeParens(Right);
-    if (Left.isIf(Line.Type != LT_PreprocessorDirective))
+    if (Right.isConditionLParen(/*IncludeFor=*/true) || Left.is(tok::pp_elif))
       return Style.SpaceBeforeParensOptions.AfterControlStatements ||
              spaceRequiredBeforeParens(Right);
 
     // TODO add Operator overloading specific Options to
     // SpaceBeforeParensOptions
     if (Right.is(TT_OverloadedOperatorLParen))
       return spaceRequiredBeforeParens(Right);
     // Function declaration or definition
bool TokenAnnotator::canBreakBefore(const AnnotatedLine &Line,
   // the l_brace, which is tracked by BreakBeforeClosingBrace.
   if (Right.is(tok::r_brace))
     return Right.MatchingParen && Right.MatchingParen->is(BK_Block);
 
   // We only break before r_paren if we're in a block indented context.
   if (Right.is(tok::r_paren)) {
     if (Style.AlignAfterOpenBracket == FormatStyle::BAS_BlockIndent)
       return Right.MatchingParen &&
-             !(Right.MatchingParen->Previous &&
-               (Right.MatchingParen->Previous->is(tok::kw_for) ||
-                Right.MatchingParen->Previous->isIf()));
-
+             !Right.MatchingParen->isConditionLParen(/*IncludeFor=*/true);
     return false;
   }
 
   // Allow breaking after a trailing annotation, e.g. after a method
   // declaration.
   if (Left.is(TT_TrailingAnnotation))
     return !Right.isOneOf(tok::l_brace, tok::semi, tok::equal, tok::l_paren,
                           tok::less, tok::coloncolon);

clang/lib/Format/UnwrappedLineParser.cpp

FormatToken *UnwrappedLineParser::parseIfThenElse(IfStmtKind *IfKind,
   nextToken();
   if (FormatTok->is(tok::exclaim))
     nextToken();
   if (FormatTok->is(tok::kw_consteval)) {
     nextToken();
   } else {
     if (FormatTok->isOneOf(tok::kw_constexpr, tok::identifier))
       nextToken();
-    if (FormatTok->is(tok::l_paren))
+    if (FormatTok->is(tok::l_paren)) {
+      FormatTok->setFinalizedType(TT_ConditionLParen);
       parseParens();
     }
+  }
   handleAttributes();
 
   bool NeedsUnwrappedLine = false;
   keepAncestorBraces();
 
   FormatToken *IfLeftBrace = nullptr;
   IfStmtKind IfBlockKind = IfStmtKind::NotIf;
 
do {
     }
     nextToken();
   } while (!eof());
 }
 
 void UnwrappedLineParser::parseForOrWhileLoop() {
   assert(FormatTok->isOneOf(tok::kw_for, tok::kw_while, TT_ForEachMacro) &&
          "'for', 'while' or foreach macro expected");
+  const FormatToken &FirstTok = *FormatTok;
   nextToken();
   // JS' for await ( ...
   if (Style.isJavaScript() && FormatTok->is(Keywords.kw_await))
     nextToken();
   if (Style.isCpp() && FormatTok->is(tok::kw_co_await))
     nextToken();
-  if (FormatTok->is(tok::l_paren))
+  if (FormatTok->is(tok::l_paren)) {
+    // Those that begin with a for require special treatment because inside the
+    // parentheses is not an expression.
+    if (FirstTok.is(tok::kw_while))
+      FormatTok->setFinalizedType(TT_ConditionLParen);
     parseParens();
+  }
 
   keepAncestorBraces();
 
   if (FormatTok->is(tok::l_brace)) {
     FormatToken *LeftBrace = FormatTok;
     CompoundStatementIndenter Indenter(this, Style, Line->Level);
     parseBlock();
     if (Style.RemoveBracesLLVM) {
void UnwrappedLineParser::parseDoWhile() {
   }
 
   // If in Whitesmiths mode, the line with the while() needs to be indented
   // to the same level as the block.
   if (Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths)
     ++Line->Level;
 
   nextToken();
+  // FIXME: Add error handling.
+  if (FormatTok->is(tok::l_paren))
+    FormatTok->setFinalizedType(TT_ConditionLParen);
   parseStructuralElement();
 }
 
 void UnwrappedLineParser::parseLabel(bool LeftAlignLabel) {
   nextToken();
   unsigned OldLineLevel = Line->Level;
   if (Line->Level > 1 || (!Line->InPPDirective && Line->Level > 0))
     --Line->Level;
do {
     nextToken();
   } while (!eof() && !FormatTok->is(tok::colon));
   parseLabel();
 }
 
 void UnwrappedLineParser::parseSwitch() {
   assert(FormatTok->is(tok::kw_switch) && "'switch' expected");
   nextToken();
-  if (FormatTok->is(tok::l_paren))
+  if (FormatTok->is(tok::l_paren)) {
+    FormatTok->setFinalizedType(TT_ConditionLParen);
     parseParens();
+  }
 
   keepAncestorBraces();
 
   if (FormatTok->is(tok::l_brace)) {
     CompoundStatementIndenter Indenter(this, Style, Line->Level);
     parseBlock();
     addUnwrappedLine();
   } else {

clang/unittests/Format/FormatTest.cpp

verifyFormat("switch (x) {\n"
                "[[likely]] case 2:\n"
                "  return;\n"
                "}");
   verifyFormat("switch (a) {\n"
                "case 1:\n"
                "[[likely]] case 2:\n"
                "  return;\n"
                "}");
+  verifyFormat("switch (x * x) { //\n"
+               "}");
+  verifyFormat("switch (x & x) { //\n"
+               "}");
   FormatStyle Attributes = getLLVMStyle();
   Attributes.AttributeMacros.push_back("LIKELY");
   Attributes.AttributeMacros.push_back("OTHER_LIKELY");
   verifyFormat("switch (a) {\n"
                "LIKELY case b:\n"
                "  return;\n"
                "}",
                Attributes);
EXPECT_EQ("try {\n"
             "  foo();\n"
             "} catch (Exception &bar) {\n"
             "  baz();\n"
             "}",
             format("try{foo();}catch(Exception&bar){baz();}", Style));
   Style.ColumnLimit =
       20; // to concentrate at brace wrapping, not line wrap due to column limit
 
+  verifyFormat("if (xxxxxxxxxx\n"
+               "        .xxxxxx)\n"
+               "  continue;",
+               Style);
+  verifyFormat("while (xxxxxxxxxx\n"
+               "           .xxxxxx)\n"
+               "  continue;",
+               Style);
+
   Style.BraceWrapping.BeforeElse = true;
   EXPECT_EQ(
       "if (foo) {\n"
       "  bar();\n"
       "}\n"
       "else if (baz ||\n"
       "         quux)\n"
       "{\n"

clang/unittests/Format/TokenAnnotatorTest.cpp

TEST_F(TokenAnnotatorTest, UnderstandsObjCBlock) {
 
   Tokens = annotate("int *p = ^int*(){ //\n"
                     "  return nullptr;\n"
                     "}();");
   ASSERT_EQ(Tokens.size(), 19u) << Tokens;
   EXPECT_TOKEN(Tokens[9], tok::l_brace, TT_ObjCBlockLBrace);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsConditionParen) {
+  auto Tokens = annotate("if (true) {\n}");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_ConditionLParen);
+  Tokens = annotate("if constexpr (true) {\n}");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[2], tok::l_paren, TT_ConditionLParen);
+  Tokens = annotate("if CONSTEXPR (true) {\n}");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[2], tok::l_paren, TT_ConditionLParen);
+
+  // The parentheses following for is not TT_ConditionLParen, because inside is
+  // not just a condition.
+  Tokens = annotate("for (;;) {\n}");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_Unknown);
+  Tokens = annotate("foreach (Item *item, itemlist) {\n}");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_Unknown);
+  Tokens = annotate("Q_FOREACH (Item *item, itemlist) {\n}");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_Unknown);
+  Tokens = annotate("BOOST_FOREACH (Item *item, itemlist) {\n}");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_Unknown);
+  Tokens = annotate("try {\n"
+                    "} catch (Exception &bar) {\n"
+                    "}");
+  ASSERT_EQ(Tokens.size(), 12u) << Tokens;
+  EXPECT_TOKEN(Tokens[4], tok::l_paren, TT_Unknown);
+
+  Tokens = annotate("while (true) {\n}");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_ConditionLParen);
+  Tokens = annotate("do {\n} while (true);");
+  ASSERT_EQ(Tokens.size(), 9u) << Tokens;
+  EXPECT_TOKEN(Tokens[4], tok::l_paren, TT_ConditionLParen);
+
+  Tokens = annotate("switch (true) {\n}");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_ConditionLParen);
+  Tokens = annotate("switch (x * x) {\n}");
+  ASSERT_EQ(Tokens.size(), 9u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::star, TT_BinaryOperator);
+  Tokens = annotate("switch (x & x) {\n}");
+  ASSERT_EQ(Tokens.size(), 9u) << Tokens;
+  EXPECT_TOKEN(Tokens[3], tok::amp, TT_BinaryOperator);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang