Page MenuHomePhabricator

[clang-tidy] Add the abseil-time-subtraction check
ClosedPublic

Authored by hwright on Feb 12 2019, 10:26 AM.

Diff Detail

Event Timeline

hwright created this revision.Feb 12 2019, 10:26 AM
Eugene.Zelenko added inline comments.
docs/ReleaseNotes.rst
91

Please use two not one ` for language constructs.

docs/clang-tidy/checks/abseil-time-subtraction.rst
12

Will be good idea to always use absl:: prefix. Same in other places.

26

` doesn't make sense in comments. Same below.

hwright updated this revision to Diff 186569.Feb 12 2019, 5:20 PM
hwright marked 3 inline comments as done.
JonasToth added inline comments.Feb 16 2019, 8:09 AM
clang-tidy/abseil/DurationRewriter.cpp
92

nit: stringref should be used as value all the time (as its a pointer itself).

clang-tidy/abseil/TimeSubtractionCheck.cpp
66

wouldn't that be rather a bug condition? I think assert is better in that case

73

please highlight the types with ''

98

Could you please split this function up into smaller ones. There are three or four distinct cases that are easier to comprehend in isolation.

99

nit: InverseName

109

please elimate the multiple blanks

clang-tidy/abseil/TimeSubtractionCheck.h
20

nit: 'Time' domain

test/clang-tidy/abseil-time-subtraction.cpp
13

please add tests where x itself is a calculation with different precedence of its operators (multiplication, addition) to ensure these cases are transformed properly as well.

79

please add tests for templates and macros.

Eugene.Zelenko added inline comments.Feb 16 2019, 8:46 AM
clang-tidy/abseil/TimeSubtractionCheck.cpp
73

But do we do this in comments? Isn't it's Sphinx syntax?

JonasToth added inline comments.Feb 16 2019, 10:08 AM
clang-tidy/abseil/TimeSubtractionCheck.cpp
73

We don't do it consistently, but its common to highlight direct usage of types in comments as well.
Sphinx uses backticks, but in comments any form of ticks are ok.

hwright updated this revision to Diff 187967.Feb 22 2019, 12:23 PM
hwright marked 15 inline comments as done.
hwright added inline comments.
clang-tidy/abseil/TimeSubtractionCheck.cpp
98

The actual bodies of these if-statements are only one or two separate statements themselves. Moving those to separate functions seems like it would just obfuscate things a bit.

clang-tidy/abseil/TimeSubtractionCheck.h
20

This doesn't refer to a type, but a library system, so it probably isn't appropriate to quote it.

(Just has how one wouldn't quote "frequency" when talking about "the frequency domain" of a Fourier transform.)

test/clang-tidy/abseil-time-subtraction.cpp
13

This doesn't actually matter in this case: x will be wrapped in a function call.

It does matter in the case where we unwrap the first argument (below) and I've already got a test which uses multiplication in this case. I've also added one for division.

79

I've add tests for macros, though I'm not sure what cases you have in mind regarding templates.

JonasToth added inline comments.Feb 23 2019, 6:34 AM
clang-tidy/abseil/TimeSubtractionCheck.cpp
98

IMHO they are complicated statements and hide what is being done. Wrapping them in a function with a name that states what is done seems appropriate.

clang-tidy/abseil/TimeSubtractionCheck.h
20

ah true, but then time would be written small i guess.

test/clang-tidy/abseil-time-subtraction.cpp
13

Yes, it should not matter if x is an expr itself or just a variable. Thats why it should be tested its actually true.

hwright updated this revision to Diff 188289.Feb 25 2019, 6:15 PM
hwright marked 5 inline comments as done.
hwright added inline comments.
clang-tidy/abseil/TimeSubtractionCheck.cpp
98

I would agree that they are complicated statements, which is why there are multi-line comments explaining what is being doing. Moving a two-line compound statement into a separate function which is only called once seems more confusing than simplifying. More information can be expressed in a prose comment than a single concise function name, no?

test/clang-tidy/abseil-time-subtraction.cpp
13

Added, though this seems more a test of the matcher infrastructure than the tool itself.

hwright updated this revision to Diff 188372.Feb 26 2019, 7:13 AM
hwright marked an inline comment as done.
hwright added inline comments.
clang-tidy/abseil/TimeSubtractionCheck.cpp
98

I've pulled out the common duplicated functionality which actually emits the diagnostic, and I think that's simplified these branches a bit more.

JonasToth accepted this revision.Feb 27 2019, 4:38 AM

I am happy now :)
Thank you for the patch, LGTM

test/clang-tidy/abseil-time-subtraction.cpp
13

True, but there are always some inconsistencies, so better to test what we want :)

This revision is now accepted and ready to land.Feb 27 2019, 4:38 AM
This revision was automatically updated to reflect the committed changes.