Closed Bug 1114701 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file)

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: → 1114671
Assignee: nobody → brian
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
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.