5
\$\begingroup\$

I'd like to know if I translated a piece of code correctly from C++ to Delphi. It looks like it is working, but I have a feeling that I'm reading and writing into memory that I'm not supposed to using Delphi.

Given C++ code:

struct tile_map
{
    int32 CountX;
    int32 CountY;

    uint32 *Tiles;
};

inline uint32
GetTileValueUnchecked(tile_map *TileMap, int32 TileX, int32 TileY)
{
    uint32 TileMapValue = TileMap->Tiles[TileY*TileMap->CountX + TileX];
    return(TileMapValue);
}

uint32 Tiles00[9][17] =
    {
        {1, 1, 1, 1,  1, 1, 1, 1,  0, 1, 1, 1,  1, 1, 1, 1, 1},
        {1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1},
        {1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1},
        {1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1},
        {0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1},
        {1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1},
        {1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1},
        {1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1},
        {1, 1, 1, 1,  1, 1, 1, 1,  1, 1, 1, 1,  1, 1, 1, 1, 1},
    };
// More tile map declarations ...   
// uint32 Tiles01[9][17] = ...
// uint32 Tiles10[9][17] = ...
// uint32 Tiles11[9][17] = ...  

    tile_map TileMaps[2][2];
    TileMaps[0][0].CountX = 17;
    TileMaps[0][0].CountY = 9;
    TileMaps[0][0].Tiles = (uint32 *)Tiles00;

    TileMaps[0][1] = TileMaps[0][0];
    TileMaps[0][1].Tiles = (uint32 *)Tiles01;

    TileMaps[1][0] = TileMaps[0][0];
    TileMaps[1][0].Tiles = (uint32 *)Tiles10;

    TileMaps[1][1] = TileMaps[0][0];
    TileMaps[1][1].Tiles = (uint32 *)Tiles11;

// Usage
    int32 PlayerTileX = 2;
    int32 PlayerTileY = 2;
    uint32 TileMapValue = GetTileValueUnchecked(&TileMap[1][1], PlayerTileX, PlayerTileY);

Delphi translation:

program Project1;

{$APPTYPE CONSOLE}

type
    Puint32 = ^uint32;

    tile_map = record
        CountX : int32;
        CountY : int32;

        Tiles : Puint32;
    end;
    Ptile_map = ^tile_map;

{$POINTERMATH ON}   
function GetTileValueUnchecked(TileMap : Ptile_map; TileX, TileY : int32) : uint32; inline;
begin
    result := TileMap^.Tiles[TileY * TileMap^.CountX + TileX];
end;


const //in the future these will be read from file, so const for now
    Tiles00:  array [0..8, 0..16] of uint32 =
    (
        (1, 1, 1, 1,  1, 1, 1, 1,  0, 1, 1, 1,  1, 1, 1, 1, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 1, 1, 1,  1, 1, 1, 1,  1, 1, 1, 1,  1, 1, 1, 1, 1)
    );
    // More tile map declarations ...
    //Tiles01:  array [0..8, 0..16] of uint32 = ...
    //Tiles10:  array [0..8, 0..16] of uint32 = ...
    //Tiles11:  array [0..8, 0..16] of uint32 = ...
var 
    TileMaps : array [0..1, 0..1] of  tile_map;
    PlayerTileX, PlayerTileY : int32;
    TileMapValue : uint32;
begin

    TileMaps[0][0].CountX := 17;
    TileMaps[0][0].CountY := 9;
    TileMaps[0][0].Tiles := Addr(Tiles00);

    TileMaps[0][1] := TileMaps[0][0];
    TileMaps[0][1].Tiles := Addr(Tiles01);

    TileMaps[1][0] := TileMaps[0][0];
    TileMaps[1][0].Tiles := Addr(Tiles10);

    TileMaps[1][1] := TileMaps[0][0];
    TileMaps[1][1].Tiles := Addr(Tiles11);

    // Usage
    PlayerTileX := 2;
    PlayerTileY := 2;
    TileMapValue = GetTileValueUnchecked(@TileMaps[1][1], PlayerTileX, PlayerTileY);
end.
\$\endgroup\$
  • \$\begingroup\$ I am down voting your question because it has broken code. \$\endgroup\$ – cpicanco Nov 27 '15 at 4:34
1
\$\begingroup\$

Everything there looks right (except for the = on the last line, which should be :=). It's about as close to a literal translation as you can get.

Converting the Delphi code to use dynamic arrays could ease your mind regarding out-of-bounds access, although the initial setup would be more cumbersome. (That might not be a problem if you're ultimately planning on loading the data from a file instead of from constants.) It would look something like this:

program Project1;

{$APPTYPE CONSOLE}

type
    tile_map = array of array of uint32;

function GetTileValueUnchecked(const TileMap: tile_map; TileX, TileY: int32): uint32; inline;
begin
    result := TileMap[TileY][TileX];
end;

const //in the future these will be read from file, so const for now
    Tiles00:  array [0..8, 0..16] of uint32 =
    (
        (1, 1, 1, 1,  1, 1, 1, 1,  0, 1, 1, 1,  1, 1, 1, 1, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0,  0, 0, 0, 0, 1),
        (1, 1, 1, 1,  1, 1, 1, 1,  1, 1, 1, 1,  1, 1, 1, 1, 1)
    );
    // More tile map declarations ...
    //Tiles01:  array [0..8, 0..16] of uint32 = ...
    //Tiles10:  array [0..8, 0..16] of uint32 = ...
    //Tiles11:  array [0..8, 0..16] of uint32 = ...
var 
    TileMaps : array [0..1, 0..1] of  tile_map;
    PlayerTileX, PlayerTileY : int32;
    TileMapValue : uint32;
    x, y: Integer;
begin
    SetLength(TileMaps[0][0], Length(Tiles00), Length(Tiles00[0]));
    for y := 0 to High(Tiles00) do
      for x := 0 to High(Tiles00[0]) do
        TileMaps[0][0][y][x] := Tiles00[y][x];

    SetLength(TileMaps[0][1], Length(Tiles01), Length(Tiles01[0]));
    for y := 0 to High(Tiles01) do
      for x := 0 to High(Tiles01[0]) do
        TileMaps[0][1][y][x] := Tiles01[y][x];

    SetLength(TileMaps[1][0], Length(Tiles10), Length(Tiles10[0]));
    for y := 0 to High(Tiles10) do
      for x := 0 to High(Tiles10[0]) do
        TileMaps[1][0][y][x] := Tiles10[y][x];

    SetLength(TileMaps[1][1], Length(Tiles11), Length(Tiles11[0]));
    for y := 0 to High(Tiles11) do
      for x := 0 to High(Tiles11[0]) do
        TileMaps[1][1][y][x] := Tiles11[y][x];

    // Usage
    PlayerTileX := 2;
    PlayerTileY := 2;
    TileMapValue := GetTileValueUnchecked(TileMaps[1][1], PlayerTileX, PlayerTileY);
end.

Test with that code for a little while to convince yourself that the array accesses are right. If they're not, Delphi's range checking will alert you. Once you're confident, you can disable range checking inside GetTileValueUnchecked so that it's true to its name:

function GetTileValueUnchecked(const TileMap: tile_map; TileX, TileY: int32): uint32; inline;
begin
    {$R-}
    result := TileMap[TileY][TileX];
    {$R+}
end;
\$\endgroup\$
  • \$\begingroup\$ Thanks for the tip about the range checking. Will it improve performance if switched off? \$\endgroup\$ – Stefan Badenhorst Jul 23 '15 at 5:42
  • \$\begingroup\$ I am down voting this answer. Reviewing broken code is not healthy for the Code Review community. \$\endgroup\$ – cpicanco Nov 27 '15 at 4:36
  • \$\begingroup\$ Thanks for the comment, @Cpicanco. It seems petty, though, to base your vote on a one-character typo that doesn't affect the execution of the code, and is really just tangential to the main code being reviewed anyway. You're free to vote however you please, of course. \$\endgroup\$ – Rob Kennedy Nov 27 '15 at 5:09
  • \$\begingroup\$ Yes, Stefan, disabling range checking generally does improve performance (but measure it yourself to confirm in your specific situation). We generally don't recommend disabling it because range checking helps find bugs and the cost is generally low. \$\endgroup\$ – Rob Kennedy Nov 27 '15 at 5:12
  • \$\begingroup\$ I am sorry, I don't want to sound petty. I am exploring the limits of the community rules, just trying to figure out what would be a proper action. Please, fell free to participate in the discussion meta.codereview.stackexchange.com/questions/6217/… \$\endgroup\$ – cpicanco Nov 27 '15 at 8:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.