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

Fix Segmentation fault on foreach_datapar_zipiter #5281

Merged
merged 10 commits into from Apr 16, 2021

Conversation

srinivasyadav18
Copy link
Contributor

Proposed Changes

  • The len variable in initialised to count but did not use rightly in the loop.
  • replace count with len, must be a typo.

Any background context you want to provide?

#5235 (comment)

@jenkins-cscs
Copy link
Collaborator

Can one of the admins verify this patch?

@msimberg
Copy link
Contributor

msimberg commented Apr 8, 2021

retest

@srinivasyadav18
Copy link
Contributor Author

Here a2d7511, is_data_aligned is logically correct and is used in correct sense, but the convention was reverse, it returns false when data is aligned. I now changed it to return true when is aligned.

@msimberg
Copy link
Contributor

msimberg commented Apr 9, 2021

retest

@msimberg msimberg added this to the 1.7.0 milestone Apr 9, 2021
Co-authored-by: Hartmut Kaiser <hartmut.kaiser@gmail.com>
@hkaiser
Copy link
Member

hkaiser commented Apr 10, 2021

why do we the need of first value (false/true) in sequencer, when we are using only from 1 to sizeof...(Is) + 1? in std::all_of(), why can't we directly use from 0 to sizeof...(Is) without the first value in sequencer?,

We definitely could. Be careful to account for a zero length sequence, however.

@srinivasyadav18
Copy link
Contributor Author

why do we the need of first value (false/true) in sequencer, when we are using only from 1 to sizeof...(Is) + 1? in std::all_of(), why can't we directly use from 0 to sizeof...(Is) without the first value in sequencer?,

We definitely could. Be careful to account for a zero length sequence, however.

@hkaiser Okay, then the existing solution is a better one. I did not realise about zero length sequence.
And could you please suggest anything about this #5281 (comment)
After adding or editing the tests, most probably there are no further issues related to datapar.

@srinivasyadav18
Copy link
Contributor Author

@msimberg @hkaiser I have made the changes! Please mention if any further changes are required :-).

@msimberg
Copy link
Contributor

@srinivasyadav18 I'll merge this into the other branch now, let's do further changes if needed there (I have nothing in mind right now).

@msimberg msimberg merged commit 7e89577 into STEllAR-GROUP:fixing_datapar Apr 16, 2021
@srinivasyadav18 srinivasyadav18 deleted the fixing_datapar branch April 17, 2021 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants