Skip to content

Conversation

@AbitTheGray
Copy link

@AbitTheGray AbitTheGray commented Oct 12, 2025

There are many changes that I've bundled into a single MR. Please say which changes you want to keep and I'll cherrypick them into dedicated MR(s).

  • First commit (eb5b67b) are many changes (see text of the commit), but those are the main changes I suggest merging.
  • Second commit (96d93d9) just replaces 35 in all the places with TICRATE. I did not try changing value of TICRATE before and after the change as I don't see it as a constant that is meant to be tweaked but more like naming the "magic number" to make the code more readable
  • Third commit (01f6cfc) unifies line indentation style in several files that had multiple styles in them. I recommend merging this one, especially for UDMF where 1st entry used one indent and the rest used a different one.

There are two more changes I've considered:

  • const char * comp_lev_str[MAX_COMPATIBILITY_LEVEL] is checked as if (sizeof(comp_lev_str)/sizeof(comp_lev_str[0]) != MAX_COMPATIBILITY_LEVEL) (both in g_game.c) which should never happen and would result in I_Error("G_ReadDemoHeader: compatibility level strings incomplete");
  • cmd->angleturn = ((unsigned char)(at = *(*data_p)++))<<8; in g_game.c where unsigned char is shifted left by 8, which is the same size as unsigned char - as I understand it, it is auto-promoted to unsigned int before the shift but I think it should be cast to it instead of casting it to type it already is (at is already unsigned char).

- missing space between `"SpechitOverrun: Warning: unable to emulate"` and `"an overrun where numspechit=%i\n"`
- `s3_floorheight >> 16` should use `FRACBITS` as it is `fixed_t`
- enums in `mm_menu.c` were either missing `typedef` before `enum` or the name should be moved after `enum` as the old way created variables that are left unused
Don't mix tabs and spaces for formatting, make it uniform across the whole file.
In general, two spaces are used. But new C++ code uses tabs and windows-specific code uses three tabs.
"\xc3\xb9",
"\xc3\xbf",
"\xc3\x96",
"\xc3\x9c ",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This surprised me. It is a tab but without visible whitespace characters, it looks like a space. Found it only while looking for \t indentation. Did not check whenever or not it makes sense there, but there are other strings with 3 characters in the array, just no tab.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it's in a conversion table between two binary text formats, so it's probably supposed to be there, and definitely shouldn't be changed without understanding the data thoroughly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing it to explicit \t does not change any behavior, just makes it easier to recognize for the character it is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's totally reasonable, then. I'd apparently misunderstood the diff.

@thecnoNSMB
Copy link
Contributor

thecnoNSMB commented Oct 12, 2025

I'm not a code reviewer, but I've noticed several places where your whitespace changes break alignment of similar elements in the code. Did you "find-replace" tabs with spaces in the codebase without reviewing the changes yourself? Nevermind, I'd read the diff backwards lol

@AbitTheGray
Copy link
Author

Thanks to @thecnoNSMB , I've double-checked the indentations and there was one find&replace error that I did not notice in gamemodes of DSDADoom_misc.cfg, I've fixed it. Even while I prefer tabs for indentation, I don't think they should appear anywhere else than at the start of the line as they usually break the formatting when you change the width when they are in the text (like there were, by my mistake).

static void D_DrawTitle1(const char *name)
{
D_SetPage(name, TICRATE * 170 / 35, mus_intro);
D_SetPage(name, TICRATE * 170 / 35, mus_intro); // 35 * 170 / 35, is that correct math?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can make this just be 170 ig

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants