0
\$\begingroup\$

I would like some feedback on a CMakeLists.txt file I created for compiling my project. I have pasted the CMakeLists as well as my source code below. One thing I would specifically appreciate feedback on the sanitization options I have enabled. Are there more I should enable, and/or should I should reduce? I know -fsanitize=address, -fsanitize=thread, and -fsanitize=memory groups can't be used with others (according to the clang documentation). Would one of the other groups be better preferred to use on a first-pass rather than the one I chose (address)?

Also - the blob feature I am using, I have based on a StackOverflow answer I read - I understand that this doesn't detect new C source files and I'm fine with that, but besides that subtle detail is this an okay practice to follow?

CMakeLists.txt

cmake_minimum_required(VERSION 3.13)
project(FirstProject C)

find_package(Curses REQUIRED)
include_directories(${CURSES_INCLUDE_DIR})

set(CMAKE_C_COMPILER clang)
set(CMAKE_C_STANDARD 99)
set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Weverything -fsanitize=undefined,integer,implicit-conversion,nullability,address,leak,cfi -flto -fvisibility=default")

FILE(GLOB Sources *.c)
add_executable(${CMAKE_PROJECT_NAME} ${Sources})
target_link_libraries(${CMAKE_PROJECT_NAME} ${CURSES_LIBRARIES})

main.c (Code comes from here)

#include <ctype.h>
#include <errno.h>
#include <inttypes.h>
#include <limits.h>
#include <ncurses.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>

// Generous estimate of the maximum number of digits
// https://stackoverflow.com/a/10536254
#define ULL_DIGITS (3 * sizeof(unsigned long long))
#define ERR_MSG_MAX_LENGTH 32
#define NUL '\0'
#define NUL_SIZE 1

int ask_ull(unsigned long long *result, const char *prompt);

/**
 * Prints a prompt then reads an unsigned long long, using ncurses.

 * Returns 0 on success.  Returns errno on failure, which is set to

 * ERANGE, EDOM, or EIO.
 */
int ask_ull(unsigned long long *result, const char *prompt) {
    char buf[ULL_DIGITS + NUL_SIZE];
    char *endptr;
    printw("%s", prompt);
    getnstr(buf, ULL_DIGITS);
    *result = strtoull(buf, &endptr, 10);
    if (errno == ERANGE) {
        // Overflow or underflow
        return errno;
    }
    if (endptr == buf || strchr(buf, '-')) {
        // Unsuccessful conversion
        errno = EDOM;
        return errno;
    }
    while (isspace(*endptr)) endptr++;
    if (*endptr) {
        // Trailing junk
        errno = EIO;
        return errno;
    }
    errno = 0;
    return errno;
}

int main(void) {
    unsigned long long height, width, length;
    height = width = length = 0;

    char errmsg[ERR_MSG_MAX_LENGTH];
    errmsg[0] = NUL;

    initscr();
    printw("--- Volume Calculator --\n");
    if (!ask_ull(&length, "Enter length: ")) {
        sscanf(errmsg, "%s", "Unable to scan length");
    }
    if (!ask_ull(&width, "Enter width: ")) {
        sscanf(errmsg, "%s", "Unable to scan width");
    }
    if (!ask_ull(&height, "Enter height: ")) {
        sscanf(errmsg, "%s", "Unable to scan height");
    }

    if (errmsg[0] != NUL) {
        refresh();
        endwin();
        perror(errmsg);
        return errno;
    }

    unsigned long long volume = length * width * height;
    printw("Volume: %llu", volume);
    refresh();
    getch();
    endwin();
}
\$\endgroup\$
1
\$\begingroup\$

I am not familiar with the Clang sanitization command-line options so I can't give any feedback about those, but regarding the CMake code I would suggest the following CMakeLists.txt file

cmake_minimum_required(VERSION 3.13)
project(FirstProject C)

find_package(Curses REQUIRED)

add_executable(${CMAKE_PROJECT_NAME} main.c)

target_include_directories(${CMAKE_PROJECT_NAME} PRIVATE ${CURSES_INCLUDE_DIR})
target_link_libraries(${CMAKE_PROJECT_NAME} PRIVATE ${CURSES_LIBRARIES})

if(NOT CMAKE_C_COMPILER_ID STREQUAL "Clang")
  message(WARNING "Use the Clang compiler instead. "
                  "FirstProject officially supports Clang "
                  "(although other compilers might work).") 
endif()       

target_compile_features(${CMAKE_PROJECT_NAME} PRIVATE c_std_99)
target_compile_options(${CMAKE_PROJECT_NAME} PRIVATE
  $<$<C_COMPILER_ID:Clang>:
    -Weverything
    -fsanitize=undefined,integer,implicit-conversion,nullability,address,leak,cfi
    -flto
    -fvisibility=default>)
target_link_options(${CMAKE_PROJECT_NAME} PRIVATE
  $<$<C_COMPILER_ID:Clang>:
    -fsanitize=undefined,integer,implicit-conversion,nullability,address,leak,cfi
    -flto>)

Some comments

Avoid using FILE(GLOB) to specify source code files

Avoid using FILE(GLOB), instead specify the source code files explicitly either by

add_executable(${CMAKE_PROJECT_NAME} main.c)

or

add_executable(${CMAKE_PROJECT_NAME})
target_sources(${CMAKE_PROJECT_NAME} PRIVATE main.c)

Śee also https://stackoverflow.com/questions/32411963/why-is-cmake-file-glob-evil

Use target_* commands

Avoid

Instead use

The FindCurses module does not yet support imported targets as of today (2 January 2019, CMake 3.13.2) so it needs to be used in the old style

target_include_directories(${CMAKE_PROJECT_NAME} PRIVATE ${CURSES_INCLUDE_DIR})
target_link_libraries(${CMAKE_PROJECT_NAME} PRIVATE ${CURSES_LIBRARIES})

In the future (when support has been added to CMake for imported targets in FindCurses) the two lines should be replaced by the line:

target_link_libraries(${CMAKE_PROJECT_NAME} PRIVATE Curses::Curses)

Instead of setting the CMake variable CMAKE_C_STANDARD

set(CMAKE_C_STANDARD 99)

is good practice to use target_compile_features() instead

 target_compile_features(${CMAKE_PROJECT_NAME} PRIVATE c_std_99)

In this very case it makes no practical difference but for a C++ header-only library, such compile features could be specified in the INTERFACE

 add_library(myheaderonly INTERFACE)
 target_compile_features(headeronlylib INTERFACE cxx_std_11)

to provide usage requirements for consumers of the library (see also https://cmake.org/cmake/help/latest/manual/cmake-buildsystem.7.html)

The target_link_options() line was added to be able to build the executable. (I am not sure it is correct).

Use generator expressions

The generator expression

$<$<C_COMPILER_ID:Clang>:-Weverything -fsanitize=undefined,integer,implicit-conversion,nullability,address,leak,cfi -flto -fvisibility=default>

is expanded to

-Weverything -fsanitize=undefined,integer,implicit-conversion,nullability,address,leak,cfi -flto -fvisibility=default

when the Clang compiler is used, but for other compilers it is expanded to nothing.

Avoid setting CMAKE_C_COMPILER

Instead of setting the CMake variable CMAKE_C_COMPILER, give a WARNING or a FATAL_ERROR whenever a non-supported C compiler is used.

if(NOT CMAKE_C_COMPILER_ID STREQUAL "Clang")
  message(WARNING "Use the Clang compiler instead. FirstProject officially supports Clang (although other compilers might work).") 
endif()       

(WARNING could be replaced by FATAL_ERROR to prevent the use of any other C compiler than Clang)

To compile the project, specify the C compiler with the environment variable CC

mkdir /tmp/build
cd /tmp/build
CC=clang cmake -G Ninja ~/FirstProject
ninja -v

Use the ninja command-line flag -v if you want to see the actual commands being run.

\$\endgroup\$
  • \$\begingroup\$ I appreciate the thorough answer. The last segment though, any particular advantage of using ninja over traditional make? Will it make a difference in compile-time speed? \$\endgroup\$ – Faraz Jan 2 at 22:37
  • 1
    \$\begingroup\$ Probably very little. For me the main advantages of ninja is that you don't need to specify the number of threads (e.g. make -j) and that the software is available for Linux, Mac and Windows. \$\endgroup\$ – Erik Sjölund Jan 2 at 22:42

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.