[boost] [review][qvm] QVM formal review report

Subject: [boost] [review][qvm] QVM formal review report
From: Adam Wulkiewicz (adam.wulkiewicz_at_[hidden])
Date: 2016-01-03 13:11:08

Dear Boost community,

The review period for Emil Dotchevski's QVM library has concluded. I'd 
like to thank everyone who participated in the discussions, esspecially 
those who sent formal reviews.

Only four votes were casted:
- 1 vote for acceptance
  * Rajaditya Mukherjee
- 2 votes for conditional acceptance
  * Rainer Deyke
  * Oswin Krause
- 1 vote for rejection
  * Phil Endecott

However more people commented on issues on the mailing list. Comments 
received during the course of the review were generally positive, with a 
number of technical issues identified that need to be addressed. The 
review identified no fundamental problems in the design or 
implementation of the library that would entail rejection. The final 
version of the library can be fully accepted once Emil has addressed the 
issues identified during the review. Therefore I'm happy to say:

     Emil Dotchevski's Boost.QVM library is CONDITIONALLY ACCEPTED.

Below is a summary of technical concerns:

- The identifiers should be made more verbose. This is true both for 
function names as well as identifers containing "q"/"v"/"m" parts. The 
former should be replaced with full words, e.g. "transp" -> "transpose". 
The latter should be replaced at least with "qua"/"quat"/"vec"/"mat" 
respectively, so e.g. "v_traits" -> "vector_traits"/"vec_traits", 
"col_m" -> "col_mat", etc.

- Many participants found the swizzling syntax using comma operator 
confusing or not good enough. There were many suggestions about the 
possible replacements:
  * v%XY           - the old operator% syntax
  * (v,XY)         - the comma operator syntax currently used by QVM
  * (v,asXY)       - in order to limit potential conflicts with macros
  * XY(v)          - function syntax, MOST PREFERED
  * v.XY()         - probably the best but currently not possible 
(n4165, n4174)
  * ref(v).XY()
  * ref(v)[xy]
  * swizzle(v, xy)
  * ref(v)->xy
The function syntax, i.e. XY(v), was the most prefered solution. Some of 
the participants appreciated the comma operator as well because of the 
similarity with shader languages syntax. The library could provide a few 
alternatives that could be picked by the user, e.g. both a comma and 
function syntax.

- There were concerns regarding deduce_xx set of traits, in particular 
that traits taking two arguments (deduce_q2, deduce_v2 and deduce_m2) 
can cause ODR issues in a case when e.g. third-party libraries depending 
on QVM specialized these traits differently and those libraries were 
used together. There were no consensus if this is indeed a library issue 
or how this issue could be handled in general. Taking into account that 
the specialization of these traits is optional it'd be sufficient to 
describe the issue in the documentation and discourage the users from 
specializing those traits for types defined outside of their own code.

- There was also a suggestion to use the same names that are already 
being used in other well-known libraries to make the interface more 
intuitive, e.g. mag() could be replaced by norm() (used in Eigen and 
Armadillo libraries), mag2() could be replaced by norm_squared() or 
norm_sqr(), etc. This would also make the latter more verbose. I'd let 
the decission how to deal with it to Emil.

- Another suggestion was to replace read/write semantics with get/set 
semantics or at least to make the r/w/ir/iw identifiers more verbose. 
Here, I'd let the decission how to deal with it to Emil as well.

- Aside from the issues described above, there were also suggestions 
that the scope of QVM library could/should be broadened to include 
functionalities from domains other than computer graphics, e.g. linear 
algebra, geometry, physics, robotics, etc. Oswin Krause requested that 
an consensus on the scope of the library should be made before the 
acceptance of the library. Phil Endecott expressed that the library is 
too small to be a standalone library. However as a Boost.Geometry 
developer I may say that the scopes, design rationales, internal 
structures, etc. of both libraries are different. AFAIK it's also true 
for Boost.Polygon. Furthermore, if my understanding is correct after 
reading various discussions the community finds the library useful even 
in the current state, some of the participants already used the library 
in their projects. Furthermore I'm sure that future users of QVM will 
suggest additions to the QVM library if they think it's necessary, so 
the scope will be broadened in a natural way. Though of course I 
encourage Emil to start a discussion with the community regarding this 
issue.

- Regarding the documentation, various people suggested the following 
improvements:
  * it should start with a "Quick Start" page showing how to use the QVM 
built-in types to do some high-school linear algebra,
  * make it more obvious which functions are classified as "operations" 
and which are "view proxies",
  * contain more examples of every function as well as some bigger 
real-life examples.

Please let me know if you think something is wrong.

Regards,
Adam