Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2c74c2c0c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
One policy for every temporary directory: allocated by the OS
(SpecsIo.createTempDirectory, PR from specs-java-libs), owned by the
lifecycle that created it, deleted when that lifecycle ends (kept in
debug mode for inspection, with the path reported).
- CxxWeaver: rebuild folders are now owned by the weaver instance
instead of a static ThreadLocal<Buffer>, isolating concurrent
weaver instances sharing one thread; the fixed cwd-relative
'__clava_woven_for_file_rebuild' folder is now a per-invocation
temp dir; close() no longer deletes cwd-relative '__clava_woven'
and '__clava_src' folders, which could destroy unrelated user
directories ('__clava_src' was already dead). Regression test
added.
- ParallelCodeParser/ClangAstDumper: drop the 'clava_parsing_<UUID>'
session root and the baseFolder plumbing; each dumper invocation
owns one OS temp folder, deleted after use. Retire the now
redundant CodeParser.CLEAN option.
- AClangAstTester: output folders are OS-allocated instead of
'<nanoTime>-<threadId>' names in the working directory, keeping
debug leftovers out of the repo.
- Cache root unified with Clava-JS: DUMPER_FOLDER now defaults to
the OS user cache folder ('~/.cache/@specs-feup/clava' on Linux),
the same location Clava-JS already used, so downloads are shared
instead of duplicated; Clava-JS drops its own override and OS
detection.
Addresses #251, #264, #248, #249, #205, #250
Keep the per-instance folder lifecycle from 0c3fef8 and change only the rebuild and file-include directory prefixes.
Allocate a fresh temporary directory for each rebuild, retain only the two most recent per weaver, and use SpecsIo's safe temporary-directory cleanup when retiring or closing them.
Run per-instance rebuild-folder cleanup even if close-time syntax checking fails. Allocate Clang AST test output only when the test runs so skipped CUDA tests leave no empty directory.
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8685e6c557
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Files.deleteIfExists(sentinelWoven.toPath()); | ||
| Files.deleteIfExists(sentinelSrc.toPath()); | ||
| Files.deleteIfExists(sentinelSrc.getParentFile().toPath()); | ||
| Files.deleteIfExists(sentinelWoven.getParentFile().toPath()); |
There was a problem hiding this comment.
When the test runs from a checkout that already contains __clava_woven or __clava_src—the exact scenario this regression is intended to protect—the setup overwrites any existing sentinel.txt, and this cleanup then deletes it and may remove an originally empty directory. If either directory contains other files, deleteIfExists also throws DirectoryNotEmptyException, failing the test during cleanup. Run the assertion in an isolated working directory or record and restore the pre-test state instead of unconditionally deleting these paths.
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
One policy for every temporary directory, shared by the open temp-dir issues: allocated by the OS (
SpecsIo.createTempDirectory, specs-feup/specs-java-libs#34), owned by the lifecycle that created it (weaver instance, dumper invocation, test instance), and deleted when that lifecycle ends — kept for inspection in debug mode with the path logged.ThreadLocal<Buffer>, isolating concurrent weaver instances sharing one thread; the fixed cwd-relative__clava_woven_for_file_rebuildfolder is a per-invocation temp dir;close()no longer deletes cwd-relative__clava_wovenand__clava_srcfolders, which could destroy unrelated user directories (__clava_srcwas already dead). Regression test added.clava_parsing_<UUID>session root and thebaseFolderplumbing; each dumper invocation owns one OS temp folder, deleted after use. Retires the redundantCodeParser.CLEANoption.<nanoTime>-<threadId>names in the working directory, keeping debug leftovers out of the repo.DUMPER_FOLDERdefaults to the OS user cache folder (~/.cache/@specs-feup/clavaon Linux), the location Clava-JS already used, so downloads are shared instead of duplicated; Clava-JS drops its own override and OS detection.Stacked on #265 (merge that one first). #188 (rebuild flakiness) should be re-verified after this change, since the shared/reused rebuild folders were a plausible cause.
Closes #251, closes #264, closes #248, closes #249, closes #205, closes #250, closes #235, closes #223.