-
Notifications
You must be signed in to change notification settings - Fork 19
Creating NFT
from NFA
#526
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
Conversation
Please let me know before merge, this will break noodler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
We will wait with merging this until everything which needs to be done for Noodler is done to not introduce breaking changes in the middle of merging those PRs. If there is an urgent need to merge this, let me know. |
You can merge. |
@@ -108,7 +108,23 @@ Nft parse_from_mata(const std::filesystem::path& nft_file); | |||
* @param epsilons Which symbols handle as epsilons. | |||
* @return NFT representing @p nfa_state with @p num_of_levels number of levels. | |||
*/ | |||
Nft create_from_nfa(const nfa::Nfa& nfa_state, size_t num_of_levels = DEFAULT_NUM_OF_LEVELS, std::optional<Symbol> next_levels_symbol = {}, const std::set<Symbol>& epsilons = { EPSILON }); | |||
Nft from_nfa_with_levels_zero(const nfa::Nfa& nfa_state, size_t num_of_levels = DEFAULT_NUM_OF_LEVELS, bool explicit_transitions = true, std::optional<Symbol> next_levels_symbol = {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The decsription of this function was not updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #535.
const State marker_state{ dft_marker.add_state() }; | ||
dft_marker.levels.resize(marker_state + 1); | ||
dft_marker.levels[marker_state] = 1; | ||
SymbolPost& symbol_post{ *state_post.find(move.symbol) }; | ||
SymbolPost& symbol_post{ *dft_marker.delta.mutable_state_post(source).find(move.symbol) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes, because of creating a new state on line 282
, the delta can reallocate, causing the state_post
to point to memory that is no longer used, causing a SEGFAULT
. Because of this, we need to get a fresh mutable_state_post
again.
@@ -392,7 +391,7 @@ Nft ReluctantReplace::reluctant_leftmost_nft(nfa::Nfa nfa, Alphabet* alphabet, S | |||
const Word& replacement, ReplaceMode replace_mode) { | |||
nfa = reluctant_nfa_with_marker(std::move(nfa), begin_marker, alphabet); | |||
Nft nft_reluctant_leftmost{ | |||
nft::builder::create_from_nfa(nfa, 2, { EPSILON }, { EPSILON }) }; | |||
nft::builder::from_nfa_with_levels_zero(nfa, 2, true, { EPSILON }) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is.
Originally, the call was intended to create an NFT with 2 levels, where the epsilon transition symbol was EPSILON
, and EPSILON
(last parameter) was assigned to the new edges introduced between levels.
Now, the true
parameter indicates that we want explicit transitions (no jump transitions except for epsilon transitions across all levels). The epsilon symbol is implicitly defined as EPSILON
, and the EPSILON
parameter, as before, ensures that newly introduced edges are labeled with EPSILON
.
This PR:
builder::create_from_nfa
tobuilder::from_nfa_zero_levels
.nfa::from_nfa_leveled
tobuilder::from_nfa_increasing_levels
.from_nfa_increasing_levels
(previouslyfrom_nfa_leveled
) from thenfa
namespace tobuilder
.from_nfa_zero_levels
, and closesmata::nft::builder::create_from_nfa
on nfa without transitions #491. This may also resolve issue Replace transducer construction from NFAs #506.ReluctantReplace::marker_nft
, where adding a new state to the automaton caused invalidation of a reference to a mutable state post, leading to a SEGFAULT.