Skip to content

Address several warnings emitted during compilation - #41

Merged
simondsmart merged 1 commit into
developfrom
fix/several-warnings
Sep 26, 2025
Merged

Address several warnings emitted during compilation#41
simondsmart merged 1 commit into
developfrom
fix/several-warnings

Conversation

@Ozaq

@Ozaq Ozaq commented May 7, 2025

Copy link
Copy Markdown
Contributor

@Ozaq
Ozaq requested a review from simondsmart May 7, 2025 15:40
Comment thread src/odc/tools/TestInt16_MissingCodec.cc Outdated
@codecov-commenter

codecov-commenter commented May 7, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.94%. Comparing base (faa1805) to head (cf4e20f).
⚠️ Report is 4 commits behind head on develop.

Files with missing lines Patch % Lines
src/odc/ODBAPISettings.cc 0.00% 2 Missing ⚠️
src/odc/WriterDispatchingIterator.cc 75.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #41      +/-   ##
===========================================
+ Coverage    86.92%   86.94%   +0.01%     
===========================================
  Files          188      188              
  Lines        15116    15116              
  Branches      1366     1363       -3     
===========================================
+ Hits         13140    13142       +2     
+ Misses        1976     1974       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread tests/c_api/encode.cc Outdated
long icol[nrows] = {1111, 2222, 3333, 4444, 5555, 6666, 7777, 8888, 9999, 0};
long bcol[nrows] = {1101, 2202, 3303, 4404, 5505, 6606, 7707, 8808, 9909, 0};
char scol[nrows][3 * sizeof(double)] = {0};
char scol[nrows][3 * sizeof(double)]{};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These are not the same.

= {} should zero-initialise the memory. Please check if that makes a difference in this case. And if so, you need to provide a mechanism to do the zeroing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They behave identical with c++11 and later.

Both are aggregate initialization. = {0} is actually ={{0}} due to brace elision and initializes scol[0][0] to 0. Then copy list initialization is used for the remaining elements, which are value initialized.

char[3][4]{} is also an aggregate initialization with an empty init-list, so the same rule as above is applied for all elements:

Otherwise, if the initializer list has no elements, the object is value-initialized.

See also:
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/n4659.pdf
11.6.1 / 11-12 pag2 226/227
11.6.4 / 3.10 page 234

Comment thread src/odc/ODBAPISettings.cc
#include "eckit/log/Log.h"
#include "eckit/thread/ThreadSingleton.h"
#include "eckit/utils/StringTools.h"
#include "eckit/utils/Literals.h"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This definitely requires a bump to the minimum version of eckit, in the CMakeLists.txt.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah yes good catch!

@Ozaq Ozaq added the approved-for-ci Approved for CI run label May 7, 2025
@Ozaq
Ozaq force-pushed the fix/several-warnings branch 2 times, most recently from 4938249 to 631b7ff Compare May 8, 2025 09:51
@Ozaq
Ozaq force-pushed the fix/several-warnings branch from 631b7ff to cf4e20f Compare September 26, 2025 12:57
@Ozaq Ozaq removed the approved-for-ci Approved for CI run label Sep 26, 2025
@Ozaq
Ozaq requested a review from simondsmart September 26, 2025 12:57
Comment thread src/odc/core/Table.cc
std::vector<size_t> lastDecoded(ncols, 0);
double decodeBuffer[maxDoublesDecode];

std::vector<double> decodeBufferVec(maxDoublesDecode);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this really a change we want to make. This shifts having the decode buffer from being on the stack to in the heap.


long missing_integers[nrows];
cycle_longs(missing_integers, nrows, integer_pool, integer_pool_size);
std::vector<long> missing_integers(nrows);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No particular objection to this, but is there a reason to make this change?

@simondsmart
simondsmart merged commit e418c2c into develop Sep 26, 2025
195 of 197 checks passed
@simondsmart
simondsmart deleted the fix/several-warnings branch September 26, 2025 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants