aaron.ballman added inline comments.

================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h:25
 public:
+  enum class AsyncSafeFunctionSetType { Minimal, POSIX };
+
----------------
balazske wrote:
> aaron.ballman wrote:
> > I don't think this needs to be public?
> The `getEnumMapping` function uses it that is external to the class.
Ah, I hadn't caught that, thanks!


================
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``.
----------------
aaron.ballman wrote:
> 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.
I didn't need to reach out to them because I realized the latest POSIX spec is 
still based on C99 and they've not released the updated POSIX spec yet.

I think the POSIX functions should be a superset of the C functions.


================
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++ 
-*-===//
 //
----------------
balazske wrote:
> aaron.ballman wrote:
> > 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.
> My concern was to add these many small header files with just some functions 
> in them. For `accept` more than one header is needed if we want to exactly 
> replicate the system files. And data types like `size_t` should have a common 
> header too. So I decided to have one header that contains all system 
> functions and data types. This can be used by multiple tests and extended as 
> needed.
I don't think we're too worried about having a bunch of small test headers 
around -- I think it's more important the headers used to check system header 
behavior be understandable as to what you're getting from them. For instance, 
there are clang-tidy checks for llvm-libc that may have very different needs 
from what you're doing here.

What's more, these changes break existing tests -- I don't see any companion 
changes to fix those up.


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

Reply via email to