Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LLVM config bug fixes + structure changes #4863

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jyates301
Copy link

The provided CMake code appears to be checking for the presence of the LLVM library and setting up the necessary include directories and libraries for a project. There are a few potential issues and improvements that were made:

  1. Redundant find_package(LLVM CONFIG ...) blocks:
    The code redundantly repeats the same find_package(LLVM CONFIG ...) block multiple times with different LLVM versions. It's better to use a loop or a function to avoid repetitive code.

  2. Redundant if(NOT LLVM_FOUND) checks:
    The code redundantly checks for LLVM_FOUND after each find_package(LLVM CONFIG ...) block. Since the code continues to search for LLVM with different versions, these checks can be removed.

  3. Repetitive find_package(LLVM CONFIG ...) without version check:
    The code uses find_package(LLVM CONFIG ...) without specifying a version in the last block. It's recommended to always specify the required LLVM version to avoid unexpected issues.

  4. Use of LLVM_LIBRARY_DIR:
    The code uses the variable ${LLVM_LIBRARY_DIR} in find_library calls. Ensure that this variable is defined or set appropriately before using it.

  5. Use of find_library with NO_SYSTEM_ENVIRONMENT_PATH and NO_CMAKE_SYSTEM_PATH:
    These options may prevent the library from being found in standard system paths or paths set by the environment. Consider removing these options if they are causing issues.

  6. find_llvm_libs function behavior:
    The function find_llvm_libs seems to be searching for LLVM libraries and setting the LLVM_LIBS variable. However, it doesn't appear to be used in the provided code. Ensure that this function is called and its results are utilized elsewhere in your CMakeLists.txt file.

This revision uses a loop to find the first available LLVM version, removes redundant checks, and addresses some of the issues mentioned. Adjust the LLVM libraries in the find_llvm_libs function according to your project's requirements.

The provided CMake code appears to be checking for the presence of the LLVM library and setting up the necessary include directories and libraries for a project. However, there are a few potential issues and improvements that could be made:

1. **Redundant `find_package(LLVM CONFIG ...)` blocks:**
   The code redundantly repeats the same `find_package(LLVM CONFIG ...)` block multiple times with different LLVM versions. It's better to use a loop or a function to avoid repetitive code.

2. **Redundant `if(NOT LLVM_FOUND)` checks:**
   The code redundantly checks for `LLVM_FOUND` after each `find_package(LLVM CONFIG ...)` block. Since the code continues to search for LLVM with different versions, these checks can be removed.

3. **Repetitive `find_package(LLVM CONFIG ...)` without version check:**
   The code uses `find_package(LLVM CONFIG ...)` without specifying a version in the last block. It's recommended to always specify the required LLVM version to avoid unexpected issues.

4. **Use of `LLVM_LIBRARY_DIR`:**
   The code uses the variable `${LLVM_LIBRARY_DIR}` in `find_library` calls. Ensure that this variable is defined or set appropriately before using it.

5. **Use of `find_library` with `NO_SYSTEM_ENVIRONMENT_PATH` and `NO_CMAKE_SYSTEM_PATH`:**
   These options may prevent the library from being found in standard system paths or paths set by the environment. Consider removing these options if they are causing issues.

6. **`find_llvm_libs` function behavior:**
   The function `find_llvm_libs` seems to be searching for LLVM libraries and setting the `LLVM_LIBS` variable. However, it doesn't appear to be used in the provided code. Ensure that this function is called and its results are utilized elsewhere in your CMakeLists.txt file.

This revision uses a loop to find the first available LLVM version, removes redundant checks, and addresses some of the issues mentioned. Adjust the LLVM libraries in the `find_llvm_libs` function according to your project's requirements.
Copy link

google-cla bot commented Jan 11, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@florian-kuebler
Copy link
Collaborator

@beckerhe Can you take a look?

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.

None yet

2 participants