[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

I cannot reproduce the problem and there is just not enough info to go on.
Is there any way to get anything in addition? e.g. run the same test with `-vv` 
or add some additional print commands?
Maybe it's some access rights problem, or different version of some non obvious 
dependency or something like that.
Maybe it's as trivial as having to use double quotes for 
`-checks='-*,bugprone-sizeof-container,modernize-use-auto'`.
It seems run-clang-tidy exits with code 2, but there is no way it would do 
that. So I'm totally lost.
http://lab.llvm.org:8011/#/builders/109/builds/690/steps/6/logs/FAIL__Clang_Tools__run-clang-tidy_export-diagnosti


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

https://reviews.llvm.org/D74299

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

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin marked an inline comment as done.
AlexanderLanin added a comment.

Could someone commit this as I cannot? Thanks a lot!
Alexander Lanin 

Thanks for review @aaron.ballman!


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

https://reviews.llvm.org/D74299

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

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

[Alexander Lanin via Phabricator via cfe-commits]

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

Readd removed 'print help without crashing' test


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

https://reviews.llvm.org/D74299

Files:
  clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_config-file.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_export-diagnostics.cpp


Index: 
clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_export-diagnostics.cpp
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_export-diagnostics.cpp
@@ -0,0 +1,33 @@
+// under test:
+// - parsing and using compile_commands
+// - export fixes to yaml file
+
+// use %t as directory instead of file,
+// because "compile_commands.json" must have exactly that name:
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: echo '[{"directory":"%/S","command":"clang++ -c %s","file":"%s"}]' \
+// RUN:  > %t/compile_commands.json
+
+// execute and check:
+// RUN: cd "%t"
+// RUN: %run_clang_tidy 
-checks='-*,bugprone-sizeof-container,modernize-use-auto' \
+// RUN: -p="%/t" -export-fixes=%t/fixes.yaml > %t/msg.txt 2>&1
+// RUN: FileCheck -input-file=%t/msg.txt -check-prefix=CHECK-MESSAGES %s \
+// RUN:   -implicit-check-not='{{warning|error|note}}:'
+// RUN: FileCheck -input-file=%t/fixes.yaml -check-prefix=CHECK-YAML %s
+
+#include 
+int main()
+{
+  std::vector vec;
+  std::vector::iterator iter = vec.begin();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when declaring iterators
+  // CHECK-YAML: modernize-use-auto
+  
+  return sizeof(vec);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: sizeof() doesn't return the 
size of the container; did you mean .size()? [bugprone-sizeof-container]
+  // CHECK-YAML: bugprone-sizeof-container
+  // After https://reviews.llvm.org/D72730 --> CHECK-YAML-NOT: 
bugprone-sizeof-container
+}
+
Index: 
clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_config-file.cpp
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_config-file.cpp
@@ -0,0 +1,33 @@
+// under test:
+// - .clang-tidy read from file & treat warnings as errors
+// - return code of run-clang-tidy on those errors
+
+// First make sure clang-tidy is executable and can print help without 
crashing:
+// RUN: %run_clang_tidy --help
+
+// use %t as directory instead of file:
+// RUN: rm -rf %t
+// RUN: mkdir %t
+
+// add this file to %t, add compile_commands for it and .clang-tidy config:
+// RUN: echo "[{\"directory\":\".\",\"command\":\"clang++ -c 
%/t/test.cpp\",\"file\":\"%/t/test.cpp\"}]" | sed -e 's/\\//g' > 
%t/compile_commands.json
+// RUN: echo "Checks: '-*,modernize-use-auto'" > %t/.clang-tidy
+// RUN: echo "WarningsAsErrors: '*'" >> %t/.clang-tidy
+// RUN: echo "CheckOptions:" >> %t/.clang-tidy
+// RUN: echo "  - key: modernize-use-auto.MinTypeNameLength" >> 
%t/.clang-tidy
+// RUN: echo "value:   '0'" >> %t/.clang-tidy
+// RUN: cp "%s" "%t/test.cpp"
+
+// execute and check:
+// RUN: cd "%t"
+// RUN: not %run_clang_tidy "test.cpp" > %t/msg.txt 2>&1
+// RUN: FileCheck -input-file=%t/msg.txt -check-prefix=CHECK-MESSAGES %s \
+// RUN:   -implicit-check-not='{{warning|error|note}}:'
+
+int main()
+{
+  int* x = new int();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: error: {{.+}} 
[modernize-use-auto,-warnings-as-errors]
+
+  delete x;
+}
Index: clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy.cpp
+++ /dev/null
@@ -1,18 +0,0 @@
-// RUN: %run_clang_tidy --help
-// RUN: rm -rf %t
-// RUN: mkdir %t
-// RUN: echo "[{\"directory\":\".\",\"command\":\"clang++ -c 
%/t/test.cpp\",\"file\":\"%/t/test.cpp\"}]" | sed -e 's/\\//g' > 
%t/compile_commands.json
-// RUN: echo "Checks: '-*,modernize-use-auto'" > %t/.clang-tidy
-// RUN: echo "WarningsAsErrors: '*'" >> %t/.clang-tidy
-// RUN: echo "CheckOptions:" >> %t/.clang-tidy
-// RUN: echo "  - key: modernize-use-auto.MinTypeNameLength" >> 
%t/.clang-tidy
-// RUN: echo "value:   '0'" >> %t/.clang-tidy
-// RUN: cp "%s" "%t/test.cpp"
-// RUN: cd "%t"
-// RUN: not %run_clang_tidy "test.cpp"
-
-int main()
-{
-  int* x = new int();
-  delete x;
-}


Index: clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_export-diagnostics.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy_export-diagnostics.cpp
@@ -0,0 +1,33 @@
+// under test:
+// - parsing and using compile_commands
+// - export fixes to yaml file
+
+// use %t as directory 

[PATCH] D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

Off-Topic: I was just attempting to apply this to my codebase at work.
Seems this will be more challenging than anticipated  
My best guess is this is related to D72730 

  Applying fixes ...
  terminate called after throwing an instance of 'std::bad_alloc'
what():  std::bad_alloc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

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

[PATCH] D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

"Minimal" example for the behavior I described before.

`clang++ -fsyntax-only  ~/llvm/clang/test/SemaCXX/warn-range-loop-analysis.cpp 
-Wrange-loop-analysis 2>&1 | wc -l`
269

Then fix it by:
`~/llvm/build-const-fix/bin/clang-tidy 
~/llvm/clang/test/SemaCXX/warn-range-loop-analysis.cpp 
-checks="-*,cppcoreguidelines-const-correctness" -fix`

And recount:
`clang++ -fsyntax-only  ~/llvm/clang/test/SemaCXX/warn-range-loop-analysis.cpp 
-Wrange-loop-analysis 2>&1 | wc -l`
283

Yet again I want to mentioned that this is technically not a bug, but in 
combination with Werror it's not ideal. Basically I cannot compile my codebase 
after this fixit since Werror is set. Although the code is arguably better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

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

[PATCH] D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

> Could you please provide me a full reproducer (optimally without dependencies 
> on includes/libraries)?

I can certainly do that based on my old version from ~9 months ago or 
preferably if you provide me some branch somewhere (github?). I have trouble 
applying the latest diff to latest master myself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

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

[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

Observed behavior:

Change: `for(string_view token : split_into_views(file_content, " \t\r\n"))` 
--> `for(string_view const token : split_into_views(file_content, " \t\r\n"))`.
Note: `std::vector split_into_views(string_view input, const char* 
separators);`
Problem: Now it triggers `error: loop variable 'token' of type 'const 
nonstd::sv_lite::string_view' (aka 'const basic_string_view') creates a 
copy from type 'const nonstd::sv_lite::string_view' 
[-Werror,-Wrange-loop-analysis]`
Fixed manually by making it `for(std::string_view& const token : 
split_into_views(file_content, " \t\r\n"))`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

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

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin planned changes to this revision.
AlexanderLanin added a comment.

The reason was that I thought that line doesn't do anything.
Of course "not crashing" is a valid expectation. If nothing else, then "not 
crashing" should indeed be tested.
I'll try to improve this ancient diff.


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

https://reviews.llvm.org/D74299

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

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

*ping*


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

https://reviews.llvm.org/D74299



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

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

*ping*


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

https://reviews.llvm.org/D74299



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

[PATCH] D74299: [clang-tidy] extend tests of run-clang-tidy

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

*ping*


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

https://reviews.llvm.org/D74299



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

[PATCH] D76037: [clang] tooling replacements are escaped when exporting to YAML

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

Fine for me. Fixes newline bug in https://bugs.llvm.org/show_bug.cgi?id=45150.
However I don't have "review privileges" here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76037



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

[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

Unfortunately I cannot say whether the code is good, but the fix works and 
certainly helps readability-braces-around-statements which can sometimes add 
multiple } at the same offset to close several scopes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76054



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

[PATCH] D76037: [clang] tooling replacements are escaped when exporting to YAML

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin requested changes to this revision.
AlexanderLanin added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/Tooling/ReplacementsYaml.cpp:22
+static constexpr Escapes EscapeChars[] = {
+{'\n', 'n'}, {'\r', 'r'}, {'\t', 't'}, {'\\', '\\'}};
+

Just so I have asked ;-)
Escaping every \ would be incorrect? Basically duplicate every '\'.



Comment at: clang/unittests/Tooling/ReplacementsYamlTest.cpp:68
+  ASSERT_EQ(Doc.Replacements.size(), NewDoc.Replacements.size());
+  if (Doc.Replacements.size() == NewDoc.Replacements.size()) {
+for (auto DocR = Doc.Replacements.begin(),

njames93 wrote:
> Can this ever be false. Does execution of a test case stop once an ASSERT_EQ 
> fails
Yes, assert stops the test case. After the assert you can safely assume they 
are identical.



Comment at: clang/unittests/Tooling/ReplacementsYamlTest.cpp:55
+  Doc.Replacements.emplace_back(FilePath, 0, 0, "#include \n");
+  Doc.Replacements.emplace_back(FilePath, 0, 0, "'\\\t\r\n");
 

I think it would be worthwhile to test other characters as well.
50% of that would be purely for documentation purposes. What would happen when 
you escape \x and unescape \\x?



Comment at: clang/unittests/Tooling/ReplacementsYamlTest.cpp:76
+  ASSERT_EQ(DocR->getOffset(), NewDocR->getOffset());
+  ASSERT_EQ(DocR->getReplacementText(), NewDocR->getReplacementText());
+}

I assume this kind of test would have been green even without your change? Or 
would it fail?
You are testing that it is reconstructed correctly (which is indeed the main 
point), but not the escaping and unescaping.
You should probably test a concrete example with(escaped text, expected escaped 
test).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76037



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

[PATCH] D16267: Handle C++11 brace initializers in readability-braces-around-statements

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

Here is the r_brace use case:

  if (true) while ("ok") {}

Unfortunately it looks quite similar to:

  if (true) s = {};


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

https://reviews.llvm.org/D16267



___
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] D73856: [docu] Improve docu of misc-misplaced-const

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin abandoned this revision.
AlexanderLanin added a comment.

@aaron.ballman I was ready to complain, but you are right. It would make 
absolutely no sense to move the const, it's just where it should be.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73856



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

[PATCH] D73856: [docu] Improve docu of misc-misplaced-const

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin created this revision.
AlexanderLanin added a project: clang-tools-extra.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

East const makes the problem more obvious. With west const people were telling 
me the const is on the wrong side.

Also it's time to use using instead of typedef.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73856

Files:
  clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst


Index: clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst
+++ clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst
@@ -13,10 +13,9 @@
 
 .. code-block:: c++
 
-  typedef int *int_ptr;
-  void f(const int_ptr ptr) {
+  using int_ptr = int*;
+  void f(int_ptr const ptr) {
 *ptr = 0; // potentially quite unexpectedly the int can be modified here
-ptr = 0; // does not compile
   }
 
 The check does not diagnose when the underlying ``typedef``/``using`` type is a


Index: clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst
+++ clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst
@@ -13,10 +13,9 @@
 
 .. code-block:: c++
 
-  typedef int *int_ptr;
-  void f(const int_ptr ptr) {
+  using int_ptr = int*;
+  void f(int_ptr const ptr) {
 *ptr = 0; // potentially quite unexpectedly the int can be modified here
-ptr = 0; // does not compile
   }
 
 The check does not diagnose when the underlying ``typedef``/``using`` type is a
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29692: [clang-tidy] add check modernize-use-const-instead-of-define

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin abandoned this revision.
AlexanderLanin added a comment.

This is now detected by cppcoreguidelines-macro-usage, so this seems out of 
date.


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

https://reviews.llvm.org/D29692



___
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] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

In D54943#1815772 , @0x8000- wrote:

> This generated 56 "const const" declarations, of which 25 were triple const!


I've tried this on ccache (326 fixes, nice!), but some came out as 'const 
const'.
At least in my case I traced it back to having `src/..` and `./src/..` 
within the fixes.yml file.
On first glance these result from having `./src` as an include directory.

After replacing `-I./` with `-I` within compile_commands.json I'm not getting 
`const const` anymore.

Not sure who is at fault here / who should fix it.
But at least in the trivial case of ./ I feel clang-tidy should fix it when it 
writes the yml files?!

This surfaces here simply due to the shire number of files with fixes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943



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

[PATCH] D72373: [clang-tidy] extend misc-misplaced-const to detect using besides typedef

[Alexander Lanin via Phabricator via cfe-commits]

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

Updated misc-misplaced-const.c to reflect new output and fixed one wrong col in 
misc-misplaced-const.cpp - I really do not understand how that happened.
Rebased and verified with check-clang-tools.


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

https://reviews.llvm.org/D72373

Files:
  clang-tools-extra/clang-tidy/misc/MisplacedConstCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst
  clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.c
  clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.cpp
@@ -1,20 +1,40 @@
-// RUN: %check_clang_tidy %s misc-misplaced-const %t
-
-typedef int plain_i;
-typedef int *ip;
-typedef const int *cip;
-
-void func() {
-  if (const int *i = 0)
-;
-  if (const plain_i *i = 0)
-;
-  if (const cip i = 0)
-;
-
-  // CHECK-MESSAGES: :[[@LINE+1]]:16: warning: 'i' declared with a const-qualified typedef type; results in the type being 'int *const' instead of 'const int *'
-  if (const ip i = 0)
-;
+// RUN: %check_clang_tidy %s misc-misplaced-const %t -- -- -DUSING
+// RUN: %check_clang_tidy %s misc-misplaced-const %t -- -- -DTYPEDEF
+
+#ifdef TYPEDEF
+typedef int int_;
+typedef int *ptr_to_int;
+typedef const int *ptr_to_const_int;
+#endif
+#ifdef USING
+using int_ = int;
+using ptr_to_int = int *;
+using ptr_to_const_int = const int *;
+#endif
+
+void const_pointers() {
+  if (const int *i = 0) {
+i = 0;
+// *i = 0;
+  }
+
+  if (const int_ *i = 0) {
+i = 0;
+// *i = 0;
+  }
+
+  if (const ptr_to_const_int i = 0) {
+// i = 0;
+// *i = 0;
+  }
+
+  // Potentially quite unexpectedly the int can be modified here
+  // CHECK-MESSAGES: :[[@LINE+1]]:24: warning: 'i' declared with a const-qualified {{.*}}; results in the type being 'int *const' instead of 'const int *'
+  if (const ptr_to_int i = 0) {
+//i = 0;
+
+*i = 0;
+  }
 }
 
 template 
@@ -24,8 +44,8 @@
 };
 
 template struct S;
-template struct S; // ok
-template struct S;
+template struct S; // ok
+template struct S;
 
 template 
 struct U {
Index: clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.c
===
--- clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.c
+++ clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.c
@@ -14,13 +14,13 @@
 
   // Not ok
   const ip i3 = 0;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'i3' declared with a const-qualified typedef type; results in the type being 'int *const' instead of 'const int *'
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'i3' declared with a const-qualified typedef; results in the type being 'int *const' instead of 'const int *'
 
   ip const i4 = 0;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'i4' declared with a const-qualified
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'i4' declared with a const-qualified typedef; results in the type being 'int *const' instead of 'const int *'
 
   const volatile ip i5 = 0;
-  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'i5' declared with a const-qualified typedef type; results in the type being 'int *const volatile' instead of 'const int *volatile'
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'i5' declared with a const-qualified typedef; results in the type being 'int *const volatile' instead of 'const int *volatile'
 }
 
 void func2(const plain_i *i1,
Index: clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst
+++ clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst
@@ -3,10 +3,10 @@
 misc-misplaced-const
 
 
-This check diagnoses when a ``const`` qualifier is applied to a ``typedef`` to a
-pointer type rather than to the pointee, because such constructs are often
-misleading to developers because the ``const`` applies to the pointer rather
-than the pointee.
+This check diagnoses when a ``const`` qualifier is applied to a ``typedef``/
+``using`` to a pointer type rather than to the pointee, because such constructs
+are often misleading to developers because the ``const`` applies to the pointer
+rather than the pointee.
 
 For instance, in the following code, the resulting type is ``int *`` ``const``
 rather than ``const int *``:
@@ -14,9 +14,12 @@
 .. code-block:: c++
 
   typedef int *int_ptr;
-  void f(const int_ptr ptr);
+  void f(const int_ptr ptr) {
+*ptr = 0; // potentially quite unexpectedly the int can be modified here
+ptr = 0; // does not 

[PATCH] D72373: [clang-tidy] extend misc-misplaced-const to detect using besides typedef

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

ping

> Could someone commit this? As I can not.
>  Alexander Lanin 




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

https://reviews.llvm.org/D72373



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

[PATCH] D72373: [clang-tidy] extend misc-misplaced-const to detect using besides typedef

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

Thanks for the review!

Could someone commit this? As I can not.
Alexander Lanin 


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

https://reviews.llvm.org/D72373



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

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

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin created this revision.
AlexanderLanin added a project: clang-tools-extra.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
AlexanderLanin added a comment.

Important: I'm not a python expert. It works fine as far as I can tell. I would 
feel better if someone with more than a day python experience would say that it 
is fine.


The generated file is now small enough to not hinder any post processing.

>> Create a yaml file to store suggested fixes in, which can be applied with 
>> clang-apply-replacements.

So it's enough to store the fixes, not all the warnings.
Before this change the resulting file was simply too large as it contained all 
warnings, least of which had FIX-ITs.

This is trivially achievable with no measureable runtime overhead during 
run-clang-tidy runs, instead of post processing where even parsing the file 
once is extremely slow.


Repository:
  rG LLVM Github Monorepo

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
+def isAutoFixable(diag):
+  if diag["DiagnosticMessage"]["Replacements"]: return True
+
+  if "Notes" in diag:
+for note in diag["Notes"]:
+  if "Replacements" in note and note["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
+def isAutoFixable(diag):
+  if diag["DiagnosticMessage"]["Replacements"]: return True
+
+  if "Notes" in diag:
+for note in diag["Notes"]:
+  if "Replacements" in note and note["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-tools-extra] run-clang-tidy -export-fixes exports only fixes, not all warnings

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

Important: I'm not a python expert. It works fine as far as I can tell. I would 
feel better if someone with more than a day python experience would say that it 
is fine.


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

[PATCH] D72373: [clang-tidy] extend misc-misplaced-const to detect using besides typedef

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin updated this revision to Diff 237558.
AlexanderLanin marked an inline comment as done.
AlexanderLanin added a comment.

Updated revision with llvm_unreachable


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

https://reviews.llvm.org/D72373

Files:
  clang-tools-extra/clang-tidy/misc/MisplacedConstCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst
  clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.cpp
@@ -1,20 +1,40 @@
-// RUN: %check_clang_tidy %s misc-misplaced-const %t
-
-typedef int plain_i;
-typedef int *ip;
-typedef const int *cip;
-
-void func() {
-  if (const int *i = 0)
-;
-  if (const plain_i *i = 0)
-;
-  if (const cip i = 0)
-;
-
-  // CHECK-MESSAGES: :[[@LINE+1]]:16: warning: 'i' declared with a const-qualified typedef type; results in the type being 'int *const' instead of 'const int *'
-  if (const ip i = 0)
-;
+// RUN: %check_clang_tidy %s misc-misplaced-const %t -- -- -DUSING
+// RUN: %check_clang_tidy %s misc-misplaced-const %t -- -- -DTYPEDEF
+
+#ifdef TYPEDEF
+typedef int int_;
+typedef int *ptr_to_int;
+typedef const int *ptr_to_const_int;
+#endif
+#ifdef USING
+using int_ = int;
+using ptr_to_int = int *;
+using ptr_to_const_int = const int *;
+#endif
+
+void const_pointers() {
+  if (const int *i = 0) {
+i = 0;
+// *i = 0;
+  }
+
+  if (const int_ *i = 0) {
+i = 0;
+// *i = 0;
+  }
+
+  if (const ptr_to_const_int i = 0) {
+// i = 0;
+// *i = 0;
+  }
+
+  // Potentially quite unexpectedly the int can be modified here
+  // CHECK-MESSAGES: :[[@LINE+1]]:23: warning: 'i' declared with a const-qualified {{.*}}; results in the type being 'int *const' instead of 'const int *'
+  if (const ptr_to_int i = 0) {
+//i = 0;
+
+*i = 0;
+  }
 }
 
 template 
@@ -24,8 +44,8 @@
 };
 
 template struct S;
-template struct S; // ok
-template struct S;
+template struct S; // ok
+template struct S;
 
 template 
 struct U {
Index: clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst
+++ clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst
@@ -3,10 +3,10 @@
 misc-misplaced-const
 
 
-This check diagnoses when a ``const`` qualifier is applied to a ``typedef`` to a
-pointer type rather than to the pointee, because such constructs are often
-misleading to developers because the ``const`` applies to the pointer rather
-than the pointee.
+This check diagnoses when a ``const`` qualifier is applied to a ``typedef``/
+``using`` to a pointer type rather than to the pointee, because such constructs
+are often misleading to developers because the ``const`` applies to the pointer
+rather than the pointee.
 
 For instance, in the following code, the resulting type is ``int *`` ``const``
 rather than ``const int *``:
@@ -14,9 +14,12 @@
 .. code-block:: c++
 
   typedef int *int_ptr;
-  void f(const int_ptr ptr);
+  void f(const int_ptr ptr) {
+*ptr = 0; // potentially quite unexpectedly the int can be modified here
+ptr = 0; // does not compile
+  }
 
-The check does not diagnose when the underlying ``typedef`` type is a pointer to
-a ``const`` type or a function pointer type. This is because the ``const``
-qualifier is less likely to be mistaken because it would be redundant (or
-disallowed) on the underlying pointee type.
+The check does not diagnose when the underlying ``typedef``/``using`` type is a
+pointer to a ``const`` type or a function pointer type. This is because the
+``const`` qualifier is less likely to be mistaken because it would be redundant
+(or disallowed) on the underlying pointee type.
Index: clang-tools-extra/clang-tidy/misc/MisplacedConstCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/MisplacedConstCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/MisplacedConstCheck.cpp
@@ -17,13 +17,16 @@
 namespace misc {
 
 void MisplacedConstCheck::registerMatchers(MatchFinder *Finder) {
+  auto NonConstAndNonFunctionPointerType = hasType(pointerType(unless(
+  pointee(anyOf(isConstQualified(), ignoringParens(functionType()));
+
   Finder->addMatcher(
-  valueDecl(hasType(isConstQualified()),
-hasType(typedefType(hasDeclaration(
-typedefDecl(hasType(pointerType(unless(pointee(
-anyOf(isConstQualified(),
-  ignoringParens(functionType(
-.bind("typedef")
+  valueDecl(
+  

[PATCH] D72373: [clang-tidy] extend misc-misplaced-const to detect using besides typedef

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

looking at this at least many are indeed valid results: F11183407: 
partial-misc-misplaced-const-llvm-output.zip 

I guess false positives could be reduced by eliminating those cases where the 
variables are (intentionally) modified.

FIX-IT isn't quite that obvious. Some options:

- look for other typedef which contains the same as const
- create new typedef/using
- remove "*" from typedef and adjust all usage accordingly. Implies removing 
"Ptr" suffix.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.cpp:2
+// RUN: %check_clang_tidy %s misc-misplaced-const %t -- -- -DUSING
+// RUN: %check_clang_tidy %s misc-misplaced-const %t -- -- -DTYPEDEF
+

is this good practice? I didn't want to duplicate everything here.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.cpp:32
+  // Potentially quite unexpectedly the int can be modified here
+  // CHECK-MESSAGES: :[[@LINE+1]]:23: warning: 'i' declared with a 
const-qualified {{.*}}; results in the type being 'int *const' instead of 
'const int *'
+  if (const ptr_to_int i = 0) {

not sure how to remove the regex here without duplicating everything


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72373



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

[PATCH] D72373: [clang-tidy] extend misc-misplaced-const to detect using besides typedef

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin created this revision.
AlexanderLanin added a reviewer: aaron.ballman.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72373

Files:
  clang-tools-extra/clang-tidy/misc/MisplacedConstCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst
  clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.cpp
@@ -1,20 +1,40 @@
-// RUN: %check_clang_tidy %s misc-misplaced-const %t
-
-typedef int plain_i;
-typedef int *ip;
-typedef const int *cip;
-
-void func() {
-  if (const int *i = 0)
-;
-  if (const plain_i *i = 0)
-;
-  if (const cip i = 0)
-;
-
-  // CHECK-MESSAGES: :[[@LINE+1]]:16: warning: 'i' declared with a const-qualified typedef type; results in the type being 'int *const' instead of 'const int *'
-  if (const ip i = 0)
-;
+// RUN: %check_clang_tidy %s misc-misplaced-const %t -- -- -DUSING
+// RUN: %check_clang_tidy %s misc-misplaced-const %t -- -- -DTYPEDEF
+
+#ifdef TYPEDEF
+typedef int int_;
+typedef int *ptr_to_int;
+typedef const int *ptr_to_const_int;
+#endif
+#ifdef USING
+using int_ = int;
+using ptr_to_int = int *;
+using ptr_to_const_int = const int *;
+#endif
+
+void const_pointers() {
+  if (const int *i = 0) {
+i = 0;
+// *i = 0;
+  }
+
+  if (const int_ *i = 0) {
+i = 0;
+// *i = 0;
+  }
+
+  if (const ptr_to_const_int i = 0) {
+// i = 0;
+// *i = 0;
+  }
+
+  // Potentially quite unexpectedly the int can be modified here
+  // CHECK-MESSAGES: :[[@LINE+1]]:23: warning: 'i' declared with a const-qualified {{.*}}; results in the type being 'int *const' instead of 'const int *'
+  if (const ptr_to_int i = 0) {
+//i = 0;
+
+*i = 0;
+  }
 }
 
 template 
@@ -24,8 +44,8 @@
 };
 
 template struct S;
-template struct S; // ok
-template struct S;
+template struct S; // ok
+template struct S;
 
 template 
 struct U {
Index: clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst
+++ clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst
@@ -3,10 +3,10 @@
 misc-misplaced-const
 
 
-This check diagnoses when a ``const`` qualifier is applied to a ``typedef`` to a
-pointer type rather than to the pointee, because such constructs are often
-misleading to developers because the ``const`` applies to the pointer rather
-than the pointee.
+This check diagnoses when a ``const`` qualifier is applied to a ``typedef``/
+``using`` to a pointer type rather than to the pointee, because such constructs
+are often misleading to developers because the ``const`` applies to the pointer
+rather than the pointee.
 
 For instance, in the following code, the resulting type is ``int *`` ``const``
 rather than ``const int *``:
@@ -14,9 +14,12 @@
 .. code-block:: c++
 
   typedef int *int_ptr;
-  void f(const int_ptr ptr);
+  void f(const int_ptr ptr) {
+*ptr = 0; // potentially quite unexpectedly the int can be modified here
+ptr = 0; // does not compile
+  }
 
-The check does not diagnose when the underlying ``typedef`` type is a pointer to
-a ``const`` type or a function pointer type. This is because the ``const``
-qualifier is less likely to be mistaken because it would be redundant (or
-disallowed) on the underlying pointee type.
+The check does not diagnose when the underlying ``typedef``/``using`` type is a
+pointer to a ``const`` type or a function pointer type. This is because the
+``const`` qualifier is less likely to be mistaken because it would be redundant
+(or disallowed) on the underlying pointee type.
Index: clang-tools-extra/clang-tidy/misc/MisplacedConstCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/MisplacedConstCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/MisplacedConstCheck.cpp
@@ -17,13 +17,16 @@
 namespace misc {
 
 void MisplacedConstCheck::registerMatchers(MatchFinder *Finder) {
+  auto NonConstAndNonFunctionPointerType = hasType(pointerType(unless(
+  pointee(anyOf(isConstQualified(), ignoringParens(functionType()));
+
   Finder->addMatcher(
-  valueDecl(hasType(isConstQualified()),
-hasType(typedefType(hasDeclaration(
-typedefDecl(hasType(pointerType(unless(pointee(
-anyOf(isConstQualified(),
-  ignoringParens(functionType(
-.bind("typedef")
+  valueDecl(
+  hasType(isConstQualified()),
+  

[PATCH] D72057: [docs] Remove outdated svn/git information from hacking page

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

In D72057#1802982 , @aaron.ballman 
wrote:

> LGTM with a small typo fix. Do you need someone to commit this on your behalf?


Yes, please. I cannot commit myself.
Alexander Lanin 


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

https://reviews.llvm.org/D72057



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

[PATCH] D72057: [docs] Remove outdated svn/git information from hacking page

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin updated this revision to Diff 236088.
AlexanderLanin marked an inline comment as done.
AlexanderLanin added a comment.

`Getting Started` since it's `Getting Started with the LLVM System`


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

https://reviews.llvm.org/D72057

Files:
  clang/www/hacking.html


Index: clang/www/hacking.html
===
--- clang/www/hacking.html
+++ clang/www/hacking.html
@@ -275,30 +275,8 @@
   Creating Patch Files
   
 
-  To return changes to the Clang team, unless you have checkin
-  privileges, the preferred way is to send patch files
-  https://llvm.org/docs/Contributing.html#how-to-submit-a-patch;>using 
LLVM's Phabricator with an explanation of what the patch is for. Clang 
follows https://llvm.org/docs/DeveloperPolicy.html;>LLVM's developer 
policy.
-  If your patch requires a wider discussion (for example, because it is an
-  architectural change), you can use the cfe-dev mailing list.
-
-  To create these patch files, change directory
-  to the llvm/tools/clang root and run:
-
-  svn diff (relative path) >(patch file name)
-
-  For example, for getting the diffs of all of clang:
-
-  svn diff . >~/mypatchfile.patch
-
-  For example, for getting the diffs of a single file:
-
-  svn diff lib/Parse/ParseDeclCXX.cpp >~/ParseDeclCXX.patch
-
-  Note that the paths embedded in the patch depend on where you run it,
-  so changing directory to the llvm/tools/clang directory is recommended.
-
-  It is also possible to https://llvm.org/docs/GettingStarted.html#sending-patches-with-git;>use 
git to contribute to Clang.
+  To contribute changes to Clang see
+https://llvm.org/docs/GettingStarted.html#sending-patches;>LLVM's 
Getting Started page
 
   
   LLVM IR Generation


Index: clang/www/hacking.html
===
--- clang/www/hacking.html
+++ clang/www/hacking.html
@@ -275,30 +275,8 @@
   Creating Patch Files
   
 
-  To return changes to the Clang team, unless you have checkin
-  privileges, the preferred way is to send patch files
-  https://llvm.org/docs/Contributing.html#how-to-submit-a-patch;>using LLVM's Phabricator with an explanation of what the patch is for. Clang follows https://llvm.org/docs/DeveloperPolicy.html;>LLVM's developer policy.
-  If your patch requires a wider discussion (for example, because it is an
-  architectural change), you can use the cfe-dev mailing list.
-
-  To create these patch files, change directory
-  to the llvm/tools/clang root and run:
-
-  svn diff (relative path) >(patch file name)
-
-  For example, for getting the diffs of all of clang:
-
-  svn diff . >~/mypatchfile.patch
-
-  For example, for getting the diffs of a single file:
-
-  svn diff lib/Parse/ParseDeclCXX.cpp >~/ParseDeclCXX.patch
-
-  Note that the paths embedded in the patch depend on where you run it,
-  so changing directory to the llvm/tools/clang directory is recommended.
-
-  It is also possible to https://llvm.org/docs/GettingStarted.html#sending-patches-with-git;>use git to contribute to Clang.
+  To contribute changes to Clang see
+https://llvm.org/docs/GettingStarted.html#sending-patches;>LLVM's Getting Started page
 
   
   LLVM IR Generation
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72057: [docs] Remove outdated svn/git information from hacking page

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin updated this revision to Diff 235968.
AlexanderLanin retitled this revision from "[docs] Update dead anchor in 
hacking page" to "[docs] Remove outdated svn/git information from hacking page".
AlexanderLanin edited the summary of this revision.
AlexanderLanin added a comment.

Not sure we can call this updated proposal 'love': it's definitely not the goal 
to have less documentation, just less redundancy.

I'll remove redundancies between those two in a separate pull request:

- https://llvm.org/docs/Contributing.html#how-to-submit-a-patch
- https://llvm.org/docs/GettingStarted.html#sending-patches


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

https://reviews.llvm.org/D72057

Files:
  clang/www/hacking.html


Index: clang/www/hacking.html
===
--- clang/www/hacking.html
+++ clang/www/hacking.html
@@ -275,30 +275,8 @@
   Creating Patch Files
   
 
-  To return changes to the Clang team, unless you have checkin
-  privileges, the preferred way is to send patch files
-  https://llvm.org/docs/Contributing.html#how-to-submit-a-patch;>using 
LLVM's Phabricator with an explanation of what the patch is for. Clang 
follows https://llvm.org/docs/DeveloperPolicy.html;>LLVM's developer 
policy.
-  If your patch requires a wider discussion (for example, because it is an
-  architectural change), you can use the cfe-dev mailing list.
-
-  To create these patch files, change directory
-  to the llvm/tools/clang root and run:
-
-  svn diff (relative path) >(patch file name)
-
-  For example, for getting the diffs of all of clang:
-
-  svn diff . >~/mypatchfile.patch
-
-  For example, for getting the diffs of a single file:
-
-  svn diff lib/Parse/ParseDeclCXX.cpp >~/ParseDeclCXX.patch
-
-  Note that the paths embedded in the patch depend on where you run it,
-  so changing directory to the llvm/tools/clang directory is recommended.
-
-  It is also possible to https://llvm.org/docs/GettingStarted.html#sending-patches-with-git;>use 
git to contribute to Clang.
+  To contribute changes to Clang see
+https://llvm.org/docs/GettingStarted.html#sending-patches;>LLVM's 
Getting started page
 
   
   LLVM IR Generation


Index: clang/www/hacking.html
===
--- clang/www/hacking.html
+++ clang/www/hacking.html
@@ -275,30 +275,8 @@
   Creating Patch Files
   
 
-  To return changes to the Clang team, unless you have checkin
-  privileges, the preferred way is to send patch files
-  https://llvm.org/docs/Contributing.html#how-to-submit-a-patch;>using LLVM's Phabricator with an explanation of what the patch is for. Clang follows https://llvm.org/docs/DeveloperPolicy.html;>LLVM's developer policy.
-  If your patch requires a wider discussion (for example, because it is an
-  architectural change), you can use the cfe-dev mailing list.
-
-  To create these patch files, change directory
-  to the llvm/tools/clang root and run:
-
-  svn diff (relative path) >(patch file name)
-
-  For example, for getting the diffs of all of clang:
-
-  svn diff . >~/mypatchfile.patch
-
-  For example, for getting the diffs of a single file:
-
-  svn diff lib/Parse/ParseDeclCXX.cpp >~/ParseDeclCXX.patch
-
-  Note that the paths embedded in the patch depend on where you run it,
-  so changing directory to the llvm/tools/clang directory is recommended.
-
-  It is also possible to https://llvm.org/docs/GettingStarted.html#sending-patches-with-git;>use git to contribute to Clang.
+  To contribute changes to Clang see
+https://llvm.org/docs/GettingStarted.html#sending-patches;>LLVM's Getting started page
 
   
   LLVM IR Generation
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72057: [docs] Update dead anchor in hacking page

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin marked an inline comment as done.
AlexanderLanin added inline comments.



Comment at: clang/www/hacking.html:301
 
-  It is also possible to https://llvm.org/docs/GettingStarted.html#sending-patches-with-git;>use 
git to contribute to Clang.
+  It is also possible to https://llvm.org/docs/GettingStarted.html#checkout-llvm-from-git;>use git 
to contribute to Clang.
 

aaron.ballman wrote:
> AlexanderLanin wrote:
> > aaron.ballman wrote:
> > > Should this instead point to 
> > > `https://llvm.org/docs/GettingStarted.html#sending-patches` to more 
> > > closely match the original target anchor?
> > For a precise match yes, but I figured that one would need to start with 
> > git first of all as git is not mentioned anywhere else on the page.
> That's fair, but I think this whole section needs a bit more love than what's 
> proposed. You cannot use svn diff for creating patches within a git repo. 
> This text only makes sense when we were still doing the transition from svn 
> to git, and the bit you're changing is the "oh yeah, or you can use git if 
> you want" stuff. Now we need it to read "This is how you do this with git", 
> at which point the checkout from git link isn't as useful as pointing out how 
> you send a patch (which is the next logical step after forming a patch file).
> 
> Would you like to take a stab at updating this section rather than just the 
> link?
Actually it would make sense to remove the 'Creating Patch Files' section here 
as that's redundant to https://llvm.org/docs/GettingStarted.html. There is 
nothing clang specific here. We are talking about a monorepo after all.
While it may seem nice to have one single clang page which explains everything, 
it's not:
It doesn't cover the topics in sufficient depth, it's redundant to other pages 
and it's hard to keep it up to date as we see with svn usage.

It's the same with:
* https://llvm.org/docs/Contributing.html#how-to-submit-a-patch
* https://llvm.org/docs/GettingStarted.html#sending-patches
which seem to simply have diverged over time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72057



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

[PATCH] D71982: [docs] Update path to clang-tools-extra

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

@Jim `git commit --amend --author="Alexander Lanin "`


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

https://reviews.llvm.org/D71982



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

[PATCH] D71982: [docs] Update path to clang-tools-extra

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin updated this revision to Diff 235801.
AlexanderLanin marked an inline comment as done.
AlexanderLanin edited the summary of this revision.
AlexanderLanin added a comment.

Removed change in hacking page as discussed.
Can someone commit this as apparently I cannot do it myself (I'll create a PR 
with updated getting started documentation later...), thanks.


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

https://reviews.llvm.org/D71982

Files:
  clang-tools-extra/docs/clang-include-fixer.rst
  clang-tools-extra/docs/clang-tidy/Contributing.rst
  clang-tools-extra/docs/pp-trace.rst

Index: clang-tools-extra/docs/pp-trace.rst
===
--- clang-tools-extra/docs/pp-trace.rst
+++ clang-tools-extra/docs/pp-trace.rst
@@ -104,16 +104,16 @@
 
   ---
   - Callback: FileChanged
-Loc: "c:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-include.cpp:1:1"
+Loc: "c:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-include.cpp:1:1"
 Reason: EnterFile
 FileType: C_User
 PrevFID: (invalid)
 (etc.)
   - Callback: FileChanged
-Loc: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-include.cpp:5:1"
+Loc: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-include.cpp:5:1"
 Reason: ExitFile
 FileType: C_User
-PrevFID: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/Input/Level1B.h"
+PrevFID: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/Input/Level1B.h"
   - Callback: EndOfMainFile
   ...
 
@@ -172,7 +172,7 @@
 Example:::
 
   - Callback: FileChanged
-Loc: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-include.cpp:1:1"
+Loc: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-include.cpp:1:1"
 Reason: EnterFile
 FileType: C_User
 PrevFID: (invalid)
@@ -248,8 +248,8 @@
 FileName: "Input/Level1B.h"
 IsAngled: false
 FilenameRange: "Input/Level1B.h"
-File: "D:/Clang/llvmnewmod/tools/clang/tools/extra/test/pp-trace/Input/Level1B.h"
-SearchPath: "D:/Clang/llvmnewmod/tools/clang/tools/extra/test/pp-trace"
+File: "D:/Clang/llvmnewmod/clang-tools-extra/test/pp-trace/Input/Level1B.h"
+SearchPath: "D:/Clang/llvmnewmod/clang-tools-extra/test/pp-trace"
 RelativePath: "Input/Level1B.h"
 Imported: (null)
 
@@ -271,8 +271,8 @@
 Example:::
 
   - Callback: moduleImport
-ImportLoc: "d:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-modules.cpp:4:2"
-Path: [{Name: Level1B, Loc: "d:/Clang/llvmnewmod/tools/clang/tools/extra/test/pp-trace/pp-trace-modules.cpp:4:9"}, {Name: Level2B, Loc: "d:/Clang/llvmnewmod/tools/clang/tools/extra/test/pp-trace/pp-trace-modules.cpp:4:17"}]
+ImportLoc: "d:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-modules.cpp:4:2"
+Path: [{Name: Level1B, Loc: "d:/Clang/llvmnewmod/clang-tools-extra/test/pp-trace/pp-trace-modules.cpp:4:9"}, {Name: Level2B, Loc: "d:/Clang/llvmnewmod/clang-tools-extra/test/pp-trace/pp-trace-modules.cpp:4:17"}]
 Imported: Level2B
 
 `EndOfMainFile `_ Callback
@@ -309,7 +309,7 @@
 Example:::
 
   - Callback: Ident
-Loc: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-ident.cpp:3:1"
+Loc: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-ident.cpp:3:1"
 str: "$Id$"
 
 `PragmaDirective `_ Callback
@@ -329,7 +329,7 @@
 Example:::
 
   - Callback: PragmaDirective
-Loc: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-pragma.cpp:3:1"
+Loc: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-pragma.cpp:3:1"
 Introducer: PIK_HashPragma
 
 `PragmaComment `_ Callback
@@ -350,7 +350,7 @@
 Example:::
 
   - Callback: PragmaComment
-Loc: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-pragma.cpp:3:1"
+Loc: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-pragma.cpp:3:1"
 Kind: library
 Str: kernel32.lib
 
@@ -372,7 +372,7 @@
 Example:::
 
   - Callback: PragmaDetectMismatch
-Loc: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-pragma.cpp:3:1"
+Loc: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-pragma.cpp:3:1"
 Name: name
 Value: value
 
@@ -393,7 +393,7 @@
 Example:::
 
   - Callback: PragmaDebug
-Loc: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-pragma.cpp:3:1"
+Loc: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-pragma.cpp:3:1"
 DebugType: warning
 
 `PragmaMessage `_ Callback
@@ -415,7 +415,7 @@
 Example:::
 
   - Callback: PragmaMessage
-Loc: 

[PATCH] D72057: [docs] Update dead anchor in hacking page

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin marked an inline comment as done.
AlexanderLanin added inline comments.



Comment at: clang/www/hacking.html:301
 
-  It is also possible to https://llvm.org/docs/GettingStarted.html#sending-patches-with-git;>use 
git to contribute to Clang.
+  It is also possible to https://llvm.org/docs/GettingStarted.html#checkout-llvm-from-git;>use git 
to contribute to Clang.
 

aaron.ballman wrote:
> Should this instead point to 
> `https://llvm.org/docs/GettingStarted.html#sending-patches` to more closely 
> match the original target anchor?
For a precise match yes, but I figured that one would need to start with git 
first of all as git is not mentioned anywhere else on the page.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72057



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

[PATCH] D71982: [docs] Update path to clang-tools-extra

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin marked 3 inline comments as done.
AlexanderLanin added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:66
 
-Once you are done, change to the ``llvm/tools/clang/tools/extra`` directory, 
and
+Once you are done, change to the ``llvm/clang-tools-extra`` directory, and
 let's start!

aaron.ballman wrote:
> Jim wrote:
> > I am no sure that "llvm/clang-tools-extra" should be replaced as 
> > "llvm-project/clang-tools-extra".
> > Maybe someone would confuse with "llvm", "llvm-project" and 
> > "llvm-project/llvm"
> Elsewhere we use `path/to/llvm/source`, which seems to be sufficiently clear.
While this goes slightly beyond the scope of the original pull request I tend 
to agree as `llvm` can easily be confused with `llvm-project/llvm` as Jim wrote.
However I'm not clear on the exact target: looking through other docs probably 
most often you'll find `path/to/llvm/source` as Aaron mentioned, but other 
times it's `/path/to/llvm-project/`, `llvm-project/`, `~/llvm/`, 
`~/clang-llvm/`, `/path/to/llvm`, `/path/to/llvm/src`, `/path/to/llvm/sources` 
or `/path/to/llvm/tree`.

While this is not that important, it's difficult enough to get started with 
anything inside llvm as it is. This is low hanging fruit. I would create a 
separate pull request afterwards to align those.



Comment at: clang/www/hacking.html:301
 
-  It is also possible to https://llvm.org/docs/GettingStarted.html#sending-patches-with-git;>use 
git to contribute to Clang.
+  It is also possible to https://llvm.org/docs/GettingStarted.html#checkout-llvm-from-git;>use git 
to contribute to Clang.
 

Jim wrote:
> This change should be in a separate patch.
Ok, separate patch: https://reviews.llvm.org/D72057


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

https://reviews.llvm.org/D71982



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

[PATCH] D72057: [docs] Update dead anchor in hacking page

[Alexander Lanin via Phabricator via cfe-commits]

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

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72057

Files:
  clang/www/hacking.html


Index: clang/www/hacking.html
===
--- clang/www/hacking.html
+++ clang/www/hacking.html
@@ -298,7 +298,7 @@
   Note that the paths embedded in the patch depend on where you run it,
   so changing directory to the llvm/tools/clang directory is recommended.
 
-  It is also possible to https://llvm.org/docs/GettingStarted.html#sending-patches-with-git;>use 
git to contribute to Clang.
+  It is also possible to https://llvm.org/docs/GettingStarted.html#checkout-llvm-from-git;>use git 
to contribute to Clang.
 
   
   LLVM IR Generation


Index: clang/www/hacking.html
===
--- clang/www/hacking.html
+++ clang/www/hacking.html
@@ -298,7 +298,7 @@
   Note that the paths embedded in the patch depend on where you run it,
   so changing directory to the llvm/tools/clang directory is recommended.
 
-  It is also possible to https://llvm.org/docs/GettingStarted.html#sending-patches-with-git;>use git to contribute to Clang.
+  It is also possible to https://llvm.org/docs/GettingStarted.html#checkout-llvm-from-git;>use git to contribute to Clang.
 
   
   LLVM IR Generation
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71982: [docs] Update path to clang-tools-extra

[Alexander Lanin via Phabricator via cfe-commits]

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

Hi,
I don't see the error message you mentioned anywhere, but I'm gonna assume it 
involves the number of surrounding lines. Here is another attempt with

> git show HEAD -U99 > mypatch.patch



---

Source of problem:
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
 tells me to use

> git format-patch -U99 @ {u}

But then upload fails:

> Diff Parse Exception: Expected a hunk header, like 'Index: /path/to/file.ext' 
> (svn), 'Property changes on: /path/to/file.ext' (svn properties), 'commit 
> 59bcc3ad6775562f845953cf01624225' (git show), 'diff --git' (git diff), '--- 
> filename' (unified diff), or 'diff -r' (hg diff or patch).
> 
>   1885for more information.
>   1886
>   1887
>   1888
>   1889
>   >>> 1890   -- 
>   1891   2.24.1.windows.2
>   1892   

https://llvm.org/docs/GettingStarted.html#sending-patches just casually 
mentions git show and git format-patch without telling anything about options 
to be used.

Assuming 'git show HEAD -U99' is the one and only correct way? 'git diff 
HEAD~1 -U99' seems to work ok also.
However 'git format-patch' doesn't work as mentioned above. At least for me 
with git version 2.24.1.windows.2


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

https://reviews.llvm.org/D71982

Files:
  clang-tools-extra/docs/clang-include-fixer.rst
  clang-tools-extra/docs/clang-tidy/Contributing.rst
  clang-tools-extra/docs/pp-trace.rst
  clang/www/hacking.html

Index: clang/www/hacking.html
===
--- clang/www/hacking.html
+++ clang/www/hacking.html
@@ -298,7 +298,7 @@
   Note that the paths embedded in the patch depend on where you run it,
   so changing directory to the llvm/tools/clang directory is recommended.
 
-  It is also possible to https://llvm.org/docs/GettingStarted.html#sending-patches-with-git;>use git to contribute to Clang.
+  It is also possible to https://llvm.org/docs/GettingStarted.html#checkout-llvm-from-git;>use git to contribute to Clang.
 
   
   LLVM IR Generation
Index: clang-tools-extra/docs/pp-trace.rst
===
--- clang-tools-extra/docs/pp-trace.rst
+++ clang-tools-extra/docs/pp-trace.rst
@@ -104,16 +104,16 @@
 
   ---
   - Callback: FileChanged
-Loc: "c:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-include.cpp:1:1"
+Loc: "c:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-include.cpp:1:1"
 Reason: EnterFile
 FileType: C_User
 PrevFID: (invalid)
 (etc.)
   - Callback: FileChanged
-Loc: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-include.cpp:5:1"
+Loc: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-include.cpp:5:1"
 Reason: ExitFile
 FileType: C_User
-PrevFID: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/Input/Level1B.h"
+PrevFID: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/Input/Level1B.h"
   - Callback: EndOfMainFile
   ...
 
@@ -172,7 +172,7 @@
 Example:::
 
   - Callback: FileChanged
-Loc: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-include.cpp:1:1"
+Loc: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-include.cpp:1:1"
 Reason: EnterFile
 FileType: C_User
 PrevFID: (invalid)
@@ -248,8 +248,8 @@
 FileName: "Input/Level1B.h"
 IsAngled: false
 FilenameRange: "Input/Level1B.h"
-File: "D:/Clang/llvmnewmod/tools/clang/tools/extra/test/pp-trace/Input/Level1B.h"
-SearchPath: "D:/Clang/llvmnewmod/tools/clang/tools/extra/test/pp-trace"
+File: "D:/Clang/llvmnewmod/clang-tools-extra/test/pp-trace/Input/Level1B.h"
+SearchPath: "D:/Clang/llvmnewmod/clang-tools-extra/test/pp-trace"
 RelativePath: "Input/Level1B.h"
 Imported: (null)
 
@@ -271,8 +271,8 @@
 Example:::
 
   - Callback: moduleImport
-ImportLoc: "d:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-modules.cpp:4:2"
-Path: [{Name: Level1B, Loc: "d:/Clang/llvmnewmod/tools/clang/tools/extra/test/pp-trace/pp-trace-modules.cpp:4:9"}, {Name: Level2B, Loc: "d:/Clang/llvmnewmod/tools/clang/tools/extra/test/pp-trace/pp-trace-modules.cpp:4:17"}]
+ImportLoc: "d:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-modules.cpp:4:2"
+Path: [{Name: Level1B, Loc: "d:/Clang/llvmnewmod/clang-tools-extra/test/pp-trace/pp-trace-modules.cpp:4:9"}, {Name: Level2B, Loc: "d:/Clang/llvmnewmod/clang-tools-extra/test/pp-trace/pp-trace-modules.cpp:4:17"}]
 Imported: Level2B
 
 `EndOfMainFile `_ Callback
@@ -309,7 +309,7 @@
 Example:::
 
   - Callback: Ident
-Loc: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-ident.cpp:3:1"
+Loc: 

[PATCH] D71982: [docs] Update path to clang-tools-extra

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin created this revision.
AlexanderLanin added projects: clang-tools-extra, clang.
Herald added a subscriber: cfe-commits.

> tools/clang/tools/extra

has become

> clang-tools-extra

which was not updated in all docs.

Also hacking page has an outdated anchor for git.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71982

Files:
  clang-tools-extra/docs/clang-include-fixer.rst
  clang-tools-extra/docs/clang-tidy/Contributing.rst
  clang-tools-extra/docs/pp-trace.rst
  clang/www/hacking.html

Index: clang/www/hacking.html
===
--- clang/www/hacking.html
+++ clang/www/hacking.html
@@ -298,7 +298,7 @@
   Note that the paths embedded in the patch depend on where you run it,
   so changing directory to the llvm/tools/clang directory is recommended.
 
-  It is also possible to https://llvm.org/docs/GettingStarted.html#sending-patches-with-git;>use git to contribute to Clang.
+  It is also possible to https://llvm.org/docs/GettingStarted.html#checkout-llvm-from-git;>use git to contribute to Clang.
 
   
   LLVM IR Generation
Index: clang-tools-extra/docs/pp-trace.rst
===
--- clang-tools-extra/docs/pp-trace.rst
+++ clang-tools-extra/docs/pp-trace.rst
@@ -104,16 +104,16 @@
 
   ---
   - Callback: FileChanged
-Loc: "c:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-include.cpp:1:1"
+Loc: "c:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-include.cpp:1:1"
 Reason: EnterFile
 FileType: C_User
 PrevFID: (invalid)
 (etc.)
   - Callback: FileChanged
-Loc: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-include.cpp:5:1"
+Loc: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-include.cpp:5:1"
 Reason: ExitFile
 FileType: C_User
-PrevFID: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/Input/Level1B.h"
+PrevFID: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/Input/Level1B.h"
   - Callback: EndOfMainFile
   ...
 
@@ -172,7 +172,7 @@
 Example:::
 
   - Callback: FileChanged
-Loc: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-include.cpp:1:1"
+Loc: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-include.cpp:1:1"
 Reason: EnterFile
 FileType: C_User
 PrevFID: (invalid)
@@ -248,8 +248,8 @@
 FileName: "Input/Level1B.h"
 IsAngled: false
 FilenameRange: "Input/Level1B.h"
-File: "D:/Clang/llvmnewmod/tools/clang/tools/extra/test/pp-trace/Input/Level1B.h"
-SearchPath: "D:/Clang/llvmnewmod/tools/clang/tools/extra/test/pp-trace"
+File: "D:/Clang/llvmnewmod/clang-tools-extra/test/pp-trace/Input/Level1B.h"
+SearchPath: "D:/Clang/llvmnewmod/clang-tools-extra/test/pp-trace"
 RelativePath: "Input/Level1B.h"
 Imported: (null)
 
@@ -271,8 +271,8 @@
 Example:::
 
   - Callback: moduleImport
-ImportLoc: "d:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-modules.cpp:4:2"
-Path: [{Name: Level1B, Loc: "d:/Clang/llvmnewmod/tools/clang/tools/extra/test/pp-trace/pp-trace-modules.cpp:4:9"}, {Name: Level2B, Loc: "d:/Clang/llvmnewmod/tools/clang/tools/extra/test/pp-trace/pp-trace-modules.cpp:4:17"}]
+ImportLoc: "d:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-modules.cpp:4:2"
+Path: [{Name: Level1B, Loc: "d:/Clang/llvmnewmod/clang-tools-extra/test/pp-trace/pp-trace-modules.cpp:4:9"}, {Name: Level2B, Loc: "d:/Clang/llvmnewmod/clang-tools-extra/test/pp-trace/pp-trace-modules.cpp:4:17"}]
 Imported: Level2B
 
 `EndOfMainFile `_ Callback
@@ -309,7 +309,7 @@
 Example:::
 
   - Callback: Ident
-Loc: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-ident.cpp:3:1"
+Loc: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-ident.cpp:3:1"
 str: "$Id$"
 
 `PragmaDirective `_ Callback
@@ -329,7 +329,7 @@
 Example:::
 
   - Callback: PragmaDirective
-Loc: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-pragma.cpp:3:1"
+Loc: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-pragma.cpp:3:1"
 Introducer: PIK_HashPragma
 
 `PragmaComment `_ Callback
@@ -350,7 +350,7 @@
 Example:::
 
   - Callback: PragmaComment
-Loc: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-pragma.cpp:3:1"
+Loc: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-pragma.cpp:3:1"
 Kind: library
 Str: kernel32.lib
 
@@ -372,7 +372,7 @@
 Example:::
 
   - Callback: PragmaDetectMismatch
-Loc: "D:/Clang/llvm/tools/clang/tools/extra/test/pp-trace/pp-trace-pragma.cpp:3:1"
+Loc: "D:/Clang/llvm/clang-tools-extra/test/pp-trace/pp-trace-pragma.cpp:3:1"
 Name: 

[PATCH] D29692: [clang-tidy] add check modernize-use-const-instead-of-define

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin updated this revision to Diff 88447.
AlexanderLanin marked an inline comment as done.
AlexanderLanin added a comment.

Applied review comments and added test cases regarding parenthesis, floats, 
doubles, wide chars etc


https://reviews.llvm.org/D29692

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp
  clang-tidy/modernize/UseConstInsteadOfDefineCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst
  test/clang-tidy/modernize-use-const-instead-of-define.cpp

Index: test/clang-tidy/modernize-use-const-instead-of-define.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-const-instead-of-define.cpp
@@ -0,0 +1,67 @@
+// RUN: %check_clang_tidy %s modernize-use-const-instead-of-define %t
+
+// Although there might be cases where the - sign is actually used those
+// should be rare enough. e.g. int a = 5 BAD1;
+#define BAD1  -1
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD2  2
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD3(A)   0
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD4(X)   (7)
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD5(X)   +4
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD6 'x'
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD7 0xff
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD8 L'c'
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD9 U'c'
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD101U
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD111.5
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD121.4f
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD131.3L
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD14((-3))
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD15-(-3)
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+
+#define GOOD1 -
+#define GOOD2 (1+2)
+#define GOOD3(A)  #A
+#define GOOD4(A,B)A+B
+#define GOOD5(T)  ((T*)0)
+#define GOOD6(type)   (type(123))
+#define GOOD7(car, ...)   car
+#define GOOD8 a[5]
+#define GOOD9(x)  ;x;
+#define GOOD10;-2;
+#define GOOD11=2
+#define GOOD12+'a'
+#define GOOD13-'z'
+#define GOOD14!3
+#define GOOD15~-1
+#define GOOD16"str"
+#define GOOD17L"str"
+#define GOOD18U"str"
+#define GOOD19()
+#define GOOD20++
+#define GOOD21++i
+#define GOOD22k--
+#define GOOD23m
+#define GOOD245-
+#define GOOD25((3)
+#define GOOD26(3))
+#define GOOD27)2(
+
+#define NOT_DETECTED_YET_1(x)  ((unsigned char)(0xff))
+#define NOT_DETECTED_YET_2 (int)7
+#define NOT_DETECTED_YET_3 int(-5)
+#define NOT_DETECTED_YET_4 (int)(0)
Index: docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst
@@ -0,0 +1,57 @@
+.. title:: clang-tidy - modernize-use-const-instead-of-define
+
+modernize-use-const-instead-of-define
+=
+
+C++ ``const`` variables should be preferred over ``#define`` directives as
+``#define`` does not obey type checking and scope rules.
+
+Example
+---
+
+.. code-block:: c++
+
+  // calc.h
+  namespace RealCalculation {
+#define PI 3.14159
+  }
+
+  // quick.h
+  namespace QuickCalculation {
+#define PI 1
+  }
+
+  // calc.cpp
+  #include "calc.h"
+  #include "quick.h"
+
+  double calc(const double r) {
+return 2*PI*r*r;
+  }
+
+Strings
+---
+
+Strings are excluded from this rule as they might be used for string
+concatenations. Example:
+
+.. code-block:: c++
+
+  #define A "A"
+  #define B "B"
+  char AB[3] = A B;
+
+Code guidelines
+---
+
+While many C++ code guidelines like to prohibit macros completely with the
+exception of include guards, that's a 

[PATCH] D29692: [clang-tidy] add check modernize-use-const-instead-of-define

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin marked 5 inline comments as done.
AlexanderLanin added inline comments.



Comment at: clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp:86
+else if (isAnyParenthesis(Tok) || isAnyNumericLiteral(Tok) ||
+ (!hasPrefix && isAnyCharLiteral(Tok))) {
+  // prefix shall not be accepted anymore after any number

aaron.ballman wrote:
> What about string literals? e.g., `#define NAME "foobar"`
Excluded (for now) due to string concatenation done by preprocessor when you 
write NAME1 NAME2 in your code. I've added a section into 
modernize-use-const-instead-of-define.rst to explain this.


https://reviews.llvm.org/D29692



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

[PATCH] D29692: [clang-tidy] add check modernize-use-const-instead-of-define

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

Thanks for the feedback. As I'm new to this I cannot say whether checking only 
the file in question fits better with clang-tidy’s policy or not.
Also, I’m not sure about ODR. Of course, it’s a possibility, but isn’t the 
programmer responsible for that? This will be more of an issue as soon as this 
check provides a Fix-It solution.

As for the other part, I've checked some guidelines (without any order or 
selection process)
MISRA C++: Don’t use `#define`, use `const` variables; Also don’t do math on 
enums
CppCoreGuidelines: Don’t use `#define`, use `constexpr` variables 
SEI CERT C++: No mention of `#define` as far as I can tell
JSF AV C++: Don’t use `#define`, use `const` variables
HIC++: Don’t use `#define`, use `const` objects (reference to JSF AV C++ and 
MISRA C++)

So basically they're all the same. The only question is `const` vs `constexpr`


https://reviews.llvm.org/D29692



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

[PATCH] D29692: [clang-tidy] add check modernize-use-const-instead-of-define

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin updated this revision to Diff 88103.
AlexanderLanin marked 2 inline comments as done.
AlexanderLanin added a comment.

Fixes as reported in review


https://reviews.llvm.org/D29692

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp
  clang-tidy/modernize/UseConstInsteadOfDefineCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst
  test/clang-tidy/modernize-use-const-instead-of-define.cpp

Index: test/clang-tidy/modernize-use-const-instead-of-define.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-const-instead-of-define.cpp
@@ -0,0 +1,40 @@
+// RUN: %check_clang_tidy %s modernize-use-const-instead-of-define %t
+
+// Although there might be cases where the - sign is actually used those
+// should be rare enough. e.g. int a = 5 BAD1;
+#define BAD1  -1
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD2  2
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD3(A)   0
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD4(X)   (7)
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD5(X)   +4
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD6 'x'
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD7 0xff
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+
+#define GOOD1 -
+#define GOOD2 (1+2)
+#define GOOD3(A)  #A
+#define GOOD4(A,B)A+B
+#define GOOD5(T)  ((T*)0)
+#define GOOD6(type)   (type(123))
+#define GOOD7(car, ...)   car
+#define GOOD8 a[5]
+#define GOOD9(x)  ;x;
+#define GOOD10;-2;
+#define GOOD11=2
+#define GOOD12+'a'
+#define GOOD13-'z'
+#define GOOD14!3
+#define GOOD15~-1
+#define GOOD16"str"
+
+#define NOT_DETECTED_YET_1(x)  ((unsigned char)(0xff))
+#define NOT_DETECTED_YET_2 (int)7
+#define NOT_DETECTED_YET_3 int(-5)
+#define NOT_DETECTED_YET_4 (int)(0)
Index: docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst
@@ -0,0 +1,41 @@
+.. title:: clang-tidy - modernize-use-const-instead-of-define
+
+modernize-use-const-instead-of-define
+=
+
+C++ const variables should be preferred over ``#define`` directives as
+``#define`` does not obey type checking and scope rules.
+
+Example
+---
+
+.. code-block:: c++
+
+  `// calc.h
+  namespace RealCalculation {
+#define PI 3.14159
+  }
+
+  // quick.h
+  namespace QuickCalculation {
+#define PI 1
+  }
+
+  // calc.cpp
+  #include "calc.h"
+  #include "quick.h"
+
+  double calc(const double r) {
+return 2*PI*r*r;
+  }
+
+Code guidelines
+---
+
+While many C++ code guidelines like to prohibit macros completely with the
+exception of include guards, that's a rather strict limitation.
+Disallowing simple numbers should not require any significant code change and
+may even offer fix-it suggestions in the future.
+
+See also:
+https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es31-dont-use-macros-for-constants-or-functions
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -112,6 +112,7 @@
modernize-shrink-to-fit
modernize-use-auto
modernize-use-bool-literals
+   modernize-use-const-instead-of-define
modernize-use-default-member-init
modernize-use-emplace
modernize-use-equals-default
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,12 @@
 Improvements to clang-tidy
 --
 
+
+- New `modernize-use-const-instead-of-define
+  `_ check
+
+  Finds uses of ``#define`` where a const value would be more appropriate.
+
 - New `safety-no-assembler
   `_ check
 
Index: clang-tidy/modernize/UseConstInsteadOfDefineCheck.h
===
--- /dev/null
+++ clang-tidy/modernize/UseConstInsteadOfDefineCheck.h
@@ -0,0 +1,40 @@
+//===--- 

[PATCH] D29692: [clang-tidy] add check modernize-use-const-instead-of-define

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin marked 9 inline comments as done.
AlexanderLanin added a comment.

Not sure about CppCoreGuidelines as several guidelines have the same rule and I 
only used CppCoreGuidelines as it's convenient to link a specific rule. But I 
can move it if you like?!




Comment at: clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp:41
+/// others like ~ are not so obvious and depend on usage
+bool isReasonableNumberPrefix(const Token ) {
+  return T.isOneOf(tok::plus, tok::minus);

Eugene.Zelenko wrote:
> In LLVM Functions should be static, not inside anonymous namespace. Same 
> below.
mhh copied from MacroParentheses check



Comment at: docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst:11
+
+voif defineSeven() {
+  #define X 7

malcolm.parsons wrote:
> s/voif/void/
> Why is this in a function anyway?
that's why I wrote strange example ;-)
Hope the new one is better ?!



Comment at: test/clang-tidy/modernize-use-const-instead-of-define.cpp:6
+#define BAD1  -1
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD2  2

malcolm.parsons wrote:
> Check the message?
I didn't check the exact message since I changed the wording several times and 
it's currently the same message for everything that's detected anyway. Shall I 
check the exact message anyway?


Repository:
  rL LLVM

https://reviews.llvm.org/D29692



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

[PATCH] D29692: [clang-tidy] add check modernize-use-const-instead-of-define

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin created this revision.
Herald added subscribers: JDevlieghere, mgorny.

Suggestion for a new check that will warn on #defines that should rather be 
constant values. Const variables should be preferred  as #define does not obey 
type checking and scope rules.

Please feel free to criticize as strict as you like, this is my very first 
patch to llvm/clang. Also I'm not sure about the check name itself...


https://reviews.llvm.org/D29692

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp
  clang-tidy/modernize/UseConstInsteadOfDefineCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst
  test/clang-tidy/modernize-use-const-instead-of-define.cpp

Index: test/clang-tidy/modernize-use-const-instead-of-define.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-use-const-instead-of-define.cpp
@@ -0,0 +1,42 @@
+// RUN: %check_clang_tidy %s modernize-use-const-instead-of-define %t
+
+// allthough there might be cases where the - sign is actually used those
+// should be rare enough. e.g. int a = 5 BAD1;
+#define BAD1  -1
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD2  2
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD3(A)   0
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD4(X)   (7)
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD5(X)   +4
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD6 'x'
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+#define BAD7 0xff
+// CHECK-MESSAGES: :[[@LINE-1]]:{{.*}} [modernize-use-const-instead-of-define]
+
+
+#define GOOD1 -
+#define GOOD2 (1+2)
+#define GOOD3(A)  #A
+#define GOOD4(A,B)A+B
+#define GOOD5(T)  ((T*)0)
+#define GOOD6(type)   (type(123))
+#define GOOD7(car, ...)   car
+#define GOOD8 a[5]
+#define GOOD9(x)  ;x;
+#define GOOD10;-2;
+#define GOOD11=2
+#define GOOD12+'a'
+#define GOOD13-'z'
+#define GOOD14!3
+#define GOOD15~-1
+#define GOOD16"str"
+
+
+#define NOT_DETECTED_YET_1(x)  ((unsigned char)(0xff))
+#define NOT_DETECTED_YET_2 (int)7
+#define NOT_DETECTED_YET_3 int(-5)
+#define NOT_DETECTED_YET_4 (int)(0)
Index: docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst
@@ -0,0 +1,25 @@
+.. title:: clang-tidy - modernize-use-const-instead-of-define
+
+modernize-use-const-instead-of-define
+=
+
+C++ const variables should be preferred over #define statements as #define does
+not obey type checking and scope rules.
+
+a rather strange example might be:
+
+voif defineSeven() {
+  #define X 7
+}
+int returnSeven() {
+  return X;
+}
+
+
+While many C++ code guidelines like to prohibit macros completely with the
+exception of include guards, that's a rather strict limitation.
+Disallowing simple numbers should not require any significant code change and
+may even offer fix-it suggestions in the future.
+
+See also:
+https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es31-dont-use-macros-for-constants-or-functions
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -112,6 +112,7 @@
modernize-shrink-to-fit
modernize-use-auto
modernize-use-bool-literals
+   modernize-use-const-instead-of-define
modernize-use-default-member-init
modernize-use-emplace
modernize-use-equals-default
Index: clang-tidy/modernize/UseConstInsteadOfDefineCheck.h
===
--- /dev/null
+++ clang-tidy/modernize/UseConstInsteadOfDefineCheck.h
@@ -0,0 +1,40 @@
+//===--- UseConstInsteadOfDefineCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_CONST_INSTEAD_OF_DEFINE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_CONST_INSTEAD_OF_DEFINE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {