Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

Conversation

@ScottCarda-MS
Copy link
Contributor

@ScottCarda-MS ScottCarda-MS commented Jul 28, 2020

Adding a utility class, TypeResolutionSummary TypeResolutionCombination, that can be used to summarize combine the effect of multiple type parameter resolution dictionaries into one resolution dictionary. This summarizing combination is calculated during the construction of these summary objects and the resulting summaries combinations are immutable. Validation for the given resolutions is also done during construction and can be check by referencing the IsValidSummary IsValid flag.

There are two constructors for the class. The first one takes an arbitrary list of resolution dictionaries that is expected be given in a specific order. It will construct a different summary combination if the input is in an unexpected order and validation is likely to fail. This constructor is internal only as it is likely that only the call graph will make use of it. The second constructor takes a TypedExpression object and collects all the relevant type parameter resolutions by walking the expression. It can then just use the logic from the first constructor to build the summary combination. This constructor is public as it will be used in the runtime repo.

In total there are 4 public members to the combination object:

  • IndependentResolutionDictionaries: a readonly immutable array of the input resolutions to the combination.
  • CombinedResolutionDictionary: the merged resolution for all the type parameters found in the IndependentResolutionDictionaries.
  • IsValid: a bool that can tell the user if there were any invalid scenarios encountered during the merging of the resolutions.
  • The public constructor, which takes a TypedExpression and collects all the appropriate resolutions from the expression and sub-expressions as input into the merging.

Copy link
Contributor

@cesarzc cesarzc left a comment

Choose a reason for hiding this comment

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

Looks good and seems like it will be very useful to have this logic as a separate class.

@ScottCarda-MS ScottCarda-MS changed the title Type Parameter Resolution Summaries Type Parameter Resolution Combinations Jul 30, 2020
Copy link
Contributor

@bettinaheim bettinaheim left a comment

Choose a reason for hiding this comment

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

Now that this functionality is properly split out, you should replace the corresponding implementation in the ClassicallyControlled transformation and the Monomorphization (and later the CallGraph) with it. If you prefer to do this in a separate PR, then please retarget this to a feature branch - we shouldn't have redundant implementations.

@ScottCarda-MS
Copy link
Contributor Author

Now that this functionality is properly split out, you should replace the corresponding implementation in the ClassicallyControlled transformation and the Monomorphization (and later the CallGraph) with it. If you prefer to do this in a separate PR, then please retarget this to a feature branch - we shouldn't have redundant implementations.

The changes to ClassicallyControlled were already done in a branch under the CallGraph feature branch, so it is was easy and small enough to migrate those into this PR. I can't immediately see any changes to be done to the Monomorphization transformation with regards to this utility. The Monomorphization will need to be heavily changed once the call graph is ready anyway, so it might not be a big deal to leave it as it is for now.

@ScottCarda-MS
Copy link
Contributor Author

ScottCarda-MS commented Aug 3, 2020

Now that this functionality is properly split out, you should replace the corresponding implementation in the ClassicallyControlled transformation and the Monomorphization (and later the CallGraph) with it. If you prefer to do this in a separate PR, then please retarget this to a feature branch - we shouldn't have redundant implementations.

The changes to ClassicallyControlled were already done in a branch under the CallGraph feature branch, so it is was easy and small enough to migrate those into this PR. I can't immediately see any changes to be done to the Monomorphization transformation with regards to this utility. The Monomorphization will need to be heavily changed once the call graph is ready anyway, so it might not be a big deal to leave it as it is for now.

The additions from the call graph branch for ClassicallyControlled includes a new static member of TypedExpression called AsTypeArguments which I believe was originally added by @bettinaheim. Since this is a public-facing addition, I just wanted to point that out.

@bettinaheim
Copy link
Contributor

Now that this functionality is properly split out, you should replace the corresponding implementation in the ClassicallyControlled transformation and the Monomorphization (and later the CallGraph) with it. If you prefer to do this in a separate PR, then please retarget this to a feature branch - we shouldn't have redundant implementations.

The changes to ClassicallyControlled were already done in a branch under the CallGraph feature branch, so it is was easy and small enough to migrate those into this PR. I can't immediately see any changes to be done to the Monomorphization transformation with regards to this utility. The Monomorphization will need to be heavily changed once the call graph is ready anyway, so it might not be a big deal to leave it as it is for now.

The additions from the call graph branch for ClassicallyControlled includes a new static member of TypedExpression called AsTypeArguments which I believe was originally added by @bettinaheim. Since this is a public-facing addition, I just wanted to point that out.

Thanks for pointing it out!
Ok, let's schedule an update to the monomorphization after the call graph is completed then. I am happy to have it go to master as long as something is using it and we don't add redundancy (before this functionality in the classically controlled logic was redundant with the same functionality needed for the monomorphizations).

Copy link
Contributor

@bettinaheim bettinaheim left a comment

Choose a reason for hiding this comment

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

I had one more question that came to my mind, but otherwise looks good!

@ScottCarda-MS ScottCarda-MS merged commit b7cbbe6 into master Aug 5, 2020
@ScottCarda-MS ScottCarda-MS deleted the sccarda/GeneralCombineTypeParams branch August 13, 2020 22:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants