style: Add options to select checkers and apply fixes

Information
Submitter:	Andreas Sandberg
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 11669:258c76f88f24
---------------------------
style: Add options to select checkers and apply fixes

Add an option, --checker/-c, to style.py that selects individual style
checkers to apply. When this option isn't specified, the script
defaults to all available style checkers. The option may be specified
multiple times to run multiple style checkers.

The option, --fix/-f, can be specified to automatically fix style
violations.

Change-Id: Id7597fba6b65cecfa17a88b1c87c8a4c8315af59
Signed-off-by: Andreas Sandberg <andreas.sandberg@arm.com>
Reviewed-by: Andreas Hansson <andreas.hansson@arm.com>

Testing Done

Issue Summary

Description	From	Last Updated	Status
I'm going to be picky here... Can you change this to the below so you don't have the awkward newline ...	Jason Lowe-Power	Oct. 7, 2016, 8:19 a.m.	Dropped

Minor thing below, but other than that it looks good.

util/style.py (Diff revision 1)

I'm going to be picky here... Can you change this to the below so you don't have the awkward newline when printing the help text.

help="Style checkers to run. Can be specified "
"multiple times."

Show all issues

Andreas Sandberg Oct. 7, 2016, 8:24 a.m. (Oct. 7, 2016, 8:24 a.m.)

Argparse actually ignores this newline. There is a help formatter thingy that removes newlines and generally cleans up indentation. We actually have a custom formatter in tests/tests.py to make it print paragraphs correctly in the usage epilog.

Jason Lowe-Power Oct. 7, 2016, 8:58 a.m. (Oct. 7, 2016, 8:58 a.m.)

I didn't know that. Thanks for informing me.

We should pick one or the other and put it in the style guide. I see a mix of both throughout the codebase. I would personally rather not use the triple quotes to keep things consistent between python and C/C++. Thoughts?

Andreas Sandberg Oct. 7, 2016, 9:52 a.m. (Oct. 7, 2016, 9:52 a.m.)

I'm not too bothered about C++ and Python being consistent. In general, we should follow the official Python style guide (there is such a thing) for Python files. Otherwise, we end up with weird looking Python code. It's not possible to do consistently though because the C++ world "leaks" into Python.

Do you think it's worthwhile to adopt a more detailed organization for headers?

Currently, we have this:
1) primary header (foo.hh for foo.cc)
2) mix of C/C++ headers alphabetically ordered
3) project headers alphabetically ordered

Should we move to something like the Google Style Guide's ruleset? https://google.github.io/styleguide/cppguide.html

It looks like this:
1) dir2/foo2.h.
2) C system files.
3) C++ system files.
4) Other libraries' .h files.
5) Your project's .h files.

Having the primary header on top is useful, but I do not know what the other ordering provides or that it protects us against anything. It does provide more organization so that it's easier to identify an include if you happen to glance up at the header sections, but that seems minor and probably isn't their justification. Does anyone else have thoughts on the topic?

Andreas Sandberg Oct. 10, 2016, 10:25 a.m. (Oct. 10, 2016, 10:25 a.m.)

With the exception of the distinction between 4 & 5, isn't this what we are doing already?

Brandon Potter Oct. 10, 2016, 11:05 a.m. (Oct. 10, 2016, 11:05 a.m.)

Yeah, it is the same except 4 & 5. I thought that 2 & 3 might have been crammed together in our files, but it doesn't actually seem to be the case.

Andreas Sandberg Oct. 10, 2016, 1:28 p.m. (Oct. 10, 2016, 1:28 p.m.)

There might be a few other subtle differences. We actually don't distinguish between system files and other libraries that we depend on with the exception of ext. In practice, this probably makes very little differences since gem5 has few external library dependencies.

I'm tempted to say that adding more organization to the header block isn't really worth it at the moment.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Status: Closed (submitted)