[clang] [llvm] [NFC][analyzer] Extract bounds checking library (PR #202372)

[Donát Nagy via cfe-commits]

================
@@ -0,0 +1,211 @@
+//===- BoundsChecking.h - Bounds checking related APIs ----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file defines APIs for performing a bounds check (i.e. comparing a
+//  symbolic Offset value to zero and a symbolic Extent value) and composing
+//  descriptions that explain its results.
+//
+//  This is intended as a replacement for `ProgramState::assumeInBound` to
+//  avoid its incorrect logic and compensate for deficiencies of other parts of
+//  the analyzer.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_BOUNDSCHECKING_H
+#define LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_BOUNDSCHECKING_H
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "llvm/Support/FormatVariadic.h"
+#include <optional>
+
+namespace clang {
+namespace ento {
+
+/// If `E` is an array subscript expression with a base that is "clean" (= not
+/// modified by pointer arithmetic = the beginning of a memory region), return
+/// it as a pointer to ArraySubscriptExpr; otherwise return nullptr.
+/// This helper function is used by two separate heuristics that are only valid
+/// in these "clean" cases.
+const ArraySubscriptExpr *getAsCleanArraySubscriptExpr(const Expr *E,
----------------
NagyDonat wrote:

> Until it is moved, it would be great if this was not at the very beginning of the header. The main readers of this file are the users of these APIs. Let's try to not waste their time with info that they are not interested in. And if we absolutely need to have some implementation details here, let's try to move that towards the end.

If I move this to the end of the header file, I would need to move the definition of the trivial method `forExpr` (which uses it) to the `.cpp` file, which I don't like.

Also, this function will go away very soon (my next PR after this one will either delete it or at least move to `ArrayBoundChecker.cpp`) so please don't spend more time on it.

---------

> I also wonder if we can come up with a better term than `Clean`. Could be like, `getZeroBasedArraySubscriptExpr` or `getArraySubscriptExprFromBeginning`.

As this PR moves this function to a different file, I would strongly prefer to preserve its old name to provide continuity. I will probably eliminate this function in the non-NFC follow-up PR  that eliminates `SizeUnit::forExpr` (one of the two call sites; I will inline this at the other call site), which will make its name irrelevant.

By the way, for me the two suggested names are both confusing: as I see `ZeroBased` I'm immediately sidetracked by thoughts like "this is C/C++, all array indices are zero-based, why is this highlighted?", while  `getArraySubscriptExprFromBeginning` is IMO as unclear as the current name.

https://github.com/llvm/llvm-project/pull/202372