Skip to content

Fix clang builds#9

Merged
neacsum merged 13 commits intoneacsum:mainfrom
cesiumlang:fix-clang
Oct 2, 2025
Merged

Fix clang builds#9
neacsum merged 13 commits intoneacsum:mainfrom
cesiumlang:fix-clang

Conversation

@emanspeaks
Copy link
Contributor

This PR presents numerous changes throughout the project to be more compatible with clang, which I have recently begun to use for my new project and encountered many issues using this library. Also various fixes and formatting adjustments to documentation.

@emanspeaks emanspeaks marked this pull request as draft September 28, 2025 16:15
@neacsum
Copy link
Owner

neacsum commented Sep 28, 2025

@emanspeaks I'm reviewing your changes and it seems you had troubles with CPM. Could you open an issue, either here on at CPM, and I'll try to solve it.

@emanspeaks emanspeaks marked this pull request as ready for review September 28, 2025 22:41
@emanspeaks
Copy link
Contributor Author

The main problem I was trying to solve with this PR was ultimately that there are slight syntactic differences between C++17 and C++20, neither of which MSVC enforces quite as strictly as Clang, so I was seeking a way to make code that would compile with both standards in both MSVC and Clang. Those changes are actually rather minor.

But testing the library turned out to be highly problematic: first trying to get it to build and run the tests and sample code locally and then, later, in the Actions workflow here once I created the PR. Furthermore, because I wanted to include this in my library using CMake FetchContent, I had a lot of issues also getting it to fetch utpp and, furthermore, lots of issues with differences in language standards between THOSE libraries.

CMake was struggling with having it put the utpp includes in the same include/ dir as the main utf8 repo, so I ended up changing the CMake to checkout utpp into another directory (namely, build/.dep-cache). This put it in conflict with how you have the CPM config for utf8, so I adjusted the GitHub action to match how I was doing things in CMake.

I am aware all of this sort of goes against the spirit of what you're trying to do with the current CPM config, so my hope was to first get this PR to pass the tests for the original problem of the core library code and handling language/compiler discrepancies, and then figured we can discuss if you're ok with this change or if you want to iterate on this and/or CPM (or utpp?) to make it work how you intended. However, in the latter case, I'd like to still have this repo support being installed via CMake as I was trying to do.

I wrote an issue for CPM and linked to this PR. As described above, the issue there was having CPM not providing an easy way to test changes on any branch, commit, or repo that was not the main branch of your copy of the repo, which makes it otherwise impossible to test or accept PRs from other users, something I don't think you intended 😄

@neacsum neacsum merged commit a9651fd into neacsum:main Oct 2, 2025
1 check passed
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