Page MenuHomePhabricator

[analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.
Needs ReviewPublic

Authored by NoQ on Tue, Sep 10, 4:59 PM.

Details

Summary

We're finally more or less ready to allow users outside of the Static Analyzer to take advantage of path diagnostic consumers for emitting their warnings in different formats. I didn't really try to do that in practice, but most of the necessary APIs should be at least available now.

I'm not planning to convert Clang-Tidy to use these APIs directly because i understand that they're more complicated than Clang-Tidy really needs them to be. I'll either simplify these APIs (if they indeed can be simplified) or (more likely) introduce a convenient wrapper for easily building path diagnostics.

I'm not sure about renaming these classes to get rid of the "Path" prefix. These diagnostics aren't necessarily displaying the path (and they never did), but they're flexible enough for displaying paths and that's still the primary difference between our path diagnostics and ordinary diagnostics. Also if we just call them "Diagnostics" it'll be too generic and hard to distinguish from the Clang diagnostic engine. Suggestions are welcome :)

Diff Detail

Event Timeline

NoQ created this revision.Tue, Sep 10, 4:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Sep 10, 4:59 PM
gribozavr requested changes to this revision.Wed, Sep 11, 6:04 AM
gribozavr added inline comments.
clang/include/clang/Analysis/PathDiagnosticConsumers.h
37

Does this code declare e.g., createPlistDiagnosticConsumer? That function is defined in libStaticAnalyzer. Declaring it libAnalysis is not appropriate, and effectively introduces a layering violation. Users who link against libAnalysis will get link errors if they don't link against libStaticAnalyzer.

This revision now requires changes to proceed.Wed, Sep 11, 6:04 AM
NoQ marked an inline comment as done.Wed, Sep 11, 11:23 AM
NoQ added inline comments.
clang/include/clang/Analysis/PathDiagnosticConsumers.h
37

Oh crap, i forgot to move them.

NoQ added a comment.Wed, Sep 11, 11:53 AM

Hmm, does anybody want me to write an example tool that emits path diagnostics but doesn't link to libStaticAnalyzer*?

NoQ updated this revision to Diff 219829.Wed, Sep 11, 4:19 PM

Unforget to actually move the consumer implementations to libAnalysis, not just their headers.

Casually rename ClangDiagPathDiagConsumer to TextDiagnostics according to the local naming tradition.

gribozavr added inline comments.Thu, Sep 12, 2:33 AM
clang/include/clang/Analysis/PathDiagnosticConsumers.def
23

80 columns?

clang/include/clang/Analysis/PathDiagnosticConsumers.h
36

Maybe not quite an appropriate comment for this patch, but I just realized that this factory function returns void, which confused me -- where does the new consumer go? Only after reading an implementation of one of these functions I understood that they add the new consumer to C, which is a vector (whose type is conveniently hidden by a typedef -- why?)

I'd suggest to make these factories more like factories, and make them return a unique_ptr that the caller can put wherever they need. Of course, in a separate patch.

I'd also suggest to remove the PathDiagnosticConsumers typedef altogether. It is used just a couple of times in the code, and obfuscates code more than it helps.

clang/lib/Analysis/PlistHTMLDiagnostics.cpp
27

Passing it by value would have made it less subtle, and probably as efficient.

clang/lib/Analysis/TextDiagnostics.cpp
23

"TextDiagnosticsConsumer"

Or even better, "TextPathDiagnosticsConsumer".

Same for the file name. Same for other files that define diagnostics consumers.

Clarity is really important here. Very few places in the code will mention this type by name, however, it is really important to distinguish this diagnostics infrastructure from the rest of Clang's diagnostics.

I'm also starting to wonder whether this diagnostics infrastructure should be in its own library, not in libAnalysis -- what do you think?