Menu
▾
▴
Re: [fetchmail-devel] Some PATRICIA numbers
|
From: Rainer W. <rwe...@ms...> - 2010-05-20 22:34:49
|
"Matthias Andree" <mat...@gm...> writes:
[...]
> Looks quite alright at first glance. As a general remark, I wonder if
> it would be worthwhile splitting the PATRICIA core code from the
> fetchmail interfacing code.
My 'general' opinion on this is that abstractions aren't free and
creating them 'just in case' should rather be avoided.
[...]
> Could I also ask that you spell out implicit "int" types, i. e. change
> unsigned to unsigned int? Implicit int is obsolecscent usage. It is
> valid C89, but causes C99 and C++ compiler complaints.
For the sake of completeness: 'implicit int' refers to not declaring a
function return value which traditionally caused compilers to assume
that the function would return a value of type 'int'. This is no
longer valid C, as of ISO/IEC 9899:1999 (E). OTOH, the set of valid
type specifiers and type specifier combinations is defined in 6.7.2
and includes the following text:
Each list of type specifiers shall be one of the following
sets (delimited by commas, when there is more than one set on
a line);
[...]
-- int, signed, or signed int
-- unsigned, or unsigned int
[6.7.2|2]
Meaning, not only is 'unsigned' a completely legitimate way to declare
an unsigned integer but actually, 'signed' is also valid instead of
'int'. That said, I am completely willing to comply with any 'style
rules' you consider to be essential.
[...]
> To give a feeling for compiler verbosity, I'm including compiler
> diagnostics from three compilers below, omitting warnings for unused
> variables, and more comments inline.
The (meanwhile completely integrated) code compiled as part of
fetchmail without warnings but I have done a -W -Wall gcc pass and
adressed everything which came up (including one actual error). I'd
like to make a few statemens regarding specific warnings below.
General (so that it's before the 'long quoted text'): As I wrote,
integration of this code with fetchmail was completed yesterday and
the resulting binary has survived a night of pulling e-mail from my
account. The most expensive part is now the lexer. I am going to run
this for another night tonight, this time with fastuidl
enabled. Should this also go well, I will most likely move this onto
our 'production server' tomorrow. Except a few bits of missing
functionality (such as a traversal routine), the most notable change I
have done is that I have replaced the recursive trie-freeing routine
with an iterative one: While this is - at best - only marginally
faster (for my test dataset), it has the nice property of needing no
additional storage beyond what's already used for the trie itself.
I've also had an idea how to restrict the size of the num_ndx array to
what is actually needed: Since the maximum message number is known in
advanced, the vector can grow 'downward' such that position zero is
assumed to hold the largest possible message number and lower indices
are then inserted sufficiently distant from that with the array being
extended as required. Lookup would then use negative offsets relative
to a pointer to the end of the array, adjusted according to the
'logical position' of the end of the array relative to the message
number 1. I think I will get this implemented tomorrow and could then
basically produce a patch relative to 6.3.17 which could serve as
base for further changes.
?
> $ gcc -O -Wc++-compat -std=c99 -Wall -Wextra -pedantic \
> -o uidl_db uidl_db.c -Wno-unused -Dinline=__inline__
> # -Dinline for -std=c89 :-)
> uidl_db.c: In function ‘init_uidl_db’:
> uidl_db.c:63: warning: request for implicit conversion from ‘void *’
> to ‘struct uidl_record **’ not permitted in C++
This is just my usual laziness :-): Since void * is kind of a
passepartout type in C, I usually just cast to void * whenever any
pointer conversion is needed. I've changed that to use the actual
destination type.
> uidl_db.c: In function ‘walk_down’:
> uidl_db.c:83: warning: comparison between signed and unsigned integer
> expressions
> uidl_db.c:85: warning: comparison between signed and unsigned integer
> expressions
This was due to the ofs variable being of type int. I've changed that
to unsigned, which didn't matter for the code. This will, of course,
break horribly if UIDs longer than 2^32 - 1 chars are supposed to be
processed (or if bit indices higher than 2^31-1 are needed) but I
assume that both propositions are not exactly realistic (the POP3
specification already prohibits this: Maximum 'payload' of a response
line is 510 octets, according to RFC1939, sec. 3).
> uidl_db.c:314: warning: ‘v’ may be used uninitialized in this function
> /tmp/cc6WZFTd.o: In function `main':
I don't usually add spurious initializations just to silence these
warnings if they are obviously wrong.
[...]
> uidl_db.c(92): warning #1684: conversion from pointer to same-sized
> integral type (potential portability problem)
> offsetof(struct pat_node, ptrs_[2])
> ^
I do not have the slightest idea what this is supposed to refer to.
[...]
> uidl_db.c(119): remark #2259: non-pointer conversion from "int" to
> "unsigned char" may lose significant bits
> v = r0->id[ofs] ^ r1->id[ofs];
> ^
Since both operands come from arrays of char and are only insofar of
type 'int' as the values are supposed to be promoted to int, this
seems to be an extremly poor warning.
> uidl_db.c(244): warning #2259: non-pointer conversion from
> "size_t={unsigned long}" to "unsigned int" may lose significant bits
> id_len = strlen(id);
> ^
I would rather keep this as 'unsigned' because size_t may be a 64-bit
type on 64-bit systems and this seems exessive for representing the
length of something restricted to less than 510 octets.
[...]
> uidl_db.c:92:6: warning: using extended field designator is an
> extension [-pedantic]
> offsetof(struct pat_node, ptrs_[2])
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The relevant text of the C-standard is
offsetof(type, member-designator)
[...]
The type and member designator shall be such that given
static type t;
then the expression &(t.member-designator) evaluates to an
address constant.
[7.17|3]
Assuming the following code:
static struct pat_node t;
&(t.ptrs_[2]) certainly evaluates to an address constant, so this
appears to either be a traditional restriction or a C89-one which
accidentally escaped into LLVM/clang using someone remembering it as
'transport medium'.
|
