Do not expect a copysign function to be defined in <cmath> with clang-cl

Do not expect a copysign function to be defined in <cmath> with clang-cl

clang-cl defines __cplusplus to 201103L, but it uses the runtime library
provided by MSVC, so the copysign function will not be available there.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/c34b0d4e9ad5806c1f882a1f85191f2ea8ddcdba

[ehsan, 2014-09-09 11:50:10]

ehsan.akhgari@gmail.com changed reviewers:
+ reed@google.com

[reed1, 2014-09-12 19:16:02]

The CQ bit was checked by reed@google.com

[reed1, 2014-09-12 19:16:03]

lgtm

[commit-bot: I haz the power, 2014-09-12 19:16:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/526813002/1

[commit-bot: I haz the power, 2014-09-12 19:30:38]

Committed patchset #1 (id:1) as c34b0d4e9ad5806c1f882a1f85191f2ea8ddcdba

[bungeman-skia, 2014-09-12 20:15:26]

On 2014/09/12 19:30:38, I haz the power (commit-bot) wrote:
> Committed patchset #1 (id:1) as c34b0d4e9ad5806c1f882a1f85191f2ea8ddcdba

I don't like this change, especially without a comment. The copysign function is
quite available in newer runtime libraries provided by MSVC, so what we really
want to know here is which MSVC library we're going to link against. I certainly
do not want clang-cl to run slower and non portably simply because of this bogus
macro condition. In fact, copysign should always be available with vc++ 18+
(visual studio 2013+) and we're already basically requiring that already (we
don't really test compiling with anything older).

[bungeman-skia, 2014-09-12 20:19:18]

On 2014/09/12 20:15:26, bungeman1 wrote:
> On 2014/09/12 19:30:38, I haz the power (commit-bot) wrote:
> > Committed patchset #1 (id:1) as c34b0d4e9ad5806c1f882a1f85191f2ea8ddcdba
> 
> I don't like this change, especially without a comment. The copysign function
is
> quite available in newer runtime libraries provided by MSVC, so what we really
> want to know here is which MSVC library we're going to link against. I
certainly
> do not want clang-cl to run slower and non portably simply because of this
bogus
> macro condition. In fact, copysign should always be available with vc++ 18+
> (visual studio 2013+) and we're already basically requiring that already (we
> don't really test compiling with anything older).

As an additional note, Chromium has been building Skia warning free with
clang-cl for some time now, and has had no issue with this, since Chromium
definitely demands vs2013+.

[bungeman-skia, 2014-09-12 20:54:52]

https://codereview.chromium.org/526813002/diff/1/include/core/SkFloatingPoint.h
File include/core/SkFloatingPoint.h (right):

https://codereview.chromium.org/526813002/diff/1/include/core/SkFloatingPoint...
include/core/SkFloatingPoint.h:34: #if (!defined(_MSC_VER) && __cplusplus >=
201103L) || (defined(_MSC_VER) && _MSC_VER >= 1800)
So this won't break Chromium for now because Chromium does set
'-fmsc-version=1800'. On the other hand, this change doesn't make it very clear
that clang-cl causes '__cplusplus >= 201103L' to be true even when the runtime
library it's going to link against doesn't actually support that.


I think what we might want is a

#define SK_BUILD_CLANG_CL (defined(_MSC_VER) && defined(__clang__))

and change this line to

#if (!SK_BUILD_CLANG_CL && __cplusplus >= 201103L) || (defined(_MSC_VER) &&
_MSC_VER >= 1800)

to state clearly that we don't trust the clang-cl __cplusplus.

[ehsan, 2014-09-12 21:22:13]

On 2014/09/12 20:54:52, bungeman1 wrote:
>
https://codereview.chromium.org/526813002/diff/1/include/core/SkFloatingPoint.h
> File include/core/SkFloatingPoint.h (right):
> 
>
https://codereview.chromium.org/526813002/diff/1/include/core/SkFloatingPoint...
> include/core/SkFloatingPoint.h:34: #if (!defined(_MSC_VER) && __cplusplus >=
> 201103L) || (defined(_MSC_VER) && _MSC_VER >= 1800)
> So this won't break Chromium for now because Chromium does set
> '-fmsc-version=1800'. On the other hand, this change doesn't make it very
clear
> that clang-cl causes '__cplusplus >= 201103L' to be true even when the runtime
> library it's going to link against doesn't actually support that.
> 
> 
> I think what we might want is a
> 
> #define SK_BUILD_CLANG_CL (defined(_MSC_VER) && defined(__clang__))
> 
> and change this line to
> 
> #if (!SK_BUILD_CLANG_CL && __cplusplus >= 201103L) || (defined(_MSC_VER) &&
> _MSC_VER >= 1800)
> 
> to state clearly that we don't trust the clang-cl __cplusplus.

OK, just one question about the submission process.  It seems like this patch
was committed.  Should I submit another review requests which reverts the
current patch and implements your suggestion above?  Or should this get reverted
separately?

[bungeman-skia, 2014-09-12 21:49:25]

On 2014/09/12 21:22:13, ehsan wrote:
> On 2014/09/12 20:54:52, bungeman1 wrote:
> >
>
https://codereview.chromium.org/526813002/diff/1/include/core/SkFloatingPoint.h
> > File include/core/SkFloatingPoint.h (right):
> > 
> >
>
https://codereview.chromium.org/526813002/diff/1/include/core/SkFloatingPoint...
> > include/core/SkFloatingPoint.h:34: #if (!defined(_MSC_VER) && __cplusplus >=
> > 201103L) || (defined(_MSC_VER) && _MSC_VER >= 1800)
> > So this won't break Chromium for now because Chromium does set
> > '-fmsc-version=1800'. On the other hand, this change doesn't make it very
> clear
> > that clang-cl causes '__cplusplus >= 201103L' to be true even when the
runtime
> > library it's going to link against doesn't actually support that.
> > 
> > 
> > I think what we might want is a
> > 
> > #define SK_BUILD_CLANG_CL (defined(_MSC_VER) && defined(__clang__))
> > 
> > and change this line to
> > 
> > #if (!SK_BUILD_CLANG_CL && __cplusplus >= 201103L) || (defined(_MSC_VER) &&
> > _MSC_VER >= 1800)
> > 
> > to state clearly that we don't trust the clang-cl __cplusplus.
> 
> OK, just one question about the submission process.  It seems like this patch
> was committed.  Should I submit another review requests which reverts the
> current patch and implements your suggestion above?  Or should this get
reverted
> separately?

I think it's ok to leave this as-is over the weekend. I'm not sure where or
exactly what to name the 'SK_BUILD_CLANG_CL' macro at the moment. I also
remember that when all of this code here landed there was a general discussion
of how we should be testing for c++11 features, an I'd like to make sure that we
do something sane about that. I'll take a better look at this soon, and I'll
probably end up putting together a CL. My initial reaction was mostly a result
of not knowing how this change would affect Chromium when we roll.

[ehsan, 2014-09-15 14:59:01]

On 2014/09/12 21:49:25, bungeman1 wrote:
> I think it's ok to leave this as-is over the weekend. I'm not sure where or
> exactly what to name the 'SK_BUILD_CLANG_CL' macro at the moment. I also
> remember that when all of this code here landed there was a general discussion
> of how we should be testing for c++11 features, an I'd like to make sure that
we
> do something sane about that. I'll take a better look at this soon, and I'll
> probably end up putting together a CL. My initial reaction was mostly a result
> of not knowing how this change would affect Chromium when we roll.

Sounds good!  Please let me know if there is anything else for me to do. 
Thanks!

[bungeman-skia, 2014-09-17 18:50:40]

On 2014/09/15 14:59:01, ehsan wrote:
> On 2014/09/12 21:49:25, bungeman1 wrote:
> > I think it's ok to leave this as-is over the weekend. I'm not sure where or
> > exactly what to name the 'SK_BUILD_CLANG_CL' macro at the moment. I also
> > remember that when all of this code here landed there was a general
discussion
> > of how we should be testing for c++11 features, an I'd like to make sure
that
> we
> > do something sane about that. I'll take a better look at this soon, and I'll
> > probably end up putting together a CL. My initial reaction was mostly a
result
> > of not knowing how this change would affect Chromium when we roll.
> 
> Sounds good!  Please let me know if there is anything else for me to do. 
> Thanks!

This has been cleared up enough for the time being with
https://codereview.chromium.org/576023003/ .