Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: Enable system library semantics for nall in other targets #1839

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

jcm93
Copy link
Contributor

@jcm93 jcm93 commented Feb 17, 2025

Description

When other targets (ares, ruby, hiro, etc.) are compiled, they will now treat the 'include directory' for nall (the directory from which nall include paths are resolved, i.e. #include <nall/nall.hpp>) as a "system" directory, using the SYSTEM CMake property.

Principally, the directory having this property means that compiling non-nall targets will not emit compiler diagnostic flags from code contained in nall headers. The exact behavior will depend on the compiler's interpretation of the "system" property.

This is more idiomatic behavior for nall being treated as a "standard library", and moreover a "header-only" standard library, where including headers includes the definitions of all relevant functions (other than certain system-conflicting headers previously separated into separate implementations in #898 and #1064).

Also adds the headers.cpp file to nall, which includes all nall headers that are included in different parts of ares.

The addition of headers.cpp means that compiling nall itself will also compile these headers (once), and thus we can define diagnostic flags for nall separately and make use of diagnostic information effectively on a per-target basis; we may still receive compiler diagnostic information for our standard library.

To enable this behavior, the entirety of nall is moved down one level in the source tree into a nall parent directory. Since CMake defines the SYSTEM property on a directory basis (or at least, this is how it must be defined for our usage of nall as a mostly header-centric library), and nall paths must be resolved such that #include <nall/nall.hpp> is a valid path, this change is necessary so that other targets are not also treated as libraries with SYSTEM semantics when their headers or implementations are #included relative to the top-level repository directory.

This behavior is switched by the CMake cache variable ARES_TREAT_NALL_AS_SYSTEM.

Motivation and Context

As part of ares upkeep and build system modernization, we may wish to increase compiler strictness in ares and build the project with more diagnostic flags enabled, to catch bugs in new and old code, keep up with evolving C/C++ standards, and generally improve code quality and readability where appropriate.

Under our existing build system code, defining diagnostic flags on a per-target basis did not work effectively. For example:

target_compile_options(ruby PRIVATE -Wcomma)

Semantically, this would indicate we want to raise the -Wcomma diagnostic flag in ruby code and ruby code only. But under our current patterns, this flag would necessarily not only be raised in ruby code, but in all nall headers that ruby code directly #includes. And since nall is our standard library, and the vast majority of nall function definitions are present in these headers by virtue of nall being largely a 'header-only' library, each inclusion would raise the flag for most of, if not the entire nall standard library.

This situation would be further compounded in targets with many translation units (such as ares), which could include the omnibus nall/nall.hpp header dozens of times. For a warning like -Wshorten-64-to-32, this could generate tens of thousands of identical compiler warnings in the same source files, with the potential to overwhelm available memory and slow build times to a crawl.

With these headers in "system" directories, we can apply flags per-target and successfully receive warnings for only code that is semantically part of those targets.

@jcm93 jcm93 force-pushed the nall-guard-the-third branch from b096a3e to 5de2fdc Compare February 18, 2025 18:47
@LukeUsher LukeUsher merged commit 32fd70f into ares-emulator:master Feb 19, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants