Skip to content

Change it so that the header files are directly included#1865

Open
AustinBenoit wants to merge 4 commits into
mainfrom
direct-include
Open

Change it so that the header files are directly included#1865
AustinBenoit wants to merge 4 commits into
mainfrom
direct-include

Conversation

@AustinBenoit
Copy link
Copy Markdown
Contributor

Description

Provide details of the change, and generalize the change in the PR title above.

Change it so that the header files are directly included


Testing

Describe how you've tested these changes. Link any manually triggered Integration tests or CPP binary SDK Packaging Github Action workflows, if applicable.

Run all integration tests in github


Type of Change

Place an x the applicable box:

  • Bug fix. Add the issue # below if applicable.
  • New feature. A non-breaking change which adds functionality.
  • Other, such as a build process or documentation change.

Notes

  • Bug fixes and feature changes require an update to the Release Notes section of release_build_files/readme.md.
  • Read the contribution guidelines CONTRIBUTING.md.
  • Changes to the public API require an internal API review. If you'd like to help us make Firebase APIs better, please propose your change in a feature request so that we can discuss it together.

Remove the cmake injection and just include the header files as needed
@AustinBenoit AustinBenoit added the skip-release-notes Skip release notes check label Jun 3, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request removes the automatic global inclusion of headers like assert.h, string.h, and stdint.h from CMakeLists.txt files across various modules, replacing them with explicit #include statements in the individual source and header files. The reviewer feedback recommends two main improvements: first, reordering includes so that companion headers are always included first (consistent with the Google C++ Style Guide); second, preferring C++ standard library headers (such as , , and ) over their C-style counterparts (like <assert.h>, <stdint.h>, and <string.h>) to ensure consistency and proper namespace scoping.

Comment thread app/rest/controller_curl.cc Outdated
Comment thread app/src/app_android.cc Outdated
Comment thread database/src/common/data_snapshot.cc Outdated
Comment thread firestore/src/android/aggregate_query_android.cc Outdated
Comment thread functions/src/android/callable_reference_android.cc Outdated
Comment thread app/rest/response_json.h Outdated
Comment thread app/src/util_android.h Outdated
Comment thread firestore/src/main/field_value_main.h Outdated
Comment thread storage/src/include/firebase/storage/metadata.h
@AustinBenoit AustinBenoit force-pushed the direct-include branch 2 times, most recently from 0461c0e to d644d53 Compare June 3, 2026 19:50
@AustinBenoit
Copy link
Copy Markdown
Contributor Author

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request removes the automatic inclusion of standard headers (such as assert.h, string.h, and stdint.h) from CMakeLists.txt files across various Firebase components, replacing them with explicit #include directives in the individual source and header files where they are needed. The reviewer's feedback consistently suggests replacing C-style header inclusions (such as <assert.h>, <stdint.h>, and <string.h>) with their C++ equivalents (like , , and ) in C++ header files to align with standard C++ practices and the Google C++ Style Guide.

Comment thread app/rest/request_json.h Outdated
Comment thread app/rest/response_json.h Outdated
Comment thread app/src/reference_counted_future_impl.h Outdated
Comment thread auth/src/desktop/auth_util.h Outdated
Comment thread auth/src/desktop/validate_credential.h Outdated
Comment thread auth/src/desktop/auth_desktop.h Outdated
Comment thread auth/src/include/firebase/auth.h Outdated
Comment thread database/src/desktop/connection/connection.h Outdated
Comment thread firestore/src/jni/traits.h Outdated
Comment thread storage/src/include/firebase/storage/controller.h Outdated
@AustinBenoit AustinBenoit added the tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). label Jun 5, 2026
@AustinBenoit AustinBenoit requested a review from a-maurice June 5, 2026 16:58
@github-actions github-actions Bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: full Trigger a FULL set of integration tests (uses expanded test matrix). labels Jun 5, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 5, 2026

Integration test with FLAKINESS (succeeded after retry)

Requested by @AustinBenoit on commit 71d3649
Last updated: Fri Jun 5 12:32 PDT 2026
View integration test log & download artifacts

Failures Configs
database [TEST] [FLAKINESS] [Android] [1/3 os: ubuntu] [1/4 android_device: android_target]
(1 failed tests)  CRASH/TIMEOUT
functions [TEST] [FLAKINESS] [Android] [1/3 os: macos] [1/4 android_device: android_target]
(1 failed tests)  CRASH/TIMEOUT
installations [TEST] [FLAKINESS] [Android] [1/3 os: windows] [1/4 android_device: emulator_ftl_target]
(1 failed tests)  FirebaseInstallationsTest.TestCanGetToken
remote_config [TEST] [FLAKINESS] [Android] [1/3 os: ubuntu] [1/4 android_device: android_latest]
(1 failed tests)  FirebaseRemoteConfigTest.TestAddOnConfigUpdateListener
storage [TEST] [FLAKINESS] [Android] [1/3 os: macos] [1/4 android_device: emulator_ftl_target]
(1 failed tests)  CRASH/TIMEOUT

Add flaky tests to go/fpl-cpp-flake-tracker

@github-actions github-actions Bot added the tests: succeeded This PR's integration tests succeeded. label Jun 5, 2026
@firebase-workflow-trigger firebase-workflow-trigger Bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-release-notes Skip release notes check tests: succeeded This PR's integration tests succeeded.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant