Clang14 tidy #1048
Clang14 tidy #1048
Conversation
include/aws/common/date_time.h
Outdated
| @@ -10,8 +10,7 @@ | |||
|
|
|||
| AWS_PUSH_SANE_WARNING_LEVEL | |||
|
|
|||
| #define AWS_DATE_TIME_STR_MAX_LEN 100 | |||
| #define AWS_DATE_TIME_STR_MAX_BASIC_LEN 20 | |||
| enum { AWS_DATE_TIME_STR_MAX_LEN = 100, AWS_DATE_TIME_STR_MAX_BASIC_LEN = 20 }; | |||
There was a problem hiding this comment.
nit: both constants on same line look really awkward
source/allocator.c
Outdated
| @@ -291,7 +291,7 @@ static void *s_cf_allocator_reallocate(void *ptr, CFIndex new_size, CFOptionFlag | |||
| aws_mem_realloc(allocator, &original_allocation, original_size, (size_t)new_size); | |||
|
|
|||
| size_t new_allocation_size = (size_t)new_size; | |||
| memcpy(original_allocation, &new_allocation_size, sizeof(size_t)); | |||
| memcpy(original_allocation, &new_allocation_size, sizeof(size_t)); /* NOLINT(clang-analyzer-unix.cstring.NullArg) */ | |||
There was a problem hiding this comment.
is it worth enforcing this case with fatal assert instead of ignoring the warning?
| @@ -58,8 +58,10 @@ enum aws_log_level { | |||
| typedef uint32_t aws_log_subject_t; | |||
|
|
|||
| /* Each library gets space for 2^^10 log subject entries */ | |||
| #define AWS_LOG_SUBJECT_STRIDE_BITS 10 | |||
| #define AWS_LOG_SUBJECT_STRIDE (1U << AWS_LOG_SUBJECT_STRIDE_BITS) | |||
| enum { | |||
There was a problem hiding this comment.
Not a fan of this change. Adjacent integral constant defines is not necessarily something that should be an enum
There was a problem hiding this comment.
is your concern mostly with just grouping of unrelated constants under the same enum or enum usage for constants in general?
include/aws/common/logging.h
Outdated
| #define AWS_LOG_SUBJECT_STRIDE (1U << AWS_LOG_SUBJECT_STRIDE_BITS) | ||
| enum { | ||
| AWS_LOG_SUBJECT_STRIDE_BITS = 10, | ||
| AWS_LOG_SUBJECT_STRIDE = (1U << AWS_LOG_SUBJECT_STRIDE_BITS), |
There was a problem hiding this comment.
This might be changing AWS_LOG_SUBJECT_STRIDE from unsigned to signed.
all enums are int in C
but it looks like the old #define was trying to be unsigned
include/aws/common/statistics.h
Outdated
| #define AWS_CRT_STATISTICS_CATEGORY_STRIDE (1U << AWS_CRT_STATISTICS_CATEGORY_STRIDE_BITS) | ||
| enum { | ||
| AWS_CRT_STATISTICS_CATEGORY_STRIDE_BITS = 8, | ||
| AWS_CRT_STATISTICS_CATEGORY_STRIDE = (1U << AWS_CRT_STATISTICS_CATEGORY_STRIDE_BITS) |
There was a problem hiding this comment.
same thing with signed/unsigned
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## pickup-cjson #1048 +/- ##
================================================
+ Coverage 82.23% 82.25% +0.01%
================================================
Files 52 52
Lines 5641 5641
================================================
+ Hits 4639 4640 +1
+ Misses 1002 1001 -1
☔ View full report in Codecov by Sentry. |
bugprone-easily-swappable-parametersreadability-identifier-length-- variable name needs to be more than 3 chars :)misc-no-recursion-- :)readability-function-cognitive-complexity-- Nice to have, but waay too late, and way to manyreadability-magic-numbers-- Const numbers should be declared as variables, not too bad, but just waay too many in our codebaseBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.