Page MenuHomePhabricator

Make add_new_check.py's insertion of registerCheck<> match the sort order
ClosedPublic

Authored by dsanders on Aug 20 2019, 4:22 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

dsanders created this revision.Aug 20 2019, 4:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2019, 4:22 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Given that the alphabetization we want really is based on the string literal, would it make sense to look for that rather than the check name? Adding a few more reviewers for a better mix of opinions.

Given that the alphabetization we want really is based on the string literal, would it make sense to look for that rather than the check name? Adding a few more reviewers for a better mix of opinions.

Ideally, it would be nice to sort the list of registrations by the check name. But given that the call can span two lines, that may be slightly trickier to implement. It's worth giving it a try though ;)

dsanders updated this revision to Diff 218010.Aug 29 2019, 7:13 PM

Sort on the check name string

dsanders retitled this revision from Make add_new_check.py's insertion of registerCheck<> more closely match the sort order to Make add_new_check.py's insertion of registerCheck<> match the sort order.Aug 29 2019, 7:15 PM
dsanders edited the summary of this revision. (Show Details)

Mostly LG, if you've verified that this works. A couple of comments below.

clang-tools-extra/clang-tidy/add_new_check.py
192 ↗(On Diff #218010)

Are there actually any registerCheck calls with spaces between the > and the (? Should we clang-format these instead of supporting them in the script?

193 ↗(On Diff #218010)

I'd call this previous_line or prev_line. "Last" is a bit misleading here.

Generally LG to me as well

clang-tools-extra/clang-tidy/add_new_check.py
198 ↗(On Diff #218010)

Missing a full stop at the end of the comment.

dsanders marked 4 inline comments as done.Aug 30 2019, 11:19 AM

Mostly LG, if you've verified that this works. A couple of comments below.

I verified it using

./add_new_check.py llvm prefer-register-over-unsigned
./add_new_check.py llvm zzz
./add_new_check.py llvm nb
./add_new_check.py llvm iz
./add_new_check.py llvm a

which is enough to cover every insertion point in the LLVM module

clang-tools-extra/clang-tidy/add_new_check.py
192 ↗(On Diff #218010)

Are there actually any registerCheck calls with spaces between the > and the (?

No, it was just trivial to be robust against it in case it does somehow happen. Likewise for the whitespace after the '(' but before the '"' or newline.

Should we clang-format these instead of supporting them in the script?

I think the script should stick to inserting the new code rather than correcting existing mistakes that somehow crept in. I do agree that people should generally clang-format their patches before commit though which will prevent it creeping in.

dsanders updated this revision to Diff 218137.Aug 30 2019, 11:20 AM
dsanders marked an inline comment as done.
  • Full stop at end of comment
  • last_line -> prev_line

Missed one of the commands from my history that I used to verify it:

./add_new_check.py llvm i
aaron.ballman accepted this revision.Aug 30 2019, 12:03 PM

LGTM, thank you for working on this!

This revision is now accepted and ready to land.Aug 30 2019, 12:03 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2019, 1:46 PM