Added builddir option to clang-tidy to point to json folder by gagbo · Pull Request #688 · dense-analysis/ale

gagbo · 2017-06-23T14:43:52Z

Related to #392 but does not fix it (it only gives clang-tidy the possibility to read properly compilation db in order to find all includes)

Clangtidy linter currently deactivates the look for json compilation databases if the option cpp_clangtidy_options is set to a non-empty string. This is problematic when the only option we want to pass to clangtidy is actually a compilation database.

To compensate for this, another option is added to clangtidy linter, named ale_cpp_clangtidy_builddir that specifically only takes the path to the build directory. Setting this option will nullify all other ale_cpp_clangtidy_options set, since finding a compilation database means we have all the compilation flags we care about. (Also this behaviour means we can have default flags in the vimrc, that will be overwritten by compilation db flags for the specific projects/folders that do support it).

Only the vader test is missing, the only fringe case I can imagine is to verify that when both sets, clang-tidy effectively takes the builddir option. I'll look into vader folder to try and implement the test.

w0rp · 2017-06-23T14:47:31Z

It would be better to search for this automatically with a list of names for the build directory, which can be changed with an option. Set the option to ['bin', 'build'] by default, and search upwards through directories to find 'bin/compile_commands.json', etc.

gagbo · 2017-06-23T16:01:56Z

If I run CMake in the build dir and have CMAKE_EXPORT_COMPILE_COMMANDS on, then the json is directly in the build folder and not in a bin subdirectory. Maybe just searching upwards for build folder and then just check for an existing compilé_commands.json ? That would mean using only 1 string for the option Le 23 juin 2017 16:47, "w0rp" <notifications@github.com> a écrit :

…

It would be better to search for this automatically with a list of names for the build directory, which can be changed with an option. Set the option to ['bin', 'build'] by default, and search upwards through directories to find 'bin/compile_commands.json'. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#688 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKAoo1HkaDvtnareS3VQyBvaikXpx4Prks5sG9AFgaJpZM4ODqGP> .

w0rp · 2017-06-23T16:04:41Z

In that case, just use ale#path#FindNearestFile(a:buffer, 'compile_commands.json') and set that if it's not empty. Remember to apply ale#Escape() to the path when writing it into the command string. Then add a test for that.

…angtidy-build_option

gagbo · 2017-06-23T19:10:26Z

I have one example case to illustrate why I finally wasn't able to work out a solution with ale#path#FindNearestFile()

If the source file is in ${PROJECT_ROOT}/src/ui and then the build dir we should find is ${PROJECT_ROOT}/build, then using FindNearestFile will fail because running only upward search means we will look for compile_commands.json only in ${PROJECT_ROOT}/src/ui, ${PROJECT_ROOT}/src, ${PROJECT_ROOT} and irrelevant directories after. I think we don't want to change the FindNearestFile function to run combined {up/down}wards search, as vim help really shows it's not really a good solution.

Therefore I feel like the quicker solution to this problem would be to give directly the build directory as an option. But I agree that I could :

use a default list with common names,
run FindNearestDir for the names in the list,
and then look for compile_commands.json in these folders

And have that as a default behaviour if the option is not set.

It allows looking for specific names in the project tree. Setting a default value for this option means clang-tidy should work out of the box for most "out-of-source build" projects. Overriding either this value or the builddir option allows to finely control the expected location of compile_commands.json

Since the build folder is found upwards from current directory the returned path for the linter is absolute, and it needs to be adjusted to follow Travis architecture.

gagbo · 2017-06-23T21:33:53Z

I finally got over my dumbness and implemented the behaviour mentioned earlier :

If ale_cpp_clangtidy_builddir is set, then the -p ale_cpp_clangtidy_builddir is set to the linter and we're done
If ale_cpp_clangtidy_builddir is not set, then for each name in the list ale_cpp_clangtidy_builddirnames (which defaults to ['build', 'bin'] )
- We look upwards for any directory named name and then verify if there is a name/compile_commands.json readable file.
- Whenever a suitable directory with a readable file is found, we set ale_cpp_clangtidy_builddir to this subfolder and stop the search.

The method relies on finddir() giving a list ordered from the nearest subdirectory to the furthest, so that the first valid candidate is really the one we want (that is why I added a build folder to the root of test_c_projects, to check the priority is followed).

w0rp

Nice. This is looking better. My hope is that with the right list of default names, it will work automatically for the majority of projects.

w0rp · 2017-06-24T08:04:31Z

+    if empty(l:user_builddir)
+        let l:builddir_names = ale#Var(a:buffer, 'cpp_clangtidy_builddirnames')
+        for l:name in l:builddir_names
+            let l:candidates = finddir(l:name, expand('#' . a:buffer.':p:h') . ';', -1)


Have a look at the implementation of ale#python#FindVirtualenv. You can use the ale#path#Upwards function with the isdirectory function for implementing this. I suggest implementing this as a ale#c#FindCompileCommands function. It could be useful for other tools.

w0rp · 2017-06-24T08:06:35Z

+    " Build directory has the priority if
+    " both builddir and builddirnames options are defined
+    if empty(l:user_builddir)
+        let l:builddir_names = ale#Var(a:buffer, 'cpp_clangtidy_builddirnames')


I recommend calling this variable ale_c_build_dir_names. It can be used for a few C and C++ tools, and that name will match the ale_virtualenv_dir_names variable name.

I agree that this variable should be shared betwwen c-family linters, I wasn't sure about how to do it, I'll look into this

w0rp · 2017-06-24T08:07:36Z

-    let l:extra_options = !empty(l:user_options)
-    \   ? ' -- ' . l:user_options
-    \   : ''
+    let l:user_builddir = ale#Var(a:buffer, 'cpp_clangtidy_builddir')


I'm not sure if this one should be left as-is, or should be named c_build_dir, or maybe cpp_build_dir. I think leave it for now.

w0rp · 2017-06-24T08:08:04Z

+" flags are contained in the json
+let g:ale_cpp_clangtidy_builddir = get(g:, 'ale_cpp_clangtidy_builddir', '')
+
+let g:ale_cpp_clangtidy_builddirnames =


You can initialize this in autoload/ale/c.vim with the function below.

w0rp · 2017-06-24T08:08:47Z

+
+  AssertEqual
+  \   'clang-tidy -checks=''*'' %s -p ''/testplugin/test/test_c_projects/json_project/build'''
+  \ , ale_linters#cpp#clangtidy#GetCommand(bufnr(''))


The goal is to use the same variables for all C-family linters

It implies using the new autoload/ale/c.vim file, and cleaning up variable names to be less linter-specific and more language-specific

Calling only ale#Var is bad, since we need to look at least once in ale#c autoload file in order to load the relevant global variable

Removed the trailing /, as it is taken into account in ale#c#FindCompileCommands by looking for `/compile_commands.json` in each candidate folder

gagbo · 2017-06-24T10:44:34Z

I'm so bad with these tests it's incredible.

Well, I've done the changes as proposed. It looks better this way indeed.

I chose to keep ale_c_build* names, because I think there is no reason to separate the behaviours between c and cpp (and most likely with any language of the C-family supported by CMake)
The doc has not been written yet. Should I add a 'Global options' section under cpp language in the doc like it's done for Python ?
If we agree on this behaviour, I'll also change Add clangcheck Linter to cpp #686 to make use of the C-family feature.

Then a discussion can be started on the right choice of ale_c_build_dir_names but I don't think I'm experienced enough to propose names. Maybe starting the conversation in #392 could help have a better default list. [ 'build', 'build/bin', 'buildLinux', 'buildDebug', 'build_debug', 'build_release', ...]

EDIT : And arguably a new test file should be done to test the features of autoload/ale/c.vim (which is really only FindCompileCommands for now). I'm already crying.

w0rp · 2017-06-24T10:49:17Z

Yeah, you can document the option like the Python virtualenv option.

w0rp

Looks good. 👍 I'll wait for the option documentation.

w0rp · 2017-06-24T11:40:20Z

Cheers! 🍻

…analysis#688

