[PATCH] Add writeFileWithSystemEncoding to LibLLVMSupport
Rafael Auler
rafaelauler at gmail.com
Mon Aug 25 13:56:05 PDT 2014
Hi Rafael,
Thanks for this extra round of review, I answered your questions below. I will send an updated patch shortly.
================
Comment at: include/llvm/Support/Program.h:146
@@ +145,3 @@
+ /// \returns true if successful, otherwise \p ErrMsg is filled accordingly.
+ std::error_code writeFileWithEncoding(const char *FileName, StringRef contents,
+ std::string *ErrMsg = nullptr,
----------------
rafael wrote:
> Please don't pass in the ErrMsg string. The caller can use EC.message() method. That is an old API design we try to avoid. I see it is used because raw_fd_osstream was never updated to avoid it :-(
>
> Suggestion: pass in a file descriptor. That way you can use a raw_fd_ostream with a saner interface and we don't spread the use of std::string pointers for error messages to other APIs.
>
> Another option: keep the filename, but use openFileForWrite + the fd raw_fd_ostream constructor.
I will use your updated constructor from r216293, thanks for updating it!
================
Comment at: lib/Support/Unix/Program.inc:479
@@ -450,3 +478,3 @@
// Conservatively account for space required by environment variables.
- ArgMax /= 2;
+ long halfArgMax = ArgMax / 2;
----------------
rafael wrote:
> This bug fix could be in another patch, no?
Ok! Sent it in http://reviews.llvm.org/D5053
================
Comment at: lib/Support/Windows/Program.inc:171
@@ -168,15 +170,3 @@
-static bool Execute(ProcessInfo &PI, StringRef Program, const char **args,
- const char **envp, const StringRef **redirects,
- unsigned memoryLimit, std::string *ErrMsg) {
- if (!sys::fs::can_execute(Program)) {
- if (ErrMsg)
- *ErrMsg = "program not executable";
- return false;
- }
-
- // Windows wants a command line, not an array of args, to pass to the new
- // process. We have to concatenate them all, while quoting the args that
- // have embedded spaces (or are empty).
-
+static std::unique_ptr<char[]> flattenArgs(const char **args) {
// First, determine the length of the command line.
----------------
rafael wrote:
> This refactoring could be in another patch, no?
Ok! Sent it in http://reviews.llvm.org/D5054
http://reviews.llvm.org/D4896
More information about the llvm-commits
mailing list