Fix clktreedelay crash by openroad-ci · Pull Request #9839 · The-OpenROAD-Project/OpenROAD

openroad-ci · 2026-03-19T17:58:18Z

OpenSTA change in https://github.com/The-OpenROAD-Project-private/OpenSTA/pull/23, The-OpenROAD-Project/OpenSTA#313

Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>

gemini-code-assist · 2026-03-19T18:03:59Z

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

github-actions

clang-tidy made some suggestions

github-actions · 2026-03-19T18:05:24Z

src/cts/src/LatencyBalancer.cpp

-                  insDelay = (rise + fall) / 2.0;
+                  insDelay = (rise != 0 && fall != 0)
+                                 ? (rise + fall) / 2.0
+                                 : (rise != 0 ? rise : fall);


warning: conditional operator is used as sub-expression of parent conditional operator, refrain from using nested conditional operators [readability-avoid-nested-conditional-operator]

: (rise != 0 ? rise : fall); ^

Additional context

src/cts/src/LatencyBalancer.cpp:201: parent conditional operator here

insDelay = (rise != 0 && fall != 0) ^

This is a simple ternary operator and no need for another variable to store intermediate results

github-actions · 2026-03-19T18:05:24Z

src/cts/src/LatencyBalancer.cpp

              if (rise != 0 || fall != 0) {
-                insDelay = (rise + fall) / 2.0;
+                insDelay = (rise != 0 && fall != 0) ? (rise + fall) / 2.0
+                                                    : (rise != 0 ? rise : fall);


warning: conditional operator is used as sub-expression of parent conditional operator, refrain from using nested conditional operators [readability-avoid-nested-conditional-operator]

: (rise != 0 ? rise : fall); ^

Additional context

src/cts/src/LatencyBalancer.cpp:345: parent conditional operator here

insDelay = (rise != 0 && fall != 0) ? (rise + fall) / 2.0 ^

github-actions · 2026-03-19T18:05:24Z

src/cts/src/TritonCTS.cpp

-        double delayPerSec = (rise + fall) / 2.0;
+        double delayPerSec = (rise != 0 && fall != 0)
+                                 ? (rise + fall) / 2.0
+                                 : (rise != 0 ? rise : fall);


warning: conditional operator is used as sub-expression of parent conditional operator, refrain from using nested conditional operators [readability-avoid-nested-conditional-operator]

: (rise != 0 ? rise : fall); ^

Additional context

src/cts/src/TritonCTS.cpp:2205: parent conditional operator here

double delayPerSec = (rise != 0 && fall != 0) ^

github-actions · 2026-03-19T18:05:24Z

src/gui/src/clockWidget.cpp

-            ins_delay = (rise + fall) / 2.0;
-          }
+          (rise != 0 && fall != 0) ? (rise + fall) / 2.0
+                                   : (rise != 0 ? rise : fall);


warning: conditional operator is used as sub-expression of parent conditional operator, refrain from using nested conditional operators [readability-avoid-nested-conditional-operator]

: (rise != 0 ? rise : fall); ^

Additional context

src/gui/src/clockWidget.cpp:1340: parent conditional operator here

(rise != 0 && fall != 0) ? (rise + fall) / 2.0 ^

github-actions · 2026-03-19T18:05:24Z

src/gui/src/clockWidget.cpp

-            ins_delay = (rise + fall) / 2.0;
-          }
+          (rise != 0 && fall != 0) ? (rise + fall) / 2.0
+                                   : (rise != 0 ? rise : fall);


warning: expression result unused [clang-diagnostic-unused-value]

: (rise != 0 ? rise : fall); ^

Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>

dsengupta0628 · 2026-03-19T18:23:34Z

/gemini review

github-actions

clang-tidy made some suggestions

github-actions · 2026-03-19T18:23:56Z

src/gui/src/clockWidget.cpp

-            ins_delay = (rise + fall) / 2.0;
-          }
+          ins_delay = (rise != 0 && fall != 0) ? (rise + fall) / 2.0
+                                               : (rise != 0 ? rise : fall);


warning: conditional operator is used as sub-expression of parent conditional operator, refrain from using nested conditional operators [readability-avoid-nested-conditional-operator]

: (rise != 0 ? rise : fall); ^

Additional context

src/gui/src/clockWidget.cpp:1340: parent conditional operator here

ins_delay = (rise != 0 && fall != 0) ? (rise + fall) / 2.0 ^

gemini-code-assist

Code Review

The pull request consistently updates the calculation of insertion delays across multiple files. The change introduces a ternary operator to handle cases where either rise or fall delay might be zero, ensuring that if only one is non-zero, that value is used directly instead of averaging with zero. If both are non-zero, they are averaged. This makes the delay calculation more robust and explicit, preventing potential misinterpretations of delay values when one component is absent. The change also improves code conciseness in clockWidget.cpp by removing a redundant if condition.

gemini-code-assist · 2026-03-19T18:27:20Z

src/cts/src/LatencyBalancer.cpp

+                  insDelay = (rise != 0 && fall != 0)
+                                 ? (rise + fall) / 2.0
+                                 : (rise != 0 ? rise : fall);


The updated logic for insDelay correctly handles scenarios where either rise or fall might be zero. This prevents averaging with a zero value when only one valid delay is present, making the calculation more robust and accurate.

gemini-code-assist · 2026-03-19T18:27:20Z

src/cts/src/LatencyBalancer.cpp

+                insDelay = (rise != 0 && fall != 0) ? (rise + fall) / 2.0
+                                                    : (rise != 0 ? rise : fall);


This change improves the robustness of the insDelay calculation by explicitly handling cases where rise or fall might be zero. If only one is non-zero, that value is used directly, which is a more accurate representation than averaging with zero.

gemini-code-assist · 2026-03-19T18:27:20Z

src/cts/src/TritonCTS.cpp

+        double delayPerSec = (rise != 0 && fall != 0)
+                                 ? (rise + fall) / 2.0
+                                 : (rise != 0 ? rise : fall);


The modification to delayPerSec calculation ensures that if only one of rise or fall is non-zero, that specific value is used. This is a good improvement for correctness and clarity, as it avoids diluting a valid delay with a zero value.

gemini-code-assist · 2026-03-19T18:27:21Z

src/gui/src/clockWidget.cpp

+          ins_delay = (rise != 0 && fall != 0) ? (rise + fall) / 2.0
+                                               : (rise != 0 ? rise : fall);


This change streamlines the ins_delay calculation by directly using the ternary operator, removing the redundant if condition. The logic remains correct, as ins_delay will be 0.0 if both rise and fall are zero, and will use the single non-zero value if only one is present, or their average if both are non-zero. This improves code conciseness and readability.

Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>

github-actions

clang-tidy made some suggestions

github-actions · 2026-03-19T18:48:31Z

src/cts/src/LatencyBalancer.cpp

+                  insDelay
+                      = (rise != 0 && fall != 0)
+                            ? (rise + fall) / 2.0
+                            : (rise != 0


warning: conditional operator is used as sub-expression of parent conditional operator, refrain from using nested conditional operators [readability-avoid-nested-conditional-operator]

: (rise != 0 ^

Additional context

src/cts/src/LatencyBalancer.cpp:202: parent conditional operator here

= (rise != 0 && fall != 0) ^

github-actions · 2026-03-19T18:48:32Z

src/cts/src/LatencyBalancer.cpp

+                insDelay
+                    = (rise != 0 && fall != 0)
+                          ? (rise + fall) / 2.0
+                          : (rise != 0


warning: conditional operator is used as sub-expression of parent conditional operator, refrain from using nested conditional operators [readability-avoid-nested-conditional-operator]

: (rise != 0 ^

Additional context

src/cts/src/LatencyBalancer.cpp:349: parent conditional operator here

= (rise != 0 && fall != 0) ^

github-actions · 2026-03-19T18:48:32Z

src/cts/src/TritonCTS.cpp

+        double delayPerSec
+            = (rise != 0 && fall != 0)
+                  ? (rise + fall) / 2.0
+                  : (rise != 0


warning: conditional operator is used as sub-expression of parent conditional operator, refrain from using nested conditional operators [readability-avoid-nested-conditional-operator]

: (rise != 0 ^

Additional context

src/cts/src/TritonCTS.cpp:2206: parent conditional operator here

= (rise != 0 && fall != 0) ^

github-actions · 2026-03-19T18:48:32Z

src/gui/src/clockWidget.cpp

+          ins_delay
+              = (rise != 0 && fall != 0)
+                    ? (rise + fall) / 2.0
+                    : (rise != 0


warning: conditional operator is used as sub-expression of parent conditional operator, refrain from using nested conditional operators [readability-avoid-nested-conditional-operator]

: (rise != 0 ^

Additional context

src/gui/src/clockWidget.cpp:1341: parent conditional operator here

= (rise != 0 && fall != 0) ^

dsengupta0628 · 2026-03-19T18:54:23Z

Secure CI: #3 - OpenROAD-flow-scripts-Private » secure-fix_clktreedelay_crash - Jenkins

maliberty · 2026-03-19T19:10:41Z

Fix rather than NOLINT as it isn't terribly readable.

Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>

github-actions · 2026-03-19T19:33:05Z

clang-tidy review says "All clean, LGTM! 👍"

maliberty

code looks fine

dsengupta0628 added 3 commits March 19, 2026 13:14

Fix clktree visualization crash from gui

180b266

Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>

nullcheck on arc in libertyport::clkTreeDelay and associated callers

0f59482

Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>

update sta with fixed regression

3933aa1

Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>

openroad-ci assigned dsengupta0628 Mar 19, 2026

github-actions bot reviewed Mar 19, 2026

View reviewed changes

lhs ins_delay was missing- fixed now

7b80f59

Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>

dsengupta0628 mentioned this pull request Mar 19, 2026

clkTreeDelay to check arc without assuming it exists The-OpenROAD-Project/OpenSTA#313

Merged

github-actions bot reviewed Mar 19, 2026

View reviewed changes

dsengupta0628 requested review from LucasYuki and maliberty March 19, 2026 18:24

gemini-code-assist bot reviewed Mar 19, 2026

View reviewed changes

ignore readability problem for ternary ops

1d1d963

Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>

github-actions bot reviewed Mar 19, 2026

View reviewed changes

reformatted

d59d9b6

Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>

maliberty approved these changes Mar 20, 2026

View reviewed changes

		insDelay = (rise != 0 && fall != 0) ? (rise + fall) / 2.0
		: (rise != 0 ? rise : fall);

		ins_delay = (rise != 0 && fall != 0) ? (rise + fall) / 2.0
		: (rise != 0 ? rise : fall);

Conversation

openroad-ci commented Mar 19, 2026

Uh oh!

gemini-code-assist bot commented Mar 19, 2026

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

dsengupta0628 Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

dsengupta0628 commented Mar 19, 2026

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 19, 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 Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

dsengupta0628 commented Mar 19, 2026

Uh oh!

maliberty commented Mar 19, 2026

Uh oh!

github-actions bot commented Mar 19, 2026

Uh oh!

maliberty left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development