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

Add clang-format infrastructure #1926

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from

Conversation

jleidel
Copy link
Contributor

@jleidel jleidel commented Jul 22, 2022

Adding clang format configuration file and clang format script. The .clang-format script was copied directly from the sst-core master branch on 07/22/2022. The clang-format-test.sh script was also pulled from sst-core, but modified slightly for the sst-elements directory structure. This PR is in prep to begin reformatting the sst-elements code (which contains a rather diverse set of code styles) in the same manner as sst-core. We hope to move to a centrally formatted set of sst-elements in the future and add the clang-format-test script to the standard PR checks.

@sst-autotester
Copy link
Contributor

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

@feldergast
Copy link
Contributor

I like the idea of making the .clang-format file available in the elements if lead developers for each library would like to use it. However, in the near to medium term there are no plans to wholesale force the formatting on all the elements and add a test to the autotester. Code formatting will be left to the lead developers for each element.

Give this, I think we would probably want to modify the test script to require a specific directory to check as a required argument. The script should check to make sure that the directory is not the top level directory for the elements (or the script could just refuse to run at the top level directory and force the users to run it in a subdirectory instead of requiring the directory name as input). I don't want someone accidentally affecting the whole elements repo. Another issue is that some of the element directories have .hpp and .cpp files that would need to be included in the script (if it's not already there). Once these changes are added to the test script, we can release the PR for testing.

@jleidel
Copy link
Contributor Author

jleidel commented Aug 3, 2022

I like the idea of making the .clang-format file available in the elements if lead developers for each library would like to use it. However, in the near to medium term there are no plans to wholesale force the formatting on all the elements and add a test to the autotester. Code formatting will be left to the lead developers for each element.

Give this, I think we would probably want to modify the test script to require a specific directory to check as a required argument. The script should check to make sure that the directory is not the top level directory for the elements (or the script could just refuse to run at the top level directory and force the users to run it in a subdirectory instead of requiring the directory name as input). I don't want someone accidentally affecting the whole elements repo. Another issue is that some of the element directories have .hpp and .cpp files that would need to be included in the script (if it's not already there). Once these changes are added to the test script, we can release the PR for testing.

I made the prescribed changes. The script now checks to see whether it's running in ./ or ./src/sst/elements (and all variants thereof). I created a sample directory under src/sst/elements/ and ran the script. It correctly utilizes the prescribed .clang-format file and finds the necessary issues.

@sst-autotester
Copy link
Contributor

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

@sst-autotester
Copy link
Contributor

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

@hughes-c hughes-c added this to the Future milestone Oct 8, 2024
@sst-autotester
Copy link
Contributor

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

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

Successfully merging this pull request may close these issues.

4 participants