Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vvl: Test a new .clang-format #9172

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arno-lunarg
Copy link
Contributor

No description provided.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 340475.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 18574 running.

@arno-lunarg
Copy link
Contributor Author

So for instance I like that for GPU-AV specifically because it turns this:
image

into this:

image

So parsing the captured variables list in the lambda is way easier

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 18574 failed.

namespace bp_state {
class Image : public vvl::Image {
namespace bp_state
{
Copy link
Contributor

Choose a reason for hiding this comment

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

moving brackets around seems very unnecessary to me

CALL_STATE vkGetPhysicalDeviceSurfacePresentModesKHRState = UNCALLED;
CALL_STATE vkGetPhysicalDeviceSurfaceFormatsKHRState = UNCALLED;
uint32_t surface_formats_count = 0;
CALL_STATE vkGetPhysicalDeviceLayerPropertiesState = UNCALLED; // Currently unused
Copy link
Contributor

Choose a reason for hiding this comment

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

not a huge fan of lining things up in vertical columns. Fortan called and wants it format back

uint32_t numDrawCallsDepthOnly = 0;
struct RenderPassState
{
bool depthAttachment = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

🤮

Copy link
Contributor Author

@arno-lunarg arno-lunarg Jan 9, 2025

Choose a reason for hiding this comment

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

Is the issue the blanks between the variable name and its default value? I should be able to remove that

Copy link
Contributor

Choose a reason for hiding this comment

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

All the extra spaces on lines like this. trying to make vertical columns will lead to lots of churn whenever someone adds or removes a declaration that causes the columns to change. And inevitably someone who isn't fully up to speed on git clang-format will waste a bunch of time trying to fix the columns manually.

const VkMemoryHeap& memory_heap,
std::optional<vvl::DedicatedBinding>&& dedicated_binding,
uint32_t physical_device_count) :
vvl::DeviceMemory(handle,
Copy link
Contributor

Choose a reason for hiding this comment

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

this part is mostly nicer

Copy link
Contributor

Choose a reason for hiding this comment

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

well until you have a 13-inch laptop screen and this takes up half of it 🤷‍♂️

@arno-lunarg arno-lunarg force-pushed the arno-new-clang-format branch from 5230e4a to 997f2ba Compare January 9, 2025 09:46
@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 340999.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 18582 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 18582 failed.

return ValidateBuildAccelerationStructure(commandBuffer, error_obj.location);
}

bool BestPractices::ValidateBuildAccelerationStructure(VkCommandBuffer commandBuffer, const Location& loc) const {
bool skip = false;
bool skip = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get why we need this white space like this... I'm gonna just go crazy and end up trying to add more newlines to satisfy my OCD to not have this like this

uint64_t num_submits = 0;
bool uses_vertex_buffer = false;
uint32_t small_indexed_draw_call_count = 0;
uint64_t num_submits = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, not of fan have to highlight over a line to make sure I know which type it is once these get many lines stack on it

@spencer-lunarg
Copy link
Contributor

just want to say if we needed to revert something like #9200 that would have been a nightmare/impossible if we touch every file like this.... I just don't see what we gain from this, the whole idea of clang-format is we picked something and stick with it, we plan to update this clang-format again in 2 years when someone else suggests it is better, if we change it now, what justification is there to not change it again in the future

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