Skip to content
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

Updated for_loop.hpp #5778

Merged
merged 21 commits into from Feb 19, 2022
Merged

Updated for_loop.hpp #5778

merged 21 commits into from Feb 19, 2022

Conversation

Alchemist1411
Copy link
Contributor

@Alchemist1411 Alchemist1411 commented Feb 10, 2022

This fixes the issue #5735 of for_loop function taking absurd values as arguments.

This fixes the issue of for_loop function taking absurd values as arguments.
@jenkins-cscs
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

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

In order for the test to be run you will need to modify the CMakeLists.txt here: https://github.com/STEllAR-GROUP/hpx/blob/master/libs/core/algorithms/tests/regressions/CMakeLists.txt

Also, please take care of the clang-format and inspect errors as flagged by the CI

@hkaiser hkaiser linked an issue Feb 11, 2022 that may be closed by this pull request
@hkaiser hkaiser added this to the 1.8.0 milestone Feb 11, 2022
@hkaiser hkaiser mentioned this pull request Feb 11, 2022
@gonidelis
Copy link
Contributor

gonidelis commented Feb 11, 2022

@deepaksuresh1411 As circleCI indicates and Hartmut already mentioned you need to clang-format your files. We provide a clang-format config file (.clang-format) on the top of our repo. You should clang-format -i -style=file <yourfiles> to format accordingly.

Hint: If you are using VS Code you can easily configure it to format every time you save a file. Just be careful not to use the defalt clang-format config cause tests will fail again.

Moreover, check your ci/circleci: inspect errors (I don't think clang-format fixes those). It's just a bunch of redundant (or missing whitespaces). Please fix accordingly ;)

@Alchemist1411
Copy link
Contributor Author

@deepaksuresh1411 As circleCI indicates and Hartmut already mentioned you need to clang-format your files. We provide a clang-format config file (.clang-format) on the top of our repo. You should clang-format -i -style=file <yourfiles> to format accordingly.

Hint: If you are using VS Code you can easily configure it to format every time you save a file. Just be careful not to use the defalt clang-format config cause tests will fail again.

Moreover, check your ci/circleci: inspect errors (I don't think clang-format fixes those). It's just a bunch of redundant (or missing whitespaces). Please fix accordingly ;)

@gonidelis I'll do it ASAP.

formatted the code according to clang-format
Formatted the file
@Alchemist1411
Copy link
Contributor Author

@hkaiser I did the clang-format , but in circle-ci tests, there is one issue related to #include <iostream> . But, the program still runs fine without iostream also.
testfail

@hkaiser
Copy link
Member

hkaiser commented Feb 12, 2022

@hkaiser I did the clang-format , but in circle-ci tests, there is one issue related to #include <iostream> . But, the program still runs fine without iostream also. testfail

Yes, that's the inspect error I referred to earlier. It asks you to add a #include <iostream> to your code as you're using std::cout on line 20.

