[PATCH] D155857: [clang] fix nonnull warnings during build

fzakaria added a comment. Changed to the if condition. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155857/new/ https://reviews.llvm.org/D155857 ___ cfe-commits mailing list cfe-commits@lists.llvm.org h

[PATCH] D155857: [clang] fix nonnull warnings during build

fzakaria updated this revision to Diff 543244. fzakaria added a comment. The previous CMake variable wasn't working. Changed it to the pattern I see in the codebase. Confirmed it via `ninja` command and grepping. ❯ rg --color=always no-nonnull build.ninja | head -n 1 FLAGS = -fPIC -fno-sem

[PATCH] D155857: [clang] fix nonnull warnings during build

fzakaria added a comment. Ugh you are right. So weird -- This works: if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU") append("-Wno-nonnull" CMAKE_CXX_FLAGS) endif() I read the CMake documentation and it looked like the variable I used should have worked. https://cmake.org/cmake/help/latest/va

[PATCH] D155857: [clang] fix nonnull warnings during build

MaskRay added a comment. In D155857#4525421 , @fzakaria wrote: > Address feedback from maskray@ > > - imperative comment > - switch to CMAKE_GNU_COMPILER_ID for variable Does `append_if(CMAKE_GNU_COMPILER_ID "-Wno-nonnull" CMAKE_CXX_FLAGS)` work? `build

[PATCH] D155857: [clang] fix nonnull warnings during build

fzakaria marked an inline comment as done. fzakaria added a comment. @MaskRay thanks for the review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155857/new/ https://reviews.llvm.org/D155857 ___ cfe-com

[PATCH] D155857: [clang] fix nonnull warnings during build

fzakaria updated this revision to Diff 543238. fzakaria added a comment. Address feedback from maskray@ - imperative comment - switch to CMAKE_GNU_COMPILER_ID for variable Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155857/new/ https://reviews.l

[PATCH] D155857: [clang] fix nonnull warnings during build

MaskRay added inline comments. Comment at: llvm/cmake/modules/HandleLLVMOptions.cmake:773 + # Disabling this GCC warning as it is emitting a lot of false positives + append_if(CMAKE_COMPILER_IS_GNUCXX "-Wno-nonnull" CMAKE_CXX_FLAGS) Use an imperative sentence

[PATCH] D155857: [clang] fix nonnull warnings during build

fzakaria added a comment. @rsmith -- sounds good. Please commit on my behalf when you are ready as I don't have commit access. Thank you for the review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155857/new/ https://reviews.llvm.org/D155857 __

[PATCH] D155857: [clang] fix nonnull warnings during build

fzakaria updated this revision to Diff 542663. fzakaria added a comment. Added comment to explain why we are disabling Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155857/new/ https://reviews.llvm.org/D155857 Files: llvm/cmake/modules/HandleLLV

[PATCH] D155857: [clang] fix nonnull warnings during build

rsmith accepted this revision as: rsmith. rsmith added a comment. This revision is now accepted and ready to land. Seems reasonable to me. Given that this affects all of LLVM I'd like to wait a day or so to see if anyone has concerns. Comment at: llvm/cmake/modules/HandleLLVMO

[PATCH] D155857: [clang] fix nonnull warnings during build

MaskRay added a comment. Perhaps this can be placed beside `llvm/cmake/config-ix.cmake` `-Wmaybe-uninitialized`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155857/new/ https://reviews.llvm.org/D155857 __

[PATCH] D155857: [clang] fix nonnull warnings during build

MaskRay added a comment. Perhaps `-Wno-nonnull` should be GCC specific (i.e. not apply to Clang). This option will disable certain warnings related to Clang `_Nonnull` (seems unused in llvm-project) and `__attribute__((nonnull))` (used in a few places). Repository: rG LLVM Github Monorepo C

[PATCH] D155857: [clang] fix nonnull warnings during build

fzakaria added a comment. @rsmith and @MaskRay thank you for the feedback. I think this is more in line with what we discussed on the #LLVM chat Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155857/new/ https:/

[PATCH] D155857: [clang] fix nonnull warnings during build

fzakaria updated this revision to Diff 542614. fzakaria added a comment. Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Updated instead to disable it in CMake Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155857/new/ https:/

[PATCH] D155857: [clang] fix nonnull warnings during build

MaskRay added a comment. I agree with the analysis. My understanding is that we don't worsen the code quality for Clang to work around a GCC warning. And generally we care strongly about Clang warnings, but less about GCC warnings, especially in `.cpp` files. However, this is in a header and per

[PATCH] D155857: [clang] fix nonnull warnings during build

rsmith requested changes to this revision. rsmith added a comment. This revision now requires changes to proceed. This branch doesn't look necessary or correct to me. We should never call this function on a lazy pointer in offset mode unless we have an external source that produced the offset, a

[PATCH] D155857: [clang] fix nonnull warnings during build

fzakaria created this revision. Herald added a project: All. fzakaria requested review of this revision. Herald added subscribers: cfe-commits, wangpc. Herald added a project: clang. I noticed during the build that GCC would emit a ton of nonnull warnings. Example: /usr/local/google/home/fmza