Skip to content

Conversation

@galanCA
Copy link
Contributor

@galanCA galanCA commented Jun 2, 2022

Description

Updates CNLOADS to build at all platforms.

Changes:

  • Hide the FPCHECK Macro
  • Change static_assert. non-constant condition for static assertion
  • Derefencing a null pointer *0

Replace the content in this section with:

  • The motivation and context for this change (if it is not immediately clear from the title)
  • If it fixes an open issue, specify the issue number (e.g., "fixes #XXXX")
  • A summary of the behavior expected from this change
  • A description of tests performed

Author Progress Checklist:

  • Open draft pull request
    • Make title clearly understandable in a standalone change log context
    • Assign yourself the issue
    • Add at least one label (enhancement, bug, or maintenance)
    • Link the issue(s) addressed by this PR (under "Development" in the sidebar menu)
  • Make code changes (if you haven't already)
  • Self-review of code
    • My code follows the style guidelines of this project
    • I have added comments to my code, particularly in hard-to-understand areas
    • I have only committed the necessary changes for this fix or feature
    • I have made corresponding changes to the documentation
    • My changes generate no new warnings
    • I have ensured that my fix is effective or that my feature works as intended by:
      • exercising the code changes in the test framework, and/or
      • manually verifying the changes (as explained in the the pull request description above)
    • My changes pass all local tests
    • My changes successfully passes CI checks
    • Add any unit test for proof and documentation.
    • Merge in main branch and address resulting conflicts and/or test failures.
  • Move pull request out of draft mode and assign reviewers
  • Iterate with reviewers until all changes are approved
    • Make changes in response to reviewer comments
    • Merge in main branch and address resulting conflicts and/or test failures.
    • Re-request review in GitHub

Reviewer Checklist:

  • Read the pull request description
  • Perform a code review on GitHub
  • Confirm all CI checks pass and there are no build warnings
  • Pull, build, and run automated tests locally
  • Perform manual tests of the fix or feature locally
  • Add any review comments, if applicable
  • Submit review in GitHub as either
    • Request changes, or
    • Approve
  • Iterate with author until all changes are approved

@galanCA galanCA changed the title Hide FPCHECK. CNLOADS upgrade Jun 2, 2022
src/CNLOADS.CPP Outdated
// fixed sequence allows array access by rs_mode (see code below)
// rsmHEAT/rsmCOOL/rsmOAV definitions must be consistent with member sequences.
#define QZONECHK( m, oDif) static_assert( &((RSYSRES_IVL_SUB *)0)->m-&((RSYSRES_IVL_SUB *)0)->qhZoneSen == oDif, "Bad seq " #m)
#define QZONECHK( m, oDif) assert( &((RSYSRES_IVL_SUB *)0)->m-&((RSYSRES_IVL_SUB *)0)->qhZoneSen == oDif && "Bad seq " #m)
Copy link
Contributor

Choose a reason for hiding this comment

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

assert() is runtime. static_assert() is compile time. This was deliberately coded to be a compile time check so the runtime cost is not incurred every subhour. Can a method be found that does the compile time check and is OK on all compilers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can look for a replacement. I was worried if that would be a problem. The error that I got was that it was non-constant condition. non-constant condition for static assertion.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could try &((const RSYSRES_IVL_SUB *)0)->m - &((const RSYSRES_IVL_SUB *)0)->qhZoneSen

That is, add consts.

src/CNLOADS.CPP Outdated
{ // shades closed
xs_pFENAW[ 1]->fa_Subhr( min( fSC, 1.f), bDoFrm);
}
#ifdef _WIN32
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do this? Why not conditionally #define FPCHECK as needed for different platforms. E.g., #define it as nothing if suitable functionality cannot be found. Then you touch cnglob.h only, not every place FPCHECK is used.

@galanCA galanCA self-assigned this Jun 6, 2022
@galanCA galanCA added enhancement multi-platform issues related to compiling / running on non-Windows platforms labels Jun 6, 2022
@galanCA galanCA marked this pull request as ready for review June 6, 2022 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement multi-platform issues related to compiling / running on non-Windows platforms

Projects

Status: To be reviewed by Tanaya

Development

Successfully merging this pull request may close these issues.

3 participants