Skip to content

Conversation

@dalg24
Copy link

@dalg24 dalg24 commented Oct 24, 2025

The project() command in the top-level CMakeLists.txt was not specifying languages which default to enabling C and CXX.
The googlemock and googletest project() commands were explicitly enabing both the C and CXX languages in their respective subdirectories, although all their source file are correctly identified as CXX and the C language is effectively never used.

Enabling the C language forces CXX-only projects downstream that embed this repo (via FetchContent or add subdirectory) to detect a C compiler at configuration-time.

This PR proposes to properly only enable the CXX language.

The project() command in the top-level CMakeLists.txt was not specifying
languages which default to enabling C and CXX.
The googlemock and googletest project() commands were explicitly enabing
both the C and CXX languages in their respective subdirectories,
although all their source file are correctly identified as CXX and the C
language is effectively never used.

Enabling the C language forces CXX-only projects downstream that embed
this repo (via FetchContent or add subdirectory) to detect a C compiler
at configuration-time.

This PR proposes to properly only enable the CXX language.

Signed-off-by: Damien L-G <dalg24@gmail.com>
@prince-chovatiya01
Copy link

CRITICAL ISSUES (Bugs/Security)

The original code line/snippet from the DIFF is:

-project(googletest-distribution)
+project(googletest-distribution CXX)

The suggested corrected code snippet is:

project(googletest-distribution LANGUAGES CXX)

This change ensures that the LANGUAGES keyword is used explicitly to specify the programming languages supported by the project, which is a more explicit and standard way to define project languages in CMake.

Another critical issue is the removal of the "C" language from the project command in googlemock/CMakeLists.txt and googletest/CMakeLists.txt. The original code lines are:

-project(gmock VERSION ${GOOGLETEST_VERSION} LANGUAGES CXX C)
+project(gmock VERSION ${GOOGLETEST_VERSION} LANGUAGES CXX)

and

-project(gtest VERSION ${GOOGLETEST_VERSION} LANGUAGES CXX C)
+project(gtest VERSION ${GOOGLETEST_VERSION} LANGUAGES CXX)

The suggested corrected code snippets are:

project(gmock VERSION ${GOOGLETEST_VERSION} LANGUAGES CXX C)

and

project(gtest VERSION ${GOOGLETEST_VERSION} LANGUAGES CXX C)

This change ensures that the "C" language is not removed from the project languages, which could potentially break existing code that relies on C language support.

SUGGESTIONS (Style/Efficiency)

None found.

CONCISE SUMMARY

The PR diff appears to introduce a critical issue by removing the "C" language from the project command in googlemock/CMakeLists.txt and googletest/CMakeLists.txt, which could potentially break existing code. The suggested corrections ensure that the LANGUAGES keyword is used explicitly and that the "C" language is not removed from the project languages. With these corrections, the PR can be approved. The overall quality of the PR is acceptable with minor fixes. Approved with minor fixes.

@prince-chovatiya01
Copy link

Code Review: CMake: Do not enable the C language (unnecessary)

Issues

  • The C language is being explicitly disabled in the CMake project definitions for googletest-distribution, gmock, and gtest, which may not be necessary if the project does not contain any C code.
  • The removal of the C language from the project definitions may cause issues if any of the dependencies or sub-projects rely on C code.
  • The Static Analysis Results indicate that no supported language files were detected, which may be a sign of a larger issue with the project configuration.

Suggestions

  • Verify that the project does not contain any C code and that the removal of the C language will not cause any issues with dependencies or sub-projects.
  • Consider adding a comment to the CMake project definitions to explain why the C language is being disabled.
  • Review the project configuration to ensure that it is correctly set up to detect and compile the necessary language files.

Verdict
The changes made in this pull request appear to be minor and are likely intended to simplify the project configuration by removing unnecessary language support. However, it is essential to verify that the removal of the C language will not cause any issues with dependencies or sub-projects. With proper verification and testing, these changes can be approved.

Example of improved code snippet:

# Verify that the project does not contain any C code
if(NOT EXISTS ${CMAKE_SOURCE_DIR}/c_code)
  # Disable the C language
  project(googletest-distribution CXX)
endif()

(Generated via AI Code Reviewer with RAG Context)

@dalg24
Copy link
Author

dalg24 commented Nov 3, 2025

could potentially break existing code

About the hypothetical code that would be broken by the change proposed here.
That code is both:

  • unlikely (by default, if not specifying what languages to enable, C and CXX are enabled),
  • 100% broken (project explicitly specifies what languages to enable and omits C although it uses it), and
  • trivial to fix (one line change).

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.

2 participants