Discussion on refactoring of some things, only comments, not intended for merge.

include/mata/nfa/algorithms.hh

@@ -68,6 +68,8 @@ Nfa complement_classical(const Nfa& aut, const Mata::Util::OrdVector<Symbol>& sy
  * i.e., if the final intersection of smaller complement of bigger is empty.
  */
 bool is_included_naive(const Nfa& smaller, const Nfa& bigger, const Alphabet* alphabet = nullptr, Run* cex = nullptr);
+//naive is not a good word, maybe textbook, or explicit, construct_first
+//the alphabet parameter is useless in inclusion
 
 /**
  * Inclusion implemented by antichain algorithms.
@@ -79,6 +81,7 @@ bool is_included_naive(const Nfa& smaller, const Nfa& bigger, const Alphabet* al
  * i.e., if the final intersection of smaller complement of bigger is empty.
  */
 bool is_included_antichains(const Nfa& smaller, const Nfa& bigger, const Alphabet*  alphabet = nullptr, Run* cex = nullptr);
+//antichains -> antichain. Everywhere here.
 
 /**
  * Universality check implemented by checking emptiness of complemented automaton
@@ -88,6 +91,7 @@ bool is_included_antichains(const Nfa& smaller, const Nfa& bigger, const Alphabe
  * @return True if the complemented automaton has non empty language, i.e., the original one is not universal
  */
 bool is_universal_naive(const Nfa& aut, const Alphabet& alphabet, Run* cex);
+//naive -> textbook/explicit
 
 /**
  * Universality checking based on subset construction with antichain.
@@ -98,6 +102,22 @@ bool is_universal_naive(const Nfa& aut, const Alphabet& alphabet, Run* cex);
  */
 bool is_universal_antichains(const Nfa& aut, const Alphabet& alphabet, Run* cex);
 
+//the alphabet parameter should be optional? The automaton can have its alphabet too. Run should be optional.
+//actually, what about to use this, with more optional parameters through structures
+//struct univ_params
+//{
+//    Alphabet* alphabet = NULL,
+//    Run* cex = NULL,
+//};
+//   bool is_universal_antichains(const Nfa& aut, univ_params={NULL,NULL});
+//   and call as
+//   bool is_universal_antichains(const Nfa& aut);
+//   bool is_universal_antichains(const Nfa& aut,{.alphabet = bla});
+//   bool is_universal_antichains(const Nfa& aut,{.cex = bli});
+//   bool is_universal_antichains(const Nfa& aut,{.alphabet = bla, .cex = bli});
+
+//comment missing
+//Do we really need this?
 Simlib::Util::BinaryRelation compute_relation(
         const Nfa& aut,
         const StringMap&  params = {{"relation", "simulation"}, {"direction", "forward"}});
@@ -112,6 +132,7 @@ Simlib::Util::BinaryRelation compute_relation(
  * @param[out] prod_map Mapping of pairs of the original states (lhs_state, rhs_state) to new product states.
  * @return NFA as a product of NFAs @p lhs and @p rhs with ε-transitions preserved.
  */
+// all parameters needed/used?
 Nfa intersection_eps(const Nfa& lhs, const Nfa& rhs, bool preserve_epsilon, const std::set<Symbol>& epsilons,
     std::unordered_map<std::pair<State,State>, State> *prod_map = nullptr);
 
@@ -127,6 +148,7 @@ Nfa intersection_eps(const Nfa& lhs, const Nfa& rhs, bool preserve_epsilon, cons
  * @param[out] rhs_result_states_map Map mapping rhs states to result states.
  * @return Concatenated automaton.
  */
+//Are both maps needed?
 Nfa concatenate_eps(const Nfa& lhs, const Nfa& rhs, const Symbol& epsilon, bool use_epsilon = false,
     StateToStateMap* lhs_result_states_map = nullptr, StateToStateMap* rhs_result_states_map = nullptr);
 

include/mata/nfa/builder.hh

@@ -17,6 +17,7 @@ namespace Mata::Nfa::Builder {
 /**
  * Create an automaton accepting only a single @p word.
  */
+//should we drop the create_ from all the names?
 Nfa create_single_word_nfa(const std::vector<Symbol>& word);
 
 /**
@@ -45,10 +46,12 @@ Nfa create_sigma_star_nfa(Alphabet* alphabet = new OnTheFlyAlphabet{});
 /** Loads an automaton from Parsed object */
 // TODO this function should the same thing as the one taking IntermediateAut or be deleted
 Nfa construct(const Mata::Parser::ParsedSection& parsec, Alphabet* alphabet, StringToStateMap* state_map = nullptr);
+//construct does not cary much info, what is better? In the comment, don't know what Parsed object is, overall I don't get what this function does.
 
 /** Loads an automaton from Parsed object */
 Nfa construct(const Mata::IntermediateAut& inter_aut, Alphabet* alphabet, StringToStateMap* state_map = nullptr);
 
+//comment missing
 template<class ParsedObject>
 Nfa construct(const ParsedObject& parsed, Mata::StringToSymbolMap* symbol_map = nullptr,
               StringToStateMap* state_map = nullptr) {

include/mata/nfa/delta.hh

@@ -8,6 +8,19 @@ namespace Mata::Nfa {
 /**
  * Structure represents a move which is a symbol and a set of target states of transitions.
  */
+    //HOW TO RENAME STUFF
+    //delta/coniferation (a set/sequence of cones)/cone
+    //choice,
+    //iconary,
+    //lexicon,
+    //choice/fan/fanout/cone/fork
+    //symbolbrach, sourcebranch
+    //srcbranch, charbranch
+    //srcfork,symfork,tgtfork
+    //srctree,symtree
+    //srctree,symtree,tgttree
+    //delta,srcpost,sympost
+    //AND THE WINNER IS: delta, srcpost, sympost. Huraaa
 class Move {
 public:
     Symbol symbol{};
@@ -97,6 +110,10 @@ public:
 class Delta {
 private:
     std::vector<Post> posts;
+    //this internal vector of things is done in a different way in each of the structures, post, delta, move.
+    //would be nice to make it uniform
+    //the delta way is the cleanest, no? a private data member
+    //lets do it that way?
 
 public:
     inline static const Post empty_post; // When posts[q] is not allocated, then delta[q] returns this.
@@ -125,6 +142,7 @@ public:
     // But it feels fragile, before doing something like that, better think and talk to people.
     Post& get_mutable_post(State q);
 
+    //Needs explanation. Maybe refactoring, replacing by several more sensible functions
     void defragment(const BoolVector& is_staying, const std::vector<State>& renaming);
 
     // Get a constant reference to the post of a state. No side effects.
@@ -134,6 +152,7 @@ public:
 
     void clear() { posts.clear(); }
 
+    //Still fishy, what do we mean by size here? Probably should be just the size of the vector. However, below we call it num_of_states.
     void increase_size(size_t n) {
         assert(n >= posts.size());
         posts.resize(n);
@@ -143,9 +162,12 @@ public:
      * @return Number of states in the whole Delta, including both source and target states.
      */
     size_t num_of_states() const { return posts.size(); }
+    //should it just be size() ?
+
 
     void add(State state_from, Symbol symbol, State state_to);
     void add(const Trans& trans) { add(trans.src, trans.symb, trans.tgt); }
+    //more add functions below, should be together?
     void remove(State src, Symbol symb, State tgt);
     void remove(const Trans& trans) { remove(trans.src, trans.symb, trans.tgt); }
 
@@ -192,6 +214,7 @@ public:
     /**
      * Iterator over transitions. It iterates over triples (lhs, symbol, rhs) where lhs and rhs are states.
      */
+    //So this should become the thing discussed, delta.transitions.begin() ..., with internal transition, * will return a const reference to it
     struct const_iterator {
     private:
         const std::vector<Post>& post;

include/mata/nfa/nfa.hh

@@ -106,18 +106,21 @@ public:
     /**
      * Clear transitions but keep the automata states.
      */
+    //method of delta?
     void clear_transitions();
 
     /**
      * Add a new (fresh) state to the automaton.
      * @return The newly created state.
      */
+     //obsolete?
     State add_state();
 
     /**
      * Add state @p state to @c delta if @p state is not in @c delta yet.
      * @return The requested @p state.
      */
+     //obsolete?
     State add_state(State state);
 
     /**
@@ -126,6 +129,7 @@ public:
      * This includes the initial and final states as well as states in the transition relation.
      * @return The number of states.
      */
+     //rename, again, to num_of_states?
      size_t size() const;
 
     /**
@@ -145,6 +149,7 @@ public:
      *
      * The whole NFA is cleared, each member is set to its zero value.
      */
+     //used?
     void clear();
 
     /**
@@ -165,6 +170,8 @@ public:
      */
     Util::OrdVector<Symbol> get_used_symbols() const;
 
+    //this is to be debordelized, the variants were here just to play with data structures
+    //should be a method of delta
     Mata::Util::OrdVector<Symbol> get_used_symbols_vec() const;
     std::set<Symbol> get_used_symbols_set() const;
     Mata::Util::SparseSet<Symbol> get_used_symbols_sps() const;
@@ -201,6 +208,7 @@ public:
      * @return Set of useful states.
      * TODO: with the new get_useful_states, we can delete this probably.
      */
+     //keep just the new one
     StateSet get_useful_states_old() const;
 
     BoolVector get_useful_states() const;
@@ -215,6 +223,7 @@ public:
      * @param[out] state_map Mapping of trimmed states to new states.
      * TODO: we can probably keep just trim_reverting, much faster. But the speed difference and how it is achieved is interesting. Keeping as a demonstration for now.
      */
+     //to be compared on some good data
     void trim_inplace(StateToStateMap* state_map = nullptr);
     void trim_reverting(StateToStateMap* state_map = nullptr);
     void trim(StateToStateMap* state_map = nullptr) { trim_inplace(state_map); }
@@ -246,19 +255,22 @@ public:
      *
      * The operation has constant time complexity.
      */
+     //remove get_ ?
     size_t get_num_of_trans() const { return static_cast<size_t>(std::distance(delta.begin(), delta.end())); }
 
     /**
      * Get transitions as a sequence of @c Trans.
      * @return Sequence of transitions as @c Trans.
      */
+     //this will become obsolete, we will have iterators in delta
     TransSequence get_trans_as_sequence() const;
 
     /**
      * Get transitions from @p state_from as a sequence of @c Trans.
      * @param state_from[in] Source state_from of transitions to get.
      * @return Sequence of transitions as @c Trans from @p state_from.
      */
+    //this will become obsolete, we will have iterators in delta
     TransSequence get_trans_from_as_sequence(State state_from) const;
 
     /**
@@ -269,6 +281,7 @@ public:
      * @param state_from[in] Source state for transitions to get.
      * @return List of transitions leading from @p state_from.
      */
+    //this will become obsolete, we will have iterators in delta
     const Post& get_moves_from(const State state_from) const {
         assert(state_from < size());
         return delta[state_from];
@@ -280,13 +293,15 @@ public:
      * @return Sequence of @c Trans transitions leading to @p state_to.
      * (!slow!, traverses the entire delta)
      */
+     //this function somehow does not fit with the planned iterator interface, but I don't know what to do
     TransSequence get_transitions_to(State state_to) const;
 
     /**
      * Unify transitions to create a directed graph with at most a single transition between two states.
      * @param[in] abstract_symbol Abstract symbol to use for transitions in digraph.
      * @return An automaton representing a directed graph.
      */
+     //why the get_ everywhere?
     Nfa get_one_letter_aut(Symbol abstract_symbol = 'x') const;
 
     /**
@@ -338,6 +353,7 @@ public:
     void print_to_mata(std::ostream &output) const;
 
     // TODO: Relict from VATA. What to do with inclusion/ universality/ this post function? Revise all of them.
+    // One usage in universality, should disappear and this function should be removed.
     StateSet post(const StateSet& states, const Symbol& symbol) const;
 
     struct const_iterator {
@@ -374,6 +390,7 @@ public:
      * @return Returns reference element of transition list with epsilon transitions or end of transition list when
      * there are no epsilon transitions.
      */
+     //These things should be methods of delta?
     Post::const_iterator get_epsilon_transitions(State state, Symbol epsilon = EPSILON) const;
 
     /**
@@ -383,13 +400,15 @@ public:
      * @return Returns reference element of transition list with epsilon transitions or end of transition list when
      * there are no epsilon transitions.
      */
+    //These things should be methods of delta?
     static Post::const_iterator get_epsilon_transitions(const Post& post, Symbol epsilon = EPSILON);
 
     /**
      * @brief Expand alphabet by symbols from this automaton to given alphabet
      *
      * The value of the already existing symbols will NOT be overwritten.
      */
+     //A method of delta as well?
     void add_symbols_to(OnTheFlyAlphabet& target_alphabet) const;
 }; // struct Nfa.
 
@@ -398,9 +417,11 @@ public:
   * @param[in] nfa NFA with symbols to fill @p alphabet with.
   * @param[out] alphabet Alphabet to be filled with symbols from @p nfa.
   */
+    //A method of delta as well?
 void fill_alphabet(const Mata::Nfa::Nfa& nfa, Mata::OnTheFlyAlphabet& alphabet);
 
 // Adapted from: https://www.fluentcpp.com/2019/01/25/variadic-number-function-parameters-type/.
+//Please explain in a comment here what this is.
 template<bool...> struct bool_pack{};
 /// Checks for all types in the pack.
 template<typename... Ts> using conjunction = std::is_same<bool_pack<true,Ts::value...>, bool_pack<Ts::value..., true>>;
@@ -413,6 +434,7 @@ template<typename... Ts> using AreAllNfas = typename conjunction<std::is_same<Ts
  * @param[in] nfas NFAs to create alphabet from.
  * @return Created alphabet.
  */
+ //What is this, what are these types? Is it sane?
 template<typename... Nfas, typename = AreAllNfas<Nfas...>>
 inline OnTheFlyAlphabet create_alphabet(const Nfas&... nfas) {
     Mata::OnTheFlyAlphabet alphabet{};
@@ -428,6 +450,7 @@ inline OnTheFlyAlphabet create_alphabet(const Nfas&... nfas) {
  * @param[in] nfas Vector of NFAs to create alphabet from.
  * @return Created alphabet.
  */
+ //We are to refactor alphabets, right?
 OnTheFlyAlphabet create_alphabet(const ConstAutRefSequence& nfas);
 
 /**
@@ -449,6 +472,7 @@ OnTheFlyAlphabet create_alphabet(const ConstAutPtrSequence& nfas);
  * @param[in] nfas Vector of pointers to NFAs to create alphabet from.
  * @return Created alphabet.
  */
+ //Do we need all the variants? If yes, can we have one with iterators, or a template?
 OnTheFlyAlphabet create_alphabet(const AutPtrSequence& nfas);
 
 /// Do the automata have disjoint sets of states?
@@ -460,8 +484,10 @@ bool are_state_disjoint(const Nfa& lhs, const Nfa& rhs);
  * @param[out] cex Counter-example path for a case the language is not empty.
  * @return True if the language is empty, false otherwise.
  */
+ //should be a method?
 bool is_lang_empty(const Nfa& aut, Run* cex = nullptr);
 
+//could be in-place method
 Nfa uni(const Nfa &lhs, const Nfa &rhs);
 
 /**
@@ -482,6 +508,8 @@ Nfa uni(const Nfa &lhs, const Nfa &rhs);
  * @param[out] prod_map Mapping of pairs of the original states (lhs_state, rhs_state) to new product states.
  * @return NFA as a product of NFAs @p lhs and @p rhs with ε-transitions preserved.
  */
+
+//preserve_epsilon optional too? Do we need both options?
 Nfa intersection(const Nfa& lhs, const Nfa& rhs,
                  bool preserve_epsilon = false, std::unordered_map<std::pair<State, State>, State> *prod_map = nullptr);
 
@@ -497,6 +525,7 @@ Nfa intersection(const Nfa& lhs, const Nfa& rhs,
  * @return Concatenated automaton.
  */
 // TODO: check how fast is using just concatenate over epsilon and then call remove_epsilon().
+// choice of optional parameters using structure?
 Nfa concatenate(const Nfa& lhs, const Nfa& rhs, bool use_epsilon = false,
                 StateToStateMap* lhs_result_states_map = nullptr, StateToStateMap* rhs_result_states_map = nullptr);
 
@@ -513,6 +542,8 @@ Nfa concatenate(const Nfa& lhs, const Nfa& rhs, bool use_epsilon = false,
  * @param[in] sink_state The state into which new transitions are added.
  * @return True if some new transition was added to the automaton.
  */
+ //method complete
+ //alphabet optional, sink stata too?
 bool make_complete(Nfa& aut, const Alphabet& alphabet, State sink_state);
 
 /**
@@ -532,6 +563,7 @@ bool make_complete(Nfa& aut, const Alphabet& alphabet, State sink_state);
  * @param[in] sink_state The state into which new transitions are added.
  * @return True if some new transition was added to the automaton.
  */
+ //eventually only one variant, the version with the optional alphabet (some enum alphabet type) ?
 bool make_complete(Nfa& aut, const Util::OrdVector<Symbol>& symbols, State sink_state);
 
 /**
@@ -555,6 +587,7 @@ inline bool make_complete(Nfa& aut, const Alphabet& alphabet) { return make_comp
  * - "minimize": "true"/"false" (whether to compute minimal deterministic automaton for classical algorithm);
  * @return Complemented automaton.
  */
+ //method
 Nfa complement(const Nfa& aut, const Alphabet& alphabet,
    const StringMap& params = {{"algorithm", "classical"}, {"minimize", "false"}});
 
@@ -573,6 +606,7 @@ Nfa complement(const Nfa& aut, const Alphabet& alphabet,
  * - "minimize": "true"/"false" (whether to compute minimal deterministic automaton for classical algorithm);
  * @return Complemented automaton.
  */
+//Do we need this? How many algorithms do we have? If yes, maybe these syntactic sugar functions could really have their own file.
 Nfa complement(const Nfa& aut, const Util::OrdVector<Symbol>& symbols,
    const StringMap& params = {{"algorithm", "classical"}, {"minimize", "false"}});
 
@@ -741,6 +775,7 @@ Run encode_word(const StringToSymbolMap& symbol_map, const std::vector<std::stri
 } // namespace Mata::Nfa.
 
 namespace std {
+    //some comments needed
 template <>
 struct hash<Mata::Nfa::Trans> {
 	inline size_t operator()(const Mata::Nfa::Trans& trans) const {