Closed Bug 1015430 Opened 5 years ago Closed 5 years ago

Fix more XPCOM constructors to clarify whether they should be explicit

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file)

No description provided.
Assignee: nobody → ehsan
Blocks: explicit
Attachment #8428003 - Flags: review?(nfroyd)
Comment on attachment 8428003 [details] [diff] [review]
Fix more XPCOM constructors to clarify whether they should be explicit

Review of attachment 8428003 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/BackgroundImpl.cpp
@@ +926,5 @@
>  
>      MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(runnable)));
>    }
>  
> +  return already_AddRefed<ContentParent>(actor->mContent.get());

This seems really dodgy, but I guess it's what the code already does, so...

::: xpcom/string/public/nsTString.h
@@ +58,1 @@
>    nsTString_CharT( const substring_tuple_type& tuple )

Does MOZ_IMPLICIT nsTString_CharT(...) overflow a line or something?  Can you put the MOZ_IMPLICIT on the same line if not?

@@ +529,1 @@
>    nsTAutoString_CharT( const substring_tuple_type& tuple )

Here too.
Attachment #8428003 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #2)
> Comment on attachment 8428003 [details] [diff] [review]
> Fix more XPCOM constructors to clarify whether they should be explicit
> 
> Review of attachment 8428003 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/glue/BackgroundImpl.cpp
> @@ +926,5 @@
> >  
> >      MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(runnable)));
> >    }
> >  
> > +  return already_AddRefed<ContentParent>(actor->mContent.get());
> 
> This seems really dodgy, but I guess it's what the code already does, so...

See bug 1015182.  Surprisingly, this is intentional!

> ::: xpcom/string/public/nsTString.h
> @@ +58,1 @@
> >    nsTString_CharT( const substring_tuple_type& tuple )
> 
> Does MOZ_IMPLICIT nsTString_CharT(...) overflow a line or something?  Can
> you put the MOZ_IMPLICIT on the same line if not?

Sure.
(In reply to Nathan Froyd from comment #2)
> > +  return already_AddRefed<ContentParent>(actor->mContent.get());
> 
> This seems really dodgy

It is indeed dodgy, it should say dont_AddRef(actor->mContent.get());
(partly for the free type deduction and partly for the self-documenting function name)
https://hg.mozilla.org/mozilla-central/rev/28d085d43fa6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.