OpenTK.Bind project refactoring
Posted Friday, 14 August, 2009 - 20:04 by Barlog| Project: | The Open Toolkit library |
| Version: | 0.9.9-2b |
| Component: | Code |
| Category: | task |
| Priority: | minor |
| Assigned: | Barlog |
| Status: | closed |
Description
Greetings.
I've decided to make some code cleanup on your OpenTK project.
Here it is "Bind project refactoring". Made on revision 2125 from SVN. Diff patch in attachment.
Could you apply it and test if anything is done right? Unfortunately, I do not have access to MSVS 2005 and Mono. (I'm using MSVS 2008)
Any feedback is appreciated. :) And I think there would be a lot of it, because some parts of my refactorings can make project uncompilable on MSVS 2005 and/or Mono. Sorry about that.
Best regards.
| Attachment | Size |
|---|---|
| BindProjectRefactoringDiffPatch.zip | 14.09 KB |
Comments
#1
Thank you for your patch. The Bind project is the oldest and most convoluted part of OpenTK - any attempt to clean it up is welcome.
From a quick glance, it seems that most changes are relatively low-risk code cleanups (adding using directives, replacing 'if' statements with switches or the ternary operator, removing unused code) plus a few bug fixes. A few parts are using C# 3.0 features, but these seem few and far between. No large-scale structural changes that I could see.
Have you verified that the output remains the same? For example, running Bind.exe with parameter "-mode:gl" should either leave GL.cs, GLCore.cs etc unchanged or fix specific issues.
Also, are you planning to make larger fixes/modifications to Bind? If so, be warned that several parts are rather fragile (especially the spec reader and wrapper generation code). The code in SVN is somewhat better than what we had one or two releases ago, but it still has a long way to go.
The biggest structural issues are:
In any case, thank you for the patch - I'll test it and post results.
#2
Have you verified that the output remains the same? For example, running Bind.exe with parameter "-mode:gl" should either leave GL.cs, GLCore.cs etc unchanged or fix specific issues.
As you suggested, I have compared output between Bind revision 2125 (head revision from svn) and my refactored version. No changes in auto-generated GL, ES, CL output files.
Also, are you planning to make larger fixes/modifications to Bind?
I won't plan to do larger fixes until I better understand whole project structure.
Currently, I'm planing to do same "cosmetic" refactorings to whole OpenTK project. Refactoring Bind was first part of it.
#3
Thanks, I'll check this patch in VS2005 and commit it.
#4
Ok, I have merged the patch without the C# 3.0 changes, but either the patch or my modifications seem to be causing some issues with the output (missing CLSCompliant attributes).
I will try to find the cause and fix it, but this will take some time as it is not a high priority.
One suggestion: try to separate patches into smaller, distinct parts. For example, this patch could be conceptually split between "patch to remove unnecessary using directives", "patch to reduces explicit namespace usage (e.g. Generator vs Bind.GL2.Generator)", "patch to replace if statements with switches". This makes reviews much easier. Also, even if a part contains a bug, the rest can still be applied.
Unfortunately, SVN does not offer enough control to do this sort of stuff easily. Bazaar, Git, etc are much better in this regard, with support for local commits (maybe TortoiseBzr and bzr-svn can offer a simple solution).
#5
Ok, I have merged the patch without the C# 3.0 changes, but either the patch or my modifications seem to be causing some issues with the output (missing CLSCompliant attributes).
Hmm... That is not good. I think I need to refactor code again and split changes between different patches as you suggest. I'll do it in couple of hours.
I will try to find the cause and fix it, but this will take some time as it is not a high priority.
I've posted patch with, possible, glitches. And you will try to find and to fix them? I think that is not fair. I'll try to pinpoint them myself.
Best regards.
#6
Thank you.
If you do this, you might wish to avoid named initializers which are a C# 3.0 feature, e.g.:
If you find the current multi-line approach particularly offensive, one solution is to add a new constructor that takes the necessary parameters and write:
Of course, we could also make Bind a C# 3.0 project and be done with it (OpenTK already contains a C# 3.0 project, named "Converter"). We'd need to remove it from the default solution, but that's not *that* big an issue.
Still, I'd like to hear the opinions of Inertia and kanato prior to doing this.
#7
The issue of the missing CLSCompliant attributes is not cause by your patch, but rather from one of the recent updates to the generator. I'm trying to trace, but haven't succeeded so far.
Edit: and of course the issue was right in front of my eyes. I had added "uint" to the list of non-CLScompliant types, but forgot to add "uint32". Should be fixed now.
Please let me know if you have any updates to the patch so I can try again!
#8
I've created two distinct patches.
One for "Optimizing using directives", and second for "Shorten qualifier references".
But I'm stuck on how to apply both to the same revision. (to my local working copy in my case)
If I apply one then I can't apply another.
Both patches in attachment.
#9
I think it's reasonable to use C# 3.0 features in Bind, as long as they are compatible with the latest mono. It's not required to run bind to build OpenTK. The generated bindings are committed to SVN, so someone could still compile OpenTK without having to compile Bind. All my development I do in VS 2008, but I've never actually used Bind myself.
#10
#11
"Shorten qualifier references" patch for bind project for revision 2172.
p.s. I will make the next patch after you add this to SVN. Because I can't make multiple distinct patches on same revision.
#12
Thanks, patch committed to rev. 2183. Marking issue as fixed.
#13
"Arrange this Qualifier" patch for rev. 2183
#14
Committed. Please reopen if you have more patches to share. :-)
#15
Closing issues fixed in 0.9.9-2.