Add build_config_functions.h to avoid #ifdef

Add build_config_functions.h to avoid #ifdef

An "#ifdef" statement is more confusing than using C++ syntax and
"if (XXX)". They should be used only when strictly necessary (i.e. when
code cannot compile).

For the cases where #ifdef are not strictly necessary, these new helpers can
be used.

Thanks to compiler optimization, the final compiled code will be the
same when these helpers are used instead of #ifdef.

R=rsesek@chromium.org

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

[jln (very slow on Chromium), 2013-12-04 00:55:21]

Robert: please take a look.

I've been pondering having a common prefix for all of these. (IsBuildXXX ?)

I couldn't decide, so let's discuss this here.

[Robert Sesek, 2013-12-04 18:41:22]

On 2013/12/04 00:55:21, jln wrote:
> Robert: please take a look.
> 
> I've been pondering having a common prefix for all of these. (IsBuildXXX ?)
> 
> I couldn't decide, so let's discuss this here.

I think IsBuildLinux() is a bit weird. I'd expect something like IsOSLinux(),
which also matches how I'm used to using the #ifdefs. I like IsArchitectureX(),
though.

https://codereview.chromium.org/103293003/diff/1/build/build_config_functions.h
File build/build_config_functions.h (right):

https://codereview.chromium.org/103293003/diff/1/build/build_config_functions...
build/build_config_functions.h:7: // binary will look the same when using these.
Is this guaranteed on every platform/arch on which we build?

https://codereview.chromium.org/103293003/diff/1/build/build_config_functions...
build/build_config_functions.h:13: 
Should these be in base:: ?

