clang-tidy readability-redundant-control-flow removes comments and conditional compilation blocks before the return statement

[bobtista]

It seems that everything between the last normal line in a code block and the closing brace is deleted, including things that should not be touched. Examples:

Bigger problem (conditional compilation block):

-#if defined(RTS_DEBUG)
-       m_networkOn = TRUE;
-#endif
-
-       return;
- 

Annoying problem (Comments removed)

-       //{{AFX_DATA_MAP(CBackgroundBMPDialog)
-               // NOTE: the ClassWizard will add DDX and DDV calls here
-       //}}AFX_DATA_MAP
-    return ;

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Bobby Battista (bobtista)

Details

It seems that everything between the last normal line in a code block and the closing brace is deleted, including things that should not be touched. Examples:

Bigger problem (conditional compilation block):

-#if defined(RTS_DEBUG)
-       m_networkOn = TRUE;
-#endif
-
-       return;
- 

Annoying problem (Comments removed)

-       //{{AFX_DATA_MAP(CBackgroundBMPDialog)
-               // NOTE: the ClassWizard will add DDX and DDV calls here
-       //}}AFX_DATA_MAP
-    return ;

[vbvictor]

Could you please provide a godbolt reproducer, like https://godbolt.org/z/Erd8e794G

[bobtista]

https://godbolt.org/z/Erd8e794G

It's not reproducing there :( https://godbolt.org/z/9o8x48MhM
But if I run clang-tidy -p build/clang-tidy Core/GameEngine/Source/GameNetwork/Network.cpp -checks="-*,readability-redundant-control-flow" --fix locally, it happens consistently. Would it help of I shared the repo with you?

[vbvictor]

https://godbolt.org/z/Erd8e794G

It's not reproducing there :( https://godbolt.org/z/9o8x48MhM But if I run clang-tidy -p build/clang-tidy Core/GameEngine/Source/GameNetwork/Network.cpp -checks="-*,readability-redundant-control-flow" --fix locally, it happens consistently. Would it help of I shared the
repo with you?

Yes, at least a repo with instructions on how to setup/run would be helpful.
What version of clang-tidy do you use? If not 21/22 (main branch), please try running with them. We only support the latest releases of tools.

[bobtista]

https://godbolt.org/z/Erd8e794G

It's not reproducing there :( https://godbolt.org/z/9o8x48MhM But if I run clang-tidy -p build/clang-tidy Core/GameEngine/Source/GameNetwork/Network.cpp -checks="-*,readability-redundant-control-flow" --fix locally, it happens consistently. Would it help of I shared the
repo with you?

Yes, at least a repo with instructions on how to setup/run would be helpful. What version of clang-tidy do you use? If not 21/22 (main branch), please try running with them. We only support the latest releases of tools.

https://github.com/TheSuperHackers/GeneralsGameCode

LLVM version 21.1.7

To set up I'd just clone it, then run
cmake -B build/clang-tidy -DCMAKE_DISABLE_PRECOMPILE_HEADERS=ON -G "Visual Studio 17 2022" -A Win32
Then
clang-tidy -p build/clang-tidy Core/GameEngine/Source/GameNetwork/Network.cpp -checks="-*,readability-redundant-control-flow" --fix-errors

The warning for me in the output includes the conditional code before the return, and using --fix-errors removes the additional lines. There's a error: 'PreRTS.h' file not found [clang-diagnostic-error], so I used the --fix-errors.

EDIT: I thought that my mac had a different outcome - just tested again, it's the same on the mac :(

[vbvictor]

By your comment

The warning for me in the output includes the conditional code before the return, and using --fix-errors removes the additional lines. There's a error: 'PreRTS.h' file not found [clang-diagnostic-error], so I used the --fix-errors.

I see that clang fronted didn't parse the file properly (by emitting error), so clang-tidy must operate on a malformed AST, which could lead to incorrect warnings/fixes. Maybe it messes up its internal locations inside files, and it removes more than needed thus you see incorrect fixes.

Do you have any clang-diagnostic-error when compiling on Mac?

[vbvictor]

Could you set up clang-tidy to process files without errors on Windows?

[bobtista]

On Mac, I get error: 'atlbase.h' file not found [clang-diagnostic-error], not PreRTS.h itself. I think PreRTS includes it, and it isn't available on macOS.

Could this be about LF vs CRLF in compile_commands.json or in the file itself? Or about paths using forward slashes vs backwards slashes on windows/mac? Or something entirely different?

[bobtista]

Ok I noticed that on Windows I'd ran the initial cmake command from git bash instead of x86 native VS terminal - so it failed to make compile_commands.json. After I generated it using the right terminal window, the PreRTS error disappeared.
However, the problem persists.
I also tried converting from CRLS to LF in the file locally, but the issue still persists, so it's not that.

