This makes it an error to not do something with a returned smart
pointer, which should help prevent mistakes. In cases where you do need
to ignore the value, casting to void will placate the compiler.
I did have to add comments to disable clang-format on a couple of lines,
where it wanted to format the code like this:
```c++
private : NonnullRefPtr() = delete;
```
This isn't a complete conversion to ErrorOr<void>, but a good chunk.
The end goal here is to propagate buffer allocation failures to the
caller, and allow the use of TRY() with formatting functions.
While I was working on LibWeb, I got a page fault at 0xe0e0e0e4.
This indicates a destroyed RefPtr if compiled with SANITIZE_PTRS
defined. However, the page fault handler didn't print out this
indication.
This makes the page fault handler print out a note if the faulting
address looks like a recently destroyed RefPtr, OwnPtr, NonnullRefPtr,
NonnullOwnPtr, ThreadSafeRefPtr or ThreadSafeNonnullRefPtr. It will
only do this if SANITIZE_PTRS is defined, as smart pointers don't get
scrubbed without it being defined.
Some time ago, automatic locking was added to the AK smart pointers to
paper over various race conditions in the kernel. Until we've actually
solved the issues in the kernel, we're stuck with the locking.
However, we don't need to punish single-threaded userspace programs with
the high cost of locking. This patch moves the thread-safe variants of
RefPtr, NonnullRefPtr, WeakPtr and RefCounted into Kernel/Library/.
This commit moves the KResult and KResultOr objects to Kernel/API to
signify that they may now be freely used by userspace code at points
where a syscall-related error result is to be expected. It also exposes
KResult and KResultOr to the global namespace to make it nicer to use
for userspace code.
Our existing implementation did not check the element type of the other
pointer in the constructors and move assignment operators. This meant
that some operations that would require explicit casting on raw pointers
were done implicitly, such as:
- downcasting a base class to a derived class (e.g. `Kernel::Inode` =>
`Kernel::ProcFSDirectoryInode` in Kernel/ProcFS.cpp),
- casting to an unrelated type (e.g. `Promise<bool>` => `Promise<Empty>`
in LibIMAP/Client.cpp)
This, of course, allows gross violations of the type system, and makes
the need to type-check less obvious before downcasting. Luckily, while
adding the `static_ptr_cast`s, only two truly incorrect usages were
found; in the other instances, our casts just needed to be made
explicit.
And also try_create<T> => try_make_ref_counted<T>.
A global "create" was a bit much. The new name matches make<T> better,
which we've used for making single-owner objects since forever.
Aggregate initialization with brace-enclosed parameters is a
[C++20 feature][1] not yet implemented by Clang. This caused compile
errors if we tried to use the factory functions to create smart pointers
to aggregates.
As a (temporary) fix, [the LWG's previously proposed solution][2] is
implemented by this commit.
Now, wherever it's not possible to direct-initialize, aggregate
initialization is performed.
[1]:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0960r3.html
[2]: http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-active.html#2089
These functions abstract away the need to call the proper new operator
("throwing" or "non-throwing") and manually adopt the resulting raw
pointer. Modelled after the existing `NonnullOwnPtr<T> make()`
functions, these forward their parameters to the object's constructor.
Note: These can't be used in the common "factory method" idiom, as
private constructors can't be called from a standalone function.
The naming is consistent with AK's and Shell's previous implementation
of these:
- `make` creates a `NonnullOwnPtr<T>` and aborts if the allocation could
not be performed.
- `try_make` creates an `OwnPtr<T>`, which may be null if the allocation
failed.
- `create` creates a `NonnullRefPtr<T>`, and aborts on allocation
failure.
- `try_create` creates a `RefPtr<T>`, which may be null if the
allocation was not successful.
Other software might not expect these to be defined and behave
differently if they _are_ defined, e.g. scummvm which checks if
the TODO macro is defined and fails to build if it is.
Unfortunately adopt_ref requires a reference, which obviously does not
work well with when attempting to harden against allocation failure.
The adopt_ref_if_nonnull() variant will allow you to avoid using bare
pointers, while still allowing you to handle allocation failure.
Also, the PeekType of smart pointers is now T* instead of const T*.
Note: This commit doesn't compile, it breaks HashMap::get() for some
types. Fixed in the next commit.
SPDX License Identifiers are a more compact / standardized
way of representing file license information.
See: https://spdx.dev/resources/use/#identifiers
This was done with the `ambr` search and replace tool.
ambr --no-parent-ignore --key-from-file --rep-from-file key.txt rep.txt *
This commit makes the user-facing StdLibExtras templates and utilities
arguably more nice-looking by removing the need to reach into the
wrapper structs generated by them to get the value/type needed.
The C++ standard library had to invent `_v` and `_t` variants (likely
because of backwards compat), but we don't need to cater to any codebase
except our own, so might as well have good things for free. :^)
Alot of code is shared between i386/i686/x86 and x86_64
and a lot probably will be used for compatability modes.
So we start by moving the headers into one Directory.
We will probalby be able to move some cpp files aswell.
(...and ASSERT_NOT_REACHED => VERIFY_NOT_REACHED)
Since all of these checks are done in release builds as well,
let's rename them to VERIFY to prevent confusion, as everyone is
used to assertions being compiled out in release.
We can introduce a new ASSERT macro that is specifically for debug
checks, but I'm doing this wholesale conversion first since we've
accumulated thousands of these already, and it's not immediately
obvious which ones are suitable for ASSERT.
Problem:
- Many constructors are defined as `{}` rather than using the ` =
default` compiler-provided constructor.
- Some types provide an implicit conversion operator from `nullptr_t`
instead of requiring the caller to default construct. This violates
the C++ Core Guidelines suggestion to declare single-argument
constructors explicit
(https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c46-by-default-declare-single-argument-constructors-explicit).
Solution:
- Change default constructors to use the compiler-provided default
constructor.
- Remove implicit conversion operators from `nullptr_t` and change
usage to enforce type consistency without conversion.
Problem:
- `typedef` is a keyword which comes from C and carries with it old
syntax that is hard to read.
- Creating type aliases with the `using` keyword allows for easier
future maintenance because it supports template syntax.
- There is inconsistent use of `typedef` vs `using`.
Solution:
- Use `clang-tidy`'s checker called `modernize-use-using` to update
the syntax to use the newer syntax.
- Remove unused functions to make `clang-tidy` happy.
- This results in consistency within the codebase.
This makes most operations thread safe, especially so that they
can safely be used in the Kernel. This includes obtaining a strong
reference from a weak reference, which now requires an explicit
call to WeakPtr::strong_ref(). Another major change is that
Weakable::make_weak_ref() may require the explicit target type.
Previously we used reinterpret_cast in WeakPtr, assuming that it
can be properly converted. But WeakPtr does not necessarily have
the knowledge to be able to do this. Instead, we now ask the class
itself to deliver a WeakPtr to the type that we want.
Also, WeakLink is no longer specific to a target type. The reason
for this is that we want to be able to safely convert e.g. WeakPtr<T>
to WeakPtr<U>, and before this we just reinterpret_cast the internal
WeakLink<T> to WeakLink<U>, which is a bold assumption that it would
actually produce the correct code. Instead, WeakLink now operates
on just a raw pointer and we only make those constructors/operators
available if we can verify that it can be safely cast.
In order to guarantee thread safety, we now use the least significant
bit in the pointer for locking purposes. This also means that only
properly aligned pointers can be used.
If these methods get inlined, the compiler is able to statically eliminate most
of the assertions. Alas, it doesn't realize this, and believes inlining them to
be too expensive. So give it a strong hint that it's not the case.
This *decreases* the kernel binary size.
We were allowing this dangerous kind of thing:
RefPtr<Base> base;
RefPtr<Derived> derived = base;
This patch changes the {Nonnull,}RefPtr constructors so this is no
longer possible.
To downcast one of these pointers, there is now static_ptr_cast<T>:
RefPtr<Derived> derived = static_ptr_cast<Derived>(base);
Fixing this exposed a ton of cowboy-downcasts in various places,
which we're now forced to fix. :^)
This allows RefPtr to be stored in a HashTable<RefPtr<T>> :^)
It's unfortunate about the const_casts. We'll need to fix HashMap::get
to play nice with non-const Traits<T>::PeekType at some point.
As suggested by Joshua, this commit adds the 2-clause BSD license as a
comment block to the top of every source file.
For the first pass, I've just added myself for simplicity. I encourage
everyone to add themselves as copyright holders of any file they've
added or modified in some significant way. If I've added myself in
error somewhere, feel free to replace it with the appropriate copyright
holder instead.
Going forward, all new source files should include a license header.
Add the concept of a PeekType to Traits<T>. This is the type we'll
return (wrapped in an Optional) from HashMap::get().
The PeekType for OwnPtr<T> and NonnullOwnPtr<T> is const T*,
which means that HashMap::get() will return an Optional<const T*> for
maps-of-those.
Use AK::exchange() to switch out the internal storage. Also mark these
functions with [[nodiscard]] to provoke an compile-time error if they
are called without using the return value.
We shouldn't allow constructing e.g an OwnPtr from a RefPtr, and similar
conversions. Instead just delete those functions so the compiler whines
loudly if you try to use them.
This patch also deletes constructing OwnPtr from a WeakPtr, even though
that *may* be a valid thing to do, it's sufficiently weird that we can
make the client jump through some hoops if he really wants it. :^)
This patch removes copy_ref() from RefPtr and NonnullRefPtr. This means that
it's now okay to simply copy these smart pointers instead:
- RefPtr = RefPtr // Okay!
- RefPtr = NonnullRefPtr // Okay!
- NonnullRefPtr = NonnullRefPtr // Okay!
- NonnullRefPtr = RefPtr // Not okay, since RefPtr can be null.
I had a silly ambition that we would avoid unnecessary ref count churn by
forcing explicit use of "copy_ref()" wherever a copy was actually needed.
This was making RefPtr a bit clunky to work with, for no real benefit.
This patch adds the missing copy construction/assignment stuff to RefPtr.
