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 unknown pragma warning on MSVC in test_type_traits.cpp #4061

Merged

Conversation

SunBlack
Copy link
Contributor

@SunBlack SunBlack commented May 7, 2020

Should fix MSVC warning on Azure:

D:\a\1\s\test\common\test_type_traits.cpp(56): warning C4068: unknown pragma [D:\a\build\test\common\test_type_traits.vcxproj]
D:\a\1\s\test\common\test_type_traits.cpp(57): warning C4068: unknown pragma [D:\a\build\test\common\test_type_traits.vcxproj]
D:\a\1\s\test\common\test_type_traits.cpp(59): warning C4068: unknown pragma [D:\a\build\test\common\test_type_traits.vcxproj]

@SergioRAgostinho
Copy link
Member

If the pragmas are not enclosed inside if-guards, then clang will complain about not recognizing #pragma warning(push). The osx build should confirm this.

@SunBlack
Copy link
Contributor Author

SunBlack commented May 7, 2020

If the pragmas are not enclosed inside if-guards, then clang will complain about not recognizing #pragma warning(push). The osx build should confirm this.

Oh really? I just copy & pasted it from here and there is no #ifdef around

/** \brief Copy constructor.
* \param[in] pc the cloud to copy into this
* \todo Erase once mapping_ is removed.
*/
// Ignore unknown pragma warning on MSVC (4996)
#pragma warning(push)
#pragma warning(disable: 4068)
// Ignore deprecated warning on clang compilers
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
PointCloud (const PointCloud<PointT> &pc) = default;
#pragma clang diagnostic pop
#pragma warning(pop)

@SunBlack
Copy link
Contributor Author

SunBlack commented May 7, 2020

ok @SergioRAgostinho you are right, macOS build is failing here (but why the hell not in pointcloud.h)?

But instead of fixing it by:

    // Ignore unknown pragma (C4068) warning on MSVC
    #ifdef _MSC_VER
    #pragma warning(push)
    #pragma warning(disable: 4068)
    #endif
    #pragma clang diagnostic push
    #pragma clang diagnostic ignored "-Wunused-local-typedef"
    PCL_MAKE_ALIGNED_OPERATOR_NEW
    #pragma clang diagnostic pop
    #ifdef _MSC_VER
    #pragma warning(pop)
    #endif

I could even use the shorter variant:

    #ifdef __clang__
    #pragma clang diagnostic push
    #pragma clang diagnostic ignored "-Wunused-local-typedef"
    #endif
    PCL_MAKE_ALIGNED_OPERATOR_NEW
    #ifdef __clang__
    #pragma clang diagnostic pop
    #endif

@SunBlack SunBlack force-pushed the fix_msvc_unknown_pragma_warning branch from 6ec3e78 to 9115616 Compare May 7, 2020 12:42
@SergioRAgostinho
Copy link
Member

ok @SergioRAgostinho you are right, macOS build is failing here (but why the hell not in pointcloud.h)?

No clue. I really did not dig deep. It was more reporting what I experienced in the past.

The cleaner, the best. Go for it.

@stale
Copy link

stale bot commented Jun 6, 2020

Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this.

@stale stale bot added the status: stale label Jun 6, 2020
@kunaltyagi
Copy link
Member

Could you please rebase and trigger another rebuild?

@stale stale bot removed the status: stale label Jun 7, 2020
@SunBlack SunBlack force-pushed the fix_msvc_unknown_pragma_warning branch from 9115616 to 4a0a491 Compare June 15, 2020 14:17
@SunBlack
Copy link
Contributor Author

Done. Sorry for late response - didn't had time first and later our admins changed some certificates and my second PC didn't liked it (when I connected via RDP to the PC he lost the internet connection due to issues from 802.1X 😢 - hope it works now finally as before)

@kunaltyagi kunaltyagi merged commit 7004787 into PointCloudLibrary:master Jun 16, 2020
@SunBlack SunBlack deleted the fix_msvc_unknown_pragma_warning branch June 16, 2020 12:21
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.

3 participants