[PATCH] Add non-blocking Wait() for launched processes

Rafael Ávila de Espíndola rafael.espindola at gmail.com
Wed Sep 25 11:54:54 PDT 2013



================
Comment at: include/llvm/Support/Program.h:43
@@ +42,3 @@
+  // to pass around the process handle and the last object needs to free it.
+  friend class llvm::RefCountedBase<ProcessInfo>;
+
----------------
How is this different from other OS resources? A file must be closed for example, but having all files be refcounted is probably not a good design.


================
Comment at: include/llvm/Support/Program.h:157
@@ +156,3 @@
+      ///< until child has terminated.
+      std::string *ErrMsg = 0 ///< If non-zero, provides a pointer to a string
+      ///< instance in which error messages will be returned. If the string
----------------
This is the old style of error handling. The new one would be for this function to return an error_code.


================
Comment at: lib/Support/Program.cpp:26
@@ +25,3 @@
+bool operator==(const ProcessInfo &LHS, const ProcessInfo &RHS) {
+  return (LHS.Pid == RHS.Pid && LHS.Program == RHS.Program);
+}
----------------
We cannot have two running process with the same pid. You don't need to compare the Program field.

================
Comment at: include/llvm/Support/Program.h:67
@@ +66,3 @@
+  /// The path to the executable this process is running.
+  llvm::SmallString<256> Program;
+
----------------
This is just for the sys::fs::exists check in Wait, right? Does any code depend on it? If not, just delete it. In the unlikely case that we do depend on it, can you move the check to Execute and avoid having this member variable?


http://llvm-reviews.chandlerc.com/D1728



More information about the llvm-commits mailing list