balazske added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:169
"bugprone-terminating-continue");
+ CheckFactories.registerCheck<ThreadCanceltypeAsynchronousCheck>(
+ "bugprone-thread-canceltype-asynchronous");
----------------
aaron.ballman wrote:
> I think this check is likely better surfaced in the `concurrency` module.
> WDYT?
It is really better in ``concurrency``, I was not aware of that module (it
contains only one check).
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/ThreadCanceltypeAsynchronousCheck.cpp:29
+
+static Preprocessor *PP;
+
----------------
aaron.ballman wrote:
> Any particular reason to use a global variable here rather than making it a
> member of `ThreadCanceltypeAsynchronousCheck`?
Member is better, I just copied the code from another file (that was added not
long ago) and assumed that it is correct even if it looks not good.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/ThreadCanceltypeAsynchronousCheck.cpp:38
+ };
+ const auto TryExpandAsInteger =
+ [](Preprocessor::macro_iterator It) -> Optional<unsigned> {
----------------
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).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96719/new/
https://reviews.llvm.org/D96719
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
