Skip to content

Conversation

@Kronos3
Copy link
Collaborator

@Kronos3 Kronos3 commented Mar 26, 2025

This PR removes the special handling of "builtin" types, which are type aliases that were previously handled inside FpConfig.h by C-level typedefs.

Some notes:

  • I removed the #include <FpConfig.h[pp]> from generated alias headers since it's redundant now. It was previously there in case there was an alias of a built-in
  • I removed the tests that tested builtin types since they are no longer relevant as there is no additional code coverage over the simple type alias tests.
  • I found a bug in the #define PRI code that was not referencing built-in PRIs properly
    • We had not hit this yet because C++ code did not use an alias type's #define in generated code. With builtin's removed now the old builtin PRIs were being hit.
    • The common pattern was like this:
// Queue priority type
typedef PlatformQueuePriorityType FwQueuePriorityType;
#define PRI_FwQueuePriorityType PRI_PlatformQueuePriorityType

// The type used to serialize a size value
typedef U16 FwSizeStoreType;
#define PRI_FwSizeStoreType PRIu16

Notice that builtin types like U16 have PRIs defined PRI[typeName.lower() where-as aliases are PRI_[typeName]. That underscore was always assumed in previously generated code (see diffs for what I'm talking about).

Closes #649

@Kronos3
Copy link
Collaborator Author

Kronos3 commented Mar 26, 2025

I've opened a sister PR in nasa/fprime#3417 that updates the FpConfig.h which this PR's check-cpp will depend on.

@Kronos3 Kronos3 requested a review from bocchino March 27, 2025 00:13
@Kronos3
Copy link
Collaborator Author

Kronos3 commented Mar 28, 2025

Future note to self, the PRIs for scalars are defined twice with slightly different names:
https://github.com/nasa/fprime/pull/3422/files#diff-4a4e7f315f68d77a9f724564407a9a79e284175fbd14fd0e81a428a00dca0293R51-R88

@bocchino
Copy link
Collaborator

Notice that builtin types like U16 have PRIs defined PRI[typeName.lower() where-as aliases are PRI_[typeName].

I think there are additional macros in there now so that PRI_U16 should work. If that's true, then maybe we should update FPP to use that form. That way the PRIu16 form would be used once to define PRI_U16, which is the F Prime standard, and PRI_U16 would be used everywhere else.

@LeStarch
Copy link
Collaborator

@bocchino should be ready for re-review, and once finished a point-release!

@bocchino
Copy link
Collaborator

bocchino commented Mar 31, 2025

@LeStarch. It looks good so far. Just a couple of comments:

  1. I think there may be a missing header include for Fw/FPrimeBasicTypes.hpp. I noted this in an inline comment.

  2. Let's be consistent and write #include "Fw/FPrimeBasicTypes.hpp" everywhere. Sometimes it is written #include <Fw/FPrimeBasicTypes.hpp>. I started noting this in comments, but then there were several cases. Let's sweep through and make everything consistent. grep -r "#include <Fw/FPrimeBasicTypes.hpp>" in the fpp-check/test directory should come up clean.

Where is the corresponding F Prime branch, so I can run check-cpp and verify that it passes?

@bocchino
Copy link
Collaborator

It looks like the corresponding F Prime branch is here: nasa/fprime#3422.

@bocchino
Copy link
Collaborator

bocchino commented Apr 1, 2025

@LeStarch I'm not seeing Fw/FPrimeBasicTypes.hpp on that branch.

@LeStarch
Copy link
Collaborator

LeStarch commented Apr 1, 2025

@bocchino, I have fixes the issues, except for the one I left a comment about. I have also pushed to the FPrime branch so you can run cpp-check.

I have:

  1. Fixed suggestions
  2. Updated refs
  3. Confirmed No angle bracket includes for the new header
  4. Ran cpp-check

@bocchino bocchino self-requested a review April 1, 2025 21:31
Copy link
Collaborator

@bocchino bocchino left a comment

Choose a reason for hiding this comment

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

Looks good! I added a few comments explaining what is going on with the headers in the alias code gen. I confirmed that check-cpp runs cleanly.

@bocchino bocchino added the blocked Issue is blocked pending resolution of another issue label Apr 2, 2025
@bocchino
Copy link
Collaborator

bocchino commented Apr 2, 2025

Before merging this branch into fpp/main, I would like to get nasa/fprime#3434 reviewed and merged. It fixes problems in FppTest, which we need for testing the integration of this branch with F Prime.

@bocchino bocchino removed the blocked Issue is blocked pending resolution of another issue label Apr 2, 2025
@bocchino
Copy link
Collaborator

bocchino commented Apr 2, 2025

I confirmed that everything works with the updated FppTest. Merging now.

@bocchino bocchino merged commit 5c91ac3 into main Apr 2, 2025
11 checks passed
@bocchino bocchino deleted the tumbar-remove-builtin branch April 2, 2025 18:16
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.

Replace builtin types with type aliases

4 participants