Repository navigation
Conversation
|
FYI there is this ticket regarding G_DEFINE_TYPE: https://trac.cppcheck.net/ticket/10890 |
I've never seen warnings like these when running cppcheck on the Xfce codebase (which makes extensive use of G_DEFINE_TYPE). I'm inclined to believe that the problem stems from the code in question, which is using this macro incorrectly. |
|
Can you add test code using (some of) the new macros in gtk.c? |
c7260cd to
c855854
Compare
Added: G_DEFINE_ENUM_TYPE G_DEFINE_ENUM_VALUE G_DEFINE_EXTENDED_ERROR G_DEFINE_FLAGS_TYPE G_DEFINE_POINTER_TYPE G_DEFINE_POINTER_TYPE_WITH_CODE
c855854 to
977dcde
Compare
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found no correctness problems. The reordering keeps every existing G_DEFINE_* define unchanged: I compared the sorted name lists of base and PR, and the only difference is the six new defines. The updated test/cfg/gtk.c passes, and it does exercise the new G_DEFINE_ENUM_TYPE, G_DEFINE_FLAGS_TYPE and G_DEFINE_ENUM_VALUE defines: with the base gtk.cfg it fails with unknownMacro.
There are two minor points, neither a blocker:
G_DEFINE_EXTENDED_ERROR,G_DEFINE_POINTER_TYPEandG_DEFINE_POINTER_TYPE_WITH_CODEhave no test code in gtk.c.- Because
G_DEFINE_EXTENDED_ERRORexpands to nothing, the user's<error_type>_private_init/_copy/_clearfunctions getunusedFunctionwith--enable=all.G_DEFINE_TYPEalready does the same thing to*_class_init/*_init, so this matches the existing approach.
Testing
- Built
cppcheckfrompr/. - Ran the gtk.c cfg test with the options from
test/cfg/runtests.sh(--check-library --platform=unix64 --enable=style,information --inconclusive --force --check-level=exhaustive --inline-suppr --debug-warnings --suppress=checkersReport --library=gtk test/cfg/gtk.c): no output, exit code 0. There is no pkg-config or GTK in the container, so the compiler syntax check of gtk.c was skipped. - Ran the same check on the PR's gtk.c with
--library=../base/cfg/gtk.cfg. It fails withgtk.c:684: unknownMacro ... G_DEFINE_ENUM_TYPE, so the new test covers the change. - Compared the sorted
G_DEFINE_*names in base and PR gtk.cfg: the only difference is the six added defines, so none was lost in the reorder. - Wrote a scratch file that uses
G_DEFINE_EXTENDED_ERROR(MyError, my_error)with staticmy_error_private_init/copy/clearfunctions, plusG_DEFINE_POINTER_TYPEandG_DEFINE_POINTER_TYPE_WITH_CODE. It parses with no syntax or unknownMacro errors. With--enable=all, the three private functions getunusedFunction. AG_DEFINE_TYPEfile withw_class_init/w_initgives the same warning for those two functions, so this behaviour was already there before the PR.
Automated review of 977dcde by Claude. The pull request was built and tested locally.
| <define name="G_DEFINE_DYNAMIC_TYPE_EXTENDED(TN, t_n, T_P, _f_, _C_)" value=""/> | ||
| <define name="G_DEFINE_ENUM_TYPE(TypeName, type_name, ...)" value=""/> | ||
| <define name="G_DEFINE_ENUM_VALUE(EnumValue, EnumNick)" value=""/> | ||
| <define name="G_DEFINE_EXTENDED_ERROR(ErrorType, error_type)" value=""/> |
There was a problem hiding this comment.
AI review
Minor: the real G_DEFINE_EXTENDED_ERROR calls the user-written <error_type>_private_init, _private_copy and _private_clear functions. Because this define expands to nothing, --enable=unusedFunction reports all three as never used. G_DEFINE_TYPE already has the same problem with *_class_init/*_init, so this is consistent and not a blocker. It might still be worth a line in gtk.c, together with the G_DEFINE_POINTER_TYPE* defines, which also have no test yet.
Added:
G_DEFINE_ENUM_TYPE
G_DEFINE_ENUM_VALUE
G_DEFINE_EXTENDED_ERROR
G_DEFINE_FLAGS_TYPE
G_DEFINE_POINTER_TYPE
G_DEFINE_POINTER_TYPE_WITH_CODE