Bug1227624-Protection

Bug1227624-Protection

[wtc1, 2015-11-25 19:09:22]

Review comments on patch set 1:

https://codereview.appspot.com/275560043/diff/1/lib/certdb/alg1485.c
File lib/certdb/alg1485.c (right):

https://codereview.appspot.com/275560043/diff/1/lib/certdb/alg1485.c#newcode37
lib/certdb/alg1485.c:37: SEC_ASN1_DS},
The original code aligns all the SEC_ASN1_DS's. We should either preserve that,
or just not align anything.

https://codereview.appspot.com/275560043/diff/1/lib/certdb/alg1485.c#newcode142
lib/certdb/alg1485.c:142: ((c) == '=') ||                                \
The original code aligns the line-continuation backslashes. We should either
preserve that, or just use one space before the line-continuation backslashes.

https://codereview.appspot.com/275560043/diff/1/lib/certdb/genname.c
File lib/certdb/genname.c (right):

https://codereview.appspot.com/275560043/diff/1/lib/certdb/genname.c#newcode1594
lib/certdb/genname.c:1594: "\x16\x14" "igca@sgdn.pm.gouv.fr" /* emailAddress */ 
                     \
The last line-continuation backslash on this line and on line 1610 probably
should be deleted. They require us to follow them with a blank line, which is
not obvious.

https://codereview.appspot.com/275560043/diff/1/lib/certdb/polcyxtn.c
File lib/certdb/polcyxtn.c (right):

https://codereview.appspot.com/275560043/diff/1/lib/certdb/polcyxtn.c#newcode21
lib/certdb/polcyxtn.c:21: const SEC_ASN1Template CERT_DisplayTextTypeTemplate[]
= {
This file doesn't have the "clang-format off" command. Should it be in this
patch?

https://codereview.appspot.com/275560043/diff/1/lib/certdb/xauthkid.c
File lib/certdb/xauthkid.c (right):

https://codereview.appspot.com/275560043/diff/1/lib/certdb/xauthkid.c#newcode24
lib/certdb/xauthkid.c:24: const SEC_ASN1Template CERTAuthKeyIDTemplate[] = {
This file doesn't have the "clang-format off" command. Should it be in this
patch?

https://codereview.appspot.com/275560043/diff/1/lib/certdb/xconst.c
File lib/certdb/xconst.c (right):

https://codereview.appspot.com/275560043/diff/1/lib/certdb/xconst.c#newcode65
lib/certdb/xconst.c:65: };
This file doesn't have the "clang-format off" command. Should it be in this
patch?

[franziskus, 2015-11-30 16:49:23]

On 2015/11/25 19:09:22, wtc1 wrote:
> Review comments on patch set 1:
> 
> https://codereview.appspot.com/275560043/diff/1/lib/certdb/alg1485.c
> File lib/certdb/alg1485.c (right):
> 
> https://codereview.appspot.com/275560043/diff/1/lib/certdb/alg1485.c#newcode37
> lib/certdb/alg1485.c:37: SEC_ASN1_DS},
> The original code aligns all the SEC_ASN1_DS's. We should either preserve
that,
> or just not align anything.
> 
>
https://codereview.appspot.com/275560043/diff/1/lib/certdb/alg1485.c#newcode142
> lib/certdb/alg1485.c:142: ((c) == '=') ||                                \
> The original code aligns the line-continuation backslashes. We should either
> preserve that, or just use one space before the line-continuation backslashes.
> 
> https://codereview.appspot.com/275560043/diff/1/lib/certdb/genname.c
> File lib/certdb/genname.c (right):
> 
>
https://codereview.appspot.com/275560043/diff/1/lib/certdb/genname.c#newcode1594
> lib/certdb/genname.c:1594: "\x16\x14" mailto:"igca@sgdn.pm.gouv.fr" /*
emailAddress */ 
>                      \
> The last line-continuation backslash on this line and on line 1610 probably
> should be deleted. They require us to follow them with a blank line, which is
> not obvious.
> 
> https://codereview.appspot.com/275560043/diff/1/lib/certdb/polcyxtn.c
> File lib/certdb/polcyxtn.c (right):
> 
>
https://codereview.appspot.com/275560043/diff/1/lib/certdb/polcyxtn.c#newcode21
> lib/certdb/polcyxtn.c:21: const SEC_ASN1Template
CERT_DisplayTextTypeTemplate[]
> = {
> This file doesn't have the "clang-format off" command. Should it be in this
> patch?
> 
> https://codereview.appspot.com/275560043/diff/1/lib/certdb/xauthkid.c
> File lib/certdb/xauthkid.c (right):
> 
>
https://codereview.appspot.com/275560043/diff/1/lib/certdb/xauthkid.c#newcode24
> lib/certdb/xauthkid.c:24: const SEC_ASN1Template CERTAuthKeyIDTemplate[] = {
> This file doesn't have the "clang-format off" command. Should it be in this
> patch?
> 
> https://codereview.appspot.com/275560043/diff/1/lib/certdb/xconst.c
> File lib/certdb/xconst.c (right):
> 
> https://codereview.appspot.com/275560043/diff/1/lib/certdb/xconst.c#newcode65
> lib/certdb/xconst.c:65: };
> This file doesn't have the "clang-format off" command. Should it be in this
> patch?

Hi Wan-Teh, thanks for the comments. Seems like the two patch stets got a little
mixed up. Sorry for that. Cleaned this one up, only two files with protection
and nicer alignment now.

[wtc1, 2015-11-30 23:20:21]

Patch set 2 LGTM.

https://codereview.appspot.com/275560043/diff/20001/lib/certdb/alg1485.c
File lib/certdb/alg1485.c (right):

https://codereview.appspot.com/275560043/diff/20001/lib/certdb/alg1485.c#newc...
lib/certdb/alg1485.c:128: ((c) == '<') || ((c) == '>') || ((c) == '#') ||       
                   \
Nit: I probably would not align the line-continuation backslashes.
Your choice of putting them at the 80-th character position is fine.
The only drawback is that they are more likely to be missed.

https://codereview.appspot.com/275560043/diff/20001/lib/certdb/alg1485.c#newc...
lib/certdb/alg1485.c:140: (((c) >= '+') && ((c) <= '/')) ||      /* + , - . / */
                   \
The comments on these three lines should either be aligned like in the original
code, or be a fixed distance (for example, two spaces) from the code.

https://codereview.appspot.com/275560043/diff/20001/lib/certdb/genname.c
File lib/certdb/genname.c (right):

https://codereview.appspot.com/275560043/diff/20001/lib/certdb/genname.c#newc...
lib/certdb/genname.c:1585: #define ANSSI_SUBJECT_DN \
Nit: should this backslash be aligned with the other backslashes?
See also line 1596.

[franziskus, 2015-12-01 14:13:32]

On 2015/11/30 23:20:21, wtc1 wrote:
> Patch set 2 LGTM.
> 
> https://codereview.appspot.com/275560043/diff/20001/lib/certdb/alg1485.c
> File lib/certdb/alg1485.c (right):
> 
>
https://codereview.appspot.com/275560043/diff/20001/lib/certdb/alg1485.c#newc...
> lib/certdb/alg1485.c:128: ((c) == '<') || ((c) == '>') || ((c) == '#') ||     
 
>                    \
> Nit: I probably would not align the line-continuation backslashes.
> Your choice of putting them at the 80-th character position is fine.
> The only drawback is that they are more likely to be missed.
> 
>
https://codereview.appspot.com/275560043/diff/20001/lib/certdb/alg1485.c#newc...
> lib/certdb/alg1485.c:140: (((c) >= '+') && ((c) <= '/')) ||      /* + , - . /
*/
>                    \
> The comments on these three lines should either be aligned like in the
original
> code, or be a fixed distance (for example, two spaces) from the code.
> 
> https://codereview.appspot.com/275560043/diff/20001/lib/certdb/genname.c
> File lib/certdb/genname.c (right):
> 
>
https://codereview.appspot.com/275560043/diff/20001/lib/certdb/genname.c#newc...
> lib/certdb/genname.c:1585: #define ANSSI_SUBJECT_DN \
> Nit: should this backslash be aligned with the other backslashes?
> See also line 1596.

Thanks for the comments, fixed the nits and put it on bugzilla for check-in.
I actually like when you don't really see the line-continuation backslashes. But
the main reason I'm doing that is to be consistent with clang-format, which does
it when having a column limit.

[wtc1, 2015-12-01 18:53:45]

On 2015/12/01 14:13:32, franziskus wrote:
>
> I actually like when you don't really see the line-continuation backslashes.
But
> the main reason I'm doing that is to be consistent with clang-format, which
does
> it when having a column limit.

Yes, our manual formatting should be consistent with clang-format.

Wan-Teh