-
Notifications
You must be signed in to change notification settings - Fork 453
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
Fixing XRT compilation Issue on Ubuntu 23.10 (issue #7806) #7834
Fixing XRT compilation Issue on Ubuntu 23.10 (issue #7806) #7834
Conversation
Signed-off-by: Homa Aghilinasab <haghilin@amd.com>
Signed-off-by: Homa Aghilinasab <haghilin@amd.com>
Just curious, why has it been closed? |
Hi Keryell, Yeah, sorry by mistake I added 5 files without change to the commit so was trying to remove them from the PR. Will create the PR again shortly. :) Thanks |
You can just reopen the PR and just force-push again onto the same branch instead. |
This reverts commit 7530f37.
Thanks Keryell for the help. I reverted the commit. It is now available for review. |
@@ -64,6 +64,10 @@ | |||
#include <utility> | |||
using namespace std::chrono_literals; | |||
|
|||
#ifdef __linux__ | |||
#pragma GCC diagnostic ignored "-Woverloaded-virtual" |
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.
Would it be possible to fix the root cause instead of make the compiler blind?
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.
Would it be possible to fix the root cause instead of make the compiler blind?
The warning isn't applicable here. A base class (B) has virtual methods (overloaded) foo()
and foo(int)
and some derived classes (D) redefine only a subset of these (say foo(int)
, so D->foo() won't work, but B->foo(10) does and the objects are used only through B. The warning is too aggressive IMO.
Build failed :( |
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.
Thank you. Let's not disable the warning in header files, targeted few .cpp is okay. Else pragma push/pop.
#ifdef __linux__ | ||
#pragma GCC diagnostic ignored "-Woverloaded-virtual" | ||
#endif |
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.
Not in header file, that disables for all compilation units that end up including this file.
#ifdef __linux__ | ||
#pragma GCC diagnostic ignored "-Wpessimizing-move" | ||
#endif |
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.
Ditto.
#ifdef __linux__ | ||
#pragma GCC diagnostic ignored "-Wpessimizing-move" | ||
#endif |
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.
Ditto
Thanks. With your branch I can compile but I hit later this issue:
|
This PR cannot be built due to long branch name. I created another PR #7841. |
Problem solved by the commit:
There was compilation issues on Ubuntu 23.10 which was reported in issue #7806.
Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered
issue #7806
How problem was solved, alternative solutions (if any) and why they were rejected
Risks (if any) associated the changes in the commit
What has been tested and how, request additional testing if necessary