1114701 - Replace all uses of function pointers with function references in mozilla::pkix

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

in the mozilla::pkix code, no function pointer is allowed to be null, so we should use references instead, to show that the non-nullness is guaranteed.

Comment 1

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

Bug 1114671 shows we have to be careful about the use of const in order to avoid warnings (which get turned into errors) on some versions of clang.

Comment 2

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

Attachment: replace function pointers with function references

Could you please also build this with your local version of clang to make sure it won't cause any build failures like bug 1114671? Thanks!

Comment 3

[Daniel Holbert [:dholbert]]

Bug 1114671 is the first time I've ever encountered function *references* in C++ (rather than function pointers), so I'm not confident enough in my understanding of them to r+ a change like this.

Happy to sanity-check that my clang version compiles it successfully, though. :)

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8540433 [details] [diff] [review]
replace function pointers with function references

Yup, I can compile this successfully.

Setting feedback+ instead of r+ per previous comment, and punting review to keeler.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8540433 [details] [diff] [review]
replace function pointers with function references

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

LGTM with comment addressed.

::: security/pkix/include/pkix/bind.h
@@ +142,2 @@
>    typedef R (C1::*F)(P1&, P2&, P3, P4&);
>    BindToMemberFunction4(F f, C1* that) : f(f), that(that) { }

In internal::BindToMemberFunction4, you made 'that' a reference - should/can we do so here as well?

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/09f9ef671687

I made the change you suggested! Thanks for the fast review turnaround.

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/09f9ef671687