Barlog's picture

OpenTK.Bind project refactoring

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.

AttachmentSize
BindProjectRefactoringDiffPatch.zip14.09 KB

Comments

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
the Fiddler's picture

#1

Status:open» in progress (review)

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:

  1. Dependency cycles (e.g. GL2.Generator.Process() calls into Enums.GLEnums.Initialize with calls back into GL2.Generator).
  2. Commandline options that are either ignored or overriden (e.g. output directory with "-mode:es")
  3. Translation code that is scattered in many different places (e.g. parts are in the spec reader, parts in the Structures classes and parts in the "Translate" functions). This makes the translation logic very difficult to follow.

In any case, thank you for the patch - I'll test it and post results.

Barlog's picture

#2

the Fiddler wrote:

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.

the Fiddler wrote:

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.

the Fiddler's picture

#3

Thanks, I'll check this patch in VS2005 and commit it.

the Fiddler's picture

#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).

Barlog's picture

#5

the Fiddler wrote:

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.

the Fiddler wrote:

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.

the Fiddler's picture

#6

Thank you.

If you do this, you might wish to avoid named initializers which are a C# 3.0 feature, e.g.:

Parameter p = new Parameter { Name = "stride", Type = "int" };

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:

Parameter p = new Parameter("stride", "int" );

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.

the Fiddler's picture

#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!

Barlog's picture

#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.

AttachmentSize
rev2145_patches.zip6.69 KB
kanato's picture

#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.

the Fiddler's picture

#10

Status:in progress (review)» in progress (commit)