Skip to content

Move all the various id_sett typedefs into irep.h #2075

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

Closed

Conversation

NathanJPhillips
Copy link
Contributor

@NathanJPhillips NathanJPhillips commented Apr 16, 2018

Call them unordered_id_sett to reflect the fact that these are unordered sets
Add matching id_sett for signed sets of identifiers.

@NathanJPhillips
Copy link
Contributor Author

Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

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

  1. The cleanup seems like a good idea.
  2. I don't think id_usett is a great name - can we use unordered_id_sett instead?
  3. Any use of id_sett should be audited: it's useful for doing set intersection/set union, but not for much else, because it won't provide the performance of a hash container and doesn't provide lexicographic ordering either (unless irep_idt == std::string).

@smowton
Copy link
Contributor

smowton commented Apr 17, 2018

Agreed re: unordered_id_sett. I wouldn't wade into set -> unordered_set replacements in this PR, since each case requires some searching to check if anything cares about reproducible ordering and/or stable iterators.

@kroening
Copy link
Member

This only needs the commit messages reworded, then seems fine.

@kroening
Copy link
Member

Considering #2082, I am wondering whether this actually improves readability.

@tautschnig
Copy link
Collaborator

I would prefer: merge #2082 and then make good use of the commits in this PR and do sed -i 's/unordered_id_sett/std::unordered_set<irep_idt>/g' (which now actually is possible thanks to all the consistency cleanup done by @NathanJPhillips.

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.

4 participants