Fixed conversion warnings on framework tests #2606

JohanBertrand · 2024-03-19T22:54:54Z


*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 nasa/fpp#400 has been merged.

Rationale

Example of implementation of one of the warning flags mentioned here: #2588

Testing/Review Recommendations

N/A

Future Work

N/A

Drv/LinuxGpioDriver/LinuxGpioDriverComponentImpl.cpp

@@ -157,8 +157,8 @@

        FW_ASSERT(fd != -1);

-        NATIVE_INT_TYPE stat1 = lseek(fd, 0, SEEK_SET); // Must seek back to the starting
-        NATIVE_INT_TYPE stat2 = read(fd, &ch, 1);
+        NATIVE_INT_TYPE stat1 = static_cast<NATIVE_INT_TYPE>(lseek(fd, 0, SEEK_SET)); // Must seek back to the starting


Drv/LinuxGpioDriver/LinuxGpioDriverComponentImpl.cpp

-        NATIVE_INT_TYPE stat1 = lseek(fd, 0, SEEK_SET); // Must seek back to the starting
-        NATIVE_INT_TYPE stat2 = read(fd, &ch, 1);
+        NATIVE_INT_TYPE stat1 = static_cast<NATIVE_INT_TYPE>(lseek(fd, 0, SEEK_SET)); // Must seek back to the starting
+        NATIVE_INT_TYPE stat2 = static_cast<NATIVE_INT_TYPE>(read(fd, &ch, 1));


Drv/LinuxUartDriver/LinuxUartDriver.cpp

Os/Posix/File.cpp

Svc/BufferAccumulator/BufferAccumulator.cpp

@@ -56,7 +56,7 @@
 ) {

  this->m_allocatorId = identifier;
-  NATIVE_UINT_TYPE memSize = sizeof(Fw::Buffer) * maxNumBuffers;
+  NATIVE_UINT_TYPE memSize = static_cast<NATIVE_UINT_TYPE>(sizeof(Fw::Buffer) * maxNumBuffers);


Svc/LinuxTimer/LinuxTimerComponentImplTimerFd.cpp

@@ -36,7 +36,7 @@

      while (true) {
          unsigned long long missed;
-          int ret = read (fd, &missed, sizeof (missed));
+          int ret = static_cast<int>(read (fd, &missed, sizeof (missed)));


Os/test/ut/file/SyntheticFileSystem.cpp

@@ -212,7 +212,7 @@
    } else if (Os::File::Mode::OPEN_READ == this->m_data->m_mode) {
        status = Os::File::Status::INVALID_MODE;
    } else {
-        const FwSignedSizeType original_size = this->m_data->m_data.size();
+        const FwSignedSizeType original_size = static_cast<FwSignedSizeType>(this->m_data->m_data.size());


Os/test/ut/file/SyntheticFileSystem.cpp

@@ -138,12 +138,12 @@
        return Os::File::Status::INVALID_MODE;
    }
    FwSignedSizeType original_position = this->m_data->m_pointer;
-    FwSignedSizeType original_size = this->m_data->m_data.size();
+    FwSignedSizeType original_size = static_cast<FwSignedSizeType>(this->m_data->m_data.size());


Os/test/ut/file/SyntheticFileSystem.cpp

@@ -157,14 +157,14 @@
              static_cast<size_t>((Os::File::Mode::OPEN_APPEND == this->m_data->m_mode) ? original_size : FW_MAX(original_position, original_size)));

    FwSignedSizeType pre_write_position = this->m_data->m_pointer;
-    FwSignedSizeType pre_write_size = this->m_data->m_data.size();
+    FwSignedSizeType pre_write_size = static_cast<FwSignedSizeType>(this->m_data->m_data.size());


Os/test/ut/file/SyntheticFileSystem.cpp

@@ -105,15 +105,15 @@
    }
    std::vector<U8> output;
    FwSignedSizeType original_pointer = this->m_data->m_pointer;
-    FwSignedSizeType original_size = this->m_data->m_data.size();
+    FwSignedSizeType original_size = static_cast<FwSignedSizeType>(this->m_data->m_data.size());


github-advanced-security

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

JohanBertrand · 2024-04-04T23:55:00Z

@LeStarch is it possible to trigger the CI please?

LeStarch · 2024-04-05T00:11:02Z

@JohanBertrand let me fix the conflict, then sure!

LeStarch · 2024-04-05T00:14:41Z

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

Os/Posix/File.cpp

@@ -239,7 +242,7 @@

    for (FwSignedSizeType i = 0; i < maximum && accumulated < size; i++) {
        // char* for some posix implementations
-        ssize_t write_size = ::write(this->m_handle.m_file_descriptor, reinterpret_cast<const CHAR*>(&buffer[accumulated]), size - accumulated);
+        ssize_t write_size = ::write(this->m_handle.m_file_descriptor, reinterpret_cast<const CHAR*>(&buffer[accumulated]), static_cast<size_t>(size - accumulated));


Os/Posix/File.cpp

@@ -206,7 +206,10 @@

    for (FwSignedSizeType i = 0; i < maximum && accumulated < size; i++) {
        // char* for some posix implementations
-        ssize_t read_size = ::read(this->m_handle.m_file_descriptor, reinterpret_cast<CHAR*>(&buffer[accumulated]), size - accumulated);
+        ssize_t read_size = ::read(


Os/Posix/Task.cpp

@@ -11,7 +11,7 @@
 #include <climits>
 #include <Fw/Logger/Logger.hpp>

-#ifdef TGT_OS_TYPE_LINUX 
+#ifdef TGT_OS_TYPE_LINUX


Fw/Port/InputPortBase.cpp

@@ -27,10 +27,10 @@

 #if FW_OBJECT_TO_STRING == 1
    void InputPortBase::toString(char* buffer, NATIVE_INT_TYPE size) {
-#if FW_OBJECT_NAMES == 1 
+#if FW_OBJECT_NAMES == 1


Fw/Test/UnitTestAssert.hpp

@@ -63,7 +63,7 @@
        private:
            File m_file;
            NATIVE_UINT_TYPE m_lineNo;
-            NATIVE_INT_TYPE m_numArgs;
+            NATIVE_UINT_TYPE m_numArgs;


Fw/Comp/PassiveComponentBase.cpp

@@ -13,7 +13,7 @@
    void PassiveComponentBase::toString(char* buffer, NATIVE_INT_TYPE size) {
        FW_ASSERT(size > 0);
        FW_ASSERT(buffer != nullptr);
-        PlatformIntType status = snprintf(buffer, size, "Comp: %s", this->m_objName.toChar());
+        PlatformIntType status = snprintf(buffer, static_cast<size_t>(size), "Comp: %s", this->m_objName.toChar());


Fw/Comp/ActiveComponentBase.cpp

@@ -46,7 +46,7 @@
    void ActiveComponentBase::toString(char* buffer, NATIVE_INT_TYPE size) {
        FW_ASSERT(size > 0);
        FW_ASSERT(buffer != nullptr);
-        PlatformIntType status = snprintf(buffer, size, "ActComp: %s", this->m_objName.toChar());
+        PlatformIntType status = snprintf(buffer, static_cast<size_t>(size), "ActComp: %s", this->m_objName.toChar());


Fw/Comp/QueuedComponentBase.cpp

@@ -23,7 +23,7 @@
    void QueuedComponentBase::toString(char* buffer, NATIVE_INT_TYPE size) {
        FW_ASSERT(size > 0);
        FW_ASSERT(buffer != nullptr);
-        PlatformIntType status = snprintf(buffer, size, "QueueComp: %s", this->m_objName.toChar());
+        PlatformIntType status = snprintf(buffer, static_cast<size_t>(size), "QueueComp: %s", this->m_objName.toChar());


Drv/LinuxUartDriver/LinuxUartDriver.cpp

@@ -324,9 +324,9 @@
        status = Drv::SendStatus::SEND_ERROR;
    } else {
        unsigned char *data = serBuffer.getData();
-        NATIVE_INT_TYPE xferSize = serBuffer.getSize();
+        NATIVE_INT_TYPE xferSize = static_cast<NATIVE_INT_TYPE>(serBuffer.getSize());


Os/QueueCommon.cpp

@@ -22,13 +22,13 @@
    Queue::QueueStatus Queue::receive(Fw::SerializeBufferBase &buffer, NATIVE_INT_TYPE &priority, QueueBlocking block) {

        U8* msgBuff = buffer.getBuffAddr();
-        NATIVE_INT_TYPE buffCapacity = buffer.getBuffCapacity();
+        NATIVE_INT_TYPE buffCapacity = static_cast<NATIVE_INT_TYPE>(buffer.getBuffCapacity());


Os/QueueCommon.cpp

@@ -13,7 +13,7 @@
    Queue::QueueStatus Queue::send(const Fw::SerializeBufferBase &buffer, NATIVE_INT_TYPE priority, QueueBlocking block) {

        const U8* msgBuff = buffer.getBuffAddr();
-        NATIVE_INT_TYPE buffLength = buffer.getBuffLength();
+        NATIVE_INT_TYPE buffLength = static_cast<NATIVE_INT_TYPE>(buffer.getBuffLength());


Os/Pthreads/Queue.cpp

@@ -152,7 +152,7 @@
    }

    // Push item onto queue:
-    bool pushSucceeded = queue->push(buffer, size, priority);
+    bool pushSucceeded = queue->push(buffer, static_cast<NATIVE_UINT_TYPE>(size), priority);


Os/Pthreads/Queue.cpp

@@ -108,7 +108,7 @@
    ///////////////////////////////

    // Push item onto queue:
-    bool pushSucceeded = queue->push(buffer, size, priority);
+    bool pushSucceeded = queue->push(buffer, static_cast<NATIVE_UINT_TYPE>(size), priority);


Svc/DpWriter/DpWriter.cpp

@@ -179,16 +186,18 @@
        // Set write size to file size
        // On entry to the write call, this is the number of bytes to write
        // On return from the write call, this is the number of bytes written
-        FwSignedSizeType writeSize = fileSize;
+        FwSignedSizeType writeSize = static_cast<FwSignedSizeType>(fileSize);


JohanBertrand · 2024-04-09T07:21:27Z

@LeStarch I think most of the warnings are corrected for the Linux side, except the one related to fpp (nasa/fpp#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 · 2024-04-09T16:55:56Z

@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 · 2024-04-09T16:56:23Z

As for the mac-side things are:

Let CI run it
Ask one of the maintainers to work through mac issues

LeStarch · 2024-04-10T02:58:03Z

@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:

Keep this PR open
Finish other work
Let one of the maintainers (e.g. myself) fix collisions and conflicts
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 · 2024-04-10T21:25:02Z

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.

JohanBertrand · 2024-04-10T23:10:47Z

Fw/Types/PolyType.cpp

@@ -211,7 +211,7 @@ namespace Fw {

    PolyType::PolyType(I64 val) {
        this->m_dataType = TYPE_I64;
-        this->m_val.u64Val = val;
+        this->m_val.i64Val = val;


I am not sure if that has any impact on other functions or components. Also, I did not find where this it currently used.

However, the unit tests are passing here because of the casts performed there. That's why it has not been seen before

LeStarch · 2024-04-11T17:49:19Z

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:

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

This solves both concerns:

Your work is merged without code-rotting
"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 · 2024-04-12T20:23:40Z

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!

…ings

LeStarch · 2024-04-16T23:53:08Z

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

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

Other items are flagged for future review.

LeStarch · 2024-04-16T23:54:30Z

@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.

LeStarch · 2024-04-17T00:00:49Z

Fw/Types/StringType.cpp

@@ -49,7 +49,7 @@ bool StringBase::operator==(const CHAR* other) const {
    }

    const SizeType capacity = this->getCapacity();
-    const size_t result = strncmp(us, other, capacity);
+    const size_t result = static_cast<size_t>(strncmp(us, other, capacity));


For future review: do we need to cast to size_t just for a comparison to 0 below? Perhaps we should just use the native result type from strncmp

LeStarch · 2024-04-17T00:06:03Z

Drv/Ip/IpSocket.cpp

@@ -182,13 +182,14 @@ SocketIpStatus IpSocket::send(const U8* const data, const U32 size) {
            return SOCK_SEND_ERROR;
        }
        FW_ASSERT(sent > 0, sent);
-        total += sent;
+        total += static_cast<U32>(sent);


For future review: why U32 here? Seems arbitrary.

LeStarch · 2024-04-17T00:06:17Z

Drv/Ip/IpSocket.cpp

@@ -202,7 +203,7 @@ SocketIpStatus IpSocket::recv(U8* data, I32& req_read) {
    // Try to read until we fail to receive data
    for (U32 i = 0; (i < SOCKET_MAX_ITERATIONS) && (size <= 0); i++) {
        // Attempt to recv out data
-        size = this->recvProtocol(data, req_read);
+        size = this->recvProtocol(data, static_cast<U32>(req_read));


For future review: why U32 here? Seems arbitrary.

LeStarch · 2024-04-17T00:28:13Z

Svc/CmdDispatcher/CommandDispatcherImpl.cpp

@@ -39,7 +39,7 @@ namespace Svc {
                this->m_entryTable[slot].opcode = opCode;
                this->m_entryTable[slot].port = portNum;
                this->m_entryTable[slot].used = true;
-                this->log_DIAGNOSTIC_OpCodeRegistered(opCode,portNum,slot);
+                this->log_DIAGNOSTIC_OpCodeRegistered(opCode,portNum,static_cast<I32>(slot));


For future review: should slot in the EVR match the iteration value of U32?

LeStarch · 2024-04-17T00:34:37Z

Utils/CRCChecker.cpp

    }

-    remaining_bytes = int_file_size % CRC_FILE_READ_BLOCK;
+    remaining_bytes = static_cast<PlatformIntType>(int_file_size % CRC_FILE_READ_BLOCK);


For future review: PlatformIntType usage here may need to be fixed.

LeStarch

Here are a list of comments for what I have reviewed thus far. I see a few missing casts. We also need to review static_cast<FwIndexType> as perhaps the variable type being cast should change.

Nothing blocking except the need to fix CI. I'll look at that tomorrow.

LeStarch · 2024-04-17T00:36:55Z

Svc/StaticMemory/StaticMemoryComponentImpl.cpp

+    FW_ASSERT(
+        (fwBuffer.getData() + fwBuffer.getSize()) <= (m_static_memory[portNum] + sizeof(m_static_memory[0])),
+        static_cast<FwAssertArgType>(fwBuffer.getSize()),
+        sizeof(m_static_memory[0]));


I think this line needs a static cast to FwAssertArgType

LeStarch · 2024-04-17T00:40:41Z

Svc/Health/HealthComponentImpl.cpp

@@ -48,12 +48,15 @@ namespace Svc {
        // make sure not asking for more pings than ports
        FW_ASSERT(numPingEntries <= NUM_PINGSEND_OUTPUT_PORTS);

-        this->m_numPingEntries = numPingEntries;
+        this->m_numPingEntries = static_cast<U32>(numPingEntries);
        this->m_watchDogCode = watchDogCode;

        // copy entries to private data
        for (NATIVE_INT_TYPE entry = 0; entry < numPingEntries; entry++) {


For future review: this should iterate using FwIndexType

LeStarch · 2024-04-17T00:43:18Z

Svc/GenericHub/GenericHubComponentImpl.cpp

    FW_ASSERT(rawSize == static_cast<U32>(size));
    if (type == HUB_TYPE_PORT) {
        // Com buffer representations should be copied before the call returns, so we need not "allocate" new data
        Fw::ExternalSerializeBuffer wrapper(rawData, rawSize);
        status = wrapper.setBuffLen(rawSize);
        FW_ASSERT(status == Fw::FW_SERIALIZE_OK, static_cast<NATIVE_INT_TYPE>(status));
-        portOut_out(port, wrapper);
+        portOut_out(static_cast<FwIndexType>(port), wrapper);


For future review: port should use FwIndexType

LeStarch · 2024-04-17T00:45:14Z

Svc/Deframer/Deframer.cpp

-        FW_ASSERT(status == Fw::FW_SERIALIZE_OK, status, offset, serSize);
+        FW_ASSERT(
+            status == Fw::FW_SERIALIZE_OK,
+            status,


Another missing cast to assert arg type.

LeStarch · 2024-04-17T00:46:16Z

Svc/DpWriter/DpWriter.cpp

        fileStatus = file.write(buffer.getData(), writeSize);
        // If a successful write occurred, then update the number of bytes written
        if (fileStatus == Os::File::OP_OK) {
-            this->m_numBytesWritten += writeSize;
+            this->m_numBytesWritten += static_cast<U64>(writeSize);


For future review: U64 use seems suspicious.

LeStarch · 2024-04-17T00:48:21Z

Svc/ComLogger/ComLogger.cpp

    }

    FW_ASSERT(Fw::StringUtils::string_length(incomingFilePrefix, sizeof(this->m_filePrefix)) < sizeof(this->m_filePrefix),
-      Fw::StringUtils::string_length(incomingFilePrefix, sizeof(this->m_filePrefix)), sizeof(this->m_filePrefix)); // ensure that file prefix is not too big
+      static_cast<FwAssertArgType>(Fw::StringUtils::string_length(incomingFilePrefix, sizeof(this->m_filePrefix))),
+      sizeof(this->m_filePrefix)); // ensure that file prefix is not too big


Missing a cast.

LeStarch · 2024-04-17T00:50:12Z

Svc/ComQueue/ComQueue.cpp

@@ -71,7 +71,9 @@ void ComQueue::configure(QueueConfigurationTable queueConfig,
        for (NATIVE_UINT_TYPE entryIndex = 0; entryIndex < FW_NUM_ARRAY_ELEMENTS(queueConfig.entries); entryIndex++) {
            // Check for valid configuration entry
            FW_ASSERT(queueConfig.entries[entryIndex].priority < TOTAL_PORT_COUNT,
-                      queueConfig.entries[entryIndex].priority, TOTAL_PORT_COUNT, entryIndex);
+                      queueConfig.entries[entryIndex].priority,
+                      TOTAL_PORT_COUNT,


Just to make sure, do you want to have the cast static_cast<FwAssertArgType>() even if it is a compatible/equivalent type?

Svc/ComQueue/ComQueue.cpp

LeStarch · 2024-04-17T00:51:48Z

Svc/ComQueue/ComQueue.cpp

@@ -158,15 +166,15 @@ void ComQueue::comStatusIn_handler(const NATIVE_INT_TYPE portNum, Fw::Success& c
 void ComQueue::run_handler(const NATIVE_INT_TYPE portNum, U32 context) {
    // Downlink the high-water marks for the Fw::ComBuffer array types
    ComQueueDepth comQueueDepth;
-    for (FwSizeType i = 0; i < comQueueDepth.SIZE; i++) {
+    for (U32 i = 0; i < comQueueDepth.SIZE; i++) {


For future review: use of U32 in this iterator. comQueueDepth.SIZE?

LeStarch · 2024-04-17T00:51:58Z

Svc/ComQueue/ComQueue.cpp

        comQueueDepth[i] = this->m_queues[i].get_high_water_mark();
        this->m_queues[i].clear_high_water_mark();
    }
    this->tlmWrite_comQueueDepth(comQueueDepth);

    // Downlink the high-water marks for the Fw::Buffer array types
    BuffQueueDepth buffQueueDepth;
-    for (FwSizeType i = 0; i < buffQueueDepth.SIZE; i++) {
+    for (U32 i = 0; i < buffQueueDepth.SIZE; i++) {


For future review: use of U32 in this iterator. comQueueDepth.SIZE?

Note: this is an autocoder problem, as array types use U32 indices.

JohanBertrand · 2024-04-17T10:49:51Z

@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 · 2024-04-17T16:53:29Z

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 · 2024-04-17T20:48:04Z

@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 · 2024-04-18T00:40:55Z

Huzzah!

JohanBertrand · 2024-04-18T17:36:05Z

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?

…ings

JohanBertrand · 2024-04-28T10:02:23Z

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

…prime into Fix-Wconversion-warnings

LeStarch

I went ahead and fixed anything that seems like it needed to be fixed here. Otherwise I left comments to review further.

Systematically, there seems to be no clear consensus in how to represent sizes in API calls and more specifically in model calls. We should discuss a resolution and standards for this.

LeStarch · 2024-04-26T19:43:29Z

Svc/ActiveTextLogger/LogFile.cpp

@@ -70,7 +70,7 @@ namespace Svc {
                // Only return a good status if the write was valid
                status = (writeSize > 0);

-                this->m_currentFileSize += writeSize;
+                this->m_currentFileSize += static_cast<U32>(writeSize);


For future review: This is an established U32 interface, but it likely should switch to FwSizeType?

LeStarch · 2024-04-26T19:44:15Z

Svc/AssertFatalAdapter/AssertFatalAdapterComponentImpl.cpp

+              this->log_FATAL_AF_ASSERT_1(
+                fileArg,
+                lineNo,
+                static_cast<U32>(arg1));


For future review: these U32s need re-review.

LeStarch · 2024-04-26T19:45:15Z

Svc/BufferLogger/BufferLoggerFile.cpp

@@ -212,7 +212,7 @@ namespace Svc {
    else {
      Fw::LogStringArg string(this->m_name.toChar());

-      this->m_bufferLogger.log_WARNING_HI_BL_LogFileWriteError(fileStatus, size, length, string);
+      this->m_bufferLogger.log_WARNING_HI_BL_LogFileWriteError(fileStatus, static_cast<U32>(size), length, string);


For future review: U32 use in events.

LeStarch · 2024-04-26T19:51:05Z

Svc/Health/HealthComponentImpl.cpp

@@ -48,12 +48,15 @@ namespace Svc {
        // make sure not asking for more pings than ports
        FW_ASSERT(numPingEntries <= NUM_PINGSEND_OUTPUT_PORTS);

-        this->m_numPingEntries = numPingEntries;
+        this->m_numPingEntries = static_cast<U32>(numPingEntries);


For future review: is U32 tracking ping-entries appropriate?

LeStarch · 2024-04-26T19:56:57Z

Svc/ComQueue/ComQueue.cpp

    FW_ASSERT(portNum >= 0, portNum);
    Fw::SerializeStatus status = this->m_queues[queueNum].enqueue(data, size);
    if (status == Fw::FW_SERIALIZE_NO_ROOM_LEFT && !this->m_throttle[queueNum]) {
-        this->log_WARNING_HI_QueueOverflow(queueType, portNum);
+        this->log_WARNING_HI_QueueOverflow(queueType, static_cast<U32>(portNum));


For future review: is U32 as port number appropriate?

LeStarch · 2024-04-29T19:34:12Z

Svc/CmdSequencer/FPrimeSequence.cpp

@@ -188,14 +188,14 @@ namespace Svc {
    if (status and readLen != Sequence::Header::SERIALIZED_SIZE) {
      this->m_events.fileInvalid(
          CmdSequencer_FileReadStage::READ_HEADER_SIZE,
-          readLen
+          static_cast<I32>(readLen)


For future review: reconsider I32 as size in events.

LeStarch · 2024-04-29T19:42:18Z

Svc/ComQueue/ComQueue.cpp

        comQueueDepth[i] = this->m_queues[i].get_high_water_mark();
        this->m_queues[i].clear_high_water_mark();
    }
    this->tlmWrite_comQueueDepth(comQueueDepth);

    // Downlink the high-water marks for the Fw::Buffer array types
    BuffQueueDepth buffQueueDepth;
-    for (FwSizeType i = 0; i < buffQueueDepth.SIZE; i++) {
+    for (U32 i = 0; i < buffQueueDepth.SIZE; i++) {


Note: this is an autocoder problem, as array types use U32 indices.

LeStarch · 2024-04-29T19:44:41Z

Svc/FileManager/FileManager.cpp

@@ -180,7 +180,7 @@ namespace Svc {
      );
    } else {
      this->log_WARNING_HI_ShellCommandFailed(
-          logStringCommand, status
+          logStringCommand, static_cast<U32>(status)


For future review: the use of U32 here is probably incorrect. Shell commands can return negative status.

LeStarch · 2024-04-29T19:46:45Z

Svc/PrmDb/PrmDbImpl.cpp

@@ -154,18 +154,21 @@ namespace Svc {
                stat = paramFile.write(&delim,writeSize,Os::File::WaitType::WAIT);
                if (stat != Os::File::OP_OK) {
                    this->unLock();
-                    this->log_WARNING_HI_PrmFileWriteError(PrmWriteError::DELIMITER,numRecords,stat);
+                    this->log_WARNING_HI_PrmFileWriteError(PrmWriteError::DELIMITER,static_cast<I32>(numRecords),stat);


For future review: types in events here.

LeStarch · 2024-04-29T19:50:15Z

Svc/DpCatalog/DpCatalog.cpp

                DpStateEntry entry;
-                entry.dir = dir;
+                entry.dir = static_cast<FwIndexType>(dir);


For future review: why is entry.dir FwIndexType here and FwSizeType in the iterator.

LeStarch · 2024-04-29T19:54:24Z

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

Drv/Ip/IpSocket.cpp

    return SOCK_SUCCESS;
 }

-SocketIpStatus IpSocket::recv(U8* data, I32& req_read) {
+SocketIpStatus IpSocket::recv(U8* data, U32& req_read) {


JohanBertrand added 3 commits March 19, 2024 23:47

Fixed conversion warnings on framework tests

a872e0c

Fix conversion warnings from CI

ebeb0de

Fix static_cast in SyntheticFileSystem

5573b6f

github-advanced-security bot found potential problems Mar 28, 2024

View reviewed changes

Fix read and write argument conversion

9e65768

Merge remote-tracking branch 'nasa/devel' into Fix-Wconversion-warnings

0f6ddcf

Fix new CI issues

3465a16

github-advanced-security bot found potential problems Apr 8, 2024

View reviewed changes

JohanBertrand added 2 commits April 8, 2024 22:18

Fix errors from clang-tidy

bd77430

Fix last errors from clang

3c2103d

github-advanced-security bot found potential problems Apr 8, 2024

View reviewed changes

JohanBertrand added 2 commits April 9, 2024 20:23

Fix macOS issue

402053f

Fix last macOS IPCQueueStub warning

cc9abc1

Last Os-MacOs warning fix

e206b6d

JohanBertrand commented Apr 10, 2024

View reviewed changes

JohanBertrand added 3 commits April 15, 2024 23:42

Merge remote-tracking branch 'origin/devel' into Fix-Wconversion-warn…

680ad29

…ings

Fix merge conflict

a4b7f84

Fix new warnings and merge conflicts

618689b

LeStarch reviewed Apr 17, 2024

View reviewed changes

Fix unit tests and one more macOS error

e00c7e8

JohanBertrand mentioned this pull request Apr 17, 2024

Improve error display on failure of fputil_action #2695

Closed

LeStarch added 2 commits April 17, 2024 15:52

Fix macOS errors - pt 1

dd983a1

sp

af5be5b

JohanBertrand added 3 commits April 28, 2024 11:26

Merge remote-tracking branch 'origin/devel' into Fix-Wconversion-warn…

6567fad

…ings

Fix warnings from development

2b0ee7d

Fixes from review

310bb57

LeStarch added 3 commits April 29, 2024 12:21

Further fixes from review - WIP

b0cf15f

Merge branch 'Fix-Wconversion-warnings' of github.com:JohanBertrand/f…

f5437dc

…prime into Fix-Wconversion-warnings

Finished review fixes

9ca89e9

LeStarch approved these changes Apr 29, 2024

View reviewed changes

Fixing =CI issues

bbb29e9

github-advanced-security bot found potential problems Apr 29, 2024

View reviewed changes

thomas-bc approved these changes Apr 30, 2024

View reviewed changes

thomas-bc merged commit d518121 into nasa:devel Apr 30, 2024
35 checks passed

Fixed conversion warnings on framework tests #2606

Fixed conversion warnings on framework tests #2606

Conversation

JohanBertrand commented Mar 19, 2024 • edited Loading

Change Description

Rationale

Testing/Review Recommendations

Future Work

github-advanced-security bot left a comment

Choose a reason for hiding this comment

JohanBertrand commented Apr 4, 2024

LeStarch commented Apr 5, 2024

LeStarch commented Apr 5, 2024

JohanBertrand commented Apr 9, 2024

LeStarch commented Apr 9, 2024

LeStarch commented Apr 9, 2024

LeStarch commented Apr 10, 2024

JohanBertrand commented Apr 10, 2024

JohanBertrand Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

LeStarch commented Apr 11, 2024 • edited Loading

JohanBertrand commented Apr 12, 2024

LeStarch commented Apr 16, 2024

LeStarch commented Apr 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeStarch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanBertrand commented Apr 17, 2024

LeStarch commented Apr 17, 2024

JohanBertrand commented Apr 17, 2024

LeStarch commented Apr 18, 2024

JohanBertrand commented Apr 18, 2024

JohanBertrand commented Apr 28, 2024

LeStarch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeStarch commented Apr 29, 2024

JohanBertrand commented Mar 19, 2024 •

edited

Loading

JohanBertrand Apr 10, 2024 •

edited

Loading

LeStarch commented Apr 11, 2024 •

edited

Loading