CppCoreGuidelines icon indicating copy to clipboard operation
CppCoreGuidelines copied to clipboard

F.7 rule does not take into account the guarantees smart pointers provide.

Open ImaginaryExponent opened this issue 6 months ago • 3 comments

Per the stated guideline, "F.7: For general use, take T* or T& arguments rather than smart pointers" we should avoid accepting smart pointers as function arguments. The reasoning provided is, of course, valid, but it seems to miss an important point.

A function is a contract: "Given valid input arguments the function will do whatever it is intended to do". Specifically, the issue is in "valid" of the input arguments. When a function accepts raw pointers it has no means to distinguish between non-null valid and non-null invalid arguments.

void run_task(const workload *arg)
{
 if (nullptr == arg)
  return;
...
}

This check can clearly detect one of the invalid parameter types. But it cannot protect against a dangling pointer. A very contrived example would be

workload *wl = &workload_storage.front();
workload_storage.pop_front();
run_task(wl);

Here run_task cannot know it has been provided a deleted pointer. It gets a non-null value and crashes at runtime. But there is no other check the run_task could perform. It can only rely on the caller to perform the validations. Which is perfectly safe in many cases, but not when the function is a part of some public interface. On the other hand, if workload is already stored as, for instance, std::shared_ptr, or any other pointer-tracking type, the behavior is different:

void run_task(const std::shared_ptr<workload> &arg)
{
 if (nullptr == arg)
  return;
...
}

now the function relies on the std::shared_ptr for the guarantees. It does not affect the ownership, yes, but it actively relies on its side-effect. Now a non-null value will definitely point to a valid object in memory. The check is now necessary and sufficient for the validity of the argument.

This guarantee seems important enough to be at least mentioned as an option for the argument passing.

ImaginaryExponent avatar Aug 07 '25 10:08 ImaginaryExponent

This would need to reference different kinds of smart pointers. Some are valid to pass around without changing ownership and others are not. So the issue is more nuanced. However, I do agree that smart pointers should be valid to use as arguments since they could provide both (a) reference counting and (b) null checks, things that are extremely hard to do otherwise and even more so to do correctly.

Now, if the C library and OS interfaces provided an ability to check the validity of a random pointer than this may be moot. However, they do not do that typically and even then there so many methods of handing around a pointer (stack, heap) that it is nearly impossible to truly know the validity other than that is is not 0x0 (or 0xCCCCCCCCC... for MSVC Debug Mode).

BenjamenMeyer avatar Aug 07 '25 14:08 BenjamenMeyer

If I were to see void run_task(const std::shared_ptr<workload> &arg) in code review, I'd say "please change to void run_task(const Task& arg).

Here run_task cannot know it has been provided a deleted pointer. It gets a non-null value and crashes at runtime

The way I see it, you're addressing ES.65: Don’t dereference an invalid pointer by replacing a pointer by a RAII type that guarantees validity of the pointer during its own lifetime (good, that's the first option ES.65 presents), but shared_ptr is a bad choice for that role: it does more, both cognitively (I see a function that's taking a shared_ptr, my mental model is that it's going to move it to some indeterminately-scheduled thread pool or equivalent) and at run time (those atomics aren't free)

cubbimew avatar Aug 07 '25 15:08 cubbimew

shared_ptr is a bad choice for that role

Please-please-please, don't take this specific class literally. I was trying to refer to a broader concept and might have made a poor choice of example. Any smart pointer would do, any class that manages the pointer inside and ensures that it is either null or valid.

Of course there are performance considerations, of course this specific call might look strange. It is both a slideware and a reference to Microsoft COM pointers that get auto-generated. Some times there is a need to write a function that accepts such a pointer. Given that the project basically consists of these entities, passing them around is less unnatural than otherwise might seem.

My concern is still that the recommendation to "use raw pointers" may be read as "even if you have a pointer-managing class that would offer you more guarantees, you should ignore that and pass a raw pointer instead". Later in the guidelines a gsl::non_null is mentioned as the means to avoid null checks, so the argument is "valid pointer only". OK, so raw pointer is "null, valid non-null and invalid non-null". To that extent std::unique_ptr is the "null or valid" kind of deal. Feels wasteful to throw away the functionality that we might have available.

I would suggest updating one of the examples in F.7 with some sort of just this consideration. "if you have a smart pointer, it can offer the non-null-valid guarantee."

ImaginaryExponent avatar Aug 07 '25 15:08 ImaginaryExponent