Hahnfeld added a subscriber: cfe-commits.
Hahnfeld edited reviewers, added: rsmith, rengolin; removed: cfe-commits.
Hahnfeld added a comment.
Some comments inline. In general you should consider posting an RFC on cfe-dev
because this change will basically affect all compilations on GNU/Linux if the
file is present.
Adding Richard (general maintainer) and Renato (ARM Linux) so they are aware.
================
Comment at: include/clang/Driver/ToolChain.h:459-462
+ /// \brief Add arguments to use system-specific GNU includes.
+ virtual void AddGnuIncludeArgs(const llvm::opt::ArgList &DriverArgs,
+ llvm::opt::ArgStringList &CC1Args)
+ const;
+
----------------
Why do you need to define this method in the generic `ToolChain`?
================
Comment at: lib/Driver/ToolChains/Gnu.cpp:2332-2349
+void Generic_GCC::addGnuIncludeArgs(const ArgList &DriverArgs,
+ ArgStringList &CC1Args) const {
+ const Generic_GCC::GCCVersion &Version =
+GCCInstallation.getVersion();
+ if (!DriverArgs.hasArg(options::OPT_ffreestanding) &&
+ !DriverArgs.hasArg(clang::driver::options::OPT_nostdinc) &&
+ !Version.isOlderThan(4, 8, 0)) {
+ // If stdc-predef.h exists in the sytem includes, then -include it.
----------------
If this is GNU/Linux only, why not move to the `Linux` toolchain entirely?
================
Comment at: lib/Driver/ToolChains/Gnu.h:328-330
+ void addGnuIncludeArgs(const llvm::opt::ArgList &DriverArgs,
+ llvm::opt::ArgStringList &CC1Args) const;
+
----------------
This starts with a lower-case character and won't override the method in
`ToolChain`. Adding the keyword `override` should reveal this mistake.
================
Comment at: test/Driver/gcc-predef.c:1
+// Test that gcc preincludes stdc-predef.h on Linux //
----------------
s/gcc/clang/ ?
================
Comment at: test/Driver/gcc-predef.c:9
+ #else
+ #if !defined( _STDC_PREDEF_H )
+ #error "stdc-predef.h should be preincluded for GNU/Linux 4.8 and higher"
----------------
The driver will only include `stdc-predef.h` if it can be found in the system.
With that, the current version of this test will only run on such Linux system.
Maybe add a basic tree in `test/Driver/Inputs` and test that the corresponding
header is included?
>>>OK I understand what you mean now.
================
Comment at: test/Driver/gcc-predef.c:14-17
+// Test that gcc preincludes stdc-predef.h on Linux // // RUN: %clang
+-c --target=i386-unknown-linux %s -Xclang -verify //
+expected-no-diagnostics
----------------
What's the difference to the test above?
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
