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

Fixing problems as reported by clang-check. #1889

Merged
merged 3 commits into from Dec 18, 2015
Merged

Conversation

sithhell
Copy link
Member

@sithhell sithhell commented Dec 1, 2015

- Fixing memory leaks
- Removing unused variables
- Fixing potential division by zero bugs

    - Fixing memory leaks
    - Removing unused variables
    - Fixing potential division by zero bugs
@sithhell sithhell added this to the 0.9.12 milestone Dec 1, 2015
@@ -125,7 +125,8 @@ struct HPX_EXPORT big_boot_barrier : boost::noncopyable

void add_thunk(util::unique_function_nonser<void()>* f)
{
thunks.push(f);
while(!thunks.push(f))
; // Wait until succesfully pushed ...
Copy link
Member

Choose a reason for hiding this comment

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

This should time out with an error and not sit indefinitely.

Copy link
Member Author

Choose a reason for hiding this comment

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

In hpx/runtime/agas/big_boot_barrier.hpp:

@@ -125,7 +125,8 @@ struct HPX_EXPORT big_boot_barrier : boost::noncopyable

 void add_thunk(util::unique_function_nonser<void()>* f)
 {
  •    thunks.push(f);
    
  •    while(!thunks.push(f))
    
  •        ; // Wait until succesfully pushed ...
    

This should time out with an error and not sit indefinitely.

push is guaranteed to succeed eventually, iiuc. Should we really time out
and throw here?

Copy link
Member

Choose a reason for hiding this comment

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

push is guaranteed to succeed eventually, iiuc. Should we really time out and throw here?

Not sure. I usually try to avoid things like that. Smells like a live-lock...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think having an additional exponential back off mechanism might help to improve the picture.

@hkaiser
Copy link
Member

hkaiser commented Dec 12, 2015

@sithhell Can this be merged now?

@sithhell
Copy link
Member Author

All comments have been addressed.

@hkaiser
Copy link
Member

hkaiser commented Dec 18, 2015

LGTM, thanks!

sithhell added a commit that referenced this pull request Dec 18, 2015
Fixing problems as reported by clang-check.
@sithhell sithhell merged commit 0dfb8ee into master Dec 18, 2015
@sithhell sithhell deleted the clang_analyzer_fixes branch December 18, 2015 08:22
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

3 participants