Draft
Conversation
Author
|
@greptile-apps review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+91
to
+114
| template <typename T> std::set<T*> | ||
| process_predicate(const char *property, | ||
| const char *op, | ||
| const char *pattern, | ||
| std::set<T*> &all) | ||
| { | ||
| auto filtered_objects = std::set<T*>(); | ||
| bool exact_match = stringEq(op, "=="); | ||
| bool pattern_match = stringEq(op, "=~"); | ||
| bool not_match = stringEq(op, "!="); | ||
| bool not_pattern_match = stringEq(op, "!~"); | ||
| for (T *object : all) { | ||
| PropertyValue value = Sta::sta()->properties().getProperty(object, property); | ||
| std::string prop_str = value.to_string(Sta::sta()->network()); | ||
| const char *prop = prop_str.c_str(); | ||
| if (prop && | ||
| ((exact_match && stringEq(prop, pattern)) | ||
| || (not_match && !stringEq(prop, pattern)) | ||
| || (pattern_match && patternMatch(pattern, prop)) | ||
| || (not_pattern_match && !patternMatch(pattern, prop)))) | ||
| filtered_objects.insert(object); | ||
| } | ||
| return filtered_objects; | ||
| } |
There was a problem hiding this comment.
missing explanatory comment for this complex filtering function
Context Used: Context from dashboard - Always add comments to explain the purpose of complex functions or code blocks to improve code reada... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+116
to
+249
| template <typename T> Vector<T*> | ||
| filter_objects(const char *filter_expression, | ||
| Vector<T*> *objects, | ||
| bool sta_boolean_props_as_int | ||
| ) { | ||
| Vector<T*> result; | ||
| if (objects) { | ||
| auto all = std::set<T*>(); | ||
| for (auto object: *objects) { | ||
| all.insert(object); | ||
| } | ||
| auto postfix = sta::FilterExpr(filter_expression).postfix(sta_boolean_props_as_int); | ||
| std::stack<std::set<T*>> eval_stack; | ||
| for (auto &pToken: postfix) { | ||
| if (pToken->kind == sta::FilterExpr::Token::Kind::op_or) { | ||
| if (eval_stack.size() < 2) { | ||
| throw sta::FilterError("attempted to run a logical or on less than two predicates"); | ||
| } | ||
| auto arg0 = eval_stack.top(); | ||
| eval_stack.pop(); | ||
| auto arg1 = eval_stack.top(); | ||
| eval_stack.pop(); | ||
| auto union_result = std::set<T*>(); | ||
| std::set_union( | ||
| arg0.cbegin(), arg0.cend(), | ||
| arg1.cbegin(), arg1.cend(), | ||
| std::inserter(union_result, union_result.begin()) | ||
| ); | ||
| eval_stack.push(union_result); | ||
| } else if (pToken->kind == sta::FilterExpr::Token::Kind::op_and) { | ||
| if (eval_stack.size() < 2) { | ||
| throw sta::FilterError("attempted to run a logical and on less than two predicates"); | ||
| } | ||
| auto arg0 = eval_stack.top(); | ||
| eval_stack.pop(); | ||
| auto arg1 = eval_stack.top(); | ||
| eval_stack.pop(); | ||
| auto intersection_result = std::set<T*>(); | ||
| std::set_intersection( | ||
| arg0.cbegin(), arg0.cend(), | ||
| arg1.cbegin(), arg1.cend(), | ||
| std::inserter(intersection_result, intersection_result.begin()) | ||
| ); | ||
| eval_stack.push(intersection_result); | ||
| } else if (pToken->kind == sta::FilterExpr::Token::Kind::op_inv) { | ||
| if (eval_stack.size() < 1) { | ||
| throw sta::FilterError("attempted to run an inversion on no predicates"); | ||
| } | ||
| auto arg0 = eval_stack.top(); | ||
| eval_stack.pop(); | ||
|
|
||
| auto difference_result = std::set<T*>(); | ||
| std::set_difference( | ||
| all.cbegin(), all.cend(), | ||
| arg0.cbegin(), arg0.cend(), | ||
| std::inserter(difference_result, difference_result.begin()) | ||
| ); | ||
| eval_stack.push(difference_result); | ||
| } else if (pToken->kind == sta::FilterExpr::Token::Kind::defined || | ||
| pToken->kind == sta::FilterExpr::Token::Kind::undefined) { | ||
| bool should_be_defined = (pToken->kind == sta::FilterExpr::Token::Kind::defined); | ||
| auto result = std::set<T*>(); | ||
| for (auto object : all) { | ||
| PropertyValue value = Sta::sta()->properties().getProperty(object, pToken->text); | ||
| bool is_defined = false; | ||
| switch (value.type()) { | ||
| case PropertyValue::Type::type_float: | ||
| is_defined = value.floatValue() != 0; | ||
| break; | ||
| case PropertyValue::Type::type_bool: | ||
| is_defined = value.boolValue(); | ||
| break; | ||
| case PropertyValue::Type::type_string: | ||
| case PropertyValue::Type::type_liberty_library: | ||
| case PropertyValue::Type::type_liberty_cell: | ||
| case PropertyValue::Type::type_liberty_port: | ||
| case PropertyValue::Type::type_library: | ||
| case PropertyValue::Type::type_cell: | ||
| case PropertyValue::Type::type_port: | ||
| case PropertyValue::Type::type_instance: | ||
| case PropertyValue::Type::type_pin: | ||
| case PropertyValue::Type::type_net: | ||
| case PropertyValue::Type::type_clk: | ||
| is_defined = value.to_string(Sta::sta()->network()) != ""; | ||
| break; | ||
| case PropertyValue::Type::type_none: | ||
| is_defined = false; | ||
| break; | ||
| case PropertyValue::Type::type_pins: | ||
| is_defined = value.pins()->size() > 0; | ||
| break; | ||
| case PropertyValue::Type::type_clks: | ||
| is_defined = value.clocks()->size() > 0; | ||
| break; | ||
| case PropertyValue::Type::type_paths: | ||
| is_defined = value.paths()->size() > 0; | ||
| break; | ||
| case PropertyValue::Type::type_pwr_activity: | ||
| is_defined = value.pwrActivity().isSet(); | ||
| break; | ||
| } | ||
| if (is_defined == should_be_defined) { | ||
| result.insert(object); | ||
| } | ||
| } | ||
| eval_stack.push(result); | ||
| } else if (pToken->kind == sta::FilterExpr::Token::Kind::predicate) { | ||
| auto predicate_token = std::static_pointer_cast<sta::FilterExpr::PredicateToken>(pToken); | ||
| auto predicate_result = process_predicate<T>(predicate_token->property.c_str(), predicate_token->op.c_str(), predicate_token->arg.c_str(), all); | ||
| eval_stack.push(predicate_result); | ||
| } | ||
| } | ||
| if (eval_stack.size() == 0) { | ||
| throw sta::FilterError("filter expression is empty"); | ||
| } | ||
| if (eval_stack.size() > 1) { | ||
| throw sta::FilterError("filter expression evaluated to multiple sets"); | ||
| } | ||
| auto result_set = eval_stack.top(); | ||
| result.resize(result_set.size()); | ||
| std::copy(result_set.begin(), result_set.end(), result.begin()); | ||
|
|
||
| // Maintain pre-filter ordering | ||
| std::map<T*, int> objects_i; | ||
| for (int i = 0; i < objects->size(); ++i) | ||
| objects_i[objects->at(i)] = i; | ||
|
|
||
| std::sort(result.begin(), result.end(), | ||
| [&](T* a, T* b) { | ||
| return objects_i[a] < objects_i[b]; | ||
| }); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
missing explanatory comment for this complex filter evaluation function that implements postfix expression evaluation with set operations
Context Used: Context from dashboard - Always add comments to explain the purpose of complex functions or code blocks to improve code reada... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Adds new variable ::sta_enable_collections that when enabled, returns the results of collection operations as an opaque pointer in Tcl to avoid conversion overhead. Involved moving as much of the gathering and filtering logic to C++ as possible to C++. Currently confirmed working for get_ports and collection operations on the result of get_ports. Partial work done on get_insts. This commit is not final and will be amended as work on this progresses.
…d paths still return a collection
Author
|
@greptile-apps Re-review. Especially try to flag any inconsistencies in the implementations of |
Greptile SummaryThis PR introduces an opaque collection system controlled by
Confidence Score: 2/5
Important Files Changed
Flowchartflowchart TD
A["Tcl: get_cells/get_pins/get_nets/get_ports/get_clocks"] --> B{"-of_objects?"}
B -->|Yes| C["Tcl: iterate pins/nets/insts"]
C --> D["filter_objs (if -filter)"]
D --> E["copy_collection → return"]
B -->|No| F["C++: find_*_complete()"]
F --> G["PatternMatch + network search"]
G --> H{"filter_expression?"}
H -->|Yes| I["filter_objects<T>() in FilterExpr.hh"]
I --> J["Return CollectionType*"]
H -->|No| J
J --> K{"::sta_enable_collections?"}
K -->|1| L["SWIG_NewInstanceObj → opaque pointer"]
K -->|0| M["seqPtrTclList → Tcl list"]
L --> N["Collections.tcl dispatches via is_collection"]
M --> O["Collections.tcl uses Tcl list ops"]
Last reviewed commit: 06cc7b9 |
Comment on lines
+36
to
+51
| proc foreach_in_collection {variable_name collection body} { | ||
| if {[sta::is_collection $collection]} { | ||
| set it [sta::get_iterator $collection] | ||
| while {[$it has_next]} { | ||
| set current [$it next] | ||
| uplevel 1 set $variable_name $current | ||
| uplevel 1 $body | ||
| } | ||
| $it finish | ||
| } else { | ||
| foreach current $collection { | ||
| uplevel 1 set $variable_name $current | ||
| uplevel 1 $body | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Iterator leak when break/continue used in body
When the body passed to foreach_in_collection uses break (or continue that propagates as an exception), the uplevel 1 $body call on line 42 will cause the break to exit the enclosing while loop. This means $it finish on line 44 is never called, leaking the iterator.
This already happens in practice at line 216-218 of this file, where filter_collection uses break inside foreach_in_collection.
A common Tcl pattern to handle this is to catch the return code from uplevel:
Suggested change
| proc foreach_in_collection {variable_name collection body} { | |
| if {[sta::is_collection $collection]} { | |
| set it [sta::get_iterator $collection] | |
| while {[$it has_next]} { | |
| set current [$it next] | |
| uplevel 1 set $variable_name $current | |
| uplevel 1 $body | |
| } | |
| $it finish | |
| } else { | |
| foreach current $collection { | |
| uplevel 1 set $variable_name $current | |
| uplevel 1 $body | |
| } | |
| } | |
| } | |
| proc foreach_in_collection {variable_name collection body} { | |
| if {[sta::is_collection $collection]} { | |
| set it [sta::get_iterator $collection] | |
| set rc 0 | |
| while {[$it has_next]} { | |
| set current [$it next] | |
| uplevel 1 set $variable_name $current | |
| set rc [catch {uplevel 1 $body} result options] | |
| if {$rc == 1} { | |
| # error - finish iterator, then re-throw | |
| $it finish | |
| return -options $options $result | |
| } elseif {$rc == 3} { | |
| # break | |
| break | |
| } elseif {$rc == 4} { | |
| # continue - do nothing, loop continues | |
| } elseif {$rc != 0} { | |
| $it finish | |
| return -options $options $result | |
| } | |
| } | |
| $it finish | |
| } else { | |
| foreach current $collection { | |
| uplevel 1 set $variable_name $current | |
| uplevel 1 $body | |
| } | |
| } | |
| } |
Comment on lines
+279
to
+286
| if { [sta::is_collection $object] } { | ||
| set it [sta::get_iterator $object] | ||
| while {[$it has_next]} { | ||
| # TODO: StringSeq as collection? | ||
| lappend full_names [$it next] | ||
| } | ||
| $it finish | ||
| return $full_names |
There was a problem hiding this comment.
get_full_name returns raw pointers for collections
When get_full_name is called with a collection, the iterator appends raw SWIG pointer values ([$it next]) instead of calling get_full_name on each element. Compare with the Tcl list path at lines 288-290 which correctly recurses get_full_name on each element.
Suggested change
| if { [sta::is_collection $object] } { | |
| set it [sta::get_iterator $object] | |
| while {[$it has_next]} { | |
| # TODO: StringSeq as collection? | |
| lappend full_names [$it next] | |
| } | |
| $it finish | |
| return $full_names | |
| if { [sta::is_collection $object] } { | |
| set it [sta::get_iterator $object] | |
| while {[$it has_next]} { | |
| lappend full_names [get_full_name [$it next]] | |
| } | |
| $it finish | |
| return $full_names |
Comment on lines
+77
to
+79
| void sort_collection_by_properties(CollectionType *v, StringSeq *property_names, bool descending = false) { | ||
| auto network = Sta::sta()->network(); | ||
| auto properties = Sta::sta()->properties(); |
There was a problem hiding this comment.
Unnecessary expensive copy of Properties object
auto properties = Sta::sta()->properties(); copies the entire Properties object by value, including all internal PropertyRegistry maps. Since properties is only used for calling getProperty and is captured by reference in the lambda anyway, this should be a reference:
Suggested change
| void sort_collection_by_properties(CollectionType *v, StringSeq *property_names, bool descending = false) { | |
| auto network = Sta::sta()->network(); | |
| auto properties = Sta::sta()->properties(); | |
| auto &properties = Sta::sta()->properties(); |
| sta_warn 366 "port '$pattern' not found." | ||
| } | ||
| if { [object_type $pattern] != "Port" } { | ||
| sta_error 333 "object '$pattern' is not an port." |
There was a problem hiding this comment.
Grammar typo: "an port" → "a port"
Suggested change
| sta_error 333 "object '$pattern' is not an port." | |
| sta_error 333 "object '$pattern' is not a port." |
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
1 participant
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.
Adds new variable ::sta_enable_collections that when enabled, returns the results of collection operations as an opaque pointer in Tcl to avoid conversion overhead.
This was done by creating two sets of SWIG macros to handle the Tcl-to-C++ conversions and add helper functions for collections:
Either valid Tcl lists or the opaque pointers to any of
ClockSeq CellSeq PortSeq InstanceSeq PinSeq NetSeq LibertyLibrarySeq LibertyCellSeq LibertyPortSeqcan be used as inputs, and outputs of these types will be converted to a Tcl list if::sta_enable_collectionsis0(default) or left opaque if it's1.A number of helper functions are available for these opaque pointers and their (some newly-defined) iterators.
Some codepaths have been moved to C++ for better efficiency; namely, the non-
-of_objectsvariants of:get_cellsget_netsget_portsget_pinsget_clocksCollections.tcl has been rewritten to use the new helper functions and iterators, while maintaining support for the previous Tcl-style lists. The catch is while the dummy aliases previously used in Collections.tcl supported heterogenous "collections" (as all Tcl lists are heterogeneous), some of the new functions require a homogenous collection to work properly.