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 17382 - void main(){}pragma(msg,main()); crashes DMD #7316

Merged
merged 3 commits into from
Nov 17, 2017

Conversation

RazvanN7
Copy link
Contributor

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close Bugzilla Description
17382 void main(){}pragma(msg,main()); crashes DMD

@stefan-koch-sociomantic

NO!
That'll generate an uncompilable version.
As well as silencing real errors!
I suggest you look for a different fix.

@RazvanN7
Copy link
Contributor Author

NO!
That'll generate an uncompilable version.

Can you be more specific?

@stefan-koch-sociomantic

the generated header will have <void> in it which has no place in our langauge.
Also If the case happens which would have asserted before it will now get swallowed.

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Nov 14, 2017

You might be right, however asserts are not errors; assert(0) is used on code paths that should never be taken and the user should never be faced with a failed assert so from this point of view no error is swollen.

On the other hand, if you use pragma(msg, foo()) where foo is declared as void foo() {} the output is going to be < void > . Now, in the case of this PR, I agree that there might be cases where < void > is outputted and it shouldn't but I don't know any and it seems that the tests all pass. So, when I was asking you to be specific I was expecting an example. Thank you.

@stefan-koch-sociomantic

I cannot think of an example as of now.
As the assert would have triggered in such a case.
However that is no excuse to open us up to more subtle bugs.

Besides having the hdrgen write-out invalid D is hardly an acceptable solution.

I'd rather fix this at the point where the main-symbol is rewritten.

@MetaLang
Copy link
Member

Excuse the drive-by comment given that I know nothing about the compiler, but why does the compiler even try to const-fold main?

@andralex
Copy link
Member

It doesn't matter that it's called main, does it? @RazvanN7 please arrange that a compile-time error with a good message is issued if someone attempts to pragma msg a void expression. If anyone does want to, there are many means such as pragma(msg, (main(), "")); etc

@RazvanN7
Copy link
Contributor Author

@andralex Is this ok?

@@ -2917,6 +2917,11 @@ private extern(C++) final class DsymbolSemanticVisitor : Visitor
e = resolveProperties(sc, e);
sc = sc.endCTFE();
// pragma(msg) is allowed to contain types as well as expressions
if (e.type && e.type.ty == Tvoid)
{
error(pd.loc, "Cannot pass argument `%s` to `pragma msg` because it is `void`", e.toChars);
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing () at toChars call (style doesn't match anywhere in this module).

Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

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

"has type void" is more pedantically correct, but this is fine as is - thanks

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