Hey Matt, I forgot to include a couple of important pieces of information :).
1. There are places that we want to disable clang format. It would be impossible to find these places in one big formatting change. 2. The big change will cause huge headaches merging. The benefits of having a more compliant codebase are probably not worth those headaches. To answer your question, I envision only two small things changing once 49063 is merged. 1. When a reviewer says "that doesn't match the style" the patch submitter can run `clang-format <file>` and say "now it does". There will be nothing to argue about. 2. If someone notices a file that doesn't have the right style, rather than fixing a subset of the file, they can run `clang-format` and fix the whole thing at once without worrying about not following the style guide perfectly. We would *not* enforce the style across the board automatically (at least not for now). We can have a separate discussion on if/when to replace style.py with clang-format. My opinion is that we should do it the same way we run the current checker, which is we only check the lines that have changed. But this is getting ahead of what I'm currently proposing :). Jason On Fri, Aug 13, 2021 at 11:13 AM Matt Sinclair <sincl...@cs.wisc.edu> wrote: > What does this practically mean though, Jason? Does it mean now when > somebody makes the next change to a file foo (after 49063 is checked in), > that all of the formatting and spacing changes will be included in that > patch, since it would need to be run through the checker? Perhaps that's a > short-term problem, but it seems to me like that will lead to some > confusing patches where spacing is ~90% of what is being checked in. In > comparison, I'd rather have 49064 that makes all the spacing changes at > once, but doesn't pollute subsequent patches. Maybe there is a third > option I'm not getting though? > > Matt > > On Fri, Aug 13, 2021 at 12:45 PM Jason Lowe-Power via gem5-dev < > gem5-dev@gem5.org> wrote: > >> Hi all, >> >> I have an updated proposal. >> >> FYI: It looks like since the changes are marked as WIP they don't send >> emails when they are updated. >> >> For those of you who haven't seen the changesets, you can see the >> discussion (and some great ideas from others!) at the following links: >> >> Clang-format file: >> https://gem5-review.googlesource.com/c/public/gem5/+/49063 >> The giant changeset: >> https://gem5-review.googlesource.com/c/public/gem5/+/49064/ >> >> New proposal: >> ------------------- >> >> Daniel had a great suggestion to apply this change "little by little" and >> only apply one formatting rule at a time. Unfortunately, I don't think >> clang-format supports this. However, this fact, combined with the number of >> places where gem5's current style is underspecified got me thinking. >> >> Instead of applying this format for the entire repo at once, I suggest we >> use the clang-format file as the definitive style guide for gem5. Rather >> than humans discussing an underspecified style format, whenever anything >> style related comes up, we can point to clang-format and say "that's the >> way we do it." In other words, let the machine decide. >> >> As previously mentioned, there's lots of places where our current style >> guide is under specified. Currently it states something like "If not >> specified here, we use the Google style". So, the clang-format file I >> created does the *exact same thing*. >> >> My goal is for the clang-format file to implement all of the differences >> from the Google style that we have, and then it falls back on Google for >> everything else. If you notice anywhere that this is wrong, please comment >> on the commit. >> >> So, to summarize: I propose committing 49036 and dropping 49064. Then, >> from now on, if anyone has a style comment on a file or updates the style >> in the file, rather than having a potentially flawed human make the >> changes, we can have a deterministic machine (clang-format, the program) >> make the changes. >> >> Any objections? :) >> >> Cheers, >> Jason >> >> >> On Thu, Aug 5, 2021 at 4:39 PM Jason Lowe-Power <power...@gmail.com> >> wrote: >> >>> Hi everyone, >>> >>> For fun(?), I have come up with a clang-format file that's pretty darn >>> close to our current style. >>> >>> However, the problem is that most of our files don't actually meet our >>> style guide! :'( So, running this clang-format generates an enormous >>> changeset. Skimming through the (100,000+) changes clang-format makes all >>> of them look "correct" to me. But I didn't check them all, just a few >>> spot-checks. >>> >>> I've put together a WIP commit as an example of what it looks like to >>> run clang-format on gem5. There are a few caveats, however. >>> >>> 1. BitUnion really confuses clang-format for two reasons: (1) We indent >>> the code inside the BitUntion macro. (2) there are no semicolons after the >>> macro calls for BitUnion and EndBitUntion. >>> 2. There are a few lines that go over 79 characters, but they're easy to >>> manually fix (it's only 4 files). >>> 3. When I ran style.py on all files in src/ there were a huge number >>> that had invalid sorting of includes. In at least one case, this causes >>> compiler failures when I let style.py fix it. >>> 4. I'm skipping sorting includes with clang-format right now. There is a >>> way to specify the blocks that we define, but I don't want to dive into >>> this right now. I'll create a Jira issue so we can come back to it. >>> >>> Finally, I'm using the following shell script to test things. This runs >>> sed to tell clang-format to ignore the BitUnion blocks, runs clang-format, >>> and then undoes the sed script. >>> >>> If this seems like something we want to continue with, then I'll commit >>> these changes and we can add clang-format to the commit hooks. >>> >>> Clang-format file: >>> https://gem5-review.googlesource.com/c/public/gem5/+/49063 >>> The giant changeset: >>> https://gem5-review.googlesource.com/c/public/gem5/+/49064/ >>> >>> The script I used (don't judge my sed ability :D): >>> ``` >>> # Make clang-format ignore bitunions >>> grep -r -l BitUnion src | xargs sed -i -e '/^ *BitUnion.*(.*)/i \ >>> // clang-format off' -e '/^ *EndBitUnion(.*)/a; \ >>> // clang-format on' >>> >>> # undo the change to the bitunion file >>> git checkout src/base/bitunion.hh >>> >>> # Run clang format >>> find src -name "*.cc" -or -name "*.hh" -exec clang-format -style=file -i >>> {} \; >>> >>> # Remove clang-format off in BitUnion files >>> grep -r -l BitUnion src | xargs sed -i -e '/^ *\/\/ clang-format.*$/d' >>> -e '/^; *$/d' >>> ``` >>> >>> Cheers, >>> Jason >>> >> _______________________________________________ >> gem5-dev mailing list -- gem5-dev@gem5.org >> To unsubscribe send an email to gem5-dev-le...@gem5.org >> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s > >
_______________________________________________ gem5-dev mailing list -- gem5-dev@gem5.org To unsubscribe send an email to gem5-dev-le...@gem5.org %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
