teemperor added a comment.
That's a really nice approach to this problem, good job Yuka! See my inline
comments for some minor remarks.
================
Comment at: clang/include/clang/Driver/CC1Options.td:104
+ const char* Values =
+#define GET_CHECKERS
+#define CHECKER(FULLNAME, CLASS, DESCFILE, HT, G, H) FULLNAME ","
----------------
Please also add the `PACKAGE` macros from Checkers.inc here. There are also
valid macros:
```
#define GET_PACKAGES
#define PACKAGE(FULLNAME, G, D) FULLNAME ","
#include "clang/StaticAnalyzer/Checkers/Checkers.inc"
#undef GET_PACKAGES
```
================
Comment at: clang/include/clang/Driver/CC1Options.td:107
+#include "clang/StaticAnalyzer/Checkers/Checkers.inc"
+#undef GET_CHECKERS
+;
----------------
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 =
#define GET_CHECKERS
#define CHECKER(FULLNAME, CLASS, DESCFILE, HT, G, H) FULLNAME ","
#include "clang/StaticAnalyzer/Checkers/Checkers.inc"
#undef GET_CHECKERS
;
```
================
Comment at: clang/test/Driver/autocomplete.c:97
+// RUN: %clang --autocomplete=-analyzer-checker, | FileCheck %s
-check-prefix=ANALYZER
+// ANALYZER: alpha.clone.CloneChecker
----------------
Can you test for `unix.Malloc` instead which is more mature?
`alpha.clone.CloneChecker` will probably change soon-ish :)
================
Comment at: llvm/include/llvm/Option/OptTable.h:60
private:
/// \brief The static option information table.
+ std::vector<Info> OptionInfos;
----------------
This is no longer static :)
================
Comment at: llvm/include/llvm/Option/OptTable.h:149
+ /// \param [in] Option - Prefix + Name of the flag which Values will be
+ // changed
+ /// \param [in] Values - String of Values seperated by ",", such as
----------------
`///` instead of `//` (same below).
Also maybe add an example here like `... which values will be changed. For
example "-fstd="/`.
================
Comment at: llvm/include/llvm/Option/OptTable.h:153
+ // takes
+ void addValues(const char *Option, const char *Values);
+
----------------
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.
================
Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:309
+ if (!isa<UnsetInit>(R.getValueInit("ValuesCode"))) {
+ OS << R.getValueAsString("ValuesCode");
+ OS << "\n";
----------------
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.
================
Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:312
+ for (const std::string &Pref : R.getValueAsListOfStrings("Prefixes")) {
+ OS << "Opt.addValues(";
+ std::string S = (Pref + R.getValueAsString("Name")).str();
----------------
If addValues returns true, we should also assert here if it fails (which should
never happen unless something seriously breaks).
https://reviews.llvm.org/D36782
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
