Fl_Window's "Fl_X *i" causes "warning C4458: declaration of 'i' hides class member" in Visual Studio

[Remy Oukaour]

"i" is quite a common variable name, and there's probably a more descriptive name for this member.

There are various other warnings printed by the latest Visual Studio or gcc when compiling FLTK, but this one in particular affects application code extending Fl_Window.

[Albrecht Schlosser]

On 11/17/19 7:13 PM Remy Oukaour wrote:
> "i" is quite a common variable name, and there's probably a more
> descriptive name for this member.

I agree 100% that this variable (member) name is a bad choice, but I'm
afraid we can't do anything about this w/o breaking existing programs.
Even if we used a new name we'd have to keep the old one for
compatibility reasons.

One way I can imagine right now would be to rename the variable and
define "something" to keep the old variable name 'i', but this
"something" should obviously not be a macro.

We could also deprecate the usage of the variable, define a new member
(accessor) function to use it, and remove/rename it later - but given
our development speed this would take years. :-(

Suggestions welcome.

> There are various other warnings printed by the latest Visual Studio or
> gcc when compiling FLTK, but this one in particular affects application
> code extending Fl_Window.

Can you give a short, complete, compileable example how this "affects
application code extending Fl_Window" ? Please post a short example
program and the exact error message or warning you get, together with
the platform, compiler, and version.

Guessing: given the warning text in the subject "declaration of 'i'
hides class member" seems to imply that /you/ can't define another
variable 'i' in /your/ derived class because this definition would hide
the Fl_Window /private/ class member 'i'. Is this true? Or is it
anything else I don't see?

There's also (in class Fl_X):

static Fl_X* i(const Fl_Window* w) {return (Fl_X*)w->i;}

It's hard to tell what exactly triggers the warning w/o seeing code and
the exact compiler message.

[Greg Ercolano]

> On 11/17/19 7:13 PM Remy Oukaour wrote:
>> "i" is quite a common variable name, and there's probably a more
>> descriptive name for this member.

It's a common /local/ variable name, and as such should not
interfere with compiling user applications.

Where this would cause a problem is only if you use 'i' as
a class member or class method name, which is not advised
because as you say, it's not descriptive.

Which is perhaps a good reason FLTK uses it; no high level
user application should be using one letter names for methods
and members. And thus, arguably, a good reason FLTK uses them,
because they're not something a user app would use.

On 2019-11-18 07:49, Albrecht Schlosser wrote:
> I agree 100% that this variable (member) name is a bad choice, but I'm
> afraid we can't do anything about this w/o breaking existing programs.

Ditto -- Bad choice, but stuck with it because it's part of the public api;
a method name within the Fl_X class.

It's been in there since the beginning of time (fltk 1.0), so changing that
would break 20 years of legacy code built against FLTK.

When possible FLTK development opts not to break old code, instead
embracing non-critical API warts when possible. We only make API breaking
changes if something is truly egregious.

But why is this a problem really: one has to avoid several one letter names
for methods and classes that derive from Fl_Xxx widgets -- x,y,w,h, come to mind,
and for images d() isn't allowed, and probably others I can't remember.

> It's hard to tell what exactly triggers the warning w/o seeing code and
> the exact compiler message.

I too would like to see the actual warnings from the compiler and some context.

It should not be a problem because the method/member name is used within a class,
so it's not global scope. And applications deriving from classes must be aware
of the classes they derive from.

The problem here might be: I don't think there's docs for this Fl_X::i() method?
At least I didn't see any doxygen comments around that method declaration.

So we should /at least/ document this method.
FWIW, I imagine 'i()' stands for 'instance'?

[Albrecht Schlosser]

On 11/18/19 10:29 PM Greg Ercolano wrote:
>
> The problem here might be: I don't think there's docs for this Fl_X::i() method?
> At least I didn't see any doxygen comments around that method declaration.

I believe Fl_X is intentionally not documented. It's only mentioned in
chapter "Operating System Issues". Citation:

"(Allocate) a hidden class called an Fl_X, put the XID into it, ..."
https://www.fltk.org/doc-1.3/osissues.html#osissues_specialx

Emphasis on *hidden* class. But, although it's called "hidden", there
are some code examples using it.

> So we should /at least/ document this method.

... or better not. See above.

> FWIW, I imagine 'i()' stands for 'instance'?

Maybe. My interpretation is 'internal' because it's internal, OS
specific stuff, but maybe only Bill knows...

There's another *note* in this chapter: "Access to the Fl_X hidden class
requires to #define FL_INTERNALS before compilation", which IMHO clearly
states that Fl_X is internal stuff.

I would add: "this is subject to be changed in future versions w/o
notice; use at your own risk" or something like that, maybe.

[Greg Ercolano]

On 2019-11-18 14:29, Albrecht Schlosser wrote:
> On 11/18/19 10:29 PM Greg Ercolano wrote:
>>
>> The problem here might be: I don't think there's docs for this Fl_X::i() method?
>> At least I didn't see any doxygen comments around that method declaration.
>
> I believe Fl_X is intentionally not documented. It's only mentioned in
> chapter "Operating System Issues". Citation:
>
> "(Allocate) a hidden class called an Fl_X, put the XID into it, ..."
> https://www.fltk.org/doc-1.3/osissues.html#osissues_specialx
>
> Emphasis on *hidden* class. But, although it's called "hidden", there
> are some code examples using it.

Mmm, but if it's a public method that is affecting users deriving widgets
from things like Fl_Window, we probably should at least document that it's
not recommended for use and why, just so people know it's there.

Otherwise people will find it, see it's public, and use it, assuming it
simply was overlooked for documentation. It's not even 'protected', it's 'public'.

So I'll just offer this: might it be better to create a contract in the docs
saying it's there and not recommended for use, rather than leave undoc'ed
and hoping people won't find it.

And if not in doxygen, at minimum a non-doxygen comment with the method,
so if someone sees it, they'll at least know not to use it, or perhaps refer
to the OS specific docs that cover its use. Just suggesting..

[Remy Oukaour]

Here's a minimal example project: https://github.com/Rangi42/fltk-example

The entire source:

#pragma warning(push, 0)

#include <FL/Fl.H>

#include <FL/Fl_Window.H>

#include <FL/Fl_Box.H>

#pragma warning(pop)

class Example : public Fl_Window {

public:

Example(int X, int Y, int W, int H, const char *L = NULL);

};

Example::Example(int X, int Y, int W, int H, const char* L) : Fl_Window(X, Y, W, H, L) {

for (int i = 0; i < 4; i++) {

Fl_Box* box = new Fl_Box(16, 16 + 64 * i, 288, 64, "Hello World!");

box->box(FL_UP_BOX);

}

end();

}

int main(int argc, char **argv) {

Example *example = new Example(160, 144, 320, 288);

example->show(argc, argv);

return Fl::run();

}

And the compiler warning from Visual Studio 2019, version 16.3.9, on Windows 8.1 x64, built with warning level /W4:

1>------ Build started: Project: FLTK Example, Configuration: Debug Win32 ------

1>fltk-example.cpp

1>E:\fltk-example\src\fltk-example.cpp(13,16): warning C4458: declaration of 'i' hides class member

1>E:\fltk-example\include\FL\Fl_Window.H(97,9): message : see declaration of 'Fl_Window::i'

1>fltk-example.vcxproj -> E:\fltk-example\ide\..\bin\Debug\fltk-example-debug.exe

1>Done building project "fltk-example.vcxproj".

========== Build: 1 succeeded, 0 failed, 0 up-to-date, 0 skipped ==========

Note that when I remove the #pragmas from around the FLTK #includes, there are further warnings:

1>------ Rebuild All started: Project: FLTK Example, Configuration: Debug Win32 ------

1>fltk-example.cpp

1>E:\fltk-example\include\FL\Fl_Widget.H(372,40): warning C4244: '=': conversion from 'Fl_Boxtype' to 'uchar', possible loss of data

1>E:\fltk-example\include\FL\Fl_Widget.H(450,60): warning C4244: '=': conversion from 'Fl_Labeltype' to 'uchar', possible loss of data

1>E:\fltk-example\include\FL\Fl_Widget.H(466,49): warning C4244: '=': conversion from 'Fl_Labeltype' to 'uchar', possible loss of data

1>E:\fltk-example\include\FL\Fl_Window.H(438,97): warning C4458: declaration of 'minw' hides class member

1>E:\fltk-example\include\FL\Fl_Window.H(113,7): message : see declaration of 'Fl_Window::minw'

1>E:\fltk-example\include\FL\Fl_Window.H(438,97): warning C4458: declaration of 'minh' hides class member

1>E:\fltk-example\include\FL\Fl_Window.H(113,13): message : see declaration of 'Fl_Window::minh'

1>E:\fltk-example\include\FL\Fl_Window.H(438,97): warning C4458: declaration of 'maxw' hides class member

1>E:\fltk-example\include\FL\Fl_Window.H(113,19): message : see declaration of 'Fl_Window::maxw'

1>E:\fltk-example\include\FL\Fl_Window.H(438,97): warning C4458: declaration of 'maxh' hides class member

1>E:\fltk-example\include\FL\Fl_Window.H(113,25): message : see declaration of 'Fl_Window::maxh'

1>E:\fltk-example\include\FL\Fl_Window.H(438,97): warning C4458: declaration of 'dw' hides class member

1>E:\fltk-example\include\FL\Fl_Window.H(114,7): message : see declaration of 'Fl_Window::dw'

1>E:\fltk-example\include\FL\Fl_Window.H(438,97): warning C4458: declaration of 'dh' hides class member

1>E:\fltk-example\include\FL\Fl_Window.H(114,11): message : see declaration of 'Fl_Window::dh'

1>E:\fltk-example\include\FL\Fl_Window.H(438,97): warning C4458: declaration of 'aspect' hides class member

1>E:\fltk-example\include\FL\Fl_Window.H(114,15): message : see declaration of 'Fl_Window::aspect'

1>E:\fltk-example\src\fltk-example.cpp(13,16): warning C4458: declaration of 'i' hides class member

1>E:\fltk-example\include\FL\Fl_Window.H(97,9): message : see declaration of 'Fl_Window::i'

1>fltk-example.vcxproj -> E:\fltk-example\ide\..\bin\Debug\fltk-example-debug.exe

1>Done building project "fltk-example.vcxproj".

========== Rebuild All: 1 succeeded, 0 failed, 0 skipped ==========

[Remy Oukaour]

        When possible FLTK development opts not to break old code, instead 
        embracing non-critical API warts when possible. We only make API breaking 
        changes if something is truly egregious. 

My understanding was that FLTK 1.4 would make API-breaking changes; I've been using --with-abiversion=10305 for the sake of some such changes, like making certain members protected instead of private. 

On Sunday, November 17, 2019 at 1:13:05 PM UTC-5, Remy Oukaour wrote:

[Greg Ercolano]

On 2019-11-18 18:17, Remy Oukaour wrote:
> My understanding was that FLTK 1.4 would make API-breaking changes;

Note: ABI, not API.

We can break ABI on major releases, but try never to break API.
The abi=<version> flags are to get ABI breaking changes into
maintenance releases so they can be tried out and used as an abi option,
and then commit them into the next major release as the new default.

> like making certain members protected instead of private.

Yes -- we can open visibility up without breaking APIs; moving things
from being private to protected.

But we try not to do the reverse; once the cat's outta the bag as being public,
we don't try to get it back into the bag by making it protected or private.
By being public, it's already out.

At least with documented things.

With undocumented things, we /might/ make an exception, I'm not sure.
Apparently we have example code that Albrecht noticed which may mean we're
committed to it being public, as that means it can appear in user's existing
applications, and we don't want to break those.

[Manolo]

The warning mentionned in this thread results from private member variable "i" of the Fl_Window class.

That is not public member function Fl_X::i(), which is a public readonly accessor to that private variable.

Since this is a private member of an FLTK-defined class we can change its name with the certainty

not to break any user code. Since our CMP states that private members should be given names

ending with underscore, I intend to rename i as i_, unless there's opposition.

[Albrecht Schlosser]

Thanks, Manolo, that's exactly what I found out yesterday, but I
couldn't post my results due to other work.

I agree that we can rename the variable to another name with underscore,
but while we're doing this I also suggest to use a better, more
descriptive name as the OP mentioned.

I tried to refactor the code yesterday, using the name 'flx' (which
stands for 'a pointer to an instance of Fl_X') and my code is almost
ready to commit. I used 'flx' also because this variable name was not
used anywhere else, hence it should be easy to change it to something
even better, it was just a proof of concept. I did, however, not (yet)
add the underscore.

My hopefully final proposal would be to change the variable name to
'flx_' (with trailing underscore) and I'm willing to continue this work.
Better proposals would be appreciated.

I compiled my changes successfully on Linux and Windows but I'd need
help for testing on MacOS (and, to make it all complete, on Android,
with the SDL and pico drivers, etc). I can post a diff or add a test
branch in my FLTK fork for everybody to test.

The next step (already included in my code) would be to "rename" the
method Fl_X::i() to Fl_X::flx() or a corresponding name as well. I think
we can easily do this but keep the old Fl_X::i() as deprecated in FLTK
1.4 for backwards compatibility and remove it in FLTK 1.5 or any later
version.

Notes to the OP (Remy) and others:

(1) IMHO the Microsoft warning "declaration of 'i' hides class member
... 'Fl_Window::i'" is questionable or even wrong. How can a variable in
an outer scope (class Example) hide a private member in an inner scope
(Fl_Window) that is NOT VISIBLE in the outer scope (Example)? But, given
our CMP, that's not important and we should rename it anyway. Thanks for
the heads-up.

(2) The other Fl_Window private class members and the "shadowing"
warnings are similar (minw, minh, maxw, maxh, dw, dh). Although the same
as in (1) applies I think we should rename them as well (adding the
underscore).

(3) "warning C4244" is a known issue but benign and nobody found the
time to fix it. We could add some casts that would likely remove the
warnings but this would make the code longer and ugly. Anyway, it might
be done if someone could find the time to do it. Patches (valid for all
platforms!) always welcome...

[Albrecht Schlosser]

On 11/20/19 2:49 PM Albrecht Schlosser wrote:
>
> I tried to refactor the code yesterday, using the name 'flx' (which
> stands for 'a pointer to an instance of Fl_X') and my code is almost
> ready to commit. I used 'flx' also because this variable name was not
> used anywhere else, hence it should be easy to change it to something
> even better, it was just a proof of concept. I did, however, not (yet)
> add the underscore.
>
> My hopefully final proposal would be to change the variable name to
> 'flx_' (with trailing underscore) and I'm willing to continue this work.
> Better proposals would be appreciated.
>
> I compiled my changes successfully on Linux and Windows but I'd need
> help for testing on MacOS (and, to make it all complete, on Android,
> with the SDL and pico drivers, etc). I can post a diff or add a test
> branch in my FLTK fork for everybody to test.

You can check out my branch 'refactor_Fl_Windows_i' form my fork
'https://github.com/Albrecht-S/fltk' if you want to test. I'd
particularly appreciate all platforms except Windows and Linux (which I
can test myself).

In case someone needs a patch instead, I'm attaching a diff to this post
(~41 KB).

[Manolo]

I built and ran successfully here under macOS your branch 'refactor_Fl_Windows_i'.

[Albrecht Schlosser]

On 11/21/19 4:47 PM Manolo wrote:
> I built and ran successfully here under macOS your branch
> 'refactor_Fl_Windows_i'.

Great! Thanks for testing, I appreciate this very much.

One missing piece of this branch is the backwards compatible static
method Fl_X::i(...) which I renamed to Fl_X::flx();

I intend to add it with a corresponding comment that it's deprecated and
only for backwards compatibility. I also need to update the docs. Once
I'm done with this I'll post something in fltk.coredev.