Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add bindings for QtMsgType, QMessageLogContext and qt_message_output #814

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

redstrate
Copy link
Contributor

This allows Rust and CXX-Qt applications to send messages to the Qt logger.

@redstrate redstrate self-assigned this Jan 17, 2024
crates/cxx-qt-lib/src/core/qtlogging.rs Outdated Show resolved Hide resolved
examples/cargo_without_cmake/src/main.rs Outdated Show resolved Hide resolved
@redstrate redstrate marked this pull request as draft January 17, 2024 19:10
@Montel
Copy link
Contributor

Montel commented Jan 18, 2024

Hi,
could you add info in CHANGELOG file please ?
Thanks

@redstrate redstrate marked this pull request as ready for review February 11, 2024 19:13
// https://codebrowser.dev/qt5/qtbase/src/corelib/global/qlogging.h.html#QMessageLogContext
assert_alignment_and_size(QMessageLogContext, alignof(intptr_t), sizeof(intptr_t) * 4);

static_assert(::std::is_trivially_copyable<QMessageLogContext>::value,
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it actually trivially_copyable ? types that have that don't need rust::IsRelocatable definition.

Normally you either have to check all of these, if copy is false then you need to impl Clone manually and if deconstructible is failse then you need to manually implement Drop (which tends to be the case for these types.

// in the header have the isrelocatable
rust::IsRelocatable<T> ...

// in the source
static_assert(!::std::is_trivially_copy_assignable<T>::value);
static_assert(!::std::is_trivially_copy_constructible<T>::value);
static_assert(!::std::is_trivially_destructible<T>::value);

static_assert(QTypeInfo<T>::isRelocatable);

or if the type is actually trivial then you only have the following and then derive the rest

static_assert(::std::is_trivially_copyable<T>::value);

As this type uses pointers internally it suggests it's probably the former (or is it actually trivially copyable as it's just const char* ?) but it looks like the type doesn't have any custom deconstructors or constructors so you probably don't implement Clone and Drop, fun :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the type is trivial, although you suggest to remove IsRelocatable in that case (IIUC) the CXX compilation fails with:

  cargo:warning=/home/josh/work/cxx-qt/target/debug/build/cxx-qt-lib-9ea1616c001b64fd/out/cxx-qt-gen/src/qtlogging.cxx.cpp:62:50: error: static assertion failed: type QMessageLogContext should be trivially move constructible and trivially destructible in C++ to be used as a return value of `qmessagelogcontext_default` or non-pinned mutable reference in signature of `set_line`, `set_file`, `set_function` in Rust
  cargo:warning=   62 |     ::rust::IsRelocatable<::QMessageLogContext>::value,

Copy link
Collaborator

@LeonMatthesKDAB LeonMatthesKDAB left a comment

Choose a reason for hiding this comment

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

Good first pass, unfortunately the lifetimes need sorting out.

crates/cxx-qt-lib/src/core/qtlogging.rs Show resolved Hide resolved
version: i32,
line: i32,
file: &'a *const c_char,
function: &'a *const c_char,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not the correct type!

&*const c_char is a reference to a pointer, so similar to a const c_char**.
So reading this from Rust after being assigned in C++ is undefined behavior, as C++ thinks it's a const char*.

This needs to just be a *const c_char.

To introduce the required lifetime, you can add a single PhantomData<&'a c_char> member to the struct.
I'm a bit uncertain how that interacts with #[repr(C)]. In #[repr(Rust)] the PhantomData won't change the size of the struct, but in #[repr(C)] it might add a single byte, which unfortunately wouldn't allow us to make this type trivial...
Please use static assertions to check what happens either way :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, it seems like PhantomData does not change the memory layout of the struct - it's still 32 bytes. However, how do I add a static_assert for this? It looks like all of the ones in the .cpp files are checking the size of the Qt type, not the Rust type?

@redstrate redstrate force-pushed the wip/josh/qtlogging branch 6 times, most recently from 40efe50 to 7dc5dbb Compare August 11, 2024 14:55
sizeof(intptr_t) * 4);

static_assert(::std::is_trivially_copyable<QMessageLogContext>::value,
"QMessageLogContext must be trivially copyable");
Copy link
Collaborator

Choose a reason for hiding this comment

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

so looks like the CI failure on Windows at least might be coming from this?

  qstring.cpp
  src/core/qstring.cpp(46): warning C4267: 'argument': conversion from 'size_t' to 'int', possible loss of data
  qstringlist.cpp
  qurl.cpp
  qtlogging.cpp
  src/core/qtlogging.cpp(17): error C2338: static_assert failed: 'QMessageLogContext must be trivially copyable'

I also note that qstring warning that is unrelated but interesting :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Meh I somehow missed the Q_DISABLE_COPY on the struct

This allows Rust and CXX-Qt applications to send messages to the Qt
logger.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants