Skip to content

Assign outer_attrs in IfLetExpr constructor #388

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 25, 2021
Merged

Assign outer_attrs in IfLetExpr constructor #388

merged 1 commit into from
Apr 25, 2021

Conversation

CohenArthur
Copy link
Member

@CohenArthur CohenArthur commented Apr 25, 2021 •
edited
Loading

Closes #360

This fixes IfLetExpr's behavior and removes the previous unused warning as well

@CohenArthur CohenArthur force-pushed the outer-attrs-ifletexpr branch from d6205f8 to a601990 Compare April 25, 2021 12:09
@dkm
Copy link
Member

dkm commented Apr 25, 2021

Can't really say anything about the change (it looks correct from the C++ pov, but maybe the field should be removed as the comment is saying it is not allowed).
I would still suggest adding more content to the commit message ? I'm not really clear on what is the expected flow with bors as it adds a commit with the PR details.

@CohenArthur
Copy link
Member Author

Can't really say anything about the change (it looks correct from the C++ pov, but maybe the field should be removed as the comment is saying it is not allowed).
I would still suggest adding more content to the commit message ? I'm not really clear on what is the expected flow with bors as it adds a commit with the PR details.

From @philberty in #380:

IfLetExpr does have outer_attrs. The actual issue here is that the primary constructor does not initialise the outer_attrs field from the outer_attrs parameter.

My understanding is that the outer_attrs were not used because it was believed at the time that they were not allowed for IfLetExprs. I can definitely add more to the commit message, but I'd like to get some more clarification too to make sure I'm not spewing anything wrong

SimplyTheOther
SimplyTheOther approved these changes Apr 25, 2021
View reviewed changes
Copy link
Member

@SimplyTheOther SimplyTheOther left a comment

Choose a reason for hiding this comment

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

PR looks great.

@SimplyTheOther
Copy link
Member

Can't really say anything about the change (it looks correct from the C++ pov, but maybe the field should be removed as the comment is saying it is not allowed).
I would still suggest adding more content to the commit message ? I'm not really clear on what is the expected flow with bors as it adds a commit with the PR details.

From @philberty in #380:

IfLetExpr does have outer_attrs. The actual issue here is that the primary constructor does not initialise the outer_attrs field from the outer_attrs parameter.

My understanding is that the outer_attrs were not used because it was believed at the time that they were not allowed for IfLetExprs. I can definitely add more to the commit message, but I'd like to get some more clarification too to make sure I'm not spewing anything wrong

This is correct. The field was once thought to not be allowed, but that was an incorrect assumption (or has changed in a more recent version of the Rust reference).

@CohenArthur
Copy link
Member Author

Can't really say anything about the change (it looks correct from the C++ pov, but maybe the field should be removed as the comment is saying it is not allowed).
I would still suggest adding more content to the commit message ? I'm not really clear on what is the expected flow with bors as it adds a commit with the PR details.

From @philberty in #380:

IfLetExpr does have outer_attrs. The actual issue here is that the primary constructor does not initialise the outer_attrs field from the outer_attrs parameter.

My understanding is that the outer_attrs were not used because it was believed at the time that they were not allowed for IfLetExprs. I can definitely add more to the commit message, but I'd like to get some more clarification too to make sure I'm not spewing anything wrong

This is correct. The field was once thought to not be allowed, but that was an incorrect assumption (or has changed in a more recent version of the Rust reference).

Thanks for shedding light on the situation! I'll rebase and add more explanation in the commit message

@philberty
Copy link
Member

is this ready to merge @CohenArthur ?

@CohenArthur
IfLetExpr: Assign outer_attrs in constructor
8ee80ab 
The `outer_attrs` member was not used in the IfLetExpr type, as it was
believed to be forbidden. However, that assumption was false and
outer_attrs are indeed allowed here. This commit also fixes the unused
warning for the `outer_attrs` constructor's argument, which was not
assigned before.
@CohenArthur CohenArthur force-pushed the outer-attrs-ifletexpr branch from a601990 to 8ee80ab Compare April 25, 2021 15:09
@CohenArthur
Copy link
Member Author

is this ready to merge @CohenArthur ?

Yes, thanks! I've added a bit more background in the commit's message

@philberty
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 25, 2021

Build succeeded:

@bors bors bot merged commit e5aedbb into Rust-GCC:master Apr 25, 2021
@CohenArthur CohenArthur deleted the outer-attrs-ifletexpr branch April 25, 2021 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philberty philberty philberty approved these changes

@SimplyTheOther SimplyTheOther SimplyTheOther approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

IfLetExpr does have outer_attrs.
4 participants
@CohenArthur @dkm @SimplyTheOther @philberty