Skip to content

Comments

Fix index handling in mismatch branch to remove side effects and improve maintainability#45

Closed
AADARSHPUROHIT466 wants to merge 0 commit intognu-octave:defaultfrom
AADARSHPUROHIT466:fix-reset-j-mismatch
Closed

Fix index handling in mismatch branch to remove side effects and improve maintainability#45
AADARSHPUROHIT466 wants to merge 0 commit intognu-octave:defaultfrom
AADARSHPUROHIT466:fix-reset-j-mismatch

Conversation

@AADARSHPUROHIT466
Copy link
Contributor

This patch addresses a maintainability issue in liboctave/array/Array-base.cc
where the index variable j was incremented inside the right-hand operand of
a logical && operator, introducing side effects and violating MISRA/CERT guidelines.

Changes introduced:

  • Reset j = 0 before reuse in the mismatch branch.
  • Removed side effects from logical && operand by separating increment logic.
  • Added explicit bounds check before accessing rhdv.
  • Introduced temporary variable rhs_is_empty for clarity.
  • Added comments to improve readability and defensive programming practices.

Impact:

  • Ensures safe and predictable index handling.
  • Prevents potential invalid memory access.
  • Improves code readability and maintainability.
  • Aligns with MISRA/CERT compliance and passes SonarCloud Quality Gate.

// Bounds check before accessing rhdv
if (j < rhdvl)
{
bool rhs_is_empty = (rhdv[j] == 0); // safe access
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason that you changed this from operator() to operator[]?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched from operator() to operator[] because in this context rhdv is already guaranteed to be a flat array, and the explicit bounds check if (j < rhdvl) ensures safe access. Using operator[] avoids the extra overhead of operator() while still maintaining correctness. If consistency with the rest of the codebase is preferred, I can revert to operator().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I see.

To be honest, I can't recall the reason for that. But if dimension checks can be skipped, typically the xelem member function is used in Octave. Maybe, that could be done here, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve updated the code to use xelem(j) as suggested. This provides raw, efficient access after the explicit bounds check and aligns with Octave’s coding style. Please let me know if further adjustments are needed.

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
D Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants