Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

Conversation

WalterBright
Copy link
Member

Added function body for backwards compatibility.

It should never have been @safe.

@alexrp
Copy link
Contributor

alexrp commented Jan 18, 2013

It should never have been @safe.

Why not? As far as I can see, it did nothing memory-unsafe or non-portable? I mean, sure, it involves reinterpreting a pointer as an integer, but since we assume everywhere in the language that size_t.sizeof == (void*).sizeof, what's the problem?

int bt(in size_t* p, size_t bitnum) pure;
@system
{
int bt(in size_t* p, size_t bitnum) pure
Copy link
Contributor

Choose a reason for hiding this comment

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

unusual lack of indentation

@andralex
Copy link
Member

Interesting. I'd say forging pointers is unsafe, but peeking at their bits - knock yourself out. @WalterBright, let's make it back @safe. Thanks!

@WalterBright
Copy link
Member Author

So, *(pointer + random_integer_value) is @safe? I don't agree.

I might add that array[random_integer_value] is range checked.

@WalterBright
Copy link
Member Author

BTW, this was caught because the compiler no longer allows *(p + random_integer_value) in @safe code.

@WalterBright
Copy link
Member Author

@alexrp bt() does not reinterpret a pointer as an integer. Look at its implementation.

@andralex
Copy link
Member

You're right, I misinterpreted the semantics without reading the code.

@alexrp
Copy link
Contributor

alexrp commented Jan 18, 2013

To be fair, that documentation is pretty bad... can we improve it so that it actually describes what happens?

@WalterBright
Copy link
Member Author

BTW, the reason I'm doing this is to simplify user code (I've run into various codes that don't call bt(), but do the equivalent), simplify the interface to the code generator (no need for the code generator to deal with bt() or for the front end to recognize b() as builtin), and enable bt() to work in CTFE.

It's the same reason rol() and ror() are no longer builtins.

@WalterBright
Copy link
Member Author

I need this so dmd pull #1510 will work.

@WalterBright
Copy link
Member Author

dlang/dmd#1510

@andralex
Copy link
Member

Cool. One question would be how much of a deviation from the standard pattern is accepted...

@WalterBright
Copy link
Member Author

In the diffs for 1510 of dmd, you can see the patterns that are accepted. It's a superset of the bt() prototype.

WalterBright added a commit that referenced this pull request Jan 19, 2013
bt() is no longer a builtin
@WalterBright WalterBright merged commit e47a00b into dlang:master Jan 19, 2013
@ghost
Copy link

ghost commented Jan 19, 2013

@WalterBright: @alexrp left two notes which you haven't responded to. Generally I think we need an "OK" before a pull can be merged, and we have a rule that the person who makes a pull request should never be the one to merge the request.

It seems you're only using pull requests for the autotester and not for reviews.

@MartinNowak
Copy link
Member

This causes a performance regression for me because the function call is not inlined. I suggest we turn this back into an intrinsic but keep the pattern recognition in the compiler so both generate optimal code.

Also the negated pattern is not detected.

bool bt(in uint[] ary, size_t bitnum)
{
    return !!(ary[bitnum >> 5] & 1 << (bitnum & 31)); // uses bt
}

bool neg_bt(in uint[] ary, size_t bitnum)
{
    return !(ary[bitnum >> 5] & 1 << (bitnum & 31)); // does not use bt
}

@braddr
Copy link
Member

braddr commented Jul 25, 2013

Please file a new bug report, if you haven't already, referencing this pull request.

@MartinNowak
Copy link
Member

Issue 10714 and Issue 10715

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants