Skip to content

Commit 886a915

Browse files
dcristoloveanuCopilot
dcristoloveanu
and
Copilot
authored
[MrBot] Reduce TWO_D_ARRAY and PAGED_SPARSE_ARRAY logging noise (#581)
* Reduce TWO_D_ARRAY and PAGED_SPARSE_ARRAY logging noise Change TWO_D_ARRAY_GET_ROW and PAGED_SPARSE_ARRAY_GET to return typed enum results (TWO_D_ARRAY_GET_ROW_RESULT, PAGED_SPARSE_ARRAY_GET_RESULT) with output pointer parameters instead of returning raw pointers. New enum values: OK, INVALID_ARGS, NOT_ALLOCATED This allows callers to distinguish between true errors (invalid args) and expected conditions (element not yet allocated) without noisy LogError messages for the not-allocated case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Use TEST_DEFINE_ENUM_TYPE for GET_ROW_RESULT in int test Replace ASSERT_ARE_EQUAL(int, (int)...) casts with proper ASSERT_ARE_EQUAL(TWO_D_ARRAY_GET_ROW_RESULT, ...) using TEST_DEFINE_ENUM_TYPE for better test output on failure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Merge argument checks into single if statement Combine NULL/bounds checks into one if with a single LogError that logs all arguments, per review feedback. Remove separate SRS_88_035 and SRS_07_020 spec entries (merged into SRS_88_034 and SRS_07_019). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Put each argument check on separate line with Codes tag Per review: split condition checks onto individual lines within the single if statement, with the Codes_SRS tag comment before the checks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Split into separate spec tags per argument check Each condition in the if statement now has its own Codes_SRS tag: - TWO_D_ARRAY: _07_019 (NULL handle), _07_023 (NULL row), _07_020 (bounds) - PAGED_SPARSE_ARRAY: _88_034 (NULL handle), _88_040 (NULL item), _88_035 (bounds) This allows each unit test to be tagged to its specific spec requirement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix duplicate spec tag: rename _88_040 to _88_045 for GET item NULL check The _88_040 tag was already used by PAGED_SPARSE_ARRAY_CREATE overflow check. Renamed the new GET item-is-NULL spec to _88_045 to avoid collision. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Move Codes_SRS comments outside if() in macro definitions Comments inside if() conditions within #define macros with \ line continuations cause VS2022 C2143 syntax errors. Move the spec tag comments before the if statement instead of between conditions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix extra closing brace in paged_sparse_array_int.c Removed duplicate '}' that closed the switch case block prematurely, causing a syntax error at the next macro expansion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Remove accidental preprocessor output file Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Restore inline spec comments above each condition The CI failure was caused by an extra closing brace in the int test, not by inline comments in macros. Move each Codes_SRS comment back directly above the condition it documents. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 9252fe4 commit 886a915

11 files changed

Lines changed: 238 additions & 123 deletions

File tree

‎CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,7 @@ set(c_util_c_files
168168
./src/rc_string_array.c
169169
./src/rc_string_utils.c
170170
./src/sliding_window_average_by_count.c
171+
./src/two_d_array.c
171172
./src/singlylinkedlist.c
172173
./src/strings.c
173174
./src/sync_wrapper.c

‎devdoc/paged_sparse_array_requirements.md‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,10 @@ Elements in the array are **not stored contiguously in memory**. Each page is al
4949

5050
**Important:** Pointer arithmetic does not work with this data structure. For example:
5151
```c
52-
T* elem0 = PAGED_SPARSE_ARRAY_GET(T)(array, 0);
53-
T* elem1 = PAGED_SPARSE_ARRAY_GET(T)(array, 1);
52+
T* elem0;
53+
T* elem1;
54+
PAGED_SPARSE_ARRAY_GET(T)(array, 0, &elem0);
55+
PAGED_SPARSE_ARRAY_GET(T)(array, 1, &elem1);
5456
// elem0 + 1 is NOT guaranteed to equal elem1
5557
```
5658
@@ -110,6 +112,14 @@ typedef void (*PAGED_SPARSE_ARRAY_ITEM_DISPOSE_FUNC(T))(T* item);
110112
PAGED_SPARSE_ARRAY_ALLOCATE_ERROR
111113
112114
MU_DEFINE_ENUM(PAGED_SPARSE_ARRAY_ALLOCATE_RESULT, PAGED_SPARSE_ARRAY_ALLOCATE_RESULT_VALUES)
115+
116+
/*result codes for PAGED_SPARSE_ARRAY_GET*/
117+
#define PAGED_SPARSE_ARRAY_GET_RESULT_VALUES \
118+
PAGED_SPARSE_ARRAY_GET_OK, \
119+
PAGED_SPARSE_ARRAY_GET_INVALID_ARGS, \
120+
PAGED_SPARSE_ARRAY_GET_NOT_ALLOCATED
121+
122+
MU_DEFINE_ENUM(PAGED_SPARSE_ARRAY_GET_RESULT, PAGED_SPARSE_ARRAY_GET_RESULT_VALUES)
113123
```
114124

115125
The macros expand to these useful APIs:
@@ -118,7 +128,7 @@ The macros expand to these useful APIs:
118128
PAGED_SPARSE_ARRAY(T) PAGED_SPARSE_ARRAY_CREATE(T)(uint64_t max_size, uint64_t page_size, PAGED_SPARSE_ARRAY_ITEM_DISPOSE_FUNC(T) item_dispose_func);
119129
PAGED_SPARSE_ARRAY_ALLOCATE_RESULT PAGED_SPARSE_ARRAY_ALLOCATE(T)(PAGED_SPARSE_ARRAY(T) paged_sparse_array, uint64_t index, T** allocated_ptr);
120130
void PAGED_SPARSE_ARRAY_RELEASE(T)(PAGED_SPARSE_ARRAY(T) paged_sparse_array, uint64_t index);
121-
T* PAGED_SPARSE_ARRAY_GET(T)(PAGED_SPARSE_ARRAY(T) paged_sparse_array, uint64_t index);
131+
PAGED_SPARSE_ARRAY_GET_RESULT PAGED_SPARSE_ARRAY_GET(T)(PAGED_SPARSE_ARRAY(T) paged_sparse_array, uint64_t index, T** item);
122132
```
123133
124134
### PAGED_SPARSE_ARRAY(T)
@@ -257,19 +267,21 @@ void PAGED_SPARSE_ARRAY_RELEASE(T)(PAGED_SPARSE_ARRAY(T) paged_sparse_array, uin
257267
### PAGED_SPARSE_ARRAY_GET(T)
258268
259269
```c
260-
T* PAGED_SPARSE_ARRAY_GET(T)(PAGED_SPARSE_ARRAY(T) paged_sparse_array, uint64_t index);
270+
PAGED_SPARSE_ARRAY_GET_RESULT PAGED_SPARSE_ARRAY_GET(T)(PAGED_SPARSE_ARRAY(T) paged_sparse_array, uint64_t index, T** item);
261271
```
262272

263273
`PAGED_SPARSE_ARRAY_GET(T)` gets the element at the specified index.
264274

265-
**SRS_PAGED_SPARSE_ARRAY_88_034: [** If `paged_sparse_array` is `NULL`, `PAGED_SPARSE_ARRAY_GET(T)` shall fail and return `NULL`. **]**
275+
**SRS_PAGED_SPARSE_ARRAY_88_034: [** If `paged_sparse_array` is `NULL`, `PAGED_SPARSE_ARRAY_GET(T)` shall fail and return `PAGED_SPARSE_ARRAY_GET_INVALID_ARGS`. **]**
276+
277+
**SRS_PAGED_SPARSE_ARRAY_88_045: [** If `item` is `NULL`, `PAGED_SPARSE_ARRAY_GET(T)` shall fail and return `PAGED_SPARSE_ARRAY_GET_INVALID_ARGS`. **]**
266278

267-
**SRS_PAGED_SPARSE_ARRAY_88_035: [** If `index` is greater than or equal to `max_size`, `PAGED_SPARSE_ARRAY_GET(T)` shall fail and return `NULL`. **]**
279+
**SRS_PAGED_SPARSE_ARRAY_88_035: [** If `index` is greater than or equal to `max_size`, `PAGED_SPARSE_ARRAY_GET(T)` shall fail and return `PAGED_SPARSE_ARRAY_GET_INVALID_ARGS`. **]**
268280

269281
**SRS_PAGED_SPARSE_ARRAY_88_036: [** `PAGED_SPARSE_ARRAY_GET(T)` shall compute the page index as `index / page_size`. **]**
270282

271-
**SRS_PAGED_SPARSE_ARRAY_88_037: [** If the page is not allocated, `PAGED_SPARSE_ARRAY_GET(T)` shall fail and return `NULL`. **]**
283+
**SRS_PAGED_SPARSE_ARRAY_88_037: [** If the page is not allocated, `PAGED_SPARSE_ARRAY_GET(T)` shall return `PAGED_SPARSE_ARRAY_GET_NOT_ALLOCATED`. **]**
272284

273-
**SRS_PAGED_SPARSE_ARRAY_88_038: [** If the element at `index` is not allocated, `PAGED_SPARSE_ARRAY_GET(T)` shall fail and return `NULL`. **]**
285+
**SRS_PAGED_SPARSE_ARRAY_88_038: [** If the element at `index` is not allocated, `PAGED_SPARSE_ARRAY_GET(T)` shall return `PAGED_SPARSE_ARRAY_GET_NOT_ALLOCATED`. **]**
274286

275-
**SRS_PAGED_SPARSE_ARRAY_88_039: [** `PAGED_SPARSE_ARRAY_GET(T)` shall return a pointer to the element at `index`. **]**
287+
**SRS_PAGED_SPARSE_ARRAY_88_039: [** `PAGED_SPARSE_ARRAY_GET(T)` shall store in `item` a pointer to the element at `index` and return `PAGED_SPARSE_ARRAY_GET_OK`. **]**

‎devdoc/two_d_array_requirements.md‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ The macros expand to these useful somewhat more useful APIs:
3333
TWO_D_ARRAY(T) TWO_D_ARRAY_CREATE(T)(uint32_t row_size, uint32_t col_size);
3434
int TWO_D_ARRAY_FREE_ROW(T)(TWO_D_ARRAY(T) two_d_array, uint32_t row_index);
3535
int TWO_D_ARRAY_ALLOCATE_NEW_ROW(T)(TWO_D_ARRAY(T) two_d_array, uint32_t row_index);
36-
T* TWO_D_ARRAY_GET_ROW(T)(TWO_D_ARRAY(T) two_d_array, uint32_t row_index);
36+
TWO_D_ARRAY_GET_ROW_RESULT TWO_D_ARRAY_GET_ROW(T)(TWO_D_ARRAY(T) two_d_array, uint32_t row_index, T** row);
3737
void TWO_D_ARRAY_FREE(T)(TWO_D_ARRAY(T) two_d_array);
3838

3939
```
@@ -154,15 +154,17 @@ int TWO_D_ARRAY_ALLOCATE_NEW_ROW(T)(TWO_D_ARRAY(T) two_d_array, uint32_t row_ind
154154
### TWO_D_ARRAY_GET_ROW(T)
155155
156156
```c
157-
T* TWO_D_ARRAY_GET_ROW(T)(TWO_D_ARRAY(T) two_d_array, uint32_t row_index);
157+
TWO_D_ARRAY_GET_ROW_RESULT TWO_D_ARRAY_GET_ROW(T)(TWO_D_ARRAY(T) two_d_array, uint32_t row_index, T** row);
158158
```
159159

160-
`TWO_D_ARRAY_GET_ROW(T)` shall return the entire column stored in `row_index`.
160+
`TWO_D_ARRAY_GET_ROW(T)` gets the row at the specified `row_index`.
161161

162-
**SRS_TWO_D_ARRAY_07_019: [** If `two_d_array` is `NULL`, `TWO_D_ARRAY_GET_ROW(T)` shall fail return `NULL`. **]**
162+
**SRS_TWO_D_ARRAY_07_019: [** If `two_d_array` is `NULL`, `TWO_D_ARRAY_GET_ROW(T)` shall fail and return `TWO_D_ARRAY_GET_ROW_INVALID_ARGS`. **]**
163163

164-
**SRS_TWO_D_ARRAY_07_020: [** If `row_index` is equal or greater than `row_size`, `TWO_D_ARRAY_GET_ROW(T)` shall fail return `NULL`. **]**
164+
**SRS_TWO_D_ARRAY_07_023: [** If `row` is `NULL`, `TWO_D_ARRAY_GET_ROW(T)` shall fail and return `TWO_D_ARRAY_GET_ROW_INVALID_ARGS`. **]**
165165

166-
**SRS_TWO_D_ARRAY_07_021: [** If the array stored in `row_index` is `NULL`, `TWO_D_ARRAY_GET_ROW(T)` shall return `NULL`. **]**
166+
**SRS_TWO_D_ARRAY_07_020: [** If `row_index` is equal or greater than `row_size`, `TWO_D_ARRAY_GET_ROW(T)` shall fail and return `TWO_D_ARRAY_GET_ROW_INVALID_ARGS`. **]**
167167

168-
**SRS_TWO_D_ARRAY_07_022: [** Otherwise, `TWO_D_ARRAY_GET_ROW(T)` shall return the entire column stored in the corresponding `row_index`. **]**
168+
**SRS_TWO_D_ARRAY_07_021: [** If the row at `row_index` has not been allocated, `TWO_D_ARRAY_GET_ROW(T)` shall return `TWO_D_ARRAY_GET_ROW_NOT_ALLOCATED`. **]**
169+
170+
**SRS_TWO_D_ARRAY_07_022: [** Otherwise, `TWO_D_ARRAY_GET_ROW(T)` shall store in `row` a pointer to the row at `row_index` and return `TWO_D_ARRAY_GET_ROW_OK`. **]**

‎inc/c_util/paged_sparse_array_ll.h‎

Lines changed: 34 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,14 @@
3131

3232
MU_DEFINE_ENUM(PAGED_SPARSE_ARRAY_ALLOCATE_RESULT, PAGED_SPARSE_ARRAY_ALLOCATE_RESULT_VALUES)
3333

34+
/*result codes for PAGED_SPARSE_ARRAY_GET*/
35+
#define PAGED_SPARSE_ARRAY_GET_RESULT_VALUES \
36+
PAGED_SPARSE_ARRAY_GET_OK, \
37+
PAGED_SPARSE_ARRAY_GET_INVALID_ARGS, \
38+
PAGED_SPARSE_ARRAY_GET_NOT_ALLOCATED
39+
40+
MU_DEFINE_ENUM(PAGED_SPARSE_ARRAY_GET_RESULT, PAGED_SPARSE_ARRAY_GET_RESULT_VALUES)
41+
3442
/*PAGED_SPARSE_ARRAY is backed by a THANDLE build on the structure below*/
3543
#define PAGED_SPARSE_ARRAY_STRUCT_TYPE_NAME_TAG(T) MU_C2(PAGED_SPARSE_ARRAY_TYPEDEF_NAME(T), _TAG)
3644

@@ -109,7 +117,7 @@ struct PAGED_SPARSE_ARRAY_STRUCT_TYPE_NAME_TAG(T)
109117

110118
/*introduces a function declaration for paged_sparse_array_get*/
111119
#define PAGED_SPARSE_ARRAY_LL_GET_DECLARE(C, T) \
112-
MOCKABLE_FUNCTION(, T*, PAGED_SPARSE_ARRAY_LL_GET(C), PAGED_SPARSE_ARRAY_LL(T), paged_sparse_array, uint64_t, index);
120+
MOCKABLE_FUNCTION(, PAGED_SPARSE_ARRAY_GET_RESULT, PAGED_SPARSE_ARRAY_LL_GET(C), PAGED_SPARSE_ARRAY_LL(T), paged_sparse_array, uint64_t, index, T**, item);
113121

114122
/*helper to compute bitmap size in bytes (1 bit per element, rounded up to nearest byte)*/
115123
#define PAGED_SPARSE_ARRAY_BITMAP_SIZE(page_size) (((page_size) + 7) / 8)
@@ -352,48 +360,46 @@ void PAGED_SPARSE_ARRAY_LL_RELEASE(C)(PAGED_SPARSE_ARRAY_LL(T) paged_sparse_arra
352360
}
353361

354362
#define PAGED_SPARSE_ARRAY_LL_GET_DEFINE(C, T) \
355-
T* PAGED_SPARSE_ARRAY_LL_GET(C)(PAGED_SPARSE_ARRAY_LL(T) paged_sparse_array, uint64_t index) \
363+
PAGED_SPARSE_ARRAY_GET_RESULT PAGED_SPARSE_ARRAY_LL_GET(C)(PAGED_SPARSE_ARRAY_LL(T) paged_sparse_array, uint64_t index, T** item) \
356364
{ \
357-
T* result; \
358-
/* Codes_SRS_PAGED_SPARSE_ARRAY_88_034: [ If paged_sparse_array is NULL, PAGED_SPARSE_ARRAY_GET(T) shall fail and return NULL. ]*/ \
359-
if (paged_sparse_array == NULL) \
360-
{ \
361-
LogError("Invalid arguments: PAGED_SPARSE_ARRAY(" MU_TOSTRING(T) ") paged_sparse_array=%p", paged_sparse_array); \
362-
} \
363-
/* Codes_SRS_PAGED_SPARSE_ARRAY_88_035: [ If index is greater than or equal to max_size, PAGED_SPARSE_ARRAY_GET(T) shall fail and return NULL. ]*/ \
364-
else if (index >= paged_sparse_array->max_size) \
365-
{ \
366-
LogError("Invalid arguments: uint64_t index=%" PRIu64 " out of bound, max_size=%" PRIu64 "", index, paged_sparse_array->max_size); \
367-
} \
365+
PAGED_SPARSE_ARRAY_GET_RESULT result; \
366+
if ( \
367+
/* Codes_SRS_PAGED_SPARSE_ARRAY_88_034: [ If paged_sparse_array is NULL, PAGED_SPARSE_ARRAY_GET(T) shall fail and return PAGED_SPARSE_ARRAY_GET_INVALID_ARGS. ]*/ \
368+
paged_sparse_array == NULL || \
369+
/* Codes_SRS_PAGED_SPARSE_ARRAY_88_045: [ If item is NULL, PAGED_SPARSE_ARRAY_GET(T) shall fail and return PAGED_SPARSE_ARRAY_GET_INVALID_ARGS. ]*/ \
370+
item == NULL || \
371+
/* Codes_SRS_PAGED_SPARSE_ARRAY_88_035: [ If index is greater than or equal to max_size, PAGED_SPARSE_ARRAY_GET(T) shall fail and return PAGED_SPARSE_ARRAY_GET_INVALID_ARGS. ]*/ \
372+
index >= paged_sparse_array->max_size) \
373+
{ \
374+
LogError("Invalid arguments: PAGED_SPARSE_ARRAY(" MU_TOSTRING(T) ") paged_sparse_array=%p, item=%p, uint64_t index=%" PRIu64, paged_sparse_array, item, index); \
375+
result = PAGED_SPARSE_ARRAY_GET_INVALID_ARGS; \
376+
} \
368377
else \
369378
{ \
370379
/* Codes_SRS_PAGED_SPARSE_ARRAY_88_036: [ PAGED_SPARSE_ARRAY_GET(T) shall compute the page index as index / page_size. ]*/ \
371-
uint64_t page_index = index / paged_sparse_array->page_size; \
372-
\
373-
/* Codes_SRS_PAGED_SPARSE_ARRAY_88_037: [ If the page is not allocated, PAGED_SPARSE_ARRAY_GET(T) shall fail and return NULL. ]*/ \
374-
if (paged_sparse_array->pages[page_index] == NULL) \
380+
uint64_t page_index = index / paged_sparse_array->page_size; \
381+
\
382+
/* Codes_SRS_PAGED_SPARSE_ARRAY_88_037: [ If the page is not allocated, PAGED_SPARSE_ARRAY_GET(T) shall return PAGED_SPARSE_ARRAY_GET_NOT_ALLOCATED. ]*/ \
383+
if (paged_sparse_array->pages[page_index] == NULL) \
375384
{ \
376-
LogError("Page at page_index=%" PRIu64 " is not allocated", page_index); \
385+
result = PAGED_SPARSE_ARRAY_GET_NOT_ALLOCATED; \
377386
} \
378387
else \
379388
{ \
380-
uint64_t index_in_page = index % paged_sparse_array->page_size; \
381-
/* Codes_SRS_PAGED_SPARSE_ARRAY_88_038: [ If the element at index is not allocated, PAGED_SPARSE_ARRAY_GET(T) shall fail and return NULL. ]*/ \
382-
if (!PAGED_SPARSE_ARRAY_IS_ALLOCATED(PAGED_SPARSE_ARRAY_GET_BITMAP(paged_sparse_array->pages[page_index], paged_sparse_array->page_size), index_in_page)) \
389+
uint64_t index_in_page = index % paged_sparse_array->page_size; \
390+
/* Codes_SRS_PAGED_SPARSE_ARRAY_88_038: [ If the element at index is not allocated, PAGED_SPARSE_ARRAY_GET(T) shall return PAGED_SPARSE_ARRAY_GET_NOT_ALLOCATED. ]*/ \
391+
if (!PAGED_SPARSE_ARRAY_IS_ALLOCATED(PAGED_SPARSE_ARRAY_GET_BITMAP(paged_sparse_array->pages[page_index], paged_sparse_array->page_size), index_in_page)) \
383392
{ \
384-
LogError("Element at index=%" PRIu64 " is not allocated", index); \
393+
result = PAGED_SPARSE_ARRAY_GET_NOT_ALLOCATED; \
385394
} \
386395
else \
387396
{ \
388-
/* Codes_SRS_PAGED_SPARSE_ARRAY_88_039: [ PAGED_SPARSE_ARRAY_GET(T) shall return a pointer to the element at index. ]*/ \
389-
result = &paged_sparse_array->pages[page_index]->items[index_in_page]; \
390-
goto all_ok; \
397+
/* Codes_SRS_PAGED_SPARSE_ARRAY_88_039: [ PAGED_SPARSE_ARRAY_GET(T) shall store in item a pointer to the element at index and return PAGED_SPARSE_ARRAY_GET_OK. ]*/ \
398+
*item = &paged_sparse_array->pages[page_index]->items[index_in_page]; \
399+
result = PAGED_SPARSE_ARRAY_GET_OK; \
391400
} \
392401
} \
393402
} \
394-
result = NULL; \
395-
all_ok: \
396403
return result; \
397404
}
398-
399405
#endif /*PAGED_SPARSE_ARRAY_LL_H*/

0 commit comments

Comments
 (0)