balazske added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/ThreadCanceltypeAsynchronousCheck.cpp:38
+ };
+ const auto TryExpandAsInteger =
+ [](Preprocessor::macro_iterator It) -> Optional<unsigned> {
----------------
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.
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
