[NFC] Eliminate the majority of `dynamic_cast` usage in the codebase in favor of ICastable interface #4382

asl · 2024-01-30T19:52:28Z

This is a spin-off from #4377

asl · 2024-01-30T19:53:00Z

asl · 2024-01-30T23:25:10Z

frontends/p4/typeChecking/typeUnification.cpp

@@ -450,9 +450,8 @@ bool TypeUnification::unify(const BinaryConstraint *constraint) {
        }
        return constraint->reportError(constraints->getCurrentSubstitution());
    } else if (dest->is<IR::Type_Declaration>() && src->is<IR::Type_Declaration>()) {
-        bool canUnify = typeid(dest) == typeid(src) && dest->to<IR::Type_Declaration>()->name ==


This is a pre-existing p4c bug. typeid(dest) == typeid(src) is always true since both dest and src are of IR::Type* types and typeid of these expressions matches. Changing to typeid(*dest) == typeid(*src) would yield an error as it will consider generic substituted type and correspondent substitution to be separate types, when they are not. This change just removes always true condition. No test changes / failures.

I definitely agree this was always-true conjunct. I am just wondering if this is a bug that we are not checking that the "kinds" of types match, but I would say it is probably safe to compare names only. It doubt there can be two entities with the same name and one declaring different kind of thing then the other (e.g. extern vs. parser), as you already have an example where the types does not match correctly, I agree with just dropping the conjunct.

If we take the exiting testcase for this code, then checking the typeids yields to very confusing error message. As I noted – the ordinary type and the substituted generic type are different nodes, so the typeid check would fail. However, the diagnostics produced is confusing, IIRC – cannot unify "int" with "int" or something like this.

asl · 2024-01-30T23:27:49Z

ir/node.h

-    /// produces a message that is easier to debug.
+
+    // default checkedTo implementation for nodes: just fallback to generic ICastable method
+    template <typename T>


Note that static_type_name is defined only on concrete classes, not on interfaces. So, without this change use of checkedCastTo<T> where T is interface would produce compile error. As a result, one was able to use as<T>() with T being interface, but not checkedCastTo<T>().

Hmm, maybe we should have static_type_name generated also for interfaces. That is a separate task though.

Likely there was some rationale for this behavior. But maybe not. Though, I tend to agree that having a static type name everywhere would be be better.

Likely there was some rationale for this behavior.

I added the ICastable class at some point because I noticed we repeated this pattern a lot. At the time node_type_name and static_type_name were simply not required. There is no reason not to add them, but it will likely take some effort across the entire compiler.

@fruffy Sorry, I do not follow. What are you suggesting? Indeed we can get rid of this implementation, however we'll end with less friendly error message. Some tests checks this btw.

Just clarifying on

Likely there was some rationale for this behavior.

There was not a concrete reason why things were done this way. No need to get rid of the implementation.

Btw, as far as I can see static_type_name and its usage in checkedTo predates ICastable. It's just checkedTo is rarely used in the codebase for issue to trigger :)

Yes, there were a few nodes that implemented their own checkedTo and to but did not add the other functions. This class just unified things, the mangled typename was, well, "good enough".

vlstill

Thank you for this changes and for splitting them off, this is a great cleanup. I have just a few minor points and questions.

backends/ebpf/ebpfControl.cpp

backends/p4tools/common/core/abstract_execution_state.cpp

frontends/p4/reassociation.cpp

ir/ir-inline.h

ir/node.h

vlstill · 2024-01-31T08:17:42Z

ir/node.h

-    /// produces a message that is easier to debug.
+
+    // default checkedTo implementation for nodes: just fallback to generic ICastable method
+    template <typename T>


Hmm, maybe we should have static_type_name generated also for interfaces. That is a separate task though.

ir/node.h

lib/castable.h

vlstill · 2024-01-31T08:36:00Z

frontends/p4/typeChecking/typeUnification.cpp

@@ -450,9 +450,8 @@ bool TypeUnification::unify(const BinaryConstraint *constraint) {
        }
        return constraint->reportError(constraints->getCurrentSubstitution());
    } else if (dest->is<IR::Type_Declaration>() && src->is<IR::Type_Declaration>()) {
-        bool canUnify = typeid(dest) == typeid(src) && dest->to<IR::Type_Declaration>()->name ==


I definitely agree this was always-true conjunct. I am just wondering if this is a bug that we are not checking that the "kinds" of types match, but I would say it is probably safe to compare names only. It doubt there can be two entities with the same name and one declaring different kind of thing then the other (e.g. extern vs. parser), as you already have an example where the types does not match correctly, I agree with just dropping the conjunct.

asl · 2024-01-31T12:05:39Z

@vlstill Added suggestions from the review, please check

fruffy · 2024-01-31T15:52:52Z

lib/castable.h

@@ -32,12 +32,18 @@ class ICastable {
        return to<T>() != nullptr;
    }

-    /// Tries to convert the class to type T. Returns a nullptr if the cast fails.
+    /// Performs a checked cast. A BUG occurs if the cast fails.


std::bad_cast instead?

Let's leave it as-is. As I said, custom-rtti branch implements it via checkedCast. So we won't change the comment all the time.

Mostly asking because it might take a while longer until the other PR is merged. But the comment was wrong in the first place and this is an improvement...

BUG and std::bad_cast are close enough in behavior so it is fine.

fruffy · 2024-01-31T15:53:49Z

ir/node.h

-    /// produces a message that is easier to debug.
+
+    // default checkedTo implementation for nodes: just fallback to generic ICastable method
+    template <typename T>


Likely there was some rationale for this behavior.

I added the ICastable class at some point because I noticed we repeated this pattern a lot. At the time node_type_name and static_type_name were simply not required. There is no reason not to add them, but it will likely take some effort across the entire compiler.

…or of ICastable interface

Co-authored-by: Vladimír Štill <git@vstill.eu>

fruffy

I have no further comments from my side on this PR.

backends/p4tools/common/core/abstract_execution_state.cpp

ir/node.h

asl · 2024-02-01T17:32:45Z

@fruffy @vlstill Thanks for the reviews. I addressed the last comments. Will you please merge when you will have a chance? I will rebase custom-rtti branch afterwards.

Co-authored-by: Vladimír Štill <git@vstill.eu>

asl · 2024-02-01T21:09:01Z

@fruffy Looks like bazel is stuck, I cannot restart it sadly:

Received 987342403 of 999925315 (98.7%), 0.2 MBs/sec
Received 987342403 of 999925315 (98.7%), 0.2 MBs/sec
Received 987342403 of 999925315 (98.7%), 0.2 MBs/sec
Received 987342403 of 999925315 (98.7%), 0.2 MBs/sec
Received 987342403 of 999925315 (98.7%), 0.2 MBs/sec
Received 987342403 of 999925315 (98.7%), 0.2 MBs/sec
Received 987342403 of 999925315 (98.7%), 0.2 MBs/sec
Received 987342403 of 999925315 (98.7%), 0.2 MBs/sec
Received 987342403 of 999925315 (98.7%), 0.2 MBs/sec

asl · 2024-02-01T21:32:54Z

@fruffy Thanks for restarting! Could you please merge it for me? Auto-merge does not work since I do not have write access.

fruffy · 2024-02-01T21:57:21Z

@fruffy Thanks for restarting! Could you please merge it for me? Auto-merge does not work since I do not have write access.

If you plan on contributing more to p4c going forward we could make Kyle and you contributors with merge rights (https://github.com/orgs/p4lang/teams/p4c-committers).

asl · 2024-02-01T21:58:44Z

@fruffy Thanks for restarting! Could you please merge it for me? Auto-merge does not work since I do not have write access.

If you plan on contributing more to p4c going forward we could make Kyle and you contributors with merge rights (https://github.com/orgs/p4lang/teams/p4c-committers).

I do have other patches in my queue, yes. Just one step at a time :)

kfcripps · 2024-02-02T01:11:10Z

If you plan on contributing more to p4c going forward we could make Kyle and you contributors with merge rights (https://github.com/orgs/p4lang/teams/p4c-committers).

Yes, that will be great. As Anton mentioned earlier, we have already made some of these changes to our compiler over a year ago, but haven't been able to upstream them yet for a number of reasons. But moving forward, we should be able to contribute more, I think. :)

fruffy · 2024-02-02T17:22:25Z

If you plan on contributing more to p4c going forward we could make Kyle and you contributors with merge rights (https://github.com/orgs/p4lang/teams/p4c-committers).

Yes, that will be great. As Anton mentioned earlier, we have already made some of these changes to our compiler over a year ago, but haven't been able to upstream them yet for a number of reasons. But moving forward, we should be able to contribute more, I think. :)

Could you send me an email (on my website) with your email addresses and some context? I can then start the discussion.

asl force-pushed the icastable branch 3 times, most recently from 16be4f6 to 9d008e1 Compare January 30, 2024 22:53

asl commented Jan 30, 2024

View reviewed changes

vlstill added the run-sanitizer Use this tag to run a Clang+Sanitzers CI run. label Jan 31, 2024

vlstill reviewed Jan 31, 2024

View reviewed changes

asl force-pushed the icastable branch from fe0b2b6 to bdfea14 Compare January 31, 2024 12:05

asl requested a review from vlstill January 31, 2024 12:05

fruffy reviewed Jan 31, 2024

View reviewed changes

asl and others added 3 commits January 31, 2024 16:56

Eliminate the majority of dynamic_cast usage in the codebase in fav…

fee0cf6

…or of ICastable interface

Produce slightly better error message in case of failed cast

b1ff849

Apply suggestions from code review

2298ff1

Co-authored-by: Vladimír Štill <git@vstill.eu>

asl force-pushed the icastable branch from bdfea14 to 2298ff1 Compare February 1, 2024 00:56

fruffy approved these changes Feb 1, 2024

View reviewed changes

backends/p4tools/common/core/abstract_execution_state.cpp Outdated Show resolved Hide resolved

vlstill approved these changes Feb 1, 2024

View reviewed changes

ir/node.h Outdated Show resolved Hide resolved

fruffy enabled auto-merge (squash) February 1, 2024 17:36

auto-merge was automatically disabled February 1, 2024 18:00
Head branch was pushed to by a user without write access

asl force-pushed the icastable branch from 4bf8249 to f0331d3 Compare February 1, 2024 18:00

Apply suggestions from code review

f5dd68a

Co-authored-by: Vladimír Štill <git@vstill.eu>

asl force-pushed the icastable branch from f0331d3 to f5dd68a Compare February 1, 2024 19:38

fruffy merged commit a16fd93 into p4lang:main Feb 1, 2024
16 checks passed

asl deleted the icastable branch February 1, 2024 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NFC] Eliminate the majority of `dynamic_cast` usage in the codebase in favor of ICastable interface #4382

[NFC] Eliminate the majority of `dynamic_cast` usage in the codebase in favor of ICastable interface #4382

asl commented Jan 30, 2024

asl commented Jan 30, 2024

asl Jan 30, 2024

vlstill Jan 31, 2024

asl Jan 31, 2024 •

edited

Loading

asl Jan 30, 2024

vlstill Jan 31, 2024

asl Jan 31, 2024

fruffy Jan 31, 2024

asl Jan 31, 2024

fruffy Jan 31, 2024

asl Jan 31, 2024

fruffy Jan 31, 2024

vlstill left a comment

vlstill Jan 31, 2024

vlstill Jan 31, 2024

asl commented Jan 31, 2024

fruffy Jan 31, 2024

asl Jan 31, 2024

fruffy Jan 31, 2024

fruffy Jan 31, 2024

fruffy left a comment

asl commented Feb 1, 2024

asl commented Feb 1, 2024

asl commented Feb 1, 2024

fruffy commented Feb 1, 2024

asl commented Feb 1, 2024

kfcripps commented Feb 2, 2024

fruffy commented Feb 2, 2024

[NFC] Eliminate the majority of dynamic_cast usage in the codebase in favor of ICastable interface #4382

[NFC] Eliminate the majority of dynamic_cast usage in the codebase in favor of ICastable interface #4382

Conversation

asl commented Jan 30, 2024

asl commented Jan 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asl Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlstill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asl commented Jan 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fruffy left a comment

Choose a reason for hiding this comment

asl commented Feb 1, 2024

asl commented Feb 1, 2024

asl commented Feb 1, 2024

fruffy commented Feb 1, 2024

asl commented Feb 1, 2024

kfcripps commented Feb 2, 2024

fruffy commented Feb 2, 2024

[NFC] Eliminate the majority of `dynamic_cast` usage in the codebase in favor of ICastable interface #4382

[NFC] Eliminate the majority of `dynamic_cast` usage in the codebase in favor of ICastable interface #4382

asl Jan 31, 2024 •

edited

Loading