Re: [chromium-dev] Linting chrome/ in pre-submit checks
On Tue, Nov 17, 2009 at 20:43, Peter Kasting wrote: > On Tue, Nov 17, 2009 at 11:40 AM, Evan Martin wrote: > >> Since we're talking about style, I'll note that this pattern is no >> good (and I've seen it explicitly called out somewhere before). >> >> The problem is that your assertions are not helpful. You get >> "expected 'foo', got 'bar' on line 80" but line 80 is just the body of >> a for loop. > > > (a) I frequently write EXPECT_EQ(a, b) << "Test case in question is " << c; > (b) Even when this is not true it costs me almost no time to track down the > case in question in the debugger (if I need to; often the expected result is > unique) > You can also use SCOPED_TRACE (learned that from eroman). -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Linting chrome/ in pre-submit checks
On Tue, Nov 17, 2009 at 11:40 AM, Evan Martin wrote: > Since we're talking about style, I'll note that this pattern is no > good (and I've seen it explicitly called out somewhere before). > > The problem is that your assertions are not helpful. You get > "expected 'foo', got 'bar' on line 80" but line 80 is just the body of > a for loop. (a) I frequently write EXPECT_EQ(a, b) << "Test case in question is " << c; (b) Even when this is not true it costs me almost no time to track down the case in question in the debugger (if I need to; often the expected result is unique) Don't assume what is and is not helpful to me. PK -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Linting chrome/ in pre-submit checks
On Tue, Nov 17, 2009 at 11:31 AM, Nico Weber wrote: > Is cpplint the thing that generates the "lint errors" column on codereview? > If so, it doesn't work very well for objective-c++ files (.mm) and .h files > that contain objc++ declarations. The PRESUBMIT.py file only runs on cc and h files and explicitly blacklists any file in the cocoa/ directory or any file that has "_mac.(cc|h)" or "*_mac_*" in its file name. -- Elliot -- Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Linting chrome/ in pre-submit checks
On Tue, Nov 17, 2009 at 11:29 AM, Peter Kasting wrote:
> There are some unittests where we have super-long bits which go over 80
> chars, and there are also some where the linter thinks our indenting is
> strange when we do something like:
> struct Foo { ... };
> Foo foo_cases[] = {
> {"a",
>"b",
>"c"},
> }
This construct is why I make cpplint run at a different verbosity
level for test code.
-- Elliot
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Linting chrome/ in pre-submit checks
On Tue, Nov 17, 2009 at 11:16 AM, Elliot Glaysher (Chromium) <
e...@chromium.org> wrote:
> Currently, it only runs it at (gcl/git cl)
> upload time and only generates warnings. In the future, it should
> error at commit time, but I want to put this through a trial period so
> please pay attention to the warnings and yell and scream at me if
> there are false positives.
There are some unittests where we have super-long bits which go over 80
chars, and there are also some where the linter thinks our indenting is
strange when we do something like:
struct Foo { ... };
Foo foo_cases[] = {
{"a",
"b",
"c"},
}
(It doesn't like the uneven number of spaces when lining up the struct
member initializers).
I don't know whether those will trigger warnings/errors in your tool.
- ';' shouldn't be used in empty loops. Use "{}" instead.
>
This makes me super sad. ";" alone, indented, is so much more obvious.
PK
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
Re: [chromium-dev] Linting chrome/ in pre-submit checks
To be clear, it'll be a warning prompt on commit, not an error. You
can say (y)es to continue.
M-A
On Tue, Nov 17, 2009 at 2:16 PM, Elliot Glaysher (Chromium)
wrote:
> Chromium developers,
>
> I have just submitted a PRESUBMIT.py for chrome/ which will run
> cpplint.py on your change as part of the presubmit process. cpplint is
> currently run at reduced strictness--cpplint run separately may
> generate more errors[1]. Currently, it only runs it at (gcl/git cl)
> upload time and only generates warnings. In the future, it should
> error at commit time, but I want to put this through a trial period so
> please pay attention to the warnings and yell and scream at me if
> there are false positives. If I hear nothing, I'll enable errors at
> commit time sometime next week.
>
> I've also gone through chrome/ code and fixed most style errors.
> Here's a few recurring problems to watch out for:
>
> - There is supposed to be a space between (if|while|for) and the
> opening parenthesis. There ISN'T supposed to be a space between a
> function name and it's arguments.
> - When declaring a class that inherits, the ':' should not just be
> hanging on the previous line.
> - On that note, please remember that "class x : public baseclass" and
> "class x : baseclass" may both compile but have different meanings and
> that you probably want the first.
> - Remember that 'private:', 'public:' and 'protected:' should be
> indented one space.
> - Don't use tabs.
> - Header guards should be of the form "CHROME_DIR_DIR_DIR_FILE_H_".
> Header files require header guards; don't omit them. (Exception: the
> "-message.h" headers which do multiple include trickery.)
> - ';' shouldn't be used in empty loops. Use "{}" instead.
> - If an else has a brace on one side, it should have it on both.
>
> Time permitting, I also hope to have app/ , base/ , and maybe views/
> lint clean with presubmit checks in the future. I also hope to make
> the linter more strict in the future; this is just a starting point.
>
> -- Elliot
>
> [1] For the curious: currently, the presubmit process runs normal
> chrome/ code through "--verbose=4" and unit test code through
> "--verbose=5". In addition, there's a list of tests that we instruct
> cpplint.py to not run due either to common false positives or style
> violations that are really, really common.
>
> --
> Chromium Developers mailing list: chromium-dev@googlegroups.com
> View archives, change email options, or unsubscribe:
> http://groups.google.com/group/chromium-dev
>
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
[chromium-dev] Linting chrome/ in pre-submit checks
Chromium developers,
I have just submitted a PRESUBMIT.py for chrome/ which will run
cpplint.py on your change as part of the presubmit process. cpplint is
currently run at reduced strictness--cpplint run separately may
generate more errors[1]. Currently, it only runs it at (gcl/git cl)
upload time and only generates warnings. In the future, it should
error at commit time, but I want to put this through a trial period so
please pay attention to the warnings and yell and scream at me if
there are false positives. If I hear nothing, I'll enable errors at
commit time sometime next week.
I've also gone through chrome/ code and fixed most style errors.
Here's a few recurring problems to watch out for:
- There is supposed to be a space between (if|while|for) and the
opening parenthesis. There ISN'T supposed to be a space between a
function name and it's arguments.
- When declaring a class that inherits, the ':' should not just be
hanging on the previous line.
- On that note, please remember that "class x : public baseclass" and
"class x : baseclass" may both compile but have different meanings and
that you probably want the first.
- Remember that 'private:', 'public:' and 'protected:' should be
indented one space.
- Don't use tabs.
- Header guards should be of the form "CHROME_DIR_DIR_DIR_FILE_H_".
Header files require header guards; don't omit them. (Exception: the
"-message.h" headers which do multiple include trickery.)
- ';' shouldn't be used in empty loops. Use "{}" instead.
- If an else has a brace on one side, it should have it on both.
Time permitting, I also hope to have app/ , base/ , and maybe views/
lint clean with presubmit checks in the future. I also hope to make
the linter more strict in the future; this is just a starting point.
-- Elliot
[1] For the curious: currently, the presubmit process runs normal
chrome/ code through "--verbose=4" and unit test code through
"--verbose=5". In addition, there's a list of tests that we instruct
cpplint.py to not run due either to common false positives or style
violations that are really, really common.
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
