[PATCH] D23848: Add a clang-tidy Visual Studio extension

[Alexander Kornienko via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

Awesome!

A few comments.


================
Comment at: CMakeLists.txt:6
@@ -5,2 +5,3 @@
 add_subdirectory(clang-tidy)
+add_subdirectory(clang-tidy-vs)
 endif()
----------------
Should the plugin be placed inside clang-tidy directory?

================
Comment at: clang-tidy-vs/ClangTidy/CategoryVerb.cs:29
@@ +28,3 @@
+        {
+
+        }
----------------
Remove the empty line.

================
Comment at: clang-tidy-vs/ClangTidy/CategoryVerb.cs:34
@@ +33,3 @@
+        {
+            if (value.GetType() == typeof(string))
+            {
----------------
Should this be `value is string`?

================
Comment at: clang-tidy-vs/ClangTidy/CheckTree.cs:130
@@ +129,3 @@
+            CheckTree Root = new CheckTree();
+            string[][] Groups = new string[][] {
+                new string[] {"boost"},
----------------
Ideally, this should be configurable without re-compilation. Not necessary for now, but please add a FIXME.

================
Comment at: clang-tidy-vs/ClangTidy/ClangTidyConfigurationPage.cs:10
@@ +9,3 @@
+using System.ComponentModel;
+using System.Linq;
+using System.Runtime.InteropServices;
----------------
Are all of these actually used?

================
Comment at: clang-tidy-vs/ClangTidy/ClangTidyConfigurationPage.cs:26
@@ +25,3 @@
+            get
+            {   if (Grid == null)
+                    Grid = new ClangTidyPropertyGrid();
----------------
^K^F or whatever poor C# folks do to format code without clang-format ;)

================
Comment at: clang-tidy-vs/ClangTidy/ClangTidyPackage.cs:58
@@ +57,3 @@
+        {
+            //var page = (OptionPageGrid)GetDialogPage(typeof(OptionPageGrid));
+            //return page.Style;
----------------
Remove commented-out code?

================
Comment at: clang-tidy-vs/ClangTidy/ClangTidyProperties.cs:81
@@ +80,3 @@
+        [ClangTidyCheck("cert-dcl50-cpp")]
+        public bool CERTDCL50
+        {
----------------
I hope, this file is generated?

A way to update this file should be documented.

================
Comment at: clang-tidy-vs/ClangTidy/Properties/AssemblyInfo.cs:32
@@ +31,3 @@
+
+[assembly: AssemblyVersion("1.1.0.0")]
+[assembly: AssemblyFileVersion("1.1.0.0")]
----------------
Ideally, the file should be generated by CMake to use proper version number. Please add a FIXME.

================
Comment at: clang-tidy-vs/ClangTidy/PropertyFileParser.cs:80
@@ +79,3 @@
+
+            string ClangTidy = Path.Combine(ClangTidyFilePath, ".clang-tidy");
+            using (StreamWriter Writer = new StreamWriter(ClangTidy))
----------------
s/ClangTidy/ConfigFile/

================
Comment at: clang-tidy-vs/ClangTidy/PropertyFileParser.cs:208
@@ +207,3 @@
+
+                ClangTidyProperties.CheckMapping[] Matches = Props.FindChecksMatching(Check).ToArray();
+                foreach (var Match in Matches)
----------------
Is the copy needed to avoid aliasing? (i.e. will this break if you iterate over `Props.FindChecksMatching(Check)` instead?)

================
Comment at: clang-tidy-vs/ClangTidy/Utility.cs:31
@@ +30,3 @@
+        {
+            string RE = Regex.Escape(Pattern).Replace(@"\*", ".*").Replace(@"\?", ".");
+            return Regex.IsMatch(Value, RE);
----------------
Question marks are not supported in check patterns, so `.Replace(@"\?", ".")` can be dropped.


https://reviews.llvm.org/D23848