This revision was automatically updated to reflect the committed changes.
Closed by commit rL352470: [analyzer] Added a checklist to help checker authors
and reviewers (authored by xazax, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/
NoQ accepted this revision.
NoQ added a comment.
Thanks!!~
I'm slow, but at least i admit it... Just made myself a fancier phabricator
query so that not to forget about patches, so hopefully i won't forget about
patches that often anymore :/
I mean, seriously, i'm very happy that we're all doi
xazax.hun updated this revision to Diff 176403.
xazax.hun marked 4 inline comments as done.
xazax.hun added a comment.
- Addressed further comments.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D52984/new/
https://reviews.llvm.org/D52984
Files:
www/analyzer/checker_dev_manual.html
I
NoQ added inline comments.
Herald added a subscriber: gamesh411.
Comment at: www/analyzer/checker_dev_manual.html:678
+Making Your Check Better
+
Probably `Checker` here as well.
Comment at: www/analyzer/checker_dev_manual.html:681
+User faci
Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.
LGTM! Let's continue the conversation about coding standards somewhere else.
Can you please mark inlines as done?
https://reviews.llvm.org/D52984
xazax.hun updated this revision to Diff 173654.
xazax.hun added a comment.
- Use the term `checker` instead of `check`.
https://reviews.llvm.org/D52984
Files:
www/analyzer/checker_dev_manual.html
Index: www/analyzer/checker_dev_manual.html
Szelethus added a comment.
Mainly my problem is that checker files can be large and hard to maneuver, some
forward declare and document everything, some are a big bowl of spaghetti, and
I think having a checklist of how it should be organized to keep uniformity and
readability would be great.
Szelethus added a comment.
In https://reviews.llvm.org/D52984#1294258, @NoQ wrote:
> In https://reviews.llvm.org/D52984#1294233, @aaron.ballman wrote:
>
> > Personally, I think it's detrimental to the community for subprojects to
> > come up with their own coding guidelines. My preference is for
NoQ added a comment.
In https://reviews.llvm.org/D52984#1294233, @aaron.ballman wrote:
> Personally, I think it's detrimental to the community for subprojects to come
> up with their own coding guidelines. My preference is for the static analyzer
> to come more in line with the rest of the proj
aaron.ballman added a comment.
In https://reviews.llvm.org/D52984#1294223, @xazax.hun wrote:
> - Move the checklist up before additional info in the HTML file.
> - Fix minor nits.
> - Add missing bullet points (thanks @Szelethus for noticing)
>
> I did not add any coding convention related item
xazax.hun added inline comments.
Comment at: www/analyzer/checker_dev_manual.html:719
+
+User facing documentation is important for adoption! Make sure the check
list updated
+at the homepage of the analyzer. Also ensure that the description is good
quality in
-
xazax.hun updated this revision to Diff 173513.
xazax.hun marked 11 inline comments as done.
xazax.hun added a comment.
- Move the checklist up before additional info in the HTML file.
- Fix minor nits.
- Add missing bullet points (thanks @Szelethus for noticing)
I did not add any coding conventi
Szelethus added inline comments.
Comment at: www/analyzer/checker_dev_manual.html:719
+
+User facing documentation is important for adoption! Make sure the check
list updated
+at the homepage of the analyzer. Also ensure that the description is good
quality in
-
xazax.hun added inline comments.
Comment at: www/analyzer/checker_dev_manual.html:719
+
+User facing documentation is important for adoption! Make sure the check
list updated
+at the homepage of the analyzer. Also ensure that the description is good
quality in
-
NoQ added a comment.
Thx!! Tiny nits.
Comment at: www/analyzer/checker_dev_manual.html:720
+User facing documentation is important for adoption! Make sure the check
list updated
+at the homepage of the analyzer. Also ensure that the description is good
quality in
+Che
Szelethus added a comment.
In https://reviews.llvm.org/D52984#1269850, @NoQ wrote:
> - Warning and note messages should be clear and easy to understand, even if a
> bit long.
> - Messages should start with a capital letter (unlike Clang warnings!) and
> should not end with `.`.
> - Articles
Szelethus added a comment.
Also, context is missing :)
https://reviews.llvm.org/D52984
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Szelethus added a comment.
We probably should also add an entry about some code conventions we use here,
for example, the use of `auto` was debated recently when used with
`SVal::getAs`, maybe something like this:
- As an LLVM subproject, the code in the Static Analyzer should too follow the
h
xazax.hun updated this revision to Diff 172354.
xazax.hun added a comment.
This new version based on the bullets by NoQ. I also included some additional
ones from other lists and added some new ones, e.g. the NamedDecl::getName will
fail if the name of the decl is not a single token. I also reor
NoQ added a comment.
Ok, so where do we put it? :)
I think we should put the everyday checklist into the checker developer manual
under a title like "Making Your Checker Better" and put the out-of-alpha
checklist somewhere into docs/analyzer as plain text (because that's not a sort
of info you
Szelethus added inline comments.
Comment at: www/analyzer/checker_dev_manual.html:720
+Are Checkers.td and the CMakeLists entry alphabetically ordered?
+
+
aaron.ballman wrote:
> Do we want to add an item for "Is the help text for the checker a useful
> descript
aaron.ballman added inline comments.
Comment at: www/analyzer/checker_dev_manual.html:720
+Are Checkers.td and the CMakeLists entry alphabetically ordered?
+
+
Do we want to add an item for "Is the help text for the checker a useful
description of what the check
Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.
I think @NoQ's list would be great to add! @dkrupp, as far as I'm aware, has
some works in progress to avoid the usage of HTML, so let's put this on hold
for a bit.
Repositor
NoQ added a comment.
Yes. Not just for checkers, not just for beginners :)
https://reviews.llvm.org/D52984
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Szelethus added a comment.
One more thing.
From what I've seen, new checkers are in an overwhelming majority (again, from
what **I've** seen) made by beginners, so I'd propose to include comments in
checkers that are at least in part understandable by a beginner. I don't mean
to make make 60%
NoQ added a comment.
Hmm, i guess i sent this out too early.
As per our long-lasting tradition, i forgot the point about the web page :\
Bullets about RUN-lines, Checkers.td descriptions and CMakeLists are also great
for everyday use :)
https://reviews.llvm.org/D52984
_
NoQ added a comment.
My take on the out-of-alpha checklist:
- The checker should be evaluated on a large codebase in order to discover
crashes and false positives specific to the checker. It should demonstrate the
expected reasonably good results on it, with the stress on having as little
fals
NoQ added a comment.
> is it though?
If we simply add a new menu point, it'll require us to modify a huge number of
files. We might get away with good comments that explain the situation and
making sure that all changes are done with sed, but i guess we'll still be
stuck forever.
> HTML files
george.karpenkov added a comment.
> This too will most likely require server maintainers to intervene, so i think
> fixing SSI is more realistic.
Not really.
The way it is done e.g. for
http://clang.llvm.org/docs/LibASTMatchersReference.html is the HTML files are
in the repo, there is a script
NoQ added a comment.
> we probably also should be moving towards an auto-generated model anyway, if
> we want to e.g. automatically build sections of a website from the code.
This too will most likely require server maintainers to intervene, so i think
fixing SSI is more realistic.
https://re
george.karpenkov added a comment.
> inline the header into every page, which is horrible
is it though?
we probably also should be moving towards an auto-generated model anyway, if we
want to e.g. automatically build sections of a website from the code.
https://reviews.llvm.org/D52984
_
NoQ added a comment.
In https://reviews.llvm.org/D52984#1260761, @george.karpenkov wrote:
> @Szelethus @xazax.hun Since you guys are looking into the website, do you
> know why the top bar has disappeared from all pages? (E.g.
> http://clang-analyzer.llvm.org/available_checks.html )
The relev
Szelethus added a comment.
I noticed it too, it's already on my TODO list. Didn't look into the causes
just yet though.
https://reviews.llvm.org/D52984
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/li
george.karpenkov added a comment.
Herald added a subscriber: donat.nagy.
@Szelethus @xazax.hun Since you guys are looking into the website, do you know
why the top bar has disappeared from all pages? (E.g.
http://clang-analyzer.llvm.org/available_checks.html )
https://reviews.llvm.org/D52984
MTC added a comment.
Greate idea! If we can enrich this list, it will not only help the reviewer,
but also help beginners, like me, avoid some pitfalls when developing a new
checker.
I agree with NoQ to classify the lists. In addition to the two categories
proposed by NoQ, there is another cla
xazax.hun added inline comments.
Comment at: www/analyzer/checker_dev_manual.html:708
+Checker Reviewer Checklist
+
NoQ wrote:
> I think we actually need two separate checklists:
> * for common bugs (careful use of non-fatal error nodes, etc. - stuff we
> chec
NoQ added inline comments.
Comment at: www/analyzer/checker_dev_manual.html:708
+Checker Reviewer Checklist
+
I think we actually need two separate checklists:
* for common bugs (careful use of non-fatal error nodes, etc. - stuff we check
for on every review)
Szelethus added a comment.
LGTM! I can think of a couple more, like
- making sure that `clang-format` was run on the new files,
- not forgetting header guards,
- making sure that variables and functions only intended for use within an
`assert` call are handled in a way that release build bots wo
xazax.hun updated this revision to Diff 168664.
xazax.hun added a comment.
- Added the ideas from Kristof.
https://reviews.llvm.org/D52984
Files:
www/analyzer/checker_dev_manual.html
Index: www/analyzer/checker_dev_manual.html
Szelethus added a comment.
I love the idea. I got a couple more:
Is `-analyzer-checker=core` included in all `RUN:` lines?
Is the description in `Checkers.td` clear to a regular user?
Is `Checkers.td` and the CMakeLists file alphabetically sorted?
Repository:
rC Clang
https://reviews.llvm.or
xazax.hun created this revision.
xazax.hun added reviewers: NoQ, george.karpenkov, Szelethus, rnkovacs, szepet,
dcoughlin, a.sidorin, MTC.
xazax.hun added a project: clang.
Herald added subscribers: mikhail.ramalho, dkrupp, baloghadamsoftware,
whisperity.
This is a first proposal to have a check
41 matches
Mail list logo
