This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][NFC] Format all files in OpenMP
AbandonedPublic

Authored by tianshilei1992 on Jan 24 2021, 1:35 PM.

Details

Summary

This patch just executed clang-format on all source files of OpenMP,
excluding all test cases. Here is the Python script:

import glob
import subprocess

worklist = []

for f in glob.glob('llvm-project/openmp/**', recursive=True):
  if f.endswith('.h') or f.endswith('.hpp') or f.endswith('.c') or f.endswith('.cpp') or f.endswith('.cu') or f.endswith('.hip'):
    if '/tests/' in f or '/test/' in f:
      continue
    worklist.append(f)

for f in worklist:
  subprocess.Popen(['clang-format', f, '-i'])

Diff Detail

Event Timeline

tianshilei1992 created this revision.Jan 24 2021, 1:35 PM
tianshilei1992 requested review of this revision.Jan 24 2021, 1:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2021, 1:35 PM

I am afraid such action may be needed regularly, unless we freeze the clang-format version and write it in the documentation.
Because each version of clang-format may produce different result of formatting, unfortunately.
At least that was the case couple of years ago.

I am afraid such action may be needed regularly, unless we freeze the clang-format version and write it in the documentation.
Because each version of clang-format may produce different result of formatting, unfortunately.
At least that was the case couple of years ago.

Unluckily yes. The last time we did whole project format is probably three years ago, so I guess it's fine.

I am afraid such action may be needed regularly, unless we freeze the clang-format version and write it in the documentation.
Because each version of clang-format may produce different result of formatting, unfortunately.
At least that was the case couple of years ago.

I propose the following:

  1. Clang format the repo, or parts of it (libomptarget for example) [optional!]
  2. From now on forward, require each patch to be clang formatted, with git-clang-format or similar.
  3. If it is beneficial for a specific piece of code to be formatted explicitly before a diff is applied, do so in a NFC pre-commit.
  4. If we went with 1., we can from time to time redo 1. but from my experience with the Attributor this is mostly necessary because 1) was violated.

We *should not* freeze the clang-format version. We *should* write it in the documentation if we require 2), as we *should* write that doxygen comments are necessary and LLVM formatting is preferred. If a difference in formatting is from clang-format changes, rule 3. might apply but that is arguably not a problem.

I guess we should bring this up Wednesday.

There is a 'correct' version of clang-format to use. It's whatever phab runs as part of the auto-checking machinery. Hopefully that's clang-format from trunk, as that one is always available on disk, but it might also be the one from the last release.

Aside from clang-format, changes to this library are plagued by clang-tidy warnings. Some are fixable - our_function can become ourFunction. Some are not, such as warnings on uint32_t. Some might be - errors on unrecognised intrinsics because it assumes x64 (or at least, doesn't assume amdgcn/nvptx), or warnings about <gelf.h> not being found.

I'm in favour of running (preferably the above version) of clang-format across everything, ideally at a moment when there aren't a lot of large open reviews. Arcanist barks at me when I forget to clang-format the change, so perhaps that will suffice to avoid needing this again in a year or so.

tianshilei1992 abandoned this revision.Feb 16 2021, 11:38 AM

Discussion are still open. ;-)