-
Notifications
You must be signed in to change notification settings - Fork 85
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
Prevent usage of 'in' with non-readonly structs by a compiler error #463
Prevent usage of 'in' with non-readonly structs by a compiler error #463
Conversation
Works only with |
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.
Builds for me.
As I am not using in
keyword anyway, i'm happy to have this. It will be up to @krzychu124 to make it work on CI, or something? :)
Okay, didn't know you're on VS2017. Should I fork that analyzer and downgrade it back to VS2017 toolset, or you'd rather prefer to upgrade your IDE? |
I've upgraded my IDE 😃 |
Updated dev tools wiki page to note VS 2019 requirement: https://github.com/krzychu124/Cities-Skylines-Traffic-Manager-President-Edition/wiki/Dev-Tools I assume it works on Rider too? Will this also work on Visual Studio Code - I think @FireController1847 was using that for EVE dev. |
Clean and rebuild in both FullDebug and Release, worked on Rider with no visible effect. |
Maybe try to pass a non-readonly |
Works on Rider 2019.1.3 but only while building project/solution. I will check 2019,2 Early Access Preview |
On Rider, you need to explicitly enable Roslyn Analyzers somewhere in the settings. If I recall that correctly. |
Regarding activating the Roslyn Analyzers on Rider, post some details in a comment and I'll update wiki. I think the two Build guides will also need updating. |
I wanted to add screenshot but I couldn't upload due to timeout to GitHub Amazon S3 server 😄 In Rider 2019.2 EAP works too. |
Ah, the .dll has to be manually downloaded?
|
No, the NuGet package manager should automatically restore the packages and download the binaries. |
Looks like I built before it finished downloading lol. It's working now. I've not tested non-readonly structs and the EDIT: Could we add a unit test for testing that the analyser is working? |
This is not possible. The Analyzer will generate a compiler error, so the code won't compile and the unit test cannot be run. If the code compiles though and the test executes, that means the Analyzer doesn't work. |
@aubergine10 what do you think? Looks good to me. |
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.
LGTM 👍
Note: I've not tested to see what happens when a non-readonly struct is used with in
parameter (I don't really know how to as I've not worked with structs before). If that's not yet been tested merging would be premature.
I did a few tests private void Test(in Vector2 vec){
Debug.Log(vec); // whatever
} Call Vector2 vec = new Vector2(1f, 1f);
Test(vec); //this line should create compilation error because Vector2 is not readonly struct |
👍 |
Using a Roslyn Analyzer ReflectionIT.Analyzer.Structs, issue a compiler error when a non-
readonly
struct is passed to a method using thein
modifier.Passing a non-readonly struct with an
in
modifier can drastically decrease performance, because in the called method, the compiler will create defensive struct copies on every method call or property access of that passed struct.Closes #440.