Conversation
6ac78ff to
7b2cbc8
Compare
rubidium42
left a comment
There was a problem hiding this comment.
I support the removal of boost.
| #define INTERNAL_ERROR(var, val) \ | ||
| do { \ | ||
| const std::source_location location = std::source_location::current(); \ | ||
| IssueMessage(0, INTERNAL_ERROR_TEXT, location.file_name(), location.line(), _spritenum, #var, val, location.function_name()); \ | ||
| assert(false); \ | ||
| exit(EFATAL); \ | ||
| } while (false) |
There was a problem hiding this comment.
Why not convert this into a [[noreturn]] function while you're at it? Something akin:
[[noreturn]] static inline void INTERNAL_ERROR(std::string_view var, const auto &val, const std::source_location location = std::source_location::current())
{
IssueMessage(0, INTERNAL_ERROR_TEXT, location.file_name(), location.line(), _spritenum, var, val, location.function_name());
assert(false);
exit(EFATAL);
}
There was a problem hiding this comment.
I did try that initially, but ran into the issue that var is not always a string and I wasn't willing to change all the callsites. That said doing so is probably smaller than all the other changes I ended up making
There was a problem hiding this comment.
The other reason not to is that it depends on the INTERNAL_ERROR_TEXT string and _spritenum globals, which either means being dependent on header include order, or trying to somehow fit the function into a cpp file (which is difficult due to the 'auto')
src/nforenum.h
Outdated
| if(!(cond))INTERNAL_ERROR(var,var);\ | ||
| else\ | ||
| ((void)0) | ||
| if(!(cond))INTERNAL_ERROR(#var,var) |
There was a problem hiding this comment.
This triggers my spidey-sense:
if (foo) VERIFY(bar, baz);
else std::println("foo: {}", foo);
Is going to print foo: true. I know this situation doesn't exist in the code, but it might become surprising at some point. The previous code did not open that hole.
src/pseudo.cpp
Outdated
| - static_cast<local_days>(SINCE_1920) | ||
| + static_cast<day>(extra)); |
There was a problem hiding this comment.
For what it's worth, I reckon extra is equivalent to either 0 or local_days(SINCE_1920). It would be more logical if extra became something like epoch, with 0 in the _D_ branch and SINCE_1920 in the _W_ branch. And then just subtracting epoch in this return.
I'm not going to claim it works, needs someone with far more knowledge of grfcodec to me to actually test it. But it does compile, so...
boost::for_eachwith actual ranged for loops and algorithmsboost::bimapsusage for NFO escapes withstd::vector<std:pair>boost::gregorianwithstd::chrono