Replace all string classes with StringTemplate aliases#167
Replace all string classes with StringTemplate aliases#167devin-ai-integration[bot] wants to merge 3 commits into
Conversation
Replace the hand-coded InternalInterfaceString class with a using alias for Fw::StringTemplate<FW_INTERNAL_INTERFACE_STRING_MAX_SIZE>. This demonstrates the pattern for eliminating boilerplate string classes that are structurally identical to StringTemplate. The unused SERIALIZED_TYPE_ID enum member is dropped (never referenced outside the header). Relates to nasa#5018 Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>
Original prompt from michael.d.starch
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
…aliases Replace three more hand-coded string classes with using aliases for Fw::StringTemplate. All three already had explicit on the const char* constructor (matching StringTemplate) and none had their SERIALIZED_TYPE_ID referenced outside the header. - CmdStringArg = StringTemplate<FW_CMD_STRING_MAX_SIZE> - FileNameString = StringTemplate<FileNameStringSize> - ObjectName = StringTemplate<FW_OBJ_NAME_BUFFER_SIZE> The remaining string classes (String, TlmString, LogStringArg, TextLogString, ParamString) lack explicit on their const char* constructor, so replacing them would change implicit conversion behavior and is not a simple drop-in. Relates to nasa#5018 Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>
Coverage report — base
|
| Module | Line | Function | Branch |
|---|---|---|---|
CFDP/Checksum |
71.15 | 57.14 | 53.85 |
Drv/AsyncByteStreamBufferAdapter |
100.00 | 100.00 | 100.00 |
Drv/ByteStreamBufferAdapter |
100.00 | 100.00 | 100.00 |
Drv/Ip |
44.32 | 57.38 | 25.36 |
Drv/TcpClient |
75.00 | 100.00 | 47.37 |
Drv/TcpServer |
84.72 | 100.00 | 60.00 |
Drv/Udp |
68.09 | 90.91 | 42.86 |
Fw/Buffer |
81.25 | 89.47 | 58.62 |
Fw/DataStructures |
98.21 | 97.14 | 82.84 |
Fw/Dp |
94.83 | 96.67 | 95.95 |
Fw/FilePacket |
75.24 | 89.06 | 54.30 |
Fw/Log |
71.43 | 72.41 | 60.00 |
Fw/Logger |
100.00 | 100.00 | 100.00 |
Fw/SerializableFile |
90.00 | 100.00 | 79.41 |
Fw/Time |
88.62 | 85.48 | 86.84 |
Fw/Tlm |
53.76 | 60.00 | 37.50 |
Fw/Types |
54.33 | 56.97 | 34.75 |
Os |
18.07 | 19.52 | 14.52 |
Os/Generic |
89.10 | 96.30 | 67.18 |
Os/Generic/Types |
92.45 | 91.67 | 82.14 |
Os/Posix |
62.40 | 84.21 | 44.10 |
Svc/ActiveRateGroup |
100.00 | 100.00 | 92.31 |
Svc/ActiveTextLogger |
79.05 | 90.00 | 71.23 |
Svc/AssertFatalAdapter |
94.74 | 100.00 | 86.67 |
Svc/BufferAccumulator |
88.00 | 94.12 | 74.49 |
Svc/BufferLogger |
92.62 | 86.96 | 80.46 |
Svc/BufferManager |
99.05 | 100.00 | 83.64 |
Svc/BufferRepeater |
91.67 | 100.00 | 75.00 |
Svc/ChronoTime |
100.00 | 100.00 | 100.00 |
Svc/CmdDispatcher |
96.97 | 91.67 | 91.75 |
Svc/CmdSequencer |
93.89 | 97.12 | 84.63 |
Svc/CmdSplitter |
100.00 | 100.00 | 100.00 |
Svc/ComLogger |
97.37 | 91.67 | 83.91 |
Svc/ComSplitter |
100.00 | 100.00 | 100.00 |
Svc/ComStub |
98.51 | 100.00 | 85.19 |
Svc/DpCatalog |
78.00 | 100.00 | 66.27 |
Svc/DpManager |
97.44 | 100.00 | 100.00 |
Svc/DpWriter |
97.58 | 90.00 | 97.22 |
Svc/FileDownlink |
84.15 | 90.91 | 71.90 |
Svc/FileManager |
88.41 | 93.33 | 82.44 |
Svc/FileUplink |
92.35 | 96.55 | 80.95 |
Svc/FileWorker |
90.29 | 100.00 | 84.87 |
Svc/FprimeDeframer |
100.00 | 100.00 | 97.92 |
Svc/FprimeFramer |
100.00 | 100.00 | 95.45 |
Svc/FprimeRouter |
88.89 | 100.00 | 78.26 |
Svc/FpySequencer |
86.76 | 99.04 | 76.89 |
Svc/GenericHub |
100.00 | 100.00 | 85.64 |
Svc/Health |
100.00 | 100.00 | 88.66 |
Svc/LinuxTimer |
97.06 | 100.00 | 82.35 |
Svc/OsTime |
70.00 | 83.33 | 60.98 |
Svc/PassiveRateGroup |
100.00 | 100.00 | 89.47 |
Svc/PolyDb |
100.00 | 100.00 | 53.57 |
Svc/PosixTime |
100.00 | 100.00 | 75.00 |
Svc/PrmDb |
92.75 | 89.47 | 88.24 |
Svc/RateGroupDriver |
100.00 | 100.00 | 68.75 |
Svc/SeqDispatcher |
73.08 | 80.00 | 71.05 |
Svc/StaticMemory |
100.00 | 100.00 | 100.00 |
Svc/SystemResources |
98.63 | 100.00 | 76.19 |
Svc/TlmChan |
77.59 | 85.71 | 63.03 |
Svc/TlmPacketizer |
92.49 | 100.00 | 77.45 |
Svc/Version |
96.10 | 100.00 | 86.96 |
Utils |
20.04 | 26.44 | 24.07 |
Utils/Types |
92.11 | 95.83 | 77.08 |
Modules without UTs
CFDP/Checksum/GTest, Drv/ByteStreamDriverModel, Drv/Interfaces, Drv/LinuxGpioDriver, Drv/LinuxI2cDriver, Drv/LinuxSpiDriver, Drv/LinuxUartDriver, Drv/Ports, Drv/Ports/DataTypes, FppTestProject/FppTest/interfaces, FppTestProject/FppTest/topology/async, FppTestProject/FppTest/topology/components/Comp, FppTestProject/FppTest/topology/components/Framework, FppTestProject/FppTest/topology/components/Receiver, FppTestProject/FppTest/topology/components/Sender, FppTestProject/FppTest/topology/guarded, FppTestProject/FppTest/topology/ports, FppTestProject/FppTest/topology/sync, FppTestProject/FppTest/topology/top_ports, FppTestProject/FppTest/topology/types, Fw/Cmd, Fw/Com, Fw/Comp, Fw/FilePacket/GTest, Fw/Fpy, Fw/Interfaces, Fw/Obj, Fw/Port, Fw/Ports/CompletionStatus, Fw/Ports/Ready, Fw/Ports/Signal, Fw/Ports/SuccessCondition, Fw/Prm, Fw/SerializableFile/test/TestSerializable, Fw/Sm, Fw/Test, Fw/Types/GTest, Os/Models, Svc/Cycle, Svc/DpPorts, Svc/Fatal, Svc/FatalHandler, Svc/FileDownlinkPorts, Svc/FprimeProtocol, Svc/Interfaces, Svc/PassiveConsoleTextLogger, Svc/Ping, Svc/PolyIf, Svc/Ports/CommsPorts, Svc/Ports/FilePorts, Svc/Ports/OsTimeEpoch, Svc/Ports/TlmPacketizerPorts, Svc/Ports/VersionPorts, Svc/Sched, Svc/Seq, Svc/Subtopologies/CdhCore, Svc/Subtopologies/ComCcsds, Svc/Subtopologies/ComFprime, Svc/Subtopologies/ComLoggerTee, Svc/Subtopologies/DataProducts, Svc/Subtopologies/FileHandling, Svc/Types/TlmPacketizerTypes, Svc/WatchDog, TestDeploymentsProject/Ref/PingReceiver, TestDeploymentsProject/Ref/RecvBuffApp, TestDeploymentsProject/Ref/SendBuffApp, TestDeploymentsProject/Ref/Top, TestDeploymentsProject/Ref/TypeDemo, cmake/test/data/TestDeployment/TestBuildAutocoder, cmake/test/data/TestDeployment/TestChainedAutocoder, cmake/test/data/TestDeployment/TestHeaderAutocoder, cmake/test/data/TestDeployment/TestTargetAutocoder, cmake/test/data/test-fprime-library/TestLibrary/TestComponent, cmake/test/data/test-fprime-library2/TestLibrary2/TestComponent
Remove explicit from StringTemplate's const char* constructor to match the concrete string classes, then replace the remaining five: - String = StringTemplate<FW_FIXED_LENGTH_STRING_SIZE> - TlmString = StringTemplate<FW_TLM_STRING_MAX_SIZE> - LogStringArg = StringTemplate<FW_LOG_STRING_MAX_SIZE> - TextLogString = StringTemplate<FW_LOG_TEXT_BUFFER_SIZE> - ParamString = StringTemplate<FW_PARAM_STRING_MAX_SIZE> All nine hand-coded string classes are now using aliases for StringTemplate, eliminating ~500 lines of duplicated boilerplate. Relates to nasa#5018 Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>
| const char* toChar() const { return this->m_buf; } | ||
|
|
||
| StringBase::SizeType getCapacity() const { return sizeof this->m_buf; } | ||
| using CmdStringArg = StringTemplate<FW_CMD_STRING_MAX_SIZE>; |
There was a problem hiding this comment.
🚩 Type aliasing collapses previously distinct types when sizes match
With default configuration values, several previously distinct class types now become identical types:
CmdStringArg,TlmString, andParamStringall becomeStringTemplate<40>InternalInterfaceString,TextLogString, andString(Fw::String) all becomeStringTemplate<256>
Previously, passing a Fw::TlmString where a Fw::CmdStringArg was expected by value would require an implicit conversion through ConstStringBase. Now they are the same type, so there's no type-system distinction at all. This doesn't break existing code, but it weakens compile-time type safety: mistakes like passing the wrong string category to a function will no longer produce type errors when the sizes happen to match. If any downstream code ever adds function overloads distinguishing these types, it would get ambiguous overload errors. No current overload conflicts were found.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good observation. This is an inherent tradeoff of the using alias approach — types with matching sizes collapse to the same StringTemplate<N> instantiation. If stronger type distinction is needed in the future, the template could add a tag parameter (e.g., StringTemplate<size, Tag>) to keep types nominally distinct. For now, no overload conflicts exist and the maintainer has accepted this tradeoff.
| StringTemplate(const ConstStringBase& src) : StringBase() { *this = src; } | ||
|
|
||
| explicit StringTemplate(const char* src) : StringBase() { *this = src; } | ||
| StringTemplate(const char* src) : StringBase() { *this = src; } |
There was a problem hiding this comment.
🚩 Removal of explicit from StringTemplate affects all instantiations
The PR removes explicit from StringTemplate(const char* src) at Fw/Types/StringTemplate.hpp:30. This was necessary because some of the old classes being replaced (LogStringArg, TlmString, ParamString, TextLogString, String) did NOT have explicit on their const char* constructor, and since they now alias StringTemplate, it can't be explicit.
However, this also removes explicit from types that previously had it: CmdStringArg (Fw/Cmd/CmdString.hpp:31), FileNameString (Fw/Types/FileNameString.hpp:32), InternalInterfaceString (Fw/Types/InternalInterfaceString.hpp:32), and ObjectName (Fw/Types/ObjectName.hpp:31). Additionally, it affects all pre-existing direct uses of StringTemplate (e.g., FppTestProject/FppTest/component/types/FormalParamTypes.hpp:197-198), which previously required explicit construction from const char* and now allow implicit conversion. No concrete breakage was found, but this is a broadening of the implicit conversion surface.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — this is intentional per the maintainer's request. The four classes that previously had explicit (CmdStringArg, FileNameString, InternalInterfaceString, ObjectName) now allow implicit const char* conversion. No concrete breakage was found, and loosening explicit is backwards-compatible (existing call sites continue to compile).
| StringTemplate(const ConstStringBase& src) : StringBase() { *this = src; } | ||
|
|
||
| explicit StringTemplate(const char* src) : StringBase() { *this = src; } | ||
| StringTemplate(const char* src) : StringBase() { *this = src; } |
| StringTemplate(const ConstStringBase& src) : StringBase() { *this = src; } | ||
|
|
||
| explicit StringTemplate(const char* src) : StringBase() { *this = src; } | ||
| StringTemplate(const char* src) : StringBase() { *this = src; } |
| StringTemplate(const ConstStringBase& src) : StringBase() { *this = src; } | ||
|
|
||
| explicit StringTemplate(const char* src) : StringBase() { *this = src; } | ||
| StringTemplate(const char* src) : StringBase() { *this = src; } |
| StringTemplate(const ConstStringBase& src) : StringBase() { *this = src; } | ||
|
|
||
| explicit StringTemplate(const char* src) : StringBase() { *this = src; } | ||
| StringTemplate(const char* src) : StringBase() { *this = src; } |
Change Description
Replaces all nine hand-coded string classes with
usingaliases forFw::StringTemplate, and removesexplicitfromStringTemplate'sconst char*constructor to match the concrete classes:Each replaced class was structurally identical to
StringTemplate. The only difference was the unusedSERIALIZED_TYPE_IDenum member (never referenced outside the headers, confirmed via codebase search). Eliminates ~500 lines of duplicated boilerplate.Rationale
All nine concrete string classes duplicated the exact same implementation that
StringTemplatealready provides. TheSERIALIZED_TYPE_IDenum members were defined but never referenced in the codebase.Testing/Review Recommendations
STRING_SIZEandSERIALIZED_SIZEare still available viaStringTemplate's enum — existing code referencing e.g.FileNameString::STRING_SIZE(8 call sites),Fw::String::STRING_SIZE/SERIALIZED_SIZE(multiple call sites) continues to compileSERIALIZED_TYPE_IDfor any of these string classesexplicitremoval fromStringTemplateis intentional: five of the nine concrete classes (String,TlmString,LogStringArg,TextLogString,ParamString) allowed implicitconst char*conversion, soStringTemplatemust allow it tooFuture Work
None for string classes — all are now converted. The parallel effort for buffer classes (LinearBufferTemplate, PR #166) follows the same pattern.
AI Usage (see policy)
AI was used for codebase analysis (identifying all replaceable classes, verifying interface parity, confirming no
SERIALIZED_TYPE_IDreferences, checkingexplicitconsistency), and generating the replacement code.IAMAI
Link to Devin session: https://nasa-jpl-demo.devinenterprise.com/sessions/469e7bbc20f145f48eef872b618ecee2