View Issue Details

IDProjectCategoryView StatusLast Update
0037565FPCPackagespublic2020-08-26 11:53
ReporterCudaText man_ Assigned ToMichael Van Canneyt  
PrioritynormalSeverityminorReproducibilityN/A
Status closedResolutionwon't fix 
Product Version3.3.1 
Summary0037565: fpJson: some code duplication, DRY it
Descriptionjsonscanner.pp
            Case FTokenStr^ of
              '"' : S:='"';
              '''' : S:='''';
              't' : S:=0000009;
              'b' : S:=0000008;
              'n' : S:=0000010;
              'r' : S:=0000013;
              'f' : S:=0000012;
              '\' : S:='\';
              '/' : S:='/';
              'u' : begin
                    S:='0000';
                    u2:=0;
                    For I:=1 to 4 do
                      begin
                      Inc(FTokenStr);
                      c2:=FTokenStr^;
                      Case c2 of
                        '0'..'9': u2:=u2*16+ord(c2)-ord('0');
                        'A'..'F': u2:=u2*16+ord(c2)-ord('A')+10;
                        'a'..'f': u2:=u2*16+ord(c2)-ord('a')+10;
                      else
                        Error(SErrInvalidCharacter, [CurRow,CurColumn,FTokenStr[0]]);
                      end;
                      end;


fpjson.pp
        begin
        Inc(I);
        App:='';
        Case S[I] of
          '\','"','/'
              : App:=S[I];
          'b' : App:=0000008;
          't' : App:=0000009;
          'n' : App:=0000010;
          'f' : App:=0000012;
          'r' : App:=0000013;
          'u' : begin
                U2:=BufferHexToInt(PAnsiChar(@S[I+1]));
                if U2=-1 then
                   Raise EJSON.Create('Invalid unicode hex code: '+Copy(S,I+1,4));
                Inc(I,4);
                if (U1<>0) then
                  begin
                  App:={$IFDEF FPC_HAS_CPSTRING}UTF8Encode({$ENDIF}WideChar(U1)+WideChar(U2){$IFDEF FPC_HAS_CPSTRING}){$ENDIF};
                  U2:=0;
                  end
                else
                  U1:=U2;
                end;

TagsNo tags attached.
Fixed in Revision
FPCOldBugId
FPCTarget-
Attached Files

Activities

Benito van der Zander

2020-08-13 16:17

reporter   ~0124844

It could decode \uxxxx by calling Val on a shortstring[5]. The four characters could be copied by a single unaligned int32 mov

Val does not seem to do any memory allocations. Although it might be slower due to checking for different bases. But if that is an issue, one could try to improve Val.

CudaText man

2020-08-14 07:56

reporter   ~0124867

No need to call Val on a shortstring. we have already the helper BufferHexToInt (commited in last weeek).

Michael Van Canneyt

2020-08-20 14:20

administrator   ~0125020

It's not worth it. To reuse would require to expose BufferHexToInt, and I don't want to pollute the interface with such helper functions.

CudaText man_

2020-08-20 15:47

reporter   ~0125026

Dont expose BufferHexToInt, but expose new helper like JsonBufferGetChar (which calls BufferHexToInt). It will be clear.

Michael Van Canneyt

2020-08-20 15:53

administrator   ~0125027

Like I said: I do not think it is worth it.
But if you submit a patch, I will apply it.

CudaText man

2020-08-21 11:36

reporter   ~0125055

Added 2 small patches.
p1.diff (1,215 bytes)   
--- fpjson_old.pp	2020-08-21 12:01:07.860871889 +0300
+++ fpjson.pp	2020-08-21 12:20:35.000000000 +0300
@@ -801,7 +801,7 @@
 
 implementation
 
-Uses typinfo;
+Uses typinfo, jsonscanner;
 
 Resourcestring
   SErrCannotConvertFromNull = 'Cannot convert data from Null value';
@@ -940,30 +940,6 @@
 end;
 {$ELSE}
 
-    function BufferHexToInt(P : PAnsiChar): integer;
-    var
-      N, i: integer;
-      ch: char;
-    begin
-      Result:= 0;
-      for i:= 1 to 4 do
-      begin
-        ch:= p^;
-        case ch of
-          '0'..'9':
-            N:= Ord(ch)-Ord('0');
-          'a'..'f':
-            N:= Ord(ch)-(Ord('a')-10);
-          'A'..'F':
-            N:= Ord(ch)-(Ord('A')-10);
-          else
-            exit(-1);
-        end;
-        Inc(P);
-        Result:= Result*16+N;
-      end;
-    end;
-
 Var
 
   I,J,L,U1,U2 : Integer;
