Teuchos: throw exception at duplicate YAML names #2308
Conversation
|
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_autotester_test
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Using Repos:
Pull Request Author: ibaned |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_autotester_test
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
| @@ -277,8 +278,8 @@ class Reader : public Teuchos::Reader { | |||
| break; | |||
| } | |||
| case Teuchos::YAML::PROD_BMAP_FSEQ: { | |||
| TEUCHOS_ASSERT(rhs.at(4).type() == typeid(Array<Scalar>) || | |||
| rhs.at(4).type() == typeid(Array<Array<Scalar> >)); | |||
| TEUCHOS_ASSERT(rhs.at(4).type() == typeid(std::vector<Scalar>) || | |||
There was a problem hiding this comment.
@ibaned, why are all of these being switched over from Teuchos::Array to std::vector? Teuchos::Array just uses std::vector in non-debug checking mode but has very strong runtime checking in a debug-mode build.
There was a problem hiding this comment.
I just saw the comment "I also did some unrelated cleanup" in the description field.
Unless there is some technical reason to use std::vector instead of Teuchos::Array, the usage of Teuchos::Array should be kept for safety. Even in this context, std::vector::operator is completely unchecked. And we can't turn on checked STL because we need to link against C++ TPLs that are built without checked STL turned on. For the motivation for using Teuchos::Array over raw std::vector, see section "5.5.3 Teuchos::Array" in:
If you have questions about this, then let's talk.
There was a problem hiding this comment.
The motivation was to avoid having to define std::ostream& operator<<(std::ostream&, Scalar const&) and std::ostream& operator<<(std::ostream&, std::vector<Scalar>&). It is possible to access std::vector in a checked way with std::vector::at(), I can make sure to use that if thats the main issue. Otherwise if Teuchos::Array still sounds better then I can put all that back.
There was a problem hiding this comment.
The problem with std::vector::at() is that it always checks. There is no portable way to turn on and off array bounds checking with std::vector due to C++ TPLs that need to be linked in.
What is the issue with operator<<() exactly?
There was a problem hiding this comment.
It just seems like unnecessary boilerplate (recall the same issue with Teuchos::any). I used to have empty implementations for both. I can put them back just to get Teuchos::Array working just to have toggle-able bounds checking. Most of TeuchosParser already unconditionally calls std::vector::at since I don't consider it performance-critical.
There was a problem hiding this comment.
std::vector::at() does not completely make std::vector safe. YOu can also easily over-step and increment std::vector iterator returned from std::vector::begin(). The motivation for Teuchos::Array as an off-by-one error I made in a code several years ago that segfaulted in a very strange place. When I ran valgrind on it, it segfaulted inside of valgrind. It took me several days to find the off-by-one error with std::vector and I only found it by staring at the code for days. Teuchos::Array would have caught that defect in 1 sec.
| @@ -277,8 +278,8 @@ class Reader : public Teuchos::Reader { | |||
| break; | |||
| } | |||
| case Teuchos::YAML::PROD_BMAP_FSEQ: { | |||
| TEUCHOS_ASSERT(rhs.at(4).type() == typeid(Array<Scalar>) || | |||
| rhs.at(4).type() == typeid(Array<Array<Scalar> >)); | |||
| TEUCHOS_ASSERT(rhs.at(4).type() == typeid(std::vector<Scalar>) || | |||
There was a problem hiding this comment.
I just saw the comment "I also did some unrelated cleanup" in the description field.
Unless there is some technical reason to use std::vector instead of Teuchos::Array, the usage of Teuchos::Array should be kept for safety. Even in this context, std::vector::operator is completely unchecked. And we can't turn on checked STL because we need to link against C++ TPLs that are built without checked STL turned on. For the motivation for using Teuchos::Array over raw std::vector, see section "5.5.3 Teuchos::Array" in:
If you have questions about this, then let's talk.
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
|
|
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_autotester_test
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Using Repos:
Pull Request Author: ibaned |
There was a problem hiding this comment.
@ibaned, thanks for making the change back to Teuchos::Array. I just have lost too many days (and weeks) of my life tracking down undefined memory errors.
I wonder if there is some way that we can use C++11 features to avoid having to create dummy operators when they don't already exist for a type to be able to use these classes?
|
@bartlettroscoe we could use the same trick I used in |
|
Can we put in recursive checks for "Is comparable"? Can the check for "is comparable" for Teuchos::Array check to see if the type T is comparable and if not return false? |
|
Maybe... you may have to add a recursive definition for every existing container type, or come up with some common concept of a container they all satisfy. I'd say thats future work, and I intend to hit the merge button on this particular pull request unless someone objects. |
|
Hmm the autotester hasn't quite passed on the latest commit, so I did that a bit too early. Hopefully it does pass, otherwise we'll revert the merge. |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_autotester_test
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
|
|
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' - Pull Request MUST BE MERGED MANUALLY BY Project Team - Master Automerge is disabled (in .cfg file) |
Yes, I was not intending for this to hold up this PR. Thanks. |
[#2306]
@trilinos/teuchos
Description
Please see issue #2306 . I also did some unrelated cleanup.
Motivation and Context
@bathmatt requested that there should be an error (exception) thrown when the same name shows up twice in the same ParameterList. This is a useful debugging tool to catch user error.
Related Issues
How Has This Been Tested?
A custom build of Teuchos passed all tests.