-
Notifications
You must be signed in to change notification settings - Fork 520
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
[Rgen] Add support for the Methods/Constructors in the transformer. #22162
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -122,7 +122,7 @@ public override string ToString () | |
sb.Append ($"ReturnType: {ReturnType}, "); | ||
sb.Append ($"SymbolAvailability: {SymbolAvailability}, "); | ||
sb.Append ($"ExportMethodData: {ExportMethodData}, "); | ||
sb.Append ($"BindAs: {BindAs}, "); | ||
sb.Append ($"BindAs: {BindAs?.ToString () ?? "null"}, "); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and eventually share / reuse some of the logic ? |
||
sb.Append ("Attributes: ["); | ||
sb.AppendJoin (", ", Attributes); | ||
sb.Append ("], Modifiers: ["); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -131,10 +131,9 @@ static BindingType GetBindingType (Dictionary<string, List<AttributeData>> attri | |
/// <summary> | ||
/// Retrieve the base class for a given binding based on its binding infor. | ||
/// </summary> | ||
/// <param name="symbol">The symbol whose base class we want to retrieve..</param> | ||
/// <param name="bindingInfo">The binding information.</param> | ||
/// <returns>The base class to use for the new style SDK based on the binding information.</returns> | ||
static string GetBaseClass (INamedTypeSymbol symbol, BindingInfo bindingInfo) | ||
static string GetBaseClass (BindingInfo bindingInfo) | ||
{ | ||
// collecting the base class and the interface is a little more complicated in the older SDK style because | ||
// we have to make the difference between a class base and a protocol base | ||
|
@@ -198,6 +197,23 @@ internal static bool Skip (PropertyDeclarationSyntax propertyDeclarationSyntax, | |
return !attributes.HasExportAttribute () && !attributes.HasFieldAttribute (); | ||
} | ||
|
||
/// <summary> | ||
/// Decide if a method should be ignored as a change. | ||
/// </summary> | ||
/// <param name="methodDeclarationSyntax">The method declaration under test.</param> | ||
/// <param name="semanticModel">The semantic model of the compilation.</param> | ||
/// <returns>True if the method should be skipped, false otherwise.</returns> | ||
internal static bool Skip (MethodDeclarationSyntax methodDeclarationSyntax, SemanticModel semanticModel) | ||
{ | ||
var methodSymbol = semanticModel.GetDeclaredSymbol (methodDeclarationSyntax); | ||
if (methodSymbol is null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's a one liner on which I'd still add curly braces (due to the comment) |
||
// skip if we cannot retrieve the symbol | ||
return true; | ||
var attributes = methodSymbol.GetAttributeData (); | ||
// we will skip methods that do not have the export attribute | ||
return !attributes.HasExportAttribute (); | ||
} | ||
|
||
/// <summary> | ||
/// Create a new binding based on the interface declaration. Because in the old SDK old the bindings | ||
/// are represented by an interface, this constructor will ensure that the correct binding type is set. | ||
|
@@ -216,7 +232,7 @@ internal Binding (InterfaceDeclarationSyntax interfaceDeclarationSyntax, INamedT | |
name = symbol.Name; | ||
availability = symbol.GetAvailabilityForSymbol (); | ||
namespaces = symbol.GetNamespaceArray (); | ||
baseClass = GetBaseClass (symbol, BindingInfo); | ||
baseClass = GetBaseClass (BindingInfo); | ||
|
||
// retrieve the interfaces and protocols, notice that this are two out params | ||
GetInterfaceAndProtocols (symbol, out interfaces, out protocols); | ||
|
@@ -234,6 +250,17 @@ internal Binding (InterfaceDeclarationSyntax interfaceDeclarationSyntax, INamedT | |
// loop over the different members and add them to the model | ||
GetMembers<PropertyDeclarationSyntax, Property> (interfaceDeclarationSyntax, context, Skip, Property.TryCreate, | ||
out properties); | ||
// methods are a little diff, in the old SDK style, both methods and constructors are methods, we will get | ||
// all exported methods and then filter accordingly | ||
GetMembers<MethodDeclarationSyntax, Method> (interfaceDeclarationSyntax, context, Skip, Method.TryCreate, | ||
out ImmutableArray<Method> allMethods); | ||
methods = [.. allMethods.Where (m => !m.IsConstructor)]; | ||
constructors = allMethods.Where (m => m.IsConstructor) | ||
.Select (m => m.ToConstructor ()) | ||
.Where (c => c is not null) | ||
.Select (c => c!.Value) | ||
.ToImmutableArray (); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 3 Wheres, 2 Selects. A LINQ query that could have been a for loop (for perf reasons). but the meaning is quite clear... |
||
|
||
} | ||
|
||
/// <inheritdoc/> | ||
|
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.
it feels strange to override ToString() for generating code. wouldn't it be clearer to name it ToDisplayString() ? (and add ToString() to the banned API list, making sure it's not used ?)