mibintc planned changes to this revision.
mibintc added a comment.
I will prepare another patch responding to joerg's comment:
> Quoted Text
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
joerg added a comment.
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 header name is not mandated by any standard. It is not in any
joerg added a comment.
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 header name is not mandated by any standard. It is not in any
mibintc updated this revision to Diff 108999.
mibintc added a comment.
This patch responds to @fedor.sergeev 's feedback from earlier today. This is a
change to the test case test/Driver/stdc-predef.c in the situation that the
system does not supply a version of stdc-predef.h. Fedor had
fedor.sergeev added inline comments.
Comment at: test/Driver/stdc-predef.c:15
+ /* In this test, the file stdc-predef.h is missing from the
+installation */ #if _STDC_PREDEF_H
+ #error "stdc-predef.h should not be included"
I would rather see a real check
fedor.sergeev added inline comments.
Comment at: test/Driver/stdc-predef.c:15
+ /* In this test, the file stdc-predef.h is missing from the installation */
+#if _STDC_PREDEF_H
+ #error "stdc-predef.h should not be included"
I would rather see a real check on
mibintc updated this revision to Diff 108669.
mibintc added a comment.
Here's an updated patch which is using angle brackets to do the include, so the
search for stdc-predef.h is limited to system directories. Also my previous
revision was missing the new test cases since i had gotten a new
mibintc planned changes to this revision.
mibintc added a comment.
@erichkeane contacted me offline and pointed out I'm twine-ing with " not <.
i'm planning to change the option name from "finclude if exists" to "fsystem
include if exists", then i'll quote with < not ". hope to get this updated
fedor.sergeev added a comment.
In https://reviews.llvm.org/D34158#824079, @jyknight wrote:
> In https://reviews.llvm.org/D34158#823316, @fedor.sergeev wrote:
>
> > Hmm... I tried this patch and now the following worries me:
> >
> > - it passes -finclude-if-exists stdc-predef.h on all platforms
On Fri, Jul 28, 2017 at 02:07:29PM +, Blower, Melanie wrote:
>
>
> fedor.sergeev added a comment.
>
> Hmm... I tried this patch and now the following worries me:
>
> - it passes -finclude-if-exists stdc-predef.h on all platforms (say,
> including my Solaris platform that has no system
jyknight added a comment.
In https://reviews.llvm.org/D34158#823316, @fedor.sergeev wrote:
> Hmm... I tried this patch and now the following worries me:
>
> - it passes -finclude-if-exists stdc-predef.h on all platforms (say,
> including my Solaris platform that has no system stdc-predef.h)
jyknight added a comment.
In https://reviews.llvm.org/D34158#823316, @fedor.sergeev wrote:
> Hmm... I tried this patch and now the following worries me:
>
> - it passes -finclude-if-exists stdc-predef.h on all platforms (say,
> including my Solaris platform that has no system stdc-predef.h)
fedor.sergeev added a comment.
Hmm... I tried this patch and now the following worries me:
- it passes -finclude-if-exists stdc-predef.h on all platforms (say, including
my Solaris platform that has no system stdc-predef.h)
- it searches all the paths, not just "system include" ones
That
fedor.sergeev added a comment.
Hmm... I tried this patch and now the following worries me:
- it passes -finclude-if-exists stdc-predef.h on all platforms (say, including
my Solaris platform that has no system stdc-predef.h)
- it searches all the paths, not just "system include" ones
That
mibintc updated this revision to Diff 108519.
mibintc added a comment.
Here is an updated diff which is rebased to current trunk per @fedor.sergeev
's suggestion (thank you). make check-all and check-clang are working for me
on Linux with no unexpected failures.
The associated patch for test
mibintc planned changes to this revision.
mibintc added a comment.
I'm going to rebase the patch to latest.
Repository:
rL LLVM
https://reviews.llvm.org/D34158
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
mibintc added a comment.
Ping.
Repository:
rL LLVM
https://reviews.llvm.org/D34158
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@jyknight I've removed the check on nostdinc. AFAIK, I've addressed all the
feedback which I've received on the patch, is it OK to commit? --Melanie
-Original Message-
From: James Y Knight via Phabricator [mailto:revi...@reviews.llvm.org]
Sent: Monday, July 10, 2017 11:57 AM
To:
mibintc added a comment.
OK folks, I was off the grid last week but I'm back now, and at my grindstone
again.
I haven't received any comments since I updated the patch to remove the checks
on "-nostdinc".
Look OK to commit?
Many thanks for all your reviews
--Melanie
Repository:
rL LLVM
mibintc updated this revision to Diff 105904.
mibintc edited the summary of this revision.
mibintc added a comment.
I removed the check on -nostdinc; and made some updates to the test cases
Repository:
rL LLVM
https://reviews.llvm.org/D34158
Files:
include/clang/Driver/CC1Options.td
mibintc added a comment.
In https://reviews.llvm.org/D34158#803752, @jyknight wrote:
> This version is still disabling upon -nostdinc, which doesn't make sense to
> me.
>
> You didn't remove that, nor respond explaining why you think it does make
> sense?
You're right, I should remove the
hfinkel added a comment.
In https://reviews.llvm.org/D34158#803752, @jyknight wrote:
> This version is still disabling upon -nostdinc, which doesn't make sense to
> me.
I agree. If I pass -nostdinc, so that I then arrange include paths for my own
libc headers, I'd then like to pick up the
jyknight added a comment.
This version is still disabling upon -nostdinc, which doesn't make sense to me.
You didn't remove that, nor respond explaining why you think it does make sense?
https://reviews.llvm.org/D34158
___
cfe-commits mailing list
23 matches
Mail list logo
