Page MenuHomePhabricator

[clang-tidy] Refactor: Extract Class CheckRunner on check_clang_tidy.py
AcceptedPublic

Authored by LegalizeAdulthood on Jan 4 2019, 5:09 PM.

Details

Summary

Break up the huge main() function

Diff Detail

Event Timeline

Extract Function try_run

Eugene.Zelenko set the repository for this revision to rCTE Clang Tools Extra.Jan 4 2019, 5:54 PM
Eugene.Zelenko added a project: Restricted Project.
JonasToth added inline comments.
test/clang-tidy/check_clang_tidy.py
112

Its better to use .format() instead of % syntax

206

If would prefer keeping the if check_notes outside of the function call and remove that one argument. Same for the other check_... functions

I really want to get these reviewed quickly. Otherwise, I will run out of available time to complete them and get them submitted.

I really want to get these reviewed quickly. Otherwise, I will run out of available time to complete them and get them submitted.

I will give my best to be available this weekend but the review takes time until the patch is in an acceptable state, same for the other revision.
This process is required to keep good quality and catch bugs, even subtile ones early. Breaking code is the worst of all options.

test/clang-tidy/check_clang_tidy.py
103

Such a long list of returned values make me wonder if it would be worth creating a NamedTuple / class to hold them.
This may make the call site more readable too.

141

These three check_* function all use the same 'FileCheck', '-....'] pattern. Maybe it's worth syndicating that to a try_run_filecheck(input_file0, input_file1, *extra_args)` function.

178

This is the verbose call site I was pointing to above.

To avoid passing lots of state to/from extracted functions, extract a class instead and use member variables for most of the state

LegalizeAdulthood retitled this revision from [clang-tidy] Refactor: Compose Method on check_clang_tidy.py to [clang-tidy] Refactor: Extract Class CheckRunner on check_clang_tidy.py.Mar 15 2019, 12:35 AM

From my side its LGTM, but I would let @serge-sans-paille accept, as he is probably more familiar with python then I am.

LegalizeAdulthood marked 9 inline comments as done.Mar 20 2019, 10:59 AM
LegalizeAdulthood added inline comments.
test/clang-tidy/check_clang_tidy.py
112

That can be done in a later commit; it is not the point of this review.

141

That can be done in a later commit; it is not the point of this review.

178

Addressed by Extract Class instead of Extract Function

206

It's the wrong level of abstraction for the if check to be inside run()

Sorry for the long delay, this LGTM

This revision is now accepted and ready to land.Mon, Aug 19, 6:33 AM