-
Notifications
You must be signed in to change notification settings - Fork 603
i#4856: Add Doxygen links for instr_t and fix formatting #7700
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
i#4856: Add Doxygen links for instr_t and fix formatting #7700
Conversation
9267dd3 to
2e247fc
Compare
|
@derekbruening ,I'm having trouble resolving the CI formatting failures on this PR. Could you please point me to the exact formatting command or script the project uses? I seem to be missing a step and would appreciate your guidance. Thanks for your help! |
|
2e247fc to
b6dd64b
Compare
b6dd64b to
4b494fc
Compare
|
@derekbruening all the formatting checks (ci-clang-format, etc.) are passing now after rebasing onto the latest master and ensuring the correct clang-format-14 version was used for the header files. Thanks for the guidance! However, there's one remaining failure in the ci-x86 / test (ubuntu-22.04, x86_64, RelWithDebInfo) job (ID: 53988260379). Looking at the logs for that job, the errors seem related to the build environment itself: Cross-compiler aarch64-linux-gnu-gcc not found,Cross-compiler riscv64-linux-gnu-gcc not found Since my changes in this PR were only focused on fixing Doxygen @ref links for instr_t within comment blocks in .h files, these cross-compilation and build tool errors seem unlikely to be caused by my code changes. Could someone please take a look or advise on how to proceed? |
The cross compiler is not an issue: that is just how the scripts operate, attempting various configs. The errors are the test failures at the very bottom: That is #7678 which is a regression that appeared recently and shows up occasionally but no one has been able to reproduce locally or root cause it, unfortunately. I tried a re-run of that job. |
|
Please reply to all comments and mark resolved, per https://dynamorio.org/page_code_reviews.html#autotoc_md118, and then click the arrows to re-request a review. |
Please do not force-push to a shared branch: it breaks the history and makes it hard to review. Seeing changes since the last review becomes impossible; comments lose their anchors in the code. Not doing force-pushes is documented in multiple places: https://dynamorio.org/page_code_reviews.html#sec_code_review_non_member, https://dynamorio.org/page_code_reviews.html#autotoc_md118. The final merge squashes all the commits and the description comes from the PR text box: so there is no upside to a force-push and many significant downsides. |
|
@derekbruening,Thanks again for the review and clarifying the formatting issues. I really appreciate your guidance and patience as I learn the contribution process. Regarding the specific formatting changes you pointed out (like the multi-line split for WINDOW_SUBDIR_FORMAT): You were absolutely right. My previous attempts at formatting were incorrect. To fix this properly, I followed the CI setup precisely: Installed clang-format-14 using the exact repository source specified in the ci-clang-format.yml workflow. Ran clang-format-14 -i on all the files I had modified. This process did fix the incorrect multi-line splits you identified. However, I observed that this correct run of clang-format-14 also removed all the # symbols I had manually added before instr_t in the Doxygen comments (which was the original purpose of this PR, addressing #4856). Before I push the new commit containing these formatting changes (which does pass the CI checks but removes the # symbols), I wanted to ask for guidance: what's the recommended way to handle the conflict with clang-format-14 ,Please let me know how you'd prefer to proceed. |
|
@derekbruening, I've pushed the final fixes for the formatting and the missing newline. All CI checks are passing now. Thanks again for all your help |
|
i have merged master to sync the branch. The new CI run is complete, and it looks like all the style errors are now fixed Thanks again for all your help and patience |
derekbruening
left a comment
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.
LGTM. Thanks again for contributing!
This PR adds explicit
#Doxygen links forinstr_tin various header files, as discussed in issue #4856.Fixes #4856