-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
Allow switching to static readonly properties instead of constants #64
Comments
Hi. I've been looking at this and thinking through how to approach it. The main thing i'm noticing is the implementation may be cleaner with some code reuse. It looks like several of the libraries use constants so they would all needs some updates. The shared code would primarily offer shared logic to look for the csproj property or maybe an assembly attribute Also, since you mentioned the generated code being internal by default, it might be good to update the templates to reflect that. From the testing I did I mostly see no access restriction specified or public. Let me know what you think. |
Just to clarify: should they be fields... public static readonly string Foo = "Bar"; ...properties... public static string Foo => "Bar"; ... or properties with backing fields? public static string Foo { get; } = "Bar"; Quick notes:
Given the above, I'd go for fields; is there something I overlooked? |
Notes:
|
Since the auto-properties are (I'm pretty sure nearly always) optimized away, I'm tempted to say the default should be that. Perhaps the generated class visibility should determine one or the other? If it's default (internal), use fields, if it's public, use props? And perhaps this becomes a |
On further reflection... I'd say if visibility is internal/default, they should be constants; if public, then they should be readonly properties. Constants will always have (slightly) better performance behavior than properties, and cannot be changed by reflection, so if access is only available internally, then stick to constants. No one else can depend on them anyway, and any code changes will require recompile, so it works out regardless. If public, then properties provide a (very) slight level of protection, because the property itself cannot be used to change the internal value. The value is stored in the private compiler-named backing field, so one would have to search the private fields of the class to get to it and change it. And public auto-property getter should be optimized away in most cases, so performance should be nearly perfect. |
Yep, precisely put. |
In some limited scenarios, it might be worth switching to static readonly properties (perhaps backed by the private constant) so that updates to the library providing the ThisAssembly don't require recompiling callers in order to get the updated values (since consts are embedded raw in the callsite at compile-time).
This might not be so common since ThisAssembly is (by default) internal and (mostly?) intended for internal consumption, but making it public via a partial class is definitely an option, which could lead to this unintended side-effect.
Perhaps introduce a new project-level property that the analyzer uses to switch the template used to generate code, like
ThisAssemblyUseStaticProperties
?Originally posted by @Cologler in #63
Based on some articles:
If any package has not to recompile, we may get the wrong information. For example, if the main process tries to read the version string from a hotfix dll file, it will always read the wrong value.
The text was updated successfully, but these errors were encountered: