Skip to content

[gen] Use explicit equality helpers for edge comparison#1913

Open
ShaleXIONG wants to merge 2 commits into
herd:masterfrom
ShaleXIONG:code-explicit-equal
Open

[gen] Use explicit equality helpers for edge comparison#1913
ShaleXIONG wants to merge 2 commits into
herd:masterfrom
ShaleXIONG:code-explicit-equal

Conversation

@ShaleXIONG

Copy link
Copy Markdown
Collaborator

Add equality helpers for code, edge components, dependencies, and RMWs. This avoids polymorphic equality for tedge internals. Use existing compare_atom for atom equality check.

This pull request separates from #1835 .

Add equality helpers for code, edge components, dependencies, and RMWs.
This avoids polymorphic equality for `tedge` internals.
Use existing `compare_atom` for `atom` equality check.
Comment thread gen/common/edge.ml

let get_access_atom = A.get_access_atom

let equal_atomo = Option.equal (fun a1 a2 -> A.compare_atom a1 a2 = 0)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a lot of code, which doesn't seem trivial to maintain. I think it might be worthwhile to use a ppx processor for the data types to generate this code: https://github.com/ocaml-ppx/ppx_deriving#plugins-eq-and-ord

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

In short, I agree. However, I believe the general consensus is to not introduce extra dependency. I will be in favour of use ppx to deriving many pp and equal function in diy* but not sure how it will impact the compilation and dependencies? @psafont Do you have insight on this?

Comment thread gen/common/code.ml Outdated
Comment on lines +79 to +82
| Int,Int
| Ext,Ext
| UnspecCom,UnspecCom -> true
| (Int|Ext|UnspecCom),_ -> false

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you please indent the pattern matching bars (here and elsewhere), for easier readability and consistency with the rest of the code?

Suggested change
| Int,Int
| Ext,Ext
| UnspecCom,UnspecCom -> true
| (Int|Ext|UnspecCom),_ -> false
| Int,Int
| Ext,Ext
| UnspecCom,UnspecCom -> true
| (Int|Ext|UnspecCom),_ -> false

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I systematically added two spaces for similar code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants