Dealii: Are we happy with clang-format?

Created on 28 May 2018  路  26Comments  路  Source: dealii/dealii

So now that we've been able to look at code for a few days, what do people think? What do we need to tweak?

I have to say that there are a number of things that I really don't like. Here are a couple of things:

1. This kind of code:

                    cell_matrix_lhs(k, l) +=
                      (-i * fe_values.shape_value(k, q_index) *
                         fe_values.shape_value(l, q_index) +
                       time_step / 4 * fe_values.shape_grad(k, q_index) *
                         fe_values.shape_grad(l, q_index) +
                       time_step / 2 * potential_values[q_index] *
                         fe_values.shape_value(k, q_index) *
                         fe_values.shape_value(l, q_index)) *
                      fe_values.JxW(q_index);

I had originally written this as follows:

cell_matrix_lhs(k, l) += (-i * 
                          fe_values.shape_value(k, q_index) *
                          fe_values.shape_value(l, q_index) 
                          +
                          time_step / 4 * 
                          fe_values.shape_grad(k, q_index) *
                          fe_values.shape_grad(l, q_index) 
                          +
                          time_step / 2 * 
                          potential_values[q_index] *
                          fe_values.shape_value(k, q_index) *
                          fe_values.shape_value(l, q_index)) 
                         *
                         fe_values.JxW(q_index);

I think most will agree that this is vastly easier to read. In fact, the clang-format version above is, with due respect, unreadable.

2. This list of member variables

    Triangulation<dim> triangulation;
    FE_Q<dim>          fe;
    DoFHandler<dim>    dof_handler;

    ConstraintMatrix constraints;

    SparsityPattern                    sparsity_pattern;
    SparseMatrix<std::complex<double>> system_matrix;
    SparseMatrix<std::complex<double>> rhs_matrix;

    Vector<std::complex<double>> solution;
    Vector<std::complex<double>> old_solution;
    Vector<std::complex<double>> system_rhs;

    double       time;
    double       time_step;
    unsigned int timestep_number;

    double kappa;

I had them all nicely aligned, but clang seems to only want to align blocks that are not separated by an empty line.

3. This kind of inconsistency
From step-6:

 VectorTools::interpolate_boundary_values(
    dof_handler, 0, Functions::ZeroFunction<dim>(), constraints);
  [...]
  DoFTools::make_sparsity_pattern(dof_handler,
                                  dsp,
                                  constraints,
                                  /*keep_constrained_dofs = */ false);

I find the first form not very readable. And I don't know why it doesn't do what the second call does.

I'll hold my tongue a bit, but looking around I'm not too convinced that we really made progress in this switch.

Discussion

Most helpful comment

@bangerth Allow me to share my opinion on this very topic please.

1/ I am a bit upset about having a "Are we happy with clang-format" discussion after the switch. (I was suprised that a majority of developers actually wanted to change without me lobbying for it and my first reaction was that we should got with astyle to avoid this kind of train wreck we have right now.) In retrospect we should have taken another iteration of looking at the actual output of clang-format before doing the switch.

Further, I have the feeling that we are mixing two discussion here. One regarding the current output clang-format produces (and possible improvements), and the other rergarding the question whether we want to have a tool such as clang-format at all.

Behavior that we can fix by tweaking the configuration isn't an exactly good argument against clang-format.

2/ Regarding the technical merits (and to answer your questions): No, I do not see an alternative technical solution to clang-format that would produce results even close to the quality that clang-format delivers (see point 4/). If clang-format would only be an epsilon improvement over astyle (or other more heuristic indenter) I wouldn't have supported, or recommended a change. And I think no one else would have. We haven't done any of the past changes lightly (there is google projects, cmake, git, github, development model, etc.) - and I think we have been right in almost all of them :-)

I read from your comment that you are questioning the benefits. I will not reiterate the technical side again - I just want to point that in my opinion clang-format does improve readibility (and not just by an epsilon). Uniform indentation is a very important aspect of code readibility... But to give you another example regarding teaching: I require all students (if it is a project in C++) to indent their code with clang-format. For two reasons:

  • they get their code indented (nearly) perfectly automatically and such restructuring helps a lot in understanding control flow, further, it is also a lesson in conforming to programming best principles,
  • it makes code easier to read for others (myself included).

Having the same tool upstream in deal.II is actually fantastic for that - I can teach how to indent the blessed way (our way).

3/ We have spent a significant amount of time and resources on providing clang-format support and setting up the infrastructure. I thus find your suggestion of reverting back to astyle for the code base upsetting. I would suggest we go a much more constructive route and identify key problems with the current behavior and solutions for that. I will open a PR in a minute that shows the difference between the old indentation of example steps and the (revised) new one in one clear commit. I am sure that we can tweak the current behavior such that the output we get from clang-format is almost as good as completely setting by hand and much more to your liking.

4/ ... to be continued ...

All 26 comments

Frankly, I find 1 and 2 to be nitpicks and I actually ok with the formatting as of now.

Are we happy with clang-format?

I am happy that we switched for the reasons summarized in the original thread.

I'll add what I find particularly aggravating about the first of these issues: For years, I've been explaining to people that writing the bilinear form in deal.II is not substantially more difficult than writing it on a piece of paper (or in FEniCS, for that matter) because it's really just a 1:1 translation of the mathematical formula. And that case is easy to make if the bilinear form looks like this (http://dealii.org/9.0.0/doxygen/deal.II/step_20.html#MixedLaplaceProblemassemble_system):

              local_matrix(i,j) += (phi_i_u * k_inverse_values[q] * phi_j_u
                                    - div_phi_i_u * phi_j_p
                                    - phi_i_p * div_phi_j_u)
                                   * fe_values.JxW(q);

But you can't really make this point any more when it looks like this (http://dealii.org/developer/doxygen/deal.II/step_20.html#MixedLaplaceProblemassemble_system):

              local_matrix(i, j) +=
                (phi_i_u * k_inverse_values[q] * phi_j_u -
                 div_phi_i_u * phi_j_p - phi_i_p * div_phi_j_u) *
                fe_values.JxW(q);

That's unreadable, and looks completely unrelated to the mathematical formulation. I'm really unhappy about this.

(I agree that we can argue about 2 and 3 -- that's just personal preference and my OCD wanting things to look the same.)

That's unreadable, and looks completely unrelated to the mathematical formulation. I'm really unhappy about this.

I would be dare to say that this is a very personal opinion, @bangerth . To me both are almost as good and certanly

completely unrelated

is a stretch here. I do see why you prefer the former, though.

@bangerth for such special cases like 1/ it is simply best to instruct clang to not indent that block of code:

/* clang-format off */
cell_matrix_lhs(k, l) += (-i * 
                          fe_values.shape_value(k, q_index) *
                          fe_values.shape_value(l, q_index) 
                          +
                          time_step / 4 * 
                          fe_values.shape_grad(k, q_index) *
                          fe_values.shape_grad(l, q_index) 
                          +
                          time_step / 2 * 
                          potential_values[q_index] *
                          fe_values.shape_value(k, q_index) *
                          fe_values.shape_value(l, q_index)) 
                         *
                         fe_values.JxW(q_index);
/* clang-format on */

There is absolutely no way how you can teach an AST pretty printer to get that particular piece of code indented the way you want.

Edit: I absolutely agree that bilinear forms or other complicated formulas have to be indented by hand. (I do that in my own projects.)

Regarding 3/. Clang format is not inconsistent here. We simply allow it to put parameters either all in one line, or all on a separate on ("bin packing"). I actually like that behavior (and prefer it over always going with parameters on one line.)

On 05/28/2018 04:47 PM, Matthias Maier wrote:

|There is absolutely no way how you can teach an AST pretty printer to get
that particular piece of code indented the way you wantd. |

I was afraid that you'd say that. I see you shrug your shoulders, and I
understand that this is so from a technical perspective. But the question I'd
like to ask is this:

* Is this the behavior that we want? *

I know that it's too late to have this kind of discussion now. But it's still
true that the purpose of indenting is to enhance readability. If it fails to
do that, then we have a problem. I can live with some of this if it happens to
be inside our library, but the example I showed was in one of our teaching
tools (the tutorial). We really ought to make sure that these are as easy to
understand as we can.

There is absolutely no way how you can teach an AST pretty printer to get that particular piece of code indented the way you want.

I agree with @bangerth that there are places where we do not want to have automatic change in breaking the lines for the case where a user has made a concious decision. For example, the examples folder should get the automatic wrap disabled in my opinion. One might maybe run it through manually when first writing a tutorial, but clang should not interfere when a human has done something on purpose.

An to add on that, I do not consider adding /* clang-format on/off */ an acceptable solution for our tutorials.

@kronbichler The problem is that clang-format does not support the kind of operation you have in mind. It is basically a sophisticated pretty printer for an abstract syntax tree. As such, it has no information about the original layout - it can only print source code with a given column limit in mind. (Actually, it is a bit more complicated - you can define custom penalty weights for how clang-format should operate.) To make it short - there is no such thing as disabling line-wrap.

@kronbichler @bangerth To summarize - what you two basically are saying is that you do not want to have a full text reformatting program like clang-format. If so, we should disable it for the examples.

If allowed, I would like to disagree why /* clang format on/off */ is not an acceptable solution. But maybe that's just my taste.

I agree with @kronbichler: I think it would be reasonable to just manually indent the examples (though we will have to add that as a release step).

To answer the original question: I am quite happy with clang-format because we are now guaranteed uniform and sane formatting across the code base. Its not always perfect (unaligned &s in function declarations is annoying, but I believe that bug will be fixed eventually)

@bangerth To be very clear at one point here:

I see you shrug your shoulders, [...]

No, I am not. I actually fully agree with your philosophy here. All I am saying is that the only thing we can do to get the expected behavior here is to not use clang-format for that particular snippet of code. We can do that by either using /* clang-format off/on */, or disabling clang-format for the examples.

Edit: I also agree that we should have looked a bit closer at the actual formatting result before the actual switch. :-(

Sometimes I need to sleep over things to get them into coherent and impassionate form. Let me also start by recognizing that me bringing this issue up again is aggravating and may come over as either sour grapes or as wanting to revisit a discussion where the majority felt differently than I did. That's not my goal, but to explain this I'll need a bit of space -- read on if you have a minute (or ten -- it's going to take a while).

Where I'm coming from is this: Collectively, we've spent an enormous amount of work on our documentation and teaching materials, and the tutorial is sort of the pinnacle of it. The .cc files alone have 65k lines of code, equivalent to around 3 man years of work. When I go to conferences or give seminars and talk to people who use deal.II -- which kind of happens a lot --, the thing I always hear is how much they appreciate all of the work that has gone into the documentation. Their number one criterion why they chose deal.II over the other libraries is the documentation, not the technical correctness or the long list of features. If I talk to them about what else they have looked at and why they're not using it any more or how they think about it, the number one complaint I hear is that library X is not well documented. That's something I think we can be proud of, but also be careful with.

Now, specifically about the tutorial: Whenever I teach my deal.II classes at home, or short courses elsewhere, I spend a lot of time looking over students' shoulders (literally and figuratively) trying to understand how they learn, what they understand and what not, and if they don't then to understand why not. I walk around with a little notebook where I write down what doesn't work, and then in the evening or the following weeks, I write patches to improve this. I'm sure I'm not alone in this practice of trying to figure out how to teach our software better.

One of the things that's common is that basically none of our new users are good at C++. They have a lot of trouble with syntax, and things like typename A::B, for(auto &) or [](){...} is difficult for them. Also just the general flow of a program. So over the years, our tutorial programs have changed substantially. They used to be a lot more diverse, but today from step-3 to step-60, they basically all follow the same principle with one main class and the same (+/-) 5 basic functions. That's not an accident. (It's also why I'm so unhappy with step-13 and step-14.) It's the result of careful observation, and why I spend time on things such as #6649, #6645, #6640, #6619, and a million other patches. My goal is to make these tutorials easier to read and understand, in response to seeing how students learn. I know some of you have done the same.

So issues like the one with the bilinear form discussed in a previous comment greatly bother me. It's not about a personal preference. It's about the ability of a student (or an instructor who demonstrates these things in a class) to show the formula and the code side-by-side and say: "Look, they really look the same", and the student immediately being able to see what the code does. To make this possible, the author of the code needs to have the ability to support this kind of learning, by grouping terms appropriately in a way that supports understanding. An answer of the form "Well, I guess we can't do that any more with the current tool" is greatly frustrating to me as a teacher. It's like a publishing company wanting to switch to a text processing tool that can no longer do paragraph breaks and just shows a wall of text -- not helpful to the educational mission. Of course, I and all of you can read the code even if the terms all appear on the same line; but we're not the audience of the tutorial, and everyone who has taught a course on deal.II knows that the audience can not read such code. They can barely read any C++ to begin with.

I've only talked about the tutorial so far because I really think this is the most critical part. I'm not so concerned about the rest of the code base, but I do think it's worthwhile discussing why we do automatic indentation to begin with. That's where the rest of the code base comes back in. To me, it's about enhancing readability. It is true that consistency is a big part of this (both as far as indentation is concerned, but also naming conventions, parallel structure of codes, common layout of classes and functions, etc.). But consistency by itself is not a useful approach: Using s/^ *//g or even s/\n/ /g is also consistent, but it does not help readability. And we recognized this and had a long discussion about which clang-format options we do and do not want. I just wanted to point out that consistency is a goal towards readability, not a goal in itself.

So when we discuss using one tool or another, I'm not so much interested in whether it produces more or less consistent indentations. I'm interested in whether it enhances readability. @masterleinad in https://github.com/dealii/dealii/issues/6597#issuecomment-389454763 pointed out a number of reasons why he liked clang-format, and they are good ones: #4936/#4555 and the wonky indentation astyle produced in #6550 are good examples. As is of course astyle's complete inability to indent lambdas correctly (at least in 2.04). It's interesting to note, though, that at least some of the examples people have given can also be done through other means. For example, #4936/#4555 were done by script. So were #6661 and #6607. Running a script doesn't fix the issue for code added later, but it covers 95% of the code base for a good long time, and if someone gets aggravated enough, they can always re-run a script. We're also pretty good at incorporating these kinds of style changes into our reviewing practices -- so new code often follows our current style.

But that's a digression. The question that's worth asking is whether clang-format is the right tool to achieve the goals we may have. It's probably worth listing them:

  • Readability is my top goal, of course.
  • Integration into IDEs. This has the advantage that you get to immediately see how it looks, don't get surprised by what make indent yields, and in fact may not have to run make indent at all. So less work, fewer surprises.
  • Automate more aspects of our coding practices. A tool like clang-format allows us to specify more pieces of the indentation puzzle and that makes current code more consistent and also covers future code. (Astyle incidentally can also do more than we've been using in the past, see #6607.)
  • Maybe there are more -- I'd appreciate your perspectives on this.

I don't have technical superiority on this list -- I geek out just as much as anyone else when it comes to things like having a compiler indent my code. But in the end, how a tool achieves the goals is not that important to me -- I care about how well it does it.

So how did the switch to clang-format work out in this regard? I think it clearly addresses the second and third point much better than what we had before. I haven't tried to integrate clang-format into eclipse yet, but I have no doubt that it can be done and will make my life easier.

About the readability, I'm not so sure yet. I think that there are clearly some issues where it is better than previous astyle. I've spent an hour or so, gone through the first several tutorial programs, and looked the changes #6667 makes to them. You'll get my discussion of some prototypical examples next. Most of the changes are midly positive (e.g., adding or removing spaces between function name and argument list): There is no clear better or worse as far as readability is concerned, but they are nice because they improve consistency. Some are very clearly improvements. Some are not. There are many cases where I'm not convinced we really improve consistency; right-align of references is, for example, pretty completely broken.

In the end, almost all of the examples where I think the end result is worse than it was before is where someone clearly understood how a particular piece of code is going to be easiest to read. The bilinear forms are examples, but there are many other examples -- in particular where someone has purposefully grouped function arguments based on their logical connection. The issue, of course, is that an AST pretty printer is not ever going to be able to understand this. It takes a human to understand how humans think, and it takes a flexibility with rules to figure out what works best.

So where do we go from here? I'm not sure what to suggest. Typically, the improvements I find are minor (the authors got the major things right automatically) whereas the cases I don't like are frequently substantially worse. What's particularly awkward is that the changes are irreversible: no future version of clang-format will ever be able to recover the human-arranged statements we used to have. So if we don't back out things basically right now, we will have to live with this to the indefinite future.

Someone made the suggestion to just not apply the indentation rules to the tutorials. I suppose we have to apply some rules periodically at least, and that would then likely involve astyle in whatever version. It seems silly, though, to use two different tools for two parts of the code base. We'd also have to come to an agreement on this soon because we'd have to back out at least those changes that are controversial, before we pile on more patches.

Another option is that we just back out the whole change, and do so relatively soon before it all is buried under current development. I'm not sure I like this either, though I suspect that with a little bit of ingenuity, we can rescue a large part of the changes -- for example, it should not be very difficult to consistently do function_name( instead of function_name ( based on some simple script, and this probably already accounts for half of the entire patch.

In the end, I'm a pragmatist. I'd like it if we could have another discussion about this issue that is focused not on how cool a tool is or what it could do for us in supporting future language features, but very concretely (i) what goals we have for such a tool right now, (ii) what benefit it currently provides us with that we can't achieve with other means, (iii) how these benefits compare against the points I hope I have explained well enough above. I am sorry to raise all of these only now. I wished I had been able to articulate them better in the original discussion.

The marked-up changes I mention above are at https://github.com/dealii/dealii/pull/6667#pullrequestreview-123838091 .

@bangerth the majority voted in favor of clang-format, i understand that we have some degradation of formatting now, but I also think that it's un-fair to revert the vote and un-do the patch. IMO the right approach is to find solution for those rare occasions when the new indentation is much worse.

I, for one, do not agree with

Typically, the improvements I find are minor

statement. The re-flow of comments to fit 80 char width is a big one for me. There are other things that I like, i.e. sorting of headers. And I would guess other folks who voted in favour of clang-format have their own pros.

Also if you look at the table then there are like 6 (!) options where we unanimously chose one parameter over the other. I think that tells that there are positive sides and in general our preferences, as a group, in indentation are similar.

I suppose we have to apply some rules periodically at least, and that would then likely involve astyle in whatever version.

not necessarily. Just keep /* clang-format off */ in few places and as part of the release -- remove these lines. That is a solution. Another solution was suggested by @tamiko .

Bottom line is -- let's look for the solution and go on with new tools as opposed to trying to revert the whole discussion, vote and everyone's time to get back to astyle because of some issues with clang-format. Nothing is ever perfect, but we can always find a compromise or a workaround.

p.s. At the end of the day, all those things are very personal. We all agree that tutorials are important, but if you would ask everyone to assign some number on how important indentation of bilinear forms is, I am sure those numbers would be different.

@bangerth Allow me to share my opinion on this very topic please.

1/ I am a bit upset about having a "Are we happy with clang-format" discussion after the switch. (I was suprised that a majority of developers actually wanted to change without me lobbying for it and my first reaction was that we should got with astyle to avoid this kind of train wreck we have right now.) In retrospect we should have taken another iteration of looking at the actual output of clang-format before doing the switch.

Further, I have the feeling that we are mixing two discussion here. One regarding the current output clang-format produces (and possible improvements), and the other rergarding the question whether we want to have a tool such as clang-format at all.

Behavior that we can fix by tweaking the configuration isn't an exactly good argument against clang-format.

2/ Regarding the technical merits (and to answer your questions): No, I do not see an alternative technical solution to clang-format that would produce results even close to the quality that clang-format delivers (see point 4/). If clang-format would only be an epsilon improvement over astyle (or other more heuristic indenter) I wouldn't have supported, or recommended a change. And I think no one else would have. We haven't done any of the past changes lightly (there is google projects, cmake, git, github, development model, etc.) - and I think we have been right in almost all of them :-)

I read from your comment that you are questioning the benefits. I will not reiterate the technical side again - I just want to point that in my opinion clang-format does improve readibility (and not just by an epsilon). Uniform indentation is a very important aspect of code readibility... But to give you another example regarding teaching: I require all students (if it is a project in C++) to indent their code with clang-format. For two reasons:

  • they get their code indented (nearly) perfectly automatically and such restructuring helps a lot in understanding control flow, further, it is also a lesson in conforming to programming best principles,
  • it makes code easier to read for others (myself included).

Having the same tool upstream in deal.II is actually fantastic for that - I can teach how to indent the blessed way (our way).

3/ We have spent a significant amount of time and resources on providing clang-format support and setting up the infrastructure. I thus find your suggestion of reverting back to astyle for the code base upsetting. I would suggest we go a much more constructive route and identify key problems with the current behavior and solutions for that. I will open a PR in a minute that shows the difference between the old indentation of example steps and the (revised) new one in one clear commit. I am sure that we can tweak the current behavior such that the output we get from clang-format is almost as good as completely setting by hand and much more to your liking.

4/ ... to be continued ...

With slightly revised clang-format options

Edit: Thinking about it - we just take the time to go over all example steps again and update them to C++11 best practices anyway. I think this will improve readibility quite a bit and naturally removes a lot of awkward corner cases. Especially after some minor tweaks made it to master, #6696.

Further, I have the feeling that we are mixing two discussion here. One regarding the current output clang-format produces (and possible improvements), and the other rergarding the question whether we want to have a tool such as clang-format at all.

Since I'm also one of the more sceptical people, I just wanted to assert that I believe that the switch to clang-format was right due to the technical reasons regarding uniformity (even though I do not care about the technical details in themselves). So my objections were directed to what we can do technically to improve the situation.

@bangerth I read what you wrote several times. I am grateful that you were willing to lay out this discussion in a clear way.

My understanding is that there are two independent issues to address: whether or not we should use any tool to indent the examples (1) and whether or not we should use clang-format (2).

1/ I had not considered the perspective of someone who is a complete novice when I voted for the changes. If I have understood what you wrote correctly, the primary concern is that clang-format is (tautologically) never as good as someone indenting code and that good style matters a lot with newbies.

I think that the solution here is clear. Aside from step-60, I found three commits to the examples from non-principal developers this year: one (0d1af5b46f) fixed a bug, two fixed typos. I believe it is reasonable to completely disable (or, minimally, just delete trailing whitespace with sed) all automatic code formatting for the examples simply because we can do it better ourselves (and principal developers, who are de facto the only ones contributing code to the examples, are not going to commit poorly indented code).

Hence: I propose that we disable all (astyle, clang-format, etc) automatic indentation tools for the examples. We can do it better on our own and they change so infrequently that this is a very minor maintenance burden (especially since they have been run through clang-format recently).

2/ I believe everyone agrees that we need to use some tool to indent the library itself. Using an indentation tool means living with some warts and bugs of the tool itself. I agree completely with @tamiko that clang-format is substantially better than astyle: i.e., for every wart in clang-format, there is a much larger wart in astyle. Its not a monotonic improvement: some things, like the unaligned & or * in function declarations, are annoying, but they are significantly outweighed by the massive improvements that this tool offers (uniform function calls, uniform line length, uniform spacing around operators, etc. etc. etc.). I simply do not see any compelling reason to use any other tool for this job: based on the clang-format vote this is, I believe, the majority opinion of the principal developers.

I am not as concerned with novice-friendliness of indentation here since we do not write down many bilinear forms in the library itself, and also that anyone who wants to contribute code to the library has probably already worked through the tutorials and can handle less beautiful indentation.

I more or less agree with everything @tamiko said. I tried multiple times to get more feedback on things we need to improve before we actually use clang-format and was surprised that there were few responses regarding issues we urgently need to address. In the end, what happened was actually exactly what I feared: We only looked closely at the changes only after we merged. That said, it is understandable: I judge tools best when I work with them. In my opinion, it is a good approach to gather all changes we want to do now in one place. @tamiko already opened #6696 for that.
Maybe, it's a good idea to just try the suggestions in that PR for any larger PR you might have laying around so we can get a good feeling for the formatting impact.

Regarding the examples, I am totally fine with just disabling clang-format. If we decide to try our best to get the examples in a form suitable to be formatted by clang-format, I will help of course.

Alright, it seems everyone who wanted to contribute to the discussion has done so. I'd like to add that (i) I blame nobody other than myself for not looking at the changes in more detail before they were merged, (ii) the general use of an indentation tool is not in question. My dissatisfaction was simply with the fact that I learned from looking at the changes that humans are often quite good at splitting things onto different lines to indicate grouping, purpose, and intent -- and that a tool that first converts everything into an AST and then prints that again will generally not respect that human hand.

Either way, it is what it is. Let's move on. What we have to decide is this:

  • Do we want to exempt the tutorials from automatic indentation? (Vote thumbs up)
  • Do we want to accept the current state and manually touch up places we think are worth touching up. (Vote thumbs down)

I'll post a follow-up patch in a second that tries to isolate the places that are potentially controversial (with moderate success). It gives you an idea what would be involved in either of these options.

@bangerth I still believe that

  • clang-format generally did a good job of improving the readability of all example steps
  • Most of the points you critized are resolved by #6696
  • It would be best to manually set bilinear forms (and other critical code segments) via // at the end.
  • The example steps would improve dramatically if we would spend the time and actually port them to proper C++11 (instead of working around ancient cludges that were necessary for C++98). While doing that, the previous point would not take much time at all. (I am willing to spend some time doing that.)
  • We are making a grave mistake disabling clang-format for example steps [1].

Further, I disagree with the statement that the example steps where better indented by hand + astyle.

[1] When users start to work with an example step, shall I teach them to us astyle for that - because clang-format was so terrible indenting the examples? Or shall I teach them to use clang-format and answering the question why significant portions of the code changed by "The guys who wrote the examples didn't like clang-format"?

Edit: My apologies, the last paragraph is worded much stronger than it should be.

I think we resolved that using clang-format for the tutorials is probably the way to go. (That may not have been my first choice, but that's where we are, and I accept the majority's decision.) I'd just like to find a way to make them more readable. #6696 is probably useful in this regard. #6714 is not useful except to figure out how much code one would have to look through.

I entirely agree that it would be totally worthwhile to update many of the tutorials. It's just a humongous amount of work: as mentioned, the tutorials together have 35,000 lines of non-comment lines (though adjusting a line of code often also necessitates at least reading the commentary around it). I appreciate your offer to go through some of the steps -- but overall it's going to be a lot of work that we somehow have to divvy up.

@bangerth What about we sleep at least a night over that before jumping to conclusions? :smile: (The last time I counted we also had more than three developers.)

Sorry again for the unnecessary harsh words.

Yes, updating the examples is a great amount of work. But we also have a great number of developers with a bit of an OCD obsession out here and by splitting the work we can get it done relatively quickly. I also agree that we should find some clear rules beforehand so that we do not make the example steps inconsistent.

I think that I am now caught up with the discussions on this topic that occurred while I was away.

Since #6774 exists, does this mean that the consensus is to keep clang-format for the examples and append // to lines that should not be changed? If so, shall we close this? I, of course, volunteer to reindent some of the examples.

Yes, I think that's what we agreed. I'm not a fan of blank // statements, but we can use them to attach useful things at the end of the line.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

adamqc picture adamqc  路  3Comments

bangerth picture bangerth  路  6Comments

tamiko picture tamiko  路  5Comments

davydden picture davydden  路  7Comments

jppelteret picture jppelteret  路  6Comments