Skip to content

Conversation

@hez2010
Copy link
Contributor

@hez2010 hez2010 commented Feb 11, 2023

Making TextMateSharp trimming and NativeAOT compatible.

image

}
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method will be inlined by the compiler so it won't affect the performance at all.

@hez2010 hez2010 changed the title Trimming compatible Trimming and NativeAOT compatible Feb 11, 2023
@danipen
Copy link
Owner

danipen commented Feb 12, 2023

@hez2010 first of all thanks for the contribution.

Question: do we need to change something in the NuGet package generation?

@danipen
Copy link
Owner

danipen commented Feb 12, 2023

I'm just wondering just for clarity when reading the code, if we can set the attribute DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All) in each class itself (GrammarDefinition, Repository, Contributes, etc ... ) instead of defining the KeepType in the InitializeAvailableGrammars.

What do you think?

@hez2010
Copy link
Contributor Author

hez2010 commented Feb 12, 2023

I'm just wondering just for clarity when reading the code, if we can set the attribute DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All) in each class itself (GrammarDefinition, Repository, Contributes, etc ... ) instead of defining the KeepType in the InitializeAvailableGrammars.

What do you think?

Unfortunately, DynamicallyAccessedMembers doesn't work in that way.

@danipen
Copy link
Owner

danipen commented Feb 12, 2023

Sorry I'm getting this in the demo application when testing you PR :-\ Not sure what's wrong.
image

@hez2010
Copy link
Contributor Author

hez2010 commented Feb 12, 2023

Sorry I'm getting this in the demo application when testing you PR :-\ Not sure what's wrong. image

It seems that you are running as a x86 process? I changed the default architecture to x64 on Windows, so on x86 the onigwrap.dll won't be copied to the artifact directory in the development environment.

@danipen
Copy link
Owner

danipen commented Feb 12, 2023

Also, it seems that the tests in the CI pipeline are hanging due to the x68/x64 issue.

@danipen
Copy link
Owner

danipen commented Feb 12, 2023

Now all passed green!

@danipen
Copy link
Owner

danipen commented Feb 12, 2023

Well I'm running "Debug" and "Any CPU" environments, but it seems that VS wants x86 not sure why, let's keep this for compatibility since x86 is compatible with both but the other way around ;-)

@danipen
Copy link
Owner

danipen commented Feb 12, 2023

Unfortunately, DynamicallyAccessedMembers doesn't work in that way.

No worries, we can leave it as it.

@hez2010
Copy link
Contributor Author

hez2010 commented Feb 12, 2023

To test NativeAOT, you can change the demo project to net7.0 and use dotnet publish -c Release -r win-x64 /p:PublishAot=true. Note that by default the x86 onigwrap.dll will be copied to the publish directory and results in architecture mismatch, you need to copy the x64 onigwrap.dll to the publish directory manually. (This only happens in the development environment and won't affect the published nuget package).

But I'm thinking that maybe we can reimplement the searcher using built-in Regex instead of oniguruma in the future?

@danipen
Copy link
Owner

danipen commented Feb 12, 2023

But I'm thinking that maybe we can reimplement the searcher using built-in Regex instead of oniguruma in the future?

It would be great to have 100% managed code and get rid of native deps!

I'm not sure how hard it would be. Currently, grammar definition files (which have been taken from Visual Studio code) talk Oniguruma dialect, so I don't know how hard would be to write a mechanism that converts those regexes to something understandable by C# built-in regex syntax.

@danipen danipen merged commit c1d4ad8 into danipen:master Feb 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants