Note: contrary to what I said previously, I didn't change .clang-format nor utils/generate_feature_test_macro_components.py script.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
FYI, https://clang.llvm.org/docs/ClangFormattedStatus.html shows the status of LLVM parts being clang-formatted.
Comment Actions
I really like that we get clang-format to work. Live is simply to short to worry about formatting.
I assume we should simply make one monster commit adding that to the exception list for git blame and be done with it
Comment Actions
That would still make it more difficult to blame stuff if lines move around a lot. Another option would be to mandate that we clang-format everything we touch from now on. Thus, as we make changes to the library, the lines we change (and some amount of surrounding context) would be re-formatted. I'm not sure which approach is best, since I haven't done it before. I'd be curious to hear stories of folks who moved existing codebases to Clang format.
| libcxx/utils/generate_feature_test_macro_components.py | ||
|---|---|---|
| 684 ↗ | (On Diff #308042) | Shouldn't this be # ifdef (3 spaces) to get the ifdef aligned at 4 columns? |
Comment Actions
That would be interesting, but I believe that moving /splitting of lines should be common enough that it is already sorted out
Comment Actions
Has clang-format been improved enough that it will not "wreck" std::less<void>? Last time I tried it on that code, it pessimized it pretty thoroughly.
Code pasted here for reference:
#if _LIBCPP_STD_VER > 11
template <>
struct _LIBCPP_TEMPLATE_VIS less<void>
{
template <class _T1, class _T2>
_LIBCPP_CONSTEXPR_AFTER_CXX11 _LIBCPP_INLINE_VISIBILITY
auto operator()(_T1&& __t, _T2&& __u) const
_NOEXCEPT_(noexcept(_VSTD::forward<_T1>(__t) < _VSTD::forward<_T2>(__u)))
-> decltype (_VSTD::forward<_T1>(__t) < _VSTD::forward<_T2>(__u))
{ return _VSTD::forward<_T1>(__t) < _VSTD::forward<_T2>(__u); }
typedef void is_transparent;
};
#endifAlso, @missco wrote:
I really like that we get clang-format to work. Live is simply to short to worry about formatting.
Writing is once, reading is many, many times.
Formatting that reveals the underlying structure of the code is a time-saver to future you (and others).
Comment Actions
I would like to note that with MSVC STL we simply work around those rare cases via //clang-format off. One main use case are requires clauses that are not yet supported. That said, the vast majority of the code works fine.
Also, @missco wrote:
I really like that we get clang-format to work. Live is simply to short to worry about formatting.
Writing is once, reading is many, many times.
Formatting that reveals the underlying structure of the code is a time-saver to future you (and others).
That is true, but consistency in a code base is incredibly valuable for new contributors. Having to wonder how to indent / format complicated STL code is a really hard hurdle for a first time contributor
Also consistency helps reading a lot. and as said above the few rare cases where it is detrimental one can always simply use // clang-format off
Comment Actions
I agree with @miscco, the advantages of consistent formatting outweigh the inconveniences caused by clang-format's shortcomings.
Comments like "clang-format on/off" are one of the solutions to the problem. Another being to force line breaks with empty trailing comments.
Regarding the strategy of adopting clang-format, the majority of projects I've seen cannot reformat everything at once.
First step is always to enforce git clang-format (but with macro-heavy code care must be taken because the context may accidentally become bigger than intended, at least for all the already released versions of clang-format, there are some improvements in master).
Apart from that, reformatting rarely touched files is another option (there's been a proposal to more or less automatically reformat files that haven't been touched for e.g. 6 months). But that's not a necessity IMO.
| libcxx/utils/generate_feature_test_macro_components.py | ||
|---|---|---|
| 684 ↗ | (On Diff #308042) | The indentation of preprocessor directives depends on IndentWidth: (currently 2). Anyway, I wanted to match how clang-format would format it. |
Comment Actions
I think it's clear that we'll benefit from clang-format.
However, Marshall does have a point that sometimes, you want to format things in a specific way and not have clang-format mess it up. I think it's fine to use // clang-format off for those cases.
@curdeius Could you add a CI job that makes sure that the commit we're testing has been run through git clang-format? You could run it first in the pipeline, and we could even consider adding a wait step in the pipeline so that the rest of the pipeline doesn't run if you haven't clang-formated your changes.
Comment Actions
Thanks @curdeius for looking into this! The lack of proper clang-format rules for libc++ also bothered me a bit. Ut didn't bother me enough to look into it yet.
I also prefer to use automatic formatting as much as possible. If clang-format has things it doesn't format well, we might want to ask the clang-format devs so they can improve it. Then we can use the automatic formatting after the next release. That would benefit us and the other users of clang-format.
I prefer to have one commit to format everything, then we only need to blacklist one commit in the blame list. I would at least prefer to do all code in include and src in one commit. I don't have a preference whether we should also do the unit tests and benchmarks in one go or fix them when the files are touched.
I don't have a strong preference on which clang-format rules we use. However I think we should decide on the clang-format rules before formatting the code.
Comment Actions
If the idea is to reformat everything, I'd recommend just using the normal LLVM style, like the rest of the project.
This is a mostly solved problem: Git 2.23 added support for ignoring whitespace commits in blame output. You store hashes that should be blame-transparent in a file called .git-blame-ignore-revs (or any other name), check that file in, and then either pass --ignore-revs-file .git-blame-ignore-revs to git blame or run git config blame.ignoreRevsFile .git-blame-ignore-revs once to make git blame always use that blame file.
We auto-formatted a big chunk of Chromium's code a while ago. We did this before git 2.23 so we wrote a custom tool that does the same thing (https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html), but with git 2.23+ there's even upstream support for this.
Comment Actions
@ldionne, I'll need your help to set this up. Agents need to have git-clang-format installed. I haven't tried but I guess I can't apt install clang-format in the buildkite script and it doesn't sound like a great idea anyway.
And, wouldn't it be possible to reuse the existing machinery in llvm/clang premerge checks that runs git-clang-format? I am afraid it can't be reused because it uses different CI pipeline and different agents...
Comment Actions
I've used that for smaller whitespace-changing patches, however does git do a good job at figuring out the blame correctly when stuff moves around a lot? My concern was that .git-blame-ignore-revs wouldn't work well for such a large reflow, however if it does, then I'd say my concerns are mostly alleviated.
We auto-formatted a big chunk of Chromium's code a while ago. We did this before git 2.23 so we wrote a custom tool that does the same thing (https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html), but with git 2.23+ there's even upstream support for this.
That's large enough to be considered a success story, clearly.
No, indeed. You'd need to update the Dockerfile and install the utility on the macos builders. I'll do it depending on what Mikhail says below -- it's easier for me since they are sitting a meter away :-).
And, wouldn't it be possible to reuse the existing machinery in llvm/clang premerge checks that runs git-clang-format? I am afraid it can't be reused because it uses different CI pipeline and different agents...
I guess it may be possible. @goncharov Mikhail, if we decide to enable clang-format for libc++, would it be possible to re-enable the clang-format checks? IIRC, I think I had requested to remove them cause they were always broken.
| libcxx/include/variant | ||
|---|---|---|
| 1666 ↗ | (On Diff #308270) | What controls this? I would *love* to move away from declaring the return type of the function above, since it gets lost in our numerous attributes. |
| libcxx/utils/ci/run-buildbot | ||
| 111 | I think it would be reasonable to set this for the whole script. WDYT? | |
| 115 | We should also include libcxxabi. | |
Comment Actions
- Set -o pipefail for the whole script.
- Run git-clang-format also on libcxxabi/.
- Rebase.
| libcxx/include/variant | ||
|---|---|---|
| 1666 ↗ | (On Diff #308270) | I guess the hypothetical AlwaysBreak*Before*ReturnType (there is only AlwaysBreakAfterReturnType currently). That shouldn't be something difficult to implement in clang-format I guess. |
Comment Actions
Rebase to trigger CI after buildbot changes in 202df6870ea2ac883ac64fab8e01c836da86978b ("[libc++] Install clang-format on CI nodes").
Comment Actions
Do we need to discuss the wanted formatting in this patch or on Slack/dev-ml?
| libcxx/.clang-format | ||
|---|---|---|
| 5 ↗ | (On Diff #311515) | Do we need to keep this at Cpp03? Changing this to newer versions of C++ will of course break code like 'std::vector<std::pair<int, int>>`. But this seems to break formatting the C++11 string literal prefixes: u8, u, U. |
Comment Actions
Concerning the style itself, personally I'd go for a style that is as close to vanilla LLVM style as possible (except for things like disabling comment reflowing for tests, but I'd put this specific .clang-format config file into libcxx/test).
For the moment I'm waiting for the buildbots to be ready so that I can finish the build part.
| libcxx/.clang-format | ||
|---|---|---|
| 5 ↗ | (On Diff #311515) | I'd love to remove Cpp03... But clang in c++03 mode gives error: error: a space is required between consecutive right angle brackets (use '> >'). Ref: https://clang.llvm.org/docs/ClangFormatStyleOptions.html |
| libcxx/.clang-format | ||
|---|---|---|
| 5 ↗ | (On Diff #311515) | True for C++98 the right angle brackets need spaces. AFAIK it's not possible to force that when using a newer C++ version. But C++03 will add a space when using u8"foo" and turn it into u8 "foo". This gives issue with C++11. C++14 code like 1'000'000 will also fail. So maybe we should switch to the newest mode and not format everything, but format per patch basis? |
| libcxx/utils/ci/buildkite-pipeline.yml | ||
|---|---|---|
| 18 | To unblock this and get started even without having entirely settled all formatting questions, I suggest we make this step a soft-fail: soft_fail:
- exit_status: 1(described at https://buildkite.com/docs/pipelines/command-step#soft-fail-attributes) Then, we'll be able to play around a bit with the configuration. Once we've settled on something, we can remove the soft-fail on the step to enforce formatting for new commits. | |
Comment Actions
Add soft_fail. Revert .clang-format and generate_feature_test_macro_components.py changes.
Comment Actions
It would be cool if Buildkite -> Phabricator bridge handled the soft fails.
See previous version https://reviews.llvm.org/D92229?id=320075. It shows CI passed, no warning or whatever, even though Format (soft-) failed, https://buildkite.com/llvm-project/libcxx-ci/builds/1171.
Anyone knows where the code responsible for this is?
Comment Actions
@goncharov might know.
This LGTM, I think this is a good start. We can start running that manually before we commit for now, to see what things look like. Once we're comfortable, we can move to the next level and make it mandatory.
To unblock this and get started even without having entirely settled all formatting questions, I suggest we make this step a soft-fail:
soft_fail: - exit_status: 1(described at https://buildkite.com/docs/pipelines/command-step#soft-fail-attributes)
Then, we'll be able to play around a bit with the configuration. Once we've settled on something, we can remove the soft-fail on the step to enforce formatting for new commits.