[clang][cli] Convert Analyzer option string based options to new option parsing system

[jansvoboda11]

Summary

Depends on D84185

[dang]

Update with latest changes

[jansvoboda11]

Taking over this patch, as Daniel is no longer involved.

[jansvoboda11]

Rebase, remove InlineMaxStackDepth initialization from AnalyzerOptions.h

[jansvoboda11]

Ready for a review.

clang/lib/Frontend/CompilerInvocation.cpp

261

We should keep an eye on the number of instantiations of this function template (and normalizeStringIntegral).

If it grows, we can employ the SFINAE trick used for makeFlagToValueNormalizer.

[dexonsmith]

clang/lib/Frontend/CompilerInvocation.cpp

261

Does it work to take the parameter as a Twine to avoid the template?

static void
denormalizeString(SmallVectorImpl<const char *> &Args, const char *Spelling,
                  CompilerInvocation::StringAllocator SA, unsigned, Twine Value) {
  Args.push_back(Spelling);
  Args.push_back(SA(Value));
}

[jansvoboda11]

clang/lib/Frontend/CompilerInvocation.cpp

261

In general no. The Twine constructor is explicit for some types (integers, chars, etc.).

[dexonsmith]

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

261–263 ↗

(On Diff #311527)

Hmm, I wonder if this is entirely better. It's not obvious at all, looking at the code here, whether InlineMaxStackDepth can be used without getting initialized at all.

I agree we should have the default value in two places -- I think removing assignment to 5 is the right thing to do -- but I'm not sure leaving this entirely uninitialized is a good thing. WDYT of initializing it to 0?

clang/lib/Frontend/CompilerInvocation.cpp

261

Okay, then I suggest at least handling the ones that are convertible separately (without the template):

static void denormalizeString(..., Twine Value) {
  Args.push_back(Spelling);
  Args.push_back(SA(Value));
}

template <class T,
          std::enable_if_t<!std::is_convertible<T, Twine>::value &&
                           std::is_constructible<Twine, T>::value, bool> = false>
static void denormalizeString(..., T Value) {
  denormalizeString(..., Twine(Value));
}

263–264

I think it'd be better to reduce the overload set using std::enable_if than to add a static_assert here.

[jansvoboda11]

Reduce number of instantiations of denormalizeString template

[jansvoboda11]

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

261–263 ↗

(On Diff #311527)

I think leaving this uninitialized communicates the fact that it will be initialized somewhere else.

If I saw unsigned InlineMacStackDepth = 0; and then observed it changing to 5 without passing -analyzer-inline-max-stack-depth=5, I'd be surprised.

clang/lib/Frontend/CompilerInvocation.cpp

261

That looks good, thanks.

[dexonsmith]

LGTM.

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h

261–263 ↗

(On Diff #311527)

Okay, that's fair.

clang/include/clang/Driver/Options.td

def analyzer_opt_analyze_headers : Flag<["-"], "analyzer-opt-analyze-headers">,
   MarshallingInfoFlag<"AnalyzerOpts->AnalyzeAll", "false">;
 def analyzer_opt_analyze_nested_blocks : Flag<["-"], "analyzer-opt-analyze-nested-blocks">,
   HelpText<"Analyze the definitions of blocks in addition to functions">,
   MarshallingInfoFlag<"AnalyzerOpts->AnalyzeNestedBlocks", "false">;
 def analyzer_display_progress : Flag<["-"], "analyzer-display-progress">,
   HelpText<"Emit verbose output about the analyzer's progress">,
   MarshallingInfoFlag<"AnalyzerOpts->AnalyzerDisplayProgress", "false">;
 def analyze_function : Separate<["-"], "analyze-function">,
-  HelpText<"Run analysis on specific function (for C++ include parameters in name)">;
+  HelpText<"Run analysis on specific function (for C++ include parameters in name)">,
+  MarshallingInfoString<"AnalyzerOpts->AnalyzeSpecificFunction">;
 def analyze_function_EQ : Joined<["-"], "analyze-function=">, Alias<analyze_function>;
 def trim_egraph : Flag<["-"], "trim-egraph">,
   HelpText<"Only show error-related paths in the analysis graph">,
   MarshallingInfoFlag<"AnalyzerOpts->TrimGraph", "false">;
 def analyzer_viz_egraph_graphviz : Flag<["-"], "analyzer-viz-egraph-graphviz">,
   HelpText<"Display exploded graph using GraphViz">,
   MarshallingInfoFlag<"AnalyzerOpts->visualizeExplodedGraphWithGraphViz", "false">;
 def analyzer_dump_egraph : Separate<["-"], "analyzer-dump-egraph">,
   HelpText<"Dump exploded graph to the specified file">,
   MarshallingInfoString<"AnalyzerOpts->DumpExplodedGraphTo">;
 def analyzer_dump_egraph_EQ : Joined<["-"], "analyzer-dump-egraph=">, Alias<analyzer_dump_egraph>;
 
 def analyzer_inline_max_stack_depth : Separate<["-"], "analyzer-inline-max-stack-depth">,
-  HelpText<"Bound on stack depth while inlining (4 by default)">;
+  HelpText<"Bound on stack depth while inlining (4 by default)">,
+  MarshallingInfoStringInt<"AnalyzerOpts->InlineMaxStackDepth", "5">;
 def analyzer_inline_max_stack_depth_EQ : Joined<["-"], "analyzer-inline-max-stack-depth=">,
   Alias<analyzer_inline_max_stack_depth>;
 
 def analyzer_inlining_mode : Separate<["-"], "analyzer-inlining-mode">,
   HelpText<"Specify the function selection heuristic used during inlining">;
 def analyzer_inlining_mode_EQ : Joined<["-"], "analyzer-inlining-mode=">, Alias<analyzer_inlining_mode>;
 
 def analyzer_disable_retry_exhausted : Flag<["-"], "analyzer-disable-retry-exhausted">,
   HelpText<"Do not re-analyze paths leading to exhausted nodes with a different strategy (may decrease code coverage)">,
   MarshallingInfoFlag<"AnalyzerOpts->NoRetryExhausted", "false">;
 
 def analyzer_max_loop : Separate<["-"], "analyzer-max-loop">,
-  HelpText<"The maximum number of times the analyzer will go through a loop">;
+  HelpText<"The maximum number of times the analyzer will go through a loop">,
+  MarshallingInfoStringInt<"AnalyzerOpts->maxBlockVisitOnPath", "4">;
 def analyzer_stats : Flag<["-"], "analyzer-stats">,
   HelpText<"Print internal analyzer statistics.">,
   MarshallingInfoFlag<"AnalyzerOpts->PrintStats", "false">;
 
 def analyzer_checker : Separate<["-"], "analyzer-checker">,
   HelpText<"Choose analyzer checkers to enable">,
   ValuesCode<[{
     const char *Values =

clang/lib/Frontend/CompilerInvocation.cpp

 #include <algorithm>
 #include <atomic>
 #include <cassert>
 #include <cstddef>
 #include <cstring>
 #include <memory>
 #include <string>
 #include <tuple>
+#include <type_traits>
 #include <utility>
 #include <vector>
 
 using namespace clang;
 using namespace driver;
 using namespace options;
 using namespace llvm::opt;
 
static Optional<std::string> normalizeString(OptSpecifier Opt, int TableIndex,
                                              const ArgList &Args,
                                              DiagnosticsEngine &Diags) {
   auto *Arg = Args.getLastArg(Opt);
   if (!Arg)
     return None;
   return std::string(Arg->getValue());
 }
 
+template <typename T>
 static void denormalizeString(SmallVectorImpl<const char *> &Args,
                               const char *Spelling,
                               CompilerInvocation::StringAllocator SA,
-                              unsigned TableIndex, const std::string &Value) {
+                              unsigned TableIndex, T &&Value) {
+  static_assert(std::is_constructible<Twine, T>::value,
+                "Cannot convert this value to Twine");
   Args.push_back(Spelling);
-  Args.push_back(SA(Value));
+  Args.push_back(SA(Twine(std::forward<T>(Value))));
+}
+
+template <typename IntTy>
+static Optional<IntTy> normalizeStringIntegral(OptSpecifier Opt, int TableIndex,
+                                               const ArgList &Args,
+                                               DiagnosticsEngine &Diags) {
+  auto *Arg = Args.getLastArg(Opt);
+  if (!Arg)
+    return None;
+  IntTy Res;
+  if (StringRef(Arg->getValue()).getAsInteger(0, Res)) {
+    Diags.Report(diag::err_drv_invalid_int_value)
+        << Arg->getAsString(Args) << Arg->getValue();
+  }
+  return Res;
 }
 
 static Optional<std::string> normalizeTriple(OptSpecifier Opt, int TableIndex,
                                              const ArgList &Args,
                                              DiagnosticsEngine &Diags) {
   auto *Arg = Args.getLastArg(Opt);
   if (!Arg)
     return None;
#include "clang/StaticAnalyzer/Core/Analyses.def"
 
   Opts.ShouldEmitErrorsOnInvalidConfigValue =
       /* negated */ !llvm::StringSwitch<bool>(
           Args.getLastArgValue(OPT_analyzer_config_compatibility_mode))
           .Case("true", true)
           .Case("false", false)
           .Default(false);
 
-  Opts.AnalyzeSpecificFunction =
-      std::string(Args.getLastArgValue(OPT_analyze_function));
-  Opts.maxBlockVisitOnPath =
-      getLastArgIntValue(Args, OPT_analyzer_max_loop, 4, Diags);
-  Opts.InlineMaxStackDepth =
-      getLastArgIntValue(Args, OPT_analyzer_inline_max_stack_depth,
-                         Opts.InlineMaxStackDepth, Diags);
-
   Opts.CheckersAndPackages.clear();
   for (const Arg *A :
        Args.filtered(OPT_analyzer_checker, OPT_analyzer_disable_checker)) {
     A->claim();
     bool IsEnabled = A->getOption().getID() == OPT_analyzer_checker;
     // We can have a list of comma separated checker names, e.g:
     // '-analyzer-checker=cocoa,unix'
     StringRef CheckerAndPackageList = A->getValue();

llvm/include/llvm/Option/OptParser.td

 }
 
 class MarshallingInfoString<code keypath, code defaultvalue="std::string()">
   : MarshallingInfo<keypath, defaultvalue> {
   code Normalizer = "normalizeString";
   code Denormalizer = "denormalizeString";
 }
 
+class MarshallingInfoStringInt<code keypath, code defaultvalue="0", code type="unsigned">
+  : MarshallingInfo<keypath, defaultvalue> {
+  code Normalizer = "normalizeStringIntegral<"#type#">";
+  code Denormalizer = "denormalizeString";
+}
+
 class MarshallingInfoFlag<code keypath, code defaultvalue>
   : MarshallingInfo<keypath, defaultvalue> {
   code Normalizer = "normalizeSimpleFlag";
   code Denormalizer = "denormalizeSimpleFlag";
 }
 
 class MarshallingInfoBitfieldFlag<code keypath, code value> : MarshallingInfoFlag<keypath, "0u"> {
   code Normalizer = "(normalizeFlagToValue<unsigned, "#value#">)";