On Thu, 2007-03-15 at 11:46 +0100, Max Kellermann wrote:
Some of the changes may be arguable, escpecially the const warning fixes, example:
char *pass = "";
I changed that to "pass = NULL" and changed the function call:
- auth_request_verify_plain(request, pass,
- auth_request_verify_plain(request, pass == NULL ? "" : pass,
That's ok, but I'm not sure about bsearch_insert_pos(). It's the way it is mostly because I wanted to keep bsearch() API. If it can't return void * then maybe it could be easier to just change the whole API to something like:
/* If key is found, returns TRUE and sets pos_r to the position where the key was found. If key isn't found, returns FALSE and sets pos_r to the position where the key should be inserted. */ bool bsearch_insert_pos(const void *key, const void *base, unsigned int nmemb, size_t size, int (*cmp)(const void *, const void *), unsigned int *pos_r);
Because that's how it's usually used anyway, so it probably makes the code simpler also. Hmm. And maybe s/pos/idx/ :)
In other sources, I added "const" to parameters or to the return value where it seemed to make sense and didn't cause harm. Dealing with the const problem can be tricky, and sometimes dirty deconstification hacks are the only way to go.. however I believe we should always turn on "-Wconst" and explicitly add manual deconstitication where we assume it's not dangerous.
You mean -Wwrite-strings I guess? Looks like I've added these comments about various gcc warning options before:
# -Wcast-qual -Wcast-align -Wconversion # too many warnings # -Wstrict-prototypes -Wredundant-decls # may give warnings in some systems # -Wmissing-format-attribute -Wmissing-noreturn -Wwrite-strings # a couple of warnings
I guess those "warnings in some systems" warnings could be enabled by default and then disabled on those specific systems.
-Wwrite-strings would be nice as default too..
I didn't fix all "const" and "missing noreturn" warnings, but my patches may be a good start... if there is further interest in fixing all of them, I'd be glad to do it.
I've interest, but I don't think these changes should be made to v1.0 tree anymore. But for CVS HEAD, sure.
I've also enabled -Wstrict-aliasing=2 in CVS HEAD, which is now giving several warnings. I should either fix those somehow or disable the warning after all..
I'll see about applying your patches (minus bsearch) to CVS HEAD. Or I wouldn't mind getting the patches directly against it either :)