Skip to content
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

[feat][dynamic reflection] small edit #290

Merged
merged 4 commits into from
Jul 5, 2024

Conversation

171930433
Copy link
Contributor

  1. add const to_* but to_pb
  2. using auto const& in range for

2. using auto const& in range for
@171930433
Copy link
Contributor Author

@qicosmos

@qicosmos
Copy link
Owner

qicosmos commented Jul 2, 2024

ok, please resolve the conflicts

@171930433
Copy link
Contributor Author

ok, please resolve the conflicts

done

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 78.94737% with 4 lines in your changes missing coverage. Please review.

Project coverage is 49.09%. Comparing base (1d774ee) to head (36f04f2).
Report is 14 commits behind head on master.

Files Patch % Lines
iguana/reflection.hpp 0.00% 4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #290      +/-   ##
==========================================
+ Coverage   48.19%   49.09%   +0.89%     
==========================================
  Files          53       53              
  Lines        6241     6363     +122     
==========================================
+ Hits         3008     3124     +116     
- Misses       3233     3239       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -564,15 +564,16 @@ struct field_info {
struct base {
virtual void to_pb(std::string &str) {}
Copy link
Owner

Choose a reason for hiding this comment

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

这个为啥没有加const?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to_pb 的实现较为复杂,直接改成const会有大量的编译错误,所以这次没做修改

std::string str;
p1.to_json(str);

// p1.to_pb(str); // compile failed
Copy link
Owner

Choose a reason for hiding this comment

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

to_pb为什么会编译失败?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

原来的to_*都是非const的函数,所以任意的T const a; a.to_*都会编译失败,所以写了这个测试用例

@qicosmos
Copy link
Owner

qicosmos commented Jul 3, 2024

我晚点看一下。

@171930433
Copy link
Contributor Author

我晚点看一下。

image

@qicosmos qicosmos self-requested a review July 5, 2024 00:47
@qicosmos
Copy link
Owner

qicosmos commented Jul 5, 2024

你把cache_zize变量前加一个mutable,然后就能用const了。

Copy link
Owner

@qicosmos qicosmos left a comment

Choose a reason for hiding this comment

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

LGTM

@qicosmos qicosmos merged commit 3795450 into qicosmos:master Jul 5, 2024
20 checks passed
@171930433 171930433 deleted the gsk/add_const branch July 5, 2024 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants