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

Allow multiple message arguments for static assert #14611

Merged
merged 14 commits into from
Dec 19, 2022

Conversation

ntrel
Copy link
Contributor

@ntrel ntrel commented Nov 2, 2022

Reboot of #11757 by @thewilsonator.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @ntrel! 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
17378 enhancement Allow multiple arguments for assert and static assert

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

compiler/src/dmd/astbase.d Outdated Show resolved Hide resolved
@tgehr
Copy link
Contributor

tgehr commented Nov 2, 2022

I guess one thing that is a bit weird is that this does not expand sequences:

alias Seq(T...)=T;
static assert(false, Seq!("123", "456"));

Idk if it's desirable for pragma(msg, ...), but for static assert it may be confusing. In either case, probably good to add an explicit test case for this to document the intention.

@ntrel
Copy link
Contributor Author

ntrel commented Nov 2, 2022

@tgehr I just tried calling expandTuples(sa.msg); but it doesn't change the array. It has no effect for:

static assert(false, Seq!("123", 456));

@thewilsonator thewilsonator added Review:Needs Changelog A changelog entry needs to be added to /changelog Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org Severity:New Language Feature labels Nov 3, 2022
@thewilsonator
Copy link
Contributor

I guess one thing that is a bit weird is that this does not expand sequences:

Yeah, Ideally this should behave the same way as pragma(msg, bunch, "of",argu+ments)

@ntrel ntrel marked this pull request as ready for review November 5, 2022 17:59
@ntrel ntrel requested a review from ibuclaw as a code owner November 5, 2022 17:59
@ntrel
Copy link
Contributor Author

ntrel commented Nov 10, 2022

@thewilsonator

I guess one thing that is a bit weird is that this does not expand sequences:

Yeah, Ideally this should behave the same way as pragma(msg, bunch, "of",argu+ments)

This does behave the same:

alias Seq(T...) = T;
pragma(msg, Seq!(int, 0)); // tuple((int), 0)

@ntrel
Copy link
Contributor Author

ntrel commented Nov 10, 2022

@tgehr I just tried calling expandTuples(sa.msg); but it doesn't change the array. It has no effect for:

static assert(false, Seq!("123", 456));

I found out that's because the Seq instantiation gets parsed as a ScopeExp, not a TupleExp (or TypeExp) which expandTuples expects.

compiler/src/dmd/hdrgen.d Outdated Show resolved Hide resolved
@@ -442,12 +442,19 @@ struct ASTBase
extern (C++) final class StaticAssert : Dsymbol
{
Expression exp;
Expression msg;
Expressions* msg;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe call it msgs?

Copy link
Contributor

Choose a reason for hiding this comment

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

If this requires many changes it could be left for a subsequent PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RazvanN7 now done and removed style changes. Also there is a spec PR.

@RazvanN7 RazvanN7 removed the Review:Needs Changelog A changelog entry needs to be added to /changelog label Nov 15, 2022
@RazvanN7
Copy link
Contributor

RazvanN7 commented Nov 30, 2022

Looks good to me. cc @WalterBright.

Also, mind the spec PR.

@RazvanN7 RazvanN7 removed the Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org label Dec 19, 2022
@RazvanN7
Copy link
Contributor

@ntrel I assume @WalterBright is busy with other things but I don't think he would oppose this addition. I'm going to go ahead and merge this.

@dlang-bot dlang-bot merged commit 8cc2707 into dlang:master Dec 19, 2022
@ntrel ntrel deleted the static-assert branch December 19, 2022 14:15
@thewilsonator
Copy link
Contributor

Thanks for getting this in. Should we add a changelog entry for this?

@ntrel
Copy link
Contributor Author

ntrel commented Dec 19, 2022

@thewilsonator Thanks for making it originally ;-) The changelog was included:
https://dlang.org/changelog/pending.html#dmd.static-assert

RazvanN7 pushed a commit to WalterBright/dmd that referenced this pull request Dec 21, 2022
Allow multiple message arguments for static assert

Signed-off-by: Razvan Nitu <razvan.nitu1305@gmail.com>
Merged-on-behalf-of: Razvan Nitu <razvan.nitu1305@gmail.com>
RazvanN7 pushed a commit to WalterBright/dmd that referenced this pull request Dec 21, 2022
Allow multiple message arguments for static assert

Signed-off-by: Razvan Nitu <razvan.nitu1305@gmail.com>
Merged-on-behalf-of: Razvan Nitu <razvan.nitu1305@gmail.com>
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.

5 participants