Skip to content

Conversation

@16bit-ykiko
Copy link
Member

No description provided.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @16bit-ykiko, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the CompilationDatabase component by introducing powerful new capabilities for manipulating compile commands. It allows users to specify arguments to be removed or appended to existing commands through the CommandOptions structure. This is facilitated by a newly implemented ArgumentParser and a refactored command processing pipeline, which provides more granular control over how compile commands are constructed and modified. These changes improve the flexibility and robustness of command handling within the compiler infrastructure.

Highlights

  • Configurable Command Manipulation: Introduced remove and append options within the CommandOptions structure, allowing for dynamic modification of compile commands by specifying arguments to be excluded or added.
  • Robust Argument Parsing: Implemented a new ArgumentParser class, inheriting from llvm::opt::ArgList, to provide more accurate and flexible parsing of command-line arguments, which is crucial for the new command manipulation features.
  • Refactored Command Processing: A new process_command method has been added to CompilationDatabase. This method centralizes the logic for filtering, modifying, and preparing compile arguments based on the provided CommandOptions, including handling response files and path conversions.
  • Improved Test Coverage: New unit tests, specifically the RemoveAppend test case, have been added to thoroughly validate the functionality of removing and appending command-line arguments, ensuring correct behavior across various scenarios.
  • Logging and Renaming: The suppress_log field in CommandOptions has been renamed to suppress_logging for improved clarity, and logging initialization has been integrated into unit tests.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces functionality to remove and append command-line arguments from a configuration file. The changes are quite extensive, including a new ArgumentParser and significant refactoring of command processing. Overall, the implementation looks promising, but I've identified a few issues. There's a high-severity bug where response_file_index is not being updated, and a significant design choice of using a raw void* pointer with manual memory management which could be improved for safety and maintainability by using std::unique_ptr. I've also pointed out several medium-severity issues such as dead code, incorrect comments, and a test case lacking assertions. Addressing these points will improve the robustness and quality of the code.

@16bit-ykiko
Copy link
Member Author

/gemini summary

@gemini-code-assist
Copy link
Contributor

Summary of Changes

This pull request significantly enhances the CompilationDatabase component by introducing powerful new capabilities for manipulating compile commands. It allows users to specify arguments to be removed or appended to existing commands through the CommandOptions structure. This is facilitated by a newly implemented ArgumentParser and a refactored command processing pipeline, which provides more granular control over how compile commands are constructed and modified. These changes improve the flexibility and robustness of command handling within the compiler infrastructure.

Highlights

  • Configurable Command Manipulation: Introduced remove and append options within the CommandOptions structure, allowing for dynamic modification of compile commands by specifying arguments to be excluded or added.
  • Robust Argument Parsing: Implemented a new ArgumentParser class, inheriting from llvm::opt::ArgList, to provide more accurate and flexible parsing of command-line arguments, which is crucial for the new command manipulation features.
  • Refactored Command Processing: A new process_command method has been added to CompilationDatabase. This method centralizes the logic for filtering, modifying, and preparing compile arguments based on the provided CommandOptions, including handling response files and path conversions.
  • Improved Resource Management for CompilationDatabase: Implemented move constructor, move assignment operator, and a destructor for CompilationDatabase to correctly manage the lifetime of the newly introduced ArgumentParser.
  • Enhanced Test Coverage: New unit tests, specifically the RemoveAppend test case, have been added to thoroughly validate the functionality of removing and appending command-line arguments, ensuring correct behavior across various scenarios.
  • Logging and Renaming: The suppress_log field in CommandOptions has been renamed to suppress_logging for improved clarity, and logging initialization has been integrated into unit tests.
Changelog
  • include/Compiler/Command.h
    • Added ignore_unknown, remove, and append fields to the CommandOptions struct.
    • Renamed suppress_log to suppress_logging in CommandOptions.
    • Added response_file and response_file_index fields to the CommandInfo struct.
    • Implemented move constructor, move assignment operator, and destructor for CompilationDatabase.
    • Added a new process_command method to CompilationDatabase.
    • Added a void* parser member to the private section of CompilationDatabase.
  • include/Test/Tester.h
    • Updated references from options.suppress_log to options.suppress_logging in two locations.
  • src/Compiler/Command.cpp
    • Introduced a new ArgumentParser class for parsing command-line arguments.
    • Refactored driver_invocation_argv and driver_invocation_env logic directly into query_driver and removed the standalone helper functions.
    • Modified the CompilationDatabase constructor to initialize the parser member.
    • Implemented the move constructor, move assignment operator, and destructor for CompilationDatabase to manage the parser's memory.
    • Refactored the update_command method to utilize the new ArgumentParser and to handle filtering of output/input files and response files.
    • Added the process_command method to apply remove and append options and filter arguments.
    • Modified the get_command method to use the new process_command for argument processing.
    • Updated suppress_log to suppress_logging in relevant sections.
  • src/Driver/unit_tests.cc
    • Reordered include directives for better organization.
    • Added logic to Runner::run_tests() to filter tests by suite name based on test_filter.
    • Added logging::create_stderr_logger call in main for logging initialization.
  • tests/unit/Compiler/Command.cpp
    • Added a static option_table member.
    • Renamed the printArgv function to print_argv.
    • Removed the parse_and_dump function.
    • Updated the expect_strip lambda to use suppress_logging and print_argv.
    • Added a new RemoveAppend test case to verify the functionality of removing and appending command-line arguments.
    • Added a test for GET_OPTION_ID("-omain.o").
    • Modified the Module test to be skipped (skip / test) and included basic CompilationDatabase usage with a response file.
Activity
  • A summary of changes was provided by gemini-code-assist[bot].
  • The author, 16bit-ykiko, requested a summary.
  • gemini-code-assist[bot] provided several review comments:
  • High priority: Suggested using std::unique_ptr with the pImpl idiom for the parser member instead of void* for type safety and automatic memory management.
  • High priority: Noted that response_file_index was not being updated when a response file was found, indicating a potential bug.
  • Medium priority: Pointed out an incomplete comment for response_file_index.
  • Medium priority: Identified an incorrect comment for CompilationDatabase::save_cstring_list as the function was implemented below it.
  • Medium priority: Recommended using an empty list {} for the env variable on Windows instead of {""} to avoid unintended side effects.
  • Medium priority: Noted that the unknown_remove_args vector was declared but unused, suggesting it be removed.
  • Medium priority: Indicated that the append local variable was unused and could be removed.
  • Medium priority: Highlighted that the Module test case lacked assertions and did not verify any behavior.

@16bit-ykiko 16bit-ykiko changed the title Implement remove and append in the config file Add remove and append options for CompilationDatabase Oct 2, 2025
@16bit-ykiko 16bit-ykiko merged commit 7cd20c8 into main Oct 2, 2025
9 checks passed
@16bit-ykiko 16bit-ykiko deleted the update-command branch October 2, 2025 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants