916632 - Add a simple scoped pointer type ScopedPtr that is independent of MFBT

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P1)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: ScopedPtr.patch

In the absence of std::unique_ptr being available on all platforms, and in order to allow insanity::pkix and CertVerifier to be built independently of Gecko, we need a scoped pointer type like boost::scoped_ptr.

I hope to eventually/soon replace the use of MFBT's Scoped.h in ScopedNSSTypes.h with usage of this ScopedPtr class, if my upcoming attempt to get unique_ptr working on all platforms fails.

Camilo, you've already reviewed these patches, but please give them a once-over to see if there are any style nits or whatnot that need to be addressed.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 2: Add typedefs for commonly-used ScopedPtrs

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 1: Add ScopedPtr

Comment 3

[Camilo Viecco (:cviecco)]

Brian, I notice these are in the insanity namespace, do you want this?

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Camilo Viecco (:cviecco) from comment #3)
> Brian, I notice these are in the insanity namespace, do you want this?

Yes. Do you think they should go into another namespace?

Comment 5

[Camilo Viecco (:cviecco)]

Yes, if the goal is to make them generic (which seems to be implied by the name of the bug)

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Camilo Viecco (:cviecco) from comment #5)
> Yes, if the goal is to make them generic (which seems to be implied by the
> name of the bug)

I think the current namespace (insanity::pkix) is OK. That's what it has always been and the important thing is that insanity::pkix is self-contained.

Comment 7

[Camilo Viecco (:cviecco)]

Comment on attachment 805094 [details] [diff] [review]
Part 1: Add ScopedPtr

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

Style r+, funcionality (when not using protected members) r- for clarity in comments.

::: include/insanity/ScopedPtr.h
@@ +48,5 @@
> +
> +protected:
> +  T* mValue;
> +
> +  ScopedPtr(const ScopedPtr&) /*delete*/;

The delete comment is not clear.

Comment 8

[Camilo Viecco (:cviecco)]

Also, is the location of the files what you want. Not scoping them to security?

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 805094 [details] [diff] [review]
Part 1: Add ScopedPtr

The /*delete*/ comments refer to the new C++03 feature described here:
http://en.wikipedia.org/wiki/C%2B%2B11#Explicitly_defaulted_and_deleted_special_member_functions

Unfortunately, not all of our compilers support that mechanism yet, so I had to use a comment instead. I put these in the private section of the class because that is the standard way of prohibiting copying/assignment prior to the introduction to the new C++03 feature.

If you'd like, I can write the comment as /* = delete */. If so, please let me know when you r+ the patch and I will make that change before checking in.

Note that the posted patches are against a repo rooted in security/insanity. When I check them in, they will be in the mozilla-central repo in the correct place.

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 3: Work around old GCC's lack of nullptr, r=briansmith

Comment 11

[Camilo Viecco (:cviecco)]

Comment on attachment 805094 [details] [diff] [review]
Part 1: Add ScopedPtr

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

Issue with dirs addressed via irc.

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fa7eae64dbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8cbe1b47eb8
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb6294833688

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5fa7eae64dbe
https://hg.mozilla.org/mozilla-central/rev/a8cbe1b47eb8
https://hg.mozilla.org/mozilla-central/rev/fb6294833688