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

Don't use volatile #2907

Merged
merged 1 commit into from Oct 18, 2021
Merged

Don't use volatile #2907

merged 1 commit into from Oct 18, 2021

Conversation

rootkea
Copy link
Contributor

@rootkea rootkea commented Sep 26, 2021

Hello!

This fixes following clang warning:

stash.c:92:6: warning: passing 'typeof (*(&type)) *' (aka 'volatile unsigned long *') to parameter of type 'gsize *' (aka 'unsigned long *') discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
        if (g_once_init_enter(&type))
            ^~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/glib-2.0/glib/gthread.h:260:7: note: expanded from macro 'g_once_init_enter'
    (!g_atomic_pointer_get (location) &&                             \
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/glib-2.0/glib/gatomic.h:117:38: note: expanded from macro 'g_atomic_pointer_get'
    __atomic_load (gapg_temp_atomic, &gapg_temp_newval, __ATOMIC_SEQ_CST); \
                                     ^~~~~~~~~~~~~~~~~
1 warning generated.

Thanks!

@elextr
Copy link
Member

elextr commented Sep 26, 2021

You need to find what version of glib the volatile was removed, if it was waaaaay in the past ok, otherwise it might need a guard.

@rootkea
Copy link
Contributor Author

rootkea commented Sep 27, 2021

Glib docs say:
"While location has a volatile qualifier, this is a historical artifact and the pointer passed to it should not be volatile."

Here's glib maintainer asking not to use volatile: https://discourse.gnome.org/t/should-the-g-once-init-enter-argument-be-marked-as-volatile/6320/3 (The link from his comment has moved to http://c.isvolatileusefulwiththreads.com/)

PS - FWIW, on latest main g_once_init_enter still has arg qualified by volatile qualifier right after asking not to use volatile.

@elextr
Copy link
Member

elextr commented Sep 27, 2021

@rootkea I'm not asking if the volatile is needed with current Glib, I know volatile is useless between threads.

As you said the volatile is still in the function definition in recentish Glib versions (2.64 from Ubuntu LTS for eg) but its gone from the docs for Glib 2.70, don't know about the code.

What I'm asking is will removing it on our variable make compilers complain the opposite way because the volatile is still on the function prototype? I know in C++ its UB to pass a non-volatile to a volatile, but C may be ok.

Edit: actually it should be ok to pass a variable of less restrictive type to a parameter of more restrictive type.

@rootkea
Copy link
Contributor Author

rootkea commented Sep 27, 2021

Ah, as I said - on latest main branch g_once_init_enter still has arg qualified by volatile qualifier right after asking not to use volatile.

The volatile qualifier has been there on the function prototype since the very beginning of g_once_init_enter (as per git blame 2007) and has never been removed. (but now I'm puzzled why this warning then? Maybe I'm missing something)

And I don't see a warning on this PR with clang version 12.0.0-3ubuntu1~21.04.2.

@elextr
Copy link
Member

elextr commented Sep 27, 2021

Well I was more worried about gcc 7 than clang 12 but I see Travis is using gcc 7.5 so probably ok.

Ah, as I said - on latest main branch g_once_init_enter still has arg qualified by volatile qualifier right after asking not to use volatile.

Ahh well, who knows what they are thinking, but anyway as my edit said it should be fine to pass a non-volatile to a volatile. And as I noted below, its a macro, not a real function, so parameter types are not checked.

(but now I'm puzzled why this warning then? Maybe I'm missing something)

Well, all the "functions" are actually macros (hisss booo) according to the error messages in the OP, until the _atomic load() which is the first "real" function, so the type from the user code gets passed through to that function which does not have "volatile" on its prototype.

@kugel- kugel- merged commit 951bbaa into geany:master Oct 18, 2021
@kugel- kugel- self-requested a review October 18, 2021 21:56
@b4n b4n added this to the 1.39/2.0 milestone Feb 20, 2023
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.

None yet

4 participants