-
-
Notifications
You must be signed in to change notification settings - Fork 741
Issue 10875 - Introduce LinkageType enum for safer string comparisons. #1587
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
Conversation
LGTM |
C = "C", /// | ||
Windows = "Windows", /// | ||
Pascal = "Pascal", /// | ||
Cpp = "C++" /// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to mark all these with ditto
. If you don't, you'll generate an empty paragraph for each entry, rather than a paragraph that holds everything.
EG compare:
http://dlang.org/phobos/std_traits.html#.FunctionAttribute
http://dlang.org/phobos/std_traits.html#.Variadic
I think the template "SetFunctionAttributes" would also need some migrating. |
The tests for this function use |
…rison of function linkage types.
It was mostly the line of code enum linkages = ["D", "C", "Windows", "Pascal", "C++", "System"]; I was thinking about. But I'm not sure it's actually related. |
C = "C", /// ditto | ||
Windows = "Windows", /// ditto | ||
Pascal = "Pascal", /// ditto | ||
Cpp = "C++" /// ditto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strange, I thought I had left a message about this before, but I can't find it anymore. Maybe someone deleted it? There's no un-delete in git-hub, AFAIK?
Anyways, this mostly looks good to me, and I would have pulled this earlier, but I do have a reservation: It's the:
Cpp = "C++",
What troubles me is that:
void main()
{
auto a = LinkageType.Cpp;
assert(a == "C++");
writeln(a);
}
Prints
Cpp
In particular, I could see this potentially breaking code generating code, that does things such as
mixin(format("extern (%s) foo()", functionLinkage!foo));
Do you think this might be a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://d.puremagic.com/issues/show_bug.cgi?id=11571
I've been wanting to file this for a while, you just gave me the excuse :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I think we can close the pull and the enhancement, it's almost guaranteed that functionLinkage
is mostly used during code-generation, and this pull would break code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for comment deletions, for my own comments I use a <del>text</del>
section rather than delete them.
http://d.puremagic.com/issues/show_bug.cgi?id=10875
Note that this doesn't break existing code even though the actual type was changed. Well, it /shouldn't/, but if there are any reports of a regression we can turn it back into a string. In that case it will still work with the new
LinkageType
since these string<=>enum conversions are implicit when the template is called.