Skip to content
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

Add resoning behind comparison to -0 and +0 #143

Merged
merged 1 commit into from
Apr 14, 2022

Conversation

VermaSh
Copy link
Contributor

@VermaSh VermaSh commented Apr 7, 2022

Signed-off-by: Shubham Verma [email protected]

@VermaSh VermaSh force-pushed the update_daa_tests branch from 5ef9f04 to ebad70c Compare April 7, 2022 18:39
@VermaSh
Copy link
Contributor Author

VermaSh commented Apr 7, 2022

@joransiu / @r30shah, @Mesbah-Alam Can I please get a review?

Copy link
Contributor

@Mesbah-Alam Mesbah-Alam left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -2519,6 +2519,19 @@ public void testShiftLeftInvalidPrecision()
@Test
public void testLeftOffsetOne()
{
/* Both expectedArray and expectedArray2 are needed because
Copy link
Contributor

Choose a reason for hiding this comment

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

@VermaSh We do not need to quote PoPs here. I think if you elaborate on behavior difference from z14 and z15 here, it would be sufficient. (I think one of the updates you posted earlier on internal issue would be more suitable).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have replaced the quote with a summary explaining the behavior change.

@joransiu
Copy link

joransiu commented Apr 7, 2022

@VermaSh: can you fixresoning toreasoning in the commit message?

@VermaSh
Copy link
Contributor Author

VermaSh commented Apr 12, 2022

I have updated the PR as suggested. This is ready for another review. @joransiu @r30shah

Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

LGTM

@VermaSh
Copy link
Contributor Author

VermaSh commented Apr 13, 2022

@Mesbah-Alam this is ready to be merged

@Mesbah-Alam
Copy link
Contributor

I don't have merge rights in OpenJ9. @pshipton - could you please merge this?

@llxia
Copy link
Contributor

llxia commented Apr 14, 2022

@Mesbah-Alam I will merge it

@llxia llxia merged commit 8fc0315 into adoptium:master Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants