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

64-bit compatibility issue in muParserBase.h #62

Closed
VioletGiraffe opened this issue Jan 7, 2019 · 11 comments
Closed

64-bit compatibility issue in muParserBase.h #62

VioletGiraffe opened this issue Jan 7, 2019 · 11 comments

Comments

@VioletGiraffe
Copy link

 void  Error(EErrorCodes a_iErrc, 
                int a_iPos = (int)mu::string_type::npos, // <--- This line doesn't look safe
                const string_type &a_strTok = string_type() ) const;

std::string::npos is size_type, it's a 64-bit value in 64-bit mode. The highlighted line truncates it to 32-bit signed int. It's probably not causing any actual runtime errors, but looks bad and causes a host of compiler warnings (remember, it's in .h file so a warning is emitted for every .cpp that includes it).

@beltoforion
Copy link
Owner

Should be -1 but it not really dangerous since strind::npos is defined to be -1. No truncation taking place.

@VioletGiraffe
Copy link
Author

I see what you mean, agreed. But MSVC does complain about truncation; the warning itself should be enough reason to fix this piece of code, IMO, regardless of how justified the warning is.

@jschueller
Copy link
Collaborator

@beltoforion could you register the project on appveyor to enable msvc builds so we can actually see what's going on ? (https://www.appveyor.com/)

The appveyor configuration was added in #38 but not actually used. If you want I can set it up for you if you grant me the rights to the repository.

@giraldeau
Copy link
Collaborator

I registered the project in my account, but there is no automatic trigger.

https://ci.appveyor.com/project/giraldeau/muparser

@beltoforion
Copy link
Owner

Sorry, I have no idea what appveyyor is or does. Never used it before.
@jschueller i have added you as a collaborator so if you want to set it up feel free to do it.

@jschueller
Copy link
Collaborator

Great, I could enable Appveyor, see the new README.rst.
It seems I need admin rights on the repo too to enable Travis though.

@jschueller
Copy link
Collaborator

@VioletGiraffe what of msvc version did you use ? any special flags ?

@VioletGiraffe
Copy link
Author

@jschueller: msvc 2017 (v141 toolset), /W4 flag.

@beltoforion
Copy link
Owner

@jschueller: giving admin rights does not seem to be so easy. Seems i need to create a new organization and transfer project ownership. (Something i would do if everything else fails)

I connected muparser to travis-ci can you access it there?

@jschueller
Copy link
Collaborator

Travis works properly, thanks.
Now I realize AppVeyor was only enabled on my fork and does not trigger builds from here. Could you do the same for AppVeyor ?

@beltoforion
Copy link
Owner

I added it to appveyor. Please check out if you see it.

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

No branches or pull requests

4 participants