[bobtista]

Without -Werror (normal run):
No "Error while processing" message
Only warnings shown

That said, with -Werror:

• clang-diagnostic-new-returns-null (22 occurrences)
  -- 'operator new' should not return a null pointer unless it is declared 'throw()' or 'noexcept'
  -- Files: AudioEventRTS.h, GameMemory.h, MessageStream.h, Module.h, NameKeyGenerator.h, Overridable.h, Science.h, Thing.h, Upgrade.h
• clang-diagnostic-implicit-exception-spec-mismatch (2 occurrences)
  -- function previously declared with an explicit exception specification redeclared with an implicit exception specification
  -- File: GameMemory.h (lines 854, 857)
• clang-diagnostic-logical-not-parentheses (1 occurrence)
  -- logical not is only applied to the left hand side of this bitwise operator
  -- File: wwmath.h:587
• clang-diagnostic-switch (2 occurrences)
  -- enumeration values not handled in switch
  -- File: Damage.h (lines 118, 135)
• clang-diagnostic-unused-command-line-argument (1 occurrence)
  -- argument unused during compilation: '/MP'
• clang-diagnostic-error (1 occurrence)
  -- too many errors emitted, stopping now

Are any of these suppressed errors likely to cause a malformed AST? Are any of them things I should actually fix? Or false positives? Are there work arounds or options I can give clang-tidy here?

[bobtista]

I made an edit above - the Mac also has the same issue, I was wrong when I thought it was not affecting the mac. I can reproduce it there too.

[vbvictor]

All diagnostics except for clang-diagnostic-error just warning from compiler flags, see https://clang.llvm.org/extra/clang-tidy/index.html#using-clang-tidy. They don't affect AST.

Clang diagnostics are treated in a similar way as check diagnostics. Clang diagnostics are displayed by clang-tidy and can be filtered out using the -checks= option. However, the -checks= option does not affect compilation arguments, so it cannot turn on Clang warnings which are not already turned on in the build configuration. The -warnings-as-errors= option upgrades any warnings emitted under the -checks= flag to errors (but it does not enable any checks itself).

Clang diagnostics have check names starting with clang-diagnostic-. Diagnostics which have a corresponding warning option, are named clang-diagnostic-, e.g. Clang warning controlled by -Wliteral-conversion will be reported with check name clang-diagnostic-literal-conversion.

Clang compiler errors (such as syntax errors, semantic errors, or other failures that prevent Clang from compiling the code) are reported with the check name clang-diagnostic-error. These represent fundamental compilation failures that must be fixed before clang-tidy can perform its analysis. Unlike other diagnostics, clang-diagnostic-error cannot be disabled, as clang-tidy requires valid code to function.

So without -Werror there are no warnings clang-diagnostic-*, right?

[bobtista]

All diagnostics except for clang-diagnostic-error just warning from compiler flags, see https://clang.llvm.org/extra/clang-tidy/index.html#using-clang-tidy. They don't affect AST.

Clang diagnostics are treated in a similar way as check diagnostics. Clang diagnostics are displayed by clang-tidy and can be filtered out using the -checks= option. However, the -checks= option does not affect compilation arguments, so it cannot turn on Clang warnings which are not already turned on in the build configuration. The -warnings-as-errors= option upgrades any warnings emitted under the -checks= flag to errors (but it does not enable any checks itself).

Clang diagnostics have check names starting with clang-diagnostic-. Diagnostics which have a corresponding warning option, are named clang-diagnostic-, e.g. Clang warning controlled by -Wliteral-conversion will be reported with check name clang-diagnostic-literal-conversion.

Clang compiler errors (such as syntax errors, semantic errors, or other failures that prevent Clang from compiling the code) are reported with the check name clang-diagnostic-error. These represent fundamental compilation failures that must be fixed before clang-tidy can perform its analysis. Unlike other diagnostics, clang-diagnostic-error cannot be disabled, as clang-tidy requires valid code to function.

So without -Werror there are no warnings clang-diagnostic-*, right?

Correct

[vbvictor]

I could reproduce it too

\Network.cpp:364:2: warning: redundant return statement at the end of a function with a void return type [readability-redundant-control-flow]
  358 |
  359 |
  360 | #if defined(RTS_DEBUG)
      | ~~~~~~~~~~~~~~~~~~~~~~
  361 |         m_networkOn = TRUE;
      |         ~~~~~~~~~~~~~~~~~~~
  362 | #endif
      | ~~~~~~
  363 |
  364 |         return;
      |         ^~~~~~~
  365 | }