Page MenuHomePhabricator

[clang-format] [NFC] Ensure clang-format is itself clang-formatted.
ClosedPublic

Authored by MyDeveloperDay on Sun, Oct 6, 2:42 AM.

Details

Summary

Before making a proposed change, ensure ClangFormat.cpp is fully clang-formatted,

no functional change just clang-formatting using the in tree .clang-format.

Diff Detail

Repository
rL LLVM

Event Timeline

MyDeveloperDay created this revision.Sun, Oct 6, 2:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptSun, Oct 6, 2:42 AM

If clang-format if clang-formatted now completely, I think will be good idea to add rule to check this during build, like Polly does.

If clang-format if clang-formatted now completely, I think will be good idea to add rule to check this during build, like Polly does.

This is a great idea... Do you know if is this run as part of the build or as a custom rule they have to remember to run? I guess some additional checking might be needed to allow bootstrapping this, but I really like this idea of once a directory is clean keeping it clean because of its part of a normal build.

Keeping code clang-format clean with all the best will in the world is super hard, changes drift in very slowly even with asking people to git-clang-format then if one is missing you are asking for what I'm doing here which is making a clang-format only commit and some don't like that.

# Add target to check formatting of polly files
file( GLOB_RECURSE files *.h lib/*.cpp lib/*.c tools/*.cpp tools/*.c tools/*.h unittests/*.cpp)
file( GLOB_RECURSE external lib/External/*.h lib/External/*.c lib/External/*.cpp isl_config.h)
list( REMOVE_ITEM files ${external})

set(check_format_depends)
set(update_format_depends)
set(i 0)
foreach (file IN LISTS files)
  add_custom_command(OUTPUT polly-check-format${i}
    COMMAND clang-format -sort-includes -style=llvm ${file} | diff -u ${file} -
    VERBATIM
    COMMENT "Checking format of ${file}..."
  )
  list(APPEND check_format_depends "polly-check-format${i}")

  add_custom_command(OUTPUT polly-update-format${i}
    COMMAND clang-format -sort-includes -i -style=llvm ${file}
    VERBATIM
    COMMENT "Updating format of ${file}..."
  )
  list(APPEND update_format_depends "polly-update-format${i}")

  math(EXPR i ${i}+1)
endforeach ()

add_custom_target(polly-check-format DEPENDS ${check_format_depends})
set_target_properties(polly-check-format PROPERTIES FOLDER "Polly")

add_custom_target(polly-update-format DEPENDS ${update_format_depends})
set_target_properties(polly-update-format PROPERTIES FOLDER "Polly")

I think when polly's script detects a change it's just showing the diff, this might be so much nicer if we could implement D68554: [clang-format] Proposal for clang-format to give compiler style warnings

mitchell-stellar accepted this revision.Mon, Oct 7, 9:34 AM

LGTM.

I agree that a system in place that either enforces clang-formatting on commit or after the fact would be ideal. Otherwise, I don't see a need to have to approve these NFC commits.

This revision is now accepted and ready to land.Mon, Oct 7, 9:34 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMon, Oct 7, 10:15 PM

I agree that a system in place that either enforces clang-formatting on commit or after the fact would be ideal. Otherwise, I don't see a need to have to approve these NFC commits.

The current coding policy contains "Our long term goal is for the entire codebase to follow the convention, but we explicitly do not want patches that do large-scale reformatting of existing code." that was added after someone removed all trailing whitespace all LLVM files. Reformatting the code you are going to work in is fine, but not on the entire code base. Ideally we'd also run the regression tests in a pre-commit hook.

Btw, I am the author of the CMakeLists snippet quoted by @MyDeveloperDay. Before that, it was a shell script that didn't run on Windows. Making it part of the regression test basically eliminated all discussion about code formatting, but we had to run large-scale reformatting whenever clang-format changed in some way. It also runs by the polly-* buildbots which I personally do not like since I don't see code formatting as a reason why a build should fail.

Btw, I am the author of the CMakeLists snippet quoted by @MyDeveloperDay. Before that, it was a shell script that didn't run on Windows. Making it part of the regression test basically eliminated all discussion about code formatting, but we had to run large-scale reformatting whenever clang-format changed in some way. It also runs by the polly-* buildbots which I personally do not like since I don't see code formatting as a reason why a build should fail.

Thank you for your comment, do we have CMake infrastructure (I'm not a CMake expert) to be able to parameterize that snippet and put it somewhere centrally so that others could simply inherit this in their CMakeList.txt like:

file( GLOB files ../lib/Format/*.h ../lib/Format/*.cpp ../unittests/*.cpp ../include/clang/Format/*.h)
add_clang_format_target(XXX,files)

so that they'd get your XXX-check-format and XXX-update-format rules? It might help the proliferation of clang-formatted areas? (and keep them clean)

We should seek having only one formatting policy for the entire project, it's too confusing otherwise (e.g. LLVMSupport/ADT is also used by clang-format, are they force-formatted as well? What about the tools/utils subdirectories?). If you would like to gradually move into that direction, it should be discussed on llvm-dev.