Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Instantiate inferred extensions #461
feat: Instantiate inferred extensions #461
Changes from 4 commits
4491bad
69287bf
cc04a84
1bcbcab
8e55229
fac0a47
1802ccd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a doclink to
infer::infer_extensions
here?And, should the return type here be
ExtensionSolution
? (A typedef to the same type as here)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be, yes. I was trying to distinguish the genuine solution (as
ExtensionSolution
) from the closure, but they really should be the same type regardlessThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now addressed both of these in
8e55229
and1802ccd
, respectivelyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth separating
solution
into two parts? Or only returning the input extensions? (Is there any use for the output ones?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth only returning the input extensions. As you say, there's no use for the rest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll open a separate PR for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which case - this PR looks ok as it stands, if that's a followup; or do you want to roll that in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, I'll do it as a followup, no need for it to block this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
driveby nit:
NodeType::pure
seems a bit misnamed. The so-called "purity" is not about the nodetype - it's about what's already present on the inputs before we even get to that node. A "pure" nodetype would be one with an empty delta;pure
should be callednew_no_extensions
or something...."no extensions" == unextended(??) with synonyms "brief", "truncated", "compact" and "concise". I admit none of these are great. "Basic"? "Retracted" (!)?? Maybe justNodeType::no_extensions
parallelingopen_extensions
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair, but I'm not sure what the better name is. Maybe
open_extensions => open
andpure => closed
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - let's leave renaming to another PR, but I see two reasonable schemes:
open_extensions
andno_extensions
oropen
andclosed
(where currently we haveopen_extensions
andpure
)