-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ale-cpp] Linting in headers don't work. Add option to run clang tools in source file. #782
Comments
We could consider creating temporary .c files for checking header files which include the header files. There are two problems with this.
We could write the temporary files with a It sounds like this problem should really be fixed in |
It is a long standing problem, even cmake, which generates the compilation_database knows nothing about headers. There is this clangd (Language protocol server by clang) that might solve most of these problems. But still on development. One non-automated solution would be to let the user choose the file included in the compilation database which uses the header. YouCompleteMe does some sophisticated thing to guess headers, but still it doesn't work when the class in the header uses templates ( a lot of them). Pointing to a source file will solve this, because the template is instantiated there. It is not an easy problem, and not the job of Ale to solve it, but it would be super nice to have something, with the flexibility of your plugin. |
Could we just stop setting the |
This is all yet another reason to want modules to become part of the C++ standard. |
Sounds good, and the list for headers extensions should be flexible (there are .hxx, .txx, .tcc, ...). #725 sounds reasonable, but still, has a problem, how do you choose the source file to get the relevant arguments for the current header? And even with the right ones, what happen with non-instantiated templates? Isn't the checker going to ignore them in that case? Relevant conversation in YCM when this arose: ycm-core/YouCompleteMe#174. I, as an ALE user, would be happy to setlocal a variable in the .h buffer, pointing to the relevant source (included in the compilation database to get the flags) that actually use that header. If the project is long/complex enough, maybe even creating a vimL auto-command doing this mapping automatically instead of manually. |
You could use |
Can I change the target file where run the checker for a file different than the current? |
I don't understand your last message. You can change any buffer-local option with |
Testing it with no luck:
Being editing
|
The guy who implemented the build directory options made it so any additional options are ignored if the build directory is detected. This is because you can't set additional options if you're using a We might consider adding another option to disable using |
I pushed a commit now which makes it so the |
Passing a |
* upstream/master: dense-analysis#782 - Do not set the build directory for clang-tidy for header files, which does not work Update the installation notes for the standard package management sysytem so helptag generation will actually work Fix dense-analysis#786 - Only set --no-local-style for yapf if a configuration file is detected
Provide extra logic to handle header files in cpp. Only using clangtidy, but same workaround will work in other clang checkers. First, provide a list of headers suffixes: By default: let g:cpp_clangtidy_header_suffixes = ['h', 'hpp', 'hxx', 'tcc'] And then, use a variable specific for headers: let b:cpp_clangtidy_header_sourcefile = '' This variable should point to a source file (a .cpp) which uses the current header. This source file could be set by the user when editing the buffer, with a simple: let b:cpp_clangtidy_header_sourcefile Or set a g:ale_pattern_options in a project vimL file. let g:ale_pattern_options = { \ 'my_favourite_header\.h$': { \ 'ale_linters': {'cpp': ['clangtidy']}, \ 'ale_cpp_clangtidy_header_sourcefile': '/path/test_header.cpp', \ }, \} Related: dense-analysis#782
Provide extra logic to handle header files in cpp. Only using clangtidy, but same workaround will work in other clang checkers. First, provide a list of headers suffixes: By default: let g:cpp_clangtidy_header_suffixes = ['h', 'hpp', 'hxx', 'tcc'] And then, use a variable specific for headers: let b:cpp_clangtidy_header_sourcefile = '' This variable should point to a source file (a .cpp) which uses the current header. This source file could be set by the user when editing the buffer, with a simple: let b:cpp_clangtidy_header_sourcefile Or set a g:ale_pattern_options in a project vimL file. let g:ale_pattern_options = { \ 'my_favourite_header\.h$': { \ 'ale_linters': {'cpp': ['clangtidy']}, \ 'ale_cpp_clangtidy_header_sourcefile': '/path/test_header.cpp', \ }, \} Related: dense-analysis#782
I have created a PR with an option I have tested with: And ALEInfo:
|
I closed that pull request. I don't like the solution, I think it's much too complicated, and that's a recipe for creating a series of further issues. I think we should either not check header files with |
I understand, it is hack. clang-tools works with cpp files. I will use it though, I work most of the time in headers with templates. And thanks for the review, first time with vimL. In terms of checking for the directory of the headers, usually all the headers are in an |
I wonder what tools are best to use for this. If you're writing a lot of generic code, you're probably looking at headers almost entirely. If you ever need to, you can also add your own custom linter in |
I'm not sure clang-tools will be able to deal with this issue. I looked a little bit into Rip-Rip/clang_complete plugin because the doc speaks about the header issue, and apparently, the workaround is to use actual clang to run a dry compilation and passing the -header flag :
My main problem with this investigation is that I can't find information on either |
There is the concept of "companion files" which are files with the same basename, one cpp and one h. A simple option would be to add a flag that when turned on does
If myfile.cpp exist when opening myfile.h |
A workaround right now would be to create a clang-tidy wrapper that just does this check and then runs
|
Python is not my language but I took a swing at making a simple wrapper. Works for simple cases: ...except ALE removes the compilation database from the options. Is there a flag to turn that "feature" off ? |
I don't know what you're talking about, so I don't know. I know that Just about all code in this repository must be VimL only. |
I meant that you did a change so that "-p " is not added when running clang-tidy on a header; |
I'm not sure I understand either. The help from clang-tidy seem to tell that
Therefore, we still have the same issue (How can we actually run the clang-tools checking header files with the correct include and compilation flags from a compilation database). Also, clang-check does not have any options like this. And the workaround you propose ( Do you have a MWE to show how we can make it work ? Like 1 cpp, 1 header, 1 compilation database so that I think our best shot is to parse the compilation database and try to guess relevant folder from here #725 but the PR does not advance much these days (time is a rare resource) |
So I've noticed that if you run the linter on a cpp file and then later on the header, for a moment, the correct errors will show up in the header, before being replaced by garbage. As a temporary solution it seems like having header files do the callbacks when you call :ALELint without actually trying to lint would at least give the correct errors/warnings. It's not a real solution, but since linting headers doesnt work at all right now, and i'm guessing (maybe?) it wouldnt be too hard to implement, it might be worth doing. |
When you 'lint' cpp files, you call a compiler so you can check the translation unit it's associated with. This program will find some errors and warnings, linked to lines written in the headers they include, so you get hits for these lines. It seems that what's happening on what you describe is that you see the warnings associated to the implementation file you just linted, and they just disappear after some time because an event triggered The current issue with header files (if you want to find "errors" and "warnings" that are not just whitespace changes), is that we need a translation unit to try to "compile" the code and see if warnings and issues arise. I don't know if it is that easy. Usually, to get the hits inside a header file, you need to try to compile the files including them. So there seem to be only 2 solutions in my opinion :
Both solutions do not look that easy to implement efficiently. EDIT : I commented without really checking the whole thread because I thought my answer would be short, I hope I don't repeat myself too much |
yeah, which is why as a temporary measure you let :ALELint in header files only call the callbacks and skip trying to lint, so if you've run the linter in the implementation file, you get warnings and errors in the associated header. it's not a real solution, because it requires linting the implementation file manually, but it lets you lint a header. |
Until the compile_commands.json doesn't include headers this task is complicated. I tried a workaround that I personally use, but I understand it is not the best solution: phcerdan@5d9f6f9 There you set in the local buffer of your header: And ALE will compile the source file when your header is modified, that will lint your current header. |
I think the solution for |
That automatic lookup is something YouCompleteMe uses as well, and it is good. |
Okay, sounds like a good idea. I'll give it a go when I can. |
The problem with the look up is that cpp doesn't impose a convention in structure as rust (for example) does. |
Yes, that's the big problem with C++. There are no standards for these things. If the standards existed, the this problem would have already been solved in some way. I think the best you can do is try to look for what the majority of people use, and make most people happy. |
Thank you so much for ale, it's really great!
if and only if I turn on
clang-tidy should get what it needs vom compile-commands.json. |
Open an issue for that. |
Using the compile_commands files for header files is not as easy as it can be, since they contain only compilation commands (so only matching implementation files, not header files) @phcerdan exposed the quickest reasonable solution in an earlier comment, guessing the good implementation files to lint in order to get the warnings in a selected header file is no easy issue. This is hard because a header file can be included potentially in the whole project so you'd need to actually compile everything to be sure to catch all clang-tidy errors in the header. At this point I really do think it is more sensible to either use the fix mentioned above, or maybe wait and hope that LSP can provide ways of generating this file list like https://github.com/Sarcasm/compdb is apparently aiming to do, but without having to add another dependency on the user's side. |
I don't really understand that reasoning. However, I can't get ale to use my compilation database at all right now, not even for the |
What might work for this is using |
Provide extra logic to handle header files in cpp. Only using clangtidy, but same workaround will work in other clang checkers. First, provide a list of headers suffixes: By default: let g:cpp_clangtidy_header_suffixes = ['h', 'hpp', 'hxx', 'tcc'] And then, use a variable specific for headers: let b:cpp_clangtidy_header_sourcefile = '' This variable should point to a source file (a .cpp) which uses the current header. This source file could be set by the user when editing the buffer, with a simple: let b:cpp_clangtidy_header_sourcefile Or set a g:ale_pattern_options in a project vimL file. let g:ale_pattern_options = { \ 'my_favourite_header\.h$': { \ 'ale_linters': {'cpp': ['clangtidy']}, \ 'ale_cpp_clangtidy_header_sourcefile': '/path/test_header.cpp', \ }, \} Related: dense-analysis#782
As a note, I have rebased my workaround on top of current master. Maybe instead of modifying clangtidy, this could be called clangtidy-with-headers or similar. Minimal example to test it: https://gist.github.com/phcerdan/ee005812a62753214124783ee4e14256 |
Provide extra logic to handle header files in cpp. Only using clangtidy, but same workaround will work in other clang checkers. First, provide a list of headers suffixes: By default: let g:cpp_clangtidy_header_suffixes = ['h', 'hpp', 'hxx', 'tcc'] And then, use a variable specific for headers: let b:cpp_clangtidy_header_sourcefile = '' This variable should point to a source file (a .cpp) which uses the current header. This source file could be set by the user when editing the buffer, with a simple: let b:cpp_clangtidy_header_sourcefile Or set a g:ale_pattern_options in a project vimL file. let g:ale_pattern_options = { \ 'my_favourite_header\.h$': { \ 'ale_linters': {'cpp': ['clangtidy']}, \ 'ale_cpp_clangtidy_header_sourcefile': '/path/test_header.cpp', \ }, \} Related: dense-analysis#782
Provide extra logic to handle header files in cpp. Only using clangtidy, but same workaround will work in other clang checkers. First, provide a list of headers suffixes: By default: let g:cpp_clangtidy_header_suffixes = ['h', 'hpp', 'hxx', 'tcc'] And then, use a variable specific for headers: let b:cpp_clangtidy_header_sourcefile = '' This variable should point to a source file (a .cpp) which uses the current header. This source file could be set by the user when editing the buffer, with a simple: let b:cpp_clangtidy_header_sourcefile Or set a g:ale_pattern_options in a project vimL file. let g:ale_pattern_options = { \ 'my_favourite_header\.h$': { \ 'ale_linters': {'cpp': ['clangtidy']}, \ 'ale_cpp_clangtidy_header_sourcefile': '/path/test_header.cpp', \ }, \} Related: dense-analysis#782
PR #2272 seems to have fixed linting header files with clang/gcc linters when compilation database is used. |
It fixed a bug, but not this "issue". |
In a project I was working on, compile_commands.json didn't include entries for headers where there are entries for the .c files that go with them. Now ALE will fall back on using arguments for source files with a similar name to a header file. |
I don't see the behaviour described in the last patch. It seems that
Seems like the previous behaviour of ignoring build directories when in a header file is still present. The following patch will fix it. diff --git a/autoload/ale/c.vim b/autoload/ale/c.vim
index 9b42870..8202afe 100644
--- a/autoload/ale/c.vim
+++ b/autoload/ale/c.vim
@@ -9,13 +9,6 @@ let s:sep = has('win32') ? '\' : '/'
let g:__ale_c_project_filenames = ['.git/HEAD', 'configure', 'Makefile', 'CMakeLists.txt']
function! ale#c#GetBuildDirectory(buffer) abort
- " Don't include build directory for header files, as compile_commands.json
- " files don't consider headers to be translation units, and provide no
- " commands for compiling header files.
- if expand('#' . a:buffer) =~# '\v\.(h|hpp)$'
- return ''
- endif
-
let l:build_dir = ale#Var(a:buffer, 'c_build_dir')
" c_build_dir has the priority if defined |
There is already a MR open for this. #2919 |
Almost all C/C++ issues with flags should now be discussed in #3276, so a comprehensive solution can be found for most people, for automatically detecting flags. |
Clang tools, like clang-tidy, clang-check, don't show errors when run in headers, even with a compilation_database (the compilation database doesn't include a compile command for headers)
This is not an ALE problem, but it would be great to add options for a workaround. One option would to run the tool instead of pointing to the current header, pointing to the source file (which is compiled) including the header file.
so:
The text was updated successfully, but these errors were encountered: