Skip to content

fix member shadowing warnings#133

Merged
mourner merged 4 commits into
mapbox:masterfrom
skeptopic:fix-shadow-warnings
Jul 7, 2026
Merged

fix member shadowing warnings#133
mourner merged 4 commits into
mapbox:masterfrom
skeptopic:fix-shadow-warnings

Conversation

@skeptopic

Copy link
Copy Markdown
Contributor

MSVC treats locals that hide class members as an error under common strict-warning setups (C4458).

Rename the block and diagonal bbox locals in indexSegment and intersectsPolygon, and give the Refiner scratch vectors a Vec suffix so the raw pointers cached in refine's hot loop can keep the names used throughout the algorithm.

MSVC treats locals that hide class members as an error under common
strict-warning setups (C4458). Rename the block and diagonal bbox
locals in indexSegment and intersectsPolygon, and give the Refiner
scratch vectors a Vec suffix so the raw pointers cached in refine's
hot loop can keep the names used throughout the algorithm.
@skeptopic skeptopic requested a review from a team as a code owner July 6, 2026 19:53

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

Nice! Before we merge this, is there a compilation flag we could pass in CI to catch those issues in the future? Otherwise it's easy to merge the fix this one time but accidentally regress later.

@skeptopic

skeptopic commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Nice! Before we merge this, is there a compilation flag we could pass in CI to catch those issues in the future? Otherwise it's easy to merge the fix this one time but accidentally regress later.

Yep it's -WShadow on CLANG and /we4458 on MSVC, I pushed up a change to the CMAKE.

@skeptopic skeptopic requested a review from mourner July 7, 2026 08:32
@mourner

mourner commented Jul 7, 2026

Copy link
Copy Markdown
Member

Now failing 4 CI jobs — probably needs fixing or making the flags more granular (e.g. only affecting the source, not tests/bench)

@skeptopic

Copy link
Copy Markdown
Contributor Author

Yep, I see that. Do the tests run automatcially on this repo? Or do I need to kick something?

@mourner

mourner commented Jul 7, 2026

Copy link
Copy Markdown
Member

@skeptopic one failure remaining (Viz). You can see what CI is running in the config here https://github.com/mapbox/earcut.hpp/blob/master/.github/workflows/build.yml

@skeptopic

Copy link
Copy Markdown
Contributor Author

I think it's going to go green now. But now the commit history is nasty. Do you want me to squash it all into a single commit and re-open?

@mourner mourner merged commit f25bc76 into mapbox:master Jul 7, 2026
9 checks passed
@mourner

mourner commented Jul 7, 2026

Copy link
Copy Markdown
Member

No worries, GitHub can squash via UI when merging. Thanks for the contribution!

@skeptopic

Copy link
Copy Markdown
Contributor Author

Dude, thank you for this triangulator! It's awesome!

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