172937 - Scan for constructors that could be explicit, but aren't

Status: RESOLVED WORKSFORME

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Mark Stankus]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20020905
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20020905


  In Cpp, the keyword "explicit" really helps for understanding code
and making it smaller and faster. Explicit is used to avoid temporary
variables and relates to constructors (with one argument which are 
not copy constructors). I changed nsInt64.h (in xp*/ds/) to add 
the explicit keyword in my copy of the code and it caused other
code to not compile (code in mozilla/netwerk in particular related
to nsTime.h). A/The solution in this case is to add more flavors of operator=.

Scott ([e-mail]) added explicit in the string classes and it
made a big difference there.


  As an example, the code

  nsInt64 A;  nsPRInt64 B;
  // code here
  A = B;

is really the code

  nsInt64 A;  PRInt64 B;
  // code here
  nsInt64 temp_for_here(B);
  A = temp_for_here;

Notice the extra constructor. 

Adding explicit should:
1) Reduce the footprint
2) Reduce run time.
3) Make the code more understandable (supressing conversion/casting/temporaries)

I recommend defining CPP_EXPLICIT to either explicit or nothing
depending on the build. (There is a NS_EXPLICIT which is in one
file). When CPP_EXPLICIT is explicit, we can compile and fix code.
If the issues are too overwhelming, set CPP_EXPLICIT to nothing
and the checking is avoided.

Note: I submitted this bug with a C then a plus sign and another plus sign
and it did not seem to make it.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Boris Zbarsky [:bzbarsky]]

To build config?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I noticed the nsInt64 problem a few days ago (see bug 170555 comment 53) due to
build bustage that it caused.  Are there other specific problems you've noticed?

Comment 3

[Mark Stankus]

I just picked a directory at random to decide whether or not there 
were problems with explicit. I found nsInt64. 
I think that it would be a good idea to write a program which 
reads files line-by-line, determines
which class you are in, finds constructors with one argument
(that includes cases like: class X { public: X(int a,int b=0); };)
and spits out code with the explicit or CPP_EXPLICIT added.
We can always "turn back the clock" by setting CPP_EXPLICIT
to the empty string. (See the original post)

The reason I have not worked at this is that the changes are 
trivial until you make a discovery of a bug and typing
explicit in each time is boring.

Comment 4

[hacker formerly known as [e-mail]]

Over to the language lawyer.

Comment 5

[Justin Wood (:Callek)]

Over to Core as this surely is not a SeaMonkey Build Config bug.

From comments this might be a cause to try taras automation work to further this ideal.

Comment 6

[(dormant account)]

I'll do up a script to produce a list of single-argument constructors(ie ones with default arguments) that could be annotated with explicit.

Comment 7

[[e-mail]]

-> Me, I'm rolling a pass for this. It seems like the worst offender by far is nsCOMPtr - apparently it used to have explicit annotations but no longer does - which certainly seems worthwhile fixing.

On the whole, though, I'm dubious about the value this will add. In optimized builds, helper classes like nsCOMPtr should generally be inlined and thus the cost of temporaries is optimized away. (Though with nsCOMPtr, this is not true iirc, due to AddRef and Release.) For more complex classes which aren't inlined, we probably don't care. Am I missing something?

Comment 8

[[e-mail]]

Attachment: explicit.js analysis pass

scans for constructors of a single argument (not counting |this|) of a different type to the base class, which isn't already annotated |explicit|.

Comment 9

[[e-mail]]

Attachment: dehydra patch to export |explicit|

adds member.isExplicit for constructors.

Comment 10

[[e-mail]]

on mozilla-central this gives 116k warnings for nsCOMPtr constructors, 10k for the string classes, and 26k other.

Comment 11

[(dormant account)]

Comment on attachment 386071 [details] [diff] [review]
dehydra patch to export |explicit|

but you need to add a testcase

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Shouldn't you also check for the case where the constructor has additional parameters that have default values?

Comment 13

[[e-mail]]

Yes, I should do that.

I tried making the constructors in nsCOMPtr explicit, but that prevents code of the form

nsCOMPtr<nsIFoo> foo = do_QueryInterface(...);

from compiling, since we're preventing the implicit construction. And we're not gaining anything, since nsCOMPtr already implements assignment operators to match its conversion constructors. These operators will be preferred over temporary construction as described in comment 0, so there's nothing to be gained there. For other classes that don't implement assignment operators, this isn't necessarily true.

So it sounds like we should exclude from consideration classes that implement assignment operators corresponding to their conversion constructors. Sound good?

Comment 14

[(dormant account)]

