[PATCH] D115149: [analyzer][solver] Fix assertion on (NonLoc, Op, Loc) expressions

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:463
+  if (const Optional<Loc> RV = rhs.getAs<Loc>()) {
+    const auto IsCommutative = [](BinaryOperatorKind Op) {
+      return Op == BO_Mul || Op == BO_Add || Op == BO_And || Op == BO_Xor ||
----------------
steakhal wrote:
> ASDenysPetrov wrote:
> > IMO it should be in a family of `BinaryOperator::is..` methods.
> It isn't as easy as it might seem at first glance. I've considered doing it that way.
> In our context, we don't really deal with floating-point numbers, but in the generic implementation, we should have the type in addition to the OpKind. We don't need that complexity, so I sticked to inlining them here.
I see your concerns about floats but the operators don't stop to be commutative themselves. Yes, it may not work with some types but it will still be commutative. Leave the context for a user outside the function. It may bother you only a few operators from the set and you just check whether they belong to the set or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115149