Skip to content

Conversation

@RReverser
Copy link
Contributor

@RReverser RReverser commented Nov 21, 2023

Description of Changes

Unlike Rust, previously this would only accept literal constants and not field captures.

Unlike Rust, this will now accept not only constants and captures, but arbitrary expressions - we can either do the same in Rust or limit this similarly in the future.

Some of the changes here are just to add queries to primary integration tests too and make checking logs a bit easier, feel free to review on per-commit basis.

API and ABI breaking changes

If this is an API or ABI breaking change, please apply the
corresponding GitHub label.

Expected complexity level and risk

2 - this is using special C# dynamic evaluation API to evaluate non-constant RHS expressions, but tests show it works as expected in practice.

How complicated do you think these changes are? Grade on a scale from 1 to 5,
where 1 is a trivial change, and 5 is a deep-reaching and complex change.

This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.

If you answered more than a 2, explain what is complex about the PR,
and what other components it interacts with in potentially concerning ways.

@RReverser RReverser force-pushed the csharp-extended-rhs-query branch from 4624e7d to 382f4cc Compare November 21, 2023 15:59
Unlike Rust, previously this would only accept literal constants.

Unlike Rust, this will now accept not only constants and captures, but arbitrary expressions - we can probably do the same in Rust or limit this in the future.
///
/// Sync with: `core::database_logger::Record`. We can't use it
/// directly because the types are wrong for deserialization.
/// (Rust!)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what Rust! means here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Me neither. This code was just moved around.

"Hello, Tyrion!",
"Hello, Cersei!",
"Hello, World!",
"Cersei has age 31 >= 30",
Copy link
Contributor

Choose a reason for hiding this comment

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

I bet she'd deny it.

@RReverser RReverser merged commit ab63654 into master Dec 5, 2023
@RReverser RReverser deleted the csharp-extended-rhs-query branch December 5, 2023 17:28
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