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

Patch 1.49.3 for geodesic package. #868

Merged
merged 3 commits into from Mar 19, 2018
Merged

Patch 1.49.3 for geodesic package. #868

merged 3 commits into from Mar 19, 2018

Conversation

cffk
Copy link
Contributor

@cffk cffk commented Mar 17, 2018

Set flags for Intel compiler to prevent incorrect optimization of
arithmetic expressions #826. Guard against nans in sincosdx #843.
Issue #831 is not addressed here (need more information...).

@kbevers and @rouault I flagged some of the changes made to
satisfy cppcheck, e.g.,

    if ((outmask & GEOD_LATITUDE) && plat2) /* plat2 check redundant */
      *plat2 = lat2;

The outmask test makes the plat2 test unnecessary. So, if possible, I'd
like to back out of this change and revert to

    if (outmask & GEOD_LATITUDE) 

Sorry not to have caught this earlier.

Set flags for Intel compiler to prevent incorrect optimization of
arithmetic expressions OSGeo#826.  Guard against nans in sincosdx OSGeo#834.
Issue OSGeo#831 is not addressed here (need more information...).
@cffk
Copy link
Contributor Author

cffk commented Mar 17, 2018

The setting of the flags for the Intel compiler has been tested on Linux only (cmake + autoconf). I've specified the right flag for the Intel compiler on Windows (with cmake), but I haven't tested this.

@rouault
Copy link
Member

rouault commented Mar 17, 2018

The outmask test makes the plat2 test unnecessary.

This is actually not for cppcheck but for clang static analyzer, for which I don't know how to put suppressions as annotations in the code. This was so immediately obvious to me that the test is unnecessary. CSA is probably confused by the line "outmask &= l->caps & OUT_ALL;" which modifies the flags, but as and masking can only remove a bit, indeed the current code is correct, but it needs a (too) clever brain to realize it. I'd vote to let the additional check. If not, some exception should be added in the last line of travis/csa/install.sh

@cffk
Copy link
Contributor Author

cffk commented Mar 17, 2018

@rouault OK, I'll let it be. Maybe I'll change the comment to put the blame on CSA. (You'll see that I did the same with the initial assignments. Perhaps you can check that I blamed the right party!)

but I doubt this will fix the build failures on the CI machines.
@cffk
Copy link
Contributor Author

cffk commented Mar 18, 2018

I can't easily debug the configuration failures on the CI machines. autogen.sh and configure work OK for me (Intel and gcc). I suspect something needs to be done to bring AX_CHECK_COMPILE_FLAG into scope. Worst comes to worst, I remove the changes to configure.ac and Intel is only supported via cmake.

@QuLogic
Copy link
Contributor

QuLogic commented Mar 18, 2018

You need to add the file defining the AX_CHECK_COMPILE_FLAG macro into the m4 directory (the autoconf archive is all copylibs).

@cffk
Copy link
Contributor Author

cffk commented Mar 18, 2018

@QuLogic Thanks that did it.
@mwtoews Perhaps you could verify that I've got the Intel flags right on as many platforms as possible (+ configure + cmake). Thanks.

@cffk cffk merged commit a2c0a41 into OSGeo:master Mar 19, 2018
@kbevers kbevers added this to the 5.0.1 milestone Mar 19, 2018
kbevers pushed a commit that referenced this pull request Mar 19, 2018
Patch 1.49.3 for geodesic package.

Closes #826, partially closes #843.
@kbevers
Copy link
Member

kbevers commented Mar 19, 2018

cherry-picked into 5.0 branch

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.

None yet

4 participants