This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy-vs] Visual Studio plugin extended
ClosedPublic

Authored by jutocz on Mar 14 2017, 4:00 AM.
Tokens
"Like" token, awarded by mloskot.

Details

Summary

Extending clang-tidy Visual Studio plugin functionality:

  • Added running clang-tidy directly from VS and displaying results in output window (output uses .clang-tidy-vsfilters file for filtering clang-tidy output message)
  • Added highlighting places in code where validation warnings/errors were found
  • Added intelisense quick info description for highlighted warnings in code (MEF component)
  • Added VS toolbar for running clang-tidy
  • Added new icon for buttons
  • Modified project and manifest to reflect all changes and improve debugging capabilities
  • Limited VS compatibility to VS2015 and higher (due to MEF component requirements)

Diff Detail

Event Timeline

jutocz created this revision.Mar 14 2017, 4:00 AM
zturner edited edge metadata.Mar 14 2017, 8:24 AM

Looks like some very nice improvements. But since I'm not familiar with the various interfaces you've implemented and what parts of the IDE they extend / customize, it's hard to give a thorough review. Can you address the comments I've outlined here, and then I can look again?

clang-tidy-vs/CMakeLists.txt
24–28

I don't have much experience with Nuget, but from what I can tell you're using this to ensure the existence of VSSDK? If that's the case, I'd rather just document VSSDK as a dependency and fail to work if it's not present. Because otherwise, if someone installs VSSDK as part of the normal visual studio installation, they will still require nuget in the path or CMake configuration, which is not ideal since most people working on a vsix know they need VSSDK installed.

but if I've misunderstood what you're using it for, let me know.

clang-tidy-vs/ClangTidy/ClangTidy.csproj
39–40

What does this line do?

82–95

I don't think any of these new references are needed. Granted they were probably added automatically by the IDE, but if we don't need them, let's just remove them.

clang-tidy-vs/ClangTidy/ClangTidyPackage.cs
26

Can remove this.

clang-tidy-vs/ClangTidy/ClassifierAndTagger/Classifier.cs
2

Is this a UTF-8 BOM? Regardless, can you remove it? (It appears in some more files as well, please remove it from those too)

11–14

Can you put a better comment here? I'm not familiar with this area of the VSSDK so it would be nice if there a short summary of how this is relevant to the clang-tidy extension. I gather that it's somehow related to the text that is visible in the current editor window, but some additional explanation would be nice here.

clang-tidy-vs/ClangTidy/ClassifierAndTagger/ClassifierProvider.cs
16

Same here.

clang-tidy-vs/ClangTidy/ClassifierAndTagger/QuickInfoController.cs
9

And here (Perhaps just assume this applies to all of the remaining VSSDK interface implementations)

clang-tidy-vs/ClangTidy/Resources/VSOutputFilter.yaml
16 ↗(On Diff #91695)

Missing newline here.

jutocz added inline comments.Mar 16 2017, 1:26 AM
clang-tidy-vs/CMakeLists.txt
24–28

You are right. I was looking at clang-format VS plugin to see if there are some useful cmake commands for it (like adding timestamp to version for easier testing). Nuget is not really essential in here, though other plugin does require it.

clang-tidy-vs/ClangTidy/ClangTidy.csproj
39–40

Automatically added. I'll get rid of it.

82–95

I'll check all of them so we have only those that are needed.

clang-tidy-vs/ClangTidy/ClangTidyPackage.cs
26

You are right. I'll check other includes and remove them if they are not needed.

clang-tidy-vs/ClangTidy/ClassifierAndTagger/Classifier.cs
2

I'll remove it.

11–14

Sure, I'll add better comments for all VSSDK interface implementations.

jutocz added inline comments.Mar 16 2017, 2:01 AM
clang-tidy-vs/ClangTidy/ClassifierAndTagger/Classifier.cs
2

After investigating this matter I found out Visual Studio automatically sets file encoding to utf-8 BOM... so all files in clang-tidy-vs solution use this encoding.
If we want to change this, we would have to change encoding for all files manually, but still other source files added to solution through VS will use BOM encoding (as it's default for VS).

What do you think?

jutocz updated this revision to Diff 92127.Mar 17 2017, 4:43 AM

I've updated patch adding better comments and doing some cosmetic work.

As to nuget I'm not quite sure how it should work. Right now I've left nuget in script, because otherwise when LLVM solution tries to build plugin, it fails due to missing packages (YamlDotNet and StrongNameSigner). With nuget in script those packages are restored by cmake using nuget restore command. Otherwise user has to launch plugin solution and restore packages, then return to LLVM, so the whole solution compiles without errors.

I've removed other nuget VSSDK packages I had added the last time, so the user needs to have VSSDK installed to make it all work.

jutocz updated this revision to Diff 92274.Mar 19 2017, 6:54 AM
jutocz edited the summary of this revision. (Show Details)

Updating diff with:

  • added synchronization for results container using multiple readers, single writer lock (ReaderWriterLockSlim)
  • removing .vsixmanifest file - it's generated by cmake using source.extension.vsixmanifest.in
  • added config files (.clang-tidy.vsfilters) for filtering/modifying clang-tidy output message in Visual Studio's output
zturner accepted this revision.Mar 22 2017, 2:33 PM

It's hard for me to review on correctness since I'm not familiar with these APIs (and I don't think anyone else is either) but from a functionality standpoint I think these are great changes and I don't see anything glaringly wrong, so lgtm unless someone else has objections.

clang-tidy-vs/ClangTidy/ThreadRunner/ClangTidyRunner.cs
27

Not critical, but will this display the clang-tidy output in a separate category in the output window? By default there is "Build", "Debug", "Build Order". Which category will this be displayed under? Should we have our own category for "Clang Tidy"?

121

seems like this would be better using C# async and yield, but this gets the job done in any case.

This revision is now accepted and ready to land.Mar 22 2017, 2:33 PM

About correctness: I made these changes so programmers in my company may start using clang-tidy in everyday's work... and so far nobody complained.
If we continue using this extension in here, I hope to continue improving it.

And I'd be glad to hear any suggestions now or in the future.

clang-tidy-vs/ClangTidy/ThreadRunner/ClangTidyRunner.cs
27

Yes, there is a new "Clang Tidy" category in output window for displaying all info from our extension. Category appears in output window and becomes active whenever user runs clang-tidy check.

121

Do you mean replacing use of BackgroundWorker? Or replacing while loop with use of async + await?

Nobody seems to have any objections... So I need help with committing changes. Also there are binary file changes. So how should I proceed?

alexfh edited edge metadata.Apr 6 2017, 8:00 AM

Re: binary files. Are you using git? Then, I think, you need to do something similar to https://reviews.llvm.org/D31763: add a .gitattributes file containing "*.png binary" and use arcanist to upload the patch (not sure if Phab's web-interface can allow uploading patches with binary files). I also don't know whether git-svn will handle "svn propset svn:mime-type application/octet-stream ...", but that's a problem of whoever will submit the patch for you (Zachary, probably, since it makes sense to at least build the new code and I don't have Windows and MSVS).

clang-tidy-vs/ClangTidy/ClassifierAndTagger/Classifier.cs
2

So, utf8 BOM is only added for new files and not on each save from MSVS? Then please remove BOMs, since they are somewhat poorly supported by some tools (e.g. Phabricator ;).

Juliusz, are you going to upload a complete patch?

mloskot added a subscriber: mloskot.

Juliusz, are you going to upload a complete patch?

@jutocz Any update on this?

Any update on this?

shafik closed this revision.Sep 21 2023, 11:48 AM
shafik added a subscriber: shafik.

I am closing this since the author has ignored several pings

Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2023, 11:48 AM
Herald added a subscriber: aheejin. · View Herald Transcript

I am closing this since the author has ignored several pings

What's the benefit of closing a review that hans't been touched in 6 years? Is there an effort to close reviews on Phabricator before it goes into read-only mode or something?

I am closing this since the author has ignored several pings

What's the benefit of closing a review that hans't been touched in 6 years? Is there an effort to close reviews on Phabricator before it goes into read-only mode or something?

Apologies for the extra noise but a few of us are going through old phab PRs that are accepted. It would be a shame to lose good changes because someone got pulled away during an old review. Several authors we reached out to have said they will try and land things before phab is closed for business which is excellent.

Other PRs I don't know the area well enough, so I leave a comment on them so folks who know can determine for sure if they are really relevant or not and they want to make and effort to land them.

Otherwise I close it so that it is off the queue of accepted PRs and someone does not go over a PR I already spent time looking at.

I am closing this since the author has ignored several pings

What's the benefit of closing a review that hans't been touched in 6 years? Is there an effort to close reviews on Phabricator before it goes into read-only mode or something?

Apologies for the extra noise but a few of us are going through old phab PRs that are accepted. It would be a shame to lose good changes because someone got pulled away during an old review. Several authors we reached out to have said they will try and land things before phab is closed for business which is excellent.

Other PRs I don't know the area well enough, so I leave a comment on them so folks who know can determine for sure if they are really relevant or not and they want to make and effort to land them.

Otherwise I close it so that it is off the queue of accepted PRs and someone does not go over a PR I already spent time looking at.

Thanks for the context, that makes sense!

clang-tidy-vs/ClangTidy/Utility.cs