odb: use snake_case for variables in LEF58 cut spacing parser by areporeporepo · Pull Request #10091 · The-OpenROAD-Project/OpenROAD

areporeporepo · 2026-04-09T00:52:58Z

Summary

Rename variables to snake_case in the LEF58 cut spacing parser files per #2275, and clean up the curRule member naming per Coding Practice #6 ("Don't name variables theThingy, curThingy or myThingy").

Changes

Commit 1 — rename (pure refactor, no behavior change):

curRule member renamed to rule in three classes:
- lefTechLayerMinStepParser
- lefTechLayerCutSpacingParser
- lefTechLayerCutSpacingTableParser
Local variables in lefTechLayerCutSpacingParser.cpp renamed to snake_case:
- camelCase: secondLayer, twocuts, className, sideParallelNoPrl, sameMask, cutClassName, cutClass, exceptWhat, numCut, aboveBelow
- ALL_CAPS locals that matched LEF keyword names: ABOVE, CUTCLASS, EXCEPTTWOEDGES, EXCEPTSAMEVIA
In lefTechLayerCutSpacingTableParser::createOrthogonalSubRule, the local variable rule (an OrthRule) renamed to orth_rule to avoid shadowing the newly renamed rule member.

Commit 2 — fix pre-existing bugs (behavior change):

setOrthogonalSpacing() was calling setOrthogonalSpacingValid() twice. The second call should have been setOrthogonalSpacing(). The implicit int→bool conversion masked the error: the validity flag was set but the actual orthogonal spacing distance was never stored.
lefTechLayerMinStepParser::rule and lefTechLayerCutSpacingParser::rule were declared without initialization. Both parse() paths have an if (!valid && rule != nullptr) check that is undefined behavior if parse() fails before the member is assigned. Default-initialized both to nullptr, matching the pattern already used by lefTechLayerCutSpacingTableParser.

Commit 3 — fix typo:

createOrthongonalSubRule → createOrthogonalSubRule (transposed 'n' in "Orthongonal").

Commit 4 — address Gemini re-review:

Rename cut_class → cut_class_rule for the rule-pointer local variable (3 sites), resolving ambiguity with class-name strings.
Rename cutclass → cut_class in addSameMetalSharedEdgeSubRule for proper snake_case.

Notes

clang-format applied to all touched .cpp files; diff is clean.
Bug fixes and typo fix are in separate commits from the rename to keep history clean for git bisect.

Fixes #2275

Follow Google C++ style and Coding Practice The-OpenROAD-Project#6 ("Don't name variables theThingy, curThingy or myThingy"): - Rename the curRule member in lefTechLayerMinStepParser, lefTechLayerCutSpacingParser, and lefTechLayerCutSpacingTableParser to rule. - Rename local variables in lefTechLayerCutSpacingParser.cpp to snake_case: secondLayer, twocuts, className, sideParallelNoPrl, sameMask, cutClassName, cutClass, exceptWhat, numCut, aboveBelow, and the ALL_CAPS locals ABOVE, CUTCLASS, EXCEPTTWOEDGES, EXCEPTSAMEVIA that matched LEF keyword names. - In lefTechLayerCutSpacingTableParser::createOrthongonalSubRule, rename the local rule (OrthRule) to orth_rule to avoid name shadowing after the curRule member rename. Pure refactor. No behavior change. LEF58 keyword string literals inside lit() are untouched. Fixes The-OpenROAD-Project#2275 Signed-off-by: anh nguyen <areporeporepo@users.noreply.github.com>

github-actions

Welcome to OpenROAD! Thanks for opening your first PR.
Before we review:

Contribution Guide: https://openroad.readthedocs.io/en/latest/contrib/contributing.html
Build Instructions: https://openroad.readthedocs.io/en/latest/contrib/BuildWithCMake.html

Please ensure:

CI passes
Code is properly formatted
Tests are included where applicable
A maintainer will review shortly!

Copilot

Pull request overview

Mechanical refactor to align LEF58 cut spacing parser code with the project’s snake_case naming convention and remove the curRule member naming pattern, without intending behavioral changes.

Changes:

Renamed curRule member to rule in lefTechLayerMinStepParser, lefTechLayerCutSpacingParser, and lefTechLayerCutSpacingTableParser.
Renamed various local variables in lefTechLayerCutSpacingParser.cpp to snake_case.
Avoided member shadowing in lefTechLayerCutSpacingTableParser.cpp by renaming a local rule to orth_rule.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
src/odb/src/lefin/lefTechLayerMinStepParser.cpp	Updates member usage from `curRule` to `rule` throughout the MINSTEP rule parsing logic.
src/odb/src/lefin/lefTechLayerCutSpacingTableParser.cpp	Updates member usage from `curRule` to `rule` and renames a local to avoid shadowing.
src/odb/src/lefin/lefTechLayerCutSpacingParser.cpp	Updates member usage from `curRule` to `rule` and renames locals to snake_case within the LEF58 cut spacing parser.
src/odb/src/lefin/lefLayerPropParser.h	Renames parser class members from `curRule` to `rule` to match updated implementation.

