Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2532583002:
Presubmit: Warn about useless forward declarations (Closed)
DescriptionPresubmit: Warn about useless forward declarations
Checks that added or removed lines in affected header files
do not lead to new useless class or struct forward declaration.
BUG=662195
TEST=PRESUBMIT_test.py ForwardDeclarationTest
Committed: https://crrev.com/9e12f16d49b411b9375e3f9eed9a639081c6aac2
Cr-Commit-Position: refs/heads/master@{#434449}
Patch Set 1 #Patch Set 2 : Original patch #Patch Set 3 : Changed version #
Messages
Total messages: 21 (9 generated)
jbriance@cisco.com changed reviewers: + asvitkine@google.com, dcheng@chromium.org, jochen@chromium.org
Better version of the presubmit check. Deleted header files are handled, and only added or removed lines in a header should trigger the warning.
Honestly, I'm OK with the aggressive version of this check (checking all forward declares, even if they weren't touched in the diff), but before we land the presubmit, it would be good to land a project-wide cleanup. Otherwise, this breaks a lot of random patches in progress that otherwise would have passed the presubmit.
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
If you're re-landing a patch, it's customary to upload patch set 1 as the original patch and then your changed version in the next patch set. This way, it's easy to review what's different between the two.
On 2016/11/24 20:37:39, dcheng wrote: > Honestly, I'm OK with the aggressive version of this check (checking all > forward declares, even if they weren't touched in the diff), but before we > land the presubmit, it would be good to land a project-wide cleanup. Work is ongoing, but I didn't process the chrome/ part yet, where there are more than 880 useless forward declaration. > Otherwise, this breaks a lot of random patches in progress that otherwise > would have passed the presubmit. I understand. To be honest, I wanted to do a presubmit like this one in the first place, but I thought it was too complex to implement. On 2016/11/24 21:23:44, Alexei Svitkine (slow) wrote: > If you're re-landing a patch, it's customary to upload patch set 1 as the > original patch and then your changed version in the next patch set. This > way, it's easy to review what's different between the two. Indeed, that would have been easier to review. Do you want me to do that in this CL ? (i.e send a 2nd patch set that reverts the 1st, send 3rd patch set which is the old one and the current one as patch set 4)
Yes please - but I don't think you need a patch that reverts it - you can just upload the original CL as patchset 2 and then re-upload the new version as patchset 3. On Thu, Nov 24, 2016 at 4:42 PM, <jbriance@cisco.com> wrote: > On 2016/11/24 20:37:39, dcheng wrote: > > Honestly, I'm OK with the aggressive version of this check (checking all > > forward declares, even if they weren't touched in the diff), but before > we > > land the presubmit, it would be good to land a project-wide cleanup. > > Work is ongoing, but I didn't process the chrome/ part yet, where there > are more > than 880 useless forward declaration. > > > Otherwise, this breaks a lot of random patches in progress that otherwise > > would have passed the presubmit. > > I understand. To be honest, I wanted to do a presubmit like this one in the > first place, but I thought it was too complex to implement. > > > On 2016/11/24 21:23:44, Alexei Svitkine (slow) wrote: > > If you're re-landing a patch, it's customary to upload patch set 1 as the > > original patch and then your changed version in the next patch set. This > > way, it's easy to review what's different between the two. > > Indeed, that would have been easier to review. Do you want me to do that > in this > CL ? (i.e send a 2nd patch set that reverts the 1st, send 3rd patch set > which is > the old one and the current one as patch set 4) > > https://codereview.chromium.org/2532583002/ > -- 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.
On 2016/11/24 21:44:41, chromium-reviews wrote: > Yes please - but I don't think you need a patch that reverts it - you can > just upload the original CL as patchset 2 and then re-upload the new > version as patchset 3. Done
On 2016/11/24 21:42:28, jbriance wrote: > On 2016/11/24 20:37:39, dcheng wrote: > > Honestly, I'm OK with the aggressive version of this check (checking all > > forward declares, even if they weren't touched in the diff), but before we > > land the presubmit, it would be good to land a project-wide cleanup. > > Work is ongoing, but I didn't process the chrome/ part yet, where there are more > than 880 useless forward declaration. Hm, I didn't hit any in //chrome but I did run into some in //content and //third_party/WebKit, if that's a useful data point. (I don't expect this to be globally fixed due to races between CLs, but I was surprised at how often my relatively small CL hit this right after the original CL landed) > > > Otherwise, this breaks a lot of random patches in progress that otherwise > > would have passed the presubmit. > > I understand. To be honest, I wanted to do a presubmit like this one in the > first place, but I thought it was too complex to implement. > > > On 2016/11/24 21:23:44, Alexei Svitkine (slow) wrote: > > If you're re-landing a patch, it's customary to upload patch set 1 as the > > original patch and then your changed version in the next patch set. This > > way, it's easy to review what's different between the two. > > Indeed, that would have been easier to review. Do you want me to do that in this > CL ? (i.e send a 2nd patch set that reverts the 1st, send 3rd patch set which is > the old one and the current one as patch set 4)
lgtm
The CQ bit was checked by jbriance@cisco.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 jbriance@cisco.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480060466034570,
"parent_rev": "b7eef73a607ae6dc7186e393323723b783dc680e", "commit_rev":
"fea47f97219b0e977240fd6fa59d56313a493148"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Presubmit: Warn about useless forward declarations Checks that added or removed lines in affected header files do not lead to new useless class or struct forward declaration. BUG=662195 TEST=PRESUBMIT_test.py ForwardDeclarationTest ========== to ========== Presubmit: Warn about useless forward declarations Checks that added or removed lines in affected header files do not lead to new useless class or struct forward declaration. BUG=662195 TEST=PRESUBMIT_test.py ForwardDeclarationTest Committed: https://crrev.com/9e12f16d49b411b9375e3f9eed9a639081c6aac2 Cr-Commit-Position: refs/heads/master@{#434449} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9e12f16d49b411b9375e3f9eed9a639081c6aac2 Cr-Commit-Position: refs/heads/master@{#434449}
Message was sent while issue was closed.
On 2016/11/25 00:09:51, dcheng wrote: > Hm, I didn't hit any in //chrome but I did run into some in //content and > //third_party/WebKit, if that's a useful data point. Oh these useless parts grow fast, I cleaned up //content less than 3 weeks ago (https://codereview.chromium.org/2473003002) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
