Skip to content

Conversation

@owen-mc
Copy link
Contributor

@owen-mc owen-mc commented Jun 14, 2023

Python and Ruby have been (ab)using CastNode to do this. Go now has a use-case as well, so I have made a proper language-specific hook.

owen-mc added 2 commits June 14, 2023 14:27
This allows individual languages to specify `FlowCheckNode`s, which
break up the big step relation and make sure that those nodes appear in
path summaries.
@owen-mc owen-mc force-pushed the dataflow/add-flowCheckNodeSpecific branch from 8cbe6b0 to e34bcef Compare June 14, 2023 13:46
Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. Only minor comment is that flowCheckNodeSpecific isn't a very good predicate name, I think. Perhaps neverSkip, neverSuppress, neverHide, or something like that would be better. The qldoc is also a little off since FlowCheckNode is a rather internal name and not really a good name to mean anything outside its small private context. Here's my alternative take on the qldoc:

Holds if `n` should never be skipped over in the `PathGraph` and in path explanations.

(And having just written that qldoc, I think I'm leaning towards neverSkip as the predicate name unless someone has a better suggestion.)

@smowton
Copy link
Contributor

smowton commented Jun 14, 2023

neverSkip is pretty vague if you don't know this has to do with path explanations. I don't have a good answer right now though.

@aschackmull
Copy link
Contributor

neverSkip is pretty vague if you don't know this has to do with path explanations. I don't have a good answer right now though.

neverSkipInPath, neverSkipInPathGraph, includeInPath, includeInPathGraph?

@owen-mc owen-mc added the no-change-note-required This PR does not need a change note label Jun 14, 2023
@owen-mc
Copy link
Contributor Author

owen-mc commented Jun 14, 2023

I've gone for neverSkipInPathGraph.

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

Nice, thanks for doing this. Abused semantics is always a liability, so getting rid of that feels very good to me 👍

@owen-mc owen-mc merged commit d7c97f8 into github:main Jun 20, 2023
@owen-mc owen-mc deleted the dataflow/add-flowCheckNodeSpecific branch June 20, 2023 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants