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 eSink to library file generation #15505

Merged
merged 1 commit into from Aug 13, 2023

Conversation

WalterBright
Copy link
Member

Also:

  1. reduce dependency on target.d
  2. eliminate dependency on errors.d
  3. tabs converted to spaces in main.d

@WalterBright WalterBright added Easy Review Refactoring No semantic changes to code labels Aug 5, 2023
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

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 + dmd#15505"

@WalterBright WalterBright force-pushed the libFactory branch 5 times, most recently from 7d89013 to af7136a Compare August 6, 2023 16:31
@@ -9,7 +10,7 @@ Use a regex because the path is really strange on Azure (OMF_32, 64):

TEST_OUTPUT:
----
$r:.*$: Error: corrupt $?:windows=MS Coff|osx=Mach|ELF$ object module $?:windows=fail_compilation\extra-files\fake.lib|fail_compilation/extra-files/fake.a$ $n$
Error: corrupt ELF object module fail_compilation/extra-files/fake.a 112
Copy link
Contributor

Choose a reason for hiding this comment

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

If this PR is a refactor, why is the test changed? Did the behavior change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not understand the test case. There are no comments in it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's testing that linking with a corrupt library generates the correct message, which depends on the platform. Now there is no test for Mac or Windows anymore. Can't you leave it as it is?

Copy link
Member Author

Choose a reason for hiding this comment

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

It failed as is, saying it got the green message instead of the red one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, it's because you now pass Loc.initial to the error functions instead of this.loc. Please add the correct location back, that's useful information.

@WalterBright WalterBright force-pushed the libFactory branch 4 times, most recently from e4f8bb0 to 7c32b41 Compare August 12, 2023 22:17
@WalterBright
Copy link
Member Author

@dkorpel thanks for putting your finger on the actual problem. It's working now!

@WalterBright WalterBright merged commit 5fb9de2 into dlang:master Aug 13, 2023
44 checks passed
@WalterBright WalterBright deleted the libFactory branch August 13, 2023 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy Review Refactoring No semantic changes to code
Projects
None yet
3 participants