Repository navigation
Conversation
c9e91c7 to
5a66d89
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| /// @param IdxOrigin the index of * or _ that might start a delimiter run. | ||
| /// @return A pair denoting the type of run and the index where the run stops | ||
| std::pair<std::optional<DelimiterContext>, size_t> | ||
| processDelimiters(SmallString<64> &Line, const size_t &Origin = 0); |
There was a problem hiding this comment.
| /// @param IdxOrigin the index of * or _ that might start a delimiter run. | |
| /// @return A pair denoting the type of run and the index where the run stops | |
| std::pair<std::optional<DelimiterContext>, size_t> | |
| processDelimiters(SmallString<64> &Line, const size_t &Origin = 0); | |
| /// @param Origin the index of * or _ that might start a delimiter run. | |
| /// @return A pair denoting the type of run and the index where the run stops | |
| std::pair<std::optional<DelimiterContext>, size_t> | |
| processDelimiters(SmallString<64> &Line, const size_t &Origin = 0); |
The doc name seems stale. I do wonder if you should just use Start or StartIdx, though.
| return Result; | ||
| } | ||
|
|
||
| void MarkdownParser::parseLine(SmallString<64> &Line, Node *Current) { |
There was a problem hiding this comment.
can line also just be StringRef here? maybe I missed it, but I don't see you manipulating it at all.
| if (Closer->Length >= 2 && Opener->Length >= 2) { | ||
| // We have at least one strong node. | ||
| Closer->Length -= 2; | ||
| Opener->Length -= 2; | ||
| Emphasis = new (Arena) Node(); | ||
| Emphasis->Type = MDType::Strong; | ||
| auto *Child = createTextNode(Text); | ||
| Child->Parent = Emphasis; | ||
| Emphasis->Children.push_back(Child); | ||
| } else if (Closer->Length == 1 && Opener->Length == 1) { | ||
| Closer->Length -= 1; | ||
| Opener->Length -= 1; | ||
| Emphasis = new (Arena) Node(); | ||
| Emphasis->Type = MDType::Emphasis; | ||
| auto *Child = createTextNode(Text); | ||
| Child->Parent = Emphasis; | ||
| Emphasis->Children.push_back(Child); |
There was a problem hiding this comment.
The difference in these blocks seems to be the MDType, and the length adjustment. maybe just use a helper? or just set those bits on their own to avoid repeating mostly the same code?
| char Preceeding = (Origin == 0) ? ' ' : Line[Origin - 1]; | ||
| char Proceeding = (Idx >= Line.size()) ? ' ' : Line[Idx]; |
There was a problem hiding this comment.
| char Preceeding = (Origin == 0) ? ' ' : Line[Origin - 1]; | |
| char Proceeding = (Idx >= Line.size()) ? ' ' : Line[Idx]; | |
| char Leading = (Origin == 0) ? ' ' : Line[Origin - 1]; | |
| char Trailing = (Idx >= Line.size()) ? ' ' : Line[Idx]; |
Maybe this is a bit easier to follow/not accidentally typo? happy with any name, but these are really just 1 char apart right now...
| if (LeftFlanking && RightFlanking) | ||
| return {DelimiterContext{LeftFlanking, RightFlanking, true, true}, Idx}; | ||
| if (LeftFlanking) | ||
| return {DelimiterContext{LeftFlanking, RightFlanking, true, false}, Idx}; | ||
| if (RightFlanking) | ||
| return {DelimiterContext{LeftFlanking, RightFlanking, false, true}, Idx}; | ||
| return {std::nullopt, 0}; |
There was a problem hiding this comment.
| if (LeftFlanking && RightFlanking) | |
| return {DelimiterContext{LeftFlanking, RightFlanking, true, true}, Idx}; | |
| if (LeftFlanking) | |
| return {DelimiterContext{LeftFlanking, RightFlanking, true, false}, Idx}; | |
| if (RightFlanking) | |
| return {DelimiterContext{LeftFlanking, RightFlanking, false, true}, Idx}; | |
| return {std::nullopt, 0}; | |
| if(!LeftFlanking && !RightFlanking) | |
| return {std::nullopt, 0}; | |
| return {DelimiterContext{LeftFlanking, RightFlanking, LeftFlanking, RightFlanking}, Idx}; |
I think this is equivalent, right?
| for (const auto *Node : Text) { | ||
| TextNode->Content.append(Node->Content); | ||
| } |
There was a problem hiding this comment.
| for (const auto *Node : Text) { | |
| TextNode->Content.append(Node->Content); | |
| } | |
| for (const auto *Node : Text) | |
| TextNode->Content.append(Node->Content); |
5a66d89 to
571e578
Compare
|
I've rewritten a large part of the initial implementation idea. The biggest change is that I've separated the Markdown AST, parser, and HTML generation (which doesn't exist here yet). The parser is independent of the AST. The AST owns the arena and string saver. The motivation behind the separation is that a single parser object could be reused for different ASTs in the future or several ASTs could be parsed in parallel. Then, these ASTs could be fed to different engines and not just HTML. You can see an example of the usage in the tests. There's still a lot of work to be done here but I thought some initial feedback on the design direction could be helpful, especially the problems with arena allocations and memory leaks that I had last time. It might also motivate discussions for the recent RFC or help GSoC applicants. |
🐧 Linux x64 Test ResultsThe build failed before running any tests. Click on a failure below to see the details. tools/clang/tools/extra/clang-doc/CMakeFiles/obj.clangDoc.dir/MDParser.cpp.oIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
ilovepi
left a comment
There was a problem hiding this comment.
Took a first pass. Will give more feedback tomorrow.
| static bool isEmpty(StringRef Line) { | ||
| if (Line.size() == 0) | ||
| return true; | ||
| if (Line.size() == 1 && Line[0] == ' ') | ||
| return true; | ||
| for (auto &Char : Line) { | ||
| if (Char != ' ') | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I think I don't follow. Can you get something similar with
| static bool isEmpty(StringRef Line) { | |
| if (Line.size() == 0) | |
| return true; | |
| if (Line.size() == 1 && Line[0] == ' ') | |
| return true; | |
| for (auto &Char : Line) { | |
| if (Char != ' ') | |
| return false; | |
| } | |
| return true; | |
| } | |
| static bool isEmpty(StringRef Line) { | |
| return Line.trim().empty(); | |
| } |
| } | ||
|
|
||
| static MDType determineBlockType(StringRef Line) { | ||
| StringRef TrimmedLine = Line.ltrim(' '); |
There was a problem hiding this comment.
What about other whitespace characters? like \t? Also, is ther a particular spec I should look at while reviewing?
| return MDType::Paragraph; | ||
| case '>': | ||
| return MDType::Paragraph; | ||
| default: | ||
| return MDType::Paragraph; |
There was a problem hiding this comment.
Are these all intended to be the same? If so, why not fall through all of them?
| StringRef ASTContext::intern(std::string &String) { | ||
| return SSaver.save(String); | ||
| } | ||
| StringRef ASTContext::intern(StringRef String) { return SSaver.save(String); } |
There was a problem hiding this comment.
Do yo need these overloads? I thought Twine would cover all of them...
| std::optional<std::pair<DelimiterContext, size_t>> | ||
| MarkdownParser::processDelimiters(StringRef Line, const size_t &Start) { | ||
| size_t Idx = Start; | ||
| while (Idx < Line.size() && Line[Idx] == Line[Start]) { |
There was a problem hiding this comment.
you can avoid the call to size() in the loop header.

Initial implementation of a Markdown parser and Markdown AST. Currently, the parser only recognizes emphasis and strong syntax. It also only creates paragraph blocks. The AST is allocated via a BumpPtrAllocator. It attempts to conform to the CommonMark standard
The parser ingests lines and creates preprocessing LineNodes that later are converted to Nodes that form the AST.