PRESUBMIT: Bark about DISALLOW_* macros without #include "base/macros.h"

PRESUBMIT: Bark about DISALLOW_* macros without #include "base/macros.h"

I'm sick of enforcing this in like every code review.

R=cpu@chromium.org,maruel@chromium.org
TBR=phajdan.jr@chromium.org
BUG=none
TEST=PRESUBMIT_test.py

Committed: https://crrev.com/3aa3839c53008b3ee094e93f26f25d8306c06cc7
Cr-Commit-Position: refs/heads/master@{#354607}

[Dan Beam, 2015-10-16 07:50:21]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-16 07:51:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1411553003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1411553003/1

[commit-bot: I haz the power, 2015-10-16 08:01:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-16 08:01:32]

Dry run: This issue passed the CQ dry run.

[cpu_(ooo_6.6-7.5), 2015-10-16 17:50:27]

looks nice but turns out I don't know python. Please have anybody who does
review and I am happy to give you my lgtm in advance.

[Dan Beam, 2015-10-16 19:30:22]

dbeam@chromium.org changed reviewers:
+ maruel@chromium.org

[Dan Beam, 2015-10-16 19:30:23]

+maruel@ knows python!

[Dan Beam, 2015-10-16 20:48:05]

dbeam@chromium.org changed reviewers:
+ phajdan.jr@chromium.org
- maruel@chromium.org

[Dan Beam, 2015-10-16 20:48:05]

-maruel@, +phajdan.jr@

[M-A Ruel, 2015-10-16 21:09:25]

maruel@chromium.org changed reviewers:
+ maruel@chromium.org

[M-A Ruel, 2015-10-16 21:09:26]

lgtm with nit

https://codereview.chromium.org/1411553003/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/1411553003/diff/1/PRESUBMIT.py#newcode1523
PRESUBMIT.py:1523: disallows = ['DISALLOW_%s' % s for s in ('ASSIGN', 'COPY',
'EVIL')]
I'd call this an early generalisation;

disallows = ('DISALLOW_ASSIGN', 'DISALLOW_COPY', 'DISALLOW_EVIL')
disallows = ['DISALLOW_%s' % s for s in ('ASSIGN', 'COPY', 'EVIL')]

[Dan Beam, 2015-10-16 21:24:15]

https://codereview.chromium.org/1411553003/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/1411553003/diff/1/PRESUBMIT.py#newcode1523
PRESUBMIT.py:1523: disallows = ['DISALLOW_%s' % s for s in ('ASSIGN', 'COPY',
'EVIL')]
On 2015/10/16 21:09:25, M-A Ruel wrote:
> I'd call this an early generalisation;
> 
> disallows = ('DISALLOW_ASSIGN', 'DISALLOW_COPY', 'DISALLOW_EVIL')
> disallows = ['DISALLOW_%s' % s for s in ('ASSIGN', 'COPY', 'EVIL')]

Done. (those 2 characters thank you!)

[Dan Beam, 2015-10-16 21:24:24]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2015-10-16 21:24:25]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
maruel@chromium.org
Link to the patchset: https://codereview.chromium.org/1411553003/#ps20001
(title: "maruel@ review")

[commit-bot: I haz the power, 2015-10-16 21:25:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1411553003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1411553003/20001

[commit-bot: I haz the power, 2015-10-16 21:38:10]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-16 21:39:11]

Patchset 2 (id:??) landed as
https://crrev.com/3aa3839c53008b3ee094e93f26f25d8306c06cc7
Cr-Commit-Position: refs/heads/master@{#354607}

[danakj, 2015-12-04 00:25:52]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-12-04 00:25:53]

:/

I wanted to put DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND in base/move.h but
now that fails this check. Did we really need a special IWYU rule for 3 macros
in 1 header?

Things don't compile if you DISALLOW_COPY_AND_ASSIGN(T) and the macro isn't
defined.

../../cc/layers/layer_unittest.cc:14:3: error: unknown type name
'DISALLOW_COPY_AND_ASSIGN'
  DISALLOW_COPY_AND_ASSIGN(T);

[danakj, 2015-12-04 00:27:59]

Description was changed from

==========
PRESUBMIT: Bark about DISALLOW_* macros without #include "base/macros.h"

I'm sick of enforcing this in like every code review.

R=cpu@chromium.org,maruel@chromium.org
TBR=phajdan.jr@chromium.org
BUG=none
TEST=PRESUBMIT_test.py

Committed: https://crrev.com/3aa3839c53008b3ee094e93f26f25d8306c06cc7
Cr-Commit-Position: refs/heads/master@{#354607}
==========

to

==========
PRESUBMIT: Bark about DISALLOW_* macros without #include "base/macros.h"

I'm sick of enforcing this in like every code review.

R=cpu@chromium.org,maruel@chromium.org
TBR=phajdan.jr@chromium.org
BUG=none
TEST=PRESUBMIT_test.py

Committed: https://crrev.com/3aa3839c53008b3ee094e93f26f25d8306c06cc7
Cr-Commit-Position: refs/heads/master@{#354607}
==========

[Dan Beam, 2015-12-04 00:29:24]

On 2015/12/04 00:25:53, danakj (behind on reviews) wrote:
> :/
> 
> I wanted to put DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND in base/move.h but
> now that fails this check. Did we really need a special IWYU rule for 3 macros
> in 1 header?
> 
> Things don't compile if you DISALLOW_COPY_AND_ASSIGN(T) and the macro isn't
> defined.
> 
> ../../cc/layers/layer_unittest.cc:14:3: error: unknown type name
> 'DISALLOW_COPY_AND_ASSIGN'
>   DISALLOW_COPY_AND_ASSIGN(T);

it'd pretty easy to include the calling paren ('DISALLOW_COPY_AND_ASSIGN(') to
unblock you

[Dan Beam, 2015-12-04 00:39:18]

On 2015/12/04 00:29:24, Dan Beam wrote:
> On 2015/12/04 00:25:53, danakj (behind on reviews) wrote:
> > :/
> > 
> > I wanted to put DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND in base/move.h
but
> > now that fails this check. Did we really need a special IWYU rule for 3
macros
> > in 1 header?
> > 
> > Things don't compile if you DISALLOW_COPY_AND_ASSIGN(T) and the macro isn't
> > defined.

^ and yes, but it's almost always transitively defined

> > 
> > ../../cc/layers/layer_unittest.cc:14:3: error: unknown type name
> > 'DISALLOW_COPY_AND_ASSIGN'
> >   DISALLOW_COPY_AND_ASSIGN(T);
> 
> it'd pretty easy to include the calling paren ('DISALLOW_COPY_AND_ASSIGN(') to
> unblock you

[danakj, 2015-12-04 00:42:41]

On Thu, Dec 3, 2015 at 4:39 PM, <dbeam@chromium.org> wrote:

> On 2015/12/04 00:29:24, Dan Beam wrote:
>
>> On 2015/12/04 00:25:53, danakj (behind on reviews) wrote:
>> > :/
>> >
>> > I wanted to put DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND in
>> base/move.h
>>
> but
>
>> > now that fails this check. Did we really need a special IWYU rule for 3
>>
> macros
>
>> > in 1 header?
>> >
>> > Things don't compile if you DISALLOW_COPY_AND_ASSIGN(T) and the macro
>> isn't
>> > defined.
>>
>
> ^ and yes, but it's almost always transitively defined


I just don't get what's so terrible about that in this case? This happens
all the time for all kinds of things in chrome since we don't have good
IWYU tooling.

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

[Dan Beam, 2015-12-04 00:44:49]

On 2015/12/04 00:42:41, danakj (behind on reviews) wrote:
> On Thu, Dec 3, 2015 at 4:39 PM, <mailto:dbeam@chromium.org> wrote:
> 
> > On 2015/12/04 00:29:24, Dan Beam wrote:
> >
> >> On 2015/12/04 00:25:53, danakj (behind on reviews) wrote:
> >> > :/
> >> >
> >> > I wanted to put DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND in
> >> base/move.h
> >>
> > but
> >
> >> > now that fails this check. Did we really need a special IWYU rule for 3
> >>
> > macros
> >
> >> > in 1 header?
> >> >
> >> > Things don't compile if you DISALLOW_COPY_AND_ASSIGN(T) and the macro
> >> isn't
> >> > defined.
> >>
> >
> > ^ and yes, but it's almost always transitively defined
> 
> 
> I just don't get what's so terrible about that in this case? This happens
> all the time for all kinds of things in chrome since we don't have good
> IWYU tooling.

It just happens in like every header file I ever review, heh

[Dan Beam, 2015-12-04 00:49:42]

On 2015/12/04 00:29:24, Dan Beam wrote:
> On 2015/12/04 00:25:53, danakj (behind on reviews) wrote:
> > :/
> > 
> > I wanted to put DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND in base/move.h
but
> > now that fails this check. Did we really need a special IWYU rule for 3
macros
> > in 1 header?
> > 
> > Things don't compile if you DISALLOW_COPY_AND_ASSIGN(T) and the macro isn't
> > defined.
> > 
> > ../../cc/layers/layer_unittest.cc:14:3: error: unknown type name
> > 'DISALLOW_COPY_AND_ASSIGN'
> >   DISALLOW_COPY_AND_ASSIGN(T);
> 
> it'd pretty easy to include the calling paren ('DISALLOW_COPY_AND_ASSIGN(') to
> unblock you

re: ^ https://codereview.chromium.org/1490383004/

[danakj, 2015-12-04 00:52:29]

On Thu, Dec 3, 2015 at 4:44 PM, <dbeam@chromium.org> wrote:

> On 2015/12/04 00:42:41, danakj (behind on reviews) wrote:
>
>> On Thu, Dec 3, 2015 at 4:39 PM, <mailto:dbeam@chromium.org> wrote:
>>
>
> > On 2015/12/04 00:29:24, Dan Beam wrote:
>> >
>> >> On 2015/12/04 00:25:53, danakj (behind on reviews) wrote:
>> >> > :/
>> >> >
>> >> > I wanted to put DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND in
>> >> base/move.h
>> >>
>> > but
>> >
>> >> > now that fails this check. Did we really need a special IWYU rule
>> for 3
>> >>
>> > macros
>> >
>> >> > in 1 header?
>> >> >
>> >> > Things don't compile if you DISALLOW_COPY_AND_ASSIGN(T) and the macro
>> >> isn't
>> >> > defined.
>> >>
>> >
>> > ^ and yes, but it's almost always transitively defined
>>
>
>
> I just don't get what's so terrible about that in this case? This happens
>> all the time for all kinds of things in chrome since we don't have good
>> IWYU tooling.
>>
>
> It just happens in like every header file I ever review, heh
>

I guess I just think that it probably happens for other things you don't
notice in every file too. I know it happens for me but I don't bother
really. Like sometimes I ask people to add headers but I can't catch them
all and this is probably one I care about the least, cuz it's already
included everywhere so least likely to cause problems where you change
header A and it causes file B to stop compiling.

I really wish for good IWYU, but was pretty surprised to see us rolling out
own IWYU for individual macros like this. Generally I'm a fan of presubmit
checks to make reviewing easier. At first sight, this doesn't hit my bar
for value of the check vs slowing down time to upload CLs.

After this, do we add more checks for LOG macros? And then start building a
database of "Class A is in File B"? Then we add friction for every change
people want to make that refactors code, etc. In this case it's adding
friction if we make changes to base/ causing us to get a PRESUBMIT.py file
owner review that is very unrelated to the change in question. That is why
I am not a fan of this strategy.

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

[Dan Beam, 2015-12-04 00:59:46]

On 2015/12/04 00:52:29, danakj (behind on reviews) wrote:
> On Thu, Dec 3, 2015 at 4:44 PM, <mailto:dbeam@chromium.org> wrote:
> 
> > On 2015/12/04 00:42:41, danakj (behind on reviews) wrote:
> >
> >> On Thu, Dec 3, 2015 at 4:39 PM, <mailto:dbeam@chromium.org> wrote:
> >>
> >
> > > On 2015/12/04 00:29:24, Dan Beam wrote:
> >> >
> >> >> On 2015/12/04 00:25:53, danakj (behind on reviews) wrote:
> >> >> > :/
> >> >> >
> >> >> > I wanted to put DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND in
> >> >> base/move.h
> >> >>
> >> > but
> >> >
> >> >> > now that fails this check. Did we really need a special IWYU rule
> >> for 3
> >> >>
> >> > macros
> >> >
> >> >> > in 1 header?
> >> >> >
> >> >> > Things don't compile if you DISALLOW_COPY_AND_ASSIGN(T) and the macro
> >> >> isn't
> >> >> > defined.
> >> >>
> >> >
> >> > ^ and yes, but it's almost always transitively defined
> >>
> >
> >
> > I just don't get what's so terrible about that in this case? This happens
> >> all the time for all kinds of things in chrome since we don't have good
> >> IWYU tooling.
> >>
> >
> > It just happens in like every header file I ever review, heh
> >
> 
> I guess I just think that it probably happens for other things you don't
> notice in every file too. I know it happens for me but I don't bother
> really. Like sometimes I ask people to add headers but I can't catch them
> all and this is probably one I care about the least, cuz it's already
> included everywhere so least likely to cause problems where you change
> header A and it causes file B to stop compiling.
> 
> I really wish for good IWYU, but was pretty surprised to see us rolling out
> own IWYU for individual macros like this. Generally I'm a fan of presubmit
> checks to make reviewing easier. At first sight, this doesn't hit my bar
> for value of the check vs slowing down time to upload CLs.
> 
> After this, do we add more checks for LOG macros? And then start building a
> database of "Class A is in File B"? Then we add friction for every change
> people want to make that refactors code, etc. In this case it's adding
> friction if we make changes to base/ causing us to get a PRESUBMIT.py file
> owner review that is very unrelated to the change in question. That is why
> I am not a fan of this strategy.

Neither am I.  What are you concrete plans to add better IWYU tools?

I'm sorry you hit an issue when trying to make a new macro that starts with
DISALLOW_COPY_AND_ASSIGN (that I think would be fixed by:
https://codereview.chromium.org/1490383004/).  This presubmit has saved me lots
of typing already and has worked OK for the other ~8400 commits that have landed
since this one, as far as I know (oysteine@ hit an issue and we fixed it).

[danakj, 2015-12-04 01:05:54]

On Thu, Dec 3, 2015 at 4:59 PM, <dbeam@chromium.org> wrote:

> On 2015/12/04 00:52:29, danakj (behind on reviews) wrote:
>
> On Thu, Dec 3, 2015 at 4:44 PM, <mailto:dbeam@chromium.org> wrote:
>>
>
> > On 2015/12/04 00:42:41, danakj (behind on reviews) wrote:
>> >
>> >> On Thu, Dec 3, 2015 at 4:39 PM, <mailto:dbeam@chromium.org> wrote:
>> >>
>> >
>> > > On 2015/12/04 00:29:24, Dan Beam wrote:
>> >> >
>> >> >> On 2015/12/04 00:25:53, danakj (behind on reviews) wrote:
>> >> >> > :/
>> >> >> >
>> >> >> > I wanted to put DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND in
>> >> >> base/move.h
>> >> >>
>> >> > but
>> >> >
>> >> >> > now that fails this check. Did we really need a special IWYU rule
>> >> for 3
>> >> >>
>> >> > macros
>> >> >
>> >> >> > in 1 header?
>> >> >> >
>> >> >> > Things don't compile if you DISALLOW_COPY_AND_ASSIGN(T) and the
>> macro
>> >> >> isn't
>> >> >> > defined.
>> >> >>
>> >> >
>> >> > ^ and yes, but it's almost always transitively defined
>> >>
>> >
>> >
>> > I just don't get what's so terrible about that in this case? This
>> happens
>> >> all the time for all kinds of things in chrome since we don't have good
>> >> IWYU tooling.
>> >>
>> >
>> > It just happens in like every header file I ever review, heh
>> >
>>
>
> I guess I just think that it probably happens for other things you don't
>> notice in every file too. I know it happens for me but I don't bother
>> really. Like sometimes I ask people to add headers but I can't catch them
>> all and this is probably one I care about the least, cuz it's already
>> included everywhere so least likely to cause problems where you change
>> header A and it causes file B to stop compiling.
>>
>
> I really wish for good IWYU, but was pretty surprised to see us rolling out
>> own IWYU for individual macros like this. Generally I'm a fan of presubmit
>> checks to make reviewing easier. At first sight, this doesn't hit my bar
>> for value of the check vs slowing down time to upload CLs.
>>
>
> After this, do we add more checks for LOG macros? And then start building a
>> database of "Class A is in File B"? Then we add friction for every change
>> people want to make that refactors code, etc. In this case it's adding
>> friction if we make changes to base/ causing us to get a PRESUBMIT.py file
>> owner review that is very unrelated to the change in question. That is why
>> I am not a fan of this strategy.
>>
>
> Neither am I.  What are you concrete plans to add better IWYU tools?
>

Right I don't have any plans here.

I'm sorry you hit an issue when trying to make a new macro that starts with
> DISALLOW_COPY_AND_ASSIGN (that I think would be fixed by:
> https://codereview.chromium.org/1490383004/).


Yes I appreciate that you're updating the rule to be more specific.


> This presubmit has saved me lots

of typing already and has worked OK for the other ~8400 commits that have
> landed
> since this one, as far as I know (oysteine@ hit an issue and we fixed it).
>

My problem isn't that this won't work for most CLs, it's that it is
hardcoding locations of C++ code into some python file in src/, which
causes friction for people changing that C++ code. I'd bet you'd be less
than thrilled if it was hardcoding things about C++ that you change, rather
than usually only depend on.

This code changes in-frequently, so sure patching it will work here. But I
don't like this as a concept, and don't look forward to more people adding
similar checks, citing this one as existing, until we have a second copy of
the chromium repo written in some python file so it can check all these
things and suddenly terminators appear and... anyway I don't like the
precident. :)

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