Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(416)

Issue 3540041: Add --without-thread-safety support for platforms without thread libraries.

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 4 months ago by Jisi Liu
Modified:
3 years, 4 months ago
Reviewers:
kenton
CC:
kenton+urgent_google.com
Base URL:
http://protobuf.googlecode.com/svn/trunk/
Visibility:
Public.

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+43 lines, -3 lines) Patch
M configure.ac View 1 chunk +10 lines, -0 lines 2 comments Download
M src/google/protobuf/stubs/common.cc View 2 chunks +12 lines, -0 lines 1 comment Download
M src/google/protobuf/stubs/once.h View 3 chunks +16 lines, -2 lines 1 comment Download
M src/google/protobuf/stubs/once_unittest.cc View 3 chunks +5 lines, -1 line 0 comments Download

Messages

Total messages: 2
Jisi Liu
The dummy mutex and google_once_init didn't pass the multithread blocking test, so I #ifndef it ...
3 years, 4 months ago #1
kenton
3 years, 4 months ago #2
Read the last comment first...

http://codereview.appspot.com/3540041/diff/1/configure.ac
File configure.ac (right):

http://codereview.appspot.com/3540041/diff/1/configure.ac#newcode46
configure.ac:46: [do not provide thread safety. use this if the platform does
not have thread library.])])
"use thread-unsafe code if no supported threading library is available. 
Otherwise, protobuf will not compile without a threading library."

http://codereview.appspot.com/3540041/diff/1/configure.ac#newcode50
configure.ac:50: AC_DEFINE(PROTOBUF_WITHOUT_THREAD_SAFETY, 1, [define if without
thread safety support.])
"define if thread-safety is not needed."

http://codereview.appspot.com/3540041/diff/1/src/google/protobuf/stubs/common.cc
File src/google/protobuf/stubs/common.cc (right):

http://codereview.appspot.com/3540041/diff/1/src/google/protobuf/stubs/common...
src/google/protobuf/stubs/common.cc:324: struct Mutex::Internal {
Hmm, maybe have:
  bool locked;

Then have Lock() abort the process if it is called when this is already true.

This way if someone does manage to use threads, at least we fail loudly and
obviously.

http://codereview.appspot.com/3540041/diff/1/src/google/protobuf/stubs/once.h
File src/google/protobuf/stubs/once.h (right):

http://codereview.appspot.com/3540041/diff/1/src/google/protobuf/stubs/once.h...
src/google/protobuf/stubs/once.h:77: #include "config.h"
Unfortunately, you can't include config.h here because once.h is a public
header, and config.h is not.  config.h #defines a lot of poorly-named macros
(and these macro names are apparently chosen by autotools), so it would not be
safe to include in client code.

I'm not really sure how to deal with this.  I suppose the configure script would
have to generate another header which provides this information while avoiding
bad macros, and we'd make that one public.  But that seems like it's going to be
ugly.

Another possibility would be to require the user to define a particular macro
themselves...  but that seems ugly too.

Maybe we should punt this issue.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1278:e6ce13d99bf5