[PATCH] D72730: [clang-tidy] run-clang-tidy -export-fixes exports only fixes, not all warnings

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In D72730#1866043 , @AlexanderLanin 
wrote:

> On second thought maybe this should be fixed in clang-tidy and not here?
>  Maybe besides `-export-fixes` there should be an `-export-warnings` or just 
> `-yaml-export`?


I like the idea of introducing `-export-warnings` (to export warnings 
completely - with messages, notes, ranges and fixes) and changing 
`-export-fixes` to only export fixes.


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

https://reviews.llvm.org/D72730



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

[PATCH] D72730: [clang-tidy] run-clang-tidy -export-fixes exports only fixes, not all warnings

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

On second thought maybe this should be fixed in clang-tidy and not here?
Maybe besides `-export-fixes` there should be an `-export-warnings` or just 
`-yaml-export`?


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

https://reviews.llvm.org/D72730



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

[PATCH] D72730: [clang-tidy] run-clang-tidy -export-fixes exports only fixes, not all warnings

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin updated this revision to Diff 241895.
AlexanderLanin added a comment.

Review findings applied (no real measurable difference in speed, maybe 100ms) 
but it's indeed more readable.
Without this fix the export took 12.96 seconds, with this patch 11.07 seconds.
Difference is even bigger when there are less auto fixable findings (70% with 
FIX-IT in my example).

As there is no test suite available I run this without and with the change and 
manually compared the output.
All warnings without FIX-IT have disappeared from exported, all warnings with 
FIX-IT are unmodified.


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

https://reviews.llvm.org/D72730

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
@@ -105,6 +105,16 @@
   start.append(f)
   return start
 
+# returns whether passed yaml Diagnostic (see mergekey) element contains FIX-IT
+# FIX-IT replacements can be within the warning directly or within the notes.
+def isAutoFixable(diag):
+  if diag["DiagnosticMessage"]["Replacements"]: return True
+
+  for note in diag.get("Notes", []):
+if note.get("Replacements", []):
+  return True
+
+  return False
 
 def merge_replacement_files(tmpdir, mergefile):
   """Merge all replacement files in a directory into a single file"""
@@ -116,7 +126,9 @@
 content = yaml.safe_load(open(replacefile, 'r'))
 if not content:
   continue # Skip empty files.
-merged.extend(content.get(mergekey, []))
+for d in content.get(mergekey, []):
+  if isAutoFixable(d):
+merged.append(d)
 
   if merged:
 # MainSourceFile: The key is required by the definition inside


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
@@ -105,6 +105,16 @@
   start.append(f)
   return start
 
+# returns whether passed yaml Diagnostic (see mergekey) element contains FIX-IT
+# FIX-IT replacements can be within the warning directly or within the notes.
+def isAutoFixable(diag):
+  if diag["DiagnosticMessage"]["Replacements"]: return True
+
+  for note in diag.get("Notes", []):
+if note.get("Replacements", []):
+  return True
+
+  return False
 
 def merge_replacement_files(tmpdir, mergefile):
   """Merge all replacement files in a directory into a single file"""
@@ -116,7 +126,9 @@
 content = yaml.safe_load(open(replacefile, 'r'))
 if not content:
   continue # Skip empty files.
-merged.extend(content.get(mergekey, []))
+for d in content.get(mergekey, []):
+  if isAutoFixable(d):
+merged.append(d)
 
   if merged:
 # MainSourceFile: The key is required by the definition inside
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72730: [clang-tidy] run-clang-tidy -export-fixes exports only fixes, not all warnings

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

A small Python suggestion.




Comment at: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py:112
+
+  if "Notes" in diag:
+for note in diag["Notes"]:

This double lookup is unnecessary, you can do `for note in diag.get("Notes", 
[]):` to default to empty list if "Notes" is not in diag. 



Comment at: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py:114
+for note in diag["Notes"]:
+  if "Replacements" in note and note["Replacements"]:
+return True

Same here (not sure if Replacements holds a list, but I think it might) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72730



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