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

Add a builtin for checking the value of compile-time options #18117

Closed
wants to merge 1 commit into from

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Aug 18, 2016

Yet another approach for fixing the #14173 test in combination with MEMDEBUG. It's a more generic approach than adding a single purpose builtin for discovering MEMDEBUG.

This doesn't add a global (@tkelman), doesn't change the non-MEMDEBUG allocation limit (@vtjnash, I even tightened a bit, undoing 78bc312), and doesn't change how jl_gc_big_alloc counts allocations (@yuyichao).

The preprocessor magic isn't particularly clean (and only works for certain compile-time options), but being whitelist-based it shouldn't matter and we can fix or special-case other options when the need arises. Curious whether it'll work on all compilers...

Use it to fix the #14173 test in the case of MEMDEBUG.
@yuyichao
Copy link
Contributor

Why not just have a simple wrapper function like what we do for other options? (threading, llvm version, for example). This also mean that we can't change the name of the option anymore without breaking user code.

@maleadt
Copy link
Member Author

maleadt commented Aug 18, 2016

I would think a dedicated jl_is_memdebug (or similar) is as difficult to maintain, eg. when we remove/rework/split/... such internal functionality the builtin might not apply or be relevant anymore.
Just mark jl_get_option for internal use only?

@yuyichao
Copy link
Contributor

It can easily be made to work without changing any julia code and without trying to workaround the auto detection mechanism.

@maleadt
Copy link
Member Author

maleadt commented Aug 19, 2016

I don't get what you mean by that.
I just didn't want to litter the external API with functions to expose a detail such as MEMDEBUG, vs. more important information like LLVM version or threading.

@yuyichao
Copy link
Contributor

I just don't want to expose C implementation detail like the exact name of an option to julia code. jl_is_memdbg can stay as long as we have some option that increase the allocation count.

@maleadt
Copy link
Member Author

maleadt commented Aug 19, 2016

Isn't it virtually the same to have jl_is_memdbg vs jl_get_option(:MEMDEBUG)? We're exposing the option either way, and if #define MEMDEBUG were to go away when there was still an allocation-increasing option, we can can add special-case code to jl_get_option to handle :MEMDEBUG just as well as we could keep jl_is_memdbg around...

Don't get me wrong, it's not that I'm insisting to have something like jl_get_option, it just seemed a little more general in the case we'd need to expose other compile-time details. (I probably should make it return Any then)

@yuyichao
Copy link
Contributor

Isn't it virtually the same to have jl_is_memdbg vs jl_get_option(:MEMDEBUG)?

For the second one you can't rename the option or merge it with other ones.

it just seemed a little more general in the case we'd need to expose other compile-time details.

I think ccall is general enough.

@maleadt
Copy link
Member Author

maleadt commented Aug 19, 2016

For the second one you can't rename the option or merge it with other ones.

Sure you can, as I said you just special-case the :MEMDEBUG option:

jl_get_option(jl_sym_t *key)
    if (key == :MEMDEBUG)
        #ifdef MEMDEBUG2
            return jl_true
        #else
            return jl_false

    JL_HANDLE_OPTION(:OTHER)

I think ccall is general enough.

I was talking about generality from the compiler-side, only managing those options in a single location instead of exposing multiple functions.

But this is some useless bikeshedding. I guess I'll do it your way.

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