search

nasa / fprime

F´ - A flight software and embedded systems framework
https://fprime.jpl.nasa.gov
Apache License 2.0
10.02k stars 1.29k forks source link

Fixed conversion warnings on framework tests #2606

Closed JohanBertrand closed 4 months ago

JohanBertrand commented 5 months ago
Related Issue(s)
Has Unit Tests (y/n)
Documentation Included (y/n)

Change Description

Example of implementation of the -Wconversion warning fixes.

Mostly consist of static_cast to change from long (unsigned) int to (unsigned) int.

This is mainly to illustrate the impact of the warning flag on the current code.

Needs to be merged after https://github.com/fprime-community/fpp/pull/400 has been merged.

Rationale

Example of implementation of one of the warning flags mentioned here: https://github.com/nasa/fprime/discussions/2588

Testing/Review Recommendations

N/A

Future Work

N/A

JohanBertrand commented 4 months ago

@LeStarch is it possible to trigger the CI please?

LeStarch commented 4 months ago

@JohanBertrand let me fix the conflict, then sure!

LeStarch commented 4 months ago

@JohanBertrand, it has been triggered. Thanks for the reminder!

JohanBertrand commented 4 months ago

@LeStarch I think most of the warnings are corrected for the Linux side, except the one related to fpp (https://github.com/fprime-community/fpp/pull/400). I'm not sure what is the path forward for that.

For the ones related to macOS, is there a good way for me to test it without waiting for the CI?

Also, just to make sure, are you interested into merging this PR in the future? It is a lot of (small) changes. Mostly about casting, but some small bugs are also fixed.

LeStarch commented 4 months ago

@JohanBertrand I do want this PR to be merged! The above FPP PR is marked "Draft"....so you should probably make it a full PR and we can review.

LeStarch commented 4 months ago

As for the mac-side things are:

  1. Let CI run it
  2. Ask one of the maintainers to work through mac issues
LeStarch commented 4 months ago

@JohanBertrand this is on me because I didn't connect the dots between this PR and other work we have going on, but we have some fairly substantial type adjustments that we are working on.

In short NATIVE_INT_TYPE, NATIVE_UINT_TYPE are going to be replaced by correctly defined types for the use case.

As such, we expect collision with this work. The thought on how to proceed would be:

  1. Keep this PR open
  2. Finish other work
  3. Let one of the maintainers (e.g. myself) fix collisions and conflicts
  4. Merge this and the other work.

We are trying to not invalidate your work here, while also not leverage a bunch of extra work on you.

What are your thoughts on this?

JohanBertrand commented 4 months ago

I think only the fpp update is missing right now. I would have say that it's worth trying to review/merge everything now if it's ready, and try to merge it before the changes of types happen.

If you already started on it and you foresee some conflicts on your side, I'm happy following your approach and let this PR open until you're done with the types update.

LeStarch commented 4 months ago

I think only the fpp update is missing right now. I would have say that it's worth trying to review/merge everything now if it's ready, and try to merge it before the changes of types happen.

If you already started on it and you foresee some conflicts on your side, I'm happy following your approach and let this PR open until you're done with the types update.

A fair point.

Our concern is masking. You are, correctly, fixing errors, but these corrections might be better fixed with the new-methodology that was neither available nor known to you. Your corrections may mask the other solution and make it harder for us to improve upon these issues.

However, we absolutely do not want to discard your work either. These are valued contributions, and your counterpoint is essentially "don't let this code-rot", which is in itself abundantly valid.

Building on your recommendations I propose this:

  1. We review your work and note anything that may be a mask, which we should revisit in the upcoming work.
  2. Merge your work once the review has passed
  3. Revisit point one as we perform our updates without blocking this PR

This solves both concerns:

  1. Your work is merged without code-rotting
  2. "Masking" issues are flagged, resolving our concerns.

There may be many non-blocker comments from the review as we will be flagging issues for future consideration. We will clearly mark concerns that must be resolved before this merges, should anything come up!

Thanks for working with us.

JohanBertrand commented 4 months ago

I totally understand your point, and I am also concerned about masking issues through casting.

I'm all good with your proposition. I am hoping that the merge of fpp will be solving the last warnings so that you can start the review.

Do you want me to add a temporary comment line before every casts I added to better identify it in the codebase later? That's something I can do easily, if that's helping you in the future.

Thank you very much!

JohanBertrand commented 4 months ago

The merge conflicts should be fixed, and the CI should pass in theory.

Please pay attention to the changes in the Fw/Type folder, since there was quite a few conflicts

LeStarch commented 4 months ago

As my own note: here is the success criteria I am using for the review:

  1. Cast to properly set type (e.g. FwAssertArgType): good, correct type chosen
  2. Cast to API call specified type: good, obeying API
  3. Cast to NATIVE_*_TYPE: good, will be caught in future review

Other items are flagged for future review.

LeStarch commented 4 months ago

@JohanBertrand unless you strongly object, I will go ahead and close out the macOS errors as I have access to the necessary OS to fix those. Also, I have begun to review this.

JohanBertrand commented 4 months ago

@LeStarch Please go ahead with the fixes for macOS if you can, it takes too many iterations for me to solve all the warnings with the CI. Sorry for that.

Is there a problem with the CI stage CI/Framework ? I can't see the problem by looking at the CI log.

LeStarch commented 4 months ago

Yes there is. This is a known issue with our older CI jobs (that predate Github actions). Everything gets stuffed into a log file rather than the console.

If you click on "Summary" there is a zip archive one can download to look at the logs. We need to fix this such that it is easier to see what the problem is. Also, these artifacts may be restricted to maintainers...which could be another issue.

JohanBertrand commented 4 months ago

@LeStarch Thanks for the feedback. The issue should be fixed now, sorry for that. I also fixed the error on the macOS CI at the same time.

LeStarch commented 4 months ago

Huzzah!

JohanBertrand commented 4 months ago

Awesome, thanks @LeStarch!

From the review, you want me to add the cast static_cast() even if it is a compatible/equivalent type, is that correct?

Also, do you want me to add a // TODO: check type cast in front of the comments you added for future review?

JohanBertrand commented 4 months ago

@LeStarch I solved the merge conflicts and added casts you requested in the review. Let me know if there is something else missing.

LeStarch commented 4 months ago

Note: I will fix CI this afternoon, as I broke it.