This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Improve misc-redundant-expression and decrease false-positive
ClosedPublic

Authored by etienneb on Apr 28 2016, 5:18 PM.

Details

Summary

This patch is adding support for conditional expression and overloaded operators.

To decrease false-positive, this patch is adding a list of banned macro names that
has multiple variant with same integer value.

Also fixed support for template instantiation and added an unittest.

Diff Detail

Event Timeline

etienneb updated this revision to Diff 55513.Apr 28 2016, 5:18 PM
etienneb retitled this revision from to [clang-tidy] Improve misc-redundant-expression and decrease false-positive.
etienneb updated this object.
etienneb added a reviewer: alexfh.
etienneb added a subscriber: cfe-commits.
etienneb updated this revision to Diff 55526.Apr 28 2016, 9:00 PM

adding unittests

Sarcasm added a subscriber: Sarcasm.May 2 2016, 2:51 PM
alexfh requested changes to this revision.May 11 2016, 1:30 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/misc/RedundantExpressionCheck.cpp
28

Now that you have pulled an API for this, I'll wait until you update this patch.

This revision now requires changes to proceed.May 11 2016, 1:30 AM
etienneb updated this revision to Diff 56911.May 11 2016, 7:26 AM
etienneb edited edge metadata.
etienneb marked an inline comment as done.

use new API

etienneb updated this revision to Diff 56912.May 11 2016, 7:27 AM
etienneb edited edge metadata.

fix bad upload

alexfh accepted this revision.May 11 2016, 9:23 AM
alexfh edited edge metadata.

LG.

clang-tidy/misc/RedundantExpressionCheck.cpp
140

Is there a way to not create a set on each call to the matcher? There also might be a more suitable container in llvm/ADT.

189

We should add a hasAnyOverloadedOperatorName matcher at some point.

This revision is now accepted and ready to land.May 11 2016, 9:23 AM

We should add a hasAnyOverloadedOperatorName matcher at some point.

I proposed it to sbenza@ and he told me that the reason he did HasAnyName was for speed issue.
I can't tell if that one has speed issue too.

But, otherwise, I agree... for code readability... we should do it.

etienneb updated this revision to Diff 56935.May 11 2016, 10:30 AM
etienneb marked an inline comment as done.
etienneb edited edge metadata.

remove useless set creation

etienneb updated this revision to Diff 56937.May 11 2016, 10:32 AM

fix naming nits

+ Removed copy of a vector to a set
+ Fixed some minor coding-style issues

clang-tidy/misc/RedundantExpressionCheck.cpp
140

Some ideas:

  1. We may assume the vector is small and do the lookup.
  2. We may assume a sorted vector and do a binary search.
  3. The matcher could receive a set instead of a vector and lifting out the copy.

Implemented 3)

etienneb closed this revision.May 11 2016, 9:38 PM
etienneb marked 2 inline comments as done.
alexfh added inline comments.May 12 2016, 6:32 AM
clang-tidy/misc/RedundantExpressionCheck.cpp
140

Ah, found it: llvm/ADT/StringSet.h seems to be a better choice here.