Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8431007:
crypto: add simple P224 implementation. (Closed)
|
Created:
9 years, 1 month ago by agl Modified:
7 years ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptioncrypto: add simple P224 implementation.
This is intended to be the underlying group for an EKE implementation for
Remoting.
BUG=none
TEST=crypto_unittests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108866
Patch Set 1 #
Total comments: 63
Patch Set 2 : ... #Patch Set 3 : ... #
Total comments: 5
Messages
Total messages: 13 (0 generated)
Poke. You can't bug me for code and then ignore the code review :) Cheers AGL
I've had a stab at highlighting where I think things could be clarified, although I may be coming at it from a position as too much of a layman, not being familiar with elliptic curve per-se. I've not reviewed the Jacobian operations in detail, because I know precisely zero about that right now. I'm adding Lambros to review this, since he is more familiar with the maths involved. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc File crypto/p224.cc (right): http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode27 crypto/p224.cc:27: typedef crypto::P224::FieldElement FieldElement; nit: Why not just "using crypto::P224::FieldElement"? http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode32 crypto/p224.cc:32: // b mustn't exceed 2**32. Express this similarly to the comment on Sub(), for consistency. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode45 crypto/p224.cc:45: kTwo31m3, kTwo31m3, kTwo31m3, kTwo31m3 It would be great to clarify with _why_ we want to have bit 32 set in all limbs, and why the mod p causes these slightly strange-looking "reflection" terms. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode52 crypto/p224.cc:52: void Sub(FieldElement* out, const FieldElement& a, const FieldElement& b) { nit: Sub -> Subtract, according to the style guide. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode54 crypto/p224.cc:54: (*out)[i] = a[i] + kZero31ModP[i] - b[i]; It looks like the "zero" constant here is being used to handle underflow gracefully, in the expectation that the result will be subsequently be reduced back to a value meeting the input criteria, so I think there should be a comment either on this function, or perhaps along with the introductory text about field element functions, to explain that property. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode61 crypto/p224.cc:61: // kZero31ModP is 0 mod p where bit 63 is set in all limbs. kZero31ModP -> kZero63ModP http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode67 crypto/p224.cc:67: static const uint32 kBottom12Bits = 0xfff; You define this but never use it? http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode71 crypto/p224.cc:71: // still spaced 28-bits apart and in little-endian order. Since LargeFieldElement still represents 28-bits with each limb, but has twice as many limbs, for coefficients up to 448-bit, in effect? nit: I first mis-interpreted this as meaning that the limbs were packed in 28-bit chunks into this array, which was confusing (and wrong). Is there any clearer wording you can think of? http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode77 crypto/p224.cc:77: void ReduceLarge(FieldElement* out, LargeFieldElement& in) { You're modifying LargeFieldElement, so it should really be pass-by-pointer, I think. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode82 crypto/p224.cc:82: // Eliminate the coefficients at 2**224 and greater. Explain that you're using a mod p operation to reduce to an equivalent, while eliminating the coefficients? http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode86 crypto/p224.cc:86: in[i-4] += in[i] >> 16; nit: It would help laymen like me to have each of these terms commented to indicate the term in p of which they are a reflection. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode99 crypto/p224.cc:99: (*out)[4] += static_cast<uint32>(in[8] >> 16); This looks like another mod p operation? http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode156 crypto/p224.cc:156: void Reduce(FieldElement* in) { Either rename in to in_out, or update the comment. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode172 crypto/p224.cc:172: // Mask is all ones if top != 0, all zero otherwise I think you're doing this to ensure constant-time execution, in which case a comment along the lines of: // Constant-time: mask = (top != 0) ? 0xffffffff : 0 would help clarify. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode175 crypto/p224.cc:175: a[3] += top << 12; This comes, again, as a result of folding things down mod p, which the 2**96 and 1 terms? http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode178 crypto/p224.cc:178: // have added something to a[3], this it's > 2**12. Therefore we can typo: this -> thus? http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode186 crypto/p224.cc:186: // Invert calcuates *out = in^-1 using Fermat's little theorem. Suggest indicating as part of this comment that the target we are aiming for is |in| ** (2**224 - 2**96 - 1), so that the comments alongside the operations are more obvious. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode200 crypto/p224.cc:200: Square(&f2, f2); I like the exuberance of your indentation, but the style guide doesn't. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode242 crypto/p224.cc:242: for (int i = 0; i < 7; i++) { Again, a comment on this block indicating that we're just dealing with the "overflow" terms here... http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode249 crypto/p224.cc:249: out[0] -= top; ... and a comment to indicate that we're then going mod p, which reflects top into the 2**96 & 1 terms here, would aid readability. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode261 crypto/p224.cc:261: // Now we see if the value is >= p and, if so, subtract p. For what purpose? Is the value in the range 0-2p, so that if v>p then subtracting p would get us to 0-p? Again, clarifying that this implementation is non-obvious because it's constant-time would help laymen like me. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode338 crypto/p224.cc:338: // AddJacobian computes *out = a+b where a != b. nit: The body of this function may be easier to read if you leave a line between the code chunks implementing each operation, e.g: // Z1Z1 = Z1² Square(&z1z1, a.z); // Z2Z2 = Z2² Square(&z2z2, b.z); // U1 = X1*Z2Z2 Mul(&u1, a.x, z2z2); http://codereview.chromium.org/8431007/diff/1/crypto/p224.h File crypto/p224.h (right): http://codereview.chromium.org/8431007/diff/1/crypto/p224.h#newcode35 crypto/p224.h:35: static bool ToInternal(const ExternalPoint& in, InternalPoint* out); Why is this a member of P224, rather than of P224::ExternalPoint? http://codereview.chromium.org/8431007/diff/1/crypto/p224.h#newcode38 crypto/p224.h:38: static void ToExternal(const InternalPoint& in, ExternalPoint* out); Similarly here. http://codereview.chromium.org/8431007/diff/1/crypto/p224.h#newcode40 crypto/p224.h:40: // ScalarMult computes *out = in*scalar where scalar is a big-endian number. What format is |scalar| in, besides being big-endian? http://codereview.chromium.org/8431007/diff/1/crypto/p224.h#newcode42 crypto/p224.h:42: InternalPoint* out); According to style, this should be MultiplyByScalar(). Why is it a member of P224, rather than of P224::InternalPoint? http://codereview.chromium.org/8431007/diff/1/crypto/p224_unittest.cc File crypto/p224_unittest.cc (right): http://codereview.chromium.org/8431007/diff/1/crypto/p224_unittest.cc#newcode16 crypto/p224_unittest.cc:16: {0xb7, 0x0e, 0x0c, 0xbd, 0x6b, 0xb4, 0xbf, 0x7f, 0x32, 0x13, 0x90, 0xb9, 0x4a, 0x03, 0xc1, 0xd3, 0x56, 0xc2, 0x11, 0x22, 0x34, 0x32, 0x80, 0xd6, 0x11, 0x5c, 0x1d, 0x21, 0xbd, 0x37, 0x63, 0x88, 0xb5, 0xf7, 0x23, 0xfb, 0x4c, 0x22, 0xdf, 0xe6, 0xcd, 0x43, 0x75, 0xa0, 0x5a, 0x07, 0x47, 0x64, 0x44, 0xd5, 0x81, 0x99, 0x85, 0x00, 0x7e, 0x34}, Style pixie says this line is too long. :( http://codereview.chromium.org/8431007/diff/1/crypto/p224_unittest.cc#newcode277 crypto/p224_unittest.cc:277: P224::Add(sum, minus_b, &a_again); Wouldn't an implementation of Add(a, b, *c) that just copied a to c pass this test?
Removing myself from reviewers list. I don't really have any extra math knowledge that would help here, I'm afraid. And I'm sheriffing :)
http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc File crypto/p224.cc (right): http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode27 crypto/p224.cc:27: typedef crypto::P224::FieldElement FieldElement; On 2011/11/02 23:46:06, Wez wrote: > nit: Why not just "using crypto::P224::FieldElement"? Doesn't work I'm afraid: ../../crypto/p224.cc:35: error: 'crypto::P224' is not a namespace http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode32 crypto/p224.cc:32: // b mustn't exceed 2**32. On 2011/11/02 23:46:06, Wez wrote: > Express this similarly to the comment on Sub(), for consistency. Done. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode45 crypto/p224.cc:45: kTwo31m3, kTwo31m3, kTwo31m3, kTwo31m3 On 2011/11/02 23:46:06, Wez wrote: > It would be great to clarify with _why_ we want to have bit 32 set in all limbs, > and why the mod p causes these slightly strange-looking "reflection" terms. That could go on for a while so I've cited hthe subtraction section of http://www.imperialviolet.org/2010/12/04/ecc.html http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode52 crypto/p224.cc:52: void Sub(FieldElement* out, const FieldElement& a, const FieldElement& b) { On 2011/11/02 23:46:06, Wez wrote: > nit: Sub -> Subtract, according to the style guide. Done. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode54 crypto/p224.cc:54: (*out)[i] = a[i] + kZero31ModP[i] - b[i]; On 2011/11/02 23:46:06, Wez wrote: > It looks like the "zero" constant here is being used to handle underflow > gracefully, in the expectation that the result will be subsequently be reduced > back to a value meeting the input criteria, so I think there should be a comment > either on this function, or perhaps along with the introductory text about field > element functions, to explain that property. Have also cited the same section in ecc.html. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode61 crypto/p224.cc:61: // kZero31ModP is 0 mod p where bit 63 is set in all limbs. On 2011/11/02 23:46:06, Wez wrote: > kZero31ModP -> kZero63ModP Done. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode67 crypto/p224.cc:67: static const uint32 kBottom12Bits = 0xfff; On 2011/11/02 23:46:06, Wez wrote: > You define this but never use it? Removed. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode71 crypto/p224.cc:71: // still spaced 28-bits apart and in little-endian order. On 2011/11/02 23:46:06, Wez wrote: > Since LargeFieldElement still represents 28-bits with each limb, but has twice > as many limbs, for coefficients up to 448-bit, in effect? It only has coefficients up to 392 bits, but it's 64-bits wide so it stretches to 448, yes. > nit: I first mis-interpreted this as meaning that the limbs were packed in > 28-bit chunks into this array, which was confusing (and wrong). Is there any > clearer wording you can think of? I've tried: "So the limbs are at 0, 28, 56, ..., 392 bits, each 64-bits wide." http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode77 crypto/p224.cc:77: void ReduceLarge(FieldElement* out, LargeFieldElement& in) { On 2011/11/02 23:46:06, Wez wrote: > You're modifying LargeFieldElement, so it should really be pass-by-pointer, I > think. Done. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode82 crypto/p224.cc:82: // Eliminate the coefficients at 2**224 and greater. On 2011/11/02 23:46:06, Wez wrote: > Explain that you're using a mod p operation to reduce to an equivalent, while > eliminating the coefficients? Done. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode86 crypto/p224.cc:86: in[i-4] += in[i] >> 16; On 2011/11/02 23:46:06, Wez wrote: > nit: It would help laymen like me to have each of these terms commented to > indicate the term in p of which they are a reflection. Done. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode99 crypto/p224.cc:99: (*out)[4] += static_cast<uint32>(in[8] >> 16); On 2011/11/02 23:46:06, Wez wrote: > This looks like another mod p operation? Done. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode156 crypto/p224.cc:156: void Reduce(FieldElement* in) { On 2011/11/02 23:46:06, Wez wrote: > Either rename in to in_out, or update the comment. Done. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode172 crypto/p224.cc:172: // Mask is all ones if top != 0, all zero otherwise On 2011/11/02 23:46:06, Wez wrote: > I think you're doing this to ensure constant-time execution, in which case a > comment along the lines of: > > // Constant-time: mask = (top != 0) ? 0xffffffff : 0 > > would help clarify. Done. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode175 crypto/p224.cc:175: a[3] += top << 12; On 2011/11/02 23:46:06, Wez wrote: > This comes, again, as a result of folding things down mod p, which the 2**96 and > 1 terms? Yes. Commented. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode178 crypto/p224.cc:178: // have added something to a[3], this it's > 2**12. Therefore we can On 2011/11/02 23:46:06, Wez wrote: > typo: this -> thus? Done. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode186 crypto/p224.cc:186: // Invert calcuates *out = in^-1 using Fermat's little theorem. On 2011/11/02 23:46:06, Wez wrote: > Suggest indicating as part of this comment that the target we are aiming for is > |in| ** (2**224 - 2**96 - 1), so that the comments alongside the operations are > more obvious. Done. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode200 crypto/p224.cc:200: Square(&f2, f2); On 2011/11/02 23:46:06, Wez wrote: > I like the exuberance of your indentation, but the style guide doesn't. Done. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode242 crypto/p224.cc:242: for (int i = 0; i < 7; i++) { On 2011/11/02 23:46:06, Wez wrote: > Again, a comment on this block indicating that we're just dealing with the > "overflow" terms here... Done. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode249 crypto/p224.cc:249: out[0] -= top; On 2011/11/02 23:46:06, Wez wrote: > ... and a comment to indicate that we're then going mod p, which reflects top > into the 2**96 & 1 terms here, would aid readability. Done. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode261 crypto/p224.cc:261: // Now we see if the value is >= p and, if so, subtract p. On 2011/11/02 23:46:06, Wez wrote: > For what purpose? Is the value in the range 0-2p, so that if v>p then > subtracting p would get us to 0-p? > Again, clarifying that this implementation is non-obvious because it's > constant-time would help laymen like me. Done. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode338 crypto/p224.cc:338: // AddJacobian computes *out = a+b where a != b. On 2011/11/02 23:46:06, Wez wrote: > nit: The body of this function may be easier to read if you leave a line between > the code chunks implementing each operation, e.g: > > // Z1Z1 = Z1² > Square(&z1z1, a.z); > > // Z2Z2 = Z2² > Square(&z2z2, b.z); > > // U1 = X1*Z2Z2 > Mul(&u1, a.x, z2z2); Done. http://codereview.chromium.org/8431007/diff/1/crypto/p224.h File crypto/p224.h (right): http://codereview.chromium.org/8431007/diff/1/crypto/p224.h#newcode35 crypto/p224.h:35: static bool ToInternal(const ExternalPoint& in, InternalPoint* out); On 2011/11/02 23:46:06, Wez wrote: > Why is this a member of P224, rather than of P224::ExternalPoint? I'm not sure that I like it, but meh. Have changed. http://codereview.chromium.org/8431007/diff/1/crypto/p224.h#newcode38 crypto/p224.h:38: static void ToExternal(const InternalPoint& in, ExternalPoint* out); On 2011/11/02 23:46:06, Wez wrote: > Similarly here. Done. http://codereview.chromium.org/8431007/diff/1/crypto/p224.h#newcode40 crypto/p224.h:40: // ScalarMult computes *out = in*scalar where scalar is a big-endian number. On 2011/11/02 23:46:06, Wez wrote: > What format is |scalar| in, besides being big-endian? None. It's just a big-endian number. http://codereview.chromium.org/8431007/diff/1/crypto/p224.h#newcode42 crypto/p224.h:42: InternalPoint* out); On 2011/11/02 23:46:06, Wez wrote: > According to style, this should be MultiplyByScalar(). ScalarMult is a name taken from EBATS: a standard API for these sort of the things. MultiplyByScalar would make more sense if it were a member of P224::InternalPoint, but the field operation is called a scalar multiply. > Why is it a member of P224, rather than of P224::InternalPoint? That strikes me as excessive OOP. http://codereview.chromium.org/8431007/diff/1/crypto/p224_unittest.cc File crypto/p224_unittest.cc (right): http://codereview.chromium.org/8431007/diff/1/crypto/p224_unittest.cc#newcode16 crypto/p224_unittest.cc:16: {0xb7, 0x0e, 0x0c, 0xbd, 0x6b, 0xb4, 0xbf, 0x7f, 0x32, 0x13, 0x90, 0xb9, 0x4a, 0x03, 0xc1, 0xd3, 0x56, 0xc2, 0x11, 0x22, 0x34, 0x32, 0x80, 0xd6, 0x11, 0x5c, 0x1d, 0x21, 0xbd, 0x37, 0x63, 0x88, 0xb5, 0xf7, 0x23, 0xfb, 0x4c, 0x22, 0xdf, 0xe6, 0xcd, 0x43, 0x75, 0xa0, 0x5a, 0x07, 0x47, 0x64, 0x44, 0xd5, 0x81, 0x99, 0x85, 0x00, 0x7e, 0x34}, On 2011/11/02 23:46:06, Wez wrote: > Style pixie says this line is too long. :( Have manually reformatted them all. http://codereview.chromium.org/8431007/diff/1/crypto/p224_unittest.cc#newcode277 crypto/p224_unittest.cc:277: P224::Add(sum, minus_b, &a_again); On 2011/11/02 23:46:06, Wez wrote: > Wouldn't an implementation of Add(a, b, *c) that just copied a to c pass this > test? Have swapped the arguments of one of the adds and have asserted that a+b != a.
This is much more readable, IMHO. Thanks! Couple more comments, below... http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc File crypto/p224.cc (right): http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode45 crypto/p224.cc:45: kTwo31m3, kTwo31m3, kTwo31m3, kTwo31m3 On 2011/11/03 17:20:49, agl wrote: > On 2011/11/02 23:46:06, Wez wrote: > > It would be great to clarify with _why_ we want to have bit 32 set in all > limbs, > > and why the mod p causes these slightly strange-looking "reflection" terms. > > That could go on for a while so I've cited hthe subtraction section of > http://www.imperialviolet.org/2010/12/04/ecc.html What I had in mind was a sentence explaining that setting bit 31 ensures that we can subtract a 28-bit limb without underflow, and that because we're working mod p, we can express zero as a multiple of p, chosen to have that property, but perhaps I'm over-simplifying there. Your reference sounds good! http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode71 crypto/p224.cc:71: // still spaced 28-bits apart and in little-endian order. On 2011/11/03 17:20:49, agl wrote: > On 2011/11/02 23:46:06, Wez wrote: > > Since LargeFieldElement still represents 28-bits with each limb, but has twice > > as many limbs, for coefficients up to 448-bit, in effect? > > It only has coefficients up to 392 bits, but it's 64-bits wide so it stretches > to 448, yes. > > > nit: I first mis-interpreted this as meaning that the limbs were packed in > > 28-bit chunks into this array, which was confusing (and wrong). Is there any > > clearer wording you can think of? > > I've tried: "So the limbs are at 0, 28, 56, ..., 392 bits, each 64-bits wide." Would it be correct to say that each limb "represents" 28-bits, despite now being 64-bits? If so then I think that would be clear enough. http://codereview.chromium.org/8431007/diff/1/crypto/p224.h File crypto/p224.h (right): http://codereview.chromium.org/8431007/diff/1/crypto/p224.h#newcode40 crypto/p224.h:40: // ScalarMult computes *out = in*scalar where scalar is a big-endian number. On 2011/11/03 17:20:49, agl wrote: > On 2011/11/02 23:46:06, Wez wrote: > > What format is |scalar| in, besides being big-endian? > > None. It's just a big-endian number. But it has to be 224 bits == 28 bytes of big-endian number, doesn't it? http://codereview.chromium.org/8431007/diff/1/crypto/p224.h#newcode42 crypto/p224.h:42: InternalPoint* out); On 2011/11/03 17:20:49, agl wrote: > On 2011/11/02 23:46:06, Wez wrote: > > According to style, this should be MultiplyByScalar(). > > ScalarMult is a name taken from EBATS: a standard API for these sort of the > things. MultiplyByScalar would make more sense if it were a member of > P224::InternalPoint, but the field operation is called a scalar multiply. OK, I wondered if that might be the case. I think that trumps the style guide. :) > > Why is it a member of P224, rather than of P224::InternalPoint? > > That strikes me as excessive OOP. I think the issue is that right now P224 is effectively a namespace, not a class. Have you considered moving InternalPoint *into* P224, so that each P224 instance *is* an point in internal representation? Then the methods live in the class they operate on, and there is less type-name boilerplate for callers.
I still need to do the check that the arguments to Add aren't both the same, but I'll do that with the next CL. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc File crypto/p224.cc (right): http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode45 crypto/p224.cc:45: kTwo31m3, kTwo31m3, kTwo31m3, kTwo31m3 Have updated the comment. http://codereview.chromium.org/8431007/diff/1/crypto/p224.cc#newcode71 crypto/p224.cc:71: // still spaced 28-bits apart and in little-endian order. > Would it be correct to say that each limb "represents" 28-bits "represents"? Not really I'm afraid. http://codereview.chromium.org/8431007/diff/1/crypto/p224.h File crypto/p224.h (right): http://codereview.chromium.org/8431007/diff/1/crypto/p224.h#newcode40 crypto/p224.h:40: // ScalarMult computes *out = in*scalar where scalar is a big-endian number. On 2011/11/03 20:03:46, Wez wrote: > But it has to be 224 bits == 28 bytes of big-endian number, doesn't it? Oh, I see. Actually no, it could be shorter but since I don't have a length parameter, yes, it must be 28 bytes. Have updated comment.
LGTM! http://codereview.chromium.org/8431007/diff/15001/crypto/p224.h File crypto/p224.h (right): http://codereview.chromium.org/8431007/diff/15001/crypto/p224.h#newcode28 crypto/p224.h:28: bool Set(const base::StringPiece& in); nit: The external representation is what this accepts, and what ToString generates, so it seems logical to name them FromString() and ToString() (or even FromExternal() and ToExternal(), or Import() and Export()). http://codereview.chromium.org/8431007/diff/15001/crypto/p224.h#newcode30 crypto/p224.h:30: // ToString returns an external representation of the Point. nit: Consider clarifying that it's a byte-string, not a printable string.
http://codereview.chromium.org/8431007/diff/15001/crypto/p224.h File crypto/p224.h (right): http://codereview.chromium.org/8431007/diff/15001/crypto/p224.h#newcode28 crypto/p224.h:28: bool Set(const base::StringPiece& in); On 2011/11/04 22:13:46, Wez wrote: > nit: The external representation is what this accepts, and what ToString > generates, so it seems logical to name them FromString() and ToString() (or even > FromExternal() and ToExternal(), or Import() and Export()). Went with SetFromString because FromString sounds like it should be a static that returns a new object. http://codereview.chromium.org/8431007/diff/15001/crypto/p224.h#newcode30 crypto/p224.h:30: // ToString returns an external representation of the Point. On 2011/11/04 22:13:46, Wez wrote: > nit: Consider clarifying that it's a byte-string, not a printable string. Done.
Random drive-by: It was reverted because it broke the shared builders. You need to add CRYPTO_EXPORT to all of the functions/structs. This is because _unittest is in a different module.
On Mon, Nov 7, 2011 at 11:08 AM, <rsleevi@chromium.org> wrote: > It was reverted because it broke the shared builders. You need to add > CRYPTO_EXPORT to all of the functions/structs. This is because _unittest is > in a different module. So I noticed :) Cheers AGL
Message was sent while issue was closed.
https://codereview.chromium.org/8431007/diff/15001/crypto/p224_unittest.cc File crypto/p224_unittest.cc (right): https://codereview.chromium.org/8431007/diff/15001/crypto/p224_unittest.cc#ne... crypto/p224_unittest.cc:805: EXPECT_TRUE(memcmp(&sum, &a, sizeof(sum) != 0)); A warning I'm prototyping (http://llvm.org/PR18297) flags this line. The warning is correct, right? ../../crypto/p224_unittest.cc:806:44: error: size argument in 'memcmp' call is a comparison [-Werror] EXPECT_TRUE(memcmp(&sum, &a, sizeof(sum) != 0)); ~~~~~~~~~~~~^~~~ ../../testing/gtest/include/gtest/gtest.h:1858:23: note: expanded from macro 'EXPECT_TRUE' GTEST_TEST_BOOLEAN_(condition, #condition, false, true, \ ^ ../../testing/gtest/include/gtest/internal/gtest-internal.h:1100:34: note: expanded from macro 'GTEST_TEST_BOOLEAN_' ::testing::AssertionResult(expression)) \ ^ ../../crypto/p224_unittest.cc:806:15: error: did you mean to compare the result of 'memcmp' instead? [-Werror] EXPECT_TRUE(memcmp(&sum, &a, sizeof(sum) != 0)); ^ ~ ../../testing/gtest/include/gtest/gtest.h:1858:23: note: expanded from macro 'EXPECT_TRUE' GTEST_TEST_BOOLEAN_(condition, #condition, false, true, \ ^ ../../testing/gtest/include/gtest/internal/gtest-internal.h:1100:34: note: expanded from macro 'GTEST_TEST_BOOLEAN_' ::testing::AssertionResult(expression)) \ ^ ../../crypto/p224_unittest.cc:806:32: error: explicitly cast the argument to size_t to silence this warning [-Werror] EXPECT_TRUE(memcmp(&sum, &a, sizeof(sum) != 0)); ^ (size_t)( ../../testing/gtest/include/gtest/gtest.h:1858:23: note: expanded from macro 'EXPECT_TRUE' GTEST_TEST_BOOLEAN_(condition, #condition, false, true, \ ^ ../../testing/gtest/include/gtest/internal/gtest-internal.h:1100:34: note: expanded from macro 'GTEST_TEST_BOOLEAN_' ::testing::AssertionResult(expression)) \ ^ 3 errors generated.
On Sat, Dec 21, 2013 at 12:50 AM, <thakis@chromium.org> wrote: > > https://codereview.chromium.org/8431007/diff/15001/crypto/p224_unittest.cc > File crypto/p224_unittest.cc (right): > > https://codereview.chromium.org/8431007/diff/15001/crypto/p224_unittest.cc#ne... > crypto/p224_unittest.cc:805: EXPECT_TRUE(memcmp(&sum, &a, sizeof(sum) != > 0)); > A warning I'm prototyping (http://llvm.org/PR18297) flags this line. The > warning is correct, right? Yes, thanks! I've just landed the fix because I'll forget if I leave it until after the vacation: http://src.chromium.org/viewvc/chrome?revision=242277&view=revision Cheers AGL To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
