Bug 27431 – splitting CodeGeneratorGObject.pm into library files

• Component: WebKit Misc.
• Status: RESOLVED WONTFIX

[Luke Kenneth Casson Leighton]

this is part of the series of patches to split #16401 into smaller
patches, as part of an agreement recommended by david.

CodeGeneratorGObjectLibrary1.pm is the remainder of CodeGeneratorGObject.pm which was split in order to shrink its size, so that it would be more acceptable in size for reviewers.  in order to maintain the code in a working state, the GObject Code Generator will be split into as many files as it takes for reviewers to accept them.  each split will go under its own bugreport, such that discussions of the issues for that fragment can be kept to a manageable minimum.

if the reviewers or anyone else would like to make a better suggestion in advance - one that will save both themselves and myself vast amounts of time - now would be a good time to make it.  i'm quite prepared to split the file up into 1,500 patches of 1 line each if that's what it takes (and will happily write a program to do so that will make that possible for me to do with no effort involved, thus placing the burden onto the reviewers, not myself).

[Luke Kenneth Casson Leighton]

Created attachment 33060 [details]
code generator gobject library #1

anyone got any better suggestions that will save everyone time, please make them known....

[Luke Kenneth Casson Leighton]

Comment on attachment 33060 [details]
code generator gobject library #1

cancelling review request after changes made under  https://bugs.webkit.org/show_bug.cgi?id=27424 which affect this patch.

will re-submit (and take the opportunity to correct the ChangeLog entry layout)

[Luke Kenneth Casson Leighton]

Created attachment 34289 [details]
updated from other review comments

ok - suggestions on how this [large] file should be reviewed easier welcome.

[Eric Seidel (no email)]

Comment on attachment 34289 [details]
updated from other review comments

This looks like a big hack.  If we're going to do this, lets do it right.  No commented out code.  No long lists of exclusions.  Nicely named methods which actually follow WebKit style guidlines.  Small files that reviewers have a chance of understanding.  Detailed ChangeLogs.  Use of sets/hashes instead of long lists of copy/paste ifs.  

As is, either you're just looking for a rubber-stamp.  Or you don't realize how much patches like this are just a waste of time.

Glad to have you contributing.  But if you're going to contribute to a product as mature as WebKit, you need to expect to take a bit longer on your patches, and write mature, well structured patches instead of hacks.

Contributing to a large project like WebKit is a lot of work. :)

[Luke Kenneth Casson Leighton]

(In reply to comment #4)
> (From update of attachment 34289 [details])
> This looks like a big hack.

 yep!  you got that right :)

  it's pieced together from _both_ the CodeGeneratorJS.pm _and_ the CodeGeneratorObjC.pm, and is the only major piece of perl programming i've ever forced myself to work on.  i'm deeply proud of both how much of a dog's dinner it is and that it actually works at all. *beam* :)


>  If we're going to do this, lets do it right.  No
> commented out code.  No long lists of exclusions.  Nicely named methods which
> actually follow WebKit style guidlines.  Small files that reviewers have a
> chance of understanding.  Detailed ChangeLogs.  Use of sets/hashes instead of
> long lists of copy/paste ifs.  

 .... not being funny or anything, but... seriously, take a look at the code from which this was cut/paste copied (verbatim in some cases).  i'm just looking through it, now, but some of the stuff that _was_ there has gone.

* UsesManualToGDOMImplementation came from the august 2008 version of CodeGeneratorObjC.pm.  it's gone, now.

* likewise isGDOMSubClass - that was cut/paste verbatim conceptually from the other CodeGenerators - but they're gone, now, too.

* see addIncludesForType in CodeGeneratorObjC.pm - that concept is still there, you can see it shining through in addIncHdr and addPropIncHdr.

* look at the code-density of lines 1082 to 1206 in CodeGeneratorJS.pm!  

* 1325 to 1334 in CodeGeneratorJS.pm i'm particularly fond of :)  makes me feel like i'm 5 years old again, just slide down the brackets, wheeeee :)

* GetNativeType is the _new_ version of what i originally cut/paste from CodeGeneratorJS.pm, to create GetGType() and GetPropGType().



so, what you're seeing now is what the code _used_ to look like back in august 2008, when this work was in active development, when an intelligent non-perl-programmer gets their hands on it and just goes, "err, where's something i can blatantly copy that will get me from a to b?  oh look - _there_!"

so - _fortunately_, there is yet more examples for me to blatantly cut-and-paste (GetNativeType)








> As is, either you're just looking for a rubber-stamp.  Or you don't realize how
> much patches like this are just a waste of time.

 not quiiite true - and not quite fair, either.  do take a look at the older versions of CodeGeneratorJS.pm and CodeGeneratorObjC.pm:

 http://svn.webkit.org/repository/webkit/trunk/WebCore/bindings/scripts/CodeGeneratorJS.pm?r=26579

http://svn.webkit.org/repository/webkit/trunk/WebCore/bindings/scripts/CodeGeneratorObjC.pm?r=26579

take a look at TypeCanFailConversion, GetNativeType, UsesManualToJSImplementation and you'll soon see that i'm really _not_ taking the piss, i was just working from what was there at the time.


> Glad to have you contributing. 

 *beam* :)

> But if you're going to contribute to a product
> as mature as WebKit, you need to expect to take a bit longer on your patches,
> and write mature, well structured patches instead of hacks.

 eric - you're going to be amused re-reading this after looking at the older versions i was working from, at the time, i guarantee it.


> Contributing to a large project like WebKit is a lot of work. :)

 mmmm :)

 ok.

 rrrright.

 i'll start with some splits into sensible / reasonable sizes, and make some more cut/paste copies, from what's already there.

 but - PLEASE do not expect me to make "functionality" changes / logic changes.  i've validated this code, hair-raisingly nasty as it is, by using pyjamas-desktop's 20 examples / samples, so i know that it works.

 i reaaallly don't want to be making additions / changes whilst also at the same going through a massive restructuring.

 if it wasn't for the massive amount of in-built logic and decisions, yes i'd do a from-scratch rewrite.  but, like the linux kernel floppy code, there's simply too much that's been taken into account to go throwing it away and starting again.

 i'm happy to make it at least _look_ nicer, though :)

[Luke Kenneth Casson Leighton]

Created attachment 34339 [details]
tidyup / function split / typemap etc.

... something like this?

i tried splitting it into separate files, and somehow / somewhy it didn't work.  i'll try to find out why.

[Luke Kenneth Casson Leighton]

Created attachment 34341 [details]
split into two library files; requesting veery quick review

ok - i worked it out (the splitting)  do you mean something like this?  if so, i'll break it down further, as separate bugreports.

[Luke Kenneth Casson Leighton]

Comment on attachment 34341 [details]
split into two library files; requesting veery quick review

cancelling review whilst auto-patch-maintenance script is written which puts ChangeLog at top of file

[Gour]

Created attachment 34466 [details]
uploaded

[Mark Rowe (bdash)]

Comment on attachment 34466 [details]
uploaded

Numbered files is not a logical way for splitting a Perl module.

[Anders Carlsson]

No point in doing this.