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

[Zachary Turner via cfe-commits]

The unnecessary usings are added by Visual Studio, so I would rather leave
them.  For example, they often provide extension methods to containers so
that containers can be used with functional paradigms, and you expect to
see those methods in the Intellisense window when you type "Foo.", but
without the using statement they would not appear.  In general they don't
affect performance or compilation the same way that #includes do in C++, so
for the ones that are added automatically by Visual Studio I would rather
leave them alone.  I've cleaned up all the unnecessary ones that were not
automatically added though.

On Wed, Sep 7, 2016 at 4:27 AM Alexander Kornienko 
wrote:

> alexfh added inline comments.
>
> 
> Comment at: clang-tidy-vs/ClangTidy/ClangTidyPackage.cs:53
> @@ +52,3 @@
> +private void MenuItemCallback(object sender, EventArgs args)
> +{
> +}
> 
> Add a FIXME to actually implement this.
>
>
> https://reviews.llvm.org/D23848
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.


Comment at: clang-tidy-vs/ClangTidy/ClangTidyPackage.cs:53
@@ +52,3 @@
+private void MenuItemCallback(object sender, EventArgs args)
+{
+}

Add a FIXME to actually implement this.


https://reviews.llvm.org/D23848



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zachary Turner via cfe-commits]

zturner added a comment.

I am planning to submit this today if there are no further suggestions.


https://reviews.llvm.org/D23848



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zachary Turner via cfe-commits]

zturner added a comment.

Anyone else have any thoughts on this or is this good to go?


https://reviews.llvm.org/D23848



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian McCarthy via cfe-commits]

amccarth added inline comments.


Comment at: clang-tidy-vs/ClangTidy/CheckTree.cs:68
@@ +67,3 @@
+return Name_;
+return ParentPath + "-" + Name_;
+}

OK, cool.  I assumed that the root had a name.  Thus a node with no parent (a 
root) could return its name and the rest of the children would each append '-' 
+ Name_.  But if roots are nameless, then your solution is fine.


https://reviews.llvm.org/D23848



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zachary Turner via cfe-commits]

zturner added a comment.

In https://reviews.llvm.org/D23848#525485, @Eugene.Zelenko wrote:

> I think will be good idea to mention this plugin in  docs/ReleaseNotes.rst.


I was planning to wait until it's "finished".  This patch will give you a user 
interface for editing .clang-tidy files, but it still won't provide any means 
of *running* clang-tidy on the currently active source file, project, or 
solution.

I had planned to add those features incrementally to make the reviews more 
manageable.  What do you guys think?


https://reviews.llvm.org/D23848



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eugene Zelenko via cfe-commits]

Eugene.Zelenko added a comment.

I think will be good idea to mention this plugin in  docs/ReleaseNotes.rst.


https://reviews.llvm.org/D23848



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian McCarthy via cfe-commits]

amccarth added a comment.

I don't know enough C# to review for language usage.  I was mostly reading for 
understandability.  Overall, I think this looks really nice.



Comment at: clang-tidy-vs/ClangTidy/CheckTree.cs:67
@@ +66,3 @@
+if (ParentPath == null)
+return Name_;
+return ParentPath + "-" + Name_;

This seems overly complicated so I assume I'm missing some nuance.  Why not:

if (Parent_ == null)
return Name_;
string ParentPath = Parent_.Path;

Is there a case where Parent_.Path could return null besides the one that you 
created?




Comment at: clang-tidy-vs/ClangTidy/InheritablePropertyComponent.cs:14
@@ +13,3 @@
+/// have properties which might inherit from their parent, or be 
overridden.
+/// It turns out this is somewhat non-trivial.  The .NET PropertyGrid is 
good makes
+/// displaying simple properties with a static notion of what constitutes a

s/good makes/makes/



Comment at: clang-tidy-vs/README.txt:8
@@ +7,3 @@
+- Visual Studio 2010 Professional (?)
+- Visual Studio 2010 SDK (?)
+

2010?  2015?  Will Community editions work?


https://reviews.llvm.org/D23848



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.


