Optimizations:
- Only build the skeleton for each identifier once, rather than once for
each declaration of that identifier.
- Only compute the contexts in which identifiers are declared for
identifiers that have the same skeleton as another identifier in the
translation unit.
- Only compare pairs of declarations that are declared in related
contexts, rather than comparing all pairs of declarations with the same
skeleton.
Also simplify by removing the caching of enclosing `DeclContext` sets,
because with the above changes we don't even compute the enclosing
`DeclContext` sets in common cases. Instead, we terminate the traversal
to enclosing `DeclContext`s immediately if we've already found another
declaration in that context with the same identifier. (This optimization
is not currently applied to the `forallBases` traversal, but could be
applied there too if needed.)
This also fixes two bugs that together caused the check to fail to find
some of the issues it was looking for:
- The old check skipped comparisons of declarations from different
contexts unless both declarations were type template parameters. This
caused the checker to not warn on some instances of the CVE it is
intended to detect.
- The old check skipped comparisons of declarations in all base classes
other than the first one found by the traversal. This appears to be an
oversight, incorrectly returning `false` rather than `true` from the
`forallBases` callback, which terminates traversal.
This also fixes an issue where the check would have false positives for
template parameters and function parameters in some cases, because those
parameters sometimes have a parent `DeclContext` that is the parent of
the parameterized entity, or sometimes is the translation unit. In
either case, this would cause warnings about declarations that are never
visible together in any scope.
This decreases the runtime of this check, especially in the common case
where there are few or no skeletons with two or more different
identifiers. Running this check over LLVM, clang, and clang-tidy, the
wall time for the check as reported by clang-tidy's internal profiler is
reduced from 5202.86s to 3900.90s.
This is final optimization for this check. Main
improvements comes from changing a logic order
in mayShadow function, to first validate result
of mayShadowImpl, then search primary context in
a vectors. Secondary improvement comes from excluding
all implicit code by using TK_IgnoreUnlessSpelledInSource.
All other changes are just cosmetic improvements.
Tested on Cataclysm-DDA open source project, result in
check execution time reduction from 3682 seconds to
100 seconds (~0.25s per TU). That's 97.2% reduction for
this change alone. Resulting in cumulative improvement for
this check around -99.6%, finally bringing this check
into a cheap category.
Reviewed By: serge-sans-paille
Differential Revision: https://reviews.llvm.org/D151594
Main performance issue in this check were caused by many
calls to getPrimaryContext and constant walk up to declaration
contexts using getParent. Also there were issue with forallBases
that is slow.
Profiled with perf and tested on open-source project Cataclysm-DDA.
Before changes check took 27320 seconds, after changes 3682 seconds.
That's 86.5% reduction. More optimizations are still possible in this
check.
Reviewed By: serge-sans-paille
Differential Revision: https://reviews.llvm.org/D151051
We forgot to apply the change to headers in the previous patch,
due to missing "-header-filter" in the run-clang-tidy invocation.
Differential Revision: https://reviews.llvm.org/D142307
