[PATCH] D64089: [Driver] Introduce -stdlib++-isystem

[Louis Dionne via Phabricator via cfe-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: dexonsmith.

This looks fine to me, but I'd like to have someone else familiar with the Driver/Frontend design sign off too. The implementation is trivial but I'd like support on the fact that this is a good idea (which I think it is).



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1307
+          HasStdlibxxIsystem ? TC.AddClangCXXStdlibIsystemArgs(Args, CmdArgs)
+                             : TC.AddClangCXXStdlibIncludeArgs(Args, CmdArgs);
+        });
----------------
smeenai wrote:
> ldionne wrote:
> > So, before, we would populate `-stdlib=XXX` here unconditionally for `cc1`.
> > 
> > After the patch, we do _not_ populate `-stdlib=XXX` and instead we pass `-internal-isystem` to `cc1` with the contents of `-stdlib++-isystem` whenever that option is provided, otherwise we fall back to the previous behaviour.
> > 
> > Did I get that right? If so, could we investigate getting rid of `AddClangCXXStdlibIncludeArgs` altogether? I don't think it is needed anymore since my refactor of the driver for Darwin.
> Sorry, I'd missed this. You're right; we'll continue passing `-stdlib=XXX` in the old case (i.e. when there's no `-stdlib++-isystem` argument), but not if it's there.
> 
> When you say getting rid of `AddClangCXXStdlibIncludeArgs`, that's the one in the driver (in the toolchains) right? I thought your Darwin refactor involved moving C++ header search logic from the frontend into the driver ... did you mean getting rid of the C++ header search path logic in the frontend?
Sorry, I should have been more clear. I meant specifically getting rid of this: https://github.com/llvm-project/clang/blob/master/lib/Driver/ToolChain.cpp#L832

In that snippet, we pass `-stdlib=XXX` down to the frontend. My understanding is that this was only necessary because the frontend on Darwin needed to know which stdlib was in use, in order to setup the header search paths correctly. Now that it's not needed anymore, I think we could get rid of that hack altogether.

However, looking at it further, changing that is way beyond the scope of your PR. I had not realized that other non-Darwin toolchain provided implementations of `AddClangCXXStdlibIncludeArgs` (which is `virtual` and meant to be overridden).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64089/new/

https://reviews.llvm.org/D64089