[PATCH] [Support][Program] Add findProgramByName(Name, OptionalPaths)

Rafael Ávila de Espíndola rafael.espindola at gmail.com
Fri Oct 31 22:17:30 PDT 2014


Seems a reasonable API. I just have a few small questions.

================
Comment at: include/llvm/Support/Program.h:70
@@ +69,3 @@
+      StringRef Name,
+      Optional<ArrayRef<StringRef>> Paths = Optional<ArrayRef<StringRef>>());
+
----------------
Why do we need the Optional? Can't we just use an empty Paths as a special value that means "read $PATH"?

================
Comment at: lib/Support/Unix/Program.inc:107
@@ +106,3 @@
+  // Check some degenerate cases
+  if (Name.empty()) // no program
+    return std::errc::no_such_file_or_directory;
----------------
Can't we just say that name should not be empty and assert? It looks like a misuse of the API, not an error.

================
Comment at: lib/Support/Unix/Program.inc:114
@@ +113,3 @@
+
+  SmallVector<StringRef, 16> SearchPaths;
+  if (Paths)
----------------
Why do you need the extra storage for all cases? Can't this be

if (using $PATH) {
  build the array
  return  recurse passing the array
}

use the arrayref argument directly

http://reviews.llvm.org/D6067






More information about the llvm-commits mailing list