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

Renamed union #412

Merged
merged 9 commits into from
Jul 9, 2024
Merged

Renamed union #412

merged 9 commits into from
Jul 9, 2024

Conversation

Adda0
Copy link
Collaborator

@Adda0 Adda0 commented Jun 26, 2024

This PR resolves the issue with naming the operation union from #172. This PR is a rebase and update of #354.

There are currently two versions of the union operation:

  1. nondeterministic union (the classic union), called union_nondet(), and
  2. union using product construction (performing product construction and adding final states whenever either automaton has the corresponding state in the product state final), called union_product().

The in-place variant of union_nondet() is called unite_nondet_with(), as per the suggestion in #277 (comment).

Another suggestion for the union_nondet() was lang_union().

*/
Nfa& uni(const Nfa &aut);
Copy link
Member

Choose a reason for hiding this comment

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

I think there should still be some main simply-named union function (uni, unionn, unite, or whatever), because all other operations have such functions. It can even have parameters, like other functions (one now could be preserve_determinism), but maybe we do not want that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What would the function do, however? I think that the most basic "union" operation is the current union_nondet(). Therefore, the function would just call the union_nondet(), and if the parameter preserve_determinism was set, the function would just call union_product(). And the purpose of this whole PR was to remove the general uni() function. If we were to return it back, the naming problem would be here again. I personally am all for unite(), for example, but I am not sure what the purpose of such a function would be if the only thing the method does is calling other methods without any reasonable modifications.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, so the point of this PR was to remove the general function. Then yes, this solution is ok.

The purpose would be for the user to not have to think what function to use for union, if we want for him to always think and decide what to use, then this is ok. It would be very similar to how is_lang_empty() is implemented, by default it just calls one of the specialized emptiness test functions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not necessarily to remove the general function, but to remove the naming problem for union() function which cannot be called union. We could maybe agree on adding a general function called unite(), or lang_union(), for example, which would call these two functions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kilohsakul Any preferences?

@@ -59,7 +59,7 @@ void construct(Nfa* result, const ParsedObject& parsed, Alphabet* alphabet = nul
*result = builder::construct(parsed, alphabet, state_map);
}

inline void uni(Nfa *unionAutomaton, const Nfa &lhs, const Nfa &rhs) { *unionAutomaton = uni(lhs, rhs); }
Copy link
Member

Choose a reason for hiding this comment

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

Same as in previous comment, keep some simply-named union function.

src/nfa/nfa.cc Outdated Show resolved Hide resolved
src/nfa/nfa.cc Outdated Show resolved Hide resolved
@Adda0
Copy link
Collaborator Author

Adda0 commented Jul 9, 2024

I pushed the agreed upon changes. We will resolve whether to add a general union function in another issue and PR. For now, I will merge the changes, so I can create PRs for other dependent changes.

@Adda0 Adda0 merged commit a377851 into devel Jul 9, 2024
18 checks passed
@Adda0 Adda0 deleted the renamed_union branch July 9, 2024 12:23
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