Be more generous in what types we allow in the FESystem(...) constructor #5090

bangerth · 2017-09-14T22:11:18Z

This is a follow-up to the new variadic constructor of FESystem introduced in #5026: We now allow the argument list to be composed of either (i) pairs of a pointer to finite element and multiplicity, or (ii) just finite element. This allows writing things such as

  FESystem (FE_Q(2)^dim, FE_Q(1));

where the second argument has no multiplicity.

This works by routing each argument to a function promote_to_pair() that just forwards the original pair if it really is a pair, or that creates a pair of that element and multiplicity one if it is not a pair.

The last commit extends this a bit by adding another function promote_to_pair() that just eats all other possible argument types but has a static_assert that explains in detail that you can't do that. For example, if you pass an integer as an argument, you'd get this sort of error message:

40: In file included from /home/fac/f/bangerth/p/deal.II/1/dealii/tests/fe/system_04.cc:25:0:
40: /home/fac/f/bangerth/p/deal.II/1/dealii/include/deal.II/fe/fe_system.h: In function ‘std::pair<std::unique_ptr<dealii::FiniteElement<<anonymous>, <anonymous> > >, unsigned int> dealii::{anonymous}::promote_to_fe_pair(const T&)’:
40: /home/fac/f/bangerth/p/deal.II/1/dealii/include/deal.II/fe/fe_system.h:1183:5: error: static assertion failed: The arguments to the FESystem constructor that takes a variable number of arguments all need to either be finite element objects, or pairs of a std::unique pointer to a finite element and an integer multiplicity (i.e., what you get when you write, for example, (FE_Q<dim>(2)^3'). But here, you are calling that constructor with an argument that is neither of these two options.
40:      static_assert (false,
40:      ^

There is a new test, system_04 that is a variation of system_03 that just omits the ^1 cases. The output is identical to the previous test.

bangerth · 2017-09-14T22:11:58Z

See also #4249.

masterleinad · 2017-09-14T22:48:09Z

Nice. That looks easier than expected.

I am bit surpised that this actually compiles for you. since the universal reference just matches everything. In particular, all the places where the old constructors are used are problematic and I get lots of compile errors using gcc-7.1.0.

Hence, I think that you have to go back to some std::enable_if approach.

masterleinad · 2017-09-14T22:51:03Z

Regarding your change in FiniteElement I get errors like

 ../source/fe/fe.cc:159:24: error: no matching function for call to ‘make_pair<std::unique_ptr<dealii::FiniteElement<1, 1>, std::default_delete<dealii::                 FiniteElement<1, 1> > >, unsigned int>(std::remove_reference<std::unique_ptr<dealii::FiniteElement<1, 1>, std::default_delete<dealii::FiniteElement<1, 1> > > >::type,  const unsigned int&)’
    return std::make_pair<std::unique_ptr<FiniteElement<dim, spacedim>>,
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           unsigned int> (std::move(this->clone()), multiplicity);
           ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 In file included from /opt/compiler/gcc-7.2.0/include/c++/7.2.0/bits/stl_algobase.h:64:0,
                  from /opt/compiler/gcc-7.2.0/include/c++/7.2.0/bits/specfun.h:45,
                  from /opt/compiler/gcc-7.2.0/include/c++/7.2.0/cmath:1914,
                  from ../include/deal.II/base/numbers.h:23,
                  from include/deal.II/base/config.h:335,
                  from ../include/deal.II/base/memory_consumption.h:20,
                  from ../source/fe/fe.cc:16:
 /opt/compiler/gcc-7.2.0/include/c++/7.2.0/bits/stl_pair.h:519:5: note: candidate: template<class _T1, class _T2> constexpr std::pair<typename std::                     __decay_and_strip<_Tp>::__type, typename std::__decay_and_strip<_T2>::__type> std::make_pair(_T1&&, _T2&&)
      make_pair(_T1&& __x, _T2&& __y)
      ^~~~~~~~~
 /opt/compiler/gcc-7.2.0/include/c++/7.2.0/bits/stl_pair.h:519:5: note:   template argument deduction/substitution failed:
 ../source/fe/fe.cc:159:24: note:   cannot convert ‘multiplicity’ (type ‘const unsigned int’) to type ‘unsigned int&&’
    return std::make_pair<std::unique_ptr<FiniteElement<dim, spacedim>>,
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           unsigned int> (std::move(this->clone()), multiplicity);
           ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 ../source/fe/fe.cc: In instantiation of ‘std::pair<std::unique_ptr<dealii::FiniteElement<<anonymous>, <anonymous> > >, unsigned int> dealii::                           FiniteElement<<anonymous>, <anonymous> >::operator^(unsigned int) const [with int dim = 1; int spacedim = 2]’:
 source/fe/fe.inst:17:17:   required from here

telling me that the second argument has to be a rvalue reference.

masterleinad · 2017-09-14T23:51:28Z

Adding to the last comment, cppreference says that std::make_pair requires rvalue references from C++11 on.
The constructor of std::pair also allows both arguments to be const references, but this is also not applicable here since std::unique_ptr is not copy-constructible.

bangerth · 2017-09-15T15:28:06Z

I see the issue with the compile error as well now. I apparently only verified with the tests that actually use the variadic constructor.

Will fix.

bangerth · 2017-09-15T16:58:38Z

What a bizarre definition of std::make_pair, btw. But I have a workaround for that as well.

bangerth · 2017-09-15T19:47:51Z

OK, this should work now. Can you take another look?

I had to work a bit with additional classes such as all_same, but I think that makes things a bit clearer. I also had to get rid of the function with the static_assert -- I just couldn't get that going, and it's now covered with the enable_if anyway. GCC error messages don't help either because they just show the error from the static_assert, but not how I got there nor the template arguments of the function in which this happens :-(

masterleinad · 2017-09-16T21:38:50Z

include/deal.II/base/template_constraints.h

@@ -25,10 +25,26 @@

 DEAL_II_NAMESPACE_OPEN

-namespace
+namespace internal


why don't you like the anonymous namespace here?

i think @drwells was recently fixing those, i.e. adding internal to anonymous namespaces. I forgot the reason, though 😄

Anonymous namespaces in header files are generally a not so great idea.

An anonymous namespace is equivalent to a named namespace where the name is automatically created for each "translation unit" (i.e., .cc file that #includes the header). That means that a symbol in an anonymous namespace in a header file is distinct between two object files. Most of the time this has no practical impact other than the fact that the linker cannot, for example, unify the code for one function. But you'd get into trouble if you had, for example, a global variable in such a namespace, or maybe a static variable in a function in such a namespace: these variables are not in fact global, but duplicated in every .cc file that #includes the header file.

For such reasons, it's generally not a great idea to use anonymous namespaces in header files. Most of the time, nothing bad happens, but when something bad happens, it's a really bizarre situation that difficult to debug.

masterleinad · 2017-09-16T21:42:08Z

include/deal.II/base/template_constraints.h

 };


+/*
+ * A generalization of `std::enable` that only works if


std::enable-> std::enable_if

masterleinad · 2017-09-16T21:43:41Z

include/deal.II/base/template_constraints.h

 */
 template <class Type, class... Types>
 struct all_same_as
 {
  static constexpr bool value =
-    std::is_same<BoolStorage<std::is_same<Type, Types>::value..., true>,
-    BoolStorage<true, std::is_same<Type, Types>::value...>>::value;
+    internal::TemplateConstraints::all_true<std::is_same<Type, Types>::value...>::value;
 };




This should be three blank lines, shouldn't it?

masterleinad · 2017-09-16T21:49:28Z

include/deal.II/base/template_constraints.h

 {
-  // helper struct for is_base_of_all and all_same_as
-  template <bool... Types> struct BoolStorage;
+  namespace TemplateConstraints


This would probably a good namespace name for everything in this file... Again, I don't quite see the need for a named namespace here.

masterleinad · 2017-09-16T22:07:47Z

include/deal.II/fe/fe_system.h

+  template <class... FEPairs,
+            typename = typename enable_if_all<
+              (std::is_same<std::pair<std::unique_ptr<FiniteElement<dim, spacedim>>, unsigned int>,
+                            typename std::decay<FEPairs>::type>::value


Don't we normally test using syntax like
x==1 instead of 1==x?

masterleinad · 2017-09-16T22:10:43Z

include/deal.II/base/template_constraints.h

+ * true.
+ */
+template <bool... Values>
+struct enable_if_all : std::enable_if<internal::TemplateConstraints::all_true<Values...>::value>


For this to be analogous to std::enable_if I would expect to be able to specify a type (default void) I can access via std::enable_if_all<...>::type if all of the statements are true. Can you add this?

Well, that's sort of a problem. I had tried that, but to be analogous to std::enable_if, I would have to declare this as

template <bool... Values, typename T = void> struct enable_if_all : std::enable_if<..., T> {};

But that's not allowed: parameter packs must appear at the end of the template argument list. So my only choice would have been to move T to be the first argument, and then I can't give it a default value.

So, I didn't know what to do and went with the easy path which was to omit T altogether. I couldn't come up with a better solution. What do you think the best approach is, @masterleinad ?

masterleinad · 2017-09-16T22:12:07Z

include/deal.II/fe/fe_system.h

@@ -1147,17 +1167,42 @@ namespace
      return fe_system.second > 0;
    });
  }
+
+
+  template <int dim, int spacedim>


Also three blank lines.

masterleinad · 2017-09-16T22:13:11Z

Apart from the things I commented on, I really like this.

Specifically, include the one that is necessary for std::enable_if.

While there, simplify some other classes a bit by introducing an 'all_true' class. Also move classes into a named internal namespace.

bangerth · 2017-09-17T23:57:48Z

I think I addressed all of the issues other than the enable_if_all one.

masterleinad · 2017-09-18T10:42:07Z

What about about the following?

diff --git a/include/deal.II/base/template_constraints.h b/include/deal.II/base/template_constraints.h
index 1455c9f..31c20e8 100644
--- a/include/deal.II/base/template_constraints.h
+++ b/include/deal.II/base/template_constraints.h
@@ -77,14 +77,28 @@ struct all_same_as
 
 
 
-/*
+/**
  * A generalization of `std::enable_if` that only works if
  * <i>all</i> of the given boolean template parameters are
  * true.
+ * In this case, a type <code>T</code> can be stored and accessed
+ * using the following syntax:
+ * @code
+ *   using Type = std::enable_if_all<Values...>::template type<T>;
+ * @endcode
+ * By default, <code>T=default</code> just as for the regular
+ * <code>std::enable_if</code> and the above example simplifies to:
+ * @code
+ *   using Type = std::enable_if_all<Values...>::template type<>;
+ * @endcode
  */
 template <bool... Values>
-struct enable_if_all : std::enable_if<internal::TemplateConstraints::all_true<Values...>::value>
-{};
+struct enable_if_all
+{
+  template<typename T = void>
+  using type =
+    typename std::enable_if<internal::TemplateConstraints::all_true<Values...>::value, T>::type;
+};
 
 
 
diff --git a/include/deal.II/fe/fe_system.h b/include/deal.II/fe/fe_system.h
index f921833..221b5c8 100644
--- a/include/deal.II/fe/fe_system.h
+++ b/include/deal.II/fe/fe_system.h
@@ -469,7 +469,7 @@ public:
                std::is_base_of<FiniteElement<dim, spacedim>,
                                typename std::decay<FEPairs>::type>::value)
               ...
-              >::type
+              >::template type<>
             >
   FESystem (FEPairs &&... fe_pairs);

edit: default type parameter

bangerth · 2017-09-18T22:06:04Z

Hm, I tried this, but I can't seem to make it work. Does the patch you propose above actually work for you, including all of the existing and new tests?

masterleinad · 2017-09-18T23:22:01Z

Using

 template <class... FEPairs,
             typename = typename enable_if_all<
               (std::is_same<typename std::decay<FEPairs>::type,
                             std::pair<std::unique_ptr<FiniteElement<dim, spacedim>>, unsigned int>>::value
                ||
                std::is_base_of<FiniteElement<dim, spacedim>,
                                typename std::decay<FEPairs>::type>::value)
               ...
               >::template type<>
             >

in fe/fe_system.h works for me.

bangerth · 2017-09-19T14:45:23Z

I tried this again but it doesn't seem to work with GCC 4.8. This may well be a compiler bug in C++11 support. The errors are like this:

/home/fac/f/bangerth/p/deal.II/1/dealii/include/deal.II/base/template_constraints.h: In substitution of ‘template<bool ...Values> template<class T> using type = typename std::enable_if<dealii::internal::TemplateConstraints::all_true<Values>::value, T>::type [with T = void; bool ...Values = {false, false}]’:
/home/fac/f/bangerth/p/deal.II/1/dealii/include/deal.II/fe/fe_system.h:465:13:   required from ‘dealii::FE_Enriched<dim, spacedim>::FE_Enriched(const std::vector<const dealii::FiniteElement<dim, spacedim>*>&, const std::vector<unsigned int>&, const std::vector<std::vector<std::function<const dealii::Function<spacedim>*(const typename dealii::Triangulation<dim, spacedim>::cell_iterator&)> > >&) [with int dim = 1; int spacedim = 1; typename dealii::Triangulation<dim, spacedim>::cell_iterator = dealii::TriaIterator<dealii::CellAccessor<1, 1> >]’
source/fe/fe_enriched.inst:11:17:   required from here
/home/fac/f/bangerth/p/deal.II/1/dealii/include/deal.II/base/template_constraints.h:100:107: error: no type named ‘type’ in ‘struct std::enable_if<false, void>’
   using type = typename std::enable_if<internal::TemplateConstraints::all_true<Values...>::value, T>::type;
                                                                                                           ^

In other words, SFINAE doesn't seem to actually work here.

How do you want to proceed? We don't actually need the nested type, so the current void type would work just fine.

masterleinad · 2017-09-19T15:03:56Z

Let's just stick with the current version. We can alyways extend enable_if_all later if we want to use it with a non-void type.

masterleinad · 2017-09-19T15:04:08Z

/run-tests

masterleinad · 2017-09-20T19:06:34Z

Thanks!

bangerth added the C++11 label Sep 14, 2017

bangerth force-pushed the fesystem branch from 250d74a to 856647e Compare September 15, 2017 19:46

masterleinad reviewed Sep 16, 2017

View reviewed changes

bangerth added 6 commits September 17, 2017 17:56

Add an #include file.

1e84306

Specifically, include the one that is necessary for std::enable_if.

Add enable_if_all template constraint class.

659e1d2

While there, simplify some other classes a bit by introducing an 'all_true' class. Also move classes into a named internal namespace.

Rename template type and argument name to better reflect their purpose.

189ce0b

Make an argument 'const'.

a8cffed

Allow not specifying the multiplicity in the FESystem constructor.

dc2d367

Add a test.

a28e7ad

bangerth force-pushed the fesystem branch from 856647e to a28e7ad Compare September 17, 2017 23:57

bangerth force-pushed the fesystem branch from eeb0206 to a28e7ad Compare September 18, 2017 22:05

masterleinad added the Reviewed and ready to merge label Sep 19, 2017

masterleinad merged commit a91c6db into dealii:master Sep 20, 2017

bangerth deleted the fesystem branch September 20, 2017 19:08

masterleinad mentioned this pull request Jun 15, 2018

Fix compiling with older Intel compilers #6769

Merged

bangerth mentioned this pull request Oct 20, 2020

Use operator^ to create FESystem objects in the tutorials. #11066

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Be more generous in what types we allow in the FESystem(...) constructor #5090

Be more generous in what types we allow in the FESystem(...) constructor #5090

bangerth commented Sep 14, 2017

bangerth commented Sep 14, 2017

masterleinad commented Sep 14, 2017

masterleinad commented Sep 14, 2017

masterleinad commented Sep 14, 2017

bangerth commented Sep 15, 2017

bangerth commented Sep 15, 2017

bangerth commented Sep 15, 2017

masterleinad Sep 16, 2017

davydden Sep 16, 2017

bangerth Sep 17, 2017

masterleinad Sep 16, 2017

masterleinad Sep 16, 2017 •

edited

masterleinad Sep 16, 2017

masterleinad Sep 16, 2017

masterleinad Sep 16, 2017

bangerth Sep 17, 2017

masterleinad Sep 16, 2017

masterleinad commented Sep 16, 2017

bangerth commented Sep 17, 2017

masterleinad commented Sep 18, 2017 •

edited

bangerth commented Sep 18, 2017

masterleinad commented Sep 18, 2017

bangerth commented Sep 19, 2017

masterleinad commented Sep 19, 2017

masterleinad commented Sep 19, 2017

masterleinad commented Sep 20, 2017

Be more generous in what types we allow in the FESystem(...) constructor #5090

Be more generous in what types we allow in the FESystem(...) constructor #5090

Conversation

bangerth commented Sep 14, 2017

bangerth commented Sep 14, 2017

masterleinad commented Sep 14, 2017

masterleinad commented Sep 14, 2017

masterleinad commented Sep 14, 2017

bangerth commented Sep 15, 2017

bangerth commented Sep 15, 2017

bangerth commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masterleinad Sep 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masterleinad commented Sep 16, 2017

bangerth commented Sep 17, 2017

masterleinad commented Sep 18, 2017 • edited

bangerth commented Sep 18, 2017

masterleinad commented Sep 18, 2017

bangerth commented Sep 19, 2017

masterleinad commented Sep 19, 2017

masterleinad commented Sep 19, 2017

masterleinad commented Sep 20, 2017

masterleinad Sep 16, 2017 •

edited

masterleinad commented Sep 18, 2017 •

edited