Skip to content

coreclr: Automatic port of triple slash from System #2729

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

Merged
merged 21 commits into from
Jul 24, 2019

Conversation

carlossanlop
Copy link
Contributor

Fixed a bug in the porting tool and found additional coreclr comments that could be automatically ported from this area.
Not sure who's the proper owner of these APIs, but the comments are simple enough and I tried to correct them as much as possible.
Adding @mairaw @rpetrusha for language review.

@carlossanlop carlossanlop requested review from mairaw and rpetrusha July 8, 2019 23:44
@carlossanlop carlossanlop self-assigned this Jul 8, 2019
@carlossanlop carlossanlop added 🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 new-content Indicates PRs that contain new articles waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews labels Jul 8, 2019
<summary>Calculate the offset from the start using the giving collection length.</summary>
<returns>The offset.</returns>
<remarks>
<format type="text/markdown">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This remark was a bit difficult to understand, especially the 3rd sentence, so I left it untouched. Can I get some help to properly rewrite it?
There's another similar remark in another API in this same PR.

Copy link
Contributor Author

@carlossanlop carlossanlop Jul 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I gave it a try. Hopefully it looks a bit better.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't think it's clear. There's nothing that ties length to the length of a collection. And the source ignores the length argument if the index is from the start and just returns the original index:

        public int GetOffset(int length)
        {
            int offset = _value;
            if (IsFromEnd)
            {
                // offset = length - (~value)
                // offset = length + (~(~value) + 1)
                // offset = length + value + 1
 
                offset += length + 1;
            }
            return offset;
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Help us, @tarekgh, you're our only hope.
Can you help clarify this remark?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I understood from Tarek's explanation is that length is only considered when retrieving the offset from the end. You are correct, the method ignores the length if it's from the start.

@carlossanlop
Copy link
Contributor Author

I removed the System.GC* APIs from this PR and moved them to their own PR: #2759

I added two extra files I found in System: SequencePosition and AggregateException.

Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed this up to the Range.Equals method, @carlossanlop. I'll continue the review tomorrow.

Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've finished reviewing this PR, @carlossanlop. I realize that I created suggestions for the first set of types rather than editing the documentation directly. (I also did that from String because I could never load the file successfully in my browser.) For the remaining types, I made changes directly in the document, so please review them.

@rpetrusha rpetrusha removed the waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews label Jul 18, 2019
@carlossanlop carlossanlop requested a review from tarekgh July 19, 2019 00:36
@carlossanlop
Copy link
Contributor Author

@tarekgh it seems you added the GetOffset function for Index. Would you mind helping us clarify the remark that you wrote for that method?

@carlossanlop
Copy link
Contributor Author

carlossanlop commented Jul 19, 2019

@rpetrusha I can't commit your suggestions in a batch. Can I ask you a favor? Can you please try it yourself? I am getting an error saying "the diff has changed" whenever I try to commit the batch.
I was merging all of them except the one where I left a question for Tarek.

Edit:

Nevermind. I figured out the root cause: The String.xml file is so large (14433 lines!) that GitHub is unable to merge suggestions directly here, which is why I was seeing the error "the diff has changed".

I had to merge each file's suggestions individually to find the root cause, then merge the String.xml changes via my enlistment.

Anyway, problem solved!

carlossanlop and others added 4 commits July 18, 2019 22:43
Co-Authored-By: Ron Petrusha <[email protected]>
Co-Authored-By: Ron Petrusha <[email protected]>
Co-Authored-By: Ron Petrusha <[email protected]>
@carlossanlop
Copy link
Contributor Author

@rpetrusha I addressed the last comments based on Tarek's explanation of Index.GetOffset. Can you please take a look in case it still needs some more rewording? I am hoping it's clearer now.

@carlossanlop carlossanlop added the changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review label Jul 24, 2019
@carlossanlop carlossanlop requested a review from rpetrusha July 24, 2019 00:15
Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made some minor changes, and the GetOffset method still is not clear, but this is ready to merge once the build completes successfully, @carlossanlop

@rpetrusha rpetrusha added verify-build-before-merge and removed changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review labels Jul 24, 2019
@carlossanlop
Copy link
Contributor Author

Do you mind if we open a separate issue to review the GetOffset documentation? @rpetrusha

@mairaw mairaw added this to the July 2019 milestone Jul 24, 2019
@rpetrusha
Copy link

No, a separate PR is fine, @carlossanlop. It would be good to get this merged -- although it never seems to build successfully...

@rpetrusha rpetrusha merged commit f1abff3 into dotnet:master Jul 24, 2019
@carlossanlop carlossanlop deleted the clr_System branch July 30, 2019 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 new-content Indicates PRs that contain new articles
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants