This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jansvoboda11 on Jul 20 2020, 10:02 AM.

Diff Detail

Event Timeline

dang created this revision.Jul 20 2020, 10:02 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 20 2020, 10:02 AM
dang updated this revision to Diff 280948.Jul 27 2020, 9:47 AM

Update with latest changes

jansvoboda11 commandeered this revision.Dec 14 2020, 1:22 AM
jansvoboda11 added a reviewer: dang.
jansvoboda11 added a subscriber: jansvoboda11.

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

jansvoboda11 retitled this revision from Convert Analyzer option string based options to new option parsing system to [clang][cli] Convert Analyzer option string based options to new option parsing system.Dec 14 2020, 1:24 AM

Rebase, remove InlineMaxStackDepth initialization from AnalyzerOptions.h

Ready for a review.

clang/lib/Frontend/CompilerInvocation.cpp
287

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 requested changes to this revision.Dec 14 2020, 10:57 AM
dexonsmith added inline comments.
clang/lib/Frontend/CompilerInvocation.cpp
287

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));
}
This revision now requires changes to proceed.Dec 14 2020, 10:57 AM
jansvoboda11 added inline comments.Dec 15 2020, 1:31 AM
clang/lib/Frontend/CompilerInvocation.cpp
287

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

dexonsmith added inline comments.Dec 15 2020, 3:07 PM
clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
261–263

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
287

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));
}
289–290

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

Reduce number of instantiations of denormalizeString template

jansvoboda11 added inline comments.Dec 16 2020, 1:57 AM
clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
261–263

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
287

That looks good, thanks.

dexonsmith accepted this revision.Dec 16 2020, 10:38 AM

LGTM.

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
261–263

Okay, that's fair.

This revision is now accepted and ready to land.Dec 16 2020, 10:38 AM
This revision was landed with ongoing or failed builds.Dec 17 2020, 11:56 PM
This revision was automatically updated to reflect the committed changes.