Cleanup regex compiler operators and operands source #10879
Conversation
davidwendt
added
2 - In Progress
davidwendt
changed the title
Codecov Report
@@ Coverage Diff @@
## branch-22.08 #10879 +/- ##
===============================================
Coverage ? 86.32%
===============================================
Files ? 144
Lines ? 22696
Branches ? 0
===============================================
Hits ? 19593
Misses ? 3103
Partials ? 0 Continue to review full report at Codecov.
|
davidwendt
added
3 - Ready for Review
cpp/src/strings/regex/regcomp.cpp
Outdated
|
|
||
| static std::vector<int> tokens{STAR, STAR_LAZY, QUEST, QUEST_LAZY, PLUS, PLUS_LAZY, RBRA}; | ||
| _last_was_and = | ||
| std::any_of(tokens.begin(), tokens.end(), [token](auto t) { return t == token; }); |
There was a problem hiding this comment.
I guess tokens are cheap (and this is host code) so this doesn't really matter, but you could capture the token by reference to avoid copying right? Also, you could iterator using cbegin and cend instead and make the function parameter auto const t.
There was a problem hiding this comment.
Copy by value for int types is preferred over by reference if the value is not changed. I think the cbegin/cend is a good idea. I don't think declaring t as const is that helpful here. The compiler will generate the same code and a human only has one line of code to parse.
There was a problem hiding this comment.
I'm dumb, when I wrote "I guess tokens are cheap" I didn't check the vector above to see exactly what tokens are. Yes, definitely agree that copy by value is preferred here. I also agree that the const is not necessary, was just a minor suggestion.
Should we make the tokens vector constexpr, then? Also, just a note that if we ever switch OperatorType to be a C++-style scoped enum we should probably add a base type specifier that aligns with the type of the tokens vector just to be explicit and avoid type conversions.
There was a problem hiding this comment.
I think constexpr would be more appropriate. Unfortunately you can only declare constexpr for literal or reference types.
So I prefer using static here.
It looks like c++20 will support constexpr with std::vector.
There was a problem hiding this comment.
Right, that's a good point. This would probably be a good place to use std::array (with CTAD so that you don't even need to specify the length).
|
@gpucibot merge |
Cleans up the
regcomp.cppsource to fix class names, comments, and simplify logic around processing operators and operands returned by the parser. Several class member variables used for state are moved or eliminated. Some member functions and variables are renamed. Cleanup of the parser logic will be in a follow-on PR.Reference #3582
Follow on to #10843