Fix wrong component comparison in SpecularReflationPos.Cor. #19262

Merged
merged 2 commits into from Mar 30, 2017

Conversation

@SimonHeybrock
Contributor

SimonHeybrock commented Mar 29, 2017

The old code compared the parent shared_ptr. However, IComponent::getParent returns a newly allocated IComponent, so this gives nonsense results. Instead the base component must be compared.

Miraculously, the old implementation mostly worked. The reason is most likely that the old pointer immediately went out of scope and the next loop iteration would typically allocate the next parent in the same memory location.

Marked as high priority since it influences builds of all PRs.

To test:

Code review.

No issue, fixes regular test failures on Linux build servers.

No release notes.


Reviewer

Please comment on the following (full description):

Code Review
  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards? Is it well structured with small focussed classes/methods/functions?
  • Are there unit/system tests in place? Are the unit tests small and test the a class in isolation?
  • If there are changes in the release notes then do they describe the changes appropriately?
Functional Tests
  • Do changes function as described? Add comments below that describe the tests performed?

  • How do the changes handle unexpected situations, e.g. bad input?

  • Has the relevant documentation been added/updated?

  • Is user-facing documentation written in a user-friendly manner?

  • Has developer documentation been updated if required?

  • Does everything look good? Comment with the ship it emoji but don't merge. A member of @mantidproject/gatekeepers will take care of it.

@OwenArnold

This comment has been minimized.

Show comment
Hide comment
@OwenArnold

OwenArnold Mar 29, 2017

Member

Based on the behaviour of getComponentID() I think that this will not be correctly comparing the same pointers. i.e. always the base pointer.

Member

OwenArnold commented Mar 29, 2017

Based on the behaviour of getComponentID() I think that this will not be correctly comparing the same pointers. i.e. always the base pointer.

@OwenArnold OwenArnold referenced this pull request Mar 29, 2017

Merged

ComponentInfo #19153

0 of 9 tasks complete
@SimonHeybrock

This comment has been minimized.

Show comment
Hide comment
@SimonHeybrock

SimonHeybrock Mar 29, 2017

Contributor

@OwenArnold I am not sure I understand your comment?

Contributor

SimonHeybrock commented Mar 29, 2017

@OwenArnold I am not sure I understand your comment?

@OwenArnold

This comment has been minimized.

Show comment
Hide comment
@OwenArnold

OwenArnold Mar 29, 2017

Member

Based on the behaviour of getComponentID() I think that this will NOW be correctly comparing the same pointers. i.e. always the base pointer. Which is what should happen.

Member

OwenArnold commented Mar 29, 2017

Based on the behaviour of getComponentID() I think that this will NOW be correctly comparing the same pointers. i.e. always the base pointer. Which is what should happen.

@rosswhitfield

This comment has been minimized.

Show comment
Hide comment
@rosswhitfield

rosswhitfield Mar 29, 2017

Member

👍 Also confirming that the SpecularReflectionPositionCorrectTest now passes consistently.

Member

rosswhitfield commented Mar 29, 2017

👍 Also confirming that the SpecularReflectionPositionCorrectTest now passes consistently.

Re #0. Fix more issues in hasCommonParent for Spec.Refl.Pos.Cor.
nullptr parents are not the same, and cannot be dereferenced.
- bool sameParentComponent = true;
- auto lastParentComponent = detectors[0]->getParent()->getComponentID();
+ auto firstParent = detectors[0]->getParent();
+ if (!firstParent)

This comment has been minimized.

@OwenArnold

OwenArnold Mar 30, 2017

Member

nullptr check looks to make things safer. Was this causing things to fail?

@OwenArnold

OwenArnold Mar 30, 2017

Member

nullptr check looks to make things safer. Was this causing things to fail?

This comment has been minimized.

@SimonHeybrock

SimonHeybrock Mar 30, 2017

Contributor

@OwenArnold Yes, the OFFSPECReflRedOneAutoPolarizationCorrection system test.

@SimonHeybrock

SimonHeybrock Mar 30, 2017

Contributor

@OwenArnold Yes, the OFFSPECReflRedOneAutoPolarizationCorrection system test.

This comment has been minimized.

@OwenArnold

OwenArnold Mar 30, 2017

Member

👍

@OwenArnold

This comment has been minimized.

Show comment
Hide comment
@OwenArnold

OwenArnold Mar 30, 2017

Member

Latest round of changes look sensible. If tests pass :shipit: when green system tests.

Member

OwenArnold commented Mar 30, 2017

Latest round of changes look sensible. If tests pass :shipit: when green system tests.

@ianbush ianbush self-assigned this Mar 30, 2017

@ianbush ianbush merged commit 51850cd into master Mar 30, 2017

9 checks passed

ClangFormat Jenkins build pull_requests-clang-format 12441 has succeeded
Details
Doxygen Jenkins build pull_requests-doxygen 11770 has succeeded
Details
Flake8 Jenkins build pull_requests-flake8 3093 has succeeded
Details
OSX Jenkins build pull_requests-osx 12957 has succeeded
Details
RHEL7 + System Tests Jenkins build pull_requests-rhel7 12839 has succeeded
Details
Ubuntu + Doc Tests Jenkins build pull_requests-ubuntu 13434 has succeeded
Details
Ubuntu Python 3 Jenkins build pull_requests-ubuntu-python3 977 has succeeded
Details
Windows Jenkins build pull_requests-win7 13702 has succeeded
Details
cppcheck Jenkins build pull_requests-cppcheck 13428 has succeeded
Details

@ianbush ianbush deleted the fix_SpecularReflectionPositionCorrect branch Mar 30, 2017

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