Skip to content

Fix ament_clang_format output with empty files#580

Open
nwn wants to merge 2 commits into
ament:rollingfrom
nwn:nwn-fix-clang-format-empty-files
Open

Fix ament_clang_format output with empty files#580
nwn wants to merge 2 commits into
ament:rollingfrom
nwn:nwn-fix-clang-format-empty-files

Conversation

@nwn

@nwn nwn commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

Clang format doesn't output anything for empty files that are passed to it. This means an empty source code file causes our file indices to get offset from the parsed replacements, resulting in the printed replacements being generated for the wrong file. This behaviour has been verified on clang-format versions 10–21. (This doesn't impact the correctness of the --reformat option, only the correctness of the rendered output without that flag.)

This change detects empty files and fixes the indices accordingly. It also outputs a special output for them.

Example:

Files:

// malformed.cpp
void this_should_get_formatted(     );
// proper.cpp
#include <but/this/file/gets/output/instead>
// empty.cpp (must have size 0)

Before this change:

$ ament_clang_format test/empty.cpp test/proper.cpp test/malformed.cpp 
No code style divergence in file 'test/empty.cpp'

Code style divergence in file 'test/proper.cpp':

[test/proper.cpp:1:31]: Replace [tput/] with []
- #include <but/this/file/gets/output/instead>
+ #include <but/this/file/gets/ouinstead>

1 files with 1 code style divergences

After this change:

$ ament_clang_format test/empty.cpp test/proper.cpp test/malformed.cpp 
Skipping empty file 'test/empty.cpp'

No code style divergence in file 'test/proper.cpp'

Code style divergence in file 'test/malformed.cpp':

[test/malformed.cpp:1:31]: Replace [     ] with []
- void this_should_get_formatted(     );
+ void this_should_get_formatted();

1 files with 1 code style divergences

@nwn nwn force-pushed the nwn-fix-clang-format-empty-files branch from 78269be to 9d07895 Compare April 24, 2026 15:12

@cfveeden cfveeden left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@christophebedard

christophebedard commented Apr 27, 2026

Copy link
Copy Markdown
Member

Pulls: #580
Gist: https://gist.githubusercontent.com/christophebedard/7803d2e56e602540255c390c9beef55c/raw/74341a7e44219679321555654be52a632d24a994/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/19077

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@christophebedard christophebedard left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test would be ideal, but there are no tests for ament_clang_format right now other than linter tests on the package itself. However, you could try extracting this logic to a separate function and then unit-testing it

@tony-p

tony-p commented May 6, 2026

Copy link
Copy Markdown

@christophebedard this PR adds a basic executiion unit test for ament_clang_format #563

nwn added 2 commits July 4, 2026 13:00
clang-format doesn't output anything for empty files that are passed to
it. This causes our file indices to get offset, resulting in the printed
replacements being generated for the wrong file.

Instead, we'll detect empty files and output a special line for them
instead.

Signed-off-by: Nathan Wiebe Neufeldt <wn.nathan@gmail.com>
Without the previous change, the `test_empty_files` test would fail,
though the `test_empty_files_reformat` succeeded. With the previous
change, they both pass.

Signed-off-by: Nathan Wiebe Neufeldt <wn.nathan@gmail.com>
@nwn nwn force-pushed the nwn-fix-clang-format-empty-files branch from 9d07895 to bf57caf Compare July 4, 2026 21:06
@mergify

mergify Bot commented Jul 4, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@nwn

nwn commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

@christophebedard I've added two unit tests to cover the case of empty files, similarly to the example provided in the PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants