Skip to content

Fix #10378 - bump nlohmann/json to support most recent clang/apple-clang #11041

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
Merged
Myoldmopar merged 10 commits into from
May 14, 2025
Merged

Fix #10378 - bump nlohmann/json to support most recent clang/apple-clang #11041

Myoldmopar merged 10 commits into from
May 14, 2025

Conversation

@jmarrec
Copy link
Contributor

@jmarrec jmarrec commented Apr 22, 2025

Pull request overview

Description of the purpose of this PR

This bumps nlohmann/json from 3.7.2 to 3.12.0 (latest), so that it keeps on compiling with the most recent clang.

Pull Request Author

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
  • If any diffs are expected, author must demonstrate they are justified using plots and descriptions
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions
  • If any defect files are updated to a more recent version, upload new versions here or on DevSupport
  • If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
  • If structural output changes, add to output rules file and add OutputChange label
  • If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

@jmarrec jmarrec added Defect Includes code to repair a defect in EnergyPlus NotIDDChange Code does not impact IDD (can be merged after IO freeze) Developer Issue Related to cmake, packaging, installers, or developer tooling (CI, etc) labels Apr 22, 2025
@jmarrec jmarrec requested review from Myoldmopar and RKStrand April 22, 2025 13:50
@jmarrec jmarrec self-assigned this Apr 22, 2025
@jmarrec
Copy link
Contributor Author

jmarrec commented Apr 22, 2025 •
edited
Loading

I think I need to bump valijson too for MSVC...

https://github.com/tristanpenman/valijson/releases/tag/v1.0.2

Update NlohmannJsonAdapter to make it compatible with latest NlohmannJson

jmarrec added 3 commits April 22, 2025 18:24
@jmarrec
Patch valijson to avoid a std::regex (there are 4 std::regex still in…
296f4eb 
… use for matching date/times) and improve error messages
@jmarrec
use RE2 as a regex engine instead of std::regex, using the valijson f…
e37afca 
…acilities

I must use a pointer to the RE2 member, because it's not movable (nor copyable, but don't care here)
@jmarrec
not sure why C-style comments bad, but shush the check
0448cf8
@Myoldmopar
Copy link
Member

Myoldmopar commented Apr 22, 2025

not sure why C-style comments bad, but shush the check

Because we do lots of parsing the code sniffing out bad patterns and issues, and some of the parsing relies on a line-by-line analysis. And they don't keep track of multiline C style comments. So they could wreak havoc in those scripts. We can have discussions about it, but that's the real reason. If we want to allow them, we'll need to invest substantially to beef up our analysis scripts.

@jmarrec
Copy link
Contributor Author

jmarrec commented Apr 23, 2025

Meh, I'm fine with it, easier to just shush it than change the scripts!

Myoldmopar
Myoldmopar approved these changes May 14, 2025
View reviewed changes
Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

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

Most excellent. Thanks @jmarrec !

@Myoldmopar
Copy link
Member

Myoldmopar commented May 14, 2025

All clean. @RKStrand merging this in now. Thanks @jmarrec .

@Myoldmopar Myoldmopar merged commit 0df4c14 into develop May 14, 2025
9 checks passed
@Myoldmopar Myoldmopar deleted the 10378-bump-nlohmann-clang branch May 14, 2025 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Myoldmopar Myoldmopar Myoldmopar approved these changes

@RKStrand RKStrand Awaiting requested review from RKStrand

Assignees

@jmarrec jmarrec

Labels

Defect Includes code to repair a defect in EnergyPlus Developer Issue Related to cmake, packaging, installers, or developer tooling (CI, etc) NotIDDChange Code does not impact IDD (can be merged after IO freeze)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

std::char_traits is deprecated in clang 18 and removed in clang 19

5 participants

@jmarrec @Myoldmopar @nrel-bot-2 @nrel-bot-2c