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

Few simple changes to allow code to cleanly compile on Linux/MacOS/Windows (VS2019/22) - add github action to do builds / add unit tests #11

Open
wants to merge 17 commits into
base: cmake-support
Choose a base branch
from

Conversation

alastairUK
Copy link

Some simple changes to automatically build on Window/Linux/Mac OS using GitHub actions. Have upped the warning level to W4 and fixed a few issues. Also added a very simple set of unit tests.

One thing my linter is picking up is the use of reserved identifiers (basically __)

https://clang.llvm.org/extra/clang-tidy/checks/bugprone-reserved-identifier.html

This is obviously all over the code as it's part of your coding standard. But something to be aware of I guess.

@alastairUK alastairUK changed the title New version Few simple changes to allow code to cleanly compile on Linux/MacOS/Windows (VS2017/19) - add github action to do builds / add unit tests Jul 8, 2021
opsaunders
opsaunders approved these changes Jan 26, 2022
@wkozmaNTIA
Copy link
Member

I don't believe this should have been closed, so I'm reopening it for a later time.

@wkozmaNTIA wkozmaNTIA reopened this Jan 27, 2022
@wkozmaNTIA wkozmaNTIA self-requested a review June 13, 2022 15:02
Copy link
Member

@wkozmaNTIA wkozmaNTIA left a comment

Choose a reason for hiding this comment

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

I have 1 comment for you to respond to. Also, I will probably merge this but remove the github actions file for the time being. You can resubmit that as a new pull request, but not ready for that at the moment. Want to update the Linux support and the couple other code quality items you suggest.

@@ -18,7 +17,7 @@
| Outputs: tropo - Struct containing resulting parameters
|
*===========================================================================*/
void Troposcatter(Path *path, Terminal *terminal_1, Terminal *terminal_2, double d__km, double f__mhz, TroposcatterParams *tropo)
void Troposcatter(Path * /*path*/, Terminal *terminal_1, Terminal *terminal_2, double d__km, double f__mhz, TroposcatterParams *tropo)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand what you are doing here?

@wkozmaNTIA
Copy link
Member

Even though I approved this request, @alastairUK if you could respond to the above question that would be appreciated. I'm holding off merging for a bit to give you a chance to respond

@wkozmaNTIA wkozmaNTIA changed the base branch from master to cmake-support June 13, 2022 15:08
@alastairUK alastairUK force-pushed the NewVersion branch 2 times, most recently from 7539d14 to 06fed75 Compare June 13, 2022 15:41
@alastairUK
Copy link
Author

I was compiling with these options

image

So warning are errors.

Without that change you get...

image

I have rebased this PR onto cmake-support branch and fixed a couple more things

Note the github action compiles cleanly

https://github.com/alastairUK/p528/runs/6864839884

image

@alastairUK alastairUK changed the title Few simple changes to allow code to cleanly compile on Linux/MacOS/Windows (VS2017/19) - add github action to do builds / add unit tests Few simple changes to allow code to cleanly compile on Linux/MacOS/Windows (VS2019/22) - add github action to do builds / add unit tests Jun 13, 2022
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.

3 participants