Make transforms part of the public MPS #214

rptb1 · 2023-04-14T08:54:19Z

Fixes #111 .

Progress towards resolving #110 .

Part of the plan to meet Configura's requirements by separating and reviewing each implementation.

This branch cherry-picks and edits changes from branch/2022-01-23/cet-merge-2.

EDIT: Original requirements are discussed and linked in design.mps.transform, introduced by this work.

(cherry picked from commit 6bd64a2)

* Move transforms sources to the core sections of the makefiles. * Move function declarations to the public header mps.h. * Update copyright notices for transforms code. * Remove references to Configura from the comments. * Remove trailing whitespace. * Translate design to reStructuredText. * Move documentation to reference section of manual. * Add warning about unsuitability when ambiguous references may exist. (cherry picked from commit b1c3097)

…ual now that they are part of the main MPS.

… conflicts.

…ransform completing.

…ticular) spills ambiguous references to stack and prevents it working properly.

…C 11.3.0.

rptb1 · 2023-06-14T10:46:09Z

Executing proc.review.entry

Start time 11:29.
Entry criteria: entry.universal, entry.impl, entry.design.
Source documents for recent requirements are available and linked via issues, and original discussions with Configura are linked via the design document.
Most source documents are unreviewed.
proc.review.brief-check: @thejayps and @rptb1 observed that the test, ztfm.c would not pass review and needs reworking. We have raised Testing of transforms is unclear and overengineered #242 and ztfm.c fails to meet its requirement to test transforms on all platforms #243 to fix this. We propose excluding ztfm.c from review except as a source document.
Entry passed otherwise.
End time 11:45.
Entry took 16 minutes.

rptb1 · 2023-06-14T11:03:22Z

Executing proc.review.plan

Start time 11:46.
plan.review.time: Git reports about 2400 lines, but we're not reviewing ztfm.c whic h is 1400 of those, leaving about 1000 lines. This is a bit over 1h @ 10 l/min but since this is mature tested and reviewed code, what we are mostly checking is consistency and correct merging of CET-specific code into master, so we can check faster over the tranforms code itself. I think we can do this within 1h of checking, and time during logging.
proc.review.plan.schedule: Review is scheduled for 2023-06-15 13:00 for about 3h with breaks.
proc.review.plan.source: Public MPS source code does not include transforms #111 , Ravenbrook MPS sources are out of sync with Configura's sources #110 , design references, and Testing of transforms is unclear and overengineered #242 , ztfm.c fails to meet its requirement to test transforms on all platforms #243 concerning why ztfm.c is excluded.
proc.review.plan.rule: All rules except rule.code.python.
proc.review.plan.roles: @thejayps is .role.leader, @UNAA008 could do .role.check.source and .role.check.clarity , @thejayps could do .role.check.consistency and .role.check.source , @rptb1 .role.check.consistency , .role.check.backwards and .role.check.correctness. Note that we do not consider correctness (of the transform code) to be a primary focus because it's mature and well tested code.
proc.review.plan.homework: none.
proc.review.plan.invite: done via Keybase.
proc.review.plan.doc: See above for links, and this pull request.
Planning finished 12:03.
Time taken: 17 minutes.

rptb1 · 2023-06-14T12:54:20Z

@thejayps asks that we spend a bit of time at the end of the review dividing the issues into those that must be edited before 1.118.0 and those which can be raised for later.

thejayps · 2023-06-15T12:29:26Z

Executing proc.review.ko

Start time 1305
@thejayps specified that the product document does not include ztfm.c as it includes many defects. It is a source document.
@thejayps explained that FIXME checks started failing after entry, but this does not need hold up the review.
Checking roles as per suggested in plan: @thejayps to start from code and @rptb1 to start from documentation to achieve some role.check.backwards.
.improve @rptb1 notes that comments that aren't inline ones haven't specifically referenced exactly what they are talking about. References must be made unambiguous and clear by checkers.
Checking started at 1328, logging to commence at 1435 (5 mins break included in 1h check time).

thejayps · 2023-06-15T13:35:09Z

Checking
comment: this PR doesn't obviously link to the review comments made by @gareth-rees in #76 .

M1 - this pull request doesn't obviously refer to the original requirements from Configura

Fix - reference added to PR description

thejayps

performing proc.review.check

code/trans.c

thejayps · 2023-06-15T13:42:26Z

1h checking, checked all of trans.c
Found: 1M 8m 2comments 2questions

rptb1

Executing proc.review.check

Start time 13:30.
Built and ran tests on Kiwi-Ubuntu (lii6ll) and build the manual, no errors.
m. design.mps.transform is missing from design/index.txt. rule.generic.complete
10M, 16m.
Finished checking 14:22.
Checking took 52 minutes.

design/transform.txt

code/trans.c

UNAA008

M: Issue #111 refers to speculation about the use of mps_pool_walk and
mps mps_arena_roots_walk similar objectives to transforms
but the narrative in this issue doesn't reach a conclusion about
whether these are resolved sufficiently for the issue to be closed.
for example
#111 (comment)

M: Issue #110 refers to PR #76 as something that would close this issue.
Unclear whether that branch has been completely mined.

m: Copyright dates require update in design/transform.txt

M: The "not yet written" section in design/transform.txt is extensive, requires explicit review
to determine if any of it is essential for general release.
M: The transform.rst section of the manual does not appear to me to be comprehensive enough for a general release. Perhaps a use case would make a big difference.

thejayps · 2023-06-15T14:06:30Z

Note to editor:

M: Issue #110 refers to PR #76 as something that would close this issue. Unclear whether that branch has been completely mined.

From #214 (review)
Check that #76 is completely mined as one of the steps pre release of 118

UNAA008 · 2023-06-15T14:17:03Z

I: Estimates for review time based on lines of code don't tend to work for role.check.source because this requires reading linked documents which add significantly to the bulk of the work.
Perhaps this could be estimated after a bit of analysis of what's listed as source documents.

…he test, and converting some to TODOs. Linking issues to be resolved.

… updating, clarifying, and cross-referencing, in response to review <#214 (comment)>.

rptb1 · 2023-06-16T08:38:46Z

M: The transform.rst section of the manual does not appear to me to be comprehensive enough for a general release. Perhaps a use case would make a big difference.

Raise: #246

rptb1 · 2023-06-16T08:39:34Z

m: Copyright dates require update in design/transform.txt

Fixed: Updated in ebe0af1

…ew <#214 (comment)>

…ef description of purpose and links, in response to review <#214 (comment)>.

…g the interface more consistent with the rest of the MPS and simplifying the documentation. In response to review <#214 (comment)>.

… automatic pools, in response to review <#214 (comment)>.

…n response to review <#214 (comment)>.

…en transforms are empty or trivial. Raising this for later in GitHub issue #242.

rptb1 · 2023-06-16T12:34:18Z

M1 - this pull request doesn't obviously refer to the original requirements from Configura

Fixed by edit to #214 (comment)

rptb1 · 2023-06-16T12:40:03Z

m. design.mps.transform is missing from design/index.txt. rule.generic.complete

Fixed in 24417c2

rptb1 · 2023-06-16T12:55:15Z

Added pending label so that we later deal with process improvements and meta issues raised in #214 (review)

thejayps

resolving conversations pre exit

manual/source/release.rst

manual/source/topic/transform.rst

…sform may fail and the possible set of return codes

thejayps · 2023-06-17T01:23:05Z

Executing proc.review.exit

Start time 0150
Transgression - Improvement not acted on: Make transforms part of the public MPS #214 (comment) - Raise Ways of estimating progress through checking are not clearly described for all checking roles #250
Transgression - no remaining defects estimated. Do now: ~10 defects discovered considering duplicates. -> Use "75%" rule: 10/0.75 = 13.3 -> 3.3 Major defects remaining. This is at face value above the acceptable level for exit.pass, however we note that the code itself is mature and well tested with Configura, and most of the defects are around the documentation. As we have no other clients actively using transforms now we choose to exceptionally be more accommodating than usual of this estimated number of remaining defects in considering the outcome of this exit. Raise Transforms is estimated to contain more than 2 undiscovered major defects #251
exit.pass
review.exit.calc:
- hours used: 25-30 (very roughly estimated),
- hours saved: 100 - 30 = 70
- major defects remaining: 3.3
exit finished at 0222 - exit took ~30 mins

thejayps

proc.review.exit.pass

thejayps · 2023-06-17T01:39:38Z

executing proc.merge.pull-request (currently in #228)

Start time 0225
3 'sort of': There is a test: ztfm.c which we believe tests transforms correctly. It has not been reviewed as it is likely to have many defects (not suspected to be around .correctness) and it needs redesigning. There are issues logged that will prompt this to happen.
Commit messages for this merge may be incorrect, I was never invited to prepare the correct commit message despite following the merge procedure (and adapting it reasonably to take account of the merge conflicts)
Merge finished at 0238

>.

…ment)> and <#214 (comment)>.

rptb1 · 2023-06-17T10:46:55Z

Fixing up merge mistakes by catching up from master, re-merging, and force-pushing to master. See #253 for prevention.

<#214>.

rptb1 added the essential Will cause failure to meet customer requirements. Assign resources. label Apr 14, 2023

rptb1 added this to the version 1.118 milestone Apr 14, 2023

rptb1 assigned rptb1 and thejayps Apr 14, 2023

Part of making transforms part of the public MPS.

b745b69

(cherry picked from commit 6bd64a2)

rptb1 force-pushed the branch/2023-04-13/transforms branch from a56d1e2 to b745b69 Compare April 14, 2023 09:01

gareth-rees and others added 7 commits June 12, 2023 14:19

Fixing type puns found by GCC 11.3.0.

3ad9c5f

Removing custom CET section (containing only transforms) from the man…

5b9f759

…ual now that they are part of the main MPS.

Fixing typo in Sphinx directive.

08aaf46

Catch-up merge of master into branch/2023-04-13/transforms to resolve…

a7ebcbd

… conflicts.

Avoid creating ambiguous references in stack slots that prevent the t…

380fbf5

…ransform completing.

Temporarily disabling ztfm test except on Windows because GCC (in par…

9f71e31

…ticular) spills ambiguous references to stack and prevents it working properly.

thejayps mentioned this pull request Jun 14, 2023

Testing of transforms is unclear and overengineered #242

Open

Suppressing "may be used uninitialized" warning in rash build with GC…

cec1509

…C 11.3.0.

rptb1 mentioned this pull request Jun 14, 2023

ztfm.c fails to meet its requirement to test transforms on all platforms #243

Open

add FIXMEs to ztfm.c testbench after walkthrough with RB

b0cca65

thejayps requested changes Jun 15, 2023

View reviewed changes

rptb1 commented Jun 15, 2023

View reviewed changes

UNAA008 reviewed Jun 15, 2023

View reviewed changes

rptb1 added 2 commits June 16, 2023 07:43

Converting FIXMEs to comments containing what we have deduced about t…

521e664

…he test, and converting some to TODOs. Linking issues to be resolved.

Moving transforms design statements out of leader comment of trans.c,…

ebe0af1

… updating, clarifying, and cross-referencing, in response to review <#214 (comment)>.

rptb1 mentioned this pull request Jun 16, 2023

Transforms documentation should have a use case example #246

Open

rptb1 added 7 commits June 16, 2023 09:43

Adding spaces to conform to rule.code.style.control.

dc6feb3

Marking up list of causes as a list, for clarity, in response to revi…

54e34ed

…ew <#214 (comment)>

Clarifying relationship between mps_pool_walk and transforms with bri…

0eeefad

…ef description of purpose and links, in response to review <#214 (comment)>.

Transforms are no longer implicitly destroyed by applying them, makin…

d769826

…g the interface more consistent with the rest of the MPS and simplifying the documentation. In response to review <#214 (comment)>.

Clarifying why and documenting that transformed references must be in…

38b7812

… automatic pools, in response to review <#214 (comment)>.

Enforcing parking restrictions when adding references to transforms i…

6663459

…n response to review <#214 (comment)>.

Removing bogus test block introduced in d769826 that does not work wh…

0b27a6c

…en transforms are empty or trivial. Raising this for later in GitHub issue #242.

rptb1 mentioned this pull request Jun 16, 2023

Ignoring identity transforms is unjustified #248

Open

Adding design.mps.transform to design index.

24417c2

rptb1 added the pending Something needs doing, even if closed. label Jun 16, 2023

thejayps marked this pull request as ready for review June 16, 2023 17:35

thejayps reviewed Jun 16, 2023

View reviewed changes

manual/source/release.rst Outdated Show resolved Hide resolved

manual/source/topic/transform.rst Show resolved Hide resolved

remove ambiguity and add clarity about the set of reasons that a tran…

8b169cf

…sform may fail and the possible set of return codes

This was referenced Jun 17, 2023

Ways of estimating progress through checking are not clearly described for all checking roles #250

Open

Transforms is estimated to contain more than 2 undiscovered major defects #251

Open

thejayps approved these changes Jun 17, 2023

View reviewed changes

thejayps merged commit 3af4410 into master Jun 17, 2023

rptb1 added a commit that referenced this pull request Jun 17, 2023

Cross-referencing rank assumptions in response to review <#214 (comment)

7cddef5

>.

rptb1 added a commit that referenced this pull request Jun 17, 2023

Adding references and clarifications in response to review <#214 (com…

151ab13

…ment)> and <#214 (comment)>.

rptb1 added a commit that referenced this pull request Jun 17, 2023

Merging branch branch/2023-04-13/transforms for GitHub pull request #214

554513b

<#214>.

rptb1 mentioned this pull request Nov 6, 2023

Apply all changes from Configura to the public MPS. #76

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make transforms part of the public MPS #214

Make transforms part of the public MPS #214

rptb1 commented Apr 14, 2023 •

edited

Loading

rptb1 commented Jun 14, 2023

rptb1 commented Jun 14, 2023

rptb1 commented Jun 14, 2023

thejayps commented Jun 15, 2023 •

edited

Loading

thejayps commented Jun 15, 2023 •

edited

Loading

thejayps left a comment

thejayps commented Jun 15, 2023

rptb1 left a comment

UNAA008 left a comment •

edited

Loading

thejayps commented Jun 15, 2023

UNAA008 commented Jun 15, 2023

rptb1 commented Jun 16, 2023

rptb1 commented Jun 16, 2023

rptb1 commented Jun 16, 2023

rptb1 commented Jun 16, 2023

rptb1 commented Jun 16, 2023

thejayps left a comment

thejayps commented Jun 17, 2023

thejayps left a comment

thejayps commented Jun 17, 2023

rptb1 commented Jun 17, 2023

Make transforms part of the public MPS #214

Make transforms part of the public MPS #214

Conversation

rptb1 commented Apr 14, 2023 • edited Loading

rptb1 commented Jun 14, 2023

rptb1 commented Jun 14, 2023

rptb1 commented Jun 14, 2023

thejayps commented Jun 15, 2023 • edited Loading

thejayps commented Jun 15, 2023 • edited Loading

thejayps left a comment

Choose a reason for hiding this comment

thejayps commented Jun 15, 2023

rptb1 left a comment

Choose a reason for hiding this comment

UNAA008 left a comment • edited Loading

Choose a reason for hiding this comment

thejayps commented Jun 15, 2023

UNAA008 commented Jun 15, 2023

rptb1 commented Jun 16, 2023

rptb1 commented Jun 16, 2023

rptb1 commented Jun 16, 2023

rptb1 commented Jun 16, 2023

rptb1 commented Jun 16, 2023

thejayps left a comment

Choose a reason for hiding this comment

thejayps commented Jun 17, 2023

thejayps left a comment

Choose a reason for hiding this comment

thejayps commented Jun 17, 2023

rptb1 commented Jun 17, 2023

rptb1 commented Apr 14, 2023 •

edited

Loading

thejayps commented Jun 15, 2023 •

edited

Loading

thejayps commented Jun 15, 2023 •

edited

Loading

UNAA008 left a comment •

edited

Loading