Skip to content

Conversation

@serprex
Copy link
Contributor

@serprex serprex commented May 18, 2025

@serprex serprex force-pushed the fix-warnings branch 2 times, most recently from cb8b83c to 22eac0c Compare May 18, 2025 22:42
@serprex
Copy link
Contributor Author

serprex commented May 18, 2025

First commit is not about warnings, but instead addressing TODO about whether uint8_t is necessary

I've updated hint code to use size_t in places where the indexes are just being passed around, & not actually stored anywhere. At best uint8_t in these scenarios generates similar code, at worst you have the compiler generating unnecessary code to maintain 8 bit semantics on size_t registers

Copy link
Contributor

@briaguya0 briaguya0 left a comment

Choose a reason for hiding this comment

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

overall looks great! couple little comments, one has a small suggestion

@serprex
Copy link
Contributor Author

serprex commented May 20, 2025

this seemed to fix something for @Pepper0ni

@Rozelette
Copy link
Contributor

You've marked a lot of classes final, and there's nothing wrong with that. But, the root of the problem is that their parent classes do not have a virtual destructor. These warnings are going to crop up again if more class inherit from these parents. These problems should be fixed upstream, like I've done in Kenix3/libultraship#871. I see you've address this issue in Kenix3/libultraship#886, but I'll stress that the better fix is to mark the parents' destructors as virtual. It is good practice in C++ to have virtual destructors if a class is intended to be inherited from, and expecting children to be final will lead to obscure bugs.

Copy link
Contributor

@Pepper0ni Pepper0ni left a comment

Choose a reason for hiding this comment

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

The hint stuff seems fine, but I can't give an opinion on Rozelette's comment.

serprex added 3 commits May 23, 2025 15:45
warning: second argument to 'va_arg' is of promotable type 's16' (aka 'short'); this va_arg has undefined behavior because arguments will be promoted to 'int' [-Wvarargs]
@Malkierian Malkierian merged commit 3a069e6 into HarbourMasters:develop May 23, 2025
6 checks passed
krazyjakee pushed a commit to krazyjakee/OOT that referenced this pull request Sep 6, 2025
* use size_t instead of uint8_t for hint ids

* va_arg int instead of s16

warning: second argument to 'va_arg' is of promotable type 's16' (aka 'short'); this va_arg has undefined behavior because arguments will be promoted to 'int' [-Wvarargs]

* more issues like HarbourMasters#5443
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.

5 participants