[PATCH] D114790: [syntax][pseudo] Add grammar facilities for the pseudo-parser

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.


================
Comment at: clang/include/clang/Tooling/Syntax/Pseudo/Grammar.h:85
+  static constexpr unsigned SizeBits = 4;
+  static_assert(SizeBits + SymbolBits <= 16,
+                "Must be able to store symbol ID + size efficiently");
----------------
hokein wrote:
> sammccall wrote:
> > We're being quite fiddly with layout but actually pretty wasteful.
> > 
> > wc says 652 with 1323 total RHS elements on the RHS - average ~2.
> > So we could aim for 6 bytes per rule = 4KB but we're using 34 bytes per rule = 22KB.
> > 
> > Can't see a nice concrete way to achieve that though.
> Yes :(
> I'd keep it as-is (and decrease the MaxElements to 9). One idea is to use a flat array as a center storage of all sequences, then Rule class just need an index, and size. But there will be more data (annotations, hooks) in this class, we could figure it out later.
> Yes :(
> I'd keep it as-is (and decrease the MaxElements to 9).
SGTM

> One idea is to use a flat array as a center storage of all sequences, then Rule class just need an index, and size.

This makes APIs awkward though: Rule.seq() needs extra params to build an ArrayRef, unless you want to carry around a pointer to the big array (which gives back most of the size savings)


================
Comment at: clang/lib/Tooling/Syntax/Pseudo/GrammarBNF.cpp:140
+    for (llvm::StringRef Line : llvm::split(Lines, '\n')) {
+      Line = Line.trim();
+      // Strip anything coming after the '#' (comment).
----------------
move this after the take_while, so that `   # this is a comment` is still skipped?


================
Comment at: clang/lib/Tooling/Syntax/Pseudo/GrammarBNF.cpp:153
+
+  bool ParseLine(llvm::StringRef Line, RuleSpec &Out) {
+    auto Parts = Line.split(":=");
----------------
ParseLine -> parseLine


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114790/new/

https://reviews.llvm.org/D114790