Move AdditionalData outside of class definition

[sebproell]

When one wants to fill the AdditionalData struct of a class (especially solvers), currently one needs to know the VectorType template parameter although it is not necessary for the struct and only for the surrounding solver. This patch suggests a way to move these structure outside and still retain the convenient access for generic code. Instead of ExampleClass::AdditionalData the struct is now called ExampleClassAdditionalData. Due to the using statement, I think this should be backward-compatible (not sure about the binary representation of the struct though)?

I only changed it for GMRES. If we decide to go for such a change I am happy to change it everywhere.

fyi @mschreter

[tjhei]

I like the idea. Due to the large number of changes required, I would suggest that we do not deprecate the old names.

Arguments against this change:

1. It is easy to "find" AdditionalData class if it is inside (doxygen, IDE, etc.)
2. Having two names for it might be confusing (if we keep both)
3. This will require code changes in pretty much every program (if we deprecate the old)

[sebproell]

I like the idea. Due to the large number of changes required, I would suggest that we do not deprecate the old names.

Agree. I would not deprecate it. Referring to the type in a generic way via the class-internal typedef is useful.

Arguments against this change:

1. It is easy to "find" `AdditionalData` class if it is inside (doxygen, IDE, etc.)

Should we add @relates SolverGMRES to the moved outside class? Would at least help with doxygen. In the IDE it is trivial to find if one "follows" the right-hand side of the typedef.

2. Having two names for it might be confusing (if we keep both)

Maybe. I think it is quite similar for the case of the cell_iterator typedef. If you really want to do something with an object of this type, you need to consult the documentation of the actual object. Or click in the IDE until you find the type's declaration and look there. In the case of the present struct, there is only one typedef indirection, which should be easy to follow?

[peterrum]

Let's see what the test say...

/rebuild

[sebproell]

Since the tests pass: any more opinions on the proposed change? Should I adapt it for all the Solver classes?

[tjhei]

Should I adapt it for all the Solver classes?

As this is a bigger change, I would suggest we wait for a few more opinions on this.

[kronbichler]

I think this can be helpful, as it is annoying to state all parameters. I would definitely vote for the @relates statement. One thing I would remark is that it won't work for all classes without template arguments, e.g. https://www.dealii.org/current/doxygen/deal.II/structPreconditionChebyshev_1_1AdditionalData.html needs one of the class's template parameter.

[sebproell]

One thing I would remark is that it won't work for all classes without template arguments, e.g. https://www.dealii.org/current/doxygen/deal.II/structPreconditionChebyshev_1_1AdditionalData.html needs one of the class's template parameter.

Hm, that's unfortunate. At least PreconditionerType only requires vmult afaik. In larger user codes one might have an interface, concept, type-erasure, ... for this anyway, which will simplify the creation of the struct a priori to selcting the specific internal preconditioner. For me it will already help if I pull it outside with this one template parameter.

[bangerth]

I'm not opposed to this, but if we do it I would like it if we did it for all 11 places at once where we currently have an AdditionalData class. I think there is nothing wrong if we have one place where the new class keeps a template argument (which in many cases will simply be left at its default).

I will say that SolverGMRESAdditionalData is not so much shorter than

  SolverGMRES<Vector<double>>::AdditionalData

or

  decltype(solver)::AdditionalData

I think the work involved results in a rather small gain, but then I don't have to do it myself :-)

[sebproell]

I would like it if we did it for all 11 places at once

Agreed. I will change it everywhere.

I will say that SolverGMRESAdditionalData is not so much shorte

My intention behind this change is not brevity. I want to specify solver parameters in a very general place where I have no idea what the VectorType is going to be. This is only possible (or at least a lot easier) with a struct that doesn't require this template parameter.

[peterrum]

I don't think these are all places where AdditionalData is within a class. See PreconditionChebyshev, MatrixFree, ...

[sebproell]

I don't think these are all places where AdditionalData is within a class. See PreconditionChebyshev, MatrixFree, ...

Correct. I am not done yet. Neverteheless, the question is whether this change is truly useful for every class. Personally, I would not benefit from having the parameters of MatrixFree outside the class, since these are not something I would enter in an input file. Others might have other requirements though. Changing every struct AdditionalData is quite a lot of work. There are more than a hundred hits if I search for this expression. If we do it I would do it gradually.

I planned to also do the preconditioners and other solvers. However, I would leave the structs inside the class if the class is not a template.

[bangerth]

I would really like it if we did it uniformly across the entire library.

[sebproell]

@bangerth @peterrum Ok, makes sense I guess for consistencies sake. I'll do the following:

• For any class X which has an internal struct AdditionalData, move the struct outside (same scope as class X) and name it XAdditionalData.
• Add a typedef AdditionalData inside the class
• Do this regardless if the class is a template or not
• Do this also for empty structs
• Don't do it for implementation detail classes.

[sebproell]

Still not finished. There are some packages that I don't use locally and thus will not be able to adapt right now. These include arpack, gingko, slepc, cgal.

[bangerth]

@sebproell What do we want to do with this PR? It's become stale with merge conflicts. Are you still interested in this, or should we just close it?