Fixed version of "no more warnings"

[ingowald]

Now only defining a single macro in internal.h, and including this in tools. Macro now fixed, too. (I hope)

[josch]

Thanks, but I have another nitpick:

• why do you open a new pull request instead of fixing up the old one?
• why do you propose two commits (one with a problem and one that fixes the problem) instead of a single working commit?

If you need help, I can merge your commits for you. Otherwise, if you want to experiment, feel free to try it out. ;)

[nigels-com]

I think there are a few things that could be cleaner in the change, but I'll be glad to have things compiling cleaner for gcc 5 and gcc 6.

[ingowald]

Quite frankly, if you really want to go "true" C++ there's bigger fish to fry. For instance, All the "GLUI_Xyz" class names should be changed to a namespace glui, and then glui::Xyz; all the errors etc should become exceptions, all the "set_int" should become polymorphic set(int)'s, all the

define constants should become enums and god knows what else (probably all

the callback function pointer should become template functor objects... ugh). For now (having used the original glui before) my goal is to get this version to compile w/o errors and warnings on all the platforms that our 'main' software supports - at minimum changes to the lib itself - so I can use it in there as a git submodule (rather than using pre-built libraries).

On Mon, Dec 5, 2016 at 1:35 AM, Nigel Stewart notifications@github.com wrote:

@nigels-com commented on this pull request.

In glui_internal.h https://github.com/libglui/glui/pull/57#pullrequestreview-11342292:

@@ -20,6 +20,7 @@

include

include

+#include

In C++ it's recommended to #include

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/libglui/glui/pull/57#pullrequestreview-11342292, or mute the thread https://github.com/notifications/unsubscribe-auth/ABwZtbEoTi9QZ-bLWUpQb5fF9bZ1fwd-ks5rE9rEgaJpZM4LDwwn .

[ingowald]

Wow. I'm coding low-level C for 20ish years ... and I've never ever even heard of this one. Now that doesn't happen all that often any more. Thanks man!

On Mon, Dec 5, 2016 at 1:39 AM, josch notifications@github.com wrote:

@josch commented on this pull request.

In glui_textbox.cpp https://github.com/libglui/glui/pull/57:

{ fprintf( out,

• "%s (edittext@%p): line:%d ins_pt:%d subs:%d/%d sel:%d/%d len:%d\n",
• "%s (edittext@%p): line:%d ins_pt:%d subs:%d/%d sel:%d/%d len:%ld\n", name, this, curr_line, insertion_pt, substring_start, substring_end, sel_start, sel_end, text.length());

http://stackoverflow.com/questions/2524611/ suggests that %z is the right modifier.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/libglui/glui/pull/57, or mute the thread https://github.com/notifications/unsubscribe-auth/ABwZtSkQLrZonTJPxTHKKE-zoHNi6ImVks5rE9vdgaJpZM4LDwwn .

[ingowald]

Lol. This is the first time I have to use this "pull request" thing at all

• in all other projects I'm on I have full write permissions, so we do this through branches. Hence, I didn't even know that you can change existing pull requests ;-)

On Sun, Dec 4, 2016 at 10:21 PM, josch notifications@github.com wrote:

Thanks, but I have another nitpick:

• why do you open a new pull request instead of fixing up the old one?
• why do you propose two commits (one with a problem and one that fixes the problem) instead of a single working commit?

If you need help, I can merge your commits for you. Otherwise, if you want to experiment, feel free to try it out. ;)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/libglui/glui/pull/57#issuecomment-264776508, or mute the thread https://github.com/notifications/unsubscribe-auth/ABwZtbNZDNETnYLUFYgPgL-rwd9q6MC4ks5rE61xgaJpZM4LDwwn .

[ingowald]

Yeah; the reason I prefer the macro is that it keeps the "temporary" return value ('n' in above) as a local inside its own scope. This means it can be used multiple times in the same parent scope, without fear of polluting the scope, duplicate definitions of 'n', etcpp. But as said above/below, I don't particularly care as long as it all builds without warnings/errors.

On Mon, Dec 5, 2016 at 1:34 AM, Nigel Stewart notifications@github.com wrote:

@nigels-com commented on this pull request.

In tools/ppm.cpp https://github.com/libglui/glui/pull/57#pullrequestreview-11342086:

@@ -46,13 +47,13 @@ void LoadPPM(const char FileName, unsigned char &Color, int &Width, int &Heigh int c,s; do{ do { s=fgetc(fp); } while (s!='\n'); } while ((c=fgetc(fp))=='#'); ungetc(c,fp);

• fscanf(fp, "%d %d\n255\n", &Width, &Height);
• AssertResult(2,fscanf(fp, "%d %d\n255\n", &Width, &Height));

I'd actually prefer to see this expanded out, rather than wrapped in a custom macro:

n = fscanf(fp, "%d %d\n255\n", &Width, &Height); assert(n==2);

It's more likely that other developers are familiar with assert

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/libglui/glui/pull/57#pullrequestreview-11342086, or mute the thread https://github.com/notifications/unsubscribe-auth/ABwZtcaFLLW9izzyYIUnJtV6ZKn0CU70ks5rE9qNgaJpZM4LDwwn .

[josch]

No worries, if you don't want to deal with this github interface (I can definitely understand that) then as I offered, I can just do the final cleanups myself.

[nigels-com]

No drama. I review a lot of code, day to day. I tend to lean towards blunt clarity rather than flowery praise. C code does tend to lean on macros more than C++ code. It's no big deal, it's just one opinion among many.

GLUI is a fairly old C++ codebase, I don't think we should get carried away and make huge structural changes. But a bit of maintenance here and there to keep the compilers happy - yes indeed.

[nigels-com]

Declining pull request for now, due to broader divergence/conflicts.