Part 2 of repairing regressions to my old clang check plugins so Nico can

Part 2 of repairing regressions to my old clang check plugins so Nico can
deploy the clang plugins to the waterfall/trybots.

BUG=none
TEST=compiles

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=73014

[Elliot Glaysher, 2011-01-27 22:58:19]

More!

[Elliot Glaysher, 2011-01-28 19:38:16]

On 2011/01/27 22:58:19, Elliot Glaysher wrote:
> More!

Ping!

[Nico, 2011-01-28 20:04:51]

LG

sorry for the delay :-/

How much work is it to make your plugin happy for all targets? Since chrome
builds, huge chunks seem to be clean already. The gmock stuff has shown that
out-of-lining test code is important too. Also, "your code must meet these
guidelines" is easier to understand than "your code must meet these guidelines
except if it's in this or that target".

[Elliot Glaysher, 2011-01-28 20:27:51]

It may take a week or more to clean up the code. Just based off what I
see in the net_unittests, there's a lot of deinling to be done; see
http://codereview.chromium.org/6264013/ for an example in net/.

On Fri, Jan 28, 2011 at 12:04 PM,  <thakis@chromium.org> wrote:
> LG
>
> sorry for the delay :-/
>
> How much work is it to make your plugin happy for all targets? Since chrome
> builds, huge chunks seem to be clean already. The gmock stuff has shown that
> out-of-lining test code is important too. Also, "your code must meet these
> guidelines" is easier to understand than "your code must meet these
> guidelines
> except if it's in this or that target".
>
> http://codereview.chromium.org/6272025/
>

[Nico, 2011-01-28 20:29:23]

On Fri, Jan 28, 2011 at 12:27 PM, Elliot Glaysher (Chromium)
<erg@chromium.org> wrote:
> It may take a week or more to clean up the code. Just based off what I
> see in the net_unittests, there's a lot of deinling to be done; see
> http://codereview.chromium.org/6264013/ for an example in net/.

Can you temporarily check for .*_unittest.cc in the plugin and skip
matching files until they're cleaned up?

>
> On Fri, Jan 28, 2011 at 12:04 PM,  <thakis@chromium.org> wrote:
>> LG
>>
>> sorry for the delay :-/
>>
>> How much work is it to make your plugin happy for all targets? Since chrome
>> builds, huge chunks seem to be clean already. The gmock stuff has shown that
>> out-of-lining test code is important too. Also, "your code must meet these
>> guidelines" is easier to understand than "your code must meet these
>> guidelines
>> except if it's in this or that target".
>>
>> http://codereview.chromium.org/6272025/
>>
>

[Elliot Glaysher, 2011-01-28 20:48:30]

On Fri, Jan 28, 2011 at 12:29 PM, Nico Weber <thakis@chromium.org> wrote:
> Can you temporarily check for .*_unittest.cc in the plugin and skip
> matching files until they're cleaned up?

I don't think so, because what clang passes to me is the location of
the definition I'm considering right now, and the majority of the code
that needs to be deinlined aren't in _*test.{h,cc} files.

Now I could just add a hack that looks at *test* in names which will
reduce the amount of work I need to do, but that'll still require a
bit of work before we could turn this on for everything.

-- Elliot

[tim (not reviewing), 2011-02-02 00:53:20]

On 2011/01/28 20:48:30, Elliot Glaysher wrote:
> On Fri, Jan 28, 2011 at 12:29 PM, Nico Weber <mailto:thakis@chromium.org>
wrote:
> > Can you temporarily check for .*_unittest.cc in the plugin and skip
> > matching files until they're cleaned up?
> 
> I don't think so, because what clang passes to me is the location of
> the definition I'm considering right now, and the majority of the code
> that needs to be deinlined aren't in _*test.{h,cc} files.
> 
> Now I could just add a hack that looks at *test* in names which will
> reduce the amount of work I need to do, but that'll still require a
> bit of work before we could turn this on for everything.
> 
> -- Elliot

Just curious - how did the inner-class in syncer_thread2.h mess things up?

[Elliot Glaysher, 2011-02-02 01:02:31]

The plugin warns on any struct/class type that has a templated member
that is in itself not a POD (or having 3 or more non-POD class types;
or having >= 10 integral types that are constructed). So linked_ptr<>
set it off since would have to instantiate
linked_ptr<sessions::SyncSession> (vs types for
linked_ptr<AnythingElse>).

In this case, it's easy to fix; the enum could be pulled out of the
inner class and the definition could be put in the implementation
(which is a good idea anyway). I would have had to instead add an
explicit ctor/dtor if a friend was reaching in and using this class in
a file other than syncer_thread2.h (I've seen that pattern happen a
few times now).

On Tue, Feb 1, 2011 at 4:53 PM,  <tim@chromium.org> wrote:
> On 2011/01/28 20:48:30, Elliot Glaysher wrote:
>>
>> On Fri, Jan 28, 2011 at 12:29 PM, Nico Weber <mailto:thakis@chromium.org>
>
> wrote:
>>
>> > Can you temporarily check for .*_unittest.cc in the plugin and skip
>> > matching files until they're cleaned up?
>
>> I don't think so, because what clang passes to me is the location of
>> the definition I'm considering right now, and the majority of the code
>> that needs to be deinlined aren't in _*test.{h,cc} files.
>
>> Now I could just add a hack that looks at *test* in names which will
>> reduce the amount of work I need to do, but that'll still require a
>> bit of work before we could turn this on for everything.
>
>> -- Elliot
>
> Just curious - how did the inner-class in syncer_thread2.h mess things up?
>
> http://codereview.chromium.org/6272025/
>

[tim (not reviewing), 2011-02-02 01:37:46]

On 2011/02/02 01:02:31, Elliot Glaysher wrote:
> The plugin warns on any struct/class type that has a templated member
> that is in itself not a POD (or having 3 or more non-POD class types;
> or having >= 10 integral types that are constructed). So linked_ptr<>
> set it off since would have to instantiate
> linked_ptr<sessions::SyncSession> (vs types for
> linked_ptr<AnythingElse>).
> 
> In this case, it's easy to fix; the enum could be pulled out of the
> inner class and the definition could be put in the implementation
> (which is a good idea anyway). I would have had to instead add an
> explicit ctor/dtor if a friend was reaching in and using this class in
> a file other than syncer_thread2.h (I've seen that pattern happen a
> few times now).
> 
> On Tue, Feb 1, 2011 at 4:53 PM,  <mailto:tim@chromium.org> wrote:
> > On 2011/01/28 20:48:30, Elliot Glaysher wrote:
> >>
> >> On Fri, Jan 28, 2011 at 12:29 PM, Nico Weber <mailto:thakis@chromium.org>
> >
> > wrote:
> >>
> >> > Can you temporarily check for .*_unittest.cc in the plugin and skip
> >> > matching files until they're cleaned up?
> >
> >> I don't think so, because what clang passes to me is the location of
> >> the definition I'm considering right now, and the majority of the code
> >> that needs to be deinlined aren't in _*test.{h,cc} files.
> >
> >> Now I could just add a hack that looks at *test* in names which will
> >> reduce the amount of work I need to do, but that'll still require a
> >> bit of work before we could turn this on for everything.
> >
> >> -- Elliot
> >
> > Just curious - how did the inner-class in syncer_thread2.h mess things up?
> >
> > http://codereview.chromium.org/6272025/
> >

A ha. I think I get it :)  Thanks for the explanation!