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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
