Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code Tidy-up: Use private static methods for primary GLUT callbacks #68

Merged
merged 1 commit into from Jun 25, 2017

Conversation

nigels-com
Copy link
Collaborator

Improved object-orientation, hiding internals.

@nigels-com
Copy link
Collaborator Author

Done.

@nigels-com nigels-com merged commit f3d8133 into libglui:master Jun 25, 2017
@nigels-com nigels-com deleted the glut-callbacks-static branch June 28, 2017 10:18
@m-7761
Copy link

m-7761 commented Jun 28, 2017

Since you directed me here, I don't understand why these are under GLUI_Main. If they are static why not move them into glui.cpp in the global namespace?

The convention I use that I believe works best is to whenever possible put procedures in the CPP file they are needed in, making them "static" by default (avoid the "anonymous namespace" because it's too attention grabbing) and name them with the name of the CPP file with the extension removed and an underscore where the . would be. If I want to work in a namespace I use the name of the file with the _cpp as the name of its namespace. (So things done in its namespace can not use the prefix.)

This way if a state needs to be 'extern' the "static" can just be replaced with "extern" and that marks it as shared with another CPP file somewhere, and there will be a matching extern in those files where it's shared. And the "extern" state will be named after the file it originates.

In this case, these callbacks would still be called glui_x because they'd live in the "glui.cpp" file. It's a good system. Think of the TUs as big master objects. In any case, parking these under GLUI_Main just clutters its namespace, so clients are presented with a menu of red herrings when they use auto-complete facilities.

@nigels-com
Copy link
Collaborator Author

Ah, the auto-complete can't distiguish between private and public?

@m-7761
Copy link

m-7761 commented Jun 29, 2017

I set mine to not distinguish. IDEs can't even change formatting preferences on a per project basis. How do they know if data is interesting to the programmer or not? Private members are interesting to the developers working with (typing) them.

So no. And generally they are interesting too. Clutter should be minimized. If something is only used in one file and is static then it has no earthly business existing outside of that file. The linker has to consider everything that's not static to the translation-unit. You think oh it doesn't matter, but a million things that don't matter funnily in concert do matter, so it's a matter of developing good habits.

@nigels-com
Copy link
Collaborator Author

It seems to me a normal sort of thing to have static private functions for callback purposes, assuming that they're truely private. The alternative is polluting the global namespace, which you seem to be proposing for the sake of code completion convenience. I really don't mind much either way, but it doesn't seem much to do with good habits.

@m-7761
Copy link

m-7761 commented Jun 29, 2017

The alternative is polluting the global namespace

You misunderstand. The glui.cpp file isn't the global namespace. Where its code begins is the end of the global namespace as far as pollution is concerned. This is code 101. The whole idea of pollution is to keep things out of the CPP (translation-unit) that it doesn't need or want. Well, glui.cpp is the CPP. And everything in it is completely outside the scope of all other translation-units by definition.

Think about it.

@nigels-com
Copy link
Collaborator Author

Sure, it's out of the header. But it's still the global namespace from the perspective of the .cpp

@nigels-com
Copy link
Collaborator Author

So speaking of namespaces, next on my list was to move GLUI to a namespace and remove the GLUI_ prefix from all the class names. In that arrangement I suppose "polluting" the GLUI namespace is less of a concern. But I think I still prefer the to be private to the relevant class.

@m-7761
Copy link

m-7761 commented Jun 29, 2017

So speaking of namespaces, next on my list was to move GLUI to a namespace and remove the GLUI_ prefix from all the class names. In that arrangement I suppose "polluting" the GLUI namespace is less of a concern. But I think I still prefer the to be private to the relevant class.

I think that's good and will make it easier to reintegrate what work I do. But the GLUI::Main namespace is a different namespace from the GLUI namespace. Adding members that could be static/internal to a TU increases the linker's workload and is just untidy. (Something's scope should be as narrow as its use. Period.)

If these private members used an underscore prefix (which I know is controversial but still beats the hell out of any alternative) it would be less of a nuisance. But it would still be pointless and misleading to file them as static private members. Something programmers (people) seem unable to appreciate at large is that the absence of something is every bit as meaningful as the presence of something. (That is to say for example if you put parentheses around every term in every expression then you are reducing the meaning of parentheses, like the little boy who cries wolf.)

@nigels-com
Copy link
Collaborator Author

Can we compromise and change them to private with _ prefix?

@m-7761
Copy link

m-7761 commented Jul 1, 2017

Can we compromise and change them to private with _ prefix?

Sure. If it feels better :) I don't really see a value in exposing it. I guess in a debugger a user can see what functions GLUI has assigned to them (these are just GLUI's own fixed callbacks right?) and maybe jump to them through the memory inspector.

Off-topic: Today I saw some GitHub action: nanoant/CMakePCHCompiler#11

That reminds me, I downloaded the GitHub Desktop app, but neglected to install it. I found the edit function that lets you do edits/commits in the browser. That's pretty nifty for small changes I'll admit. The pull-request workflow seems more logical there, and I got the impression that forking is the normal thing if you don't have write-access. It defaults to doing a pull-request via a branch; but I think it would make more sense to default via a fork (or to use common sense and see if the logged in user can even edit the branches.)

(I had to get precompiled headers working with COLLADA-DOM because its generated headers are pretty insane; especially since I switched them over to using templates to do crazy things (every value has a pointer-to-member to itself and so can statically talk to its container.) The first PCH (GCH) was 1.8GB and it is only the header description of parts of two schemas. Granted I regret installing 64-bit Cygwin and I just removed the debug information, so next rebuild it should get smaller.)

ctype.h goes crazy with macros. It uses _U,L,N,S,P,C,X,B to define numbers. When you start using _ prefixes you're bound to run into trouble eventually, but the GCC system headers really take the cake. Still I really like using _ and __. Nothing else works so well. It's stupid that C reserves those for the system. Especially since the end of the "Hungarian" debate. (God help us all.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants