Details
Diff Detail
- Repository
- rCTE Clang Tools Extra
Event Timeline
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. |
clang-tidy/abseil/TimeSubtractionCheck.cpp | ||
---|---|---|
73 | But do we do this in comments? Isn't it's Sphinx syntax? |
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. |
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. |
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. |
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. |
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. |
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 :) |
nit: stringref should be used as value all the time (as its a pointer itself).