User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090310 Minefield/3.2a1pre Build Identifier: In bug 58904, we want to deprecate PR_NewLock() in favor of nsLock::NewLock() (and similarly for PR_Monitor/nsMonitor). It would great to have a simple analysis that warned or error'd when it came across these uses. Reproducible: Always

Ben T. pointed out that we already have an NS_DEPRECATED decl attribute that will result in warnings on at least gcc. This should work for most cases. The proposed analysis is intended to complement this attribute in two ways: (1) when the function is not entirely deprecated, but shouldn't be used outside of a particular code module/class (as in bug 58904); and (2) for code patterns that are too complicated to detect by just putting an NS_DEPRECATED attribute on a decl.

Are you planning on writing the analysis? It doesn't sound hard, and I'd love to get more people involved with writing simple analysis passes.

Yes. The first check I'm adding is quite trivial, so it'll be done in a few minutes.

I'll wait for review until I figure out how to integrate this into the build.

Benjamin: should I track this with bug 430328? I feel almost guilty putting this next to "serious" analyses like outparams and stack, as this code is more like a rich grep.

(In reply to comment #5) > Benjamin: should I track this with bug 430328? I feel almost guilty putting > this next to "serious" analyses like outparams and stack, as this code is more > like a rich grep. yes. I have no intention of excluding simpler analyses. I think in the longer term simpler ones are the winning ones as they encourage people to get into analyzing their code.

Since this "analysis" does no more at this point than essentially "grep -rn PR_Lock mozilla-central/* | grep -v nsLock", I'm going to wait until we have a more compelling use for seeking review etc.

Updated to recommend newly-renamed API.

Comment on attachment 371558 [details] [diff] [review] Checker, v3 >diff --git a/config/static-checking-config.mk b/config/static-checking-config.mk >--- a/config/static-checking-config.mk >+++ b/config/static-checking-config.mk >@@ -7,16 +7,17 @@ DEHYDRA_MODULES = \ > $(topsrcdir)/xpcom/analysis/final.js \ > $(topsrcdir)/layout/generic/frame-verify.js \ > $(NULL) > > TREEHYDRA_MODULES = \ > $(topsrcdir)/xpcom/analysis/outparams.js \ > $(topsrcdir)/xpcom/analysis/stack.js \ > $(topsrcdir)/xpcom/analysis/flow.js \ >+ $(topsrcdir)/xpcom/analysis/deprecated-warn.js \ > $(topsrcdir)/js/src/jsstack.js \ > $(NULL) > > DEHYDRA_ARGS = \ > --topsrcdir=$(topsrcdir) \ > --objdir=$(DEPTH) \ > --dehydra-modules=$(subst $(NULL) ,$(COMMA),$(strip $(DEHYDRA_MODULES))) \ > --treehydra-modules=$(subst $(NULL) ,$(COMMA),$(strip $(TREEHYDRA_MODULES))) \ >diff --git a/xpcom/analysis/deprecated-warn.js b/xpcom/analysis/deprecated-warn.js >new file mode 100644 >--- /dev/null >+++ b/xpcom/analysis/deprecated-warn.js >@@ -0,0 +1,182 @@ >+/** >+ * This analysis warns about uses of deprecated APIs or code patterns. >+ */ >+ >+require ({ after_gcc_pass: 'cfg' }); >+ >+include ('gcc_print.js'); >+include ('gcc_util.js'); >+include ('treehydra.js'); >+include ('unstable/analysis.js'); >+include ('util.js'); >+ >+//----------------------------------------------------------------------------- >+// Configuration >+// >+ >+/** Should instances of deprecated code be warnings or errors? */ >+let DEPRECATED_IS_ERROR = false; >+ >+/** Warn when we come across nodes that DeprecatedCodeCheck doesn't >+ * understand? */ >+let WARN_NYI = false; >+ >+/** How much detail to see on PartialDeprecation checks. */ >+let TRACE_PARTIAL_DEPRECATED = 0; s/let/const/ here. >+ >+//----------------------------------------------------------------------------- >+// Check dispatcher, with convenience functions. >+// >+function DeprecatedCodeCheck () { >+ this.onFundeclDudes = []; >+ this.onCallexprDudes = []; >+} >+DeprecatedCodeCheck.prototype = new Object; don't need that >+ >+DeprecatedCodeCheck.prototype.deprecate = function (why, node) { >+ let msg = 'deprecated code: '+ why; >+ let loc = node ? location_of (node) : undefined; >+ if (DEPRECATED_IS_ERROR) >+ error (msg, loc); >+ else >+ warning (msg, loc); >+}; >+ >+DeprecatedCodeCheck.prototype.addListener = function (evt, handler) { >+ this[evt+'Dudes'].push (handler); >+}; to save some typing you can do DeprecatedCodeCheck.prototype = { deprecate: function(..) {}, addListener: function(..) {} } also, addListener smells too much like Java :)

I don't think we want to go through with something like this.