[RFC] CMake formatting for LLVM libc

In [RFC] Having an auto-formatter for CMake @Sukumarsawant raised the idea of a CMake formatter for all of LLVM, but there wasn’t concensus to proceed. However, I still want one and figured that we could start with our project.

This weekend I spent time creating a formatter that would handle the non-controversial items. If we accept this, I would get it checked it and run as part of the formatting presubmit GitHub action.

These are the formatting rules I went with, based on my observation of LLVM libc (and taking a glance at the rest of the LLVM project so that we don’t gratuitously deviate)

  1. Command Casing: Built-in language commands cased in lowercase (add_entrypoint_object, set, if); module commands (like ExternalProject_Add) and custom functions retain canonical/declared casing.
  2. Parenthesis Spacing: No space between command name and opening (. Collapses multiple spaces between arguments down to a single space.
  3. Quoted String Immutability: Quoted arguments ("...") and bracket arguments ([=[...]=]) are single immutable AST tokens. Multi-line quoted strings are preserved 100% untouched.
  4. Empty Closures: endif(), else(), endfunction(), endmacro(), endforeach(), endwhile().
  5. Schema-Aware Keyword Casing: Keywords in command schema upper-cased; positional args, function parameters & file paths untouched.
  6. Multi-line Argument Layout: Keywords and positional args indented +2 spaces relative to call base; multi-value list items indented +4 spaces; closing ) at +0 spaces.
  7. Control Block Indentation: 2-space indentation inside if/foreach/function/macro.
  8. Comment Formatting: Line comments buffer and align with the indentation level of the code element immediately following them, unless separated by a blank line (standalone comments) or immediately preceding a closing parenthesis ).
  9. Cleanliness: Trailing whitespace stripped, single trailing newline for non-empty files; empty files preserved 0-byte.

Here’s a branch with the resulting formatter run:

There are two areas that are not formatted correctly right now (hopefully. I’ve stared at the diff too long and might miss things). In both of these cases, I am inclined to change the source rather than the formatter:

  1. Multi-line trailing quote:
Foo bar baz # This is a trailing quote
  # that spans multiple lines.

Previously we had space aligned the two comments. However, semantically, there is no way to tell whether the second line is a continuation or whether it’s a comment on the following item. The correct thing here is to make it a multi-line quote before the Foo bar baz line.

  1. Variables at the end of multi-value lists:
add_header_library(
  scanf_config
  HDRS
    scanf_config.h
    ${scanf_config_copts}
)

There is no way to tell whether or not ${scanf_config_copts} should be part of HDRS or should be an ARGN value of add_header_library. In this case, I will add a comment to these # ARGN that will both tell the human and the formatter that this isn’t intended to be part of HDRS.

I am intentionally not sharing the formatter yet, because it’s definitely not polished at all =) But it’s in Python and I would provide unit tests, etc. for it. It doesn’t rely on anything from PIP so doesn’t require new things on people’s workstations or the buildbots to incorporate as part of their flow.

Tks,
Jeff Bailey

2 Likes

Also, thanks for @michaelrj-google for a pre-review of the formatted result and catching something I’d missed. =)

I’ve already reviewed the proposed change. Regardless of if we decide to commit to a cmake formatter longterm I think most of the changes are obviously good cleanups. Specifically it makes spaces vs not after an if but before the (parentheses) consistent, fixes a whole bunch of indentation, and trims any trailing spaces.

There are a couple places that need to be cleaned up before/after, as mentioned. I think those should be cleaned up regardless. When we do merge this commit I’d like to add it to the git-blame-ignore-revs list so it doesn’t mess up the blame.

In short: I’m in favor.

Not specifically about libc, but elsewere a difference is made between control-flow commands (if(c)) and function/macro call commands (add_executable()). I find that distinction useful since if, foreach, … do not behave like function calls. This practice may have originated from C/C++ where if (), for () are also not to be interpreted as function calls. However, CMake’s own CMake code mostly does not have that space, but is itself not entirely concistent.

This would be easy to implement in the formatter since it already has to know what the keywords are for formatting the contents correctly. However, I don’t love it. Take a look at llvm-project/libc/benchmarks/gpu/src/math/CMakeLists.txt at main · llvm/llvm-project · GitHub.

if(LIBC_TARGET_ARCHITECTURE_IS_NVPTX)
  if(CUDAToolkit_FOUND)
    set(libdevice_path ${CUDAToolkit_BIN_DIR}/../nvvm/libdevice/libdevice.10.bc)
    if (EXISTS ${libdevice_path})
      list(APPEND math_benchmark_flags
        "SHELL:-Xclang -mlink-builtin-bitcode -Xclang ${libdevice_path}")
      # Compile definition needed so the benchmark knows to register
      # NVPTX benchmarks.
      list(APPEND math_benchmark_flags "-DNVPTX_MATH_FOUND=1")
    endif()
  endif()
endif()

I don’t gain any more information from the if having a space after it, and the predominant style in the libc part of the code has no space after the keyword. Combined with what you mentioned about how CMake’s own code doesn’t tend to have the space, I’d prefer to continue not having the space after the if (and similar keywords) unless there’s other feedback.

FWIW, I’m currently slated to apply the patch result of the current formatter to the tree tomorrow as this has been the only comment other than Michael’s supportive one before this. I wanted to wait a few days after it hit LLVM Weekly News in case more feedback came.

Tks,
Jeff Bailey