#44 Fixes for compiling under C++

closed-accepted
None
5
2011-03-11
2011-03-08
No

A variety of small changes to enable compiling under C++ - mainly adding casts to memory allocations

Discussion

  • Marcelo Roberto Jimenez

    • assigned_to: nobody --> mroberto
    • status: open --> pending
     
  • Marcelo Roberto Jimenez

    Hi Iain,

    As I posted in your previous patch, please use git to build them.

    There are some things I don't agree in your patch. But before all, the first thing to understand is that the language of the project is C, not C++, so I would advise you to compile it with a C compiler, not a C++ one. Insisting in using C++, there will be very subtle and hard to spot issues to deal and I don't want to go there.

    1 - You should not have to cast malloc() return values. This creates unnecessary code.
    2 - enuns and ints do not need typecasts too. The same reasoning as above apply.
    2 - The template "classes" are a C solution, not a C++ solution. These can be done in C++ much more elegantly with templates.
    3 - You should be consistent with the code formatting style of the project, particularly the use of spaces. We use the linux kernel coding style. Please read this http://lxr.linux.no/linux/Documentation/CodingStyle
    4 - extern "C" is acceptable on header files, but not on source files. Anyway, you have caught a mistake, the # character should not be in front of extern.

    You actually got this one right, as SOCKET was created for WIN32 compatibility:

    --- a/head/libupnp/upnp/src/genlib/net/sock.c
    +++ b/working/libupnp/upnp/src/genlib/net/sock.c
    @@ -91,7 +91,7 @@ int sock_destroy(SOCKINFO *info, int ShutdownMethod)
    if (sock_close(info->socket) == -1) {
    ret = UPNP_E_SOCKET_ERROR;
    }
    - info->socket = -1;
    + info->socket = (SOCKET)(-1);
    }

    return ret;
    

    A patch in this line of creating typecasts has close to zero chance to get applied.

    Since there are things that you have found that should be corrected, would you like to try again with git? Never mind breaking the patch into smaller pieces, if you build the patch right, it is not more work to me, it is less, believe me.

    I will wait for your answer, since I much rather prefer that you do the job instead of me.

    Regards,
    Marcelo.

     
  • - 2011-03-11

    Hi Marcelo

    Thanks for the detailed response.

    Apologies for the patch formatting issues - I had some trouble creating them, so it's no surprise they are a little broken. I'll try to do better this time! :oP

    On the issues with the patch content, I understand the language of the project is C - the reason for the patch is that I encountered problems with building the project with the Visual Studio solution files as provided - several of which were set to building as C++. Given this setting and the presence of #ifdef __cplusplus in many places I assumed the whole project was intended to be buildable as C++ while remaining fully correct C. I apologize for this misunderstanding - all the typecast changes were required for C++ compilation to succeed which is the only reason I added them (I realize that C++ has "better" alternatives for things like the templates, however it was not my goal to make it C++, just to enable it to compile as such. And I disliked the usage of extern C in source files - but it was simply a very quick (and easy) fix to the issues I was experiencing!)

    On the formatting - my changes _were_ consistent, just with the surrounding code. When making changes with code other people wrote I try to copy the style in the surrounding lines (in order to make the whole code easier to read). From now on, however, I will use the style you suggest.

    I have uploaded new a new patch file addressing the issues - hopefully this one is better.

    Note that I moved the SOCKET issue you mention into one of the other patches as it better fits with them.

    Thanks,
    Iain

     
  • Marcelo Roberto Jimenez

    Hi Iain,

    Thank you very much for your prompt action!

    The patch is fine and has been accepted.

    No need to apologize, this is part of the process.

    About the language issue, the headers may be included from a C++ source, and the library can also be linked to a C++ project, that is why there are #ifdef __cplusplus around the headers.

    About the coding style, I must add that the code in this library is not fully conforming, but we try to enforce it without being too radical on the new patches and additions.

    Regards,
    Marcelo.

     
  • Marcelo Roberto Jimenez

    • status: pending --> closed-accepted
     

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