Also, it complains about the SPDX license tag to be missing from the file (i.e. something like: https://github.com/STEllAR-GROUP/hpx/blob/master/libs/core/algorithms/include/hpx/parallel/algorithms/for_loop.hpp#L4)

@Alchemist1411
Copy link
Contributor Author

@hkaiser I did the clang-format , but in circle-ci tests, there is one issue related to #include <iostream> . But, the program still runs fine without iostream also. testfail

Yes, that's the inspect error I referred to earlier. It asks you to add a #include <iostream> to your code as you're using std::cout on line 20.

Also, it complains about the SPDX license tag to be missing from the file (i.e. something like: https://github.com/STEllAR-GROUP/hpx/blob/master/libs/core/algorithms/include/hpx/parallel/algorithms/for_loop.hpp#L4)

I'll, rectify it

@hkaiser
Copy link
Member

hkaiser commented Feb 12, 2022

Just in case if you can't access the CI results, here is the compiler output generated while compiling your test (taken from here: https://app.circleci.com/pipelines/github/STEllAR-GROUP/hpx/10384/workflows/43d53655-7cc1-40eb-b505-dcdd96b4d6a9/jobs/286870/steps):

/hpx/source/libs/core/algorithms/tests/regressions/for_loop_test.cpp:16:18: error: unused parameter 'argc' [clang-diagnostic-unused-parameter]
int hpx_main(int argc, char* argv[])
                 ^
/hpx/source/libs/core/algorithms/tests/regressions/for_loop_test.cpp:16:30: error: unused parameter 'argv' [clang-diagnostic-unused-parameter]
int hpx_main(int argc, char* argv[])
                             ^
/hpx/source/libs/core/algorithms/tests/regressions/for_loop_test.cpp:21:10: error: 'for_loop' is deprecated: hpx::for_loop is deprecated. Please use hpx::experimental::for_loop instead. (This functionality is deprecated starting HPX V1.8 and will be removed in the future. You can define HPX_HAVE_DEPRECATION_WARNINGS_V1_8=0 to acknowledge that you have received this warning.) [clang-diagnostic-deprecated-declarations]
    hpx::for_loop(hpx::execution::seq, start, end,
         ^
/hpx/source/libs/core/algorithms/include/hpx/parallel/algorithms/for_loop.hpp:1552:5: note: 'for_loop' has been explicitly marked deprecated here
    HPX_DEPRECATED_V(1, 8,
    ^
/hpx/source/libs/core/config/include/hpx/config/deprecation.hpp:113:5: note: expanded from macro 'HPX_DEPRECATED_V'
    HPX_PP_CAT(HPX_PP_CAT(HPX_PP_CAT(HPX_DEPRECATED_V, major), _), minor)(x)
    ^
/hpx/source/libs/core/preprocessor/include/hpx/preprocessor/cat.hpp:28:26: note: expanded from macro 'HPX_PP_CAT'
#define HPX_PP_CAT(a, b) HPX_PP_CAT_I(a, b)
                         ^
/hpx/source/libs/core/preprocessor/include/hpx/preprocessor/cat.hpp:36:28: note: expanded from macro 'HPX_PP_CAT_I'
#define HPX_PP_CAT_I(a, b) a##b
                           ^
note: expanded from here
/hpx/source/libs/core/config/include/hpx/config/deprecation.hpp:103:5: note: expanded from macro 'HPX_DEPRECATED_V1_8'
    HPX_DEPRECATED(x " (" HPX_PP_EXPAND(HPX_DEPRECATED_MSG_V1_8) ")")
    ^
/hpx/source/libs/core/config/include/hpx/config/attributes.hpp:69:31: note: expanded from macro 'HPX_DEPRECATED'
#  define HPX_DEPRECATED(x) [[deprecated(x)]]
                              ^

Please note that the deprecation warning was added just a couple of days ago, please change your test accordingly.

@Alchemist1411
Copy link
Contributor Author

@hkaiser I tried correcting the most part of failing tests. Now only two are showing . I could not figure this one out , it would be nice, If you could give a hint on this-> https://app.circleci.com/pipelines/github/STEllAR-GROUP/hpx/10396/workflows/3496082f-bbbe-4671-b355-ff91820e3828/jobs/287107/parallel-runs/0?invite=true#step-103-51

@gonidelis
Copy link
Contributor

gonidelis commented Feb 14, 2022

@deepaksuresh1411 Very nice! We have an ongoing issue with segmented tests so you shouldn't expect for them to pass.

Let's wait for the other workflows to finish (just initiated them).

Seems like this is good to go.

@Alchemist1411
Copy link
Contributor Author

Alchemist1411 commented Feb 15, 2022

@deepaksuresh1411 Very nice! We have an ongoing issue with segmented tests so you shouldn't expect for them to pass.

Let's wait for the other workflows to finish (just initiated them).

Seems like this is good to go.

@gonidelis Thanks :)
so, now should I have to do anything else.

@gonidelis
Copy link
Contributor

@deepaksuresh1411 Very nice! We have an ongoing issue with segmented tests so you shouldn't expect for them to pass.
Let's wait for the other workflows to finish (just initiated them).
Seems like this is good to go.

@gonidelis Thanks :) so, now should I have to do anything else.

Don't think so. Let's wait for @hkaiser to verify.

@Alchemist1411
Copy link
Contributor Author

@deepaksuresh1411 Very nice! We have an ongoing issue with segmented tests so you shouldn't expect for them to pass.
Let's wait for the other workflows to finish (just initiated them).
Seems like this is good to go.

@gonidelis Thanks :) so, now should I have to do anything else.

Don't think so. Let's wait for @hkaiser to verify.

Sounds good..

@Alchemist1411
Copy link
Contributor Author

@hkaiser is there anything else I have to do in the code.

Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@hkaiser
Copy link
Member

hkaiser commented Feb 19, 2022

bors merge

@bors
Copy link

bors bot commented Feb 19, 2022

👎 Rejected by code reviews

@hkaiser
Copy link
Member

hkaiser commented Feb 19, 2022

Congratulations, your first PR to the HPX repository has just been merged! As a 'thank you' we offer a free STE||AR-Group t-shirt to all of our first-time contributors. If you are interested in receiving one, please get back to me directly so we can set up the delivery.

@hkaiser hkaiser merged commit 45881ec into STEllAR-GROUP:master Feb 19, 2022
@Alchemist1411
Copy link
Contributor Author

@hkaiser @gonidelis @srinivasyadav18 @NK-Nikunj
Thanks to all for helping me out.

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

Successfully merging this pull request may close these issues.

hpx::for_loop executes without checking start and end bounds
5 participants