Reduce generated invocations to FindNameScope
#15370
Conversation
|
You can test this PR using the following package version. |
| @@ -22,7 +22,8 @@ namespace Sample.App | |||
| AvaloniaXamlLoader.Load(this); | |||
| } | |||
|
|
|||
| UserNameTextBox = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBox>("UserNameTextBox"); | |||
| var __thisNameScope__ = this.FindNameScope(); | |||
There was a problem hiding this comment.
Is the prefix and suffic double underscores following some convention I'm unaware of? Otherwise, naming conventions are important and this probably shouldn't have been done. thisNameScope would be fine IMO
There was a problem hiding this comment.
My intention was to avoid any potential conflicts, hence the uncommon naming. This is still only a generated local so it won't ever leak outside, and thus barely matters imo
There was a problem hiding this comment.
Chance of conflicts is pretty low, but not zero.
With IL code generation we usually generate some obscure names like "!CompiledXaml" that can't be really compiled via C#. It's not possible with source generators though.
There was a problem hiding this comment.
I was thinking this is a local variable so why even worry about it. But there is risk of conflicts with control names. That said, we probably should have a standard for this. I have seen two underscore prefixes used for generated variables but never as suffixes too.
There was a problem hiding this comment.
Accessing instance members with this would solve the problem, I think. No special naming would be needed. Probably static members would have to be qualified with the current type name as well.
There was a problem hiding this comment.
@jp2masa I had the same thought after my comment above and was going to check it later to make sure. But if you are thinking the same that's probably a better solution.
There was a problem hiding this comment.
I don't know why we're arguing over a generated local's name that has already been merged. This is too much mental effort for something that literally doesn't impact users.
There was a problem hiding this comment.
Well, it was merged awful quickly. I try to read through what I can just to be aware. I agree it's low impact and can be changed later as/if required. I wouldn't shut down discussion like this though, long-term it's probably useful especially as we add more generated code.
There was a problem hiding this comment.
Since a local/parameter always has priority, imo it's usually best in source generators to make sure member references are correctly qualified, and leave the local/parameters "unescaped".
e.g. this.Property = someLocal; instead of Property = __someLocal__
I agree with Robloo, there's no arguing here, just discussion that could help future PRs.
maxkatz6
added
the
backport-candidate-11.1.x
maxkatz6
added
backported-11.1.x
and removed
backport-candidate-11.1.x
What does the pull request do?
Reduces the invocations of the
FindNameScopemethod onthison generatedInitializeComponent. See #15365What is the current behavior?
For every named control, the generated code invokes
FindNameScopewithinInitializeComponent.What is the updated/expected behavior with this PR?
The
FindNameScopemethod is invoked only once if there are any named controls that would use this method.Checklist
Breaking changes
None, assuming
FindNameScopedoes not return different results during the execution ofInitializeComponent.Fixed issues
Fixes #15365