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

Add pretty print for DOM #2026

Merged
merged 1 commit into from Jul 17, 2023
Merged

Conversation

Cuda-Chen
Copy link
Contributor

@Cuda-Chen Cuda-Chen commented Jul 3, 2023

Add pretty print for DOM with documentation.
Currently, the indentation is fixed to four spaces.

Close #1329.

@Cuda-Chen Cuda-Chen marked this pull request as ready for review July 3, 2023 07:17
@Cuda-Chen Cuda-Chen changed the title [WIP] Add pretty print for DOM Add pretty print for DOM Jul 3, 2023
@lemire
Copy link
Member

lemire commented Jul 3, 2023

Lovely!

@Cuda-Chen
Copy link
Contributor Author

Cuda-Chen commented Jul 4, 2023

Hi @lemire ,

Thanks for the reviews. As working on this PR, I was wondering about the adjustable indentation setting, i.e., some kind of pretty_formatter(3) to set the indentation to be three spaces. I tried with the following implementation but met compilation error:

// include/simdjson/dom/serialization.h

class pretty_formatter : public mini_formatter {
public:
    pretty_formatter(int n): mini_formatter(), indent_step(n) {} // a constructor for setting indentation

private:
    int indent_step = 4;
};

template <class T, class fmt = simdjson::internal::pretty_formatter>
std::string prettify(T x)  { // implement prettify with adjustable indentation setting
    simdjson::internal::string_builder<fmt> sb;
    sb.append(x);
    std::string_view answer = sb.str();
    return std::string(answer.data(), answer.size());
}


// prettify_test.cpp
pretty_formatter fmt(3); // this constructor is not included in this PR yet
simdjson::dom::element doc; // assume doc is loaded with parse JSON
auto serial = simdjson::prettify<fmt>(doc); // <-- this results in compilation error

For the adjustable indentation settings, I would like to ask whether we should implement it in this PR or not?

@lemire
Copy link
Member

lemire commented Jul 4, 2023

I would like to ask whether we should implement it in this PR or not?

It is fine to do multiple PRs. I don't mind reviewing 10 PRs a week from you if needed.

<-- this results in compilation error

What is the compilation error?

Copy link
Member

@jkeiser jkeiser left a comment

Choose a reason for hiding this comment

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

Ultimately, if we're doing C++ we probably want to c++-ify the interface as a formatter like cout << prettify::indent(4) << jsonthingy or some such. But that is absolutely not a thing we need to deal with in this PR :)

It's been long enough since I've been in the codebase that I'll let @lemire handle the approval here, but it looks good to me.

I would suggest removing the virtuals for now, though, and let's add them when we get a second implementation ... we might find at that point that we don't need it. (I suspect we will find that we can avoid it without complicating things :) )

@Cuda-Chen
Copy link
Contributor Author

// include/simdjson/dom/serialization.h

class pretty_formatter : public mini_formatter {
public:
    pretty_formatter(int n): mini_formatter(), indent_step(n) {} // a constructor for setting indentation

private:
    int indent_step = 4;
};

template <class T, class fmt = simdjson::internal::pretty_formatter>
std::string prettify(T x)  { // implement prettify with adjustable indentation setting
    simdjson::internal::string_builder<fmt> sb;
    sb.append(x);
    std::string_view answer = sb.str();
    return std::string(answer.data(), answer.size());
}


// prettify_test.cpp
pretty_formatter fmt(3); // this constructor is not included in this PR yet
simdjson::dom::element doc; // assume doc is loaded with parse JSON
auto serial = simdjson::prettify<fmt>(doc); // <-- this results in compilation error

What is the compilation error?

