New issue
Advanced search Search tips

Issue 794619 link

Starred by 3 users

Issue metadata

Status: Untriaged
Owner: ----
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 923078



Sign in to add a comment

Enable -Wshadow in more components

Project Member Reported by brucedaw...@chromium.org, Dec 13 2017

Issue description

Variable shadowing can lead to bugs, or at least confusing code. Where it can easily be avoided it should be.

Base has been building with -Wshadow since November 16, 2017 (crrev.com/c/773832) and it may be worthwhile to try enabling -Wshadow for other small components.

 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b9d88673a80c35f907482ea512582912b1bf1359

commit b9d88673a80c35f907482ea512582912b1bf1359
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Thu Dec 14 01:38:49 2017

Enable noshadowing in blink/core

Fix minor shadowing issues.

BUG=794619

Change-Id: I0656c25c13be3e7652122e1b1b1ded282f352da1
Reviewed-on: https://chromium-review.googlesource.com/825903
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523975}
[modify] https://crrev.com/b9d88673a80c35f907482ea512582912b1bf1359/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp
[modify] https://crrev.com/b9d88673a80c35f907482ea512582912b1bf1359/third_party/WebKit/Source/bindings/core/v8/custom/V8DevToolsHostCustom.cpp
[modify] https://crrev.com/b9d88673a80c35f907482ea512582912b1bf1359/third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp
[modify] https://crrev.com/b9d88673a80c35f907482ea512582912b1bf1359/third_party/WebKit/Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl
[modify] https://crrev.com/b9d88673a80c35f907482ea512582912b1bf1359/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/b9d88673a80c35f907482ea512582912b1bf1359/third_party/WebKit/Source/platform/loader/fetch/CachedMetadata.cpp
[modify] https://crrev.com/b9d88673a80c35f907482ea512582912b1bf1359/third_party/WebKit/Source/platform/loader/fetch/CachedMetadata.h

Project Member

Comment 2 by bugdroid1@chromium.org, Jan 5 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eed94eefc27d05d07072cffd0a72da372bb346ed

commit eed94eefc27d05d07072cffd0a72da372bb346ed
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Fri Jan 05 21:33:20 2018

Avoid shadowed parameters in GPU command buffer header files.

Add a "_" prefix (which is consistent with the namings in the files)
for parameter attributes for SetHeader/ComputeSize/ComputeDataSize

BUG=794619

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: Ia5fe1f145383fbd35a4cf00ab286b4f0ba859894
Reviewed-on: https://chromium-review.googlesource.com/852434
Reviewed-by: Victor Miura <vmiura@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527388}
[modify] https://crrev.com/eed94eefc27d05d07072cffd0a72da372bb346ed/gpu/command_buffer/build_gles2_cmd_buffer.py
[modify] https://crrev.com/eed94eefc27d05d07072cffd0a72da372bb346ed/gpu/command_buffer/common/cmd_buffer_common.h
[modify] https://crrev.com/eed94eefc27d05d07072cffd0a72da372bb346ed/gpu/command_buffer/common/gles2_cmd_format_autogen.h

Project Member

Comment 3 by bugdroid1@chromium.org, Jan 5 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4b798010116d7b5f3ec38581a323e185f65a7c23

commit 4b798010116d7b5f3ec38581a323e185f65a7c23
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Fri Jan 05 22:31:06 2018

Avoid shadowed parameters in media content/media files.

BUG=794619

Change-Id: I2e506128ebedb5e0bd0a10a7cbd29604a5cc6ebd
Reviewed-on: https://chromium-review.googlesource.com/852640
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527407}
[modify] https://crrev.com/4b798010116d7b5f3ec38581a323e185f65a7c23/content/browser/media/capture/aura_window_capture_machine.cc
[modify] https://crrev.com/4b798010116d7b5f3ec38581a323e185f65a7c23/content/browser/media/media_internals.cc
[modify] https://crrev.com/4b798010116d7b5f3ec38581a323e185f65a7c23/content/browser/renderer_host/media/audio_input_sync_writer.cc
[modify] https://crrev.com/4b798010116d7b5f3ec38581a323e185f65a7c23/content/browser/renderer_host/media/media_stream_manager.cc
[modify] https://crrev.com/4b798010116d7b5f3ec38581a323e185f65a7c23/content/browser/renderer_host/media/video_capture_manager.cc
[modify] https://crrev.com/4b798010116d7b5f3ec38581a323e185f65a7c23/content/renderer/media/media_stream_constraints_util.cc
[modify] https://crrev.com/4b798010116d7b5f3ec38581a323e185f65a7c23/content/renderer/media/media_stream_video_source.cc
[modify] https://crrev.com/4b798010116d7b5f3ec38581a323e185f65a7c23/content/renderer/media/rtc_peer_connection_handler.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c96ddb526391b3706a578d19f75b5cb607c072bb

commit c96ddb526391b3706a578d19f75b5cb607c072bb
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Wed Jan 10 21:57:46 2018

Fix shadowed variables in accessibility files.

Append "updated" prefix to some variables that were shadowed.
Adjust the name of other variables.

BUG=794619

Change-Id: If35d06010760cb9900cfda60f3360265da409ab0
Reviewed-on: https://chromium-review.googlesource.com/852932
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528438}
[modify] https://crrev.com/c96ddb526391b3706a578d19f75b5cb607c072bb/content/browser/accessibility/accessibility_tree_formatter_blink.cc
[modify] https://crrev.com/c96ddb526391b3706a578d19f75b5cb607c072bb/content/renderer/accessibility/render_accessibility_impl.cc
[modify] https://crrev.com/c96ddb526391b3706a578d19f75b5cb607c072bb/ui/accessibility/ax_position.h

Project Member

Comment 6 by bugdroid1@chromium.org, Jan 11 2018

Project Member

Comment 7 by bugdroid1@chromium.org, Feb 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/465eba706cd50b18f4fd862744b82037b3d535a7

commit 465eba706cd50b18f4fd862744b82037b3d535a7
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Fri Feb 16 17:42:12 2018

Fix shadowed variables in the content layer.

Shadowed variables can make code harder to read. Don't support them
in the content layer.

BUG=794619

This CL was uploaded by git cl split.

R=csharrison@chromium.org

Change-Id: Ia68095876eb2a6de4337cee243c0ae5b4c93ff8c
Reviewed-on: https://chromium-review.googlesource.com/924171
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537344}
[modify] https://crrev.com/465eba706cd50b18f4fd862744b82037b3d535a7/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/465eba706cd50b18f4fd862744b82037b3d535a7/content/browser/loader/signed_exchange_signature_verifier.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Feb 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/675a519188970f4722199f033867036fb250acb4

commit 675a519188970f4722199f033867036fb250acb4
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Fri Feb 16 18:26:42 2018

Fix shadowed variables in the content layer.

Shadowed variables can make code harder to read. Don't support them
in the content layer.

BUG=794619

This CL was uploaded by git cl split.

R=tdresser@chromium.org

Change-Id: Ic062c1ef20684790628223113ee62e577b374e9e
Reviewed-on: https://chromium-review.googlesource.com/924166
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537354}
[modify] https://crrev.com/675a519188970f4722199f033867036fb250acb4/content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Feb 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b7d3a7d92239d9ed9f97521b5359a657c03b234f

commit b7d3a7d92239d9ed9f97521b5359a657c03b234f
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Fri Feb 16 18:38:41 2018

Fix shadowed variables in the content layer.

Shadowed variables can make code harder to read. Don't support them
in the content layer.

BUG=794619

This CL was uploaded by git cl split.

R=paulmeyer@chromium.org

Change-Id: I0151100128e83c9746375ad898dac8bfc1a89e0e
Reviewed-on: https://chromium-review.googlesource.com/923345
Reviewed-by: Paul Meyer <paulmeyer@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537359}
[modify] https://crrev.com/b7d3a7d92239d9ed9f97521b5359a657c03b234f/content/browser/find_request_manager.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Feb 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4876a48f7db281c36d9f30175f19522967161a44

commit 4876a48f7db281c36d9f30175f19522967161a44
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Fri Feb 16 19:02:39 2018

Fix shadowed variables in the content layer.

Shadowed variables can make code harder to read. Don't support them
in the content layer.

BUG=794619

This CL was uploaded by git cl split.

R=bauerb@chromium.org

Change-Id: I5743df2f237a1dd0f88e880b96abf6a0aea3b1d3
Reviewed-on: https://chromium-review.googlesource.com/924165
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537367}
[modify] https://crrev.com/4876a48f7db281c36d9f30175f19522967161a44/content/common/plugin_list.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Feb 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1644fba4cd805be3281f4323c015154639d8fbaf

commit 1644fba4cd805be3281f4323c015154639d8fbaf
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Fri Feb 16 19:10:14 2018

Fix shadowed variables in the content layer.

Shadowed variables can make code harder to read. Don't support them
in the content layer.

BUG=794619

This CL was uploaded by git cl split.

R=haraken@chromium.org

Change-Id: I294dede7daefd705b508bb78c4bf0c2dedc8a2a9
Reviewed-on: https://chromium-review.googlesource.com/924170
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537370}
[modify] https://crrev.com/1644fba4cd805be3281f4323c015154639d8fbaf/content/child/child_thread_impl.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Feb 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/13157c316f008ad95936e0d5adcaab672e26c4ac

commit 13157c316f008ad95936e0d5adcaab672e26c4ac
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Fri Feb 16 19:18:54 2018

Fix shadowed variables in the content layer.

Shadowed variables can make code harder to read. Don't support them
in the content layer.

BUG=794619

This CL was uploaded by git cl split.

R=kbr@chromium.org

Change-Id: I90d8149cf7352e70793b2d4b7e94ffe33f3f7b72
Reviewed-on: https://chromium-review.googlesource.com/924162
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537372}
[modify] https://crrev.com/13157c316f008ad95936e0d5adcaab672e26c4ac/content/browser/gpu/gpu_data_manager_impl_private.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Feb 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e2e003827563803a38794069d4922a84fedfc703

commit e2e003827563803a38794069d4922a84fedfc703
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Fri Feb 16 19:51:47 2018

Fix shadowed variables in the content layer.

Shadowed variables can make code harder to read. Don't support them
in the content layer.

BUG=794619

This CL was uploaded by git cl split.

R=iclelland@chromium.org

Change-Id: I347990cab77ee1c23a63bb2eb9fb924758459621
Reviewed-on: https://chromium-review.googlesource.com/924164
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537389}
[modify] https://crrev.com/e2e003827563803a38794069d4922a84fedfc703/content/browser/background_sync/background_sync_manager.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Feb 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/acf33d8d6b9bca742dbe0e8711a4f3c4015e8c12

commit acf33d8d6b9bca742dbe0e8711a4f3c4015e8c12
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Fri Feb 16 19:54:11 2018

Fix shadowed variables in the content layer.

Shadowed variables can make code harder to read. Don't support them
in the content layer.

BUG=794619

This CL was uploaded by git cl split.

R=dmurph@chromium.org

Change-Id: Ie36fb07ec35d59a1bfbeb21773fe68b04ac14bf1
Reviewed-on: https://chromium-review.googlesource.com/923352
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537390}
[modify] https://crrev.com/acf33d8d6b9bca742dbe0e8711a4f3c4015e8c12/content/browser/dom_storage/local_storage_context_mojo.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Feb 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d7f70c8797cce3ac68a146f64dec8ec6597155a9

commit d7f70c8797cce3ac68a146f64dec8ec6597155a9
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Fri Feb 16 20:27:02 2018

Fix shadowed variables in the content layer.

Shadowed variables can make code harder to read. Don't support them
in the content layer.

BUG=794619

This CL was uploaded by git cl split.

R=michaeln@chromium.org

Change-Id: Ie7cb617a06ec2faf3ba1eb9d2a110f4c982793f8
Reviewed-on: https://chromium-review.googlesource.com/924167
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537403}
[modify] https://crrev.com/d7f70c8797cce3ac68a146f64dec8ec6597155a9/content/browser/service_worker/service_worker_database.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Feb 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/137732d58bdbe634d1526f3ec4902564c2b03994

commit 137732d58bdbe634d1526f3ec4902564c2b03994
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Sun Feb 18 14:49:14 2018

Fix shadowed variables in the content layer.

Shadowed variables can make code harder to read. Don't support them
in the content layer.

BUG=794619

This CL was uploaded by git cl split.

R=dalecurtis@chromium.org

Change-Id: Ida89064c5c1c68cf24708d2f6536569812507186
Reviewed-on: https://chromium-review.googlesource.com/924169
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537562}
[modify] https://crrev.com/137732d58bdbe634d1526f3ec4902564c2b03994/content/renderer/media/webrtc/rtc_video_decoder.cc

Project Member

Comment 17 by bugdroid1@chromium.org, Feb 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0f9bf3e4c67a45e52b9689fdd31361f90581ba01

commit 0f9bf3e4c67a45e52b9689fdd31361f90581ba01
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Sun Feb 18 15:36:54 2018

Fix shadowed variables in the content layer.

Shadowed variables can make code harder to read. Don't support them
in the content layer.

BUG=794619

This CL was uploaded by git cl split.

R=vmpstr@chromium.org

Change-Id: I683ff26a587f65c1339fda72c4b20877974925f1
Reviewed-on: https://chromium-review.googlesource.com/923344
Reviewed-by: vmpstr <vmpstr@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537563}
[modify] https://crrev.com/0f9bf3e4c67a45e52b9689fdd31361f90581ba01/content/renderer/gpu/actions_parser.cc

Project Member

Comment 18 by bugdroid1@chromium.org, Feb 18 2018

Project Member

Comment 19 by bugdroid1@chromium.org, Feb 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0269536ee08e0371c087567ff6aaceeb3dd68a20

commit 0269536ee08e0371c087567ff6aaceeb3dd68a20
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Mon Feb 19 01:03:42 2018

Fix shadowed variables in the content layer.

Shadowed variables can make code harder to read. Don't support them
in the content layer.

BUG=794619

This CL was uploaded by git cl split.

R=raymes@chromium.org

Change-Id: Ic204074056ad3a3a7060c520c34a0a6728340d3b
Reviewed-on: https://chromium-review.googlesource.com/923347
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537569}
[modify] https://crrev.com/0269536ee08e0371c087567ff6aaceeb3dd68a20/content/renderer/pepper/content_renderer_pepper_host_factory.cc
[modify] https://crrev.com/0269536ee08e0371c087567ff6aaceeb3dd68a20/content/renderer/pepper/pepper_graphics_2d_host.cc
[modify] https://crrev.com/0269536ee08e0371c087567ff6aaceeb3dd68a20/content/renderer/pepper/pepper_video_capture_host.cc
[modify] https://crrev.com/0269536ee08e0371c087567ff6aaceeb3dd68a20/content/renderer/pepper/ppb_var_deprecated_impl.cc

Project Member

Comment 20 by bugdroid1@chromium.org, Feb 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0111ab0776467c2aff137305bbedfda11c289886

commit 0111ab0776467c2aff137305bbedfda11c289886
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Mon Feb 19 16:44:07 2018

Fix shadowed variables in the content layer.

Shadowed variables can make code harder to read. Don't support them
in the content layer.

BUG=794619

This CL was uploaded by git cl split.

R=kinuko@chromium.org

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I496a5dbfd270826969deea1edb01dd572dff2d99
Reviewed-on: https://chromium-review.googlesource.com/923353
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537665}
[modify] https://crrev.com/0111ab0776467c2aff137305bbedfda11c289886/services/network/resource_scheduler.cc

Project Member

Comment 21 by bugdroid1@chromium.org, Feb 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/92d6cbee49a77b1ffa97693c56753939474372d6

commit 92d6cbee49a77b1ffa97693c56753939474372d6
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Mon Feb 19 17:40:32 2018

Fix shadowed variables in the content layer.

Shadowed variables can make code harder to read. Don't support them
in the content layer.

BUG=794619

This CL was uploaded by git cl split.

R=fsamuel@chromium.org

Change-Id: Id33101d8feb26d33e4b3c8301bfb496bd9535298
Reviewed-on: https://chromium-review.googlesource.com/924163
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537673}
[modify] https://crrev.com/92d6cbee49a77b1ffa97693c56753939474372d6/content/browser/renderer_host/delegated_frame_host.cc

Project Member

Comment 22 by bugdroid1@chromium.org, Feb 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/223fa7dcfbdc876bcd3a0bc5bc70fde78a44e4f6

commit 223fa7dcfbdc876bcd3a0bc5bc70fde78a44e4f6
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Tue Feb 20 19:51:34 2018

Fix shadowed variables in the content layer.

Shadowed variables can make code harder to read. Don't support them
in the content layer.

BUG=794619

This CL was uploaded by git cl split.

Change-Id: I89b52d89edbaff91c681700fa58bd591757abc28
Reviewed-on: https://chromium-review.googlesource.com/923346
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537854}
[modify] https://crrev.com/223fa7dcfbdc876bcd3a0bc5bc70fde78a44e4f6/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_template_definition.tmpl

Project Member

Comment 23 by bugdroid1@chromium.org, Feb 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/968a6a74b2f59921e1d1c3386a9684dccb10440e

commit 968a6a74b2f59921e1d1c3386a9684dccb10440e
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Tue Feb 20 19:58:16 2018

Fix shadowed variables in the content layer.

Shadowed variables can make code harder to read. Don't support them
in the content layer.

BUG=794619

Change-Id: I117ee483681a2788f93f544dee037dee856f7680
Reviewed-on: https://chromium-review.googlesource.com/926566
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537859}
[modify] https://crrev.com/968a6a74b2f59921e1d1c3386a9684dccb10440e/content/browser/accessibility/web_contents_accessibility_android.cc
[modify] https://crrev.com/968a6a74b2f59921e1d1c3386a9684dccb10440e/content/browser/find_request_manager.cc
[modify] https://crrev.com/968a6a74b2f59921e1d1c3386a9684dccb10440e/content/renderer/java/gin_java_function_invocation_helper.cc

