From 05d39cb09c12608249dc0ec18c52ab21299fcb0f Mon Sep 17 00:00:00 2001 From: Stefan Becker Date: Fri, 14 Dec 2018 13:27:11 +0200 Subject: [PATCH] Prepare PR for merging - part II - remove unnecessary "struct" from TokenPool - add PAPCFUNC cast to QueryUserAPC() - remove hard-coded MAKEFLAGS string from win32 - remove useless build test CompleteNoWork - rename TokenPoolTest to TestTokenPool - add tokenpool modules to CMake build - remove unused no-op TokenPool implementation - fix errors flagged by codespell & clang-tidy - POSIX GNUmakeTokenPool should return same token - address review comments from https://github.com/ninja-build/ninja/pull/1140#discussion_r1004798172 https://github.com/ninja-build/ninja/pull/1140#pullrequestreview-195195803 https://github.com/ninja-build/ninja/pull/1140#pullrequestreview-185089255 https://github.com/ninja-build/ninja/pull/1140#issuecomment-473898963 https://github.com/ninja-build/ninja/pull/1140#issuecomment-596624610 --- CMakeLists.txt | 8 ++++- src/build.cc | 2 +- src/build_test.cc | 12 +------ src/subprocess-posix.cc | 4 +-- src/subprocess-win32.cc | 2 +- src/subprocess.h | 2 +- src/subprocess_test.cc | 26 +++++++------- src/tokenpool-gnu-make-posix.cc | 45 ++++++++++++++--------- src/tokenpool-gnu-make-win32.cc | 36 ++++++++++--------- src/tokenpool-gnu-make.cc | 63 +++++++++++++++++---------------- src/tokenpool-gnu-make.h | 6 ++-- src/tokenpool-none.cc | 22 ------------ src/tokenpool.h | 2 +- src/tokenpool_test.cc | 22 ++++++++---- 14 files changed, 125 insertions(+), 127 deletions(-) delete mode 100644 src/tokenpool-none.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index 98f7948b77..1a40a88f96 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -118,6 +118,7 @@ add_library(libninja OBJECT src/state.cc src/status.cc src/string_piece_util.cc + src/tokenpool-gnu-make.cc src/util.cc src/version.cc ) @@ -129,13 +130,17 @@ if(WIN32) src/msvc_helper_main-win32.cc src/getopt.c src/minidump-win32.cc + src/tokenpool-gnu-make-win32.cc ) # Build getopt.c, which can be compiled as either C or C++, as C++ # so that build environments which lack a C compiler, but have a C++ # compiler may build ninja. set_source_files_properties(src/getopt.c PROPERTIES LANGUAGE CXX) else() - target_sources(libninja PRIVATE src/subprocess-posix.cc) + target_sources(libninja PRIVATE + src/subprocess-posix.cc + src/tokenpool-gnu-make-posix.cc + ) if(CMAKE_SYSTEM_NAME STREQUAL "OS400" OR CMAKE_SYSTEM_NAME STREQUAL "AIX") target_sources(libninja PRIVATE src/getopt.c) # Build getopt.c, which can be compiled as either C or C++, as C++ @@ -224,6 +229,7 @@ if(BUILD_TESTING) src/string_piece_util_test.cc src/subprocess_test.cc src/test.cc + src/tokenpool_test.cc src/util_test.cc ) if(WIN32) diff --git a/src/build.cc b/src/build.cc index 0763dd5cf6..4bf5557ac9 100644 --- a/src/build.cc +++ b/src/build.cc @@ -467,7 +467,7 @@ struct RealCommandRunner : public CommandRunner { // copy of config_.max_load_average; can be modified by TokenPool setup double max_load_average_; SubprocessSet subprocs_; - TokenPool *tokens_; + TokenPool* tokens_; map subproc_to_edge_; }; diff --git a/src/build_test.cc b/src/build_test.cc index 0ef6ece0a2..4d34066866 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -4406,7 +4406,7 @@ struct BuildTokenTest : public BuildTest { void ExpectWaitForCommand(int count, ...); private: - void EnqueueBooleans(vector& booleans, int count, va_list ao); + void EnqueueBooleans(vector& booleans, int count, va_list ap); }; void BuildTokenTest::SetUp() { @@ -4452,16 +4452,6 @@ void BuildTokenTest::EnqueueBooleans(vector& booleans, int count, va_list } } -TEST_F(BuildTokenTest, CompleteNoWork) { - // plan should not execute anything - string err; - - EXPECT_TRUE(builder_.Build(&err)); - EXPECT_EQ("", err); - - EXPECT_EQ(0u, token_command_runner_.commands_ran_.size()); -} - TEST_F(BuildTokenTest, DoNotAquireToken) { // plan should execute one command string err; diff --git a/src/subprocess-posix.cc b/src/subprocess-posix.cc index 74451b0be2..31839276c4 100644 --- a/src/subprocess-posix.cc +++ b/src/subprocess-posix.cc @@ -250,7 +250,7 @@ Subprocess *SubprocessSet::Add(const string& command, bool use_console) { } #ifdef USE_PPOLL -bool SubprocessSet::DoWork(struct TokenPool* tokens) { +bool SubprocessSet::DoWork(TokenPool* tokens) { vector fds; nfds_t nfds = 0; @@ -315,7 +315,7 @@ bool SubprocessSet::DoWork(struct TokenPool* tokens) { } #else // !defined(USE_PPOLL) -bool SubprocessSet::DoWork(struct TokenPool* tokens) { +bool SubprocessSet::DoWork(TokenPool* tokens) { fd_set set; int nfds = 0; FD_ZERO(&set); diff --git a/src/subprocess-win32.cc b/src/subprocess-win32.cc index ce3e2c20a4..2926e9a221 100644 --- a/src/subprocess-win32.cc +++ b/src/subprocess-win32.cc @@ -252,7 +252,7 @@ Subprocess *SubprocessSet::Add(const string& command, bool use_console) { return subprocess; } -bool SubprocessSet::DoWork(struct TokenPool* tokens) { +bool SubprocessSet::DoWork(TokenPool* tokens) { DWORD bytes_read; Subprocess* subproc; OVERLAPPED* overlapped; diff --git a/src/subprocess.h b/src/subprocess.h index 9ea67ea477..1ec78171e8 100644 --- a/src/subprocess.h +++ b/src/subprocess.h @@ -86,7 +86,7 @@ struct SubprocessSet { ~SubprocessSet(); Subprocess* Add(const std::string& command, bool use_console = false); - bool DoWork(struct TokenPool* tokens); + bool DoWork(TokenPool* tokens); Subprocess* NextFinished(); void Clear(); diff --git a/src/subprocess_test.cc b/src/subprocess_test.cc index f625963462..7b146f31be 100644 --- a/src/subprocess_test.cc +++ b/src/subprocess_test.cc @@ -35,7 +35,7 @@ const char* kSimpleCommand = "cmd /c dir \\"; const char* kSimpleCommand = "ls /"; #endif -struct TokenPoolTest : public TokenPool { +struct TestTokenPool : public TokenPool { bool Acquire() { return false; } void Reserve() {} void Release() {} @@ -58,7 +58,7 @@ struct TokenPoolTest : public TokenPool { struct SubprocessTest : public testing::Test { SubprocessSet subprocs_; - TokenPoolTest tokens_; + TestTokenPool tokens_; }; } // anonymous namespace @@ -73,7 +73,7 @@ TEST_F(SubprocessTest, BadCommandStderr) { // Pretend we discovered that stderr was ready for writing. subprocs_.DoWork(NULL); } - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); EXPECT_EQ(ExitFailure, subproc->Finish()); EXPECT_NE("", subproc->GetOutput()); @@ -89,7 +89,7 @@ TEST_F(SubprocessTest, NoSuchCommand) { // Pretend we discovered that stderr was ready for writing. subprocs_.DoWork(NULL); } - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); EXPECT_EQ(ExitFailure, subproc->Finish()); EXPECT_NE("", subproc->GetOutput()); @@ -109,7 +109,7 @@ TEST_F(SubprocessTest, InterruptChild) { while (!subproc->Done()) { subprocs_.DoWork(NULL); } - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); EXPECT_EQ(ExitInterrupted, subproc->Finish()); } @@ -135,7 +135,7 @@ TEST_F(SubprocessTest, InterruptChildWithSigTerm) { while (!subproc->Done()) { subprocs_.DoWork(NULL); } - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); EXPECT_EQ(ExitInterrupted, subproc->Finish()); } @@ -161,7 +161,7 @@ TEST_F(SubprocessTest, InterruptChildWithSigHup) { while (!subproc->Done()) { subprocs_.DoWork(NULL); } - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); EXPECT_EQ(ExitInterrupted, subproc->Finish()); } @@ -190,7 +190,7 @@ TEST_F(SubprocessTest, Console) { while (!subproc->Done()) { subprocs_.DoWork(NULL); } - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); EXPECT_EQ(ExitSuccess, subproc->Finish()); } @@ -206,7 +206,7 @@ TEST_F(SubprocessTest, SetWithSingle) { while (!subproc->Done()) { subprocs_.DoWork(NULL); } - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); ASSERT_EQ(ExitSuccess, subproc->Finish()); ASSERT_NE("", subproc->GetOutput()); @@ -243,7 +243,7 @@ TEST_F(SubprocessTest, SetWithMulti) { ASSERT_GT(subprocs_.running_.size(), 0u); subprocs_.DoWork(NULL); } - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); ASSERT_EQ(0u, subprocs_.running_.size()); ASSERT_EQ(3u, subprocs_.finished_.size()); @@ -278,7 +278,7 @@ TEST_F(SubprocessTest, SetWithLots) { subprocs_.ResetTokenAvailable(); while (!subprocs_.running_.empty()) subprocs_.DoWork(NULL); - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); for (size_t i = 0; i < procs.size(); ++i) { ASSERT_EQ(ExitSuccess, procs[i]->Finish()); ASSERT_NE("", procs[i]->GetOutput()); @@ -298,7 +298,7 @@ TEST_F(SubprocessTest, ReadStdin) { while (!subproc->Done()) { subprocs_.DoWork(NULL); } - ASSERT_EQ(false, subprocs_.IsTokenAvailable()); + ASSERT_FALSE(subprocs_.IsTokenAvailable()); ASSERT_EQ(ExitSuccess, subproc->Finish()); ASSERT_EQ(1u, subprocs_.finished_.size()); } @@ -322,7 +322,7 @@ TEST_F(SubprocessTest, TokenAvailable) { subprocs_.DoWork(&tokens_); #ifdef _WIN32 tokens_._token_available = false; - // we need to loop here as we have no conrol where the token + // we need to loop here as we have no control where the token // I/O completion post ends up in the queue while (!subproc->Done() && !subprocs_.IsTokenAvailable()) { subprocs_.DoWork(&tokens_); diff --git a/src/tokenpool-gnu-make-posix.cc b/src/tokenpool-gnu-make-posix.cc index 70d84bfff7..4f436765f6 100644 --- a/src/tokenpool-gnu-make-posix.cc +++ b/src/tokenpool-gnu-make-posix.cc @@ -23,6 +23,7 @@ #include #include #include +#include // TokenPool implementation for GNU make jobserver - POSIX implementation // (http://make.mad-scientist.net/papers/jobserver-implementation/) @@ -32,8 +33,8 @@ struct GNUmakeTokenPoolPosix : public GNUmakeTokenPool { virtual int GetMonitorFd(); - virtual const char *GetEnv(const char *name) { return getenv(name); }; - virtual bool ParseAuth(const char *jobserver); + virtual const char* GetEnv(const char* name) { return getenv(name); }; + virtual bool ParseAuth(const char* jobserver); virtual bool AcquireToken(); virtual bool ReturnToken(); @@ -44,6 +45,16 @@ struct GNUmakeTokenPoolPosix : public GNUmakeTokenPool { struct sigaction old_act_; bool restore_; + // See https://www.gnu.org/software/make/manual/html_node/POSIX-Jobserver.html + // + // It’s important that when you release the job slot, you write back + // the same character you read. Don’t assume that all tokens are the + // same character different characters may have different meanings to + // GNU make. The order is not important, since make has no idea in + // what order jobs will complete anyway. + // + std::stack tokens_; + static int dup_rfd_; static void CloseDupRfd(int signum); @@ -64,9 +75,7 @@ bool GNUmakeTokenPoolPosix::CheckFd(int fd) { if (fd < 0) return false; int ret = fcntl(fd, F_GETFD); - if (ret < 0) - return false; - return true; + return ret >= 0; } int GNUmakeTokenPoolPosix::dup_rfd_ = -1; @@ -82,14 +91,13 @@ bool GNUmakeTokenPoolPosix::SetAlarmHandler() { act.sa_handler = CloseDupRfd; if (sigaction(SIGALRM, &act, &old_act_) < 0) { perror("sigaction:"); - return(false); - } else { - restore_ = true; - return(true); + return false; } + restore_ = true; + return true; } -bool GNUmakeTokenPoolPosix::ParseAuth(const char *jobserver) { +bool GNUmakeTokenPoolPosix::ParseAuth(const char* jobserver) { int rfd = -1; int wfd = -1; if ((sscanf(jobserver, "%*[^=]=%d,%d", &rfd, &wfd) == 2) && @@ -136,6 +144,7 @@ bool GNUmakeTokenPoolPosix::AcquireToken() { if (dup_rfd_ != -1) { struct sigaction act, old_act; int ret = 0; + char buf; // Temporarily replace SIGCHLD handler with our own memset(&act, 0, sizeof(act)); @@ -147,8 +156,6 @@ bool GNUmakeTokenPoolPosix::AcquireToken() { memset(&timeout, 0, sizeof(timeout)); timeout.it_value.tv_usec = 100 * 1000; // [ms] -> [usec] if (setitimer(ITIMER_REAL, &timeout, NULL) == 0) { - char buf; - // Now try to read() from dup_rfd_. Return values from read(): // // 1. token read -> 1 @@ -171,8 +178,10 @@ bool GNUmakeTokenPoolPosix::AcquireToken() { CloseDupRfd(0); // Case 1 from above list - if (ret > 0) + if (ret > 0) { + tokens_.push(buf); return true; + } } } @@ -183,11 +192,13 @@ bool GNUmakeTokenPoolPosix::AcquireToken() { } bool GNUmakeTokenPoolPosix::ReturnToken() { - const char buf = '+'; + const char buf = tokens_.top(); while (1) { int ret = write(wfd_, &buf, 1); - if (ret > 0) + if (ret > 0) { + tokens_.pop(); return true; + } if ((ret != -1) || (errno != EINTR)) return false; // write got interrupted - retry @@ -195,9 +206,9 @@ bool GNUmakeTokenPoolPosix::ReturnToken() { } int GNUmakeTokenPoolPosix::GetMonitorFd() { - return(rfd_); + return rfd_; } -struct TokenPool *TokenPool::Get() { +TokenPool* TokenPool::Get() { return new GNUmakeTokenPoolPosix; } diff --git a/src/tokenpool-gnu-make-win32.cc b/src/tokenpool-gnu-make-win32.cc index 2719f2c1fc..b2bb52fadb 100644 --- a/src/tokenpool-gnu-make-win32.cc +++ b/src/tokenpool-gnu-make-win32.cc @@ -14,7 +14,8 @@ #include "tokenpool-gnu-make.h" -// always include first to make sure other headers do the correct thing... +// Always include this first. +// Otherwise the other system headers don't work correctly under Win32 #include #include @@ -32,8 +33,8 @@ struct GNUmakeTokenPoolWin32 : public GNUmakeTokenPool { virtual void WaitForTokenAvailability(HANDLE ioport); virtual bool TokenIsAvailable(ULONG_PTR key); - virtual const char *GetEnv(const char *name); - virtual bool ParseAuth(const char *jobserver); + virtual const char* GetEnv(const char* name); + virtual bool ParseAuth(const char* jobserver); virtual bool AcquireToken(); virtual bool ReturnToken(); @@ -98,19 +99,19 @@ GNUmakeTokenPoolWin32::~GNUmakeTokenPoolWin32() { } } -const char *GNUmakeTokenPoolWin32::GetEnv(const char *name) { +const char* GNUmakeTokenPoolWin32::GetEnv(const char* name) { // getenv() does not work correctly together with tokenpool_tests.cc static char buffer[MAX_PATH + 1]; - if (GetEnvironmentVariable("MAKEFLAGS", buffer, sizeof(buffer)) == 0) + if (GetEnvironmentVariable(name, buffer, sizeof(buffer)) == 0) return NULL; - return(buffer); + return buffer; } -bool GNUmakeTokenPoolWin32::ParseAuth(const char *jobserver) { +bool GNUmakeTokenPoolWin32::ParseAuth(const char* jobserver) { // match "--jobserver-auth=gmake_semaphore_..." - const char *start = strchr(jobserver, '='); + const char* start = strchr(jobserver, '='); if (start) { - const char *end = start; + const char* end = start; unsigned int len; char c, *auth; @@ -119,14 +120,15 @@ bool GNUmakeTokenPoolWin32::ParseAuth(const char *jobserver) { break; len = end - start; // includes string terminator in count - if ((len > 1) && ((auth = (char *)malloc(len)) != NULL)) { + if ((len > 1) && ((auth = (char*)malloc(len)) != NULL)) { strncpy(auth, start + 1, len - 1); auth[len - 1] = '\0'; - if ((semaphore_jobserver_ = OpenSemaphore(SEMAPHORE_ALL_ACCESS, /* Semaphore access setting */ - FALSE, /* Child processes DON'T inherit */ - auth /* Semaphore name */ - )) != NULL) { + if ((semaphore_jobserver_ = + OpenSemaphore(SEMAPHORE_ALL_ACCESS, /* Semaphore access setting */ + FALSE, /* Child processes DON'T inherit */ + auth /* Semaphore name */ + )) != NULL) { free(auth); return true; } @@ -171,7 +173,7 @@ DWORD GNUmakeTokenPoolWin32::SemaphoreThread() { } DWORD WINAPI GNUmakeTokenPoolWin32::SemaphoreThreadWrapper(LPVOID param) { - GNUmakeTokenPoolWin32 *This = (GNUmakeTokenPoolWin32 *) param; + GNUmakeTokenPoolWin32* This = (GNUmakeTokenPoolWin32*) param; return This->SemaphoreThread(); } @@ -216,7 +218,7 @@ void GNUmakeTokenPoolWin32::WaitForTokenAvailability(HANDLE ioport) { bool GNUmakeTokenPoolWin32::TokenIsAvailable(ULONG_PTR key) { // alert child thread to break wait on token semaphore - QueueUserAPC(&NoopAPCFunc, child_, (ULONG_PTR)NULL); + QueueUserAPC((PAPCFUNC)&NoopAPCFunc, child_, (ULONG_PTR)NULL); // return true when GetQueuedCompletionStatus() returned our key return key == (ULONG_PTR) this; @@ -232,6 +234,6 @@ void GNUmakeTokenPoolWin32::WaitForObject(HANDLE object) { Win32Fatal("WaitForSingleObject"); } -struct TokenPool *TokenPool::Get() { +TokenPool* TokenPool::Get() { return new GNUmakeTokenPoolWin32; } diff --git a/src/tokenpool-gnu-make.cc b/src/tokenpool-gnu-make.cc index 92ff611721..60e0552924 100644 --- a/src/tokenpool-gnu-make.cc +++ b/src/tokenpool-gnu-make.cc @@ -31,36 +31,37 @@ GNUmakeTokenPool::~GNUmakeTokenPool() { bool GNUmakeTokenPool::Setup(bool ignore, bool verbose, double& max_load_average) { - const char *value = GetEnv("MAKEFLAGS"); - if (value) { - // GNU make <= 4.1 - const char *jobserver = strstr(value, "--jobserver-fds="); + const char* value = GetEnv("MAKEFLAGS"); + if (!value) + return false; + + // GNU make <= 4.1 + const char* jobserver = strstr(value, "--jobserver-fds="); + if (!jobserver) // GNU make => 4.2 - if (!jobserver) - jobserver = strstr(value, "--jobserver-auth="); - if (jobserver) { - LinePrinter printer; - - if (ignore) { - printer.PrintOnNewLine("ninja: warning: -jN forced on command line; ignoring GNU make jobserver.\n"); - } else { - if (ParseAuth(jobserver)) { - const char *l_arg = strstr(value, " -l"); - int load_limit = -1; - - if (verbose) { - printer.PrintOnNewLine("ninja: using GNU make jobserver.\n"); - } - - // translate GNU make -lN to ninja -lN - if (l_arg && - (sscanf(l_arg + 3, "%d ", &load_limit) == 1) && - (load_limit > 0)) { - max_load_average = load_limit; - } - - return true; + jobserver = strstr(value, "--jobserver-auth="); + if (jobserver) { + LinePrinter printer; + + if (ignore) { + printer.PrintOnNewLine("ninja: warning: -jN forced on command line; ignoring GNU make jobserver.\n"); + } else { + if (ParseAuth(jobserver)) { + const char* l_arg = strstr(value, " -l"); + int load_limit = -1; + + if (verbose) { + printer.PrintOnNewLine("ninja: using GNU make jobserver.\n"); + } + + // translate GNU make -lN to ninja -lN + if (l_arg && + (sscanf(l_arg + 3, "%d ", &load_limit) == 1) && + (load_limit > 0)) { + max_load_average = load_limit; } + + return true; } } } @@ -76,10 +77,10 @@ bool GNUmakeTokenPool::Acquire() { // token acquired available_++; return true; - } else { - // no token available - return false; } + + // no token available + return false; } void GNUmakeTokenPool::Reserve() { diff --git a/src/tokenpool-gnu-make.h b/src/tokenpool-gnu-make.h index d3852088e2..c94cca5e2d 100644 --- a/src/tokenpool-gnu-make.h +++ b/src/tokenpool-gnu-make.h @@ -17,7 +17,7 @@ // interface to GNU make token pool struct GNUmakeTokenPool : public TokenPool { GNUmakeTokenPool(); - virtual ~GNUmakeTokenPool(); + ~GNUmakeTokenPool(); // token pool implementation virtual bool Acquire(); @@ -27,8 +27,8 @@ struct GNUmakeTokenPool : public TokenPool { virtual bool Setup(bool ignore, bool verbose, double& max_load_average); // platform specific implementation - virtual const char *GetEnv(const char *name) = 0; - virtual bool ParseAuth(const char *jobserver) = 0; + virtual const char* GetEnv(const char* name) = 0; + virtual bool ParseAuth(const char* jobserver) = 0; virtual bool AcquireToken() = 0; virtual bool ReturnToken() = 0; diff --git a/src/tokenpool-none.cc b/src/tokenpool-none.cc deleted file mode 100644 index 613d16882d..0000000000 --- a/src/tokenpool-none.cc +++ /dev/null @@ -1,22 +0,0 @@ -// Copyright 2016-2018 Google Inc. All Rights Reserved. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#include "tokenpool.h" - -#include - -// No-op TokenPool implementation -struct TokenPool *TokenPool::Get() { - return NULL; -} diff --git a/src/tokenpool.h b/src/tokenpool.h index 1be8e1d5ce..931c22754d 100644 --- a/src/tokenpool.h +++ b/src/tokenpool.h @@ -38,5 +38,5 @@ struct TokenPool { #endif // returns NULL if token pool is not available - static struct TokenPool *Get(); + static TokenPool* Get(); }; diff --git a/src/tokenpool_test.cc b/src/tokenpool_test.cc index 8d4fd7d33a..5858ef6cf4 100644 --- a/src/tokenpool_test.cc +++ b/src/tokenpool_test.cc @@ -43,13 +43,17 @@ const double kLoadAverageDefault = -1.23456789; struct TokenPoolTest : public testing::Test { double load_avg_; - TokenPool *tokens_; + TokenPool* tokens_; char buf_[1024]; #ifdef _WIN32 - const char *semaphore_name_; + const char* semaphore_name_; HANDLE semaphore_; #else int fds_[2]; + + char random() { + return int((rand() / double(RAND_MAX)) * 256); + } #endif virtual void SetUp() { @@ -65,7 +69,7 @@ struct TokenPoolTest : public testing::Test { ASSERT_TRUE(false); } - void CreatePool(const char *format, bool ignore_jobserver = false) { + void CreatePool(const char* format, bool ignore_jobserver = false) { if (format) { sprintf(buf_, format, #ifdef _WIN32 @@ -199,7 +203,8 @@ TEST_F(TokenPoolTest, TwoTokens) { ASSERT_TRUE(ReleaseSemaphore(semaphore_, 1, &previous)); ASSERT_EQ(0, previous); #else - ASSERT_EQ(1u, write(fds_[1], "T", 1)); + char test_tokens[1] = { random() }; + ASSERT_EQ(1u, write(fds_[1], test_tokens, sizeof(test_tokens))); #endif EXPECT_TRUE(tokens_->Acquire()); tokens_->Reserve(); @@ -209,7 +214,7 @@ TEST_F(TokenPoolTest, TwoTokens) { tokens_->Release(); EXPECT_TRUE(tokens_->Acquire()); - // release implict token - must return 2nd token back to jobserver + // release implicit token - must return 2nd token back to jobserver tokens_->Release(); EXPECT_TRUE(tokens_->Acquire()); @@ -220,6 +225,7 @@ TEST_F(TokenPoolTest, TwoTokens) { EXPECT_EQ(0, previous); #else EXPECT_EQ(1u, read(fds_[0], buf_, sizeof(buf_))); + EXPECT_EQ(test_tokens[0], buf_[0]); #endif // implicit token @@ -243,7 +249,8 @@ TEST_F(TokenPoolTest, Clear) { ASSERT_TRUE(ReleaseSemaphore(semaphore_, 2, &previous)); ASSERT_EQ(0, previous); #else - ASSERT_EQ(2u, write(fds_[1], "TT", 2)); + char test_tokens[2] = { random(), random() }; + ASSERT_EQ(2u, write(fds_[1], test_tokens, sizeof(test_tokens))); #endif EXPECT_TRUE(tokens_->Acquire()); tokens_->Reserve(); @@ -262,6 +269,9 @@ TEST_F(TokenPoolTest, Clear) { EXPECT_EQ(0, previous); #else EXPECT_EQ(2u, read(fds_[0], buf_, sizeof(buf_))); + // tokens are pushed onto a stack, hence returned in reverse order + EXPECT_EQ(test_tokens[0], buf_[1]); + EXPECT_EQ(test_tokens[1], buf_[0]); #endif // implicit token