-
Notifications
You must be signed in to change notification settings - Fork 9
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
Expérimentation M++ #227
Expérimentation M++ #227
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.
Merci beaucoup David ! J'ai pas fini ma review (il me manque les changements dans m_frontend
notamment) mais je te laisse déjà quelques commentaires.
Makefile
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.
OK, c'est une grosse réécriture des Makefiles
mais ta nouvelle organisation m'a l'air assez propre donc je suis pas contre changer ça. Par contre ça mérite de la doc, notamment une réécriture du README.md
, pour expliquer comment appeler tes nouveaux Makefile
pour builder, tester, etc.
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.
On va essayer...
Makefile.config.template
Outdated
@@ -9,6 +9,8 @@ | |||
|
|||
# SOURCE_FILES=$(SOURCE_DIR_2018) | |||
|
|||
# SOURCE_EXT_FILES=$(SOURCE_EXT_DIR_2018) |
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.
On peut mettre à jour < 2018 -> 2022 partout sur ces choses là.
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.
Suite à notre discussion, on a convenu que ces fichiers dans examples/dgfip_c/ml_primitif/c_driver_inline_4
et examples/dgfip_c/ml_primitif/c_driver_with_macro
ne sont pas directement utiles au ml_driver
mais servent en réalité au build d'artefacts internes à la DGFiP, donc il faut les supprimer d'ici et les mettre dans un dépôt interne à la DGFiP.
#else | ||
int i = 0; | ||
int trouve = 0; | ||
T_discord *pDisco = discords; | ||
nb_err_finalise = 0; | ||
while (pDisco != NULL) { | ||
trouve = 0; | ||
for (i = 0; i < nb_err_archive && ! trouve; i++) { | ||
if (strcmp(pDisco->erreur->nom, err_archive[i]) == 0) { | ||
trouve = 1; | ||
} | ||
} | ||
if (! trouve) { | ||
ajouter_espace(&sz_err_archive, &err_archive, nb_err_archive); | ||
err_archive[nb_err_archive] = pDisco->erreur->nom; | ||
nb_err_archive++; | ||
ajouter_espace(&sz_err_finalise, &err_finalise, nb_err_finalise); | ||
err_finalise[nb_err_finalise] = pDisco->erreur->nom; | ||
nb_err_finalise++; | ||
} | ||
pDisco = pDisco->suivant; |
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.
CF discussions avec @mdurero, est-il nécessaire de maintenir ces deux versions séparées du code qui diffèrent juste par le fait qu'on ait irdata
dans un cas et ses champs séparément dans l'autre ? Le FLAG_MULTITHREAD
n'est-il pas activé tout le temps ?
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 FLAG_MULTITHREAD
n'est pas stable pour le moment. Dans le doute, les deux versions sont conservées (sachant que seule la version monothread est testée, vu qu'elle n'est pas compatible avec le reste de l'interface C côté DGFiP).
Le flag sera sans aucun doute supprimé quand la version de référence sera écrite et décidée.
} | ||
} | ||
|
||
void print_double(FILE *std, T_print_context *pr_ctx, double f, int pmin, int pmax) { |
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 quoi cette fonction du démon ?
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 la fonction qui affiche les valeurs numériques dans les appels aux 'instruction afficher
et afficher_erreur
. Elle permet l'affichage des flottants avec un paramétrage du nombre de décimales affichées, et surtout l'utilisation de la virgule comme séparateur décimal sans avoir recours au mécanisme des «locales».
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.
Fichier à supprimer comme on en a discuté.
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.
Grand nettoyage en cours.
@@ -626,6 +765,32 @@ struct | |||
else raise (RuntimeError (e, ctx)) | |||
else out | |||
|
|||
let _report_error (err : Mir.error) (var_opt : string option) (_pos : Pos.t) |
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.
Pourquoi le nom commence par _
?
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.
Cette fonction n'est plus utilisée. Elle a été commentée de cette manière pour éviter que l'indentateur la rende illisible (il est pénible ce truc: il ne devrait pas toucher au contenu des commentaires).
| None -> | ||
evaluate_stmts canBlock p ctx | ||
(Bir.FunctionMap.find f p.mpp_functions).mppf_stmts loc 0) | ||
(* Mpp_function arguments seem to be used only to determine which variables |
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.
Vieux commentaire à virer
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.
SFunctionCall
continue à trimballer des arguments qui ne correspondent à rien. Je n'ai pas encore nettoyé cette partie.
@@ -19,6 +19,8 @@ module type NumberInterface = sig | |||
|
|||
val format_t : Format.formatter -> t -> unit | |||
|
|||
val format_prec_t : int -> int -> Format.formatter -> t -> 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.
Ça fait quoi ça ?
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 la version de print_double
pour l'interpréteur, qui sert à afficher les flottants de la même manière en C et en interprété. Cela ne fonctionne qu'avec des doubles; les autres formats appellent directement format_t
. J'ai laissé le reste pour plus tard, vu que je ne travaille qu'avec les doubles pour l'instant.
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.
J'ai pas du tout tout regardé mais 2-3 commentaires
|
||
exception AutoCycle of (vertex * edge) | ||
|
||
let sort ?(auto_cycle = None) (g : 'a graph) = |
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 y a un tri topo dans l'oir (eg https://github.com/MLanguage/mlang/blob/experimentation_mpp/src/mlang/optimizing_ir/dead_code_removal.ml#L190), ce serait bien de le modifier pour prendre le tien ou de modifier celui-là pour la cohérence ?
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 fonction dead_code_removal
n'est plus appelée nulle-part. Je l'ai finalement supprimée.
G.vertexMapFold fold (G.vertices g) G.vertexMapEmpty | ||
in | ||
let rec parcours nd orig_opt (couls, ord, tmp) = | ||
match Option.get (G.vertexMapFindOpt nd couls) with |
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.
Ça vaudrait le coup d'avoir un G.vertexMapFind ? (j'ai l'impression qu'il suffit de transmettre le find du module intMap à G, au lieu de transmettre juste find_opt)
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.
On pourrait, mais ça alourdirait l'interface pour pas grand chose.
let format_loop_domain fmt (ld : loop_domain) = | ||
ParamsMap.pp (Format_mast.pp_print_list_comma format_loop_param_value) fmt ld | ||
|
||
let add_const (name, name_pos) (cval, cval_pos) const_map = |
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.
J'aurais été d'avis d'avoir une seule variable qui encapsule chaque tuple, comme qqch du genre
let add_const (name, name_pos) (cval, cval_pos) const_map = | |
let add_const (name_marked : variable_name Pos.marked) (cval_marked : literal Pos.marked) (const_map : const_context) = |
ou a minima l'annotation de type (sauf s'il y avait une raison de ne pas le faire bien sûr)
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.
On est ensuite obligé de récupérer les éléments du tuple avec des Pos.unmark
et des Pos.get_position
. Dans la version actuelle, la destructuration se fait au plus tôt.
| None -> Err.unknown_constant cval_pos) | ||
| _ -> assert false) | ||
|
||
let expand_table_size const_map table_size = |
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.
Annotation de type : const_context
? (imo plus pour la cohérence avec la suite)
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.
Pourquoi pas. J'ai effectué la modification.
in | ||
List.rev expanded_prog | ||
|
||
(** Screugneugneuh ! *) |
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.
Peut-être expliciter un peu cette doc ;)
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 le problème avec le paramétrage du complilateur: il refuse que les fonctions déclarées en soient pas utilisées, ce qui pose problème avec les fonctions qui servent au débeugage (typiquement des fonctions d'affichage ou de formatage des structures de données). Quand on ne s'en sert pas, on fait quoi ? On renomme les screugheugheuh
en _screugneugneuh
? Ou alors il faut les utiliser d'une manière ou d'une autre, en les ignorant au passage...
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.
OK j'ai fini de review ! Demain je te fais le merge depuis master
si ça te va :)
README.md
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.
Merci pour la mise à jour du README.md
!
let unexpected_errs = StrSet.diff err expected_err in | ||
if not (StrSet.is_empty missing_errs && StrSet.is_empty unexpected_errs) then ( | ||
result := false; | ||
StrSet.iter (Printf.eprintf "KO | %s attendue non recue\n") missing_errs; |
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.
OK, mais dans ce cas il faut qu'on donne un vrai statut à evalcalc.sh
et potentiellement en le publiant ici.
@@ -200,11 +201,23 @@ let main () = | |||
| None | Some "0"-> false | |||
| _ -> exit 31 | |||
in | |||
let annee_exec = | |||
match Sys.getenv_opt "FAKE_YEAR" with |
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.
Tout comme toi je n'aime pas la bidouille de FAKETIME
. Mais plutôt qu'une variable d'environnement, tu veux pas plut^ôt récupérer l'année par un paramètre de ligne de commande (obligatoire) ? Je trouve ça plus propre, comme ça les utilisateurs ne peuvent pas oublier de dire quelle est l'année avec laquelle ils veulent faire tourner les tests.
let ic = Char.code c in | ||
match c with | ||
| '"' | '%' -> | ||
let cc = Format.sprintf "\\%03o" ic in |
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.
Ça veut dire quoi "\\%03o"
? C'est pour échapper les caractères non-ASCII dans la sortie C qui va être compilée dans les mainframes ?
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.
Dans une chaîne échappée, les caractères interdits ou non-affichables sont encodés en octal avec la notation littérale \ooo
(par exemple, \012
pour un saut de ligne NL
).
(Mir.map_expr_var Bir.var_to_mir) | ||
cond.cond_expr)))) ), | ||
ctx )) | ||
(ConditionViolated (fst cond.cond_error, cond.cond_expr, []), ctx)) |
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.
Pourquoi tu as supprimé la liste des variables dépendantes de ce message d'erreur ? Je trouvais ça utile pour débbuguer.
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 fonction qui englobe ce code n'est plus appelée depuis que les anomalies sont stockées dans une liste au lieu d'être encapsulée dans une exception ML. Le constructeur SVerif
a disparu.
if def_kind = NoIndex then | ||
{ Mir.var_definition = Mir.SimpleVar var_expr; Mir.var_typ } | ||
else | ||
Errors.raise_multispanned_error |
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.
Ça aurait pas du être vérifié avant dans check-validity.ml
ça ?
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 dictionnaire des variables est encore géré par l'ancien système. Je ferai sans doute la transition après le merge car ce serait trop long à faire maintenant.
let s = Re.Str.string_before s (String.length s - 1) in | ||
let l = String.length s in | ||
let buf = Buffer.create l in | ||
let rec aux = 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.
Alors que fais-tu ici ? C'est bien cryptique pour moi, ça mérite un commentaire :)
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 fonction parse_string
transforme une chaîne littérale en une suite de char. Il faut donc enlever le premier guillemet (ligne 111), le dernier aussi (ligne 112), puis ajouter dans un Buffer.t chaque caractère, éventuellement en les décodant quand ils sont spécifiés par des échappements (\t
, \xA0
, etc.)
Des commentaires équivalents ont été ajoutés.
src/mlang/test_framework/dune
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.
OK tu l'as déplacé là, parfait
@@ -129,11 +129,9 @@ let retrieve_loc_text (pos : t) : string = | |||
let lng = String.length src in | |||
let rec new_curr () = | |||
if !curr < lng then | |||
if src.[!curr] = '\n' || src.[!curr] = '\r' || !curr = lng then ( | |||
if src.[!curr] = '\n' then ( |
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.
Ça corrige un bug ça ?
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 cas !curr = lng
n'arrive jamais puisque !curr < lng
. Quant au cas src.[!curr] = \r
, je ne sais plus; je crois que certains sauts de ligne étaient ignorés quand les \r
et les \n
étaient bizarrement ordonnés.
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.
OK mais on retire complètement la dépendance à ocamlgraph
alors.
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.
Elle a disparu.
No description provided.