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

[Jan Svoboda via Phabricator via llvm-commits]

jansvoboda11 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;
 
----------------
dexonsmith wrote:
> 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?
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.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:287
+static void
+denormalizeString(SmallVectorImpl<const char *> &Args, const char *Spelling,
+                  CompilerInvocation::StringAllocator SA, unsigned, T &&Value) {
----------------
dexonsmith wrote:
> 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));
> }
> ```
> 
That looks good, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84186