OS_MACOSX -> OS_MAC?

[Dan Beam]

Anybody know why the #ifdef for Mac is OS_MACOSX instead of. OS_MAC?  OS_MACOSX seems error prone and redundant.  People seem to accidentally type:

  #if defined(OS_MAC)

  #if defined(OS_MACOS)

which requires cleanup CLs to fix this, here are some examples:

  https://src.chromium.org/viewvc/chrome?view=rev&revision=183477
  https://src.chromium.org/viewvc/chrome?view=rev&revision=173253
  http://src.chromium.org/viewvc/chrome?view=rev&revision=127389
  http://src.chromium.org/viewvc/chrome?view=rev&revision=60904
  http://src.chromium.org/viewvc/chrome?view=rev&revision=43370
  http://src.chromium.org/viewvc/chrome?view=rev&revision=36000
  http://src.chromium.org/viewvc/chrome?view=rev&revision=19405
  http://src.chromium.org/viewvc/chrome?view=rev&revision=10438
  http://src.chromium.org/viewvc/chrome?view=rev&revision=432
  (this is not a new problem)

This adds subtly wrong or dead code and wastes a couple bits (not a real issue, but wouldn't hurt to shave ~6k from src/).

Any objections to changing?  Was there a reason this wasn't OS_MAC to begin with?

-- 

Dan Beam
db...@chromium.org

[Alexei Svitkine]

+1 to change to OS_MAC

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
 
 
 

[Dan Beam]

On Tue, Mar 19, 2013 at 12:17 PM, Dan Beam <db...@chromium.org> wrote:

Anybody know why the #ifdef for Mac is OS_MACOSX instead of. OS_MAC?  OS_MACOSX seems error prone and redundant.  People seem to accidentally type:

  #if defined(OS_MAC)

  #if defined(OS_MACOS)

which requires cleanup CLs to fix this, here are some examples:

  https://src.chromium.org/viewvc/chrome?view=rev&revision=183477
  https://src.chromium.org/viewvc/chrome?view=rev&revision=173253
  http://src.chromium.org/viewvc/chrome?view=rev&revision=127389
  http://src.chromium.org/viewvc/chrome?view=rev&revision=60904
  http://src.chromium.org/viewvc/chrome?view=rev&revision=43370
  http://src.chromium.org/viewvc/chrome?view=rev&revision=36000
  http://src.chromium.org/viewvc/chrome?view=rev&revision=19405
  http://src.chromium.org/viewvc/chrome?view=rev&revision=10438
  http://src.chromium.org/viewvc/chrome?view=rev&revision=432
  (this is not a new problem)

Whoops, forgot:

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

https://codereview.chromium.org/12829005/

I guess you get the point, though, ;).

[Mark Mentovai]

There was a collision with something in third_party. And OS_WIN is OS_WIN because there was a collision with OS_WINDOWS.

Why is OS_MAC better than OS_MACOSX? We don’t need more than one macro for this, and it seems like people are going to type the wrong one no matter which one we choose.

--

[Rachel Blum]

The proper answer to fixing misspellings is obviously creative abuse of the preprocessor.

I.e.

#define OS_OSX 1

#define OS_WIN 0

#define HAS_OS(x) (OS_##x / defined(OS_##x))

#if HAS_OS(MACOSX) // will yield div-by-zero

I'm not sure if this is tongue-in-cheek or serious yet. :)

 - rachel

[Dan Beam]

On Tue, Mar 19, 2013 at 12:23 PM, Mark Mentovai <ma...@chromium.org> wrote:

There was a collision with something in third_party. And OS_WIN is OS_WIN because there was a collision with OS_WINDOWS.

Why is OS_MAC better than OS_MACOSX?

It's less redundant (there's no second "OS") and shorter to type (less to screw up, less wrapping, less bits).

 

We don’t need more than one macro for this, and it seems like people are going to type the wrong one no matter which one we choose.

I don't want another macro, I want to change the existing one :).  This idea came from my fiddling around in src/PRESUBMIT.py to catch OS_MAC/OS_MACOS.  I figured I'd double check why it's named this way first.  Also, just anecdotally, I haven't seen as many OS_LINUX or OS_WIN mispelling CLs fly past me.

-- 

Dan Beam
db...@chromium.org

[Nico Weber]

On Tue, Mar 19, 2013 at 12:49 PM, Dan Beam <db...@chromium.org> wrote:

On Tue, Mar 19, 2013 at 12:23 PM, Mark Mentovai <ma...@chromium.org> wrote:

There was a collision with something in third_party. And OS_WIN is OS_WIN because there was a collision with OS_WINDOWS.

Why is OS_MAC better than OS_MACOSX?

It's less redundant (there's no second "OS") and shorter to type (less to screw up, less wrapping, less bits).

 

We don’t need more than one macro for this, and it seems like people are going to type the wrong one no matter which one we choose.

I don't want another macro, I want to change the existing one :).  This idea came from my fiddling around in src/PRESUBMIT.py to catch OS_MAC/OS_MACOS.  I figured I'd double check why it's named this way first.  Also, just anecdotally, I haven't seen as many OS_LINUX or OS_WIN mispelling CLs fly past me.

There are a few:

$ git log --grep OS_WINDOWS | wc -l

     124

[Scott Hess]

On Tue, Mar 19, 2013 at 12:49 PM, Dan Beam <db...@chromium.org> wrote:

Also, just anecdotally, I haven't seen as many OS_LINUX or OS_WIN mispelling CLs fly past me.

Possibly because people are testing that their code actually does what it's supposed to before checking it in?

-scott

 

[Dan Beam]

On Tue, Mar 19, 2013 at 1:21 PM, Nico Weber <tha...@chromium.org> wrote:

On Tue, Mar 19, 2013 at 12:49 PM, Dan Beam <db...@chromium.org> wrote:

On Tue, Mar 19, 2013 at 12:23 PM, Mark Mentovai <ma...@chromium.org> wrote:

There was a collision with something in third_party. And OS_WIN is OS_WIN because there was a collision with OS_WINDOWS.

Why is OS_MAC better than OS_MACOSX?

It's less redundant (there's no second "OS") and shorter to type (less to screw up, less wrapping, less bits).

 

We don’t need more than one macro for this, and it seems like people are going to type the wrong one no matter which one we choose.

I don't want another macro, I want to change the existing one :).  This idea came from my fiddling around in src/PRESUBMIT.py to catch OS_MAC/OS_MACOS.  I figured I'd double check why it's named this way first.  Also, just anecdotally, I haven't seen as many OS_LINUX or OS_WIN mispelling CLs fly past me.

There are a few:

$ git log --grep OS_WINDOWS | wc -l

     124

You know that counts every line of a commit message, etc., right?

I see 7 actual commits:

http://crrev.com/163418

http://crrev.com/93643

http://crrev.com/60580

http://crrev.com/48454

http://crrev.com/25412

http://crrev.com/26295

http://crrev.com/11847

We should make a PRESUBMIT.py check for OS_WINDOWS as well.

Dan Beam

db...@chromium.org

[Scott Hess]

git gs OS_WINDOWS | egrep -v third_party

media/filters/gpu_video_decoder.cc:140:#if defined(OS_WINDOWS)

ppapi/ppapi_gl.gypi:31:            '_EGL_OS_WINDOWS',

[Andrew Scherkus]

On Tue, Mar 19, 2013 at 2:33 PM, Scott Hess <sh...@chromium.org> wrote:

git gs OS_WINDOWS | egrep -v third_party

media/filters/gpu_video_decoder.cc:140:#if defined(OS_WINDOWS)

Hah! CL out: https://codereview.chromium.org/12879010/

[Scott Graham]

How about something like this? https://codereview.chromium.org/12944002/

(I don't have any opinion on MAC vs MACOSX, but this could also let us
use whichever we prefer, independent of third_party.)

On Tue, Mar 19, 2013 at 2:33 PM, Scott Hess <sh...@chromium.org> wrote:

[Rachel Blum]

Huh. I guess I was serious with my proposal after all :)

+1 on Scott's suggestion.

[Dan Beam]

I assume that shess@ is saying renaming is just as error prone.

FYI: These are the current Mac problems as well :(,

  $ git gs 'OS_MAC\b' | grep -v third_party | grep -v 'testing/' | grep -v '#endif'

  base/path_service_unittest.cc:65:#if defined(OS_MAC)

  chrome/browser/autofill/autofill_browsertest.cc:749:#if defined(OS_MAC)

  chrome/browser/download/download_path_reservation_tracker_unittest.cc:458:#if defined(OS_WIN) || defined(OS_MAC) || defined(OS_CHROMEOS)

  $ git gs 'OS_MACOS\b' | grep -v third_party

  chrome/browser/ui/browser_command_controller_unittest.cc:258:#if defined(OS_MACOS)

  chrome/browser/ui/browser_command_controller_unittest.cc:298:#endif  // defined(OS_MACOS)

It seems obvious to me that this is a problem.  If renaming isn't bulletproof, does adding a check for the usual suspects (OS_MAC, OS_MACOS, OS_WINDOWS) in src/PRESUBMIT.py and/or changing base/build_defs.h (the way scottmg@/groby@ have proposed) sound like a better answer to everyone?

--

Dan Beam

db...@chromium.org

On Tue, Mar 19, 2013 at 2:33 PM, Scott Hess <sh...@chromium.org> wrote:

[Rachel Blum]

BTW: First three fixed at https://codereview.chromium.org/12828008

[Mark Mentovai]

Dan Beam wrote:

I assume that shess@ is saying renaming is just as error prone.

FYI: These are the current Mac problems as well :(,

  $ git gs 'OS_MAC\b' | grep -v third_party | grep -v 'testing/' | grep -v '#endif'

  base/path_service_unittest.cc:65:#if defined(OS_MAC)

  chrome/browser/autofill/autofill_browsertest.cc:749:#if defined(OS_MAC)

  chrome/browser/download/download_path_reservation_tracker_unittest.cc:458:#if defined(OS_WIN) || defined(OS_MAC) || defined(OS_CHROMEOS)

  $ git gs 'OS_MACOS\b' | grep -v third_party

  chrome/browser/ui/browser_command_controller_unittest.cc:258:#if defined(OS_MACOS)

  chrome/browser/ui/browser_command_controller_unittest.cc:298:#endif  // defined(OS_MACOS)

It seems obvious to me that this is a problem.  If renaming isn't bulletproof, does adding a check for the usual suspects (OS_MAC, OS_MACOS, OS_WINDOWS) in src/PRESUBMIT.py and/or changing base/build_defs.h (the way scottmg@/groby@ have proposed) sound like a better answer to everyone?

Either of these are fine.

I responded privately with a weak reason to stick with the OS_* names instead of an OS(…) funcro, but it’s admittedly a weak reason at this point.

[James Robinson]

WebKit uses OS(...) funcros which expand out to longer macros that don't have the same collision risk.  Could you share your reason to not use that pattern in chromium?  I'm not sure it's better or worse but I'd like to know more about the tradeoffs.

- James

[Rachel Blum]

Ah. I withdraw my CL :)

On Tue, Mar 19, 2013 at 3:12 PM, Dale Curtis <dalec...@google.com> wrote:

Already landed https://codereview.chromium.org/12829005/ (presumably what started this discussion :)

- dale

[Scott Hess]

Rewriting all the macros is attractive, but I can see the downside to it, both in terms of churn and in terms of training.  So I'll let others argue that.

Last time I did a pass of finding these things, I think I got pretty good results from just searching for defined\(OS_([^\)]+)\) or something of the sort, which implies that a presubmit check would go a long way with little effort.  In fact:

git gs defined\(OS_ | egrep -v third_party | perl -pe 'while (<>) { while (/defined\((OS_[^)]+)\)/g) { print "$1\n";}}' | sort | uniq -c

1039 OS_ANDROID

  16 OS_BSD

   1 OS_CAT

   1 OS_CHROME

1559 OS_CHROMEOS

  28 OS_FREEBSD

 438 OS_IOS

 619 OS_LINUX

   5 OS_MAC

   2 OS_MACOS

2001 OS_MACOSX

   1 OS_MAXOSX

 183 OS_NACL

 113 OS_OPENBSD

 923 OS_POSIX

  16 OS_SOLARIS

   2 OS_SUN

3394 OS_WIN

   2 OS_WINDOW

   1 OS_WINDOWS

OSX to the Max!

-scott

[Gregg Tavares]

On Tue, Mar 19, 2013 at 3:12 PM, James Robinson <jam...@google.com> wrote:

On Tue, Mar 19, 2013 at 3:09 PM, Mark Mentovai <ma...@chromium.org> wrote:

Dan Beam wrote:

I assume that shess@ is saying renaming is just as error prone.

FYI: These are the current Mac problems as well :(,

  $ git gs 'OS_MAC\b' | grep -v third_party | grep -v 'testing/' | grep -v '#endif'

  base/path_service_unittest.cc:65:#if defined(OS_MAC)

  chrome/browser/autofill/autofill_browsertest.cc:749:#if defined(OS_MAC)

  chrome/browser/download/download_path_reservation_tracker_unittest.cc:458:#if defined(OS_WIN) || defined(OS_MAC) || defined(OS_CHROMEOS)

  $ git gs 'OS_MACOS\b' | grep -v third_party

  chrome/browser/ui/browser_command_controller_unittest.cc:258:#if defined(OS_MACOS)

  chrome/browser/ui/browser_command_controller_unittest.cc:298:#endif  // defined(OS_MACOS)

It seems obvious to me that this is a problem.  If renaming isn't bulletproof, does adding a check for the usual suspects (OS_MAC, OS_MACOS, OS_WINDOWS) in src/PRESUBMIT.py and/or changing base/build_defs.h (the way scottmg@/groby@ have proposed) sound like a better answer to everyone?

Either of these are fine.

I responded privately with a weak reason to stick with the OS_* names instead of an OS(…) funcro, but it’s admittedly a weak reason at this point.

WebKit uses OS(...) funcros which expand out to longer macros that don't have the same collision risk.  Could you share your reason to not use that pattern in chromium?  I'm not sure it's better or worse but I'd like to know more about the tradeoffs.

I don't really care what the decision is but my personal experience is to always pick names that are not common otherwise you'll run into conflicts with 3rd party code. So 

   WIN, OS_WIN, MAC, OS_MAC, OSX, OS_OSX, LINUX, DEBUG,  OS()  etc... // bad

   CHROMIUM_OS_WIN, CHROMIUM_DEBUG, CHROMIUM_OS() etc...  // good

But that's just my experience. Clearly Chromium has been going with names likely to clash with other projects for a while and either not having any clashes or dealing with them as they come up.

[Dirk Pranke]

I'm not suggesting we rewrite all of the macros, but hopefully we can agree that having four different variants of Mac define is bad and should be fixed, right? I don't think "fear of churn" should outweigh consistency in this particular case, assuming we pick the macros with the vast majority of uses.

-- Dirk

[Scott Hess]

The three extra versions of the Mac macros might as well be random line noise, because they don't currently have meaning!

I didn't mean "We shouldn't fix an identified problem".  I meant that changing 10 to 12 thousand lines of code from one preprocessor style to another seems like a bigger job than fixing a few dozen currently-broken sites and adding a presubmit so that errors have to try harder to get landed.

-scott

[Dan Beam]

On Tue, Mar 19, 2013 at 5:07 PM, Scott Hess <sh...@google.com> wrote:

The three extra versions of the Mac macros might as well be random line noise, because they don't currently have meaning!

I didn't mean "We shouldn't fix an identified problem".  I meant that changing 10 to 12 thousand lines of code from one preprocessor style to another seems like a bigger job than fixing a few dozen currently-broken sites and adding a presubmit so that errors have to try harder to get landed.

This could be a reasonable compromise, I've just received push back in the past that presubmits shouldn't essentially "compile the code", which this would get closeer to doing.  If this seems like the best trade-off, it's fine by me.

--

Dan Beam

db...@chromium.org

 

-scott

[Jói Sigurðsson]

A presubmit seems like a good solution here. It could just parse line
by line to look for patterns like #if.*\bOS_[A-Z0-9_]+\b and give you
a warning (and maybe mention the OS_XYZ defines that are common) if
you're using one that isn't on some reasonably short list of most
common defines.

Cheers,
Jói

[Scott Hess]

Maybe I'll go learn enough Python to do it ... in case anyone else jumps first, though, based on past experience I think matching /defined\((OS_[^\)]+)\)/ and then checking $1 against the approved items would be pretty close.  third_party is a mess on this front, so perhaps ignore that.  There are enough cases where preprocessor directives span lines to be annoying, and AFAICT there aren't false positives if you ignore it.  I also in the past have avoided comment-only changes, but since it makes the check more annoying, I'm all for fixing comments to be consistent, if they say defined(OS_MAC) or whatever.  If they say OS_MAC, well, whatever, there are limits.

In the above, I assume this is running against something like git-diff output, and only checking the changed lines.  So maybe even checking third_party would be fine, someone can always override the presubmit check.

-scott

[Darin Fisher]

I support such a change.  WebKit does something like this, and I think it works quite well.  I wish we had established a similar approach from the start.

It would probably be a tedious change to implement at this point.

-Darin

[Dan Beam]

A patch to src/PRESUBMIT.py is up for review to catch some of these here:

https://chromiumcodereview.appspot.com/12845013/

--

Dan Beam

db...@chromium.org