steakhal added a comment.
It looks great.
Let me check the test coverage and the generated docs page it everything looks
great.
================
Comment at: clang/docs/analyzer/checkers.rst:2538
+may change value of ``errno`` if the call does not fail.
+Therefore ``errno`` should only be used if it is known from the return value
+of a function that the call has failed.
----------------
================
Comment at: clang/docs/analyzer/checkers.rst:2540
+of a function that the call has failed.
+There are exceptions to this rule but the affected functions are not
+yet supported by the checker.
----------------
Please mention at least one of those functions.
================
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",
----------------
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.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:145
+
+ if (IsLoad) {
+ switch (EState) {
----------------
Might be more readable to early return instead.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:205
+ if (CallF->isExternC() && CallF->isGlobal() &&
+ C.getSourceManager().isInSystemHeader(CallF->getLocation()) &&
+ !isErrno(CallF)) {
----------------
Does this refer to the callsite or to the Decl location?
I think the former, which would be a bug I think.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:46
+
+ /// Indicates if a read (load) of 'errno' is allowed in a non-conditional
+ /// statement.
----------------
martong wrote:
> Or `in a statement without a condition` ?
I don't really understand this comment. What is a //non-condition part of a
statement//?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:280
+ BR.markNotInteresting(ErrnoR);
+ return Message.c_str();
+ }
----------------
balazske wrote:
> steakhal wrote:
> >
> I can not omit `c_str`, there is a compile error probably related to lambda
> return type.
Yes, you will need to specify explicitly the return type of the lambda: `->
std::string`.
================
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))
----------------
balazske wrote:
> 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.
I was thinking about this:
```lang=C++
bool isErrno(const Decl *D) {
if (const auto *VD = dyn_cast_or_null<VarDecl>(D))
if (const IdentifierInfo *II = VD->getIdentifier())
return II->getName() == ErrnoVarName;
if (const auto *FD = dyn_cast_or_null<FunctionDecl>(D))
if (const IdentifierInfo *II = FD->getIdentifier())
return llvm::is_contained(ErrnoLocationFuncNames, II->getName());
return false;
}
```
This way both checks would follow the same pattern, symmetry.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h:34
+ /// value.
+ Errno_MustNotBeChecked = 2
+};
----------------
balazske wrote:
> 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_`?
But you always refer to it by it's qualified name:
`errno_modeling::Errno_Irrelevant`.
In which case this prefix is just noise IMO. That's what I'm about.
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
