Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
504503002:
Add a tool to look for source files that have unneeded grit header includes. (Closed)
|
Created:
6 years, 4 months ago by Lei Zhang Modified:
6 years, 3 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd a tool to look for source files that have unneeded grit header includes.
NOTRY=true
Committed: https://crrev.com/5312633de2202a2c1018c9ed56a951e384107db7
Cr-Commit-Position: refs/heads/master@{#291984}
Patch Set 1 #
Total comments: 22
Patch Set 2 : address comments #Patch Set 3 : look through else tags too #Messages
Total messages: 12 (0 generated)
There are some false positives like content/content_resources.grd vs content/app/resources/content_resources.grd, but I'm trying to fix that as a separate issue.
That's pretty long, compared to the shell 3-liner. I suppose it's more precise though. https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py File tools/unused-grit-header.py (right): https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:6: """A tool to scan source files for unneeded grit includes.""" Include a usage line https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:11: 2 empty lines between toplevels https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:13: print prog_name, 'GRD_FILE DIRS_TO_SCAN' Consider using optparse or argparse (see e.g. tools/sort-headers.py) https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:74: break Since you have this for loop 3 times, consider having a FindNodeWithTag(release_node, 'includes') function https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:96: elif child.tag == 'if': do you need to look at 'else' children of if nodes? I think someone added else nodes to grit somewhat recently. (Is there a way to ask grit for this stuff instead of doing the parsing yourself?) https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:100: assert output_file == None nit: it's more common to see "output_file is None" for comparisons with None (also in line 95) https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:121: break Maybe `assert not output_node` before the assignment instead of breaking, to make sure there's only one outputs? https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:131: add a newline https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:188: add newline https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:190: main(sys.argv) nit: the usual thing is `sys.exit(main(sys.argv))`, then main() can return error codes
https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py File tools/unused-grit-header.py (right): https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:6: """A tool to scan source files for unneeded grit includes.""" On 2014/08/24 23:58:25, Nico (hiding) wrote: > Include a usage line Usage() is literally 6 lines below... https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:11: On 2014/08/24 23:58:26, Nico (hiding) wrote: > 2 empty lines between toplevels Done. https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:13: print prog_name, 'GRD_FILE DIRS_TO_SCAN' On 2014/08/24 23:58:25, Nico (hiding) wrote: > Consider using optparse or argparse (see e.g. tools/sort-headers.py) There's no options. If we do add options later, I'll switch over. https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:74: break On 2014/08/24 23:58:25, Nico (hiding) wrote: > Since you have this for loop 3 times, consider having a > FindNodeWithTag(release_node, 'includes') function Done. https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:96: elif child.tag == 'if': On 2014/08/24 23:58:26, Nico (hiding) wrote: > do you need to look at 'else' children of if nodes? I think someone added else > nodes to grit somewhat recently. (Is there a way to ask grit for this stuff > instead of doing the parsing yourself?) I don't think grit has <else> nodes? It may be possible to use grit rather than parsing myself, but I'm not sure if that's more painful or not. https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:100: assert output_file == None On 2014/08/24 23:58:25, Nico (hiding) wrote: > nit: it's more common to see "output_file is None" for comparisons with None > (also in line 95) Done. https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:121: break On 2014/08/24 23:58:26, Nico (hiding) wrote: > Maybe `assert not output_node` before the assignment instead of breaking, to > make sure there's only one outputs? Done. And elsewhere above. https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:131: On 2014/08/24 23:58:26, Nico (hiding) wrote: > add a newline Done. https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:188: On 2014/08/24 23:58:25, Nico (hiding) wrote: > add newline Done. https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:190: main(sys.argv) On 2014/08/24 23:58:25, Nico (hiding) wrote: > nit: the usual thing is `sys.exit(main(sys.argv))`, then main() can return > error codes Done.
https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py File tools/unused-grit-header.py (right): https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:13: print prog_name, 'GRD_FILE DIRS_TO_SCAN' On 2014/08/26 02:35:24, Lei Zhang wrote: > On 2014/08/24 23:58:25, Nico (hiding) wrote: > > Consider using optparse or argparse (see e.g. tools/sort-headers.py) > > There's no options. If we do add options later, I'll switch over. It also offers usage printing (you can hand it the module docstring as help, etc) https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... tools/unused-grit-header.py:96: elif child.tag == 'if': On 2014/08/26 02:35:25, Lei Zhang wrote: > On 2014/08/24 23:58:26, Nico (hiding) wrote: > > do you need to look at 'else' children of if nodes? I think someone added else > > nodes to grit somewhat recently. (Is there a way to ask grit for this stuff > > instead of doing the parsing yourself?) > > I don't think grit has <else> nodes? see https://codereview.chromium.org/11155024
On Mon, Aug 25, 2014 at 7:43 PM, <thakis@chromium.org> wrote: > > https://codereview.chromium.org/504503002/diff/1/tools/ > unused-grit-header.py > File tools/unused-grit-header.py (right): > > https://codereview.chromium.org/504503002/diff/1/tools/ > unused-grit-header.py#newcode13 > tools/unused-grit-header.py:13: print prog_name, 'GRD_FILE DIRS_TO_SCAN' > On 2014/08/26 02:35:24, Lei Zhang wrote: > >> On 2014/08/24 23:58:25, Nico (hiding) wrote: >> > Consider using optparse or argparse (see e.g. tools/sort-headers.py) >> > > There's no options. If we do add options later, I'll switch over. >> > > It also offers usage printing (you can hand it the module docstring as > help, etc) (this one's optional of course – keep as is if you like that more) > > > https://codereview.chromium.org/504503002/diff/1/tools/ > unused-grit-header.py#newcode96 > tools/unused-grit-header.py:96: elif child.tag == 'if': > On 2014/08/26 02:35:25, Lei Zhang wrote: > >> On 2014/08/24 23:58:26, Nico (hiding) wrote: >> > do you need to look at 'else' children of if nodes? I think someone >> > added else > >> > nodes to grit somewhat recently. (Is there a way to ask grit for >> > this stuff > >> > instead of doing the parsing yourself?) >> > > I don't think grit has <else> nodes? >> > > see https://codereview.chromium.org/11155024 > > https://codereview.chromium.org/504503002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/26 02:43:38, Nico (hiding) wrote: > https://codereview.chromium.org/504503002/diff/1/tools/unused-grit-header.py#... > tools/unused-grit-header.py:96: elif child.tag == 'if': > On 2014/08/26 02:35:25, Lei Zhang wrote: > > On 2014/08/24 23:58:26, Nico (hiding) wrote: > > > do you need to look at 'else' children of if nodes? I think someone added > else > > > nodes to grit somewhat recently. (Is there a way to ask grit for this stuff > > > instead of doing the parsing yourself?) > > > > I don't think grit has <else> nodes? > > see https://codereview.chromium.org/11155024 Huh, we should start using it then. Ok, patch set 3 looks through the else tags too.
lgtm
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/thestig@chromium.org/504503002/40001
Message was sent while issue was closed.
Committed patchset #3 (40001) as 2b14a70c710d9e380ea50a963b7860bb8b464bfe
Message was sent while issue was closed.
lgtm (rubber-stamp).
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5312633de2202a2c1018c9ed56a951e384107db7 Cr-Commit-Position: refs/heads/master@{#291984} |