Comments suppressed due to low confidence (1)

src/odb/src/lefin/lefLayerPropParser.h:110

lefTechLayerCutSpacingParser::rule is declared but not initialized. The parser is constructed as a local variable in lefin.cpp, so rule can be indeterminate if parsing fails before setCutSpacing() runs, making the 'rule != nullptr' check undefined behavior and potentially calling destroy() on garbage. Initialize rule to nullptr (and/or set rule=nullptr at the start of parse()).

class lefTechLayerCutSpacingParser
{
 public:
  odb::dbTechLayerCutSpacingRule* rule;
  bool parse(const std::string&,
             dbTechLayer*,
             lefinReader*,
             std::vector<std::pair<odb::dbObject*, std::string>>&);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-09T00:56:25Z

src/odb/src/lefin/lefTechLayerCutSpacingParser.cpp

-  parser->curRule->setOrthogonalSpacingValid(true);
-  parser->curRule->setOrthogonalSpacingValid(lefinReader->dbdist(value));
+  parser->rule->setOrthogonalSpacingValid(true);
+  parser->rule->setOrthogonalSpacingValid(lefinReader->dbdist(value));


In setOrthogonalSpacing(), the second call mistakenly uses setOrthogonalSpacingValid(dbdist(value)). That sets the validity flag to a non-boolean and never assigns the orthogonal spacing value. Replace the second call with setOrthogonalSpacing(lefinReader->dbdist(value)) so the parsed value is stored correctly.

Suggested change

parser->rule->setOrthogonalSpacingValid(lefinReader->dbdist(value));

parser->rule->setOrthogonalSpacing(lefinReader->dbdist(value));

Copilot · 2026-04-09T00:56:25Z

src/odb/src/lefin/lefLayerPropParser.h


 private:
-  odb::dbTechLayerMinStepRule* curRule;
+  odb::dbTechLayerMinStepRule* rule;


lefTechLayerMinStepParser::rule is declared but not initialized. Since lefin.cpp instantiates lefTechLayerMinStepParser as a local variable, rule will be indeterminate if parse() fails before createSubRule() runs, making the 'rule != nullptr' check undefined behavior. Initialize rule to nullptr (and/or set rule=nullptr at the start of parse()).

Suggested change

odb::dbTechLayerMinStepRule* rule;

odb::dbTechLayerMinStepRule* rule = nullptr;

gemini-code-assist

Code Review

This pull request refactors several LEF parser files by renaming the curRule member variable to rule and updating various local variables to follow snake_case naming conventions. A logic error was identified in lefTechLayerCutSpacingParser.cpp within the setOrthogonalSpacing function, where setOrthogonalSpacingValid was incorrectly called twice instead of setting the actual spacing value.

gemini-code-assist · 2026-04-09T00:59:13Z

src/odb/src/lefin/lefTechLayerCutSpacingParser.cpp

-  parser->curRule->setOrthogonalSpacingValid(true);
-  parser->curRule->setOrthogonalSpacingValid(lefinReader->dbdist(value));
+  parser->rule->setOrthogonalSpacingValid(true);
+  parser->rule->setOrthogonalSpacingValid(lefinReader->dbdist(value));


There is a logic error here. The second call should be setOrthogonalSpacing to set the actual distance value, rather than calling setOrthogonalSpacingValid again with an integer cast to boolean.

Suggested change

parser->rule->setOrthogonalSpacingValid(lefinReader->dbdist(value));

parser->rule->setOrthogonalSpacing(lefinReader->dbdist(value));

Two bugs flagged during review of The-OpenROAD-Project#10091 on lines touched by that PR's rename: 1. setOrthogonalSpacing() was calling setOrthogonalSpacingValid() twice instead of calling setOrthogonalSpacing() on the second call. The result was that the validity flag was set (via implicit int->bool conversion) but the actual orthogonal spacing distance was never stored. Fix: call setOrthogonalSpacing() with the parsed value as intended. 2. lefTechLayerMinStepParser::rule and lefTechLayerCutSpacingParser::rule were declared without initialization. Both parse() paths have a 'if (!valid && rule != nullptr)' check that is undefined behavior if parse() fails before the member is ever assigned (which happens when the input doesn't match any of the grammar's entry keywords). Fix: default-initialize both to nullptr in the header, matching the pattern already used by lefTechLayerCutSpacingTableParser. Both issues were flagged by Copilot and Gemini Code Assist on the PR review. They pre-date this branch but sit on lines that the rename touched, so addressing them here keeps the review loop short. Signed-off-by: anh nguyen <areporeporepo@users.noreply.github.com>

gemini-code-assist · 2026-04-09T01:25:27Z

Thanks for the update and for addressing those pre-existing issues in a separate commit. That approach is definitely preferred for keeping the history clean and bisectable. The fixes look correct and align with the existing patterns in the codebase.

areporeporepo · 2026-04-09T01:27:25Z

/gemini review

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-09T01:30:15Z

src/odb/src/lefin/lefTechLayerCutSpacingTableParser.cpp

