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).