Skip to content

Fix Conversion Warnings with Conservative Marking - #1385

Merged
Aidan63 merged 4 commits into
HaxeFoundation:masterfrom
Aidan63:mark-conservative-no-warn
Sep 3, 2026
Merged

Fix Conversion Warnings with Conservative Marking#1385
Aidan63 merged 4 commits into
HaxeFoundation:masterfrom
Aidan63:mark-conservative-no-warn

Conversation

@Aidan63

@Aidan63 Aidan63 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

After a comment in #1339 this is the first of probably a few PRs where I'm going through the IMMIX GC with all of clangs conversion warnings set to errors and fixing them up.
In this case I've gone through the conservative marking function and what it calls to make them compile even if you enable all of clangs warnings as errors. There are a few things going on here.

  • Curly bracket initialisation is now used, this forbids narrowing / lossy conversion which will silently happen other wise.
  • uintptr_t is used instead of size_t for pointer manipulation. As of C++11 this type is the standard sanctioned way for pointer editing.
  • C++ style casts are used instead of C-style.

For reference below is the clang pragma I used to enable all the warnings as errors, I put these just inside the MarkConservative function with a pragma pop at the end of the function to limit them to just that function and what it calls.

#pragma clang diagnostic push
#pragma clang diagnostic error "-Wconversion"
#pragma clang diagnostic error "-Wsign-conversion"
#pragma clang diagnostic error "-Wimplicit"
#pragma clang diagnostic error "-Wimplicit-int-conversion"
#pragma clang diagnostic error "-Wall"
#pragma clang diagnostic ignored "-Wunused-variable"
#pragma clang diagnostic ignored "-Wunused-but-set-variable"

Aidan Lee added 2 commits September 2, 2026 20:43

@tobil4sk tobil4sk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like a good change. I've also got some WIP local fixes to help get rid of some unrelated warnings in other files, I'm hoping we can eventually have cleaner code and enable warnings more globally.

Comment thread src/hx/gc/Immix.cpp Outdated
void *lastWatch = 0;
bool isWatch = false;
uintptr_t lastWatch{};
bool isWatch{false};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems excessive for bools, is it intended?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got rid of the false since thats what bools default to.

Comment thread src/hx/gc/Immix.cpp Outdated
int z = 0;
#endif
printf("but got alloc type=%d, enclosing=%d nurs=%d o=%d\n",x,y,z,sgCheckInternalOffset);
printf("but got alloc type=%d, enclosing=%d nurs=%d o=%d\n", x, y, z, static_cast<int>(sgCheckInternalOffset));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here we probably want to change the format specifier, rather than casting to int

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did that because I wasn't sure how uintptr_t worked with printf, turns out you're supposed to use some magic PRIdPTR macro.

Comment thread src/hx/gc/Immix.cpp
hx::localCount++;
#endif
MemType mem = sGlobalAlloc->GetMemType(vptr);
MemType mem{ sGlobalAlloc->GetMemType(potentialObject) };

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here the braces also seem unnecessary, since it is MemType -> MemType

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree but I also don't like mixing and matching the initialisation syntaxes, so I decided to go with this one for all of them.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I understand the need for this syntax when converting between types initialisation, but using it in universally makes the code less natural and less readable imo. However if it's just limited to this marking function then I guess that's fine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think ideally I'd rather use auto for all this stuff, e.g.

auto pos = size_t{ potentialObject & IMMIX_BLOCK_BASE_MARK };
auto mem = sGlobalAlloc->GetMemType(potentialObject)

I think that's consistent and I can align the variable names which I like, but I can't remember if this is C++11 or 14. Stuff around bracket initialisation changed in 14.

@Aidan63
Aidan63 merged commit ecda943 into HaxeFoundation:master Sep 3, 2026
145 checks passed
@Aidan63
Aidan63 deleted the mark-conservative-no-warn branch September 3, 2026 16:06
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