[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [FEATURE_REQUEST] support openssl checksum format too
From: |
Jim Meyering |
Subject: |
Re: [FEATURE_REQUEST] support openssl checksum format too |
Date: |
Sat, 03 Oct 2009 22:18:17 +0200 |
Guenter Knauf wrote:
> Jim,
> thanks for the very quick review.
>
> Jim Meyering schrieb:
>> Guenter Knauf wrote:
>>> - size_t i;
>>> + size_t i = 0;
>>> bool escaped_filename = false;
>>> size_t algo_name_len;
>>>
>>> - i = 0;
>>> while (ISWHITE (s[i]))
>>> ++i;
>>
>> Instead, please move the declaration "down".
> hmm, not sure what you mean here - moving it down is bad since var
> declarations in the middle of the code are not allowed, although
> accepted by gcc.
stmt-after-decl is part of C99, now a 10-year-old standard.
> In this case its no problem, but if a while later
> someone adds code before the declaration then it clashes with non-gcc
> compilers. What's bad with initializing a var with the declaration?
> That's valid for all compilers AFAIK. Anyway, since its not needed I
> left this part out from my new patch - was just only a suggestion to
> save a line.
This is what I meant:
...
- i = 0;
+ size_t i = 0;
while (ISWHITE (s[i]))
++i;
There is already code in coreutils that requires C99 decl-after-stmt,
so even if someone added a statement before it, that would be fine.
>> That allows two or more white-space bytes.
>> Let's restrict it to just 0 or 1 space (not white-space) byte.
> ok.
>
>>> + if (strncmp (s + j, "(", 1) == 0)
>>
>> This would be better as:
>>
>> if (s[j] == '(')
> ok.
>
> here's 2nd trial inline (and also attached) which looks even more simple
No need to include a patch twice.
> thanks to your comments:
>
> --- src/md5sum.c.orig 2009-09-01 13:01:16.000000000 +0200
> +++ src/md5sum.c 2009-10-03 20:32:28.000000000 +0200
> @@ -263,11 +263,13 @@
> algo_name_len = strlen (DIGEST_TYPE_STRING);
> if (strncmp (s + i, DIGEST_TYPE_STRING, algo_name_len) == 0)
> {
> - if (strncmp (s + i + algo_name_len, " (", 2) == 0)
> + if (s[i + algo_name_len] == ' ')
> + ++i;
> + if (s[i + algo_name_len] == '(')
> {
> *binary = 0;
> - return bsd_split_3 (s + i + algo_name_len + 2,
> - s_len - (i + algo_name_len + 2),
> + return bsd_split_3 (s + i + algo_name_len + 1,
> + s_len - (i + algo_name_len + 1),
> hex_digest, file_name);
> }
> }
>
>> And a test, please.
> I guess you mean something like that (also attached)?
>
...
>
> does this work for you?
Right idea. I haven't tried them.
> Sorry, but I've not yet figured out how I could
> run these tests - a 'make tests' didnt work (therefore wrote above
"make check" runs most tests.
Use this
make check -C tests TESTS=misc/md5sum VERBOSE=yes
to run just the one you changed.
> blindly assumed)- can you please give me a quick hint before I read me
> dead? Thanks. Once I know I create patches for the sha*sum tests too.
> Also, is it possible to run single tests?
- Re: [FEATURE_REQUEST] support openssl checksum format too, Guenter Knauf, 2009/10/03
- Re: [FEATURE_REQUEST] support openssl checksum format too, Jim Meyering, 2009/10/03
- Re: [FEATURE_REQUEST] support openssl checksum format too, Guenter Knauf, 2009/10/03
- Re: [FEATURE_REQUEST] support openssl checksum format too,
Jim Meyering <=
- Re: [FEATURE_REQUEST] support openssl checksum format too, Guenter Knauf, 2009/10/03
- Re: [FEATURE_REQUEST] support openssl checksum format too, Guenter Knauf, 2009/10/06
- Re: [FEATURE_REQUEST] support openssl checksum format too, Jim Meyering, 2009/10/07
- Re: [FEATURE_REQUEST] support openssl checksum format too, Guenter Knauf, 2009/10/07
- Re: [FEATURE_REQUEST] support openssl checksum format too, Jim Meyering, 2009/10/07
- Re: [FEATURE_REQUEST] support openssl checksum format too, Eric Blake, 2009/10/07