Comment at: clang-tidy-vs/ClangTidy/ClangTidyProperties.cs:82
@@ +81,3 @@
+public bool CERTDCL50
+{
+get { return GetInheritableProperty("CERTDCL50").Value;  }

zturner wrote:
> alexfh wrote:
> > zturner wrote:
> > > Are the .rst files in the repo somewhere already?  I don't see them.
> > > 
> > > As for the display name, I agree this one is bad (I forgot to change it). 
> > >  But you can look at some of the ones below for better examples.  For 
> > > example, I find `I.22 - Complex Global Initializers` to be a better short 
> > > descriptor than `cppcoreguidelines-interfaces-global-init`.
> > > 
> > > What about a hand-maintained Yaml file that adheres to a format similar 
> > > to the following?
> > > 
> > > ```
> > > ---
> > > Checks:
> > >   - Name:cert-dcl54-cpp
> > > Label:   Overloaded allocation function pairs
> > > Description: Checks for violations of CERT DCL54-CPP - Overload 
> > > allocation and deallocation functions as a pair in the same scope
> > > Category:CERT Secure Coding Standards
> > >   - Name:cppcoreguidelines-interfaces-global-init
> > > Label:   I.22 - Complex Global Initializers
> > > Description: Checks for violations of Core Guideline I.22 - Avoid 
> > > complex initializers of global objects
> > > Category:C++ Core Guidelines
> > > ...
> > > 
> > > ```
> > > 
> > > Some file somewhere is going to have to be maintained by hand, and since 
> > > a file such as this doesn't appear to exist in clang-tidy already, we 
> > > might as well use a format that we already have good tools to parse at 
> > > runtime, such as Yaml.
> > The .rst files are there: 
> > https://reviews.llvm.org/diffusion/L/browse/clang-tools-extra/trunk/docs/clang-tidy/checks/
> > 
> > Docs + source code are already enough hassle to maintain, so adding yet 
> > another file with a similar information seems too much. We can start with a 
> > hand-written YAML file and later add a script to generate it from the docs. 
> > I'm not sure though, what `Label` could be mapped on. Do you need it for 
> > the property editor or can you get away with just the name?
> Well you can look at the screenshot to see how I'm using it.  Basically the 
> left hand side of the grid are all the "label" values.  It's what people are 
> going to see when deciding whether they want to enable a check or not.  So it 
> should be mostly self-explanatory without having to click on it to get more 
> detail.  The name kinda is, but the point of the extension is to be 
> friendlier than editing files by hand, so I think we should try to provide a 
> friendlier name too.  When you're using the command line tool you have no 
> choice but to specify the check name, but if you're using a UI I think we can 
> give the user a little bit more info about each check.
> 
> Generating YAML from docs seems like a decent idea.  Perhaps to map label we 
> could extend the rst format a little to add the label to it.  Sort of like 
> "summary" vs "description".
> 
> I'll do a hand-written Yaml file for now, and think about the best way to 
> generate the Yaml from RST in a later patch.
SG.

Re: label vs check name. I think, even if we show a friendlier label (which 
we'll need to require for all new checks and back-fill for the existing ones), 
the extension should also show the check name somewhere for transparency. It's 
supposed to be human-readable to some degree (not a proper explanation though, 
rather a hint to what the check is about). It's also shown in the diagnostic 
messages, so there's no point in hiding it elsewhere.


https://reviews.llvm.org/D23848



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zachary Turner via cfe-commits]

zturner added inline comments.


Comment at: clang-tidy-vs/ClangTidy/ClangTidyProperties.cs:82
@@ +81,3 @@
+public bool CERTDCL50
+{
+get { return GetInheritableProperty("CERTDCL50").Value;  }

alexfh wrote:
> zturner wrote:
> > Are the .rst files in the repo somewhere already?  I don't see them.
> > 
> > As for the display name, I agree this one is bad (I forgot to change it).  
> > But you can look at some of the ones below for better examples.  For 
> > example, I find `I.22 - Complex Global Initializers` to be a better short 
> > descriptor than `cppcoreguidelines-interfaces-global-init`.
> > 
> > What about a hand-maintained Yaml file that adheres to a format similar to 
> > the following?
> > 
> > ```
> > ---
> > Checks:
> >   - Name:cert-dcl54-cpp
> > Label:   Overloaded allocation function pairs
> > Description: Checks for violations of CERT DCL54-CPP - Overload 
> > allocation and deallocation functions as a pair in the same scope
> > Category:CERT Secure Coding Standards
> >   - Name:cppcoreguidelines-interfaces-global-init
> > Label:   I.22 - Complex Global Initializers
> > Description: Checks for violations of Core Guideline I.22 - Avoid 
> > complex initializers of global objects
> > Category:C++ Core Guidelines
> > ...
> > 
> > ```
> > 
> > Some file somewhere is going to have to be maintained by hand, and since a 
> > file such as this doesn't appear to exist in clang-tidy already, we might 
> > as well use a format that we already have good tools to parse at runtime, 
> > such as Yaml.
> The .rst files are there: 
> https://reviews.llvm.org/diffusion/L/browse/clang-tools-extra/trunk/docs/clang-tidy/checks/
> 
> Docs + source code are already enough hassle to maintain, so adding yet 
> another file with a similar information seems too much. We can start with a 
> hand-written YAML file and later add a script to generate it from the docs. 
> I'm not sure though, what `Label` could be mapped on. Do you need it for the 
> property editor or can you get away with just the name?
Well you can look at the screenshot to see how I'm using it.  Basically the 
left hand side of the grid are all the "label" values.  It's what people are 
going to see when deciding whether they want to enable a check or not.  So it 
should be mostly self-explanatory without having to click on it to get more 
detail.  The name kinda is, but the point of the extension is to be friendlier 
than editing files by hand, so I think we should try to provide a friendlier 
name too.  When you're using the command line tool you have no choice but to 
specify the check name, but if you're using a UI I think we can give the user a 
little bit more info about each check.

Generating YAML from docs seems like a decent idea.  Perhaps to map label we 
could extend the rst format a little to add the label to it.  Sort of like 
"summary" vs "description".

I'll do a hand-written Yaml file for now, and think about the best way to 
generate the Yaml from RST in a later patch.


https://reviews.llvm.org/D23848



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.


Comment at: clang-tidy-vs/ClangTidy/ClangTidyProperties.cs:82
@@ +81,3 @@
+public bool CERTDCL50
+{
+get { return GetInheritableProperty("CERTDCL50").Value;  }

zturner wrote:
> Are the .rst files in the repo somewhere already?  I don't see them.
> 
> As for the display name, I agree this one is bad (I forgot to change it).  
> But you can look at some of the ones below for better examples.  For example, 
> I find `I.22 - Complex Global Initializers` to be a better short descriptor 
> than `cppcoreguidelines-interfaces-global-init`.
> 
> What about a hand-maintained Yaml file that adheres to a format similar to 
> the following?
> 
> ```
> ---
> Checks:
>   - Name:cert-dcl54-cpp
> Label:   Overloaded allocation function pairs
> Description: Checks for violations of CERT DCL54-CPP - Overload 
> allocation and deallocation functions as a pair in the same scope
> Category:CERT Secure Coding Standards
>   - Name:cppcoreguidelines-interfaces-global-init
> Label:   I.22 - Complex Global Initializers
> Description: Checks for violations of Core Guideline I.22 - Avoid complex 
> initializers of global objects
> Category:C++ Core Guidelines
> ...
> 
> ```
> 
> Some file somewhere is going to have to be maintained by hand, and since a 
> file such as this doesn't appear to exist in clang-tidy already, we might as 
> well use a format that we already have good tools to parse at runtime, such 
> as Yaml.
The .rst files are there: 
https://reviews.llvm.org/diffusion/L/browse/clang-tools-extra/trunk/docs/clang-tidy/checks/

Docs + source code are already enough hassle to maintain, so adding yet another 
file with a similar information seems too much. We can start with a 
hand-written YAML file and later add a script to generate it from the docs. I'm 
not sure though, what `Label` could be mapped on. Do you need it for the 
property editor or can you get away with just the name?


https://reviews.llvm.org/D23848



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zachary Turner via cfe-commits]

zturner added a comment.

BTW, since I forgot to do so in the original review, here is a screenshot of 
what the UI for the property editor looks like in this extension:

F2328405: clang-tidy.png 



Comment at: clang-tidy-vs/ClangTidy/ClangTidy.vsct:23
@@ +22,3 @@
+
+
+

This file is auto-generated by Visual Studio.  You can manually add your own 
things to it (as we've done at the end of the file), but it doesn't seem 
worthwhile to go back and fix auto-generated code.

BTW, this is just an XML file, so it gets collapse functionality in the Visual 
Studio Editor.  That is, you can collapse tags so that they are not displayed.  
Not sure if that makes a difference.


https://reviews.llvm.org/D23848



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zachary Turner via cfe-commits]

zturner added inline comments.


Comment at: clang-tidy-vs/ClangTidy/ClangTidyProperties.cs:82
@@ +81,3 @@
+public bool CERTDCL50
+{
+get { return GetInheritableProperty("CERTDCL50").Value;  }

Are the .rst files in the repo somewhere already?  I don't see them.

As for the display name, I agree this one is bad (I forgot to change it).  But 
you can look at some of the ones below for better examples.  For example, I 
find `I.22 - Complex Global Initializers` to be a better short descriptor than 
`cppcoreguidelines-interfaces-global-init`.

What about a hand-maintained Yaml file that adheres to a format similar to the 
following?

```
---
Checks:
  - Name:cert-dcl54-cpp
Label:   Overloaded allocation function pairs
Description: Checks for violations of CERT DCL54-CPP - Overload allocation 
and deallocation functions as a pair in the same scope
Category:CERT Secure Coding Standards
  - Name:cppcoreguidelines-interfaces-global-init
Label:   I.22 - Complex Global Initializers
Description: Checks for violations of Core Guideline I.22 - Avoid complex 
initializers of global objects
Category:C++ Core Guidelines
...

```

Some file somewhere is going to have to be maintained by hand, and since a file 
such as this doesn't appear to exist in clang-tidy already, we might as well 
use a format that we already have good tools to parse at runtime, such as Yaml.


https://reviews.llvm.org/D23848



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D23848#524608, @zturner wrote:

> I can fix the empty lines, but keep in mind that Visual Studio's C# editor is 
> MUCH more aggressive about auto-formatting your code.  So it seems like a 
> fruitless endeavor to me, as we will constantly be fighting against their 
> auto formatter, which does this automatically every time you add a new 
> function.


I would definitely not fight with visual editors that reset formatting every 
time you touch the file. However, if VS only adds empty lines when initially 
adding a function, that doesn't promise much resistance ;)



Comment at: CMakeLists.txt:6
@@ -5,2 +5,3 @@
 add_subdirectory(clang-tidy)
+add_subdirectory(clang-tidy-vs)
 endif()

zturner wrote:
> alexfh wrote:
> > Should the plugin be placed inside clang-tidy directory?
> I followed the same way that the clang format VS extension uses.  I don't 
> mind to move it, I just did it this way for consistency.
I don't feel strongly either way. Let's figure this out later.


Comment at: clang-tidy-vs/ClangTidy/ClangTidy.vsct:22
@@ +21,3 @@
+  
+
+

Eugene.Zelenko wrote:
> Unnecessary empty line, same below in many places here.
If there's a way to visually edit this file, I wouldn't care about formatting.


Comment at: clang-tidy-vs/ClangTidy/ClangTidyProperties.cs:81
@@ +80,3 @@
+[ClangTidyCheck("cert-dcl50-cpp")]
+public bool CERTDCL50
+{

zturner wrote:
> alexfh wrote:
> > I hope, this file is generated?
> > 
> > A way to update this file should be documented.
> No, I actually typed this entire file :-/  I don't know of a good way to auto 
> generate it.  We would need a single file, perhaps Yaml or something, 
> consisting of:
> 
> 1. Category Name
> 2. Display Name
> 3. Description
> 4. clang-tidy built in default value of check.
> 
> At that point, we wouldn't even need to generate the file, we could read it 
> at runtime and add the properties dynamically (through the 
> `ICustomTypeDescriptor` interface).
> 
> For now though, the way to update the file is to copy / paste one property 
> and change the values accordingly to add a new check.
Manually updating this file won't fly =[

I guess, we could try to use .rst files as the source of truth. They follow a 
strict naming scheme (.rst) and we could require the first 
paragraph of the text to be a single-sentence description of the check suitable 
for this purpose as well.

I'd also make the display name the same as the full clang-tidy check name 
(`DCL50-CPP` is neither more useful nor convenient than the full check name). 

The only thing we'd need to maintain manually in this case is category 
descriptions.

WDYT?


https://reviews.llvm.org/D23848



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zachary Turner via cfe-commits]

zturner added inline comments.


Comment at: CMakeLists.txt:6
@@ -5,2 +5,3 @@
 add_subdirectory(clang-tidy)
+add_subdirectory(clang-tidy-vs)
 endif()

alexfh wrote:
> Should the plugin be placed inside clang-tidy directory?
I followed the same way that the clang format VS extension uses.  I don't mind 
to move it, I just did it this way for consistency.


Comment at: clang-tidy-vs/ClangTidy/ClangTidyProperties.cs:81
@@ +80,3 @@
+[ClangTidyCheck("cert-dcl50-cpp")]
+public bool CERTDCL50
+{

alexfh wrote:
> I hope, this file is generated?
> 
> A way to update this file should be documented.
No, I actually typed this entire file :-/  I don't know of a good way to auto 
generate it.  We would need a single file, perhaps Yaml or something, 
consisting of:

1. Category Name
2. Display Name
3. Description
4. clang-tidy built in default value of check.

At that point, we wouldn't even need to generate the file, we could read it at 
runtime and add the properties dynamically (through the `ICustomTypeDescriptor` 
interface).

For now though, the way to update the file is to copy / paste one property and 
change the values accordingly to add a new check.


Comment at: clang-tidy-vs/ClangTidy/PropertyFileParser.cs:208
@@ +207,3 @@
+
+ClangTidyProperties.CheckMapping[] Matches = 
Props.FindChecksMatching(Check).ToArray();
+foreach (var Match in Matches)

alexfh wrote:
> Is the copy needed to avoid aliasing? (i.e. will this break if you iterate 
> over `Props.FindChecksMatching(Check)` instead?)
The copy is not needed to avoid aliasing.  It's fine to iterate over 
`Props.FindChecksMatching(Check)`.  I wrote it this way because it helps for 
debugging, as I could just add a watch for `Matches` and view the array in the 
debugger.


https://reviews.llvm.org/D23848



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Zachary Turner via cfe-commits]

zturner added a comment.

I can fix the empty lines, but keep in mind that Visual Studio's C# editor is 
MUCH more aggressive about auto-formatting your code.  So it seems like a 
fruitless endeavor to me, as we will constantly be fighting against their auto 
formatter, which does this automatically every time you add a new function.


https://reviews.llvm.org/D23848



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Eugene Zelenko via cfe-commits]

Eugene.Zelenko added a subscriber: Eugene.Zelenko.


Comment at: clang-tidy-vs/ClangTidy/CheckTree.cs:79
@@ +78,3 @@
+}
+
+

Unnecessary empty line.


Comment at: clang-tidy-vs/ClangTidy/CheckTree.cs:93
@@ +92,3 @@
+{
+
+}

Unnecessary empty line.


Comment at: clang-tidy-vs/ClangTidy/ClangTidy.vsct:22
@@ +21,3 @@
+  
+
+

Unnecessary empty line, same below in many places here.


Comment at: clang-tidy-vs/ClangTidy/ClangTidyPropertyGrid.cs:178
@@ +177,3 @@
+{
+
+}

Unnecessary empty line.


Comment at: clang-tidy-vs/ClangTidy/PropertyFileParser.cs:14
@@ +13,3 @@
+{
+
+public class CheckOption

Unnecessary empty line.


Comment at: clang-tidy-vs/ClangTidy/PropertyFileParser.cs:23
@@ +22,3 @@
+}
+public class ClangTidyYaml
+{

Please add empty line.


https://reviews.llvm.org/D23848



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits