I don't think we should add 'utildefines.h' in the root of the project. Perhaps add them to ioncore/common.h ?
I see you're sometimes #ifdef/commenting out dead code. Better to just remove it.
I see you're sometimes adding something like '(void)cwin', why?
Might be better to split the patch into multiple patches
You're sometimes indenting method parameters by 8 spaces, sometimes aligning them with the '(' of the method declaration. Do we have a preference? Should we decide on one?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
ioncore/common.h - sounds fine. I wasnt sure where to add this, agree root is bad.
removing code is fine too, sometimes there are functions which should be kept for completeness though, like min/max (when only max is used, you know...).
Some variables are conditionally used in a function, based on compiler options, this (void)var; will quiet the compiler warning about it being unused.
Re: indentation, "aligning them with the '(' of the method declaration" - seems default here. Best go with that.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
For simple functions like min/max, OK, but sometimes work has gone to define a small local API, where one functions happens not to be used, in this case I would sometimes keep for completeness. But your call here, can just remove them too.
re: (void)var; I can see it seems a bit redundant, the reason this can be useful is it lets everyone compile with -Werror, which I find very useful in general.
Some projects define a macro for this... FAKE_USER(var);
I can update the patch, sure.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Added an updated patch:
- moved utildefines to compat/utildefines.h
- removed some obviously redundant functions (instead of ifdef'ing)
- removed unused macros/defines (and explicitly ignore them for exports).
- keep non-utf8 text, can make utf8 compat later.
About the UNUSED macro: For the location, libtu/misc.h might be a suitable place. I'm still not so sure this is the way to go. "int UNUSED(index)" looks rather ugly. "UNUSED(int index)" would be nicer, but then we can't have the compiler protect us against using the variable marked as 'unused'. Using 'UNUSED' as a postfix, as done in mod_dock/dock.c suffers from that same problem.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Previous patch had minor issue with creating exports.c, updated.
ioncore/common.h- sounds fine. I wasnt sure where to add this, agree root is bad.removing code is fine too, sometimes there are functions which should be kept for completeness though, like min/max (when only max is used, you know...).
Some variables are conditionally used in a function, based on compiler options, this
(void)var;will quiet the compiler warning about it being unused.Do you plan to prepare an updated patch?
re: removing 'min' if only 'max' is used,
For simple functions like min/max, OK, but sometimes work has gone to define a small local API, where one functions happens not to be used, in this case I would sometimes keep for completeness. But your call here, can just remove them too.
re:
(void)var;I can see it seems a bit redundant, the reason this can be useful is it lets everyone compile with-Werror, which I find very useful in general.Some projects define a macro for this...
FAKE_USER(var);I can update the patch, sure.
Note, I didnt realize
common.halready existed,I'd really prefer to keep compiler specific macros and headers which bundle includes separate (if it were my choice).
This way if there is some part of the source which has nothing todo with X11 (low level api), it can still include
utildefines.hEdit,
libmainloop/isnt includingioncore/as an include path.Why not have some generic include dir added for portability headers and OS defines?
eg
compat/utildefines.h, then any headers for compatibility can be added there.Last edit: Campbell Barton 2014-04-13
Added an updated patch:
- moved utildefines to
compat/utildefines.h- removed some obviously redundant functions (instead of
ifdef'ing)- removed unused macros/defines (and explicitly ignore them for exports).
- keep non-utf8 text, can make utf8 compat later.
About the UNUSED macro:
For the location, libtu/misc.h might be a suitable place.
I'm still not so sure this is the way to go. "int UNUSED(index)" looks rather ugly. "UNUSED(int index)" would be nicer, but then we can't have the compiler protect us against using the variable marked as 'unused'. Using 'UNUSED' as a postfix, as done in mod_dock/dock.c suffers from that same problem.
@Arnout. right, I had the same concerns and while not perfect I think
int UNUSED(index)is the best choice since it avoids accidental use.Last edit: Campbell Barton 2014-04-19