Patch: Don't lowercase existing CamelCase

older
patch applied (c2hs): Depend on...

Achim Schneider

24 Jan 2009 24 Jan '09

12:19 p.m.

Agar (http://libagar.org) uses AG_FunctionFoo as naming convention and I like to write hooks of the form {#fun FunctionFoo as ^ ... #} As it stands, though, c2hs-0.15.1 will lowercase the second F, unnecessarily. I attached a patch against c2hs/chs/CHS.hs that fixes this... I'm not completely sure of it, though, as apathToIdent looks like it could be important. Anyway, for me the patch does what it's supposed to do. -- (c) this sig last receiving data processing entity. Inspect headers for copyright history. All rights reserved. Copying, hiring, renting, performance and/or quoting of this signature prohibited.

Attachments:

c2hs-tooMuchLowerCase.patch (text/x-patch — 287 bytes)

Show replies by date

Duncan Coutts

24 Jan 24 Jan

3:56 p.m.

On Sat, 2009-01-24 at 18:19 +0100, Achim Schneider wrote:

...

Agar (http://libagar.org) uses AG_FunctionFoo as naming convention and I like to write hooks of the form

{#fun FunctionFoo as ^ ... #}

As it stands, though, c2hs-0.15.1 will lowercase the second F, unnecessarily.

I attached a patch against c2hs/chs/CHS.hs that fixes this... I'm not completely sure of it, though, as apathToIdent looks like it could be important. Anyway, for me the patch does what it's supposed to do.

Thanks Achim. 930,931c930 < let lowerFirst (c:cs) = toLower c : cs < in onlyPosIdent (posOf ide) (lowerFirst $ identToLexeme ide) ---

...

onlyPosIdent (posOf ide) (identToLexeme ide)

This one I don't understand. I don't understand why converting the first char to lower case is bad here or how it relates to your example. 1086c1085 < adjustCase (c:cs) = toUpper c : map toLower cs ---

...

adjustCase (c:cs) = toUpper c : cs

This one makes perfect sense to me. Benedikt, do you agree? Duncan

Achim Schneider

4:29 p.m.

Duncan Coutts wrote:

...

930,931c930 < let lowerFirst (c:cs) = toLower c : cs < in onlyPosIdent (posOf ide) (lowerFirst $ identToLexeme ide) ---

...
onlyPosIdent (posOf ide) (identToLexeme ide)

This one I don't understand. I don't understand why converting the first char to lower case is bad here or how it relates to your example.

Me neither. The problem is that, if the original is FunctionFoo and the renamed functionFoo, the original code thinks that they're the same and replaces functionFoo with Nothing, which then amounts to outputing {#fun FunctionFoo {...#} instead of {#fun FunctionFoo as functionFoo {...#} , which of course chokes ghc. -- (c) this sig last receiving data processing entity. Inspect headers for copyright history. All rights reserved. Copying, hiring, renting, performance and/or quoting of this signature prohibited.

Benedikt Huber

7:28 p.m.

On 24.01.2009, at 22:29, Achim Schneider wrote:

...

Duncan Coutts wrote:

...
930,931c930 < let lowerFirst (c:cs) = toLower c : cs < in onlyPosIdent (posOf ide) (lowerFirst $ identToLexeme ide) ---

...
onlyPosIdent (posOf ide) (identToLexeme ide)

This one I don't understand. I don't understand why converting the first char to lower case is bad here or how it relates to your example.

Me neither. The problem is that, if the original is FunctionFoo and the renamed functionFoo, the original code thinks that they're the same and replaces functionFoo with Nothing, ... Hi Achim, I agree with Duncan that the second part of the patch

1086c1085 < adjustCase (c:cs) = toUpper c : map toLower cs --- adjustCase (c:cs) = toUpper c : cs is fine. I tried it, and it seems to work. I think we would not want to apply the first part of the patch. The problem is elsewhere, but I think I've already fixed this before. Could you try whether the second part of the patch alone works fine with the latest c2hs ? If not, could you give a detailed description of the problem you're experiencing when not applying the first part ? benedikt

Achim Schneider

11:03 p.m.

New subject: lowercase/C evaluation [was:Don't lowercase...]

Benedikt Huber wrote:

...

I think we would not want to apply the first part of the patch. The problem is elsewhere, but I think I've already fixed this before. Could you try whether the second part of the patch alone works fine with the latest c2hs ? If not, could you give a detailed description of the problem you're experiencing when not applying the first part ?

Indeed, it works fine with darcs. Summing up, darcs diff gives me, right now, diff -rN old-c2hs/src/C2HS/CHS.hs new-c2hs/src/C2HS/CHS.hs 1105c1105 < adjustCase (c:cs) = toUpper c : map toLower cs ---

...

adjustCase (c:cs) = toUpper c : cs

diff -rN old-c2hs/src/C2HS/Gen/Bind.hs new-c2hs/src/C2HS/Gen/Bind.hs 112a113

...

import Data.Bits ((.|.), (.&.)) 2031a2033,2036 applyBin _ COrOp (IntResult x) (IntResult y) = return $ IntResult (x .|. y) applyBin _ CAndOp (IntResult x) (IntResult y) = return $ IntResult (x .&. y)

Strictly speaking, I don't even need CAndOp, but I felt like adding it alongside. or'ing up things like MAXIMIZE_V and MAXIMIZE_H to MAXIMIZE is a quite usual technique, so I guess I'm not going to be the only one using it once enum define comes into usage. -- (c) this sig last receiving data processing entity. Inspect headers for copyright history. All rights reserved. Copying, hiring, renting, performance and/or quoting of this signature prohibited.

Duncan Coutts

25 Jan 25 Jan

11:36 a.m.

New subject: lowercase/C evaluation [was:Don't lowercase...]

On Sun, 2009-01-25 at 05:03 +0100, Achim Schneider wrote:

...

Benedikt Huber wrote:

...
I think we would not want to apply the first part of the patch. The problem is elsewhere, but I think I've already fixed this before. Could you try whether the second part of the patch alone works fine with the latest c2hs ? If not, could you give a detailed description of the problem you're experiencing when not applying the first part ?

Indeed, it works fine with darcs.

Great.

...

Summing up, darcs diff gives me, right now,

diff -rN old-c2hs/src/C2HS/CHS.hs new-c2hs/src/C2HS/CHS.hs 1105c1105 < adjustCase (c:cs) = toUpper c : map toLower cs ---

...
adjustCase (c:cs) = toUpper c : cs

Applied. I also added a case for [] which would happen with C names like "foo__bar".

...

diff -rN old-c2hs/src/C2HS/Gen/Bind.hs new-c2hs/src/C2HS/Gen/Bind.hs 112a113

...
import Data.Bits ((.|.), (.&.)) 2031a2033,2036 applyBin _ COrOp (IntResult x) (IntResult y) = return $ IntResult (x .|. y) applyBin _ CAndOp (IntResult x) (IntResult y) = return $ IntResult (x .&. y)

Applied. Thanks Achim. So if no more issues come up in the next few days I'll put a release out on hackage. Duncan

Benedikt Huber

26 Jan 26 Jan

5:41 a.m.

New subject: Release plans [was: Re: lowercase/C evaluation]

On 25.01.2009, at 17:36, Duncan Coutts wrote:

...

So if no more issues come up in the next few days I'll put a release out on hackage. Hi Duncan, I've just uploaded a minor release of language-c (0.3.1.1) to hackage, fixing some small bugs. I noticed the constraint language-c < 0.4.0 in c2hs - that's a good idea, I will keep it in mind for the next proper release.

c2hs right now doesn't build with the latest language-c, because of a change to Data.Position (we add absolute offsets in the preprocessed files). With 0.3.1.1, it would be possible to hide uses of Data.Position in c2hs. It isn't neccessary, so its up to you whether you want to apply the attached patch. I didn't notice any performance penalty - Data.Position is strict in row and col, and incPos, adjustPos etc. should be inlined. If the patch is included, c2hs needs to depend on language-c-0.3.1.1 though. benedikt

Duncan Coutts

27 Jan 27 Jan

4:16 p.m.

New subject: Release plans [was: Re: lowercase/C evaluation]

On Mon, 2009-01-26 at 11:41 +0100, Benedikt Huber wrote:

...

On 25.01.2009, at 17:36, Duncan Coutts wrote:

...
So if no more issues come up in the next few days I'll put a release out on hackage. Hi Duncan, I've just uploaded a minor release of language-c (0.3.1.1) to hackage, fixing some small bugs. I noticed the constraint language-c < 0.4.0 in c2hs - that's a good idea, I will keep it in mind for the next proper release.

Ok. It was just a guess about your versioning policy. http://haskell.org/haskellwiki/Package_versioning_policy

...

c2hs right now doesn't build with the latest language-c, because of a change to Data.Position (we add absolute offsets in the preprocessed files).

With 0.3.1.1, it would be possible to hide uses of Data.Position in c2hs. It isn't neccessary, so its up to you whether you want to apply the attached patch. I didn't notice any performance penalty - Data.Position is strict in row and col, and incPos, adjustPos etc. should be inlined.

BTW: src/Language/C/Data/Ident.hs:49:19: Unrecognised pragma You mean UNPACK not UNBOXED

...

If the patch is included, c2hs needs to depend on language-c-0.3.1.1 though.

Yep. Applied, thanks. Duncan

Benedikt Huber

28 Jan 28 Jan

7:07 a.m.

New subject: Release plans [was: Re: lowercase/C evaluation]

...

On Mon, 2009-01-26 at 11:41 +0100, Benedikt Huber wrote:

...
On 25.01.2009, at 17:36, Duncan Coutts wrote:

...
So if no more issues come up in the next few days I'll put a release out on hackage. Hi Duncan, I've just uploaded a minor release of language-c (0.3.1.1) to hackage, fixing some small bugs. I noticed the constraint language- c < 0.4.0 in c2hs - that's a good idea, I will keep it in mind for the next proper release.

Ok. It was just a guess about your versioning policy.

http://haskell.org/haskellwiki/Package_versioning_policy Thanks for the link - maybe something to put on the Hackage upload

On 27.01.2009, at 22:16, Duncan Coutts wrote: page ?

...

BTW: src/Language/C/Data/Ident.hs:49:19: Unrecognised pragma

You mean UNPACK not UNBOXED Thanks, this has been fixed recently, but accidently was not merged into 0.3.1.1. The performance impact is negligible, though.

...

...
If the patch is included, c2hs needs to depend on language-c-0.3.1.1 though.

Yep. Applied, thanks. Great, thank you.

benedikt

Duncan Coutts

7:16 a.m.

New subject: Release plans [was: Re: lowercase/C evaluation]

On Wed, 2009-01-28 at 13:07 +0100, Benedikt Huber wrote:

...

...
On Mon, 2009-01-26 at 11:41 +0100, Benedikt Huber wrote:

...
On 25.01.2009, at 17:36, Duncan Coutts wrote:

...
So if no more issues come up in the next few days I'll put a release out on hackage. Hi Duncan, I've just uploaded a minor release of language-c (0.3.1.1) to hackage, fixing some small bugs. I noticed the constraint language- c < 0.4.0 in c2hs - that's a good idea, I will keep it in mind for the next proper release.

Ok. It was just a guess about your versioning policy.

http://haskell.org/haskellwiki/Package_versioning_policy Thanks for the link - maybe something to put on the Hackage upload

On 27.01.2009, at 22:16, Duncan Coutts wrote: page ?

Yeah, it's worth thinking about trying to make it more official and encourage it more strongly. We're currently building up tool support for it. Duncan

5957

Age (days ago)

5961

Last active (days ago)

List overview

Download

9 comments

3 participants

participants (3)

Achim Schneider
Benedikt Huber
Duncan Coutts