Jump to content
Clément

Do you consider this type construct to be a smelly code?

Recommended Posts

Posted (edited)

Hi,

 

I always tried to use the right type for the job, but there's a certain type construction that when used properly is just great.
Since this type construct uses pointers, I'm afraid it might die as pointers are evil (by some, I just can't imagine coding without them) and might get removed at some point.
Is there a better way to handle this construct?

 

function DoSomething( const Data; const DataSize : Cardinal ) : TBytes;
type
    PUInt64Array = ^TUInt64Array;
    TUInt64Array =  array[0..MaxInt div 16] of UInt64;

var

 {...};
begin
{.. some code ..}
	n := HashBlockSize div SizeOf(UInt64);
	for i := 0 to n-1 do begin
      PUInt64Array(@oString[0])[i] := PUInt64Array(@oString[0])[i] xor UInt64($5c5c5c5c5c5c5c5c);
      PUInt64Array(@iString[0])[i] := PUInt64Array(@iString[0])[i] xor UInt64($3636363636363636);
   end;
   {.. some more code .. }
end;

 

I'm using such construct as a helper for a HMAC_SHA256 implementation, and it works great. Do you consider it a smelly code? Is there a better way to reproduce without pointers?

 

TIA,

Clément
 

Edited by Clément

Share this post


Link to post
Posted (edited)

@Clément, To be sincere, I tend to keep pointer constructs to a minimum (to be on the safe side) since the size of integers varies on a various platforms supported by Delphi.

Also any reason why you reimplemented HMAC when it is supported in modern Delphi versions?

Edited by Ugochukwu Mmaduekwe

Share this post


Link to post
7 minutes ago, Ugochukwu Mmaduekwe said:

@Clément, To be sincere, I tend to keep pointer constructs to a minimum (to be on the safe side) since the size of integers varies on a various platforms supported by Delphi.

Also any reason why you reimplemented HMAC when it is supported in modern Delphi versions?

For this project I must use Delphi XE. Unfortunately most free implementation are not fast enough and/or depends on DLLs.
 

Share this post


Link to post
Posted (edited)
18 minutes ago, Clément said:

For this project I must use Delphi XE. Unfortunately most free implementation are not fast enough and/or depends on DLLs.
 

Well, have you tried Mormot's implementation?

 

https://github.com/synopse/SynPDF/blob/master/SynCrypto.pas

 

You can also try HashLib4Pascal if you want.

 

https://github.com/Xor-el/HashLib4Pascal/

 

Also you can try out Wolfgang Ehrhardt aka Gammatester (RIP) excellent Hash libraries which contains HMAC implementations too.

 

http://www.wolfgang-ehrhardt.de/crchash_en.html

 

These are guaranteed to work on Delphi 2010 and above and don't require any external DLLs for as long as I remember.

Edited by Ugochukwu Mmaduekwe
  • Like 1

Share this post


Link to post
20 minutes ago, Ugochukwu Mmaduekwe said:

Well, have you tried Mormot's implementation?

 

https://github.com/synopse/SynPDF/blob/master/SynCrypto.pas

 

You can also try HashLib4Pascal if you want.

 

https://github.com/Xor-el/HashLib4Pascal/

 

Also you can try out Wolfgang Ehrhardt aka Gammatester (RIP) excellent Hash libraries which contains HMAC implementations too.

 

http://www.wolfgang-ehrhardt.de/crchash_en.html

 

These are guaranteed to work on Delphi 2010 and above and don't require any external DLLs for as long as I remember.

Thanks I'll have a look.

Share this post


Link to post

To answer the OQ (Original Question). The answer is "No" if you add comments to your code and name your methods "DoThis".

Share this post


Link to post

This looks like beautiful pointer and math magic to me.
If this works as expected, it would be worth to figure out why, you're right.
I never looked into crypto algorithms too deeply, but probably this could be a common practice to safe some computing power and/or memory

by fractioning or grouping the memory access somehow.

Maybe you can find similar structure in C++ or somewhere else too, with a little more description.

Share this post


Link to post

I only smell "magic numbers" there. Rest looks fine.

Share this post


Link to post
Posted (edited)

Incomplete code?

 

Using pointers is fine(*), repeating code, especially inside a loop, is not. 

 

Move expressions like:

 

PUInt64Array(@oString[0])

 

outside the loop, assign to local vars. Then you can probably just use inc on the pointer vars instead of using slower array notation. 

 

(*) as long as you understand them, use them correctly, efficiently and readably. 

Edited by Hallvard Vassbotn

Share this post


Link to post

Your question was "Is this type of construct smelly code?"  If you ever feel compelled to ask that about a specific chunk of code, then I would the assert that the obvious answer is YES.

  • Like 1

Share this post


Link to post
Posted (edited)

I suspect the main reason this occurs as "smelly" is because there are so few reasons to do this kind of thing in Delphi. It's far more common in c/c++ as old-timers think it's perfectly acceptable.

 

if you have a justifiable need to do bit-whacking like this, then I say, do it. I probably wouldn't do it for, say, a mere 5% performance gain, but 50%+ is far more justifiable.

 

It would be nice to see some comments as to what's supposed to be going on because average Delphi coders won't make heads or tails of the code. (I personally have trouble dealing with pointers in Delphi, since I do it so seldom.)

 

And be especially careful to point out any dependencies on assumed sizes of vars, especially integers that can vary depending on the platform.

 

(Better yet, use explicit types that ensure fixed sizes and explain why.)

Edited by David Schwartz
  • Like 1

Share this post


Link to post

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

×