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.

Sign up for GitHub

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

Jump to bottom

Add Fw::ExternalString and revise string implementations #2679

Merged
merged 40 commits into from
Apr 25, 2024
Merged

Add Fw::ExternalString and revise string implementations #2679

merged 40 commits into from
Apr 25, 2024

Conversation

bocchino
Copy link
Collaborator

@bocchino bocchino commented Apr 13, 2024 •
edited
Loading

This PR does the following:

  1. Add a new class Fw::ExternalString. This class is derived from Fw::StringBase. It uses an externally supplied character buffer to implement a string. I added tests for Fw::ExternalString to the Fw/Types unit tests.
  2. Refactor the string implementations in the framework to reduce code duplication.

Rationale:

  1. Fw::ExternalString is needed to improve the C++ code generation strategy for strings. See Minimize use of auto-generated string types in C++ code gen fpp#406.
  2. The refactored implementation eliminates a significant amount of copy-paste code when creating a new concrete string type. The new version should be functionally identical to the old version.

Notes:

  1. Review and merge Format cpp and hpp files in Fw/Types #2677 first. A lot of changes in this PR are formatting changes that will go away once Format cpp and hpp files in Fw/Types #2677 is merged.
  2. Static analysis is telling us that single-argument constructors should be marked explicit. When I tried this, I saw compilation errors in the generated C++. I think this issue will have to be addressed separately.

@bocchino bocchino requested a review from LeStarch April 13, 2024 00:00
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 13, 2024
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

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

@bocchino bocchino mentioned this pull request Apr 13, 2024
Closed
3 tasks
@bocchino bocchino marked this pull request as draft April 13, 2024 01:05
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 13, 2024
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

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

github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 13, 2024
View reviewed changes
Fw/Types/PolyType.cpp Outdated
} else {
dest = valString;
}
#if FW_SERIALIZABLE_TO_STRING || BUILD_UT

Check notice

Code scanning / CodeQL

Conditional compilation Note

Use of conditional compilation must be kept to a minimum.
Fw/Types/PolyType.cpp Outdated
case TYPE_I8:
(void)snprintf(valString, sizeof(valString), "%" PRId8 " ", this->m_val.i8Val);
break;
#if FW_HAS_16_BIT

Check notice

Code scanning / CodeQL

Conditional compilation Note

Use of conditional compilation must be kept to a minimum.
Fw/Types/PolyType.cpp Outdated
(void)snprintf(valString, sizeof(valString), "%" PRId16 " ", this->m_val.i16Val);
break;
#endif
#if FW_HAS_32_BIT

Check notice

Code scanning / CodeQL

Conditional compilation Note

Use of conditional compilation must be kept to a minimum.
Fw/Types/PolyType.cpp Outdated
(void)snprintf(valString, sizeof(valString), "%" PRId32 " ", this->m_val.i32Val);
break;
#endif
#if FW_HAS_64_BIT

Check notice

Code scanning / CodeQL

Conditional compilation Note

Use of conditional compilation must be kept to a minimum.
Fw/Types/PolyType.cpp Outdated
#endif
#if FW_HAS_F64

Check notice

Code scanning / CodeQL

Conditional compilation Note

Use of conditional compilation must be kept to a minimum.
Fw/Types/Serializable.cpp Outdated

#ifdef BUILD_UT

Check notice

Code scanning / CodeQL

Conditional compilation Note

Use of conditional compilation must be kept to a minimum.
@bocchino
Copy link
Collaborator Author

Converting this PR to draft because I need to do some refactoring.

github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 13, 2024
View reviewed changes
Fw/Types/MemAllocator.hpp Outdated

private:
MemAllocator(MemAllocator&); //!< disable
MemAllocator(MemAllocator*); //!< disable

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.
Fw/Types/PolyType.hpp Outdated
PolyType& operator=(U8 val); //!< U8 operator=
class PolyType : public Serializable {
public:
PolyType(U8 val); //!< U8 constructor

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.
Fw/Types/PolyType.hpp Outdated
void get(I8& val); //!< I8 accessor
bool isI8(); //!< I8 checker
PolyType& operator=(I8 val); //!< I8 operator=
PolyType(I8 val); //!< I8 constructor

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.
Fw/Types/PolyType.hpp Outdated
void get(I16& val); //!< I16 accessor
bool isI16(); //!< I16 checker
PolyType& operator=(I16 val); //!< I16 operator=
PolyType(U16 val); //!< U16 constructor

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.
Fw/Types/PolyType.hpp Outdated
bool isU16(); //!< U16 checker
PolyType& operator=(U16 val); //!< I8 operator=

PolyType(I16 val); //!< I16 constructor

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.
Fw/Types/PolyType.hpp Outdated
bool isBool(); //!< bool checker
PolyType& operator=(bool val); //!< bool operator=

PolyType(void* val); //!< void* constructor.

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.
Fw/Prm/PrmString.hpp

const char* toChar() const;
NATIVE_UINT_TYPE getCapacity() const;
ParamString(const StringBase& src) : StringBase() { *this = src; }

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.
Fw/Prm/PrmString.hpp

private:
ParamString(const char* src) : StringBase() { *this = src; }

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.
Fw/Test/String.hpp

const char* toChar() const; //!< gets char buffer
NATIVE_UINT_TYPE getCapacity() const ; //!< return buffer size
String(const StringBase& src) : StringBase() { *this = src; }

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.
Fw/Test/String.hpp

private:
String(const char* src) : StringBase() { *this = src; }

Check warning

Code scanning / CppCheck

Single-parameter constructors should be marked explicit. Warning

Single-parameter constructors should be marked explicit.
@bocchino bocchino marked this pull request as ready for review April 13, 2024 19:06
LeStarch
LeStarch reviewed Apr 16, 2024
View reviewed changes
Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

I did not see what I expected from this PR. Awaiting more input.

Fw/Cmd/CmdString.hpp Show resolved Hide resolved
Fw/Cmd/CmdString.hpp Show resolved Hide resolved
@bocchino bocchino mentioned this pull request Apr 23, 2024
Merged
@LeStarch LeStarch merged commit 88d65d7 into nasa:devel Apr 25, 2024
34 checks passed
@bocchino bocchino deleted the external-string branch April 25, 2024 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeStarch LeStarch LeStarch approved these changes

@thomas-bc thomas-bc thomas-bc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bocchino @LeStarch @thomas-bc