Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
330823006:
Define Color::transparent et al. when using Clang on Windows (Closed)
|
Created:
6 years, 6 months ago by hans Modified:
6 years, 6 months ago CC:
blink-reviews, jamesr, krit, blink-reviews-wtf_chromium.org, jbroman, abarth-chromium, danakj, Rik, Stephen Chennney, Mikhail, pdr., rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionDefine Color::transparent et al. when using Clang on Windows
MSVC treats their in-class initializations as definitions, but other
compilers don't. This updates the #ifdef to handle the Clang on Windows
case.
BUG=82385
TEST=build blink_web.dll with Clang on Windows in Release mode
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176469
Patch Set 1 #Patch Set 2 : Just fix it locally #Messages
Total messages: 13 (0 generated)
This fixes another component build link error with Clang on Windows. Please take a look.
Do you know how many of these you'll need? We (chromium) tend to not use static members for this reason… The usual workaround is to have unnamed enums instead, but the colors that's tricky since they need 32bits and enum signedness is different in msvs too as far as I remember :-/
On 2014/06/18 21:15:35, Nico (away) wrote: > Do you know how many of these you'll need? > > We (chromium) tend to not use static members for this reason… > > The usual workaround is to have unnamed enums instead, but the colors that's > tricky since they need 32bits and enum signedness is different in msvs too as > far as I remember :-/ You can use C++11 strong enums to get the signedness right MSVC since 2010, but you can't use it unconditionally in gcc or clang's C++98 mode. =/
On Wed, Jun 18, 2014 at 2:18 PM, <rnk@chromium.org> wrote: > On 2014/06/18 21:15:35, Nico (away) wrote: > >> Do you know how many of these you'll need? >> > > We (chromium) tend to not use static members for this reason... >> > > The usual workaround is to have unnamed enums instead, but the colors >> that's >> tricky since they need 32bits and enum signedness is different in msvs >> too as >> far as I remember :-/ >> > > You can use C++11 strong enums to get the signedness right MSVC since > 2010, but > you can't use it unconditionally in gcc or clang's C++98 mode. =/ > sooooon To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2014/06/18 21:15:35, Nico (away) wrote: > Do you know how many of these you'll need? > > We (chromium) tend to not use static members for this reason… I tried to grep for other definitions guarded by #if !COMPILER(MSVC), but didn't find any more.
On 2014/06/18 21:22:38, hans wrote: > On 2014/06/18 21:15:35, Nico (away) wrote: > > Do you know how many of these you'll need? > > > > We (chromium) tend to not use static members for this reason… > > I tried to grep for other definitions guarded by #if !COMPILER(MSVC), but didn't > find any more. I wonder if we should just say !COMPILER(MSVC) && !COMPILER(CLANG) in this one place. With a quirk, this looks like some official pattern. Not sure, though.
On 2014/06/18 21:33:12, Nico (away) wrote:
> On 2014/06/18 21:22:38, hans wrote:
> > On 2014/06/18 21:15:35, Nico (away) wrote:
> > > Do you know how many of these you'll need?
> > >
> > > We (chromium) tend to not use static members for this reason…
> >
> > I tried to grep for other definitions guarded by #if !COMPILER(MSVC), but
> didn't
> > find any more.
>
> I wonder if we should just say !COMPILER(MSVC) && !COMPILER(CLANG) in this one
> place. With a quirk, this looks like some official pattern. Not sure, though.
I was trying to keep such logic away from implementation code because I feel it
can be a little confusing ("what do you mean compile this code if it's both msvc
and clang?"), but since it's just the one place I'd be fine with it here.
On Wed, Jun 18, 2014 at 2:38 PM, <hans@chromium.org> wrote: > On 2014/06/18 21:33:12, Nico (away) wrote: > >> On 2014/06/18 21:22:38, hans wrote: >> > On 2014/06/18 21:15:35, Nico (away) wrote: >> > > Do you know how many of these you'll need? >> > > >> > > We (chromium) tend to not use static members for this reason... >> > >> > I tried to grep for other definitions guarded by #if !COMPILER(MSVC), >> but >> didn't >> > find any more. >> > > I wonder if we should just say !COMPILER(MSVC) && !COMPILER(CLANG) in >> this one >> place. With a quirk, this looks like some official pattern. Not sure, >> though. >> > > I was trying to keep such logic away from implementation code because I > feel it > can be a little confusing ("what do you mean compile this code if it's > both msvc > and clang?"), but since it's just the one place I'd be fine with it here. > Yeah, that's a bit weird. We only have one GCC && !CLANG place ( https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... ). Since it's just one place and we seem to have a plan of making it go away once c++11 is here, I think it's ok. Maybe add a TODO to use a strong enum instead once we can use c++11. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2014/06/18 21:43:15, Nico (away) wrote:
> On Wed, Jun 18, 2014 at 2:38 PM, <mailto:hans@chromium.org> wrote:
>
> > On 2014/06/18 21:33:12, Nico (away) wrote:
> >
> >> On 2014/06/18 21:22:38, hans wrote:
> >> > On 2014/06/18 21:15:35, Nico (away) wrote:
> >> > > Do you know how many of these you'll need?
> >> > >
> >> > > We (chromium) tend to not use static members for this reason...
> >> >
> >> > I tried to grep for other definitions guarded by #if !COMPILER(MSVC),
> >> but
> >> didn't
> >> > find any more.
> >>
> >
> > I wonder if we should just say !COMPILER(MSVC) && !COMPILER(CLANG) in
> >> this one
> >> place. With a quirk, this looks like some official pattern. Not sure,
> >> though.
> >>
> >
> > I was trying to keep such logic away from implementation code because I
> > feel it
> > can be a little confusing ("what do you mean compile this code if it's
> > both msvc
> > and clang?"), but since it's just the one place I'd be fine with it here.
> >
>
> Yeah, that's a bit weird. We only have one GCC && !CLANG place (
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> ). Since it's just one place and we seem to have a plan of making it go
> away once c++11 is here, I think it's ok. Maybe add a TODO to use a strong
> enum instead once we can use c++11.
OK, I've updated the patch and description.
lgtm, thanks!
The CQ bit was checked by hans@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hans@chromium.org/330823006/20001
Message was sent while issue was closed.
Change committed as 176469 |
