balazske marked 2 inline comments as done.
balazske added inline comments.
================
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 {
----------------
LegalizeAdulthood wrote:
> Style guide says no braces on single statement blocks
Improved the code but I think if an `if` branch has braces then the `else`
should have too.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:247
+ if (!getLangOpts().CPlusPlus17 && getLangOpts().CPlusPlus)
+ return checkFunctionCPP14(FD, CallOrRef, ChainReporter);
----------------
LegalizeAdulthood wrote:
> How can the first expression here ever be false when
> we rejected C++17 in the `isLanguageVersionSupported`
> override?
I was thinking for the case when this check supports C++17 too. The change can
be added later, this makes current code more readable.
================
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")) {
----------------
LegalizeAdulthood wrote:
> 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?
It should return the statement class name, comes somehow from TableGen. Here if
the class name contains `CXX` it is recognized as a C++ statement. This check
is not fully accurate (for example `LambdaExpr` is missing) but there is likely
some other `CXX` statement around.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:323-325
+ if (SkipPathEnd) {
+ SkipPathEnd = false;
+ } else {
----------------
LegalizeAdulthood wrote:
> drop braces on 'then' block per style guide
I think that readability does not get better if the braces are removed here,
specially if only from one branch. The rules [[
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
| here ]] are not strict in this case.
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