https://codereview.chromium.org/103293003/diff/1/build/build_config_functions...
build/build_config_functions.h:46: inline bool IsPosix() {
You capitalize ASAN above, but not POSIX here.

https://codereview.chromium.org/103293003/diff/1/build/build_config_functions...
build/build_config_functions.h:61: 
IsMac() ?

[jln (very slow on Chromium), 2013-12-04 20:37:35]

https://codereview.chromium.org/103293003/diff/1/build/build_config_functions.h
File build/build_config_functions.h (right):

https://codereview.chromium.org/103293003/diff/1/build/build_config_functions...
build/build_config_functions.h:7: // binary will look the same when using these.
On 2013/12/04 18:41:22, rsesek wrote:
> Is this guaranteed on every platform/arch on which we build?

No, "inline" itself is not even guaranteed, changing to "should".

https://codereview.chromium.org/103293003/diff/1/build/build_config_functions...
build/build_config_functions.h:13: 
On 2013/12/04 18:41:22, rsesek wrote:
> Should these be in base:: ?

Yes, I think you're right, this would be better in base. Let's see what Will
says.

https://codereview.chromium.org/103293003/diff/1/build/build_config_functions...
build/build_config_functions.h:46: inline bool IsPosix() {
On 2013/12/04 18:41:22, rsesek wrote:
> You capitalize ASAN above, but not POSIX here.

Done.

https://codereview.chromium.org/103293003/diff/1/build/build_config_functions...
build/build_config_functions.h:61: 
On 2013/12/04 18:41:22, rsesek wrote:
> IsMac() ?

Done.

[jln (very slow on Chromium), 2013-12-04 20:39:19]

Will: I think Robert is right that this would better live in base. WDYT?

Also maybe we should add "constexpr" to compiler_specific.h and make use of it
here.

[jln (very slow on Chromium), 2013-12-04 20:41:16]

Adding Jeffrey as well. Jeffrey, feel free to punt if you're busy, but your
expertise could be useful here.

[willchan no longer on Chromium, 2013-12-04 21:44:45]

So build/ is lower level than base/ (i.e. base/ can depend on build/, as seen in
https://code.google.com/p/chromium/codesearch#search/&q=%23include%20%22build...).
Is there a reason to prefer moving this above to base/?

[Robert Sesek, 2013-12-04 21:46:43]

On 2013/12/04 21:44:45, willchan wrote:
> So build/ is lower level than base/ (i.e. base/ can depend on build/, as seen
in
>
https://code.google.com/p/chromium/codesearch#search/&q=%2523include%2520%252...).
> Is there a reason to prefer moving this above to base/?

I was braindead and read "build/" as "base/". Maybe put this in a build::
namespace.

[jln (very slow on Chromium), 2013-12-04 22:23:23]

On 2013/12/04 21:46:43, rsesek wrote:

> I was braindead and read "build/" as "base/". Maybe put this in a build::
> namespace.

Ok. I've added a build:: namespace.

Given that stuff like compiler_specific.h lives in base/ and that these would be
the first "functions" inside build/, I thought that base could make sense, but
build/ is fine.

PTAL!

[jln (very slow on Chromium), 2013-12-04 22:24:09]

(also, I've removed content/common/sandbox_seccomp_bpf_linux.cc from this CL
because there are too many pending CL with a non trivial rebase)

[Robert Sesek, 2013-12-05 14:51:45]

https://codereview.chromium.org/103293003/diff/1/build/build_config_functions.h
File build/build_config_functions.h (right):

https://codereview.chromium.org/103293003/diff/1/build/build_config_functions...
build/build_config_functions.h:7: // binary will look the same when using these.
On 2013/12/04 20:37:36, jln wrote:
> On 2013/12/04 18:41:22, rsesek wrote:
> > Is this guaranteed on every platform/arch on which we build?
> 
> No, "inline" itself is not even guaranteed, changing to "should".

Yes, so I worry that extensive use of these may mean products ship dead weight.
:/

https://codereview.chromium.org/103293003/diff/30001/build/build_config_funct...
File build/build_config_functions.h (right):

https://codereview.chromium.org/103293003/diff/30001/build/build_config_funct...
build/build_config_functions.h:90: inline bool IsArchitectureArm() {
Arm -> ARM

[jln (very slow on Chromium), 2013-12-05 18:34:54]

> Yes, so I worry that extensive use of these may mean products ship dead
weight.
> :/

There no guarantee, but in practice, I'm convinced it *will* be in-lined.
Jeffrey?

[Jeffrey Yasskin, 2013-12-05 18:39:03]

On Thu, Dec 5, 2013 at 10:34 AM,  <jln@chromium.org> wrote:
>> Yes, so I worry that extensive use of these may mean products ship dead
>
> weight.
>>
>> :/
>
>
> There no guarantee, but in practice, I'm convinced it *will* be in-lined.
> Jeffrey?

It's very likely, but for a top-down inliner (gcc iirc) there is a
chance that the calling function will already be past the size
threshold, and the compiler won't even look at the contents. You'd be
safer on this point if you used a global const variable, since that's
just used as a constant and doesn't need the inliner to work.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Robert Sesek, 2013-12-05 20:37:02]

LGTM then

[jln (very slow on Chromium), 2013-12-05 23:37:24]

https://codereview.chromium.org/103293003/diff/30001/build/build_config_funct...
File build/build_config_functions.h (right):

https://codereview.chromium.org/103293003/diff/30001/build/build_config_funct...
build/build_config_functions.h:90: inline bool IsArchitectureArm() {
On 2013/12/05 14:51:46, rsesek wrote:
> Arm -> ARM

Done.

[jln (very slow on Chromium), 2013-12-06 00:07:01]

Committed patchset #5 manually as r239079 (presubmit successful).

[jam, 2013-12-06 00:09:25]

hmm, it seems unfortunate to now have two ways of doing this, and two different
strings to search for when looking through code.

Can you get more feedback on chromium-dev before spreading the use of this?
Thanks

[jln (very slow on Chromium), 2013-12-06 00:13:56]

On 2013/12/06 00:09:25, jam wrote:
> hmm, it seems unfortunate to now have two ways of doing this, and two
different
> strings to search for when looking through code.
> 
> Can you get more feedback on chromium-dev before spreading the use of this?
> Thanks

It's necessary for a lot of low level sandboxing code, to avoid it being plague
with #ifdef and being even more readable.

Previously, Linux sandboxing code was only in a few files, so they had their own
definition of these. I'm refactoring this code and splitting to multiple files
so we should have a central place for this. However, it could live in
linux/sandbox/ if no one else wants to use it.

I'll start a thread, thanks!

[jam, 2013-12-06 00:20:11]

On 2013/12/06 00:13:56, jln wrote:
> On 2013/12/06 00:09:25, jam wrote:
> > hmm, it seems unfortunate to now have two ways of doing this, and two
> different
> > strings to search for when looking through code.
> > 
> > Can you get more feedback on chromium-dev before spreading the use of this?
> > Thanks
> 
> It's necessary for a lot of low level sandboxing code, to avoid it being
plague
> with #ifdef and being even more readable.
> 
> Previously, Linux sandboxing code was only in a few files, so they had their
own
> definition of these. I'm refactoring this code and splitting to multiple files
> so we should have a central place for this. However, it could live in
> linux/sandbox/ if no one else wants to use it.

I think it would be best if the few methods that you want for the linux
sandboxing code to be more readable can be in a header just for that directory,
and not spread this to the rest of the code.

IMO we've had ifdefs for ~5 years and it has worked out fine. Having ifdefs for
code that's got to be eliminated at compile time, and runtime checks for other
code, seems worse than having one method for both.

> 
> I'll start a thread, thanks!

[darin (slow to review), 2013-12-06 04:48:23]

Is there code outside of sandbox/linux/ where you intend to use these functions?
We typically only generalize a solution when we have a use case that warrants
it. This patch doesn't show the new intended consumer.

[jln (very slow on Chromium), 2013-12-06 04:57:52]

On 2013/12/06 04:48:23, darin wrote:
> Is there code outside of sandbox/linux/ where you intend to use these
functions?
> We typically only generalize a solution when we have a use case that warrants
> it. This patch doesn't show the new intended consumer.

I didn't add consumers in this CL, because I have a few CLs in work that would
need a non trivial rebase if I did.

content/common/sandbox_seccomp_bpf_linux.cc is another example. It's a giant
horrific file that I'm splitting into
a lot of smaller files and refactoring.

But everything for which I intended to use it is sandboxing related indeed.
Given the thread on http://goo.gl/3ufXOJ,
I'm removing this from build/ and moving it to sandbox/
(https://chromiumcodereview.appspot.com/107693002/).

I have struggled in the past with superfluous #ifdef, but it looks like others
don't feel as strongly against it.
Also, a good argument against it is that superfluous #ifdef are probably a sign
that something needs to be re-factored.

[jln (very slow on Chromium), 2013-12-06 08:52:23]

A revert of this CL has been created in
https://chromiumcodereview.appspot.com/108013002/ by jln@chromium.org.

The reason for reverting is: http://goo.gl/3ufXOJ.