Skip to content

Comments

Define static xxxExp::emplace member function#9269

Merged
dlang-bot merged 1 commit intodlang:masterfrom
ibuclaw:staticemplace
Feb 18, 2019
Merged

Define static xxxExp::emplace member function#9269
dlang-bot merged 1 commit intodlang:masterfrom
ibuclaw:staticemplace

Conversation

@ibuclaw
Copy link
Member

@ibuclaw ibuclaw commented Jan 15, 2019

For new()'ing expressions without allocating memory.

These should be marked @nogc, but cannot as none of the Expression AST constructors themselves are @nogc either.

@ibuclaw ibuclaw added the Compiler:GDC Gnu D Compiler label Jan 15, 2019
@ibuclaw ibuclaw requested a review from dnadlinger as a code owner January 15, 2019 19:02
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @ibuclaw!

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 fetch digger
dub run digger -- build "master + dmd#9269"

emplaceExp!(IntegerExp)(pue, loc, value, type);
}

static void emplacei(UnionExp* pue, Loc loc, int value, Type type)
Copy link
Member

Choose a reason for hiding this comment

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

What's the point of this overload? Line 1713 can handle it.

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'd send that question back to you, as I think you introduced createi, but I've yet to see any users of that overload.

I'm all for removing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #8243.

Copy link
Member Author

Choose a reason for hiding this comment

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

And infact, osx64 builds are still failing.

Undefined symbols for architecture x86_64:
  "IntegerExp::emplace(UnionExp*, Loc, unsigned long, Type*)", referenced from:
      test_emplace() in cxxfrontend.o

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can see, DMD (at least 2.084.0) mangles dinteger_t correctly, as m (on macOS 64bit). But DMD 2.079.0 mangles dinteger_t as y, which is not correct.

Copy link
Member

Choose a reason for hiding this comment

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

I'm all for removing it.

Yank it.

For new()'ing expressions without allocating memory.

These should be marked `@nogc`, but cannot as none of the Expression AST
constructors themselves are `@nogc` either.
@ibuclaw
Copy link
Member Author

ibuclaw commented Jan 27, 2019

Keeping emplacei and createi for the benefit of dmd/osx64.

Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Looks like this is good to go?

@dlang-bot dlang-bot merged commit 591f4a3 into dlang:master Feb 18, 2019
@ibuclaw ibuclaw deleted the staticemplace branch February 18, 2019 19:42
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