Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure we are finding proper operator<< during debug printing #4860

Merged
merged 3 commits into from
Aug 14, 2024

Conversation

asl
Copy link
Contributor

@asl asl commented Aug 10, 2024

Some cleanup while there

First step of #4698

@asl asl requested review from vlstill, qobilidop and fruffy August 10, 2024 08:53
@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Aug 10, 2024
@ChrisDodd
Copy link
Contributor

So it looks like this forces everyone to implement both dbprint and operator<< for any class they want to output in a LOG message? I suspect backends will need adjustment for this. @grg ?

@asl
Copy link
Contributor Author

asl commented Aug 11, 2024

So it looks like this forces everyone to implement both dbprint and operator<< for any class they want to output in a LOG message? I suspect backends will need adjustment for this. @grg ?

Well, they indeed will require to do this after adding namespaces as "generic" operator<< will not be found as it is defined in different namespace and cannot be found via ADL.

For now, it will be enough to inherit from IHasDbPrint. As a second step, I will implement a custom "log stream", so operator<<(log stream&, T) will call dbprint if present.

See some discussion in #4825 (comment) and #4698 (comment)

BUILD.bazel Outdated
@@ -253,6 +253,7 @@ cc_library(
"@com_google_absl//absl/container:inlined_vector",
"@com_google_absl//absl/strings",
"@com_google_absl//absl/strings:str_format",
"@com_google_absl//absl/strings:str_join",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bazel CI failure is because there isn't a separate :str_join target. str_join.h is included in the :strings target: https://github.com/abseil/abseil-cpp/blob/ac267be5cf9b722a89c0293ccaa7df1ecdb3a446/absl/strings/BUILD.bazel#L84. I think the "@com_google_absl//absl/strings" line above should have included it already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I also hoped so, but it failed being unable to find str_join in https://github.com/p4lang/p4c/actions/runs/10330407855/job/28599406481

Need to check what's going on more... maybe some includes are missed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like

p4c/backends/tc/tc.def

Lines 17 to 21 in 0e2a7d6

#emit
#include "backends/tc/tc_defines.h"
#include "absl/strings/str_cat.h"
#include "absl/strings/str_join.h"
#end
seems to be a solution.

Copy link
Contributor Author

@asl asl Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I'm wondering why this is not required for non-bazel builds though :) Have not checked yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, didn't realize this. That's interesting 🤔

Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this goes in the right direction.

Do I understand it correctly that for things what inherit IHasDbPrint the operator gets found by ADL?

ir/visitor.h Show resolved Hide resolved
@asl
Copy link
Contributor Author

asl commented Aug 12, 2024

Do I understand it correctly that for things what inherit IHasDbPrint the operator gets found by ADL?

Yes. But this is an interim solution to not break LOG() constructs that use operator<<. Also, there are some implementations of dbprint / toString of IR nodes that rely on operator<< being present. This would be untangled later on :)

@asl asl force-pushed the ostream-operator branch from 9a04eb8 to 90d28f7 Compare August 12, 2024 18:36
@asl asl force-pushed the ostream-operator branch from cdd3b21 to 427a5c8 Compare August 13, 2024 22:47
@asl asl added this pull request to the merge queue Aug 14, 2024
Merged via the queue into p4lang:main with commit 1f5468c Aug 14, 2024
18 checks passed
@asl asl deleted the ostream-operator branch August 14, 2024 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants