Skip to content

Workflow update#51

Merged
JasonBrave merged 9 commits intomainfrom
workflow
Oct 15, 2025
Merged

Workflow update#51
JasonBrave merged 9 commits intomainfrom
workflow

Conversation

@JasonBrave
Copy link
Member

No description provided.

@JasonBrave JasonBrave marked this pull request as ready for review October 12, 2025 14:19
Copy link
Contributor

@taisirhassan taisirhassan left a comment

Choose a reason for hiding this comment

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

look at the comments please and change

while ((opt = getopt(argc, argv, "s:")) != -1) {
switch (opt) {
case 's':
seed = atoi(optarg);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we replace atoi with std::stoi for proper error handling.

atoi returns 0 for invalid input with no error indication. If a user provides an invalid seed value (e.g., "abc"), it silently becomes 0, which could lead to confusion during debugging.

				try {
					seed = std::stoi(optarg);
				} catch (const std::exception &e) {
					std::cerr << "Invalid seed value: " << optarg << std::endl;
					return EXIT_FAILURE;
				}

Comment on lines +66 to +67
tests[test_name] = this;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add validation for nullptr and duplicate test names?

rn the constructor has two potential issues:

if test_name is nullptr, storing it in the map will cause undefined behaviour.

also duplicate test names silently overwrite the previous test, which could mask bugs.
can we consider adding validation:

smth like

 rockettest_test::rockettest_test(const char *test_name) : name(test_name) {
	if (test_name == nullptr) {
		std::cerr << "Error: test_name cannot be null" << std::endl;
		std::abort();
	}
	if (tests.contains(test_name)) {
		std::cerr << "Error: duplicate test name '" << test_name << "'" << std::endl;
		std::abort();
	}
     tests[test_name] = this;
 }


rockettest_test() = delete;
rockettest_test(const char *test_name);

Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a virtual destructor to the polymorphic base class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
virtual ~rockettest_test() = default;

Comment on lines +14 to +19
#define rockettest_assert(statement) \
if (!(statement)) { \
std::cout << "Assertion failed " << __FILE__ << ':' << __LINE__ << ' ' << #statement \
<< std::endl; \
test_passed = false; \
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can we wrap the macro in do-while(0) and document the test_passed requirement that users must declare a bool test_passed variable in the scope where this macro is invoked?

- name: Run lint check
id: run-lint
shell: bash
run: clang-tidy-21 include/*.h common/*.c --warnings-as-errors="*" --checks="clang-*,misc-*" --extra-arg-before="-std=c99" --extra-arg-before="-pedantic" --extra-arg-before="-Iinclude"
Copy link
Contributor

@taisirhassan taisirhassan Oct 14, 2025

Choose a reason for hiding this comment

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

rn the current command has several issues this also applies to local script as well:

  • one Glob patterns (include/.h, common/.c) may not expand correctly in this context

  • No compilation database is provided, which clang-tidy needs for accurate cross-file analysis

  • Missing error handling (the step should fail if clang-tidy finds issues, which --warnings-as-errors handles, but shell errors are not caught)

I would change to use a shell env script something like this is more reliable and better for clang-tidy

       - name: Run lint check
         id: run-lint
         shell: bash
        run: |
          set -e
          find include -name "*.h" -o -name "*.c" > files.txt
          find common -name "*.c" >> files.txt
          if [ -s files.txt ]; then
            xargs -a files.txt clang-tidy-21 \
              --warnings-as-errors="*" \
              --checks="clang-*,misc-*" \
              -- -std=c99 -pedantic -Iinclude
          else
            echo "No files found to lint"
            exit 1
          fi

@JasonBrave
Copy link
Member Author

The rockettest is copied from canlib, will take a look at the comments in a future PR

@JasonBrave JasonBrave merged commit 6cdd73e into main Oct 15, 2025
4 of 5 checks passed
@JasonBrave JasonBrave deleted the workflow branch October 15, 2025 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants