Hi Rafael,
I have answered your comments below, thanks. I will upload a patch fixing your
concerns shortly after this message.
Sean, regarding the change to llvm::sys::argumentsFitWithinSystemLimits() to
avoid code duplication, I am afraid it is not necessary after my new patch. I
will send it now.
Regards,
Rafael
================
Comment at: include/clang/Driver/Job.h:87
@@ +86,3 @@
+ /// file, when it is used.
+ void PrintArgsWithRespFile(llvm::raw_ostream &OS,
+ const char *Terminator, bool Quote,
----------------
rafael wrote:
> printArgsWithRespFile
changed
================
Comment at: include/clang/Driver/Job.h:100
@@ +99,3 @@
+ /// limits (the maximum command line length).
+ bool NeedsResponseFile() const { return needsResponseFile; }
+
----------------
rafael wrote:
> The function should be lowercase, the variable Uppercase.
changed
================
Comment at: lib/Driver/Job.cpp:32
@@ +31,3 @@
+ Executable(_Executable), Arguments(_Arguments),
+ needsResponseFile(
+ !llvm::sys::argumentsFitWithinSystemLimits(_Arguments) &&
----------------
rafael wrote:
> I wonder if we should say that the job needs a response file based only on
> llvm::sys::argumentsFitWithinSystemLimits and then emit an error if
> Creator.getResponseFilesSupport() says it is not supported.
The problem is that llvm::sys::argumentsFitWithinSystemLimits() is not exact,
see:
bool llvm::sys::argumentsFitWithinSystemLimits(ArrayRef<const char*> Args) {
static long ArgMax = sysconf(_SC_ARG_MAX);
// System says no practical limit.
if (ArgMax == -1)
return true;
// Conservatively account for space required by environment variables.
ArgMax /= 2;
...
Thus, sometimes it may say that you need a response file but, when building the
argv vector, you discover that it actually fits in system limits. Therefore, I
think that our best action is to silently try to pass the arguments without a
response file because it may actually work.
================
Comment at: lib/Driver/Job.cpp:279
@@ +278,3 @@
+ if (needsResponseFile && ResponseFile.data() != nullptr) {
+ PrintArgsWithRespFile(OS, Terminator, Quote, CrashReport);
+ return;
----------------
rafael wrote:
> The code in PrintArgsWithRespFile looks fairly duplicated with what just
> follows. Couldn't this just patch Arguments and use the existing code path?
>
Done.
http://reviews.llvm.org/D4897
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
