LGTM
On Tue, Apr 12, 2016 at 11:40 AM, Nico Weber <tha...@chromium.org> wrote: > All done. phab's back, but since we started with a patch attachment, let's > keep it that way. > > > > On Tue, Apr 12, 2016 at 12:12 PM, Hans Wennborg <h...@chromium.org> wrote: >> >> On Mon, Apr 11, 2016 at 7:16 PM, Nico Weber <tha...@chromium.org> wrote: >> > r260990 exposed -isystem in clang-cl. -isystem adds a directory to the >> > front >> > of the system include search path. The idea was to use this to point to >> > a >> > hermetic msvc install, but as it turns out this doesn't work: -isystem >> > then >> > adds the hermetic headers in front of clang's builtin headers, and >> > clang's >> > headers that are supposed to wrap msvc headers (say, stdarg.h) aren't >> > picked >> > up at all anymore. >> > >> > So revert that, and instead expose -imsvc which works as if the passed >> > directory was part of %INCLUDE%: The header is treated as a system >> > header, >> > but it is searched after clang's lib/Header headers. >> >> Sounds great. >> >> I wonder if this is useful beyond clang-cl and should have a more >> general name, but I can't think of anything, so let's go with this. >> >> > Also expose -nostdlibinc so that clang-cl can be told to not look for >> > system >> > headers in the usual places. >> >> Can you land this in a separate commit? >> >> >> > With this change, it's possible to use -imsvc to point clang-cl at a >> > hermetic MSVC installation and not make it look at the headers of a >> > possibly >> > installed msvc. Fixes PR26751. >> > >> > I'd upload this to phab, but it's currently down. >> >> Old-school review below: >> >> > Index: include/clang/Driver/CLCompatOptions.td >> > =================================================================== >> > --- include/clang/Driver/CLCompatOptions.td (revision 265325) >> > +++ include/clang/Driver/CLCompatOptions.td (working copy) >> > @@ -210,6 +210,8 @@ >> > HelpText<"Enable exception handling">; >> > def _SLASH_GX_ : CLFlag<"GX-">, >> > HelpText<"Enable exception handling">; >> > +def _SLASH_imsvc : CLJoinedOrSeparate<"imsvc">, >> > + HelpText<"Add directory to system include search path, as if part of >> > %INCLUDE%">, MetaVarName<"<dir>">; >> >> Nit: I'd have gone with a line break before MetaVarName to pretend >> we're at least trying to keep the lines short. >> >> > Index: include/clang/Driver/Options.td >> > =================================================================== >> > --- include/clang/Driver/Options.td (revision 265325) >> > +++ include/clang/Driver/Options.td (working copy) >> > @@ -1246,7 +1246,7 @@ >> > def isysroot : JoinedOrSeparate<["-"], "isysroot">, >> > Group<clang_i_Group>, Flags<[CC1Option]>, >> > HelpText<"Set the system root directory (usually /)">, >> > MetaVarName<"<dir>">; >> > def isystem : JoinedOrSeparate<["-"], "isystem">, Group<clang_i_Group>, >> > - Flags<[CC1Option, CoreOption]>, >> > + Flags<[CC1Option]>, >> > HelpText<"Add directory to SYSTEM include search path">, >> > MetaVarName<"<directory>">; >> > def iwithprefixbefore : JoinedOrSeparate<["-"], "iwithprefixbefore">, >> > Group<clang_i_Group>, >> > HelpText<"Set directory to include search path with prefix">, >> > MetaVarName<"<dir>">, >> > @@ -1673,7 +1673,7 @@ >> > def noseglinkedit : Flag<["-"], "noseglinkedit">; >> > def nostartfiles : Flag<["-"], "nostartfiles">; >> > def nostdinc : Flag<["-"], "nostdinc">; >> > -def nostdlibinc : Flag<["-"], "nostdlibinc">; >> > +def nostdlibinc : Flag<["-"], "nostdlibinc">, Flags<[CoreOption]>; >> >> Commit this one separately. >> >> > Index: lib/Driver/MSVCToolChain.cpp >> > =================================================================== >> > --- lib/Driver/MSVCToolChain.cpp (revision 265325) >> > +++ lib/Driver/MSVCToolChain.cpp (working copy) >> > @@ -527,6 +527,10 @@ >> > "include"); >> > } >> > >> > + // Add %INCLUDE%-like directories from the -systemI flag. >> > + for (const auto &path : >> > DriverArgs.getAllArgValues(options::OPT__SLASH_imsvc)) >> > + addSystemInclude(DriverArgs, CC1Args, path); >> >> Update flag name in the comment. Spelling the loop variable Path would >> be more LLVM-style. >> >> > Index: test/Driver/cl-options.c >> > =================================================================== >> > --- test/Driver/cl-options.c (revision 265325) >> > +++ test/Driver/cl-options.c (working copy) >> > @@ -82,6 +82,10 @@ >> > // RUN: %clang_cl /I myincludedir -### -- %s 2>&1 | FileCheck >> > -check-prefix=SLASH_I %s >> > // SLASH_I: "-I" "myincludedir" >> > >> > +// RUN: %clang_cl /imsvcmyincludedir -### -- %s 2>&1 | FileCheck >> > -check-prefix=SLASH_imsvc %s >> > +// RUN: %clang_cl /imsvc myincludedir -### -- %s 2>&1 | FileCheck >> > -check-prefix=SLASH_imsvc %s >> > +// SLASH_imsvc: "-internal-isystem" "myincludedir" >> >> Would it be possible to check that this flag comes after the >> -internal-isystem for the resource headers? > > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
