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

Include directories should be marked as system includes #2644

Closed
hadrielk opened this issue Dec 11, 2021 · 2 comments
Closed

Include directories should be marked as system includes #2644

hadrielk opened this issue Dec 11, 2021 · 2 comments

Comments

@hadrielk
Copy link

When compiling fmt by the direct CMake add_subdirectory(fmt) method (as recommended in the docs), the fmt build inherits the same compilation flags as the project including it.

But it fails for various errors/warnings, in clang at least.

For example missing-noreturn, signed-enum-bitfield, and documentation-unknown-command:

In file included from /fmt/include/fmt/format.h:48:
/fmt/include/fmt/core.h:451:5: error: unknown command tag name [-Werror,-Wdocumentation-unknown-command]
    \rst
    ^~~~
/fmt/include/fmt/core.h:604:38: error: function 'on_error' could be declared with attribute 'noreturn' [-Werror,-Wmissing-noreturn]
  void on_error(const char* message) { throw_format_error(message); }
                                     ^
/fmt/include/fmt/core.h:2075:9: error: enums in the Microsoft ABI are signed integers by default; consider giving the enum type an unsigned underlying type to make this code portable [-Werror,-Wsigned-enum-bitfield]
        align(align::none),
        ^

There were more instances than the above, but this gives the idea.

From my perspective, users of fmt would normally consider it a system include, not a regular part of their codebase. So I suggest that for the target_include_directories() in fmt/CMakeLists.txt, the SYSTEM cmake attribute be used, or at least a CMake OPTION be available to do so.

@vitaut
Copy link
Contributor

vitaut commented Dec 11, 2021

This should definitely not be the default because it would just hide potentially useful diagnostic but a PR to add a CMake option would be welcome.

@daquexian
Copy link

FYI, CMake 3.25 introduces add_subdirectory(... SYSTEM) to mark 3rd libraries as system includes: https://cmake.org/cmake/help/latest/command/add_subdirectory.html

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

No branches or pull requests

3 participants