New user self-registration is currently disabled. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Bug 1278 - Abused C operator precedence in use of ternary within conditional.
Abused C operator precedence in use of ternary within conditional.
Status: RESOLVED FIXED
Product: Eigen
Classification: Unclassified
Component: Core - matrix products
unspecified
All All
: Normal Compilation Problem
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-08-20 02:01 UTC by Glenn
Modified: 2016-08-22 13:02 UTC (History)
2 users (show)



Attachments

Description Glenn 2016-08-20 02:01:52 UTC
A static code analysis tool flagged this code in GeneralMatrixVector.h as problematic.

https://bitbucket.org/eigen/eigen/src/cd5824eb9fb275e5b0edc8a7af95e8af85fe9231/Eigen/src/Core/products/GeneralMatrixVector.h?at=default&fileviewer=file-view-default#GeneralMatrixVector.h-186

Here is the code in question:
  const Index offset1 = (FirstAligned && alignmentStep==1?3:1);
  const Index offset3 = (FirstAligned && alignmentStep==1?1:3);

What is the intent here? I believe this is parsed in the following way due to C operator precedence (http://en.cppreference.com/w/c/language/operator_precedence):
  const Index offset1 = (FirstAligned && alignmentStep==1)?3:1;
  const Index offset3 = (FirstAligned && alignmentStep==1)?1:3;

But that's not entirely clear.  The current use of () strikes me as actually misleading, implying the following:
  const Index offset1 = (FirstAligned && (alignmentStep==1?3:1));
  const Index offset3 = (FirstAligned && (alignmentStep==1?1:3));

While the code in question may be technically correct, it can be made much easier to read and understand.
Comment 1 Gael Guennebaud 2016-08-22 13:02:02 UTC
https://bitbucket.org/eigen/eigen/commits/8d6627a538a7/

Note You need to log in before you can comment on or make changes to this bug.