(In reply to comment #13)

> So it sounds like we should exclude from consideration classes that implement
> assignment operators corresponding to their conversion constructors. Sound
> good?

Sounds good to me.

Comment 15

[[e-mail]]

Attachment: prohibit unnamed temporaries (v1)

bsmedberg suggested that a more useful thing to enforce, particularly in the case of nsCOMPtr, is that no unnamed temporaries are ever instantiated. This would cover the case that comment 0 seeks to avoid, as well as explicit instantiations as a temporary variable, such as myFunction(nsCOMPtr<nsIFoo>(someRawPtr), ...).

This requires annotating the class with an attribute "NS_nontemporary", and is somewhat orthogonal to the |explicit| stuff, so we should keep it as a separate pass. Doing this for nsCOMPtr generated 61 warnings throughout the tree. If this looks reasonable, we could add more classes here. I'm guessing we don't want to do the string classes, or perhaps only for certain cases, since temporaries are heavily used there.

Does this look useful?

Comment 16

[(dormant account)]

> Does this look useful?

please post your results too

Comment 17

[[e-mail]]

Attachment: build log for temporary.js pass

build log (using grep -B 4) for temporary.js pass with annotated nsCOMPtr.

Comment 18

[Benjamin Smedberg]

The one at http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#446 is interesting... which side of that expression has a temporary nsCOMPtr?

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#2009 is definitely not good... the constructor takes a nsWeakPtr instead of a const nsWeakPtr&.

http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/IMETextTxn.h#78 has the same problem.

There actually appear to be quite a few of that pattern.

Is http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsWebShellWindow.cpp#611 a false positive? I can't see where a temporary would be created in that code, but the analysis warns on lines 617, 621, 626 as well.

Comment 19

[[e-mail]]

(In reply to comment #18)
> The one at
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#446
> is interesting... which side of that expression has a temporary nsCOMPtr?

It looks like the |do_QueryInterface(aWindow)| generates a temporary nsCOMPtr. I guess gcc wants the selectable arguments to ternary |operator?| to both be of type nsCOMPtr{&}, which forces the creation of a temporary for the QI case. (Interestingly, making the second argument a non-nsCOMPtr or non-nsQueryInterface type, such as nsQueryInterfaceWithError or just T*, makes gcc bail with 'no match for ternary operator?'. So it's clearly not evaluating operator? in terms of two separately matched operator= with a branch, and it can only handle one type-mismatched argument. Weird.)

I think, since it is actually generating a temporary here, that we want to rewrite the code to not use operator?.

> There actually appear to be quite a few of that pattern.

Will try to fix these.

> Is
> http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsWebShellWindow.cpp#611
> a false positive? I can't see where a temporary would be created in that code,
> but the analysis warns on lines 617, 621, 626 as well.

The function signature returns an |nsCOMPtr<nsIDOMDocument>|, so each |return| statement should warn.

Comment 20

[Benjamin Smedberg]

Oof. Functions should never return nsCOMPtr<>, always already_AddRefed<>. Sounds like bugs to fix!

Comment 21

[[e-mail]]

Attachment: dehydra patch and analysis pass for |explicit| (WIP)

This patch does a few things to dehydra:

1) exports |isExplicit|, per previous patch
2) exports |isArtificial| for function parameters such as |this|
3) exports |hasDefault| for parameters with default values

We could also report the actual value of the default if we wanted, but I haven't done that here.

This also updates the analysis pass to use the new magic, but it's not quite complete yet - still need to exclude classes with matching operator= methods.

Comment 22

[[e-mail]]

Attachment: dehydra patch, v1

as described in previous comment.

Comment 23

[[e-mail]]

Attachment: tree patch, v1

Fixes a few instances within xpcom/, for starters. Also adds some static-checker testcases. (I'm not sure if these should go in dehydra itself, xpcom, or both; let me know what you'd prefer.)

After these fixes, I get about 43k warnings across the tree. 11k of these are in ns*HashKey, 2k from nsAutoPtr, 2k from nsGenericElement, etc...

If this analysis looks useful, I can start going through them.

Comment 24

[[e-mail]]

Comment on attachment 388404 [details] [diff] [review]
dehydra patch, v1

Also, I should note the strategy this analysis uses. It's trying to find candidates for explicit, excluding cases where operator= would be preferred over calling the constructor (i.e. where explicit wouldn't make a difference). The rules I came up with for this are:

1) Constructor must be single-arg, not counting |this| and default args.
2) Must be public.
3) A public operator= taking the same arg type must not exist. (constness is considered when determining if arg types are compatible.)

Allowing the presence of an appropriate operator= to suppress the warning will let some cases slip through, I think - for instance, a function that returns a class by value will invoke a constructor implicitly, regardless of operator=.

Comment 25

[(dormant account)]

Comment on attachment 388404 [details] [diff] [review]
dehydra patch, v1


> const char *ARGUMENTS = "arguments";
>+const char *ARTIFICIAL = "isArtificial";

I try to not add functionality to dehydra if it can be obtained in other ways. Artificial stuff is questionable.

>+void dehydra_labelDefaults (Dehydra *this, JSObject *obj) {
I think if you are going to move the defaults, instead of keeping the weird .hasDefaults property, just delete that property.
>+  jsval val;
>+
>+  JS_GetProperty(this->cx, obj, TYPE, &val);
>+  if (val == JSVAL_VOID) return;
>+  JSObject *type_obj = JSVAL_TO_OBJECT(val);
>+
>+  JS_GetProperty(this->cx, type_obj, HAS_DEFAULT, &val);
>+  if (val == JSVAL_VOID) return;
>+  JSObject *defaults_obj = JSVAL_TO_OBJECT(val);
>+
>+  JS_GetProperty(this->cx, obj, PARAMETERS, &val);
>+  if (val == JSVAL_VOID) return;
>+  JSObject *params_obj = JSVAL_TO_OBJECT(val);
>+
>+  jsuint length;
>+  JS_GetArrayLength(this->cx, params_obj, &length);
>+  int i, j = 0;
>+  for (i = 0; i < length; ++i) {
>+    JS_GetElement(this->cx, params_obj, i, &val);
>+    JSObject *param_obj = JSVAL_TO_OBJECT(val);
>+
>+    // check if the param is artificial, and skip if so:
>+    // artificial params aren't called out in the function type
>+    JS_GetProperty(this->cx, param_obj, ARTIFICIAL, &val);

Instead of using isArtificial, check if the function is a non-static member.


The rest of the patch looks pretty good, but you need to add testcases for any new properties.

Comment 26

[Benjamin Smedberg]

Comment on attachment 388404 [details] [diff] [review]
dehydra patch, v1

I don't need to review this part.

Comment 27

[[e-mail]]

Attachment: dehydra patch, v2

with testcases!

Comment 28

[(dormant account)]

Comment on attachment 389215 [details] [diff] [review]
dehydra patch, v2


>   /* return type */
>   dehydra_defineProperty (this, obj, TYPE, 
>                           dehydra_convert_type (this, TREE_TYPE (type)));  
>   JSObject *params = JS_NewArrayObject (this->cx, 0, NULL);
>+  JSObject *defaults = JS_NewArrayObject (this->cx, 0, NULL);
>   dehydra_defineProperty (this, obj, PARAMETERS, OBJECT_TO_JSVAL (params));
>+  dehydra_defineProperty (this, obj, HAS_DEFAULT, OBJECT_TO_JSVAL (defaults));
>   int i = 0;
>   while (arg_type && (arg_type != void_list_node))
>     {
>-      JS_DefineElement(this->cx, params, i++,
>-                       dehydra_convert_type (this, TREE_VALUE (arg_type)),      
>+      JS_DefineElement(this->cx, params, i,
>+                       dehydra_convert_type (this, TREE_VALUE (arg_type)),
>                        NULL, NULL, JSPROP_ENUMERATE);
>+      JS_DefineElement(this->cx, defaults, i,
>+                       TREE_PURPOSE(arg_type) ? JSVAL_TRUE : JSVAL_FALSE,
>+                       NULL, NULL, JSPROP_ENUMERATE);
>+      i++;
>       arg_type = TREE_CHAIN (arg_type);
>     }

Should drop the property if no defaults were found. Looks like otherwise you'll end up with a hasdefaults property on every function type.

Rest looks good r+ for now, but submit a fixup patch for dropping the property.

Comment 29

[[e-mail]]

Attachment: dehydra patch, v3

Removes |hasDefault| array from the type if it's empty.

Comment 30

[[e-mail]]

(never mind test.cc and test.js, i should set up an ignore for those...)

Comment 31

[[e-mail]]

http://hg.mozilla.org/rewriting-and-analysis/dehydra/rev/50232b7c06d9

dehydra bits landed.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 388405 [details] [diff] [review]
tree patch, v1

sr=dbaron.  Sorry for the delay.

Comment 33

[[e-mail]]

Reassigning to nobody. If anyone wants to work on this, feel free!

Comment 34

[Avikalpa Kundu [:kalpa]]

Greetings.
I'm new to FF development and would like to work on this bug.

Comment 35

[Nathan Froyd [:froydnj]]

We have an analysis now that requires explicit on single-argument constructors, and you need to opt-in for implicit single-arg constructors.  I think we can consider this done.