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

[Test.cpp] Optimize size #97

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

[Test.cpp] Optimize size #97

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions src/aunit/Test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,25 +76,20 @@ void Test::resolve() {
if (!isVerbosity(Verbosity::kTestAll)) return;

Print* printer = Printer::getPrinter();
printer->print(TEST_STRING);
Copy link
Owner

Choose a reason for hiding this comment

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

Good attempt, but now the behavior is subtly different when mStatus == Test::kStatusUnknown, right? The current code prints nothing, but your code now prints "Test ", without a trailing newline, which I think is a bit worse. The code duplication should be inconsequential on a desktop, so the memory savings is relevant only to an 8-bit processor, and I think even there, I think the flash memory savings will amount to about 3 x (10-15) bytes, or about 30-50 bytes.

Copy link
Author

@hsaturn hsaturn Jan 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes ... and this is exactly why you should use enum instead of const ints. The compiler sends a warning when unused enums are not in any case.
This is exactly what I'm doing for AUnitPC (I'm gonna rename it with this name I guess).

edit: I've finished to group many unrelated const uint8_t into three enum classes

  • LifeCycle
  • Verbosity
  • Status

You may have a look to LifeCycle and Verbosity that should apply as they are.
For Status, I've changed Test::resolve() a way you may not like.

I prefer a lot to have enum classes because compiler will report an error when one tries to mix together values that are not related.

LifeCycle : hsaturn@1fe7947
Verbosity: hsaturn@741c164
Status: hsaturn@58f0ca8

Important note : I've not regenerated the doc after each commit, instead sed -i have changed the names of the constants, ugly.

The documentation once regenerated is pretty cool with enums 👍
Constants are grouped by function instead of mixed together. Here is what I have for the Status:
image

Regards

Copy link
Author

Choose a reason for hiding this comment

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

By the way, doing the changes, I've found that kStatusDone in documentation does not exists. Same for kStatusSetup if I'm not wrong.

Copy link
Owner

Choose a reason for hiding this comment

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

enum class is probably safer, but I created AUnit before I learned about it, and I didn't have motivation to go back to this, since most (if not all) of this is internal and not exposed to the end-user except through the Failed/Passed status messages. The LifeCycle and Status variables actually started as a single variable, and only later did it occur to me that it makes more sense to split them, so there might be some residual commingling between them.

Funny thing about enum class, when I learned about it, I started using it, then I started to find them a bit annoying and unergonomic, so I found myself slowly shifting back to using just static constexpr for internal things. I don't know, I've started to write more C and Golang, and neither of them have enums, so I guess they caused a shift in my thinking process. I think it's all part of my "C++ has too much syntax and cognitive overhead" annoyance.

Copy link
Owner

Choose a reason for hiding this comment

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

Correction: C has enum, not enum class, but if I recall, C-enum is in the global namespace and doesn't introduce a new type, so basically the same as an int

mName.print(printer);

if (mStatus == Test::kStatusPassed
&& isVerbosity(Verbosity::kTestPassed)) {
printer->print(TEST_STRING);
mName.print(printer);
printer->println(F(" passed."));
} else if (mStatus == Test::kStatusFailed
&& isVerbosity(Verbosity::kTestFailed)) {
printer->print(TEST_STRING);
mName.print(printer);
printer->println(F(" failed."));
} else if (mStatus == Test::kStatusSkipped
&& isVerbosity(Verbosity::kTestSkipped)) {
printer->print(TEST_STRING);
mName.print(printer);
printer->println(F(" skipped."));
} else if (mStatus == Test::kStatusExpired
&& isVerbosity(Verbosity::kTestExpired)) {
printer->print(TEST_STRING);
mName.print(printer);
printer->println(F(" timed out."));
}
}
Expand Down