Closed Bug 172937 Opened 17 years ago Closed Last year

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

Categories

(Firefox Build System :: Source Code Analysis, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: mstankus, Unassigned)

References

Details

(Keywords: student-project)

Attachments

(4 files, 5 obsolete files)

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 (scc@netscape.com) 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:
To build config?
Assignee: asa → seawood
Status: UNCONFIRMED → NEW
Component: Browser-General → Build Config
Ever confirmed: true
QA Contact: asa → granrose
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?
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.
Over to the language lawyer.
Assignee: seawood → scc
Product: Browser → Seamonkey
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.
Assignee: scc → nobody
Component: Build Config → General
Product: SeaMonkey → Core
QA Contact: granrosebugs → general
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.
Summary: Explicit keyword not used much in Cpp → Scan for constructors that could be explicit, but aren't
Component: General → Rewriting and Analysis
QA Contact: general → rewriting-and-analysis
-> 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?
Assignee: nobody → dwitte
Attached patch explicit.js analysis pass (obsolete) — Splinter Review
scans for constructors of a single argument (not counting |this|) of a different type to the base class, which isn't already annotated |explicit|.
adds member.isExplicit for constructors.
on mozilla-central this gives 116k warnings for nsCOMPtr constructors, 10k for the string classes, and 26k other.
Comment on attachment 386071 [details] [diff] [review]
dehydra patch to export |explicit|

but you need to add a testcase
Attachment #386071 - Flags: review+
Shouldn't you also check for the case where the constructor has additional parameters that have default values?
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?
(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.
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?
> Does this look useful?

please post your results too
build log (using grep -B 4) for temporary.js pass with annotated nsCOMPtr.
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.
(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.
Oof. Functions should never return nsCOMPtr<>, always already_AddRefed<>. Sounds like bugs to fix!
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.
Attachment #386070 - Attachment is obsolete: true
Attachment #386071 - Attachment is obsolete: true
Attached patch dehydra patch, v1 (obsolete) — Splinter Review
as described in previous comment.
Attachment #387986 - Attachment is obsolete: true
Attachment #388404 - Flags: review?(tglek)
Attached patch tree patch, v1Splinter Review
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.
Attachment #388405 - Flags: review?(benjamin)
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=.
Attachment #388404 - Flags: superreview?(benjamin)
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.
Attachment #388404 - Flags: review?(tglek) → review-
Comment on attachment 388404 [details] [diff] [review]
dehydra patch, v1

I don't need to review this part.
Attachment #388404 - Flags: superreview?(benjamin)
Attachment #388405 - Flags: review?(benjamin) → review+
Attached patch dehydra patch, v2 (obsolete) — Splinter Review
with testcases!
Attachment #388404 - Attachment is obsolete: true
Attachment #389215 - Flags: review?(tglek)
Attachment #388405 - Flags: superreview?(dbaron)
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.
Attachment #389215 - Flags: review?(tglek) → review+
Removes |hasDefault| array from the type if it's empty.
Attachment #389215 - Attachment is obsolete: true
(never mind test.cc and test.js, i should set up an ignore for those...)
Comment on attachment 388405 [details] [diff] [review]
tree patch, v1

sr=dbaron.  Sorry for the delay.
Attachment #388405 - Flags: superreview?(dbaron) → superreview+
Blocks: 530158
Reassigning to nobody. If anyone wants to work on this, feel free!
Assignee: dwitte → nobody
Greetings.
I'm new to FF development and would like to work on this bug.
Product: Core → Firefox Build System
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.
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.