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

#1140 (comment)
#1140 (review)
#1140 (review)
#1140 (comment)
#1140 (comment)

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)

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<const Subprocess*, Edge*> subproc_to_edge_;
 };
 

src/build_test.cc

@@ -4406,7 +4406,7 @@ struct BuildTokenTest : public BuildTest {
   void ExpectWaitForCommand(int count, ...);
 
 private:
-  void EnqueueBooleans(vector<bool>& booleans, int count, va_list ao);
+  void EnqueueBooleans(vector<bool>& booleans, int count, va_list ap);
 };
 
 void BuildTokenTest::SetUp() {
@@ -4452,16 +4452,6 @@ void BuildTokenTest::EnqueueBooleans(vector<bool>& 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;

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<pollfd> 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);

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;

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();
 

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_);

src/tokenpool-gnu-make-posix.cc

@@ -23,6 +23,7 @@
 #include <stdio.h>
 #include <string.h>
 #include <stdlib.h>
+#include <stack>
 
 // 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<char> 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,21 +192,23 @@ 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
   }
 }
 
 int GNUmakeTokenPoolPosix::GetMonitorFd() {
-  return(rfd_);
+  return rfd_;
 }
 
-struct TokenPool *TokenPool::Get() {
+TokenPool* TokenPool::Get() {
   return new GNUmakeTokenPoolPosix;
 }