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.

Sign up for GitHub

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

Jump to bottom

Fix if/ifdef WIN32 issues #3668

Merged
merged 2 commits into from
Feb 25, 2020
Merged

Fix if/ifdef WIN32 issues #3668

merged 2 commits into from
Feb 25, 2020

Conversation

FSund
Copy link
Contributor

@FSund FSund commented Feb 24, 2020 •
edited by kunaltyagi
Loading

Fixes #3667

Replaced all occurrences (a scary sentence) of #if WIN32, #if _WIN32 and other variants with the proper #ifdef or #ifndef, in all *.cpp, *.h and *.hpp files, as discussed in #3667.

I'm not sure if I was supposed to touch _WIN32 occurrences, but have done so anway.

Please do check my work.

@FSund FSund mentioned this pull request Feb 24, 2020
Closed
@kunaltyagi
Copy link
Member

kunaltyagi commented Feb 24, 2020 •
edited
Loading

Thanks @FSund for the PR

Apparently, WIN32 is an error. There is no such macro. Only _WIN32 exists. This is a handy guide. I have no idea if other such errors are lurking around 😱

kunaltyagi
kunaltyagi reviewed Feb 24, 2020
View reviewed changes
io/src/pcd_grabber.cpp Outdated Show resolved Hide resolved
@FSund @kunaltyagi
Replaced WIN32 with _WIN32
3a81d2b 
Co-Authored-By: Kunal Tyagi <tyagi.kunal@live.com>
@FSund
Copy link
Contributor Author

FSund commented Feb 24, 2020

I'm pretty sure I saw some #define WIN32 somewhere along the way when replacing the others, so there might be some instances of WIN32 internally. I'll have a look tomorrow.

@FSund
Copy link
Contributor Author

FSund commented Feb 25, 2020

I went ahead and checked for any #if WIN64 and #if _WIN64 issues, but couldn't find any, so that's good.

Any idea why the tests are failing? I'm not that familiar with how to read those Azure logs.

@kunaltyagi
Copy link
Member

#3656 is the cause. Don't worry

@kunaltyagi kunaltyagi merged commit b1d9c7e into PointCloudLibrary:master Feb 25, 2020
@FSund FSund deleted the fix_if_win32 branch February 25, 2020 09:54
@taketwo
Copy link
Member

taketwo commented Feb 25, 2020

Offtopic: @kunaltyagi I think it makes sense if this PR is included in the changelog. If we make changelog inclusion "opt-in", then this needs to be labeled somehow. But this is neither of "changelog: x" that we have at the moment. Do we need something like "changelog: fix"?

@kunaltyagi
Copy link
Member

I'd say that instead of manually creating the changelog, have sufficient labels to enable automatic generation. It doesn't take long to label each PR. Would speed up releases. Would be a giant help in automatic releases (eg: Azure has a release pipeline which can be created and triggered whenever you'd want a new release) since there's no (maybe reduced) manual work

@kunaltyagi kunaltyagi added the changelog: fix Meta-information for changelog generation label Feb 25, 2020
FSund added a commit to FSund/pcl that referenced this pull request Mar 16, 2020
@FSund @kunaltyagi
Fixed if/ifdef WIN32 issues (PointCloudLibrary#3668)
61eb9d2 
* Fixed if/ifdef WIN32

* Replaced WIN32 with _WIN32

Co-authored-by: Kunal Tyagi <tyagi.kunal@live.com>
@taketwo taketwo changed the title Fixed if/ifdef WIN32 issues Fix if/ifdef WIN32 issues Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taketwo taketwo taketwo approved these changes

@kunaltyagi kunaltyagi kunaltyagi approved these changes

Assignees
No one assigned
Labels
changelog: fix Meta-information for changelog generation module: apps module: io platform: windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error with "#if WIN32" in pcd_grabber.cpp?
3 participants
@FSund @kunaltyagi @taketwo