TheLartians / PEGParser

💡 Build your own programming language! A C++17 PEG parser generator supporting parser combination, memoization, left-recursion and context-dependent grammars.
BSD 3-Clause "New" or "Revised" License
240 stars 21 forks source link

Add subscript operator to Expression to access sub-expressions by name #60

Closed jteuber closed 3 years ago

jteuber commented 3 years ago

Hi there! It might be just me, but accessing certain sub-expressions of a rule by name seems very handy for me, especially with very complicated rules. So I added this. It seems to work quite well. Especially the std::optional makes parsing expressions with optional expressions much simpler.

TheLartians commented 3 years ago

Hey, thanks for the PR, I definitely see the advantages of the rule based query, and good call with the optional return! Could you also add a test case, so the feature remains stable in the future?

jteuber commented 3 years ago

Sure, there you go! :)

codecov[bot] commented 3 years ago

Codecov Report

Merging #60 (e1b5022) into master (dcec3a0) will increase coverage by 0.02%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   97.67%   97.69%   +0.02%     
==========================================
  Files           9        9              
  Lines         646      652       +6     
==========================================
+ Hits          631      637       +6     
  Misses         15       15              
Impacted Files Coverage Δ
include/peg_parser/interpreter.h 95.71% <100.00%> (+0.40%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update dcec3a0...e1b5022. Read the comment docs.

jteuber commented 3 years ago

Yes, you're absolutely right. That's a much better test case. I had a hard time coming up with a simple test yesterday so I basically simplified the use case I had for the subscript operator. As soon as I have some time on my PC today I copy over your suggestion.

Feb 5, 2021 10:43:43 Lars Melchior notifications@github.com:

@TheLartians commented on this pull request.


In test/source/parser.cpp[https://github.com/TheLartians/PEGParser/pull/60#discussion_r570842318]:

+  ParserGenerator program; +  program.setSeparator(program["Whitespace"] << "[\t ]"); + +  program["Word"] << "[a-z]+"; +  program["Yell"] << "[A-Z]+"; +  program["Number"] << "[0-9]+"; +  program["IntSubscript"] << "Word Yell? Number" >> [](auto e) { return e[1].string(); }; +  program["StringSubscriptOptional"] << "Number Yell? Word" >> [](auto e) { +    if (auto expr = e["Yell"]) return expr->string(); +    return std::string(); +  }; +  program["Start"] << "IntSubscript | StringSubscriptOptional"; + +  program.setStart(program["Start"]); + +  REQUIRE(program.run("ab 0") == "0"); +  REQUIRE(program.run("ab CD 1") == "CD"); +  REQUIRE(program.run("2 ef") == ""); +  REQUIRE(program.run("2 GH ij") == "GH");

Hey, thanks for adding the test case! I do find it to be a hard to follow, as it's doing a lot of things at once. What do you think about simplifying it to a single rule that checks the new [string] operator?

For example, something like

  ParserGenerator program;   program["Word"] << "[a-z]+";   program["Yell"] << "[A-Z]+";   program["Start"] << "Word | Yell" >> [](auto e){ return bool(e["Yell"]);  }   REQUIRE(!program.run("hello"));   REQUIRE(program.run("HELLO"));

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub[https://github.com/TheLartians/PEGParser/pull/60#pullrequestreview-584173066], or unsubscribe[https://github.com/notifications/unsubscribe-auth/ADTEN343AV7EBEC4YRMQ3L3S5O4U3ANCNFSM4XBT36QA]. [data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAADUAAAA1CAYAAADh5qNwAAAAAXNSR0IArs4c6QAAAARzQklUCAgICHwIZIgAAAAiSURBVGiB7cEBDQAAAMKg909tDjegAAAAAAAAAAAAAIB7AywZAAGURgP6AAAAAElFTkSuQmCC###24x24:true###][Tracking image][https://github.com/notifications/beacon/ADTEN37QNKKWFOYXQNH6H5DS5O4U3A5CNFSM4XBT36QKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOELI4MCQ.gif]

TheLartians commented 3 years ago

The feature is released in v2.2.0! 🎉