I've being reading the code of the Matrix object in Matrix.cpp/.h, and it seems to me that there is room for improvement (original author did great, don't get me wrong )
I'm posting my thought before pushing code, once we agree on any changes, I will go ahead and make a PR.
I hope that this post in not too long...
Don't hesitate to pinpoint me any rules that I could have be missing ! (note that the reason "Because we do that everywhere else" is fine for me, I don't want to be disturbing the whole code )
These note are only "cleaning", I'm sure the code actually work already !
Notes come in any order of importance.
Here are the thing that could be improved, IMHO:
1) Removing unused code : in PR : https://github.com/FreeCAD/FreeCAD/pull/5494
2) Avoid code duplication in -= , +=, *= operator : we define the = operator, and the + , - , * . So instead of duplicating code from + (for instance) into +=, I think I could write instead in += something like
Code: Select all
(*this) = (this) + input;
return (*this);
3) There is use of short as for-loop counter. I think we could change them back to int. In fact, using shorter type don't improve speed, if it was the intent. Actually, the computer must operate additional masking to work on shorter values, rather than on datatype with regular size.
I would be happy to be proven wrong, ideally I should be running benchmark to verify my point, but still, my personal rule is that int is fine in those cases.
4) In for-loop, counter could be declared inside of the loop statement, instead of before. That reduce scope (well, that not very useful here anyway), but since C++ allows it, I'm used to doing so... Is there any rules to not doing so ? note that transpose() do it, for instance, so that would make think more homogeneous.
Code: Select all
for (int iz = 0; iz < 4; iz++)
for (int is = 0; is < 4; is++)
5) More generally I'm fan of reducing scope of variables, so putting them on the smallest block possible. I do feel more readable as well, and we are ensure that the value is not used outside of the block.
6)
EDIT : This point will not be done, as the "best" way to do is not clear, as rediscussed in https://forum.freecadweb.org/viewtopic. ... 71#p568171
I'm also used to defensive programming pattern, where the main code (nominal case) is the least indented. For instance giving that function :
Code: Select all
PyObject* MatrixPy::row(PyObject * args)
{
int index;
if (PyArg_ParseTuple(args, "i", &index)) {
index = index % 4;
Matrix4D* mat = getMatrixPtr();
Base::Vector3d v = mat->getRow(index);
return Py::new_reference_to(Py::Vector(v));
}
PyErr_SetString(Base::BaseExceptionFreeCADError, "int expected");
return nullptr;
}
Code: Select all
PyObject* MatrixPy::row(PyObject * args)
{
if (PyArg_ParseTuple(args, "i", &index)) {
PyErr_SetString(Base::BaseExceptionFreeCADError, "int expected");
return nullptr;
}
int index = index % 4;
Matrix4D* mat = getMatrixPtr();
Base::Vector3d v = mat->getRow(index);
return Py::new_reference_to(Py::Vector(v));
}
Note that it's my very personal take on that point
7) In transpose(), why not just swap two values in place ? That would additionally avoid read/write the diagonal, and avoid buffer and memcpy. Something like : (I might have mess up the indices, but it's just to get an idea) (exchange() could simply use a tmp variable to work, instead of a whole function call, indeed)
Code: Select all
for (int i = 0; i < 4; i++)
{
for (int j = 0; j < i; j++)
exchange(dNew[j][i], dMtrx4D[i][j]);
}
9) Why define typedef double * Matrix; ? Could me not use Matrix4D ?
It seems like we want to perform the invertion on regular array rather than on the "same" array located in the class. Is it a performance concern ?
If we could use Matrix4D, in inverseGauss() we could remove the explicit identity initialisation, as the Matrix4D default to that.
Code: Select all
double inversematrix [16] = { 1 ,0 ,0 ,0 ,
0 ,1 ,0 ,0 ,
0 ,0 ,1 ,0 ,
0 ,0 ,0 ,1 };
10) In inverse () : are they German comment ? anyway, should we translate them in English ?
11) The code of PyObject* MatrixPy::rotateX / Y / Z seems to be factorizable a lot : if we define a function with the whole code in the do[ ... }while(false); We could reduce the quantity of code by a lot, and remove the usage of the do{ ... } while(false) altogether !
12) int hasScale(double tol=0.0) const; define an default value for tol, but in that case, given that we do
Code: Select all
if (tol == 0.0)
tol = 1e-9;
I'm aware that change slightly the API of this function. Because user that use 0 in they call have learn that the code got a protection for that.
The C++ code would not be impacted (it always call that with no parameter, so it would not be impacted), but only Python API
Given that :
Code: Select all
PyObject* MatrixPy::hasScale(PyObject * args)
{
double tol=0;
if (!PyArg_ParseTuple(args, "|d", &tol))
return nullptr;
return Py::new_reference_to(Py::Int(getMatrixPtr()->hasScale(tol)));
}
But it would allows to clean to whole optional parameter thingy on the C++ side, without breaking anything on the Python side.
13) In analyse(), we do
Code: Select all
sub[0][0] = dMtrx4D[0][0]; sub[0][1] = dMtrx4D[0][1];
sub[0][2] = dMtrx4D[0][2]; sub[1][0] = dMtrx4D[1][0];
sub[1][1] = dMtrx4D[1][1]; sub[1][2] = dMtrx4D[1][2];
sub[2][0] = dMtrx4D[2][0]; sub[2][1] = dMtrx4D[2][1];
sub[2][2] = dMtrx4D[2][2];
That would allows usage of subMatrix elsewhere in the core code of FreeCAD.
in the same function, we do also
Code: Select all
bool ortho = true;
for (unsigned short i=0; i<4 && ortho; i++) {
for (unsigned short j=0; j<4 && ortho; j++) {
if (i != j) {
if (fabs(trp[i][j]) > eps) {
ortho = false;
break;
}
}
}
}
Just a thought, however.
14) analyse() use a
Code: Select all
const double eps=1.0e-06;
15) analyse() use a weird mix of std::string text; and std::ostringstream stringStream; to format the output message with .str() everywhere to convert.
I think follow the pattern of toString() could be nice :
Use one ostringstream, and then convert back to string with .str() just before return (moreover analyse() as only one return already, should be easy enough to use that pattern)
Thank for the readers that arrived here !
Feel free to give me any feedback, I would definitely not be offended if I have to discard my thought , Especially because I could be picky on the code, and some(all?) notes could be considered as no real improvement
Cheers,