conditional fields of constexpr literal structs#989
Merged
emilio merged 2 commits intomozilla:masterfrom May 20, 2025
Merged
Conversation
youknowone
changed the title
youknowone
force-pushed
the
constexpr-conditional-field
branch
from
b890eef to
ddd17f7
Compare
emilio
approved these changes
Oct 27, 2024
Collaborator
emilio
left a comment
emilio
left a comment
There was a problem hiding this comment.
Let's do this, just some nits about whitespace, but looks good.
| write!(out, "/* .{} = */ ", ordered_key); | ||
| self.write_literal(out, &lit.value); | ||
| if i + 1 != ordered_fields.len() { | ||
| write!(out, ", "); |
Collaborator
There was a problem hiding this comment.
This adds trailing white-space, please remove the space if is_constexpr?
| write!(out, "{}", export_name); | ||
| } | ||
|
|
||
| write!(out, "{{ "); |
Collaborator
There was a problem hiding this comment.
Same, this space needs to be removed.
Collaborator
|
Sorry for the lag getting to this btw :( |
youknowone
force-pushed
the
constexpr-conditional-field
branch
from
ddd17f7 to
df9f46e
Compare
Contributor
Author
|
@emilio Thank you for the review! I updated the both PRs |
emilio
force-pushed
the
constexpr-conditional-field
branch
from
df9f46e to
62c856f
Compare
Collaborator
|
Could you rebase this please? Thanks! |
youknowone
force-pushed
the
constexpr-conditional-field
branch
from
62c856f to
cea2ef2
Compare
Contributor
Author
|
rebased |
Contributor
Author
|
@emilio Could you check it again? |
youknowone
force-pushed
the
constexpr-conditional-field
branch
from
cea2ef2 to
2ed778c
Compare
Contributor
Author
|
I updated my local patch to this branch. It worked well for the project. |
Contributor
Author
|
@emilio is there any blocker? |
Contributor
Author
|
@emilio reminding |
youknowone
force-pushed
the
constexpr-conditional-field
branch
from
2ed778c to
64c80d2
Compare
Contributor
Author
|
rebased |
Contributor
Author
|
@emilio reminding when you come back |
emilio
approved these changes
May 20, 2025
Collaborator
|
Thaks for the ping! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Partially fix #955 about constexpr
#988 is included in this PR
Because constexpr and defines require very different approach about this problem, I created another PR for constexpr fix while keeping the other one for define fix.