Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cppcheck/v2 #11967

Closed
wants to merge 6 commits into from
Closed

Cppcheck/v2 #11967

wants to merge 6 commits into from

Conversation

victorjulien
Copy link
Member

Cppcheck 2.14 fixes plus a few misc improvements.

Replaces #11946, implementing review comments.

This is a more compact time format.
It tripped up cppcheck:

src/util-time.h:124:35: error: syntax error [syntaxError]
    return !timercmp(first, second, >);
Clang 19:

util-hash-string.c:41:16: error: implicit conversion loses integer precision: 'unsigned long' to 'int' [-Werror,-Wshorten-64-to-32]
   41 |     int len1 = strlen((char *)data1);
      |         ~~~~   ^~~~~~~~~~~~~~~~~~~~~
util-hash-string.c:42:16: error: implicit conversion loses integer precision: 'unsigned long' to 'int' [-Werror,-Wshorten-64-to-32]
   42 |     int len2 = strlen((char *)data2);
      |         ~~~~   ^~~~~~~~~~~~~~~~~~~~~
2 errors generated.
Issue warning if thread sleeps for a long time.
@victorjulien victorjulien mentioned this pull request Oct 15, 2024
Copy link

codecov bot commented Oct 15, 2024

Codecov Report

Attention: Patch coverage is 93.93939% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.78%. Comparing base (378f678) to head (48b22b4).
Report is 25 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11967      +/-   ##
==========================================
+ Coverage   82.76%   82.78%   +0.01%     
==========================================
  Files         910      910              
  Lines      249014   249015       +1     
==========================================
+ Hits       206105   206149      +44     
+ Misses      42909    42866      -43     
Flag Coverage Δ
fuzzcorpus 60.77% <33.33%> (+<0.01%) ⬆️
livemode 18.70% <15.15%> (-0.01%) ⬇️
pcap 44.07% <57.57%> (+0.22%) ⬆️
suricata-verify 62.19% <78.78%> (+0.02%) ⬆️
unittests 59.00% <30.30%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 23109

Copy link
Contributor

@jufajardini jufajardini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems way more time-related than cpp :P

Noticed that in one case we're not using SCTIME_INITIALIZER, not sure if relevant.

struct timeval systs;
gettimeofday(&systs, NULL);
t->sys_sec_stamp = (uint32_t)systs.tv_sec;
struct timeval tv;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that in same situations we're still using timeval. What's the criteria, now? Is timeval only for local variables, and for setting or initializing an SCTime_t?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we still need it for gettimeofday

SCMutexUnlock(&thread_store_lock);
}

bool TmThreadsTimeSubsysIsReady(void)
{
static SCTime_t nullts;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SCTIME_INITIALIZER not needed in this case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add it

@victorjulien victorjulien mentioned this pull request Oct 24, 2024
@victorjulien
Copy link
Member Author

replaced by #12027

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants