#5 Warning/Code cleanup (PATCH) v3

Unstable (example)
open
nobody
None
5
2014-04-19
2014-04-12
Campbell Barton
No

This is an update to an existing patch but Im not able to edit that, so sending a new one.

1 Attachments

Discussion

  • Previous patch had minor issue with creating exports.c, updated.

     
  • Arnout Engelen
    Arnout Engelen
    2014-04-12

    • I've applied some of the changes
    • 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?
     
    • 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.
     
  • Arnout Engelen
    Arnout Engelen
    2014-04-12

    • I don't have a problem with removing 'min' if only 'max' is used :)
    • Adding '(void)var' just to quiet the compiler doesn't seem like an improvement to me
    • Yeah, let's go with aligning with the '('

    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.h already 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.h

    Edit, libmainloop/ isnt including ioncore/ 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.

     
  • Arnout Engelen
    Arnout Engelen
    2014-04-17

    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