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



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

Reply via email to