WAUrlEncoder not using primitive

[dalehenrich]

From Gerhard:

It seems that the gemstone primitve encodeUsing: is not working 
with the WAUrlEncoder table.
The primitive doesn't like nil as entries in the table.

The method table ^table at WATableBaseEncoder must be implemented to be able to run the test.
'asd' encodeUsing: WAUrlEncoder table    .....    always returns nil

I don't have a real patch for this. If the table is initialized with strings only, it seems to work.
But then also the nextPut: method at WATableBasedEncoder must be changed.

e.g.
initializeTable
    "Initializes the encoding table."
    | stream characterLimit |
    characterLimit := self maximumCharacterValue.
    "character values at zero so we need to add 1"
    table := Array new: characterLimit + 1.
    stream := WriteStream on: (String new: 6).
    0 to: characterLimit do: [ :index | 
        | character value |
        character := Character codePoint: index.
        self encode: character on: stream reset.
        "Smalltalk indices are one based but character values start at 0"
        value := stream contents = (String with: character)
            ifTrue: [ (String with: character) ]    .... instead of nil
            ifFalse: [ stream contents ].
        table at: index + 1 put: value ]

[dalehenrich]

@obi068, I'm not quite sure that this is a real issue or perhaps I am misunderstanding what you are asking?

[obi068]

Hi Dale,

I think i only want to point out that the seaside implementation of the
WAUrlEncoder table
could no be used because of the restrictions (nils not allowed) in the
encodeUsing: primitive.

I am now using a subclass of WAUrlEncoder with a different initializeTable
method.

Gerhard

On Tue, May 26, 2015 at 11:43 PM, Dale Henrichs [email protected]
wrote:

@obi068 https://github.com/obi068, I'm not quite sure that this is a
real issue or perhaps I am misunderstanding what you are asking?

—
Reply to this email directly or view it on GitHub
#31 (comment).

[dalehenrich]

Hmmmm, which version of GemStone/Seaside are you using? Nils are handled by failing the primitive and then falling back to Smalltalk code .... I ran into this in 3.3 because the primitives had been changed to throw an error on nils and the Seaside tests started failing ... This is part of the reason I am confused:)

[obi068]

Gemstone 3.2 and Seaside 3.1 (not the final one)!

On Thu, Jun 4, 2015 at 11:35 PM, Dale Henrichs [email protected]
wrote:

Hmmmm, which version of GemStone/Seaside are you using? Nils are handled
by failing the primitive and then falling back to Smalltalk code .... I ran
into this in 3.3 because the primitives had been changed to throw an error
on nils and the Seaside tests started failing ... This is part of the
reason I am confused:)

—
Reply to this email directly or view it on GitHub
#31 (comment).

[dalehenrich]

Gemstone 3.2.0, 3.2.1, 3.2.6? I'd like to narrow my search down a bit:) A little more detail for the seaside version as well ... a commit SHA would be best ...

[obi068]

Sorry, GS 3.2.0.

Not sure how to find out the version of Seaside.
I loaded from github://glassdb/Seaside31:gemstone3.1/repository a year ago.
How can i find out the commit SHA.

On Tue, Jun 9, 2015 at 1:32 AM, Dale Henrichs [email protected]
wrote:

Gemstone 3.2.0, 3.2.1, 3.2.6? I'd like to narrow my search down a bit:) A
little more detail for the seaside version as well ... a commit SHA would
be best ...

—
Reply to this email directly or view it on GitHub
#31 (comment).

[dalehenrich]

To get the SHA of a github download, take a look at the directory associated with your github://glassdb/Seaside31:gemstone3.1/repository (an instance of MCGitHubRepository) .. the SHA should be embedded in the path. for example here is a directory path from one of mine:

/export/foos1/users/dhenrich/gsDevKitHome/gemstone/stones/seaside/logs/github-cache/GsDevKit/ServiceVM/master/GsDevKit-ServiceVM-7364278/repository

and the SHA is 7364278...

[dalehenrich]

Okay, I've also looked at the implementation of primitive 654 for String>>encodeUsing:

encodeUsing: encodingArray

  "A new instance of the class of the receiver will be returned where
   each character in the receiver is replaced by the character or string
   from encodingArray that is located at the position in encodingArray for
   the character.  The ascii value of the character is used as an index
   into the encodingArray,  value 16r0 accesses   encodingArray at:1 .
   If the ascii value is >= size of the encodingArray,
   nil is returned.
   If the value from the encodingArray is a Character with value > 16rFF
   or is not a kind of String,  nil is returned."

  <primitive: 654>
  ^ nil

And it is as I expected ... the primitive doesn't fail when it encounters a nil (or any other unexpected value or size mismatch, etc. in the encodingArray) ... encodeUsing: returns a nil ... The current implementation of WATableBasedEncoder>>nextPutAll: in the GsDevKit variant of Seaside31 handles the nil result correctly:

nextPutAll: aString
  "#encodeUsing: is a primitive in GemStone, so it does the mapping very quickly..."

  | encodedString |
  encodedString := aString encodeUsing: table.
  encodedString ~~ nil
    ifTrue: [ stream nextPutAll: encodedString ]
    ifFalse: [ 1 to: aString size do: [ :index | self nextPut: (aString at: index) ] ]

So at least for the current version of Seaside31 the use of encodeUsing: and building a table with embedded nils is consistent.

[dalehenrich]

The same method is available in the glassdb variant of Seaside31 with the change made on December 15, 2013...

Prior to the fix by @jbrichau the code was indeed wrong:

nextPutAll: aString 
    "uses #to:do: for speed reasons (on Pharo)
    this is not premature optimization, this is a hotspot method method
    and #to:do: shows measurable speed improvements for rendering seaside pages"
    1 to: aString size do: [ :index |
        self nextPut: (aString at: index) ]

So It looks like you should update your version of Seaside31 to a version that is a bit later ... @jbrichau tagged 3.1.1, so I would think you'd want to upgrade to that point at a minimum as that does correspond to the general 3.1.1 release and includes a fix for this particular bug ...

On GsDevKit/Seaside31 we're up to 3.1.4.1 and catching up to that point wouldn't be a bad idea as there are quite a few bugfixes available ...