Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2068863002:
Added warning for variable-length arrays to GYP (Closed)
DescriptionAdded warning for variable-length arrays to GYP, since they are unsupprted under Win targets
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2068863002
Committed: https://skia.googlesource.com/skia/+/115e925dc85f3c6dbb140a2c1be3309ff72d3d8b
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixed formatting #Patch Set 3 : Made array size variable 'const' in file triggering -Wvla #
Messages
Total messages: 23 (12 generated)
Description was changed from ========== Added warning for variable-length arrays to GYP DO NOT LAND. I don't know if this will work on OSX/Android/Solaris/etc. Added warning for variable-length arrays to GYP, since they are unsupprted under Win targets BUG=skia: ========== to ========== Added warning for variable-length arrays to GYP DO NOT LAND. I don't know if this will work on OSX/Android/Solaris/etc. Added warning for variable-length arrays to GYP, since they are unsupprted under Win targets BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2068863002 ==========
Description was changed from ========== Added warning for variable-length arrays to GYP DO NOT LAND. I don't know if this will work on OSX/Android/Solaris/etc. Added warning for variable-length arrays to GYP, since they are unsupprted under Win targets BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2068863002 ========== to ========== Added warning for variable-length arrays to GYP DO NOT LAND. I don't know if this will work on OSX/Android/Solaris/etc. Added warning for variable-length arrays to GYP, since they are unsupprted under Win targets BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2068863002 ==========
dvonbeck@google.com changed reviewers: + egdaniel@google.com
mtklein@google.com changed reviewers: + mtklein@google.com
This is a very good idea. The trybots should stop us from landing something that will break the tree, so don't worry too much about breaking things. It's easier to revert a CL than it is to land it in the first place. I added some small formatting suggestions, but this otherwise lgtm. https://codereview.chromium.org/2068863002/diff/1/gyp/common_conditions.gypi File gyp/common_conditions.gypi (right): https://codereview.chromium.org/2068863002/diff/1/gyp/common_conditions.gypi#... gyp/common_conditions.gypi:249: '-Wvla', You might want to have these match the indentation of the code around them, and have Wvla placed consistently in the Linux+Android and Mac sections?
Description was changed from ========== Added warning for variable-length arrays to GYP DO NOT LAND. I don't know if this will work on OSX/Android/Solaris/etc. Added warning for variable-length arrays to GYP, since they are unsupprted under Win targets BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2068863002 ========== to ========== Added warning for variable-length arrays to GYP Added warning for variable-length arrays to GYP, since they are unsupprted under Win targets BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2068863002 ==========
Description was changed from ========== Added warning for variable-length arrays to GYP Added warning for variable-length arrays to GYP, since they are unsupprted under Win targets BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2068863002 ========== to ========== Added warning for variable-length arrays to GYP, since they are unsupprted under Win targets BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2068863002 ==========
I fixed the formatting (oops). Should I land it then? https://codereview.chromium.org/2068863002/diff/1/gyp/common_conditions.gypi File gyp/common_conditions.gypi (right): https://codereview.chromium.org/2068863002/diff/1/gyp/common_conditions.gypi#... gyp/common_conditions.gypi:249: '-Wvla', On 2016/06/15 14:43:27, mtklein wrote: > You might want to have these match the indentation of the code around them, and > have Wvla placed consistently in the Linux+Android and Mac sections? Oops I didn't notice. Done!
lgtm
The CQ bit was checked by dvonbeck@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com Link to the patchset: https://codereview.chromium.org/2068863002/#ps20001 (title: "Fixed formatting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2068863002/20001
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2068863002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)
Nice! You found a bug! SkNSView.mm:347: static int kAttributeCount = SK_ARRAY_COUNT(attributes); that needs to be static const int to pass -Wvla.
On 2016/06/15 15:12:38, mtklein wrote: > Nice! You found a bug! > > SkNSView.mm:347: static int kAttributeCount = SK_ARRAY_COUNT(attributes); > > that needs to be static const int to pass -Wvla. Done! I'm re-running it on the failed trybot.
The CQ bit was checked by dvonbeck@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from egdaniel@google.com, mtklein@google.com Link to the patchset: https://codereview.chromium.org/2068863002/#ps40001 (title: "Made array size variable 'const' in file triggering -Wvla")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2068863002/40001
Message was sent while issue was closed.
Description was changed from ========== Added warning for variable-length arrays to GYP, since they are unsupprted under Win targets BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2068863002 ========== to ========== Added warning for variable-length arrays to GYP, since they are unsupprted under Win targets BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2068863002 Committed: https://skia.googlesource.com/skia/+/115e925dc85f3c6dbb140a2c1be3309ff72d3d8b ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/115e925dc85f3c6dbb140a2c1be3309ff72d3d8b |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
