1359463 - Move clang plugin checkers into a subdirectory

Status: NEW

(Developer Infrastructure :: Source Code Analysis, enhancement, P3)

Description

[Nika Layzell [:nika] (ni? for response)]

Currently all C++ files involved in building the clang plugin are found directly in build/clang-plugin/. This includes both utility/scaffolding files (like DiagnosticsMatcher.h/cpp and Util.h), as well as individual checkers (like ArithmeticArgChecker.h/cpp). 

It would increase the readability of the code in this directory if these two types of files were separated, such as by putting the meta checker files in the build/clang-plugin/ directory, while moving the individual checkers into build/clang-plugin/checker.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Ehsan, you designed the current layout for the clang plugin. How would you feel about these changes?

Comment 2

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #1)
> Ehsan, you designed the current layout for the clang plugin. How would you
> feel about these changes?

That's fine...  But to be honest the next thing that I had in my mind to increase the readability of all of this was to add a docs directory with a .md file per checker explaining what these are doing.  :-)  I was *just* talking to janx on IRC and wishing we had docs for these checks!

But this sounds fine.  Please make sure to update import_mozilla_checks.py to make sure the clang-tidy jobs won't break...

Comment 3

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #2)
> (In reply to Michael Layzell [:mystor] from comment #1)
> > Ehsan, you designed the current layout for the clang plugin. How would you
> > feel about these changes?
> 
> That's fine...  But to be honest the next thing that I had in my mind to
> increase the readability of all of this was to add a docs directory with a
> .md file per checker explaining what these are doing.  :-)  I was *just*
> talking to janx on IRC and wishing we had docs for these checks!
> 
> But this sounds fine.  Please make sure to update import_mozilla_checks.py
> to make sure the clang-tidy jobs won't break...

This idea mostly came from the pain of me working on the clang plugin and being unable to find the helper files which contain the functions we've defined to help implement checkers. 

I'll make sure that import_mozilla_checks.py keeps working.

Once we land this we could even have a .md file next to each *Checker.cpp/h file with a description of that checker right next to the file, or we could take advantage of the fact that the header files are short and add long comments to the beginning of each checker's .h file which describe what that checker do. No matter what I would probably add a README.md file in the root of build/clang-plugin which explains what is going on in this directory and where to look to get the documentation.

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Can we rename clang-plugin to clang-static-analyzer?

clang-plugin can be anything. it is making the static analyzer work less discovererable.

Comment 5

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #3)
> Once we land this we could even have a .md file next to each *Checker.cpp/h
> file with a description of that checker right next to the file, or we could
> take advantage of the fact that the header files are short and add long
> comments to the beginning of each checker's .h file which describe what that
> checker do. No matter what I would probably add a README.md file in the root
> of build/clang-plugin which explains what is going on in this directory and
> where to look to get the documentation.

Cool.  FWIW my original plan was to have something compatible/similar to http://clang.llvm.org/extra/clang-tidy/checks/list.html but I forget where the source for these docs lives...  :-)

(In reply to Sylvestre Ledru [:sylvestre] from comment #4)
> Can we rename clang-plugin to clang-static-analyzer?
> 
> clang-plugin can be anything. it is making the static analyzer work less
> discovererable.

That should probably move to a different bug...  Note that there is a configure flag using this name, in-tree mozconfigs, possibly people's out of tree mozconfigs, etc to worry about.  I have previously considered doing this and have given up on the idea after enumerating the list of things to think about.  :-)  But I would love it if someone else does that hard work.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

> Cool.  FWIW my original plan was to have something compatible/similar to
> http://clang.llvm.org/extra/clang-tidy/checks/list.html but I forget where
> the source for these docs lives...  :-)
https://github.com/llvm-mirror/clang-tools-extra/tree/master/docs/clang-tidy/checks

> (In reply to Sylvestre Ledru [:sylvestre] from comment #4)
> > Can we rename clang-plugin to clang-static-analyzer?
> > 
> > clang-plugin can be anything. it is making the static analyzer work less
> > discovererable.
> 
> That should probably move to a different bug...  Note that there is a
> configure flag using this name, in-tree mozconfigs, possibly people's out of
> tree mozconfigs, etc to worry about.  I have previously considered doing
> this and have given up on the idea after enumerating the list of things to
> think about.  :-)  But I would love it if someone else does that hard work.
OK! Will see what we can do here!

Comment 7

[Andi [:andi]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #6)
> > Cool.  FWIW my original plan was to have something compatible/similar to
> > http://clang.llvm.org/extra/clang-tidy/checks/list.html but I forget where
> > the source for these docs lives...  :-)
> https://github.com/llvm-mirror/clang-tools-extra/tree/master/docs/clang-tidy/
> checks
> 
> > (In reply to Sylvestre Ledru [:sylvestre] from comment #4)
> > > Can we rename clang-plugin to clang-static-analyzer?
> > > 
> > > clang-plugin can be anything. it is making the static analyzer work less
> > > discovererable.
> > 
> > That should probably move to a different bug...  Note that there is a
> > configure flag using this name, in-tree mozconfigs, possibly people's out of
> > tree mozconfigs, etc to worry about.  I have previously considered doing
> > this and have given up on the idea after enumerating the list of things to
> > think about.  :-)  But I would love it if someone else does that hard work.
> OK! Will see what we can do here!

I've opened this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1362948 and marked it as a good first bug. If we won't find any volunteers for it soon I'll do the job.