balazske added inline comments.
================ Comment at: clang/docs/analyzer/checkers.rst:2538-2539 +yet supported by the checker. +The return values for the failure cases are documented in the standard Linux man +pages of the functions. + ---------------- steakhal wrote: > Maybe place a crossref for this. > > Also, am I right that there is a CERT rule about this issue? > We should probably mention that, while placing a crossref too. Probably there is not a preferred or "standard" site for man page lookup and it can be done from command line too. But I can include a POSIX link https://pubs.opengroup.org/onlinepubs/9699919799/. ================ Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:550 +def ErrnoChecker : Checker<"Errno">, + HelpText<"Check not recommended uses of 'errno'">, + Dependencies<[ErrnoModeling]>, ---------------- steakhal wrote: > antipatterns? I like "improper use of `errno`" better, it is similar to `HelpText` of other checkers. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:101-102 + break; + default: + break; + } ---------------- steakhal wrote: > We should also handle `BinaryOperator` for conditional operators (EQ, NE, LT, > LE, ...). Or at least leave a FIXME about this. > Have a test demonstrating the behavior. The idea is here that this search does not stop at binary operators but goes up the AST until a condition is found. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:261 + if (unsigned EState = State->get<ErrnoState>()) + return static_cast<ErrnoCheckState>(EState); + return Errno_Irrelevant; ---------------- steakhal wrote: > I believe you no longer need to cast enums to numbers and back for enums and > other types. > `get<ErrnoState>()` should return the right type. > See D126801. How to handle if no value exists? ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:280 + BR.markNotInteresting(ErrnoR); + return Message.c_str(); + } ---------------- steakhal wrote: > I can not omit `c_str`, there is a compile error probably related to lambda return type. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:267-268 + if (const auto *VD = dyn_cast_or_null<VarDecl>(D)) + return VD->getIdentifier() && + VD->getIdentifier()->getName() == ErrnoVarName; + if (const auto *FD = dyn_cast_or_null<FunctionDecl>(D)) ---------------- steakhal wrote: > It looks odd that in the very next `if` block you have `II` and here you > don't. I do not get what you mean here. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h:34 + /// value. + Errno_MustNotBeChecked = 2 +}; ---------------- steakhal wrote: > You don't need to prefix these with `Errno_`. It's already contained by a > specific namespace. > https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly > > Unless the enumerators are defined in their own small namespace or inside a > > class, enumerators should have a prefix corresponding to the enum > > declaration name Without `Errno_` these sound too generic. Probably have only `Errno` prefix, not `Errno_`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122150/new/ https://reviews.llvm.org/D122150 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
