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 8044 - Print name of enum passed a tmpl param #11841

Merged
merged 1 commit into from
Oct 15, 2020

Conversation

Geod24
Copy link
Member

@Geod24 Geod24 commented Oct 9, 2020

The fix was actually quite trivial. Looking at the code flow, I doubt this actually change any other diagnostic, but let's see what the CI says.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @Geod24!

Bugzilla references

Auto-close Bugzilla Severity Description
8044 enhancement Print names, not casted values when using enum template parameter

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#11841"

@Geod24 Geod24 force-pushed the fix-8044 branch 2 times, most recently from b89fbcb to 24585a7 Compare October 10, 2020 00:00
@Geod24
Copy link
Member Author

Geod24 commented Oct 10, 2020

I doubt this actually change any other diagnostic

It does, but for the best!

@Geod24
Copy link
Member Author

Geod24 commented Oct 10, 2020

This should also fix https://issues.dlang.org/show_bug.cgi?id=14067 (marked as a duplicate now) and so improve the documentation.

@Geod24
Copy link
Member Author

Geod24 commented Oct 10, 2020

-----------------------------------------------------------
ERROR: Newly generated header file (/home/circleci/dmd/generated/linux/release/64/frontend.h) doesn't match with the reference header file (/home/circleci/dmd/src/dmd/frontend.h)

DETAILS:

diff --git a/home/circleci/dmd/src/dmd/frontend.h b/home/circleci/dmd/generated/linux/release/64/frontend.h
index a845385d4..86c99e3aa 100644
--- a/home/circleci/dmd/src/dmd/frontend.h
+++ b/home/circleci/dmd/generated/linux/release/64/frontend.h
@@ -1703,7 +1703,7 @@ struct ParameterList
     Array<Parameter*>* parameters;
     StorageClass stc;
     VarArg varargs;
-    ParameterList(Array<Parameter*>* parameters, VarArg varargs = cast(VarArg)cast(ubyte)0u, StorageClass stc = 0);
+    ParameterList(Array<Parameter*>* parameters, VarArg varargs = VarArg.none, StorageClass stc = 0);
     size_t length();
     Parameter* opIndex(size_t i);
     ParameterList() :

===============

The header generation depended on that bug ? :|

src/dmd/frontend.h Outdated Show resolved Hide resolved
@ibuclaw
Copy link
Member

ibuclaw commented Oct 10, 2020

The header generation depended on that bug ? :|

Strictly for dmd, I don't think we require any of these automatic constructors, as initialisation is handed over to the D layer.

@UplinkCoder
Copy link
Member

If I recall correctly there are still cases in which you'll see cast(EnumType)value
and the comment out assert(0) was there for that reason, to help debug such cases.

@Geod24
Copy link
Member Author

Geod24 commented Oct 11, 2020

If I recall correctly there are still cases in which you'll see cast(EnumType)value

If you find any, please report. As can be seen from the diff in the testsuite, many, if not all of them, should disappear. I left the cast in to be sure no regression is introduced.

and the comment out assert(0) was there for that reason, to help debug such cases.

If someone want to debug such case, adding back and assert or printf is trivial. There's no need to have this assert here as it's not something frequently needed as opposed to the other debugging printf (e.g. printf in semantic routines).

@Geod24 Geod24 force-pushed the fix-8044 branch 2 times, most recently from 1134938 to 4c3ef60 Compare October 11, 2020 23:00
@MoonlightSentinel
Copy link
Contributor

Reduced test for the dtoh segfault:

enum E { m }

extern (C++) void foo(ulong = E.m) {}

Working on it.

src/dmd/dtoh.d Outdated Show resolved Hide resolved
@Geod24 Geod24 force-pushed the fix-8044 branch 2 times, most recently from 94c80bf to a81dd98 Compare October 14, 2020 18:39
@Geod24
Copy link
Member Author

Geod24 commented Oct 14, 2020

This should be good to ggo

@dlang-bot dlang-bot merged commit c32f220 into dlang:master Oct 15, 2020
@Geod24 Geod24 deleted the fix-8044 branch October 15, 2020 00:31
JohanEngelen added a commit to weka/ldc that referenced this pull request Jun 11, 2022
… lead to ABI hash changes.

Temporary revert to old behavior.
JohanEngelen added a commit to weka/ldc that referenced this pull request Jan 3, 2023
…ms. This lead to ABI hash changes."

This reverts commit 7950392.
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