-
Notifications
You must be signed in to change notification settings - Fork 564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ROL: correct warnings in examples and tests #2364
Conversation
The tests were mostly signed/unsigned comparisons which I cast to match. I did consider changing the types, but other comparisons in the same code would have required a cast in the opposite direction so that was pointless. The example code was complaining about unused variables. I did lleave those in place but commented out so that the example is still there but the compiler does not complain. If someone wanted to instead use those in some way that would be good. closes issue trilinos#2284
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: prwolfe |
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... |
@prwolfe I created a ROL issue to mirror this PR: trilinos/trilinos.github.io#10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle this looks fine, but I have a question. We would like to use more C++11 features in ROL, and have started converting some code to use 'auto', for example. Is there a cleaner way to fix these type mismatch issues by using 'auto'?
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... |
@dridzal - I am not sure that auto would help here as no matter what the compiler assigned at least one comparison would need a cast. In this case an unsigned int is being compared to both signed and unsigned int's. Nice idea otherwise (I have normally used this for pointers instead of integral types but it should work.) |
Warnings due to signed/unsigned comparisons have been an ongoing issue since there have been many instances in the code where containers like std::vector have been indexed with with int type (probably because ROL::Vector::dimension() returns int). The correct type for indexing is std::vector::size_type, which is unsigned and is the type that the size() method returns in the (for loop) comparisons. auto can be used to loop over vectors quite easily if you only want the elemental values and don't explicitly need the index. You can do
but something like
doesn't solve the problem because the auto type deduction is set by the i=0 assignment which is still an int (you could do i=0u I guess). A better approach if you are doing something where using the iterator would be clunky would be something like
It would be nice to ultimately allow a generic integral type for dimension and basis. I already pushed the unused warning fix for the tempus example_01 to the ROL dev fork last week. Otherwise, these look fine to me. |
If @gregvw has already pushed these changes in the standalone ROL repo, then we may get a conflict when merging the ROL and Trilinos repos. To provide some background, ROL development takes place in a separate private github repo (essentially a snapshot of Trilinos), and we occasionally perform a two-way merge with Trilinos proper. Don't know how to proceed considering the potential merge conflict. |
Is there a "line item veto" for pull requests? |
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ dridzal ]! |
Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - Master Automerge is disabled (in .cfg file) |
No line item veto that I know of. I have the PR open for editing by contributors if you wanted to do that, but if you are more comfortable pulling it into your workflow I am fine with that as long as it makes it's way into develop at some point. |
However you choose to do this, please please run with |
Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - Master Automerge is disabled (in .cfg file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks ok to me. We can handle the merge conflict in the tempus example when we sync.
I've applied the suggested changes directly to the code in the ROL repo, and am performing a sync with the Trilinos repo right now. If the sync succeeds, we can ignore the PR and close the issue. |
@gregvw If you know the right type, just use it for the index variable. Otherwise, use |
There is a build to watch and I looked this morning at https://testing.sandia.gov/cdash/index.php?project=Trilinos&parentid=3437562 and ROL is clean. I am abandoning and closing this and the issue as well. Thanks! |
The tests were mostly signed/unsigned comparisons
which I cast to match. I did consider changing
the types, but other comparisons in the same code
would have required a cast in the opposite direction
so that was pointless. The example code was
complaining about unused variables. I did lleave those
in place but commented out so that the example is still
there but the compiler does not complain. If someone
wanted to instead use those in some way that would be good.
closes issue #2284
@trilinos/
Description
Motivation and Context
Related Issues
How Has This Been Tested?
Screenshots
Checklist
Additional Information