aaron.ballman added a comment. One thing that's not clear to me in the patch is what the behavior should be when the user specifies that they want the POSIX set but they're compiling for a non-POSIX system like Windows. Do you have thoughts on that situation?
================ Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:215 +// https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03 +// This list is not the same that is shown in the CERT page. +llvm::StringSet<> SignalHandlerCheck::POSIXConformingFunctions{ ---------------- You should also mention here that the POSIX list is not a strict superset of the minimal list. ================ Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h:25 public: + enum class AsyncSafeFunctionSetType { Minimal, POSIX }; + ---------------- I don't think this needs to be public? ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst:29-30 + <https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03>`_ + for more information). This is not an extension of the minimal set (``quick_exit`` + is not included). Default is ``POSIX``. ---------------- Huh, that's really strange that POSIX leaves `quick_exit` off the list. I'm going to reach out to someone from the Austin Group to see if that's intentional or not. ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/system-header-posix-api.h:1 -//===--- signal.h - Stub header for tests -----------------------*- C++ -*-===// +//===--- system-header-posix-api.h - Stub header for tests ------*- C++ -*-===// // ---------------- I think we should strive to replicate the system headers rather than fake up a system header (these headers can be used by more than one check). So I think we want to keep signal.h and stdlib.h, and should include the POSIX-specific functionality with a macro. This also helps us test the behavior on systems like Windows which are not POSIX systems. ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c:4 +// RUN: [{key: bugprone-signal-handler.AsyncSafeFunctionSet, value: "POSIX"}]}' \ +// RUN: -- -isystem %S/Inputs/Headers + ---------------- I'd like to make sure we have a test for this that explicitly runs on Windows. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90851/new/ https://reviews.llvm.org/D90851 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
