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

fix unittest in gcc13 #40

Merged
merged 2 commits into from
Jul 22, 2023
Merged

fix unittest in gcc13 #40

merged 2 commits into from
Jul 22, 2023

Conversation

coyorkdow
Copy link
Contributor

@coyorkdow coyorkdow commented Jul 8, 2023

GCC13 added a new warning Wself-move (regarding info https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81159). Therefore, the relevant codes in unittest will throw errors as -Werror is opened.

Add a diagnostic pragma to remove the errors.

Copy link
Collaborator

@mingxwa mingxwa left a comment

Choose a reason for hiding this comment

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

Thank you for reporting the issue and proposing the fix! Please refer to the inline comments.

In addition, since this one should repro in GCC 13 while our pipeline is running the tests against GCC 11, we appreciate your help if you could help update the pipeline to use GCC 13 by default, or file an issue to track this if you are not familiar with GitHub Actions.

tests/proxy_lifetime_tests.cpp Outdated Show resolved Hide resolved
@coyorkdow
Copy link
Contributor Author

@microsoft-github-policy-service agree

@coyorkdow
Copy link
Contributor Author

Thank you for reporting the issue and proposing the fix! Please refer to the inline comments.

In addition, since this one should repro in GCC 13 while our pipeline is running the tests against GCC 11, we appreciate your help if you could help update the pipeline to use GCC 13 by default, or file an issue to track this if you are not familiar with GitHub Actions.

A new commit is appened to this MR to update the ci to gcc-13. plz review it :)

@coyorkdow
Copy link
Contributor Author

@mingxwa

Nit: Since the lines above looks similar to the new block, can we merge them to make the block shorter?

I found that it seems clang is compatiable with #pragma GCC, so we can simply use same pragma for both clang and gcc.

#if defined(__clang__) || (defined(__GNUC__) && __GNUC__ >= 13)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wself-move"
#endif
    p = std::move(p);
#if defined(__clang__) || (defined(__GNUC__) && __GNUC__ >= 13)
#pragma GCC diagnostic pop
#endif

But I am not sure if it will affect the portability.

Copy link
Collaborator

@mingxwa mingxwa left a comment

Choose a reason for hiding this comment

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

LGTM

@coyorkdow
Copy link
Contributor Author

coyorkdow commented Jul 11, 2023

Should we merge the pragmas of clang and gcc or just keep them unchanged? @mingxwa

@coyorkdow
Copy link
Contributor Author

@mingxwa Hi. Can it be merged?

@mingxwa
Copy link
Collaborator

mingxwa commented Jul 22, 2023

@coyorkdow Sorry for the delayed reply. Given that -Wself-move is the only overlap between clang and GCC for now (and GCC requires a version), I suggest checking-in as-is. If there are more overlaps in the future, we can refactor in a separate PR.

@coyorkdow
Copy link
Contributor Author

@mingxwa Got it. Does this PR is already to merge? Or any modification do I need to do?

@mingxwa mingxwa merged commit b8ddbd3 into microsoft:main Jul 22, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants