Add NullAssemblyResolver and PathAssemblyResolver - #721
Conversation
This comment has been minimized.
This comment has been minimized.
|
I think I think a SearchDirectoryAssemblyResolver is not needed because you can express that as a |
c146283 to
69a770c
Compare
js6pak
changed the title
| /// <inheritdoc /> | ||
| public ResolutionStatus Resolve(AssemblyDescriptor assembly, ModuleDefinition? originModule, out AssemblyDefinition? result) | ||
| { | ||
| if (assembly.Name is null || !_simpleNameMap.TryGetValue(assembly.Name, out var path) || !File.Exists(path)) |
There was a problem hiding this comment.
I think the File.Exists check could be moved to the ctor.
There was a problem hiding this comment.
I thought about it, but I could see a situation where someone creates the PathAssemblyResolver instance before the files are created, as weird as it might be.
There was a problem hiding this comment.
Wouldn't the same be true of directories?
There was a problem hiding this comment.
Well.. yeah. But I'd say it's easier to stomach since the purpose of PathAssemblyResolver is not resolving from directories and FromSearchDirectories is just a helper factory method.
It is pretty nasty though.. I wonder if we should just always iterate in Resolve and support both files and directories as valid paths.
There was a problem hiding this comment.
I like the O(1) lookup complexity we get with this. Perhaps it deserves a separate LazyPathAssemblyResolver to handle that niche case.
There was a problem hiding this comment.
I don't think O(1) is crucial; IAssemblyResolver is not a hot path anymore given that cache management is done on RuntimeContext level.
That being said, I am not sure I can see a real use-case for a PathAssemblyResolver where its input files do not exist yet. Do you have any specific scenario in mind?
There was a problem hiding this comment.
You could keep the lookup built in the ctor, and iterate the search directories as a fallback.
Also I wonder if clashed filenames should be iterated over instead of only trying the first. (Dictionary<string, List<string>>)
There was a problem hiding this comment.
You could keep the lookup built in the ctor, and iterate the search directories as a fallback.
Including search directories would be against the main purpose of a ReferencePathAssemblyResolver (i.e., specific set of files vs directory search), so I wouldn't really be in favor of this.
Also I wonder if clashed filenames should be iterated over instead of only trying the first. (Dictionary<string, List>)
As far as I know, DLLs are always preferred over EXE files? I could be wrong, I am not too familiar with the inner workings of msbuild or build tasks, but I have not seen the opposite happen at runtime.
There was a problem hiding this comment.
I'm thinking more if assemblies with the same name exist in different search directories, not concerning MSBuild's ReferencePath.
There was a problem hiding this comment.
Including search directories would be against the main purpose of a
ReferencePathAssemblyResolver(i.e., specific set of files vs directory search), so I wouldn't really be in favor of this.
Maybe better to keep it simple, then, and drop the search directories factory?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Washi1337
left a comment
Washi1337
left a comment
There was a problem hiding this comment.
Small comments.
Also, please make sure to use the explicit keywords for built-in types string, int, string[] etc. in local variable declarations to keep code style consistent.
Other than that, LGTM!
Test Results
Failing runs |
3 participants
No description provided.