Re: [gem5-dev] Review Request 3648: style: Add options to select checkers and apply fixes

2016-10-10 Thread Andreas Sandberg 


> On Oct. 7, 2016, 11:24 p.m., Brandon Potter wrote:
> > 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 wrote:
> With the exception of the distinction between 4 & 5, isn't this what we 
> are doing already?
> 
> Brandon Potter wrote:
> 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.

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.


- Andreas


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3648/#review8795
---


On Oct. 7, 2016, 4:11 p.m., Andreas Sandberg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3648/
> ---
> 
> (Updated Oct. 7, 2016, 4:11 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> 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 
> Reviewed-by: Andreas Hansson 
> 
> 
> Diffs
> -
> 
>   util/style.py 380375085863 
> 
> Diff: http://reviews.gem5.org/r/3648/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andreas Sandberg
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3648: style: Add options to select checkers and apply fixes

2016-10-10 Thread Brandon Potter 


> On Oct. 7, 2016, 10:24 p.m., Brandon Potter wrote:
> > 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 wrote:
> With the exception of the distinction between 4 & 5, isn't this what we 
> are doing already?

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.


- Brandon


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3648/#review8795
---


On Oct. 7, 2016, 3:11 p.m., Andreas Sandberg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3648/
> ---
> 
> (Updated Oct. 7, 2016, 3:11 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> 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 
> Reviewed-by: Andreas Hansson 
> 
> 
> Diffs
> -
> 
>   util/style.py 380375085863 
> 
> Diff: http://reviews.gem5.org/r/3648/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andreas Sandberg
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3648: style: Add options to select checkers and apply fixes

2016-10-10 Thread Andreas Sandberg 


> On Oct. 7, 2016, 11:24 p.m., Brandon Potter wrote:
> > 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?

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


- Andreas


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3648/#review8795
---


On Oct. 7, 2016, 4:11 p.m., Andreas Sandberg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3648/
> ---
> 
> (Updated Oct. 7, 2016, 4:11 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> 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 
> Reviewed-by: Andreas Hansson 
> 
> 
> Diffs
> -
> 
>   util/style.py 380375085863 
> 
> Diff: http://reviews.gem5.org/r/3648/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andreas Sandberg
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3648: style: Add options to select checkers and apply fixes

2016-10-07 Thread Brandon Potter 

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3648/#review8795
---


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?

- Brandon Potter


On Oct. 7, 2016, 3:11 p.m., Andreas Sandberg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3648/
> ---
> 
> (Updated Oct. 7, 2016, 3:11 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> 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 
> Reviewed-by: Andreas Hansson 
> 
> 
> Diffs
> -
> 
>   util/style.py 380375085863 
> 
> Diff: http://reviews.gem5.org/r/3648/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andreas Sandberg
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3648: style: Add options to select checkers and apply fixes

2016-10-07 Thread Andreas Sandberg 


> On Oct. 7, 2016, 4:16 p.m., Jason Lowe-Power wrote:
> > util/style.py, line 117
> > 
> >
> > 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."
> > ```
> 
> Andreas Sandberg wrote:
> 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 wrote:
> 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?

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.


- Andreas


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3648/#review8783
---


On Oct. 7, 2016, 4:11 p.m., Andreas Sandberg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3648/
> ---
> 
> (Updated Oct. 7, 2016, 4:11 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> 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 
> Reviewed-by: Andreas Hansson 
> 
> 
> Diffs
> -
> 
>   util/style.py 380375085863 
> 
> Diff: http://reviews.gem5.org/r/3648/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andreas Sandberg
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3648: style: Add options to select checkers and apply fixes

2016-10-07 Thread Jason Lowe-Power 


> On Oct. 7, 2016, 3:16 p.m., Jason Lowe-Power wrote:
> > util/style.py, line 117
> > 
> >
> > 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."
> > ```
> 
> Andreas Sandberg wrote:
> 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.

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?


- Jason


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3648/#review8783
---


On Oct. 7, 2016, 3:11 p.m., Andreas Sandberg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3648/
> ---
> 
> (Updated Oct. 7, 2016, 3:11 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> 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 
> Reviewed-by: Andreas Hansson 
> 
> 
> Diffs
> -
> 
>   util/style.py 380375085863 
> 
> Diff: http://reviews.gem5.org/r/3648/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andreas Sandberg
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3648: style: Add options to select checkers and apply fixes

2016-10-07 Thread Andreas Sandberg 


> On Oct. 7, 2016, 4:16 p.m., Jason Lowe-Power wrote:
> > util/style.py, line 117
> > 
> >
> > 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."
> > ```

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.


- Andreas


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3648/#review8783
---


On Oct. 7, 2016, 4:11 p.m., Andreas Sandberg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3648/
> ---
> 
> (Updated Oct. 7, 2016, 4:11 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> 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 
> Reviewed-by: Andreas Hansson 
> 
> 
> Diffs
> -
> 
>   util/style.py 380375085863 
> 
> Diff: http://reviews.gem5.org/r/3648/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andreas Sandberg
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Re: [gem5-dev] Review Request 3648: style: Add options to select checkers and apply fixes

2016-10-07 Thread Jason Lowe-Power 

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3648/#review8783
---

Ship it!


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


util/style.py (line 117)


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."
```


- Jason Lowe-Power


On Oct. 7, 2016, 3:11 p.m., Andreas Sandberg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3648/
> ---
> 
> (Updated Oct. 7, 2016, 3:11 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> 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 
> Reviewed-by: Andreas Hansson 
> 
> 
> Diffs
> -
> 
>   util/style.py 380375085863 
> 
> Diff: http://reviews.gem5.org/r/3648/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andreas Sandberg
> 
>

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

[gem5-dev] Review Request 3648: style: Add options to select checkers and apply fixes

2016-10-07 Thread Andreas Sandberg 

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3648/
---

Review request for Default.


Repository: gem5


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 
Reviewed-by: Andreas Hansson 


Diffs
-

  util/style.py 380375085863 

Diff: http://reviews.gem5.org/r/3648/diff/


Testing
---


Thanks,

Andreas Sandberg

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev