[all-commits] [llvm/llvm-project] bde571: [analyzer][NFC] Rework SVal kind representation (#...

[Balazs Benics via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: bde5717d4638c27614d9d4a2e53df27087a69841
      https://github.com/llvm/llvm-project/commit/bde5717d4638c27614d9d4a2e53df27087a69841
  Author: Balazs Benics <benicsbalazs at gmail.com>
  Date:   2023-11-04 (Sat, 04 Nov 2023)

  Changed paths:
    M clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
    M clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValVisitor.h
    M clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.def
    M clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
    M clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
    M clang/lib/StaticAnalyzer/Core/SVals.cpp
    M clang/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp
    M clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
    M clang/lib/StaticAnalyzer/Core/Store.cpp

  Log Message:
  -----------
  [analyzer][NFC] Rework SVal kind representation (#71039)

The goal of this patch is to refine how the `SVal` base and sub-kinds
are represented by forming one unified enum describing the possible
SVals. This means that the `unsigned SVal::Kind` and the attached
bit-packing semantics would be replaced by a single unified enum. This
is more conventional and leads to a better debugging experience by
default. This eases the need of using debug pretty-printers, or the use
of runtime functions doing the printing for us like we do today by
calling `Val.dump()` whenever we inspect the values.

Previously, the first 2 bits of the `unsigned SVal::Kind` discriminated
the following quartet: `UndefinedVal`, `UnknownVal`, `Loc`, or `NonLoc`.
The rest of the upper bits represented the sub-kind, where the value
represented the index among only the `Loc`s or `NonLoc`s, effectively
attaching 2 meanings of the upper bits depending on the base-kind. We
don't need to pack these bits, as we have plenty even if we would use
just a plan-old `unsigned char`.

Consequently, in this patch, I propose to lay out all the (non-abstract)
`SVal` kinds into a single enum, along with some metadata (`BEGIN_Loc`,
`END_Loc`, `BEGIN_NonLoc`, `END_NonLoc`) artificial enum values, similar
how we do with the `MemRegions`.

Note that in the unified `SVal::Kind` enum, to differentiate
`nonloc::ConcreteInt` from `loc::ConcreteInt`, I had to prefix them with
`Loc` and `NonLoc` to resolve this ambiguity.
This should not surface in general, because I'm replacing the
`nonloc::Kind` enum items with `inline constexpr` global constants to
mimic the original behavior - and offer nicer spelling to these enum
values.

Some `SVal` constructors were not marked explicit, which I now mark as
such to follow best practices, and marked others as `/*implicit*/` to
clarify the intent.
During refactoring, I also found at least one function not marked
`LLVM_ATTRIBUTE_RETURNS_NONNULL`, so I did that.

The `TypeRetrievingVisitor` visitor had some accidental dead code,
namely: `VisitNonLocConcreteInt` and `VisitLocConcreteInt`.

Previously, the `SValVisitor` expected visit handlers of
`VisitNonLocXXXXX(nonloc::XXXXX)` and `VisitLocXXXXX(loc::XXXXX)`, where
I felt that envoding `NonLoc` and `Loc` in the name is not necessary as
the type of the parameter would select the right overload anyways, so I
simplified the naming of those visit functions.

The rest of the diff is a lot of times just formatting, because
`getKind()` by nature, frequently appears in switches, which means that
the whole switch gets automatically reformatted. I could probably undo
the formatting, but I didn't want to deviate from the rule unless
explicitly requested.