    odb::lefinReader* lefinReader)
 {
-  odb::dbTechLayerCutSpacingTableOrthRule* rule
+  odb::dbTechLayerCutSpacingTableOrthRule* orth_rule


Minor typo in identifier: this helper is named createOrthongonalSubRule ("Orthongonal"), which is likely meant to be "Orthogonal". Since this file is already being touched for naming cleanup, consider renaming it (and its call site) to avoid propagating the misspelling.

Flagged by Copilot on re-review of The-OpenROAD-Project#10091. The helper function was named createOrthongonalSubRule with a transposed 'n' ("Orthongonal"). Rename to createOrthogonalSubRule, including the single call site in the same file. Since this file is already being touched for naming cleanup, fix the typo in the same PR to avoid propagating it. Signed-off-by: anh nguyen <areporeporepo@users.noreply.github.com>

gemini-code-assist

Code Review

This pull request performs a mechanical refactoring across lefTechLayerMinStepParser, lefTechLayerCutSpacingParser, and lefTechLayerCutSpacingTableParser classes. The primary change involves renaming the curRule member variable to rule and updating all its references for consistency. Review feedback suggests further improving naming consistency by renaming cut_class to cut_class_rule and cutclass to cut_class in lefTechLayerCutSpacingParser.cpp for better clarity and adherence to snake_case. Additionally, one specific change in lefTechLayerCutSpacingParser.cpp (lines 306-307) was identified as a bug fix, where setOrthogonalSpacingValid was incorrectly called with a distance value, now corrected to setOrthogonalSpacing. This functional change should be reflected in the pull request description, as it contradicts the stated 'pure mechanical refactor' nature of the PR.

gemini-code-assist · 2026-04-09T01:45:58Z

src/odb/src/lefin/lefTechLayerCutSpacingParser.cpp

+    auto cut_class = layer->findTechLayerCutClassRule(cut_class_name.c_str());
+    if (cut_class != nullptr) {
+      parser->rule->setCutClass(cut_class);
    }


For clarity and consistency, please rename the rule object cut_class to cut_class_rule. The name cut_class is ambiguous and could be mistaken for a class name string.

Suggested change

auto cut_class = layer->findTechLayerCutClassRule(cut_class_name.c_str());

if (cut_class != nullptr) {

parser->rule->setCutClass(cut_class);

}

auto cut_class_rule = layer->findTechLayerCutClassRule(cut_class_name.c_str());

if (cut_class_rule != nullptr) {

parser->rule->setCutClass(cut_class_rule);

}

gemini-code-assist · 2026-04-09T01:45:58Z

src/odb/src/lefin/lefTechLayerCutSpacingParser.cpp

+  auto cutclass = at_c<2>(params);
+  auto except_two_edges = at_c<3>(params);
+  auto except_same_via = at_c<4>(params);
+  parser->rule->setWithin(lefinReader->dbdist(within));
+  if (above.is_initialized()) {
+    parser->rule->setAbove(true);
  }
-  if (CUTCLASS.is_initialized()) {
-    const auto& cutClassName = CUTCLASS.value();
-    auto cutClass = layer->findTechLayerCutClassRule(cutClassName.c_str());
-    if (cutClass != nullptr) {
-      parser->curRule->setCutClass(cutClass);
+  if (cutclass.is_initialized()) {
+    const auto& cut_class_name = cutclass.value();
+    auto cut_class = layer->findTechLayerCutClassRule(cut_class_name.c_str());
+    if (cut_class != nullptr) {
+      parser->rule->setCutClass(cut_class);
    }
  }


There are a couple of naming inconsistencies in this block:

cutclass (line 179) is not in snake_case. It should be cut_class to be consistent with the PR's goal.

cut_class (line 188) is a rule object, which is confusingly named. It also creates a name collision if cutclass is renamed.

To resolve this, I suggest renaming cutclass to cut_class and the rule object cut_class to cut_class_rule for clarity.

Suggested change

auto cutclass = at_c<2>(params);

auto except_two_edges = at_c<3>(params);

auto except_same_via = at_c<4>(params);

parser->rule->setWithin(lefinReader->dbdist(within));

if (above.is_initialized()) {

parser->rule->setAbove(true);

}

if (CUTCLASS.is_initialized()) {

const auto& cutClassName = CUTCLASS.value();

auto cutClass = layer->findTechLayerCutClassRule(cutClassName.c_str());

if (cutClass != nullptr) {

parser->curRule->setCutClass(cutClass);

if (cutclass.is_initialized()) {

const auto& cut_class_name = cutclass.value();

auto cut_class = layer->findTechLayerCutClassRule(cut_class_name.c_str());

if (cut_class != nullptr) {

parser->rule->setCutClass(cut_class);

}

}

auto cut_class = at_c<2>(params);

auto except_two_edges = at_c<3>(params);

auto except_same_via = at_c<4>(params);

parser->rule->setWithin(lefinReader->dbdist(within));

if (above.is_initialized()) {

parser->rule->setAbove(true);

}

if (cut_class.is_initialized()) {

const auto& cut_class_name = cut_class.value();

auto cut_class_rule = layer->findTechLayerCutClassRule(cut_class_name.c_str());

if (cut_class_rule != nullptr) {

parser->rule->setCutClass(cut_class_rule);

}

}

gemini-code-assist · 2026-04-09T01:45:58Z

src/odb/src/lefin/lefTechLayerCutSpacingParser.cpp

+  parser->rule->setOrthogonalSpacingValid(true);
+  parser->rule->setOrthogonalSpacing(lefinReader->dbdist(value));


This change fixes a bug where setOrthogonalSpacingValid was called with a distance value instead of a boolean. The new code correctly calls setOrthogonalSpacing. This is a functional change, which contradicts the pull request description that states this is a 'Pure mechanical refactor. No behavior change.' Please consider updating the PR description to reflect this bug fix for clarity.

gemini-code-assist · 2026-04-09T01:45:58Z

src/odb/src/lefin/lefTechLayerCutSpacingParser.cpp

+  auto cut_class = layer->findTechLayerCutClassRule(value.c_str());
+  if (cut_class != nullptr) {
+    parser->rule->setCutClass(cut_class);
  }


For clarity and consistency, please rename the rule object cut_class to cut_class_rule. The name cut_class is ambiguous and could be mistaken for a class name string.

Suggested change

auto cut_class = layer->findTechLayerCutClassRule(value.c_str());

if (cut_class != nullptr) {

parser->rule->setCutClass(cut_class);

}

auto cut_class_rule = layer->findTechLayerCutClassRule(value.c_str());

if (cut_class_rule != nullptr) {

parser->rule->setCutClass(cut_class_rule);

}

gemini-code-assist

Code Review

This pull request refactors several LEF parser classes by renaming the curRule member variable to rule and standardizing local variable names to snake_case. It also includes a bug fix in setOrthogonalSpacing where the correct setter is now called instead of a duplicate validation call, and corrects a typo in the function name createOrthogonalSubRule. I have no feedback to provide.

Gemini Code Assist flagged that the local variable `cut_class` (the pointer returned by findTechLayerCutClassRule) was ambiguous and could be confused with a class name string. Rename it to `cut_class_rule` in all three sites where it appears. Also rename `cutclass` → `cut_class` in addSameMetalSharedEdgeSubRule to follow snake_case consistently. The prior name avoided a collision that is now resolved by the cut_class → cut_class_rule rename. Signed-off-by: anh nguyen <areporeporepo@users.noreply.github.com>

gemini-code-assist · 2026-04-09T15:00:36Z

Thanks for the detailed update, @areporeporepo! It's great to see that the ambiguities with cut_class and cutclass have been resolved by renaming them to cut_class_rule and cut_class respectively. The updated PR description also clearly separates the refactor from the bug fix, which is excellent for maintaining a clean commit history. All the findings have been addressed effectively.

Copilot AI review requested due to automatic review settings April 9, 2026 00:53

github-actions bot added the size/M label Apr 9, 2026

Copilot started reviewing on behalf of areporeporepo April 9, 2026 00:54 View session

github-actions bot reviewed Apr 9, 2026

View reviewed changes

Copilot AI reviewed Apr 9, 2026

View reviewed changes

gemini-code-assist bot reviewed Apr 9, 2026

View reviewed changes

areporeporepo requested a review from Copilot April 9, 2026 01:25

Copilot started reviewing on behalf of areporeporepo April 9, 2026 01:27 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

gemini-code-assist bot reviewed Apr 9, 2026

View reviewed changes

	parser->rule->setOrthogonalSpacingValid(lefinReader->dbdist(value));
	parser->rule->setOrthogonalSpacing(lefinReader->dbdist(value));

	odb::dbTechLayerMinStepRule* rule;
	odb::dbTechLayerMinStepRule* rule = nullptr;

Conversation

areporeporepo commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Notes

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot commented Apr 9, 2026

Uh oh!

areporeporepo commented Apr 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot commented Apr 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

areporeporepo commented Apr 9, 2026 •

edited

Loading