Microsoft Visual Studio Support

[Albrecht Schlosser]

Hi Devs,

I found some incompatibilities when I tried to compile our current code with MS Visual Studio 2010 and 2008, the oldest versions I have available. These incompatibilities were in different "classes" of warnings/errors:

1) compiler warnings
2) compilation errors
3) compile and build errors

Points 1 and 2 are "simple" but point 3 is annoying because vsscanf() is not available in older MSVC versions, but it exists in current versions.

So the question is: do we care about older MSVC versions than, say, VS 2015 (which is currently documented on the MS site) or what should be the oldest MSVC version we are supporting?


@Greg: ISTR that you're using an older version for compatibility reasons. Can you please

(a) tell us which (oldest) version you need to compile your applications, and
(b) if the current code can still be compiled?

I wouldn't be surprised if the following commit broke builds with "ancient" MSVC versions (like VS 2010, VS 2008, and older):

commit 09eff7243a6e8e37d9615df7b951ffa3c03c0ae2
Author: Matthias Melcher <git...@matthiasm.com>
Date:   Wed Jan 19 16:08:29 2022 +0100
...

I don't know which version between VS 2010 and VS 2015 add vsscanf() support (VS 2015 has it).

The attached patch addresses the 'vsscanf' issue so I could build FLTK but it is definitely NOT a solution - it returns 0 rather than using a replacement function.

Comments and suggestions welcome. Patches with working replacement functions for vsscanf() would also be appreciated. Personally I don't feel comfortable writing such a patch and I don't have the time to do it.

[imacarthur]

On Monday, 21 March 2022 at 17:54:42 UTC Albrecht Schlosser wrote:

Hi Devs,

I found some incompatibilities when I tried to compile our current code with MS Visual Studio 2010 and 2008, the oldest versions I have available. These incompatibilities were in different "classes" of warnings/errors:

1) compiler warnings
2) compilation errors
3) compile and build errors

Points 1 and 2 are "simple" but point 3 is annoying because vsscanf() is not available in older MSVC versions, but it exists in current versions.

So the question is: do we care about older MSVC versions than, say, VS 2015 (which is currently documented on the MS site) or what should be the oldest MSVC version we are supporting?

 

I don't know which version between VS 2010 and VS 2015 add vsscanf() support (VS 2015 has it).

It looks like it might have been added in VS2013 (but I can't confirm that, as I do not have 2013 to check against...) 

 

The attached patch addresses the 'vsscanf' issue so I could build FLTK but it is definitely NOT a solution - it returns 0 rather than using a replacement function.

Comments and suggestions welcome. Patches with working replacement functions for vsscanf() would also be appreciated. Personally I don't feel comfortable writing such a patch and I don't have the time to do it.

This works, providing the number of varargs is 5 or fewer - note this was tested on mingw (not on VS) where vsscanf is available anyway, so I called both the "real" vsscanf and my proposed substitute.

// Test of dummy vsscanf option //

#include <stdio.h>
#include <stdarg.h>

// substitute vsscanf implementation
int like_vsscanf (const char *str, const char *fmt, va_list p_list)
{
    const int max_args = 5;
    void *argp_array [max_args];
    int idx;
    for (idx = 0; idx < max_args; idx++)
    {
        argp_array[idx] = NULL;
    }

    void *nxt_arg = va_arg(p_list, void*);
    idx = 0;
    while ((nxt_arg) && (idx < max_args))
    {
        argp_array[idx] = nxt_arg;
        idx++;
        nxt_arg = va_arg(p_list, void*);
    }

    return sscanf(str, fmt, argp_array[0], argp_array[1], argp_array[2], argp_array[3], argp_array[4]);
} // like_vsscanf

// wrapper to test substitute vsscanf implementation
int test_of_like_vsscanf (const char *str, const char *fmt, ...)
{
    va_list argp;
    va_start(argp, fmt);
    int res = like_vsscanf(str, fmt, argp);
    va_end(argp);
    return res;
} /* test_of_like_vsscanf */

// wrapper to test "real" vsscanf implementation
int test_of_vsscanf (const char *str, const char *fmt, ...)
{
    va_list argp;
    va_start(argp, fmt);
    int res = vsscanf(str, fmt, argp);
    va_end(argp);
    return res;
} /* test_of_vsscanf */

int main(void)
{
   int i1 = 0, i2 = 0, i3 = 0, i4 = 0;
   int res;
   res = test_of_vsscanf ("9 8 7", "%d %d %d", &i1, &i2, &i3);
   printf ("Saw: %d %d %d (%d)\n", i1, i2, i3, res);

   res = test_of_like_vsscanf ("6 5 4 3", "%d %d %d %d", &i1, &i2, &i3, &i4);
   printf ("Saw: %d %d %d %d (%d)\n", i1, i2, i3, i4, res);

   return 0;
} // main

/* End of File */

 

[imacarthur]

Doh! 

I just *actually looked* at Albrecht's patch, and realised it is "vsscanf_l" that we need here, not "vsscanf"...

 

 

[Albrecht Schlosser]

On 3/22/22 16:08 imacarthur wrote:

Doh! 

I just *actually looked* at Albrecht's patch, and realised it is "vsscanf_l" that we need here, not "vsscanf"...

Thanks for your help!

Please don't take my (pseudo) patch too seriously. It may well be that we need vsscanf as well, but I'm not sure.

Fact is that the latest master contains code that can't be built with VS 2010 and earlier [1] because none of the vsscanf variants is available in these VS versions. I don't know about any versions between 2010 and 2015 [2]. The latter has it according to MS docs but docs of older MS versions don't seem to be available. Maybe in the web archive...


[1] I have VS 2008, 2010 and 2019 available for testing and I can certainly install VS 2022 as well (but I don't intend to do this soon).

[2] I found download links for older versions (2012, 2013, 2015, 2017) here: https://visualstudio.microsoft.com/vs/older-downloads/

[imacarthur]

On Tuesday, 22 March 2022 at 15:26:49 UTC Albrecht Schlosser wrote:

On 3/22/22 16:08 imacarthur wrote:

Doh! 

I just *actually looked* at Albrecht's patch, and realised it is "vsscanf_l" that we need here, not "vsscanf"...

Thanks for your help!

Please don't take my (pseudo) patch too seriously. It may well be that we need vsscanf as well, but I'm not sure.

Hmm... I wasn't at all familiar with vsscanf_l, so went googling, and it rather looks like it might be a BSD'ism (and hence present in Darwin/macOS) but not generally available anyway - indeed our "Fl_UNIX_System_Driver" does not use it (only uses vsscanf) and I rather think that the WIN32 driver should not use it either... Though it would still need vsscanf.

Fact is that the latest master contains code that can't be built with VS 2010 and earlier [1] because none of the vsscanf variants is available in these VS versions. I don't know about any versions between 2010 and 2015 [2]. The latter has it according to MS docs but docs of older MS versions don't seem to be available. Maybe in the web archive...

Maybe so - I do not see vsscanf_l on the MSDN docs (though it looks like vsscanf is there for the current version) so  in any case our code that uses _vsscanf_l looks likely to be wrong, even with the latest MS tools - unless someone knows different?

[imacarthur]

On Tuesday, 22 March 2022 at 15:39:15 UTC imacarthur wrote:

Please don't take my (pseudo) patch too seriously. It may well be that we need vsscanf as well, but I'm not sure.

Hmm... I wasn't at all familiar with vsscanf_l, so went googling, and it rather looks like it might be a BSD'ism (and hence present in Darwin/macOS) but not generally available anyway - indeed our "Fl_UNIX_System_Driver" does not use it (only uses vsscanf) and I rather think that the WIN32 driver should not use it either... Though it would still need vsscanf.

Fact is that the latest master contains code that can't be built with VS 2010 and earlier [1] because none of the vsscanf variants is available in these VS versions. I don't know about any versions between 2010 and 2015 [2]. The latter has it according to MS docs but docs of older MS versions don't seem to be available. Maybe in the web archive...

Maybe so - I do not see vsscanf_l on the MSDN docs (though it looks like vsscanf is there for the current version) so  in any case our code that uses _vsscanf_l looks likely to be wrong, even with the latest MS tools - unless someone knows different?

So I went looking - and I'm now (pretty well) convinced that "vsscanf_l" is  a BSD'ism and should not be in the Windows driver at all - it looks like our Windows implementation of clocale_sscanf() was largely cut'n'pasted from the Darwin/macOS version, whereas the Unix driver version could have been more appropriate.

Or maybe even just...

int Fl_WinAPI_System_Driver::clocale_sscanf(const char *input, const char *format, va_list args) {

  char *saved_locale = setlocale(LC_NUMERIC, NULL);

  setlocale(LC_NUMERIC, "C");

  int retval = vsscanf(input, format, args);

  setlocale(LC_NUMERIC, saved_locale);

  return retval;

}

Though even that assumes that at least vsscanf() is available -- which will not be the case in pre-2013 VS versions AFAICT.

All that aside, our clocale_sscanf() methods are all misnamed (we have 4 of them, in Fl_System_Driver, Fl_Unix_System_Driver, Fl_Darwin_System_Driver and Fl_WinAPI_System_Driver.)

ALL of these should have been called "clocale_vsscanf" (with a "V") instead, since they take a va_list item, rather than a varags list "..."

I note that there is also a wrapper in Fl_preferences that is correctly called clocale_sscanf() that does take a "..." vararg so it is OK - these others are all misnamed here.

Should we correct the naming now?

I do not know, but AFAICT the only place they are used is from Fl_Preferences, so the "damage" from renaming them would be very limited.

I doubt anyone has used these methods directly in user code!

By the same token, in our driver layer (for each platform) we also have clocale_printf that should be clocale_vprintf and clocale_snprintf that should be clocale_vsnprintf

since both take a va_list rather than "..." as the parameter...

[Albrecht Schlosser]

On 3/22/22 17:18 imacarthur wrote:

On Tuesday, 22 March 2022 at 15:39:15 UTC imacarthur wrote:

Hmm... I wasn't at all familiar with vsscanf_l, so went googling, and it rather looks like it might be a BSD'ism (and hence present in Darwin/macOS) but not generally available anyway - indeed our "Fl_UNIX_System_Driver" does not use it (only uses vsscanf) and I rather think that the WIN32 driver should not use it either... Though it would still need vsscanf.

[...] I do not see vsscanf_l on the MSDN docs (though it looks like vsscanf is there for the current version) so  in any case our code that uses _vsscanf_l looks likely to be wrong, even with the latest MS tools - unless someone knows different?

So I went looking - and I'm now (pretty well) convinced that "vsscanf_l" is  a BSD'ism and should not be in the Windows driver at all - it looks like our Windows implementation of clocale_sscanf() was largely cut'n'pasted from the Darwin/macOS version, whereas the Unix driver version could have been more appropriate.

Or maybe even just...

int Fl_WinAPI_System_Driver::clocale_sscanf(const char *input, const char *format, va_list args) {

  char *saved_locale = setlocale(LC_NUMERIC, NULL);

  setlocale(LC_NUMERIC, "C");

  int retval = vsscanf(input, format, args);

  setlocale(LC_NUMERIC, saved_locale);

  return retval;

}

Though even that assumes that at least vsscanf() is available -- which will not be the case in pre-2013 VS versions AFAICT.

OK, but this is better than using the wrong method. Once this is corrected we can still tackle the vsscanf() issue for VS < 2013.

BTW: I'm thinking about adding a feature check for vsscanf() at least in CMake (which is always used to create the VS build environment). I'll check this option later so we can maybe simplify the #if... logic in the driver files a little bit. Feature checks are always more appropriate than "guessing" based on the compiler version. But this should not stop us doing all the other changes mentioned above and below.

All that aside, our clocale_sscanf() methods are all misnamed (we have 4 of them, in Fl_System_Driver, Fl_Unix_System_Driver, Fl_Darwin_System_Driver and Fl_WinAPI_System_Driver.)

ALL of these should have been called "clocale_vsscanf" (with a "V") instead, since they take a va_list item, rather than a varags list "..."

I note that there is also a wrapper in Fl_preferences that is correctly called clocale_sscanf() that does take a "..." vararg so it is OK - these others are all misnamed here.

Should we correct the naming now?

Yes, please. It would be a great help if you could do this (since you dived into the code already). TIA

This (renaming) and the replacement of the wrong code above would be very much appreciated.

I do not know, but AFAICT the only place they are used is from Fl_Preferences, so the "damage" from renaming them would be very limited.

I doubt anyone has used these methods directly in user code!

These are all driver methods derived from Fl_System_Driver whose header is private (in the src/ folder), hence they can't be called in user code. And I'm pretty sure most of these methods have been added only recently (likely this year) when Matt updated Fl_Preferences.

Now is the right time to change all this!

By the same token, in our driver layer (for each platform) we also have clocale_printf that should be clocale_vprintf and clocale_snprintf that should be clocale_vsnprintf

since both take a va_list rather than "..." as the parameter...

Again, since you did already look at it, I'd appreciate if you could change this as well.

Otherwise, please reply here, and I'll take a look and fix it according to the info posted here by you. But I'd really appreciate if you could do it or at least provide one or two complete patches. Thank you very much in advance!

[imm]

On Tue, 22 Mar 2022, 16:53 Albrecht Schlosser wrote:

Yes, please. It would be a great help if you could do this (since you dived into the code already). TIA

This (renaming) and the replacement of the wrong code above would be very much appreciated.

OK, I'll have a look, though won't be this evening (currently watching a kids water polo match, now they're allowed to play competitive matches again...)

The preferred approach is to make a clone on GitHub, do the patch, then send (you?) a PR, I think?

Is that the way?

--
Ian
From my Fairphone FP3

[Albrecht Schlosser]

On 3/22/22 20:11 imm wrote:

On Tue, 22 Mar 2022, 16:53 Albrecht Schlosser wrote:

Yes, please. It would be a great help if you could do this (since you dived into the code already). TIA

This (renaming) and the replacement of the wrong code above would be very much appreciated.

OK, I'll have a look, though won't be this evening (currently watching a kids water polo match, now they're allowed to play competitive matches again...)

Have fun!

The preferred approach is to make a clone on GitHub, do the patch, then send (you?) a PR, I think?

Is that the way?

Well, you have write access, so you could commit and push to GitHub (fltk/fltk) yourself.

However, if you prefer to make a PR so someone can review it, then this is how it works:

1) fork on GitHub (fltk/fltk to your-account/fltk)
2) create a working branch on your fork on top of master
3) push your commit(s) to this branch
4) create a PR on GitHub

