Teuchos::{Array,ArrayView} Fix #2749 #4332
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
@trilinos/teuchos Teuchos::Array and Teuchos::ArrayView now have a .data() method, for compatibility with std::vector and other C++ Standard Library classes (as of C++11), as well as Kokkos::View. I took the liberty to use std::vector::data in the implementation of Teuchos::Array instead of &vec[0], and to replace a few instances of 0 with nullptr. By analogy with std::shared_ptr<T[]>, I did _not_ add a .data() method to Teuchos::ArrayRCP. The whole point of the new methods is to increase compatibility with the C++ Standard Library. Here is a table of correspondences between Teuchos Memory Management Classes and C++ Standard Library classes: Teuchos class | is like ============================================ Teuchos::Array | std::vector Teuchos::ArrayView | std::span (C++20 draft) Teuchos::ArrayRCP | std::shared_ptr<T[]>
mhoemmen
requested review from
ibaned,
bartlettroscoe,
dsunder and
ndellingwood
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
Using Repos:
Pull Request Author: mhoemmen |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_4.8.4 # 2379 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_intel_17.0.1 # 2180 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_4.9.3_SERIAL # 671 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0 # 300 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_cuda_9.2 # 83 (click to expand) |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
Using Repos:
Pull Request Author: mhoemmen |
bartlettroscoe
approved these changes
Feb 6, 2019
There was a problem hiding this comment.
Just have a question about doxygen documentation and a suggestion for reducing duplication. But otherwise, this looks good.
@mhoemmen, thanks for taking the time to add all of these tests!
| @@ -407,9 +407,19 @@ class Array | |||
| /** \brief Return a raw pointer to beginning of array or NULL if unsized. */ | |||
| inline T* getRawPtr(); | |||
|
|
|||
| /// \brief Return a raw pointer to beginning of array. | |||
There was a problem hiding this comment.
@mhoemmen, does Doxygen render this comment correctly using ///?
There was a problem hiding this comment.
@bartlettroscoe I've had success doing that with Tpetra. Thanks for asking, though!
| template<class T> inline | ||
| T* ArrayView<T>::data() const | ||
| { | ||
| debug_assert_valid_ptr(); |
There was a problem hiding this comment.
To avoid duplication, why not just inline call this->getRawPtr()?
There was a problem hiding this comment.
@bartlettroscoe I wanted to keep the function call stack short so the compiler has a chance at inlining :-)
There was a problem hiding this comment.
@mhoemmen said:
I wanted to keep the function call stack short so the compiler has a chance at inlining :-)
Okay, I can accept that. Performance and maintainability are often at odds. This is pretty minor duplication. But it is fragile to someone removing the call to debug_assert_valid_ptr() and there are no unit tests that ensure that data() throws in debug mode for null or invalid pointers (like dangling references). So technically, there is now a lack of testing.
There was a problem hiding this comment.
@bartlettroscoe Not true -- line 353 of teuchos/core/test/MemoryManagement/ArrayView_UnitTests.cpp tests that .data() throws in debug mode on dangling reference.
There was a problem hiding this comment.
Thanks! I missed that looking over the tests.
| template<class T> inline | ||
| const T* ArrayView<const T>::data() const | ||
| { | ||
| debug_assert_valid_ptr(); |
There was a problem hiding this comment.
Same deal here. To void duplication, why not just call this->getRawPtr()?
|
Thanks Ross! |
mhoemmen
commented
Feb 6, 2019
| @@ -334,6 +350,7 @@ TEUCHOS_UNIT_TEST_TEMPLATE_1_DECL( ArrayView, danglingView_rcp_std_vector, T ) | |||
| } | |||
| #ifdef TEUCHOS_DEBUG | |||
| TEST_THROW(av.getRawPtr(), DanglingReferenceError); | |||
| TEST_THROW(av.data(), DanglingReferenceError); | |||
There was a problem hiding this comment.
@bartlettroscoe Here's the unit test that .data() does the same debug-mode dangling reference testing that .getRawPtr() does.
There was a problem hiding this comment.
Okay, I missed that. Thanks for adding that! I think we are 100% good then.
| @@ -334,6 +350,7 @@ TEUCHOS_UNIT_TEST_TEMPLATE_1_DECL( ArrayView, danglingView_rcp_std_vector, T ) | |||
| } | |||
| #ifdef TEUCHOS_DEBUG | |||
| TEST_THROW(av.getRawPtr(), DanglingReferenceError); | |||
| TEST_THROW(av.data(), DanglingReferenceError); | |||
There was a problem hiding this comment.
Okay, there is the dangling reference check
There was a problem hiding this comment.
But there are two data() functions (const and a non-const function). Do both get tested?
There was a problem hiding this comment.
@bartlettroscoe I think only one gets tested. If you like, I can delay this PR and push another commit that fixes this.
There was a problem hiding this comment.
I think only one gets tested. If you like, I can delay this PR and push another commit that fixes this.
@mhoemmen, no need for delay. I think this missing test did not show in the 100% coverage because all lines of code are covered but that does not technically guarantee full coverage of behavior.
Thanks for adding these tests.
Cross fingers the PR tester will not fail again.
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_4.8.4 # 2385 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_intel_17.0.1 # 2185 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_4.9.3_SERIAL # 676 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0 # 305 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_cuda_9.2 # 88 (click to expand) |
bartlettroscoe
added
the
AT: RETEST
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
Using Repos:
Pull Request Author: mhoemmen |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_4.8.4 # 2390 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_intel_17.0.1 # 2190 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_4.9.3_SERIAL # 681 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0 # 311 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_cuda_9.2 # 93 (click to expand) |
trilinos-autotester
removed
the
AT: RETEST
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
Using Repos:
Pull Request Author: mhoemmen |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_4.8.4 # 2401 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_intel_17.0.1 # 2201 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_4.9.3_SERIAL # 691 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0 # 322 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_cuda_9.2 # 103 (click to expand) |
mhoemmen
added
the
AT: RETEST
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
Using Repos:
Pull Request Author: mhoemmen |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
|
trilinos-autotester
removed
the
AT: RETEST
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ bartlettroscoe ]! |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@trilinos/teuchos
Teuchos::ArrayandTeuchos::ArrayViewnow have a.data()method, forcompatibility with
std::vectorand other C++ Standard Library classes(as of C++11), as well as
Kokkos::View.I took the liberty to use
std::vector::datain the implementation ofTeuchos::Arrayinstead of&vec[0], and to replace a few instances of0(in a pointer context) with
nullptr. Note that the Teuchos MemoryManagement Classes always return
nullptrif the array length is zero;std::vectordoes not promise that.By analogy with
std::shared_ptr<T[]>, I did not add a.data()methodto
Teuchos::ArrayRCP. The whole point of the new methods is toincrease compatibility with the C++ Standard Library. Here is a table
of correspondences between Teuchos Memory Management Classes and C++
Standard Library classes:
Arraystd::vectorArrayViewstd::span(C++20 draft)ArrayRCPstd::shared_ptr<T[]>Related Issues