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

RESOLVED FIXED in mozilla37

Status

()

Core
Security: PSM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

Trunk
mozilla37
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
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.
See Also: → bug 1114671
Assignee: nobody → brian
Created attachment 8540433 [details] [diff] [review]
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!
Attachment #8540433 - Flags: review?(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 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.
Attachment #8540433 - Flags: review?(dkeeler)
Attachment #8540433 - Flags: review?(dholbert)
Attachment #8540433 - Flags: feedback+
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?
Attachment #8540433 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/09f9ef671687

I made the change you suggested! Thanks for the fast review turnaround.
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/09f9ef671687
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.