There's no one you send the PR to. You just create it directly on GitHub (web interface).

You'll get instructions when you push to your branch (point 3 above) for the first time.

In short commands:

1) fork on GitHub (user interface)
2) git clone <your-URL> <your-working-dir>
3) cd <your-working-dir>
4) git checkout master (this is the default anyway)
5) git branch working
6) git checkout working
7) git add, git commit, ...
8) git push <remote> working
9) create PR on GitHub or as instructed

Notes:

- In (2) <your-URL> is the likely https://github.com/UserName/fltk.git
- In (2) and (3) <your-working-dir> would likely be 'fltk'
- I used the branch name 'working' (select a better one)
- In (7) the remote name defaults to 'origin' if you forked and cloned as described above
- If you want to push to your fork with a 'https' URL you need to create a "personal access token" (with "repo" permission) on your GitHub account first (only once) and use your username with this token as the password to authenticate. Store this token in a safe place, you can't see it again (but you can create another one if you lost it). Alternatively you can set up git/ssh access but that would be OT here.
- When you do (8) on the commandline you'll get instructions how to create the PR
- Since you have write access you can also finally "merge" the PR on GitHub or let someone else (me?) do it.

Thanks
Albrecht

[imacarthur]

On Tuesday, 22 March 2022 at 20:18:10 UTC Albrecht Schlosser wrote:

Well, you have write access, so you could commit and push to GitHub (fltk/fltk) yourself.

I could, but I'd still rather someone else looked over the change first - it's the process we follow at work...

However, if you prefer to make a PR so someone can review it, then this is how it works:

PR #420 raised - think it is OK.

Tried the code on Windows (mingw), *nix (Ubuntu) and on my macOS box. Seems "fine", or at least it builds and runs!

BUT - it turns out that none of the Windows machines I actually have here, have VS installed, and certainly no pre-2013 version... So the actual "vsscanf on old VS versions" issue that started all this is still unaddressed...