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

Run clang-tidy (or clang-format?) on src/_ext.cpp #124

Closed
jpivarski opened this issue Oct 26, 2022 · 3 comments · Fixed by #125
Closed

Run clang-tidy (or clang-format?) on src/_ext.cpp #124

jpivarski opened this issue Oct 26, 2022 · 3 comments · Fixed by #125

Comments

@jpivarski
Copy link
Member

There are loops in src/_ext.cpp that aren't indented, which would make it a lot easier to follow their logic. If this were Python, we could just run black on all the code (well, if this were Python, the wrong-indentation would be syntactically wrong).

fastjet/src/_ext.cpp

Lines 134 to 146 in f12e4ea

for(int i = 0; i < len; i++){
auto jets = ow.cse[i]->inclusive_jets(min_pt);
for (unsigned int j = 0; j < jets.size(); j++)
{
ptrpx[idxe] = jets[j].px();
ptrpy[idxe] = jets[j].py();
ptrpz[idxe] = jets[j].pz();
ptrE[idxe] = jets[j].E();
idxe++;
}
*ptroff = jets.size()+*(ptroff-1);
ptroff++;
}

Is there an equivalent with clang-tidy (or clang-format)?

That is, no meticulous setup, specifying which rules you want to use or not use, just run it and the code is better? Perhaps it would be ideal to have it in CI, but even that is overkill: running it once and getting everything indented once would make a huge impact on readability, even if it isn't enforced for future commits and slowly decays. (Then just run it again!)

@henryiii
Copy link
Member

even if it isn't enforced for future commits and slowly decays. (Then just run it again!)

Just enforce it with pre-commit. It'll fix it and you'll never have to worry about poorly formatted code again.

And yes, the tool is clang-format, there's a pre-commit check in skhep dev pages, and you can use -style=llvm if you want no setup. Or just grab the .clang-format file from boost-histogram.

@jpivarski
Copy link
Member Author

I wanted the request to sound really easy, so that it wouldn't be postponed due to not being sure how to add something to pre-commit. Running it once would be life-changing; running it in every commit would be even better.

@chrispap95
Copy link
Collaborator

I was looking about this in the scikit-hep documentation earlier today. The fix seems like this

- repo: https://github.com/pre-commit/mirrors-clang-format
  rev: "v14.0.6"
  hooks:
    - id: clang-format
      types_or: [c++, c, cuda]

plus a .clang-format file. Should we try this?

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