|
1 |
| -#!/usr/bin/env python2 |
| 1 | +#!/usr/bin/env python |
2 | 2 | #
|
3 |
| -# ===- run-clang-tidy.py - Parallel clang-tidy runner ---------*- python -*--===# |
| 3 | +#===- run-clang-tidy.py - Parallel clang-tidy runner ---------*- python -*--===# |
4 | 4 | #
|
5 |
| -# The LLVM Compiler Infrastructure |
| 5 | +# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. |
| 6 | +# See https://llvm.org/LICENSE.txt for license information. |
| 7 | +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception |
6 | 8 | #
|
7 |
| -# This file is distributed under the University of Illinois Open Source |
8 |
| -# License. See LICENSE.TXT for details. |
9 |
| -# |
10 |
| -# ===------------------------------------------------------------------------===# |
| 9 | +#===------------------------------------------------------------------------===# |
11 | 10 | # FIXME: Integrate with clang-tidy-diff.py
|
12 | 11 |
|
13 | 12 | """
|
|
35 | 34 | """
|
36 | 35 |
|
37 | 36 | from __future__ import print_function
|
| 37 | + |
38 | 38 | import argparse
|
| 39 | +import glob |
39 | 40 | import json
|
40 | 41 | import multiprocessing
|
41 | 42 | import os
|
42 |
| -import Queue |
43 | 43 | import re
|
44 | 44 | import shutil
|
45 | 45 | import subprocess
|
|
48 | 48 | import threading
|
49 | 49 | import traceback
|
50 | 50 |
|
| 51 | +try: |
| 52 | + import yaml |
| 53 | +except ImportError: |
| 54 | + yaml = None |
51 | 55 |
|
52 |
| -class TidyQueue(Queue.Queue): |
53 |
| - def __init__(self, max_task): |
54 |
| - Queue.Queue.__init__(self, max_task) |
55 |
| - self.has_error = False |
| 56 | +is_py2 = sys.version[0] == '2' |
56 | 57 |
|
| 58 | +if is_py2: |
| 59 | + import Queue as queue |
| 60 | +else: |
| 61 | + import queue as queue |
57 | 62 |
|
58 | 63 | def find_compilation_database(path):
|
59 |
| - """Adjusts the directory until a compilation database is found.""" |
60 |
| - result = './' |
61 |
| - while not os.path.isfile(os.path.join(result, path)): |
62 |
| - if os.path.realpath(result) == '/': |
63 |
| - print('Error: could not find compilation database.') |
64 |
| - sys.exit(1) |
65 |
| - result += '../' |
66 |
| - return os.path.realpath(result) |
67 |
| - |
68 |
| - |
69 |
| -def get_tidy_invocation(f, clang_tidy_binary, checks, warningsaserrors, |
70 |
| - tmpdir, build_path, |
71 |
| - header_filter, extra_arg, extra_arg_before, quiet): |
72 |
| - """Gets a command line for clang-tidy.""" |
73 |
| - start = [clang_tidy_binary] |
74 |
| - if header_filter is not None: |
75 |
| - start.append('-header-filter=' + header_filter) |
76 |
| - else: |
77 |
| - # Show warnings in all in-project headers by default. |
78 |
| - start.append('-header-filter=^' + build_path + '/.*') |
79 |
| - if checks: |
80 |
| - start.append('-checks=' + checks) |
81 |
| - if warningsaserrors: |
82 |
| - start.append('-warnings-as-errors=' + warningsaserrors) |
83 |
| - if tmpdir is not None: |
84 |
| - start.append('-export-fixes') |
85 |
| - # Get a temporary file. We immediately close the handle so clang-tidy can |
86 |
| - # overwrite it. |
87 |
| - (handle, name) = tempfile.mkstemp(suffix='.yaml', dir=tmpdir) |
88 |
| - os.close(handle) |
89 |
| - start.append(name) |
90 |
| - for arg in extra_arg: |
91 |
| - start.append('-extra-arg=%s' % arg) |
92 |
| - for arg in extra_arg_before: |
93 |
| - start.append('-extra-arg-before=%s' % arg) |
94 |
| - start.append('-p=' + build_path) |
95 |
| - if quiet: |
96 |
| - start.append('-quiet') |
97 |
| - start.append(f) |
98 |
| - return start |
| 64 | + """Adjusts the directory until a compilation database is found.""" |
| 65 | + result = './' |
| 66 | + while not os.path.isfile(os.path.join(result, path)): |
| 67 | + if os.path.realpath(result) == '/': |
| 68 | + print('Error: could not find compilation database.') |
| 69 | + sys.exit(1) |
| 70 | + result += '../' |
| 71 | + return os.path.realpath(result) |
| 72 | + |
| 73 | + |
| 74 | +def make_absolute(f, directory): |
| 75 | + if os.path.isabs(f): |
| 76 | + return f |
| 77 | + return os.path.normpath(os.path.join(directory, f)) |
| 78 | + |
| 79 | + |
| 80 | +def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path, |
| 81 | + header_filter, extra_arg, extra_arg_before, quiet, |
| 82 | + config): |
| 83 | + """Gets a command line for clang-tidy.""" |
| 84 | + start = [clang_tidy_binary] |
| 85 | + if header_filter is not None: |
| 86 | + start.append('-header-filter=' + header_filter) |
| 87 | + if checks: |
| 88 | + start.append('-checks=' + checks) |
| 89 | + if tmpdir is not None: |
| 90 | + start.append('-export-fixes') |
| 91 | + # Get a temporary file. We immediately close the handle so clang-tidy can |
| 92 | + # overwrite it. |
| 93 | + (handle, name) = tempfile.mkstemp(suffix='.yaml', dir=tmpdir) |
| 94 | + os.close(handle) |
| 95 | + start.append(name) |
| 96 | + for arg in extra_arg: |
| 97 | + start.append('-extra-arg=%s' % arg) |
| 98 | + for arg in extra_arg_before: |
| 99 | + start.append('-extra-arg-before=%s' % arg) |
| 100 | + start.append('-p=' + build_path) |
| 101 | + if quiet: |
| 102 | + start.append('-quiet') |
| 103 | + if config: |
| 104 | + start.append('-config=' + config) |
| 105 | + start.append(f) |
| 106 | + return start |
| 107 | + |
| 108 | + |
| 109 | +def merge_replacement_files(tmpdir, mergefile): |
| 110 | + """Merge all replacement files in a directory into a single file""" |
| 111 | + # The fixes suggested by clang-tidy >= 4.0.0 are given under |
| 112 | + # the top level key 'Diagnostics' in the output yaml files |
| 113 | + mergekey="Diagnostics" |
| 114 | + merged=[] |
| 115 | + for replacefile in glob.iglob(os.path.join(tmpdir, '*.yaml')): |
| 116 | + content = yaml.safe_load(open(replacefile, 'r')) |
| 117 | + if not content: |
| 118 | + continue # Skip empty files. |
| 119 | + merged.extend(content.get(mergekey, [])) |
| 120 | + |
| 121 | + if merged: |
| 122 | + # MainSourceFile: The key is required by the definition inside |
| 123 | + # include/clang/Tooling/ReplacementsYaml.h, but the value |
| 124 | + # is actually never used inside clang-apply-replacements, |
| 125 | + # so we set it to '' here. |
| 126 | + output = { 'MainSourceFile': '', mergekey: merged } |
| 127 | + with open(mergefile, 'w') as out: |
| 128 | + yaml.safe_dump(output, out) |
| 129 | + else: |
| 130 | + # Empty the file: |
| 131 | + open(mergefile, 'w').close() |
99 | 132 |
|
100 | 133 |
|
101 | 134 | def check_clang_apply_replacements_binary(args):
|
102 |
| - """Checks if invoking supplied clang-apply-replacements binary works.""" |
103 |
| - try: |
104 |
| - subprocess.check_call([args.clang_apply_replacements_binary, '--version']) |
105 |
| - except: |
106 |
| - print('Unable to run clang-apply-replacements. Is clang-apply-replacements ' |
107 |
| - 'binary correctly specified?', file=sys.stderr) |
108 |
| - traceback.print_exc() |
109 |
| - sys.exit(1) |
| 135 | + """Checks if invoking supplied clang-apply-replacements binary works.""" |
| 136 | + try: |
| 137 | + subprocess.check_call([args.clang_apply_replacements_binary, '--version']) |
| 138 | + except: |
| 139 | + print('Unable to run clang-apply-replacements. Is clang-apply-replacements ' |
| 140 | + 'binary correctly specified?', file=sys.stderr) |
| 141 | + traceback.print_exc() |
| 142 | + sys.exit(1) |
110 | 143 |
|
111 | 144 |
|
112 | 145 | def apply_fixes(args, tmpdir):
|
113 |
| - """Calls clang-apply-fixes on a given directory. Deletes the dir when done.""" |
114 |
| - invocation = [args.clang_apply_replacements_binary] |
115 |
| - if args.format: |
116 |
| - invocation.append('-format') |
117 |
| - if args.style: |
118 |
| - invocation.append('-style=' + args.style) |
119 |
| - invocation.append(tmpdir) |
120 |
| - subprocess.call(invocation) |
121 |
| - |
122 |
| - |
123 |
| -def run_tidy(args, tmpdir, build_path, queue): |
124 |
| - """Takes filenames out of queue and runs clang-tidy on them.""" |
125 |
| - while True: |
126 |
| - name = queue.get() |
127 |
| - invocation = get_tidy_invocation(name, args.clang_tidy_binary, args.checks, |
128 |
| - args.warningsaserrors, tmpdir, build_path, |
129 |
| - args.header_filter, args.extra_arg, |
130 |
| - args.extra_arg_before, args.quiet) |
131 |
| - if not args.no_command_on_stdout: |
132 |
| - sys.stdout.write(' '.join(invocation) + '\n') |
133 |
| - try: |
134 |
| - subprocess.check_call(invocation) |
135 |
| - except subprocess.CalledProcessError: |
136 |
| - queue.has_error = True |
137 |
| - queue.task_done() |
| 146 | + """Calls clang-apply-fixes on a given directory.""" |
| 147 | + invocation = [args.clang_apply_replacements_binary] |
| 148 | + if args.format: |
| 149 | + invocation.append('-format') |
| 150 | + if args.style: |
| 151 | + invocation.append('-style=' + args.style) |
| 152 | + invocation.append(tmpdir) |
| 153 | + subprocess.call(invocation) |
| 154 | + |
| 155 | + |
| 156 | +def run_tidy(args, tmpdir, build_path, queue, lock, failed_files): |
| 157 | + """Takes filenames out of queue and runs clang-tidy on them.""" |
| 158 | + while True: |
| 159 | + name = queue.get() |
| 160 | + invocation = get_tidy_invocation(name, args.clang_tidy_binary, args.checks, |
| 161 | + tmpdir, build_path, args.header_filter, |
| 162 | + args.extra_arg, args.extra_arg_before, |
| 163 | + args.quiet, args.config) |
| 164 | + |
| 165 | + proc = subprocess.Popen(invocation) |
| 166 | + proc.wait() |
| 167 | + if proc.returncode != 0: |
| 168 | + failed_files.append(name) |
| 169 | + queue.task_done() |
138 | 170 |
|
139 | 171 |
|
140 | 172 | def main():
|
141 |
| - parser = argparse.ArgumentParser(description='Runs clang-tidy over all files ' |
142 |
| - 'in a compilation database. Requires ' |
143 |
| - 'clang-tidy and clang-apply-replacements in ' |
144 |
| - '$PATH.') |
145 |
| - parser.add_argument('-clang-tidy-binary', metavar='PATH', |
146 |
| - default='clang-tidy', |
147 |
| - help='path to clang-tidy binary') |
148 |
| - parser.add_argument('-clang-apply-replacements-binary', metavar='PATH', |
149 |
| - default='clang-apply-replacements', |
150 |
| - help='path to clang-apply-replacements binary') |
151 |
| - parser.add_argument('-checks', default=None, |
152 |
| - help='checks filter, when not specified, use clang-tidy ' |
153 |
| - 'default') |
154 |
| - parser.add_argument('-warningsaserrors', default=None, |
155 |
| - help='warnings-as-errors filter, when not specified, ' |
156 |
| - 'use clang-tidy default') |
157 |
| - parser.add_argument('-header-filter', default=None, |
158 |
| - help='regular expression matching the names of the ' |
159 |
| - 'headers to output diagnostics from. Diagnostics from ' |
160 |
| - 'the main file of each translation unit are always ' |
161 |
| - 'displayed.') |
162 |
| - parser.add_argument('-j', type=int, default=0, |
163 |
| - help='number of tidy instances to be run in parallel.') |
164 |
| - parser.add_argument('files', nargs='*', default=['.*'], |
165 |
| - help='files to be processed (regex on path)') |
166 |
| - parser.add_argument('-fix', action='store_true', help='apply fix-its') |
167 |
| - parser.add_argument('-format', action='store_true', help='Reformat code ' |
168 |
| - 'after applying fixes') |
169 |
| - parser.add_argument('-style', default='file', help='The style of reformat ' |
170 |
| - 'code after applying fixes') |
171 |
| - parser.add_argument('-p', dest='build_path', |
172 |
| - help='Path used to read a compile command database.') |
173 |
| - parser.add_argument('-extra-arg', dest='extra_arg', |
174 |
| - action='append', default=[], |
175 |
| - help='Additional argument to append to the compiler ' |
176 |
| - 'command line.') |
177 |
| - parser.add_argument('-extra-arg-before', dest='extra_arg_before', |
178 |
| - action='append', default=[], |
179 |
| - help='Additional argument to prepend to the compiler ' |
180 |
| - 'command line.') |
181 |
| - parser.add_argument('-quiet', action='store_true', |
182 |
| - help='Run clang-tidy in quiet mode') |
183 |
| - parser.add_argument('-no-command-on-stdout', action='store_true', |
184 |
| - help='Run clang-tidy without printing invocation on ' |
185 |
| - 'stdout') |
186 |
| - args = parser.parse_args() |
187 |
| - |
188 |
| - db_path = 'compile_commands.json' |
189 |
| - |
190 |
| - if args.build_path is not None: |
191 |
| - build_path = args.build_path |
| 173 | + parser = argparse.ArgumentParser(description='Runs clang-tidy over all files ' |
| 174 | + 'in a compilation database. Requires ' |
| 175 | + 'clang-tidy and clang-apply-replacements in ' |
| 176 | + '$PATH.') |
| 177 | + parser.add_argument('-clang-tidy-binary', metavar='PATH', |
| 178 | + default='clang-tidy', |
| 179 | + help='path to clang-tidy binary') |
| 180 | + parser.add_argument('-clang-apply-replacements-binary', metavar='PATH', |
| 181 | + default='clang-apply-replacements', |
| 182 | + help='path to clang-apply-replacements binary') |
| 183 | + parser.add_argument('-checks', default=None, |
| 184 | + help='checks filter, when not specified, use clang-tidy ' |
| 185 | + 'default') |
| 186 | + parser.add_argument('-config', default=None, |
| 187 | + help='Specifies a configuration in YAML/JSON format: ' |
| 188 | + ' -config="{Checks: \'*\', ' |
| 189 | + ' CheckOptions: [{key: x, ' |
| 190 | + ' value: y}]}" ' |
| 191 | + 'When the value is empty, clang-tidy will ' |
| 192 | + 'attempt to find a file named .clang-tidy for ' |
| 193 | + 'each source file in its parent directories.') |
| 194 | + parser.add_argument('-header-filter', default=None, |
| 195 | + help='regular expression matching the names of the ' |
| 196 | + 'headers to output diagnostics from. Diagnostics from ' |
| 197 | + 'the main file of each translation unit are always ' |
| 198 | + 'displayed.') |
| 199 | + if yaml: |
| 200 | + parser.add_argument('-export-fixes', metavar='filename', dest='export_fixes', |
| 201 | + help='Create a yaml file to store suggested fixes in, ' |
| 202 | + 'which can be applied with clang-apply-replacements.') |
| 203 | + parser.add_argument('-j', type=int, default=0, |
| 204 | + help='number of tidy instances to be run in parallel.') |
| 205 | + parser.add_argument('files', nargs='*', default=['.*'], |
| 206 | + help='files to be processed (regex on path)') |
| 207 | + parser.add_argument('-fix', action='store_true', help='apply fix-its') |
| 208 | + parser.add_argument('-format', action='store_true', help='Reformat code ' |
| 209 | + 'after applying fixes') |
| 210 | + parser.add_argument('-style', default='file', help='The style of reformat ' |
| 211 | + 'code after applying fixes') |
| 212 | + parser.add_argument('-p', dest='build_path', |
| 213 | + help='Path used to read a compile command database.') |
| 214 | + parser.add_argument('-extra-arg', dest='extra_arg', |
| 215 | + action='append', default=[], |
| 216 | + help='Additional argument to append to the compiler ' |
| 217 | + 'command line.') |
| 218 | + parser.add_argument('-extra-arg-before', dest='extra_arg_before', |
| 219 | + action='append', default=[], |
| 220 | + help='Additional argument to prepend to the compiler ' |
| 221 | + 'command line.') |
| 222 | + parser.add_argument('-quiet', action='store_true', |
| 223 | + help='Run clang-tidy in quiet mode') |
| 224 | + args = parser.parse_args() |
| 225 | + |
| 226 | + db_path = 'compile_commands.json' |
| 227 | + |
| 228 | + if args.build_path is not None: |
| 229 | + build_path = args.build_path |
| 230 | + else: |
| 231 | + # Find our database |
| 232 | + build_path = find_compilation_database(db_path) |
| 233 | + |
| 234 | + try: |
| 235 | + invocation = [args.clang_tidy_binary, '-list-checks'] |
| 236 | + invocation.append('-p=' + build_path) |
| 237 | + if args.checks: |
| 238 | + invocation.append('-checks=' + args.checks) |
| 239 | + invocation.append('-') |
| 240 | + if args.quiet: |
| 241 | + # Even with -quiet we still want to check if we can call clang-tidy. |
| 242 | + with open(os.devnull, 'w') as dev_null: |
| 243 | + subprocess.check_call(invocation, stdout=dev_null) |
192 | 244 | else:
|
193 |
| - # Find our database |
194 |
| - build_path = find_compilation_database(db_path) |
195 |
| - |
| 245 | + subprocess.check_call(invocation) |
| 246 | + except: |
| 247 | + print("Unable to run clang-tidy.", file=sys.stderr) |
| 248 | + sys.exit(1) |
| 249 | + |
| 250 | + # Load the database and extract all files. |
| 251 | + database = json.load(open(os.path.join(build_path, db_path))) |
| 252 | + files = [make_absolute(entry['file'], entry['directory']) |
| 253 | + for entry in database] |
| 254 | + |
| 255 | + max_task = args.j |
| 256 | + if max_task == 0: |
| 257 | + max_task = multiprocessing.cpu_count() |
| 258 | + |
| 259 | + tmpdir = None |
| 260 | + if args.fix or (yaml and args.export_fixes): |
| 261 | + check_clang_apply_replacements_binary(args) |
| 262 | + tmpdir = tempfile.mkdtemp() |
| 263 | + |
| 264 | + # Build up a big regexy filter from all command line arguments. |
| 265 | + file_name_re = re.compile('|'.join(args.files)) |
| 266 | + |
| 267 | + return_code = 0 |
| 268 | + try: |
| 269 | + # Spin up a bunch of tidy-launching threads. |
| 270 | + task_queue = queue.Queue(max_task) |
| 271 | + # List of files with a non-zero return code. |
| 272 | + failed_files = [] |
| 273 | + lock = threading.Lock() |
| 274 | + for _ in range(max_task): |
| 275 | + t = threading.Thread(target=run_tidy, |
| 276 | + args=(args, tmpdir, build_path, task_queue, lock, failed_files)) |
| 277 | + t.daemon = True |
| 278 | + t.start() |
| 279 | + |
| 280 | + # Fill the queue with files. |
| 281 | + for name in files: |
| 282 | + if file_name_re.search(name): |
| 283 | + task_queue.put(name) |
| 284 | + |
| 285 | + # Wait for all threads to be done. |
| 286 | + task_queue.join() |
| 287 | + if len(failed_files): |
| 288 | + return_code = 1 |
| 289 | + |
| 290 | + except KeyboardInterrupt: |
| 291 | + # This is a sad hack. Unfortunately subprocess goes |
| 292 | + # bonkers with ctrl-c and we start forking merrily. |
| 293 | + print('\nCtrl-C detected, goodbye.') |
| 294 | + if tmpdir: |
| 295 | + shutil.rmtree(tmpdir) |
| 296 | + os.kill(0, 9) |
| 297 | + |
| 298 | + if yaml and args.export_fixes: |
| 299 | + print('Writing fixes to ' + args.export_fixes + ' ...') |
196 | 300 | try:
|
197 |
| - invocation = [args.clang_tidy_binary, '-list-checks', '-p=' + build_path] |
198 |
| - if args.checks: |
199 |
| - invocation.append('-checks=' + args.checks) |
200 |
| - if args.warningsaserrors: |
201 |
| - invocation.append('-warnings-as-errors=' + args.warningsaserrors) |
202 |
| - invocation.append('-') |
203 |
| - print(subprocess.check_output(invocation)) |
| 301 | + merge_replacement_files(tmpdir, args.export_fixes) |
204 | 302 | except:
|
205 |
| - print("Unable to run clang-tidy.", file=sys.stderr) |
206 |
| - sys.exit(1) |
207 |
| - |
208 |
| - # Load the database and extract all files. |
209 |
| - database = json.load(open(os.path.join(build_path, db_path))) |
210 |
| - files = [entry['file'] for entry in database] |
211 |
| - |
212 |
| - max_task = args.j |
213 |
| - if max_task == 0: |
214 |
| - max_task = multiprocessing.cpu_count() |
215 |
| - |
216 |
| - tmpdir = None |
217 |
| - if args.fix: |
218 |
| - check_clang_apply_replacements_binary(args) |
219 |
| - tmpdir = tempfile.mkdtemp() |
220 |
| - |
221 |
| - # Build up a big regexy filter from all command line arguments. |
222 |
| - file_name_re = re.compile('|'.join(args.files)) |
| 303 | + print('Error exporting fixes.\n', file=sys.stderr) |
| 304 | + traceback.print_exc() |
| 305 | + return_code=1 |
223 | 306 |
|
| 307 | + if args.fix: |
| 308 | + print('Applying fixes ...') |
224 | 309 | try:
|
225 |
| - # Spin up a bunch of tidy-launching threads. |
226 |
| - queue = TidyQueue(max_task) |
227 |
| - for _ in range(max_task): |
228 |
| - t = threading.Thread(target=run_tidy, |
229 |
| - args=(args, tmpdir, build_path, queue)) |
230 |
| - t.daemon = True |
231 |
| - t.start() |
232 |
| - |
233 |
| - # Fill the queue with files. |
234 |
| - for name in files: |
235 |
| - if file_name_re.search(name): |
236 |
| - queue.put(name) |
237 |
| - |
238 |
| - # Wait for all threads to be done. |
239 |
| - queue.join() |
240 |
| - |
241 |
| - # If one clang-tidy process found and error, exit with non-zero |
242 |
| - # status |
243 |
| - if queue.has_error: |
244 |
| - sys.exit(2) |
245 |
| - |
246 |
| - except KeyboardInterrupt: |
247 |
| - # This is a sad hack. Unfortunately subprocess goes |
248 |
| - # bonkers with ctrl-c and we start forking merrily. |
249 |
| - print('\nCtrl-C detected, goodbye.') |
250 |
| - if args.fix: |
251 |
| - shutil.rmtree(tmpdir) |
252 |
| - os.kill(0, 9) |
253 |
| - |
254 |
| - if args.fix: |
255 |
| - print('Applying fixes ...') |
256 |
| - successfully_applied = False |
257 |
| - |
258 |
| - try: |
259 |
| - apply_fixes(args, tmpdir) |
260 |
| - successfully_applied = True |
261 |
| - except: |
262 |
| - print('Error applying fixes.\n', file=sys.stderr) |
263 |
| - traceback.print_exc() |
264 |
| - |
265 |
| - shutil.rmtree(tmpdir) |
266 |
| - if not successfully_applied: |
267 |
| - sys.exit(1) |
| 310 | + apply_fixes(args, tmpdir) |
| 311 | + except: |
| 312 | + print('Error applying fixes.\n', file=sys.stderr) |
| 313 | + traceback.print_exc() |
| 314 | + return_code=1 |
268 | 315 |
|
| 316 | + if tmpdir: |
| 317 | + shutil.rmtree(tmpdir) |
| 318 | + sys.exit(return_code) |
269 | 319 |
|
270 | 320 | if __name__ == '__main__':
|
271 |
| - main() |
| 321 | + main() |
0 commit comments