[PATCH] D96719: [clang-tidy] Add new check 'bugprone-thread-canceltype-asynchronous' and alias 'cert-pos47-c'.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ThreadCanceltypeAsynchronousCheck.cpp:38
+    };
+    const auto TryExpandAsInteger =
+        [](Preprocessor::macro_iterator It) -> Optional<unsigned> {
----------------
balazske wrote:
> aaron.ballman wrote:
> > balazske wrote:
> > > aaron.ballman wrote:
> > > > Do we know that POSIX implementations always use a simple integer here, or do we have to worry about code like:
> > > > ```
> > > > #define PTHREAD_CANCEL_ASYNCHRONOUS (1 << 0)
> > > > ```
> > > > or
> > > > ```
> > > > #define PTHREAD_CANCEL_ASYNCHRONOUS SOME_OTHER_MACRO
> > > > ```
> > > Theoretically it is possible that the macro is defined not as a simple number ([[https://pubs.opengroup.org/onlinepubs/9699919799/ | at this page ]] see "Symbolic Constant"). But I am not sure if it is possible to get the value from the preprocessor for any constant expression.
> > > 
> > > There is a similar function `tryExpandAsInteger` already in clang (CheckerHelpers.cpp) that can be reused here, probably it retrieves the macro definition better. The function could be put into one of the "utils" files, maybe **LexerUtils.h**, it is already needed at 2 places (bad-signal-to-kill-thread and here).
> > > 
> > Having a single place to get this information would be an improvement, but not necessary for this patch. If you don't know of a POSIX implementation that uses something other than a positive integer literal for the expansion, I think the current code is fine.
> I found that it is good to rely on `isExpandedFromMacro` AST matcher instead of this whole `tryExpandAsInteger` function. The current solution can find only cases where the macro is defined as an integer literal. If `isExpandedFromMacro` is used it will work at least in all cases regardless of the value of the macro. It will not work if a variable is used to pass the value, but this does not work in the current code either.
That's a good point! I don't think it's important to handle the case where a variable or an integer literal is used as the function call argument; we can tackle those cases if they crop up in practice and there's a need.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96719/new/

https://reviews.llvm.org/D96719