Skip to content

Fix mata::nft::algorithms::concatenate_eps #542

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

Merged
merged 2 commits into from
Jun 4, 2025
Merged

Conversation

jurajsic
Copy link
Member

@jurajsic jurajsic commented Jun 3, 2025

No description provided.

@jurajsic jurajsic requested review from Adda0 and koniksedy June 3, 2025 11:55

// Add epsilon transitions connecting lhs and rhs automata.
// The epsilon transitions lead from lhs original final states to rhs original initial states.
std::vector<Symbol> nft_epsilon(result.num_of_levels, epsilon);
Copy link
Collaborator

Choose a reason for hiding this comment

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

In our current setup, it should be necessary to explicitly create n epsilon transitions over n levels only when dealing with JumpMode::AppendDontCares. For a JumpMode::RepeatSymbol, it is sufficient to have only one transition. Even though at this moment we use explicit transitions in JumpMode::RepeatSymbol, we support long jumps over the epsilon symbol.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably add the JumpMode parameter to the function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree. I would keep the jump transition here unless JumpMode::AppendDontCares is set.

Copy link
Member Author

Choose a reason for hiding this comment

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

I need explicit NFT transition for Noodler here. It seems a bit weird for this function to take JumpMode if the point of the parameter is to decide whether we add one or multiple epsilons.

for (const auto& lhs_final_state: lhs.final) {
for (const auto& rhs_initial_state: rhs.initial) {
result.delta.add(lhs_final_state, epsilon,
result.add_transition(lhs_final_state, nft_epsilon,
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, this can be reversed, in my opinion.

@Adda0
Copy link
Collaborator

Adda0 commented Jun 4, 2025

We will merge for now, since Noodler development must continue, but in the future, we will have to think about what to do with jumps in NFTs (which currently Noodler does not support at all).

@Adda0 Adda0 merged commit 83cc624 into devel Jun 4, 2025
15 checks passed
@Adda0 Adda0 deleted the nft_concatenate_eps branch June 4, 2025 06:33
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