#38 ixml warnings fixes

closed-postponed
None
1
2010-11-04
2010-10-21
No

- 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

Discussion

  • Marcelo Roberto Jimenez

    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.

     
  • Marcelo Roberto Jimenez

    • assigned_to: nobody --> mroberto
    • priority: 5 --> 1
    • status: open --> pending-postponed
     
  • SourceForge Robot

    • status: pending-postponed --> closed-postponed
     
  • SourceForge Robot

    This Tracker item was closed automatically by the system. It was
    previously set to a Pending status, and the original submitter
    did not respond within 14 days (the time period specified by
    the administrator of this Tracker).

     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:

JavaScript is required for this form.





No, thanks