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: visitor #53

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

feat: visitor #53

wants to merge 11 commits into from

Conversation

neko-para
Copy link
Contributor

No description provided.

Copy link
Owner

@MistEO MistEO left a comment

Choose a reason for hiding this comment

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

visitor 里的接口可以严谨一点参考下其他第三方库,比如 https://pypi.org/project/json-visitor/

include/parser/parser.hpp Outdated Show resolved Hide resolved
include/parser/parser.hpp Outdated Show resolved Hide resolved
private:
parsing_iter_t _cur;
parsing_iter_t _end;

parse_visitor<string_t>* _vis;
Copy link
Owner

Choose a reason for hiding this comment

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

初始化

include/parser/parser.hpp Outdated Show resolved Hide resolved
}
else {
return invalid_value<string_t>();
}
}

if constexpr (has_visitor) {
_vis->value(basic_value<string_t>(), cur_pos, _pos, _path);
Copy link
Owner

Choose a reason for hiding this comment

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

指针判空,还有后面的

Copy link
Contributor Author

Choose a reason for hiding this comment

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

就是因为配置了has_visitor的时候强制传入了visitor,所以直接不判空了,从前面的入口处理了

cur_pos = _pos;
}
else {
std::ignore = cur_pos;
Copy link
Owner

Choose a reason for hiding this comment

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

这个不加有 warning 吗?

Copy link
Owner

Choose a reason for hiding this comment

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

是不是可以不改原来的函数,而是在外面在包一层 has_visitor=true 特化的 parse_string,这些结构就都放新的里面

Copy link
Contributor Author

Choose a reason for hiding this comment

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

不太确定,不过直接if constexpr简洁一点,特化实在是有点啰嗦

{
if constexpr (has_visitor) {
_pos.offset++;
if (*_cur == '\n') {
Copy link
Owner

Choose a reason for hiding this comment

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

_cur != _end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这些移动逻辑是直接替换原来的_cur++,原来的各个地方你不是都写了判断的吗(

@@ -269,6 +341,8 @@ void parsing()
// it's a std::optional<json::value>
auto ret = json::parse(content);

auto another = json::parse(content, new my_visitor());
Copy link
Owner

Choose a reason for hiding this comment

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

sample 和文档保持同步,不放不重要的功能。这些都放 test 里。另外这个 new 泄露了

Copy link
Owner

Choose a reason for hiding this comment

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

fuzzing test 也加一下

template <typename string_t = default_string_t>
struct parse_visitor
{
using json_path = std::vector<std::variant<string_t, size_t>>;
Copy link
Owner

Choose a reason for hiding this comment

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

这个是干嘛的?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@windsgo
Copy link
Contributor

windsgo commented Mar 19, 2024

所以visitor是用来干嘛的

@neko-para
Copy link
Contributor Author

visitor 里的接口可以严谨一点参考下其他第三方库,比如 https://pypi.org/project/json-visitor/

参考的这个 https://www.npmjs.com/package/jsonc-parser

@MistEO MistEO force-pushed the master branch 2 times, most recently from 430d0a9 to 676cb37 Compare May 24, 2024 08:20
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