Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1421793009:
apply mozilla patch for skbug.com/4547 (Closed)
Descriptionapply mozilla patch
BUG=skia:4547
Committed: https://skia.googlesource.com/skia/+/d9ffaed6d21f85c717117904da26a2bac2295607
Patch Set 1 #
Total comments: 1
Messages
Total messages: 14 (4 generated)
Description was changed from ========== apply mozilla patch for skbug.com/4547 BUG=skia: ========== to ========== apply mozilla patch BUG=skia:4547 ==========
reed@google.com changed reviewers: + bsalomon@google.com, lsalzman@mozilla.com
ptal
On 2015/11/10 01:04:46, reed1 wrote: > ptal egads, lgtm
The CQ bit was checked by bsalomon@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1421793009/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1421793009/1
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://skia.googlesource.com/skia/+/d9ffaed6d21f85c717117904da26a2bac2295607
Message was sent while issue was closed.
bungeman@google.com changed reviewers: + bungeman@google.com
Message was sent while issue was closed.
In the future, better commit messages would be nice. ' apply mozilla patch' isn't very specific. https://codereview.chromium.org/1421793009/diff/1/include/gpu/GrGpuResourceRef.h File include/gpu/GrGpuResourceRef.h (right): https://codereview.chromium.org/1421793009/diff/1/include/gpu/GrGpuResourceRe... include/gpu/GrGpuResourceRef.h:185: explicit operator bool() const { return SkToBool(fResource); } Explicit operator bool has issues in VS2013, which is why we're still avoiding it. We need to be really careful with this.
Message was sent while issue was closed.
Ben, is there a better way to address the current issue / patch ?
Message was sent while issue was closed.
On 2015/11/10 18:06:19, reed1 wrote: > Ben, is there a better way to address the current issue / patch ? Just #ifdef'ing the explicit out for older MSVC would be fine for us on the Mozilla end, since it is clang that we have our implicit conversion plugin for. Could that work?
Message was sent while issue was closed.
On 2015/11/10 18:13:23, lsalzman1 wrote: > On 2015/11/10 18:06:19, reed1 wrote: > > Ben, is there a better way to address the current issue / patch ? > > Just #ifdef'ing the explicit out for older MSVC would be fine for us on the > Mozilla end, since it is clang that we have our implicit conversion plugin for. > Could that work? It's not that msvc doesn't recognize 'explicit' (it does in 2013 on), but that in 2013 the support is buggy (see https://social.msdn.microsoft.com/Forums/vstudio/en-US/af733e56-8045-4553-a9a... , and there's a connect bug I can't seem to find at the moment). While we wait for 2015 to be the base standard, we've been either giving such things explicit names or using the safe bool idiom ( https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Safe_bool ). So long as we're sure that all code using this 'explicit bool' operator is compiled with gcc, clang, or both vs2013 and vs2015 (until we no longer support 2013) we should be ok to start using it, I think.
Message was sent while issue was closed.
On 2015/11/10 18:19:22, bungeman1 wrote: > On 2015/11/10 18:13:23, lsalzman1 wrote: > > On 2015/11/10 18:06:19, reed1 wrote: > > > Ben, is there a better way to address the current issue / patch ? > > > > Just #ifdef'ing the explicit out for older MSVC would be fine for us on the > > Mozilla end, since it is clang that we have our implicit conversion plugin > for. > > Could that work? > > It's not that msvc doesn't recognize 'explicit' (it does in 2013 on), but that > in 2013 the support is buggy (see > https://social.msdn.microsoft.com/Forums/vstudio/en-US/af733e56-8045-4553-a9a... > , and there's a connect bug I can't seem to find at the moment). While we wait > for 2015 to be the base standard, we've been either giving such things explicit > names or using the safe bool idiom ( > https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Safe_bool ). So long as we're > sure that all code using this 'explicit bool' operator is compiled with gcc, > clang, or both vs2013 and vs2015 (until we no longer support 2013) we should be > ok to start using it, I think. I've opened skbug.com/4553 to get us a vs2015 bot.
Message was sent while issue was closed.
perfect, thanks. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
