[PATCH] D74473: [analyzer] StdLibraryFunctionsChecker: Use platform dependent EOF and UCharMax

[Kristóf Umann via Phabricator via cfe-commits]

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