Closed Bug 1114703 Opened 5 years ago Closed 5 years ago

Clean up or remove mozilla::pkix's bind.h

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file, 1 obsolete file)

(In reply to Daniel Holbert [:dholbert] from bug 1114671 comment 4)
> (Tangent: I noticed that this new Bind5 version of bind() has different args
> than the other versions of bind().  The Bind5 version has args "B1 b1, B2
> b2," etc., whereas the Bind4 version of bind() has args "const B1& b1, const
> B2& b2". I wonder if that inconsistency is there for a reason? Presumably we
> should use const-references if possible. That should probably happen in a
> separate bug/patch, though, and it's not currently causing build breakage
> like this other issue, so I'm not jumping on it. :))

This specific hack was a result of me learning that I could write fewer Bind classes if I had the user of Bind decide which things should be const or not. I'd like to go back and redo the other definitions to be like Bind5.

However, this whole file is a horrible hack. We should just switch it to use variadic templates, but I think we need compilers to be upgraded first. And, once they are upgraded, we'll be able to use lambdas and so we won't need bind.h at all any more.
Assignee: nobody → brian
No longer depends on: 1087161
Target Milestone: --- → mozilla38
Attached patch no-more-bind.patch (obsolete) — Splinter Review
Attachment #8552562 - Flags: review?(mmc)
Here's the try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d4fa1e41bbf

Not sure what happened with Linux Debug. I retriggered it to see if it was intermittent.
Status: NEW → ASSIGNED
Comment on attachment 8552562 [details] [diff] [review]
no-more-bind.patch

Clearing review request. I wrote some more code that uses lambdas, and I realized that I should be using formatting more like the formatting uesd for JS lambdas, instead of the harder-to-read formatting I used here. I'll upload a reformatted patch.
Attachment #8552562 - Flags: review?(mmc)
Here's the patch with the updated formatting, consistent with how we do lambdas in JS.
Attachment #8552562 - Attachment is obsolete: true
Attachment #8552799 - Flags: review?(mmc)
Comment on attachment 8552799 [details] [diff] [review]
no-more-bind.patch [v2]

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

Much better, thanks for making these changes. There was one change I didn't understand.

::: security/pkix/lib/pkixcheck.cpp
@@ +313,5 @@
> +      Result rv = der::OptionalBoolean(r, isCA);
> +      if (rv != Success) {
> +        return rv;
> +      }
> +      // TODO(bug 985025): If isCA is false, pathLenConstraint

Did you mean to include these changes? They seem unrelated.
Attachment #8552799 - Flags: review?(mmc) → review-
Comment on attachment 8552799 [details] [diff] [review]
no-more-bind.patch [v2]

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

::: security/pkix/lib/pkixcheck.cpp
@@ +313,5 @@
> +      Result rv = der::OptionalBoolean(r, isCA);
> +      if (rv != Success) {
> +        return rv;
> +      }
> +      // TODO(bug 985025): If isCA is false, pathLenConstraint

This code is the code from the DecodeBasicConstraints function. I could have written this as:

  Result rv = der::Nested(input, der::SEQUENCE,
                          [&isCA, &[pathLenConstraint](Reader& r) {
    return DecodeBasicConstraints(r, isCA, pathLenConstraint);
  });

However, I think putting DecodeBasicConstraints's code inline makes more sense, considering we're using a lambda anyway.
Attachment #8552799 - Flags: review- → review?(mmc)
Comment on attachment 8552799 [details] [diff] [review]
no-more-bind.patch [v2]

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

Thanks for explaining.
Attachment #8552799 - Flags: review?(mmc) → review+
https://hg.mozilla.org/mozilla-central/rev/49651d30167a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.