SL-19647: Eliminate LLSDArray entirely.#204
Merged
nat-goodspeed merged 2 commits intomainfrom May 17, 2023
Merged
Conversation
It seems newer compilers have a different interpretation of exactly when to engage LLSDArray's copy constructor. In particular, this assignment: some_LLSD_map[key] = LLSDArray(...)(...)...; used to convert the LLSDArray object directly to LLSD; now it first calls the custom copy constructor, which embeds the intended array within an outer array before assigning it into the containing map. The newer llsd::array() function avoids that problem because what it returns is already an LLSD object. Taking inventory of LLSDArray assignments of that form turned up a number of workarounds like LLSD(LLSDArray(...)). Replacing those with llsd::array() is both simpler and more readable. Tip of the hat to Chorazinallen for surfacing this issue! (cherry picked from commit bb71815)
Newer C++ compilers have different semantics around LLSDArray's special copy constructor, which was essential to proper LLSD nesting. In short, we can no longer trust LLSDArray to behave correctly. Now that we have variadic functions, get rid of LLSDArray and replace every reference with llsd::array().
simon-linden
approved these changes
May 4, 2023
Contributor
simon-linden
left a comment
simon-linden
left a comment
There was a problem hiding this comment.
Have used these changes in the puppetry branch and it seems good. It's definitely nice to clean up the old calls
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
No description provided.