@@ -1007,7 +983,7 @@
           'f' : App:=#12;
           'r' : App:=#13;
           'u' : begin
-                U2:=BufferHexToInt(PAnsiChar(@S[I+1]));
+                U2:=JsonBufferHexToInt(PAnsiChar(@S[I+1]));
                 if U2=-1 then
                    Raise EJSON.Create('Invalid unicode hex code: '+Copy(S,I+1,4));
                 Inc(I,4);
p1.diff (1,215 bytes)   
p2.diff (1,782 bytes)   
--- jsonscanner_old.pp	2020-08-21 12:01:38.420072115 +0300
+++ jsonscanner.pp	2020-08-21 12:26:39.000000000 +0300
@@ -122,9 +122,34 @@
     ''
   );
 
+function JsonBufferHexToInt(P : PAnsiChar): integer; inline;
 
 implementation
 
+function JsonBufferHexToInt(P : PAnsiChar): integer;
+var
+  N, i: integer;
+  ch: char;
+begin
+  Result:= 0;
+  for i:= 1 to 4 do
+  begin
+    ch:= p^;
+    case ch of
+      '0'..'9':
+        N:= Ord(ch)-Ord('0');
+      'a'..'f':
+        N:= Ord(ch)-(Ord('a')-10);
+      'A'..'F':
+        N:= Ord(ch)-(Ord('A')-10);
+      else
+        exit(-1);
+    end;
+    Inc(P);
+    Result:= Result*16+N;
+  end;
+end;
+
 constructor TJSONScanner.Create(Source : TStream; AUseUTF8 : Boolean = True);
 
 Var
@@ -322,20 +347,10 @@
               '\' : S:='\';
               '/' : S:='/';
               'u' : begin
-                    S:='0000';
-                    u2:=0;
-                    For I:=1 to 4 do
-                      begin
-                      Inc(FTokenStr);
-                      c2:=FTokenStr^;
-                      Case c2 of
-                        '0'..'9': u2:=u2*16+ord(c2)-ord('0');
-                        'A'..'F': u2:=u2*16+ord(c2)-ord('A')+10;
-                        'a'..'f': u2:=u2*16+ord(c2)-ord('a')+10;
-                      else
-                        Error(SErrInvalidCharacter, [CurRow,CurColumn,FTokenStr[0]]);
-                      end;
-                      end;
+                    u2 := JsonBufferHexToInt(FTokenStr+1);
+                    if u2<0 then
+                       Error(SErrInvalidCharacter, [CurRow,CurColumn,FTokenStr[1]]);
+                    Inc(FTokenStr, 4);
                     // ToDo: 4-bytes UTF16
                     if u1<>0 then
                       begin
p2.diff (1,782 bytes)   

Michael Van Canneyt

2020-08-21 11:49

administrator   ~0125056

You did exactly what I explicitly wrote that I did not want to happen: to expose buffertoint.

So, I'm sorry but I will not apply this patch.

CudaText man

2020-08-21 12:37

reporter   ~0125059

Can you at least remove the crap line
S:= '0000';
here
https://github.com/graemeg/freepascal/blob/master/packages/fcl-json/src/jsonscanner.pp#L325

Issue History

Date Modified Username Field Change
2020-08-13 14:27 CudaText man_ New Issue
2020-08-13 16:17 Benito van der Zander Note Added: 0124844
2020-08-14 07:56 CudaText man Note Added: 0124867
2020-08-20 14:16 Michael Van Canneyt Assigned To => Michael Van Canneyt
2020-08-20 14:16 Michael Van Canneyt Status new => assigned
2020-08-20 14:20 Michael Van Canneyt Status assigned => resolved
2020-08-20 14:20 Michael Van Canneyt Resolution open => won't fix
2020-08-20 14:20 Michael Van Canneyt FPCTarget => -
2020-08-20 14:20 Michael Van Canneyt Note Added: 0125020
2020-08-20 15:47 CudaText man_ Note Added: 0125026
2020-08-20 15:53 Michael Van Canneyt Note Added: 0125027
2020-08-21 11:36 CudaText man Note Added: 0125055
2020-08-21 11:36 CudaText man File Added: p1.diff
2020-08-21 11:36 CudaText man File Added: p2.diff
2020-08-21 11:41 CudaText man_ Status resolved => feedback
2020-08-21 11:41 CudaText man_ Resolution won't fix => open
2020-08-21 11:49 Michael Van Canneyt Status feedback => resolved
2020-08-21 11:49 Michael Van Canneyt Resolution open => won't fix
2020-08-21 11:49 Michael Van Canneyt Note Added: 0125056
2020-08-21 12:37 CudaText man Note Added: 0125059
2020-08-26 11:53 CudaText man_ Status resolved => closed