Mordante marked 8 inline comments as done. Mordante added a comment. Thanks for all review comments! I'm a bit out of time so I will look at the other comments later.
================ Comment at: clang/www/hacking.html:295-296 + directory will cause the update of the diff to start a CI run. This dummy + file will also add the libc++ group to the list of reviewers. The status of + the build will be available in Phabricator.</p> + ---------------- philnik wrote: > aaron.ballman wrote: > > I am guessing the dummy file should then be removed before landing the > > commit and we should document that? > > > > (FWIW, adding a dummy file feels really unclean as a design -- it's > > mysterious behavior for new contributors, which the documentation helps > > with a bit, but also it's a risk for checking in files that are never > > intended to be in the tree. If there's a way to improve this somehow so we > > don't need to trick the precommit CI into running, that would be really > > nice and totally outside of the scope of this review.) > It would be great if just the bootstrapping build could be run on all clang > reviews. That should show most problems with changes in clang. If there are > any problems in libc++, fixing them would result in a full CI run. Having > libc++ run against the patch would probably also be awesome as a smoke test > for any clang changes. +1 to what @ldionne said. One of the advantages would be that it's possible to test Clang against the libc++ code base using different language versions, by defining multiple test runs using different language versions. (Slightly related to https://discourse.llvm.org/t/lit-run-a-run-line-multiple-times-with-different-replacements/64932) @aaron.ballman is looking at extending the Clang pre-commit CI something worth looking into? ================ Comment at: clang/www/hacking.html:300 + CI lacks the resources to build every Clang diff. A single run takes about + one hour. Secondly most changes of Clang won't affect libc++.</a> + ---------------- aaron.ballman wrote: > philnik wrote: > > Having a secondly without a firstly seems weird. > +1 to the "secondly" being a bit weird, but also pushing back a bit on the > assertion that most changes in Clang won't affect libc++: we change the > output of Clang's diagnostics on an almost-daily basis. > > I think it's honest to say: "It is difficult to determine which changes in > Clang will affect libc++ and running the precommit CI in all circumstances is > prohibitively resource intensive given how actively the project is updated." > or something along those lines. I based my statement on the observations I made: - I don't see a lot of Clang changes with a dummy file. - Clang doesn't break libc++ often. (Still it would be good to get the number down). Libc++ uses diagnostics, but mainly to validate diagnostics for ill-formed code. So that probably explains why not every diagnostic is an issue. But changing the diagnostic of something like a `static_assert` will be detected. I've replaced the secondly sentence with `Unfortunately it is difficult to determine which changes in Clang will affect libc++.` I want to keep the other two sentences since it explains why an opt-in is needed. ================ Comment at: clang/www/hacking.html:311-312 + <p>Unlike Clang, libc++ supports multiple versions of Clang. Therefore when a + patch changes the diagnostics it might be required to use a regex in the + "expected" tests to make it pass the CI.</p> + ---------------- philnik wrote: > aaron.ballman wrote: > > Should we document the expectation that breaking libc++ due to conforming > > changes in Clang (in terms of diagnostics and bug fixes, not so much in > > terms of introducing new language extensions) are generally the > > responsibility of libc++ maintainers to address, but Clang contributors > > should attempt to reduce disruptions as much as possible by collaborating > > with libc++ maintainers when this situation comes up? > That's definitely a good idea. Maybe mention that clang contributors should > ping the #libc group to get the attention of libc++ contributors so we can > prepare a follow-up patch or, if the author is comfortable with it, also fix > libc++ in the same patch (and hopefully get fast approval). Most breakages > should be relatively simple to fix. I like the idea to collaborate more. I want to give this some thought and especially how to word it. I want to avoid that it's seen as a carte blanche to commit Clang changes which break libc++. This would mean HEAD is broken and that's a huge issue for downstream users like Google who tend to follow HEAD closely. I would prefer to use commandeering and instead of two patches where one leave HEAD in a broken state. Even if it's short it would make bi-secting harder. Personally I really dislike the idea of having commits that knowingly leave a repository in a broken state. It also doesn't align with the LLVM developer policy https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy ``` As a community, we strongly value having the tip of tree in a good state while allowing rapid iterative development. ``` ================ Comment at: libcxx/docs/Contributing.rst:107 + +The CI tests libc++ for all :ref:`supported platforms <SupportedPlatforms>`. +The build is started for every diff uploaded. A complete CI run takes ---------------- philnik wrote: > aaron.ballman wrote: > > philnik wrote: > > > Not really currently. We still claim FreeBSD support without a CI runner. > > Also, do I remember correctly that Windows testing is not done on a CI > > runner for libc++? > Nope, we have full Windows coverage in the CI. This might have been the case > some time before I started working on libc++, but it's definitely not the > case anymore. > Not really currently. We still claim FreeBSD support without a CI runner. True. But I wonder whether we still need to claim FreeBSD support. I recently pinged their CI patch to get the current state. I feel that this detail is a bit out of the scope of this review. Still I think this is an important issue. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133249/new/ https://reviews.llvm.org/D133249 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
