ICRAR / leap-accelerate

Low-frequency Excision of the Atmosphere in Parallel
GNU General Public License v2.0
1 stars 1 forks source link

Clang Tidy Support #86

Closed calgray closed 3 years ago

calgray commented 3 years ago

This change configures clang-tidy to scan only the project source files and report issues in travis-ci. Linting can be performed by running make lint.

calgray commented 3 years ago

Linting is passing with no expected behaviour changes. There are only some narrowing conversion violations that need to be addressed in a separate PR to check for correctness and performance. Tests are failing due to travisCI timing out.

clang-tidy-10 -p=/home/calgray/Code/icrar/leap-accelerate/build/Release -quiet /home/calgray/Code/icrar/leap-accelerate/src/icrar/leap-accelerate/model/cpu/MetaData.cc
/home/calgray/Code/icrar/leap-accelerate/src/icrar/leap-accelerate/model/cpu/MetaData.cc:49:34: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
        m_constants.nbaselines = ms.GetNumBaselines();
                                 ^
/home/calgray/Code/icrar/leap-accelerate/src/icrar/leap-accelerate/model/cpu/MetaData.cc:61:28: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
        m_constants.rows = ms.GetNumRows();
                           ^
/home/calgray/Code/icrar/leap-accelerate/src/icrar/leap-accelerate/model/cpu/MetaData.cc:62:32: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
        m_constants.num_pols = ms.GetNumPols();
                               ^
/home/calgray/Code/icrar/leap-accelerate/src/icrar/leap-accelerate/model/cpu/MetaData.cc:63:32: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
        m_constants.stations = ms.GetNumStations();
                               ^
79224 warnings generated.
rtobar commented 3 years ago

BTW, I just tried this locally and found that the lint target created by cmake is bogus: it runs clang-tidy with an explicit set of checks rather than using your .clang-tidy file. My suggestion would be:

calgray commented 3 years ago

I have clang-tidy running both via a target and as part of cmake locally but I don't think it ran in the last build. For the CMake target I've been using the following modified lint.cmake script:

if(NOT TARGET lint)
    find_program(RUN_CLANG_TIDY
        NAMES 
            run-clang-tidy
        PATHS
            /usr/local/opt/llvm/bin
    )

    if (NOT RUN_CLANG_TIDY)
        message(WARNING "run-clang-tidy not found - no lint target")
    else()
        message(STATUS "run-clang-tidy found adding lint target")

        file(GLOB_RECURSE ALL_SOURCE_FILES
            ${PROJECT_SOURCE_DIR}/src/*.cc
            ${PROJECT_SOURCE_DIR}/src/*.h
        )

        add_custom_target(lint
            COMMAND ${RUN_CLANG_TIDY} -j 4 -quiet
            ${ALL_SOURCE_FILES}
            WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
            ${INCLUDE_DIRECTORIES}
        )
    endif()
endif()
calgray commented 3 years ago

I've added two approaches to linting in the CI pipeline. One is via the built in cmake support that lints while building and increases build time from 12m to 18m with a 2 thread parallel build. The other approach is with a lint-only target using run-clang-tidy that takes 15m (but it seems the -j 2 flag currently does not work for run-clang-tidy.py).

I'm happy to use either in CI or development and document how each can be used by project devs.

It's worth noting that clang-tidy doesn't currently run on cuda code as it generates warning related to shader and architecture versions, there is some discussion about this online but I haven't yet found a clean solution to supporting it.