Skip to content

Conversation

@JohanEngelen
Copy link
Member

Implements #1376 .

I did not know how to best implement a UDA syntax @weak without using a struct _weak and alias weak = _weak(). I thought @weak() would be ugly, but I have no strong opinion on this. Please advise on naming :)

@JohanEngelen JohanEngelen changed the title Add the @ldc.attributes.weak linkage attributes for global symbols. Add the @ldc.attributes.weak linkage attribute for global symbols. Mar 22, 2016
@JohanEngelen JohanEngelen force-pushed the weaklink branch 2 times, most recently from d426ece to b91eeab Compare March 25, 2016 11:08
@JohanEngelen
Copy link
Member Author

I am using this in Weka's LDC compiler to speed up a unittest build testcase by ~12%.

edit: marking druntime functions as @weak and enabling https://github.com/D-Programming-Language/dmd/blob/master/src/root/rmem.d#L166-L193

# subdirectories contain auxiliary inputs for various tests in their parent
# directories.
config.excludes = []
config.excludes = ['Input']
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be Inputs for consistency with LLVM (especially since the comment says so)? If we deviate from that, I would suggest going with a lower-case name to match the D conventions (input).

@dnadlinger
Copy link
Member

What you could do is to just have a struct weak {} – UDAs aren't required to be values.

@JohanEngelen
Copy link
Member Author

What you could do is to just have a struct weak {} – UDAs aren't required to be values.

I think I tried that, but then the logic has to be changed I think. (it's no longer a StructExpression)
Let me know what you think is best.

@dnadlinger
Copy link
Member

Either way works out fine. One might be a bit prettier, but the other is easier to implement. Your choice. ;)

@JohanEngelen
Copy link
Member Author

Well... one is already implemented.... ;)

return true;
}

bool isFromLdcAttibutes(const StructLiteralExp *e) {
Copy link
Member

Choose a reason for hiding this comment

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

Where is the new overload used?

Copy link
Member Author

Choose a reason for hiding this comment

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

At the moment, it's not used anywhere but here. It was a refactoring to allow checking of other types of ldc.attributes. I kept it for future use convenience / easier reading of code.

Copy link
Member

Choose a reason for hiding this comment

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

My critique would be that isFromLdcAttributes should really be isLdcAttributes for the ModuleDeclaration overload then, since the module is not "from" itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, changing it.

@JohanEngelen
Copy link
Member Author

Perhaps "for consistency" @weak() with parens is not so bad?

@dnadlinger
Copy link
Member

I prefer just having @weak, to be honest, regardless of what the implementation is.

@JohanEngelen
Copy link
Member Author

I've made struct _weak private, to leak less implementation detail.

@JohanEngelen
Copy link
Member Author

The druntime change = ldc-developers/druntime@ldc-ltsmaster...weaklink

@dnadlinger
Copy link
Member

Feel free to merge once this passes.

@JohanEngelen
Copy link
Member Author

@klickverbot Thanks for your comments.

@JohanEngelen
Copy link
Member Author

(recommitted to correctly trigger CI)

@JohanEngelen JohanEngelen merged commit cb9f577 into ldc-developers:ltsmaster Mar 28, 2016
@JohanEngelen JohanEngelen deleted the weaklink branch March 28, 2016 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants