Simple formatting cleanup over Uri #31874

MihaZupan · 2020-02-06T17:42:59Z

Usings, inline out params, indentation, discards etc (these aren't all such cases in Uri, I am just trying to split my changes into more reviewable chunks).

No behavioral changes.

scalablecory · 2020-02-06T19:00:45Z

src/libraries/System.Private.Uri/src/System/UncNameHelper.cs

@@ -49,7 +37,7 @@ internal static string ParseCanonicalName(string str, int start, int end, ref bo
        //
        // Assumption is the caller will check on the resulting name length
        // Remarks:  MUST NOT be used unless all input indexes are verified and trusted.
-        internal static unsafe bool IsValid(char* name, ushort start, ref int returnedEnd, bool notImplicitFile)
+        public static unsafe bool IsValid(char* name, ushort start, ref int returnedEnd, bool notImplicitFile)


This is the only one I'm not sure of; I've seen this inconsistency elsewhere and not sure if we have a recommendation.

not sure if we have a recommendation.

It might not be written down but the recommendation is to use 'internal' for classes that need to be used throughout the assembly (i.e. System.Private.Uri) but then use only 'public' or 'private' for methods, properties, etc.

stephentoub · 2020-02-06T21:07:37Z

src/libraries/System.Private.Uri/tests/ExtendedFunctionalTests/UriRelativeResolutionTest.cs


            Uri resultUri = new Uri(baseUri, testUri);

-            throwAway = resultUri.Port; // For Debugging.
+            _ = resultUri.Port; // For Debugging.


What does the // For Debugging comment mean? Is this functionally necessary? It sounds like it is for the port one above.

The code pattern above was probably used because there is a side-affect of doing a 'get' on the .Port property.

Will the behavior be the same if you use "_"?

As far as I am aware, the behavior is the same as assigning to a dummy local, but it makes it obvious which values aren't used and avoids analyzer warnings

@MihaZupan, I'm still not clear on what the "For Debugging" comment is referring to?

Thinking about it more, I think this is a possible reason why it was used:

Uri is currently parsed in 3 phases (lazy):

In the ctor - identify the type of Uri, check the authority and other basic things to determine if the Uri is valid

Determine offsets for UserInfo, Host, Path, parse the port value

Determine the rest of offsets (Query, Fragment) and check if parts are in canonical form

This can make debugging harder. For example when debugging in Visual Studio, you must be careful to explicitly turn off the feature showing Locals - you can be stepping through the ctor and VS tries to evaluate properties like Query which trigger full parsing (before the initial ctor parsing even started), thus corrupting the Uri state under the debugger.
I've hit this when debugging Uri before and it took me a few hours to realize what was happening.

If the original author was debugging this code, they could have used code like

_ = uri.Port Debugger.Launch();

to force more parsing before attaching the debugger, but forgot to remove it afterwards.

I've removed it.

src/libraries/System.Private.Uri/tests/FunctionalTests/IriTest.cs

src/libraries/System.Private.Uri/tests/FunctionalTests/UriBuilderParameterTest.cs

src/libraries/System.Private.Uri/tests/FunctionalTests/UriIpHostTest.cs

src/libraries/System.Private.Uri/tests/FunctionalTests/UriMailToTest.cs

src/libraries/System.Private.Uri/tests/FunctionalTests/UriParameterValidationTest.cs

src/libraries/System.Private.Uri/tests/FunctionalTests/UriRelativeResolutionTest.cs

MihaZupan · 2020-02-07T17:50:18Z

Made a second pass over test files, please re-review

davidsh · 2020-02-07T18:41:06Z

Made a second pass over test files, please re-review

I've noticed there are still comments on this PR that don't have answers from you. It would be helpful if you could review all of those, type in appropriate comments ("Fixed", "Not really a problem", etc.) and then click "Resolve" on the issue. That helps reviewers more quickly be able to re-review a PR. Thanks!

src/libraries/System.Private.Uri/tests/FunctionalTests/IriTest.cs

src/libraries/System.Private.Uri/tests/FunctionalTests/UriParserTest.cs

stephentoub

Other than my remaining comments, LGTM.

MihaZupan added the area-System.Net label Feb 6, 2020

MihaZupan added this to the 5.0 milestone Feb 6, 2020

MihaZupan requested review from stephentoub and a team February 6, 2020 17:42

scalablecory reviewed Feb 6, 2020

View reviewed changes

scalablecory approved these changes Feb 6, 2020

View reviewed changes