[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<bool>("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