-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
GH-37567: [C++] Migrate JSON Integration code to Result<> #37573
Conversation
dev/archery/archery/cli.py
Outdated
@@ -764,6 +764,10 @@ def integration(with_all=False, random_seed=12345, **args): | |||
enabled_languages += 1 | |||
|
|||
if gen_path: | |||
# XXX this option is only used by the JS test suite. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe put up a follow-up?
|
||
private: | ||
IntegrationJsonReader(MemoryPool* pool, const std::shared_ptr<Buffer>& data); | ||
IntegrationJsonReader(MemoryPool* pool, std::shared_ptr<Buffer> data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, but food for future thought: a pattern which maintains access control for the constructor but enables usage of factories like make_unique
is the private tag:
IntegrationJsonReader(MemoryPool* pool, std::shared_ptr<Buffer> data); | |
private: | |
class Tag { | |
Tag() = default; | |
friend class IntegrationJsonReader; | |
}; | |
public: | |
IntegrationJsonReader(Tag, MemoryPool* pool, std::shared_ptr<Buffer> data); |
... probably not worth the boilerplate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha. Yes, I suppose it might be nice for heavily-used public classes, but that's a lot of boilerplate for a small convenience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for cleaning this up!
CI failure is unrelated. |
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit d5cc9d9. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
…he#37573) ### Rationale for this change The JSON Integration APIs still use the old-style of returning `Status` together with a `T*` out-parameter. While those are internal APIs, it would still improve ease of us to migrate them to the newer idiom of returning a `Result<T>`. ### What changes are included in this PR? Migrate the relevant APIs from `Status` to `Result`. Since the APIs are internal, no deprecation period is introduced. Also, some very minor style and testing cleanups here and there. ### Are these changes tested? By existing tests. ### Are there any user-facing changes? No. * Closes: apache#37567 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
…he#37573) ### Rationale for this change The JSON Integration APIs still use the old-style of returning `Status` together with a `T*` out-parameter. While those are internal APIs, it would still improve ease of us to migrate them to the newer idiom of returning a `Result<T>`. ### What changes are included in this PR? Migrate the relevant APIs from `Status` to `Result`. Since the APIs are internal, no deprecation period is introduced. Also, some very minor style and testing cleanups here and there. ### Are these changes tested? By existing tests. ### Are there any user-facing changes? No. * Closes: apache#37567 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
Rationale for this change
The JSON Integration APIs still use the old-style of returning
Status
together with aT*
out-parameter.While those are internal APIs, it would still improve ease of us to migrate them to the newer idiom of returning a
Result<T>
.What changes are included in this PR?
Migrate the relevant APIs from
Status
toResult
. Since the APIs are internal, no deprecation period is introduced.Also, some very minor style and testing cleanups here and there.
Are these changes tested?
By existing tests.
Are there any user-facing changes?
No.