=?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com>,
=?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/112...@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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to