fix: libext broken in verific compatibility mode#4
Conversation
0015b9a to
35ff13c
Compare
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 35ff13c in 1 minute and 50 seconds. Click for details.
- Reviewed
171lines of code in5files - Skipped
0files when reviewing. - Skipped posting
2draft 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. .github/workflows/test.yml:23
- Draft comment:
Typographical error: 'comamnd' should be 'command'. - Reason this comment was not posted:
Marked as duplicate.
2. .github/workflows/test.yml:35
- Draft comment:
Typographical error: 'buld' should be 'build'. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_0ED0ipC5CCJR89eA
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
ef1e62f to
6c7ef2c
Compare
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 6c7ef2c in 1 minute and 55 seconds. Click for details.
- Reviewed
167lines of code in5files - Skipped
0files when reviewing. - Skipped posting
5draft 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. .github/workflows/test.yml:19
- Draft comment:
CI workflow is clear and effective. Consider caching build dependencies (e.g. Ninja) to speed up repeated runs. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. src/driver.cc:447
- Draft comment:
In verific compatibility mode, the flag for module search extensions is now generated by iterating over getSearchExtensions(). Ensure that omitting a flag when the list is empty is intended compared to the previous default '+libext+' insertion. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. test/run_test:24
- Draft comment:
The new test logic correctly removes the cache file and selects the input file based on existence. Ensure that this behavior (using '-f list.f' or falling back to 'test.v') is as intended for all scenarios. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. test/run_test:48
- Draft comment:
The caching test using sed and cmp is clever; however, ensure it remains robust against any extra whitespace (e.g. from the command file flag formatting). - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is purely informative and suggests ensuring robustness against whitespace, which is not specific enough to be actionable. It doesn't provide a specific code suggestion or ask for a specific test to be written.
5. test/CMakeLists.txt:11
- Draft comment:
Good addition of the 'test_no_list' entry to the test suite. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_NsCZ2ibABzunUxXo
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
a441113 to
65b5d34
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed a441113 in 1 minute and 47 seconds. Click for details.
- Reviewed
166lines of code in5files - Skipped
0files when reviewing. - Skipped posting
4draft 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. .github/workflows/test.yml:2
- Draft comment:
Consider restricting the 'push' trigger with branch filters to avoid unnecessary CI runs. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. src/driver.cc:448
- Draft comment:
The new logic for module search extensions in verific mode now iterates over getSearchExtensions(). This may change behavior when the list is empty compared to always inserting '+libext+'. Confirm if this behavior is intended. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. test/run_test:24
- Draft comment:
For robustness, quote variables in shell commands to prevent word-splitting. For example, use "rm -f "$bindir/$test.cache.json"". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Quoting variables in shell scripts is generally good practice to prevent word-splitting issues. However, in this specific case: 1) This is a test script 2) $bindir and $test are likely to be simple values without spaces 3) The script runs with set -e and set -x which would catch obvious issues 4) No evidence of actual problems has been shown. While shell variable quoting is good practice, this seems like an overly pedantic suggestion for a test script where the variables are unlikely to contain spaces or special characters. Even though it's technically correct advice, we should focus on more impactful issues rather than theoretical edge cases in test infrastructure. Delete the comment as it suggests a theoretical best practice without clear evidence of an actual problem, and the change would have minimal real impact.
4. .github/workflows/test.yml:18
- Draft comment:
Typo noticed in the comment on line 18: "slang CMakeLists.txt fetch the rest." It appears that "slang" might be a typo (perhaps intended to be 'since' or 'as') for clarity. Consider revising this comment. - 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% 1. This is a comment about a comment, not about actual code logic. 2. "slang" might actually be intentional - it could be referring to a project or tool named "slang". 3. Without more context about the project, we can't be sure if this is actually a typo. 4. Even if it is a typo, fixing comment text is not a critical change that needs to be made. I might be missing that clear documentation is important for maintainability, and typos in comments could confuse future readers. While clear documentation is important, we don't have enough context to know if this is actually a typo, and the comment is still understandable either way. Delete this comment. It's about comment text rather than code logic, and we can't be certain it's even a typo without more context.
Workflow ID: wflow_6exobcfXVBkBRIs4
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 65b5d34 in 1 minute and 53 seconds. Click for details.
- Reviewed
166lines of code in5files - Skipped
0files when reviewing. - Skipped posting
3draft 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. src/driver.cc:448
- Draft comment:
Behavior change: In verific mode, flags are now generated only for each search extension, unlike before where '+libext+' was unconditionally emitted. Confirm this is intended. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. test/run_test:26
- Draft comment:
For improved robustness, quote shell variables to handle filenames with spaces. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. .github/workflows/test.yml:18
- Draft comment:
Typo found: The comment "slang CMakeLists.txt fetch the rest." appears to have a misspelling. Please check if "slang" should be replaced with "the" or a more appropriate term. - 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% 1. "slang" could actually be the name of a project or tool. 2. Without more context about the project, we can't be sure if "slang" is a typo or an intentional reference. 3. Even if it were a typo, this is just a comment - it doesn't affect functionality. 4. The rules specifically say not to make purely informative comments that don't require code changes. I might be missing some context where "slang" is definitely wrong, or there might be a convention for documentation that makes this important. Even if there were such conventions, the rules explicitly state not to make purely informative comments and to ignore things that don't require actual code changes. This comment should be deleted as it's about documentation only, doesn't affect functionality, and we can't be certain it's even incorrect.
Workflow ID: wflow_N6aTWWodFB322PUe
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
- fix completely broken `+libext+` printing in verific compatibility mode - add no-list test to CI - update run_test to also test caching, and to use verific compatibility mode - add ci
65b5d34 to
c9d5cf2
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed c9d5cf2 in 1 minute and 44 seconds. Click for details.
- Reviewed
166lines of code in5files - Skipped
0files when reviewing. - Skipped posting
5draft 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. .github/workflows/test.yml:2
- Draft comment:
Consider including 'pull_request' triggers in addition to 'push' so that tests run on PR events as well. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. src/driver.cc:448
- Draft comment:
In verific compatibility mode the previous branch unconditionally inserted '+libext+'; now iterating over search extensions may result in no flag if the list is empty. Confirm that this change is intentional. - 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 pointing out a real behavioral change. However, it's phrased as "confirm this is intentional" which violates our rules about not asking for confirmation. The change seems intentional - the old code was inconsistent in how it handled empty lists between modes. The new code is more consistent. If this was a bug, it would likely have been caught in testing. The change could potentially break existing workflows that depend on the '+libext+' flag being present even with no extensions. Without more context about the verific compatibility requirements, we can't be 100% sure this change is safe. While there is a behavioral change, the comment is phrased as a request for confirmation rather than pointing out a clear issue. The new behavior is more consistent and logical. Delete the comment since it violates our rule about not asking for confirmation of intent. If there was a real issue with the new behavior, it should be stated directly rather than as a question.
3. src/driver.cc:470
- Draft comment:
Minor suggestion: Consider using fs::absolute(*output).string() instead of .c_str() in fmt::format for consistency and clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. test/run_test:24
- Draft comment:
Consider quoting variable references (e.g. "$bindir/$test.cache.json") to protect against potential word splitting if paths contain spaces. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. .github/workflows/test.yml:18
- Draft comment:
Typo: The word "slang" in the comment appears to be incorrect. Did you mean "single" or another term? - 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% Looking at the context, "slang" appears to be an intentional reference, likely to a project name or tool called "slang". It's common for projects to reference their own name in comments. The suggestion to change it to "single" doesn't make sense in context - "single CMakeLists.txt fetch the rest" would be grammatically incorrect. This appears to be a false positive from the automated tool. I could be wrong about "slang" being a project name. Maybe it really is a typo for something else. Even if "slang" isn't a project name, the suggested correction to "single" definitely doesn't make sense grammatically or contextually. The comment should be deleted either way. Delete this comment. It appears to be a false positive from spell checking, and the suggested correction would make the text less clear.
Workflow ID: wflow_TGnW7GrmOHl7uaLc
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
+libext+printing in verific compatibility modeImportant
Fix
+libext+printing in Verific compatibility mode and enhance CI with new tests and caching support.+libext+printing in Verific compatibility mode inwrite_output_flags()indriver.cc.test.ymlfor CI with build and test jobs onubuntu-24.04andmacos-15.test_no_listtoCMakeLists.txt.run_testto test caching and use Verific compatibility mode.test_no_list/test.vfor testing without a list file.This description was created by
for c9d5cf2. You can customize this summary. It will automatically update as commits are pushed.