[clang] [clang-tools-extra] [analyzer][clang-tidy][NFC] Clean up eagerly-assume handling (PR #112209)

[via cfe-commits]

=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.nagy at ericsson.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/112209 at github.com>


================
@@ -3742,23 +3742,20 @@ void ExprEngine::evalLocation(ExplodedNodeSet &Dst,
   BldrTop.addNodes(Tmp);
 }
 
-std::pair<const ProgramPointTag *, const ProgramPointTag*>
-ExprEngine::geteagerlyAssumeBinOpBifurcationTags() {
-  static SimpleProgramPointTag
-         eagerlyAssumeBinOpBifurcationTrue(TagProviderName,
-                                           "Eagerly Assume True"),
-         eagerlyAssumeBinOpBifurcationFalse(TagProviderName,
-                                            "Eagerly Assume False");
-  return std::make_pair(&eagerlyAssumeBinOpBifurcationTrue,
-                        &eagerlyAssumeBinOpBifurcationFalse);
+std::pair<const ProgramPointTag *, const ProgramPointTag *>
+ExprEngine::getEagerlyAssumeBifurcationTags() {
+  static SimpleProgramPointTag TrueTag(TagProviderName, "Eagerly Assume True"),
+      FalseTag(TagProviderName, "Eagerly Assume False");
+
+  return std::make_pair(&TrueTag, &FalseTag);
----------------
isuckatcs wrote:

After a quick look at the codebase, the pattern seems to be that we create these tags at the point where they are needed as `static` local variables. To not break this pattern, we should either change all of them to be class members, or leave all of them as they are.

Also by being local variables, their visibility is limited to the point where they are needed. I don't think it makes sense to give them class-wide visibility. Also when one reads through the codebase, most of the time they can immediately see what the tag is, while as a class member they would need to first find the definition (i.e.: 1 more click, or pressing 1 more button). The latter reasoning can also be applicable for debugging.

Another thing to keep in mind is that we have some additional logic like the one in `ConditionBRVisitor::VisitNodeImpl()`, which take advantage of these tags, so removing the tags is not an NFC and we might end up rewriting other logic too.

Fun fact: `ProgramPoint` comparison also takes advantage of the tags and IIRC removing the tag in [`a47ec1b`](https://github.com/llvm/llvm-project/commit/a47ec1b79797c8c48c44f9ddf36fb82f8d87229d) might lead to an infinite loop in core engine, when 2 epsilon points follow each other, but it's been a while, so I don't remember it exactly.

Tldr, I would keep the tags as they are for now.

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