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

switch effect analysis to snake case #54315

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

Conversation

LilithHafner
Copy link
Member

This PR was formed by running a bunch of global find-and-replaces

find . -name *.jl -exec sed -i 's/inaccessiblememonly/inaccessible_mem_only/g' {} \;

And manually updating the public @assume_effects macro to continue accepting the old symbols as arguments.

From discussion on Zulip (https://julialang.zulipchat.com/#narrow/stream/137791-general/topic/About.2Ejl/near/436262421)

This seems worth doing in the light of the inconsistency between inaccessiblememonly and effect_free and the fact that effect analysis is slowly becoming public.

Very open to adjusting the scope of this PR/how much we rename. I leaned pretty far into making all the effect analysis snake case in this first draft to see what folks think and to avoid having to make followups.

@non-Jedi
Copy link
Contributor

non-Jedi commented Apr 30, 2024

Particularly of note (for motivating this change) from the linked zulip discussion is the fact that somebody read inaccessiblememonly as in_accessible_mem_only and thus reversed the meaning.

@oscardssmith oscardssmith added the compiler:effects effect analysis label Apr 30, 2024
#=:noub=#true,
#=:no_task_state=#true,
#=:inaccessible_mem_only=#true,
#=:no_ub=#true,
#=:noub_if_noinbounds=#false))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should :noub_if_noinbounds be changed to no_ub_if_no_inbounds?

@Keno
Copy link
Member

Keno commented Apr 30, 2024

Not a fan of separating nothrow and noub. They are one word in my mind, but then again, I am German. effect_free is probably just badly named in general, because @assume_effects implies a much broader notion of the word effect. Perhaps unobservable would be better. inaccessiblememonly was named because that's the LLVM name for this, but we could perhaps do better.

@mikmoore
Copy link
Contributor

mikmoore commented May 1, 2024

Maybe nonaccessiblememonly or nonaccessible_mem_only? But inaccessible_mem_only also seems fine and is more grammatically conventional (I think).

@tecosaur
Copy link
Contributor

tecosaur commented Aug 4, 2024

It seems as though this PR has been a bit forgotten about?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:effects effect analysis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants