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

fix Issue 23606 - betterC with CTFE and gc #14791

Merged
merged 1 commit into from
Jan 10, 2023

Conversation

WalterBright
Copy link
Member

The comment says it won't work. Let's see what happens.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
23606 normal betterC with CTFE and gc

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#14791"

@WalterBright
Copy link
Member Author

The missing symbol errors all seem to be instantiations of Object.RTInfoImpl

@WalterBright
Copy link
Member Author

Boiled the problem down to this:

class C {  int a,b,c;   int* p,q; }
void test() {    C c = new C(); }

which does not put the RTInfoIml in the object file which it should.

@WalterBright
Copy link
Member Author

Ok, I abandoned the last approach for a new one. This allows GC operations to be done in betterC, but any functions that do them are not emitted to the object file. This enables them to be used for CTFE.

@WalterBright
Copy link
Member Author

The Azure failure looks unrelated.

@@ -600,6 +600,7 @@ class FuncDeclaration : public Declaration
// set if someone took the address of this function
int tookAddressOf;
bool requiresClosure; // this function needs a closure
bool skipCodegen; // do not generate code for this function
Copy link
Contributor

Choose a reason for hiding this comment

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

This is redundant with FUNCFLAG.isCompileTimeOnly

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, isCompileTimeOnly doesn't do what it says it does. That logic is a mess. So I avoided it pending a future refactoring.

if (checkOnly)
{
err = true;
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

With a nested function, you can remove the duplicated branches which aren't covered by tests.

@WalterBright WalterBright merged commit 314b20c into dlang:master Jan 10, 2023
@WalterBright WalterBright deleted the fix23606 branch January 10, 2023 03:56
@ibuclaw
Copy link
Member

ibuclaw commented Jan 10, 2023

Bug fixes should really target stable.

@RazvanN7
Copy link
Contributor

The problem with this approach is that you might have a template function that is GC and is both used at compile at and runtime. What will happen in this case: you get a link error that leaves you scratching your head.

Also, this seems to have caused a regression: https://issues.dlang.org/show_bug.cgi?id=23799

@RazvanN7
Copy link
Contributor

Actually, Issue 23799 has simply been exposed by this implementation. However, this has caused another regression:

string foo()()
{
    string a, b;
    return a ~ b;
}

enum s = foo();

extern(C) void main()
{
    foo();                                                                                                              
}

This code used to give a good error message, but now it just fails to link.

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

Successfully merging this pull request may close these issues.

6 participants