Use '>>' instead of '> >' to close nested templates. #420
Conversation
Codecov Report
@@ Coverage Diff @@
## main #420 +/- ##
=======================================
Coverage 99.50% 99.50%
=======================================
Files 86 86
Lines 5038 5038
=======================================
Hits 5013 5013
Misses 25 25
Continue to review full report at Codecov.
|
|
|
So done! |
| std::vector<Point<2> > | ||
| Parameters::get_vector(const std::string &name) | ||
| std::vector<Point<2>> | ||
| Parameters::get_vector(const std::string &name) |
There was a problem hiding this comment.
What is going on here? Is this caused by astyle?
There was a problem hiding this comment.
Yes, we have these places in ASPECT as well. astyle thinks that the >> is a right-shift operator and indents accordingly. That's because astyle is a pre-C++11 tool.
I'm working on a patch for ASPECT to make astyle work with this more gracefully, but for the moment you'd have to live with this.
There was a problem hiding this comment.
Ok, my main goal with the current styling is to remain in line with aspect, so if geodynamics/aspect#4562 is merged, I will be happy to merge this as well.
Would it not be easier to just update to Astyle 3.1, since that supports c++11 (http://astyle.sourceforge.net/news.html), although I didn't test whether it does this correctly.
There was a problem hiding this comment.
We tried this out a while ago and it led to many other changes. I think that using astyle 3.1 would probably be the right choice, but it means that everyone who is working on ASPECT or WorldBuilder would have to update their local version of astyle to continue working. It's a hassle. It might be interesting to know how extensive the changes would be, and whether that would lead to benefits worth the pain.
There was a problem hiding this comment.
We tried this out a while ago and it led to many other changes.
Do you remember if it was just different or if there was stuff which you disagreed with and would require a lot of tweaking?
I think that using astyle 3.1 would probably be the right choice, but it means that everyone who is working on ASPECT or WorldBuilder would have to update their local version of astyle to continue working.
I personally do not really see this as a major issue. You already have to download a manual zip for every install, now you need a different one, once per machine if you develop on that machine. So it is something all developers already know how to do. It may make it even easier since 3.1 is the latest version since 2018, this is the one which is in all package managers. So it would actually make installation much easier (until 3.2 or 4 comes out of course, but their update schedule doesn't seems to be very high frequency).
I think we discussed that after the next aspect release we may do another major version increase. That would be a good moment to do such a switch if we would want to.
I can maybe try tonight to use astyle 3.1 for both the world builder and aspect and make a pull request out of it, so we can discuss the differences (that should be 10 minutes work I think). Would that be useful?
There was a problem hiding this comment.
I don't recall that there were changes in indentation I actually disagreed with. We tried this with deal.II first, but in the end decided that it was just too much to ask everyone to upgrade for a relatively small benefit. Of course, there is a much larger developer base for deal.II than for ASPECT or WorldBuilder, and consequently the number of people who would have been impacted is also much larger.
One of the major issues is that when you make such a switch, every pending patch becomes invalid because if you merge it, the combined version no longer satisfies the indentation check in the CI. So you can't merge any patches that originated from before the switch. That, too, is very painful if you're on a project like deal.II that merges ~10 patches per day, many of which are rooted in a commit that predates the switch.
I think it would be much easier to do this for ASPECT than for deal.II, but it still comes with substantial pain in view of the fact that we have ~85 pending patches.
There was a problem hiding this comment.
hmm, I don't see why any patch would immediately become invalid, unless the differences between the different astyle versions are affecting a very large percentage of the line numbers, since otherwise it is just a patch as any which may or may not have some merge conflict which needs to be resolved. Am I missing something here or are the difference that big?
I can see a problem if deal.ii aspect and world builder are not using the same version of astyle though, for the people who are developing more than one of those three, because you would have to point to the right astyle for each project. But if I understand correctly, deal.ii is now use clang, so that leaves aspect and the gwb to make their own decision. Like I said before, the gwb will follow what aspect does, but since I was interested in what the difference was, I made pull request #421, and the amount of line changes for the gwb doesn't seem that big. That may be different for aspect of course.
There was a problem hiding this comment.
Think about this case:
- Commit A uses astyle 2.04
- You build a branch B on commit A where you implement a new feature. It uses astyle 2.04 for indentation, and you need to use 2.04 because otherwise your branch would contain a lot of unrelated changes.
- The project switches over to astyle 3.1. It commits patches that change the entire code base over to 3.1.
- Because this changes essentially the entire code base, your branch can no longer be merged easily because there are merge conflicts all over the place.
There is a variation where the switch from astyle 2.04 to 3.1 does not change nearly everything:
- Your patch then still applies without running into merge conflicts.
- So it gets merged. But there are parts of your code base that were developed with astyle 2.04 rather than 3.1, and so the indentation of the merged code does not conform to the style the project wants.
- This means that after the merge, the
mainbranch no longer passes the indentation check. As a consequence, everyone who then takesmain, changes a couple of lines somewhere, and then creates a pull request, will then be tripped up by the CI check testing the indentation: The indentation check fails, but not because of changes in the actual PR, but in the code that was merged previously.
I hope you see that that creates issues. None that aren't insurmountable, but it's not easy to deal with this in practice if indentation is one of the CI checks.
There was a problem hiding this comment.
Think about this case:
* Commit A uses astyle 2.04 * You build a branch B on commit A where you implement a new feature. It uses astyle 2.04 for indentation, and you need to use 2.04 because otherwise your branch would contain a _lot_ of unrelated changes. * The project switches over to astyle 3.1. It commits patches that change the entire code base over to 3.1. * Because this changes essentially the entire code base, your branch can no longer be merged easily because there are merge conflicts all over the place.
Yes, I agree that if the indentation change would change nearly every line, it would be a significant issue. You would definitely need to rebase and you will get a lot of merge conflicts. Although this would only be for source code related pull request, any documentation pull requests or adding/changing of tests pull request would not be affected.
There is a variation where the switch from astyle 2.04 to 3.1 does not change nearly everything:
* Your patch then still applies without running into merge conflicts. * So it gets merged. But there are parts of your code base that were developed with astyle 2.04 rather than 3.1, and so the indentation of the merged code does _not_ conform to the style the project wants. * This means that after the merge, the `main` branch no longer passes the indentation check. As a consequence, everyone who then takes `main`, changes a couple of lines somewhere, and then creates a pull request, will then be tripped up by the CI check testing the indentation: The indentation check fails, but not because of changes in the actual PR, but in the code that was merged previously.
At the moment you change to 3.1, your tester also changes to 3.1. A rebase to the newest master is unlikely to give merge conflicts in this case. The rebased code then is run against the 3.1 tester and either passes or fails (prompting the the user to switch to astyle 3.1). I can also see many kinds of pull request which won't even need to be rebased. For example, like in the above case, documentation or adding/changing tests pull request, or pull request which just changes something small in unaffected lines.
Furthermore, github actually checks every time main changes whether each pull request can be merged without conflicts. The thing the testers use is (as far as I know) a merged version with the current main. So if there are no merge conflicts, just restarting all testers once before merging the pull request should show all potential issues (that is, the indent tester will fail and prompt users to indent their code correctly). This will most likely already happen because users add commits, the tester will be rerun.
I hope you see that that creates issues. None that aren't insurmountable, but it's not easy to deal with this in practice if indentation is one of the CI checks.
Yes, I agree that there will be issues, and I am probably a bit too optimistic due to how smooth (as far as I know) the change from master to main went, which in my mind is a much more challenging change, but in my mind the worst thing which can happen is that we merge something which is not properly indented according to astyle 3.1. For the world builder we will know when that happens in about 30s after the merge and for aspect in about 3 minutes after the merge.
I just want to say that I am not advocating to do this now, but right when we start with the next major release of aspect may be a good time. The world builder currently has only 13 open pull request, so I am sure that can be managed at any point :)
On the topic of this pull request, I saw in geodynamics/aspect#4562 that you said that you have a patch which cleans this up. Would it make sense to add that to this pull request as well before merging, or did I misunderstand that?
There was a problem hiding this comment.
Yes, you'd have to rebase every pending patch. (I don't know whether the CI uses the test scripts from the version the patch is based on, or from main. I think it is the former, but the effect is the same: all patches have to be rebased.) For deal.II, at any given time, I would assume that there are 2-300 patches that are either already submitted and pending, or are in current development. It's a major disruption to rebase all of these, at least if a change of astyle/clang-format version touches a lot of lines.
I'd be curious how many lines in ASPECT change if one upgrades astyle? deal.II no longer uses astyle, so the pain in proposing the change for ASPECT would be lower.
The latter form had been necessary before C++11, but has been allowed since C++11.