lib/datetime: Reduce cognitive complexity of core datetime functions#7598
Draft
NeelGhoshal wants to merge 11 commits into
Draft
lib/datetime: Reduce cognitive complexity of core datetime functions#7598NeelGhoshal wants to merge 11 commits into
NeelGhoshal wants to merge 11 commits into
Conversation
…exity from 68 to the 25 allowed.
…|while|switch statements
…|while|switch statements
…exity from 41 to the 25 allowed
…exity from 98 to the 25 allowed
…exity from 101 to the 25 allowed
…xity from 47 to the 25 allowed
…|while|switch statements
…|while|switch statements
…exity from 56 to the 25 allowed
…exity from 35 to the 25 allowed
Member
|
Could you at least compile locally first? It doesn’t compile on any platform and configuration yet |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| static void make_incr(DateTime *, int, int, DateTime *); | ||
|
|
||
| /* Handles the "round == 0" branch for (to < dt->to): increment using all lost values */ |
Contributor
There was a problem hiding this comment.
[pre-commit] reported by reviewdog 🐶
Suggested change
| /* Handles the "round == 0" branch for (to < dt->to): increment using all lost values */ | |
| /* Handles the "round == 0" branch for (to < dt->to): increment using all lost | |
| * values */ |
| break; | ||
| } | ||
| } | ||
| /* Returns 1 if any “lost” component between dt->to down to (to+1) is non-default */ |
Contributor
There was a problem hiding this comment.
[pre-commit] reported by reviewdog 🐶
Suggested change
| /* Returns 1 if any “lost” component between dt->to down to (to+1) is non-default */ | |
| /* Returns 1 if any “lost” component between dt->to down to (to+1) is | |
| * non-default */ |
Comment on lines
+161
to
+186
| static void round_lost_precision(DateTime *dt, int to, int round) | ||
| { | ||
| DateTime incr; | ||
| int pos, carry, ndays; | ||
|
|
||
| if (to >= dt->to) | ||
| return; | ||
|
|
||
| if (round > 0) { | ||
| int x = datetime_is_absolute(dt) ? 1 : 0; | ||
|
|
||
| carry = has_lost_non_default(dt, to, x); | ||
| } | ||
|
|
||
| if (carry) { | ||
| make_incr(&incr, to, to, dt); | ||
| incr.year = 1; incr.month = 1; incr.day = 1; | ||
| incr.hour = 1; incr.minute = 1; incr.second = 1.0; | ||
| datetime_increment(dt, &incr); | ||
| } | ||
| } | ||
|
|
||
| if (round == 0) { | ||
| apply_round0_lost(dt, to, &incr); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
[pre-commit] reported by reviewdog 🐶
Suggested change
| static void round_lost_precision(DateTime *dt, int to, int round) | |
| { | |
| DateTime incr; | |
| int pos, carry, ndays; | |
| if (to >= dt->to) | |
| return; | |
| if (round > 0) { | |
| int x = datetime_is_absolute(dt) ? 1 : 0; | |
| carry = has_lost_non_default(dt, to, x); | |
| } | |
| if (carry) { | |
| make_incr(&incr, to, to, dt); | |
| incr.year = 1; incr.month = 1; incr.day = 1; | |
| incr.hour = 1; incr.minute = 1; incr.second = 1.0; | |
| datetime_increment(dt, &incr); | |
| } | |
| } | |
| if (round == 0) { | |
| apply_round0_lost(dt, to, &incr); | |
| } | |
| } | |
| static void round_lost_precision(DateTime * dt, int to, int round) | |
| { | |
| DateTime incr; | |
| int pos, carry, ndays; |
| } | ||
| } | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
[pre-commit] reported by reviewdog 🐶
Suggested change
| if (to >= dt->to) | |
| return; | |
| } | ||
|
|
||
|
|
||
| round_lost_precision(dt, to, round); |
Contributor
There was a problem hiding this comment.
[pre-commit] reported by reviewdog 🐶
Suggested change
| round_lost_precision(dt, to, round); | |
| if (round > 0) { | |
| int x = datetime_is_absolute(dt) ? 1 : 0; |
Comment on lines
+27
to
+29
| static int apply_relative_fields(DateTime *dt, int from, int to, | ||
| int year, int month, int day, | ||
| int hour, int minute, double second); |
Contributor
There was a problem hiding this comment.
[pre-commit] reported by reviewdog 🐶
Suggested change
| static int apply_relative_fields(DateTime *dt, int from, int to, | |
| int year, int month, int day, | |
| int hour, int minute, double second); | |
| static int apply_relative_fields(DateTime *dt, int from, int to, int year, | |
| int month, int day, int hour, int minute, | |
| double second); |
Comment on lines
+91
to
+92
| if (datetime_set_type(dt, DATETIME_ABSOLUTE, | ||
| DATETIME_YEAR, a.to, a.fracsec)) |
Contributor
There was a problem hiding this comment.
[pre-commit] reported by reviewdog 🐶
Suggested change
| if (datetime_set_type(dt, DATETIME_ABSOLUTE, | |
| DATETIME_YEAR, a.to, a.fracsec)) | |
| if (datetime_set_type(dt, DATETIME_ABSOLUTE, DATETIME_YEAR, a.to, | |
| a.fracsec)) |
| return 0; | ||
| if (datetime_set_type(dt, DATETIME_RELATIVE, from, to, fracsec)) | ||
| return 0; | ||
| if (!apply_relative_fields(dt, from, to, year, month, day, hour, minute, second)) |
Contributor
There was a problem hiding this comment.
[pre-commit] reported by reviewdog 🐶
Suggested change
| if (!apply_relative_fields(dt, from, to, year, month, day, hour, minute, second)) | |
| if (!apply_relative_fields(dt, from, to, year, month, day, hour, minute, | |
| second)) |
Comment on lines
+167
to
+169
| static int apply_relative_fields(DateTime *dt, int from, int to, | ||
| int year, int month, int day, | ||
| int hour, int minute, double second) |
Contributor
There was a problem hiding this comment.
[pre-commit] reported by reviewdog 🐶
Suggested change
| static int apply_relative_fields(DateTime *dt, int from, int to, | |
| int year, int month, int day, | |
| int hour, int minute, double second) | |
| static int apply_relative_fields(DateTime *dt, int from, int to, int year, | |
| int month, int day, int hour, int minute, | |
| double second) |
Comment on lines
+204
to
+205
|
|
||
|
|
Contributor
There was a problem hiding this comment.
[pre-commit] reported by reviewdog 🐶
Suggested change
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Refactor five files in
lib/datetimeto bring all functions within thecognitive complexity limit of 25 (previously flagged by static analysis).
change.cdatetime_change_from_*diff.cdatetime_diffformat.cdatetime_formatincr1.cdatetime_increment_*scan.cdatetime_scanRefactoring techniques used: early returns to reduce nesting, extraction
of repeated conditional blocks, and elimination of redundant else-after-return
patterns.
Motivation and context
High cognitive complexity makes functions hard to read, test, and maintain.
These functions in
lib/datetimehad among the highest complexity scores inthe GRASS codebase. All changes are behaviour-preserving refactors — no
logic has been altered.
How has this been tested?
lib/datetimehas no dedicated C unit test suite. These are structuralrefactors with no logic changes; correctness depends on careful review.
Reviewers are encouraged to diff the original and refactored versions
function by function.
Note: adding tests to
lib/datetimewould be a valuable follow-up.Types of changes
This is a refactoring/maintainability change only.
Checklist