-
Notifications
You must be signed in to change notification settings - Fork 8
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
Cleanup primitive #232
Cleanup primitive #232
Conversation
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.
Bravo @david-michel1, le refactoring avance bien ! Cette PR supprime plein de vieux code inutile, j'approuve. Petit bémol sur le changement de l'interpéteur d'un code fonctionnel sans effet de bord à un fonctionnement avec TGV mutable. C'est plus proche de la réalité mais ça rend plus difficile de lire une sémantique propre pour ton M étendu et du coup ça rend plus difficile la création d'analyses statiques formelles par dessus (ce qui est quand même notre idéal lointain).
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.
Il faut renommer ce fichier mir_interpreter.ml
et le mettre dans le dossier mir/
non ?
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.
La hiérarchie des passes n'est pas encore fixée. Mais l'interpréteur va sans doute opérer sur une structure Mir. D'ailleurs, Com va peut-être devenir le nouveau Mir.
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.
Le répertoire backend_ir
a été effacé. Le code de l'interpréteur est maintenant dans m_ir
.
type print_ctx = { mutable indent : int; mutable is_newline : bool } | ||
|
||
type ctx = { | ||
ctx_local_vars : value Pos.marked Mir.LocalVariableMap.t; | ||
ctx_vars : var_value Bir.VariableMap.t; | ||
ctx_it : Mir.variable IntMap.t; | ||
ctx_tgv : value Array.t; | ||
ctx_tmps : value Array.t; | ||
mutable ctx_tmps_org : int; | ||
ctx_it : Mir.Var.t Array.t; | ||
mutable ctx_it_org : int; | ||
ctx_pr_out : print_ctx; | ||
ctx_pr_err : print_ctx; | ||
ctx_anos : (Mir.error * string option) list; | ||
ctx_old_anos : StrSet.t; | ||
ctx_nb_anos : int; | ||
ctx_nb_discos : int; | ||
ctx_nb_infos : int; | ||
ctx_nb_bloquantes : int; | ||
ctx_finalized_anos : (Mir.error * string option) list; | ||
ctx_exported_anos : (Mir.error * string option) list; | ||
mutable ctx_anos : (Com.Error.t * string option) list; | ||
mutable ctx_old_anos : StrSet.t; | ||
mutable ctx_nb_anos : int; | ||
mutable ctx_nb_discos : int; | ||
mutable ctx_nb_infos : int; | ||
mutable ctx_nb_bloquantes : int; | ||
mutable ctx_finalized_anos : (Com.Error.t * string option) list; | ||
mutable ctx_exported_anos : (Com.Error.t * string option) list; |
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.
Alors je ne suis pas vraiment favorable à avoir des champs mutable
dans le contexte de l'interprète. Jusqu'à présent, l'interprète était purement fonctionnel et cela permet d'en lire le code sans avoir à se préoccuper des effets de bords. Est-ce que c'est pour un gain de performance de l'interpréteur ?
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.
C'est un essai qui ne me satisfait pas pleinement. En fait, je voulais avoir un interpréteur plus rapide (et il l'est). Mais il perd sa fonction de référence sémantique, ce qui est inacceptable. Je vais donc peut-être me retrouver avec un interpréteur de référence et un interpréteur optimisé.
Le code est dans un état intermédiaire. La pull-request porte mal son nom.
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.
Comme la sémantique est impérative, avoir un interpréteur impératif facilite le codage des backends: les différentes versions se ressemblent beaucoup plus. Je vais laisser l'interpréteur comme ça pour l'instant.
let rec evaluate_stmt (canBlock : bool) (p : Bir.program) (ctx : ctx) | ||
(stmt : Bir.stmt) (loc : code_location) = | ||
let set_var_value (p : Mir.program) (ctx : ctx) (var : Mir.Var.t) | ||
(vexpr : Mir.expression Pos.marked) : unit = |
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.
Voilà, je pense que c'est un problème que ce genre de fonctions retourne unit
et pas ctx
...
global_description = input_var.Mast.input_description; | ||
global_typ = Option.map Pos.unmark input_var.Mast.input_typ; | ||
} | ||
Mir.Var.new_tgv ~name:input_var.Mast.input_name ~is_table:None |
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.
new_tgv
créé un TGV ou une variable de TGV ?
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.
Comme la fonction est dans le module Var, c'est forcément une variable TGV, que je vais sans doute renommer "variable globale".
| None -> 0 | ||
in | ||
let prog_targets = | ||
let rec aux nbIt = function |
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.
Qu'est ce que aux
fait ? C'est pas clair en lisant le code.
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.
aux
fait des trucs auxiliaires... Elle changera de nom si ce qu'elle calcule sera conservé.
| "supzero" -> check_func 1 | ||
| "present" -> | ||
| _ -> Err.multimax_require_two_args expr_pos) | ||
| Com.SumFunc -> check_func (-1) |
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.
-1
arguments ? C'est comme ça que tu encodes "un nombre arbitraire d'arguments" ?
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.
Pour l'instant... Je n'avais pas envie de changer toute la structure de la fonction, et de créer un type juste pour ça.
@@ -2186,8 +2338,9 @@ let proceed (p : Mast.program) : program = | |||
| Mast.Rule r -> check_rule r prog | |||
| Mast.Verification v -> check_verif v prog) | |||
prog source_file) | |||
(empty_program p app) p | |||
(empty_program p app main_target) | |||
p | |||
in | |||
prog |> complete_rdom_decls |> complete_vdom_decls |> convert_rules |
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.
Alors ici ce serait vachement bien d'avoir un commentaire à chaque étape qui résume à grands traits ce que cette étape fait !
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.
La moitié de Check_validity va déménager dans un autre fichier, vu qu'il ne s'agit plus de valider la cohérence du code M mais de le transformer en code générique.
src/mlang/m_ir/com.mli
Outdated
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.
Du coup Com
c'est la partie de l'AST commune entre Mast
et Mir
? Si oui c'est malin, j'approuve.
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.
C'est commun le temps de rendre les structures communes (Mast, Mir, Bir). Quand ça sera fait, elle changera de nom puisque les choses communes auront disparu.
4a61eb7
to
0f1861b
Compare
5d80ccb
to
840b2bc
Compare
…into cleanup_primitive
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.
Let's merge this!
No description provided.