balazske marked an inline comment as done. balazske added inline comments.
================ Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:553-554 + CheckerOptions<[ + CmdLineOption<Boolean, + "AllowErrnoReadOutsideConditionExpressions", + "Allow read of undefined value from errno outside of conditions", ---------------- steakhal wrote: > Well, now I see why you marked this `Hide` previously. > This is a //modeling// checker, thus it won't appear in the documentation nor > in the `-analyzer-checker-help` etc. > > Interestingly, other checkers mark some options `Hide` and other times > `DontHide`. > E.g. `DynamicMemoryModeling.Optimistic` is //displayed//, but the > `DynamicMemoryModeling.AddNoOwnershipChangeNotes` is //hidden//. > Whatever. It looks good now, `ErrnoChecker` is not a modeling checker and the option should be accessible for users. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:47 + /// Indicates if a read (load) of \c errno is allowed in a non-condition part + /// of \c if, \c switch, loop and conditional statements when the errno + /// value may be undefined. ---------------- steakhal wrote: > You prefixed the previous two keywords with the `\c` marker. Why is it > missing from the `switch`? Is it really missing? (I do not check Doxygen documentation, probably the new documentation appears in the online pages after the commit.) ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:100 + case Expr::BinaryConditionalOperatorClass: + CondFound = (S == cast<BinaryConditionalOperator>(ParentS)->getCommon()); + break; ---------------- steakhal wrote: > Oh, so the test added for this actually uncovered a bug? Yes: Even if it looks natural that `getCond` can be used the same way as at normal `ConditionalOperator` this is not true: It returns something other (casted or converted) than the root condition expressions. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:145 + + if (IsLoad) { + switch (EState) { ---------------- steakhal wrote: > Might be more readable to early return instead. There is a `else` part. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:205 + if (CallF->isExternC() && CallF->isGlobal() && + C.getSourceManager().isInSystemHeader(CallF->getLocation()) && + !isErrno(CallF)) { ---------------- steakhal wrote: > Does this refer to the callsite or to the Decl location? > I think the former, which would be a bug I think. `CallF` is a `FunctionDecl` that should be the original (first) declaration of the function. 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