chpldoc/next - now with queries #18956

aconsroe-hpe · 2022-01-10T19:27:14Z

Squash of progress on chpldoc/next overhaul to use queries.

Note this "requires" C++17 only as a convenience step in prototyping to get the fileystem functionality. Future work can remove this.

Adds an temporary env var CHPL_CHPLDOC_NEXT so that we can force sub_test to use that binary when doing chpldoc tests.

Passes 15 tests in test/chpldoc currently

Squash of progress on chpldoc/next overhaul to use queries. That was an easy step. Adds an initial implementation of actually converting the AST into the appropriate RST format. This uses the visitor and traverse pattern. Added a hack environment variable to sub_test.py so that CHPL_CHPLDOC_NEXT can be specified to use it instead of today's chpldoc. This was because PATH get's messed with by fixpath.py Note this "requires" C++17 only as a convenience step in prototyping to get the fileystem functionality. Today we only pass 1 test case. Many are whitespace only which is a good sign, just ran out of time to address them. Signed-off-by: Andrew Consroe <andrew.consroe@hpe.com>

compiler/next/tools/chpldoc/CMakeLists.txt

mppf · 2022-01-10T20:05:07Z

compiler/next/tools/chpldoc/chpldoc.cpp

+/*
+  To test this today, you can force sub_test to use this version by doing:
+  > CHPL_CHPLDOC_NEXT=$(find $(pwd) -name chpldoc -type f) ./util/test/start_test.py test/chpldoc/enum.doc.chpl
+  You'll want to make sure that the find command finds a single chpldoc executable


An alternative would be to adjust compiler/main/driver to run this alternative chpldoc implementation.

mppf · 2022-01-10T20:19:29Z

Today we only pass 1 test case. Many are whitespace only which is a
good sign, just ran out of time to address them.

I think you are saying there are whitespace differences? Which is the 1 test that passes?

aconsroe-hpe · 2022-01-10T20:41:19Z

Yes whitespace only differences is what I meant.
The 1 test is chpldoc/enum/docConstants, nothing to write home about.

mppf

What would it take to only use C++14?

Also this version doesn't implement parts of chpldoc with queries the way I was expecting. It does use a visitor, which is good. I was expecting it to define a one or more queries, e.g. a query (with QUERY_BEGIN) for going from a uAST node for a module or function to a string containing the .rst snippet produced by chpldoc. If there is a reason that you don't want to do that, we should discuss.

util/test/sub_test.py

mppf · 2022-01-10T20:55:09Z

compiler/next/tools/chpldoc/chpldoc.cpp

+  return lines[0];
+}
+
+static const char* kindToRstString(bool isMethod, Function::Kind kind) {


maybe name it FunctionKindToRstString ?

mppf · 2022-01-10T20:55:23Z

compiler/next/tools/chpldoc/chpldoc.cpp

-                         "  /* comment 3 */\n"
-                         "}\n"
-                         "/* comment 4 */";
+static const char* kindToString(Function::Visibility kind) {


maybe name it visibilityToString ?

I think the overload is appropriate here

mppf · 2022-01-10T20:55:57Z

compiler/next/tools/chpldoc/chpldoc.cpp

-int main(int argc, char **argv) {
-  Context context;
-  Context *ctx = &context;
+static const char* kindToString(Function::Kind kind) {


functionKindToString?

mppf · 2022-01-10T20:56:05Z

compiler/next/tools/chpldoc/chpldoc.cpp


-  auto parser = Parser::build(ctx);
-  Parser *p = parser.get();
+static const char* kindToString(Function::ReturnIntent kind) {


returnIntentToString ?

mppf · 2022-01-10T20:56:14Z

compiler/next/tools/chpldoc/chpldoc.cpp


-  auto parseResult = argc == 2 ? parser->parseFile(argv[1])
-                               : parser->parseString("test1a.chpl", testString);
+static const char* kindToString(Formal::Intent kind) {


intentToString ?

mppf · 2022-01-10T20:56:22Z

compiler/next/tools/chpldoc/chpldoc.cpp

+  return "";
+}
+
+static const char* kindToString(Variable::Kind kind) {


variableKindToString?

mppf · 2022-01-10T20:57:11Z

compiler/next/tools/chpldoc/chpldoc.cpp

+  return "";
+}
+
+static const char* kindToString(New::Management kind) {


newManagementToString?

mppf · 2022-01-10T20:58:22Z

compiler/next/tools/chpldoc/chpldoc.cpp

+struct PrettyPrintVisitor {
+  std::ostream& os_;
+
+  template<typename It>


Need a comment about what this method is for

compiler/next/tools/chpldoc/CMakeLists.txt

aconsroe-hpe · 2022-01-11T15:16:28Z

What would it take to only use C++14?

Removing the call to std::filesystem::create_directories

Heard on the queries part. Seems like the biggest obstacle is knowing what your last comment is. I know we maintain the parent mapping so it should be possible to check your previous sibling but maybe there's a nicer way I'm not thinking of?

This extends the previous work of adding queries to next/chpldoc by converting the one-shot visitor approach with a qury rstDoc to compute the docs for a given node. These results form a tree with the goal being that the query framework will re-use subtrees that have not changed. This avoids a small blow-up in eagerly concatenating a tree of strings but the main motivation was to kick the tires more on queries. This now passes a whopping 15 chpldoc tests! 138 Failures still :) Also fixes a small bug in mark-functions.h Signed-off-by: Andrew Consroe <andrew.consroe@hpe.com>

Signed-off-by: Andrew Consroe <andrew.consroe@hpe.com>

aconsroe-hpe · 2022-01-12T18:48:05Z

I implemented the approach we had discussed yesterday, ready for another pass at your leisure @mppf

mppf

Thanks for the impromevents! I think this is looking good, structurally. Please make yourself a few issues to track future work:

get more chpldoc tests passing with this
add regular testing of it, somewhere
remove the C++17 dependency (or migrate our compiler sources to C++17) since we will need to do that before this code can be used in production

compiler/next/tools/chpldoc/chpldoc.cpp

Signed-off-by: Andrew Consroe <andrew.consroe@hpe.com>

mppf reviewed Jan 10, 2022

View reviewed changes

compiler/next/tools/chpldoc/CMakeLists.txt Show resolved Hide resolved

mppf reviewed Jan 10, 2022

View reviewed changes

mppf approved these changes Jan 10, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

aconsroe-hpe added 2 commits January 12, 2022 12:31

Add comment on temporary intention

7c81b28

Signed-off-by: Andrew Consroe <andrew.consroe@hpe.com>

mppf approved these changes Jan 12, 2022

View reviewed changes

compiler/next/tools/chpldoc/chpldoc.cpp Show resolved Hide resolved

compiler/next/tools/chpldoc/chpldoc.cpp Outdated Show resolved Hide resolved

Cleanup

8958ec9

Signed-off-by: Andrew Consroe <andrew.consroe@hpe.com>

aconsroe-hpe merged commit f9c86a9 into chapel-lang:main Jan 12, 2022

aconsroe-hpe deleted the next/chpldoc-2 branch January 12, 2022 19:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chpldoc/next - now with queries #18956

chpldoc/next - now with queries #18956

aconsroe-hpe commented Jan 10, 2022 •

edited

mppf Jan 10, 2022

mppf commented Jan 10, 2022

aconsroe-hpe commented Jan 10, 2022

mppf left a comment

mppf Jan 10, 2022

mppf Jan 10, 2022

aconsroe-hpe Jan 11, 2022

mppf Jan 10, 2022

mppf Jan 10, 2022

mppf Jan 10, 2022 •

edited

mppf Jan 10, 2022

mppf Jan 10, 2022

mppf Jan 10, 2022

aconsroe-hpe commented Jan 11, 2022

This comment was marked as outdated.

aconsroe-hpe commented Jan 12, 2022

mppf left a comment

chpldoc/next - now with queries #18956

chpldoc/next - now with queries #18956

Conversation

aconsroe-hpe commented Jan 10, 2022 • edited

Choose a reason for hiding this comment

mppf commented Jan 10, 2022

aconsroe-hpe commented Jan 10, 2022

mppf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mppf Jan 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aconsroe-hpe commented Jan 11, 2022

This comment was marked as outdated.

aconsroe-hpe commented Jan 12, 2022

mppf left a comment

Choose a reason for hiding this comment

aconsroe-hpe commented Jan 10, 2022 •

edited

mppf Jan 10, 2022 •

edited