Closed Bug 1204790 Opened 4 years ago Closed 4 years ago

Prefer deleted function with public access specifiers instead of private in UniquePtr

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

Details

Attachments

(1 file, 1 obsolete file)

Inspired by Bug 1198451,

The Uniqueptr cannot be copied but some compilers like MSVC will indicate the compile error by accessing private function instead of deleting function since we declare deleted function in private field.

To be more clear compile errors, just put it in public field instead.
Hi Nathan,

Could you please help for reviewing this simple patch?

Thank you very much.
Assignee: nobody → jacheng
Attachment #8661097 - Flags: review?(nfroyd)
Comment on attachment 8661097 [details] [diff] [review]
Prefer-deleted-function-with-public-acce.patch

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

::: mfbt/UniquePtr.h
@@ +392,5 @@
>              typename EnableIf<IsPointer<U>::value &&
>                                IsConvertible<U, Pointer>::value,
>                                int>::Type aDummy = 0)
>    = delete;
>  

I guess you would need to delete the |public:| directly after this line too, right?
Attachment #8661097 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #2)
> Comment on attachment 8661097 [details] [diff] [review]
> Prefer-deleted-function-with-public-acce.patch
> 
> Review of attachment 8661097 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mfbt/UniquePtr.h
> @@ +392,5 @@
> >              typename EnableIf<IsPointer<U>::value &&
> >                                IsConvertible<U, Pointer>::value,
> >                                int>::Type aDummy = 0)
> >    = delete;
> >  
> 
> I guess you would need to delete the |public:| directly after this line too,
> right?
Yes :), thanks for reminding and reviewing.

Carry r+ from Nathan.

Attach the treeherder result for reference.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4913ea8c8931

Thanks
Attachment #8661097 - Attachment is obsolete: true
Attachment #8661576 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c4333afa007f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.