[PATCH] D49851: [clang-tidy] run-clang-tidy add synchronisation to the output

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 159511.

https://reviews.llvm.org/D49851

Files:
  clang-tidy/tool/run-clang-tidy.py


Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -153,7 +153,7 @@
   subprocess.call(invocation)
 
 
-def run_tidy(args, tmpdir, build_path, queue, failed_files):
+def run_tidy(args, tmpdir, build_path, queue, lock, failed_files):
   """Takes filenames out of queue and runs clang-tidy on them."""
   while True:
 name = queue.get()
@@ -161,10 +161,15 @@
  tmpdir, build_path, args.header_filter,
  args.extra_arg, args.extra_arg_before,
  args.quiet, args.config)
-sys.stdout.write(' '.join(invocation) + '\n')
-return_code = subprocess.call(invocation)
-if return_code != 0:
+
+proc = subprocess.Popen(invocation, stdout=subprocess.PIPE, 
stderr=subprocess.PIPE)
+output, err = proc.communicate()
+if proc.returncode != 0:
   failed_files.append(name)
+with lock:
+  sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
+  if err > 0:
+sys.stderr.write(err + '\n')
 queue.task_done()
 
 
@@ -263,9 +268,10 @@
 task_queue = queue.Queue(max_task)
 # List of files with a non-zero return code.
 failed_files = []
+lock = threading.Lock()
 for _ in range(max_task):
   t = threading.Thread(target=run_tidy,
-   args=(args, tmpdir, build_path, task_queue, 
failed_files))
+   args=(args, tmpdir, build_path, task_queue, lock, 
failed_files))
   t.daemon = True
   t.start()



Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -153,7 +153,7 @@
   subprocess.call(invocation)
 
 
-def run_tidy(args, tmpdir, build_path, queue, failed_files):
+def run_tidy(args, tmpdir, build_path, queue, lock, failed_files):
   """Takes filenames out of queue and runs clang-tidy on them."""
   while True:
 name = queue.get()
@@ -161,10 +161,15 @@
  tmpdir, build_path, args.header_filter,
  args.extra_arg, args.extra_arg_before,
  args.quiet, args.config)
-sys.stdout.write(' '.join(invocation) + '\n')
-return_code = subprocess.call(invocation)
-if return_code != 0:
+
+proc = subprocess.Popen(invocation, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+output, err = proc.communicate()
+if proc.returncode != 0:
   failed_files.append(name)
+with lock:
+  sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
+  if err > 0:
+sys.stderr.write(err + '\n')
 queue.task_done()
 
 
@@ -263,9 +268,10 @@
 task_queue = queue.Queue(max_task)
 # List of files with a non-zero return code.
 failed_files = []
+lock = threading.Lock()
 for _ in range(max_task):
   t = threading.Thread(target=run_tidy,
-   args=(args, tmpdir, build_path, task_queue, failed_files))
+   args=(args, tmpdir, build_path, task_queue, lock, failed_files))
   t.daemon = True
   t.start()

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49851: [clang-tidy] run-clang-tidy add synchronisation to the output

[Andi via Phabricator via cfe-commits]

Abpostelnicu marked 2 inline comments as done.
Abpostelnicu added a comment.

Regarding the time penalty it depends very much on how many files you have. But 
I would choose anytime to have this in the detriment of having "obfuscated" 
output.


https://reviews.llvm.org/D49851



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49851: [clang-tidy] run-clang-tidy add synchronisation to the output

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

Did you notice any significant speed degradation?


https://reviews.llvm.org/D49851



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49851: [clang-tidy] run-clang-tidy add synchronisation to the output

[Andi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL339427: [clang-tidy]  run-clang-tidy.py - add 
synchronisation to the output (authored by Abpostelnicu, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D49851?vs=159511&id=160090#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D49851

Files:
  clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py


Index: clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
@@ -153,18 +153,23 @@
   subprocess.call(invocation)
 
 
-def run_tidy(args, tmpdir, build_path, queue, failed_files):
+def run_tidy(args, tmpdir, build_path, queue, lock, failed_files):
   """Takes filenames out of queue and runs clang-tidy on them."""
   while True:
 name = queue.get()
 invocation = get_tidy_invocation(name, args.clang_tidy_binary, args.checks,
  tmpdir, build_path, args.header_filter,
  args.extra_arg, args.extra_arg_before,
  args.quiet, args.config)
-sys.stdout.write(' '.join(invocation) + '\n')
-return_code = subprocess.call(invocation)
-if return_code != 0:
+
+proc = subprocess.Popen(invocation, stdout=subprocess.PIPE, 
stderr=subprocess.PIPE)
+output, err = proc.communicate()
+if proc.returncode != 0:
   failed_files.append(name)
+with lock:
+  sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
+  if err > 0:
+sys.stderr.write(err + '\n')
 queue.task_done()
 
 
@@ -263,9 +268,10 @@
 task_queue = queue.Queue(max_task)
 # List of files with a non-zero return code.
 failed_files = []
+lock = threading.Lock()
 for _ in range(max_task):
   t = threading.Thread(target=run_tidy,
-   args=(args, tmpdir, build_path, task_queue, 
failed_files))
+   args=(args, tmpdir, build_path, task_queue, lock, 
failed_files))
   t.daemon = True
   t.start()
 


Index: clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
@@ -153,18 +153,23 @@
   subprocess.call(invocation)
 
 
-def run_tidy(args, tmpdir, build_path, queue, failed_files):
+def run_tidy(args, tmpdir, build_path, queue, lock, failed_files):
   """Takes filenames out of queue and runs clang-tidy on them."""
   while True:
 name = queue.get()
 invocation = get_tidy_invocation(name, args.clang_tidy_binary, args.checks,
  tmpdir, build_path, args.header_filter,
  args.extra_arg, args.extra_arg_before,
  args.quiet, args.config)
-sys.stdout.write(' '.join(invocation) + '\n')
-return_code = subprocess.call(invocation)
-if return_code != 0:
+
+proc = subprocess.Popen(invocation, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+output, err = proc.communicate()
+if proc.returncode != 0:
   failed_files.append(name)
+with lock:
+  sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
+  if err > 0:
+sys.stderr.write(err + '\n')
 queue.task_done()
 
 
@@ -263,9 +268,10 @@
 task_queue = queue.Queue(max_task)
 # List of files with a non-zero return code.
 failed_files = []
+lock = threading.Lock()
 for _ in range(max_task):
   t = threading.Thread(target=run_tidy,
-   args=(args, tmpdir, build_path, task_queue, failed_files))
+   args=(args, tmpdir, build_path, task_queue, lock, failed_files))
   t.daemon = True
   t.start()
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49851: [clang-tidy] run-clang-tidy add synchronisation to the output

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

Strangely I haven't had those kind of issues but since you propose those 
modifications I would do one more modification, let's output everything to 
stdout and not stderr by doing:

  if err:
sys.stdout.write(str(err) + '\n')


Repository:
  rL LLVM

https://reviews.llvm.org/D49851



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49851: [clang-tidy] run-clang-tidy add synchronisation to the output

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In https://reviews.llvm.org/D49851#1203676, @JonasToth wrote:

> In https://reviews.llvm.org/D49851#1202764, @Abpostelnicu wrote:
>
> > Strangely I haven't had those kind of issues but since you propose those 
> > modifications I would do one more modification, let's output everything to 
> > stdout and not stderr by doing:
> >
> >   if err:
> > sys.stdout.write(str(err) + '\n')
> >   
>
>
> You can make this a new revision, fixing the `byte` and `str` issues would be 
> more important now.
>
> The `byte` and `str` thingie is since the whole python3 releases or did it 
> change?


I'm not sure this is the fix for this, I think we should specify the encoding 
type when building the string from the byte like:

  if err:
sys.stdout.write(str(err, 'utf-8') + '\n')
   

And of course this must be done only on python 3+ since on python 2 the str 
ctor doesn't accept the encoding parameter.


Repository:
  rL LLVM

https://reviews.llvm.org/D49851



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49851: [clang-tidy] run-clang-tidy add synchronisation to the output

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 162387.

https://reviews.llvm.org/D49851

Files:
  clang-tidy/tool/run-clang-tidy.py


Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -166,10 +166,18 @@
 output, err = proc.communicate()
 if proc.returncode != 0:
   failed_files.append(name)
+
+if is_py2:
+  output_string = output
+  err_string = err
+else:
+  output_string = str(output, 'utf-8')
+  err_string = str(err, 'utf-8')
+
 with lock:
-  sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
-  if err > 0:
-sys.stderr.write(err + '\n')
+  sys.stdout.write(' '.join(invocation) + '\n' + output_string + '\n')
+  if err:
+sys.stderr.write(err_string + '\n')
 queue.task_done()
 
 


Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -166,10 +166,18 @@
 output, err = proc.communicate()
 if proc.returncode != 0:
   failed_files.append(name)
+
+if is_py2:
+  output_string = output
+  err_string = err
+else:
+  output_string = str(output, 'utf-8')
+  err_string = str(err, 'utf-8')
+
 with lock:
-  sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
-  if err > 0:
-sys.stderr.write(err + '\n')
+  sys.stdout.write(' '.join(invocation) + '\n' + output_string + '\n')
+  if err:
+sys.stderr.write(err_string + '\n')
 queue.task_done()
 
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51220: [clang-tidy] run-clang-tidy fails using python 3.7

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 162391.

https://reviews.llvm.org/D51220

Files:
  clang-tidy/tool/run-clang-tidy.py


Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -166,18 +166,10 @@
 output, err = proc.communicate()
 if proc.returncode != 0:
   failed_files.append(name)
-
-if is_py2:
-  output_string = output
-  err_string = err
-else:
-  output_string = str(output, 'utf-8')
-  err_string = str(err, 'utf-8')
-
 with lock:
-  sys.stdout.write(' '.join(invocation) + '\n' + output_string + '\n')
+  sys.stdout.write(' '.join(invocation) + '\n' + output.decode('utf-8') + 
'\n')
   if err:
-sys.stderr.write(err_string + '\n')
+sys.stderr.write(err.decode('utf-8') + '\n')
 queue.task_done()
 
 


Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -166,18 +166,10 @@
 output, err = proc.communicate()
 if proc.returncode != 0:
   failed_files.append(name)
-
-if is_py2:
-  output_string = output
-  err_string = err
-else:
-  output_string = str(output, 'utf-8')
-  err_string = str(err, 'utf-8')
-
 with lock:
-  sys.stdout.write(' '.join(invocation) + '\n' + output_string + '\n')
+  sys.stdout.write(' '.join(invocation) + '\n' + output.decode('utf-8') + '\n')
   if err:
-sys.stderr.write(err_string + '\n')
+sys.stderr.write(err.decode('utf-8') + '\n')
 queue.task_done()
 
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51220: [clang-tidy] run-clang-tidy fails using python 3.7

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In https://reviews.llvm.org/D51220#1212463, @JonasToth wrote:

> I don't know a lot about how to write portable code!
>
> But wouldn't this be out usecase? 
> http://python-future.org/compatible_idioms.html#file-io-with-open
>  Using the `decode` is supposed to work in both pythons


Yes, indeed, this is more elegant solution!


https://reviews.llvm.org/D51220



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51220: [clang-tidy] run-clang-tidy fails using python 3.7

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

I can confirm tested on:
2.7.15
3.7.0

On both it worked.

- F7047650: signature.asc 


https://reviews.llvm.org/D51220



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49851: [clang-tidy] run-clang-tidy add synchronisation to the output

[Andi via Phabricator via cfe-commits]

Abpostelnicu created this revision.
Abpostelnicu added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, xazax.hun.

The goal of this patch is to add synchronisation of the output of the tool with 
the actual files that are verified. Without it the output of clang-tidy will 
get mixed with the clang-tidy command that is also passed to stdout.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49851

Files:
  clang-tidy/tool/run-clang-tidy.py


Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -153,7 +153,7 @@
   subprocess.call(invocation)
 
 
-def run_tidy(args, tmpdir, build_path, queue, failed_files):
+def run_tidy(args, tmpdir, build_path, queue, lock, failed_files):
   """Takes filenames out of queue and runs clang-tidy on them."""
   while True:
 name = queue.get()
@@ -161,10 +161,15 @@
  tmpdir, build_path, args.header_filter,
  args.extra_arg, args.extra_arg_before,
  args.quiet, args.config)
-sys.stdout.write(' '.join(invocation) + '\n')
-return_code = subprocess.call(invocation)
-if return_code != 0:
+
+proc = subprocess.Popen(invocation, stdout=subprocess.PIPE, 
stderr=subprocess.PIPE)
+output, err = proc.communicate()
+if proc.returncode:
   failed_files.append(name)
+with lock:
+  sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
+  if len(err):
+sys.stderr.write(err + '\n')
 queue.task_done()
 
 
@@ -263,9 +268,10 @@
 task_queue = queue.Queue(max_task)
 # List of files with a non-zero return code.
 failed_files = []
+lock = threading.Lock()
 for _ in range(max_task):
   t = threading.Thread(target=run_tidy,
-   args=(args, tmpdir, build_path, task_queue, 
failed_files))
+   args=(args, tmpdir, build_path, task_queue, lock, 
failed_files))
   t.daemon = True
   t.start()
 


Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -153,7 +153,7 @@
   subprocess.call(invocation)
 
 
-def run_tidy(args, tmpdir, build_path, queue, failed_files):
+def run_tidy(args, tmpdir, build_path, queue, lock, failed_files):
   """Takes filenames out of queue and runs clang-tidy on them."""
   while True:
 name = queue.get()
@@ -161,10 +161,15 @@
  tmpdir, build_path, args.header_filter,
  args.extra_arg, args.extra_arg_before,
  args.quiet, args.config)
-sys.stdout.write(' '.join(invocation) + '\n')
-return_code = subprocess.call(invocation)
-if return_code != 0:
+
+proc = subprocess.Popen(invocation, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+output, err = proc.communicate()
+if proc.returncode:
   failed_files.append(name)
+with lock:
+  sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
+  if len(err):
+sys.stderr.write(err + '\n')
 queue.task_done()
 
 
@@ -263,9 +268,10 @@
 task_queue = queue.Queue(max_task)
 # List of files with a non-zero return code.
 failed_files = []
+lock = threading.Lock()
 for _ in range(max_task):
   t = threading.Thread(target=run_tidy,
-   args=(args, tmpdir, build_path, task_queue, failed_files))
+   args=(args, tmpdir, build_path, task_queue, lock, failed_files))
   t.daemon = True
   t.start()
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37014: [clang-tidy] Add a checker to remove useless intermediate variables before return statements with comparisons

[Andi via Phabricator via cfe-commits]

Abpostelnicu added inline comments.



Comment at: clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp:376
+// expression wouldn't really benefit readability. Therefore we abort.
+if (NewReturnLength > MaximumLineLength) {
+  return;

lebedev.ri wrote:
> Is there really no way to format the fixes, and *then* check the line length?
> ```
> $ clang-tidy --help
> ...
>   -format-style=   - 
>  Style for formatting code around applied 
> fixes:
>- 'none' (default) turns off formatting
>- 'file' (literally 'file', not a 
> placeholder)
>  uses .clang-format file in the closest 
> parent
>  directory
>- '{  }' specifies options inline, 
> e.g.
>  -format-style='{BasedOnStyle: llvm, 
> IndentWidth: 8}'
>- 'llvm', 'google', 'webkit', 'mozilla'
>  See clang-format documentation for the 
> up-to-date
>  information about formatting styles and 
> options.
>  This option overrides the 'FormatStyle` 
> option in
>  .clang-tidy file, if any.
> ...
> ```
> so `clang-tidy` is at least aware of `clang-format`.
I think this is doable since I see this in the code:

https://code.woboq.org/llvm/clang-tools-extra/clang-tidy/ClangTidy.cpp.html#199

That leads me to think that we can have this before applying the fixes and in 
case the fix after re-format has a line that violates our rule it gets dropped. 
I'm gonna update the patch with this new addon.


https://reviews.llvm.org/D37014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60453: ClangTidy: Avoid mixing stdout with stderror when dealing with a large number of files.

[Andi via Phabricator via cfe-commits]

Abpostelnicu created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D60453

Files:
  clang-tidy/tool/run-clang-tidy.py


Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -172,6 +172,7 @@
 with lock:
   sys.stdout.write(' '.join(invocation) + '\n' + output.decode('utf-8') + 
'\n')
   if len(err) > 0:
+sys.stdout.flush()
 sys.stderr.write(err.decode('utf-8') + '\n')
 queue.task_done()
 


Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -172,6 +172,7 @@
 with lock:
   sys.stdout.write(' '.join(invocation) + '\n' + output.decode('utf-8') + '\n')
   if len(err) > 0:
+sys.stdout.flush()
 sys.stderr.write(err.decode('utf-8') + '\n')
 queue.task_done()
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60453: ClangTidy: Avoid mixing stdout with stderror when dealing with a large number of files.

[Andi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL357994: ClangTidy: Avoid mixing stdout with stderror when 
dealing with a large number… (authored by Abpostelnicu, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60453/new/

https://reviews.llvm.org/D60453

Files:
  clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py


Index: clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
@@ -172,6 +172,7 @@
 with lock:
   sys.stdout.write(' '.join(invocation) + '\n' + output.decode('utf-8') + 
'\n')
   if len(err) > 0:
+sys.stdout.flush()
 sys.stderr.write(err.decode('utf-8') + '\n')
 queue.task_done()
 


Index: clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
@@ -172,6 +172,7 @@
 with lock:
   sys.stdout.write(' '.join(invocation) + '\n' + output.decode('utf-8') + '\n')
   if len(err) > 0:
+sys.stdout.flush()
 sys.stderr.write(err.decode('utf-8') + '\n')
 queue.task_done()
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51220: [clang-tidy] run-clang-tidy fails using python 3.7

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 166095.

https://reviews.llvm.org/D51220

Files:
  clang-tidy/tool/run-clang-tidy.py


Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -167,9 +167,9 @@
 if proc.returncode != 0:
   failed_files.append(name)
 with lock:
-  sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
-  if err > 0:
-sys.stderr.write(err + '\n')
+  sys.stdout.write(' '.join(invocation) + '\n' + output.decode('utf-8') + 
'\n')
+  if len(err) > 0:
+sys.stderr.write(err.decode('utf-8') + '\n')
 queue.task_done()
 
 


Index: clang-tidy/tool/run-clang-tidy.py
===
--- clang-tidy/tool/run-clang-tidy.py
+++ clang-tidy/tool/run-clang-tidy.py
@@ -167,9 +167,9 @@
 if proc.returncode != 0:
   failed_files.append(name)
 with lock:
-  sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
-  if err > 0:
-sys.stderr.write(err + '\n')
+  sys.stdout.write(' '.join(invocation) + '\n' + output.decode('utf-8') + '\n')
+  if len(err) > 0:
+sys.stderr.write(err.decode('utf-8') + '\n')
 queue.task_done()
 
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51220: [clang-tidy] run-clang-tidy fails using python 3.7

[Andi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL342540: [clang-tidy] run-clang-tidy.py - fails using python 
3.7 (authored by Abpostelnicu, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D51220?vs=166095&id=166096#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D51220

Files:
  clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py


Index: clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
@@ -167,9 +167,9 @@
 if proc.returncode != 0:
   failed_files.append(name)
 with lock:
-  sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
-  if err > 0:
-sys.stderr.write(err + '\n')
+  sys.stdout.write(' '.join(invocation) + '\n' + output.decode('utf-8') + 
'\n')
+  if len(err) > 0:
+sys.stderr.write(err.decode('utf-8') + '\n')
 queue.task_done()
 
 


Index: clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py
@@ -167,9 +167,9 @@
 if proc.returncode != 0:
   failed_files.append(name)
 with lock:
-  sys.stdout.write(' '.join(invocation) + '\n' + output + '\n')
-  if err > 0:
-sys.stderr.write(err + '\n')
+  sys.stdout.write(' '.join(invocation) + '\n' + output.decode('utf-8') + '\n')
+  if len(err) > 0:
+sys.stderr.write(err.decode('utf-8') + '\n')
 queue.task_done()
 
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77882: [clang-tidy] Add option to use alpha checkers from clang-analyzer when using `run-clang-tidy.py`

[Andi via Phabricator via cfe-commits]

Abpostelnicu created this revision.
Abpostelnicu added a reviewer: JonasToth.
Herald added subscribers: cfe-commits, ASDenysPetrov, Charusso, dkrupp, 
donat.nagy, Szelethus, a.sidorin, baloghadamsoftware, xazax.hun.
Herald added a project: clang.
Abpostelnicu edited the summary of this revision.
Abpostelnicu edited the summary of this revision.

Add option to use alpha checkers from clang-analyzer when using 
`run-clang-tidy.py`.
In D46159  has been added the possibility to 
use alpha checkers from `clang-analyzer` but this option hasn't reach 
`run-clang-tidy.py`, the purpose of this patch is to have this option.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77882

Files:
  clang-tools-extra/clang-tidy/tool/run-clang-tidy.py


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -78,10 +78,12 @@
 
 
 def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path,
-header_filter, extra_arg, extra_arg_before, quiet,
-config):
+header_filter, allow_enabling_alpha_checkers,
+extra_arg, extra_arg_before, quiet, config):
   """Gets a command line for clang-tidy."""
   start = [clang_tidy_binary]
+  if allow_enabling_alpha_checkers is not None:
+start.append('-allow-enabling-analyzer-alpha-checkers')
   if header_filter is not None:
 start.append('-header-filter=' + header_filter)
   if checks:
@@ -159,6 +161,7 @@
 name = queue.get()
 invocation = get_tidy_invocation(name, args.clang_tidy_binary, args.checks,
  tmpdir, build_path, args.header_filter,
+ args.allow_enabling_alpha_checkers,
  args.extra_arg, args.extra_arg_before,
  args.quiet, args.config)
 
@@ -179,6 +182,9 @@
'in a compilation database. Requires '
'clang-tidy and clang-apply-replacements in 
'
'$PATH.')
+  parser.add_argument('-allow-enabling-alpha-checkers',
+  action='store_true', help='allow alpha checkers from '
+'clang-analyzer.')
   parser.add_argument('-clang-tidy-binary', metavar='PATH',
   default='clang-tidy',
   help='path to clang-tidy binary')
@@ -238,6 +244,8 @@
 
   try:
 invocation = [args.clang_tidy_binary, '-list-checks']
+if args.allow_enabling_alpha_checkers:
+  invocation.append('-allow-enabling-analyzer-alpha-checkers')
 invocation.append('-p=' + build_path)
 if args.checks:
   invocation.append('-checks=' + args.checks)


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -78,10 +78,12 @@
 
 
 def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path,
-header_filter, extra_arg, extra_arg_before, quiet,
-config):
+header_filter, allow_enabling_alpha_checkers,
+extra_arg, extra_arg_before, quiet, config):
   """Gets a command line for clang-tidy."""
   start = [clang_tidy_binary]
+  if allow_enabling_alpha_checkers is not None:
+start.append('-allow-enabling-analyzer-alpha-checkers')
   if header_filter is not None:
 start.append('-header-filter=' + header_filter)
   if checks:
@@ -159,6 +161,7 @@
 name = queue.get()
 invocation = get_tidy_invocation(name, args.clang_tidy_binary, args.checks,
  tmpdir, build_path, args.header_filter,
+ args.allow_enabling_alpha_checkers,
  args.extra_arg, args.extra_arg_before,
  args.quiet, args.config)
 
@@ -179,6 +182,9 @@
'in a compilation database. Requires '
'clang-tidy and clang-apply-replacements in '
'$PATH.')
+  parser.add_argument('-allow-enabling-alpha-checkers',
+  action='store_true', help='allow alpha checkers from '
+'clang-analyzer.')
   parser.add_argument('-clang-tidy-binary', metavar='PATH',
   default='clang-tidy',
   help='path to clang-tidy binary')
@@ -238,6 +244,8 @@
 
   try:
 invocation = [args.clang_tidy_binary, '-list-checks']
+if args.allow_enabling_alpha

[PATCH] D77882: [clang-tidy] Add option to use alpha checkers from clang-analyzer when using `run-clang-tidy.py`

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 256825.
Abpostelnicu added a comment.

Add release notes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77882/new/

https://reviews.llvm.org/D77882

Files:
  clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
  clang-tools-extra/docs/ReleaseNotes.rst


Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -188,6 +188,11 @@
 - The 'fuchsia-restrict-system-headers' check was renamed to 
:doc:`portability-restrict-system-includes
   `
 
+Other improvements
+^^
+
+- For 'run-clang-tidy.py' add option to use alpha checkers from clang-analyzer.
+
 Improvements to include-fixer
 -
 
@@ -210,4 +215,3 @@
 
 Clang-tidy visual studio plugin
 ---
-
Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -78,10 +78,12 @@
 
 
 def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path,
-header_filter, extra_arg, extra_arg_before, quiet,
-config):
+header_filter, allow_enabling_alpha_checkers,
+extra_arg, extra_arg_before, quiet, config):
   """Gets a command line for clang-tidy."""
   start = [clang_tidy_binary]
+  if allow_enabling_alpha_checkers is not None:
+start.append('-allow-enabling-analyzer-alpha-checkers')
   if header_filter is not None:
 start.append('-header-filter=' + header_filter)
   if checks:
@@ -159,6 +161,7 @@
 name = queue.get()
 invocation = get_tidy_invocation(name, args.clang_tidy_binary, args.checks,
  tmpdir, build_path, args.header_filter,
+ args.allow_enabling_alpha_checkers,
  args.extra_arg, args.extra_arg_before,
  args.quiet, args.config)
 
@@ -179,6 +182,9 @@
'in a compilation database. Requires '
'clang-tidy and clang-apply-replacements in 
'
'$PATH.')
+  parser.add_argument('-allow-enabling-alpha-checkers',
+  action='store_true', help='allow alpha checkers from '
+'clang-analyzer.')
   parser.add_argument('-clang-tidy-binary', metavar='PATH',
   default='clang-tidy',
   help='path to clang-tidy binary')
@@ -238,6 +244,8 @@
 
   try:
 invocation = [args.clang_tidy_binary, '-list-checks']
+if args.allow_enabling_alpha_checkers:
+  invocation.append('-allow-enabling-analyzer-alpha-checkers')
 invocation.append('-p=' + build_path)
 if args.checks:
   invocation.append('-checks=' + args.checks)


Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -188,6 +188,11 @@
 - The 'fuchsia-restrict-system-headers' check was renamed to :doc:`portability-restrict-system-includes
   `
 
+Other improvements
+^^
+
+- For 'run-clang-tidy.py' add option to use alpha checkers from clang-analyzer.
+
 Improvements to include-fixer
 -
 
@@ -210,4 +215,3 @@
 
 Clang-tidy visual studio plugin
 ---
-
Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -78,10 +78,12 @@
 
 
 def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path,
-header_filter, extra_arg, extra_arg_before, quiet,
-config):
+header_filter, allow_enabling_alpha_checkers,
+extra_arg, extra_arg_before, quiet, config):
   """Gets a command line for clang-tidy."""
   start = [clang_tidy_binary]
+  if allow_enabling_alpha_checkers is not None:
+start.append('-allow-enabling-analyzer-alpha-checkers')
   if header_filter is not None:
 start.append('-header-filter=' + header_filter)
   if checks:
@@ -159,6 +161,7 @@
 name = queue.get()
 invocation = get_tidy_invocation(name, args.clang_tidy_binary, args.checks,
  tmpdir, build_path, args.header_filter,
+ args.allow_enabling_alpha_checkers,
  args.extra_arg, args.extra_arg

[PATCH] D77882: [clang-tidy] Add option to use alpha checkers from clang-analyzer when using `run-clang-tidy.py`

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In D77882#1976216 , @sylvestre.ledru 
wrote:

> please add this to the release notes too :)
>  something like "new option -allow-enabling-alpha-checkers added to 
> run-clang-tidy to enable alpha checkers"


Is it ok just to add this to the `clang-tools-extra/docs/ReleaseNotes.rst` 
release notes?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77882/new/

https://reviews.llvm.org/D77882



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81917: [clang-tidy] For `run-clang-tidy.py` escape the paths that are used for analysis.

[Andi via Phabricator via cfe-commits]

Abpostelnicu created this revision.
Abpostelnicu added a reviewer: sylvestre.ledru.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.
Abpostelnicu edited reviewers, added: JonasToth; removed: sylvestre.ledru.

Some paths can have special chars like `file++c.cpp` in this case the regex 
will fail if we don't escape it.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81917

Files:
  clang-tools-extra/clang-tidy/tool/run-clang-tidy.py


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -277,6 +277,7 @@
 tmpdir = tempfile.mkdtemp()
 
   # Build up a big regexy filter from all command line arguments.
+  args.files = [re.escape(f) for f in args.files]
   file_name_re = re.compile('|'.join(args.files))
 
   return_code = 0


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -277,6 +277,7 @@
 tmpdir = tempfile.mkdtemp()
 
   # Build up a big regexy filter from all command line arguments.
+  args.files = [re.escape(f) for f in args.files]
   file_name_re = re.compile('|'.join(args.files))
 
   return_code = 0
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81917: [clang-tidy] For `run-clang-tidy.py` escape the paths that are used for analysis.

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

@njames93 wdyt if we add another parameter to distinguish if we want to use 
regex or not, and if not we escape the paths?
Also thank you so much for catching this up!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81917/new/

https://reviews.llvm.org/D81917



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81917: [clang-tidy] For `run-clang-tidy.py` escape the paths that are used for analysis.

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In D81917#2127690 , @njames93 wrote:

> In D81917#2127477 , @Abpostelnicu 
> wrote:
>
> > @njames93 wdyt if we add another parameter to distinguish if we want to use 
> > regex or not, and if not we escape the paths?
> >  Also thank you so much for catching this up!
>
>
> As the argument is documented as being a regex string, we shouldn't really 
> try to escape it, and users should be expected escape any file names 
> themselves before invoking the script.
>  If for whatever reason you want to use `.c++` as your file extension. then 
> you should pass `r'*\.c\+\+'` to the script.
>
> Can I ask what the specific use case you have that is causing the issue? Are 
> you invoking `run-clang-tidy` from a script that generates the list of files 
> to search for?


Thank you for the detailed reply! we are using this script in automation and 
some files from our repo indeed need to be escaped. I think it makes sense what 
you are saying to give to the consumer the files already escaped. I'm going to 
close this issue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81917/new/

https://reviews.llvm.org/D81917



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82784: [clang-tidy] For `run-clang-tidy.py` do not treat `allow_enabling_alpha_checkers` as a none value.

[Andi via Phabricator via cfe-commits]

Abpostelnicu created this revision.
Abpostelnicu added a reviewer: JonasToth.
Herald added subscribers: cfe-commits, Charusso, xazax.hun.
Herald added a project: clang.
Abpostelnicu edited the summary of this revision.

`allow_enabling_alpha_checkers` will never be None, it will be `True` or 
`False` from argparse so threat it accordingly.
The issue has been discovered by felixn 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82784

Files:
  clang-tools-extra/clang-tidy/tool/run-clang-tidy.py


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -84,7 +84,7 @@
 extra_arg, extra_arg_before, quiet, config):
   """Gets a command line for clang-tidy."""
   start = [clang_tidy_binary]
-  if allow_enabling_alpha_checkers is not None:
+  if allow_enabling_alpha_checkers:
 start.append('-allow-enabling-analyzer-alpha-checkers')
   if header_filter is not None:
 start.append('-header-filter=' + header_filter)


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -84,7 +84,7 @@
 extra_arg, extra_arg_before, quiet, config):
   """Gets a command line for clang-tidy."""
   start = [clang_tidy_binary]
-  if allow_enabling_alpha_checkers is not None:
+  if allow_enabling_alpha_checkers:
 start.append('-allow-enabling-analyzer-alpha-checkers')
   if header_filter is not None:
 start.append('-header-filter=' + header_filter)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78879: [clang-format] [PR45357] Fix issue found with operator spacing

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

I think we are on the right track with this but what if we are dealing with 
`tok::amp` in a context similar to `operator const FallibleTArray&();` I can 
speculate that the outcome will be `operator const FallibleTArray &();` 
which is wrong.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78879/new/

https://reviews.llvm.org/D78879



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78879: [clang-format] [PR45357] Fix issue found with operator spacing

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In D78879#2004724 , @MyDeveloperDay 
wrote:

> @sylvestre.ledru
>
> I'm taking a quick look at formatting the original bug in gecko and whilst 
> the last windows snapshot (Feb 2020) shows the bug
>
>   $ clang-format --version
>   clang-format version 11.0.0
>  
>   nsTArray.h:939:29: warning: code should be clang-formatted 
> [-Wclang-format-violations]
> operator const nsTArray&() {
>   ^
>   nsTArray.h:946:35: warning: code should be clang-formatted 
> [-Wclang-format-violations]
> operator const FallibleTArray&() {
> ^
>   nsTArray.h:1147:59: warning: code should be clang-formatted 
> [-Wclang-format-violations]
> [[nodiscard]] operator const nsTArray_Impl&() const& {
> ^
>   nsTArray.h:1151:43: warning: code should be clang-formatted 
> [-Wclang-format-violations]
> [[nodiscard]] operator const nsTArray&() const& {
> ^
>   nsTArray.h:1154:49: warning: code should be clang-formatted 
> [-Wclang-format-violations]
> [[nodiscard]] operator const FallibleTArray&() const& {
>
>
> The current trunk does not
>
>   $ clang-format --version
>   clang-format version 11.0.0 (https://github.com/llvm/llvm-project 
> 1956a8a7cb79e94dbe073e36eba2d6b003f91046)
>  
>   clang-format -n nsTArray.h
>
>
> Is Mozilla toolchain using LLVM from the v10 branch?
>
> I think this is fixed by D76850: clang-format: Fix pointer alignment for 
> overloaded operators (PR45107) 


Yes, we are using the clang tooling 10.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78879/new/

https://reviews.llvm.org/D78879



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78879: [clang-format] [PR45357] Fix issue found with operator spacing

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

I've cherry-picked D76850  to 10.x and see if 
this fixes the issue.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78879/new/

https://reviews.llvm.org/D78879



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78879: [clang-format] [PR45357] Fix issue found with operator spacing

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In D78879#2004856 , @MyDeveloperDay 
wrote:

> @sylvestre.ledru , @Abpostelnicu
>
> I believe to fix your original request you need a combination of D76850: 
> clang-format: Fix pointer alignment for overloaded operators (PR45107) 
>  and this fix (this fix will handle the `* 
> *` issue in `gecko-dev/ipc/mscom/Ptr.h`
>
> I've run this  new binary over `gecko-dev/ipc/mscom` and `gecko-dev/xpcom/ds` 
> and it shows no clang-format warnings
>
> I hope this helps


Yes, definitely you are right D76850 , only 
partially fixes the issue introduced by D69573 
, and your fix it's a good add-on in order to 
have a complete fix for the regression.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78879/new/

https://reviews.llvm.org/D78879



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78879: [clang-format] [PR45357] Fix issue found with operator spacing

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

Just to clarify something here, for me the patch looks good but until I will 
accept the revision I want to test it on the mozilla codebase.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78879/new/

https://reviews.llvm.org/D78879



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79201: [clang-format] : Fix additional pointer alignment for overloaded operators

[Andi via Phabricator via cfe-commits]

Abpostelnicu requested changes to this revision.
Abpostelnicu added a comment.
This revision now requires changes to proceed.

As per what @sammccall said.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79201/new/

https://reviews.llvm.org/D79201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79201: [clang-format] : Fix additional pointer alignment for overloaded operators

[Andi via Phabricator via cfe-commits]

Abpostelnicu accepted this revision.
Abpostelnicu added a comment.
This revision is now accepted and ready to land.

Sorry, totally forgot about this, thank you!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79201/new/

https://reviews.llvm.org/D79201



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79990: [clang-format] [PR45614] Incorrectly indents [[nodiscard]] attribute funtions after a macro without semicolon

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

@MyDeveloperDay thanks for the patch, I'm gonna run it agains mozilla to see if 
there is any fallout.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79990/new/

https://reviews.llvm.org/D79990



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79990: [clang-format] [PR45614] Incorrectly indents [[nodiscard]] attribute funtions after a macro without semicolon

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

Please see 
https://firefox-source-docs.mozilla.org/code-quality/coding-style/format_cpp_code_with_clang-format.html


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79990/new/

https://reviews.llvm.org/D79990



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75791: [clang-format] Added new option IndentExternBlock

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

Let's first see we don't break anything on `mozilla`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75791/new/

https://reviews.llvm.org/D75791



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54408: [ASTMatchers] Add matchers available through casting to derived

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.
Herald added a project: clang.

@aaron.ballman 
I think the auto usage improves and simplifies the code, however, as I would 
really like to see this code landed, I would be ok to help Steven to finish 
this work and replace auto by the "old" way. Do you think we can have this 
approved if we make the changes mentioned earlier?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54408/new/

https://reviews.llvm.org/D54408



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72333: [clang-tidy] Disable match on `if constexpr` statements in template instantiation for `readability-misleading-indentation` check.

[Andi via Phabricator via cfe-commits]

Abpostelnicu created this revision.
Abpostelnicu added a reviewer: alexfh.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.

Fixes fixes `readability-misleading-identation` for `if constexpr`. This is 
very similar to D71980 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72333

Files:
  clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
@@ -2,6 +2,7 @@
 
 void foo1();
 void foo2();
+void foo3();
 
 #define BLOCK \
   if (cond1)  \
@@ -118,3 +119,14 @@
   #pragma unroll
   for (int k = 0; k < 1; ++k) {}
 }
+
+void shouldPassNoTemplate() {
+  constexpr unsigned Value = 1;
+  if constexpr (Value == 0) {
+foo1();
+  } else if constexpr (Value == 1) {
+foo2();
+  } else {
+foo3();
+  }
+}
Index: clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
@@ -106,7 +106,11 @@
 }
 
 void MisleadingIndentationCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(ifStmt(hasElse(stmt())).bind("if"), this);
+  Finder->addMatcher(
+  ifStmt(allOf(hasElse(stmt()),
+   unless(allOf(isConstexpr(), isInTemplateInstantiation()
+  .bind("if"),
+  this);
   Finder->addMatcher(
   compoundStmt(has(stmt(anyOf(ifStmt(), forStmt(), whileStmt()
   .bind("compound"),


Index: clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
@@ -2,6 +2,7 @@
 
 void foo1();
 void foo2();
+void foo3();
 
 #define BLOCK \
   if (cond1)  \
@@ -118,3 +119,14 @@
   #pragma unroll
   for (int k = 0; k < 1; ++k) {}
 }
+
+void shouldPassNoTemplate() {
+  constexpr unsigned Value = 1;
+  if constexpr (Value == 0) {
+foo1();
+  } else if constexpr (Value == 1) {
+foo2();
+  } else {
+foo3();
+  }
+}
Index: clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
@@ -106,7 +106,11 @@
 }
 
 void MisleadingIndentationCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(ifStmt(hasElse(stmt())).bind("if"), this);
+  Finder->addMatcher(
+  ifStmt(allOf(hasElse(stmt()),
+   unless(allOf(isConstexpr(), isInTemplateInstantiation()
+  .bind("if"),
+  this);
   Finder->addMatcher(
   compoundStmt(has(stmt(anyOf(ifStmt(), forStmt(), whileStmt()
   .bind("compound"),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72333: [clang-tidy] Disable match on `if constexpr` statements in template instantiation for `readability-misleading-indentation` check.

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 236580.
Abpostelnicu added a comment.

Updated the test case to better reflect the changes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72333/new/

https://reviews.llvm.org/D72333

Files:
  clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
@@ -2,6 +2,7 @@
 
 void foo1();
 void foo2();
+void foo3();
 
 #define BLOCK \
   if (cond1)  \
@@ -118,3 +119,52 @@
   #pragma unroll
   for (int k = 0; k < 1; ++k) {}
 }
+
+template
+void mustPass() {
+  if constexpr (b) {
+foo1();
+  } else {
+foo2();
+  }
+}
+
+void mustPassNonTemplate() {
+  constexpr unsigned Value = 1;
+  if constexpr (Value == 0) {
+foo1();
+  } else if constexpr (Value == 1) {
+foo2();
+  } else {
+foo3();
+  }
+}
+
+template
+void mustFail() {
+  if constexpr (b) {
+foo1();
+  }
+else {
+  foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: different indentation for 
'if' and corresponding 'else' [readability-misleading-indentation]
+  }
+}
+
+void mustFailNonTemplate() {
+  constexpr unsigned Value = 1;
+  if constexpr (Value == 0) {
+foo1();
+  }
+else {
+  foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: different indentation for 'if' 
and corresponding 'else' [readability-misleading-indentation]
+  }
+}
+
+void call() {
+  mustPass();
+  mustPass();
+  mustFail();
+  mustFail();
+}
Index: clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
@@ -106,7 +106,11 @@
 }
 
 void MisleadingIndentationCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(ifStmt(hasElse(stmt())).bind("if"), this);
+  Finder->addMatcher(
+  ifStmt(allOf(hasElse(stmt()),
+   unless(allOf(isConstexpr(), isInTemplateInstantiation()
+  .bind("if"),
+  this);
   Finder->addMatcher(
   compoundStmt(has(stmt(anyOf(ifStmt(), forStmt(), whileStmt()
   .bind("compound"),


Index: clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
@@ -2,6 +2,7 @@
 
 void foo1();
 void foo2();
+void foo3();
 
 #define BLOCK \
   if (cond1)  \
@@ -118,3 +119,52 @@
   #pragma unroll
   for (int k = 0; k < 1; ++k) {}
 }
+
+template
+void mustPass() {
+  if constexpr (b) {
+foo1();
+  } else {
+foo2();
+  }
+}
+
+void mustPassNonTemplate() {
+  constexpr unsigned Value = 1;
+  if constexpr (Value == 0) {
+foo1();
+  } else if constexpr (Value == 1) {
+foo2();
+  } else {
+foo3();
+  }
+}
+
+template
+void mustFail() {
+  if constexpr (b) {
+foo1();
+  }
+else {
+  foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: different indentation for 'if' and corresponding 'else' [readability-misleading-indentation]
+  }
+}
+
+void mustFailNonTemplate() {
+  constexpr unsigned Value = 1;
+  if constexpr (Value == 0) {
+foo1();
+  }
+else {
+  foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: different indentation for 'if' and corresponding 'else' [readability-misleading-indentation]
+  }
+}
+
+void call() {
+  mustPass();
+  mustPass();
+  mustFail();
+  mustFail();
+}
Index: clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
@@ -106,7 +106,11 @@
 }
 
 void MisleadingIndentationCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(ifStmt(hasElse(stmt())).bind("if"), this);
+  Finder->addMatcher(
+  ifStmt(allOf(hasElse(stmt()),
+   unless(allOf(isConstexpr(), isInTemplateInstantiation()
+  .bind("if"),
+  this);
   Finder->addMatcher(
   compoundStmt(has(stmt(anyOf(ifStmt(), forStmt(), whileStmt()
   .bind("compound"),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72333: [clang-tidy] Disable match on `if constexpr` statements in template instantiation for `readability-misleading-indentation` check.

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 236596.
Abpostelnicu added a comment.

Updated test with match without compound statement.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72333/new/

https://reviews.llvm.org/D72333

Files:
  clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
@@ -2,6 +2,7 @@
 
 void foo1();
 void foo2();
+void foo3();
 
 #define BLOCK \
   if (cond1)  \
@@ -118,3 +119,58 @@
   #pragma unroll
   for (int k = 0; k < 1; ++k) {}
 }
+
+template
+void mustPass() {
+  if constexpr (b) {
+foo1();
+  } else {
+foo2();
+  }
+}
+
+void mustPassNonTemplate() {
+  constexpr unsigned Value = 1;
+  if constexpr (Value == 0) {
+foo1();
+  } else if constexpr (Value == 1) {
+foo2();
+  } else {
+foo3();
+  }
+}
+
+template
+void mustFail() {
+  if constexpr (b) {
+foo1();
+  }
+else {
+  foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: different indentation for 
'if' and corresponding 'else' [readability-misleading-indentation]
+  }
+}
+
+void mustFailNonTemplate() {
+  constexpr unsigned Value = 1;
+  if constexpr (Value == 0) {
+foo1();
+  }
+else {
+  foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: different indentation for 'if' 
and corresponding 'else' [readability-misleading-indentation]
+  }
+
+  if constexpr (Value == 0)
+foo1();
+else
+  foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: different indentation for 'if' 
and corresponding 'else' [readability-misleading-indentation]
+}
+
+void call() {
+  mustPass();
+  mustPass();
+  mustFail();
+  mustFail();
+}
Index: clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
@@ -106,7 +106,11 @@
 }
 
 void MisleadingIndentationCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(ifStmt(hasElse(stmt())).bind("if"), this);
+  Finder->addMatcher(
+  ifStmt(allOf(hasElse(stmt()),
+   unless(allOf(isConstexpr(), isInTemplateInstantiation()
+  .bind("if"),
+  this);
   Finder->addMatcher(
   compoundStmt(has(stmt(anyOf(ifStmt(), forStmt(), whileStmt()
   .bind("compound"),


Index: clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
@@ -2,6 +2,7 @@
 
 void foo1();
 void foo2();
+void foo3();
 
 #define BLOCK \
   if (cond1)  \
@@ -118,3 +119,58 @@
   #pragma unroll
   for (int k = 0; k < 1; ++k) {}
 }
+
+template
+void mustPass() {
+  if constexpr (b) {
+foo1();
+  } else {
+foo2();
+  }
+}
+
+void mustPassNonTemplate() {
+  constexpr unsigned Value = 1;
+  if constexpr (Value == 0) {
+foo1();
+  } else if constexpr (Value == 1) {
+foo2();
+  } else {
+foo3();
+  }
+}
+
+template
+void mustFail() {
+  if constexpr (b) {
+foo1();
+  }
+else {
+  foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: different indentation for 'if' and corresponding 'else' [readability-misleading-indentation]
+  }
+}
+
+void mustFailNonTemplate() {
+  constexpr unsigned Value = 1;
+  if constexpr (Value == 0) {
+foo1();
+  }
+else {
+  foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: different indentation for 'if' and corresponding 'else' [readability-misleading-indentation]
+  }
+
+  if constexpr (Value == 0)
+foo1();
+else
+  foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: different indentation for 'if' and corresponding 'else' [readability-misleading-indentation]
+}
+
+void call() {
+  mustPass();
+  mustPass();
+  mustFail();
+  mustFail();
+}
Index: clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
@@ -106,7 +106,11 @@
 }
 
 void MisleadingIndentationCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(ifStmt(hasElse(stmt())).bind("if"), this);
+  Finder->addMatcher(
+  ifStmt(allOf(hasElse(stmt()),
+   unless(allOf

[PATCH] D72333: [clang-tidy] Disable match on `if constexpr` statements in template instantiation for `readability-misleading-indentation` check.

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 236796.
Abpostelnicu added a comment.

Added more tesst for templa functions that are not instantiated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72333/new/

https://reviews.llvm.org/D72333

Files:
  clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
@@ -2,6 +2,7 @@
 
 void foo1();
 void foo2();
+void foo3();
 
 #define BLOCK \
   if (cond1)  \
@@ -118,3 +119,79 @@
   #pragma unroll
   for (int k = 0; k < 1; ++k) {}
 }
+
+template
+void mustPass() {
+  if constexpr (b) {
+foo1();
+  } else {
+foo2();
+  }
+}
+
+void mustPassNonTemplate() {
+  constexpr unsigned Value = 1;
+  if constexpr (Value == 0) {
+foo1();
+  } else if constexpr (Value == 1) {
+foo2();
+  } else {
+foo3();
+  }
+}
+
+template
+void mustFail() {
+  if constexpr (b) {
+foo1();
+  }
+else {
+  foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: different indentation for 'if' and corresponding 'else' [readability-misleading-indentation]
+  }
+}
+
+void mustFailNonTemplate() {
+  constexpr unsigned Value = 1;
+  if constexpr (Value == 0) {
+foo1();
+  }
+else {
+  foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: different indentation for 'if' and corresponding 'else' [readability-misleading-indentation]
+  }
+
+  if constexpr (Value == 0)
+foo1();
+else
+  foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: different indentation for 'if' and corresponding 'else' [readability-misleading-indentation]
+}
+
+template
+void mustFailNoInsta() {
+  if constexpr (b) {
+foo1();
+  }
+else {
+  foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: different indentation for 'if' and corresponding 'else' [readability-misleading-indentation]
+  }
+}
+
+template
+void mustPassNoInsta() {
+  if constexpr (b) {
+foo1();
+  }
+  else {
+foo2();
+  }
+}
+
+void call() {
+  mustPass();
+  mustPass();
+  mustFail();
+  mustFail();
+}
Index: clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MisleadingIndentationCheck.cpp
@@ -106,7 +106,11 @@
 }
 
 void MisleadingIndentationCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(ifStmt(hasElse(stmt())).bind("if"), this);
+  Finder->addMatcher(
+  ifStmt(allOf(hasElse(stmt()),
+   unless(allOf(isConstexpr(), isInTemplateInstantiation()
+  .bind("if"),
+  this);
   Finder->addMatcher(
   compoundStmt(has(stmt(anyOf(ifStmt(), forStmt(), whileStmt()
   .bind("compound"),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72333: [clang-tidy] Disable match on `if constexpr` statements in template instantiation for `readability-misleading-indentation` check.

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

I think the best approach here is to remove the tests that don’t have template 
instantiation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72333/new/

https://reviews.llvm.org/D72333



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72333: [clang-tidy] Disable match on `if constexpr` statements in template instantiation for `readability-misleading-indentation` check.

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

Will push shortly an update.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72333/new/

https://reviews.llvm.org/D72333



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72438: [clang-tidy] For checker `readability-misleading-indentation` update tests.

[Andi via Phabricator via cfe-commits]

Abpostelnicu created this revision.
Abpostelnicu added a reviewer: JonasToth.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.
Abpostelnicu edited the summary of this revision.
Abpostelnicu added a project: clang-tools-extra.

In D72333  we've introduced support for `if 
constexpr` but the test for uninstantiated template was not ready to land on 
windows platform since this target uses `-fdelayed-template-parsing` by 
default. This patch addresses this by passing `-fno-delayed-template-parsing` 
to the test.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72438

Files:
  
clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s readability-misleading-indentation %t
+// RUN: %check_clang_tidy %s readability-misleading-indentation %t -- -- 
-fno-delayed-template-parsing
 
 void foo1();
 void foo2();
@@ -168,6 +168,17 @@
   // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: different indentation for 'if' 
and corresponding 'else' [readability-misleading-indentation]
 }
 
+template
+void mustFailNoInsta() {
+  if constexpr (b) {
+foo1();
+  }
+else {
+  foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: different indentation for 
'if' and corresponding 'else' [readability-misleading-indentation]
+  }
+}
+
 template
 void mustPassNoInsta() {
   if constexpr (b) {


Index: clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-misleading-indentation.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s readability-misleading-indentation %t
+// RUN: %check_clang_tidy %s readability-misleading-indentation %t -- -- -fno-delayed-template-parsing
 
 void foo1();
 void foo2();
@@ -168,6 +168,17 @@
   // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: different indentation for 'if' and corresponding 'else' [readability-misleading-indentation]
 }
 
+template
+void mustFailNoInsta() {
+  if constexpr (b) {
+foo1();
+  }
+else {
+  foo2();
+  // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: different indentation for 'if' and corresponding 'else' [readability-misleading-indentation]
+  }
+}
+
 template
 void mustPassNoInsta() {
   if constexpr (b) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72438: [clang-tidy] For checker `readability-misleading-indentation` update tests.

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In D72438#1811512 , @lebedev.ri wrote:

> Hm, didn't clang gain such a diagnostic itself recently? 
> https://godbolt.org/z/MYJTvw
>  Wouldn't it make sense to migrate everything into it, and drop this 
> now-duplicating check?


True, but for the moment we are currently using this check in our review-time 
infrastructure si I'm not sure If we want to drop this just yet.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72438/new/

https://reviews.llvm.org/D72438



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77882: [clang-tidy] Add option to use alpha checkers from clang-analyzer when using `run-clang-tidy.py`

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In D77882#2274292 , @hans wrote:

> This seems to have caused https://bugs.llvm.org/show_bug.cgi?id=47512
>
> Can you take a look?

Are you sure? This has been fixed from august in trunk 
https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py#L87


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77882/new/

https://reviews.llvm.org/D77882

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77882: [clang-tidy] Add option to use alpha checkers from clang-analyzer when using `run-clang-tidy.py`

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

Has been pushed to the 11.x branch 
https://github.com/llvm/llvm-project/commit/1596c2dfd548b21cf33ad3353882ac465d78c1bb


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77882/new/

https://reviews.llvm.org/D77882

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D93543: clang-tidy: Leave the possibility of opting out having coloured diagnostic messages.

[Andi via Phabricator via cfe-commits]

Abpostelnicu created this revision.
Abpostelnicu requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Afer this 

 commit landed we always have coloured diagnotic messages. Maybe it's a good 
idea to leave the user the posibility of opting out of this.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D93543

Files:
  clang-tools-extra/clang-tidy/tool/run-clang-tidy.py


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -80,10 +80,13 @@
 
 
 def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path,
-header_filter, allow_enabling_alpha_checkers,
-extra_arg, extra_arg_before, quiet, config):
+header_filter, no_diagnostic_color,
+allow_enabling_alpha_checkers, extra_arg,
+extra_arg_before, quiet, config):
   """Gets a command line for clang-tidy."""
-  start = [clang_tidy_binary, '--use-color']
+  start = [clang_tidy_binary]
+  if not no_diagnostic_color:
+start.append('--use-color')
   if allow_enabling_alpha_checkers:
 start.append('-allow-enabling-analyzer-alpha-checkers')
   if header_filter is not None:
@@ -163,6 +166,7 @@
 name = queue.get()
 invocation = get_tidy_invocation(name, args.clang_tidy_binary, args.checks,
  tmpdir, build_path, args.header_filter,
+ args.no_diagnostic_color,
  args.allow_enabling_alpha_checkers,
  args.extra_arg, args.extra_arg_before,
  args.quiet, args.config)
@@ -204,6 +208,8 @@
   'When the value is empty, clang-tidy will '
   'attempt to find a file named .clang-tidy for '
   'each source file in its parent directories.')
+  parser.add_argument('-no-diagnostic-color', action='store_true',
+  help='Disable diagnostic message colors.')
   parser.add_argument('-header-filter', default=None,
   help='regular expression matching the names of the '
   'headers to output diagnostics from. Diagnostics from '


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -80,10 +80,13 @@
 
 
 def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path,
-header_filter, allow_enabling_alpha_checkers,
-extra_arg, extra_arg_before, quiet, config):
+header_filter, no_diagnostic_color,
+allow_enabling_alpha_checkers, extra_arg,
+extra_arg_before, quiet, config):
   """Gets a command line for clang-tidy."""
-  start = [clang_tidy_binary, '--use-color']
+  start = [clang_tidy_binary]
+  if not no_diagnostic_color:
+start.append('--use-color')
   if allow_enabling_alpha_checkers:
 start.append('-allow-enabling-analyzer-alpha-checkers')
   if header_filter is not None:
@@ -163,6 +166,7 @@
 name = queue.get()
 invocation = get_tidy_invocation(name, args.clang_tidy_binary, args.checks,
  tmpdir, build_path, args.header_filter,
+ args.no_diagnostic_color,
  args.allow_enabling_alpha_checkers,
  args.extra_arg, args.extra_arg_before,
  args.quiet, args.config)
@@ -204,6 +208,8 @@
   'When the value is empty, clang-tidy will '
   'attempt to find a file named .clang-tidy for '
   'each source file in its parent directories.')
+  parser.add_argument('-no-diagnostic-color', action='store_true',
+  help='Disable diagnostic message colors.')
   parser.add_argument('-header-filter', default=None,
   help='regular expression matching the names of the '
   'headers to output diagnostics from. Diagnostics from '
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79388: [clang-format] Fix AlignConsecutive on PP blocks

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In D79388#2321231 , @MyDeveloperDay 
wrote:

> Not to my knowledge, if we are not going to fix it we should probably revert 
> it for now, we can bring it back later

I'm a strong supporter of reverting it for the moment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79388/new/

https://reviews.llvm.org/D79388

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

I think there is a false positive with this @george.burgess.iv:
In 
[this)(https://searchfox.org/mozilla-central/source/mozglue/baseprofiler/core/platform-linux-android.cpp#222-227)
 we have the warning triggered: 
mozglue/baseprofiler/core/platform-linux-android.cpp:216:9: error: variable 'r' 
set but not used [-Werror,-Wunused-but-set-variable]

  SigHandlerCoordinator() {
PodZero(&mUContext);
int r = sem_init(&mMessage2, /* pshared */ 0, 0);
r |= sem_init(&mMessage3, /* pshared */ 0, 0);
r |= sem_init(&mMessage4, /* pshared */ 0, 0);
MOZ_ASSERT(r == 0);
  }


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In D100581#2798079 , @raj.khem wrote:

> http://sprunge.us/FJzZXL is a file from harfbuzz and it warns
>
>   a.cc:28670:32: error: variable 'supp_size' set but not used 
> [-Werror,-Wunused-but-set-variable]
>   unsigned int size0, size1, supp_size = 0;

You have, with pragma gcc error, look at hb.hh. The problem is that clang has 
an error dealing with cascading pragma when setting different levels.

> I do not have -Werror enabled but it still is reported as error. There is no 
> way to disable this warning ?

Go to the hb GitHub repo, I’ve fixed this in master branch, it will propagate 
soon to a new release.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121593: [clangd][WIP] Provide clang-include-cleaner

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

@sammccall was there any progress on this so far? This would be really useful 
to be a clang-tidy check.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121593/new/

https://reviews.llvm.org/D121593

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Andi via Phabricator via cfe-commits]

Abpostelnicu added inline comments.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:88
+
+  if (!Sources.isInMainFile(ND.getBeginLoc()))
+return;

This breaks the usage of this checker in an unified build scenario since the 
main file for the translation unit will be the unified file. 


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52136/new/

https://reviews.llvm.org/D52136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74031: Update for Clang 10 release notes in order to have reference to D66404.

[Andi via Phabricator via cfe-commits]

Abpostelnicu created this revision.
Abpostelnicu added a reviewer: hans.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Abpostelnicu edited the summary of this revision.
Abpostelnicu edited the summary of this revision.
Abpostelnicu added a subscriber: sylvestre.ledru.

Since D66404   adds some significant 
modifications to the `CFG` we should include it in the release notes.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74031

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -183,7 +183,8 @@
 libclang
 
 
-- ...
+- Various changes to reduce discrepancies in destructor calls between the
+  generated ``CFG`` and the actual ``codegen``.
 
 Static Analyzer
 ---


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -183,7 +183,8 @@
 libclang
 
 
-- ...
+- Various changes to reduce discrepancies in destructor calls between the
+  generated ``CFG`` and the actual ``codegen``.
 
 Static Analyzer
 ---
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74031: Update for Clang 10 release notes in order to have reference to D66404.

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In D74031#1859084 , @hans wrote:

> lgtm, but if you want it wouldn't hurt to include even more details from the 
> commit message.


This is a good idea, in my latest patch you will see that I've added more 
context present in D66404 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74031/new/

https://reviews.llvm.org/D74031



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74031: Update for Clang 10 release notes in order to have reference to D66404.

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 242562.
Abpostelnicu added a comment.

Adding extra comments to the release patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74031/new/

https://reviews.llvm.org/D74031

Files:
  clang/docs/ReleaseNotes.rst


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -183,7 +183,24 @@
 libclang
 
 
-- ...
+- Various changes to reduce discrepancies in destructor calls between the
+  generated ``CFG`` and the actual ``codegen``.
+
+  In particular:
+
+  - Respect C++17 copy elision; previously it would generate destructor calls
+for elided temporaries, including in initialization and return statements.
+
+  - Don't generate duplicate destructor calls for statement expressions.
+
+  - Fix initialization lists.
+
+  - Fix comma operator.
+
+  - Change printing of implicit destructors to print the type instead of the
+class name directly, matching the code for temporary object destructors.
+The class name was blank for lambdas.
+
 
 Static Analyzer
 ---


Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -183,7 +183,24 @@
 libclang
 
 
-- ...
+- Various changes to reduce discrepancies in destructor calls between the
+  generated ``CFG`` and the actual ``codegen``.
+
+  In particular:
+
+  - Respect C++17 copy elision; previously it would generate destructor calls
+for elided temporaries, including in initialization and return statements.
+
+  - Don't generate duplicate destructor calls for statement expressions.
+
+  - Fix initialization lists.
+
+  - Fix comma operator.
+
+  - Change printing of implicit destructors to print the type instead of the
+class name directly, matching the code for temporary object destructors.
+The class name was blank for lambdas.
+
 
 Static Analyzer
 ---
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74031: Update for Clang 10 release notes in order to have reference to D66404.

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In D74031#1859231 , @hans wrote:

> Looks great, thanks!
>
> Do you have commit access? If so, go ahead and push directly to the 10.x 
> branch, otherwise let me know and I'll do it for you.


Yes I have commit access, pushing it directly to `10.x` then.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74031/new/

https://reviews.llvm.org/D74031



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74031: Update for Clang 10 release notes in order to have reference to D66404.

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

done


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74031/new/

https://reviews.llvm.org/D74031



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39121: [clang-tidy] Misplaced Operator in Strlen in Alloc

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

I can test this on our repo, Mozilla, since it's a large code-base I think we 
you will have a better understanding of the false-positive ratio.


https://reviews.llvm.org/D39121



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 90171.
Abpostelnicu added a comment.

Also added tests.


Repository:
  rL LLVM

https://reviews.llvm.org/D30487

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1011,6 +1011,29 @@
   verifyFormat("class ::A::B {};");
 }
 
+TEST_F(FormatTest, BreakBeforeInhertianceDelimiter) {
+  FormatStyle StyleWithInheritanceBreak = getLLVMStyle();
+  LocalStyle.BreakBeforeInhertianceDelimiter = true;
+
+  verifyFormat("class MyClass : public X {}", LocalStyle);
+  verifyFormat("class MyClass\n"
+   ": public X\n"
+   ", public Y // some difficult comment\n"
+   ", public Z {}",
+   StyleWithInheritanceBreak);
+  verifyFormat("class MyClass"
+   "  : public X\n"
+   "  , public Y // some difficult comment\n"
+   "  , public Z\n",
+   StyleWithInheritanceBreak);
+  verifyFormat("class MyClass\n"
+   ": public X // When deriving from more than one class, put "
+   "each on its own\n"
+   "   // line.\n"
+   ", public Y {}",
+   StyleWithInheritanceBreak);
+}
+
 TEST_F(FormatTest, FormatsVariableDeclarationsAfterStructOrClass) {
   verifyFormat("class A {\n} a, b;");
   verifyFormat("struct A {\n} a, b;");
@@ -8459,6 +8482,7 @@
   CHECK_PARSE_BOOL(BreakBeforeTernaryOperators);
   CHECK_PARSE_BOOL(BreakConstructorInitializersBeforeComma);
   CHECK_PARSE_BOOL(BreakStringLiterals);
+  CHECK_PARSE_BOOL(BreakBeforeInhertianceDelimiter)
   CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -676,6 +676,8 @@
 case tok::comma:
   if (Contexts.back().InCtorInitializer)
 Tok->Type = TT_CtorInitializerComma;
+  else if (Contexts.back().InInhertiance)
+Tok->Type = TT_InheritanceComma;
   else if (Contexts.back().FirstStartOfName &&
(Contexts.size() == 1 || Line.startsWith(tok::kw_for))) {
 Contexts.back().FirstStartOfName->PartOfMultiVariableDeclStmt = true;
@@ -945,6 +947,7 @@
 bool CanBeExpression = true;
 bool InTemplateArgument = false;
 bool InCtorInitializer = false;
+bool InInhertiance = false;
 bool CaretFound = false;
 bool IsForEachMacro = false;
   };
@@ -1004,6 +1007,10 @@
Current.Previous->is(TT_CtorInitializerColon)) {
   Contexts.back().IsExpression = true;
   Contexts.back().InCtorInitializer = true;
+} else if (Current.Previous &&
+   Current.Previous->is(TT_InheritanceColon)) {
+  Contexts.back().IsExpression = true;
+  Contexts.back().InInhertiance = true;
 } else if (Current.isOneOf(tok::r_paren, tok::greater, tok::comma)) {
   for (FormatToken *Previous = Current.Previous;
Previous && Previous->isOneOf(tok::star, tok::amp);
@@ -2377,6 +2384,15 @@
  !Tok.isOneOf(TT_ObjCBlockLBrace, TT_DictLiteral);
 }
 
+// Determine if the next token from the closing scope is an inheritance token
+static bool hasMultipleInheritance(const FormatToken &Tok) {
+  // Look for a column of type TT_InheritanceComma untill we find it or
+  // we find a declaration termination
+  for (const FormatToken* nextTok = Tok.Next; nextTok; nextTok = nextTok->Next)
+if (nextTok->is(TT_InheritanceComma)) return true;
+  return false;
+}
+  
 bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line,
  const FormatToken &Right) {
   const FormatToken &Left = *Right.Previous;
@@ -2460,6 +2476,11 @@
   Style.BreakConstructorInitializersBeforeComma &&
   !Style.ConstructorInitializerAllOnOneLineOrOnePerLine)
 return true;
+  //Break only if we have multiple inheritance
+  if (Style.BreakBeforeInhertianceDelimiter &&
+  ((Right.is(TT_InheritanceColon) && hasMultipleInheritance(Right)) ||
+  Right.is(TT_InheritanceComma)))
+return true;
   if (Right.is(tok::string_literal) && Right.TokenText.startswith("R\""))
 // Raw string literals are special wrt. line breaks. The author has made a
 // deliberate choice and might have aligned the contents of the string
@@ -2634,6 +2655,12 @@
   if (Right.is(TT_CtorInitializerComma) &&
   Style.BreakConstructorInitializersBeforeComma)
 return true;
+  if (Left.is(TT_InheritanceComma) &&
+  S

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 90176.
Abpostelnicu added a comment.

added patch against the updated repo.


Repository:
  rL LLVM

https://reviews.llvm.org/D30487

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1020,6 +1020,29 @@
   verifyFormat("class ::A::B {};");
 }
 
+TEST_F(FormatTest, BreakBeforeInhertianceDelimiter) {
+  FormatStyle StyleWithInheritanceBreak = getLLVMStyle();
+  LocalStyle.BreakBeforeInhertianceDelimiter = true;
+
+  verifyFormat("class MyClass : public X {}", LocalStyle);
+  verifyFormat("class MyClass\n"
+   ": public X\n"
+   ", public Y // some difficult comment\n"
+   ", public Z {}",
+   StyleWithInheritanceBreak);
+  verifyFormat("class MyClass"
+   "  : public X\n"
+   "  , public Y // some difficult comment\n"
+   "  , public Z\n",
+   StyleWithInheritanceBreak);
+  verifyFormat("class MyClass\n"
+   ": public X // When deriving from more than one class, put "
+   "each on its own\n"
+   "   // line.\n"
+   ", public Y {}",
+   StyleWithInheritanceBreak);
+}
+
 TEST_F(FormatTest, FormatsVariableDeclarationsAfterStructOrClass) {
   verifyFormat("class A {\n} a, b;");
   verifyFormat("struct A {\n} a, b;");
@@ -8468,6 +8491,7 @@
   CHECK_PARSE_BOOL(BreakBeforeTernaryOperators);
   CHECK_PARSE_BOOL(BreakConstructorInitializersBeforeComma);
   CHECK_PARSE_BOOL(BreakStringLiterals);
+  CHECK_PARSE_BOOL(BreakBeforeInhertianceDelimiter)
   CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -676,6 +676,8 @@
 case tok::comma:
   if (Contexts.back().InCtorInitializer)
 Tok->Type = TT_CtorInitializerComma;
+  else if (Contexts.back().InInhertiance)
+Tok->Type = TT_InheritanceComma;
   else if (Contexts.back().FirstStartOfName &&
(Contexts.size() == 1 || Line.startsWith(tok::kw_for))) {
 Contexts.back().FirstStartOfName->PartOfMultiVariableDeclStmt = true;
@@ -945,6 +947,7 @@
 bool CanBeExpression = true;
 bool InTemplateArgument = false;
 bool InCtorInitializer = false;
+bool InInhertiance = false;
 bool CaretFound = false;
 bool IsForEachMacro = false;
   };
@@ -1004,6 +1007,10 @@
Current.Previous->is(TT_CtorInitializerColon)) {
   Contexts.back().IsExpression = true;
   Contexts.back().InCtorInitializer = true;
+} else if (Current.Previous &&
+   Current.Previous->is(TT_InheritanceColon)) {
+  Contexts.back().IsExpression = true;
+  Contexts.back().InInhertiance = true;
 } else if (Current.isOneOf(tok::r_paren, tok::greater, tok::comma)) {
   for (FormatToken *Previous = Current.Previous;
Previous && Previous->isOneOf(tok::star, tok::amp);
@@ -2388,6 +2395,15 @@
  !Tok.isOneOf(TT_ObjCBlockLBrace, TT_DictLiteral);
 }
 
+// Determine if the next token from the closing scope is an inheritance token
+static bool hasMultipleInheritance(const FormatToken &Tok) {
+  // Look for a column of type TT_InheritanceComma untill we find it or
+  // we find a declaration termination
+  for (const FormatToken* nextTok = Tok.Next; nextTok; nextTok = nextTok->Next)
+if (nextTok->is(TT_InheritanceComma)) return true;
+  return false;
+}
+  
 bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line,
  const FormatToken &Right) {
   const FormatToken &Left = *Right.Previous;
@@ -2471,6 +2487,11 @@
   Style.BreakConstructorInitializersBeforeComma &&
   !Style.ConstructorInitializerAllOnOneLineOrOnePerLine)
 return true;
+  //Break only if we have multiple inheritance
+  if (Style.BreakBeforeInhertianceDelimiter &&
+  ((Right.is(TT_InheritanceColon) && hasMultipleInheritance(Right)) ||
+  Right.is(TT_InheritanceComma)))
+return true;
   if (Right.is(tok::string_literal) && Right.TokenText.startswith("R\""))
 // Raw string literals are special wrt. line breaks. The author has made a
 // deliberate choice and might have aligned the contents of the string
@@ -2648,6 +2669,12 @@
   if (Right.is(TT_CtorInitializerComma) &&
   Style.BreakConstructorInitializersBeforeComma)
 return true;
+  if (Left.is(TT_Inheritan

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Andi via Phabricator via cfe-commits]

Abpostelnicu marked 9 inline comments as done.
Abpostelnicu added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:2398
 
+// Determine if the next token from the closing scope is an inheritance token
+static bool hasMultipleInheritance(const FormatToken &Tok) {

djasper wrote:
> I don't understand this comment or what the function is supposed to do. It 
> seems to search whether there is an inheritance comma somewhere in the rest 
> of the line.
I've corrected the comment, if we have single inheritance we don't want to 
break onto the next line before the inheritance operand, that's why i'm looking 
for a TT_InheritanceComma, because if there is one then we are dealing with 
multiple inheritance.  


Repository:
  rL LLVM

https://reviews.llvm.org/D30487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 90300.
Abpostelnicu marked an inline comment as done.
Abpostelnicu added a comment.

Updated patch with the proposed modifications.


Repository:
  rL LLVM

https://reviews.llvm.org/D30487

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1030,6 +1030,17 @@
   verifyFormat("class ::A::B {};");
 }
 
+TEST_F(FormatTest, BreakInhertianceBeforeColonAndComma) {
+  FormatStyle StyleWithInheritanceBreak = getLLVMStyle();
+  LocalStyle.BreakInhertianceBeforeColonAndComma = true;
+
+  verifyFormat("class MyClass : public X {}", LocalStyle);
+  verifyFormat("class MyClass\n"
+   ": public X\n"
+   ", public Y {}",
+   StyleWithInheritanceBreak);
+}
+
 TEST_F(FormatTest, FormatsVariableDeclarationsAfterStructOrClass) {
   verifyFormat("class A {\n} a, b;");
   verifyFormat("struct A {\n} a, b;");
@@ -8492,6 +8503,7 @@
   CHECK_PARSE_BOOL(BreakBeforeTernaryOperators);
   CHECK_PARSE_BOOL(BreakConstructorInitializersBeforeComma);
   CHECK_PARSE_BOOL(BreakStringLiterals);
+  CHECK_PARSE_BOOL(BreakInhertianceBeforeColonAndComma)
   CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -676,6 +676,8 @@
 case tok::comma:
   if (Contexts.back().InCtorInitializer)
 Tok->Type = TT_CtorInitializerComma;
+  else if (Contexts.back().InInhertianceList)
+Tok->Type = TT_InheritanceComma;
   else if (Contexts.back().FirstStartOfName &&
(Contexts.size() == 1 || Line.startsWith(tok::kw_for))) {
 Contexts.back().FirstStartOfName->PartOfMultiVariableDeclStmt = true;
@@ -945,6 +947,7 @@
 bool CanBeExpression = true;
 bool InTemplateArgument = false;
 bool InCtorInitializer = false;
+bool InInhertianceList = false;
 bool CaretFound = false;
 bool IsForEachMacro = false;
   };
@@ -1004,6 +1007,9 @@
Current.Previous->is(TT_CtorInitializerColon)) {
   Contexts.back().IsExpression = true;
   Contexts.back().InCtorInitializer = true;
+} else if (Current.Previous &&
+   Current.Previous->is(TT_InheritanceColon)) {
+  Contexts.back().InInhertianceList = true;
 } else if (Current.isOneOf(tok::r_paren, tok::greater, tok::comma)) {
   for (FormatToken *Previous = Current.Previous;
Previous && Previous->isOneOf(tok::star, tok::amp);
@@ -2390,6 +2396,13 @@
  !Tok.isOneOf(TT_ObjCBlockLBrace, TT_DictLiteral);
 }
 
+// Returns 'true' if there is an TT_InheritanceComma in the current token list.
+static bool hasMultipleInheritance(const FormatToken &Tok) {
+  for (const FormatToken* nextTok = Tok.Next; nextTok; nextTok = nextTok->Next)
+if (nextTok->is(TT_InheritanceComma)) return true;
+  return false;
+}
+  
 bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line,
  const FormatToken &Right) {
   const FormatToken &Left = *Right.Previous;
@@ -2473,6 +2486,11 @@
   Style.BreakConstructorInitializersBeforeComma &&
   !Style.ConstructorInitializerAllOnOneLineOrOnePerLine)
 return true;
+  // Break only if we have multiple inheritance.
+  if (Style.BreakInhertianceBeforeColonAndComma &&
+  ((Right.is(TT_InheritanceColon) && hasMultipleInheritance(Right)) ||
+  Right.is(TT_InheritanceComma)))
+return true;
   if (Right.is(tok::string_literal) && Right.TokenText.startswith("R\""))
 // Raw string literals are special wrt. line breaks. The author has made a
 // deliberate choice and might have aligned the contents of the string
@@ -2652,6 +2670,12 @@
   if (Right.is(TT_CtorInitializerComma) &&
   Style.BreakConstructorInitializersBeforeComma)
 return true;
+  if (Left.is(TT_InheritanceComma) &&
+  Style.BreakInhertianceBeforeColonAndComma)
+return false;
+  if (Right.is(TT_InheritanceComma) &&
+  Style.BreakInhertianceBeforeColonAndComma)
+return true;
   if ((Left.is(tok::greater) && Right.is(tok::greater)) ||
   (Left.is(tok::less) && Right.is(tok::less)))
 return false;
Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -48,6 +48,7 @@
   TYPE(FunctionTypeLParen) \
   TYPE(ImplicitStringLiteral) \
   TYPE(InheritanceColon) \
+  TYPE(InheritanceCom

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Andi via Phabricator via cfe-commits]

Abpostelnicu added inline comments.



Comment at: include/clang/Format/Format.h:309
+  /// inheritance.
+  bool BreakInhertianceBeforeColonAndComma;
+  

djasper wrote:
> Hm. I am still not sure about this flag and it's name. Fundamentally, this is 
> currently controlling two different things:
> - Whether to wrap before or after colon and comma
> - Whether or not to force wraps for multiple inheritance
> 
> That's going to get us in trouble if at some point people also want other 
> combinations of these two things. I have to alternative suggestions:
> - Add a single flag (e.g. InheritanceListWrapping) with multiple enum values 
> describing the
>   various choices.
> - Don't add a flag at all, but instead break inheritance lists exactly like 
> constructor initializers.
>   I *think* that should solve all the know use cases for now, is easier to 
> configure for users
>   and we can cross the bridge of naming additional flags when we have to.
I see your worries here and indeed the current flags does two things. I'm more 
inclined to use the num that you suggested since in this way it's easier to 
separate the coding style differences that somebody uses. 



Comment at: lib/Format/TokenAnnotator.cpp:2400
+// Returns 'true' if there is an TT_InheritanceComma in the current token list.
+static bool hasMultipleInheritance(const FormatToken &Tok) {
+  for (const FormatToken* nextTok = Tok.Next; nextTok; nextTok = nextTok->Next)

djasper wrote:
> I don't think you need this. If you set MustBreakBefore to true for the 
> InheritanceCommas and set NoLineBreak to true when adding the 
> InheritanceColon on the same line, then clang-format will already do the 
> right thing.
> 
> Fundamentally, this seems to be identical to how we wrap constructor 
> initializer lists in Mozilla style. So I think we should also implement this 
> the same way (if not even reusing the same implementation).
Now that i've seen the behaviour of NoLineBreak thanks for pointing it out to 
me, but still correct me if i'm wrong but shouldn't i use: 
``NoLineBreakInOperand``. 
My guess is if i use NoLineBreak, than breaking on the current line where we 
would have : or , would be prohibited.


Repository:
  rL LLVM

https://reviews.llvm.org/D30487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Andi via Phabricator via cfe-commits]

Abpostelnicu added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:2400
+// Returns 'true' if there is an TT_InheritanceComma in the current token list.
+static bool hasMultipleInheritance(const FormatToken &Tok) {
+  for (const FormatToken* nextTok = Tok.Next; nextTok; nextTok = nextTok->Next)

djasper wrote:
> Abpostelnicu wrote:
> > djasper wrote:
> > > I don't think you need this. If you set MustBreakBefore to true for the 
> > > InheritanceCommas and set NoLineBreak to true when adding the 
> > > InheritanceColon on the same line, then clang-format will already do the 
> > > right thing.
> > > 
> > > Fundamentally, this seems to be identical to how we wrap constructor 
> > > initializer lists in Mozilla style. So I think we should also implement 
> > > this the same way (if not even reusing the same implementation).
> > Now that i've seen the behaviour of NoLineBreak thanks for pointing it out 
> > to me, but still correct me if i'm wrong but shouldn't i use: 
> > ``NoLineBreakInOperand``. 
> > My guess is if i use NoLineBreak, than breaking on the current line where 
> > we would have : or , would be prohibited.
> Yes, that would be prohibited and that is intended. Remember that I'd set 
> this after placing the colon on the same line (and I should have written 
> explicitly) as the class name. After that, there must not be any further line 
> breaks.
> 
> But again, I think we should just have the exact same behavior and 
> implementation as for constructor initializer lists.
Yes we should have the exact behaviour like the constructor initialisation 
list, but that one is also controlled by this flag: 
``BreakConstructorInitializersBeforeComma``, that on our coding style is set to 
true.
But still the actual behaviour of initialiser list still breaks before ``'`` 
for only one item.


Repository:
  rL LLVM

https://reviews.llvm.org/D30487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In https://reviews.llvm.org/D30487#691506, @djasper wrote:

> Before `'`? Can you give an example?


MY mistake, i wanted to write ``:`


Repository:
  rL LLVM

https://reviews.llvm.org/D30487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In https://reviews.llvm.org/D30487#691517, @djasper wrote:

> Do you know whether that is intentional? The style guide isn't really 
> conclusive.


Well i'm not sure, because as you said the document is not clear but i think 
that when we have a single initialiser it should be on the same line as the 
ctor declaration. In this way it would be consistent with the style for 
inheritance list. The actual implementation for inheritance list breaking was 
tailored from the `BreakConstructorInitializersBeforeComma` flag.


Repository:
  rL LLVM

https://reviews.llvm.org/D30487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In https://reviews.llvm.org/D30487#691535, @djasper wrote:

> Hm. Unfortunately, this seems to have been implemented to support Webkit 
> style and Webkit style is explicit about wanting this 
> (https://webkit.org/code-style-guidelines/) :(.
>
> But maybe the solution to that is to add an extra flag like 
> AlwaysWrapInitializerList. Really not sure how best to organize this. Any 
> thoughts? (I personally care about neither of these styles, so maybe I am not 
> the best to judge)
>
> At any rate, to move forward, could you remove the hasMultipleInheritance 
> function and instead use the alternative approach discussed?


Sure will drop hasMultipleInheritance and use the variable that you mentioned 
and i won't touch the rest of the patch. After that i will repost it here.


Repository:
  rL LLVM

https://reviews.llvm.org/D30487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 90475.

Repository:
  rL LLVM

https://reviews.llvm.org/D30487

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1029,6 +1029,17 @@
   verifyFormat("class ::A::B {};");
 }
 
+TEST_F(FormatTest, BreakInhertianceBeforeColonAndComma) {
+  FormatStyle StyleWithInheritanceBreak = getLLVMStyle();
+  LocalStyle.BreakInhertianceBeforeColonAndComma = true;
+
+  verifyFormat("class MyClass : public X {}", LocalStyle);
+  verifyFormat("class MyClass\n"
+   ": public X\n"
+   ", public Y {}",
+   StyleWithInheritanceBreak);
+}
+
 TEST_F(FormatTest, FormatsVariableDeclarationsAfterStructOrClass) {
   verifyFormat("class A {\n} a, b;");
   verifyFormat("struct A {\n} a, b;");
@@ -8491,6 +8502,7 @@
   CHECK_PARSE_BOOL(BreakBeforeTernaryOperators);
   CHECK_PARSE_BOOL(BreakConstructorInitializersBeforeComma);
   CHECK_PARSE_BOOL(BreakStringLiterals);
+  CHECK_PARSE_BOOL(BreakInhertianceBeforeColonAndComma)
   CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -676,6 +676,8 @@
 case tok::comma:
   if (Contexts.back().InCtorInitializer)
 Tok->Type = TT_CtorInitializerComma;
+  else if (Contexts.back().InInhertianceList)
+Tok->Type = TT_InheritanceComma;
   else if (Contexts.back().FirstStartOfName &&
(Contexts.size() == 1 || Line.startsWith(tok::kw_for))) {
 Contexts.back().FirstStartOfName->PartOfMultiVariableDeclStmt = true;
@@ -945,6 +947,7 @@
 bool CanBeExpression = true;
 bool InTemplateArgument = false;
 bool InCtorInitializer = false;
+bool InInhertianceList = false;
 bool CaretFound = false;
 bool IsForEachMacro = false;
   };
@@ -1004,6 +1007,9 @@
Current.Previous->is(TT_CtorInitializerColon)) {
   Contexts.back().IsExpression = true;
   Contexts.back().InCtorInitializer = true;
+} else if (Current.Previous &&
+   Current.Previous->is(TT_InheritanceColon)) {
+  Contexts.back().InInhertianceList = true;
 } else if (Current.isOneOf(tok::r_paren, tok::greater, tok::comma)) {
   for (FormatToken *Previous = Current.Previous;
Previous && Previous->isOneOf(tok::star, tok::amp);
@@ -2473,6 +2479,10 @@
   Style.BreakConstructorInitializersBeforeComma &&
   !Style.ConstructorInitializerAllOnOneLineOrOnePerLine)
 return true;
+  // Break only if we have multiple inheritance.
+  if (Style.BreakInhertianceBeforeColonAndComma &&
+  Right.is(TT_InheritanceComma))
+   return true;
   if (Right.is(tok::string_literal) && Right.TokenText.startswith("R\""))
 // Raw string literals are special wrt. line breaks. The author has made a
 // deliberate choice and might have aligned the contents of the string
@@ -2652,6 +2662,12 @@
   if (Right.is(TT_CtorInitializerComma) &&
   Style.BreakConstructorInitializersBeforeComma)
 return true;
+  if (Left.is(TT_InheritanceComma) &&
+  Style.BreakInhertianceBeforeColonAndComma)
+return false;
+  if (Right.is(TT_InheritanceComma) &&
+  Style.BreakInhertianceBeforeColonAndComma)
+return true;
   if ((Left.is(tok::greater) && Right.is(tok::greater)) ||
   (Left.is(tok::less) && Right.is(tok::less)))
 return false;
Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -48,6 +48,7 @@
   TYPE(FunctionTypeLParen) \
   TYPE(ImplicitStringLiteral) \
   TYPE(InheritanceColon) \
+  TYPE(InheritanceComma) \
   TYPE(InlineASMBrace) \
   TYPE(InlineASMColon) \
   TYPE(JavaAnnotation) \
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -521,6 +521,7 @@
  false, false, false, false, false};
   LLVMStyle.BreakAfterJavaFieldAnnotations = false;
   LLVMStyle.BreakConstructorInitializersBeforeComma = false;
+  LLVMStyle.BreakInhertianceBeforeColonAndComma = false;
   LLVMStyle.BreakStringLiterals = true;
   LLVMStyle.ColumnLimit = 80;
   LLVMStyle.CommentPragmas = "^ IWYU pragma:";
@@ -674,6 +675,7 @@
   MozillaStyle.BinPackArguments = false;
   MozillaStyle.BreakBeforeBraces = FormatStyle::BS_Mozilla;
   MozillaStyle.BreakConstructorInitia

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 90480.
Abpostelnicu added a comment.

corrected some format issues.


Repository:
  rL LLVM

https://reviews.llvm.org/D30487

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1029,6 +1029,17 @@
   verifyFormat("class ::A::B {};");
 }
 
+TEST_F(FormatTest, BreakInhertianceBeforeColonAndComma) {
+  FormatStyle StyleWithInheritanceBreak = getLLVMStyle();
+  LocalStyle.BreakInhertianceBeforeColonAndComma = true;
+
+  verifyFormat("class MyClass : public X {}", LocalStyle);
+  verifyFormat("class MyClass\n"
+   ": public X\n"
+   ", public Y {}",
+   StyleWithInheritanceBreak);
+}
+
 TEST_F(FormatTest, FormatsVariableDeclarationsAfterStructOrClass) {
   verifyFormat("class A {\n} a, b;");
   verifyFormat("struct A {\n} a, b;");
@@ -8491,6 +8502,7 @@
   CHECK_PARSE_BOOL(BreakBeforeTernaryOperators);
   CHECK_PARSE_BOOL(BreakConstructorInitializersBeforeComma);
   CHECK_PARSE_BOOL(BreakStringLiterals);
+  CHECK_PARSE_BOOL(BreakInhertianceBeforeColonAndComma)
   CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -676,6 +676,8 @@
 case tok::comma:
   if (Contexts.back().InCtorInitializer)
 Tok->Type = TT_CtorInitializerComma;
+  else if (Contexts.back().InInhertianceList)
+Tok->Type = TT_InheritanceComma;
   else if (Contexts.back().FirstStartOfName &&
(Contexts.size() == 1 || Line.startsWith(tok::kw_for))) {
 Contexts.back().FirstStartOfName->PartOfMultiVariableDeclStmt = true;
@@ -945,6 +947,7 @@
 bool CanBeExpression = true;
 bool InTemplateArgument = false;
 bool InCtorInitializer = false;
+bool InInhertianceList = false;
 bool CaretFound = false;
 bool IsForEachMacro = false;
   };
@@ -1004,6 +1007,9 @@
Current.Previous->is(TT_CtorInitializerColon)) {
   Contexts.back().IsExpression = true;
   Contexts.back().InCtorInitializer = true;
+} else if (Current.Previous &&
+   Current.Previous->is(TT_InheritanceColon)) {
+  Contexts.back().InInhertianceList = true;
 } else if (Current.isOneOf(tok::r_paren, tok::greater, tok::comma)) {
   for (FormatToken *Previous = Current.Previous;
Previous && Previous->isOneOf(tok::star, tok::amp);
@@ -2473,6 +2479,10 @@
   Style.BreakConstructorInitializersBeforeComma &&
   !Style.ConstructorInitializerAllOnOneLineOrOnePerLine)
 return true;
+  // Break only if we have multiple inheritance.
+  if (Style.BreakInhertianceBeforeColonAndComma &&
+  Right.is(TT_InheritanceComma))
+   return true;
   if (Right.is(tok::string_literal) && Right.TokenText.startswith("R\""))
 // Raw string literals are special wrt. line breaks. The author has made a
 // deliberate choice and might have aligned the contents of the string
@@ -2652,6 +2662,12 @@
   if (Right.is(TT_CtorInitializerComma) &&
   Style.BreakConstructorInitializersBeforeComma)
 return true;
+  if (Left.is(TT_InheritanceComma) &&
+  Style.BreakInhertianceBeforeColonAndComma)
+return false;
+  if (Right.is(TT_InheritanceComma) &&
+  Style.BreakInhertianceBeforeColonAndComma)
+return true;
   if ((Left.is(tok::greater) && Right.is(tok::greater)) ||
   (Left.is(tok::less) && Right.is(tok::less)))
 return false;
Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -48,6 +48,7 @@
   TYPE(FunctionTypeLParen) \
   TYPE(ImplicitStringLiteral) \
   TYPE(InheritanceColon) \
+  TYPE(InheritanceComma) \
   TYPE(InlineASMBrace) \
   TYPE(InlineASMColon) \
   TYPE(JavaAnnotation) \
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -521,6 +521,7 @@
  false, false, false, false, false};
   LLVMStyle.BreakAfterJavaFieldAnnotations = false;
   LLVMStyle.BreakConstructorInitializersBeforeComma = false;
+  LLVMStyle.BreakInhertianceBeforeColonAndComma = false;
   LLVMStyle.BreakStringLiterals = true;
   LLVMStyle.ColumnLimit = 80;
   LLVMStyle.CommentPragmas = "^ IWYU pragma:";
@@ -674,6 +675,7 @@
   MozillaStyle.BinPackArguments = false;
   MozillaStyle.BreakBeforeBraces = F

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

Daniel what do you think about the last version of the patch? I don't want to 
have this stalled since we really need this modifications in order to be able 
to run clang-format on our repository.


Repository:
  rL LLVM

https://reviews.llvm.org/D30487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 91174.
Abpostelnicu added a comment.

Yes that's a better name :)


Repository:
  rL LLVM

https://reviews.llvm.org/D30487

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1029,6 +1029,17 @@
   verifyFormat("class ::A::B {};");
 }
 
+TEST_F(FormatTest, BreakBeforeInhertianceComma) {
+  FormatStyle StyleWithInheritanceBreak = getLLVMStyle();
+  LocalStyle.BreakBeforeInhertianceComma = true;
+
+  verifyFormat("class MyClass : public X {};", LocalStyle);
+  verifyFormat("class MyClass\n"
+   ": public X\n"
+   ", public Y {};",
+   StyleWithInheritanceBreak);
+}
+
 TEST_F(FormatTest, FormatsVariableDeclarationsAfterStructOrClass) {
   verifyFormat("class A {\n} a, b;");
   verifyFormat("struct A {\n} a, b;");
@@ -8582,6 +8593,7 @@
   CHECK_PARSE_BOOL(BreakBeforeTernaryOperators);
   CHECK_PARSE_BOOL(BreakConstructorInitializersBeforeComma);
   CHECK_PARSE_BOOL(BreakStringLiterals);
+  CHECK_PARSE_BOOL(BreakBeforeInhertianceComma)
   CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -676,6 +676,8 @@
 case tok::comma:
   if (Contexts.back().InCtorInitializer)
 Tok->Type = TT_CtorInitializerComma;
+  else if (Contexts.back().InInhertianceList)
+Tok->Type = TT_InheritanceComma;
   else if (Contexts.back().FirstStartOfName &&
(Contexts.size() == 1 || Line.startsWith(tok::kw_for))) {
 Contexts.back().FirstStartOfName->PartOfMultiVariableDeclStmt = true;
@@ -945,6 +947,7 @@
 bool CanBeExpression = true;
 bool InTemplateArgument = false;
 bool InCtorInitializer = false;
+bool InInhertianceList = false;
 bool CaretFound = false;
 bool IsForEachMacro = false;
   };
@@ -1004,6 +1007,9 @@
Current.Previous->is(TT_CtorInitializerColon)) {
   Contexts.back().IsExpression = true;
   Contexts.back().InCtorInitializer = true;
+} else if (Current.Previous &&
+   Current.Previous->is(TT_InheritanceColon)) {
+  Contexts.back().InInhertianceList = true;
 } else if (Current.isOneOf(tok::r_paren, tok::greater, tok::comma)) {
   for (FormatToken *Previous = Current.Previous;
Previous && Previous->isOneOf(tok::star, tok::amp);
@@ -2474,6 +2480,10 @@
   Style.BreakConstructorInitializersBeforeComma &&
   !Style.ConstructorInitializerAllOnOneLineOrOnePerLine)
 return true;
+  // Break only if we have multiple inheritance.
+  if (Style.BreakBeforeInhertianceComma &&
+  Right.is(TT_InheritanceComma))
+   return true;
   if (Right.is(tok::string_literal) && Right.TokenText.startswith("R\""))
 // Raw string literals are special wrt. line breaks. The author has made a
 // deliberate choice and might have aligned the contents of the string
@@ -2653,6 +2663,12 @@
   if (Right.is(TT_CtorInitializerComma) &&
   Style.BreakConstructorInitializersBeforeComma)
 return true;
+  if (Left.is(TT_InheritanceComma) &&
+  Style.BreakBeforeInhertianceComma)
+return false;
+  if (Right.is(TT_InheritanceComma) &&
+  Style.BreakBeforeInhertianceComma)
+return true;
   if ((Left.is(tok::greater) && Right.is(tok::greater)) ||
   (Left.is(tok::less) && Right.is(tok::less)))
 return false;
Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -48,6 +48,7 @@
   TYPE(FunctionTypeLParen) \
   TYPE(ImplicitStringLiteral) \
   TYPE(InheritanceColon) \
+  TYPE(InheritanceComma) \
   TYPE(InlineASMBrace) \
   TYPE(InlineASMColon) \
   TYPE(JavaAnnotation) \
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -521,6 +521,7 @@
  false, false, false, false, false};
   LLVMStyle.BreakAfterJavaFieldAnnotations = false;
   LLVMStyle.BreakConstructorInitializersBeforeComma = false;
+  LLVMStyle.BreakBeforeInhertianceComma = false;
   LLVMStyle.BreakStringLiterals = true;
   LLVMStyle.ColumnLimit = 80;
   LLVMStyle.CommentPragmas = "^ IWYU pragma:";
@@ -674,6 +675,7 @@
   MozillaStyle.BinPackArguments = false;
   MozillaStyle.BreakBeforeBraces = FormatStyle::BS_Mozilla;
   MozillaStyle.BreakConstructor

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 91330.
Abpostelnicu marked an inline comment as done.

Repository:
  rL LLVM

https://reviews.llvm.org/D30487

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1029,6 +1029,17 @@
   verifyFormat("class ::A::B {};");
 }
 
+TEST_F(FormatTest, BreakBeforeInheritanceComma) {
+  FormatStyle StyleWithInheritanceBreak = getLLVMStyle();
+  StyleWithInheritanceBreak.BreakBeforeInheritanceComma = true;
+
+  verifyFormat("class MyClass : public X {};", StyleWithInheritanceBreak);
+  verifyFormat("class MyClass\n"
+   ": public X\n"
+   ", public Y {};",
+   StyleWithInheritanceBreak);
+}
+
 TEST_F(FormatTest, FormatsVariableDeclarationsAfterStructOrClass) {
   verifyFormat("class A {\n} a, b;");
   verifyFormat("struct A {\n} a, b;");
@@ -8582,6 +8593,7 @@
   CHECK_PARSE_BOOL(BreakBeforeTernaryOperators);
   CHECK_PARSE_BOOL(BreakConstructorInitializersBeforeComma);
   CHECK_PARSE_BOOL(BreakStringLiterals);
+  CHECK_PARSE_BOOL(BreakBeforeInheritanceComma)
   CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -676,6 +676,8 @@
 case tok::comma:
   if (Contexts.back().InCtorInitializer)
 Tok->Type = TT_CtorInitializerComma;
+  else if (Contexts.back().InInhertianceList)
+Tok->Type = TT_InheritanceComma;
   else if (Contexts.back().FirstStartOfName &&
(Contexts.size() == 1 || Line.startsWith(tok::kw_for))) {
 Contexts.back().FirstStartOfName->PartOfMultiVariableDeclStmt = true;
@@ -945,6 +947,7 @@
 bool CanBeExpression = true;
 bool InTemplateArgument = false;
 bool InCtorInitializer = false;
+bool InInhertianceList = false;
 bool CaretFound = false;
 bool IsForEachMacro = false;
   };
@@ -1004,6 +1007,9 @@
Current.Previous->is(TT_CtorInitializerColon)) {
   Contexts.back().IsExpression = true;
   Contexts.back().InCtorInitializer = true;
+} else if (Current.Previous &&
+   Current.Previous->is(TT_InheritanceColon)) {
+  Contexts.back().InInhertianceList = true;
 } else if (Current.isOneOf(tok::r_paren, tok::greater, tok::comma)) {
   for (FormatToken *Previous = Current.Previous;
Previous && Previous->isOneOf(tok::star, tok::amp);
@@ -2474,6 +2480,10 @@
   Style.BreakConstructorInitializersBeforeComma &&
   !Style.ConstructorInitializerAllOnOneLineOrOnePerLine)
 return true;
+  // Break only if we have multiple inheritance.
+  if (Style.BreakBeforeInheritanceComma &&
+  Right.is(TT_InheritanceComma))
+   return true;
   if (Right.is(tok::string_literal) && Right.TokenText.startswith("R\""))
 // Raw string literals are special wrt. line breaks. The author has made a
 // deliberate choice and might have aligned the contents of the string
@@ -2653,6 +2663,12 @@
   if (Right.is(TT_CtorInitializerComma) &&
   Style.BreakConstructorInitializersBeforeComma)
 return true;
+  if (Left.is(TT_InheritanceComma) &&
+  Style.BreakBeforeInheritanceComma)
+return false;
+  if (Right.is(TT_InheritanceComma) &&
+  Style.BreakBeforeInheritanceComma)
+return true;
   if ((Left.is(tok::greater) && Right.is(tok::greater)) ||
   (Left.is(tok::less) && Right.is(tok::less)))
 return false;
Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -48,6 +48,7 @@
   TYPE(FunctionTypeLParen) \
   TYPE(ImplicitStringLiteral) \
   TYPE(InheritanceColon) \
+  TYPE(InheritanceComma) \
   TYPE(InlineASMBrace) \
   TYPE(InlineASMColon) \
   TYPE(JavaAnnotation) \
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -298,6 +298,8 @@
 IO.mapOptional("BreakStringLiterals", Style.BreakStringLiterals);
 IO.mapOptional("ColumnLimit", Style.ColumnLimit);
 IO.mapOptional("CommentPragmas", Style.CommentPragmas);
+IO.mapOptional("BreakBeforeInheritanceComma",
+   Style.BreakBeforeInheritanceComma);
 IO.mapOptional("ConstructorInitializerAllOnOneLineOrOnePerLine",
Style.ConstructorInitializerAllOnOneLineOrOnePerLine);
 IO.mapOptional("ConstructorInitializerIndentW

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Andi via Phabricator via cfe-commits]

Abpostelnicu added inline comments.



Comment at: lib/Format/ContinuationIndenter.cpp:355
 
+  if (Current.is(TT_InheritanceColon))
+State.Stack.back().NoLineBreak = true;

djasper wrote:
> Can you leave a comment here:
> 
>   // Don't break within the inheritance declaration unless the ":" is on a 
> new line.
I've modified the comment since this should be valid only if 
Style.BreakBeforeInheritanceComma is set to true otherwise we would be in the 
strange case where the input where the input is: 

```
class A : public B,
  public C {}
```
And this would be translated to something like:

```
class A 
  : public B,
public C {}
```
 


Repository:
  rL LLVM

https://reviews.llvm.org/D30487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 91334.

Repository:
  rL LLVM

https://reviews.llvm.org/D30487

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1029,6 +1029,17 @@
   verifyFormat("class ::A::B {};");
 }
 
+TEST_F(FormatTest, BreakBeforeInheritanceComma) {
+  FormatStyle StyleWithInheritanceBreak = getLLVMStyle();
+  StyleWithInheritanceBreak.BreakBeforeInheritanceComma = true;
+
+  verifyFormat("class MyClass : public X {};", StyleWithInheritanceBreak);
+  verifyFormat("class MyClass\n"
+   ": public X\n"
+   ", public Y {};",
+   StyleWithInheritanceBreak);
+}
+
 TEST_F(FormatTest, FormatsVariableDeclarationsAfterStructOrClass) {
   verifyFormat("class A {\n} a, b;");
   verifyFormat("struct A {\n} a, b;");
@@ -8582,6 +8593,7 @@
   CHECK_PARSE_BOOL(BreakBeforeTernaryOperators);
   CHECK_PARSE_BOOL(BreakConstructorInitializersBeforeComma);
   CHECK_PARSE_BOOL(BreakStringLiterals);
+  CHECK_PARSE_BOOL(BreakBeforeInheritanceComma)
   CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -676,6 +676,8 @@
 case tok::comma:
   if (Contexts.back().InCtorInitializer)
 Tok->Type = TT_CtorInitializerComma;
+  else if (Contexts.back().InInhertianceList)
+Tok->Type = TT_InheritanceComma;
   else if (Contexts.back().FirstStartOfName &&
(Contexts.size() == 1 || Line.startsWith(tok::kw_for))) {
 Contexts.back().FirstStartOfName->PartOfMultiVariableDeclStmt = true;
@@ -945,6 +947,7 @@
 bool CanBeExpression = true;
 bool InTemplateArgument = false;
 bool InCtorInitializer = false;
+bool InInhertianceList = false;
 bool CaretFound = false;
 bool IsForEachMacro = false;
   };
@@ -1004,6 +1007,9 @@
Current.Previous->is(TT_CtorInitializerColon)) {
   Contexts.back().IsExpression = true;
   Contexts.back().InCtorInitializer = true;
+} else if (Current.Previous &&
+   Current.Previous->is(TT_InheritanceColon)) {
+  Contexts.back().InInhertianceList = true;
 } else if (Current.isOneOf(tok::r_paren, tok::greater, tok::comma)) {
   for (FormatToken *Previous = Current.Previous;
Previous && Previous->isOneOf(tok::star, tok::amp);
@@ -2474,6 +2480,10 @@
   Style.BreakConstructorInitializersBeforeComma &&
   !Style.ConstructorInitializerAllOnOneLineOrOnePerLine)
 return true;
+  // Break only if we have multiple inheritance.
+  if (Style.BreakBeforeInheritanceComma &&
+  Right.is(TT_InheritanceComma))
+   return true;
   if (Right.is(tok::string_literal) && Right.TokenText.startswith("R\""))
 // Raw string literals are special wrt. line breaks. The author has made a
 // deliberate choice and might have aligned the contents of the string
@@ -2653,6 +2663,10 @@
   if (Right.is(TT_CtorInitializerComma) &&
   Style.BreakConstructorInitializersBeforeComma)
 return true;
+  if (Left.is(TT_InheritanceComma) && Style.BreakBeforeInheritanceComma)
+return false;
+  if (Right.is(TT_InheritanceComma) && Style.BreakBeforeInheritanceComma)
+return true;
   if ((Left.is(tok::greater) && Right.is(tok::greater)) ||
   (Left.is(tok::less) && Right.is(tok::less)))
 return false;
Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -48,6 +48,7 @@
   TYPE(FunctionTypeLParen) \
   TYPE(ImplicitStringLiteral) \
   TYPE(InheritanceColon) \
+  TYPE(InheritanceComma) \
   TYPE(InlineASMBrace) \
   TYPE(InlineASMColon) \
   TYPE(JavaAnnotation) \
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -298,6 +298,8 @@
 IO.mapOptional("BreakStringLiterals", Style.BreakStringLiterals);
 IO.mapOptional("ColumnLimit", Style.ColumnLimit);
 IO.mapOptional("CommentPragmas", Style.CommentPragmas);
+IO.mapOptional("BreakBeforeInheritanceComma",
+   Style.BreakBeforeInheritanceComma);
 IO.mapOptional("ConstructorInitializerAllOnOneLineOrOnePerLine",
Style.ConstructorInitializerAllOnOneLineOrOnePerLine);
 IO.mapOptional("ConstructorInitializerIndentWidth",
@@ -521,6 +523,7 @@
  fals

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 91335.
Abpostelnicu marked an inline comment as done.
Abpostelnicu added a comment.

Fixed two spell errors.


Repository:
  rL LLVM

https://reviews.llvm.org/D30487

Files:
  docs/ClangFormatStyleOptions.rst
  include/clang/Format/Format.h
  lib/Format/ContinuationIndenter.cpp
  lib/Format/Format.cpp
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1029,6 +1029,17 @@
   verifyFormat("class ::A::B {};");
 }
 
+TEST_F(FormatTest, BreakBeforeInheritanceComma) {
+  FormatStyle StyleWithInheritanceBreak = getLLVMStyle();
+  StyleWithInheritanceBreak.BreakBeforeInheritanceComma = true;
+
+  verifyFormat("class MyClass : public X {};", StyleWithInheritanceBreak);
+  verifyFormat("class MyClass\n"
+   ": public X\n"
+   ", public Y {};",
+   StyleWithInheritanceBreak);
+}
+
 TEST_F(FormatTest, FormatsVariableDeclarationsAfterStructOrClass) {
   verifyFormat("class A {\n} a, b;");
   verifyFormat("struct A {\n} a, b;");
@@ -8582,6 +8593,7 @@
   CHECK_PARSE_BOOL(BreakBeforeTernaryOperators);
   CHECK_PARSE_BOOL(BreakConstructorInitializersBeforeComma);
   CHECK_PARSE_BOOL(BreakStringLiterals);
+  CHECK_PARSE_BOOL(BreakBeforeInheritanceComma)
   CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -676,6 +676,8 @@
 case tok::comma:
   if (Contexts.back().InCtorInitializer)
 Tok->Type = TT_CtorInitializerComma;
+  else if (Contexts.back().InInheritanceList)
+Tok->Type = TT_InheritanceComma;
   else if (Contexts.back().FirstStartOfName &&
(Contexts.size() == 1 || Line.startsWith(tok::kw_for))) {
 Contexts.back().FirstStartOfName->PartOfMultiVariableDeclStmt = true;
@@ -945,6 +947,7 @@
 bool CanBeExpression = true;
 bool InTemplateArgument = false;
 bool InCtorInitializer = false;
+bool InInheritanceList = false;
 bool CaretFound = false;
 bool IsForEachMacro = false;
   };
@@ -1004,6 +1007,9 @@
Current.Previous->is(TT_CtorInitializerColon)) {
   Contexts.back().IsExpression = true;
   Contexts.back().InCtorInitializer = true;
+} else if (Current.Previous &&
+   Current.Previous->is(TT_InheritanceColon)) {
+  Contexts.back().InInheritanceList = true;
 } else if (Current.isOneOf(tok::r_paren, tok::greater, tok::comma)) {
   for (FormatToken *Previous = Current.Previous;
Previous && Previous->isOneOf(tok::star, tok::amp);
@@ -2474,6 +2480,10 @@
   Style.BreakConstructorInitializersBeforeComma &&
   !Style.ConstructorInitializerAllOnOneLineOrOnePerLine)
 return true;
+  // Break only if we have multiple inheritance.
+  if (Style.BreakBeforeInheritanceComma &&
+  Right.is(TT_InheritanceComma))
+   return true;
   if (Right.is(tok::string_literal) && Right.TokenText.startswith("R\""))
 // Raw string literals are special wrt. line breaks. The author has made a
 // deliberate choice and might have aligned the contents of the string
@@ -2653,6 +2663,10 @@
   if (Right.is(TT_CtorInitializerComma) &&
   Style.BreakConstructorInitializersBeforeComma)
 return true;
+  if (Left.is(TT_InheritanceComma) && Style.BreakBeforeInheritanceComma)
+return false;
+  if (Right.is(TT_InheritanceComma) && Style.BreakBeforeInheritanceComma)
+return true;
   if ((Left.is(tok::greater) && Right.is(tok::greater)) ||
   (Left.is(tok::less) && Right.is(tok::less)))
 return false;
Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -48,6 +48,7 @@
   TYPE(FunctionTypeLParen) \
   TYPE(ImplicitStringLiteral) \
   TYPE(InheritanceColon) \
+  TYPE(InheritanceComma) \
   TYPE(InlineASMBrace) \
   TYPE(InlineASMColon) \
   TYPE(JavaAnnotation) \
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -298,6 +298,8 @@
 IO.mapOptional("BreakStringLiterals", Style.BreakStringLiterals);
 IO.mapOptional("ColumnLimit", Style.ColumnLimit);
 IO.mapOptional("CommentPragmas", Style.CommentPragmas);
+IO.mapOptional("BreakBeforeInheritanceComma",
+   Style.BreakBeforeInheritanceComma);
 IO.mapOptional("ConstructorInitializerAllOnOneLineOrOnePerLine",
Style.ConstructorInitializerAllOnOneLineOrOnePerLine);
 IO.

[PATCH] D30487: ClangFormat - Add option to break before inheritance separation operator in class declaration

[Andi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL297467: [clang-format] Add option to break before 
inheritance separation operator in… (authored by Abpostelnicu).

Changed prior to commit:
  https://reviews.llvm.org/D30487?vs=91335&id=91344#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D30487

Files:
  cfe/trunk/docs/ClangFormatStyleOptions.rst
  cfe/trunk/include/clang/Format/Format.h
  cfe/trunk/lib/Format/ContinuationIndenter.cpp
  cfe/trunk/lib/Format/Format.cpp
  cfe/trunk/lib/Format/FormatToken.h
  cfe/trunk/lib/Format/TokenAnnotator.cpp
  cfe/trunk/unittests/Format/FormatTest.cpp

Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -1029,6 +1029,17 @@
   verifyFormat("class ::A::B {};");
 }
 
+TEST_F(FormatTest, BreakBeforeInheritanceComma) {
+  FormatStyle StyleWithInheritanceBreak = getLLVMStyle();
+  StyleWithInheritanceBreak.BreakBeforeInheritanceComma = true;
+
+  verifyFormat("class MyClass : public X {};", StyleWithInheritanceBreak);
+  verifyFormat("class MyClass\n"
+   ": public X\n"
+   ", public Y {};",
+   StyleWithInheritanceBreak);
+}
+
 TEST_F(FormatTest, FormatsVariableDeclarationsAfterStructOrClass) {
   verifyFormat("class A {\n} a, b;");
   verifyFormat("struct A {\n} a, b;");
@@ -8582,6 +8593,7 @@
   CHECK_PARSE_BOOL(BreakBeforeTernaryOperators);
   CHECK_PARSE_BOOL(BreakConstructorInitializersBeforeComma);
   CHECK_PARSE_BOOL(BreakStringLiterals);
+  CHECK_PARSE_BOOL(BreakBeforeInheritanceComma)
   CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
Index: cfe/trunk/docs/ClangFormatStyleOptions.rst
===
--- cfe/trunk/docs/ClangFormatStyleOptions.rst
+++ cfe/trunk/docs/ClangFormatStyleOptions.rst
@@ -528,6 +528,10 @@
 
 
 
+**BreakBeforeInheritanceComma** (``bool``)
+  If ``true``, in the class inheritance expression clang-format will
+  break before ``:`` and ``,`` if there is multiple inheritance.
+
 **BreakBeforeTernaryOperators** (``bool``)
   If ``true``, ternary operators will be placed after line breaks.
 
Index: cfe/trunk/include/clang/Format/Format.h
===
--- cfe/trunk/include/clang/Format/Format.h
+++ cfe/trunk/include/clang/Format/Format.h
@@ -422,6 +422,10 @@
   /// which should not be split into lines or otherwise changed.
   std::string CommentPragmas;
 
+  /// \brief If ``true``, in the class inheritance expression clang-format will
+  /// break before ``:`` and ``,`` if there is multiple inheritance.
+  bool BreakBeforeInheritanceComma;
+
   /// \brief If the constructor initializers don't fit on a line, put each
   /// initializer on its own line.
   bool ConstructorInitializerAllOnOneLineOrOnePerLine;
@@ -844,6 +848,7 @@
BreakAfterJavaFieldAnnotations == R.BreakAfterJavaFieldAnnotations &&
BreakStringLiterals == R.BreakStringLiterals &&
ColumnLimit == R.ColumnLimit && CommentPragmas == R.CommentPragmas &&
+   BreakBeforeInheritanceComma == R.BreakBeforeInheritanceComma &&
ConstructorInitializerAllOnOneLineOrOnePerLine ==
R.ConstructorInitializerAllOnOneLineOrOnePerLine &&
ConstructorInitializerIndentWidth ==
Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -676,6 +676,8 @@
 case tok::comma:
   if (Contexts.back().InCtorInitializer)
 Tok->Type = TT_CtorInitializerComma;
+  else if (Contexts.back().InInheritanceList)
+Tok->Type = TT_InheritanceComma;
   else if (Contexts.back().FirstStartOfName &&
(Contexts.size() == 1 || Line.startsWith(tok::kw_for))) {
 Contexts.back().FirstStartOfName->PartOfMultiVariableDeclStmt = true;
@@ -945,6 +947,7 @@
 bool CanBeExpression = true;
 bool InTemplateArgument = false;
 bool InCtorInitializer = false;
+bool InInheritanceList = false;
 bool CaretFound = false;
 bool IsForEachMacro = false;
   };
@@ -1004,6 +1007,9 @@
Current.Previous->is(TT_CtorInitializerColon)) {
   Contexts.back().IsExpression = true;
   Contexts.back().InCtorInitializer = true;
+} else if (Current.Previous &&
+   Current.Previous->is(TT_InheritanceColon)) {
+  Contexts.back().InInheritanceList = true;
 } else if (Current.isOneOf(tok::r_paren, tok::greater, tok::comma)) {
   for (FormatToken *Previous = Current.Previous;
Previous && Previous->isOneOf(tok::star, tok::amp);
@@

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

I think this added a regression, where you have this context:

  uint32_t LocalAccessible::SelectedItemCount() {
uint32_t count = 0;
AccIterator iter(this, filters::GetSelected);
LocalAccessible* selected = nullptr;
while ((selected = iter.Next())) ++count;
  
return count;
  }



  [task 2021-04-27T02:39:42.523Z] 02:39:42ERROR -  
/builds/worker/checkouts/gecko/accessible/generic/LocalAccessible.cpp:2455:20: 
error: variable 'selected' set but not used [-Werror,-Wunused-but-set-variable]
  [task 2021-04-27T02:39:42.523Z] 02:39:42 INFO -LocalAccessible* 
selected = nullptr;
  [task 2021-04-27T02:39:42.523Z] 02:39:42 INFO - ^
  [task 2021-04-27T02:39:42.523Z] 02:39:42 INFO -  1 error generated.

The file in cause is this 

 one.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

Please revert this. This change hasn’t been tested nor reviewed enough. Also I 
don’t remember seeing this proposed on cfe dev mailing list.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In D100581#2723611 , @lebedev.ri 
wrote:

> In D100581#2723505 , @Abpostelnicu 
> wrote:
>
>> Also I don’t remember seeing this proposed on cfe dev mailing list.
>
> There is no such requirement. I don't recall that happening basically ever, 
> actually.

Of course, but I would have wanted to see this discussed on the mailing list 
since the pool of the affected actors by this is very big.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101480: Revert "[Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable"

[Andi via Phabricator via cfe-commits]

Abpostelnicu accepted this revision.
Abpostelnicu added a comment.
This revision is now accepted and ready to land.

Thank you for this! Also I think it’s very well that you want to implement this 
warning in clang, with a little bit of more polish it will be up for merger!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101480/new/

https://reviews.llvm.org/D101480

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79714: [Diagnostics] Restore -Wdeprecated warning when user-declared copy assignment operator is defined as deleted (PR45634)

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

I'm seeing here something very strange, if the user provided copy assignment 
operator is provided but is = delete and the copy constructor is default this 
warning will still trigger. Is this something expected?
I'm referring at this 

 issue from the mozilla 
 
code-base.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79714/new/

https://reviews.llvm.org/D79714

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100625: [clang-tools-extra] Do not use `LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR`

[Andi via Phabricator via cfe-commits]

Abpostelnicu abandoned this revision.
Abpostelnicu added a comment.

In D100625#2741385 , @hvdijk wrote:

> Apparently Phabricator detected the presence of "revert D100625 
> " in my alternative D101851 
> 's description and marked that as reverting 
> this one. Of course, as this hadn't been committed, it cannot have been 
> reverted either. Could you confirm that things work for you now, 
> @Abpostelnicu?
>
> In D100625#2697345 , @jpalus wrote:
>
>> Do you mind taking a look at fix proposed in:
>>
>> https://bugs.llvm.org/show_bug.cgi?id=49990#c3
>
> Sorry for not noticing this before, this is pretty much what I came up with 
> too and what's committed now.

It works just fine, thank you.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100625/new/

https://reviews.llvm.org/D100625

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D93543: clang-tidy: Leave the possibility of opting out having coloured diagnostic messages.

[Andi via Phabricator via cfe-commits]

Abpostelnicu updated this revision to Diff 336797.
Abpostelnicu added a comment.

Add `-use-color` argument to make it sane with `clang-tidy --use-color` 
argument.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93543/new/

https://reviews.llvm.org/D93543

Files:
  clang-tools-extra/clang-tidy/tool/run-clang-tidy.py


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -80,10 +80,14 @@
 
 
 def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path,
-header_filter, allow_enabling_alpha_checkers,
-extra_arg, extra_arg_before, quiet, config):
+header_filter, use_color,
+allow_enabling_alpha_checkers, extra_arg,
+extra_arg_before, quiet, config):
   """Gets a command line for clang-tidy."""
-  start = [clang_tidy_binary, '--use-color']
+  start = [clang_tidy_binary]
+  # Only if we are connected to a tty we can check for enabling color mode
+  if os.isatty(sys.stdout.fileno()) and use_color:
+start.append('--use-color')
   if allow_enabling_alpha_checkers:
 start.append('-allow-enabling-analyzer-alpha-checkers')
   if header_filter is not None:
@@ -163,6 +167,7 @@
 name = queue.get()
 invocation = get_tidy_invocation(name, args.clang_tidy_binary, args.checks,
  tmpdir, build_path, args.header_filter,
+ args.use_color,
  args.allow_enabling_alpha_checkers,
  args.extra_arg, args.extra_arg_before,
  args.quiet, args.config)
@@ -204,6 +209,8 @@
   'When the value is empty, clang-tidy will '
   'attempt to find a file named .clang-tidy for '
   'each source file in its parent directories.')
+  parser.add_argument('-use-color', action='store_true',
+  help='Enables diagnostic message colors.')
   parser.add_argument('-header-filter', default=None,
   help='regular expression matching the names of the '
   'headers to output diagnostics from. Diagnostics from '


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -80,10 +80,14 @@
 
 
 def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path,
-header_filter, allow_enabling_alpha_checkers,
-extra_arg, extra_arg_before, quiet, config):
+header_filter, use_color,
+allow_enabling_alpha_checkers, extra_arg,
+extra_arg_before, quiet, config):
   """Gets a command line for clang-tidy."""
-  start = [clang_tidy_binary, '--use-color']
+  start = [clang_tidy_binary]
+  # Only if we are connected to a tty we can check for enabling color mode
+  if os.isatty(sys.stdout.fileno()) and use_color:
+start.append('--use-color')
   if allow_enabling_alpha_checkers:
 start.append('-allow-enabling-analyzer-alpha-checkers')
   if header_filter is not None:
@@ -163,6 +167,7 @@
 name = queue.get()
 invocation = get_tidy_invocation(name, args.clang_tidy_binary, args.checks,
  tmpdir, build_path, args.header_filter,
+ args.use_color,
  args.allow_enabling_alpha_checkers,
  args.extra_arg, args.extra_arg_before,
  args.quiet, args.config)
@@ -204,6 +209,8 @@
   'When the value is empty, clang-tidy will '
   'attempt to find a file named .clang-tidy for '
   'each source file in its parent directories.')
+  parser.add_argument('-use-color', action='store_true',
+  help='Enables diagnostic message colors.')
   parser.add_argument('-header-filter', default=None,
   help='regular expression matching the names of the '
   'headers to output diagnostics from. Diagnostics from '
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits