[PATCH] D147905: [clangd] Avoid passing -xobjective-c++-header to the system include extractor

Sat Apr 15 00:35:50 PDT 2023

nridge added a subscriber: dgoldman.
nridge added inline comments.

================
Comment at: clang-tools-extra/clangd/SystemIncludeExtractor.cpp:340
+    // is not installed.
+    if (Lang == "objective-c++-header") {
+      Lang = "c++-header";
----------------
kadircet wrote:
> nridge wrote:
> > kadircet wrote:
> > > this feels like too much of a layering violation and might (will?) go wrong in cases where language was explicitly set to `objective-c++-header`.
> > > 
> > > if the user is relying on fallback commands with an overwrite of `Compiler:` in the config && --query-driver globs, would it be too much of a hassle to expect them to have a `CompileFlags: Add: ...` block too?
> > > this feels like too much of a layering violation and might (will?) go wrong in cases where language was explicitly set to `objective-c++-header`.
> > 
> > This has occurred to me, and my first idea for a fix was to limit this change to cases where the `-xobjective-c++-header` originates from the fallback command.
> > 
> > However, as mentioned [here](https://github.com/clangd/clangd/issues/1568#issuecomment-1493236437), when I tested this I found that `-xobjective-c++-header` did not make any difference (compared to `-xc++-header` or  `-xc++`) in the include paths returned by gcc. In other words, in gcc's include directory structure there are no objc-specific directories. This made me think this simpler fix would be appropriate.
> > 
> > > if the user is relying on fallback commands with an overwrite of `Compiler:` in the config && --query-driver globs, would it be too much of a hassle to expect them to have a `CompileFlags: Add: ...` block too?
> > 
> > You're right, adding a section like this to the config does seem to be a viable workaround:
> > 
> > ```
> > ---
> > 
> > If:
> >   PathMatch: *\.h
> > 
> > CompileFlags:
> >   Add: [-xc++-header]
> > ```
> > 
> > But I think it would still be nice to fix this in clangd, as being foiled by objective-c support not being installed is a very unexpected failure mode for a user whose project does not involve objective-c at all.
> > 
> > For what it's worth, I don't think this kind of setup is uncommon. A common scenario seems to be a casual user playing around with a small project (hence, doesn't have a build system or compile_commands.json), on a platform where --query-driver is needed to find the standard library headers (most commonly, MinGW on Windows).
> > However, as mentioned [here](https://github.com/clangd/clangd/issues/1568#issuecomment-1493236437), when I tested this I found that `-xobjective-c++-header` did not make any difference (compared to `-xc++-header` or  `-xc++`) in the include paths returned by gcc. In other words, in gcc's include directory structure there are no objc-specific directories.
> 
> Well, that's definitely re-assuring, but I am not sure if it's enough to say it'll work that way with all gcc's or when there are other/certain "system" libraries installed. As in theory objc compilation should at least add some framework search paths and what not by default, no?
> 
> > But I think it would still be nice to fix this in clangd, as being foiled by objective-c support not being installed is a very unexpected failure mode for a user whose project does not involve objective-c at all.
> 
> Completely agree, but we're only showing that to people that already fiddled with clangd internals. So I don't think that as  unacceptable.
>  
> > For what it's worth, I don't think this kind of setup is uncommon. A common scenario seems to be a casual user playing around with a small project (hence, doesn't have a build system or compile_commands.json), on a platform where --query-driver is needed to find the standard library headers (most commonly, MinGW on Windows).
> 
> I think instead of trying to make things work with query-driver in such setups, we should try to make sure things work out-of-the-box in mingw (and other toolchain) setups. I believe people not using query-driver in such vanilla installation is way more common than people using query-driver and `CompileFlags.Compiler` override. Also this will probably make sure other clang-tools can work with those setups too.
> We have mingw toolchain detection [here](https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/MinGW.cpp).
> > However, as mentioned [here](https://github.com/clangd/clangd/issues/1568#issuecomment-1493236437), when I tested this I found that `-xobjective-c++-header` did not make any difference (compared to `-xc++-header` or  `-xc++`) in the include paths returned by gcc. In other words, in gcc's include directory structure there are no objc-specific directories.
> 
> Well, that's definitely re-assuring, but I am not sure if it's enough to say it'll work that way with all gcc's or when there are other/certain "system" libraries installed. As in theory objc compilation should at least add some framework search paths and what not by default, no?

To be honest, I don't know enough about objective-c to say either way.

Perhaps @dgoldman can help us answer this question: would you expect the `-x objective-c++` flag to cause the compiler to use any additional / objective-c specific built-in include directories (compared to `-x c++`), for any compiler you're aware of that has a gcc-compatible driver syntax?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147905