Clang style plugin: add warn-only option and use it on Windows

Clang style plugin: add warn-only option and use it on Windows

This will allow us to flip a flag to enable warnings-as-errors once
the Windows code is cleaned up enough without building a new plugin
binary.

Also, try not to compute the diagnostic level in multiple places.

BUG=467287, 483065
Committed: https://crrev.com/a7ae971275f2aa0a7a6501c72b571939c44baa2b
Cr-Commit-Position: refs/heads/master@{#327852}

[hans, 2015-04-30 22:59:29]

hans@chromium.org changed reviewers:
+ dcheng@chromium.org, thakis@chromium.org

[hans, 2015-04-30 22:59:30]

Please take a look.

[Nico, 2015-04-30 23:08:04]

(…i guess that means not having a test, but since this is transitional i suppose
that's ok)

https://codereview.chromium.org/1117163002/diff/1/tools/clang/plugins/FindBad...
File tools/clang/plugins/FindBadConstructsAction.cpp (right):

https://codereview.chromium.org/1117163002/diff/1/tools/clang/plugins/FindBad...
tools/clang/plugins/FindBadConstructsAction.cpp:61: options_.warn_only = true;
I'd make this block windows-only

[Nico, 2015-04-30 23:08:20]

other than that lgtm, and if you think that suggestion is silly lgtm as-is too

[hans, 2015-04-30 23:13:32]

https://codereview.chromium.org/1117163002/diff/1/tools/clang/plugins/FindBad...
File tools/clang/plugins/FindBadConstructsAction.cpp (right):

https://codereview.chromium.org/1117163002/diff/1/tools/clang/plugins/FindBad...
tools/clang/plugins/FindBadConstructsAction.cpp:61: options_.warn_only = true;
On 2015/04/30 23:08:04, Nico wrote:
> I'd make this block windows-only
> 
> (…i guess that means not having a test, but since this is transitional i
suppose
that's ok)

I like that test. It found the bug when I hadn't updated both functions where we
computed the error level.

I want to keep the test so I'd rather not make the flag windows-only.

[dcheng, 2015-04-30 23:19:21]

On 2015/04/30 at 23:13:32, hans wrote:
>
https://codereview.chromium.org/1117163002/diff/1/tools/clang/plugins/FindBad...
> File tools/clang/plugins/FindBadConstructsAction.cpp (right):
> 
>
https://codereview.chromium.org/1117163002/diff/1/tools/clang/plugins/FindBad...
> tools/clang/plugins/FindBadConstructsAction.cpp:61: options_.warn_only = true;
> On 2015/04/30 23:08:04, Nico wrote:
> > I'd make this block windows-only
> > 
> > (…i guess that means not having a test, but since this is transitional i
suppose
> that's ok)
> 
> I like that test. It found the bug when I hadn't updated both functions where
we computed the error level.
> 
> I want to keep the test so I'd rather not make the flag windows-only.

Always having this flag is also nice for automated fixits: for various reasons,
the fixit apply clang flags don't work well, so I have a homebrew solution for
applying them. My previous workflow involved running ninja -C out/Debug and
applying the fixits in a loop, along with -ferror-limit=0. Now, it's just one
(or two passes) over the codebase.

[hans, 2015-04-30 23:20:39]

The CQ bit was checked by hans@chromium.org

[commit-bot: I haz the power, 2015-04-30 23:21:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1117163002/1

[Nico, 2015-04-30 23:31:13]

On 2015/04/30 23:19:21, dcheng wrote:
> On 2015/04/30 at 23:13:32, hans wrote:
> >
>
https://codereview.chromium.org/1117163002/diff/1/tools/clang/plugins/FindBad...
> > File tools/clang/plugins/FindBadConstructsAction.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1117163002/diff/1/tools/clang/plugins/FindBad...
> > tools/clang/plugins/FindBadConstructsAction.cpp:61: options_.warn_only =
true;
> > On 2015/04/30 23:08:04, Nico wrote:
> > > I'd make this block windows-only
> > > 
> > > (…i guess that means not having a test, but since this is transitional i
> suppose
> > that's ok)
> > 
> > I like that test. It found the bug when I hadn't updated both functions
where
> we computed the error level.
> > 
> > I want to keep the test so I'd rather not make the flag windows-only.
> 
> Always having this flag is also nice for automated fixits: for various
reasons,
> the fixit apply clang flags don't work well, so I have a homebrew solution for
> applying them. My previous workflow involved running ninja -C out/Debug and
> applying the fixits in a loop, along with -ferror-limit=0. Now, it's just one
> (or two passes) over the codebase.

Not sure I get that. You need to regyp to toggle this flag, and if you're
regypping anyhow, can't you just as well remove -Werror (?)

[dcheng, 2015-04-30 23:41:25]

Well, I used to just edit plugin_flags.sh =)

I guess now that I have to edit common.gypi anyway it doesn't make as much
sense

On Thu, Apr 30, 2015, 16:31  <thakis@chromium.org> wrote:

> On 2015/04/30 23:19:21, dcheng wrote:
> > On 2015/04/30 at 23:13:32, hans wrote:
> > >
>
>
>
https://codereview.chromium.org/1117163002/diff/1/tools/clang/plugins/FindBad...
> > > File tools/clang/plugins/FindBadConstructsAction.cpp (right):
> > >
> > >
>
>
>
https://codereview.chromium.org/1117163002/diff/1/tools/clang/plugins/FindBad...
> > > tools/clang/plugins/FindBadConstructsAction.cpp:61: options_.warn_only
> =
> true;
> > > On 2015/04/30 23:08:04, Nico wrote:
> > > > I'd make this block windows-only
> > > >
> > > > (…i guess that means not having a test, but since this is
> > transitional i
> > suppose
> > > that's ok)
> > >
> > > I like that test. It found the bug when I hadn't updated both functions
> where
> > we computed the error level.
> > >
> > > I want to keep the test so I'd rather not make the flag windows-only.
>
> > Always having this flag is also nice for automated fixits: for various
> reasons,
> > the fixit apply clang flags don't work well, so I have a homebrew
> > solution for
> > applying them. My previous workflow involved running ninja -C out/Debug
> > and
> > applying the fixits in a loop, along with -ferror-limit=0. Now, it's just
> > one
> > (or two passes) over the codebase.
>
> Not sure I get that. You need to regyp to toggle this flag, and if you're
> regypping anyhow, can't you just as well remove -Werror (?)
>
> https://codereview.chromium.org/1117163002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2015-05-01 01:02:21]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-05-01 01:03:25]

Patchset 1 (id:??) landed as
https://crrev.com/a7ae971275f2aa0a7a6501c72b571939c44baa2b
Cr-Commit-Position: refs/heads/master@{#327852}

[Nico, 2015-11-29 15:28:17]

should we remove this again, now that we no longer need it?

[hans, 2015-11-30 19:41:52]

I started removing this, but then I thought maybe this flag useful when rolling
out new plugin warnings, for building the whole tree and collecting all
warnings?

Then again, I suppose we haven't used it for that before. What do you think?

[Nico, 2015-11-30 19:42:48]

Couldn't you just disable -Werror for this?

On Mon, Nov 30, 2015 at 2:41 PM, <hans@chromium.org> wrote:

> I started removing this, but then I thought maybe this flag useful when
> rolling
> out new plugin warnings, for building the whole tree and collecting all
> warnings?
>
> Then again, I suppose we haven't used it for that before. What do you
> think?
>
> https://codereview.chromium.org/1117163002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hans, 2015-11-30 19:45:09]

Oh right, yes that works. I'll get a patch up.

On Mon, Nov 30, 2015 at 11:42 AM, Nico Weber <thakis@chromium.org> wrote:
> Couldn't you just disable -Werror for this?
>
> On Mon, Nov 30, 2015 at 2:41 PM, <hans@chromium.org> wrote:
>>
>> I started removing this, but then I thought maybe this flag useful when
>> rolling
>> out new plugin warnings, for building the whole tree and collecting all
>> warnings?
>>
>> Then again, I suppose we haven't used it for that before. What do you
>> think?
>>
>> https://codereview.chromium.org/1117163002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hans, 2015-11-30 23:03:34]

On 2015/11/30 19:45:09, hans wrote:
> Oh right, yes that works. I'll get a patch up.

https://codereview.chromium.org/1486813002/