Support for Liberty file defined generated clocks

[stanminlee]

Important

Adds support for Liberty file-defined generated clocks in OpenSTA, including new classes, functions, and tests.

• Behavior:
  • Adds support for Liberty file-defined generated clocks in LibertyReader.cc and Liberty.cc.
  • Introduces GeneratedClock class in GeneratedClock.hh and GeneratedClock.cc to represent generated clocks.
  • Updates Sta class in Sta.cc to handle generated clocks, including functions like updateGeneratedClks() and setUpdateGenclks().
• Functions:
  • Adds makeGeneratedClock() in Liberty.cc to create generated clock objects.
  • Implements createLibertyGeneratedClocks() in Sdc.cc to create generated clocks from Liberty definitions.
  • Modifies VerilogReader.cc to map generated clock pins to Liberty cells.
• Tests:
  • Adds test files generated_clock.lib, generated_clock.tcl, generated_clock.v, and generated_clock.ok to verify generated clock functionality.
  • Updates regression_vars.tcl to include generated_clock test.

^{This description was created by for 2369c13. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed bfb008a in 34 seconds. Click for details.

• Reviewed 8 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. test/sdc_strip_escaped_bus.ok:1

• Draft comment:
  New error message 'No liberty libraries found.' is added. Confirm this output is intentional and aligns with the updated error handling for liberty clocks.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to confirm their intention regarding the new error message. This violates the rule against asking for confirmation of intention. The comment does not provide a specific code suggestion or ask for a specific test to be written.

Workflow ID: wflow_HPZKCjY0LQt3ZGVX

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[akashlevy]

Remove GeneratedClock and use Clock.cc's class instead

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed d4fd47d in 40 seconds. Click for details.

• Reviewed 23 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 3 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. test/generated_clock.v:1

• Draft comment:
  Overall, the hierarchical clock generation structure is clear and well‐organized. The instantiations of second_hierarchy, CLK_GEN, and CLK_EDGE_SHIFT look correct.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. test/generated_clock.v:1

• Draft comment:
  Consider adding a timescale directive at the top of the file (e.g. \timescale 1ns/1ps`) to ensure simulation consistency.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

3. test/generated_clock.v:30

• Draft comment:
  Ensure the modules CLK_EDGE_SHIFT and CLK_GEN are defined in your library, as these instantiations assume their correct interface.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_fuMLpM8x760n54CK

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 96687a6 in 39 seconds. Click for details.

• Reviewed 35 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. test/sdc_strip_escaped_bus.ok:1

• Draft comment:
  The expected error output for missing pins 'a' and 'y' looks correct. Ensure that these messages remain in sync with the changes for Liberty‐defined generated clocks.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_Mvddrb9ypBMDSCTP

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 95152c7 in 1 minute and 6 seconds. Click for details.

• Reviewed 21 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 3 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. Sdc.cc:5000

• Draft comment:
  Duplicate clear() calls in deleteExceptions().
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. Sdc.cc:1157

• Draft comment:
  Ensure Liberty‐defined generated clocks are well documented and fully exercised.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

3. Sdc.cc:0

• Draft comment:
  Consider using RAII/smart pointers to manage dynamic memory and reduce manual new/delete risks.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_THlYdrHRu6T2WVTX

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 8d21fbc in 1 minute and 13 seconds. Click for details.

• Reviewed 11 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. include/sta/GeneratedClock.hh:46

• Draft comment:
  Ensure the file ends with a newline for POSIX compliance.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

Workflow ID: wflow_bm94YwAsyOe8mm31

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 0caee5c in 30 seconds. Click for details.

• Reviewed 15 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. include/sta/Liberty.hh:525

• Draft comment:
  Removed extraneous blank lines after addGeneratedClock declaration. Ensure this matches the project's formatting guidelines.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_7g9jVgB7vEGgC44h

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 11d470d in 1 minute and 6 seconds. Click for details.

• Reviewed 40 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 3 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. liberty/Liberty.cc:1525

• Draft comment:
  In makeGeneratedClock, consider using RAII (e.g. std::unique_ptr) for edges_copy and edge_shifts_copy to avoid potential memory leaks if the GeneratedClock constructor throws.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. liberty/Liberty.cc:1155

• Draft comment:
  isClockGateLatchNegedge returns latch_posedge; likely a copy‐paste error and should compare to latch_negedge.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. liberty/Liberty.cc:1513

• Draft comment:
  Consider marking the 'edges' and 'edge_shifts' parameters in makeGeneratedClock as const pointers if they are not modified, to improve const correctness.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_6l4ki1J7njCbBNLQ

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed f961e80 in 4 minutes and 34 seconds. Click for details.

• Reviewed 138 lines of code in 4 files
• Skipped 0 files when reviewing.
• Skipped posting 23 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. verilog/VerilogReader.cc:142

• Draft comment:
  The constructor sets a lambda as the link function. Ensure that capturing 'this' is safe in all contexts, especially if multi-threaded use is added in the future.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. verilog/VerilogReader.cc:154

• Draft comment:
  Verify that stdstrPrint's "%llu" format matches the type returned by std::numeric_limits::max() on all target platforms.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. verilog/VerilogReader.cc:190

• Draft comment:
  Document or centralize the convention for obtaining and creating library_ via network_->findLibrary("verilog").
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. verilog/VerilogReader.cc:245

• Draft comment:
  Using raw pointer deletion for cells and modules may lead to errors; consider using smart pointers or RAII wrappers for better memory safety.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. verilog/VerilogReader.cc:267

• Draft comment:
  Passing dynamically allocated strings as const string* is non-idiomatic; use std::string by value or const reference to avoid manual memory management.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. verilog/VerilogReader.cc:1462

• Draft comment:
  In parseConstant10(), consider omitting the 'end' argument to std::stoull for clarity.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

7. verilog/VerilogReader.cc:1477

• Draft comment:
  Ensure that the string-based comparison for constant10_max works correctly for numeric values; consider zero-padding if needed.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

8. verilog/VerilogReader.cc:811

• Draft comment:
  The use of dynamic_cast assumes correct types; add null-checks or static assertions to guard against unexpected types.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

9. verilog/VerilogReader.cc:709

• Draft comment:
  Consider replacing macros like printClassMemory and printStringMemory with inline functions for improved type-safety and debuggability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

10. verilog/VerilogReader.cc:759

• Draft comment:
  Ensure that stringPrintArgs used in error() and warn() is implemented securely to prevent buffer overruns.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

11. verilog/VerilogReader.cc:887

• Draft comment:
  The frequent use of new/delete for object management suggests modernizing the code with smart pointers to enhance exception safety and avoid memory leaks.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

12. verilog/VerilogReader.cc:1120

• Draft comment:
  Verify that helper functions like stringPrintTmp use thread-local storage if the code is used in a multi-threaded environment.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

13. verilog/VerilogReader.cc:2180

• Draft comment:
  The linking method (makeModuleInstBody and makeModuleInstNetwork) is complex; consider refactoring into smaller, modular helper functions.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

14. verilog/VerilogReader.cc:409

• Draft comment:
  Consolidate parsing and conversion functions like netVerilogToSta into a separate utility module for improved clarity.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

15. verilog/VerilogReader.cc:1450

• Draft comment:
  Consider passing std::string by value or const reference instead of using pointers to avoid manual deletion.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

16. verilog/VerilogReader.cc:2200

• Draft comment:
  Review attribute statement handling to ensure duplicate attributes are managed correctly when setting cell attributes.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

17. verilog/VerilogReader.cc:4230

• Draft comment:
  When a module is not found, a black box is created with a warning. Confirm that this behavior meets user expectations for black box instantiation.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

18. verilog/VerilogReader.cc:4540

• Draft comment:
  Ensure that the warning in checkModuleDcls() for module declarations not in the port list is appropriate and does not generate false positives.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

19. verilog/VerilogReader.cc:520

• Draft comment:
  Verify that hierarchyLevel() reliably computes the intended level for merging nets and that the merge order is deterministic.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

20. verilog/VerilogReader.cc:2250

• Draft comment:
  In reportStmtCounts(), clarify the units (MB) and specify whether counts refer to allocated objects or actual memory usage.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

21. verilog/VerilogReader.cc:2270

• Draft comment:
  The file covers multiple responsibilities. Consider splitting it into smaller modules for parsing, conversion, linking, and error reporting to improve maintainability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

22. include/sta/Network.hh:509

• Draft comment:
  Typographical note: The comment refers to "liberty cell" (singular) but the variable name implies mapping to multiple cells. Consider changing it to "liberty cells" for consistency.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a very minor stylistic comment about wording in a code comment. The rules state "Do NOT make comments that are obvious or unimportant." This seems to fall into the "unimportant" category - it's a trivial wording choice that doesn't affect code functionality or clarity in any meaningful way. The comment is technically about a change (the new comment was added in the diff), but it's extremely pedantic. Both "cell" and "cells" could be considered correct depending on perspective - "cell" emphasizes that each pin maps to one cell, while "cells" emphasizes that the map contains multiple cells overall. This is not a clear code quality issue that needs to be addressed. While the comment is technically about new code in the diff, it's possible that having consistent plurality in documentation could improve readability slightly. Some teams do care about such details in their documentation standards. Even if some teams care about documentation standards, this is such a minor point that it doesn't meet the threshold of being "clearly a code change required." The existing wording is not wrong or confusing - both interpretations are valid. This falls squarely into the "obvious or unimportant" category that should not be commented on. This comment is too pedantic and unimportant to keep. It's a trivial wording preference that doesn't affect code quality or clarity in any meaningful way. The comment should be deleted.

23. sdc/Sdc.cc:1044

• Draft comment:
  Typographical error: In the comment, it refers to generated_clock_pins_to_cells_ (with an extra underscore) whereas the actual variable is generated_clock_pins_to_cells. Please remove the extra underscore in the comment for clarity.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This is a comment about a typo in a code comment (not in actual code). The automated tool is correct - there is indeed an extra underscore in the comment that doesn't match the variable name. However, I need to consider: 1) Is this a code change that was made in the diff? Looking at the diff, line 1044 is indeed part of the changes (it's in the new code). 2) Is this comment useful? It's pointing out a minor inconsistency in documentation/comments. 3) Does it violate any rules? The rules say not to make comments that are "obvious or unimportant" - a typo in a comment could be considered unimportant. However, it's also a legitimate issue that could cause confusion when reading the code. The comment provides a clear suggestion to fix it. This is a very minor issue - just a typo in a comment, not in actual code. It doesn't affect functionality at all. The rules explicitly state "Do NOT make comments that are obvious or unimportant" - this could be considered unimportant since it's just documentation. Also, while the line is in the diff, it's possible the typo existed before and wasn't introduced by this change. While it's minor, having consistent variable names in comments is good practice and helps code readability. The comment is actionable and clear. However, looking more carefully at the rules, they emphasize not making comments unless there's "clearly a code change required" - fixing a typo in a comment is borderline. The most important rule states I need "STRONG EVIDENCE that the comment is correct" to keep it - and while the comment is technically correct, the importance is questionable. This comment correctly identifies a typo in a code comment, but it's a very minor documentation issue that doesn't affect functionality. Given the rules emphasize not making "obvious or unimportant" comments and requiring strong evidence of importance, this falls into a gray area. It's technically correct but arguably unimportant.

Workflow ID: wflow_8c4GVZvdXXAtPu9c

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 54633ac in 1 minute and 59 seconds. Click for details.

• Reviewed 110 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 2 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. test/generated_clock.ok:1

• Draft comment:
  Output matches expected generated clock behavior. The warning message and clock periods (as well as the final clock table) are consistent with design intent.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. test/generated_clock.lib:57

• Draft comment:
  Typo: The statement on this line is missing a trailing semicolon. It should likely read shifts (0, 0, 0); to be consistent with the surrounding statements.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment is technically correct - there is a missing semicolon on line 57. However, the key question is whether this is about the changes made in the PR. The diff shows the PR only changed the values inside the parentheses from (0.5, 0.5, 0.5) to (0, 0, 0). The missing semicolon was already present in the original code before this PR. According to the rules, I should "Do NOT comment on anything that would be obviously caught by the build" and "only keep comments that have to do with something that was changed in this diff." Since the missing semicolon was pre-existing and not introduced by this PR, this comment is not about the changes made. The comment might still be valuable because it identifies a real syntax error that should be fixed. Even if the PR author didn't introduce the error, they are touching this line and could fix it while they're there. This could be seen as a helpful suggestion for improvement. While the comment identifies a real issue, the rules explicitly state to only comment on changes made in the diff. The missing semicolon existed before this PR and was not introduced by the changes. The PR author only changed the numeric values, not the syntax. This is a pre-existing issue, not something introduced by the current changes. This comment should be deleted because it's about a pre-existing syntax error that was not introduced or modified by the PR changes. The diff only shows a change to the numeric values, not to the semicolon.

Workflow ID: wflow_bxJWcA7VSubujdtN

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed f19d113 in 1 minute and 32 seconds. Click for details.

• Reviewed 36 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 2 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. include/sta/Network.hh:448

• Draft comment:
  Removed clearGeneratedClockPinToCell() is fine since clear() now resets the map, but adding a brief comment on ownership/lifetime of the 'pinName' key would be helpful.
• Reason this comment was not posted:
  Comment was on unchanged code.

2. network/Network.cc:1278

• Draft comment:
  The addGeneratedClockPinToCell() implementation simply inserts the key-value pair. Consider adding a null check for 'pinName' and documenting the expected ownership and lifetime of the passed-in string.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_fcX4QMx6MnsKIRA9

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed de8f6d7 in 1 minute and 55 seconds. Click for details.

• Reviewed 36 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 7 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. verilog/VerilogReader.cc:2125

• Draft comment:
  Consider handling mismatched LHS/RHS net sizes more robustly in mergeAssignNet. Currently a warning is issued if sizes differ; clarify intended behavior or add error handling.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. verilog/VerilogReader.cc:1390

• Draft comment:
  Add exception handling to std::stol and std::stoull in constant parsing to catch conversion errors.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. verilog/VerilogReader.cc:2208

• Draft comment:
  Convert net name comparisons to use std::string to avoid potential pointer-comparison pitfalls.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. verilog/VerilogReader.cc:57

• Draft comment:
  Consider modernizing manual memory management by using smart pointers to improve exception safety & maintainability.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

5. verilog/VerilogReader.cc:864

• Draft comment:
  Ensure dynamic casts (e.g., in checkInstanceName) are checked for null to avoid runtime errors.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. verilog/VerilogReader.cc:708

• Draft comment:
  Review macro definitions for memory reporting to ensure they accurately capture memory usage, considering padding and dynamic sizes.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

7. verilog/VerilogReader.cc:1530

• Draft comment:
  Evaluate replacing custom iterator implementations with standard STL iterators to simplify the code.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_hhScFfDRPeX8OLhc

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 935d800 in 2 minutes and 4 seconds. Click for details.

• Reviewed 87 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 6 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. Sdc.cc:5007

• Draft comment:
  Duplicate clear() calls on first_thru_edge_exceptions_. It’s invoked twice in succession. Remove the redundant call.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. Sdc.cc:5900

• Draft comment:
  NetWireCaps initializes subtract_pin_cap_ with a fixed size of 2. Ensure that min_max->index() always falls within [0,1] or consider using a dynamic container or bounds checking.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. Sdc.cc:5200

• Draft comment:
  There is extensive manual memory management (raw pointers, manual delete). Consider using smart pointers or modern C++ containers to improve exception-safety and reduce leaks.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. Sdc.cc:4450

• Draft comment:
  The exception merging and expansion logic is complex and lengthy. Consider refactoring these functions into smaller, well‐documented units and add thorough unit tests to ensure correctness.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. Sdc.cc:5840

• Draft comment:
  Functions findLeafLoadPins() and findLeafDriverPins() embed non‐trivial logic for hierarchical pins. Verify that the conditions (is input/outside vs. output/inside) correctly capture the intended semantics and consider adding additional inline comments or assertions.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. sdc/Sdc.cc:1044

• Draft comment:
  Typo: There should be a space between "(master clock pins)" and "will". Consider updating the comment to: "The keys of generated_clock_pins_to_cells (master clock pins) will be searched in the current clock network".
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% This is a comment about a typo in a code comment. According to the rules, I should not comment on things that are "obvious or unimportant". A missing space in a comment is a very minor issue - it doesn't affect functionality, and the comment is still perfectly readable. The rules also say "Do NOT comment unless there is clearly a code change required" - while technically this is a code change (fixing the comment), it's an extremely trivial one. The rules emphasize not making comments that are "obvious or unimportant", and a spacing typo in a comment falls into this category. This is the kind of thing that could be caught by a linter or spell checker, but doesn't warrant a PR comment from a reviewer. However, one could argue that maintaining clean, properly formatted comments is part of code quality, and fixing typos improves readability and professionalism. The comment does identify a real issue, even if minor. While maintaining clean comments is good practice, the rules explicitly state to avoid "obvious or unimportant" comments. A single missing space in a comment is both obvious (once pointed out) and unimportant (doesn't affect functionality or understanding). This is the type of nitpick that clutters PR reviews and distracts from more substantive issues. This comment should be deleted. It's pointing out a trivial typo (missing space) in a code comment that doesn't affect functionality or readability in any meaningful way. This falls under "obvious or unimportant" comments that should be avoided.

Workflow ID: wflow_N3lSVQNmZJdbqciK

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[akashlevy]

Looking mostly good! Can you address my minor nitpicks and also answer the question I had on the HACK?

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 2369c13 in 1 minute and 27 seconds. Click for details.

• Reviewed 144 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 5 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. verilog/VerilogReader.cc:2209

• Draft comment:
  Comparison of net_name with zero/one net names relies on implicit string conversion. Consider explicitly converting net_name to std::string for clarity.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

2. verilog/VerilogReader.cc:1420

• Draft comment:
  Extensive use of raw pointers and manual memory handling; consider modernizing to smart pointers to improve safety and exception resilience.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

3. verilog/VerilogReader.cc:1460

• Draft comment:
  Parsing base-10 constants using std::stoull may throw exceptions on invalid input; consider adding error handling or documentation for this behavior.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

4. verilog/VerilogReader.cc:4450

• Draft comment:
  The exception merging logic is complex; consider adding more inline comments to clarify the merging and expansion of exceptions.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

5. verilog/VerilogReader.cc:2030

• Draft comment:
  Reinterpret_cast from LibertyPort to Port is used in several places. Ensure these casts are safe and document why the types are compatible.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

Workflow ID: wflow_qj5PLJYaXPmKnsTu

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}