Skip to content

[libsolutil] Add new JSON helper functions. #13579

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

Merged
merged 2 commits into from
Nov 2, 2022
Merged

[libsolutil] Add new JSON helper functions. #13579

merged 2 commits into from
Nov 2, 2022

Conversation

aarlt
Copy link
Member

@aarlt aarlt commented Sep 28, 2022

Add new helper functions to libsolutil/JSON.h to make #12834 more readable.

cameel
cameel previously requested changes Oct 6, 2022
@axic
Copy link
Member

axic commented Oct 21, 2022

I wish #11967 would be merged before getting these helpers in, as it may remove the need for them.

@cameel
Copy link
Member

cameel commented Oct 21, 2022

#11967 is still a draft though, isn't it?

But anyway, @aarlt Maybe you could help reviewing it?

@axic
Copy link
Member

axic commented Oct 21, 2022

#11967 is still a draft though, isn't it?

It was working a bunch of times, but nobody reviewed it in time and rebasing causes a bunch of work.

@cameel
Copy link
Member

cameel commented Oct 21, 2022

If you're sure it will be mergeable, we could commit to reviewing it this time. We're focusing on killing off stale PRs now so yours would be a prime target :)

@axic
Copy link
Member

axic commented Oct 22, 2022

Need to rebase it again and finish off new failures, I can try looking at it on Monday.

@aarlt
Copy link
Member Author

aarlt commented Oct 25, 2022

I did a brief look on #11967. I was trying to fix some tests there - from my point of view it still needs some work to fix all tests. I think it is much easier to just merge this PR.

@cameel
Copy link
Member

cameel commented Oct 26, 2022

I'm fine with merging this if #11967 is not ready. We can always just remove these helpers in that other PR if it makes them obsolete. I just want to have these merged so that we can get to the important part, i.e. the asm import.

@cameel
Copy link
Member

cameel commented Oct 26, 2022

Needs rebase. gp2 is fixed on develop.

@ekpyron
Copy link
Member

ekpyron commented Nov 1, 2022

Just pushed a commit to de-macro the helpers, since macros hurt my eye :-). There'd be other ways to do that, but this is probably good enough.

Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

Fine with me, but since I added the de-macro commit, another review may be nice.

Copy link
Collaborator

@nikola-matic nikola-matic left a comment

Choose a reason for hiding this comment

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

Personally, I'm good with this, and templates are always nicer than macros. Please take a look at this comment thread just in case, and if you agree, make the necessary changes. Otherwise, merge away.

@ekpyron ekpyron dismissed cameel’s stale review November 2, 2022 09:23

Only thing left is naming of get, which should be fine.

@ekpyron ekpyron merged commit 0c168b1 into develop Nov 2, 2022
@ekpyron ekpyron deleted the refactor-json branch November 2, 2022 09:25
@axic
Copy link
Member

axic commented Nov 3, 2022

I did a brief look on #11967. I was trying to fix some tests there - from my point of view it still needs some work to fix all tests. I think it is much easier to just merge this PR.

Of course it is easier to merge this, but also makes it harder to ever complete the other one 😓

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants