Page MenuHomePhabricator

[clang-tidy] rename_check.py: maintain alphabetical order in Renamed checks section
ClosedPublic

Authored by Eugene.Zelenko on Jan 28 2020, 2:10 PM.

Details

Summary

Also use check in add_new_check.py for terminology consistency.

PS

My GitHub ID is EugeneZelenko, if it's necessary for attribution.

Diff Detail

Event Timeline

Eugene.Zelenko created this revision.Jan 28 2020, 2:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2020, 2:10 PM
Herald added a subscriber: xazax.hun. · View Herald Transcript
Andi added a subscriber: Andi.Jan 29 2020, 3:19 PM

It looks good, but could you maybe create a child revision showing what it looks like with a few checks renamed to make sure everything is all working correctly.

It looks good, but could you maybe create a child revision showing what it looks like with a few checks renamed to make sure everything is all working correctly.

I think it'll be easier to test script locally, since several (at least two) child revisions would be necessary. By the word, main script boy is borrowed from add_new_check.py, so difference only in section name and regular expression for entry.

So I tried to ran ./rename_check.py readability-braces-around-statements readability-braces and results were less than desirable. It renamed the readability-braces-around-statements but it also renamed the google alias to google-readability-braces. In the documentation for google-readability-braces it changed the subject line to

google-readability-braces
===========================================

Feel like the correct course of action is to match against the entire check name rather than a sub string of it (which i guess was unintentional). This doesn't need to be done in this review and could (should) be a follow up.

The actual alphabetical ordering of the release notes works as intended though.

alexfh added inline comments.Feb 1 2020, 6:40 PM
clang-tools-extra/clang-tidy/rename_check.py
172–174

What's the preferred variable naming convention in Python in LLVM? Looking at the two scripts now, I see that the naming style is already quite inconsistent. It would be nice to make naming consistent in these scripts.

Not an action item for this patch, just a drive-by comment.

Another side effect is that rename_check.py messes list.rst by removing "Offer fixes". Probably proper solution would be moving update_checks_list from add_new_check.py to shared module.

njames93 accepted this revision.Feb 19 2020, 3:44 PM

LGTM, For what this is trying to achieve I have no issue, follow up patches can be submitted for the other enhancements needed.

This revision is now accepted and ready to land.Feb 19 2020, 3:44 PM

LGTM, For what this is trying to achieve I have no issue, follow up patches can be submitted for the other enhancements needed.

Please commit patch if you'll have time. See summary for details.

LGTM, For what this is trying to achieve I have no issue, follow up patches can be submitted for the other enhancements needed.

Please commit patch if you'll have time. See summary for details.

I figured it out, arc patch doesn't seem to handle the author of the commit, but I could use git commit amend for that

This revision was automatically updated to reflect the committed changes.