adventuregamestudio / ags

AGS editor and engine source code
Other
672 stars 159 forks source link

Compiler: preprocessor can build without compiler #2410

Closed ericoporto closed 3 weeks ago

ericoporto commented 3 weeks ago

This PR is targeting master branch but it is actually useful in ags4 and part of #2328 as it enables building the preprocessor with both the legacy compiler but also the new ags4 compiler.

With these changes I can build the preprocessor independently of the compiler.

ivan-mogilko commented 3 weeks ago

It's better to use standard library functions instead, unless there's a clear purpose for these?

https://en.cppreference.com/w/cpp/header/cctype

is_digit is clearly replaceable with isdigit standard function. is_whitespace may be replaced with isspace.

is_alphanum name is misleading, because it combines isalnum standard function with 3 more symbols. I suggest renaming it to something more suitable; it could be modified to work as:

return std::isalnum(ch) || ch == '_' || ch == '\"' || ch == '\'';

The quick examples of new name that just came to mind: is_word, is_wordchar, etc.

EDIT: this function is strange, because it has unusual combination of characters. Taking a glance at its uses (there are only 3 cases) makes me wonder if it should not be used in some of them... because at least 1 of them should not let quote chars.

ericoporto commented 3 weeks ago

It's better to use standard library functions instead

Thanks, I will adjust to go that road instead. I will try to update this tonight.

ericoporto commented 3 weeks ago

I found a bug in the cpp preprocessor that made it pass the tests accidentally, lol, working on a fix.

I assumed the String.FindChar was identical to a function that was written in the C# preprocessor, but I didn't noticed it was written to be able to ignore escaped characters. Fixed below.

ivan-mogilko commented 3 weeks ago

I think you might be missing a standard header somewhere.

ericoporto commented 3 weeks ago

Apparently MSVC in VS 2015 and 2017 requires cctype header here, but for some reason MSVC in VS 2019 and 2022 doesn't. Fixed.

Edit: hey, after this merges would be nice to get it in ags4 too 😬

Edit2: had to do a small style fix and took the time to add two new tests.

ivan-mogilko commented 3 weeks ago

Apparently MSVC in VS 2015 and 2017 requires cctype header here, but for some reason MSVC in VS 2019 and 2022 doesn't.

Some compilers may have extra headers included into other headers, which results in them being added implicitly. It's safer to include required standard header always.

ivan-mogilko commented 3 weeks ago

Ah, you ended up not removing unnecessary functions like is_digit and is_alnum here.