[regexp] Better operand disassembly [v8/v8 : main]
Jakob Linke (Gerrit)
Jakob Linke added 1 comment![]( "Open in Gerrit")
Related details
- Patrick Thier
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Thier (Gerrit)
Patrick Thier added 13 comments![]( "Open in Gerrit")
Nice! I just have a couple of suggestions and nits.
if (comma == std::string_view::npos) break;This shouldn't ever be relevant, unless the function is called with a wrong `N`. Asserting that N actually matches what we observed would be better IMO. WDYT?
while (!part.empty() && part.front() == ' ') part.remove_prefix(1);
while (!part.empty() && part.back() == ' ') part.remove_suffix(1);nit: I don't think we need these loops.
You could use `find_first_not_of`/`find_last_not_of` together with a single `substr`.
size_t end = (comma == std::string_view::npos) ? names.size() : comma;nit: With my suggestions above, you could just use `names.size() - 1` here (if back == ')' is asserted as suggested).
names.remove_prefix(1);
names.remove_suffix(1);nit: `substr()` would be more efficient here.
Or even better: just set `start = 1`.
if (names.size() >= 2 && names.front() == '(' && names.back() == ')') {nit: This is the expected input format, so could we assert that `front == '(' and back == ')'` instead?
constexpr std::array<std::string_view, N> SplitNames(const char* raw_names) {nit: `consteval`. Or is there something that prevents it that I didn't see?
for (int i = 0; i < RegExpBytecodes::Size(bytecode); i++) {
PrintF(", %02x", pc[i]);
}In case something is completely broken with the bytecode, it might be useful to have the raw output. WDYT about keeping/adding the raw output behind a separate flag (e.g. `--print-regexp-bytecode-raw`)?
PrintF("[bit table]");I think having the values of the bit-table is also often interesting. Can we keep it?
pc, DisallowGarbageCollection{});This is a weird pattern. Please move the DisallowGC scope to the top of the method.
PrintF("%x", val);nit: It would be nice to prefix the value with `0x` to avoid confusion.
} else if constexpr (type == RegExpBytecodeOperandType::kChar) {
if (std::isprint(val)) {
PrintF("'%c'", val);
} else {
PrintF("%x", val);nit: While you change this, we could also switch to the more modern ostream approach. This would allow us to use `AsUC32` here.
PrintF("%x", static_cast<uint32_t>(val));nit: Why static_cast all the remaining operands to uint32_t instead of using the "correct" type? I guess it is just because of the 2 enum values? Can we specialize for these types instead?
Also: With the correct types now in place, maybe we don't want to just use `%x` for every value? Maybe it makes sense to add the format specifier to the operand type list?
Related details
- Jakob Linke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |