Skip to content

CNLOADS upgrade #310

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

CNLOADS upgrade #310

wants to merge 7 commits into from

Conversation

galanCA
Copy link
Contributor

@galanCA galanCA commented Jun 2, 2022 •
edited
Loading

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
chipbarnaby
chipbarnaby reviewed Jun 3, 2022
View reviewed changes
src/CNLOADS.CPP Outdated
@@ -3381,7 +3382,7 @@ RC RSYS::rs_EndSubhr()
// verify RSYSRES_IVL_SUB layout at compile time
// 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.

chipbarnaby
chipbarnaby reviewed Jun 3, 2022
View reviewed changes
src/CNLOADS.CPP Outdated
@@ -6580,7 +6581,9 @@ RC XSURF::xs_ASHWAT() // subhour calcs for ASHWAT fenestration
{ // 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
Reviewers

@chipbarnaby chipbarnaby chipbarnaby left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@galanCA galanCA

Labels
enhancement multi-platform issues related to compiling / running on non-Windows platforms
Projects
Multi-platform upgrade
Status: To be reviewed by Tanaya
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@galanCA @chipbarnaby