ifdef OS_NAME -> if defined(OS_NAME)

ifdef OS_NAME -> if defined(OS_NAME)
ifndef OS_NAME -> if !defined(OS_NAME)

BUG=none
TEST=no regressions


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190069

[Dan Beam, 2013-03-21 15:30:18]

i smell another potential presubmit

https://codereview.chromium.org/12545059/diff/1/tools/metrics/testdata/foo.cc
File tools/metrics/testdata/foo.cc (left):

https://codereview.chromium.org/12545059/diff/1/tools/metrics/testdata/foo.cc...
tools/metrics/testdata/foo.cc:15: #ifdef OS_WIN
^ intentional, maybe?

https://codereview.chromium.org/12545059/diff/1/tools/metrics/testdata/foo_ig...
File tools/metrics/testdata/foo_ignored.txt (left):

https://codereview.chromium.org/12545059/diff/1/tools/metrics/testdata/foo_ig...
tools/metrics/testdata/foo_ignored.txt:3: #ifdef OS_WIN
^ maybe this was as well?

https://codereview.chromium.org/12545059/diff/1/tools/metrics/testdata/subdir...
File tools/metrics/testdata/subdir/foo_test.mm (left):

https://codereview.chromium.org/12545059/diff/1/tools/metrics/testdata/subdir...
tools/metrics/testdata/subdir/foo_test.mm:7: #ifdef OS_WIN
and this

[groby-ooo-7-16, 2013-03-21 16:05:55]

Summoning Jói, who'll be able to tell you if this was intentional.

[Scott Hess - ex-Googler, 2013-03-21 17:13:43]

lgtm, except possibly the test cases you mentioned.

A minute or so of poking around doesn't turn up anything concrete on why we
prefer #if defined() over #ifdef.  I believe it was because you can more easily
convert to/from compound statements if you use defined(), or something.  Maybe
some chromium-dev research would turn something up.  Anyhow, I don't think this
is just an OS_FOO problem, there are probably a variety of other cases which a
presubmit should fire on.  Though getting OS_FOO right would be a nice start.

https://codereview.chromium.org/12545059/diff/1/chrome/browser/renderer_host/...
File chrome/browser/renderer_host/pepper/pepper_flash_browser_host.cc (right):

https://codereview.chromium.org/12545059/diff/1/chrome/browser/renderer_host/...
chrome/browser/renderer_host/pepper/pepper_flash_browser_host.cc:25: #elif
defined(OS_MACOSX)
I love that this manages to use the correct style right next to broken style. 
And this file was surely created by engineers steeped in the lore of Chromium
style.

[Dan Beam, 2013-03-21 17:29:05]

On 2013/03/21 17:13:43, shess wrote:
> lgtm, except possibly the test cases you mentioned.
> 
> A minute or so of poking around doesn't turn up anything concrete on why we
> prefer #if defined() over #ifdef.  I believe it was because you can more
easily
> convert to/from compound statements if you use defined(), or something.  Maybe
> some chromium-dev research would turn something up.  Anyhow, I don't think
this
> is just an OS_FOO problem, there are probably a variety of other cases which a
> presubmit should fire on.  Though getting OS_FOO right would be a nice start.

If OS_FOO's check searches for defined(OS_FOO), we should discourage people from
using #ifdef/#ifndef via PRESUBMIT.py, IMO (which is why I made this CL to begin
with).

> 
>
https://codereview.chromium.org/12545059/diff/1/chrome/browser/renderer_host/...
> File chrome/browser/renderer_host/pepper/pepper_flash_browser_host.cc (right):
> 
>
https://codereview.chromium.org/12545059/diff/1/chrome/browser/renderer_host/...
> chrome/browser/renderer_host/pepper/pepper_flash_browser_host.cc:25: #elif
> defined(OS_MACOSX)
> I love that this manages to use the correct style right next to broken style. 
> And this file was surely created by engineers steeped in the lore of Chromium
> style.

[Dan Beam, 2013-03-21 17:29:50]

+darin@ for OWNERS, still waiting for Jói

[Jói, 2013-03-21 17:42:56]

As long as ./tools/metrics/count_ifdefs_unittest.py still passes, I'm happy with
the changes.  I don't recall them being intentional.

If the test fails with those changes, you can skip them in your change and I can
take care of updating or adding a comment to say they need to stay.  Just let me
know.

Cheers,
Jói

[Scott Hess - ex-Googler, 2013-03-21 17:55:23]

On 2013/03/21 17:29:05, Dan Beam wrote:
> On 2013/03/21 17:13:43, shess wrote:
> > lgtm, except possibly the test cases you mentioned.
> > 
> > A minute or so of poking around doesn't turn up anything concrete on why we
> > prefer #if defined() over #ifdef.  I believe it was because you can more
> easily
> > convert to/from compound statements if you use defined(), or something. 
Maybe
> > some chromium-dev research would turn something up.  Anyhow, I don't think
> this
> > is just an OS_FOO problem, there are probably a variety of other cases which
a
> > presubmit should fire on.  Though getting OS_FOO right would be a nice
start.
> 
> If OS_FOO's check searches for defined(OS_FOO), we should discourage people
from
> using #ifdef/#ifndef via PRESUBMIT.py, IMO (which is why I made this CL to
begin
> with).

To be clear: I believe we _do_ discourage people from using #ifdef/#ifndef, I
just don't see any formal guidance on this.  I don't think we necessarily need
formal guidance (as in the style guide), but if someone were to turn up a
relevant thread about the topic, it may be that it would clarify whether we
discourage #ifdef/#ifndef at all times, or only for OS_BAR cases.

There was a thread on chromium-dev on 6/19/09 subject "Conventions and patterns
for multi-platform development" which indicates that our style guide discourages
#ifdef/#ifndef in favor of defined().  But even though I'm looking right at the
email, I cannot for the life of me figure out how to convince Google Groups to
turn over the thread.  Here's a non-groups link:
http://www.mail-archive.com/chromium-dev@googlegroups.com/msg05256.html

I can't find the style guidance being referred to, though.

[Dan Beam, 2013-03-21 18:19:27]

On 2013/03/21 17:42:56, Jói wrote:
> As long as ./tools/metrics/count_ifdefs_unittest.py still passes, I'm happy
with
> the changes.  I don't recall them being intentional.

it still passses, AFAICT (running it on linux)

> 
> If the test fails with those changes, you can skip them in your change and I
can
> take care of updating or adding a comment to say they need to stay.  Just let
me
> know.
> 
> Cheers,
> Jói

[Dan Beam, 2013-03-21 18:20:37]

On 2013/03/21 17:55:23, shess wrote:
> On 2013/03/21 17:29:05, Dan Beam wrote:
> > On 2013/03/21 17:13:43, shess wrote:
> > > lgtm, except possibly the test cases you mentioned.
> > > 
> > > A minute or so of poking around doesn't turn up anything concrete on why
we
> > > prefer #if defined() over #ifdef.  I believe it was because you can more
> > easily
> > > convert to/from compound statements if you use defined(), or something. 
> Maybe
> > > some chromium-dev research would turn something up.  Anyhow, I don't think
> > this
> > > is just an OS_FOO problem, there are probably a variety of other cases
which
> a
> > > presubmit should fire on.  Though getting OS_FOO right would be a nice
> start.
> > 
> > If OS_FOO's check searches for defined(OS_FOO), we should discourage people
> from
> > using #ifdef/#ifndef via PRESUBMIT.py, IMO (which is why I made this CL to
> begin
> > with).
> 
> To be clear: I believe we _do_ discourage people from using #ifdef/#ifndef, I
> just don't see any formal guidance on this.  I don't think we necessarily need
> formal guidance (as in the style guide), but if someone were to turn up a
> relevant thread about the topic, it may be that it would clarify whether we
> discourage #ifdef/#ifndef at all times, or only for OS_BAR cases.
> 
> There was a thread on chromium-dev on 6/19/09 subject "Conventions and
patterns
> for multi-platform development" which indicates that our style guide
discourages
> #ifdef/#ifndef in favor of defined().  But even though I'm looking right at
the
> email, I cannot for the life of me figure out how to convince Google Groups to
> turn over the thread.  Here's a non-groups link:
> http://www.mail-archive.com/chromium-dev%40googlegroups.com/msg05256.html
> 
> I can't find the style guidance being referred to, though.

It advocates (via example) #if defined() but doesn't formally advise against
#ifdef, no.

[Scott Hess - ex-Googler, 2013-03-21 18:30:17]

On 2013/03/21 18:20:37, Dan Beam wrote:
> It advocates (via example) #if defined() but doesn't formally advise against
> #ifdef, no.

Worse, the couple cases I turned up basically read like:

Here's how you might do a platform #ifdef:
  #if defined(OS_PLATFORM)
so it's not clear whether the #if defined() is the suggestion or just developer
preference or just random editor fail.

[groby-ooo-7-16, 2013-03-21 19:11:55]

On 2013/03/21 18:30:17, shess wrote:
> On 2013/03/21 18:20:37, Dan Beam wrote:
> > It advocates (via example) #if defined() but doesn't formally advise against
> > #ifdef, no.
> 
> Worse, the couple cases I turned up basically read like:
> 
> Here's how you might do a platform #ifdef:
>   #if defined(OS_PLATFORM)
> so it's not clear whether the #if defined() is the suggestion or just
developer
> preference or just random editor fail.

Then lets fix that message to make it clear that defined() is required. It's
clearly the case that's overwhelmingly used, _and_ it makes the presubmit check
cleaner. It's a win-win.

[Dan Beam, 2013-03-21 19:15:28]

On 2013/03/21 19:11:55, groby wrote:
> On 2013/03/21 18:30:17, shess wrote:
> > On 2013/03/21 18:20:37, Dan Beam wrote:
> > > It advocates (via example) #if defined() but doesn't formally advise
against
> > > #ifdef, no.
> > 
> > Worse, the couple cases I turned up basically read like:
> > 
> > Here's how you might do a platform #ifdef:
> >   #if defined(OS_PLATFORM)
> > so it's not clear whether the #if defined() is the suggestion or just
> developer
> > preference or just random editor fail.
> 
> Then lets fix that message to make it clear that defined() is required. It's
> clearly the case that's overwhelmingly used, _and_ it makes the presubmit
check
> cleaner. It's a win-win.

not to mention the whole reason it's the default is that you can combine
multiple defined()'s to handle complex scenarios

[Dan Beam, 2013-03-22 18:42:06]

+ben@ as well (darin@ seems to be busy right now)

[Dan Beam, 2013-03-22 18:47:02]

ben@/darin@: this is just in need of a OWNERS stamp if you agree with the idea
of #ifdef -> #if defined() as a presubmit I'm writing might use defined() to
check for invalid OS_NAMES <https://chromiumcodereview.appspot.com/12845013/>

[darin (slow to review), 2013-03-23 06:25:54]

LGTM

[commit-bot: I haz the power, 2013-03-23 12:47:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/12545059/1

[commit-bot: I haz the power, 2013-03-23 15:00:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/12545059/1

[commit-bot: I haz the power, 2013-03-23 19:10:57]

Change committed as 190069