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

Support multiline strings #83

Merged
merged 2 commits into from
May 19, 2022
Merged

Support multiline strings #83

merged 2 commits into from
May 19, 2022

Conversation

liyishuai
Copy link
Member

Fix #81

@liyishuai
Copy link
Member Author

@Lysxia call for test and review

Copy link
Member

@mjambon mjambon 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 like the right behavior to me. Thanks!

@mjambon
Copy link
Member

mjambon commented May 16, 2022

Maybe I would add a test that checks that backslashes within strings (or comments) aren't removed.

Copy link
Member Author

@liyishuai liyishuai left a comment

Choose a reason for hiding this comment

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

When adding the test/test.ref file I found some places confusing.

Comment on lines +66 to +69
# 78 "test.cppo"
1
# 78 "test.cppo"
is not 1
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this behavior expected?
Context:

cppo/test/test.cppo

Lines 77 to 78 in 08b6c59

#define one 1
one is not 1

Copy link
Member

@mjambon mjambon May 17, 2022

Choose a reason for hiding this comment

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

This is the output I would expect, yes. Unfortunately, my code from 10+ years ago was not commented in a way that explains what it's meant to achieve :-(

The original test is a misleading example. I'd change it to an English sentence that makes sense, such as "1 = 1" e.g.

#define one 1
one = 1

Regarding the expansion: to get a correct source location after replacing some text with a different amount of text, we insert a line directive to avoid shifting the source location. That's why we insert line directives after each expansion. In the original example, one is replaced by 1 followed by a line directive on its own line (# 78 "test.cppo"), followed by a number of spaces to adjust the position within the line to what it was in the source code. In this case and in general when expanding into a shorter string that doesn't add newlines, we could do without a line directive, by adding spaces and newlines as needed for padding. Here, one would have been replaced by 1 followed by two spaces.

I hope this provides the missing explanations. If not, please ask for further clarifications.

Comment on lines +6 to +7
# 6 "test.cppo"
2+ 3.14
Copy link
Member Author

Choose a reason for hiding this comment

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

Are the spaces around 3.14 expected?
Context:

cppo/test/test.cppo

Lines 3 to 6 in 08b6c59

#define pi 3.14
f(1)
#define f(x) x+pi
f(2)

Copy link
Member

Choose a reason for hiding this comment

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

Without looking at the source code, I would say that the reason to surround expanded arguments with spaces was to avoid "token squashing". The goal is preserve the original lexical units and avoid accidentally fusing them. For example, one could write:

#define plus +
x+plus

^ The above should expand to x+ +, which is equivalent to x + +, but not the same as x++ or x ++. We want to avoid the latter, where ++ has become a single OCaml token by accident.

@liyishuai
Copy link
Member Author

Propose merging as-is, and polishing other test cases in a separate PR.

@liyishuai liyishuai merged commit 77820cd into master May 19, 2022
@liyishuai liyishuai deleted the multiline branch May 19, 2022 20:03
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.

Multiline strings are not accepted in define
2 participants