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

common/Mutex.cc: fixed the error in comment #16214

Merged
merged 1 commit into from Jul 9, 2017

Conversation

Projects
None yet
2 participants
@liupan1111
Contributor

liupan1111 commented Jul 7, 2017

The type of pthread_mutex_lock is PTHREAD_MUTEX_DEFAULT, but it is commented as PTHREAD_MUTEX_NORMAL, and may make the reader misunderstand.

Signed-off-by: Pan Liu wanjun.lp@alibaba-inc.com

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jul 8, 2017

retest this please

@joscollin joscollin changed the title from Mutex: fixed the error in comment to common/Mutex.cc: fixed the error in comment Jul 8, 2017

@@ -61,7 +61,7 @@ Mutex::Mutex(const std::string &n, bool r, bool ld,
_register();
}
else {
// If the mutex type is PTHREAD_MUTEX_NORMAL, deadlock detection
// If the mutex type is PTHREAD_MUTEX_DEFAULT, deadlock detection

This comment has been minimized.

@joscollin

joscollin Jul 8, 2017

Member

In this comment, PTHREAD_MUTEX_NORMAL seems to be correct.

Please read: http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_mutex_lock.html.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jul 8, 2017

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jul 8, 2017

@joscollin pthread_mutex_init(&_m, NULL); is called in this "else" branch. So may be the whole comment should be replaced, not only the type name.

@joscollin

This comment has been minimized.

Member

joscollin commented Jul 8, 2017

@liupan1111 Yes, you are right. Considering the NULL value passed to pthread_mutex_init(), It should be PTHREAD_MUTEX_DEFAULT.

The pthread_mutex_init() function shall initialize the mutex referenced by mutex with attributes specified by attr. If attr is NULL, the default mutex attributes are used;

The whole comment should be replaced.

@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jul 8, 2017

yes, current comment really confuse

common/Mutex.cc: fixed the error in comment
Signed-off-by: Pan Liu <wanjun.lp@alibaba-inc.com>
@liupan1111

This comment has been minimized.

Contributor

liupan1111 commented Jul 8, 2017

@joscollin done, thanks.

// lock the mutex results in undefined behavior. Attempting to unlock the
// mutex if it was not locked by the calling thread results in undefined
// behavior. Attempting to unlock the mutex if it is not locked results in
// undefined behavior.

This comment has been minimized.

@joscollin

joscollin Jul 8, 2017

Member

I think the last sentence is a repetition. It is repeated in the source text (from where you copied) too. Please remove it.

This comment has been minimized.

@liupan1111

liupan1111 Jul 8, 2017

Contributor

@joscollin I am afraid the last two sentences are different ....

  1. Attempting to unlock the mutex if it was not locked by the calling thread results in undefined behavior.
  2. Attempting to unlock the mutex if it is not locked results in undefined behavior.

This comment has been minimized.

@joscollin

joscollin Jul 8, 2017

Member

@liupan1111 What is the difference you noticed ? Please clarify.

If Sentence2 is generalizing the locked state, then Sentence1 can be removed.

This comment has been minimized.

@liupan1111

liupan1111 Jul 8, 2017

Contributor

you Could refer the comment of PTHREAD_MUTEX_ERRORCHECK above in line 50. I think we do not need to consider too much at This place.

@joscollin joscollin merged commit 8fbe8b8 into ceph:master Jul 9, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@liupan1111 liupan1111 deleted the liupan1111:wip-fix-mutex branch Jul 9, 2017

@joscollin

This comment has been minimized.

Member

joscollin commented Jul 9, 2017

I'm merging the PR, as this change doesn't affect the functionality in any way. Just Comment changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment