From 5c1725062eeac918709c381ccf167462f5e05b1e Mon Sep 17 00:00:00 2001 From: Amir Kamil Date: Sun, 8 Sep 2024 03:28:51 -0400 Subject: [PATCH] Use .at() to avoid UB in tutorial code. Fix a bug in "correct" implementation. --- docs/tutorial.cpp | 14 +++--- metatest-common.mk | 62 ++++++++++++++++++++++++ tutorial_buggy_impls.cpp | 100 ++++++++++++++++++--------------------- 3 files changed, 115 insertions(+), 61 deletions(-) create mode 100644 metatest-common.mk diff --git a/docs/tutorial.cpp b/docs/tutorial.cpp index 47a66a4..24c0376 100644 --- a/docs/tutorial.cpp +++ b/docs/tutorial.cpp @@ -12,8 +12,8 @@ double add(double first, double second) { // contain [4,0,1,3,3] void slideRight(std::vector &v) { // NOTE - This implementation contains a bug! - for(int i = 1; i < static_cast(v.size()); ++i) { - v[i] = v[i-1]; + for (int i = 1; i < static_cast(v.size()); ++i) { + v.at(i) = v.at(i-1); } } @@ -25,11 +25,11 @@ void flip(std::vector &v) { // NOTE - This implementation contains a bug! int left = 0; int right = static_cast(v.size()) - 1; - while(left != right) { - int temp = v[left]; - v[left] = v[right]; - v[right] = temp; + while (left != right) { + int temp = v.at(left); + v.at(left) = v.at(right); + v.at(right) = temp; ++left; --right; } -} \ No newline at end of file +} diff --git a/metatest-common.mk b/metatest-common.mk new file mode 100644 index 0000000..6756cab --- /dev/null +++ b/metatest-common.mk @@ -0,0 +1,62 @@ +# The intent is for this makefile to provide common functionality that can be +# included in a specific makefile for each lab/project that provides additional +# targets. + +# Compiler and flags. Need to be provided here as the metatest makefiles +# are not always used via recursive calls to make from the AG makefile. +export CXX ?= g++ +export CXXFLAGS ?= --std=c++17 -Wall -pedantic -g -Wno-sign-compare -Wno-comment + +# Compiler and flags. Need to be provided here as the metatest makefiles +# are not always used via recursive calls to make from the AG makefile. +# Note that we expliclty set LD here so that it doesn't default to ld rather than g++. +export LD := $(CXX) +export LDFLAGS ?= -Wall + +# Set student_test at the command line when running the validity_check +# target. If this value is not set, all the student tests will be run. Set +# student_test and bug_name at the command line when running the bug_check +# target. If student_test is not set, all the student tests will be run. +student_test = + +# Test student unit tests against correct instructor solution. This tests for +# the absence of false positives, which we sometimes call a validity check. +# Test student unit tests against buggy implementations, AKA true positives. +# The recipe will only succeed if ALL buggy implemenations are caught by at +# least one of the student unit tests. +metatest: validity_check verify_all_bugs_caught + +# This intentionally has no dependencies +get_test_names: + -@test -f $(SUITE_NAME)_validity_check.exe && ./$(SUITE_NAME)_validity_check.exe --show_test_names + +validity_check: $(SUITE_NAME)_validity_check.exe + @./$(SUITE_NAME)_validity_check.exe $(student_test) + @echo "****************************************" + @echo "VALIDITY CHECK PASS ($(SUITE_NAME)): student unit tests pass on instructor solution" + +bug_check_%: $(SUITE_NAME)_bug_check_%.exe + ./$< $(student_test) + +verify_all_bugs_caught: $(patsubst %, verify_caught_%, $(ALL_BUGS)) + @echo "****************************************" + @echo "BUG CATCHING PASS ($(SUITE_NAME)): student unit tests caught ALL instructor bugs" + +verify_caught_%: $(SUITE_NAME)_bug_check_%.exe + ! ./$< $(student_test) + +.SECONDARY: + +.PHONY: clean metatest get_test_names validity_check bug_check verify_all_bugs_caught + +# Compile one object +%.o: %.cpp + $(CXX) $(CXXFLAGS) $< -c -o $@ + +# Verify variables set by metatest-*.mk +ifndef SUITE_NAME + $(error SUITE_NAME is not set) +endif +ifndef ALL_BUGS + $(error ALL_BUGS is not set) +endif diff --git a/tutorial_buggy_impls.cpp b/tutorial_buggy_impls.cpp index 768fb9e..659e52d 100644 --- a/tutorial_buggy_impls.cpp +++ b/tutorial_buggy_impls.cpp @@ -36,47 +36,43 @@ using namespace std; // Bug Description: The implementation slides left instead of right. void slideRight(std::vector &v) { - int *arr = v.data(); - int n = static_cast(v.size()); - int first = *arr; - for(int *ptr = arr+1; ptr < arr + n; ++ptr) { - *(ptr-1) = *ptr; + if (v.empty()) { return; } + int first = v.at(0); + for (int i = 1; i < static_cast(v.size()); ++i) { + v.at(i-1) = v.at(i); } - *(arr+n-1) = first; + v.at(v.size()-1) = first; } #elif defined SLIDE_RIGHT_BUG_2 // Bug Description: The implementation doesn't copy the last element to the beginning. void slideRight(std::vector &v) { - int *arr = v.data(); - int n = static_cast(v.size()); - for (int* p = arr+n-1; arr < p; --p) - *p = *(p-1); + for (int i = static_cast(v.size()) - 1; i > 0; --i) { + v.at(i) = v.at(i-1); + } } #elif defined SLIDE_RIGHT_BUG_3 // Bug Description: The implementation fails for an empty vector void slideRight(std::vector &v) { - if (v.empty()) { v[0] = v[100000]; return; } - int *arr = v.data(); - int n = static_cast(v.size()); - int last = *(arr+n-1); - for (int* p = arr+n-1; arr < p; --p) - *p = *(p-1); - *arr = last; + int last = v.at(v.size() - 1); + for (int i = static_cast(v.size()) - 1; i > 0; --i) { + v.at(i) = v.at(i-1); + } + v.at(0) = last; } #else void slideRight(std::vector &v) { - int *arr = v.data(); - int n = static_cast(v.size()); - int last = *(arr+n-1); - for (int* p = arr+n-1; arr < p; --p) - *p = *(p-1); - *arr = last; + if (v.empty()) { return; } + int last = v.at(v.size() - 1); + for (int i = static_cast(v.size()) - 1; i > 0; --i) { + v.at(i) = v.at(i-1); + } + v.at(0) = last; } #endif @@ -89,59 +85,55 @@ void slideRight(std::vector &v) { // Bug Description: The function does nothing. void flip(std::vector &v) { - int *arr = v.data(); - int n = static_cast(v.size()); - for (int *p = arr+n-1; arr < p; ++arr, --p) { - // Accidentally swapping pointers instead of values. arr and p are + for (int front = 0, back = static_cast(v.size() - 1); front < back; + ++front, --back) { + // Accidentally swapping indices instead of values. front and back are // swapped and immediately "cross over" each other. - int *t = arr; - arr = p; - p = t; + int tmp = front; + front = back; + back = tmp; } } #elif defined FLIP_BUG_2 // Bug Description: Logical error in swapping middle elements. -// Only manifests for odd length arrays. +// Only manifests for odd length vectors. void flip(std::vector &v) { - int *arr = v.data(); int n = static_cast(v.size()); for (int i = 0; i < (n-1)/2; ++i) { - int t = arr[i]; - arr[i] = arr[n-1-i]; - arr[n-1-i] = t; + int tmp = v.at(i); + v.at(i) = v.at(n-1-i); + v.at(n-1-i) = tmp; } - // manually flip middle elements (incorrectly, for odd length arrays) - int t = arr[n/2]; - arr[n/2] = arr[n/2-1]; - arr[n/2-1] = t; + // manually flip middle elements (incorrectly, for odd length vectors) + int tmp = v.at(n/2); + v.at(n/2) = v.at(n/2-1); + v.at(n/2-1) = tmp; } #elif defined FLIP_BUG_3 -// Bug Description: Allows left and right pointers to cross over each other. -// Pointers will go outside the arrays and eventually crash. Only manifests -// for even length arrays. +// Bug Description: Allows left and right indices to cross over each other. +// Indices will go outside the vectors and eventually crash. Only manifests +// for even length vectors. void flip(std::vector &v) { - int *arr = v.data(); - int n = static_cast(v.size()); - for (int *p = arr+n-1; arr != p; ++arr, --p) { - int t = *arr; - *arr = *p; - *p = t; + for (int front = 0, back = static_cast(v.size()-1); front != back; + ++front, --back) { + int tmp = v.at(front); + v.at(front) = v.at(back); + v.at(back) = tmp; } } #else void flip(std::vector &v) { - int *arr = v.data(); - int n = static_cast(v.size()); - for (int *p = arr+n-1; arr < p; ++arr, --p) { - int t = *arr; - *arr = *p; - *p = t; + for (int front = 0, back = static_cast(v.size()-1); front < back; + ++front, --back) { + int tmp = v.at(front); + v.at(front) = v.at(back); + v.at(back) = tmp; } }