Cleanup string classes

[Joshua-Anderson]

Originating Project/Creator
Affected Component
Affected Architectures(s)
Related Issue(s)
Has Unit Tests (y/n)
Builds Without Errors (y/n)
Unit Tests Pass (y/n)	y
Documentation Included (y/n)

---

Change Description

Cleanup F' string classes

Rationale

• Moves common functionality to StringBase, preventing code duplication
• Doesn't call the getCapacity virtual function on a class while it's being constructed, preventing clang tidy warnings
• Passing size of other string to function when calling copyBuffer

Future Work

I wan't able to cleanup the LogStringArg and TlmString classes yet - they both have setMaxSerial methods that are theoretically used for per-telm channel and per-event string truncation, but we should have a discussion about if that functionality is necessary and if there's better ways to accomplish it.

[lgtm-com]

This pull request introduces 8 alerts when merging de54549 into 7b85edc - view on LGTM.com

new alerts:

• 8 for Inconsistent definition of copy constructor and assignment ('Rule of Two')

[Joshua-Anderson]

LGTM is complaining that there are string class copy constructors but not copy assignment overloads. The copy assignment overload is unnecessary because the StringBase class already has a generic copy assignment overload for all strings.

Since string classes already have a copy constructor from StringBase, ideally I'd remove the class copy constructor to get rid of this warning. Unfortunately, removing this class copy constructor triggers compiler warnings that the copy constructor isn't defined, despite the fact a perfectly valid StringBase constructor exists. I'm curious if there's some way I could hint at the compiler to use the StringBase constructor as the class copy constructor as well.

[LeStarch]

@Joshua-Anderson you left a comment here is this work finalized or are you still working?

[Joshua-Anderson]

@LeStarch This is ready to go... I just wanted to justify the LGTM warnings and see if anybody with more C++ experience has suggestions for better ways to solve this problem.

[LeStarch]

@Joshua-Anderson the copy constructor for any class will either be explicit (user supplied) or implicit (compiler supplied) as the compiler must guarantee that the class is fully constructed after the constructor is called. For this reason it cannot use the base-class defined constructor. If an implicit constructor is used, the "default" base class constructor is used.

[Joshua-Anderson]

@LeStarch Following our discussion, I've added in copy assignment constructors as well as copy constructors. It seems likely that the compiler is properly calling the parent copy assignment constructor but it's unclear at best. Since C++ best practices recommend defining the copy assignment constructor when a copy constructor is defined, we may as well define the copy assignment constructors.

[LeStarch]

Discussed this with @timcanham. We decided it would be best to avoid virtual function use altogether in the constructors....so we should perform the copy with string_utils::string_copy in the constructors.

Also we should clean up the base-class assignment operators as they return the wrong type. Based on the stl::string class a copy constructor should do:

const T& T::operator=(const B&)

[Joshua-Anderson]

I replaced all the calls to copyBuff with a direct call to StringCopy. @LeStarch and I came to the consensus that T& T::operator=(const B&) is the C++ best practice so I didn't update the assignment operator signature

[github-actions]

@check-spelling-bot Report

Unrecognized words, please review:

• bootlin

Previously acknowledged words that are now absent

lxr

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:Joshua-Anderson/fprime.git repository
on the strings-cleanup branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/nasa/fprime/issues/comments/889563356" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

[LeStarch]

Spelling fixed in another PR.