gagbo · 2017-06-24T11:54:48Z

Yay !
I sneaked a doc explanation in the other PR #686, basically because the default behaviour is not really as evident as it should be with the linter.

Last step is to actually add a test for autoload/ale/c.vim, but I'll look into this a little later, for the time being the behaviour to test is actually exactly the same as in FindVirtualEnv in python projects

Added builddir option to clang-tidy to point to json folder

cbfac16

gagbo added 3 commits June 23, 2017 20:17

Use of upward search instead of function to find the build directory

f614d0b

revert to commit cbfac16

89633bb

Merge branch 'clangtidy-build_option' of github.com:Gagbo/ale into cl…

f8c9b62

…angtidy-build_option

gagbo added 8 commits June 23, 2017 21:28

True revert

a9dda5b

Added ale#Escape call to the build directory

7994962

Fix CI error

51fe68c

Added Clang-tidy test

a4321a0

Fix CI bug

7582656

Since the build folder is found upwards from current directory the returned path for the linter is absolute, and it needs to be adjusted to follow Travis architecture.

Fixed typo in expected result

d04ead1

I'm dumb.

b2b4b2d

w0rp requested changes Jun 24, 2017

View reviewed changes

gagbo added 9 commits June 24, 2017 10:36

Changed names of build directory variables to more global names

2b3cae3

The goal is to use the same variables for all C-family linters

Added FindCompileCommands function as suggested

69c7b2b

Cleaned up code to use new C-family global scripts

059101c

It implies using the new autoload/ale/c.vim file, and cleaning up variable names to be less linter-specific and more language-specific

Added more unit tests to find where the CI problem comes from

4aac66d

Fix typo

477433d

Comments

2ef84b1

Minor style change

16e3172

Removed badly designed test

17323b8

Calling only ale#Var is bad, since we need to look at least once in ale#c autoload file in order to load the relevant global variable

Fix typo in tests

9a62a6c

Removed the trailing /, as it is taken into account in ale#c#FindCompileCommands by looking for `/compile_commands.json` in each candidate folder

w0rp reviewed Jun 24, 2017

View reviewed changes

gagbo added 2 commits June 24, 2017 12:55

Precisions on ale#c#FindCompileCommands return value

a3431b6

Fixed doc to reflect changes

757c912

w0rp approved these changes Jun 24, 2017

View reviewed changes

w0rp merged commit e98560a into dense-analysis:master Jun 24, 2017

gagbo added a commit to gagbo/ale that referenced this pull request Jun 24, 2017

Changed the linter to handle autoload/ale/c.vim options as per dense-…

08a4f7f

…analysis#688

gagbo mentioned this pull request Jun 24, 2017

Add clangcheck Linter to cpp #686

Merged

Uh oh!

Conversation

gagbo commented Jun 23, 2017

Uh oh!

w0rp commented Jun 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gagbo commented Jun 23, 2017 via email

Uh oh!

w0rp commented Jun 23, 2017

Uh oh!

gagbo commented Jun 23, 2017

Uh oh!

gagbo commented Jun 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

w0rp left a comment

Choose a reason for hiding this comment

Uh oh!

w0rp Jun 24, 2017

Choose a reason for hiding this comment

Uh oh!

w0rp Jun 24, 2017

Choose a reason for hiding this comment

Uh oh!

gagbo Jun 24, 2017

Choose a reason for hiding this comment

Uh oh!

w0rp Jun 24, 2017

Choose a reason for hiding this comment

Uh oh!

w0rp Jun 24, 2017

Choose a reason for hiding this comment

Uh oh!

w0rp Jun 24, 2017

Choose a reason for hiding this comment

Uh oh!

gagbo commented Jun 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

w0rp commented Jun 24, 2017

Uh oh!

w0rp left a comment

Choose a reason for hiding this comment

Uh oh!

w0rp commented Jun 24, 2017

Uh oh!

gagbo commented Jun 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

w0rp commented Jun 23, 2017 •

edited

Loading

gagbo commented Jun 23, 2017 •

edited

Loading

gagbo commented Jun 24, 2017 •

edited

Loading