[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.


================
Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:33-36
+              anyOf(hasAncestor(
+                        functionTemplateDecl(HasMatchingDependentDescendant)),
+                    hasAncestor(
+                        classTemplateDecl(HasMatchingDependentDescendant))))
----------------
astrelni wrote:
> alexfh wrote:
> > The hasAncestor and hasDescendant matchers frequently have non-trivial performance implications. Especially the latter, especially when called on a large number of nodes with a large number of transitive children (this obviously depends on the code being analyzed). I don't know how likely is this to cause problems here (the matcher seems to only be used when the check is about to issue a diagnostic), but it's always good to be aware of the possible issues.
> > 
> > Frequently there's a more efficient (and easier to understand) alternative. For example, instead of trying to evaluate certain conditions while traversing the code (which may require a large number of lookups into relatively distant parts of the AST), it's sometimes more efficient to split the analysis into two or more stages. During the AST traversal (i.e. when AST matchers run) the check could collect some raw information about all potentially problematic places in the code and then (for example, in `onEndOfTranslationUnit`) analyze the collected information together in a second stage. This works best if there's a way to arrange the data gathered on the first pass such that the second pass can efficiently look up necessary information.
> Thanks, gave it a try, what do you think of this?
>From a cursory look this should be less likely to be sloooow. In any case, makes sense to profile the check on a number of large files (at least against other checks, using clang-tidy's -enable-check-profile option).


================
Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:43
+              unless(hasTemplateArgument(0, refersToType(builtinType()))),
+              anyOf(hasName("operator*="), hasName("operator/="))))),
+      this);
----------------
hasAnyName, please. It's more efficient. A few more instances below.


https://reviews.llvm.org/D53830