From 078be3bd6511cfadea224df4c9471c3239aae2cf Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Thu, 29 Jan 2026 18:56:55 -0800 Subject: [PATCH] Remove structured bindings refactoring from code conventions analyzer (#8428) * Initial plan * Remove structured bindings focus from code conventions analyzer Co-authored-by: NikolajBjorner <3085284+NikolajBjorner@users.noreply.github.com> * Regenerate code-conventions-analyzer.lock.yml after removing structured bindings Co-authored-by: NikolajBjorner <3085284+NikolajBjorner@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: NikolajBjorner <3085284+NikolajBjorner@users.noreply.github.com> --- .../code-conventions-analyzer.lock.yml | 187 +++--------------- .../workflows/code-conventions-analyzer.md | 156 ++------------- 2 files changed, 41 insertions(+), 302 deletions(-) diff --git a/.github/workflows/code-conventions-analyzer.lock.yml b/.github/workflows/code-conventions-analyzer.lock.yml index 955264003..34af7cecf 100644 --- a/.github/workflows/code-conventions-analyzer.lock.yml +++ b/.github/workflows/code-conventions-analyzer.lock.yml @@ -13,7 +13,7 @@ # \ /\ / (_) | | | | ( | | | | (_) \ V V /\__ \ # \/ \/ \___/|_| |_|\_\|_| |_|\___/ \_/\_/ |___/ # -# This file was automatically generated by gh-aw (v0.37.28). DO NOT EDIT. +# This file was automatically generated by gh-aw (v0.37.32). DO NOT EDIT. # # To update this file, edit the corresponding .md file and run: # gh aw compile @@ -44,7 +44,7 @@ jobs: comment_repo: "" steps: - name: Setup Scripts - uses: githubnext/gh-aw/actions/setup@v0.37.28 + uses: githubnext/gh-aw/actions/setup@v0.37.32 with: destination: /opt/gh-aw/actions - name: Check workflow file timestamps @@ -81,7 +81,7 @@ jobs: secret_verification_result: ${{ steps.validate-secret.outputs.verification_result }} steps: - name: Setup Scripts - uses: githubnext/gh-aw/actions/setup@v0.37.28 + uses: githubnext/gh-aw/actions/setup@v0.37.32 with: destination: /opt/gh-aw/actions - name: Checkout repository @@ -152,7 +152,7 @@ jobs: mkdir -p /tmp/gh-aw/safeoutputs mkdir -p /tmp/gh-aw/mcp-logs/safeoutputs cat > /opt/gh-aw/safeoutputs/config.json << 'EOF' - {"create_discussion":{"max":1},"create_issue":{"max":5},"create_missing_tool_issue":{"max":1,"title_prefix":"[missing tool]"},"missing_data":{},"missing_tool":{},"noop":{"max":1}} + {"create_discussion":{"expires":168,"max":1},"create_issue":{"max":5},"create_missing_tool_issue":{"max":1,"title_prefix":"[missing tool]"},"missing_data":{},"missing_tool":{},"noop":{"max":1}} EOF cat > /opt/gh-aw/safeoutputs/tools.json << 'EOF' [ @@ -369,7 +369,6 @@ jobs: "maxLength": 256 }, "tool": { - "required": true, "type": "string", "sanitize": true, "maxLength": 128 @@ -492,7 +491,7 @@ jobs: model: process.env.GH_AW_MODEL_AGENT_COPILOT || "", version: "", agent_version: "0.0.395", - cli_version: "v0.37.28", + cli_version: "v0.37.32", workflow_name: "Code Conventions Analyzer", experimental: false, supports_tools_allowlist: true, @@ -559,7 +558,7 @@ jobs: To create or modify GitHub resources (issues, discussions, pull requests, etc.), you MUST call the appropriate safe output tool. Simply writing content will NOT work - the workflow requires actual tool calls. - **Available tools**: create_discussion, create_issue, missing_tool, noop + Discover available tools from the safeoutputs MCP server. **Critical**: Tool calls write structured data that downstream jobs process. Without tool calls, follow-up actions will be skipped. @@ -603,41 +602,9 @@ jobs: ## Your Task - **PRIMARY FOCUS: Create Issues for Tuple Pattern (Structured Bindings) Refactoring** + **PRIMARY FOCUS: Create Issues for initializer_list Refactoring** - Your primary task is to identify and implement refactorings that use C++17 structured bindings instead of accessing `.first` and `.second`: - - 1. **Find tuple/pair access patterns** - Code accessing `.first` and `.second` members - 2. **Implement the refactoring** - Replace with structured bindings for clearer code - 3. **Create issues** - Automatically create an issue with your changes for tuple pattern improvements - - **Focus Areas for Tuple Pattern Refactoring:** - - Variables that access both `.first` and `.second` multiple times - - Return values from functions that are immediately decomposed - - Iterator dereferences accessing pair members (e.g., map iterators) - - Code that would be clearer with meaningful variable names instead of `.first`/`.second` - - **Naming Convention for Structured Bindings:** - When introducing structured bindings, use names that are representative of the types or field semantics: - - For `enode_pair`: use `[n1, n2]` instead of `[first, second]` or `[a, b]` - - For map iterators: use `[key, value]` or `[k, v]` instead of `[first, second]` - - For domain-specific pairs: use names reflecting the semantics (e.g., `[var, offset]`, `[expr, count]`) - - **Avoid generic names** like `first`, `second`, `third`, `a`, `b` unless the pair truly represents generic values - - **Example refactoring:** - ```cpp - // Before: Using .first and .second on enode_pair - enode_pair p = get_pair(y); - return merge(p.first, p.second); - - // After: Using structured bindings with meaningful names - auto [n1, n2] = get_pair(y); - return merge(n1, n2); - ``` - - **SECONDARY FOCUS: Create Issues for initializer_list Refactoring** - - Your secondary task is to identify and implement refactorings that use `std::initializer_list` instead of array pointer + size parameters: + Your primary task is to identify and implement refactorings that use `std::initializer_list` instead of array pointer + size parameters: 1. **Find array + size parameter patterns** - Functions taking `(unsigned sz, T* args)` or similar 2. **Implement the refactoring** - Replace with `std::initializer_list` for cleaner APIs @@ -677,7 +644,7 @@ jobs: Additionally, conduct analysis of other coding conventions and modern C++ opportunities for discussion (not immediate implementation) - ## Workflow for initializer_list Refactoring (SECONDARY) + ## Workflow for initializer_list Refactoring (PRIMARY) ### Step A: Find initializer_list Refactoring Opportunities @@ -873,7 +840,6 @@ jobs: - In-class member initializers **C++17 features:** - - Structured bindings for tuple/pair unpacking - `if constexpr` for compile-time conditionals - `std::string_view` for string parameters - Fold expressions for variadic templates @@ -967,22 +933,6 @@ jobs: - Incorrect usage of `std::move` (moving from const references, etc.) - Return value optimization opportunities being blocked - **Tuple/Pair Access Patterns:** - - **PRIMARY TASK**: Code accessing `.first` and `.second` on pairs/tuples - - **ACTION**: Replace with C++17 structured bindings for cleaner, more readable code - - **RESULT**: Create an issue with the actual code changes - - **NAMING**: Use descriptive names based on types/semantics (e.g., `[n1, n2]` for `enode_pair`, `[k, v]` for maps) - - **Example**: - ```cpp - // Before - enode_pair p = get_pair(y); - return merge(p.first, p.second); - - // After: Use meaningful names, not generic [a, b] - auto [n1, n2] = get_pair(y); - return merge(n1, n2); - ``` - **Exception String Construction:** - Using `stringstream` to build exception messages - Unnecessary string copies when raising exceptions @@ -1019,8 +969,6 @@ jobs: ## Analysis Methodology 1. **Sample key directories** in the codebase: - PROMPT_EOF - cat << 'PROMPT_EOF' >> "$GH_AW_PROMPT" - `src/util/` - Core utilities and data structures - `src/ast/` - Abstract syntax tree implementations - `src/smt/` - SMT solver core @@ -1066,13 +1014,15 @@ jobs: ### PRIMARY: Issues for Code Refactoring - When you implement refactorings (structured bindings, initializer_list), create issues using `output.create-issue` with: + When you implement refactorings (initializer_list), create issues using `output.create-issue` with: - Clear title indicating what was refactored - Description of changes and benefits - List of modified files and functions ### SECONDARY: Detailed Analysis Discussion + PROMPT_EOF + cat << 'PROMPT_EOF' >> "$GH_AW_PROMPT" For other code quality findings, create a comprehensive discussion with your findings structured as follows: ### Discussion Title @@ -1298,80 +1248,21 @@ jobs: - **Incorrect std::move**: [Move from const, unnecessary moves] - **Return Value Optimization**: [Places where RVO is blocked] - ### 4.8 Tuple Pattern (Structured Bindings) Modernization - **IMPLEMENT AS ISSUE** - - **This is the PRIMARY focus area - implement these changes directly:** - - - **Current Pattern**: Accessing `.first` and `.second` on pairs/tuples - - **Modern Pattern**: Use C++17 structured bindings for cleaner code - - **Benefits**: - - More readable variable names instead of `.first`/`.second` - - Clearer intent of what values represent - - Eliminates intermediate variables - - Reduces chance of errors from swapping `.first`/`.second` - - **Action**: Find and refactor tuple/pair access patterns: - 1. Search for patterns using `.first` and `.second` - 2. Identify cases where intermediate variable can be eliminated - 3. Refactor to use structured bindings with **meaningful names** (not generic `a`, `b`, `first`, `second`) - 4. Create an issue with changes - - **Example Pattern**: - ```cpp - // Before: Using .first and .second on enode_pair - enode_pair p = get_pair(y); - return merge(p.first, p.second); - - // After: Using structured bindings with descriptive names - auto [n1, n2] = get_pair(y); - return merge(n1, n2); - ``` - - **Another Example**: - ```cpp - // Before: Accessing pair members via iterator - auto result = map.find(key); - if (result != map.end()) { - use_key(result->first); - use_value(result->second); - } - - // After: Structured bindings to access pair - auto it = map.find(key); - if (it != map.end()) { - auto& [k, v] = *it; - use_key(k); - use_value(v); - } - - // Or for range-based loops (eliminates iterator entirely): - for (auto& [k, v] : map) { - use_key(k); - use_value(v); - } - ``` - - **Search Patterns**: Look for code using `.first` or `.second` on: - - Return values from functions returning `std::pair` or tuples - - Iterator dereferences (especially map iterators) - - Direct pair/tuple variable access - - **Candidates**: Functions or code blocks that: - - Call `.first` and `.second` on the same variable multiple times - - Create intermediate variables just to access pair members - - Have sequential uses of both `.first` and `.second` - - **Output**: Issue with refactored code - - ### 4.9 Exception String Construction + ### 4.8 Exception String Construction - **Current**: [stringstream usage for building exception messages] - **Modern**: [std::format and std::formater opportunities] - **String Copies**: [Unnecessary copies when raising exceptions] - **Examples**: [Specific exception construction sites] - ### 4.10 Array Parameter Modernization (std::span) + ### 4.9 Array Parameter Modernization (std::span) - **Current**: [Pointer + size parameter pairs for runtime-sized arrays] - **Modern**: [std::span usage opportunities] - **Type Safety**: [How span improves API safety] - **Examples**: [Function signatures to update] - ### 4.11 Array Parameter Modernization (std::initializer_list) - **IMPLEMENT AS ISSUE** + ### 4.10 Array Parameter Modernization (std::initializer_list) - **IMPLEMENT AS ISSUE** - **This is a SECONDARY focus area - implement these changes directly:** + **This is the PRIMARY focus area - implement these changes directly:** - **Current Pattern**: Functions with `unsigned sz, T* args` or `unsigned sz, T* const* args` parameters - **Modern Pattern**: Use `std::initializer_list` for functions called with compile-time constant arrays @@ -1421,20 +1312,18 @@ jobs: - **Output**: Issue with refactored code - **Note**: Only apply to internal C++ APIs, not to public C API functions that need C compatibility - ### 4.12 Increment Operator Patterns + ### 4.11 Increment Operator Patterns - **Postfix Usage**: [Count of i++ where result is unused] - **Prefix Preference**: [Places to use ++i instead] - PROMPT_EOF - cat << 'PROMPT_EOF' >> "$GH_AW_PROMPT" - **Iterator Loops**: [Heavy iterator usage areas] - ### 4.13 Exception Control Flow + ### 4.12 Exception Control Flow - **Current Usage**: [Exceptions used for normal control flow] - **Modern Alternatives**: [std::expected or error codes] - **Performance**: [Impact of exception-based control flow] - **Refactoring Opportunities**: [Specific patterns to replace] - ### 4.14 Inefficient Stream Output + ### 4.13 Inefficient Stream Output - **Current Usage**: [string stream output operator used for single characters] - **Modern Alternatives**: [use char output operator] - **Performance**: [Reduce code size and improve performance] @@ -1545,6 +1434,8 @@ jobs: - **Be constructive**: Frame findings as opportunities, not criticisms - **Quantify when possible**: Use numbers to show prevalence of patterns - **Consider backward compatibility**: Z3 is a mature project with many users + PROMPT_EOF + cat << 'PROMPT_EOF' >> "$GH_AW_PROMPT" - **Measure size improvements**: Use `static_assert` and `sizeof` to verify memory layout optimizations - **Prioritize safety**: Smart pointers and `std::span` improve type safety - **Consider performance**: Hash table optimizations and AST caching have measurable impact @@ -1670,28 +1561,6 @@ jobs: grep pattern: "bool.*\(.*\*.*\)|bool.*\(.*&" glob: "src/**/*.h" ``` - **Find tuple/pair access patterns (structured binding opportunities):** - ``` - # Find .first usage - grep pattern: "\.first" glob: "src/**/*.{cpp,h}" - - # Find .second usage - grep pattern: "\.second" glob: "src/**/*.{cpp,h}" - - # Find same variable used with both .first and .second on same line (high-value candidates) - # Note: This only matches single-line patterns where the same var is used; use separate .first/.second searches for comprehensive coverage - grep pattern: "([a-z_]+)\.first.*\1\.second" glob: "src/**/*.cpp" - - # Find iterator dereferencing with .first or .second - grep pattern: "->first|->second" glob: "src/**/*.cpp" - - # Find return statements using .first and .second - grep pattern: "return.*\.first.*\.second" glob: "src/**/*.cpp" - - # Find function calls with .first and .second as arguments (focused pattern) - grep pattern: "\([^)]*\.first[^)]*\.second[^)]*\)" glob: "src/**/*.cpp" - ``` - **Find pointer + size parameters:** ``` grep pattern: "\([^,]+\*[^,]*,\s*size_t|, unsigned.*size\)" glob: "src/**/*.h" @@ -1739,7 +1608,7 @@ jobs: - Never execute untrusted code - Use `bash` only for safe operations (git, grep patterns) - - **For code refactoring (structured bindings, initializer_list)**: Use the `edit` tool to modify files directly + - **For code refactoring (initializer_list)**: Use the `edit` tool to modify files directly - **For other findings**: Create discussions only (no code modifications) - All code changes will be reviewed through the issue process @@ -1747,7 +1616,7 @@ jobs: **Two types of outputs:** - 1. **Issue** (for refactorings like structured bindings or initializer_list): + 1. **Issue** (for refactorings like initializer_list): - Use `output.create-issue` to create an issue - Include clear title and description - List all modified files @@ -1839,7 +1708,7 @@ jobs: run: | set -o pipefail GH_AW_TOOL_BINS=""; [ -n "$GOROOT" ] && GH_AW_TOOL_BINS="$GOROOT/bin:$GH_AW_TOOL_BINS"; [ -n "$JAVA_HOME" ] && GH_AW_TOOL_BINS="$JAVA_HOME/bin:$GH_AW_TOOL_BINS"; [ -n "$CARGO_HOME" ] && GH_AW_TOOL_BINS="$CARGO_HOME/bin:$GH_AW_TOOL_BINS"; [ -n "$GEM_HOME" ] && GH_AW_TOOL_BINS="$GEM_HOME/bin:$GH_AW_TOOL_BINS"; [ -n "$CONDA" ] && GH_AW_TOOL_BINS="$CONDA/bin:$GH_AW_TOOL_BINS"; [ -n "$PIPX_BIN_DIR" ] && GH_AW_TOOL_BINS="$PIPX_BIN_DIR:$GH_AW_TOOL_BINS"; [ -n "$SWIFT_PATH" ] && GH_AW_TOOL_BINS="$SWIFT_PATH:$GH_AW_TOOL_BINS"; [ -n "$DOTNET_ROOT" ] && GH_AW_TOOL_BINS="$DOTNET_ROOT:$GH_AW_TOOL_BINS"; export GH_AW_TOOL_BINS - sudo -E awf --env-all --env 'ANDROID_HOME=${ANDROID_HOME}' --env 'ANDROID_NDK=${ANDROID_NDK}' --env 'ANDROID_NDK_HOME=${ANDROID_NDK_HOME}' --env 'ANDROID_NDK_LATEST_HOME=${ANDROID_NDK_LATEST_HOME}' --env 'ANDROID_NDK_ROOT=${ANDROID_NDK_ROOT}' --env 'ANDROID_SDK_ROOT=${ANDROID_SDK_ROOT}' --env 'AZURE_EXTENSION_DIR=${AZURE_EXTENSION_DIR}' --env 'CARGO_HOME=${CARGO_HOME}' --env 'CHROMEWEBDRIVER=${CHROMEWEBDRIVER}' --env 'CONDA=${CONDA}' --env 'DOTNET_ROOT=${DOTNET_ROOT}' --env 'EDGEWEBDRIVER=${EDGEWEBDRIVER}' --env 'GECKOWEBDRIVER=${GECKOWEBDRIVER}' --env 'GEM_HOME=${GEM_HOME}' --env 'GEM_PATH=${GEM_PATH}' --env 'GOPATH=${GOPATH}' --env 'GOROOT=${GOROOT}' --env 'HOMEBREW_CELLAR=${HOMEBREW_CELLAR}' --env 'HOMEBREW_PREFIX=${HOMEBREW_PREFIX}' --env 'HOMEBREW_REPOSITORY=${HOMEBREW_REPOSITORY}' --env 'JAVA_HOME=${JAVA_HOME}' --env 'JAVA_HOME_11_X64=${JAVA_HOME_11_X64}' --env 'JAVA_HOME_17_X64=${JAVA_HOME_17_X64}' --env 'JAVA_HOME_21_X64=${JAVA_HOME_21_X64}' --env 'JAVA_HOME_25_X64=${JAVA_HOME_25_X64}' --env 'JAVA_HOME_8_X64=${JAVA_HOME_8_X64}' --env 'NVM_DIR=${NVM_DIR}' --env 'PIPX_BIN_DIR=${PIPX_BIN_DIR}' --env 'PIPX_HOME=${PIPX_HOME}' --env 'RUSTUP_HOME=${RUSTUP_HOME}' --env 'SELENIUM_JAR_PATH=${SELENIUM_JAR_PATH}' --env 'SWIFT_PATH=${SWIFT_PATH}' --env 'VCPKG_INSTALLATION_ROOT=${VCPKG_INSTALLATION_ROOT}' --env 'GH_AW_TOOL_BINS=$GH_AW_TOOL_BINS' --container-workdir "${GITHUB_WORKSPACE}" --mount /tmp:/tmp:rw --mount "${GITHUB_WORKSPACE}:${GITHUB_WORKSPACE}:rw" --mount /usr/bin/date:/usr/bin/date:ro --mount /usr/bin/gh:/usr/bin/gh:ro --mount /usr/bin/yq:/usr/bin/yq:ro --mount /usr/local/bin/copilot:/usr/local/bin/copilot:ro --mount /home/runner/.copilot:/home/runner/.copilot:rw --mount /opt/hostedtoolcache:/opt/hostedtoolcache:ro --mount /opt/gh-aw:/opt/gh-aw:ro --allow-domains api.business.githubcopilot.com,api.enterprise.githubcopilot.com,api.github.com,api.githubcopilot.com,api.individual.githubcopilot.com,api.snapcraft.io,archive.ubuntu.com,azure.archive.ubuntu.com,crl.geotrust.com,crl.globalsign.com,crl.identrust.com,crl.sectigo.com,crl.thawte.com,crl.usertrust.com,crl.verisign.com,crl3.digicert.com,crl4.digicert.com,crls.ssl.com,github.com,host.docker.internal,json-schema.org,json.schemastore.org,keyserver.ubuntu.com,ocsp.digicert.com,ocsp.geotrust.com,ocsp.globalsign.com,ocsp.identrust.com,ocsp.sectigo.com,ocsp.ssl.com,ocsp.thawte.com,ocsp.usertrust.com,ocsp.verisign.com,packagecloud.io,packages.cloud.google.com,packages.microsoft.com,ppa.launchpad.net,raw.githubusercontent.com,registry.npmjs.org,s.symcb.com,s.symcd.com,security.ubuntu.com,ts-crl.ws.symantec.com,ts-ocsp.ws.symantec.com --log-level info --proxy-logs-dir /tmp/gh-aw/sandbox/firewall/logs --enable-host-access --image-tag 0.11.2 --agent-image act \ + sudo -E awf --env-all --env 'ANDROID_HOME=${ANDROID_HOME}' --env 'ANDROID_NDK=${ANDROID_NDK}' --env 'ANDROID_NDK_HOME=${ANDROID_NDK_HOME}' --env 'ANDROID_NDK_LATEST_HOME=${ANDROID_NDK_LATEST_HOME}' --env 'ANDROID_NDK_ROOT=${ANDROID_NDK_ROOT}' --env 'ANDROID_SDK_ROOT=${ANDROID_SDK_ROOT}' --env 'AZURE_EXTENSION_DIR=${AZURE_EXTENSION_DIR}' --env 'CARGO_HOME=${CARGO_HOME}' --env 'CHROMEWEBDRIVER=${CHROMEWEBDRIVER}' --env 'CONDA=${CONDA}' --env 'DOTNET_ROOT=${DOTNET_ROOT}' --env 'EDGEWEBDRIVER=${EDGEWEBDRIVER}' --env 'GECKOWEBDRIVER=${GECKOWEBDRIVER}' --env 'GEM_HOME=${GEM_HOME}' --env 'GEM_PATH=${GEM_PATH}' --env 'GOPATH=${GOPATH}' --env 'GOROOT=${GOROOT}' --env 'HOMEBREW_CELLAR=${HOMEBREW_CELLAR}' --env 'HOMEBREW_PREFIX=${HOMEBREW_PREFIX}' --env 'HOMEBREW_REPOSITORY=${HOMEBREW_REPOSITORY}' --env 'JAVA_HOME=${JAVA_HOME}' --env 'JAVA_HOME_11_X64=${JAVA_HOME_11_X64}' --env 'JAVA_HOME_17_X64=${JAVA_HOME_17_X64}' --env 'JAVA_HOME_21_X64=${JAVA_HOME_21_X64}' --env 'JAVA_HOME_25_X64=${JAVA_HOME_25_X64}' --env 'JAVA_HOME_8_X64=${JAVA_HOME_8_X64}' --env 'NVM_DIR=${NVM_DIR}' --env 'PIPX_BIN_DIR=${PIPX_BIN_DIR}' --env 'PIPX_HOME=${PIPX_HOME}' --env 'RUSTUP_HOME=${RUSTUP_HOME}' --env 'SELENIUM_JAR_PATH=${SELENIUM_JAR_PATH}' --env 'SWIFT_PATH=${SWIFT_PATH}' --env 'VCPKG_INSTALLATION_ROOT=${VCPKG_INSTALLATION_ROOT}' --env 'GH_AW_TOOL_BINS=$GH_AW_TOOL_BINS' --container-workdir "${GITHUB_WORKSPACE}" --mount /tmp:/tmp:rw --mount "${GITHUB_WORKSPACE}:${GITHUB_WORKSPACE}:rw" --mount /usr/bin/cat:/usr/bin/cat:ro --mount /usr/bin/curl:/usr/bin/curl:ro --mount /usr/bin/date:/usr/bin/date:ro --mount /usr/bin/find:/usr/bin/find:ro --mount /usr/bin/gh:/usr/bin/gh:ro --mount /usr/bin/grep:/usr/bin/grep:ro --mount /usr/bin/jq:/usr/bin/jq:ro --mount /usr/bin/yq:/usr/bin/yq:ro --mount /usr/bin/cp:/usr/bin/cp:ro --mount /usr/bin/cut:/usr/bin/cut:ro --mount /usr/bin/diff:/usr/bin/diff:ro --mount /usr/bin/head:/usr/bin/head:ro --mount /usr/bin/ls:/usr/bin/ls:ro --mount /usr/bin/mkdir:/usr/bin/mkdir:ro --mount /usr/bin/rm:/usr/bin/rm:ro --mount /usr/bin/sed:/usr/bin/sed:ro --mount /usr/bin/sort:/usr/bin/sort:ro --mount /usr/bin/tail:/usr/bin/tail:ro --mount /usr/bin/wc:/usr/bin/wc:ro --mount /usr/bin/which:/usr/bin/which:ro --mount /usr/local/bin/copilot:/usr/local/bin/copilot:ro --mount /home/runner/.copilot:/home/runner/.copilot:rw --mount /opt/hostedtoolcache:/opt/hostedtoolcache:ro --mount /opt/gh-aw:/opt/gh-aw:ro --allow-domains api.business.githubcopilot.com,api.enterprise.githubcopilot.com,api.github.com,api.githubcopilot.com,api.individual.githubcopilot.com,api.snapcraft.io,archive.ubuntu.com,azure.archive.ubuntu.com,crl.geotrust.com,crl.globalsign.com,crl.identrust.com,crl.sectigo.com,crl.thawte.com,crl.usertrust.com,crl.verisign.com,crl3.digicert.com,crl4.digicert.com,crls.ssl.com,github.com,host.docker.internal,json-schema.org,json.schemastore.org,keyserver.ubuntu.com,ocsp.digicert.com,ocsp.geotrust.com,ocsp.globalsign.com,ocsp.identrust.com,ocsp.sectigo.com,ocsp.ssl.com,ocsp.thawte.com,ocsp.usertrust.com,ocsp.verisign.com,packagecloud.io,packages.cloud.google.com,packages.microsoft.com,ppa.launchpad.net,raw.githubusercontent.com,registry.npmjs.org,s.symcb.com,s.symcd.com,security.ubuntu.com,ts-crl.ws.symantec.com,ts-ocsp.ws.symantec.com --log-level info --proxy-logs-dir /tmp/gh-aw/sandbox/firewall/logs --enable-host-access --image-tag 0.11.2 --agent-image act \ -- 'export PATH="$GH_AW_TOOL_BINS$(find /opt/hostedtoolcache -maxdepth 4 -type d -name bin 2>/dev/null | tr '\''\n'\'' '\'':'\'')$PATH" && /usr/local/bin/copilot --add-dir /tmp/gh-aw/ --log-level all --log-dir /tmp/gh-aw/sandbox/agent/logs/ --add-dir "${GITHUB_WORKSPACE}" --disable-builtin-mcps --allow-tool github --allow-tool safeoutputs --allow-tool '\''shell(cat)'\'' --allow-tool '\''shell(clang-format --version)'\'' --allow-tool '\''shell(date)'\'' --allow-tool '\''shell(echo)'\'' --allow-tool '\''shell(git diff:*)'\'' --allow-tool '\''shell(git log:*)'\'' --allow-tool '\''shell(git show:*)'\'' --allow-tool '\''shell(grep)'\'' --allow-tool '\''shell(head)'\'' --allow-tool '\''shell(ls)'\'' --allow-tool '\''shell(pwd)'\'' --allow-tool '\''shell(sort)'\'' --allow-tool '\''shell(tail)'\'' --allow-tool '\''shell(uniq)'\'' --allow-tool '\''shell(wc)'\'' --allow-tool '\''shell(yq)'\'' --allow-tool write --add-dir /tmp/gh-aw/cache-memory/ --allow-all-paths --share /tmp/gh-aw/sandbox/agent/logs/conversation.md --prompt "$(cat /tmp/gh-aw/aw-prompts/prompt.txt)"${GH_AW_MODEL_AGENT_COPILOT:+ --model "$GH_AW_MODEL_AGENT_COPILOT"}' \ 2>&1 | tee /tmp/gh-aw/agent-stdio.log env: @@ -2001,7 +1870,7 @@ jobs: total_count: ${{ steps.missing_tool.outputs.total_count }} steps: - name: Setup Scripts - uses: githubnext/gh-aw/actions/setup@v0.37.28 + uses: githubnext/gh-aw/actions/setup@v0.37.32 with: destination: /opt/gh-aw/actions - name: Debug job inputs @@ -2102,7 +1971,7 @@ jobs: success: ${{ steps.parse_results.outputs.success }} steps: - name: Setup Scripts - uses: githubnext/gh-aw/actions/setup@v0.37.28 + uses: githubnext/gh-aw/actions/setup@v0.37.32 with: destination: /opt/gh-aw/actions - name: Download agent artifacts @@ -2253,7 +2122,7 @@ jobs: process_safe_outputs_temporary_id_map: ${{ steps.process_safe_outputs.outputs.temporary_id_map }} steps: - name: Setup Scripts - uses: githubnext/gh-aw/actions/setup@v0.37.28 + uses: githubnext/gh-aw/actions/setup@v0.37.32 with: destination: /opt/gh-aw/actions - name: Download agent output artifact @@ -2290,7 +2159,7 @@ jobs: permissions: {} steps: - name: Setup Scripts - uses: githubnext/gh-aw/actions/setup@v0.37.28 + uses: githubnext/gh-aw/actions/setup@v0.37.32 with: destination: /opt/gh-aw/actions - name: Download cache-memory artifact (default) diff --git a/.github/workflows/code-conventions-analyzer.md b/.github/workflows/code-conventions-analyzer.md index 92be15ed0..35f1f209e 100644 --- a/.github/workflows/code-conventions-analyzer.md +++ b/.github/workflows/code-conventions-analyzer.md @@ -38,41 +38,9 @@ You are an expert C++ code quality analyst specializing in the Z3 theorem prover ## Your Task -**PRIMARY FOCUS: Create Issues for Tuple Pattern (Structured Bindings) Refactoring** +**PRIMARY FOCUS: Create Issues for initializer_list Refactoring** -Your primary task is to identify and implement refactorings that use C++17 structured bindings instead of accessing `.first` and `.second`: - -1. **Find tuple/pair access patterns** - Code accessing `.first` and `.second` members -2. **Implement the refactoring** - Replace with structured bindings for clearer code -3. **Create issues** - Automatically create an issue with your changes for tuple pattern improvements - -**Focus Areas for Tuple Pattern Refactoring:** -- Variables that access both `.first` and `.second` multiple times -- Return values from functions that are immediately decomposed -- Iterator dereferences accessing pair members (e.g., map iterators) -- Code that would be clearer with meaningful variable names instead of `.first`/`.second` - -**Naming Convention for Structured Bindings:** -When introducing structured bindings, use names that are representative of the types or field semantics: -- For `enode_pair`: use `[n1, n2]` instead of `[first, second]` or `[a, b]` -- For map iterators: use `[key, value]` or `[k, v]` instead of `[first, second]` -- For domain-specific pairs: use names reflecting the semantics (e.g., `[var, offset]`, `[expr, count]`) -- **Avoid generic names** like `first`, `second`, `third`, `a`, `b` unless the pair truly represents generic values - -**Example refactoring:** -```cpp -// Before: Using .first and .second on enode_pair -enode_pair p = get_pair(y); -return merge(p.first, p.second); - -// After: Using structured bindings with meaningful names -auto [n1, n2] = get_pair(y); -return merge(n1, n2); -``` - -**SECONDARY FOCUS: Create Issues for initializer_list Refactoring** - -Your secondary task is to identify and implement refactorings that use `std::initializer_list` instead of array pointer + size parameters: +Your primary task is to identify and implement refactorings that use `std::initializer_list` instead of array pointer + size parameters: 1. **Find array + size parameter patterns** - Functions taking `(unsigned sz, T* args)` or similar 2. **Implement the refactoring** - Replace with `std::initializer_list` for cleaner APIs @@ -112,7 +80,7 @@ foo({1, 2}); Additionally, conduct analysis of other coding conventions and modern C++ opportunities for discussion (not immediate implementation) -## Workflow for initializer_list Refactoring (SECONDARY) +## Workflow for initializer_list Refactoring (PRIMARY) ### Step A: Find initializer_list Refactoring Opportunities @@ -308,7 +276,6 @@ Z3 uses C++20 (as specified in `.clang-format`). Look for opportunities to use: - In-class member initializers **C++17 features:** -- Structured bindings for tuple/pair unpacking - `if constexpr` for compile-time conditionals - `std::string_view` for string parameters - Fold expressions for variadic templates @@ -402,22 +369,6 @@ Identify opportunities specific to Z3's architecture and coding patterns: - Incorrect usage of `std::move` (moving from const references, etc.) - Return value optimization opportunities being blocked -**Tuple/Pair Access Patterns:** -- **PRIMARY TASK**: Code accessing `.first` and `.second` on pairs/tuples -- **ACTION**: Replace with C++17 structured bindings for cleaner, more readable code -- **RESULT**: Create an issue with the actual code changes -- **NAMING**: Use descriptive names based on types/semantics (e.g., `[n1, n2]` for `enode_pair`, `[k, v]` for maps) -- **Example**: - ```cpp - // Before - enode_pair p = get_pair(y); - return merge(p.first, p.second); - - // After: Use meaningful names, not generic [a, b] - auto [n1, n2] = get_pair(y); - return merge(n1, n2); - ``` - **Exception String Construction:** - Using `stringstream` to build exception messages - Unnecessary string copies when raising exceptions @@ -499,7 +450,7 @@ Identify opportunities specific to Z3's architecture and coding patterns: ### PRIMARY: Issues for Code Refactoring -When you implement refactorings (structured bindings, initializer_list), create issues using `output.create-issue` with: +When you implement refactorings (initializer_list), create issues using `output.create-issue` with: - Clear title indicating what was refactored - Description of changes and benefits - List of modified files and functions @@ -731,80 +682,21 @@ For each opportunity, provide: - **Incorrect std::move**: [Move from const, unnecessary moves] - **Return Value Optimization**: [Places where RVO is blocked] -### 4.8 Tuple Pattern (Structured Bindings) Modernization - **IMPLEMENT AS ISSUE** - -**This is the PRIMARY focus area - implement these changes directly:** - -- **Current Pattern**: Accessing `.first` and `.second` on pairs/tuples -- **Modern Pattern**: Use C++17 structured bindings for cleaner code -- **Benefits**: - - More readable variable names instead of `.first`/`.second` - - Clearer intent of what values represent - - Eliminates intermediate variables - - Reduces chance of errors from swapping `.first`/`.second` -- **Action**: Find and refactor tuple/pair access patterns: - 1. Search for patterns using `.first` and `.second` - 2. Identify cases where intermediate variable can be eliminated - 3. Refactor to use structured bindings with **meaningful names** (not generic `a`, `b`, `first`, `second`) - 4. Create an issue with changes -- **Example Pattern**: - ```cpp - // Before: Using .first and .second on enode_pair - enode_pair p = get_pair(y); - return merge(p.first, p.second); - - // After: Using structured bindings with descriptive names - auto [n1, n2] = get_pair(y); - return merge(n1, n2); - ``` -- **Another Example**: - ```cpp - // Before: Accessing pair members via iterator - auto result = map.find(key); - if (result != map.end()) { - use_key(result->first); - use_value(result->second); - } - - // After: Structured bindings to access pair - auto it = map.find(key); - if (it != map.end()) { - auto& [k, v] = *it; - use_key(k); - use_value(v); - } - - // Or for range-based loops (eliminates iterator entirely): - for (auto& [k, v] : map) { - use_key(k); - use_value(v); - } - ``` -- **Search Patterns**: Look for code using `.first` or `.second` on: - - Return values from functions returning `std::pair` or tuples - - Iterator dereferences (especially map iterators) - - Direct pair/tuple variable access -- **Candidates**: Functions or code blocks that: - - Call `.first` and `.second` on the same variable multiple times - - Create intermediate variables just to access pair members - - Have sequential uses of both `.first` and `.second` -- **Output**: Issue with refactored code - -### 4.9 Exception String Construction +### 4.8 Exception String Construction - **Current**: [stringstream usage for building exception messages] - **Modern**: [std::format and std::formater opportunities] - **String Copies**: [Unnecessary copies when raising exceptions] - **Examples**: [Specific exception construction sites] -### 4.10 Array Parameter Modernization (std::span) +### 4.9 Array Parameter Modernization (std::span) - **Current**: [Pointer + size parameter pairs for runtime-sized arrays] - **Modern**: [std::span usage opportunities] - **Type Safety**: [How span improves API safety] - **Examples**: [Function signatures to update] -### 4.11 Array Parameter Modernization (std::initializer_list) - **IMPLEMENT AS ISSUE** +### 4.10 Array Parameter Modernization (std::initializer_list) - **IMPLEMENT AS ISSUE** -**This is a SECONDARY focus area - implement these changes directly:** +**This is the PRIMARY focus area - implement these changes directly:** - **Current Pattern**: Functions with `unsigned sz, T* args` or `unsigned sz, T* const* args` parameters - **Modern Pattern**: Use `std::initializer_list` for functions called with compile-time constant arrays @@ -854,18 +746,18 @@ For each opportunity, provide: - **Output**: Issue with refactored code - **Note**: Only apply to internal C++ APIs, not to public C API functions that need C compatibility -### 4.12 Increment Operator Patterns +### 4.11 Increment Operator Patterns - **Postfix Usage**: [Count of i++ where result is unused] - **Prefix Preference**: [Places to use ++i instead] - **Iterator Loops**: [Heavy iterator usage areas] -### 4.13 Exception Control Flow +### 4.12 Exception Control Flow - **Current Usage**: [Exceptions used for normal control flow] - **Modern Alternatives**: [std::expected or error codes] - **Performance**: [Impact of exception-based control flow] - **Refactoring Opportunities**: [Specific patterns to replace] -### 4.14 Inefficient Stream Output +### 4.13 Inefficient Stream Output - **Current Usage**: [string stream output operator used for single characters] - **Modern Alternatives**: [use char output operator] - **Performance**: [Reduce code size and improve performance] @@ -1101,28 +993,6 @@ grep pattern: "return.*nullptr.*&" glob: "src/**/*.{h,cpp}" grep pattern: "bool.*\(.*\*.*\)|bool.*\(.*&" glob: "src/**/*.h" ``` -**Find tuple/pair access patterns (structured binding opportunities):** -``` -# Find .first usage -grep pattern: "\.first" glob: "src/**/*.{cpp,h}" - -# Find .second usage -grep pattern: "\.second" glob: "src/**/*.{cpp,h}" - -# Find same variable used with both .first and .second on same line (high-value candidates) -# Note: This only matches single-line patterns where the same var is used; use separate .first/.second searches for comprehensive coverage -grep pattern: "([a-z_]+)\.first.*\1\.second" glob: "src/**/*.cpp" - -# Find iterator dereferencing with .first or .second -grep pattern: "->first|->second" glob: "src/**/*.cpp" - -# Find return statements using .first and .second -grep pattern: "return.*\.first.*\.second" glob: "src/**/*.cpp" - -# Find function calls with .first and .second as arguments (focused pattern) -grep pattern: "\([^)]*\.first[^)]*\.second[^)]*\)" glob: "src/**/*.cpp" -``` - **Find pointer + size parameters:** ``` grep pattern: "\([^,]+\*[^,]*,\s*size_t|, unsigned.*size\)" glob: "src/**/*.h" @@ -1170,7 +1040,7 @@ grep pattern: "<<\s*\".*\"\s*<<\s*\".*\"" glob: "src/**/*.cpp" - Never execute untrusted code - Use `bash` only for safe operations (git, grep patterns) -- **For code refactoring (structured bindings, initializer_list)**: Use the `edit` tool to modify files directly +- **For code refactoring (initializer_list)**: Use the `edit` tool to modify files directly - **For other findings**: Create discussions only (no code modifications) - All code changes will be reviewed through the issue process @@ -1178,7 +1048,7 @@ grep pattern: "<<\s*\".*\"\s*<<\s*\".*\"" glob: "src/**/*.cpp" **Two types of outputs:** -1. **Issue** (for refactorings like structured bindings or initializer_list): +1. **Issue** (for refactorings like initializer_list): - Use `output.create-issue` to create an issue - Include clear title and description - List all modified files