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

Look deeper in a node's source position to detect the Truffle language being used. #92

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

nirvdrum
Copy link
Contributor

No description provided.

@nirvdrum nirvdrum requested a review from rwstauner March 25, 2024 14:38
end.map(&:dup)
before_truffle_argument_nodes = @graph.nodes.values.select do |n|
n.props[:synthetic_class] == "TruffleArgument"
["example_polymorphic_receiver", "example_arith_operator"].each do |example|
Copy link
Contributor

@rwstauner rwstauner Mar 25, 2024

Choose a reason for hiding this comment

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

So this expands to describe { before, it, it, before, it, it }...
which seems weird to me (I'm concerned that the contents of the second before block would overwrite the first).
Is it possible the before blocks are attached to the it blocks that follow (rather than just having both on the describe) (or even to the "each" block)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. This is broken. I had confirmed that outer graalvm_version was correct, but @filename is wrong here. I'll take another look. I really wanted to avoid dynamically defining the specs, but if that's what necessary so be it.

Copy link
Contributor Author

@nirvdrum nirvdrum Mar 25, 2024

Choose a reason for hiding this comment

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

I wrapped everything in another describe block, this time printing out the context so it's easier to follow. I applied the same change to the GraalVM version stuff higher up. It makes the spec names a fair bit longer, but it actually looks quite nice in RubyMine (presumably VS Code as well).

Since everything ended up getting indented another level or two, it'd be best to enable the "hide whitespace" option when looking at the diff.

Copy link
Contributor

Choose a reason for hiding this comment

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

I always ignore whitespace changes when looking at diffs :)

@nirvdrum nirvdrum force-pushed the improve-truffle-language-detection branch from e96af0f to 2651ef8 Compare March 25, 2024 21:57
@nirvdrum nirvdrum force-pushed the improve-truffle-language-detection branch from 2651ef8 to 9523129 Compare March 25, 2024 21:59
Copy link
Contributor

@rwstauner rwstauner left a comment

Choose a reason for hiding this comment

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

Confirmed new tests fail without this fix 👍

@nirvdrum nirvdrum merged commit a94c82d into main Mar 25, 2024
17 checks passed
@nirvdrum nirvdrum deleted the improve-truffle-language-detection branch March 25, 2024 22:13
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