Page MenuHomePhabricator

[clang-tidy] readability-ternary-operator new check
AbandonedPublic

Authored by omtcyf0 on Feb 13 2016, 12:36 PM.

Details

Reviewers
None
Summary

This patch introduces a proof-of-concept check for clang-tidy aiming to suggest using ternary operator in order to improve readability.

It's currently really straightforward and "clever" heuristics for auto fixing more sophisticated cases would be added later.

Diff Detail

Event Timeline

omtcyf0 updated this revision to Diff 47918.Feb 13 2016, 12:36 PM
omtcyf0 retitled this revision from to [clang-tidy] readability-ternary-operator new check.
omtcyf0 updated this object.
omtcyf0 added a subscriber: cfe-commits.
docs/clang-tidy/checks/readability-ternary-operator.rst
11

Why add the parentheses around the entire statement? They seem to serve no purpose.

What happens if the condition is an expression with the same or lower precedence than the ternary operator?

Most people seem to agree that (a || b) ? e1 : e2; is more readable than (a || b ? e1 : e2);

14

readability-simplify-boolean-expr does this when the return type is bool and the return statements are returning true and false.

It also handles conditional assignment via ternary operator when the value assigned is of type bool and the resulting expressions are boolean constants.

If this check also starts modifying the same ternary operators, then we can have two checks fighting to make changes if both checks are applied on the same run of clang-tidy. I'm unsure how best to resolve the conflict. I don't know what clang-tidy would do in such instances.

19

Are you also checking for intermediate preprocessor lines?

if (cond) {
#define BLAH 1
  return BLAH;
} else {
  return 0;
}
25

A slightly better wording:

If you want the check to perform an autofix, either reposition or remove the comments.

test/clang-tidy/readability-ternary-operator.cpp
4

You don't have any test cases where the condition is an expression with operators. Please add some tests for this, particularly operators of the same or lower precedence than the ternary operator.

omtcyf0 updated this revision to Diff 48231.Feb 17 2016, 12:55 PM

Followed some suggestions Richard has kindly given me; fixed the nonsense in SourceRanges' extracting code

alexfh added inline comments.Feb 23 2016, 6:12 AM
clang-tidy/readability/BracesAroundStatementsCheck.cpp
16

This file only needs one function from this namespace, so it's better to qualify the function name instead.

clang-tidy/readability/TernaryOperatorCheck.cpp
31

No need for braces around single-line if/for/... bodies. Here and elsewhere.

36

Leftover from debugging?

47

You could skip whole tokens here instead of one character at a time.

125

When you don't need the casted pointer, use isa<T> instead of dyn_cast<T>.

clang-tidy/readability/TernaryOperatorCheck.h
52

The function name is too vague. It basically says what type the function returns without saying what exactly it is.

docs/clang-tidy/checks/readability-ternary-operator.rst
10

I don't think this particular replacement makes the code any better. A ternary operator in the void context seems rather unnatural.

What does make sense, is pulling some common part from both branches of the if, when there is a common part, e.g.:

if (c) a = x; else a = y; // => a = c ? x : y;
if (c) return x; else return y; // => return c ? x : y;
if (c) f(x); else f(y); // => f(c ? x : y);
if (c) {
  SourceRange Range(Foo.getLocStart(), Bar.getLocEnd());
  diag << FixItHint::CreateReplacement(Range, "qqq");
} else {
  SourceRange Range(Foo.getLocStart(), Bar.getLocEnd());
  diag << FixItHint::CreateReplacement(Range, "eee");
}
// => 
// SourceRange Range(Foo.getLocStart(), Bar.getLocEnd());
// diag << FixItHint::CreateReplacement(Range, c ? "qqq" : "eee");

etc.

alexfh requested changes to this revision.Feb 24 2016, 5:45 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Feb 24 2016, 5:45 AM
omtcyf0 marked 3 inline comments as done.Feb 24 2016, 5:55 AM

Thanks for the review!

I'll fix these issues as soon as I manage to get time for that!

omtcyf0 abandoned this revision.Aug 10 2016, 2:20 AM
omtcyf0 removed a subscriber: cfe-commits.