[XABT] Separate marshal method storage from MarshalMethodClassifier to MarshalMethodsCollection.
#10077
Conversation
MarshalMethodClassifier to MarshalMethodCollection.MarshalMethodClassifier to MarshalMethodsCollection.
673e9c5 to
4fb09ef
Compare
… to `MarshalMethodCollection`.
4fb09ef to
83a549f
Compare
| class MarshalMethodsCollection : JavaCallableMethodClassifier | ||
| { | ||
| /// <summary> | ||
| /// Assemblies that contain convertible marshal methods. These assemblies need to | ||
| /// have the proper assembly references added to them. | ||
| /// </summary> | ||
| public HashSet<AssemblyDefinition> AssembliesWithMarshalMethods { get; } = []; |
There was a problem hiding this comment.
We don't put this object in RegisterTaskObject() since #10058?
So, should be ok to store an AssemblyDefinition here.
There was a problem hiding this comment.
These stored AssemblyDefinition objects were simply moved from MarshalMethodsClassifier, so it should be functionally the same as what we have today. I suspect when the assembly resolver gets disposed it will close all files held by AssemblyDefinition. Attempting to use this object after that would probably result in ObjectDisposedException but I guess we have structured our code so we don't use these after the resolver has been disposed.
| public static IEnumerable<TAttribute> GetAttributes<TAttribute> (Mono.Cecil.ICustomAttributeProvider p, string attributeName, Func<CustomAttribute, TAttribute?> selector) | ||
| where TAttribute : class | ||
| { | ||
| return p.GetCustomAttributes (attributeName) | ||
| .Select (selector) | ||
| .Where (v => v != null) | ||
| .Select (v => v!); | ||
| } |
There was a problem hiding this comment.
I would usually hesitate to add an overload using this much System.Linq, when I look at the caller below...
| foreach (var a in CecilExtensions.GetAttributes<RegisterAttribute> (p, a => CecilExtensions.ToRegisterAttribute (a))) { | ||
| yield return a; | ||
| } | ||
| foreach (var c in p.GetCustomAttributes ("Java.Interop.JniTypeSignatureAttribute")) { | ||
| var r = RegisterFromJniTypeSignatureAttribute (c); | ||
| if (r == null) { | ||
| continue; | ||
| } | ||
| yield return r; | ||
| } |
There was a problem hiding this comment.
Does it matter the ordering these are yield returned?
It seems like a single foreach loop would allocate a lot less and look for the two attributes by name. But I don't know if the ordering matters.
There was a problem hiding this comment.
All of these functions were copy/pasted from the internal CecilExtensions.cs in Java.Interop.Tools.JavaCallableWrappers because we need them outside the JCW process. Once we figure out where everything ends up, we will probably need to determine where functions like this need to live if they are still needed by multiple processes.
| var v = attr.Properties.FirstOrDefault (p => p.Name == "DoNotGenerateAcw"); | ||
| r.DoNotGenerateAcw = v.Name == null ? false : (bool) v.Argument.Value; |
There was a problem hiding this comment.
Does something need to check if v is null? Maybe we should add #nullable enable to the top of this file.
There was a problem hiding this comment.
Good thought, I added #nullable enable locally and there are no warnings, so I will commit it with a future PR.
Here, var is the struct CustomAttributeNamedArgument so the "Default" will be an empty struct rather than null.
| @@ -0,0 +1,199 @@ | |||
| #nullable enable | |||
There was a problem hiding this comment.
#10093 was merged should we rebase/remove this line?
There was a problem hiding this comment.
This file is in the Xamarin.Android.Build.Tests project which wasn't touched, so I think it's still needed.
Today the marshal method scanning process is tightly coupled to scanning for JavaCallableWrappers. That is, when building
CallableWrapperTypeobjects, theMarshalMethodsClassifieris used to classify methods as marshal methods by callingShouldBeDynamicallyRegistered (...).However
ShouldBeDynamicallyRegistereddoesn't just return a simplebool, it has the side effect of building a list ofMarshalMethodEntryobjects that are used extensively throughout the build process.We are likely going to need to build this list in places other than JavaCallableWrappers scanning: assembly rewriting will likely be before JCWs, and
GenerateNativeMarshalMethodSourceswill likely be after JCWs.As such:
MarshalMethodsClassifierto simply classify methods as LLVM marshal method eligible or not (dynamically registered).MarshalMethodsCollection.Additionally, rework
MarshalMethodsClassifierto also be able to detect marshal methods that have already been rewritten and provide the information later steps (GenerateJavaStubs,GenerateNativeMarshalMethodSources) need to function properly. This isn't in use yet, but will be needed in the future.