Skip to content

Conversation

@LordAro
Copy link
Member

@LordAro LordAro commented Dec 8, 2025

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...

  • Replaces boost::for_each with actual ranged for loops and algorithms
  • Replaces boost::bimaps usage for NFO escapes with std::vector<std:pair>
    • I'd be amazed if there are enough escapes to give the bimap structure any benefit
  • Replaces boost::gregorian with std::chrono
  • Replaces some godawful preprocessor-generated classes with some inheritance (I'm least sure of this)

@LordAro LordAro force-pushed the la-rm-boost branch 2 times, most recently from 115fa07 to 6ac78ff Compare December 8, 2025 22:59
Copy link
Contributor

@rubidium42 rubidium42 left a comment

Choose a reason for hiding this comment

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

I support the removal of boost.

Comment on lines +54 to +60
#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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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); 
}

Copy link
Member Author

Choose a reason for hiding this comment

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

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

if(!(cond))INTERNAL_ERROR(var,var);\
else\
((void)0)
if(!(cond))INTERNAL_ERROR(#var,var)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +840 to +841
- static_cast<local_days>(SINCE_1920)
+ static_cast<day>(extra));
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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.

2 participants