-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Use "file local types" instead of "file types" #62514
Conversation
{ | ||
if (!possibleFileType.IsFile) | ||
if (!possibleFileLocalType.IsFile) |
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.
.IsFile
Is there a missing rename here?
roslyn/src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs
Line 829 in 7fc34f9
internal bool IsFile => HasFlag(DeclarationModifiers.File); |
I see the implementation is about the modifier, but I think the API itself is meant to be about the language concept?
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.
Renamed to IsFileLocal.
@@ -122,6 +122,6 @@ public override string MetadataName | |||
|
|||
public bool IsSerializable => false; | |||
|
|||
public bool IsFile => Modifiers.IsFile; | |||
public bool IsFileLocal => Modifiers.IsFile; |
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.
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.
I don't feel strongly about this here. It's an internal API so don't think there's a rush to make it happen.
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.
@RikkiGibson But that applies to the public INamedTypeSymbol.IsFileLocal
property as well?
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.
good point (: I missed that this was an implementation of INamedTypeSymbol.IsFileLocal
.
IMO, I think IsFileLocal
is better because we anticipate generalizing the file-local concept to non-type members. If this happens, we might end up introducing API like ISymbol.IsFileLocal
and then the INamedTypeSymbol.IsFileLocal
would shadow it.
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.
Oops, I could have placed the comment in a better location ;-)
we might end up introducing API like ISymbol.IsFileLocal and then the INamedTypeSymbol.IsFileLocal would shadow it.
Makes sense. Thanks
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 Thanks (iteration 2)
related to #62254 and #60819
We use "file local types" to refer to the language concept, e.g. in
INamedTypeSymbol.IsFileLocal
. When we are only asking whether a declaration literally has thefile
modifier we still sayIsFile
, e.g. inSyntaxGenerator
.