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

warnings pass done on macOS w/ clang #1478

Merged
merged 9 commits into from
Mar 18, 2025
Merged

Conversation

cyrush
Copy link
Member

@cyrush cyrush commented Mar 13, 2025

No description provided.

@cyrush
Copy link
Member Author

cyrush commented Mar 13, 2025

@nicolemarsaglia there is another set of warnings related to missing braces in the seed point def logic here:

I think there is also a tabs vs spaces war going on b/c it's hard for me to whip them into shape. We want spaces.
Can you take a look at these and reformat them? It can be on this branch, or on your own branch -- whatever is easiest.

@nicolemarsaglia
Copy link
Contributor

@nicolemarsaglia there is another set of warnings related to missing braces in the seed point def logic here:

I think there is also a tabs vs spaces war going on b/c it's hard for me to whip them into shape. We want spaces. Can you take a look at these and reformat them? It can be on this branch, or on your own branch -- whatever is easiest.

For sure! On it!

@cyrush
Copy link
Member Author

cyrush commented Mar 17, 2025

@nicolemarsaglia @emily-howell can you take a quick looks at these warnings fixes? Want another set of eyes to check for subtle mistakes.

seeds.push_back(vtkm::Particle({x,y_max,z}, seed_count++));
}
}
for(int j = 0; j < num_seeds_y; ++j)
Copy link
Contributor

@nicolemarsaglia nicolemarsaglia Mar 17, 2025

Choose a reason for hiding this comment

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

so weird that this is aligned in my visual studio. I guess there are tabs hidden. I'll do it again. Compare between there and here.

Copy link
Member

Choose a reason for hiding this comment

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

Should we be using something like clang-format to apply consistent formatting to all our changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, barriers to get there are creating a style file and deciding on how we want clang format to be applied (via a CI job, or always during development)

Copy link
Member

Choose a reason for hiding this comment

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

I would opt for during development personally? I'm not sure about Visual Studio but in VSCode you can have it format on save which is pretty easy. Maybe have a CI check that verifies it has been used during development?

I would be happy to make a style file once I finish up my current tickets

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll have a few style suggestions :-), if you can get this moving that would be awesome.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was also requested for conduit -- they can share the same style rules.

Copy link
Contributor

@nicolemarsaglia nicolemarsaglia left a comment

Choose a reason for hiding this comment

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

looks good other than the 1014 vs 1024 Emily pointed out.

@emily-howell
Copy link
Member

Looks good to me now too!

@cyrush cyrush merged commit 361170e into develop Mar 18, 2025
22 checks passed
@cyrush cyrush deleted the task/2025_03_warnings_pass branch March 18, 2025 15:41
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.

3 participants