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

Update `ActiveTextLogger` naming to match build system naming expectations #2509

Merged

LeStarch merged 2 commits into nasa:devel from kbotteon:kbotteon/fix-ActiveTextLogger

Feb 1, 2024

Contributor

kbotteon commented Jan 31, 2024


*Related Issue(s)*
*Has Unit Tests (y/n)*	Yes
*Documentation Included (y/n)*	Yes

Change Description

This PR updates file names and CMakeLists for ActiveTextLogger to conform with what I believe the build system requires. See Rationale below.

I could also be completely overlooking a pattern or small detail, so please let me know if this change is unnecessary! I can't imagine I'm the first person to run into this problem with such a common Component, so I suspect another issue in my configuration might be involved.

Rationale

Including ActiveTextLogger in a topology leads to a compiler error in an auto-coded file:

TopologyAc.hpp:16:10: fatal error: Svc/ActiveTextLogger/ActiveTextLogger.hpp: No such file or directory
 #include "Svc/ActiveTextLogger/ActiveTextLogger.hpp"

If I change the fpp to use ActiveTextLoggerImpl, now the build fails at fpp-to-cpp:

Declarations.fpp:79.23
instance logText: Svc.ActiveTextLoggerImpl \
                      ^
error: undefined symbol ActiveTextLoggerImpl

I think the latest build system expects naming like Svc/<ComponentName>/<ComponentName> without the ComponentImpl class or Impl file suffixes.

Testing/Review Recommendations

My topology including ActiveTextLogger now compiles. fprime-util check –all runs with 100% tests passed, 0 tests failed out of 54.

kbotteon added 2 commits

January 31, 2024 11:02


          fix: old ActiveTextLogger naming breaks with new FPrime build system

8bae22d


          fix: broke ActiveTextLogger UTs

github-advanced-security bot found potential problems

View reviewed changes

Svc/ActiveTextLogger/ActiveTextLogger.cpp

@@ @@ -35,7 +35,7 @@ @@
                   // Handlers to implement for typed input ports
                   // ----------------------------------------------------------------------
-                  void ActiveTextLoggerComponentImpl::TextLogger_handler(NATIVE_INT_TYPE portNum,
+                  void ActiveTextLogger::TextLogger_handler(NATIVE_INT_TYPE portNum,

Check notice

Code scanning / CodeQL

Use of basic integral type Note

portNum uses the basic integral type int rather than a typedef with size and signedness.

Svc/ActiveTextLogger/ActiveTextLogger.cpp

@@ @@ -132,7 +132,7 @@ @@
                   // Helper Methods
                   // ----------------------------------------------------------------------
-                  bool ActiveTextLoggerComponentImpl::set_log_file(const char* fileName, const U32 maxSize, const U32 maxBackups)
+                  bool ActiveTextLogger::set_log_file(const char* fileName, const U32 maxSize, const U32 maxBackups)

Check notice

Code scanning / CodeQL

Use of basic integral type Note

set_log_file uses the basic integral type bool rather than a typedef with size and signedness.

Svc/ActiveTextLogger/ActiveTextLogger.cpp

@@ @@ -132,7 +132,7 @@ @@
                   // Helper Methods
                   // ----------------------------------------------------------------------
-                  bool ActiveTextLoggerComponentImpl::set_log_file(const char* fileName, const U32 maxSize, const U32 maxBackups)
+                  bool ActiveTextLogger::set_log_file(const char* fileName, const U32 maxSize, const U32 maxBackups)

Check notice

Code scanning / CodeQL

Use of basic integral type Note

fileName uses the basic integral type char rather than a typedef with size and signedness.

Svc/ActiveTextLogger/ActiveTextLogger.cpp

                   {
                   }
-                  void ActiveTextLoggerComponentImpl::init(NATIVE_INT_TYPE queueDepth, NATIVE_INT_TYPE instance)
+                  void ActiveTextLogger::init(NATIVE_INT_TYPE queueDepth, NATIVE_INT_TYPE instance)

Check notice

Code scanning / CodeQL

Use of basic integral type Note

queueDepth uses the basic integral type int rather than a typedef with size and signedness.

Svc/ActiveTextLogger/ActiveTextLogger.cpp

                   {
                   }
-                  void ActiveTextLoggerComponentImpl::init(NATIVE_INT_TYPE queueDepth, NATIVE_INT_TYPE instance)
+                  void ActiveTextLogger::init(NATIVE_INT_TYPE queueDepth, NATIVE_INT_TYPE instance)

Check notice

Code scanning / CodeQL

Use of basic integral type Note

instance uses the basic integral type int rather than a typedef with size and signedness.

Svc/ActiveTextLogger/ActiveTextLogger.cpp

@@ @@ -14,19 +14,19 @@ @@
                   // Initialization/Exiting
                   // ----------------------------------------------------------------------
-                  ActiveTextLoggerComponentImpl::ActiveTextLoggerComponentImpl(const char* name) :
+                  ActiveTextLogger::ActiveTextLogger(const char* name) :

Check notice

Code scanning / CodeQL

Use of basic integral type Note

name uses the basic integral type char rather than a typedef with size and signedness.

github-advanced-security bot found potential problems

View reviewed changes

Svc/ActiveTextLogger/ActiveTextLogger.cpp

@@ @@ -35,7 +35,7 @@ @@
                   // Handlers to implement for typed input ports
                   // ----------------------------------------------------------------------
-                  void ActiveTextLoggerComponentImpl::TextLogger_handler(NATIVE_INT_TYPE portNum,
+                  void ActiveTextLogger::TextLogger_handler(NATIVE_INT_TYPE portNum,

Check notice

Code scanning / CodeQL

Function too long Note

TextLogger_handler has too many lines (77, while 60 are allowed).

Svc/ActiveTextLogger/ActiveTextLogger.cpp

@@ @@ -35,7 +35,7 @@ @@
                   // Handlers to implement for typed input ports
                   // ----------------------------------------------------------------------
-                  void ActiveTextLoggerComponentImpl::TextLogger_handler(NATIVE_INT_TYPE portNum,
+                  void ActiveTextLogger::TextLogger_handler(NATIVE_INT_TYPE portNum,

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.

thomas-bc approved these changes

View reviewed changes

LeStarch approved these changes

View reviewed changes

Collaborator

LeStarch commented Feb 1, 2024

@kbotteon we previously added headers that typedefed the Impl version of the class to the non-impl version. These headers were named with the new preferred name.

However, renaming is a much better fix and since you are doing a single component....I wholeheartedly approve!

LeStarch merged commit d1f166e into nasa:devel

34 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet