[PATCH] D74973: [analyzer] StdLibraryFunctionsChecker refactor w/ inheritance

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> Is really more kind of constraint needed than range constraint?

Yes, there are other constraints I am planning to implement:

- Size requirements E.g.: asctime_s(char *buf, rsize_t bufsz, const struct tm *time_ptr); `buf` size must be at least `bufsz`.
- Not-null
- Not-uninitalized
- Not-tainted

> A non-null can be represented as range constraint too.

Actually, to implement that we should have a branch in all `ValueRange::apply*` functions that handles `Loc` SVals. Unfortunately, a pointer cannot be handled as `NonLoc`, and the current Range based implementation handles `NonLoc`s only.

> The compare constraint is used only for the return value for which a special `ReturnConstraint` can be used to handle the return value not like a normal argument (and then the `Ret` special value is not needed).

The Compare constraint is already forced into a Range "concept" whereas it has nothing to do with ranges. By handling compare constraints separately, we attach a single responsibility to each constraint class, instead of having a monolithic god constraint class. Take a look at this coerced data representation that we have today in ValueRange:

  BinaryOperator::Opcode getOpcode() const {
    assert(Kind == ComparesToArgument);
    assert(Args.size() == 1);
    BinaryOperator::Opcode Op =
        static_cast<BinaryOperator::Opcode>(Args[0].first);
    assert(BinaryOperator::isComparisonOp(Op) &&
           "Only comparison ops are supported for ComparesToArgument");
    return Op;
  }

Subclasses are a good way to get rid of this not-so-intuitive structure and assertions.

> Or are there sometimes relations between arguments of a function?

I can't recall now a direct relation by heart.
But there could be more subtle indirect relations, see the size requirements above. (Thought I'd rather implement that in a separate constraint class.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74973/new/

https://reviews.llvm.org/D74973