Skip to content

Comments

Fix issue 18242 - Warn if object.Exception is missing#8050

Merged
dlang-bot merged 5 commits intodlang:masterfrom
LemonBoy:throw-miss
Mar 23, 2018
Merged

Fix issue 18242 - Warn if object.Exception is missing#8050
dlang-bot merged 5 commits intodlang:masterfrom
LemonBoy:throw-miss

Conversation

@LemonBoy
Copy link
Contributor

Let's not segfault if no Exception class is defined.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @LemonBoy! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
18242 blocker DMD Segmentation fault.

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

@wilzbach
Copy link
Contributor

How about adding a test like it was done in #7799 or #7786?

@JinShil
Copy link
Contributor

JinShil commented Mar 19, 2018

If Throwable is declared, but Exception isn't, one should still be able to use try-catch statements. I think the fix should be placed elsewhere where the Exception symbol/identifier is used (perhaps when searching the symbol table for Exception, or in the semantic processing of Identifier or IdentifierExp). It should behave exactly like any other undefined identifier.

@WalterBright
Copy link
Member

When creating a PR to fix a bugzilla issue, please post a link to the PR in the issue. This is so that someone browsing bug reports will know if and where a PR for it is. I did it for this one.

@jacob-carlborg
Copy link
Contributor

When creating a PR to fix a bugzilla issue, please post a link to the PR in the issue. This is so that someone browsing bug reports will know if and where a PR for it is. I did it for this one.

Isn’t there a bot that does this automatically?

@LemonBoy
Copy link
Contributor Author

I think the fix should be placed elsewhere where the Exception symbol/identifier is used

Let's try with this other approach then.

@WalterBright
Copy link
Member

Isn’t there a bot that does this automatically?

No.

@wilzbach
Copy link
Contributor

Isn’t there a bot that does this automatically?

dlang/dlang-bot#135

@JinShil
Copy link
Contributor

JinShil commented Mar 20, 2018

I just tested this and it looks good to me. It just needs a test case.

@@ -0,0 +1,15 @@
// REQUIRED_ARGS: -c
Copy link
Contributor

@JinShil JinShil Mar 20, 2018

Choose a reason for hiding this comment

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

If you add DFLAGS: (without anything following it) it will prevent all the default flags for importing the runtime, etc.. from being added implicitly to compiler invocation. Then you shouldn't need to declare the TypeInfo stuff and have to deal with all the object mismatch errors.

See #7845

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, Ignore the comment above. It appears TypeInfo is required because Throwable has been declared. This should get you passed the 32-bit tests:

module object;

class Object { }

class Throwable { }

class TypeInfo { }
class TypeInfo_Class : TypeInfo 
{ 
    version(D_LP64)
    {
        ubyte[136] _x; 
    }
    else
    {
        ubyte[68] _x;
    }
}

int _d_run_main() 
{
    try { } catch(Throwable e) { return 1; }
    return 0;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hint, I always forget about 32bit platforms.


class TypeInfo { }
class TypeInfo_Class : TypeInfo {
version(D_LP64) { ubyte[136] _x; } else { ubyte[68] _x };
Copy link
Contributor

@JinShil JinShil Mar 21, 2018

Choose a reason for hiding this comment

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

I think you need to remove the semicolon at the end of this line, and put it after the _x.

Copy link
Contributor

Choose a reason for hiding this comment

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

I modified it (though I misspelled fix in the commit message) 😢

@dlang-bot dlang-bot merged commit 2b4c626 into dlang:master Mar 23, 2018
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