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

use Base.@show for the Compiler.jl standard library #56616

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

Conversation

aviatesk
Copy link
Member

Since Base.@show is much useful than Base.Compiler.@show.

Since `Base.@show` is much useful than `Base.Compiler.@show`.
@aviatesk aviatesk requested a review from Keno November 20, 2024 10:40
if false
import Base: Base, @show
else
if !isdefined(Base, :end_base_include)
Copy link
Member

Choose a reason for hiding this comment

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

isdefined(Base, :var"@show")?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, we’re using isdefined(Base, :end_base_include) for bootstrapping/stdlib switching, so I thought this approach would be more consistent.
Also, I think what we’re essentially branching on here is not the definedness of Base.@show, but whether the entire show system in Base is available, so I feel like using isdefined(Base, :end_base_include) might be the better approach?

Copy link
Member

Choose a reason for hiding this comment

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

I guess we do need to know whether the show machinery is available in the world age that the compiler runs, though I do think they are largely equivalent. There is a definite reason to use end_base_include in the include code in particular, because that's the point at which the semantics of include change. I guess the other alternative would be to not have a Compiler-private version of @show at all, but instead have a bootstrap definition in Base that we erase later. That way, the compiler doesn't need special logic, it just picks up whatever version of @show happens to be in Base at loading time. I think I like that version best because it conceivably make the Compiler loadable on a stripped-down version of Base, but of course we aren't doing any of that right now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean something like this?

diff --git a/Compiler/src/Compiler.jl b/Compiler/src/Compiler.jl
index aedda16573..3308bcf442 100644
--- a/Compiler/src/Compiler.jl
+++ b/Compiler/src/Compiler.jl
@@ -66,7 +66,7 @@ using Base: Ordering, vect, EffectsOverride, BitVector, @_gc_preserve_begin, @_g
     get_nospecializeinfer_sig, tls_world_age, uniontype_layout, kwerr,
     moduleroot, is_file_tracked, decode_effects_override, lookup_binding_partition,
     is_some_imported, binding_kind, is_some_guard, is_some_const_binding, partition_restriction,
-    BINDING_KIND_GLOBAL, structdiff
+    BINDING_KIND_GLOBAL, structdiff, @show
 using Base.Order
 import Base: getindex, setindex!, length, iterate, push!, isempty, first, convert, ==,
     copy, popfirst!, in, haskey, resize!, copy!, append!, last, get!, size,
@@ -137,20 +137,6 @@ if length(ARGS) > 2 && ARGS[2] === "--buildsettings"
 end
 end
 
-if !isdefined(Base, :end_base_include)
-    macro show(ex...)
-        blk = Expr(:block)
-        for s in ex
-            push!(blk.args, :(println(stdout, $(QuoteNode(s)), " = ",
-                                              begin local value = $(esc(s)) end)))
-        end
-        isempty(ex) || push!(blk.args, :value)
-        blk
-    end
-else
-    using Base: @show
-end
-
 include("cicache.jl")
 include("methodtable.jl")
 include("effects.jl")
diff --git a/base/Base_compiler.jl b/base/Base_compiler.jl
index b2633c25ee..5a1199e539 100644
--- a/base/Base_compiler.jl
+++ b/base/Base_compiler.jl
@@ -145,14 +145,21 @@ eval(m::Module, x) = Core.eval(m, x)
 include("exports.jl")
 include("public.jl")
 
-if false
-    # simple print definitions for debugging. enable these if something
-    # goes wrong during bootstrap before printing code is available.
-    # otherwise, they just just eventually get (noisily) overwritten later
-    global show, print, println
-    show(io::IO, x) = Core.show(io, x)
-    print(io::IO, a...) = Core.print(io, a...)
-    println(io::IO, x...) = Core.println(io, x...)
+# simple print definitions for debugging. enable these if something
+# goes wrong during bootstrap before printing code is available.
+# otherwise, they just eventually get (noisily) overwritten later
+global show, print, println
+show(io::IO, x) = Core.show(io, x)
+print(io::IO, a...) = Core.print(io, a...)
+println(io::IO, x...) = Core.println(io, x...)
+macro show(ex...)
+    blk = Expr(:block)
+    for s in ex
+        push!(blk.args, :(println(stdout, $(QuoteNode(s)), " = ",
+                                          begin local value = $(esc(s)) end)))
+    end
+    isempty(ex) || push!(blk.args, :value)
+    blk
 end
 
 """

As you’ve already mentioned in your comment, the noisy warnings are a bit concerning.

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.

2 participants