Skip to content

Conversation

@julianbrost
Copy link
Contributor

Just noticed some small room for improvement while looking over another diff in that file:

The fallback implementation was added for GCC 4.x as that didn't yet implement std::is_trivially_copyable. However, by now we're using C++17 as our language standard and that wasn't even implemented in GCC 4.x yet1:

Some C++17 features are available since GCC 5, but support was experimental and the ABI of C++17 features was not stable until GCC 9.

Hence, this became more or less dead code and can be removed.

While at it, I also made use of std::conditional_t and std::is_trivially_copyable_v available in newer C++ versions, doing the same but more compactly (if you look at their documentation, they are defined as just what we did before).

Footnotes

  1. https://gcc.gnu.org/projects/cxx-status.html#cxx17

@julianbrost julianbrost added the core/quality Improve code, libraries, algorithms, inline docs label Sep 5, 2025
@cla-bot cla-bot bot added the cla/signed label Sep 5, 2025
The fallback implementation was added for GCC 4.x as that didn't yet implement
std::is_trivially_copyable. However, by now we're using C++17 as our language
standard and that wasn't even implemented in GCC 4.x yet[^1]:

    Some C++17 features are available since GCC 5, but support was experimental
    and the ABI of C++17 features was not stable until GCC 9.

Hence, this became more or less dead code and can be removed.

[^1]: https://gcc.gnu.org/projects/cxx-status.html#cxx17
std::conditional_t was added in C++14, is_trivially_copyable_v in C++17, both
do the same as the previous implementation and are a bit more compact.
@julianbrost julianbrost force-pushed the remove-obsolete-gcc-workaround branch from 8df52ed to d372ecc Compare October 16, 2025 14:57
Copy link
Contributor

@jschmidt-icinga jschmidt-icinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might not actually matter since we don't currently use AtomicOrLocked<T> with types like that, but std::atomic actually puts more constraints on its use than just std::is_trivially_copyable, it also wants the type to be copy/move constructable/assignable and not be a reference (see here).

From what I can see, Locked<T> only implements value semantics, so unlike std::atomic it could live with a deleted move-constructor and assignment operator. So maybe the conditional should additionally reflect that and allow for trivially constructible types with deleted move constructors.

(Also it seems that counter-intuitively, unlike std::atomic<T> and our own Atomic<T>, Locked does not support value-construction, which would have made AtomicOrLocked<T> usable with references, but that would be an easy fix if we ever needed it.)

@julianbrost
Copy link
Contributor Author

My idea with this PR was to only remove dead code, not change any functionality.

This might not actually matter since we don't currently use AtomicOrLocked<T> with types like that, but std::atomic actually puts more constraints on its use than just std::is_trivially_copyable, it also wants the type to be copy/move constructable/assignable and not be a reference (see here).

The main use-case for AtomicOrLocked is to automatically pick the best choice allowing safe concurrent access to class members generated from *.ti files:

m_Header << "\tAtomicOrLocked<" << field.Type.GetRealType() << "> m_" << field.GetFriendlyName() << ";" << std::endl;

So if possible, it should pick std::atomic and being trivially copyable is only an approximation for that. However, the good thing here is that (according to cppreference which you've linked), the programm is ill-formed otherwise (instead of being undefined behavior), so we should notice if this was ever used with a type that doesn't fit that pattern, we should notice it.

In the end, this boils down to the question: is that just good enough (it seems to be so far) or do we want the more complex condition (which will be technically more correct but also more code).

@julianbrost julianbrost merged commit 82cd88f into master Oct 17, 2025
24 checks passed
@julianbrost julianbrost deleted the remove-obsolete-gcc-workaround branch October 17, 2025 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla/signed core/quality Improve code, libraries, algorithms, inline docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants