I personally like putting the asterisk with the type, because despite
the language parsing the asterisk as a part of the name, the pointer
part is clearly a part of the return type of the function. Also,
put constness here, to indicate that the input won't be modified inside
the function.
This comment indicates that the function is used by
UtilityClass::GCString(). Which is unnecessary, because the reader can
trivially search for usages of GCChar in the file itself (the 'static'
preceding the function should be a good enough hint) - and if there
aren't any, then the reader will know the function is unused, whereas if
they read the comment, they would have been under the assumption that it
wasn't used. (There might also a compiler warning about it being unused,
which would be more confusing if the comment was still there.)
Point is, comments can get outdated, so removing the comment here makes
the code more self-documenting.
During 2.3 development, there's been a gradual shift to using SDL stdlib
functions instead of libc functions, but there are still some libc
functions (or the same libc function but from the STL) in the code.
Well, this patch replaces all the rest of them in one fell swoop.
SDL's stdlib can replace most of these, but its SDL_min() and SDL_max()
are inadequate - they aren't really functions, they're more like macros
with a nasty penchant for double-evaluation. So I just made my own
VVV_min() and VVV_max() functions and placed them in Maths.h instead,
then replaced all the previous usages of min(), max(), std::min(),
std::max(), SDL_min(), and SDL_max() with VVV_min() and VVV_max().
Additionally, there's no SDL_isxdigit(), so I just implemented my own
VVV_isxdigit().
SDL has SDL_malloc() and SDL_free(), but they have some refcounting
built in to them, so in order to use them with LodePNG, I have to
replace the malloc() and free() that LodePNG uses. Which isn't too hard,
I did it in a new file called ThirdPartyDeps.c, and LodePNG is now
compiled with the LODEPNG_NO_COMPILE_ALLOCATORS definition.
Lastly, I also refactored the awful strcpy() and strcat() usages in
PLATFORM_migrateSaveData() to use SDL_snprintf() instead. I know save
migration is getting axed in 2.4, but it still bothers me to have
something like that in the codebase otherwise.
Without further ado, here is the full list of functions that the
codebase now uses:
- SDL_strlcpy() instead of strcpy()
- SDL_strlcat() instead of strcat()
- SDL_snprintf() instead of sprintf(), strcpy(), or strcat() (see above)
- VVV_min() instead of min(), std::min(), or SDL_min()
- VVV_max() instead of max(), std::max(), or SDL_max()
- VVV_isxdigit() instead of isxdigit()
- SDL_strcmp() instead of strcmp()
- SDL_strcasecmp() instead of strcasecmp() or Win32 strcmpi()
- SDL_strstr() instead of strstr()
- SDL_strlen() instead of strlen()
- SDL_sscanf() instead of sscanf()
- SDL_getenv() instead of getenv()
- SDL_malloc() instead of malloc() (replacing in LodePNG as well)
- SDL_free() instead of free() (replacing in LodePNG as well)
This patch cleans up unnecessary exports from header files (there were
only a few), as well as adds the static keyword to all symbols that
aren't exported and are specific to a file. This helps the linker out in
not doing any unnecessary work, speeding it up and avoiding silent
symbol conflicts (otherwise two symbols with the same name (and
type/signature in C++) would quietly resolve as okay by the linker).
By "unnecessary qualifiers to self", I mean something like using the
'game.' qualifier for a variable on the Game class when you're inside a
function on the Game class itself. This patch is to enforce consistency
as most of the code doesn't have these unnecessary qualifiers.
To prevent further unnecessary qualifiers to self, I made it so the
extern in each header file can be omitted by using a define. That way,
if someone writes something referring to 'game.' on a Game function,
there will be a compile error.
However, if you really need to have a reference to the global name, and
you're within the same .cpp file as the implementation of that object,
you can just do the extern at the function-level. A good example of this
is editorinput()/editorrender()/editorlogic() in editor.cpp. In my
opinion, they should probably be split off into their own separate file
because editor.cpp is getting way too big, but this will do for now.
It's possible that SDL_atoi() could call the libc atoi(), and if a
string is provided that's too large to fit into an integer, then that
would result in undefined behavior. To avoid this, use SDL_strtol()
instead.
If necessary, the caller can provide a fallback to be returned in case
the input given isn't a valid integer, instead of having to duplicate
the is_number() check.
This is simply a wrapper function around SDL_atoi(), because SDL_atoi()
could call libc atoi(), and whether or not invalid input passed into the
libc atoi() is undefined behavior depends on the given libc. So it's
safer to just add a wrapper function that checks that the string given
isn't bogus.
std::isdigit() can be replaced with SDL_isdigit(), but there's no
equivalent for std::isxdigit() in the SDL standard library. So we'll
just use libc isxdigit() instead, it's fine.
Okay, so basically here's the include layout that this game now
consistently uses:
[The "main" header file, if any (e.g. Graphics.h for Graphics.cpp)]
[blank line]
[All system includes, such as tinyxml2/physfs/utfcpp/SDL]
[blank line]
[All project includes, such as Game.h/Entity.h/etc.]
And if applicable, another blank line, and then some special-case
include screwy stuff (take a look at editor.cpp or FileSystemUtils.cpp,
for example, they have ifdefs and defines with their includes).
That's how it should be done, because the SDL headers aren't going to be
installed in this repository. The game was a bit inconsistent before but
now it isn't anymore.
This ensures that endsWith() can be used outside of editor.cpp.
When leo60228 originally wrote endsWith(), it was static, but I asked
him on Discord just now and he more-or-less confirmed that it's fine if
it's not static. If it was static, it would be confined to
UtilityClass.cpp now instead!
Apparently it results in Undefined Behavior if the argument given isn't
representable as an unsigned char. This means that (potentially) plain
char and signed chars could be unsafe to use as well.
It's rare that this could happen in practice, though. std::isdigit() is
only used by is_positive_num() which is only used by find_tag(), so
someone would have to deliberately put something crazy after an `&#` in
a custom level file in order for this to happen. Still, better to be
safe than sorry and all.
This fixes a compile error that could happen where the compiler doesn't
know what std::isdigit() is, but I'm puzzled as to why this wasn't
happening earlier, 'cause I've only been reported that it happens by
only one person.
Unlike, say, the scriptclass i/j/k stuff, these tempstrings are only
purely visual, and there's no known glitches to manipulate them anyway.
Thus I think it's safe to make this cleanup.
This fixes a source of undefined behavior, where the int returned by
ss_toi() would be random garbage memory if the string passed into it
would be empty. That's because if the string is empty, there are no
characters to parse, so nothing simply gets put into x.
The easiest way to pass an empty string in to ss_toi() would be to use
script commands with empty arguments.
Previously, it was a hardcoded list going up to fifty.
This time, it's less hardcoded. Most of the time, it will take a number,
find out its tens-place word, find out its ones-place word, and combine
the two together. There are special cases for the teens, and the numbers
zero through nine, and one hundred.
Also, now if it is given a negative value, it will just return "???"
instead.
If there are more than one hundred it will still say "Lots".
