Make lorina's shared regexes inline const - #706
Open
marcelwa wants to merge 1 commit into
Open
Conversation
The six vendored lorina format headers declare their compiled patterns `static` at namespace scope. `static` gives internal linkage, so every translation unit including aiger.hpp gets its own private set of nine std::regex objects; linking two such TUs yields 18 distinct objects where the source describes 9. `inline` is the C++17 spelling for the single shared definition intended, and mockturtle already builds at C++17. `const` matters more. These live in public namespaces under public names, and only convention stops a caller reassigning one. Every use is std::regex_match or an sregex_iterator constructor, both taking a const reference, so they are already read-only in practice -- which is what makes concurrent parsing safe. const promotes that from a property of the current call sites into one the compiler enforces. 29 patterns across aiger, pla, verilog, bench, blif and dimacs. Declaration-only: no call site changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #706 +/- ##
=======================================
Coverage 84.06% 84.06%
=======================================
Files 190 190
Lines 29515 29515
=======================================
Hits 24812 24812
Misses 4703 4703 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi — a small, declaration-only patch to the vendored lorina copy, offered as a courtesy.
The six lorina format headers under
lib/lorina/lorina/declare their compiled patternsstaticat namespace scope —aig_regex,pla_regex,verilog_regex,bench_regex,blif_regexanddimacs_regex, 29 patterns in all. This makes theminline const. No call site changes.This same patch is open upstream at hriener/lorina#90. If you would rather take it by re-vendoring once that lands, please just close this — I opened both so the fix is available whichever route you prefer, not to pre-empt the upstream one.
staticin a header duplicates per translation unitstaticgives internal linkage, so every TU that includesaiger.hppgets its own private set of ninestd::regexobjects, each constructed during that TU's static initialization.inlineis the C++17 spelling for the single shared definition intended, andMOCKTURTLE_CXX_STANDARDalready defaults to"17".Measured on two TUs that each include
lorina/aiger.hpp, linked together:aig_regexobjects.bssnmagrees: each pattern goes from ab(local BSS) symbol private to its object file to au(GNU unique) symbol shared across them. mockturtle's readers are included from many TUs in a typical build, so this repeats.constis the half that prompted thisThese objects live in public namespaces under public names, and only convention stops a caller doing
lorina::aig_regex::header.assign(...). Every use isstd::regex_matchor ansregex_iteratorconstructor, both taking aconst std::regex&— so they are already read-only in practice, and that is exactly what makes it safe for several threads to parse at once, sincestd::regex's const member functions carry the standard's usual thread-safety guarantee.The context: we maintain aigverse, which wraps mockturtle's readers for Python, and recently released the GIL around them so threaded corpus loading actually overlaps. The audit justifying that rests on the "only ever reached through a const reference" argument, which today must be re-established by reading every call site and could be silently invalidated by a caller.
constmakes it compiler-checked.Testing
Compiles and links clean at C++17 across two TUs including all six patched headers. Upstream lorina's own suite passes with this change — 835 assertions in 74 test cases. End-to-end, aigverse's full suite passes against a mockturtle carrying it (426 passed, 94 skipped), exercising the AIGER binary and ASCII, PLA and Verilog readers, including a suite that parses each format from eight threads at once. It is also green across this repository's full CI matrix — g++-10/11/12, clang++-14 through 17, macOS, and both MSVC toolsets — via the equivalent change on our fork.
One behavior note
C++17 inline variables in a shared library emit
STB_GNU_UNIQUEsymbols, which make the.soineligible fordlcloseunloading. That holds for every C++17 inline variable and is irrelevant to header-only consumption, but it is a real difference fromstaticand worth flagging.Not included
The patterns themselves are untouched. Several — the AIGER header's nine optional capture groups,
^i(\d+) (.*)$— parse space-separated integers with a backtracking engine and would likely be faster as a hand-written scanner, most of all inverilog_regexandpla_regexwhere the match runs per line. That is a separate change wanting a profile behind it.Happy to adjust or drop this.