LegalizeAdulthood added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:41
+bool isInNoNamespace(const FunctionDecl *FD) {
+ const DeclContext *DC = FD->getDeclContext();
----------------
`isInGlobalNamespace` might be a better name?
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:126-129
+ if (LangOpts.CPlusPlus17)
return false;
return true;
----------------
`return LangOpts.CPlusPlus17 == 0;`
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:142-149
Finder->addMatcher(
callExpr(callee(SignalFunction), hasArgument(1, HandlerExpr))
.bind("register_call"),
this);
+ Finder->addMatcher(
+ callExpr(callee(SignalFunction), hasArgument(1, HandlerLambda))
+ .bind("register_call"),
----------------
Is there any utility/performance to be gained by combining these into a single
matcher?
e.g.
```
callExpr(callee(SignalFunction), anyOf(hasArgument(1, HandlerExpr),
hasArgument(1, HandlerLambda)))
```
(not sure if that is syntactically valid, but you get my point)
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:207-215
+ })) {
+ // Found problems in this function, skip functions called from it.
+ Itr.skipChildren();
+ } else {
+ ++Itr;
+ }
+ } else {
----------------
Style guide says no braces on single statement blocks
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:247
+ if (!getLangOpts().CPlusPlus17 && getLangOpts().CPlusPlus)
+ return checkFunctionCPP14(FD, CallOrRef, ChainReporter);
----------------
How can the first expression here ever be false when
we rejected C++17 in the `isLanguageVersionSupported`
override?
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:275
+ const auto *FoundS = Match.getNodeAs<Stmt>("stmt");
+ StringRef Name = FoundS->getStmtClassName();
+ if (Name.startswith("CXX")) {
----------------
Hrm, I was curious what `getStmtClassName()` does, but there doesn't seem to be
any doxygen on it or the underlying data structure it accesses. Drilling into
it with the IDE seems to say that it's a bunch of enums and corresponding names
for each statement kind?
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:323-325
+ if (SkipPathEnd) {
+ SkipPathEnd = false;
+ } else {
----------------
drop braces on 'then' block per style guide
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:334-338
+ if (!SkipPathEnd) {
+ diag(HandlerRef->getBeginLoc(),
+ "function %0 registered here as signal handler", DiagnosticIDs::Note)
+ << cast<FunctionDecl>(Caller->getDecl());
+ }
----------------
drop braces per style guide
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118996/new/
https://reviews.llvm.org/D118996
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
