Fix GCC warnings about incomplete classes#1488
Conversation
|
I think I need a little bit more explanation to understand the issue. I read some explanations about SFINAE, but I did not catch how this applies e.g. to Just looking at the first header file where this was replaced: I also failed to find a template, which could be problematic. So it seems I was just looking in the wrong places. Could you explain the problem to me, probably just on that example? Or have you some more good literature for me to read? Thanks! |
The problematic template that GCC pointed me to is this one: I don't know how or why this template is called. It isn't used for all Qt derived classes. Perhaps this quote from the GCC enablement of
Honestly, I don't quite understand how this all works either. It already took me some time to figure out that the forward declarations are the issue in the first place. |
|
For completeness, these are the warnings I encountered: warning: defining ‘UBGraphicsTextItem’, which previously failed to be complete in a SFINAE context [-Wsfinae-incomplete=]Using |
|
Thanks for this. I think I have now more understanding. I analyzed the case for In In the end this is not a real problem because So in this specific case it could also be an option not to define this function as a slot, or to include If my analysis is correct, then at least it opens more ways to mitigate this problem. |
Include QGraphicsSvgItem directly instead
UBItem.h is needed for UBGraphicsItem UBGraphicsItemDelegate is used via UBGraphicsItem::Delegate UB.h is needed for UBGraphicsItemData
The two classes are unrelated to each other. The split allows more precise includes in downstream users, and better separation of concerns for development.
Using a class as a slot parameter requires it to be fully defined. Qt checks that to ensure correct behaviour. The checking is done via templates, where the SFINAE principle means that an incomplete class doesn't lead to errors. This can lead to issues when selecting the appropriate template, hence GCC warns about that. In the cases where issues can occur, this commit includes the full class definition. Thanks to @letsfindaway for the precise investigation.
Vekhir
force-pushed
the
refactor-import-sfinae
branch
from
aa15931 to
9c4ae16
Compare
|
The PR is now split into 7 smaller commits. Each commit deals with a small part of the refactoring, grouped by the kind of change for easier review. I've also removed a few changes to reduce the complexity. The last commit solves the warning by including the definition where necessary. A noted above, it's only needed where the class is used as the parameter for a slot. In all other cases, the declaration is enough as only the pointer size is needed. |
Vekhir
changed the title
2 participants
GCC 16.1 introduces a new warning for SFINAE issues. They can cause a change in behaviour when imports get reordered, leading to hard to debug issues.
Additionally, many used definitions are now included directly instead of relying on transitive includes. Relying on transitivity is brittle and can easily cause issues in lots of files if that transitive chain is broken.
Some forward declarations cause SFINAE issues, because the definition is needed to choose the correct template. This affects the classes UBGraphicsMediaItem, UBGraphicsTextItem, UBBackgroundRuling, UBToolWidget, and WebView.
To eliminate these forward declarations, the imports need to be restructured. The easiest case is replacing the forward declaration with the include of the definition.
This doesn't quite work for UBGraphicsMediaItem as it causes an include cycle:
src/domain/UBGraphicsMediaItem.h includes src/domain/UBItem.h which includes src/domain/UBGraphicsItemDelegate.h which includes src/domain/UBGraphicsMediaItem.h
The solution provided here is to turn the UBGraphicsMediaItem class into a forward declaration in src/domain/UBGraphicsItemDelegate.h
In the course of this work, UBGraphicsItem has been split from UBItem into its own file; this surfaced a lot of transitive includes that weren't properly declared.