search

jech / polipo

The Polipo caching HTTP proxy
http://www.pps.jussieu.fr/~jch/software/polipo/
MIT License
1.8k stars 354 forks source link

Fix compiler warnings and types #66

Closed lkraider closed 8 years ago

lkraider commented 8 years ago

These changes were made to remove warnings while making polipo compile under mingw64.

Most changes are regarding pointer type portability issues.

jech commented 8 years ago

Fix pointer warnings: rejected. I don't understand why this is needed.

Fix socklen_t warning: applied, thanks.

Fix compiler warnings: rejected, since it adds new #ifdefs and therefore makes the code even more unreadable than it already is. If you find a better way of achieving this, please submit a new pull request.

Fix pid_t redefinition error: applied, thanks.

Add Makefile.mingw: rejected, I'm not going to maintain two almost identical makefiles. Please add any variables that you need to the makefile, and update the README.

netsafe commented 8 years ago

Pointer warnings IS needed because inttypes.h provides us the way not to hardcode, but to use library-provided types, pointer to types macro and makes it really integrated into system. The warning fixed by that patch are 'short-hand' warnings : of course they are OK when you're testing something, but even in beta code they are blckers of bad-style. Please approove that patch. Thanks

jech commented 8 years ago

inttypes.h was introduced in C99. Polipo is supposed to be written in C86, and is portable to things like SVR4.

Of course, it might make sense to rely on C99 now, but that should be a conscious decision, not the side-effect of an incidental patch. (And there are better reasons to use C99 than avoiding a warning on a proprietary platform — VLAs are the big one.)

lkraider commented 8 years ago

Thanks for the review.

The Makefile was not supposed to be in the pull request, I pushed it to my repo after creating this PR and forgot it would be added.

Fix Pointer warnings is not strictly needed as unsigned long should be fine for most architectures, just that using intptr_t will match the actual pointer size for the architecture without wasting space. The print format becomes that ugly mess though.

Fix Compiler warnings adds the ifdefs to remove unused definitions when S_ISLNK is not defined. Since there are blocks already conditional to it, it makes sense to remove everything related to the define explicitly. I agree a better solution would be to avoid these conditionals, which I think would mean adding a compatibility feature using WinAPI calls (use CreateSymbolicLink, then check with GetFileAttributes if FILE_ATTRIBUTE_REPARSE_POINT flag equals IO_REPARSE_TAG_SYMLINK).

netsafe commented 8 years ago

About C99 and C86 - ifndef directive was introduced initially, so if you REALLY need C86 compatibility - use the ifndef+define workaround