CMakeLists.txt (3)

Line range hint 98-107: LGTM: Conditional compilation of test files

The conditional compilation of test files is a good practice. It reduces compilation time and binary size when tests are not needed.

One suggestion for improvement:

While using file(GLOB ...) for test files is generally acceptable, consider explicitly listing the test files instead. This approach provides better control over which files are included and makes it easier to spot when a new test file is added or removed. For example:

if(VERIFIER_ENABLE_TESTS)
set(ALL_TEST
    "./src/test/test_conformance.cpp"
    "./src/test/test_marshal.cpp"
    "./src/test/test_print.cpp"
    "./src/test/test_verify.cpp"
    "./src/test/test_wto.cpp"
    "./src/test/test_yaml.cpp"
)
endif()

This approach is more explicit and less prone to accidentally including or excluding files.

---

Line range hint 133-151: LGTM: Conditional setting of test executable properties

The conditional setting of target properties for test executables is well implemented. This ensures that these properties are only set when tests are enabled, maintaining a clean build configuration.

One suggestion for improvement:

Consider using a variable for the RUNTIME_OUTPUT_DIRECTORY instead of repeating "${CMAKE_BINARY_DIR}/.." for each executable. This would make it easier to change the output directory for all test executables in the future if needed. For example:

if(VERIFIER_ENABLE_TESTS)
    set(TEST_OUTPUT_DIR "${CMAKE_BINARY_DIR}/..")
    set_target_properties(check tests run_yaml conformance_check
        PROPERTIES
        RUNTIME_OUTPUT_DIRECTORY "${TEST_OUTPUT_DIR}")

    add_dependencies(tests conformance_check)
endif()

This approach reduces repetition and makes the configuration more maintainable.

---

Line range hint 179-200: LGTM: Conditional compilation and linking of test executables

The conditional setting of compilation flags and library linkage for test executables is well implemented. This ensures that these settings are only applied when tests are enabled, maintaining a clean build configuration.

One suggestion for improvement:

Consider grouping the compile options for each executable together, followed by the link libraries. This can improve readability and make it easier to manage the settings for each executable. For example:

if(VERIFIER_ENABLE_TESTS)
    # Compile options
    foreach(target check tests)
        target_compile_options(${target} PRIVATE ${COMMON_FLAGS})
        target_compile_options(${target} PUBLIC "$<$<CONFIG:DEBUG>:${DEBUG_FLAGS}>")
        target_compile_options(${target} PUBLIC "$<$<CONFIG:RELEASE>:${RELEASE_FLAGS}>")
        target_compile_options(${target} PUBLIC "$<$<CONFIG:SANITIZE>:${SANITIZE_FLAGS}>")
    endforeach()

    # Link libraries
    target_link_libraries(check PRIVATE ebpfverifier)
    target_link_libraries(tests PRIVATE 
        ebpfverifier
        bpf_conformance
        ${CMAKE_DL_LIBS}
        Threads::Threads
        Catch2::Catch2WithMain
    )
    target_link_libraries(run_yaml PRIVATE ebpfverifier)
    target_link_libraries(conformance_check PRIVATE ebpfverifier)
endif()

This approach reduces repetition and groups related settings together, making the configuration more maintainable.

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

CMakeLists.txt (3)

Line range hint 98-107: LGTM with suggestion: Conditional inclusion of test files

The conditional inclusion of test files based on VERIFIER_ENABLE_TESTS is good. It ensures that test-related files are only processed when tests are enabled.

Consider replacing file(GLOB ...) with explicit file listings. While GLOB is convenient, it can lead to build system issues if new files are added without re-running CMake. Here's an example of how you could refactor this:

if(VERIFIER_ENABLE_TESTS)
    set(ALL_TEST
        src/test/test_conformance.cpp
        src/test/test_marshal.cpp
        src/test/test_print.cpp
        src/test/test_verify.cpp
        src/test/test_wto.cpp
        src/test/test_yaml.cpp
    )
endif()

This approach provides better control over which files are included in the build and ensures that CMake always picks up changes to the file list.

---

Line range hint 133-151: LGTM: Setting properties for test executables

The addition of runtime output directory settings for test executables and the dependency between 'tests' and 'conformance_check' are good practices.

For consistency and easier maintenance, consider using a loop or a function to set the runtime output directory for all test executables. For example:

if(VERIFIER_ENABLE_TESTS)
    foreach(test_exec check tests run_yaml conformance_check)
        set_target_properties(${test_exec}
            PROPERTIES
            RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/..")
    endforeach()

    add_dependencies(tests conformance_check)
endif()

This approach reduces repetition and makes it easier to apply the same settings to all test executables consistently.

---

Line range hint 179-200: LGTM: Conditional compilation and linking for test executables

The conditional setting of compile options and linking of libraries for test executables is well-implemented. It ensures that these settings are only applied when tests are enabled, which is consistent with the VERIFIER_ENABLE_TESTS option.

To improve readability and maintainability, consider creating a function to apply common settings to all test executables. For example:

if(VERIFIER_ENABLE_TESTS)
    function(configure_test_executable target)
        target_compile_options(${target} PRIVATE ${COMMON_FLAGS})
        target_compile_options(${target} PUBLIC "$<$<CONFIG:DEBUG>:${DEBUG_FLAGS}>")
        target_compile_options(${target} PUBLIC "$<$<CONFIG:RELEASE>:${RELEASE_FLAGS}>")
        target_compile_options(${target} PUBLIC "$<$<CONFIG:SANITIZE>:${SANITIZE_FLAGS}>")
        target_link_libraries(${target} PRIVATE ebpfverifier)
    endfunction()

    configure_test_executable(check)
    configure_test_executable(tests)
    target_link_libraries(tests PRIVATE bpf_conformance ${CMAKE_DL_LIBS} Threads::Threads Catch2::Catch2WithMain)

    configure_test_executable(run_yaml)
    configure_test_executable(conformance_check)
endif()

This approach centralizes the common configuration and makes it easier to maintain consistent settings across all test executables.

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE