Annotate more public syntax APIs#44266
Conversation
src/Compilers/CSharp/Portable/Syntax/AnonymousMethodExpressionSyntax.cs
Outdated
Show resolved
Hide resolved
|
Done review pass (commit 2) #Resolved |
|
@333fred Please take another look when you get a chance. Thanks |
|
Thanks Fred! @dotnet/roslyn-compiler for a second review. Cheers |
| /// The value for a non-array constant. | ||
| /// </summary> | ||
| public object? Value | ||
| public object Value |
There was a problem hiding this comment.
public object Value [](start = 8, length = 19)
Isn't this null for the constant null? #Closed
There was a problem hiding this comment.
| Debug.Assert(lambdaBody.Parent is object); | ||
| var lambda = lambdaBody.Parent; | ||
| if (lambda.Kind() == SyntaxKind.ArrowExpressionClause) | ||
| if (lambda.IsKind(SyntaxKind.ArrowExpressionClause)) |
There was a problem hiding this comment.
IsKind( [](start = 23, length = 7)
Just curious - why change these? #Resolved
There was a problem hiding this comment.
The lambda.Kind() on line 63 would warn. Then I think I just aligned the check on line 57 for stylistic consistency.
In reply to: 441899787 [](ancestors = 441899787)
| else | ||
| { | ||
| visited = this.VisitListElement((TNode)item.AsNode()); | ||
| visited = this.VisitListElement((TNode?)item.AsNode()); |
There was a problem hiding this comment.
(TNode?)item.AsNode() [](start = 60, length = 21)
Can we use node here? #Closed
| } | ||
|
|
||
| internal bool TryDecodeValue<T>(SpecialType specialType, [MaybeNull][NotNullWhen(returnValue: true)] out T value) | ||
| internal bool TryDecodeValue<T>(SpecialType specialType, [MaybeNull][NotNullWhen(true)] out T value) |
There was a problem hiding this comment.
[NotNullWhen(true)] [](start = 76, length = 19)
What if value == null? #Closed
| if (_type.SpecialType == specialType || (_type.TypeKind == TypeKind.Enum && specialType == SpecialType.System_Enum)) | ||
| { | ||
| value = (T)_value!; | ||
| Debug.Assert(_value is object); |
There was a problem hiding this comment.
Debug.Assert(_value is object); [](start = 16, length = 31)
Don't we get here with a constant null? #Closed
|
|
||
| Debug.Assert(args.Length <= 2); | ||
| message = (string)args[0].ValueInternal; | ||
| Debug.Assert(args[0].ValueInternal is object); |
There was a problem hiding this comment.
Debug.Assert(args[0].ValueInternal is object); [](start = 16, length = 46)
Could the value be [Obsolete(null)]? #Closed
|
|
||
| message = (string)args[0].ValueInternal; | ||
| isError = ((int)args[1].ValueInternal == 1); | ||
| Debug.Assert(args[0].ValueInternal is object); |
There was a problem hiding this comment.
Debug.Assert(args[0].ValueInternal is object) [](start = 16, length = 45)
Could the value be [Deprecated(null, ...)]? #Closed
| { | ||
| /// <summary> | ||
| /// Returns the System.String that represents the current TypedConstant. | ||
| /// For testing and debugging only |
There was a problem hiding this comment.
For testing and debugging only [](start = 12, length = 30)
This method is part of the public API so I don't think we can state how this method is used. #Closed
There was a problem hiding this comment.
|
|
||
| // Unable to decode the enum constant, just display the integral value | ||
| return constant.ValueInternal.ToString(); | ||
| Debug.Assert(constant.ValueInternal is object); |
There was a problem hiding this comment.
Debug.Assert(constant.ValueInternal is object) [](start = 12, length = 46)
Why assert rather than using !? #ByDesign
There was a problem hiding this comment.
Because this is the result of a check in a calling method (if (constant.IsNull) in ToCSharpString), but not in the present method.
In reply to: 443063994 [](ancestors = 443063994)
|
|
||
| // Unable to decode the enum constant, just display the integral value | ||
| return constant.ValueInternal.ToString(); | ||
| Debug.Assert(constant.ValueInternal is object); |
There was a problem hiding this comment.
Debug.Assert(constant.ValueInternal is object); [](start = 12, length = 47)
Why assert rather than using !? #ByDesign
There was a problem hiding this comment.
| message = (string)args[0].ValueInternal; | ||
| isError = ((int)args[1].ValueInternal == 1); | ||
| Debug.Assert(args[1].ValueInternal is object); | ||
| message = (string?)args[0].ValueInternal!; |
There was a problem hiding this comment.
! [](start = 56, length = 1)
! is incorrect. #Closed
| /// </param> | ||
| /// <returns>Normalized XML document file path or null if not found.</returns> | ||
| public override string? ResolveReference(string path, string? baseFilePath) | ||
| public override string? ResolveReference(string? path, string? baseFilePath) |
There was a problem hiding this comment.
string? path [](start = 49, length = 12)
Why is path optional? It looks like the uses of path support null values, but allowing null seems incorrect given the parameter description where it is a path to a source file. #Resolved
There was a problem hiding this comment.
I'm trying to capture/document the existing behavior of this method.
The test I added does exercise this path. I can adjust the caller instead (don't call if path is null), but result is same.
In reply to: 443154834 [](ancestors = 443154834)
| /// <param name="baseFilePath">Path of the source file that contains the <paramref name="path"/> (may also be relative), or null if not available.</param> | ||
| /// <returns>Path to the XML artifact, or null if the file can't be resolved.</returns> | ||
| public abstract string? ResolveReference(string path, string? baseFilePath); | ||
| public abstract string? ResolveReference(string? path, string? baseFilePath); |
There was a problem hiding this comment.
string? path [](start = 49, length = 12)
Why is path optional? #Resolved
No description provided.