[libcxx-commits] [PATCH] D119467: [demangler][NFC] Tabularize operator name parsing

[Chuanqi Xu via Phabricator via libcxx-commits]

ChuanqiXu accepted this revision.
ChuanqiXu added a comment.
This revision is now accepted and ready to land.

The scale of the patch shocked me at the first sight. But after I tried to visit it actually. I find the new structure seems much better. Only style comment remained and feel free to address them.



================
Comment at: libcxxabi/src/demangle/ItaniumDemangle.h:2936
+  };
+  const auto NumOps = sizeof(Ops) / sizeof(Ops[0]);
+
----------------
Maybe it is possible to use container here:
```
static constexpr ArrayRef<OperatorInfo> OpArr(Ops);
```
Then we could use C++ style here.


================
Comment at: libcxxabi/src/demangle/ItaniumDemangle.h:2941
+    // Verify table order.
+    static bool Done;
+    if (!Done) {
----------------
static bool Done = false;


================
Comment at: libcxxabi/src/demangle/ItaniumDemangle.h:2944-2945
+      Done = true;
+      for (const auto *Op = &Ops[0]; Op != &Ops[NumOps - 1]; Op++)
+        assert(Op[0] < Op[1] && "Operator table is not ordered");
+    }
----------------
Could we use `std::is_sorted ` here?


================
Comment at: libcxxabi/src/demangle/ItaniumDemangle.h:3006
+    // ::= v <digit> <source-name>        # vendor extended operator
+    if (look() >= '0' && look() <= '9') {
+      First++;
----------------
Is this not equal to `isdigit()` ?


================
Comment at: libcxxabi/src/demangle/ItaniumDemangle.h:4298
+  bool IsLeftFold = false, HasInitializer = false;
+  switch (look()) {
+  default:
----------------
I feel the original if-else is more simpler.


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

https://reviews.llvm.org/D119467