gemini-code-assist[bot] commented on code in PR #19919: URL: https://github.com/apache/tvm/pull/19919#discussion_r3503349017
########## tests/lint/check_operator_spacing.py: ########## @@ -0,0 +1,177 @@ +#!/usr/bin/env python3 +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Conservative operator-spacing checker for '<' and '>'. + +It flags only mixed-side spacing for single-character '<' or '>' ops that are +likely to be binary comparisons, while attempting to avoid templates, casts, +macros, and stream/shift operators. +""" + +from __future__ import annotations + +import re +import subprocess +import sys +from pathlib import Path +from typing import List, Tuple + +CHECK_EXTENSIONS = { + "cc", + "c", + "h", + "hh", + "hpp", + "cu", + "cuh", + "cpp", + "cxx", +} + +# Match any single '<' or '>' with optional surrounding spaces and capture the symbol +_OP_RE = re.compile(r"(?P<left_char>.)(?P<op>\s*(?P<sym>[<>])\s*)(?P<right_char>.)") + +ALNUM_UNDERSCORE = re.compile(r"[A-Za-z0-9_]") + +# Tokens or patterns that strongly indicate the line is not a plain comparison +SKIP_KEYWORDS = [ + "static_cast", + "std::", + "::", + "template", + "TVM_FFI", + "TVM_FFI_ICHECK", + "TVM_FFI_THROW", + "->", +] + +def git_ls_files() -> List[str]: + cmd = ["git", "ls-files"] + proc = subprocess.run(cmd, check=False, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) + if proc.returncode != 0: + print("git ls-files failed:", proc.stderr.strip(), file=sys.stderr) + sys.exit(2) + return [p for p in proc.stdout.splitlines() if p.strip()] + +def should_check_file(path: str) -> bool: + suffix = Path(path).suffix.lstrip(".") + return suffix in CHECK_EXTENSIONS + +def is_preprocessor_line(line: str) -> bool: + stripped = line.lstrip() + return stripped.startswith("#") + +def contains_shift_or_stream(line: str) -> bool: + return "<<" in line or ">>" in line + +def contains_skip_keyword(line: str) -> bool: + for k in SKIP_KEYWORDS: + if k in line: + return True + return False + +def is_part_of_two_char_operator(line: str, sym_pos: int) -> bool: + # avoid matching <=, >=, <<=, >>= or combinations where '=' or another < or > is adjacent + if sym_pos - 1 >= 0 and line[sym_pos - 1] in "<>=": + return True + if sym_pos + 1 < len(line) and line[sym_pos + 1] in "<>=": + return True + return False + +def looks_like_template_close(line: str, match: re.Match) -> bool: + # heuristics: if left char is alnum and the token after the op is an identifier start, + # it's probably "vector<int> v" or similar. Check a few characters after op. + left = match.group("left_char") + start = match.start("op") + after = line[start + len(match.group("op")) :] + if left and ALNUM_UNDERSCORE.match(left): + if after and (ALNUM_UNDERSCORE.match(after[0]) or (after[0].isspace() and len(after) > 1 and ALNUM_UNDERSCORE.match(after[1]))): + return True + return False Review Comment:  The current implementation of `looks_like_template_close` has a critical bug: it returns `True` for any comparison between two identifiers with mixed spacing (e.g., `x< lower` or `x> upper`), causing the checker to skip them entirely. This makes the newly added tests `test_disallow_mixed_right_space` and `test_composite_expression_matches_issue` fail. To fix this, we should: 1. Only apply template-close heuristics when the operator is `>` (since `<` is a template open, and any valid template open like `vector<int>` is already skipped by the spacing check if it has no spaces). 2. Search backwards for a matching `<` and verify if the preceding word is a likely template name (e.g., starts with an uppercase letter or is a known container like `vector`, `shared_ptr`, etc.). ```python def looks_like_template_close(line: str, match: re.Match) -> bool: if match.group("sym") != ">": return False known_templates = { "vector", "shared_ptr", "unique_ptr", "map", "set", "unordered_map", "unordered_set", "pair", "tuple", "list", "array", "function", "weak_ptr", "make_shared", "make_unique" } start = match.start("op") left_text = line[:start] lt_idx = left_text.rfind("<") if lt_idx == -1: return False idx = lt_idx - 1 while idx >= 0 and (left_text[idx].isalnum() or left_text[idx] == "_"): idx -= 1 word = left_text[idx + 1:lt_idx] if not word: return False return word[0].isupper() or word in known_templates or word.endswith("_t") ``` ########## tests/lint/check_operator_spacing.py: ########## @@ -0,0 +1,177 @@ +#!/usr/bin/env python3 +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Conservative operator-spacing checker for '<' and '>'. + +It flags only mixed-side spacing for single-character '<' or '>' ops that are +likely to be binary comparisons, while attempting to avoid templates, casts, +macros, and stream/shift operators. +""" + +from __future__ import annotations + +import re +import subprocess +import sys +from pathlib import Path +from typing import List, Tuple + +CHECK_EXTENSIONS = { + "cc", + "c", + "h", + "hh", + "hpp", + "cu", + "cuh", + "cpp", + "cxx", +} + +# Match any single '<' or '>' with optional surrounding spaces and capture the symbol +_OP_RE = re.compile(r"(?P<left_char>.)(?P<op>\s*(?P<sym>[<>])\s*)(?P<right_char>.)") + +ALNUM_UNDERSCORE = re.compile(r"[A-Za-z0-9_]") + +# Tokens or patterns that strongly indicate the line is not a plain comparison +SKIP_KEYWORDS = [ + "static_cast", + "std::", + "::", + "template", + "TVM_FFI", + "TVM_FFI_ICHECK", + "TVM_FFI_THROW", + "->", +] + +def git_ls_files() -> List[str]: + cmd = ["git", "ls-files"] + proc = subprocess.run(cmd, check=False, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) + if proc.returncode != 0: + print("git ls-files failed:", proc.stderr.strip(), file=sys.stderr) + sys.exit(2) + return [p for p in proc.stdout.splitlines() if p.strip()] + +def should_check_file(path: str) -> bool: + suffix = Path(path).suffix.lstrip(".") + return suffix in CHECK_EXTENSIONS + +def is_preprocessor_line(line: str) -> bool: + stripped = line.lstrip() + return stripped.startswith("#") + +def contains_shift_or_stream(line: str) -> bool: + return "<<" in line or ">>" in line + +def contains_skip_keyword(line: str) -> bool: + for k in SKIP_KEYWORDS: + if k in line: + return True + return False + +def is_part_of_two_char_operator(line: str, sym_pos: int) -> bool: + # avoid matching <=, >=, <<=, >>= or combinations where '=' or another < or > is adjacent + if sym_pos - 1 >= 0 and line[sym_pos - 1] in "<>=": + return True + if sym_pos + 1 < len(line) and line[sym_pos + 1] in "<>=": + return True + return False + +def looks_like_template_close(line: str, match: re.Match) -> bool: + # heuristics: if left char is alnum and the token after the op is an identifier start, + # it's probably "vector<int> v" or similar. Check a few characters after op. + left = match.group("left_char") + start = match.start("op") + after = line[start + len(match.group("op")) :] + if left and ALNUM_UNDERSCORE.match(left): + if after and (ALNUM_UNDERSCORE.match(after[0]) or (after[0].isspace() and len(after) > 1 and ALNUM_UNDERSCORE.match(after[1]))): + return True + return False + +def check_file(path: str) -> List[Tuple[int, int, str]]: + violations: List[Tuple[int, int, str]] = [] + text = Path(path).read_text(encoding="utf-8", errors="ignore") Review Comment:  If a file is deleted or renamed in a PR, `git ls-files` might still return it, leading to a `FileNotFoundError` when `Path(path).read_text()` is called. We should add a defensive check to ensure the path is a valid file before reading it. ```python def check_file(path: str) -> List[Tuple[int, int, str]]: violations: List[Tuple[int, int, str]] = [] p = Path(path) if not p.is_file(): return violations text = p.read_text(encoding="utf-8", errors="ignore") ``` ########## tests/lint/check_operator_spacing.py: ########## @@ -0,0 +1,177 @@ +#!/usr/bin/env python3 +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Conservative operator-spacing checker for '<' and '>'. + +It flags only mixed-side spacing for single-character '<' or '>' ops that are +likely to be binary comparisons, while attempting to avoid templates, casts, +macros, and stream/shift operators. +""" + +from __future__ import annotations + +import re +import subprocess +import sys +from pathlib import Path +from typing import List, Tuple + +CHECK_EXTENSIONS = { + "cc", + "c", + "h", + "hh", + "hpp", + "cu", + "cuh", + "cpp", + "cxx", +} + +# Match any single '<' or '>' with optional surrounding spaces and capture the symbol +_OP_RE = re.compile(r"(?P<left_char>.)(?P<op>\s*(?P<sym>[<>])\s*)(?P<right_char>.)") + +ALNUM_UNDERSCORE = re.compile(r"[A-Za-z0-9_]") + +# Tokens or patterns that strongly indicate the line is not a plain comparison +SKIP_KEYWORDS = [ + "static_cast", + "std::", + "::", + "template", + "TVM_FFI", + "TVM_FFI_ICHECK", + "TVM_FFI_THROW", + "->", +] + +def git_ls_files() -> List[str]: + cmd = ["git", "ls-files"] + proc = subprocess.run(cmd, check=False, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) + if proc.returncode != 0: + print("git ls-files failed:", proc.stderr.strip(), file=sys.stderr) + sys.exit(2) + return [p for p in proc.stdout.splitlines() if p.strip()] + +def should_check_file(path: str) -> bool: + suffix = Path(path).suffix.lstrip(".") + return suffix in CHECK_EXTENSIONS + +def is_preprocessor_line(line: str) -> bool: + stripped = line.lstrip() + return stripped.startswith("#") + +def contains_shift_or_stream(line: str) -> bool: + return "<<" in line or ">>" in line + +def contains_skip_keyword(line: str) -> bool: + for k in SKIP_KEYWORDS: + if k in line: + return True + return False + +def is_part_of_two_char_operator(line: str, sym_pos: int) -> bool: + # avoid matching <=, >=, <<=, >>= or combinations where '=' or another < or > is adjacent + if sym_pos - 1 >= 0 and line[sym_pos - 1] in "<>=": + return True + if sym_pos + 1 < len(line) and line[sym_pos + 1] in "<>=": + return True + return False + +def looks_like_template_close(line: str, match: re.Match) -> bool: + # heuristics: if left char is alnum and the token after the op is an identifier start, + # it's probably "vector<int> v" or similar. Check a few characters after op. + left = match.group("left_char") + start = match.start("op") + after = line[start + len(match.group("op")) :] + if left and ALNUM_UNDERSCORE.match(left): + if after and (ALNUM_UNDERSCORE.match(after[0]) or (after[0].isspace() and len(after) > 1 and ALNUM_UNDERSCORE.match(after[1]))): + return True + return False + +def check_file(path: str) -> List[Tuple[int, int, str]]: + violations: List[Tuple[int, int, str]] = [] + text = Path(path).read_text(encoding="utf-8", errors="ignore") + for lineno, line in enumerate(text.splitlines(), start=1): + if is_preprocessor_line(line): + continue + if contains_shift_or_stream(line): + continue + if contains_skip_keyword(line): + continue Review Comment:  To prevent false positives, we should strip single-line comments and string literals from the line before checking for operator spacing. This ensures that mixed spacing inside comments or string literals (e.g., `"x <y"`) is not flagged as a violation. ```python for lineno, line in enumerate(text.splitlines(), start=1): if is_preprocessor_line(line): continue if contains_shift_or_stream(line): continue if contains_skip_keyword(line): continue # Strip single-line comments if "//" in line: line = line.split("//", 1)[0] # Strip string literals to avoid false positives inside strings line = re.sub(r'"[^"]*"', lambda m: " " * len(m.group(0)), line) ``` ########## tests/lint/check_operator_spacing.py: ########## @@ -0,0 +1,177 @@ +#!/usr/bin/env python3 +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Conservative operator-spacing checker for '<' and '>'. + +It flags only mixed-side spacing for single-character '<' or '>' ops that are +likely to be binary comparisons, while attempting to avoid templates, casts, +macros, and stream/shift operators. +""" + +from __future__ import annotations + +import re +import subprocess +import sys +from pathlib import Path +from typing import List, Tuple + +CHECK_EXTENSIONS = { + "cc", + "c", + "h", + "hh", + "hpp", + "cu", + "cuh", + "cpp", + "cxx", +} + +# Match any single '<' or '>' with optional surrounding spaces and capture the symbol +_OP_RE = re.compile(r"(?P<left_char>.)(?P<op>\s*(?P<sym>[<>])\s*)(?P<right_char>.)") + +ALNUM_UNDERSCORE = re.compile(r"[A-Za-z0-9_]") + +# Tokens or patterns that strongly indicate the line is not a plain comparison +SKIP_KEYWORDS = [ + "static_cast", + "std::", + "::", + "template", + "TVM_FFI", + "TVM_FFI_ICHECK", + "TVM_FFI_THROW", + "->", +] Review Comment:  We should expand `SKIP_KEYWORDS` to include other standard C++ casts like `dynamic_cast`, `reinterpret_cast`, and `const_cast` to avoid false positives on lines containing them. ```python SKIP_KEYWORDS = [ "static_cast", "dynamic_cast", "reinterpret_cast", "const_cast", "std::", "::", "template", "TVM_FFI", "TVM_FFI_ICHECK", "TVM_FFI_THROW", "->", ] ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
