Skip to content

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

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

Change it so that the header files are directly included#1865
AustinBenoit wants to merge 2 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
Comment thread app/src/util_android.h Outdated
Comment thread firestore/src/main/field_value_main.h
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
#ifndef FIREBASE_APP_REST_REQUEST_JSON_H_
#define FIREBASE_APP_REST_REQUEST_JSON_H_

#include <assert.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.

medium

In C++ headers, standard C library headers should be included using the C++ style <c...> format (e.g., <cassert> instead of <assert.h>) to adhere to standard C++ practices and the Google C++ Style Guide.

Suggested change
#include <assert.h>
#include <cassert>

Comment thread app/rest/response_json.h
Comment on lines +20 to +21
#include <assert.h>
#include <string.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.

medium

In C++ headers, standard C library headers should be included using the C++ style <c...> format (e.g., <cassert> and <cstring> instead of <assert.h> and <string.h>) to adhere to standard C++ practices and the Google C++ Style Guide.

Suggested change
#include <assert.h>
#include <string.h>
#include <cassert>
#include <cstring>

#ifndef FIREBASE_APP_SRC_REFERENCE_COUNTED_FUTURE_IMPL_H_
#define FIREBASE_APP_SRC_REFERENCE_COUNTED_FUTURE_IMPL_H_

#include <assert.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.

medium

In C++ headers, standard C library headers should be included using the C++ style <c...> format (e.g., <cassert> instead of <assert.h>) to adhere to standard C++ practices and the Google C++ Style Guide.

#include <cassert>

#ifndef FIREBASE_AUTH_SRC_DESKTOP_AUTH_UTIL_H_
#define FIREBASE_AUTH_SRC_DESKTOP_AUTH_UTIL_H_

#include <assert.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.

medium

In C++ headers, standard C library headers should be included using the C++ style <c...> format (e.g., <cassert> instead of <assert.h>) to adhere to standard C++ practices and the Google C++ Style Guide.

Suggested change
#include <assert.h>
#include <cassert>

#ifndef FIREBASE_AUTH_SRC_DESKTOP_VALIDATE_CREDENTIAL_H_
#define FIREBASE_AUTH_SRC_DESKTOP_VALIDATE_CREDENTIAL_H_

#include <string.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.

medium

In C++ headers, standard C library headers should be included using the C++ style <c...> format (e.g., <cstring> instead of <string.h>) to adhere to standard C++ practices and the Google C++ Style Guide.

Suggested change
#include <string.h>
#include <cstring>

#ifndef FIREBASE_AUTH_SRC_DESKTOP_AUTH_DESKTOP_H_
#define FIREBASE_AUTH_SRC_DESKTOP_AUTH_DESKTOP_H_

#include <stdint.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.

medium

In C++ headers, standard C library headers should be included using the C++ style <c...> format (e.g., <cstdint> instead of <stdint.h>) to adhere to standard C++ practices and the Google C++ Style Guide.

Suggested change
#include <stdint.h>
#include <cstdint>

#ifndef FIREBASE_AUTH_SRC_INCLUDE_FIREBASE_AUTH_H_
#define FIREBASE_AUTH_SRC_INCLUDE_FIREBASE_AUTH_H_

#include <stdint.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.

medium

In C++ headers, standard C library headers should be included using the C++ style <c...> format (e.g., <cstdint> instead of <stdint.h>) to adhere to standard C++ practices and the Google C++ Style Guide.

Suggested change
#include <stdint.h>
#include <cstdint>

#ifndef FIREBASE_DATABASE_SRC_DESKTOP_CONNECTION_CONNECTION_H_
#define FIREBASE_DATABASE_SRC_DESKTOP_CONNECTION_CONNECTION_H_

#include <stdint.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.

medium

In C++ headers, standard C library headers should be included using the C++ style <c...> format (e.g., <cstdint> instead of <stdint.h>) to adhere to standard C++ practices and the Google C++ Style Guide.

Suggested change
#include <stdint.h>
#include <cstdint>

#define FIREBASE_FIRESTORE_SRC_JNI_TRAITS_H_

#include <jni.h>
#include <stdint.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.

medium

In C++ headers, standard C library headers should be included using the C++ style <c...> format (e.g., <cstdint> instead of <stdint.h>) to adhere to standard C++ practices and the Google C++ Style Guide.

Suggested change
#include <stdint.h>
#include <cstdint>

#ifndef FIREBASE_STORAGE_SRC_INCLUDE_FIREBASE_STORAGE_CONTROLLER_H_
#define FIREBASE_STORAGE_SRC_INCLUDE_FIREBASE_STORAGE_CONTROLLER_H_

#include <stdint.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.

medium

In C++ headers, standard C library headers should be included using the C++ style <c...> format (e.g., <cstdint> instead of <stdint.h>) to adhere to standard C++ practices and the Google C++ Style Guide.

Suggested change
#include <stdint.h>
#include <cstdint>

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant