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

Tuple for Infix sample language #126

Open
wants to merge 65 commits into
base: main
Choose a base branch
from

Conversation

fhackett
Copy link
Contributor

@fhackett fhackett commented Jul 8, 2024

Adds 3 different implementations of tuples in Infix, optionally enabled via flags.

Pending:

  • Prose write-up in the Markdown file(s).
  • Possibly minor clean-up on the testing code. I tried to keep it organized, but I'm not sure about polish.

Done:

  • README.md is updated
  • Core functionality

Might need reverting:

  • changes to how the maths pass handles calculations (wondered what it would look like, but probably overcomplicated)
  • doc comments for Trieste operators. Partially done. Maybe better than nothing though?

Bug in infix language that's not fixed:

  • x = 5 = 6; is accepted with no error

@fhackett fhackett marked this pull request as draft July 8, 2024 12:18
@fhackett fhackett requested a review from matajoh July 8, 2024 12:18
Copy link
Member

@matajoh matajoh left a comment

Choose a reason for hiding this comment

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

This is part 1 of at least 2, maybe 3.

include/trieste/source.h Outdated Show resolved Hide resolved
include/trieste/source.h Outdated Show resolved Hide resolved
include/trieste/source.h Outdated Show resolved Hide resolved
samples/infix/README.md Outdated Show resolved Hide resolved
samples/infix/README.md Outdated Show resolved Hide resolved
samples/infix/infix_test.cc Outdated Show resolved Hide resolved
samples/infix/bfs.h Outdated Show resolved Hide resolved
// [tuples only] Commas: might be tuple literals, function calls.
R"(,)" >>
[use_parser_tuples](auto& m) {
if (use_parser_tuples)
Copy link
Member

Choose a reason for hiding this comment

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

I think you may be better off using a separate mode for this logic, which also gives an opportunity to teach about parser modes. Basically, if the use_parser_tuples flag is set, then jump into a paren mode and then pop back to start once you encounter the close paren. That way you don't have to worry about doing these checks. There will be some duplication of patterns, but that may actually help?

Copy link
Contributor Author

@fhackett fhackett Jul 19, 2024

Choose a reason for hiding this comment

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

Interesting. I'm ambivalent about modes helping here (since the two effective modes are so very close), but it's worth a try.

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 ended up writing the same pass cloned twice with one tiny difference. You still need to count parentheses, anyhow...

Well, not sure if we actually need the separate modes, but it is nicer to work with using the parentheses counter, so maybe keep that at least?

samples/infix/reader.cc Show resolved Hide resolved
samples/infix/beyond-infix.md Outdated Show resolved Hide resolved
// in the parser.
In(Expression) *
(T(Paren)[Paren]
<< T(Group)[Group])(enable_if_parens_no_parser_tuples) >>
Copy link
Member

Choose a reason for hiding this comment

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

Might make sense to add a scanning of the group for a comma to the predicate? Something like:

([](auto& n){
  return enable_if_parens_no_parser_tuples && n.front().contains(Comma);
}

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 discuss this design option in the prose; I didn't implement it. Arguably it is a 4th option for this tutorial, and should not replace this demonstration of what happens when you don't.

Doing the comma lookahead instead of other approaches would be much simpler, but would also be very case-specific and would give fewer chances to show big rulesets.

samples/infix/reader.cc Show resolved Hide resolved
samples/infix/reader.cc Outdated Show resolved Hide resolved
samples/infix/reader.cc Show resolved Hide resolved
// --- if not, see next section

// the initial 2-element tuple case
In(Expression) * T(TupleCandidate)[TupleCandidate] *
Copy link
Member

Choose a reason for hiding this comment

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

I find this rule a bit confusing. What is a sample input that would produce this sequence of tokens? Given that parentheses are required, it feels like this would be something like:

(6 / 3, 2) 1 + 1, 5 - 3

Which I didn't think was valid (when parens are required).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parentheses are stripped in the expression pass, so this case is needed for just (6 / 3, 2).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extra note: the TupleCandidate part marks expressions that once had parentheses.

samples/infix/writers.cc Outdated Show resolved Hide resolved
samples/infix/writers.cc Outdated Show resolved Hide resolved
samples/infix/writers.cc Outdated Show resolved Hide resolved
samples/infix/writers.cc Show resolved Hide resolved
@@ -358,6 +513,42 @@ namespace
return false;
}

// These 2 outputs below are a postfix encoding of variadic operations,
Copy link
Member

Choose a reason for hiding this comment

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

I think if you pushed the size first it would be closer to what a typical postfix approach would be.

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 don't think that works. The size has to be where it is, as the tuple operator has to pop it first unconditionally. Putting it at the other end means the tuple has to already know how many elements it needs to pop to get to it.

Realistically, the prose is actually a little smarter and explains that Forth dialects usually have a special syntax for tuples rather than this counting stuff, but as-is I think this is the only way round it can work.

@fhackett fhackett marked this pull request as ready for review August 21, 2024 12:31
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.

2 participants