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

Use cpp headers over system headers #221

Closed
wants to merge 1 commit into from

Conversation

sarahec
Copy link
Contributor

@sarahec sarahec commented Dec 17, 2022

This replaces system headers such as <math.h> with the corresponding C++ header (<cmath>) ni C++ code.

Each of the compiler suites includes its own c++ headers, and there's a risk of the system headers differing from these and causing subtle bugs.

@sarahec sarahec mentioned this pull request Dec 17, 2022
@sarahec
Copy link
Contributor Author

sarahec commented Dec 17, 2022

The formatting script is sorting the headers and re-wrapped some lines in base.cc and double.cc. Please take a look.

Copy link
Contributor

@pjaaskel pjaaskel left a comment

Choose a reason for hiding this comment

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

The ? : style doesn't look like most of the OpenASIP codebase and newlib should be left with minimal modifications, otherwise LGTM.

openasip/opset/base/base.cc Outdated Show resolved Hide resolved
@sarahec
Copy link
Contributor Author

sarahec commented Dec 19, 2022

Fixes applied

openasip/opset/base/base.cc Outdated Show resolved Hide resolved
openasip/opset/base/double.cc Outdated Show resolved Hide resolved
Update to cpp headers (formatted)

Use c++ headers in c++ files

Reverts change

Rework on clean copies from main
@sarahec
Copy link
Contributor Author

sarahec commented Dec 20, 2022

Changes in the force-pushed update:

  1. Reverted all of newlib (copied over from main)
  2. Reverted base.cc and double.cc (copied from main), then changed the header and added using namespace std. No other formatting changes applied.
  3. Squashed and force-pushed the combined commits to make the merge cleaner. Here's the diff link for your review.

@pjaaskel
Copy link
Contributor

Did you run the tests? There seem to be a couple of cases which are compiled with the oacc's (incomplete C++ support), which doesn't yet have a stdlib.

<system_testing errors with Current shell environment. With revision a0b8a6fcaa7b46c131bbf4684974a38b07c2d3c1.>
[349637] ./procgen/hpu/test_hpu_embedded.testdesc: half-precision FPUs by generating and simulating a processor...FAIL (0m0.569s)
[349645] ./bintools/Compiler/RegressionTests/bool_return_value.testdesc: Test that boolean return values work...FAIL (0m0.436s)
Differences found against verification data are stored into difference.txt
</system_testing errors with Current shell environment. With revision a0b8a6fcaa7b46c131bbf4684974a38b07c2d3c1.>

Here's the info for how to run the tests if you haven't before: https://github.com/cpc/openasip/wiki/Contributor-Info

@sarahec
Copy link
Contributor Author

sarahec commented Dec 24, 2022

Thank you. I had been running tests from the test directory and missed that there's a test driver script. It's also flagging problems I hadn't seen earlier.

This may take some time, as compiletest isn't passing on a fresh checkout of the project.

@sarahec
Copy link
Contributor Author

sarahec commented Dec 30, 2022

Filed #223 which is blocking resolution of the above question.

@sarahec
Copy link
Contributor Author

sarahec commented Feb 4, 2023

I'm going to close this for now and come back when I have a reliable testing setup.

@sarahec sarahec closed this Feb 4, 2023
@sarahec sarahec deleted the use-cpp-headers branch July 29, 2023 19:48
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.

2 participants