Skip to content
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

Generates problem markers for a C++ project using clang-tidy #5533

Merged
merged 1 commit into from
Jul 15, 2019
Merged

Conversation

lmcbout
Copy link
Contributor

@lmcbout lmcbout commented Jun 19, 2019

Fixes #5532

Description

Run the C/C++ linting tool clang-tidy as a task to generate problem markers which can be later consumed by the problems-view.

Setup

  1. Open a C/C++ project
  2. Enable the preference cpp.clangTidy
    • Test using the preference only: "cpp.clangTidyChecks": "*"
    • Test using the .clang-tidy file: create a clang-tidy file at the root of the workspace, and add the following:
Checks: "read*"
WarningsAsErrors: "hicpp-*"

This will verify the linter with the selected files for the "readability" flaws.
If you happen to test for "hicpp* warnings, they will be reported as errors
because of WarningsAsErrors: "hicpp-*"

How to Test

  1. Create a tasks.json file under the directory .theia of the workspace.
  2. Add the following task:
{
    "tasks": [
        {
            "label": "[Task] clang-tidy",
            "type": "shell",
            "cwd": "${workspaceFolder}",
            "command": "clang-tidy",
            "args": [
                "*"
            ],
            "options": {},
            "problemMatcher": [
                "$clangTidyMatcher"
            ]
        }
    ]
}
  1. Run the task: F1 then Tasks: Run Tasks then shell: [Task] clang-tidy.

Additional Information

  • Requires clang-tidy executable
  • Requires clangd version 9 or later
  • Note: If you add the file and the preference, the linter will merge the preference option with the file options. If you want to apply only the preference option even if the .clang-tidy is present, just put :
    "cpp.clangTidyChecks": "-*" will disable all default checks (-*) and add you choice of linter after, reference http://clang.llvm.org/extra/clang-tidy/

@lmcbout lmcbout requested review from elaihau and vince-fugnitto and removed request for benoitf and evidolob June 19, 2019 19:31
@vince-fugnitto
Copy link
Member

You are missing a serviceIdentifier in your spec file:
https://travis-ci.org/theia-ide/theia/jobs/548224808#L5606

@vince-fugnitto
Copy link
Member

Please rebase the original PR from @elaihau, the commit sha are different e16cfb5 vs e3ba87e

@lmcbout lmcbout force-pushed the GH-5532 branch 2 times, most recently from a1c780c to a310d96 Compare June 20, 2019 18:44
@vince-fugnitto
Copy link
Member

@lmcbout can you provide a meaningful commit message?
I see that @elaihau's commit message is very detailed and it'd be nice if you commented on what your PR addresses, and provides as features.

@vince-fugnitto
Copy link
Member

vince-fugnitto commented Jun 21, 2019

@lmcbout you are missing the updates to the cpp-task-provider.spec.ts test file to include the new service identifier symbol ProblemMatcherRegistry.

@elaihau
Copy link
Contributor

elaihau commented Jul 9, 2019

I moved interfaces around in #5024. Some of the imports in this PR need update.

@vince-fugnitto
Copy link
Member

I think the commit message needs updating, it isn't informative enough about the new functionality and is quite hard to follow:

clang-tidy uses the .clang-tidy file checks
When you define in "task.json" a problem matcher with "$clangTidyMatcher",
it creates an entry in the "problems" view with a tag: "clang-tidy-task"
The result in the "Problems" view is persistent until you
re-generates the task.

Something more appropriate may be:

Enable running the `clang-tidy` C/C++ linting present from clang-tidy as a task.
With the new updates, linting can be performed across an entire workspace using user
specified checks and linting rules. The problem markers collected when executing the
task will be recorded and displayed in the `problems-widget`...

and then you can finish up with the specifics you previously mentioned.

@vince-fugnitto
Copy link
Member

@lmcbout can you re-push, it looks like the eca check didn't work?

@lmcbout
Copy link
Contributor Author

lmcbout commented Jul 10, 2019

@vince-fugnitto I just revalidate the ECA and all checks are green
Thanks for your input

@vince-fugnitto
Copy link
Member

@lmcbout am I doing something wrong, I followed the task you provided in the description and it's not running at the moment.

Screen Shot 2019-07-10 at 10 52 43 AM
Screen Shot 2019-07-10 at 10 53 10 AM

@lmcbout
Copy link
Contributor Author

lmcbout commented Jul 10, 2019

@vince-fugnitto are you using clang version 9 ?

@vince-fugnitto
Copy link
Member

@vince-fugnitto are you using clang version 9 ?

Yes, clangd version 9.0.0-svn362989-1~exp1+0~20190610220635.500~1.gbp18c431 (trunk)

@lmcbout
Copy link
Contributor Author

lmcbout commented Jul 10, 2019

@vince-fugnitto to run the task, you need to install "clang-tidy"
sudo apt-get install clang-tidy

Note: the task is using the program clang-tidy" and Theia is using "clang" version 9 with the preference setting

@vince-fugnitto
Copy link
Member

@vince-fugnitto to run the task, you need to install "clang-tidy"
sudo apt-get install clang-tidy

Note: the task is using the program clang-tidy" and Theia is using "clang" version 9 with the preference setting

Clang-tidy is already there no?
You can see from the problems-widget.

@lmcbout
Copy link
Contributor Author

lmcbout commented Jul 10, 2019

No , you see two things in the problem marker:
1- [clang-tidy] which comes from clangd version 9 and the info is build from with the Theia preferences
2-[clang-tidy-task] is using clang-tidy executable, not using clangd -clang-tidy option

@vince-fugnitto
Copy link
Member

to run the task, you need to install "clang-tidy"

If it is required to have clang-tidy as an executable then I believe it should be
reflected/documented somewhere.

@lmcbout
Copy link
Contributor Author

lmcbout commented Jul 10, 2019

Added a comment in the Readme.md file to install clang-tidy tool

@vince-fugnitto
Copy link
Member

Added a comment in the Readme.md file to install clang-tidy tool

It's fine for linux but not other operating systems unfortunately.

@vince-fugnitto
Copy link
Member

The PR works well, I had to update the task that was provided in the documentation to only lint the *.cpp files, I found it odd that the task would lint all files (even those that are not .cpp, .h) if not specified.

Maybe it'd be good another person tests as well, maybe @elaihau could you take a look too?

@lmcbout lmcbout force-pushed the GH-5532 branch 2 times, most recently from 16f1583 to d91a6a0 Compare July 12, 2019 15:04
@lmcbout
Copy link
Contributor Author

lmcbout commented Jul 12, 2019

@vince-fugnitto I added some comments in the README file

Enable running the `clang-tidy` C/C++ linting present from clang-tidy as a task.
With the new updates, linting can be performed across an entire workspace using user
specified checks and linting rules. The problem markers collected when executing the
task will be recorded and displayed in the `problems-widget`.
The result in the "Problems" view is persistent until you
re-generates the task.

Signed-off-by: Jacques Bouthillier <jacques.bouthillier@ericsson.com>
@vince-fugnitto
Copy link
Member

I won't comment anymore on the documentation :)
I'll take the time once the PR is complete to update the documentation for the cpp package more thoroughly.

Copy link
Contributor

@elaihau elaihau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested in my local env with the help from Vincent.

@lmcbout lmcbout merged commit e830fbc into master Jul 15, 2019
@lmcbout lmcbout deleted the GH-5532 branch July 15, 2019 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[tasks] run clang-tidy as a task and generate problem markers
3 participants