Uh oh!
There was an error while loading. Please reload this page.
Update code generator to handle generic types and types with multiple '+' signs in them. - #102
Conversation
yaeldekel
commented
May 9, 2018
Fixes #101 |
| } | ||
| static string GetSymbolFromType(Dictionary<string, string> typesSymbolTable, string fullTypeName, string currentNamespace) | ||
| static string GetSymbolFromType(Dictionary<string, string> typesSymbolTable, Type type, string currentNamespace) |
There was a problem hiding this comment.
static [](start = 8, length = 6)
While we're at it let's add an explicit access modifier... #Closed
| { | ||
| Contracts.AssertValue(genericTypes); | ||
| fullTypeName = fullTypeName.Substring(0, bracketIndex); | ||
| } |
There was a problem hiding this comment.
This is some fairly elaborate string manipulation going on, the utility of which will be opaque to anyone not intimately familiar with how the .NET class naming system works, and that also does not understand the specific issues raised in issue #101 that requires its introduction. #Resolved
There was a problem hiding this comment.
I have a feeling this is more complicated. For comparison, there's a similar case (maybe not exactly, but anyway): https://github.com/dotnet/orleans/blob/master/src/AdoNet/Orleans.Persistence.AdoNet/Storage/Provider/AdoNetGrainStorage.cs#L506. More laborius, but might be usable to extract this to a separate class/function and test a bit more. This is something that probably should be done in Orleans too, since there are similar needs. :)
Also perhaps interesting in case there's a possibility to refactor out commonalities and have a joint library: https://github.com/dotnet/orleans/blob/e78c7fa4ae302808ed30d71ebc14f1ef544a923e/src/Orleans.Core/AssemblyLoader/AssemblyLoader.cs. #Resolved
There was a problem hiding this comment.
Hi Veikko, thanks for your suggestion and for the reference. This method definitely doesn't handle every possible case for a type name (for example, it does not handle the case where the nested class is also generic). Since it is a private method, I think that would be enough for handle the cases we need it to handle. I did add some comments though, to make it easier to handle new cases if the need arises. #Resolved
There was a problem hiding this comment.
I might be tempted to insert a link though to the MSDN docs for this at the GetType, since we're attempting to read a (subset) of the language. Plus maybe document the limitations of this method.
Edit: um I'm blind, I see you already did that. :P
In reply to: 187414843 [](ancestors = 187414843)
| private static class Mkl | ||
| { | ||
| private const string DllName = "Microsoft.ML.MklImports.dll"; | ||
| private const string DllName = "Microsoft.MachineLearning.MklImports.dll"; |
There was a problem hiding this comment.
Microsoft.MachineLearning [](start = 44, length = 25)
Is this related to the issue raised?
Given that we decided on Microsoft.ML vs. the more verbose Microsoft.MachineLearning, should not that DLL change its name, rather than the other way around? #Resolved
There was a problem hiding this comment.
I had the same question but I also feel this should be made as a separate change and we should open an issue for this.
In reply to: 187159434 [](ancestors = 187159434)
| int backTickIndex = baseName.LastIndexOf('`'); | ||
| int dotIndex = baseName.LastIndexOf('.'); | ||
| if (backTickIndex < 0) | ||
| name += baseName.Substring(dotIndex + 1); |
There was a problem hiding this comment.
name += baseName.Substring(dotIndex + 1); [](start = 15, length = 42)
Can dotindex be -1?
There was a problem hiding this comment.
If it somehow cannot be (e.g., I might expect that everything would at least have a namespace), we should add a nice little Contracts.Assert to document that..
In reply to: 187485225 [](ancestors = 187485225)
There was a problem hiding this comment.
I think not, because the full name always begins with the namespace.
In reply to: 187485225 [](ancestors = 187485225)
| name += string.Join("", splitNames); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is great, thanks for doing this. May be put an example of how a name is generated from this control flow.
| name += fullTypeName.Substring(fullTypeName.LastIndexOf(dim) + 1); ; | ||
| for (int i = 1; i < nestedNames.Length; i++) | ||
| name += nestedNames[i]; |
There was a problem hiding this comment.
I'm assuming you are adding this to have unique names but I feel these names should go before the generated name and is it possible to only add them when we detect there is a name collision? We add them as needed and if we run out of them then we can add rolling numbers.
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks @yaeldekel for fixing code generation for these types.
… '+' signs in them. (dotnet#102) * Handle generic types and types with multiple '+' signs in them. * Delete commented code. * Address PR comments. * Skip codegen unit test. * Add example to comment. * Assert that the full type name has a dot in it. * Trigger build.
… '+' signs in them. (dotnet#102) * Handle generic types and types with multiple '+' signs in them. * Delete commented code. * Address PR comments. * Skip codegen unit test. * Add example to comment. * Assert that the full type name has a dot in it. * Trigger build.
The C# code generator needs to be able to handle types that are nested inside generic classes. It should also handle types that are nested in nested classes.