Skip to content

Fix compilation warnings and missing includes#190

Merged
nciric merged 3 commits into
unicode-org:mainfrom
nciric:fix-compilation-warnings
May 15, 2026
Merged

Fix compilation warnings and missing includes#190
nciric merged 3 commits into
unicode-org:mainfrom
nciric:fix-compilation-warnings

Conversation

@nciric
Copy link
Copy Markdown
Contributor

@nciric nciric commented May 14, 2026

This PR addresses several compilation warnings and missing includes encountered when building with stricter compilers and newer C++ standards (like C++20).

Changes included:
Missing standard headers: Added for getenv and llabs in ResourceLocator.cpp and NumberConcept.cpp.
Missing ICU headers: Added utf16.h, uchar.h, and utf8.h in files using ICU macros without explicit includes (NumberCleaverIterator.cpp, StringUtils.cpp, StringViewUtils.cpp).
Compiler Warning Fix: Removed an unused variable in ICUTokenExtractorIterator.cpp that triggered a nodiscard warning.
C++20 Compatibility: Added template deduction guides for CompressedArray and MarisaTrie in their respective headers to resolve warnings about class template argument deduction.

These changes help in making the codebase compatible with a wider range of compilers and standards.

All tests passed locally.

Added missing standard and ICU headers, removed unused variables, and added template deduction guides for C++20 compatibility.
@nciric nciric requested a review from grhoten May 14, 2026 23:22
@grhoten
Copy link
Copy Markdown
Member

grhoten commented May 15, 2026

This code already requires C++20 according to here: https://github.com/unicode-org/inflection/blob/main/inflection/CMakeLists.txt#L35

The code is definitely biased towards clang. I believe that I got the code to run on Linux with g++ once I removed the Core Foundation dependency. Either it's a different version of clang, or something beyond the typical clang++/g++, or another platform beyond the typical Linux.

The header issues are odd, but we can make those changes.

I'm not a fan of the template changes. Can you provide the actual warning? The call site should probably be changed so that the issue doesn't appear.

catch (const std::bad_alloc&) {
// Oh well, delete it below
e.what();
}
Copy link
Copy Markdown
Member

@grhoten grhoten May 15, 2026

Choose a reason for hiding this comment

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

This was meant to address a different warning about not handling such issues, but whatever. The behavior of ignoring the rare exception is intentional.

@nciric
Copy link
Copy Markdown
Contributor Author

nciric commented May 15, 2026

This code already requires C++20 according to here: https://github.com/unicode-org/inflection/blob/main/inflection/CMakeLists.txt#L35

The code is definitely biased towards clang. I believe that I got the code to run on Linux with g++ once I removed the Core Foundation dependency. Either it's a different version of clang, or something beyond the typical clang++/g++, or another platform beyond the typical Linux.

The header issues are odd, but we can make those changes.

I'm not a fan of the template changes. Can you provide the actual warning? The call site should probably be changed so that the issue doesn't appear.

The compiler (Clang) emitted -Wctad-maybe-unsupported warnings because Class Template Argument Deduction (CTAD) was used without explicit deduction guides or because the compiler wasn't sure if deduction was intended.

Let's change the call sites.

Copy link
Copy Markdown
Member

@grhoten grhoten left a comment

Choose a reason for hiding this comment

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

The changes look good. Do you have more information about the compiler or options used? Perhaps it can be added to the GitHub configurations to test so that this doesn't regress.

@nciric nciric merged commit 6cf92c1 into unicode-org:main May 15, 2026
8 checks passed
@nciric nciric deleted the fix-compilation-warnings branch May 15, 2026 16:24
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