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

__builtins: replace top-level imports with imported#3803

Merged
RazvanN7 merged 1 commit intodlang:masterfrom
huglovefan:imported_builtins
Apr 27, 2022
Merged

__builtins: replace top-level imports with imported#3803
RazvanN7 merged 1 commit intodlang:masterfrom
huglovefan:imported_builtins

Conversation

@huglovefan
Copy link
Copy Markdown
Contributor

@huglovefan huglovefan commented Apr 19, 2022

this way the modules are only imported for code that needs them

this also includes a fix to make __builtin_expect use c_long since that requires adding an import, it's done using an alias to keep it lazy without having to type imported!"..." for every use (edit: will do separately)

i checked and all the affected functions still work like before

// C code
void va(int x, ...)
{ 
	va_list ap, ap2;
	__builtin_va_start(ap, x);
	int y = __builtin_va_arg(ap, int);
	__builtin_va_copy(ap2, ap);
	__builtin_va_end(ap2);
	__builtin_va_end(ap);
}
int main()
{
	float f = __builtin_fabsf(1.0f);
	double d = __builtin_fabs(1.0);
	long double ld = __builtin_fabsl(1.0L);

	long x = __builtin_expect(0, 0);

	va(0, 0);
}

this now doesn't list any core.stdc.* modules in the output of dmd -v test.c:

// C code
int main()
{
	__builtin_assume(1);
}

@dlang-bot
Copy link
Copy Markdown
Contributor

Thanks for your pull request and interest in making D better, @huglovefan! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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 + druntime#3803"

@dkorpel
Copy link
Copy Markdown
Contributor

dkorpel commented Apr 20, 2022

this way the modules are only imported for code that needs them

So is the goal to improve compile times?

this also includes a fix to make __builtin_expect use c_long

I think this PR is small enough to let it slide, but in general, please don't mix refactors and bug fixes.

Comment thread src/__builtins.di Outdated
this way the modules are only imported for code that needs them
@huglovefan
Copy link
Copy Markdown
Contributor Author

re-pushed without the c_long fix, will do that separately

So is the goal to improve compile times?

something like that (but the difference is probably small)

this is an automatically imported file so i thought it would make sense to avoid unnecessary work there

the functions using core.bitop already do this by importing the module inside template functions, this does it for the rest of the imports

this PR was 50% to see if this would be accepted (it's how i would write the file but unsure what others think), 50% to fix the c_long thing without adding a new top-level import for it (but i see core.stdc.config is already depended upon by one of the current top-level imports so adding it without this PR wouldn't be too bad)

@RazvanN7
Copy link
Copy Markdown
Contributor

RazvanN7 commented Apr 25, 2022

this way the modules are only imported for code that needs the

How? Isn't the global alias going to instantiate the template anyway?

@huglovefan
Copy link
Copy Markdown
Contributor Author

How? Isn't the global alias going to instantiate the template anyway?

it works that way in .d files being compiled (listed on dmd command line) but not modules that are just imported like __builtins

in imported modules that aren't included in compilation, the template is instantiated only if the importing module uses the alias or a template that uses it, or if the module itself uses it outside a template/alias

i don't know if this is documented somewhere but this is how i observe it works

@RazvanN7 RazvanN7 merged commit 1abbe19 into dlang:master Apr 27, 2022
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