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

matdbg: enable source formatting #8338

Merged
merged 2 commits into from
Jan 7, 2025
Merged

matdbg: enable source formatting #8338

merged 2 commits into from
Jan 7, 2025

Conversation

poweifeng
Copy link
Contributor

We use clang-format to format the shader files. Since we assume clang is necessary for compilation, installing clang-format should not be too much of an overhead in terms of adding dependency.

Currently, we only support posix systems (macOS, linux).

@poweifeng poweifeng added the internal Issue/PR does not affect clients label Jan 6, 2025
@poweifeng poweifeng force-pushed the pf/matdbg-format branch 2 times, most recently from 48a7263 to 2f7fc3c Compare January 6, 2025 20:38
Copy link
Contributor

@z3moon z3moon left a comment

Choose a reason for hiding this comment

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

Great change, thanks!

"MaxEmptyLinesToKeep: 2"
"}'";

fp = popen(("clang-format " + CLANG_FORMAT_OPTIONS + "< " + TMP_FILENAME).c_str(), "r");
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if you use a pipe, I think you don't need to create a temporary file so a bit more efficient.

e.g., echo "int main() {return 0;}" | clang-format -style=Google

but this already looks good, so I'll leave it up to your decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

noted. I couldn't find a read-write way to do popen and also didn't want to deal with any kind of escaping of strings.

We use clang-format to format the shader files. Since we assume
clang is necessary for compilation, installing clang-format should
not be too much of an overhead in terms of adding dependency.

Currently, we only support posix systems (macOS, linux).
@poweifeng poweifeng enabled auto-merge (squash) January 7, 2025 17:58
@poweifeng poweifeng merged commit a003d37 into main Jan 7, 2025
12 checks passed
@poweifeng poweifeng deleted the pf/matdbg-format branch January 7, 2025 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Issue/PR does not affect clients
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants