Tag: Code Review

Linus Torvalds and the Supposedly “Garbage Code”

Linus Torvalds criticized a RISC-V Linux kernel contribution from a Google engineer as “garbage code.” The discussion focuses on the helper function make_u32_from_two_u16() versus Linus’s proposed explicit code. Let’s discuss the importance of using proper type casting, bit manipulation, and creating a safer, reusable macro or function for clarity and bug reduction.

Recently, Linus Torvalds publicly dismissed a RISC-V code contribution to the Linux kernel made by a Google engineer as “garbage code”:

https://lkml.org/lkml/2025/8/9/76

First, I think Linus should be more respectful of other people.

In addition, let’s focus on the make_u32_from_two_u16() helper. My understanding is that this is a C preprocessor macro (as the Linux Kernel is mainly written in C). Let’s compare that helper with the explicit code “(a << 16) + b” proposed by Linus.

First, this explicit code is likely wrong, and in fact Linus adds that “maybe you need to add a cast”.

Why should we add a cast? In Linus’s words: “[…] to make sure that ‘b’ doesn’t have high bits that pollutes the end result”. So, what should the explicit code look like according to him? “(a << 16) + (uint16_t)b”?

But let’s do a step back. We should ask ourselves: What are the types of ‘a’ and ‘b’? From the helper’s name, I would think they are two “u16”, so two uint16_t.

If I was asked to write C code that takes two uint16_t values ‘a’ and ‘b’ as input and combines them into a uint32_t, I would write something like that:

  ((uint32_t)a << 16) | (uint32_t)b

I would use the bitwise OR (|) instead of +; I find it more appropriate as we are working at the bit manipulation level here. But maybe that’s just a matter of personal preference and coding style.

Moreover, I’d use the type casts as shown above, on both ‘a’ and ‘b’.

I’m not sure what Linus meant with ‘b’ potentially having “high bits that pollutes the end result”. Could ‘b’ be a uint32_t? In that case, I would use a bitmask like 0xFFFF with bitwise AND (&) to clear the high bits of ‘b’.

Moreover, I’d probably use better names for ‘a’ and ‘b’, too, like ‘high’ and ‘low’, to make it clear what is the high 16-bit word and what is the low 16-bit word.

So, the correct explicit code is not something as simple as “(a << 16) + b”. You may need to type cast, and you have to pay attention to do it correctly with proper use of parentheses. And you may potentially need to clear the high bits of ‘b’ with a bitmask?

And, if this operation of combining two uint16_t into a uint32_t is done in several places, you sure have many opportunities to introduce bugs with the explicit code that Linus advocates for in his email!

So, it would be much better, clearer, nicer, and safer, to raise the semantic level of the code, and write a helper function or macro to do that combination safely and correctly.

A C macro could look like this:

#include <stdint.h>

#define MAKE_U32_FROM_TWO_U16(high, low) \
        ( ((uint32_t)(high) << 16) | (uint32_t)(low) )

Should we take into consideration the case in which ‘low’ has higher bits to clear? Then the macro becomes something like this:

#define MAKE_U32_FROM_TWO_U16(high, low) \
        ( ((uint32_t)(high) << 16) | ((uint32_t)(low) & 0xFFFF))

As you can see, the type casts, the parentheses, the potential bit-masking, do require attention. But once you get the code right, you can safely and conveniently reuse it every time you need!

So, the real garbage code is actually repeatedly writing explicit bug-prone or wrong code, like “(a << 16) + b”! Not hiding such code in a sane helper macro (or function), like shown above.

Instead of a preprocessor macro, we could use an inline helper function. For example, in C++ we could write something like this:

#include <stdint.h>

inline uint32_t make_u32_from_two_u16(uint16_t high, uint16_t low) 
{
    return (static_cast<uint32_t>(high) << 16) | 
           static_cast<uint32_t>(low);
}

We could even further refine this function, marking it noexcept, as it’s guaranteed to not throw exceptions.

And we could also make the function constexpr, as it can be evaluated at compile-time when the input arguments are constant.

With these additional refinements, we get:

inline constexpr uint32_t make_u32_from_two_u16(
    uint16_t high, 
    uint16_t low)  noexcept 
{
    return (static_cast<uint32_t>(high) << 16) |
           static_cast<uint32_t>(low);
}

Code Reviewing ChatGPT’s std::map C++ code

ChatGPT does C++ std::map. OK, let’s review the code produced by this AI. Are there any errors? Can it be improved?

Recently someone sent me an interesting email about the answer they got from ChatGPT to the question they asked: “Teach me about C++ std::map“.

ChatGPT provided the following code, with some additional notes.

ChatGPT's demo code showing how to use std::map.
ChatGPT trying to explain how to use std::map

Well, I read that code and noted a few things:

Since the map instance uses std::string as the key type, the associated <string> header should have been #included (although the above code could compile thanks to “indirect” inclusion of <string>; but that’s not a good practice).

Moreover, since C++20, std::map has been given a (long-awaited…) method to check if there is an element with a key equivalent to the input key in the container. So, I would use map::contains instead of invoking the map::count method to check if a key exists.

// ChatGPT suggested:
//   
//   // Checking if a key exists
//   if (ages.count("Bob") > 0) {
//       ...
//
// Starting with C++20 you can use the much clearer
// and simpler map::contains:
//
if (ages.contains("Bob")) {
    ...

In addition, I would also improve the map iteration code provided by ChatGPT.

In fact, starting with C++17, a new feature called structured binding allows writing clearer code for iterating over std::map’s content. For example:

// ChatGPT suggested:
//
//  Iterating over the map
//  for (const auto& pair : ages) {
//      std::cout << pair.first << ": " << pair.second << std::endl;
//  }
//
// Using C++17's structure bindings you can write:
//
for (const auto& [name, age]: ages) {
    std::cout << name << ": " << age << std::endl;
}

Note how using identifiers like name and age produces code that is much more readable than pair.first and pair.second (which are in the code suggested by ChatGPT).

(As a side note of kind of lesser importance, you may want to replace std::endl with ‘\n’ in the above code; although if “output performance” is not particularly important, std::endl would be acceptable.)

What conclusions can we draw from that interesting experience?

Well, I think ChatGPT did a decent job in showing a basic usage of C++ std::map. But, still, its code is not optimal. As discussed in this simple code review, with a better knowledge of the C++ language and standard library features you can produce higher-quality code (e.g. more readable, clearer) than ChatGPT did in this instance.

…But maybe ChatGPT will read this code review, learn a new thing or two, and improve? 😉