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

Upgrade to C23 and adding more static check from compiler #207

Merged
merged 6 commits into from
Jul 6, 2024

Conversation

wdhongtw
Copy link
Contributor

@wdhongtw wdhongtw commented Jul 2, 2024

For motivation, see #206.

This PR contains commits for

  • Upgrade from C99 to C23
  • Turn on -Wextra and -Werror compiler options
  • Fix corresponding errors

If desired, maybe I can extract the commit that upgrade to C23 as another PR.
Since that it's a standalone change to this project.

@wdhongtw wdhongtw marked this pull request as draft July 2, 2024 01:21
@wdhongtw
Copy link
Contributor Author

wdhongtw commented Jul 2, 2024

Used features from C23

  • [[maybe_unused]]: should be ok for both GCC and Clang
  • empty initializer: not sure if this works on all distrubutions.. can rollback this change if there is a concern.
    • Clang support this since 17, but Debian seems only provide llvm 14
    • Can not find compliance page for GCC

GCC/Clang version in some Linux distributions:

  • FreeBSD (Latest): gcc 9-15, llvm 11-18
  • Arch (Latest): gcc 13-14, llvm 17-18
  • Fedora 40: gcc 14, llvm 18
  • Ubuntu 24.04: gcc 13, llvm 18
  • Debian 12.0: gcc 12, llvm 14

Clang and GCC compliance page:

@wdhongtw wdhongtw marked this pull request as ready for review July 2, 2024 01:49
Copy link

codecov bot commented Jul 2, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 60.07%. Comparing base (2ffbf00) to head (9485582).

Files Patch % Lines
src/setup/ibus-setup-chewing-window.c 20.00% 4 Missing ⚠️
src/ibus-chewing-engine.c 40.00% 3 Missing ⚠️
src/IBusChewingPreEdit.c 0.00% 1 Missing ⚠️
src/main.c 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #207      +/-   ##
==========================================
- Coverage   60.13%   60.07%   -0.07%     
==========================================
  Files          24       24              
  Lines        2757     2760       +3     
==========================================
  Hits         1658     1658              
- Misses       1099     1102       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@kanru kanru left a comment

Choose a reason for hiding this comment

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

Checks failed at

/__w/ibus-chewing/ibus-chewing/src/setup/ibus-setup-chewing-window.c: In function 'ibus_setup_chewing_window_class_init':
/__w/ibus-chewing/ibus-chewing/src/setup/ibus-setup-chewing-window.c:81:37: error: cast between incompatible function types from 'void (*)(GtkWidget *)' to 'void (*)(GtkWidget *, const char *, GVariant *)' [-Werror=cast-function-type]
   81 |                                     (GtkWidgetActionActivateFunc)show_about);
      |                                     ^
cc1: all warnings being treated as errors
gmake[2]: *** [bin/CMakeFiles/ibus-setup-chewing.dir/build.make:95: bin/CMakeFiles/ibus-setup-chewing.dir/setup/ibus-setup-chewing-window.c.o] Error 1

.git-blame-ignore-revs Outdated Show resolved Hide resolved
@wdhongtw wdhongtw marked this pull request as draft July 2, 2024 11:48
@wdhongtw
Copy link
Contributor Author

wdhongtw commented Jul 2, 2024

Checks failed at ...

This may take longer than expected. I'm still trying to reproduce this failure locally.

@kanru
Copy link
Member

kanru commented Jul 2, 2024

Checks failed at ...

This may take longer than expected. I'm still trying to reproduce this failure locally.

The signature is defined like https://docs.gtk.org/gtk4/callback.WidgetActionActivateFunc.html

@wdhongtw
Copy link
Contributor Author

wdhongtw commented Jul 2, 2024

I'm confused why I can not reproduce this error in my local environment (Fedora 40, Clang version 18.1.6)
It tooks the same as the state in CI container...

llvm-libs x86_64 18.1.6-2.fc40 updates 28 M

So it may trigger other compile error after I update the PR later. O_Q

@wdhongtw wdhongtw marked this pull request as ready for review July 2, 2024 13:09
@kanru
Copy link
Member

kanru commented Jul 2, 2024

I'm confused why I can not reproduce this error in my local environment (Fedora 40, Clang version 18.1.6)
It tooks the same as the state in CI container...

llvm-libs x86_64 18.1.6-2.fc40 updates 28 M

So it may trigger other compile error after I update the PR later. O_Q

That's why Werror can be annoying at times : 🤭

@wdhongtw
Copy link
Contributor Author

wdhongtw commented Jul 2, 2024

That's why Werror can be annoying at times : 🤭

Now I known that...


It's may looks more natural to use the original (signed-integer) type to do comparision.
And it should be safe to cast from unsigned-int to signed-int for the value ranges in these secenarios.

Maybe I should modify the PR again?

diff --git a/src/IBusChewingLookupTable.c b/src/IBusChewingLookupTable.c
index 99cec8b..0bdd552 100644
--- a/src/IBusChewingLookupTable.c
+++ b/src/IBusChewingLookupTable.c
@@ -58,8 +58,8 @@ guint ibus_chewing_lookup_table_update(
     [[maybe_unused]] IBusChewingProperties *iProperties,
     ChewingContext *context) {
     IBusText *iText = NULL;
-    guint i;
-    guint choicePerPage = (guint)chewing_cand_ChoicePerPage(context);
+    gint i;
+    gint choicePerPage = chewing_cand_ChoicePerPage(context);
     gint totalChoice = chewing_cand_TotalChoice(context);
     gint currentPage = chewing_cand_CurrentPage(context);
 
diff --git a/src/ibus-chewing-engine.c b/src/ibus-chewing-engine.c
index 7307baf..091495b 100644
--- a/src/ibus-chewing-engine.c
+++ b/src/ibus-chewing-engine.c
@@ -989,7 +989,7 @@ void ibus_chewing_engine_candidate_clicked(IBusEngine *engine, guint index,
 
     if (is_password(self))
         return;
-    if (index >= (guint)chewing_get_candPerPage(self->icPreEdit->context)) {
+    if ((gint)index >= chewing_get_candPerPage(self->icPreEdit->context)) {
         IBUS_CHEWING_LOG(DEBUG, "candidate_clicked() index out of ranged");
         return;
     }

@kanru
Copy link
Member

kanru commented Jul 2, 2024

That's why Werror can be annoying at times : 🤭

Now I known that...

It's may looks more natural to use the original (signed-integer) type to do comparision. And it should be safe to cast from unsigned-int to signed-int for the value ranges in these secenarios.

Maybe I should modify the PR again?

Sure, there are opinions that in C/C++ you rarely need to use unsigned integer and it might be dangerous. I'm not sure if the compiler has new warnings against them. It might still worth to follow the wisdom.

https://jacobegner.blogspot.com/2019/11/unsigned-integers-are-dangerous.html

CMakeLists.txt Outdated Show resolved Hide resolved
@wdhongtw wdhongtw marked this pull request as draft July 6, 2024 08:59
wdhongtw added 6 commits July 6, 2024 17:05
C23 standard provide a cleaner way to express intentions.
Change standard from C99 to C23 and modify some preset naming.
- Use empty initializer for null-terminated array, reduce macro usage
- For partial value structures, use designated initializer instead
- For setting apply callback functions, just remove variable name
- For other cases, use maybe_unused attribute to preserve readability
- Enable extra warning from compiler
- Make warning into error in CI build
@wdhongtw
Copy link
Contributor Author

wdhongtw commented Jul 6, 2024

Rebase and force pushed again.

  • Introduce an option to control whether to use -Werror
  • Add and use new preset *-release-ci to build the project with the option above

The default and *-release presets are not changed, to avoid potential troubles addressed by @yan12125 😺

@wdhongtw wdhongtw marked this pull request as ready for review July 6, 2024 10:06
@kanru
Copy link
Member

kanru commented Jul 6, 2024

LGTM

@kanru kanru merged commit bbbb36c into chewing:master Jul 6, 2024
4 of 5 checks passed
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.

3 participants