pull request for general improvements

[Tilo Prütz]

Hi,

I made some general improvements to reduce the compiler warnings (compiling nativly on Linux). Please find them on http://code.google.com/r/tilopruetz-cocotron/source/detail?r=1fda3d1453a6fe6a08595934c0148a32623c84f2.

BTW: Isn't there a better way to do pull requests? On GitHub one can easily select a branch and click on 'pull request' to do this and then a task is automatically opened for the maintainers with the ability to discuss the request in general or even single lines of changes. Not to mention that the overview of the changes to pull is handy.

Regards

Tilo Prütz

[Christopher Lloyd]

Hi,

On Wednesday, April 18, 2012 4:20:48 AM UTC-4, Tilo Prütz wrote:

I made some general improvements to reduce the compiler warnings (compiling nativly on Linux). Please find them on http://code.google.com/r/tilopruetz-cocotron/source/detail?r=1fda3d1453a6fe6a08595934c0148a32623c84f2.

Great!

 

BTW: Isn't there a better way to do pull requests? On GitHub one can easily select a branch and click on 'pull request' to do this and then a task is automatically opened for the maintainers with the ability to discuss the request in general or even single lines of changes. Not to mention that the overview of the changes to pull is handy.

Apparently there is an outstanding issue on google code to implement this. The clones page is listed by most recently committed to, I check this regularly, look at the changes and pull into the mainline when I'm comfortable with the changes.

Chris

[Christopher Lloyd]

I know you're not a fan of the current formatting but I would strongly encourage not reformatting code that is not being significantly improved, or not improved at all, it's very time consuming to review code formatting changes. It's further problematic if someone else actually improved the reformatted code in another clone, so I have to pull changes into reformatted code, not a lot of fun :( These aren't just whitespace formatting I can filter with diff -w

If I could go through these changes and see boom boom boom simple changes which fix the errors I'd be done in minutes, instead I am looking at diff's with a lot of {} changes which I have to carefully inspect and which don't add value. We all have our formatting preferences and no one is right here. I'd really like to not see any formatting changes in future pulls which aren't improved code, it's just so much simpler to keep moving forward. I have really limited time to review these, whatever can be done to make it faster is a help.

Thanks!

Chris

[Tilo Prütz]

Hi Chris,

I tried to do all reformatting in separate commits which contain no change of logic.

I only reformat methods where I do logic changes later on except from trailing whitespaces which are automatically removed by my vim.

I know that it's time consuming to merge divergenting branches.

Nevertheless I do not think I could improve some code which I cannot even read.

I would suggest that I reduce my reformattings to the following (for future code changes - there are some changes already done which I did not requested for pull yet):

- I will continue separating formatting and logical changes.

- I will reduce my formatting changes to pure whitespace changes that are:

  - remove trainling whitespaces in whole files

  - fix indentation in function to be changed

  - maybe more space-fixes (like 'for( int i;i<j;i++){' → 'for (int i; i < j; i++) {') in _lines_ to be changed

- I will only add curly braces or change line breaks in blocks I change and do these changes in the logical change commit.

As a result of this the reformatting commits should be totally empty with 'diff -w' and all other changes should be of low impact.

Best Regards

Tilo Prütz

[Christopher Lloyd]

Hey Tilo,

In general I will try to pull a clone up to it's current state at once, all the change sets, I'd rather not pull/review individual change sets at a time. So making the formatting changes a separate commit is probably extra work for both of us since it just shows up in the single diff I review anyway. To make it easier what I'd suggest is no bracing or whitespace changes outside of the changed code, this generates a lot of multi-line changes in the diff's which make it hard to see the single line changes. I can review simple changes, e.g. warning fixes and such really quickly if there is nothing else I am seeing in the change. If it's a one line fix and I only see a one line fix in my diff it's quick.

On Monday, April 23, 2012 2:09:09 AM UTC-4, Tilo Prütz wrote:

Nevertheless I do not think I could improve some code which I cannot even read.

I do understand! :)

 

- I will continue separating formatting and logical changes.

No need really if the logical change also includes formatting changes.

 

- I will reduce my formatting changes to pure whitespace changes that are:

  - remove trainling whitespaces in whole files

  - fix indentation in function to be changed

  - maybe more space-fixes (like 'for( int i;i<j;i++){' → 'for (int i; i < j; i++) {') in _lines_ to be changed

- I will only add curly braces or change line breaks in blocks I change and do these changes in the logical change commit.

If the reformatting is isolated to the change that'd be great.

 

As a result of this the reformatting commits should be totally empty with 'diff -w' and all other changes should be of low impact.

Thanks!

Chris

[Tilo Prütz]

Hi Chris,

so I will update my suggestion with the following:

- I _may_ separate whitespace-only reformatting commits and logical changes.

- I will take care to produce changesets which contain only 'diff -w' changes related to the logic change.

In effect the outcome stays the same as by my first suggestion. But I think its clear now that I will take care to reduce your merge workload.

Best regards

Tilo Prütz

[Christopher Lloyd]

Hi Tilo,

The new constants you introduced in d1cffdeda259 don't work:

e.g.

NSNumber *kNSNumberTrue;

NSNumber *kNSNumberFalse;

const CFBooleanRef kCFBooleanTrue = (CFBooleanRef)&kNSNumberTrue;

const CFBooleanRef kCFBooleanFalse = (CFBooleanRef)&kNSNumberFalse;

This doesn't work because a CFBooleanRef is pointer to an instance and you're initializing it with a pointer to a pointer to an instance, and you can't initialize them in +initialize because they're const's.

This change should probably be reverted and the old system made to work with clang, it was basically how Apple is doing it. The constants also need to be valid before +initialize .

Rest of it looks good!

Chris

[Tilo Prütz]

Am Mittwoch, 25. April 2012 06:40:34 UTC+2 schrieb Christopher Lloyd:

Hi Tilo,

The new constants you introduced in d1cffdeda259 don't work:

e.g.

NSNumber *kNSNumberTrue;

NSNumber *kNSNumberFalse;

const CFBooleanRef kCFBooleanTrue = (CFBooleanRef)&kNSNumberTrue;

const CFBooleanRef kCFBooleanFalse = (CFBooleanRef)&kNSNumberFalse;

This doesn't work because a CFBooleanRef is pointer to an instance and you're initializing it with a pointer to a pointer to an instance, and you can't initialize them in +initialize because they're const's.

This change should probably be reverted and the old system made to work with clang, it was basically how Apple is doing it. The constants also need to be valid before +initialize .

Okay, I will do so.

IMHO the whole CoreFoundation thing was one of the worst ideas Apple ever had (regarding the Foundation). They didn't even get it right by themselves and now we have to live with such a crap.

Regards

Tilo Prütz

[Christopher Lloyd]

On Wednesday, April 25, 2012 2:59:04 AM UTC-4, Tilo Prütz wrote:

Okay, I will do so.

IMHO the whole CoreFoundation thing was one of the worst ideas Apple ever had (regarding the Foundation). They didn't even get it right by themselves and now we have to live with such a crap.

I'll agree, not a big fan of CF, the API's differ in subtle ways for the same functionality and then there are just big differences for the same class/ref in the API, you can do it in CF but not NS and vice versa. 

Then we have the whole __bridge* mess with ARC. I remind myself that CF was to move OS 9 developers over to OS X as part of the Carbon system, it is unfortunate it still has such traction with developers and Apple, largely due to Apple :(

Chris

[Tilo Prütz]

Am Mittwoch, 25. April 2012 06:40:34 UTC+2 schrieb Christopher Lloyd:

Hi Tilo,

The new constants you introduced in d1cffdeda259 don't work:

Hi Chris,

please find the revert of the erroneous commit at http://code.google.com/r/tilopruetz-cocotron/source/detail?r=5591ca753b70f329d437b53ad4a0508d874997c9&name=default

Don't worry about the branch commits; I had some headache about the insane branch concept of mercurial. Why can't anybody just use git? ;)

Regards

Tilo

[Christopher Lloyd]

Ok, these are in the mainline, thanks!

Chris