Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1666203002:
Move Google3-specific stack limitation logic to template classes. (Closed)
DescriptionMove Google3-specific stack limitation logic to template classes.
Remove #ifdefs in other files.
Reapplies https://codereview.chromium.org/1656143003; removing the implicit constructors for GLPtr and GLPtrAlias resolves the build issue on Android.
Also reverts https://codereview.chromium.org/1663013004
Does not change the public API.
TBR=reed
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1666203002
Committed: https://skia.googlesource.com/skia/+/f49c75a8f1eacf8e6cb19ce0dcc1cc9bcbf1f96e
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add comment on mysterious include. #Patch Set 3 : Fix GLPtr(Alias). #
Messages
Total messages: 37 (16 generated)
Description was changed from ========== Move Google3-specific stack limitation logic to template classes. Remove #ifdefs in other files. Reapplies https://codereview.chromium.org/1656143003; adding "#include <functional>" in src/gpu/gl/GrGLGpu.cpp resolves the build issue on Android. Also reverts https://codereview.chromium.org/1663013004 Does not change the public API. ========== to ========== Move Google3-specific stack limitation logic to template classes. Remove #ifdefs in other files. Reapplies https://codereview.chromium.org/1656143003; adding "#include <functional>" in src/gpu/gl/GrGLGpu.cpp resolves the build issue on Android. Also reverts https://codereview.chromium.org/1663013004 Does not change the public API. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1666203002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1666203002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Move Google3-specific stack limitation logic to template classes. Remove #ifdefs in other files. Reapplies https://codereview.chromium.org/1656143003; adding "#include <functional>" in src/gpu/gl/GrGLGpu.cpp resolves the build issue on Android. Also reverts https://codereview.chromium.org/1663013004 Does not change the public API. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Move Google3-specific stack limitation logic to template classes. Remove #ifdefs in other files. Reapplies https://codereview.chromium.org/1656143003; adding "#include <functional>" in src/gpu/gl/GrGLGpu.cpp resolves the build issue on Android. Also reverts https://codereview.chromium.org/1663013004 Does not change the public API. TBR=reed GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
benjaminwagner@google.com changed reviewers: + mtklein@google.com
I reproduced the build error caused by https://codereview.chromium.org/1656143003 using the build instructions here: https://www.chromium.org/developers/how-tos/android-build-instructions This CL builds successfully.
On 2016/02/04 at 18:53:51, Ben Wagner wrote: > I reproduced the build error caused by https://codereview.chromium.org/1656143003 using the build instructions here: https://www.chromium.org/developers/how-tos/android-build-instructions > > This CL builds successfully. Sorry, forgot to mention: diffs from previous are just adding "#include <functional>" in src/gpu/gl/GrGLGpu.cpp and reverting https://codereview.chromium.org/1663013004.
https://codereview.chromium.org/1666203002/diff/1/src/gpu/gl/GrGLGpu.cpp File src/gpu/gl/GrGLGpu.cpp (right): https://codereview.chromium.org/1666203002/diff/1/src/gpu/gl/GrGLGpu.cpp#newc... src/gpu/gl/GrGLGpu.cpp:8: #include <functional> // TODO: wtf? We still don't think this explains things, right? I don't see any use of std::function or other <functional> things inside this file. It looks like to me that everyone who needs it includes it themselves: ~/skia (status) $ git grep functional\> dm/DMSrcSink.cpp:#include <functional> gm/image.cpp:#include <functional> include/views/SkOSWindow_Win.h:#include <functional> src/core/SkTaskGroup.h:#include <functional> src/gpu/gl/GrGLCaps.h:#include <functional> tests/BlendTest.cpp:#include <functional> tests/ImageTest.cpp:#include <functional> ~/skia (status) $ git grep std::function dm/DMSrcSink.cpp: SkISize size, std::function<Error(SkCanvas*)> draw) { dm/DMSrcSink.cpp: ProxySrc(SkISize size, std::function<Error(SkCanvas*)> draw) : fSize(size), fDraw(draw) {} dm/DMSrcSink.cpp: std::function<Error(SkCanvas*)> fDraw; gm/image.cpp: std::function<SkImage*()> imageFactories[] = { include/views/SkOSWindow_Win.h: static void ForAllWindows(const std::function<void(void*, SkOSWindow**)>& f) { src/core/SkTaskGroup.cpp: static void Add(std::function<void(void)> fn, SkAtomic<int32_t>* pending) { src/core/SkTaskGroup.cpp: static void Batch(int N, std::function<void(int)> fn, SkAtomic<int32_t>* pending) { src/core/SkTaskGroup.cpp: std::function<void(void)> fn; // A function to call src/core/SkTaskGroup.cpp: void add(std::function<void(void)> fn, SkAtomic<int32_t>* pending) { src/core/SkTaskGroup.cpp: void batch(int N, std::function<void(int)> fn, SkAtomic<int32_t>* pending) { src/core/SkTaskGroup.cpp:void SkTaskGroup::add(std::function<void(void)> fn) { ThreadPool::Add(fn, &fPending); } src/core/SkTaskGroup.cpp:void SkTaskGroup::batch(int N, std::function<void(int)> fn) { src/core/SkTaskGroup.h: void add(std::function<void(void)> fn); src/core/SkTaskGroup.h: void batch(int N, std::function<void(int)> fn); src/gpu/gl/GrGLCaps.cpp: std::function<void (GrGLenum, GrGLint*)> getIntegerv, src/gpu/gl/GrGLCaps.cpp: std::function<bool ()> bindRenderTarget) const { src/gpu/gl/GrGLCaps.h: std::function<void (GrGLenum, GrGLint*)> getIntegerv, src/gpu/gl/GrGLCaps.h: std::function<bool ()> bindRenderTarget) const; tests/ImageTest.cpp: std::function<SkImage*()> imageFactories[] = {
https://codereview.chromium.org/1666203002/diff/1/src/gpu/gl/GrGLGpu.cpp File src/gpu/gl/GrGLGpu.cpp (right): https://codereview.chromium.org/1666203002/diff/1/src/gpu/gl/GrGLGpu.cpp#newc... src/gpu/gl/GrGLGpu.cpp:8: #include <functional> On 2016/02/04 at 19:16:05, mtklein wrote: > // TODO: wtf? > > We still don't think this explains things, right? I don't see any use of std::function or other <functional> things inside this file. It looks like to me that everyone who needs it includes it themselves: > > ~/skia (status) $ git grep functional\> > dm/DMSrcSink.cpp:#include <functional> > gm/image.cpp:#include <functional> > include/views/SkOSWindow_Win.h:#include <functional> > src/core/SkTaskGroup.h:#include <functional> > src/gpu/gl/GrGLCaps.h:#include <functional> > tests/BlendTest.cpp:#include <functional> > tests/ImageTest.cpp:#include <functional> > > ~/skia (status) $ git grep std::function > dm/DMSrcSink.cpp: SkISize size, std::function<Error(SkCanvas*)> draw) { > dm/DMSrcSink.cpp: ProxySrc(SkISize size, std::function<Error(SkCanvas*)> draw) : fSize(size), fDraw(draw) {} > dm/DMSrcSink.cpp: std::function<Error(SkCanvas*)> fDraw; > gm/image.cpp: std::function<SkImage*()> imageFactories[] = { > include/views/SkOSWindow_Win.h: static void ForAllWindows(const std::function<void(void*, SkOSWindow**)>& f) { > src/core/SkTaskGroup.cpp: static void Add(std::function<void(void)> fn, SkAtomic<int32_t>* pending) { > src/core/SkTaskGroup.cpp: static void Batch(int N, std::function<void(int)> fn, SkAtomic<int32_t>* pending) { > src/core/SkTaskGroup.cpp: std::function<void(void)> fn; // A function to call > src/core/SkTaskGroup.cpp: void add(std::function<void(void)> fn, SkAtomic<int32_t>* pending) { > src/core/SkTaskGroup.cpp: void batch(int N, std::function<void(int)> fn, SkAtomic<int32_t>* pending) { > src/core/SkTaskGroup.cpp:void SkTaskGroup::add(std::function<void(void)> fn) { ThreadPool::Add(fn, &fPending); } > src/core/SkTaskGroup.cpp:void SkTaskGroup::batch(int N, std::function<void(int)> fn) { > src/core/SkTaskGroup.h: void add(std::function<void(void)> fn); > src/core/SkTaskGroup.h: void batch(int N, std::function<void(int)> fn); > src/gpu/gl/GrGLCaps.cpp: std::function<void (GrGLenum, GrGLint*)> getIntegerv, > src/gpu/gl/GrGLCaps.cpp: std::function<bool ()> bindRenderTarget) const { > src/gpu/gl/GrGLCaps.h: std::function<void (GrGLenum, GrGLint*)> getIntegerv, > src/gpu/gl/GrGLCaps.h: std::function<bool ()> bindRenderTarget) const; > tests/ImageTest.cpp: std::function<SkImage*()> imageFactories[] = { There is an implicit conversion from lambda to std::function on lines 2081/2077 and 2101/1097. I agree that the order of includes shouldn't cause a build to succeed/fail, but at least it's an explanation.
On 2016/02/04 at 19:23:39, Ben Wagner wrote: > https://codereview.chromium.org/1666203002/diff/1/src/gpu/gl/GrGLGpu.cpp > File src/gpu/gl/GrGLGpu.cpp (right): > > https://codereview.chromium.org/1666203002/diff/1/src/gpu/gl/GrGLGpu.cpp#newc... > src/gpu/gl/GrGLGpu.cpp:8: #include <functional> > On 2016/02/04 at 19:16:05, mtklein wrote: > > // TODO: wtf? I added a comment. I am used to Google3 C++, so IMHO, directly including all headers that are used is better hygiene.
https://codereview.chromium.org/1666203002/diff/1/src/gpu/gl/GrGLGpu.cpp File src/gpu/gl/GrGLGpu.cpp (right): https://codereview.chromium.org/1666203002/diff/1/src/gpu/gl/GrGLGpu.cpp#newc... src/gpu/gl/GrGLGpu.cpp:8: #include <functional> On 2016/02/04 19:23:39, Ben Wagner wrote: > On 2016/02/04 at 19:16:05, mtklein wrote: > > // TODO: wtf? > > > > We still don't think this explains things, right? I don't see any use of > std::function or other <functional> things inside this file. It looks like to > me that everyone who needs it includes it themselves: > > > > ~/skia (status) $ git grep functional\> > > dm/DMSrcSink.cpp:#include <functional> > > gm/image.cpp:#include <functional> > > include/views/SkOSWindow_Win.h:#include <functional> > > src/core/SkTaskGroup.h:#include <functional> > > src/gpu/gl/GrGLCaps.h:#include <functional> > > tests/BlendTest.cpp:#include <functional> > > tests/ImageTest.cpp:#include <functional> > > > > ~/skia (status) $ git grep std::function > > dm/DMSrcSink.cpp: SkISize size, > std::function<Error(SkCanvas*)> draw) { > > dm/DMSrcSink.cpp: ProxySrc(SkISize size, > std::function<Error(SkCanvas*)> draw) : fSize(size), fDraw(draw) {} > > dm/DMSrcSink.cpp: std::function<Error(SkCanvas*)> fDraw; > > gm/image.cpp: std::function<SkImage*()> imageFactories[] = { > > include/views/SkOSWindow_Win.h: static void ForAllWindows(const > std::function<void(void*, SkOSWindow**)>& f) { > > src/core/SkTaskGroup.cpp: static void Add(std::function<void(void)> fn, > SkAtomic<int32_t>* pending) { > > src/core/SkTaskGroup.cpp: static void Batch(int N, std::function<void(int)> > fn, SkAtomic<int32_t>* pending) { > > src/core/SkTaskGroup.cpp: std::function<void(void)> fn; // A function > to call > > src/core/SkTaskGroup.cpp: void add(std::function<void(void)> fn, > SkAtomic<int32_t>* pending) { > > src/core/SkTaskGroup.cpp: void batch(int N, std::function<void(int)> fn, > SkAtomic<int32_t>* pending) { > > src/core/SkTaskGroup.cpp:void SkTaskGroup::add(std::function<void(void)> fn) { > ThreadPool::Add(fn, &fPending); } > > src/core/SkTaskGroup.cpp:void SkTaskGroup::batch(int N, > std::function<void(int)> fn) { > > src/core/SkTaskGroup.h: void add(std::function<void(void)> fn); > > src/core/SkTaskGroup.h: void batch(int N, std::function<void(int)> fn); > > src/gpu/gl/GrGLCaps.cpp: std::function<void > (GrGLenum, GrGLint*)> getIntegerv, > > src/gpu/gl/GrGLCaps.cpp: std::function<bool > ()> bindRenderTarget) const { > > src/gpu/gl/GrGLCaps.h: std::function<void > (GrGLenum, GrGLint*)> getIntegerv, > > src/gpu/gl/GrGLCaps.h: std::function<bool ()> > bindRenderTarget) const; > > tests/ImageTest.cpp: std::function<SkImage*()> imageFactories[] = { > > There is an implicit conversion from lambda to std::function on lines 2081/2077 > and 2101/1097. > > I agree that the order of includes shouldn't cause a build to succeed/fail, but > at least it's an explanation. Ah! What's the path for getting <functional> included into this file today?
On 2016/02/04 at 19:48:30, mtklein wrote: > https://codereview.chromium.org/1666203002/diff/1/src/gpu/gl/GrGLGpu.cpp > File src/gpu/gl/GrGLGpu.cpp (right): > > https://codereview.chromium.org/1666203002/diff/1/src/gpu/gl/GrGLGpu.cpp#newc... > src/gpu/gl/GrGLGpu.cpp:8: #include <functional> > On 2016/02/04 19:23:39, Ben Wagner wrote: > > On 2016/02/04 at 19:16:05, mtklein wrote: > > > // TODO: wtf? > > > > > > We still don't think this explains things, right? I don't see any use of > > std::function or other <functional> things inside this file. It looks like to > > me that everyone who needs it includes it themselves: > > > > > > ~/skia (status) $ git grep functional\> > > > dm/DMSrcSink.cpp:#include <functional> > > > gm/image.cpp:#include <functional> > > > include/views/SkOSWindow_Win.h:#include <functional> > > > src/core/SkTaskGroup.h:#include <functional> > > > src/gpu/gl/GrGLCaps.h:#include <functional> > > > tests/BlendTest.cpp:#include <functional> > > > tests/ImageTest.cpp:#include <functional> > > > > > > ~/skia (status) $ git grep std::function > > > dm/DMSrcSink.cpp: SkISize size, > > std::function<Error(SkCanvas*)> draw) { > > > dm/DMSrcSink.cpp: ProxySrc(SkISize size, > > std::function<Error(SkCanvas*)> draw) : fSize(size), fDraw(draw) {} > > > dm/DMSrcSink.cpp: std::function<Error(SkCanvas*)> fDraw; > > > gm/image.cpp: std::function<SkImage*()> imageFactories[] = { > > > include/views/SkOSWindow_Win.h: static void ForAllWindows(const > > std::function<void(void*, SkOSWindow**)>& f) { > > > src/core/SkTaskGroup.cpp: static void Add(std::function<void(void)> fn, > > SkAtomic<int32_t>* pending) { > > > src/core/SkTaskGroup.cpp: static void Batch(int N, std::function<void(int)> > > fn, SkAtomic<int32_t>* pending) { > > > src/core/SkTaskGroup.cpp: std::function<void(void)> fn; // A function > > to call > > > src/core/SkTaskGroup.cpp: void add(std::function<void(void)> fn, > > SkAtomic<int32_t>* pending) { > > > src/core/SkTaskGroup.cpp: void batch(int N, std::function<void(int)> fn, > > SkAtomic<int32_t>* pending) { > > > src/core/SkTaskGroup.cpp:void SkTaskGroup::add(std::function<void(void)> fn) { > > ThreadPool::Add(fn, &fPending); } > > > src/core/SkTaskGroup.cpp:void SkTaskGroup::batch(int N, > > std::function<void(int)> fn) { > > > src/core/SkTaskGroup.h: void add(std::function<void(void)> fn); > > > src/core/SkTaskGroup.h: void batch(int N, std::function<void(int)> fn); > > > src/gpu/gl/GrGLCaps.cpp: std::function<void > > (GrGLenum, GrGLint*)> getIntegerv, > > > src/gpu/gl/GrGLCaps.cpp: std::function<bool > > ()> bindRenderTarget) const { > > > src/gpu/gl/GrGLCaps.h: std::function<void > > (GrGLenum, GrGLint*)> getIntegerv, > > > src/gpu/gl/GrGLCaps.h: std::function<bool ()> > > bindRenderTarget) const; > > > tests/ImageTest.cpp: std::function<SkImage*()> imageFactories[] = { > > > > There is an implicit conversion from lambda to std::function on lines 2081/2077 > > and 2101/1097. > > > > I agree that the order of includes shouldn't cause a build to succeed/fail, but > > at least it's an explanation. > > Ah! What's the path for getting <functional> included into this file today? functional:477:0, src/gpu/gl/GrGLCaps.h:12, src/gpu/gl/GrGLContext.h:14, src/gpu/gl/GrGLGpu.h:11, src/gpu/gl/GrGLGpu.cpp:9: Here's the error log: https://build.chromium.org/p/chromium.webkit/builders/Android%20Builder/build...
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1666203002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1666203002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Move Google3-specific stack limitation logic to template classes. Remove #ifdefs in other files. Reapplies https://codereview.chromium.org/1656143003; adding "#include <functional>" in src/gpu/gl/GrGLGpu.cpp resolves the build issue on Android. Also reverts https://codereview.chromium.org/1663013004 Does not change the public API. TBR=reed GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Move Google3-specific stack limitation logic to template classes. Remove #ifdefs in other files. Reapplies https://codereview.chromium.org/1656143003; removing the implicit constructors for GLPtr and GLPtrAlias resolves the build issue on Android. Also reverts https://codereview.chromium.org/1663013004 Does not change the public API. TBR=reed GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Latest patch has results of pair programming.
lgtm
The CQ bit was checked by benjaminwagner@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1666203002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1666203002/40001
The CQ bit was unchecked by benjaminwagner@google.com
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1666203002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1666203002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1666203002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1666203002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by benjaminwagner@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1666203002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1666203002/40001
Message was sent while issue was closed.
Description was changed from ========== Move Google3-specific stack limitation logic to template classes. Remove #ifdefs in other files. Reapplies https://codereview.chromium.org/1656143003; removing the implicit constructors for GLPtr and GLPtrAlias resolves the build issue on Android. Also reverts https://codereview.chromium.org/1663013004 Does not change the public API. TBR=reed GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Move Google3-specific stack limitation logic to template classes. Remove #ifdefs in other files. Reapplies https://codereview.chromium.org/1656143003; removing the implicit constructors for GLPtr and GLPtrAlias resolves the build issue on Android. Also reverts https://codereview.chromium.org/1663013004 Does not change the public API. TBR=reed GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/f49c75a8f1eacf8e6cb19ce0dcc1cc9bcbf1f96e ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/f49c75a8f1eacf8e6cb19ce0dcc1cc9bcbf1f96e |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
