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 23148, 21723: Generate invariant id on the basis of location rather than counter. #14176

Merged
merged 1 commit into from
May 30, 2022

Conversation

FeepingCreature
Copy link
Contributor

@FeepingCreature FeepingCreature commented May 30, 2022

This makes it more resilient to template order issues.
Thanks Basile B. for the proposal!

Let's see what breaks...

@FeepingCreature FeepingCreature requested a review from RazvanN7 as a code owner May 30, 2022 09:30
@dlang-bot
Copy link
Contributor

dlang-bot commented May 30, 2022

Thanks for your pull request and interest in making D better, @FeepingCreature! 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

Auto-close Bugzilla Severity Description
21723 normal Linker error: two module static library, main compiled inline, invariant that defines a function, type alias, and an alias lambda
23148 regression Missing invariant symbol with static library when template function declares struct with destructor and invariant that instantiates template with lambda, also main has a lambda

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

@thewilsonator
Copy link
Contributor

Please include the test case from the bug report, removing the use of the standard library.

@FeepingCreature
Copy link
Contributor Author

Will add the testcase, soon as I figure out what's going on with the failures.

@FeepingCreature FeepingCreature force-pushed the fix/23148_invariant_lin_col branch 2 times, most recently from 2c6d9e6 to 384d5a2 Compare May 30, 2022 10:33
@FeepingCreature
Copy link
Contributor Author

FeepingCreature commented May 30, 2022

Should be good now, tests pass locally. Test case added.

Note: There is nothing using the standard library in the bug report. It's reduced from std.sumtype, that's why it looks like that, but it's freestanding.

@FeepingCreature
Copy link
Contributor Author

Since this also fixes 21723, I'll add a testcase for that too.

…n rather than counter.

This makes __invariant more resilient to template order issues.
@FeepingCreature FeepingCreature force-pushed the fix/23148_invariant_lin_col branch from 384d5a2 to 4e01c6a Compare May 30, 2022 10:41
@thewilsonator thewilsonator changed the title Fix issue 23148: Generate invariant id on the basis of location rather than counter. Fix issue 23148, 21723: Generate invariant id on the basis of location rather than counter. May 30, 2022
@dlang-bot dlang-bot merged commit 5c41c29 into dlang:stable May 30, 2022
@FeepingCreature FeepingCreature deleted the fix/23148_invariant_lin_col branch May 30, 2022 11:59
@dkorpel
Copy link
Contributor

dkorpel commented Jun 2, 2022

Are locations unique though? I thought this wasn't done because in static foreach you can have multiple definitions with the same location.

@FeepingCreature
Copy link
Contributor Author

Ah shit, I noticed templates but I forgot about static foreach. Hm.

@FeepingCreature
Copy link
Contributor Author

FeepingCreature commented Jun 2, 2022

Hm, I'm actually having a hard time constructing a test case of a static foreach with two struct or class definitions with different names (so they don't collide), but the same mangling and the same type location. Note that if they have different type mangling, you're disambiguated on that basis.

If it's nested in a function, struct, class or template, even if it has the same name, it's mangled with that type and can disambiguate on that basis.

edit: Hang on, isn't that impossible? If the structs have the same mangling, every function in them should collide, not just invariant.

The only reason we need the line/column number, or counter, is that we have structs with multiple invariants. We could just count from the start of the struct and get the same effect.

So maybe static foreach in the struct would collide?

@FeepingCreature
Copy link
Contributor Author

FeepingCreature commented Jun 3, 2022

Oh wait, looking at Identifier.d, it has another counter that's site specific. So we're good.

Ie. with invariant in a foreach loop, you get _L5_C3_1, _L5_C3_2, etc.

Man DMD really likes global variables though.

@FeepingCreature
Copy link
Contributor Author

FeepingCreature commented Jun 3, 2022

Ah, but that global counter causes another undefined-reference bug that I'll probably have to dustmite.

The fun never stops with global variables!!! 🤮

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.

5 participants