[pupnp-tracker] [ pupnp-Patches-3091814 ] ixml warnings fixes

[Tracker i. u. n. <pup...@li...>]

Patches item #3091814, was opened at 2010-10-21 05:49
Message generated for change (Comment added) made by mroberto
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=841028&aid=3091814&group_id=166957

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: None
>Status: Pending
>Resolution: Postponed
>Priority: 1
Private: No
Submitted By: Stefan Sommerfeld (zerocom)
>Assigned to: Marcelo Roberto Jimenez (mroberto)
Summary: ixml warnings fixes

Initial Comment:
- Fixes to make ixml compiling without warnings with VS C++ (tested on VS 8)
- Removed upnpglobal dependency (proper IXML_INLINE definition needed)
- changed BOOL to IXML_BOOL to avoid conflicts

----------------------------------------------------------------------

>Comment By: Marcelo Roberto Jimenez (mroberto)
Date: 2010-10-21 10:40

Message:
Hi Stefan,

I see you did a lot of work, but we have to change a few things before all
your patches can go in.

First, your patch descriptions are too short. You say "fixed warning on
vc++", but you do not say which warning you are fixing. Also, you do a lot
more than that. You changed BOOL for IXML_BOOL, creating a new typedef in
the process. You should explain this in the commit message. And all other
issues too.

Second, you should copy your commit message explaining the patch to the
proper place in the Changelog file. Try to follow the writing standard used
there, do not use more than 80 columns and respect the spacing. Notice that
tabs are used and are equivalent to 8 spaces (linux kernel coding style).
You should not leave white spaces in the end of the lines, look:

$ git am ../00*
Applying: fixed warning on vc++
/home/mr/programs/pupnp/maint/sf.git/.git/rebase-apply/patch:97: trailing
whitespace.
        IXML_BOOL deep,
warning: 1 line adds whitespace errors.
Applying: fixed UpnpFinish (debugout after pthread finish) fixed warnings
(changed some types to size_t) fixed typos
Applying: fixed warnings (changed some types to size_t) added simple
strndup implementation for windows
Applying: fixed mixed usage of SOCKET and int fixed warnings
...

In the ideal world, after I give the command above, everything compiles ok
and I should not need to touch your patches, so it is essential that you
also address the Changelog on them.

In your patch, you define EXPORT_SPEC in ixml.h. Your definition is too
simplistic, this will probably break on other operating systems or even on
static library compilation. This is the reason why a dependency on
UpnpGlobal.h has been added. Proper solutions are either this or repeating
the already defined sequence in UpnpGlobal.h inside ixml.h. I have chosen
to leave the include dependency so that code is not replicated because it
is unlikely that libixml will be used without libupnp. There are thousands
of other XML implementations that are much better than this one, this is
only included here because of embedded systems.

Another point: it would be much better to remove BOOL completely instead
of creating another typedef. If you want to change BOOL all over to int, I
would be more than happy to commit. Typedefs are evil, they obfuscate code
instead of making it clear.

Please, try to address those issues and resubmit your patch.

I don't know what is your experience or taste with git, but if you are
confortable with rebases, amending and stuff, we can work in github, the
project is mirrored there too. Otherwise we can stay here in source forge
exchanging git patches in the tracker, that is ok too.

Thank you for your work! I hope you keep on with us.

Regards,
Marcelo.


----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=841028&aid=3091814&group_id=166957