Skip to content

Conversation

@targos
Copy link
Member

@targos targos commented Sep 11, 2025

I'm playing with https://include-what-you-use.org/

With the changes here, it still suggests me to do the following, but it doesn't seem right to me (and to the C++ linter):

(../../src/node_sqlite.h has correct #includes/fwd-decls)

../../src/node_sqlite.cc should add these lines:
#include <_inttypes.h>                   // for PRId64
#include <_string.h>                     // for memcpy
#include <__hash_table>                  // for operator!=
#include "env.h"                         // for Environment
#include "util.h"                        // for Utf8Value, FIXED_ONE_BYTE_ST...

/cc @nodejs/cpp-reviewers

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/sqlite

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Sep 11, 2025
@codecov
Copy link

codecov bot commented Sep 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.25%. Comparing base (f1b56d6) to head (c6aa19c).
⚠️ Report is 85 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #59854      +/-   ##
==========================================
- Coverage   88.27%   88.25%   -0.02%     
==========================================
  Files         702      702              
  Lines      206706   206706              
  Branches    39757    39760       +3     
==========================================
- Hits       182469   182433      -36     
- Misses      16231    16282      +51     
+ Partials     8006     7991      -15     
Files with missing lines Coverage Δ
src/node_sqlite.cc 81.57% <ø> (ø)
src/node_sqlite.h 83.67% <ø> (ø)

... and 37 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@louwers
Copy link
Contributor

louwers commented Sep 11, 2025

The principle is solid, but something like this needs to be enforced on CI, otherwise there is little chance that over time it will stay correct.

@targos
Copy link
Member Author

targos commented Sep 11, 2025

I agree, but we can only enforce it in CI after having fixed the code, and it doesn't hurt to make the changes, even if it drifts a bit over time.

@targos
Copy link
Member Author

targos commented Sep 11, 2025

I just checked and the iwyu package is available on Fedora (on Ubuntu 24.04 too, but it depends on a too old version of Clang), so the possibility to create a CI job with it is open.

@tniessen
Copy link
Member

With the changes here, it still suggests me to do the following, but it doesn't seem right to me (and to the C++ linter):

I played around with IWYU a few years ago when it was still in an experimental state and ran into some issues across different operating systems, but maybe that's been resolved. I assumed at the time that the analysis was inconsistent across platforms because some operating systems distribute headers that act as proxies to internal header files, which technically is spec-compliant in most cases, but makes automated dependency analysis rather complex.

@targos
Copy link
Member Author

targos commented Sep 25, 2025

Doesn't seem to interest reviewers

@targos targos closed this Sep 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants