[PATCH] D36782: [Bash-autocompletion] Add support for static analyzer flags

[Raphael Isemann via Phabricator via cfe-commits]

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