@lemire , the compilation errors are shown as follows (seems that it fails to expand the template:

$ cmake --build .
<omit some compilation messages ...>
[ 19%] Building CXX object tests/CMakeFiles/prettify_tests.dir/prettify_tests.cpp.o
/home/jio/cpp_code/simdjson/tests/prettify_tests.cpp: In function ‘bool load_prettify(const char*)’:
/home/jio/cpp_code/simdjson/tests/prettify_tests.cpp:37:41: error: no matching function for call to ‘prettify<fmt>(simdjson::dom::element&)’
   37 |   auto serial1 = simdjson::prettify<fmt>(doc);                                                                                      
      |                  ~~~~~~~~~~~~~~~~~~~~~~~^~~~~                                                                                       
In file included from /home/jio/cpp_code/simdjson/include/simdjson/dom.h:15,                                                                                 from /home/jio/cpp_code/simdjson/include/simdjson.h:38,
                 from /home/jio/cpp_code/simdjson/tests/cast_tester.h:4,
                 from /home/jio/cpp_code/simdjson/tests/prettify_tests.cpp:14:
/home/jio/cpp_code/simdjson/include/simdjson/dom/serialization.h:261:13: note: candidate: ‘template<class T, class fmt> std::string simdjson
::prettify(T)’                                                                                                                                261 | std::string prettify(T x)  {                    
      |             ^~~~~~~~                                          
/home/jio/cpp_code/simdjson/include/simdjson/dom/serialization.h:261:13: note:   template argument deduction/substitution failed:
/home/jio/cpp_code/simdjson/tests/prettify_tests.cpp:37:41: error: type/value mismatch at argument 1 in template parameter list for ‘templat
e<class T, class fmt> std::string simdjson::prettify(T)’                                                                                    
   37 |   auto serial1 = simdjson::prettify<fmt>(doc);                                                                                      
      |                  ~~~~~~~~~~~~~~~~~~~~~~~^~~~~                                                                                       
/home/jio/cpp_code/simdjson/tests/prettify_tests.cpp:37:41: note:   expected a type, got ‘fmt’
In file included from /home/jio/cpp_code/simdjson/include/simdjson/dom.h:15,                                                                                 from /home/jio/cpp_code/simdjson/include/simdjson.h:38,
                 from /home/jio/cpp_code/simdjson/tests/cast_tester.h:4,
                 from /home/jio/cpp_code/simdjson/tests/prettify_tests.cpp:14:
/home/jio/cpp_code/simdjson/include/simdjson/dom/serialization.h:271:13: note: candidate: ‘template<class T> std::string simdjson::prettify(
simdjson::simdjson_result<T>)’                                                                                                                271 | std::string prettify(simdjson_result<T> x) {                                                                                        
      |             ^~~~~~~~                                          
/home/jio/cpp_code/simdjson/include/simdjson/dom/serialization.h:271:13: note:   template argument deduction/substitution failed:
/home/jio/cpp_code/simdjson/tests/prettify_tests.cpp:37:41: error: type/value mismatch at argument 1 in template parameter list for ‘templat
e<class T> std::string simdjson::prettify(simdjson::simdjson_result<T>)’                                                           
   37 |   auto serial1 = simdjson::prettify<fmt>(doc);                                                                                      
      |                  ~~~~~~~~~~~~~~~~~~~~~~~^~~~~
/home/jio/cpp_code/simdjson/tests/prettify_tests.cpp:37:41: note:   expected a type, got ‘fmt’
/home/jio/cpp_code/simdjson/tests/prettify_tests.cpp:40:41: error: no matching function for call to ‘prettify<fmt>(simdjson::dom::element&)’
   40 |   auto serial2 = simdjson::prettify<fmt>(doc);
      |                  ~~~~~~~~~~~~~~~~~~~~~~~^~~~~
In file included from /home/jio/cpp_code/simdjson/include/simdjson/dom.h:15,
                 from /home/jio/cpp_code/simdjson/include/simdjson.h:38,
                 from /home/jio/cpp_code/simdjson/tests/cast_tester.h:4,
                 from /home/jio/cpp_code/simdjson/tests/prettify_tests.cpp:14:
/home/jio/cpp_code/simdjson/include/simdjson/dom/serialization.h:261:13: note: candidate: ‘template<class T, class fmt> std::string simdjson
::prettify(T)’
  261 | std::string prettify(T x)  {
      |             ^~~~~~~~
/home/jio/cpp_code/simdjson/include/simdjson/dom/serialization.h:261:13: note:   template argument deduction/substitution failed:
/home/jio/cpp_code/simdjson/tests/prettify_tests.cpp:40:41: error: type/value mismatch at argument 1 in template parameter list for ‘templat
e<class T, class fmt> std::string simdjson::prettify(T)’
   40 |   auto serial2 = simdjson::prettify<fmt>(doc);
      |                  ~~~~~~~~~~~~~~~~~~~~~~~^~~~~
/home/jio/cpp_code/simdjson/tests/prettify_tests.cpp:40:41: note:   expected a type, got ‘fmt’
In file included from /home/jio/cpp_code/simdjson/include/simdjson/dom.h:15,
                 from /home/jio/cpp_code/simdjson/include/simdjson.h:38,
                 from /home/jio/cpp_code/simdjson/tests/cast_tester.h:4,
                 from /home/jio/cpp_code/simdjson/tests/prettify_tests.cpp:14:
/home/jio/cpp_code/simdjson/include/simdjson/dom/serialization.h:271:13: note: candidate: ‘template<class T> std::string simdjson::prettify(
simdjson::simdjson_result<T>)’
  271 | std::string prettify(simdjson_result<T> x) {
      |             ^~~~~~~~
/home/jio/cpp_code/simdjson/include/simdjson/dom/serialization.h:271:13: note:   template argument deduction/substitution failed:
/home/jio/cpp_code/simdjson/tests/prettify_tests.cpp:40:41: error: type/value mismatch at argument 1 in template parameter list for ‘templat
e<class T> std::string simdjson::prettify(simdjson::simdjson_result<T>)’
   40 |   auto serial2 = simdjson::prettify<fmt>(doc);
      |                  ~~~~~~~~~~~~~~~~~~~~~~~^~~~~
/home/jio/cpp_code/simdjson/tests/prettify_tests.cpp:40:41: note:   expected a type, got ‘fmt’
gmake[2]: *** [tests/CMakeFiles/prettify_tests.dir/build.make:76: tests/CMakeFiles/prettify_tests.dir/prettify_tests.cpp.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:1471: tests/CMakeFiles/prettify_tests.dir/all] Error 2
gmake: *** [Makefile:166: all] Error 2

@Cuda-Chen
Copy link
Contributor Author

Hi @lemire and @jkeiser ,
I will work on to use templates to get rid of virtual methods.

Add pretty print for DOM with documentation.
Currently, the indentation is fixed to four spaces.

Close simdjson#1329.
@lemire
Copy link
Member

lemire commented Jul 8, 2023

Running tests.

@jkeiser
Copy link
Member

jkeiser commented Jul 17, 2023

Note that if #2031 lands before this, it may affect it--however, this PR doesn't look like it'll be affected much if at all. I'll check after it lands and make a PR to be merged into this if there is anything major!

@jkeiser jkeiser merged commit a74e87d into simdjson:master Jul 17, 2023
40 checks passed
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.

Pretty print for DOM
3 participants