Page MenuHomePhabricator

[analyzer] Document the frontend library
Needs ReviewPublic

Authored by Szelethus on Feb 11 2019, 10:33 AM.

Details

Summary

Since I've had my fair share of complaining about the lack of documentation, it's time I did my part, hence this patch, which provides documentation for a good majority of the frontend library.

Preview:
http://cc.elte.hu/clang-docs/docs/analyzer-frontend-html/analyzer/developer-docs/FrontendLibrary.html

Several more items could be added, describing how AnalysisConsumer interacts with AnalysisManager would be the most important of those. For now, I put the greatest emphasis on checker registration.

What should this be expanded with? Is this document too large, maybe I should split it up? I haven't documented much so far, so I'd happily take any feedback.

Diff Detail

Event Timeline

Szelethus created this revision.Feb 11 2019, 10:33 AM

Please, in the first round of reviews, if you could make high level comments about what should or should not be here would be great, perfectly wrapping around the 80 column limit and finding typos don't concern me as much before the actual content is ready.

NoQ added a comment.Feb 15 2019, 4:53 PM

Had a look. Great stuff, just as planned :) Old fanboy wisdom: Try to avoid documenting bugs you want to fix! But i don't have many high-level comments here - only appreciation of the effort.

Please, in the first round of reviews, if you could make high level comments about what should or should not be here would be great, perfectly wrapping around the 80 column limit and finding typos don't concern me as much before the actual content is ready.

This should actually be sticked on top of every phabricator page (:

docs/analyzer/developer-docs/FrontendLibrary.rst
11–13

First of all, "frontend" is, as far as i understand, a weird word to use with respect to this library in general. I think what they were trying to say was something like "The Static Analyzer-specific part of the C Front End's command-line flags" (as opposed to Driver flags), but calling this UI "The Frontend" is a bit weird. We probablyshould try to somehow avoid confusion with the "compiler frontend" concept throughout this document.

15–16

Eg., we're talking about TableGen because in our case it's all about providing values for frontend flags (Checkers.td provides values for the -analyzer-checker flag, etc.). It's not because the process of compiling the Analyzer is an essential part of the very idea of the "Frontend". So it sounds a bit strange. Across the whole Clang there are many other uses of TableGen.

57–88

For the above reason i think this text deserves a better document to be put into; this is definitely important to know for a much wider audience than developers of libStaticAnalyzerFrontend.

216–220

I think originally the whole Static Analyzer was referred to as "The Checker" - and was, essentially, just one checker :)

638–643

I really need to refresh my knowledge on this one. Why is it not on by default? Why did we need ASTImporter for cross-translation-unit analysis when we already had this? Why do we need this when we already have ASTImporter? Etc.

CTU is, besides everything else, a great alternative to body farming. We can write down models as normal code and load them via CTU and in fact it'll help us avoid ASTImporter imperfections by simply only writing models that ASTImporter is able to understand.

Thanks for the review on my patches! I'm a little busy atm (I've also wasted my second weekend trying to make Static Analyzer unit tests run under check-clang-analysis), but I'll see to fixing these in the coming weeks.

In D58065#1400154, @NoQ wrote:

Old fanboy wisdom: Try to avoid documenting bugs you want to fix!

Oh, which one do you mean?

NoQ added a comment.Feb 17 2019, 9:00 PM

I've also wasted my second weekend trying to make Static Analyzer unit tests run under check-clang-analysis

:) I tried to have a quick look but got confused pretty quickly.

In D58065#1400154, @NoQ wrote:

Old fanboy wisdom: Try to avoid documenting bugs you want to fix!

Oh, which one do you mean?

Nothing in particular yet, just my own mistakes :p

High-level feedback: mixing of abstraction levels is wrong for the "bundled" documentation. This might also work better as a blogpost, if you want to jump from topic to topic.

docs/analyzer/developer-docs/FrontendLibrary.rst
11–13

+1, not sure what the word "frontend" adds here.
IMO "frontend" in folder/library name is more of a relic in this case.

57–88

Strictly speaking for tests it's better to use check-clang-analyzer.
Also again strictly speaking it's not possible to compile analyzer without compiling clang.

85

Information on compiling LLVM IMO should not be here.
Also, why Sphinx? Why X86? Why LLD and not gold?

mgrang added a subscriber: mgrang.Feb 19 2019, 4:15 PM
mgrang added inline comments.
docs/analyzer/developer-docs/FrontendLibrary.rst
12

typo: it's --> its

42

typo: severeral --> several

whisperity added inline comments.Mar 12 2019, 3:24 AM
docs/analyzer/developer-docs/FrontendLibrary.rst
85

Information on compiling LLVM IMO should not be here.
Also, why Sphinx? Why X86? Why LLD and not gold?

Analyser for development -- however, you build release instead of (at least) RelWithDebInfo.

EXPORT_COMPILE_COMMANDS is also totally unnecessary for an "analyser for development".

Perhaps individual recommended settings could be linked back to the configuration guide's sections? (Unfortunately that guide can't link to individual options sadly.)

94

When we talk about the project, use Clang. When talking about the binary, clang in monospace is good style.

140

core for emphasis.