libMesh
Remove the Point 1D/Real constructor
Something like Point(elem->node_ptr(0) - elem->node_ptr(1)) feels like (one mistake away, no warnings) it should give the point for the 0th node but it ends up doing pointer to Real to Point instead. There are so many mistakes we can make in vector math with it too
A nice thing with use C++ is to have a strongly typed language. Having Point(Real) imo loses this advantage
Imagine if we had Tensor(Point)
I think we need to keep it for (the completely theoretical, at this point) support of LIBMESH_DIM==1? Also I believe @roystgnr has gone to some trouble to prevent what you describe from happening, i.e.
/**
* Disambiguate constructing from non-Real scalars
*/
template <typename T,
typename = typename
boostcopy::enable_if_c<ScalarTraits<T>::value,void>::type>
Point (const T x) :
TypeVector<Real> (x,0,0)
{}
I don't really know what the template magic does here, but going from the comment it sounds like it's not working as expected.
A nice thing with use C++ is to have a strongly typed language. Having Point(Real) imo loses this advantage
Oh, we don't have a fully strongly typed language in C++ regardless. Recall the recent MOOSE fix for Real foo = 1/3*bar;, which for backwards-compatibility reasons C++ will happily evaluate as foo = 0, and which for God-only-knows-what reasons modern C++ compilers won't even warn about. I guess "integer division with truncation is so useful we can't warn about it" combined with "silent integer promotion to FP is so useful we can't warn about it" without considering (or concluding it's just too hard to reliably detect) "if someone ever does both in the same expression they're almost certainly screwing up badly".
I'm with you philosophically here, or perhaps insanely beyond you... E.g. IMHO for practical purposes even unsigned int is not a type! "index into container X" is a type, "integral quantity of Y" is a type, "index into container Z" is a type, and when we call them all int then the compiler won't catch any implicit type conversions between them even though in the past those interconversions have been errors as often as not. Using only built-in C++ types means you can unwittingly literally compare apples and oranges. If I were writing libMesh from scratch we wouldn't just have dof_id_type differing from subdomain_id_type so we could declare them to be different sized versions of int for RAM savings, we'd have them be different classes (likewise node_id_type, elem_id_type) that could only be explicitly converted to and from int or each other, and we'd have avoided a whole class of bugs. Plus we'd have much more self-documenting function declarations: "does this method take an element's node index or a mesh's node index" isn't even a question when the argument has to be elem_node_index_type.
I don't really know what the template magic does here, but going from the comment it sounds like it's not working as expected.
It was forever ago, but IIRC the template magic there was just to let us use the constructor directly for things like int and AD scalars while preventing us from hitting that constructor for anything - other vectors, other classes, etc. Non-Real scalars are exactly what it's supposed to allow; Point(1) was always supported and when I added support for weirder vector types, I had to do some juggling to avoid breaking it.
could we ifdef LIBMESH_DIM != 1 a compile time warning in the constructor
That ifdef would be super annoying ... but I could be talked into deprecating all uses, regardless of LIBMESH_DIM, of the constructor without all arguments. These constructors are inlined; the compiler isn't going to actually put the extra , 0, 0 on a stack. This kills my dream of someday supporting LIBMESH_DIM=4, but the more I learn about things like metric tensor signatures and the more experience I have with software, the more I think that was never going to be a good way to support things like space-time finite elements.
Imagine if we had Tensor(Point)
You mean like this, in type_tensor.h?
template<typename T2>
TypeTensor(const TypeVector<T2> & vx);
Strictly speaking a point is already a n×1 tensor, but in libMesh-speak this is the same "promote it to full size in every index" behavior as a Point(Real) construction ... and in a more annoying form, even, passing by reference rather than value, despite that being basically pointless for a templated inline function.
Would you want to get rid of the zero-argument versions of these constructors? Those don't allow crazy conversions, and they're much more commonly used than the one-argument versions. If we still allowed Point() etc., then the requirement of Tensor t{vec, TypeVector<Real>(), TypeVector<Real>()} for that particular singular tensor would be more annoying than Point p{x,0,0}, but not too much more, especially for such a rare (or unused? I probably added that constructor just out of symmetry with TypeVector, not because I expected it to be needed) requirement.
Sorry for the long stream-of-consciousness. On reflection, I think removing the 1D/2D constructors is the best way to go in the long run, though these are so long-standing that I'd want them deprecated for at least a year first.
Oh, we don't have a fully strongly typed language in C++ regardless.
Tonight I accidentally wrote some code like error_msg = "Foo."; error_msg += '\n' + "Meshes failed asserted equality in at least these aspects:\n" and was way more confused than I should have been (is there a threading bug???) when the result eventually got output as Foo.led asserted equality in at least these aspects:
I can reproduce this without the += part, i.e.
std::string error_msg = '\n' + "Meshes failed asserted equality in at least these aspects:\n";
std::cout << error_msg;
prints:
led asserted equality in at least these aspects:
Is it somehow converting the string literal into a integral value so that addition with char makes sense?
Nope - it's noticing that I'm trying to add a char * and a char, and it's treating the char as a tiny integer so that the addition to a pointer makes sense. '\n' == 10, so it advances the pointer-to-'M' to instead point 10 chars further ahead to 'l'.
That's super weird. I would have thought the string literal is more like a const char * const, so that you wouldn't be allowed to modify it at all.
It is, and you can't, but the trick is that I'm not modifying it, I'm creating a temporary from it. I spoke poorly before - the pointer-to-'M' doesn't itself get advanced, it just gets added to, to create the new temporary pointer. To the compiler this is no different than int x=0; x += 2 + 3; - neither the 2 nor the 3 get modified, but a temporary unnamed variable gets created to hold 5 and then immediately summed into x. Here the pointer-to-'M' doesn't itself get reseated, but when summed with the number 10 a new temporary pointer-to-'l' is the result.