Add .inl as valid C++ header type

[abbaswasim]

Summary

clangd doesn't consider .inl a valid C++ source (https://github.com/clangd/clangd/issues/16#issuecomment-512942589) this pull request adds support for that.

Until the export keyword is supported by compilers mentioned https://stackoverflow.com/questions/5416872/using-export-keyword-with-templates a common way of keeping header files cleaner is to move template definitions into .inl files. This seems to be a common practice. Some evidence of it being commonly used for this purpose.

https://www.randygaul.net/2015/01/31/c-keyword-inline-and-inl-files/
https://drake.mit.edu/cxx_inl.html
https://www.codeproject.com/Articles/48575/How-to-define-a-template-class-in-a-h-file-and-imp
https://stackoverflow.com/questions/31949731/very-confused-on-template-inl-file
even brdf-loader https://github.com/rgl-epfl/brdf-loader

Some other ways of separating definitions is https://github.com/RobotLocomotion/drake/issues/3141 where they say you should use .inl or .impl or -inl.h or -impl.h and this might be ok but then things like projectile can't find it because that only works with extensions.

One caveat of this is, the above methods of using .inl files means its not a complete translation unit so clangd still can't fully compile it so one has to add #include "header.hpp" at the top of .inl files to make it work but thats not a big deal.

[abbaswasim]

Hi All,

I am not fully aware of the process here could anyone point me to what is the next step? Has anyone looked at this Patch?

//Wasim

[nridge]

Adding @sammccall as reviewer

This is a simple change that would benefit clangd users (it just came up again recently, on Stack Overflow)

[aaron.ballman]

This seems to be a common practice.

FWIW, I did a search over github repos and this file extension is used, but I would hardly call it a common practice (compared to other extensions we don't support, like .inc). It does seem to be used as a header-like file, predominately, but I'm not really certain whether there is sufficient evidence that we should treat this extension as always being a header file given that it's not a super common extension (it doesn't help that I've never seen that extension used in the wild before; today was the first time I learned of it). My understanding is that we're usually pretty conservative with this list, and this is why we have the -x option so that users can specify a specific language mode to use. Is there a reason that option does not suffice for clangd?

[abbaswasim]

FWIW, I did a search over github repos and this file extension is used, but I would hardly call it a common practice

Fair enough. what would be sufficient evidence?

I think in general whether its .inl or any other extension thats used to separate template implementations out of a header needs a solution. I have started using .hh as a workaround but it not being a complete translation unit is still a problem. And clangd also complains about recursive includes in the .hh file.

[abbaswasim]

Also I couldn't make the -x option to work with lsp-mode but that's not clangds problem.

[aaron.ballman]

In D67025#3665301, @abbaswasim wrote:

FWIW, I did a search over github repos and this file extension is used, but I would hardly call it a common practice

Fair enough. what would be sufficient evidence?

I'm not certain we have any sort of agreed-upon measure for it. Personally, I'd want to see evidence that several other programming tools also treat the extension the same way. I notice that MSVC's file open dialog considers .inl to be a "Visual C++ File" and its save dialog considers .inl to be a "C++ Header File", so that's a good sign. I also notice that TextPad also considers .inl to be a C/C++ file (doesn't distinguish between language or what kind of file though). Does XCode have something similar? Notepad++ or other text editors? If so, I think that qualifies as sufficient evidence.

I think in general whether its .inl or any other extension thats used to separate template implementations out of a header needs a solution. I have started using .hh as a workaround but it not being a complete translation unit is still a problem. And clangd also complains about recursive includes in the .hh file.

I've seen: .inc, .def, .hh,, and .H all within the past few years but in each case, it meant roughly the same thing "source that gets included but isn't intended as a stand-alone header file" (though I've certainly seen .H and .hh used to mean "C++ header file" as opposed to "C header file").

[abbaswasim]

AFAICT Xcode understands and reads it as c++ header file. It highlights it fine and icon is "h+".

[sammccall]

(sorry i thought i sent these comments earlier)

This is a change to clang rather than clangd, so it affects the behavior of the compiler and other clang-based tools.
While it might do something useful in clangd, I'm skeptical of its general applicability because with this extension we do not expect that such files will be parseable.

I think the evidence you've found is that editors (e.g. clangd!) should handle these files, but not necessarily compilers.

one has to add #include "header.hpp" at the top of .inl files to make it work but thats not a big deal.

It may not be a big burden to add it to code that you own, but making the compiler misinterpret other people's code where this #include isn't present can be a problem.

I think ideally we'd express this behavior at the clangd level instead. Something like "we suspect this file contains $LANGUAGE, so if the compile flags/extension don't imply anything particular, treat it that way".
As well as being less intrusive, this is a more general, useful feature:

• LSP allows the editor to specify the language, typically based in editor UI, so this would give the user control over whether such files were treated as C/C++/ObjC/ObjC++ etc rather than hard-coding C++. (We can still have a default).
• this needn't be specific to suffix ".inl". LLVM uses ".inc", I'm sure other codebases use other extensions.

(And the reason i never sent the comment is i couldn't come to a conclusion on the best way to do this inside clangd. Needs some more thought...)

[nridge]

In D67025#3665293, @aaron.ballman wrote:

this is why we have the -x option so that users can specify a specific language mode to use. Is there a reason that option does not suffice for clangd?

One reason is that the most commonly used tools that generate compile_commands.json files, like CMake, do not create entries for header files. Users need to turn to ad-hoc post-processing or third-party tools like CompDB to add entries for header files, to even have a place to put the -x c++header option.

[MaskRay]

I agree that this is not suitable in clang, but can be in clangd.

[sammccall]

! In D67025#3665293, @aaron.ballman wrote:
this is why we have the -x option so that users can specify a specific language mode to use. Is there a reason that option does not suffice for clangd?

The difficulty here is that command-line flags are a very awkward/brittle interface but also the only one we have.

The situation we're in is:

• we have filename and a collection of clang flags mashed together from build system, config, and defaults
• we need a CompilerInvocation that reflects those flags... (which only Driver can provide)
• *except* that if the extension and flags don't imply a language then treat it as "c++-header" by default, rather than failing

It's very difficult to robustly manipulate the flags to achieve these effects, basically needing a model of the driver's behavior (identify every flag that can affect the parse language, remember which flags are global and which affect following filenames, and guarantee these rules never change).

What would solve the problem is a flag like -xfallback=c++-header to change the driver's behavior but adding that to clang is ugly.
What we may end up with is a bunch of heuristics about whether to add -x or not, which are complicated and usually buggy. We have lots of these.
I wish I had a better answer...

[tschuett]

Note that in this directory is an ,inl file:
https://github.com/openucx/ucx/tree/master/src/uct/ib/mlx5
It is a pure C library with C++ gtest.

I believe that .inl is about inlining, but it is not tied to a language.

[aaron.ballman]

In D67025#3666513, @sammccall wrote:

! In D67025#3665293, @aaron.ballman wrote:
this is why we have the -x option so that users can specify a specific language mode to use. Is there a reason that option does not suffice for clangd?

The difficulty here is that command-line flags are a very awkward/brittle interface but also the only one we have.

Thank you for the explanation (and thanks to @nridge as well). That is an awkward situation. :-(

In D67025#3666544, @tschuett wrote:

Note that in this directory is an ,inl file:
https://github.com/openucx/ucx/tree/master/src/uct/ib/mlx5
It is a pure C library with C++ gtest.

I believe that .inl is about inlining, but it is not tied to a language.

This captures my fear about trying to nail down this extension in the compiler. File extensions are the best we've got for identifying the purpose for a file, and .inl sounds a lot like it's used somewhat as a header file and somewhat as an implementation file, which means it's close in nature to a .inc or .def file. We've traditionally avoided trying to classify these because of the chances of getting the behavior wrong. That said, in all of these cases, the most common usage pattern is to include the file inside of another file rather than compile it directly, which does map fairly closely to header files.

I'm not certain what the best answer is.

clang/lib/Driver/Types.cpp

return llvm::StringSwitch<types::ID>(Ext)
            .Case("s", TY_PP_Asm)
            .Case("bc", TY_LLVM_BC)
            .Case("cc", TY_CXX)
            .Case("CC", TY_CXX)
            .Case("cl", TY_CL)
            .Case("cp", TY_CXX)
            .Case("cu", TY_CUDA)
            .Case("hh", TY_CXXHeader)
+           .Case("inl", TY_CXXHeader)
            .Case("ii", TY_PP_CXX)
            .Case("ll", TY_LLVM_IR)
            .Case("mi", TY_PP_ObjC)
            .Case("mm", TY_ObjCXX)
            .Case("rs", TY_RenderScript)
            .Case("adb", TY_Ada)
            .Case("ads", TY_Ada)
            .Case("asm", TY_PP_Asm)