Szelethus requested changes to this revision.
Szelethus added inline comments.
This revision now requires changes to proceed.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h:67-69
+/// Try to parse the value of a defined preprocessor macro. We can only parse
+/// simple expressions that consist of an optional minus sign token and then a
+/// token for an integer. If we cannot parse the value then None is returned.
----------------
While I agree that we shouldn't have to reinvent the `Preprocessor` every time
we need something macro related, I doubt that this will catch anything. I
checker my system's standard library, and this is what I found:
```lang=c
#ifndef EOF
# define EOF (-1)
#endif
```
Lets go just one step further and cover the probably majority of the cases
where the definition is in parentheses.
================
Comment at:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:512-517
+ const auto EOFv = [&C]() -> RangeInt {
+ if (const llvm::Optional<int> OptInt =
+ TryExpandAsInteger("EOF", C.getPreprocessor()))
+ return *OptInt;
+ return -1;
+ }();
----------------
Hah, would've never thought of doing this with a lambda. Me likey.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:571
+ .Case(
+ {ReturnValueCondition(WithinRange, {{EOFv, EOFv}, {0,
UCharMax}})});
};
----------------
Huh, what's happening here exactly? Doesn't `Range` take 2 `RangeInt`s as ctor
arguments? What does `{EOFv, EOFv}, {0, UCharMax}` mean in this context?
================
Comment at: clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp:114-117
+ const auto MacroIt = llvm::find_if(
+ PP.macros(), [&](const auto &K) { return K.first->getName() == Macro; });
+ if (MacroIt == PP.macro_end())
+ return llvm::None;
----------------
This seems a bit clunky even for the `Preprocessor` -- how about
```lang=c++
const auto *MacroII = PP.getIdentifierInfo(Macro);
if (!MacroII)
return;
const MacroInfo *MI = PP.getMacroInfo(MacroII);
assert(MI);
```
================
Comment at: clang/test/Analysis/std-c-library-functions-eof.c:24
+ if (y < 0) {
+ clang_analyzer_eval(y == EOF); // expected-warning{{TRUE}}
+ }
----------------
So the test is about checking whether the analyzer correctly assigned `-2` to
`y`, right? Then let's check that too.
```lang=c++
clang_analyzer_eval(y == 2);
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74473/new/
https://reviews.llvm.org/D74473
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
