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

Clean up of the XSPEC interface code #859

Merged
merged 6 commits into from Aug 7, 2020

Conversation

DougBurke
Copy link
Contributor

@DougBurke DougBurke commented Jul 20, 2020

Summary

Internal changes to the XSPEC interface code, which reduces the amount of similar (sometimes identical) code. There is no change to the behavior of the XSPEC models.

Details

Noted by @dtnguyen2 in a review of #772

I'm not likely to work on this anymore (modulo PR review) so we can either review this or through it.

This reduces the amount of duplication in the XSPEC extension
header.
Refactors the code used to create the grids for the convolution
models. This could have been made part of the general routine,
but the explicit lack of support for grids with gaps makes it
easier to have a separate routine.
There should be no change due to this.
@DougBurke
Copy link
Contributor Author

I've rebased this because of changes to the XSPEC interface (for XSPEC 12.11.0 support).

Instead of

  if a and not b
    declare symbol
  endif

  if b
  extern "C" { declare symbol }
  endif

use

  if a
  if b
    extern "C" {
  endif
  declare symbol
  if b
    }
  endif
  endif

Not sure what's cleaner.
@wmclaugh wmclaugh merged commit 4d0a506 into sherpa:master Aug 7, 2020
@DougBurke DougBurke deleted the update-xspec-bindings branch August 7, 2020 15:52
@DougBurke
Copy link
Contributor Author

It appears that this code barfs on the 98 C++ standard, which we still want to support easily (i.e. without forcing explicit calls to a newer standard).

@dtnguyen2
Copy link
Contributor

dtnguyen2 commented Aug 13, 2020

One could if def our way out of this mess:

#include <iostream>

int main(){
#if __cplusplus==201402L
  std::cout << "C++14" << std::endl;
#elif __cplusplus==201103L
  std::cout << "C++11" << std::endl;
#elif __cplusplus==199711L
  std::cout << "C++98" << std::endl;  
#else
  std::cout << "C++" << std::endl;
#endif
  
  return 0;
}

(base) [dtn@devel12 dtn]$ g++ -std=c++98 --pedantic tmp.cc; a.out
C++98
(base) [dtn@devel12 dtn]$ g++ -std=c++11 --pedantic tmp.cc; a.out
C++11

@wmclaugh
Copy link
Contributor

@DougBurke - I agree with Dan's suggestion of a conditional compile here to support the c++ 98 standard as well as the c++ 11 standard. The 11 standard provides improved performance and compatibility with other libraries built against the standard. Whether we would want to eventually drop the 98 standard is a separate issue as is the infrastructure changes to support specifying/propagating (commandline) compiler flag options.

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.

None yet

3 participants