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/matlab 2022 macos #513

Merged
merged 3 commits into from
Feb 24, 2023
Merged

Conversation

tschuldigung
Copy link
Contributor

This is the PR for #512

- Adding empty implementation for class CompileTimeError
- Chosing correct variable type for matlab based access loop
Copy link
Collaborator

@hmaarrfk hmaarrfk 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 the find.

*.orig
*.mexmaci64
src/matlab/vigraBoundaryTensor.m
Copy link
Collaborator

Choose a reason for hiding this comment

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

are these types of m files automatically generated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, auto generated by Matlab . I added them to .gitignore to prevent checkin by accident

@@ -170,13 +170,13 @@ void vigraMexFunction(vigra::matlab::OutputArray outputs, vigra::matlab::InputAr
}
}

/*+++++++++++++++++++++++MexEntryFunc++++++++++++++++++++++++++++++++*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you please undo these purely stylist changes.

git checkout master -- src/matlab/vigraDistance.cpp
git checkout master -- src/matlab/vigraResize2.cpp
git checkout master -- src/matlab/vigraResize3.cpp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's no stylist changes - the original version creates compiler warnings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but ok - if you insist I will...

Copy link
Collaborator

Choose a reason for hiding this comment

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

the mex compiler doesn't like multiline comments? i'm maybe out of touch. sorry.

Can you copy paste the warning again?

Copy link
Contributor Author

@tschuldigung tschuldigung Oct 22, 2022

Choose a reason for hiding this comment

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

RIght, I should have listed the warning in the first place. These warnings appear for almost every .cpp file within the Matlab folder

vigraDistance.cpp
compiling: vigraDistance.cpp
mex -O -I../../include  -outdir '.' ./vigraDistance.cpp
Building with 'Xcode Clang++'.
vigraDistance.cpp:175:1: warning: '/*' within block comment [-Wcomment]
/* if a certain class is NOT supported - you will have to copy the
^
vigraDistance.cpp:176:1: warning: '/*' within block comment [-Wcomment]
/* body of the callMexFunctor function and edit it here.
^
vigraDistance.cpp:177:1: warning: '/*' within block comment [-Wcomment]
/* Supports (u)int[8|16|32|64], float and double.
^
vigraDistance.cpp:178:1: warning: '/*' within block comment [-Wcomment]
/*+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++*/
^
4 warnings generated.

MEX completed successfully.
creating: vigraDistance.m

Copy link
Collaborator

Choose a reason for hiding this comment

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

ohhhh i see, they were opening a comment /* many times within the comment itself.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for the fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi Marc @hmaarrfk ! Can we merge this fix into master?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes. i was looking at this last night. hsould be ok.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thank you for your patience

@hmaarrfk hmaarrfk mentioned this pull request Oct 22, 2022
19 tasks
@hmaarrfk hmaarrfk merged commit 75ddad1 into ukoethe:master Feb 24, 2023
@tschuldigung
Copy link
Contributor Author

Thank you for your review!

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