This revision was automatically updated to reflect the committed changes.
Closed by commit rL320391: For Linux/gnu compatibility, preinclude
if the file is available (authored by erichkeane).
Changed prior to commit:
https://reviews.llvm.org/D34158?vs=123613&id=126386#toc
Repository:
rL LLV
This revision was automatically updated to reflect the committed changes.
Closed by commit rC320391: For Linux/gnu compatibility, preinclude
if the file is available (authored by erichkeane).
Changed prior to commit:
https://reviews.llvm.org/D34158?vs=123613&id=126387#toc
Repository:
rC Cla
mibintc reopened this revision.
mibintc added a comment.
Hi.
I also posted this question on IRC
@erichkeane pushed a fix for https://reviews.llvm.org/D34158 which on Linux
does a preinclude of stdc-predef.h; on Monday. Later on Monday he pulled it out
because it caused 3 test failures and word
This revision was automatically updated to reflect the committed changes.
Closed by commit rL318669: For Linux/gnu compatibility, preinclude
if the file is available (authored by erichkeane).
Changed prior to commit:
https://reviews.llvm.org/D34158?vs=120581&id=123613#toc
Repository:
rL LLV
mibintc updated this revision to Diff 120581.
mibintc added a comment.
The patch to clang is the same as before. The modifications are to the test
cases, I verified that all these test cases need modifications. I will also be
updating the associated diff for clang/tools/extra, please review that
mibintc added a comment.
I pulled out all the test changes and the only one that is needed presently is
Clang :: Driver/crash-report.c; huh, that's different than what I encountered
earlier in the summer. Now I'll recheck those extra tests.
https://reviews.llvm.org/D34158
_
mibintc added a comment.
In https://reviews.llvm.org/D34158#905637, @mibintc wrote:
> In https://reviews.llvm.org/D34158#905596, @jyknight wrote:
>
> > I'd still _very_ much like a solution that is acceptable for all libc to
> > use, and have that on by default.
> >
> > However, I guess this is
mibintc added a comment.
In https://reviews.llvm.org/D34158#905596, @jyknight wrote:
> I'd still _very_ much like a solution that is acceptable for all libc to use,
> and have that on by default.
>
> However, I guess this is fine.
>
> Only concern I have is that it seems odd that so many test-ca
jyknight added a comment.
I'd still _very_ much like a solution that is acceptable for all libc to use,
and have that on by default.
However, I guess this is fine.
Only concern I have is that it seems odd that so many test-cases needed to be
changed. What fails without those test changes?
ht
mibintc added a comment.
Hey @jyknight I heard from @erichkeane that you may want a couple more changes
to this patch. Please let me know if you have some changes to recommend.
@joerg thought this was OK for submission. Thanks --Melanie
https://reviews.llvm.org/D34158
_
On Tue, Sep 12, 2017 at 08:12:26PM +, Blower, Melanie via cfe-commits wrote:
> How is platform opt-in accomplished, is it part of the configure step?
It is part of the Linux toolchain, other platforms interested in this or
equivalent functionality would have to duplicate the hook.
Joerg
_
> -Original Message-
> From: Joerg Sonnenberger via Phabricator [mailto:revi...@reviews.llvm.org]
> Sent: Tuesday, September 12, 2017 3:24 PM
> To: Blower, Melanie ; olivier...@gmail.com;
> kalinichev.s...@gmail.com; kf...@kde.org; m...@milianw.de; Keane, Erich
> ; mgo...@gentoo.org; fedo
joerg added a comment.
This version is fine with me. The only contentious part is whether it should be
opt-in or opt-out for platforms, so getting this version in and revisiting the
issue again later is OK from my perspective.
https://reviews.llvm.org/D34158
mibintc updated this revision to Diff 114820.
mibintc added a comment.
I heard that @jyknight is still interested in this functionality, updating the
patch to latest sources, to do this i needed to make a small change to a couple
of driver tests. Other than that it's the same as previous submiss
mibintc added a comment.
Another ping. Is it possible to approve this modification, or alternatively,
let me know that it won't be approved indefinitely? I understand that it's a
controversial change. Thanks!
https://reviews.llvm.org/D34158
___
mibintc added a comment.
ping
https://reviews.llvm.org/D34158
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
mibintc updated this revision to Diff 111019.
mibintc added a comment.
@jyknight recommended adding the new option to skipArgs in Job.cpp. This
revision makes that change and tests that in crash-report.c; look OK?
https://reviews.llvm.org/D34158
Files:
include/clang/Driver/CC1Options.td
i
mibintc planned changes to this revision.
mibintc added a comment.
Need TO FIX: We should be stripping the new arg as well: add
"-fsystem-include-if-exists" argument to the list of include things in the
skipArgs() function in lib/Driver/Job.cpp
https://reviews.llvm.org/D34158
__
jyknight added a comment.
In https://reviews.llvm.org/D34158#838454, @mibintc wrote:
> This patch is responding to @jyknight 's concern about preprocessed input.
> The patch as it stands doesn't have this issue. I added 2 test cases, one
> using option -x cpp-output, and another for a source fi
mibintc updated this revision to Diff 110627.
mibintc added a comment.
This patch is responding to @jyknight 's concern about preprocessed input. The
patch as it stands doesn't have this issue. I added 2 test cases, one using
option -x cpp-output, and another for a source file suffixed with .i
joerg added a comment.
In https://reviews.llvm.org/D34158#837444, @jyknight wrote:
> Now, the "/tmp/foo-XXX.sh" also has a line labeled "Driver args: " with the
> original command-line on it. If I understand correctly, you then like to take
> this
> simpler Driver command-line, and edit it man
joerg added a comment.
In https://reviews.llvm.org/D34158#837281, @hfinkel wrote:
> In https://reviews.llvm.org/D34158#837130, @joerg wrote:
>
> > In https://reviews.llvm.org/D34158#836026, @jyknight wrote:
> >
> > > In https://reviews.llvm.org/D34158#827178, @joerg wrote:
> > >
> > > > (2) It ad
jyknight added a comment.
Just to restate: the ideal outcome of this discussion for me would be to
resolve things such that _ALL_ libc implementations will feel comfortable using
this technique to provide the C11-required predefined macros.
I'd love for linux, freebsd, macos, solaris, etc etc l
hfinkel added a comment.
In https://reviews.llvm.org/D34158#837130, @joerg wrote:
> In https://reviews.llvm.org/D34158#836026, @jyknight wrote:
>
> > In https://reviews.llvm.org/D34158#827178, @joerg wrote:
> >
> > > (2) It adds magic behavior that can make debugging more difficult.
> > > Partia
joerg added a comment.
In https://reviews.llvm.org/D34158#836026, @jyknight wrote:
> In https://reviews.llvm.org/D34158#827178, @joerg wrote:
>
> > (2) It adds magic behavior that can make debugging more difficult.
> > Partially preprocessed sources for example could be compiled with plain -c
>
fedor.sergeev added a comment.
In https://reviews.llvm.org/D34158#836026, @jyknight wrote:
> In https://reviews.llvm.org/D34158#827178, @joerg wrote:
>
> > I had a long discussion with James about this on IRC without reaching a
> > clear consensus. I consider forcing this behavior on all targets
jyknight added a comment.
In https://reviews.llvm.org/D34158#827178, @joerg wrote:
> I had a long discussion with James about this on IRC without reaching a clear
> consensus. I consider forcing this behavior on all targets to be a major bug.
> It should be opt-in and opt-in only:
>
> (1) The h
mibintc updated this revision to Diff 110193.
mibintc added a comment.
Responding to @fedor.sergeev 's comment. This is an updated patch which pulls
out the "isValid" check on GCCInstallation. Also I'm putting back the dummy
sysroot tree which contains stdc-predef.h and adding a new test run to
fedor.sergeev added a comment.
In https://reviews.llvm.org/D34158#834298, @mibintc wrote:
> In fact I did have trouble writing the new test case to pass with the
> gnu/Linux toolchain. In the file lib/Driver/ToolChains/Linux.cpp function
> AddGnuIncludeArgs checks if GCCInstallation.isValid()
fedor.sergeev added a comment.
In https://reviews.llvm.org/D34158#834298, @mibintc wrote:
> In fact I did have trouble writing the new test case to pass with the
> gnu/Linux toolchain. In the file lib/Driver/ToolChains/Linux.cpp function
> AddGnuIncludeArgs checks if GCCInstallation.isValid().
mibintc updated this revision to Diff 110055.
mibintc added a comment.
In the last review, it was deemed less controversial if I move these updates
back into the gnu/Linux tool chain. This revision is responding to that
feedback. I also simplified the test case. I tested on Linux with check-all
31 matches
Mail list logo
