Unreachable code and runtime error 201 on SimpleMsgPack

[cpicanco]

Why not do it like this?

Code: Pascal  [Select][+][-]

1. procedure TSimpleMsgPack.setAsString(pvValue: string);
2. var Len: Cardinal;
3. begin
4.   FDataType := mptString;
5.   Len:= Length(pvValue) * SizeOf(Char);
6.   SetLength(FValue, Len);
7.   if Len > 0 then Move(pvValue[1], FValue, Len);
8. end;

It is much sleeker and should not cause any problems :-)

I would guess that shl is faster than multiplication. But it is just a guess.

[Fungus]

Really? In order to gain the advantage of shl vs multiply you loose a lot by the complexity of the code. I'd bet that in the long run the simplicity of my suggestion will beat the "advantage" of your shl

[cpicanco]

The code is not mine! The guess came from the fact that MsgPack was designed for speed!

[cpicanco]

But hey, you should agree that you may have both speed and simplicity:

Code: Pascal  [Select][+][-]

1. procedure TSimpleMsgPack.setAsString(pvValue: string);
2. begin
3.   FDataType := mptString;
4. {$IFDEF X}
5.     // simple note about why shift left is needed
6.     SetLength(FValue, length(pvValue) shl 1);
7.    
8.     // simple note about why pointers is needed
9.     Move(PChar(pvValue)^, FValue[0], Length(FValue));
10. {$ELSE}
11.     SetLength(FValue, Length(pvValue));
12.     Move(PChar(pvValue)^, FValue[0], Length(FValue));
13. {$ENDIF}
14.  
15. end;  
16.  
17.  

[Fungus]

You will not gain anything by replacing mul with shl - the advantage when using 32bit registers is 2 cycles on the CPU. A modern CPU is running in an excess of 3000000000 cycles per second and if the IPC (Instructions Per Cycle) is also considered the advantage is either not present or redicoulously minor. The compiler knows how to optimize the code to run as fast as possible on the target architecture and in the end a shl operation might end up being slower than a multiplication. So my advice is: keep your code as simple and understandable as possible :-)

[Thaddy]

Oh, dear,
I am putting my own code in if he doesn't listen. The ifdefs are wrong. Plainly not necessary. Why complicate matters when you can simply add one unit for Lazarus and it...works... without changing code.

[cpicanco]

I will listen! But it was honestly just a guess guys! I am not a computer scientist.

The LazUTF8 unit, as far as I know, did not prevent the error. Strings with length 0 will raise the error regardless of it.

[cpicanco]

I should say that I really appreciated your suggestion Fungus. But I am not sure if there is something better. I will do some research and some tests and bring the results to you. But I need some time; by 11-09-16 I'll be working on this carefully. Please, don't get me wrong.   

[Fungus]

I will listen! But it was honestly just a guess guys! I am not a computer scientist.

The LazUTF8 unit, as far as I know, did not prevent the error. Strings with length 0 will raise the error regardless of it.

Of course a string of length 0 will cause an error - you latest suggestion does not even check for empty string, and accessing "FValue[0]" when FValue has no length is a bug. Another flaw in regards of speed with you latest suggestion is that you call Length(..) twice while once should be enough. The doubled overhead of the function call itself is a performance loss.