Skip to content

[libc] [Task] Prepare to enable disabled warnings #122835

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.

Sign up for GitHub

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

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

[libc] [Task] Prepare to enable disabled warnings #122835

Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
00a77f0
-Wunused
vinay-deshmukh Jan 14, 2025
3c023ed
-Wno-global-constructors for TEST_F
vinay-deshmukh Jan 14, 2025
bbec43a
trigger warnings
vinay-deshmukh Jan 14, 2025
badca59
-Wimplicit-int-conversion
vinay-deshmukh Jan 14, 2025
1a2a90d
-Wextra-semi
vinay-deshmukh Jan 14, 2025
f375ca5
clang-format patch
vinay-deshmukh Jan 14, 2025
ba19d9a
full build warnings from CI
vinay-deshmukh Jan 14, 2025
90e7a06
Refactor CMake and temporarily disable warnings
vinay-deshmukh Jan 16, 2025
d4b2fec
Merge remote-tracking branch 'upstream/main' into vinay-issue-119281
vinay-deshmukh Jan 16, 2025
9c71b2a
Merge branch 'vinay-issue-119281' of out:vinay-deshmukh/llvm-project …
vinay-deshmukh Jan 16, 2025
fb7d4ba
whitespace
vinay-deshmukh Jan 16, 2025
26d4b30
undo code changes
vinay-deshmukh Jan 16, 2025
dd8de4c
Unused command line argument
vinay-deshmukh Jan 17, 2025
85acd9a
-Wno-missing-field-initializers
vinay-deshmukh Jan 17, 2025
71dcc06
Add note on why the no-missing-initializers is needed
vinay-deshmukh Jan 17, 2025
67ed339
Refer to bug 122367
vinay-deshmukh Jan 18, 2025
be54a01
Refer to 123434
vinay-deshmukh Jan 18, 2025
9e7c2f8
Merge remote-tracking branch 'upstream/main' into vinay-issue-119281
vinay-deshmukh Jan 18, 2025
61e3690
Merge branch 'main' into vinay-issue-119281
vinay-deshmukh Jan 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 35 additions & 16 deletions libc/cmake/modules/LLVMLibCTestRules.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,34 +25,53 @@ function(_get_common_test_compile_options output_var c_test flags)
list(APPEND compile_options "-ffixed-point")
endif()

# list(APPEND compile_options "-Wall")
# list(APPEND compile_options "-Wextra")
list(APPEND compile_options "-Wall")
list(APPEND compile_options "-Wextra")
# -DLIBC_WNO_ERROR=ON if you can't build cleanly with -Werror.
if(NOT LIBC_WNO_ERROR)
# list(APPEND compile_options "-Werror")
list(APPEND compile_options "-Werror")

# TODO (https://github.com/llvm/llvm-project/issues/122367#issuecomment-2581374103)
list(APPEND compile_options "-Wno-unused-command-line-argument")

# TODO (https://github.com/llvm/llvm-project/issues/123434)
list(APPEND compile_options "-Wno-missing-field-initializers")
endif()
# list(APPEND compile_options "-Wconversion")
# list(APPEND compile_options "-Wno-sign-conversion")
# list(APPEND compile_options "-Wimplicit-fallthrough")
# list(APPEND compile_options "-Wwrite-strings")
# list(APPEND compile_options "-Wextra-semi")
list(APPEND compile_options "-Wconversion")
list(APPEND compile_options "-Wno-sign-conversion")
list(APPEND compile_options "-Wimplicit-fallthrough")
list(APPEND compile_options "-Wwrite-strings")
list(APPEND compile_options "-Wextra-semi")
# Silence this warning because _Complex is a part of C99.
if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
if(NOT c_test)
list(APPEND compile_options "-fext-numeric-literals")
endif()
else()
elseif (CMAKE_CXX_COMPILER_ID MATCHES "Clang$")
Copy link
Member

Choose a reason for hiding this comment

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

How come you use MATCHES with $ rather than STREQUAL without $?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, since I'm using AppleClang on a macOS m1, I figured I could use MATCHES to match Clang and other clang-like compilers

list(APPEND compile_options "-Wnewline-eof")
list(APPEND compile_options "-Wnonportable-system-include-path")
list(APPEND compile_options "-Wstrict-prototypes")
list(APPEND compile_options "-Wthread-safety")
# TODO(https://github.com/llvm/llvm-project/issues/119281) triggered in TEST_F implementation
list(APPEND compile_options "-Wno-global-constructors")

# TODO(https://github.com/llvm/llvm-project/issues/119281)
# These have been disabled for the time being to create explicit PRs for each change.
list(APPEND compile_options "-Wno-unused-parameter")
list(APPEND compile_options "-Wno-implicit-int-conversion")
list(APPEND compile_options "-Wno-shorten-64-to-32")
list(APPEND compile_options "-Wno-float-conversion")
list(APPEND compile_options "-Wno-implicit-float-conversion")
list(APPEND compile_options "-Wno-extra-semi")
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to just start submitting these fixes first, then revisit this PR. Here's an example for -Wextra-semi:
#123783

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh ok, I can do that, let me raise the ones for the other issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "simple" warning fixes that could be done are in: #124036.

All the other warnings seem to need fixes in multiple files and aren't straight forward to enable/trigger (i.e. to solve unused parameter, I need to enable Wextra which ends up raising a few other issues as well, like "omitting parameter name is a C23 extension").

In that case, IMO it would be a good idea to merge this PR after #124036, and then in each subsequent PR, work on removing the -Wno-* (that are disabled in this PR) one by one in subsequent PRs. This way, each subsequent PR is clearly delineated in terms of "what" to fix.

Copy link
Member

Choose a reason for hiding this comment

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

Sure. I don't mind if you need to enable -Wextra and add a few more -Wno-* options in the same PR, then work towards eliminating those new -Wno-* flags.



list(APPEND compile_options "-Wno-c99-extensions")
list(APPEND compile_options "-Wno-gnu-imaginary-constant")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nickdesaulniers

Should we also remove these no-* warnings? I didn't notice them earlier, but while I do the other PRs, I can take a look at enabling these as well?

Copy link
Member

Choose a reason for hiding this comment

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

Not in this PR. I'm not familiar with these two diagnostics in particular, so I'm not certain why they were explicitly disabled.


endif()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no else() block now, Should we do a message(WARN ...) to say, "Most warnings are enabled on *Clang$. Prefer using that compiler ..." ?

Copy link
Member

Choose a reason for hiding this comment

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

It's fine; today we only support gcc and clang. If someone cares to support a third compiler for llvm-libc, we'd like to know about it.


list(APPEND compile_options "-Wno-pedantic")
# if(NOT CMAKE_COMPILER_IS_GNUCXX)
# list(APPEND compile_options "-Wnewline-eof")
# list(APPEND compile_options "-Wnonportable-system-include-path")
# list(APPEND compile_options "-Wstrict-prototypes")
# list(APPEND compile_options "-Wthread-safety")
# list(APPEND compile_options "-Wglobal-constructors")
# endif()

endif()
set(${output_var} ${compile_options} PARENT_SCOPE)
endfunction()
Expand Down
Loading