Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1564403003:
Remove readability/inheritance from the lint blacklist (Closed)
|
Created:
4 years, 11 months ago by danakj Modified:
4 years, 11 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionRemove readability/inheritance from the lint blacklist
Now that we're using real override/final and not also using
virtual on the same methods, this transitional blacklist
can go away.
R=agable, dcheng
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=298201
Patch Set 1 #
Messages
Total messages: 18 (6 generated)
This was added in https://codereview.chromium.org/627163002 > This check never triggered before, since cpplint.py was looking for > override not OVERRIDE. Since Chromium prefers override now, the > linter is now (correctly) warning. However, the old Chromium convention > (and the one implemented by the clang plugin) is to explicitly annotate > all overrides with both virtual and override. The clang plugin code has > been updated, but new binaries have not yet been built. Until clang > rolls, suppress the warning. This should be done now :) We're only using "override" not "virtual"+"override".
Description was changed from ========== Remove readability/inheritance from the lint blacklist Now that we're using real override/final this transitional blacklist can go away. R=agable, dcheng ========== to ========== Remove readability/inheritance from the lint blacklist Now that we're using real override/final and not also using virtual on the same methods, this transitional blacklist can go away. R=agable, dcheng ==========
Is there an easy way to try linting some random files to make sure it won't fire a bunch? Mostly I'm concerned about: - Files in //third_party/WebKit - Files that implement things from //third_party/WebKit
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1564403003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1564403003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2016/01/08 23:54:17, dcheng wrote: > Is there an easy way to try linting some random files to make sure it won't fire > a bunch? Mostly I'm concerned about: > - Files in //third_party/WebKit Not sure which things you're worried about testing here but I tried third_party/WebKit/public/platform/WebGraphicsContext3D.h which has a bunch of pure virtual things that get overridden in chromium and it's fine. > - Files that implement things from //third_party/WebKit Are you looking for things that don't have an override on them at all? Or what in particular with this one? I also tried cc/test/test_web_graphics_context_3d.h which overrides things from WGC3D and does so with virtual but not override, and it's fine. Let me know what to look for and I can try more.
dcheng@chromium.org changed reviewers: + avi@chromium.org
On 2016/01/09 at 00:11:45, danakj wrote: > On 2016/01/08 23:54:17, dcheng wrote: > > Is there an easy way to try linting some random files to make sure it won't fire > > a bunch? Mostly I'm concerned about: > > - Files in //third_party/WebKit > > Not sure which things you're worried about testing here but I tried third_party/WebKit/public/platform/WebGraphicsContext3D.h which has a bunch of pure virtual things that get overridden in chromium and it's fine. > > > - Files that implement things from //third_party/WebKit > > Are you looking for things that don't have an override on them at all? Or what in particular with this one? > > I also tried cc/test/test_web_graphics_context_3d.h which overrides things from WGC3D and does so with virtual but not override, and it's fine. > > Let me know what to look for and I can try more. I know the style plugin didn't used to enforce virtual/override/final stuff in Blink, so Blink had redundant annotations in some places. I think avi@ might have fixed this though, so it might be irrelevant. +avi to confirm.
(To be clear, this LGTM as long as Blink isn't a problem)
On Fri, Jan 8, 2016 at 4:16 PM, <dcheng@chromium.org> wrote: > On 2016/01/09 at 00:11:45, danakj wrote: > >> On 2016/01/08 23:54:17, dcheng wrote: >> > Is there an easy way to try linting some random files to make sure it >> won't >> > fire > >> > a bunch? Mostly I'm concerned about: >> > - Files in //third_party/WebKit >> > > Not sure which things you're worried about testing here but I tried >> > third_party/WebKit/public/platform/WebGraphicsContext3D.h which has a > bunch of > pure virtual things that get overridden in chromium and it's fine. > > > - Files that implement things from //third_party/WebKit >> > > Are you looking for things that don't have an override on them at all? Or >> what >> > in particular with this one? > > I also tried cc/test/test_web_graphics_context_3d.h which overrides things >> > from WGC3D and does so with virtual but not override, and it's fine. > > Let me know what to look for and I can try more. >> > > I know the style plugin didn't used to enforce virtual/override/final > stuff in > Blink, so Blink had redundant annotations in some places. I think avi@ > might > have fixed this though, so it might be irrelevant. +avi to confirm. > There are a few in blink, but linter warning there seems fine and if people change the file they can fix them. To give an idea of the scale, "git gs \ virtual\ .*\ override" found a total of 2 offenders in blink. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm stampity stamp
owner LGTM!
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1564403003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1564403003/1
Message was sent while issue was closed.
Description was changed from ========== Remove readability/inheritance from the lint blacklist Now that we're using real override/final and not also using virtual on the same methods, this transitional blacklist can go away. R=agable, dcheng ========== to ========== Remove readability/inheritance from the lint blacklist Now that we're using real override/final and not also using virtual on the same methods, this transitional blacklist can go away. R=agable, dcheng Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=298201 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as http://src.chromium.org/viewvc/chrome?view=rev&revision=298201 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
