Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0005800OCamlOCaml typingpublic2012-10-27 14:182012-10-30 07:44
Reporterturpin 
Assigned Togarrigue 
PrioritylowSeveritytrivialReproducibilityalways
StatusresolvedResolutionfixed 
PlatformOSOS Version
Product Version4.00.1 
Target VersionFixed in Version4.00.2+dev 
Summary0005800: missing labels in generated applications in Typecore.type_argument
DescriptionIn Typecore.type_argument, Applications to "None" are generated to account for absent optional arguments, but the actual labels are not kept in this application. I believe that this just a mistake, which is fixed by the attached patch (only tested by bootstrapping successfully), but I admit that do not fully understand the code of the typechecker.

I assume that this has no effect on the backend, since labels are discarded anyway, but it matters for typedtree transformations.
Steps To Reproduceunparse the typedtree and recompile...
TagsNo tags attached.
Attached Filespatch file icon typecore_missing_labels.patch [^] (1,188 bytes) 2012-10-27 14:18 [Show Content]
txt file icon default_labels_comment.txt [^] (3,416 bytes) 2012-10-29 20:09 [Show Content]

- Relationships

-  Notes
(0008357)
gasche (developer)
2012-10-29 20:09

I think that the arguments are applied in the reverse order: if the function argument has type ?l1:t1 -> ?l2:t2 -> ?l3:t3 -> t -> u, then your patch produces the application (foo ~l3:None ~l2:None ~l1:None) (because "args" are acumulated by a tail-rec function in reverse position). This is correct as label order is not significant, but I would still have a preference for having the label order, in this generated code, mirror the order in the type, for easier readability of the resulting code.

Finally, it's the second time I've had to read this piece of code and it still made my head hurt. In the hope of helping future reviews of the code, I have written a more verbose comment about what it's doing (patch attached). Jacques, would you agree to include it in the code? (Do you have a policy for comments in general? Adding a tad more comment is something I sometimes wish to do when reading the type-checker code.)
(0008358)
turpin (reporter)
2012-10-29 22:23

I didn't pay attention to the order of arguments, I didn't change the order in which they were produced but since the labels were missing we had "args = rev args", which is no longer true. Is the tail-recursion really needed here ? Who needs so many optional arguments ?
(0008361)
garrigue (manager)
2012-10-30 07:43

This was already fixed in trunk by Alain, but I cannot find it in Mantis.
Anyway, I merged Alain's fix (commits 12919 and 12920) in 4.00@13050.
(0008362)
garrigue (manager)
2012-10-30 07:44

At 13050 in 4.00 and 12919-12920 in trunk.

- Issue History
Date Modified Username Field Change
2012-10-27 14:18 turpin New Issue
2012-10-27 14:18 turpin File Added: typecore_missing_labels.patch
2012-10-29 20:09 gasche Note Added: 0008357
2012-10-29 20:09 gasche File Added: default_labels_comment.txt
2012-10-29 22:23 turpin Note Added: 0008358
2012-10-30 07:43 garrigue Note Added: 0008361
2012-10-30 07:44 garrigue Note Added: 0008362
2012-10-30 07:44 garrigue Status new => resolved
2012-10-30 07:44 garrigue Fixed in Version => 4.00.2+dev
2012-10-30 07:44 garrigue Resolution open => fixed
2012-10-30 07:44 garrigue Assigned To => garrigue


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker