Page MenuHomePhabricator

[Bash-autocompletion] Add support for static analyzer flags

Authored by yamaguchi on Aug 15 2017, 11:49 PM.



This is a patch for clang autocomplete feature.

It will collect values which -analyzer-checker takes, which is defined in
clang/StaticAnalyzer/Checkers/, dynamically.
First, from ValuesCode class in, TableGen will generate C++
code in will be included in DriverOptions.cpp, and
calls OptTable's addValues function. addValues function will add second
argument to Option's Values class. Values contains string like "foo,bar,.."
which is handed to Values class
in OptTable.

Diff Detail


Event Timeline

yamaguchi created this revision.Aug 15 2017, 11:49 PM
teemperor edited edge metadata.Aug 16 2017, 12:22 AM

That's a really nice approach to this problem, good job Yuka! See my inline comments for some minor remarks.

104 ↗(On Diff #111312)

Please also add the PACKAGE macros from here. There are also valid macros:

#include "clang/StaticAnalyzer/Checkers/"
107 ↗(On Diff #111312)

Maybe we should treat the indentation of 2 here as the minimum indentation. Otherwise this file becomes hard to read. I.e. this instead:

const char* Values =
#include "clang/StaticAnalyzer/Checkers/"
97 ↗(On Diff #111312)

Can you test for unix.Malloc instead which is more mature? alpha.clone.CloneChecker will probably change soon-ish :)

60 ↗(On Diff #111312)

This is no longer static :)

149 ↗(On Diff #111312)

/// instead of // (same below).

Also maybe add an example here like ... which values will be changed. For example "-fstd="/.

153 ↗(On Diff #111312)

Right now this function just silently fails when we specify an invalid option. I think returning a bool indicating success would make this more reliable.

We maybe should check if we can change the OptTable value array type that we don't have to do a , separated list of flags here but a proper vector, but that's out of the scope of this patch.

309 ↗(On Diff #111312)

We should wrap this in a scope. E..g emit { and } around so that if you reuse the same variable name we don't get redefinition errors.

312 ↗(On Diff #111312)

If addValues returns true, we should also assert here if it fails (which should never happen unless something seriously breaks).

yamaguchi updated this revision to Diff 111318.Aug 16 2017, 1:26 AM
yamaguchi marked 7 inline comments as done.

Update diff according to Raphael's comments.

teemperor added inline comments.Aug 16 2017, 2:45 AM
14 ↗(On Diff #111318)

I think the C++ version of the assert header is more consistent: #include <cassert>

313 ↗(On Diff #111318)

Don't call this inside an assert itself. The call would be removed on release builds (where assert is a no-op) and we wouldn't do the addValues anymore which breaks everything silently in release builds. Try something like this:

bool ValuesWereAdded = Opt.addValues(...);
(void)ValuesWereAdded; // Prevents unused variable warnings
assert(ValuesWereAdded && "Couldn't add values to OptTable!");
ruiu edited edge metadata.Aug 16 2017, 2:21 PM

This patch allows us to embed a piece of C++ code to each command line option to construct a list of argument candidates at runtime. With this patch, .inc files generated by OptParserEmitter contain C macros that in turn include other .inc files. That is a flexible mechanism but can be too flexible and fragile, as you can write any code in .td file, and pieces of C++ code you write in .td files are not verified even for syntax validation until that nested text inclusions are processed by a C++ compiler. It does two-stages of code generation, one is done by OptTable and the other is done by C macros. I can imagine that a single typo in a .td file can cause a mysterious error that is hard to debug. I feel like, even though command line processing is not that easy, I wonder if it is that complicated to require two-stage code generation.

That being said, I don't oppose to this patch, as people who know more about this code seem to like this approach. I'm just expressing my concern.

60 ↗(On Diff #111312)

Just remove "non-static" instead of saying this is non-static, as being non-const is not special.

yamaguchi updated this revision to Diff 111457.Aug 16 2017, 7:44 PM
yamaguchi marked 2 inline comments as done.

Update diff.

I understand your concern. However by doing this (rather than building functions for each flag), we will be able to get all information in OptTable itself. It is true that this is quite fragile, but adding ValuesCode to new flag is not complicated, so I believe overall it's fine.

yamaguchi marked 2 inline comments as done.

Update diff.

yamaguchi updated this revision to Diff 111548.Aug 17 2017, 1:01 PM

const char* Values -> const char *Values

ruiu added inline comments.Aug 17 2017, 1:06 PM
104 ↗(On Diff #111547)

const char* Values -> const char *Values

308 ↗(On Diff #111547)

Maybe it is better to do early continue to reduce indentation depth.

if (isa<UnsetInit>(R.getValueInit("ValuesCode")))
313–314 ↗(On Diff #111547)

You can combine these two lines:

OS << "bool ValuesWereAdded = Opt.addValues(";
318–319 ↗(On Diff #111547)

It is more readable if you split it at a natural boundary:

OS << "(void)ValuesWereAdded;\n";
OS << "assert(ValuesWereAdded && \"Couldn't add values to OptTable!\");\n";

I addressed your comments in D36820, so please take a look at it. Thank you!

teemperor accepted this revision.Aug 22 2017, 7:25 AM

Found one more minor comment typo. And could you do your changes to OptParserEmitter.cpp all in this patch? Because Rui/Me pointed out those things on this review, so this patch should also fix them IMHO :)

Otherwise this is good to go, nice work!

152 ↗(On Diff #111548)

/// instead of //.

This revision is now accepted and ready to land.Aug 22 2017, 7:25 AM
This revision was automatically updated to reflect the committed changes.
yamaguchi marked an inline comment as done.