Patchwork check-code: reject module-level @cachefunc

login
register
mail settings
Submitter via Mercurial-devel
Date Jan. 13, 2017, 6:14 p.m.
Message ID <837c16d22b9fc6c27979.1484331272@martinvonz.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/18210/
State Accepted
Headers show

Comments

via Mercurial-devel - Jan. 13, 2017, 6:14 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1484331097 28800
#      Fri Jan 13 10:11:37 2017 -0800
# Node ID 837c16d22b9fc6c2797978ade5b72688d79c862e
# Parent  c390b40fe1d7d109f2b524356ce43013e570d294
check-code: reject module-level @cachefunc

Module-level @cachefunc usage is risky because it can easily create a
memory "leak". Let's reject it completely for now. If a valid usage
comes up in the future, we can always improve the check or reconsider.
Sean Farley - Jan. 13, 2017, 7:52 p.m.
Martin von Zweigbergk via Mercurial-devel
<mercurial-devel@mercurial-scm.org> writes:

> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1484331097 28800
> #      Fri Jan 13 10:11:37 2017 -0800
> # Node ID 837c16d22b9fc6c2797978ade5b72688d79c862e
> # Parent  c390b40fe1d7d109f2b524356ce43013e570d294
> check-code: reject module-level @cachefunc
>
> Module-level @cachefunc usage is risky because it can easily create a
> memory "leak". Let's reject it completely for now. If a valid usage
> comes up in the future, we can always improve the check or reconsider.

Soundn fine to me. Looks good to me!
Yuya Nishihara - Jan. 14, 2017, 6:14 a.m.
On Fri, 13 Jan 2017 11:52:01 -0800, Sean Farley wrote:
> Martin von Zweigbergk via Mercurial-devel
> <mercurial-devel@mercurial-scm.org> writes:
> 
> > # HG changeset patch
> > # User Martin von Zweigbergk <martinvonz@google.com>
> > # Date 1484331097 28800
> > #      Fri Jan 13 10:11:37 2017 -0800
> > # Node ID 837c16d22b9fc6c2797978ade5b72688d79c862e
> > # Parent  c390b40fe1d7d109f2b524356ce43013e570d294
> > check-code: reject module-level @cachefunc
> >
> > Module-level @cachefunc usage is risky because it can easily create a
> > memory "leak". Let's reject it completely for now. If a valid usage
> > comes up in the future, we can always improve the check or reconsider.
> 
> Soundn fine to me. Looks good to me!

Sure, queued, thanks.

Patch

diff -r c390b40fe1d7 -r 837c16d22b9f contrib/check-code.py
--- a/contrib/check-code.py	Wed Jan 11 21:47:19 2017 -0500
+++ b/contrib/check-code.py	Fri Jan 13 10:11:37 2017 -0800
@@ -325,6 +325,7 @@ 
     # XXX only catch mutable arguments on the first line of the definition
     (r'def.*[( ]\w+=\{\}', "don't use mutable default arguments"),
     (r'\butil\.Abort\b', "directly use error.Abort"),
+    (r'^@(\w*\.)?cachefunc', "module-level @cachefunc is risky, please avoid"),
     (r'^import Queue', "don't use Queue, use util.queue + util.empty"),
     (r'^import cStringIO', "don't use cStringIO.StringIO, use util.stringio"),
     (r'^import urllib', "don't use urllib, use util.urlreq/util.urlerr"),