-
Notifications
You must be signed in to change notification settings - Fork 27
Gwf to scs improve #494
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Gwf to scs improve #494
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to e7e2424 in 2 minutes and 38 seconds. Click for details.
- Reviewed
709lines of code in3files - Skipped
0files when reviewing. - Skipped posting
1draft 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. sc-tools/sc-builder/src/sc_scs_writer.cpp:242
- Draft comment:
The nested loop for attribute arc lookup could be optimized for performance if the element set is large. - 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% While the loop could potentially be optimized by using a different data structure, there's no evidence in the code that this is a performance bottleneck. The code already has an early break and the operation only happens for complex arcs. Without profiling data or clear evidence of performance issues, this seems like premature optimization. The comment could be valid if this code processes very large graphs where performance is critical. The current implementation is O(n) for each complex arc. However, the code appears to be part of a file writer/serializer which is likely not in a performance-critical path. The current implementation prioritizes readability and maintainability. The comment should be deleted as it suggests optimization without clear evidence of need, making it speculative rather than actionable.
Workflow ID: wflow_AzBo1HFyETNEoagk
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| std::ofstream output(outputFile, std::ios::binary); | ||
| if (!output.is_open()) | ||
| { | ||
| std::cout << "Error: Cannot open output file `" << outputFile << "`.\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using std::cerr for error messages instead of std::cout.
| std::cout << "Error: Cannot open output file `" << outputFile << "`.\n"; | |
| std::cerr << "Error: Cannot open output file `" << outputFile << "`.\n"; |
| auto link = std::dynamic_pointer_cast<SCgLink>(node); | ||
| if (link && link->GetContentType() != NO_CONTENT) | ||
| { | ||
| std::string contentTypeStr = link->GetContentType(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate contentTypeStr before converting with std::stoi to avoid exceptions from malformed data.
| } | ||
| else if (contentType == 4) // IMAGE | ||
| { | ||
| std::string fileName = link->GetFileName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sanitize fileName to prevent potential directory traversal issues when constructing file paths.
| std::string fileName = link->GetFileName(); | |
| std::string fileName = std::filesystem::path(link->GetFileName()).filename().string(); |
|
|
||
| if (!attrSourceId.empty()) | ||
| { | ||
| std::string connectorSymbol; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider refactoring the connector printing logic into a helper function to reduce code duplication.
| } | ||
| } | ||
|
|
||
| void SCsWriter::Write( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider splitting the lengthy Write() function into smaller helper functions to improve readability and maintainability.
Important
Improves GWF-to-SCs translation with new command-line options and enhanced SCs file output handling in
sc_builder_runner.cppandsc_scs_writer.cpp.--gwf-to-scsand--gwf-outputoptions insc_builder_runner.cppfor translating GWF files to SCs.SCsWriter::Write()insc_scs_writer.cppto handle nodes, arcs, and contours more effectively.PrintHelpMessage()insc_builder_runner.cppupdated with new options for GWF-to-SCs translation.SCsWriter::Write()refactored to improve node and arc processing, including handling of SCgLinks and image content.SCsWriter::CollectNodes()to recursively gather nodes, including those in contours.sc_scg_to_scs_types_converter.hppinsc_scs_writer.cppfor type conversion.SCsWriter::SCgIdentifierCorrectormethods for identifier correction.This description was created by
for e7e2424. You can customize this summary. It will automatically update as commits are pushed.