Skip to content
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 any mention of --transition flag #3897

Closed
wants to merge 2 commits into from
Closed

Conversation

gtrepta
Copy link
Contributor

@gtrepta gtrepta commented Jan 8, 2024

fixes: #1936

@gtrepta gtrepta self-assigned this Jan 8, 2024
@gtrepta gtrepta marked this pull request as ready for review January 9, 2024 03:17
@gtrepta gtrepta requested review from Baltoli and Scott-Guest January 9, 2024 03:17
@Scott-Guest
Copy link
Contributor

Scott-Guest commented Jan 9, 2024

I textually searched the codebase for transition, and there are a few lingering references missed by this PR:

Copy link
Contributor

@Scott-Guest Scott-Guest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM, but see my comments.

One thing that's still unclear to me - the outdated lessons state that --search will not explore all heating / cooling interleavings unless specifically instructed to do so with --transition. Is that still the case, and is there some modern alternative to --transition? Or does --search explore all interleavings by default nowadays?

@@ -31,99 +31,6 @@ the `krun` tool with the option `--search`. For example:

Copy link
Contributor

@Scott-Guest Scott-Guest Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've removed almost everything from this lesson - the title is "Tagging; Transition Kompilation Option", and we no longer explain tagging nor --transition. We should consider just deleting it entirely.

The only remaining content is pretty short and could be easily moved into other lessons:

  • defining the variable increment rule
  • describing --search

with the command:

```shell script
kompile imp.k --transition="addition division lookup assignment increment read print"
Copy link
Contributor

@Scott-Guest Scott-Guest Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these addition, division, lookup, etc. tags are unneeded now, so we should remove the corresponding group(_) attributes on the rules. Similarly in the other lessons too.

Comment on lines -34 to -44
Oops, we see only one solution, the same as when we ran it without search.

Here is what happens. `krun` can only explore as much of the transition
system associated to a program as `kompile` allowed the generated language
model to yield. Since most of the K users are interested in language models
that execute efficiently, that is, in faster interpreters for the defined
languages, by default `kompile` optimizes the generated language model for
execution. In particular, it inserts no backtracking markers, which `krun`
uses when called with the `--search` option in order to systematically generate
the entire transition system associated to a program. This is why `krun`
showed us only one solution when run with the `--search` option on `div.imp`.
Copy link
Contributor

@Scott-Guest Scott-Guest Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does --search still only return one solution here, or does it actually show all the different interleavings nowadays?

If it only returns one solution, I think we still need to include a good chunk of the prose here to explain why the other solutions are missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it and indeed --search returns all solutions now. I believe this was written before the search feature in the llvm backend was a thing.

Copy link
Contributor

@Baltoli Baltoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good; happy to merge with Scott's comments addressed given that there is ongoing work to slice out and review the PL tutorial anyway so any snags there aren't critical.

@gtrepta
Copy link
Contributor Author

gtrepta commented Jan 16, 2024

Closing as it's overridden by #3901 . Will open this in the pl-tutorial repo.

@gtrepta gtrepta closed this Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants