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

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:261-263
   /// The inlining stack depth limit.
-  // Cap the stack depth at 4 calls (5 stack frames, base + 4 calls).
-  unsigned InlineMaxStackDepth = 5;
+  unsigned InlineMaxStackDepth;
 
----------------
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?


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:287
+static void
+denormalizeString(SmallVectorImpl<const char *> &Args, const char *Spelling,
+                  CompilerInvocation::StringAllocator SA, unsigned, T &&Value) {
----------------
jansvoboda11 wrote:
> dexonsmith wrote:
> > jansvoboda11 wrote:
> > > 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`.
> > 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));
> > }
> > ```
> In general no. The `Twine` constructor is `explicit` for some types (integers, chars, etc.).
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));
}
```



================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:289-290
+                  CompilerInvocation::StringAllocator SA, unsigned, T &&Value) {
+  static_assert(std::is_constructible<Twine, T>::value,
+                "Cannot convert this value to Twine");
   Args.push_back(Spelling);
----------------
I think it'd be better to reduce the overload set using `std::enable_if` than to add a `static_assert` here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84186/new/

https://reviews.llvm.org/D84186