Skip to content

Fix Issue 20074 - header file generation doesn't include attributes with CallExp#10217

Merged
dlang-bot merged 1 commit intodlang:masterfrom
RazvanN7:Issue_20074
Aug 7, 2019
Merged

Fix Issue 20074 - header file generation doesn't include attributes with CallExp#10217
dlang-bot merged 1 commit intodlang:masterfrom
RazvanN7:Issue_20074

Conversation

@RazvanN7
Copy link
Contributor

I don't know why the constructor body is being outputted. Can anyone shed some light on this?

@RazvanN7 RazvanN7 requested a review from WalterBright as a code owner July 24, 2019 15:50
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @RazvanN7! 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 coverage diff by visiting the details link of the codecov check)
  • 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
20074 normal header file generation doesn't include attributes with CallExp

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#10217"

buf.writeByte(' ');
buf.writestring(str);
}
tf.attributesApply(&printAttribute);
Copy link
Member

Choose a reason for hiding this comment

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

The comment three lines above the patch says that omitting the attributes is deliberate. It actually can be a bit annoying in the error message if the problem is not related to inferred attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but on the other hand it may lead to (falsely) uncompilable code. Consider:

class C
{
    this() @safe
    {
        () @trusted
        {
            int a; int*p = &a;                                                                                                                
        }();
    }   
}

Without this patch, the compiler will omit @trusted and generate uncompilable code from perfectly fine code.

The comment baffled me also, but I think that this is the correct way to go.

Copy link
Member

Choose a reason for hiding this comment

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

You should adjust or remove the comment in that case. A compromise might be to emit explicitly specified attributes, but not inferred ones. But the required info might not exist and not worth the complications to add.

@rainers
Copy link
Member

rainers commented Jul 24, 2019

I don't know why the constructor body is being outputted. Can anyone shed some light on this?

That's because a ctor has no return type and is falsely considered auto here;

if (!tf.next || f.storage_class & STC.auto_)

@RazvanN7
Copy link
Contributor Author

That's because a ctor has no return type and is falsely considered auto here;

So this is also a bug.

@rainers
Copy link
Member

rainers commented Jul 25, 2019

So this is also a bug.

Yes, and if it is fixed the test case in this PR might get removed. So I suggest adding a test where the function literal is used by some declaration, e.g. as a template argument.

@RazvanN7
Copy link
Contributor Author

@rainers I changed the test case so that the literal is inside a templated function's body and also I deleted the comment. I think this is good to go now.

Copy link
Member

@rainers rainers left a comment

Choose a reason for hiding this comment

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

Thanks

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Aug 6, 2019

Maybe it's time we get this in?

@PetarKirov
Copy link
Member

Thanks for the reminder ;)

@dlang-bot dlang-bot merged commit 6a221a8 into dlang:master Aug 7, 2019
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.

4 participants