Skip to content
This repository was archived by the owner on Nov 1, 2020. It is now read-only.

Get Array1 to work#36

Merged
schellap merged 2 commits into
dotnet:masterfrom
schellap:master
Oct 8, 2015
Merged

Get Array1 to work#36
schellap merged 2 commits into
dotnet:masterfrom
schellap:master

Conversation

@schellap
Copy link
Copy Markdown

@schellap schellap commented Oct 7, 2015

@jkotas @MichalStrehovsky @JohnChen0

Could you please take a look? Both bounds check exception is thrown in the invalid case and the value is incremented for the array in the valid case. The test will return exit code 100 if passes.

Summary:
Add NewArr1 helper to the ReadyToRunHelper.
Created JitHelpers for static helpers like throw/rngchk.
Patching up the method table data for Array.
Add return value for Main and added test Jagged.

@MichalStrehovsky
Copy link
Copy Markdown
Member

Why do the commits have "unknown" as author?

Comment thread src/ILToNative/repro/Program.cs Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we should be updating this. Everyone has their own thing here and it will just cause unnecessary merge conflicts.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

If we add tests here, it will also make sure that we don't break what we built incrementally. I felt that a merge conflict is a smaller price to pay than debugging breaks.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We will have a better plan for tests hopefully soon - leveraging CoreCLR test bed.

We do not have a team policy that everybody has to run this project before checking in right now; and everybody tends to have their own random thing here anyway ... so adding more stuff here does not ensure that things won't break.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I removed this diff.

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Oct 8, 2015

LGTM otherwise

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Oct 8, 2015

One more nit: could you please delete "(with CR feedback)" from the commit description, and make it imperative (first word is a verb, without 'ing'). Check http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html on how to write good commit messages.

 - Add NewArr1 helper to the ReadyToRunHelper.
 - Create JitHelpers for static helpers: ex: RngChkFail.
 - Patch up the method table data for Array.
 - Throws bounds check for invalid access.
@schellap schellap changed the title Getting Array1 to work Get Array1 to work Oct 8, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants