================
@@ -88,10 +88,12 @@ class ModelDumper {
void dump(Value &V) {
JOS.attribute("value_id", llvm::to_string(&V));
- if (!Visited.insert(&V).second)
- return;
-
JOS.attribute("kind", debugString(V.getKind()));
+ if (!Visited.insert(&V).second) {
+ JOS.attribute("[in_cycle]", " ");
+ return;
+ }
+ auto EraseVisited = llvm::make_scope_exit([&] { Visited.erase(&V); });
----------------
martinboehme wrote:
Sorry, this review has gone very stale because I had other things that grabbed
my attention at the time and then forgot to get back to this PR.
Responding only to this comment first as it's about what we want the behavior
to be (rather than the details of how we implement it).
> But I'm not sure the distinction is worth the code: the idea "we've seen this
> node before, and won't print its details again" applies whether the reason is
> a cycle or just multiple paths to a node, and they both benefit from some
> explicit hint.
Well, part of the motivation of this PR is that I _do_ also want to change this
existing behavior. Here's how repeated values are displayed today:
I see this very case pretty regularly; it was very confusing the first time
(the "undefined" made me think I had a bug), and I still do a double-take when
I see it now.
Even if this was displayed better (e.g. as an `AtomicBool` value with a
"previously dumped" annotation), that still requires me to go looking for the
previous value. In this case, that's easy, because the value is directly above,
but I still need to compare the hex address to be sure.
Why do this work if I can have the computer do it for me?
I assume your concern is that we could have data structures with lots and lots
of repeated values, and this would bloat the JSON? Do we actually know that
this is a problem though?
https://github.com/llvm/llvm-project/pull/66887
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
