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 <[email protected]> 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 -- [email protected] To unsubscribe send an email to [email protected] %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
