#45 Fixes for compiling with Windows

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

Some fixes for compiling with Visual Studio, including fixes for some warnings.

Discussion

  • Marcelo Roberto Jimenez

    Hi Iain,

    Please, do not use name tags in your comments /* "ID: stuff" */. Just place the comments.

    The patch is overall ok, but this part is an important issue and should go in a different patch:

    diff --git a/head/libupnp/upnp/src/ssdp/ssdp_ctrlpt.c b/working/libupnp/upnp/src/ssdp/ssdp_ctrlpt.c
    index 1905d99..44fcabf 100644
    --- a/head/libupnp/upnp/src/ssdp/ssdp_ctrlpt.c
    +++ b/working/libupnp/upnp/src/ssdp/ssdp_ctrlpt.c
    @@ -452,7 +452,7 @@ int SearchByTarget(int Mx, char *St, void *Cookie)
    struct Handle_Info *ctrlpt_info = NULL;
    enum SsdpSearchType requestType;
    unsigned long addrv4 = inet_addr(gIF_IPV4);
    - int max_fd = 0;
    + SOCKET max_fd = 0;

    /\*ThreadData \*ThData; \*/
    ThreadPoolJob job;
    

    Please, try again using git. Also, whenever possible, try to apply your _fixes_ to branch-1.6.x, I will cherry-pick them to 1.8.0 (master).

    Regards,
    Marcelo.

     
  • - 2011-03-11

    Hi Marcelo

    Apologies for the name tags - again (see comments for patch ID: 3203113) I was copying the style of (some) other comments I had found around the code.

    I've fixed the issues and while doing so discovered some others which are included in this patch, which is now targeted at Windows in general rather than VC specifically.

    Given that all the issues are now much more related and many are to do with SOCKET related things I have not split the patch up, or added the issue you mention as a seperate patch. If you want me to split it up I'd happily do it, but I'd guess the important issue you mention is now related to others in this patch.

    In the future I will try and use the 1.6.x branch for changes (although all this set of changes still are on the master branch).

    Thanks,
    Iain

     
  • - 2011-03-11
    • summary: Fixes for compiling with VC++ --> Fixes for compiling with Windows
     
  • Marcelo Roberto Jimenez

    Hi Iain,

    Thank you again for your speed, and again, no need to apologize.

    I really prefer that you split this patch. You can do it easily with "git add -p", you will be asked if you want to include each hunk of the patch, in other words, you can add only some of the changes to a commit. The SOCKET related issues are important and deserve a separate patch.

    I still find some issues in this patch:

    In the code below, r is a variable, so "sizeof r" was fine. The type cast must be added:

    --- a/upnp/src/uuid/sysdep.c
    +++ b/upnp/src/uuid/sysdep.c
    @@ -106,7 +106,7 @@ void get_random_info(unsigned char seed[16])
    GetComputerName( r.hostname, &r.l );
    /* MD5 it */
    MD5Init(&c);
    - MD5Update(&c, &r, sizeof r);
    + MD5Update(&c, (unsigned char *)(&r), sizeof(r));
    MD5Final(seed, &c);
    };

    Also, you got one problem, but fixed it the wrong way:

    @@ -850,7 +861,11 @@ int http_WriteHttpPost( IN void *Handle,
    if (!tempbuf)
    return UPNP_E_OUTOF_MEMORY;
    /* begin chunk */
    +#ifdef UPNP_USE_MSVCPP
    + sprintf(tempbuf, "%Ix\r\n", *size);
    +#else
    sprintf(tempbuf, "%zx\r\n", *size);
    +#endif
    tempSize = strlen(tempbuf);
    memcpy(tempbuf + tempSize, buf, *size);
    memcpy(tempbuf + tempSize + *size, "\r\n", 2);

    We have created a macro PRIzu to deal with a similar situation, but you caught one we did not see before. I did the fix, please do a "git update" to get the latest code.

    Try as much as you can not to use ifdefs in the code, it makes it harder to read. This is one example. This issue is also mentioned in the coding style file link I sent you before.

    Regards,
    Marcelo.

     
  • - 2011-03-18

    Finally got round to splitting this up into two patches - both now uploaded. Think I've covered all the issues you had!

     
  • Marcelo Roberto Jimenez

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

    Sorry for the delay, patches accepted, thanks!

     

Log in to post a comment.