search

RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.34k stars 1.26k forks source link

Halfway use of clang-format is unsustainable #4843

Open jwnimmer-tri opened 7 years ago

jwnimmer-tri commented 7 years ago

Right now Drake recommends using clang-format sometimes, but we don't enforce that all code in tree is clang-format idempotent.

This is bad, because:

(1) Now when anyone runs clang-format on a file in order to fix some cpplint in their PR, it reformats things not even touched by the PR, thus destroying the original author's artisinally-crafted whitespace. The only argument against clang-format-all-the-things is that people disagree with its choices; that is logically irreconcilable with allowing people to clang-format whole files that they didn't author.

(2) Without tool assistance our team (and open-source contributers submitting PRs) are not experienced enough know how to format code readably, reliably, repeatably. I hesitate to point out dozens of authors' mistakes in PRs, but they are there. The cpplint checks alone are not enough to identify all of the problems. We need a tool that is sharper that cpplint at spotting errors. Leaving it to code review time is expensive.

Formatting defects should be fixed (or at least called out) by a tool. So maybe we should find a better tool than clang-format and cpplint.

We can look around for better tools, but absent finding any I will resume the clang-format-all-the-things march again.

liangfok commented 7 years ago

Now when anyone runs clang-format on a file in order to fix some cpplint in their PR, it reformats things not even touched by the PR, thus destroying the original author's artisinally-crafted whitespace.

This is a key reason why I have been running clang-format increasingly less often over time -- Drake's code is increasingly becoming not idempotent to clang-format meaning running clang-format is increasingly making changes that are not relevant to the current PR. It's a snowball effect since I'm sure my new code is not idempotent to clang-format.

david-german-tri commented 7 years ago

@liangfok, clang-format-diff may be useful; it only changes modified lines. Example usage to clang-format the previous commit:

git diff -U0 HEAD^ | clang-format-diff-3.9 -i -p1
liangfok commented 7 years ago

@david-german-tri: Thanks! I will check it out.

jwnimmer-tri commented 6 years ago

Last week I had an idea to start to move the forward. Formatting and layout of the pydrake C++ bindings code is often challenging for developers. The code layout is atypical versus regular imperative C++ code, so we often haggle about its whitespace and have to nitpick it to death. It might be a good starting point for opt-in required clang-format idempotency, on a package-by-package basis. In time, perhaps the whole tree works this way, but we have to start somewhere. I tried to start in //common last time and got shot down. Possibly being more rigid about boilerplate code would fly better, and prove out the tooling.

SeanCurtis-TRI commented 1 year ago

It's worth noting that this is blocked by #20011.