Add PRESUBMIT check for #ifdefs on values that are always defined.

Add PRESUBMIT check for #ifdefs on values that are always defined.

Several values are always defined and the developer actually wants to
do an #if check, not #ifdef.

BUG=417804
Committed: https://crrev.com/35bab3936e07c5bb45a6d6e7000fe4bbca1959fa
Cr-Commit-Position: refs/heads/master@{#297627}

[lliabraa, 2014-09-25 20:06:13]

[email protected] changed reviewers:
+ [email protected]

[lliabraa, 2014-09-25 20:06:13]

@stuartmorgan - can you think of any other macro like this? the simulator one is
the only one I saw in our code.

[lliabraa, 2014-09-25 20:07:11]

fyi - added bug number to CL description

[stuartmorgan, 2014-09-25 23:42:32]

https://codereview.chromium.org/595213004/diff/60001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/595213004/diff/60001/PRESUBMIT.py#newcode1401
PRESUBMIT.py:1401: "TARGET_IPHONE_SIMULATOR",
From TargetConditionals.h, I'd suggest we also add TARGET_OS_MAC,
TARGET_OS_IPHONE, and TARGET_CPU_*. At least the OS one has bitten me before.

https://codereview.chromium.org/595213004/diff/60001/PRESUBMIT.py#newcode1404
PRESUBMIT.py:1404: ifdef_macro =
input_api.re.compile(r'(ifdef\s|defined\()([^\s\)]+)')
Why separate regexes, vs ^\s*#.*(ifdef|defined\() ... ? Is it to catch multiple
violations on the same line in one pass instead of two? (I'm not sure it's worth
it since this won't work with sufficiently long ifs anyway, since after one or
two you run out of room and continue with \)

Also, if you aren't using match group one you can replace () with (?:) to make
it a non-capturing group.

https://codereview.chromium.org/595213004/diff/60001/PRESUBMIT.py#newcode1431
PRESUBMIT.py:1431: 'or add your macro to src/PRESUBMIT.py.', bad_macros)]
Where would they add it to fix the error?

[lliabraa, 2014-09-26 18:22:43]

Thanks for the review. PTAL!

https://codereview.chromium.org/595213004/diff/60001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/595213004/diff/60001/PRESUBMIT.py#newcode1401
PRESUBMIT.py:1401: "TARGET_IPHONE_SIMULATOR",
On 2014/09/25 23:42:32, stuartmorgan wrote:
> From TargetConditionals.h, I'd suggest we also add TARGET_OS_MAC,
> TARGET_OS_IPHONE, and TARGET_CPU_*. At least the OS one has bitten me before.

Done. I added all the CPU and OS defines

https://codereview.chromium.org/595213004/diff/60001/PRESUBMIT.py#newcode1404
PRESUBMIT.py:1404: ifdef_macro =
input_api.re.compile(r'(ifdef\s|defined\()([^\s\)]+)')
On 2014/09/25 23:42:32, stuartmorgan wrote:
> Why separate regexes, vs ^\s*#.*(ifdef|defined\() ... ? Is it to catch
multiple
> violations on the same line in one pass instead of two? (I'm not sure it's
worth
> it since this won't work with sufficiently long ifs anyway, since after one or
> two you run out of room and continue with \)
> 

I copied that bit from _CheckForInvalidOSMacrosInFile, but your single regex
works just as well. Thx.

> Also, if you aren't using match group one you can replace () with (?:) to make
> it a non-capturing group.

Done.

https://codereview.chromium.org/595213004/diff/60001/PRESUBMIT.py#newcode1431
PRESUBMIT.py:1431: 'or add your macro to src/PRESUBMIT.py.', bad_macros)]
On 2014/09/25 23:42:32, stuartmorgan wrote:
> Where would they add it to fix the error?

Done. I've changed this text a bit. If the ifdef is intentional then they
probably need to remove the macro from the of always defined macros.

[stuartmorgan, 2014-09-29 18:51:52]

lgtm

https://codereview.chromium.org/595213004/diff/80001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/595213004/diff/80001/PRESUBMIT.py#newcode1419
PRESUBMIT.py:1419: for match in ifdef_macro.finditer(line):
Fix indentation

https://codereview.chromium.org/595213004/diff/80001/PRESUBMIT.py#newcode1426
PRESUBMIT.py:1426: did_you_mean))
Fix indentation on this block.

[lliabraa, 2014-09-30 11:23:13]

[email protected] changed reviewers:
+ [email protected]

[lliabraa, 2014-09-30 11:23:14]

Thanks Stuart.

+phajdan.jr@ for OWNERS

https://codereview.chromium.org/595213004/diff/80001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/595213004/diff/80001/PRESUBMIT.py#newcode1419
PRESUBMIT.py:1419: for match in ifdef_macro.finditer(line):
On 2014/09/29 18:51:51, stuartmorgan wrote:
> Fix indentation

Done.

https://codereview.chromium.org/595213004/diff/80001/PRESUBMIT.py#newcode1426
PRESUBMIT.py:1426: did_you_mean))
On 2014/09/29 18:51:51, stuartmorgan wrote:
> Fix indentation on this block.

Done.

[Paweł Hajdan Jr., 2014-10-01 11:38:25]

LGTM

[lliabraa, 2014-10-01 11:41:13]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2014-10-01 11:41:55]

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

[commit-bot: I haz the power, 2014-10-01 12:16:52]

Committed patchset #6 (id:100001) as 5e4b32e8771506a1a27b6e30b404edcf473efc89

[commit-bot: I haz the power, 2014-10-01 12:17:31]

Patchset 6 (id:??) landed as
https://crrev.com/35bab3936e07c5bb45a6d6e7000fe4bbca1959fa
Cr-Commit-Position: refs/heads/master@{#297627}