Skip to content

Conversation

krlmlr
Copy link
Collaborator

@krlmlr krlmlr commented Mar 14, 2025

This is one out of three PRs to avoid accessing uninitialized memory. This change was necessary to satisfy CRAN's incoming checks.

CRAN uses gcc 14.2.0 with compile commands similar to the following:

g++  -std=gnu++17 -I"D:/RCompile/recent/R/include" -DNDEBUG -I... -O2 -Wall  -mfpmath=sse -msse2 -mstackrealign    -c duckdb/ub_src_common_types_row.cpp -o duckdb/ub_src_common_types_row.o

I believe they are looking for -Wuninitialized and -Wmaybe-uninitialized .

Would you consider having similar checks as part of the CI/CD pipeline here, perhaps with -Werror ?

@duckdb-draftbot duckdb-draftbot marked this pull request as draft March 14, 2025 07:00
@Mytherin Mytherin marked this pull request as ready for review March 14, 2025 07:51
@carlopi
Copy link
Contributor

carlopi commented Mar 14, 2025

Comment relevant for all 3 PRs: this is handy to have in upcoming 1.2.2 (if so, process is rebase the PR to be against v1.2-histrionicus branch), or this is good for 1.3.0?

Regarding the tests, I would think it makes sense, probably to be added in https://github.com/duckdb/duckdb/blob/main/.github/workflows/NightlyTests.yml, these are run once per day and then failures are flagged to relevant team member.

@krlmlr
Copy link
Collaborator Author

krlmlr commented Mar 14, 2025

Thanks.

  • Happy to rebase and retarget the PRs.

  • Are there other branches to be aware of?

  • Who can help specify the tests? Are you expecting additional input from me?

@krlmlr krlmlr changed the base branch from main to v1.2-histrionicus March 14, 2025 08:28
@carlopi
Copy link
Contributor

carlopi commented Mar 14, 2025

  1. rebase I think just makes sense here, thanks
  2. no, after merge on 1.2-histrionicus branch they will be merged onto main
  3. on testing, I saw that adding cppcoreguidelines-init-variables to clang-tidy check (via changing .clang-tidy file) flags a bunch of other stuff, unsure if that's too noisy. Or better would be set up a test that is as close as possible, basically a copy of https://github.com/duckdb/duckdb/blob/main/.github/workflows/NightlyTests.yml#L205 removing the 32bits details and adding the relevant CMAKE_CXX_FLAGS / CMAKE_C_FLAGS added to cmake invoke, since those are passed down correctly

@carlopi
Copy link
Contributor

carlopi commented Mar 14, 2025

On testing, @Mytherin to make a call on how to move forward. Thanks

@Mytherin Mytherin marked this pull request as draft March 17, 2025 08:56
@Mytherin Mytherin marked this pull request as ready for review March 17, 2025 08:56
@Mytherin Mytherin merged commit d76de45 into duckdb:v1.2-histrionicus Mar 17, 2025
49 checks passed
@Mytherin
Copy link
Collaborator

Thanks!

We do run these warnings - I think the main thing is the compiler version. We could consider running these checks also with a newer compiler.

krlmlr added a commit to duckdb/duckdb-r that referenced this pull request Apr 8, 2025
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