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