These changes are arbitrarily divided into multiple commits to make it
easier to find potentially introduced bugs with git bisect.
This commit touches some dbg() calls which are enclosed in macros. This
should be fine because with the new constexpr stuff, we ensure that the
stuff actually compiles.
Problem:
- Access to the underlying type is not provided. This limits
metaprogramming and usage in function templates.
Solution:
- Provide public access to the underlying type.
- Add test to ensure the underlying type is accessible.
Problem:
- The implementation of `find_first_of` is coupled to the
implementation of `StringView`.
Solution:
- Decouple the implementation of `find_first_of` from the class by
using a generic `find` algorithm.
Problem:
- Raw loops are often written to validate that any values in a
container meet a predicate, but raw loops are not as expressive as
functions implementing well-named algorithms and are error-prone.
Solution:
- Implement a very generic form of `any_of`.
This API was a mostly gratuitous deviation from POSIX that gave up some
portability in exchange for avoiding the occasional strlen().
I don't think that was actually achieving anything valuable, so let's
just chill out and have the same open() API as everyone else. :^)
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:
- The implementation of `find` is coupled to the implementation of
`SinglyLinkedList`.
Solution:
- Decouple the implementation of `find` from the class by using a
generic `find` algorithm.
Problem:
- The implementation of `find` is coupled to the implementation of
`DoublyLinkedList`.
- `append` and `prepend` are implemented multiple times so that
r-value references can be moved from into the new node. This is
probably not called very often because a pr-value or x-value needs
to be used here.
Solution:
- Decouple the implementation of `find` from the class by using a
generic `find` algorithm.
- Make `append` and `prepend` be function templates so that they can
have binding references which can be forwarded.
Problem:
- The implementation of `find` is coupled to the implementation of `Vector`.
- `Vector::find` takes the predicate by value which might be expensive.
Solution:
- Decouple the implementation of `find` from `Vector` by using a
generic `find` algorithm.
- Change the name of `find` with a predicate to `find_if` so that a
binding reference can be used and the predicate can be forwarded to
avoid copies.
- Change all the `find(pred)` call sites to use `find_if`.
Problem:
- `find` is implemented inside of each container. This coupling
requires that each container needs to individually provide `find`.
Solution:
- Decouple the `find` functionality from the container. This allows
provides a `find` algorithm which can work with all
containers. Containers can still provide their own `find` in the
case where it can be optimized.
- This also allows for searching sub-ranges of a container rather than
the entire container as some of the container-specific member
functions enforced.
Note:
- @davidstone's talk from 2015 C++Now conference entitled "Functions
Want to be Free" encourages this style:
(https://www.youtube.com/watch?v=_lVlC0xzXDc), but it does come at
the cost of composability.
- A logical follow-on to this is to provide a mechanism to use a
short-hand function which automatically searches the entire
container. This could automatically use the container-provided
version if available so that functions which provide their own
optimized version get the benefit.
Problem:
- Clang ToT fails to build `AK/Tests/TestTypeTraits.cpp` because
`nullptr_t` is missing the `std` namespace qualifier.
Solution:
- Prepend the namespace qualifier.
Let's adapt this class a bit better to how it's actually being used.
Instead of having valid/invalid states and storing an error in case
it's invalid, a MappedFile is now always valid, and the factory
function that creates it will return an OSError if mapping fails.
This enable using global raw pointers rather than Singleton objects,
which solves some problems because global Singleton object could
be deleted when destructors are run.
This is useful for collecting statistics, e.g.
Atomic<unsigned, MemoryOrder::memory_order_relaxed> would allow
using operators such as ++ to use relaxed semantics throughout
without having to explicitly call fetch_add with the memory order.
Add a specialization for a void ValueType. This is useful if a generic
function wants to return a Result<T, E> where the user might not
actually care abut the T, and default it to void. In this case it
basically becomes Unexpected<E> instead of Result, but hey, it works :)
Now that we have RTTI in userspace, we can do away with all this manual
hackery and use dynamic_cast.
We keep the is<T> and downcast<T> helpers since they still provide good
readability improvements. Note that unlike dynamic_cast<T>, downcast<T>
does not fail in a recoverable way, but will assert if the object being
casted is not a T.
Compared to version 10 this fixes a bunch of formatting issues, mostly
around structs/classes with attributes like [[gnu::packed]], and
incorrect insertion of spaces in parameter types ("T &"/"T &&").
I also removed a bunch of // clang-format off/on and FIXME comments that
are no longer relevant - on the other hand it tried to destroy a couple of
neatly formatted comments, so I had to add some as well.
Since RefPtr<T> decrements the ref counter to 0 and after that starts
destructing the object, there is a window where the ref count is 0
and the weak references have not been revoked.
Also change WeakLink to be able to obtain a strong reference
concurrently and block revoking instead, which should happen a lot
less often.
Fixes a problem observed in #4621
Add requires clauses to constraints on InputStream and OutputStream
operator<< / operator>>. Make the constraint on String::number a
requires clause instead of SFINAE. Also, fix some unecessary IsSame in
Trie where specialized traits exist for the given use cases.
Use SFINAE to enforce the fact that it's supposed to only be called for
Arithmetic types, rather than counting on the linker to tell us that an
instantiation of String::number(my_arg) was not found. This also adds
String::number for floating point types as a side-effect.
Use TypeLists to add test for IsIntegral, IsFloatingPoint, IsVoid,
IsNullPointer, IsArithmetic, IsFundamental, and AddConst type traits.
More can "easily" be added once the TypeList and macro magic is squinted
at for long enough :).
Also add IndexSequence and associated helpers. The TypeList class can be
queried for what type is at a certain index, and there are two helper
functions: for_each_type, and for_each_type_zipped.
for_each_type will invoke a lambda with a TypeWrapper object for
each type in the type list. The original type can be obtained by
extracting the ::Type from the type of your generic lambda's one
argument.
for_each_type_zipped will walk two TypeLists in lockstep and pass a
TypeWrapper object for the current index in each list to a generic
lambda. The original type from the TypeList can again be extracted via
the ::Type of the generic lambda's two parameters.
Seems Rust and OpenJDK both had issues with getting accurate stack size
for the main thread with MacOS Maverick and above. Apply a variant of
their workarounds. We could probably assume 8MB in all cases just to
be safe, as the only user of AK::StackInfo right now is lib JS's heap
for determining possible pointer candidates. But, this approach should
work if userspace apps start trying to add custom guard pages, as well.
I ran into this exact but at least twenty times in Serenity alone. The
C++ Standard dictates that 'unsigned long' and 'unsigned long long' are
distinct types even though on most platforms they are usually both 64
bit integers.
Also it wasn't possible to evaluate IsIntegral<T> for types that were
not integers since it used MakeUnsigned<T> internally.
This was causing WindowServer and Taskbar to crash sometimes when the
stars aligned and we tried cutting off a string ending with "..." right
on top of an emoji. :^)
A future patch could do some MacOS specific things for
set_volatile/set_nonvolatile. For now, swap out the defined(__linux__)
branches for simple not __serenity__ branches.
Create macros for the byte swap operations one would expect to be in
endian.h or byteswap.h in AK/Endian.h. It's likely a similar/different
change will be needed for BSDs, but there's no github action for those
added to the project yet.
The coarse clocks in time.h are a linux extension that we've adopted.
MacOS and the BSDs don't have it, so we need an alias in a platform
header for Lagom builds.
It's not an error to create a Userspace<T> that points to kernel memory
as the point of Userspace<T> is not to validate the address, but rather
to choose safe overloads that do validation before any data transfer
takes place.
Fixes#4581.
clang trunk with -std=c++20 doesn't seem to properly look for an
aggregate initializer here when the type being constructed is a simple
aggregate (e.g. `struct Thing { int a; int b; };`). This template fails
to compile in a usage added 12/16/2020 in `AK/Trie.h`.
Both forms of initialization are supposed to call the
aggregate-initializers but direct-list-initialization delegating to
aggregate initializers is a new addition in c++20 that might not be
implemented yet.
clang-format seems to barf on these attributes, to make it easier to
use these attributes and have clang-format not mangle the following code
we can hide them behind a macro so clang-format doesn't have to handle it.
Problem:
- C functions with no arguments require a single `void` in the argument list.
Solution:
- Put the `void` in the argument list of functions in C header files.
Problem:
- These utility functions are only used in `AK`, but are being defined
in the top-level. This clutters the top-level.
Solution:
- Move the utility functions to `Meta/CMake/utils.cmake` and include
where needed.
- Also, move `all_the_debug_macros.cmake` into `Meta/CMake` directory
to consolidate the location of `*.cmake` script files.
Problem:
- File globbing is performed at the time of build system
generation. Any files which are not there at that time are not
included. So, when a new file is added it is not built unless the
build system is recreated.
Solution:
- Remove globbing from AK/Tests directory in favor of explicitly
listing the files.
We were casting the address to Userspace<T> without validating it first
which is no good and will trap an assertion soon after.
Let's catch this sooner with an ASSERT in the Userspace<T> constructor
and update the PT_PEEK and PT_POKE handlers to avoid it.
Fixes#4505.
Problem:
- `(void)` simply casts the expression to void. This is understood to
indicate that it is ignored, but this is really a compiler trick to
get the compiler to not generate a warning.
Solution:
- Use the `[[maybe_unused]]` attribute to indicate the value is unused.
Note:
- Functions taking a `(void)` argument list have also been changed to
`()` because this is not needed and shows up in the same grep
command.
Problem:
- Interface is too permissive. It permits iterators of different types
as long as they are comparable.
Solution:
- Require iterators be the same type.
ByteBuffer previously had a flag that determined whether it owned the
bytes inside it or not (m_owned.) Owned ByteBuffers would free() on
destruction and non-owned ones would not.
This was a huge source of confusion and made it hard to reason about
lifetimes since there were no compile-time clues about whether a buffer
was owned or non-owned.
The adopt mode was used at some point to take over ownership of a
random malloc'ed buffer, but nothing was using it so this patch removes
that as well.
I was confused by the trim() API, thinking it would mutate the span it
was called on. Mark all const functions that return a new span with
[[nodiscard]] so we can catch such mistakes.
When a process crashes, we generate a coredump file and write it in
/tmp/coredumps/.
The coredump file is an ELF file of type ET_CORE.
It contains a segment for every userspace memory region of the process,
and an additional PT_NOTE segment that contains the registers state for
each thread, and a additional data about memory regions
(e.g their name).
This is a convenience API when you just want the rest of the string
starting at some index. We already had substring_view() in the same
flavor, so this is a complement to that.
This uses the KMP algorithm to implement the search.
Also replaces the slow route of the normal memmem() with KMP, which
should be fairly faster (O(n + m) as opposed to O(n * m)) :^)
Formatter<char> internally uses Formatter<StringView> when in
Mode::Character, but that would only accept Mode::{Default,String} and
ASSERT_NOT_REACHED() otherwise, causing String::formatted("{:c}", 'a')
to crash
Fixes a regression introduced by 5c1b3ce. The commit description there
asserts that the changes allow calling will_be_destroyed and
one_ref_left, which are not required to be const qualified. The
implementation in fact does require the methods to be const qualified,
because we forgot to add the const_cast inside the decltypes :^)
When creating a StringImpl for a C string that starts with a null-byte,
we would ignore the explicitly given length and return the empty
StringImpl - presumably to check for "\0", but this leads to false
positives ("\0foo") so let's only care about the length.
This patch adds a 128-byte inline buffer that we use before switching
to using a dynamically growing ByteBuffer.
This allows us to avoid heap allocations in many cases, and totally
incidentally also speeds up @nico's favorite test, "disasm /bin/id"
more than 2x. :^)
Much like with Vector::append(), you may want to append multiple items in one
go. It's actually more important to do this for prepending, because you don't
want to copy the rest of items further each time.
first_matching returns the first item in the vector that matches
the given condition.
last_matching returns the last item in the vector that matches
the given condition.
Problem:
- Several files have missing includes. This results in complaints from
`clang-tidy`.
- `#ifdef` is followed by `#elif <value>` which evaluates to `0`.
Solution:
- Add missing includes.
- Change to `#elif defined(<value>)`.
Problem:
- It is difficult to refactor because there are no tests to bind the
functionality.
- Arguments are not forwarded correctly to the constructor.
Solution:
- Add tests.
- Change constructor to take forwarding references.
Problem:
- `is_zero()` is implemented by checking each value in the array by
hand. This is error-prone and less expressive than using an
algorithm.
Solution:
- Implement `is_zero()` in terms of `all_of`.
Problem:
- Raw loops are often written to validate that all values in a
container meet a predicate, but raw loops are not as expressive as
functions implementing well-named algorithms and are error-prone.
Solution:
- Implement a very generic form of `all_of`.
Problem:
- C-style arrays do not automatically provide bounds checking and are
less type safe overall.
- `__builtin_memcmp` is not a constant expression in the current gcc.
Solution:
- Change private m_data to be AK::Array.
- Eliminate constructor from C-style array.
- Change users of the C-style array constructor to use the default
constructor.
- Change `operator==()` to be a hand-written comparison loop and let
the optimizer figure out to use `memcmp`.
Problem:
- `MACAddress` class is not usable in a compile-time context.
- `__builtin_memcpy` is not constexpr in gcc.
Solution:
- Decorate functions with `constexpr` keyword.
- Use default constructors and destructors.
- Change `__builtin_memcpy` to a hand-written `for` loop and let the
compiler's optimizer take care of it.
- Add tests to ensure compile-time capabilities.
This fixes an OOB access when the last read/written chunk is empty (as we _just_
started on a new chunk).
Also adds a test case to TestMemoryStream.
Found via human fuzzing in the shell:
```sh
for $(cat /dev/urandom) {
clear
match $it {
?* as (x) {
echo $x
sleep 1
}
}
}
```
would assert at some point.
Personally I found this unintuitive at first, but it is in line with
strtol(), Python's int() or JavaScript's parseInt(), so I guess it makes
sense.
Fixes#4097.
Problem:
- There are no unit tests for `MACAddress` class. This makes it
difficult to refactor and ensure the same behavior.
- `m_data` private member variable is uninitialized leading to undefined
behavior of `is_zero()`.
Solution:
- Add unit tests to cover basic functionality.
- Initialize `m_data`.
Problem:
- C++20 changes the way equality operators are generated. This results
in overload ambiguity as reported by clang.
Solution:
- Remove `AK::Vector::operator!=` because it will be automatically
generated in terms of `AK::Vector::operator==`.
- Change `AK::Vector::operator==` to be a function template so that
overload resolution is not confused about `a == b` vs `b == a`.
- Add tests to ensure the behavior works.
Notes:
- There is more info available at
https://brevzin.github.io/c++/2019/07/28/comparisons-cpp20/ for
deeper discussion about overload resolution, operator rewriting, and
generated functions.
This gives the compiler enough information to optimize index validation
when using range-for to iterate over a Vector, drastically reducing the
cost of such loops.
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.
Problem:
- Building with clang is broken because of the `struct` vs `class`
mismatch between the definition and declaration.
Solution:
- Change `class` to `struct` in the forward declaration.
Problem:
- IPv4Address class cannot be used in a compile-time context.
- A union is used by initializing one of the members and reading the
non-active member. This is undefined behavior and not permitted in a
`constexpr` context.
Solution:
- Eliminate undefined behavior by changing to a simple `u32` for
storage instead of the union and performing mask/shift calculations
for obtaining the individual octets.
- Decorate functions with `constexpr` where possible. Currently string
formatting and optionals are not `constexpr`-capable so functions
using those are left out.
- Modify tests to validate functionality in a `constexpr` context in
addition to the run-time tests already being run. This ensures that
functionality is the same in both contexts.
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.
Problem:
- There is no direct unit testing of the IPv4Address functionality
which makes refactoring difficult.
Solution:
- Add unit tests to cover the current functionality of
IPv4Address. This will allow future refactorings with confidence.
We had competing inline definitions of the placement operators new.
Avoid this by having <AK/kmalloc.h> pull in <new> from the compiler
and always using their definitions instead.
I feel like there must be an elegant solution to this whole situation
with the operators, but I'm not sure what it is.
`AK::URL` will now check if the URL requires a port to be set using
`AK::URL.protocol_requires_port(protocol)`.
If the URL does not specify a port, and no default port for the URL
protocol is found with `AK::URL.default_port_for_protocol(protocol)`,
the URL is considered to be invalid.
This looks at three things:
- if the type has a typedef `AllowOwnPtr', respect that
- if not, disallow construction if both of `ref()' and `unref()' are
present.
Note that in the second case, if a type only defines `ref()' or only
defines `unref()', an OwnPtr can be created, as a RefPtr of that type
would be ill-formed.
Also marks a `Performance' to explicitly allow OwnPtrs.
This allows going back one character at a time, and then re-consume
previously consumed chars.
The code I need this for looks something like this:
ASSERT(lexer.consume_specific('\\'));
if (lexer.next_is("foo"))
...
lexer.retreat();
lexer.consume_escaped_character(); // This expects lexer.peek() == '\\'
Problem:
- `constexpr_sum` is implemented using `Array` which means the
function needs to be a function template so that the size can be
deduced.
Solution:
- Change the `Array` function argument to a `Span` since `Span` now is
`constexpr`.
Problem:
- Hash functions can be `constexpr`, but are not.
Solution:
- Change `inline` keyword to `constexpr`.
- Add `static_assert` tests to ensure the hash functions work in a
`constexpr` context.
Problem:
- The hash functions have no associated tests, so there is nothing
binding their behavior.
Solution:
- Bind the hash function behavior by adding tests.
- Use the existing behavior as "correct".
Problem:
- `constexpr` functions are decorated with the `inline` specifier
keyword. This is redundant because `constexpr` functions are
implicitly `inline`.
- [dcl.constexpr], §7.1.5/2 in the C++11 standard): "constexpr
functions and constexpr constructors are implicitly inline (7.1.2)".
Solution:
- Remove the redundant `inline` keyword.
Problem:
- `Checked` is not `constexpr`-aware.
Solution:
- Decorate member functions with `constexpr` keyword.
- Add tests to ensure the functionality where possible.
Problem:
- Compiler-generated functions are being defined which results in
extra code to maintain.
Solution:
- Switch to compiler-generated default functions for default
construction, copy assignment, move assignment, copy construction
and move construction.
Problem:
- There is no test which guarantees the CircularQueue does not
construct any objects of the value type. The goal is to have
uninitialized memory which can be used.
Solution:
- Add a test requiring that the constructor of the value type is never
called.
This commit also removes a few functions like raw_out and vwarn. If we
want to write raw output, we can do this as follows:
out("{}", "Hello, World!");
The vout stuff isn't really public API anyways, so no need for another
vwarn.
Problem:
- It is not possible to perform a binary search at compile-time
because `binary_search` is not `constexpr`-aware.
Solution:
- Add `constexpr` support.
This implements the transmit time suggestion in (abandoned?)
draft-ietf-ntp-data-minimization. (The other suggestions were already
implemented as far as I can tell.)
Problem:
- `Span` is not `constexpr` aware.
Solution:
- Add `constexpr` support for all parts that do not require
`reinterpret_cast`.
- Modify tests which use the `constexpr` functions.
Double the capacity when used+deleted buckets crosses 60% of capacity.
This appears to be a sweet spot for performance based on some ad-hoc
testing with test-js. :^)
Instead of each hash bucket being a SinglyLinkedList, switch to using
closed hashing (open addressing). Buckets are chained together via
double hashing (hashing the hash until we find an unused bucket.)
This greatly reduces malloc traffic, since each added element no longer
allocates a new linked list node.
Appears performance neutral on test-js. Can definitely be tuned and
could use proper management of load factor, etc.
There is no portable way to forward declare abort because the libc
implementations disagree on the signature.
Originally, I added a __portable_abort function with a "portable"
signature which just called abort. But I really don't like it and just
including <stdlib.h> is simpler.
Note that the headers we include in <AK/TestSuite.h> are no longer
commutative now, we have to include <stdlib.h> before anything else.
Problem:
- Output of decode and encode grow as the decode and encode
happen. This is inefficient because a large size will require many
reallocations.
- `const` qualifiers are missing on variables which are not intended
to change.
Solution:
- Since the size of the decoded or encoded message is known prior to
starting, calculate the size and set the output to that size
immediately. All appends will not incur the reallocation overhead.
- Add `const` qualifiers to show intent.
* AK: Add formatter for JsonValue.
* Inspector: Use new format functions.
* Profiler: Use new format functions.
* UserspaceEmulator: Use new format functions.
Problem:
- The Base64 alphabet and lookup table are initialized at
run-time. This results in an initial start-up cost as well as a
boolean evaluation and branch every time the function is called.
Solution:
- Provide `constexpr` functions which initialize the alphabet and
lookup table at compile-time. These can be called and assigned to a
`constexpr` variable so that there is no run-time cost associated
with the initialization or lookup.
We are adding the process name as prefix and a newline as suffix to any
message written to debug. Thus, the following doesn't make any sense:
for (u8 byte : bytes)
dbg("{:02x} ", byte);
dbgln();
Which function call would put the prefix? This doesn't make any sense,
thus these functions must go.
The example above could be converted to:
StringBuilder builder;
for (u8 byte : bytes)
builder.appendff("{:02x} ", byte);
dbgln("{}", builder.build());
Problem:
- Constructors and conversion operators are not `constexpr`,
but they can be.
- `constexpr` is needed here so that other classes can add `constexpr`
evaluation.
Solution:
- Add the `constexpr` keyword to the constructors and
conversion operators.
- Add `static_assert` tests which ensure the capability works.
Problem:
- m_data is being passed to the constructor of the parent class before
it is initialized. This is not really a problem because the compiler
knows the location and it is only a span being constructed, but it
triggers a warning in clang for use-before-init.
Solution:
- Initialize using a default constructed array and then overwrite it
inside the constructor after the member is initialized.
Formatter is specialized in the header file. The definition in the
implementation file is extraneous and has no effect. Simply removing
it so that there is no confusion.
String literals are just pointers to a constant character. It should be
possible to format them as such. (The default is to print them as
strings still.)
