Page MenuHomePhabricator

[clang-tidy] performances-unnecessary-* checks: Extend isOnlyUsedAsConst to expressions and catch const methods returning non-const references/pointers.
Needs ReviewPublic

Authored by flx on Tue, May 25, 7:34 AM.

Details

Summary

This change extends isOnlyUsedAsConst() to avoid false positives in cases where
a const method returns a mutable pointer or refernce and the pointed to object
is modified.

To achieve this we look at each const method or operator call and check if it
returns a mutable pointer/reference. If that's the case the call expression
itself is checked for constant use.

We also capture assignments of expressions to non-const references/pointers and
then check that the declared alias variables are used in a const-only fasion as
well.

Diff Detail

Event Timeline

flx created this revision.Tue, May 25, 7:34 AM
flx requested review of this revision.Tue, May 25, 7:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, May 25, 7:34 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I have some concerns about the cost of this checks as it used matching over entire contexts quite extensively. At this point, the facilities involved seem quite close to doing dataflow analysis and I wonder if you might be better off with a very different implementation. Regardless, have you done any perfomance testing to see the impact on real code?

clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h
30

please expand this comment. It's not obvious (to me) what it means for a variable to be used in a "const compatible fashion".

flx added a comment.Fri, Jun 11, 11:21 AM

I have some concerns about the cost of this checks as it used matching over entire contexts quite extensively. At this point, the facilities involved seem quite close to doing dataflow analysis and I wonder if you might be better off with a very different implementation. Regardless, have you done any perfomance testing to see the impact on real code?

That's a fair point. Is there prior art in terms of dataflow analysis in ClangTidy or LLVM I could take a look at?

In terms of measuring performance, do you have suggestions how to measure this? I can add a counter that counts the recursion depth that is reached to see how often this happens in practice.

Another idea is to not count const methods from returning mutable pointer or reference types as const access. Standard types std::vector and absl::StatusOr would not be affected by this restriction, their const accessors return const references as well.

I'll hold off on this change until I see more false positives.

I have some concerns about the cost of this checks as it used matching over entire contexts quite extensively. At this point, the facilities involved seem quite close to doing dataflow analysis and I wonder if you might be better off with a very different implementation. Regardless, have you done any perfomance testing to see the impact on real code?

That's a fair point. Is there prior art in terms of dataflow analysis in ClangTidy or LLVM I could take a look at?

Added Dmitri to speak to prior art.

In terms of measuring performance, do you have suggestions how to measure this? I can add a counter that counts the recursion depth that is reached to see how often this happens in practice.

I would simply run clang-tidy over a reasonable size set of files and see the timing w/ and w/o this change. But, I think that clang-tidy may have some built-in perf monitoring as well (specifically, a way to display the cost of each check, or the top most costly ones).