Can ChatGPT debug my code? Can it write better code?
Everyone’s talking about it, so why should I not add something too.
Interview question
ChatGPT is pretty amazing. I asked it an interview question, and it just plain solved it. Well, there was one real world imperfection. I asked it to fix that problem, and then it went off the rails. The more I hinted, the worse the code got.
I won’t give the details here, because I want to keep using the question / topic.
Finding bugs
So I wondered if it could find bugs. First an easy one:
Q:
What's the problem with this C++ code:
int* foo() {
auto ret = new int;
int r2 = *ret * 2;
return &r2;
}
A:
There are several problems with the code you provided:
1. The function foo is returning a pointer to a local variable r2,
which is not safe because the memory for r2 is deallocated when the
function returns. This can cause undefined behavior when you try to
access the returned pointer after the function has returned.
2. The function foo is allocating memory for an integer with new int,
but it is not deallocating that memory before the function
returns. This can lead to a memory leak, as the allocated memory
will remain allocated until the program ends.
3. The function does not have a way to handle the case where the
memory allocation with new int fails. In this case, new int will
throw an exception, which will not be caught by the function. This
can cause the program to terminate unexpectedly.
To fix these problems, you could consider the following changes:
1. Instead of returning a pointer to a local variable, you could
return a pointer to the dynamically allocated memory. This will
ensure that the returned pointer remains valid after the function
returns.
2. You should deallocate the memory that was allocated with new int
when it is no longer needed. This can be done using the delete
operator, like this: delete ret;.
3. You should catch any exceptions that might be thrown by new int and
handle them appropriately. This can be done using a try-catch
block, like this:
try {
auto ret = new int;
// ...
} catch (std::bad_alloc& e) {
// Handle the exception here
}
That’s… pretty amazing. It missed that the value is uninitialized, and I’d argue with the last point, since an overall strategy for OOM is not something for just one little function to solve, but not bad.
How about a tricky one?
Let’s take my safe integer conversion tool from a previous post.
Q:
Is there anything wrong with this C++ code:
template<typename To, typename From>
std::optional<To> cast_int(const From from)
{
// If casting from signed to unsigned then reject negative inputs.
if (std::is_signed_v<From> && std::is_unsigned_v<To> && from < 0) {
return {};
}
const To to = static_cast<To>(from);
// If casting from unsigned to signed then the result must be positive.
if (std::is_unsigned_v<From> && std::is_signed_v<To> && to < 0) {
return {};
}
// If the number fits then it'll be the same number when cast back.
if (from != static_cast<From>(to)) {
return {};
}
return to;
}
A:
There are a few potential problems with the code you provided:
1. The function uses the std::optional type to represent a value that
may or may not be present. However, std::optional is not available
until C++17, so the code will not compile with earlier versions of
C++.
2. The function uses the std::is_signed_v and std::is_unsigned_v type
traits to check the signedness and unsignedness of the From and To
template arguments. However, these type traits are not available
until C++20, so the code will not compile with earlier versions of
C++.
3. The function checks whether the value of from is negative before
casting it to an unsigned type. However, this check will not catch
all cases where the value of from is too large to fit in the target
type. For example, if From is int and To is unsigned short, the
value of from could be 65537, which is too large to fit in an
unsigned short, but it is not negative.
To fix these problems, you could consider the following changes:
1. If you need to support earlier versions of C++, you could use a
different mechanism to represent optional values. For example, you
could use a std::pair<To, bool> where the second element is a
boolean indicating whether the value is present or not.
2. If you need to support earlier versions of C++, you could use a
different mechanism to check the signedness and unsignedness of the
template arguments. For example, you could use the std::is_signed
and std::is_unsigned type traits, which are available in C++11 and
later.
3. To catch all cases where the value of from is too large to fit in
the target type, you could use the std::numeric_limits class to
check the maximum and minimum values that can be represented by the
target type. For example:
if (from > std::numeric_limits<To>::max()) {
return {};
}
if (from < std::numeric_limits<To>::min()) {
return {};
}
Again, I’m impressed. It’s wrong, but I’m impressed. The first two comments are technically correct, but they are not actually problems. Or at least I would not call them problems.
The third one, though, is wrong. It’ll fail to convert from unsigned
0 to int
.
template <typename To, typename From>
std::optional<To> cast_int(const From from)
{
if (from > std::numeric_limits<To>::max()) {
std::cerr << "Too big: " << from << "\n";
return {};
}
if (from < std::numeric_limits<To>::min()) {
std::cerr << "Too small: " << from
<< " < " << std::numeric_limits<To>::min() << "\n";
return {};
}
return static_cast<To>(from);
}
int main()
{
std::cout << cast_int<int>(unsigned(0)).value() << "\n";
}
This produces output:
Too small: 0 < -2147483648
0
Let’s follow up:
Q: The third suggestion would fail a conversion into type int, because
of integer promotion rules.
A:
You are correct that the third suggestion I provided would not catch
all cases where the value of from is too large to fit in the target
type.
Integer promotion is a form of type coercion that occurs in C++ when
an expression involving integers of small types is used in a context
where an integer of a larger type is expected. During integer
promotion, the value of the small integer is first converted to a
larger integer type, and then the operation is performed using the
larger type. This means that if from is an integer of a small type
(e.g. char, short) and To is a larger integer type (e.g. int, long
long), then the value of from will be promoted to the larger type
before the comparison with std::numeric_limits<To>::max() or
std::numeric_limits<To>::min() is performed.
To catch all cases where the value of from is too large to fit in the
target type, you can use a combination of integer promotion and the
std::numeric_limits class. Here is an example of how you can do this:
if (from > static_cast<decltype(from)>(std::numeric_limits<To>::max())) {
return {};
}
if (from < static_cast<decltype(from)>(std::numeric_limits<To>::min())) {
return {};
}
This code first converts the maximum and minimum values of the target
type to the type of from using static_cast, and then compares the
value of from to these converted values. This ensures that the
comparison is performed using the correct types, and that integer
promotion is correctly applied.
And like with my interview followups, this is where it gets off the rails.
First of all the type of from
is already available, it’s
From
. Second, casting min and max of the type To
into From
is
the root problem, and you can’t just cast and expect it to work.
I guess ChatGPT has not read my blog post. :-)
Q: You can't just cast from To to From. If the signedness doesn't
match then it may wrap at best, or undefined behavior at worst.
A:
You are correct that simply casting from To to From is not sufficient
to properly check whether the value of from is within the range of the
target type To.
If the signedness of From and To do not match, then a simple cast from
To to From can lead to undefined behavior, as you mentioned. This is
because casting a negative value to an unsigned type is undefined
behavior in C++.
To properly check whether the value of from is within the range of the
target type To, you can use a combination of integer promotion and the
std::numeric_limits class, as I suggested in my previous
response. This will ensure that the comparison is performed using the
correct types, and that integer promotion is correctly applied.
Alternatively, you can use the std::is_signed_v and std::is_unsigned_v
type traits to check the signedness and unsignedness of From and To,
and use different logic depending on the signedness of the types. For
example:
if constexpr (std::is_signed_v<From> == std::is_signed_v<To>) {
// Both types have the same signedness
if (from > std::numeric_limits<To>::max()) {
return {};
}
if (from < std::numeric_limits<To>::min()) {
return {};
}
} else if constexpr (std::is_signed_v<From>) {
// From is signed, To is unsigned
if (from < 0) {
return {};
}
if (from > static_cast<decltype(from)>(std::numeric_limits<To>::max())) {
return {};
}
} else {
// From is unsigned, To is signed
if (from > static_cast<decltype(from)>(std::numeric_limits<To>::max())) {
return {};
}
}
This code uses different logic depending on the signedness of the
types, which ensures that the correct comparisons are performed and
that integer promotion is correctly applied.
Wow, that is… almost correct. The static casts to decltypes need to be
removed, to instead rely on integer promotions. Otherwise
e.g. converting 0
in a signed 32bit int into an unsigned 64bit int
will fail. The unsigned 64bit max will be converted into a signed
32bit int, and get the bit pattern for -1.
But damn, after that I think it’s correct.
Q: The decltype casts cause a bug with that solution.
A:
You are correct that the use of decltype in the code I provided can
cause a bug when the value of from is too large to fit in the target
type To.
The problem occurs because decltype(from) is the type of from, which
may be larger than the target type To. This means that when the
maximum or minimum value of To is converted to the type of from, the
result may be larger than the maximum or minimum value of To,
respectively. This can cause the comparison to always evaluate to
false, even if the value of from is within the range of To.
To fix this problem, you can use a different mechanism to ensure that
the comparison is performed using the correct types. One option is to
use a helper function that compares the value of from to the maximum
and minimum values of To using the correct types. Here is an example
of how you could do this:
template <typename From, typename To>
bool in_range(From from) {
using common_type = std::common_type_t<From, To>;
return static_cast<common_type>(from) >= static_cast<common_type>(std::numeric_limits<To>::min()) &&
static_cast<common_type>(from) <= static_cast<common_type>(std::numeric_limits<To>::max());
}
template<typename To, typename From>
std::optional<To> cast_int(const From from)
{
// If casting from signed to unsigned then reject negative inputs.
if (std::is_signed_v<From> && std::is_unsigned_v<To> && from < 0) {
return {};
}
const To to = static_cast<To>(from);
// If casting from unsigned to signed then the result must be positive.
if (std::is_unsigned_v<From> && std::is_signed_v<To> && to < 0) {
return {};
}
// If the number fits then it'll be the same number when cast back.
if (!in_range<From, To>(from)) {
return {};
}
return to;
}
This code uses the std::common_type type trait to determine the common
type of From and To, and then uses static_cast to convert both from
and the minimum and maximum values of To to this common type. This
ensures that the comparison is performed using the correct types, and
that integer promotion is correctly applied.
Again, I’m impressed. But again, it’s wrong! common_type
for
int32_t
and uint32_t
will (just like integer promotion rules)
become uint32_t
, and the comparison will reject good input such as
this:
std::cout << cast_int<int32_t>(uint32_t(0)).value() << "\n";
Conclusion
ChatGPT is really impressive. I’m not at risk of getting replaced yet, but for code challenges I think it does a much better job than most programmers. And certainly much faster.
But if it gets it slightly wrong then it seems unable to fix it.
For job security in the future as this gets better you should probably focus on turning business value into what to solve, not perfecting implementation. Because in the future the implementation will just be telling the computer to “add the ability for customers to upload their own profile photos via the web or the API, deduplicate and store them in GCS/S3. Write a blog post about it”.
In writing this I’ve gone back and forth about if I’m in danger in the next few years. But considering the amount of time I spend in meetings and on problem anticipation as opposed to implementation, maybe I have that to lean back on?
I guess until my employer trains on our code base and our specific customer needs. Then I’m fucked.