.clang-format definition

[AJenbo]

It would be good if we could write a .clang-format that set the code style for the project. This would help developers stick to the right/consistent style.

Preferably clang-format would be run early on for the entire source so that it's cleaner to work with and masks less history from blame.

[nomdenom]

I tried writing one that conforms to the current style, where eg. there's a newline between } and while in a do loop, and spaces inside if ( ... ), but clang-format does not actually have options to format code like that, so we would have to pick & migrate to another code style.

We can infer from line numbers and the beta how code was originally written, but this will take time. The PSX symbol tells us how many lines each function is, so a single line function that has three lines, we can infer {} were on seperate lines.

[nomdenom]

Starting reading from https://github.com/nomdenom/devil-beta/blob/f32eb0ed7ec9ca33077de32d24c4548b4feb99ac/Source/control.cpp#L811 we can probably also infer that the braces for if/else were on the same line.
(so probably a variation of the standard K&R style ie. function braces on separate lines and control braces on the same line)

[mewmew]

Starting reading from https://github.com/nomdenom/devil-beta/blob/f32eb0ed7ec9ca33077de32d24c4548b4feb99ac/Source/control.cpp#L811 we can probably also infer that the braces for if/else were on the same line.

Good find.

Would have looked something like:

  if ( gbMaxPlayers == 1 ) {
    pBtmBuff = (char *)DiabloAllocPtr(0x16800, 1018, "C:\\Diablo\\Direct\\CONTROL.CPP");
    memset(pBtmBuff, 0, 0x16800u);
  } else {
    pBtmBuff = (char *)DiabloAllocPtr(0x2D000, 1021, "C:\\Diablo\\Direct\\CONTROL.CPP");
    memset(pBtmBuff, 0, 0x2D000u);
  }
  pManaBuff = (char *)DiabloAllocPtr(0x1E40, 1024, "C:\\Diablo\\Direct\\CONTROL.CPP");
  memset(pManaBuff, 0, 0x1E40u);
  pLifeBuff = (char *)DiabloAllocPtr(0x1E40, 1026, "C:\\Diablo\\Direct\\CONTROL.CPP");

[mewmew]

FreeMissileGFX is also quite interesting. I can't quite get it to match up though.

//----- (0043ED95) --------------------------------------------------------
void __fastcall FreeMissileGFX(int mi)
{
  signed int i; // [esp+10h] [ebp-4h]

  if ( misfiledata[mi].mFlags & 4 )
  {
    if ( misfiledata[mi].mAnimData[0] )
    {
      mem_free_dbg(
        &misfiledata[mi].mAnimData[0][-4 * misfiledata[mi].mAnimFAmt],
        907,
        "C:\\Diablo\\Direct\\MISSILES.CPP");
      misfiledata[mi].mAnimData[0] = 0;
    }
  }
  else
  {
    for ( i = 0; misfiledata[mi].mAnimFAmt > i; ++i )
    {
      if ( misfiledata[mi].mAnimData[i] )
      {
        mem_free_dbg(misfiledata[mi].mAnimData[i], 916, "C:\\Diablo\\Direct\\MISSILES.CPP");
        misfiledata[mi].mAnimData[i] = 0;
      }
    }
  }
}

[mewmew]

Similarly LoadMissileGFX is quite interesting.

And likewise, I can't get it match up correctly.

//----- (0043EAA9) --------------------------------------------------------
void __fastcall LoadMissileGFX(BYTE mi)
{
  char pszName[256]; // [esp+10h] [ebp-108h]
  int i; // [esp+110h] [ebp-8h]
  unsigned __int8 *anim; // [esp+114h] [ebp-4h]

  if ( misfiledata[mi].mFlags & 4 ) {
    sprintf(pszName, "Missiles\\%s.CEL", misfiledata[mi].mName);
    anim = LoadFileInMem(pszName, 0, 866, "C:\\Diablo\\Direct\\MISSILES.CPP");
    for ( i = 0; misfiledata[mi].mAnimFAmt > i; ++i ) {
      misfiledata[mi].mAnimData[i] = &anim[*(_DWORD *)&anim[4 * i]];
    }
  } else if ( misfiledata[mi].mAnimFAmt == 1 ) {
    sprintf(pszName, "Missiles\\%s.CEL", misfiledata[mi].mName);
    if ( !misfiledata[mi].mAnimData[0] ) {
      misfiledata[mi].mAnimData[0] = LoadFileInMem(pszName, 0, 874, "C:\\Diablo\\Direct\\MISSILES.CPP");
    }
  } else {
    for ( i = 0; misfiledata[mi].mAnimFAmt > i; ++i ) {
      sprintf(pszName, "Missiles\\%s%i.CEL", misfiledata[mi].mName, i + 1);
      if ( !misfiledata[mi].mAnimData[i] ) {
        misfiledata[mi].mAnimData[i] = LoadFileInMem(pszName, 0, 879, "C:\\Diablo\\Direct\\MISSILES.CPP");
      }
    }
  }
}

[mewmew]

Ah, a clue from codec.cpp.

Invocations of assertion_failed insert an if-statement in the decompiled code, but had no such if statement in the original code.

From codec_decode

  if ( !pbSrcDst )
    assertion_failed(130, "C:\\Diablo\\Direct\\CODEC.CPP");
  if ( !pszPassword )
    assertion_failed(131, "C:\\Diablo\\Direct\\CODEC.CPP");

Where the original probably looked as follows:

assert(pbSrcDst != NULL, __LINE__, __FILE__);
assert(pszPassword != NULL, __LINE__, __FILE__);

Or even (where line numbers and source file name is added in a macro expansion):

assert(pbSrcDst != NULL);
assert(pszPassword != NULL);

[mewmew]

The line info will also help us tell which if-branch was placed first.

e.g. from AddHall:

void __fastcall AddHall(int nX1, int nY1, int nX2, int nY2, int nHd)
{
  HALLNODE *v7; // [esp+14h] [ebp-8h]
  HALLNODE *i; // [esp+18h] [ebp-4h]

  if ( pHallList )
  {
    v7 = (HALLNODE *)DiabloAllocPtr(24, 1571, "C:\\Diablo\\Direct\\DRLG_L2.CPP");
    v7->nHallx1 = nX1;
    v7->nHally1 = nY1;
    v7->nHallx2 = nX2;
    v7->nHally2 = nY2;
    v7->nHalldir = nHd;
    v7->pNext = 0;
    for ( i = pHallList; i->pNext; i = i->pNext )
      ;
    i->pNext = v7;
  }
  else
  {
    pHallList = (HALLNODE *)DiabloAllocPtr(24, 1562, "C:\\Diablo\\Direct\\DRLG_L2.CPP");
    pHallList->nHallx1 = nX1;
    pHallList->nHally1 = nY1;
    pHallList->nHallx2 = nX2;
    pHallList->nHally2 = nY2;
    pHallList->nHalldir = nHd;
    pHallList->pNext = 0;
  }
}

Here, the branches of the if statements should swap place as the else branch of the decompiled code has a line number (1562) preceding the if-branch (1571).

[mewmew]

Hmm, seems like ternary if-statement may have been used. See for instance InitControlPan:

  pPanelButtons = LoadFileInMem("CtrlPan\\Panel8bu.CEL", 0, 1055, "C:\\Diablo\\Direct\\CONTROL.CPP");
  for ( j = 0; j < 8; ++j )
    panbtn[j] = 0;
  panbtndown = 0;
  if ( gbMaxPlayers == 1 )
    numpanbtns = 6;
  else
    numpanbtns = 8;
  pChrButtons = LoadFileInMem("Data\\CharBut.CEL", 0, 1061, "C:\\Diablo\\Direct\\CONTROL.CPP");

Using ternary if-statement to get it to match up:

  pPanelButtons = LoadFileInMem("CtrlPan\\Panel8bu.CEL", 0, 1055, "C:\\Diablo\\Direct\\CONTROL.CPP");
  for ( j = 0; j < 8; ++j ) {
    panbtn[j] = 0;
  }
  panbtndown = 0;
  numpanbtns = ( gbMaxPlayers == 1 ) ? 6 : 8;
  pChrButtons = LoadFileInMem("Data\\CharBut.CEL", 0, 1061, "C:\\Diablo\\Direct\\CONTROL.CPP");

Yeah it's highly likely they used macros everywhere for assertions, which influenced the line numbers. Even for the functions LoadFileInMem/mem_free_dbg, was probably like:

#ifdef _DEBUG
#define LoadFile(x) LoadFileInMem(x, type, __LINE__, __FILE__)
#define FreeFile(x) mem_free_dbg(x, type, __LINE__, __FILE__)
#else
#define LoadFile(x) LoadFileInMem(x)
#define FreeFile(x) mem_free_dbg(x)
#endif

[AJenbo]

@squidcc pointed out a section with a few interesting lines https://github.com/nomdenom/devil-beta/blob/f32eb0ed7ec9ca33077de32d24c4548b4feb99ac/Source/control.cpp#L858
The line numbers give us a strong indication that there where no braces for single line statments:

pChrButtons = LoadFileInMem("Data\\CharBut.CEL", 0, 1061, "C:\\Diablo\\Direct\\CONTROL.CPP");
for ( k = 0; k < 4; ++k )
    chrbtn[k] = 0;
chrbtnactive = 0;
pDurIcons = LoadFileInMem("Items\\DurIcons.CEL", 0, 1065, "C:\\Diablo\\Direct\\CONTROL.CPP");

Also https://github.com/nomdenom/devil-beta/blob/f32eb0ed7ec9ca33077de32d24c4548b4feb99ac/Source/control.cpp#L810 would probably have been formatted the following:

if ( pBtmBuff )
    assertion_failed(1016, "C:\\Diablo\\Direct\\CONTROL.CPP");
if ( gbMaxPlayers == 1 ) {
    pBtmBuff = (char *)DiabloAllocPtr(0x16800, 1018, "C:\\Diablo\\Direct\\CONTROL.CPP");
    memset(pBtmBuff, 0, 0x16800u);
} else {
    pBtmBuff = (char *)DiabloAllocPtr(0x2D000, 1021, "C:\\Diablo\\Direct\\CONTROL.CPP");
    memset(pBtmBuff, 0, 0x2D000u);
}

So again no braces for single line statements, but also both braces are on the same line as the else.

[AJenbo]

Another interesting part is https://github.com/nomdenom/devil-beta/blob/f32eb0ed7ec9ca33077de32d24c4548b4feb99ac/Source/control.cpp#L871 - 889 showing that they had at most 4 lines to handle the player class differences, I don't see any sane way to do this so maybe they used a macro for this?

[squidcc]

You could do that class handling stuff on two lines using stacked ternary operators. Also if _pClass was typed as an enum the compiler might have made the assumption that it could only be one of the defined values.

[AJenbo]

seritools pointed out this as an option:

if ( plr[myplr]._pClass == 0) SpellPages[0][0] = 26;
else if ( plr[myplr]._pClass == 1) SpellPages[0][0] = 28;
else if ( plr[myplr]._pClass == 2) SpellPages[0][0] = 27;

[seritools]

@squidcc tried exactly that, but I don't have faith in the compiler regarding that anymore :D Also typing _pClass as an enum would force it to 4 bytes size (specifying the storage size wasn't a thing back then)