Limit number of columns to 160 by asuessenbach · Pull Request #1491 · KhronosGroup/Vulkan-Samples

asuessenbach · 2026-02-25T15:52:02Z

Description

Every now and then, I'm surprised about the length of some code lines. I would suggest to limit that to, say, 160.
The only manual change in this PR is in .clang-format, setting ColumnLimit : 160. Then all *.cpp and *.h files are formatted accordingly.

In addition to the column limit I changed the following option:
BinPackArguments: false
BinPackParameters: false

Build tested on Win11 with VS2022. Run tested on Win11 with NVidia GPU.

General Checklist:

Please ensure the following points are checked:

Note: The Samples CI runs a number of checks including:

I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

If this PR contains framework changes:

I did a full batch run using the batch command line argument to make sure all samples still work properly

Sample Checklist

If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist:

I have tested the sample on at least one compliant Vulkan implementation
If the sample is vendor-specific, I have tagged it appropriately
I have stated on what implementation the sample has been tested so that others can test on different implementations and platforms
Any dependent assets have been merged and published in downstream modules
For new samples, I have added a paragraph with a summary to the appropriate chapter in the readme of the folder that the sample belongs to e.g. api samples readme
For new samples, I have added a tutorial README.md file to guide users through what they need to know to implement code using this feature. For example, see conditional_rendering
For new samples, I have added a link to the Antora navigation so that the sample will be listed at the Vulkan documentation site

gpx1000 · 2026-02-25T17:52:06Z

It looks like we're gonna need to fix all the instances where the columns are long or limit it from CI being the cause of failing. But I'm all for the idea!

asuessenbach · 2026-02-26T09:58:25Z

... and it seems, I'm using a different clang-format version (21.1.0) than the CI. Is version to use specified, somewhere?

SaschaWillems · 2026-02-27T15:20:15Z

clang-format version is listed here. Not sure if that's up-to-date though.

SaschaWillems · 2026-02-28T10:05:53Z

Looking at the numerous changes: Increasing the column limit might make sense, but some of the changes make code harder to read. Not sure if this is something we want to apply across the board to all samples.

asuessenbach · 2026-03-02T17:31:26Z

Increasing the column limit might make sense

You mean, even more than 160 columns?

some of the changes make code harder to read.

Could you point me at some of those places? Maybe with some additional clang-format settings, it looks better, again.

Not sure if this is something we want to apply across the board to all samples.

I think, it should either be done for the complete repository, or not at all.

asuessenbach · 2026-03-04T09:00:02Z

clang-format version is listed here. Not sure if that's up-to-date though.clang-format version is listed here. Not sure if that's up-to-date though.

It's up-to-date. In check.yml, you'll find
container: ghcr.io/khronosgroupactions/clang-tools:15.0.0

asuessenbach · 2026-03-04T10:45:43Z

Well, in fact it's clang-format 15.0.6, that is used. And for some unknown reasons, it required a few manual adjustments.
But at least, the CI accepts the formatting now.

But I definitely won't crank up the copyright of all those formatted files.

SaschaWillems · 2026-03-23T18:24:28Z

-                 "Specify the folder containing the sample data folders.",
-                 {vkb::Hook::OnAppStart},
-                 {},
+    DataPathTags("Data Path Override", "Specify the folder containing the sample data folders.", {vkb::Hook::OnAppStart}, {},


I'm not really sure if this improves readability. I actually prefer the way it was written before this change.

Any way to fix this? There are several spots that have had their wrapping changed in a way that at least IMO hurts code readabilit.

With BinPackParameters: false, that looks better again.

I think the original was easier to read too. When we enter the realm of automatic line wrapping we've gone wrong somewhere. I'd be tempted to be much more conservative in terms of line width as seeing 2 versions side-by-side is often handy.

I think the original was easier to read too.

Do you refer to the original of this specific code part? I think, they're pretty close, now.
Or do you have other part is mind? If so, could you hint me to some of them? Maybe I can improve the readability there as well.

When we enter the realm of automatic line wrapping we've gone wrong somewhere.

I don't understand what you mean with that.

I'd be tempted to be much more conservative in terms of line width as seeing 2 versions side-by-side is often handy.

Do I read that like "It would be better to reduce the column limit to, say, 120"?

I was referring to this specific bit.

In general when line ends are added by hand or by a code formatting tool, the code is easier to read, compared to auto-wrapping done by the viewer or horizontal scrolling to see the rest of it.

I think 120 is about right. Less than this will likely result in a massive change to the existing codebase, more will be hard to work with on a laptop screen.

I was referring to this specific bit.

ok... and have you seen it with the latest adjustment from last week? Do you think, it's ok that way?

I think 120 is about right.

If there's more than just your request, I can easily cut column length down to 120.

asuessenbach · 2026-03-24T08:23:51Z

@@ -1,4 +1,4 @@
-/* Copyright (c) 2020-2025, Arm Limited and Contributors
+/* Copyright (c) 2020-2026, Arm Limited and Contributors
 * Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved.


Apparently, neither the copyright.py fixes such Copyright strings, nor the copyright check of the CI catches them?

asuessenbach · 2026-03-24T09:48:51Z

Well, no matter what clang-format version (15.0.x, 0 <= x <= 7 ), win64, I tried, there are four files that needs some manual adjustments to keep the format check happy.
Could someone extent the check.yml in a way that the actually used clang-format version is displayed? I don't even know where the clang-format.exe lives in that context.

tomek-brcm

The updated pull request reads easier than the old one. I think 180 columns is too much, especially for a side-by-side diff I'm looking at right now. I don't want to impose my personal preferences on anybody here, but I'd give 120 columns a try

tomek-brcm · 2026-04-28T13:02:56Z

-	    VK_DYNAMIC_STATE_VIEWPORT,
-	    VK_DYNAMIC_STATE_SCISSOR,
-	    VK_DYNAMIC_STATE_LINE_WIDTH,
-	    VK_DYNAMIC_STATE_DEPTH_BIAS,
-	    VK_DYNAMIC_STATE_BLEND_CONSTANTS,
-	    VK_DYNAMIC_STATE_DEPTH_BOUNDS,
-	    VK_DYNAMIC_STATE_STENCIL_COMPARE_MASK,
-	    VK_DYNAMIC_STATE_STENCIL_WRITE_MASK,
+	    VK_DYNAMIC_STATE_VIEWPORT,          VK_DYNAMIC_STATE_SCISSOR,      VK_DYNAMIC_STATE_LINE_WIDTH,           VK_DYNAMIC_STATE_DEPTH_BIAS,
+	    VK_DYNAMIC_STATE_BLEND_CONSTANTS,   VK_DYNAMIC_STATE_DEPTH_BOUNDS, VK_DYNAMIC_STATE_STENCIL_COMPARE_MASK, VK_DYNAMIC_STATE_STENCIL_WRITE_MASK,
 	    VK_DYNAMIC_STATE_STENCIL_REFERENCE,


nit: this was a bit more future-proof, it's easier to see additions and deletions with one state per line.

Added BinPackArguments: false to resolve that.

asuessenbach · 2026-04-28T19:09:28Z

I can't get what difference there is between the clang-format I'm using and the clang-format of the CI.
And I can't get what the CI is trying to tell me about the differences.
If anybody can actually decipher the CI messages on the failing formatting step, I'd be happy to manually adjust those (hopefully just a couple of) files.

Otherwise, I'm essentially out on this PR and would have to just close it.

gpx1000 · 2026-04-28T19:56:19Z

https://github.com/KhronosGroup/Vulkan-Samples/actions/runs/25072289035/artifacts/6692580894 <-- I setup CI to upload a diff of what it found.

Also, see if the script helps: scripts/clang-format.py

I'll take a real look when I get a chance and try to help.

asuessenbach · 2026-04-29T08:57:45Z

Also, see if the script helps: scripts/clang-format.py

Well, that python script changes a lot of files. But the CI is still failing.

I setup CI to upload a diff of what it found.

Sure. But how would I get what has to be changed out of that blob of characters?

asuessenbach requested a review from a team February 25, 2026 17:32

asuessenbach force-pushed the ColumnLimit branch 8 times, most recently from ca0d397 to 3eb0a5d Compare March 4, 2026 10:38

asuessenbach force-pushed the ColumnLimit branch from 06a2b6e to 5ce473a Compare March 10, 2026 07:45

asuessenbach force-pushed the ColumnLimit branch 2 times, most recently from 4414079 to 13ddd1b Compare March 23, 2026 17:16

SaschaWillems reviewed Mar 23, 2026

View reviewed changes

asuessenbach force-pushed the ColumnLimit branch from 13ddd1b to 68a2891 Compare March 24, 2026 08:18

asuessenbach commented Mar 24, 2026

View reviewed changes

asuessenbach force-pushed the ColumnLimit branch 4 times, most recently from 0fee7e7 to 03bb966 Compare March 24, 2026 08:41

asuessenbach requested review from SaschaWillems and gpx1000 March 24, 2026 19:23

asuessenbach force-pushed the ColumnLimit branch from 03bb966 to 4ff3de1 Compare April 15, 2026 08:32

asuessenbach force-pushed the ColumnLimit branch 9 times, most recently from 51bc762 to 5d4c8f5 Compare April 21, 2026 12:38

asuessenbach mentioned this pull request Apr 28, 2026

PR 1491 (Limit number of columns to 160) is gone? #1530

Closed

tomek-brcm reviewed Apr 28, 2026

View reviewed changes

asuessenbach force-pushed the ColumnLimit branch 11 times, most recently from a7753e2 to c86a291 Compare April 28, 2026 19:06

Set BinPackArguments: false

6a2fc29

asuessenbach force-pushed the ColumnLimit branch from c86a291 to 6a2fc29 Compare April 29, 2026 08:53

Conversation

asuessenbach commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

General Checklist:

Sample Checklist

Uh oh!

gpx1000 commented Feb 25, 2026

Uh oh!

asuessenbach commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SaschaWillems commented Feb 27, 2026

Uh oh!

SaschaWillems commented Feb 28, 2026

Uh oh!

asuessenbach commented Mar 2, 2026

Uh oh!

asuessenbach commented Mar 4, 2026

Uh oh!

asuessenbach commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asuessenbach commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomek-brcm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asuessenbach commented Apr 28, 2026

Uh oh!

gpx1000 commented Apr 28, 2026

Uh oh!

asuessenbach commented Apr 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

asuessenbach commented Feb 25, 2026 •

edited

Loading

asuessenbach commented Feb 26, 2026 •

edited

Loading

asuessenbach commented Mar 4, 2026 •

edited

Loading

asuessenbach commented Mar 24, 2026 •

edited

Loading