Project Member

Comment 25 by bugdroid1@chromium.org, Feb 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e7490d6fcbb4e97c8ffaaaaf073901539e6fbf7c

commit e7490d6fcbb4e97c8ffaaaaf073901539e6fbf7c
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Thu Feb 22 00:29:51 2018

Fix shadowed variable in windows accessibility.

Variable doesn't need to be declared in the nested scope. Remove the
declaration.

BUG=794619

Change-Id: Ic1467168de36e74b1fc25dc2000b5d4269ef62aa
Reviewed-on: https://chromium-review.googlesource.com/929381
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538276}
[modify] https://crrev.com/e7490d6fcbb4e97c8ffaaaaf073901539e6fbf7c/content/browser/accessibility/accessibility_tree_formatter_win.cc

Project Member

Comment 26 by bugdroid1@chromium.org, Feb 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ab92a5e35f1ef5395a23c6b90c5dbc36578a614c

commit ab92a5e35f1ef5395a23c6b90c5dbc36578a614c
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Thu Feb 22 18:20:02 2018

Fix shadowed variables in mac specific code.

Rename variables that are defined to be another type.

BUG=794619

Change-Id: I9daa74f3b12f4ddb77a788be24108c350fd716dc
Reviewed-on: https://chromium-review.googlesource.com/929306
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538489}
[modify] https://crrev.com/ab92a5e35f1ef5395a23c6b90c5dbc36578a614c/content/browser/accessibility/browser_accessibility_cocoa.mm
[modify] https://crrev.com/ab92a5e35f1ef5395a23c6b90c5dbc36578a614c/content/browser/renderer_host/input/synthetic_gesture_target_mac.mm

Project Member

Comment 27 by bugdroid1@chromium.org, Feb 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d3f0eaf75015a6be7ca3e07f3c57a14964eff506

commit d3f0eaf75015a6be7ca3e07f3c57a14964eff506
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Thu Feb 22 21:02:12 2018

Disallow shadowed variables in content layer.

Prevent shadowed variables in use on the content layer.

BUG=794619

Change-Id: I88a4d85565e47e58aba20b16be8b6f7533290bc3
Reviewed-on: https://chromium-review.googlesource.com/927181
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538560}
[modify] https://crrev.com/d3f0eaf75015a6be7ca3e07f3c57a14964eff506/content/BUILD.gn

Cc: -chengx@chromium.org
Do we really want to to do this? Several people have evaluated -Wshadow over the years (including the author of comment 0 iirc) and the result has always been that fixing it requires tons of changes with surprisingly little benefits.
When I evaluated variable shadowing at Valve I found that about 2% of the variable shadowing warnings indicated bugs of some sort, with some additional percentage being confusing or inefficient. I doubt it would be anywhere near that high at Chrome, but that 2% figure might represent something about the behavior of code that is freshly written and not yet submitted.

I found that the /analyze warning for variable shadowing was tedious because it came too late - it needs to trigger when the code is written so that variable shadowing never occurs. With that in place my assumption is that staying -Wshadow clean in particular components should be easy and probably has some benefits (readability if nothing else).

If it turns out to be problematic then we should disable the warning again. Note that some variable shadowing warnings were reported in extreme-jumbo builds - that would be one possible source of annoying cost.

This is the blog post I wrote about static analysis at Valve, the one that talks about variable shadowing:

https://randomascii.wordpress.com/2013/06/24/two-years-and-thousands-of-bugs-of-static-analysis/

Comment 31 by brat...@opera.com, Mar 2 2018

Cc: brat...@opera.com
Jumbo builds got a couple (two?) shadow warnings this last week and in general that is a good thing. We want to know if variables suddenly shadow each other just in case the closest one isn't the one that is intended. But until some CQ bot covers jumbo compilations it would also mean more fire fighting so I'm torn between "want it" and "might not like the practical consequences".


We try to use a data-driven process for turning on warnings where we look at cost of warning and true positive rate, and 2% is a pretty low true positive rate. To me this is surprising for -Wshadow since it seems such an obvious thing to warn on, but the data always looks like it's a lot of work for not much gain.

If y'all want to work on this, I don't have a problem with that (as long as we don't turn it on for gcc, whose -Wshadow warns on this common idiom:

  struct S {
    S(int a) : a(a) {}
    int a;
  };
), just thought I'd mention that there might be higher-impact warnings out there :-)

Comment 33 by beccahughes@chromium.org, Jan 17 (5 days ago)

Blocking: 923078

Sign in to add a comment