[clang] [Clang][Sema] Refactor checks on for/while loops (NFC) (PR #226101)

[via cfe-commits]

Expertcoderz wrote:

@Sirraide Greetings, I'd like to address some of the specific concerns you've raised in https://github.com/llvm/llvm-project/pull/225748#discussion_r4083103871 to make sure we have everything sorted out.

> The `CommaVisitor` check is repeated in `ActOnForStmt()` and `ActOnWhileStmt()` and I wouldn’t be surprised if it was also in the for-range code.

Fortunately, the definition of `Sema::ActOnCXXForRangeStmt` doesn't seem to reference `CommaVisitor` so there should be no duplication in that regard.

As for `ActOnForStmt()` and `ActOnWhileStmt()`, those have been fixed in this PR by moving the checks into a shared `CheckConditionalLoop()` routine.

---

> This new check you’re adding is likewise repeated in those places.

That's right; it is an issue that's solved by this refactor as defer checks will no longer need to be repeated across the `ActOnForStmt()` and `ActOnWhileStmt()`, at the very least.

---

> `DiagnoseEmptyStmtBody()` is done here directly, but _for some reason_, we don’t do that for loops and instead set `HasEmptyLoopBody` and then diagnose loops in `ActOnCompoundStatement()`

I just had a deeper look at the code to find out the reason. It turns out (assuming my understanding is correct), that `for`/`while` loops require special handling for empty body diagnostics:

```c
for (;;); // OK; no diagnostic
foo();

for (;;); // warning: for loop has empty body
  foo(); // <- note the indentation
```

Whereas the same does not apply for conditionals:

```c
if (true); // warning: if statement has empty body
foo();

if (true); // same warning
  foo();
```

In other words, the diagnostic depends on statements adjacent to the `for`/`while` statement, in the enclosing `CompoundStmt` block, hence the way it is handled. I've added some comments in this PR to clarify this situation upfront.

Thanks for giving me an opportunity to refactor.

https://github.com/llvm/llvm-project/pull/226101