[PATCH] D157246: [clang-tidy] Update tests to include C++23 and C++26

[Adrian Vogelsgesang via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGfda777849b00: [clang-tidy] Update tests to include C++23 and 
C++26 (authored by avogelsgesang).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157246

Files:
  clang-tools-extra/test/clang-tidy/check_clang_tidy.py
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values-before-cxx23.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/unconventional-assign-operator.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy -std=c++20 %s readability-container-contains %t
+// RUN: %check_clang_tidy -std=c++20-or-later %s readability-container-contains %t
 
 // Some *very* simplified versions of `map` etc.
 namespace std {
Index: clang-tools-extra/test/clang-tidy/checkers/misc/unconventional-assign-operator.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc/unconventional-assign-operator.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc/unconventional-assign-operator.cpp
@@ -45,7 +45,7 @@
   BadArgument& operator=(BadArgument&);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should take 'BadArgument const&', 'BadArgument&&' or 'BadArgument'
   BadArgument& operator=(const BadArgument&&);
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should take 'BadAr
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should take 'BadArgument const&', 'BadArgument&&' or 'BadArgument'
 };
 
 struct BadModifier {
@@ -76,7 +76,7 @@
 public:
   BadReturnStatement& operator=(BadReturnStatement&& rhs) {
 n = std::move(rhs.n);
-return rhs;
+return *
 // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: operator=() should always return '*this'
   }
 
Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
@@ -1,9 +1,9 @@
 // RUN: %check_clang_tidy %s misc-const-correctness %t -- \
 // RUN:   -config="{CheckOptions: {\
-// RUN:   misc-const-correctness.TransformValues: true, \
-// RUN:   misc-const-correctness.WarnPointersAsValues: false, \
-// RUN:   misc-const-correctness.TransformPointersAsValues: false} \
-// RUN:   }" -- -fno-delayed-template-parsing
+// RUN: misc-const-correctness.TransformValues: true, \
+// RUN: misc-const-correctness.WarnPointersAsValues: false, \
+// RUN: misc-const-correctness.TransformPointersAsValues: false \
+// RUN:   }}" -- -fno-delayed-template-parsing
 
 // --- Provide test samples for primitive builtins -
 // - every 'p_*' variable is a 'potential_const_*' variable
@@ -181,14 +181,7 @@
   return _local1;
 }
 
-double _const_ref_return() {
-  double p_local0 = 0.0;
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared 'const'
-  // CHECK-FIXES: double const p_local0
-  double np_local0 = 42.42;
-  return np_local0;
-}
-
+// Also see const-correctness-values.cpp-before-cxx23.cpp for `non_const_ref_return` and `return_non_const_pointer_ref`
 const double _ref_return() {
   double p_local0 = 0.0;
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared 'const'
@@ -199,11 +192,6 @@
   return p_local1;
 }
 
-double *_non_const_pointer_ref() {
-  double *np_local0 = nullptr;
-  return np_local0;
-}
-
 void overloaded_arguments(const int );
 void overloaded_arguments(int );
 void overloaded_arguments(const int *in);
Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values-before-cxx23.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values-before-cxx23.cpp
@@ -0,0 +1,20 @@
+// RUN: %check_clang_tidy -std=c++11,c++14,c++17,c++20 %s misc-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: {\
+// RUN: misc-const-correctness.TransformValues: true, \
+// RUN: misc-const-correctness.WarnPointersAsValues: false, \
+// RUN: misc-const-correctness.TransformPointersAsValues: false \
+// RUN:   }}" -- -fno-delayed-template-parsing
+
+
+double _const_ref_return() {
+  double p_local0 = 

[PATCH] D157246: [clang-tidy] Update tests to include C++23 and C++26

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 547931.
avogelsgesang added a comment.

Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157246

Files:
  clang-tools-extra/test/clang-tidy/check_clang_tidy.py
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values-before-cxx23.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/unconventional-assign-operator.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy -std=c++20 %s readability-container-contains %t
+// RUN: %check_clang_tidy -std=c++20-or-later %s readability-container-contains %t
 
 // Some *very* simplified versions of `map` etc.
 namespace std {
Index: clang-tools-extra/test/clang-tidy/checkers/misc/unconventional-assign-operator.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc/unconventional-assign-operator.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc/unconventional-assign-operator.cpp
@@ -45,7 +45,7 @@
   BadArgument& operator=(BadArgument&);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should take 'BadArgument const&', 'BadArgument&&' or 'BadArgument'
   BadArgument& operator=(const BadArgument&&);
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should take 'BadAr
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should take 'BadArgument const&', 'BadArgument&&' or 'BadArgument'
 };
 
 struct BadModifier {
@@ -76,7 +76,7 @@
 public:
   BadReturnStatement& operator=(BadReturnStatement&& rhs) {
 n = std::move(rhs.n);
-return rhs;
+return *
 // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: operator=() should always return '*this'
   }
 
Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
@@ -1,9 +1,9 @@
 // RUN: %check_clang_tidy %s misc-const-correctness %t -- \
 // RUN:   -config="{CheckOptions: {\
-// RUN:   misc-const-correctness.TransformValues: true, \
-// RUN:   misc-const-correctness.WarnPointersAsValues: false, \
-// RUN:   misc-const-correctness.TransformPointersAsValues: false} \
-// RUN:   }" -- -fno-delayed-template-parsing
+// RUN: misc-const-correctness.TransformValues: true, \
+// RUN: misc-const-correctness.WarnPointersAsValues: false, \
+// RUN: misc-const-correctness.TransformPointersAsValues: false \
+// RUN:   }}" -- -fno-delayed-template-parsing
 
 // --- Provide test samples for primitive builtins -
 // - every 'p_*' variable is a 'potential_const_*' variable
@@ -181,14 +181,7 @@
   return _local1;
 }
 
-double _const_ref_return() {
-  double p_local0 = 0.0;
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared 'const'
-  // CHECK-FIXES: double const p_local0
-  double np_local0 = 42.42;
-  return np_local0;
-}
-
+// Also see const-correctness-values.cpp-before-cxx23.cpp for `non_const_ref_return` and `return_non_const_pointer_ref`
 const double _ref_return() {
   double p_local0 = 0.0;
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared 'const'
@@ -199,11 +192,6 @@
   return p_local1;
 }
 
-double *_non_const_pointer_ref() {
-  double *np_local0 = nullptr;
-  return np_local0;
-}
-
 void overloaded_arguments(const int );
 void overloaded_arguments(int );
 void overloaded_arguments(const int *in);
Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values-before-cxx23.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values-before-cxx23.cpp
@@ -0,0 +1,20 @@
+// RUN: %check_clang_tidy -std=c++11,c++14,c++17,c++20 %s misc-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: {\
+// RUN: misc-const-correctness.TransformValues: true, \
+// RUN: misc-const-correctness.WarnPointersAsValues: false, \
+// RUN: misc-const-correctness.TransformPointersAsValues: false \
+// RUN:   }}" -- -fno-delayed-template-parsing
+
+
+double _const_ref_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared 'const'
+  // CHECK-FIXES: double const 

[PATCH] D157246: [clang-tidy] Update tests to include C++23 and C++26

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang created this revision.
Herald added subscribers: PiotrZSL, carlosgalvezp, xazax.hun.
Herald added a project: All.
avogelsgesang requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

This commit changes the `c++xx-or-later` definitions to also include
C++23 and the upcoming C++26.
`readability/container-contains.cpp` to also test newer C++ versions.

Also, this commit adjusts a couple of test cases slightly:

- `container-contains.cpp` now also tests newer C++ versions. Restricting it to 
C++20 was an oversight of mine when originally writing this check.
- `unconventional-assign-operator.cpp`: The `return rhs` raised a "non-const 
lvalue reference to type 'BadReturnStatement' cannot bind to a temporary" error 
in C++23. The issue is circumenvented by writing `return *`.
- `const-correctness-values.cpp` was also running into the same error in C++23. 
The troublesome test cases were moved to a separate file.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157246

Files:
  clang-tools-extra/test/clang-tidy/check_clang_tidy.py
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values-before-cxx23.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/unconventional-assign-operator.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy -std=c++20 %s readability-container-contains %t
+// RUN: %check_clang_tidy -std=c++20-or-later %s readability-container-contains %t
 
 // Some *very* simplified versions of `map` etc.
 namespace std {
Index: clang-tools-extra/test/clang-tidy/checkers/misc/unconventional-assign-operator.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc/unconventional-assign-operator.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc/unconventional-assign-operator.cpp
@@ -45,7 +45,7 @@
   BadArgument& operator=(BadArgument&);
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should take 'BadArgument const&', 'BadArgument&&' or 'BadArgument'
   BadArgument& operator=(const BadArgument&&);
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should take 'BadAr
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: operator=() should take 'BadArgument const&', 'BadArgument&&' or 'BadArgument'
 };
 
 struct BadModifier {
@@ -76,7 +76,7 @@
 public:
   BadReturnStatement& operator=(BadReturnStatement&& rhs) {
 n = std::move(rhs.n);
-return rhs;
+return *
 // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: operator=() should always return '*this'
   }
 
Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
@@ -181,14 +181,7 @@
   return _local1;
 }
 
-double _const_ref_return() {
-  double p_local0 = 0.0;
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared 'const'
-  // CHECK-FIXES: double const p_local0
-  double np_local0 = 42.42;
-  return np_local0;
-}
-
+// Also see const-correctness-values.cpp-before-cxx23.cpp for `non_const_ref_return` and `return_non_const_pointer_ref`
 const double _ref_return() {
   double p_local0 = 0.0;
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared 'const'
@@ -199,11 +192,6 @@
   return p_local1;
 }
 
-double *_non_const_pointer_ref() {
-  double *np_local0 = nullptr;
-  return np_local0;
-}
-
 void overloaded_arguments(const int );
 void overloaded_arguments(int );
 void overloaded_arguments(const int *in);
Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values-before-cxx23.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values-before-cxx23.cpp
@@ -0,0 +1,20 @@
+// RUN: %check_clang_tidy -std=c++11,c++14,c++17,c++20 %s misc-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'misc-const-correctness.TransformValues', value: true}, \
+// RUN:   {key: 'misc-const-correctness.WarnPointersAsValues', value: false}, \
+// RUN:   {key: 'misc-const-correctness.TransformPointersAsValues', value: false}, \
+// RUN:   ]}" -- -fno-delayed-template-parsing
+
+
+double _const_ref_return() {
+  double p_local0 = 0.0;

[PATCH] D131938: [C++20] [Coroutines] Disable to take the address of labels in coroutines

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

Personally, I am fine with this change as in review right now.

@ychen are you still planning to look further into this issue as part of your 
alternative patch https://reviews.llvm.org/D132084 ? Otherwise, I would propose 
that we merge @ChuanqiXu 's change for the time being. Later on, we can still 
re-enable this functionality, as soon as we figured out the LLVM coroutine 
semantics of this


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

https://reviews.llvm.org/D131938

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

[PATCH] D134269: [docs] Document the one-phase style compilation to c++ standard modules

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.



Comment at: clang/docs/StandardCPlusPlusModules.rst:916
+We call this manner as two-phase compilation
+(sources to BMIs and BMIs to object files) to disambiguate the tranditional
+compilation process (sources to object files directly),





Comment at: clang/docs/StandardCPlusPlusModules.rst:936
+
+In this example, `C.cpp` dependent on Module B and `B.cppm` dependent on 
Module A.
+So the compilation process in one-phase style will be:




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134269

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

[PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG91389000abe8: [LLDB] Add data formatter for 
std::coroutine_handle (authored by avogelsgesang).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132415

Files:
  clang/docs/tools/clang-formatted-files.txt
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test/lldbutil.py
  lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/Makefile
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -0,0 +1,40 @@
+#include 
+
+// `int_generator` is a stripped down, minimal coroutine generator
+// type.
+struct int_generator {
+  struct promise_type {
+int current_value = -1;
+
+auto get_return_object() {
+  return std::coroutine_handle::from_promise(*this);
+}
+auto initial_suspend() { return std::suspend_always(); }
+auto final_suspend() noexcept { return std::suspend_always(); }
+auto return_void() { return std::suspend_always(); }
+void unhandled_exception() { __builtin_unreachable(); }
+auto yield_value(int v) {
+  current_value = v;
+  return std::suspend_always();
+}
+  };
+
+  std::coroutine_handle hdl;
+
+  int_generator(std::coroutine_handle h) : hdl(h) {}
+  ~int_generator() { hdl.destroy(); }
+};
+
+int_generator my_generator_func() { co_yield 42; }
+
+// This is an empty function which we call just so the debugger has
+// a place to reliably set a breakpoint on.
+void empty_function_so_we_can_set_a_breakpoint() {}
+
+int main() {
+  int_generator gen = my_generator_func();
+  std::coroutine_handle<> type_erased_hdl = gen.hdl;
+  gen.hdl.resume();// Break at initial_suspend
+  gen.hdl.resume();// Break after co_yield
+  empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
+}
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -0,0 +1,79 @@
+"""
+Test lldb data formatter subsystem.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+USE_LIBSTDCPP = "USE_LIBSTDCPP"
+USE_LIBCPP = "USE_LIBCPP"
+
+class TestCoroutineHandle(TestBase):
+def do_test(self, stdlib_type):
+"""Test std::coroutine_handle is displayed correctly."""
+self.build(dictionary={stdlib_type: "1"})
+
+test_generator_func_ptr_re = re.compile(
+r"^\(a.out`my_generator_func\(\) at main.cpp:[0-9]*\)$")
+
+# Run until the initial suspension point
+lldbutil.run_to_source_breakpoint(self, '// Break at initial_suspend',
+lldb.SBFileSpec("main.cpp", False))
+# Check that we show the correct function pointers and the `promise`. 
+self.expect_expr("gen.hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "-1"),
+])
+])
+# For type-erased `coroutine_handle<>` we are missing the `promise`
+# but still show `resume` and `destroy`.
+self.expect_expr("type_erased_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+])
+
+# Run until after the `co_yield`
+process = self.process()
+lldbutil.continue_to_source_breakpoint(self, process,
+'// Break after co_yield', lldb.SBFileSpec("main.cpp", False))
+# We correctly show the updated 

[PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp:39
+  CompilerType coro_func_type = ast_ctx.CreateFunctionType(
+  /*result_type*/ void_type, /*args*/ _type, /*num_args*/ 1,
+  /*is_variadic*/ false, /*qualifiers*/ 0);

shafik wrote:
> `/*result_type=*/` see 
> https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html
> 
> applies to the rest of them as well.
done


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

https://reviews.llvm.org/D132415

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

[PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 455111.
avogelsgesang marked an inline comment as done.

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

https://reviews.llvm.org/D132415

Files:
  clang/docs/tools/clang-formatted-files.txt
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test/lldbutil.py
  lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/Makefile
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -0,0 +1,40 @@
+#include 
+
+// `int_generator` is a stripped down, minimal coroutine generator
+// type.
+struct int_generator {
+  struct promise_type {
+int current_value = -1;
+
+auto get_return_object() {
+  return std::coroutine_handle::from_promise(*this);
+}
+auto initial_suspend() { return std::suspend_always(); }
+auto final_suspend() noexcept { return std::suspend_always(); }
+auto return_void() { return std::suspend_always(); }
+void unhandled_exception() { __builtin_unreachable(); }
+auto yield_value(int v) {
+  current_value = v;
+  return std::suspend_always();
+}
+  };
+
+  std::coroutine_handle hdl;
+
+  int_generator(std::coroutine_handle h) : hdl(h) {}
+  ~int_generator() { hdl.destroy(); }
+};
+
+int_generator my_generator_func() { co_yield 42; }
+
+// This is an empty function which we call just so the debugger has
+// a place to reliably set a breakpoint on.
+void empty_function_so_we_can_set_a_breakpoint() {}
+
+int main() {
+  int_generator gen = my_generator_func();
+  std::coroutine_handle<> type_erased_hdl = gen.hdl;
+  gen.hdl.resume();// Break at initial_suspend
+  gen.hdl.resume();// Break after co_yield
+  empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
+}
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -0,0 +1,79 @@
+"""
+Test lldb data formatter subsystem.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+USE_LIBSTDCPP = "USE_LIBSTDCPP"
+USE_LIBCPP = "USE_LIBCPP"
+
+class TestCoroutineHandle(TestBase):
+def do_test(self, stdlib_type):
+"""Test std::coroutine_handle is displayed correctly."""
+self.build(dictionary={stdlib_type: "1"})
+
+test_generator_func_ptr_re = re.compile(
+r"^\(a.out`my_generator_func\(\) at main.cpp:[0-9]*\)$")
+
+# Run until the initial suspension point
+lldbutil.run_to_source_breakpoint(self, '// Break at initial_suspend',
+lldb.SBFileSpec("main.cpp", False))
+# Check that we show the correct function pointers and the `promise`. 
+self.expect_expr("gen.hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "-1"),
+])
+])
+# For type-erased `coroutine_handle<>` we are missing the `promise`
+# but still show `resume` and `destroy`.
+self.expect_expr("type_erased_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+])
+
+# Run until after the `co_yield`
+process = self.process()
+lldbutil.continue_to_source_breakpoint(self, process,
+'// Break after co_yield', lldb.SBFileSpec("main.cpp", False))
+# We correctly show the updated value inside `prommise.current_value`.
+self.expect_expr("gen.hdl",
+result_children=[
+

[PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.



Comment at: 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py:15
+
+class TestCoroutineHandle(TestBase):
+def do_test(self, stdlib_type):

jingham wrote:
> aprantl wrote:
> > Nice test!
> Would you mind changing moving this into lldbutil.py?  This seems generally 
> useful.  If you return the thread & breakpoint it would be even more 
> generally useful?
> moving this into lldbutil.py

done


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

https://reviews.llvm.org/D132415

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

[PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 455022.
avogelsgesang marked 4 inline comments as done.

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

https://reviews.llvm.org/D132415

Files:
  clang/docs/tools/clang-formatted-files.txt
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test/lldbutil.py
  lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/Makefile
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -0,0 +1,40 @@
+#include 
+
+// `int_generator` is a stripped down, minimal coroutine generator
+// type.
+struct int_generator {
+  struct promise_type {
+int current_value = -1;
+
+auto get_return_object() {
+  return std::coroutine_handle::from_promise(*this);
+}
+auto initial_suspend() { return std::suspend_always(); }
+auto final_suspend() noexcept { return std::suspend_always(); }
+auto return_void() { return std::suspend_always(); }
+void unhandled_exception() { __builtin_unreachable(); }
+auto yield_value(int v) {
+  current_value = v;
+  return std::suspend_always();
+}
+  };
+
+  std::coroutine_handle hdl;
+
+  int_generator(std::coroutine_handle h) : hdl(h) {}
+  ~int_generator() { hdl.destroy(); }
+};
+
+int_generator my_generator_func() { co_yield 42; }
+
+// This is an empty function which we call just so the debugger has
+// a place to reliably set a breakpoint on.
+void empty_function_so_we_can_set_a_breakpoint() {}
+
+int main() {
+  int_generator gen = my_generator_func();
+  std::coroutine_handle<> type_erased_hdl = gen.hdl;
+  gen.hdl.resume();// Break at initial_suspend
+  gen.hdl.resume();// Break after co_yield
+  empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
+}
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -0,0 +1,79 @@
+"""
+Test lldb data formatter subsystem.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+USE_LIBSTDCPP = "USE_LIBSTDCPP"
+USE_LIBCPP = "USE_LIBCPP"
+
+class TestCoroutineHandle(TestBase):
+def do_test(self, stdlib_type):
+"""Test std::coroutine_handle is displayed correctly."""
+self.build(dictionary={stdlib_type: "1"})
+
+test_generator_func_ptr_re = re.compile(
+r"^\(a.out`my_generator_func\(\) at main.cpp:[0-9]*\)$")
+
+# Run until the initial suspension point
+lldbutil.run_to_source_breakpoint(self, '// Break at initial_suspend',
+lldb.SBFileSpec("main.cpp", False))
+# Check that we show the correct function pointers and the `promise`. 
+self.expect_expr("gen.hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "-1"),
+])
+])
+# For type-erased `coroutine_handle<>` we are missing the `promise`
+# but still show `resume` and `destroy`.
+self.expect_expr("type_erased_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+])
+
+# Run until after the `co_yield`
+process = self.process()
+lldbutil.continue_to_source_breakpoint(self, process,
+'// Break after co_yield', lldb.SBFileSpec("main.cpp", False))
+# We correctly show the updated value inside `prommise.current_value`.
+self.expect_expr("gen.hdl",
+result_children=[
+

[PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via cfe-commits]

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



Comment at: 
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py:44
+
+lldbutil.run_to_source_breakpoint(self, '// Break after co_yield',
+lldb.SBFileSpec("main.cpp", False))

aprantl wrote:
> Does this launch a new process? It might be faster to just set an additional 
> breakpoint and run `process.Continue()`.
somehow, I assumed `run_to_source_breakpoint` would actually continue the 
running process instead of starting a new process. But looking into 
`run_to_source_breakpoint`, you are right, this launches a new process...

I changed this to instead continue execution of the already launched process


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132415

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

[PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 454911.
avogelsgesang marked 6 inline comments as done.
avogelsgesang added a comment.

addressed comments from @aprantl


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132415

Files:
  clang/docs/tools/clang-formatted-files.txt
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/Makefile
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -0,0 +1,40 @@
+#include 
+
+// `int_generator` is a stripped down, minimal coroutine generator
+// type.
+struct int_generator {
+  struct promise_type {
+int current_value = -1;
+
+auto get_return_object() {
+  return std::coroutine_handle::from_promise(*this);
+}
+auto initial_suspend() { return std::suspend_always(); }
+auto final_suspend() noexcept { return std::suspend_always(); }
+auto return_void() { return std::suspend_always(); }
+void unhandled_exception() { __builtin_unreachable(); }
+auto yield_value(int v) {
+  current_value = v;
+  return std::suspend_always();
+}
+  };
+
+  std::coroutine_handle hdl;
+
+  int_generator(std::coroutine_handle h) : hdl(h) {}
+  ~int_generator() { hdl.destroy(); }
+};
+
+int_generator my_generator_func() { co_yield 42; }
+
+// This is an empty function which we call just so the debugger has
+// a place to reliably set a breakpoint on.
+void empty_function_so_we_can_set_a_breakpoint() {}
+
+int main() {
+  int_generator gen = my_generator_func();
+  std::coroutine_handle<> type_erased_hdl = gen.hdl;
+  gen.hdl.resume(); // Break at initial_suspend
+  gen.hdl.resume(); // Break after co_yield
+  empty_function_so_we_can_set_a_breakpoint(); // Break at final_suspend
+}
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -0,0 +1,87 @@
+"""
+Test lldb data formatter subsystem.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+USE_LIBSTDCPP = "USE_LIBSTDCPP"
+USE_LIBCPP = "USE_LIBCPP"
+
+class TestCoroutineHandle(TestBase):
+def continue_to_source_breakpoint(self, bkpt_pattern, source_spec):
+target = self.target()
+breakpoint = target.BreakpointCreateBySourceRegex(
+bkpt_pattern, source_spec, None)
+self.assertTrue(breakpoint, VALID_BREAKPOINT)
+threads = lldbutil.continue_to_breakpoint(self.process(), breakpoint)
+self.assertEqual(len(threads), 1)
+
+
+def do_test(self, stdlib_type):
+"""Test std::coroutine_handle is displayed correctly."""
+self.build(dictionary={stdlib_type: "1"})
+
+test_generator_func_ptr_re = re.compile(
+r"^\(a.out`my_generator_func\(\) at main.cpp:[0-9]*\)$")
+
+# Run until the initial suspension point
+lldbutil.run_to_source_breakpoint(self, '// Break at initial_suspend',
+lldb.SBFileSpec("main.cpp", False))
+# Check that we show the correct function pointers and the `promise`. 
+self.expect_expr("gen.hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "-1"),
+])
+])
+# For type-erased `coroutine_handle<>` we are missing the `promise`
+# but still show `resume` and `destroy`.
+self.expect_expr("type_erased_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+   

[PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 454830.
avogelsgesang added a comment.

clang-format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132415

Files:
  clang/docs/tools/clang-formatted-files.txt
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/Makefile
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -0,0 +1,36 @@
+#include 
+
+// `int_generator` is a stripped down, minimal coroutine generator
+// type.
+struct int_generator {
+  struct promise_type {
+int current_value = -1;
+
+auto get_return_object() {
+  return std::coroutine_handle::from_promise(*this);
+}
+auto initial_suspend() { return std::suspend_always(); }
+auto final_suspend() noexcept { return std::suspend_always(); }
+auto return_void() { return std::suspend_always(); }
+void unhandled_exception() { __builtin_unreachable(); }
+auto yield_value(int v) {
+  current_value = v;
+  return std::suspend_always();
+}
+  };
+
+  std::coroutine_handle hdl;
+
+  int_generator(std::coroutine_handle h) : hdl(h) {}
+  ~int_generator() { hdl.destroy(); }
+};
+
+int_generator my_generator_func() { co_yield 42; }
+
+int main() {
+  int_generator gen = my_generator_func();
+  std::coroutine_handle<> type_erased_hdl = gen.hdl;
+  gen.hdl.resume(); // Break at initial_suspend
+  gen.hdl.resume(); // Break after co_yield
+  // Break at final_suspend
+}
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -0,0 +1,75 @@
+"""
+Test lldb data formatter subsystem.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+USE_LIBSTDCPP = "USE_LIBSTDCPP"
+USE_LIBCPP = "USE_LIBCPP"
+
+class TestCoroutineHandle(TestBase):
+def do_test(self, stdlib_type):
+"""Test std::coroutine_handle is displayed correctly."""
+self.build(dictionary={stdlib_type: "1"})
+
+test_generator_func_ptr_re = re.compile(
+r"^\(a.out`my_generator_func\(\) at main.cpp:[0-9]*\)$")
+
+lldbutil.run_to_source_breakpoint(self, '// Break at initial_suspend',
+lldb.SBFileSpec("main.cpp", False))
+# Check that we show the correct function pointers and the `promise`. 
+self.expect_expr("gen.hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "-1"),
+])
+])
+# For type-erased `coroutine_handle<>` we are missing the `promise`
+# but still show `resume` and `destroy`.
+self.expect_expr("type_erased_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+])
+
+lldbutil.run_to_source_breakpoint(self, '// Break after co_yield',
+lldb.SBFileSpec("main.cpp", False))
+# We correctly show the updated value inside `prommise.current_value`.
+self.expect_expr("gen.hdl",
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "42"),
+])
+])
+
+lldbutil.run_to_source_breakpoint(self, '// Break at final_suspend',
+

[PATCH] D132451: [docs] Add examples for printing asynchronous stack for coroutines

[Adrian Vogelsgesang via Phabricator via cfe-commits]

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

LGTM, thanks!

I guess I will update this documentation with LLDB examples, as soon as 
https://reviews.llvm.org/D132415 and a couple of follow-up improvements landed.
As soon as that happened, we will also have LLDB covered here. I see no problem 
with covering mostly gdb for the time being, given that lldb debugging is 
currently subject to change, anyway


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

https://reviews.llvm.org/D132451

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

[PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 454728.
avogelsgesang added a comment.

remove unrelated changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132415

Files:
  clang/docs/tools/clang-formatted-files.txt
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/Makefile
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -0,0 +1,36 @@
+#include 
+
+// `int_generator` is a stripped down, minimal coroutine generator
+// type.
+struct int_generator {
+  struct promise_type {
+int current_value = -1;
+
+auto get_return_object() {
+  return std::coroutine_handle::from_promise(*this);
+}
+auto initial_suspend() { return std::suspend_always(); }
+auto final_suspend() noexcept { return std::suspend_always(); }
+auto return_void() { return std::suspend_always(); }
+void unhandled_exception() { __builtin_unreachable(); }
+auto yield_value(int v) {
+  current_value = v;
+  return std::suspend_always();
+}
+  };
+
+  std::coroutine_handle hdl;
+
+  int_generator(std::coroutine_handle h) : hdl(h) {}
+  ~int_generator() { hdl.destroy(); }
+};
+
+int_generator my_generator_func() { co_yield 42; }
+
+int main() {
+  int_generator gen = my_generator_func();
+  std::coroutine_handle<> type_erased_hdl = gen.hdl;
+  gen.hdl.resume(); // Break at initial_suspend
+  gen.hdl.resume(); // Break after co_yield
+  // Break at final_suspend
+}
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -0,0 +1,75 @@
+"""
+Test lldb data formatter subsystem.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+USE_LIBSTDCPP = "USE_LIBSTDCPP"
+USE_LIBCPP = "USE_LIBCPP"
+
+class TestCoroutineHandle(TestBase):
+def do_test(self, stdlib_type):
+"""Test std::coroutine_handle is displayed correctly."""
+self.build(dictionary={stdlib_type: "1"})
+
+test_generator_func_ptr_re = re.compile(
+r"^\(a.out`my_generator_func\(\) at main.cpp:[0-9]*\)$")
+
+lldbutil.run_to_source_breakpoint(self, '// Break at initial_suspend',
+lldb.SBFileSpec("main.cpp", False))
+# Check that we show the correct function pointers and the `promise`. 
+self.expect_expr("gen.hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "-1"),
+])
+])
+# For type-erased `coroutine_handle<>` we are missing the `promise`
+# but still show `resume` and `destroy`.
+self.expect_expr("type_erased_hdl",
+result_summary=re.compile("^coro frame = 0x[0-9a-f]*$"),
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+])
+
+lldbutil.run_to_source_breakpoint(self, '// Break after co_yield',
+lldb.SBFileSpec("main.cpp", False))
+# We correctly show the updated value inside `prommise.current_value`.
+self.expect_expr("gen.hdl",
+result_children=[
+ValueCheck(name="resume", summary = test_generator_func_ptr_re),
+ValueCheck(name="destroy", summary = test_generator_func_ptr_re),
+ValueCheck(name="promise", children=[
+ValueCheck(name="current_value", value = "42"),
+])
+])
+
+lldbutil.run_to_source_breakpoint(self, '// Break at final_suspend',
+

[PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:1139
 
+  AddCXXSynthetic( // XXX
+  cpp_category_sp,

I need to remove this. This is a left-over from an earlier implementation sketch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132415

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

[PATCH] D132415: [LLDB] Add data formatter for std::coroutine_handle

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang created this revision.
avogelsgesang added reviewers: ChuanqiXu, aprantl, labath, mib, JDevlieghere.
Herald added a subscriber: mgorny.
Herald added a project: All.
avogelsgesang requested review of this revision.
Herald added projects: clang, LLDB.
Herald added subscribers: lldb-commits, cfe-commits.

This patch adds a formatter for `std::coroutine_handle`, both for libc++
and libstdc++. For the type-erased `coroutine_handle<>`, it shows the
`resume` and `destroy` function pointers. For a non-type-erased
`coroutine_handle` it also shows the `promise` value.

With this change, executing the `v t` command on the example from
https://clang.llvm.org/docs/DebuggingCoroutines.html now outputs

  (task) t = {
handle = coro frame = 0xb2a0 {
  resume = 0x5a10 (a.out`coro_task(int, int) at 
llvm-example.cpp:36)
  destroy = 0x6090 (a.out`coro_task(int, int) at 
llvm-example.cpp:36)
}
  }

instead of just

  (task) t = {
handle = {
  __handle_ = 0xb2a0
}
  }

Note, how the symbols for the `resume` and `destroy` function pointer
reveals which coroutine is stored inside the `std::coroutine_handle`.
A follow-up commit will use this fact to infer the coroutine's promise
type and the representation of its internal coroutine state based on
the `resume` and`destroy` pointers.

The same formatter is used for both libc++ and libstdc++. It would
also work for MSVC's standard library, however it is not registered
for MSVC, given that lldb does not provide pretty printers for other
MSVC types, either.

The formatter is in a new added  `Coroutines.{h,cpp}` file because there
does not seem to be an already existing place where we could share
formatters across libc++ and libstdc++. Also, I expect this code to grow
as we improve debugging experience for coroutines further.

**Testing**

- Added API test


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132415

Files:
  clang/docs/tools/clang-formatted-files.txt
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/source/Plugins/Language/CPlusPlus/CMakeLists.txt
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
  lldb/source/Plugins/Language/CPlusPlus/Coroutines.h
  lldb/source/Plugins/Language/CPlusPlus/LibCxx.h
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/Makefile
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
  
lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp

Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/main.cpp
@@ -0,0 +1,36 @@
+#include 
+
+// `int_generator` is a stripped down, minimal coroutine generator
+// type.
+struct int_generator {
+  struct promise_type {
+int current_value = -1;
+
+auto get_return_object() {
+  return std::coroutine_handle::from_promise(*this);
+}
+auto initial_suspend() { return std::suspend_always(); }
+auto final_suspend() noexcept { return std::suspend_always(); }
+auto return_void() { return std::suspend_always(); }
+void unhandled_exception() { __builtin_unreachable(); }
+auto yield_value(int v) {
+  current_value = v;
+  return std::suspend_always();
+}
+  };
+
+  std::coroutine_handle hdl;
+
+  int_generator(std::coroutine_handle h) : hdl(h) {}
+  ~int_generator() { hdl.destroy(); }
+};
+
+int_generator my_generator_func() { co_yield 42; }
+
+int main() {
+  int_generator gen = my_generator_func();
+  std::coroutine_handle<> type_erased_hdl = gen.hdl;
+  gen.hdl.resume(); // Break at initial_suspend
+  gen.hdl.resume(); // Break after co_yield
+  // Break at final_suspend
+}
Index: lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
===
--- /dev/null
+++ lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/coroutine_handle/TestCoroutineHandle.py
@@ -0,0 +1,75 @@
+"""
+Test lldb data formatter subsystem.
+"""
+
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+USE_LIBSTDCPP = "USE_LIBSTDCPP"
+USE_LIBCPP = "USE_LIBCPP"
+
+class TestCoroutineHandle(TestBase):
+def do_test(self, stdlib_type):
+"""Test std::coroutine_handle is displayed correctly."""
+self.build(dictionary={stdlib_type: "1"})
+
+test_generator_func_ptr_re = re.compile(
+r"^\(a.out`my_generator_func\(\) at main.cpp:[0-9]*\)$")
+
+lldbutil.run_to_source_breakpoint(self, '// Break at initial_suspend',
+ 

[PATCH] D131938: [C++20] [Coroutines] Disable to take the address of labels in coroutines

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

looks good to me on a high-level, but I don't know clang well enough to 
confidently approve this change




Comment at: clang/lib/Sema/SemaCoroutine.cpp:1107
+
+  // Corotuines will get splitted into pieces. The GNU address of label
+  // extension wouldn't be meaningful in coroutines.

Corotuines -> Coroutines



Comment at: clang/test/SemaCXX/addr-label-in-coroutines.cpp:5
+
+struct Allocator {};
+

unused


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

https://reviews.llvm.org/D131938

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

[PATCH] D131046: [NFC][clang-tools-extra]Replace find/insert with try_emplace

[Adrian Vogelsgesang via Phabricator via cfe-commits]

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

The change itself looks good.

Please look into the CI failures before landing this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131046

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

[PATCH] D131046: [clang-tools-extra]Replace find/insert with try_emplace

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.



Comment at: 
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:140
+// This replacement is a duplicate of one suggested by another TU.
+if (!InsertPos.second) {
   return;

Previously, we had an additional check `It->second != SourceTU` here. I don't 
understand what this check was doing, but this does not seem to be a pure 
refactoring.

If this is still an NFC change, please update the commit message to explain why 
this condition was unneeded


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131046

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

[PATCH] D130678: [clang-tools-extra] Refactor to reduce code duplication

[Adrian Vogelsgesang via Phabricator via cfe-commits]

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

LGTM

(note that I am rather inexperienced with LLVM, though, so take my approval 
with a grain of salt)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130678

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

[PATCH] D129138: [clang] [docs] Update the changes of C++20 Modules in clang15

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.



Comment at: clang/www/cxx_status.html:1183
 https://wg21.link/p1874r1;>P1874R1
-Partial
+Clang 15
   

should this be `class="unreleased"` instead of `class="full"`? At least this is 
what other places in this document use when mentioning clang 15


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

https://reviews.llvm.org/D129138

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

[PATCH] D116514: [clangd] Add code action to generate a constructor for a C++ class

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

> Not easy to do in this patch, we don't have a good way to associate fixes 
> with clang diagnostics yet.

Is this a clangd-specific implementation issue or a general short-coming in the 
LSP protocol?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116514

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

[PATCH] D116514: [clangd] Add code action to generate a constructor for a C++ class

[Adrian Vogelsgesang via Phabricator via cfe-commits]

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

LGTM (for whatever that's worth - still pretty unexperienced with the clang AST)

Thanks for picking this up again! Looking forward to have this available in 
clangd! :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116514

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

[PATCH] D122102: [clangd] Introduce "add subclass" tweak

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

@adamcz

> Ideally I'd like to see this code action show up as code completion option as 
> well.

I think in the long-term such a "Implement methods" code action should also 
apply as a quickfix for

  class Base {
 virtual void foo() = 0;
  };
  
  class Derived final : public Base {}; // as a quickfix for "Abstract class is 
marked 'final'"
  
  auto foo = make_unique(); // as a quickfix for "allocating an object 
of abstract class type 'Derived'"
  Derived d; // as a quickfix for "Variable type 'Derived' is an abstract class"

But that's probably for a future commit. Let's first see that we get the basic 
functionality into clangd :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122102

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

[PATCH] D122102: [clangd] Introduce "add subclass" tweak

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

Thank for your comments, @adamcz and @njames93!

I agree that a "Implement interface" tweak is probably more useful, and I can 
imagine to evolve this tweak in that direction.

The main reason, I first went for the "create a new class from scratch" 
approach, was because I was not sure how to merge the newly inserted methods 
with the potentially existing methods to get an intuitive order.

How do you think we should further proceed here?

- Do we see value in both an "Implement methods" and an "Add subclass" tweak?
- Are you, @njames93, planning to follow-up on your existing review?
- Would you like me to evolve this patch into an "Implement methods" patch, 
drawing inspiration from your existing review & the comments on it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122102

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

[PATCH] D122102: [clangd] Introduce "add subclass" tweak

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added reviewers: sammccall, nridge, njames93, hokein.
avogelsgesang added a comment.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122102

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

[PATCH] D122102: [clangd] Introduce "add subclass" tweak

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang created this revision.
Herald added subscribers: usaxena95, kadircet, arphaman, mgorny.
Herald added a project: All.
avogelsgesang requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

This commit adds a new "add subclass" tweak which facilitates quick
scaffolding of inheritance hierarchies. The tweak can be triggered
on any class with virtual methods. It then inserts a new subclass
which overrides all virtual methods with dummy implementations.

There are two variations of this tweak:

1. A variant which overrides all virtual functions
2. A variant which overrides only pure virtual functions

This tweak also supports deeper inheritance hierarchies, and collects
the methods to be overriden not only from the immediate base class but
from the complete inheritance tree.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122102

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/AST.h
  clang-tools-extra/clangd/refactor/InsertionPoint.cpp
  clang-tools-extra/clangd/refactor/tweaks/AddSubclass.cpp
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/tweaks/AddSubclassTests.cpp

Index: clang-tools-extra/clangd/unittests/tweaks/AddSubclassTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/tweaks/AddSubclassTests.cpp
@@ -0,0 +1,526 @@
+//===-- AddSubclassTests.cpp *- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Annotations.h"
+#include "TweakTesting.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TWEAK_TEST(AddSubclassAllVirtuals);
+
+TEST_F(AddSubclassAllVirtualsTest, Prepare) {
+  // Not available if there are no virtual function
+  EXPECT_UNAVAILABLE("^struct ^Base { void foo(); };");
+  // Available on virtual functions
+  EXPECT_AVAILABLE("^struct ^Base { virtual void foo(); };");
+  // Available on pure virtual functions
+  EXPECT_AVAILABLE("^struct ^Base { virtual void foo() = 0; };");
+  // Available for inherited virtual functions
+  EXPECT_AVAILABLE(R"cpp(
+struct Base { virtual void foo() = 0; };
+^struct ^Intermediate : public Base {};
+)cpp");
+  // Available for inherited virtual functions, even if already overriden
+  EXPECT_AVAILABLE(R"cpp(
+struct Base { virtual void foo() = 0; };
+^struct ^Intermediate : public Base { void foo() override; };
+)cpp");
+}
+
+TEST_F(AddSubclassAllVirtualsTest, ApplyInDifferenctScopes) {
+  struct {
+llvm::StringRef TestSource;
+llvm::StringRef ExpectedSource;
+  } Cases[]{
+  // Basic case, outside any namespace
+  {
+  R"cpp(
+struct ^Base { virtual int foo() = 0; };
+)cpp",
+  R"cpp(
+struct Base { virtual int foo() = 0; };
+
+struct BaseSub : public Base {
+  using Base::Base;
+  int foo() override { return Base::foo(); }
+};
+)cpp",
+  },
+  // Inserted between two classes
+  {
+  R"cpp(
+struct ^Base { virtual int foo() = 0; };
+struct OtherStruct {};
+)cpp",
+  R"cpp(
+struct Base { virtual int foo() = 0; };
+
+struct BaseSub : public Base {
+  using Base::Base;
+  int foo() override { return Base::foo(); }
+};
+struct OtherStruct {};
+)cpp",
+  },
+  // Inside a namespace
+  {
+  R"cpp(
+namespace NS {
+struct ^Base { virtual int foo() = 0; };
+})cpp",
+  R"cpp(
+namespace NS {
+struct Base { virtual int foo() = 0; };
+
+struct BaseSub : public Base {
+  using Base::Base;
+  int foo() override { return Base::foo(); }
+};
+})cpp",
+  },
+  // Inside an outer class
+  {
+  R"cpp(
+struct Outer {
+struct ^Base { virtual int foo() = 0; };
+};)cpp",
+  R"cpp(
+struct Outer {
+struct Base { virtual int foo() = 0; };
+
+struct BaseSub : public Base {
+  using Base::Base;
+  int foo() override { return Base::foo(); }
+};
+};)cpp",
+  },
+  // Chooses a fresh unused name
+  {
+  R"cpp(
+struct ^Base { virtual int foo() = 0; };
+struct BaseSub;
+struct BaseSub1;
+struct BaseSub2;
+struct BaseSub4;
+)cpp",
+  R"cpp(
+struct Base { virtual int foo() = 0; };
+
+struct BaseSub3 : public Base {
+  using Base::Base;
+  int foo() override { return Base::foo(); }
+};
+struct BaseSub;
+struct BaseSub1;
+struct BaseSub2;
+struct BaseSub4;
+)cpp",
+  },
+  // Does not accidentally collide with a comment on the last line
+  // of the file without a newline.
+  {
+  R"cpp(
+struct ^Base { virtual int foo() = 0; };
+// Some comment...)cpp",
+

[PATCH] D122101: [clangd] Simplify `insertDecl` and support insertion after the last declaration in a file

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang created this revision.
Herald added subscribers: usaxena95, kadircet, arphaman.
Herald added a project: All.
avogelsgesang requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

So far `insertDecl`, did not support insertion of new code after the
last top-level declaration in a file. This was, because a
`TranslationUnitDecl` does not have a valid `getEndLoc()`.

Also, the logic of `Anchor::Below` was somewhat surprising: I would have
expected `Below` to insert below the first match. Instead it insert
after the first contigous run of matches. I found this highly
unintuitive, and replaced it by an additional optional `First`/`Last`
flag which can be specified for an anchor.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122101

Files:
  clang-tools-extra/clangd/refactor/InsertionPoint.cpp
  clang-tools-extra/clangd/refactor/InsertionPoint.h
  clang-tools-extra/clangd/unittests/InsertionPointTests.cpp

Index: clang-tools-extra/clangd/unittests/InsertionPointTests.cpp
===
--- clang-tools-extra/clangd/unittests/InsertionPointTests.cpp
+++ clang-tools-extra/clangd/unittests/InsertionPointTests.cpp
@@ -29,7 +29,7 @@
 $b^// leading comment
 int b;
 $c^int c1; // trailing comment
-int c2;
+$c2^int c2;
 $a2^int a2;
   $end^};
   )cpp");
@@ -47,18 +47,24 @@
   auto  = cast(findDecl(AST, "ns"));
 
   // Test single anchors.
-  auto Point = [&](llvm::StringLiteral Prefix, Anchor::Dir Direction) {
-auto Loc = insertionPoint(NS, {Anchor{StartsWith(Prefix), Direction}});
+  auto Point = [&](llvm::StringLiteral Prefix, Anchor::Dir Direction,
+   Anchor::SearchDir SearchDirection = Anchor::First) {
+auto Loc = insertionPoint(
+NS, {Anchor{StartsWith(Prefix), Direction, SearchDirection}});
 return sourceLocToPosition(AST.getSourceManager(), Loc);
   };
   EXPECT_EQ(Point("a", Anchor::Above), Code.point("a"));
   EXPECT_EQ(Point("a", Anchor::Below), Code.point("b"));
   EXPECT_EQ(Point("b", Anchor::Above), Code.point("b"));
   EXPECT_EQ(Point("b", Anchor::Below), Code.point("c"));
-  EXPECT_EQ(Point("c", Anchor::Above), Code.point("c"));
-  EXPECT_EQ(Point("c", Anchor::Below), Code.point("a2"));
+  EXPECT_EQ(Point("c", Anchor::Above, Anchor::First), Code.point("c"));
+  EXPECT_EQ(Point("c", Anchor::Below, Anchor::First), Code.point("c2"));
+  EXPECT_EQ(Point("c", Anchor::Above, Anchor::Last), Code.point("c2"));
+  EXPECT_EQ(Point("c", Anchor::Below, Anchor::Last), Code.point("a2"));
+  EXPECT_EQ(Point("a2", Anchor::Above), Code.point("a2"));
+  EXPECT_EQ(Point("a2", Anchor::Below), Code.point("end"));
   EXPECT_EQ(Point("", Anchor::Above), Code.point("a"));
-  EXPECT_EQ(Point("", Anchor::Below), Code.point("end"));
+  EXPECT_EQ(Point("", Anchor::Below), Code.point("b"));
   EXPECT_EQ(Point("no_match", Anchor::Below), Position{});
 
   // Test anchor chaining.
@@ -83,6 +89,35 @@
 Code.point("end"));
 }
 
+TEST(InsertionPointTests, TopLevel) {
+  Annotations Code(R"cpp(
+  $a^int a;
+  $b^int b;
+  $end^)cpp");
+
+  auto HasName =
+  [&](llvm::StringLiteral S) -> std::function {
+return [S](const Decl *D) {
+  if (const auto *ND = llvm::dyn_cast(D))
+return ND->getNameAsString() == S;
+  return false;
+};
+  };
+
+  auto AST = TestTU::withCode(Code.code()).build();
+  auto  = *AST.getASTContext().getTranslationUnitDecl();
+
+  // Test single anchors.
+  auto Point = [&](llvm::StringLiteral Prefix, Anchor::Dir Direction) {
+auto Loc = insertionPoint(TUD, {Anchor{HasName(Prefix), Direction}});
+return sourceLocToPosition(AST.getSourceManager(), Loc);
+  };
+  EXPECT_EQ(Point("a", Anchor::Above), Code.point("a"));
+  EXPECT_EQ(Point("a", Anchor::Below), Code.point("b"));
+  EXPECT_EQ(Point("b", Anchor::Above), Code.point("b"));
+  EXPECT_EQ(Point("b", Anchor::Below), Code.point("end"));
+}
+
 // For CXX, we should check:
 // - special handling for access specifiers
 // - unwrapping of template decls
@@ -96,7 +131,7 @@
 $private^private:
   $field^int PrivField;
   $method^void privMethod();
-  template  void privTemplateMethod();
+  $template^template  void privTemplateMethod();
 $end^};
   )cpp");
 
@@ -114,11 +149,12 @@
   EXPECT_EQ(Point({IsMethod, Anchor::Above}, AS_public), Code.point("Method"));
   EXPECT_EQ(Point({IsMethod, Anchor::Below}, AS_public), Code.point("Field"));
   EXPECT_EQ(Point({Any, Anchor::Above}, AS_public), Code.point("Method"));
-  EXPECT_EQ(Point({Any, Anchor::Below}, AS_public), Code.point("private"));
+  EXPECT_EQ(Point({Any, Anchor::Below}, AS_public), Code.point("Method"));
   EXPECT_EQ(Point({IsMethod, Anchor::Above}, AS_private), Code.point("method"));
-  EXPECT_EQ(Point({IsMethod, Anchor::Below}, AS_private), Code.point("end"));
+  EXPECT_EQ(Point({IsMethod, Anchor::Below}, 

[PATCH] D116290: [clang-format] Add enforcement of consistent `class`/typename` keyword for template arguments

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

> Will you still work on this?

I don't know. I currently have more high-priority work, and don't know 
whether/when I will find time for this review again.
Also, I realized that I will have to read up more about class-template 
parameters, e.g. I don't quite understand 
https://quuxplusone.github.io/blog/2019/12/27/template-typename-fun/, yet.

So, if you are asking to figure out if you should still review this in more 
detail: No, for the time being you should not.
If you are asking because you would like to take over this commit and finish it 
so that it can land: Yes, I would be happy to hand this over


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116290

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

[PATCH] D119537: [clangd] Treat 'auto' params as deduced if there's a single instantiation.

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

Should this be added to the Release Notes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119537

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

[PATCH] D119537: [clangd] Treat 'auto' params as deduced if there's a single instantiation.

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

code looks good to me, but I don't know the code base well enough for a "LGTM"

Thanks for implementing this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119537

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

[PATCH] D119537: [clangd] Treat 'auto' params as deduced if there's a single instantiation.

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:508
+int ParamIndex = paramIndex(*FTD, *Auto.getDecl());
+if (ParamIndex < 0)
+  return true;

out of curiosity: when would this happen?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119537

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

[PATCH] D116514: [clangd] Add code action to generate a constructor for a C++ class

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

Can we add this tweak as the default "fix-it" action for

> Class '...' does not declare any constructor to initialize its non-modifiable 
> me

on structs like

  struct X {
 int& a;
  }

?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116514

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

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

Thanks, @xazax.hun and @whisperity for helping me to get this over the finish 
line! :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

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

[PATCH] D116514: [clangd] Add code action to generate a constructor for a C++ class

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.



Comment at: 
clang-tools-extra/clangd/refactor/tweaks/MemberwiseConstructor.cpp:38
+  std::string title() const override {
+return llvm::formatv("define constructor");
+  }

`Define constructor` (first letter uppercase)
At least this seems to be the convention for most other tweaks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116514

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

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

@xazax.hun ping re merging this to `main`. Would be amazing to get this in 
still in time for clang 14.

In case it makes merging easier for you, there is a copy of this commit on 
https://github.com/vogelsgesang/llvm-project/commits/avogelsgesang-tidy-readability-container-contains


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

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

[PATCH] D116514: [clangd] Add code action to generate a constructor for a C++ class

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.



Comment at: 
clang-tools-extra/clangd/refactor/tweaks/MemberwiseConstructor.cpp:26-28
+//   struct S {
+// S(int x, unique_ptr y) : x(x), y(std::move(y)) {}
+//   };

njames93 wrote:
> avogelsgesang wrote:
> > ```
> > // e.g. given `struct S{ int x; unique_ptr y; };`, produces:
> > //   struct S {
> > // int x; unique_ptr y;
> > // S(int x, unique_ptr y) : x(x), y(std::move(y)) {}
> > //   };
> > ```
> > or just
> > ```
> > // e.g. given `struct S{ int x; unique_ptr y; };`, produces:
> > // S(int x, unique_ptr y) : x(x), y(std::move(y)) {}
> > ```
> > 
> > The tweak does not remove the members, as currently suggested by the comment
> That's just a bad comment, the tweak won't remove the members
I know. Let's fix the comment :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116514

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

[PATCH] D116514: [clangd] Add code action to generate a constructor for a C++ class

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.



Comment at: 
clang-tools-extra/clangd/refactor/tweaks/MemberwiseConstructor.cpp:49
+  Class = N->ASTNode.get();
+if (!Class || !Class->isThisDeclarationADefinition() || Class->isUnion())
+  return false;

njames93 wrote:
> avogelsgesang wrote:
> > do we also need to exclude anonymous class declarations here? (Not sure if 
> > those are also modelled as `CXXRecordDecl` in the clang AST...)
> Good point, should also ensure there is a test case for this as well.
On 2nd thought: To trigger this tweak, I click on the class name, and since 
anonymous structs don't have class names, I think the tweak can't even be 
triggered.

Still probably worth a check here, just to be sure. Or at least an `assert`+ 
comment in the code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116514

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

[PATCH] D116514: [clangd] Add code action to generate a constructor for a C++ class

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.



Comment at: 
clang-tools-extra/clangd/refactor/tweaks/MemberwiseConstructor.cpp:26-28
+//   struct S {
+// S(int x, unique_ptr y) : x(x), y(std::move(y)) {}
+//   };

```
// e.g. given `struct S{ int x; unique_ptr y; };`, produces:
//   struct S {
// int x; unique_ptr y;
// S(int x, unique_ptr y) : x(x), y(std::move(y)) {}
//   };
```
or just
```
// e.g. given `struct S{ int x; unique_ptr y; };`, produces:
// S(int x, unique_ptr y) : x(x), y(std::move(y)) {}
```

The tweak does not remove the members, as currently suggested by the comment



Comment at: 
clang-tools-extra/clangd/refactor/tweaks/MemberwiseConstructor.cpp:49
+  Class = N->ASTNode.get();
+if (!Class || !Class->isThisDeclarationADefinition() || Class->isUnion())
+  return false;

do we also need to exclude anonymous class declarations here? (Not sure if 
those are also modelled as `CXXRecordDecl` in the clang AST...)



Comment at: 
clang-tools-extra/clangd/refactor/tweaks/MemberwiseConstructor.cpp:133
+// Decide what to do based on the field type.
+class Visitor : public TypeVisitor {
+public:

Is this visitor able to look through `using MyType = int;` and `typedef`?
I think we should also add a test case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116514

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

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

@xazax.hun Can you please merge this to `main` on my behalf? (I don't have 
commit rights)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

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

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 400820.
avogelsgesang added a comment.

Still warn for issues in macros even if not offering a fix-it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
@@ -0,0 +1,230 @@
+// RUN: %check_clang_tidy -std=c++20 %s readability-container-contains %t
+
+// Some *very* simplified versions of `map` etc.
+namespace std {
+
+template 
+struct map {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+  void *find(const Key );
+  void *end();
+};
+
+template 
+struct set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct unordered_set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct multimap {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+} // namespace std
+
+// Check that we detect various common ways to check for membership
+int testDifferentCheckTypes(std::map ) {
+  if (MyMap.count(0))
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use 'contains' to check for membership [readability-container-contains]
+// CHECK-FIXES: if (MyMap.contains(0))
+return 1;
+  bool C1 = MyMap.count(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = MyMap.contains(1);
+  auto C2 = static_cast(MyMap.count(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C2 = static_cast(MyMap.contains(1));
+  auto C3 = MyMap.count(2) != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C3 = MyMap.contains(2);
+  auto C4 = MyMap.count(3) > 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C4 = MyMap.contains(3);
+  auto C5 = MyMap.count(4) >= 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C5 = MyMap.contains(4);
+  auto C6 = MyMap.find(5) != MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C6 = MyMap.contains(5);
+  return C1 + C2 + C3 + C4 + C5 + C6;
+}
+
+// Check that we detect various common ways to check for non-membership
+int testNegativeChecks(std::map ) {
+  bool C1 = !MyMap.count(-1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = !MyMap.contains(-1);
+  auto C2 = MyMap.count(-2) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C2 = !MyMap.contains(-2);
+  auto C3 = MyMap.count(-3) <= 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C3 = !MyMap.contains(-3);
+  auto C4 = MyMap.count(-4) < 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C4 = !MyMap.contains(-4);
+  auto C5 = MyMap.find(-5) == MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C5 = !MyMap.contains(-5);
+  return C1 + C2 + C3 + C4 + C5;
+}
+
+// Check for various types
+int testDifferentTypes(std::map , std::unordered_set , std::set , std::multimap ) {
+  bool C1 = M.count(1001);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = M.contains(1001);
+  bool C2 = US.count(1002);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C2 = 

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 400813.
avogelsgesang marked an inline comment as done.
avogelsgesang added a comment.

Properly support macros as requested by @xazax.hun

Added test cases for
+#define COUNT_ONES(SET) SET.count(1)
+  // CHECK-FIXES: #define COUNT_ONES(SET) SET.count(1)
+  // Rewriting the macro would break the code
+  if (COUNT_ONES(MySet)) {
+return COUNT_ONES(MySet);
+  }
+#undef COUNT_ONES
+#define COUNT_ONES count(1)
+  // CHECK-FIXES: #define COUNT_ONES count(1)
+  // Rewriting the macro would break the code
+  if (MySet.COUNT_ONES) {
+return MySet.COUNT_ONES;
+  }
+#undef COUNT_ONES
+#define MY_SET MySet
+  // CHECK-FIXES: #define MY_SET MySet
+  // We still want to rewrite one of the two calls to `count`
+  if (MY_SET.count(1)) {
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: use 'contains' to check for 
membership [readability-container-contains]
+// CHECK-FIXES: if (MY_SET.contains(1)) {
+   return MY_SET.count(1);
+  }

Please let me know if I missed any cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
@@ -0,0 +1,223 @@
+// RUN: %check_clang_tidy -std=c++20 %s readability-container-contains %t
+
+// Some *very* simplified versions of `map` etc.
+namespace std {
+
+template 
+struct map {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+  void *find(const Key );
+  void *end();
+};
+
+template 
+struct set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct unordered_set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct multimap {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+} // namespace std
+
+// Check that we detect various common ways to check for membership
+int testDifferentCheckTypes(std::map ) {
+  if (MyMap.count(0))
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use 'contains' to check for membership [readability-container-contains]
+// CHECK-FIXES: if (MyMap.contains(0))
+return 1;
+  bool C1 = MyMap.count(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = MyMap.contains(1);
+  auto C2 = static_cast(MyMap.count(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C2 = static_cast(MyMap.contains(1));
+  auto C3 = MyMap.count(2) != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C3 = MyMap.contains(2);
+  auto C4 = MyMap.count(3) > 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C4 = MyMap.contains(3);
+  auto C5 = MyMap.count(4) >= 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C5 = MyMap.contains(4);
+  auto C6 = MyMap.find(5) != MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C6 = MyMap.contains(5);
+  return C1 + C2 + C3 + C4 + C5 + C6;
+}
+
+// Check that we detect various common ways to check for non-membership
+int testNegativeChecks(std::map ) {
+  bool C1 = !MyMap.count(-1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = !MyMap.contains(-1);
+  auto C2 = MyMap.count(-2) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C2 = !MyMap.contains(-2);
+  auto C3 = MyMap.count(-3) <= 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C3 = !MyMap.contains(-3);
+  auto C4 = MyMap.count(-4) < 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: 

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

@aaron.ballman @xazax.hun Could I ask one of you to finish the review here? :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

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

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

Happy new year! - and a gentle ping ;)

Afaict, there are only two smaller issues remaining (see non-closed inline 
comments):

- do we want a test expectation to check for unmodified code
- should we remove a couple of comments from the code

Personally, I have no real opinion on those questions. Would be happy to get 
some guidance here, @whisperity, so we can wrap this review up


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

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

[PATCH] D116290: [clang-format] Add enforcement of consistent `class`/typename` keyword for template arguments

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.



Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.h:36
+} // end namespace format
+} // end namespace clang
+

avogelsgesang wrote:
> curdeius wrote:
> > Please make the comments consistent with other files.
> Consistent with which other files?
> My comments here are consistent with `NamespaceEndCommentsFixer.h` and 
> `QualifierAlignmentFixer.h` which I used for reference while writing this 
> change
ok, now I understood what you meant...
You meant consistency with the other files in this commit, in particular with 
the corresponding cpp file `TemplateArgumentKeywordFixer.h`.

Fun fact: I copied this inconsistency from `NamespaceEndCommentsFixer.h`/`.cpp` 
which also use different comment styles between `.h` and `.cpp`.

I updated my change to consistently use the `// namespace` style


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116290

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

[PATCH] D116290: [clang-format] Add enforcement of consistent `class`/typename` keyword for template arguments

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 396403.
avogelsgesang marked an inline comment as done.
avogelsgesang added a comment.

Fix namespace comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116290

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/Format.cpp
  clang/lib/Format/TemplateArgumentKeywordFixer.cpp
  clang/lib/Format/TemplateArgumentKeywordFixer.h
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp

Index: clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp
===
--- /dev/null
+++ clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp
@@ -0,0 +1,219 @@
+//===- unittest/Format/TemplateArgumentKeywordFixerTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Format/Format.h"
+
+#include "FormatTestUtils.h"
+#include "TestLexer.h"
+#include "gtest/gtest.h"
+
+#include "../../lib/Format/TemplateArgumentKeywordFixer.h"
+
+using testing::ScopedTrace;
+
+namespace clang {
+namespace format {
+namespace {
+
+class TemplateArgumentKeywordFixerTest : public ::testing::Test {
+protected:
+  std::string format(llvm::StringRef Code,
+ const std::vector ,
+ const FormatStyle ) {
+LLVM_DEBUG(llvm::errs() << "---\n");
+LLVM_DEBUG(llvm::errs() << Code << "\n\n");
+FormattingAttemptStatus Status;
+tooling::Replacements Replaces =
+reformat(Style, Code, Ranges, "", );
+EXPECT_TRUE(Status.FormatComplete);
+auto Result = applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(Result));
+LLVM_DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
+return *Result;
+  }
+
+  std::string format(llvm::StringRef Code, const FormatStyle ) {
+return format(Code,
+  /*Ranges=*/{1, tooling::Range(0, Code.size())}, Style);
+  }
+
+  void _verifyFormat(const char *File, int Line, llvm::StringRef Expected,
+ llvm::StringRef Code, const FormatStyle ) {
+ScopedTrace t(File, Line, ::testing::Message() << Code.str());
+EXPECT_EQ(Expected.str(), format(Expected, Style))
+<< "Expected code is not stable";
+EXPECT_EQ(Expected.str(), format(Code, Style));
+EXPECT_EQ(Expected.str(), format(test::messUp(Code), Style));
+  }
+
+  void _verifyFormat(const char *File, int Line, llvm::StringRef Code,
+ const FormatStyle ) {
+_verifyFormat(File, Line, Code, Code, Style);
+  }
+};
+
+#define verifyFormat(...) _verifyFormat(__FILE__, __LINE__, __VA_ARGS__)
+
+TEST_F(TemplateArgumentKeywordFixerTest, DisabledByDefault) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.TemplateArgumentKeyword, FormatStyle::TAS_Leave);
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A",
+   "template  class A", Style);
+}
+
+TEST_F(TemplateArgumentKeywordFixerTest, FormatsAsClass) {
+  FormatStyle Style = getLLVMStyle();
+  Style.TemplateArgumentKeyword = FormatStyle::TAS_Class;
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A;",
+   "template  class A;", Style);
+  verifyFormat("template  class A;",
+   "template  class A;", Style);
+  verifyFormat("template  class Y> class A;",
+   "template  typename Y> class A;", Style);
+}
+
+TEST_F(TemplateArgumentKeywordFixerTest, ClassLeavesOtherTypenames) {
+  FormatStyle Style = getLLVMStyle();
+  Style.TemplateArgumentKeyword = FormatStyle::TAS_Class;
+  // `typename` outside of a template should stay unchanged.
+  verifyFormat("typename T::nested", Style);
+  // `typename` inside a template should stay unchanged if it refers to a nested
+  // type.
+  verifyFormat(
+  "template  class A;",
+  "template  class A;", Style);
+  // A legitimate `typename` might also occur in a nested list, directly after
+  // the `,` token It's not sufficient to just check for a comma in front of
+  // `typename`.
+  verifyFormat("template , class Z "
+   "= void> class A;",
+   "template , "
+   "typename Z = void> class A;",
+   Style);
+  // `typename` might also occur in a function call, not only in a type.
+  verifyFormat("template (), 

[PATCH] D116290: [clang-format] Add enforcement of consistent `class`/typename` keyword for template arguments

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.



Comment at: clang/include/clang/Format/Format.h:3691
+  ///  COULD lead to incorrect code formatting due to incorrect decisions made
+  ///  due to clang-formats lack of complete semantic information. As such 
extra
+  ///  care should be taken to review code changes made by the use of this

curdeius wrote:
> Nit.
Good catch!

I just copied that phrase from `QualifierAlignment` and overlooked that I also 
copied the typo. Fixed the typo in both locations.



Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:55
+// For `auto` language version, be conservative and assume we are < C++17
+KeepTemplateTemplateKW = (Style.Standard == FormatStyle::LS_Auto) ||
+ (Style.Standard < FormatStyle::LS_Cpp17);

curdeius wrote:
> Isn't it a better name?
This flag is actually about the usage of the `class` keyword instead of the 
`typename` keyword for template-template arguments.
`true` means: "Keep using the `class` instead of the `typename` keyword for 
template-template arguments."

I think the name `KeepTemplateTypenameKW` is wrong. "[...]TypenameKW = true" 
would mean "use `typename` instead of `class`" to me, and that's exactly the 
opposite way around.

As such, I think `KeepTemplateTemplateKW` is in fact the better name. If we 
want to make it even more explicit, we could also use 
`KeepTemplateTemplateClassKW`. What do you think?



Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:65
+  }
+
+  for (size_t I = 0, E = AnnotatedLines.size(); I != E; ++I) {

curdeius wrote:
> Could we check `KeepTemplateTypenameKW` and early return here?
> The optimizer might do some job, but I doubt it will get rid of the 
> unnecessary finding of template keyword etc.
> We could maybe avoid this variable altogether and return inside the switch, 
> no?
I think you misunderstood the semantics of `KeepTemplateTypenameKW`? Or did I 
misunderstand your comment?

For both `KeepTemplateTemplateKW = true` and `KeepTemplateTemplateKW = false`, 
the loop below still reformats `class` to `typename`.

E.g., for the input
```
template class X>
```
and `KeepTemplateTemplateKW = true`, we produce
```
template class X>
```
For the same input with `KeepTemplateTemplateKW = false`, we produce
```
template typename X>
```



Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:69
+
+// Find the first `template` keyword on this line
+while (Tok && Tok->isNot(tok::kw_template))

curdeius wrote:
> Please finish comments with full stops, here and below.
Fixed here and all other places I could find in my change



Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.h:36
+} // end namespace format
+} // end namespace clang
+

curdeius wrote:
> Please make the comments consistent with other files.
Consistent with which other files?
My comments here are consistent with `NamespaceEndCommentsFixer.h` and 
`QualifierAlignmentFixer.h` which I used for reference while writing this change



Comment at: clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp:155
+  // We also format template argument lists for `using`.
+  // There are no special semantics re. deduction guides and the normal
+  // implementation just works. But better test it before this breaks due to

curdeius wrote:
> Don't repeat comments, please. It's the same as below in deduction guides 
> test.
removed comment in both test cases. Reading it again, I agree that the comment 
didn't provide much value in the first place


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116290

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

[PATCH] D116290: [clang-format] Add enforcement of consistent `class`/typename` keyword for template arguments

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 396401.
avogelsgesang marked 7 inline comments as done.
avogelsgesang added a comment.

Thank you for your review, @curdeius!

I updated my change to address most of your comments.
Regarding "KeepTemplateTemplateKW", I think we had a misunderstanding (see 
replies to inline comments).
Could you take a 2nd look at those comments?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116290

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/Format.cpp
  clang/lib/Format/TemplateArgumentKeywordFixer.cpp
  clang/lib/Format/TemplateArgumentKeywordFixer.h
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp

Index: clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp
===
--- /dev/null
+++ clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp
@@ -0,0 +1,219 @@
+//===- unittest/Format/TemplateArgumentKeywordFixerTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Format/Format.h"
+
+#include "FormatTestUtils.h"
+#include "TestLexer.h"
+#include "gtest/gtest.h"
+
+#include "../../lib/Format/TemplateArgumentKeywordFixer.h"
+
+using testing::ScopedTrace;
+
+namespace clang {
+namespace format {
+namespace {
+
+class TemplateArgumentKeywordFixerTest : public ::testing::Test {
+protected:
+  std::string format(llvm::StringRef Code,
+ const std::vector ,
+ const FormatStyle ) {
+LLVM_DEBUG(llvm::errs() << "---\n");
+LLVM_DEBUG(llvm::errs() << Code << "\n\n");
+FormattingAttemptStatus Status;
+tooling::Replacements Replaces =
+reformat(Style, Code, Ranges, "", );
+EXPECT_TRUE(Status.FormatComplete);
+auto Result = applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(Result));
+LLVM_DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
+return *Result;
+  }
+
+  std::string format(llvm::StringRef Code, const FormatStyle ) {
+return format(Code,
+  /*Ranges=*/{1, tooling::Range(0, Code.size())}, Style);
+  }
+
+  void _verifyFormat(const char *File, int Line, llvm::StringRef Expected,
+ llvm::StringRef Code, const FormatStyle ) {
+ScopedTrace t(File, Line, ::testing::Message() << Code.str());
+EXPECT_EQ(Expected.str(), format(Expected, Style))
+<< "Expected code is not stable";
+EXPECT_EQ(Expected.str(), format(Code, Style));
+EXPECT_EQ(Expected.str(), format(test::messUp(Code), Style));
+  }
+
+  void _verifyFormat(const char *File, int Line, llvm::StringRef Code,
+ const FormatStyle ) {
+_verifyFormat(File, Line, Code, Code, Style);
+  }
+};
+
+#define verifyFormat(...) _verifyFormat(__FILE__, __LINE__, __VA_ARGS__)
+
+TEST_F(TemplateArgumentKeywordFixerTest, DisabledByDefault) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.TemplateArgumentKeyword, FormatStyle::TAS_Leave);
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A",
+   "template  class A", Style);
+}
+
+TEST_F(TemplateArgumentKeywordFixerTest, FormatsAsClass) {
+  FormatStyle Style = getLLVMStyle();
+  Style.TemplateArgumentKeyword = FormatStyle::TAS_Class;
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A;",
+   "template  class A;", Style);
+  verifyFormat("template  class A;",
+   "template  class A;", Style);
+  verifyFormat("template  class Y> class A;",
+   "template  typename Y> class A;", Style);
+}
+
+TEST_F(TemplateArgumentKeywordFixerTest, ClassLeavesOtherTypenames) {
+  FormatStyle Style = getLLVMStyle();
+  Style.TemplateArgumentKeyword = FormatStyle::TAS_Class;
+  // `typename` outside of a template should stay unchanged.
+  verifyFormat("typename T::nested", Style);
+  // `typename` inside a template should stay unchanged if it refers to a nested
+  // type.
+  verifyFormat(
+  "template  class A;",
+  "template  class A;", Style);
+  // A legitimate `typename` might also occur in a nested list, directly after
+  // the `,` token It's not sufficient to just check for a comma in front of
+  // `typename`.
+  verifyFormat("template , class Z "
+   "= 

[PATCH] D116290: [clang-format] Add enforcement of consistent `class`/typename` keyword for template arguments

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:4003
 
+**TemplateArgumentKeyword** (``TemplateArgumentStyle``) 
:versionbadge:`clang-format 14`
+  Keyword to use for template arguments (``class`` or ``typename``)

HazardyKnusperkeks wrote:
> Have you edited this by hand? It is on a different (the right) position than 
> in `Format.h`.
No, this file is auto-generated using the `dump_format_style.py` script. 
Afaict, it sorts the entries alphabetically during generating the docs.



Comment at: clang/include/clang/Format/Format.h:1963
+  /// \version 14
+  TemplateArgumentStyle TemplateArgumentKeyword;
+

HazardyKnusperkeks wrote:
> Please sort (as good as possible, I know some things are out of order).
moved after `TabWidth`



Comment at: clang/lib/Format/Format.cpp:144-145
+IO.enumCase(Value, "Leave", FormatStyle::TAS_Leave);
+IO.enumCase(Value, "typename", FormatStyle::TAS_Typename);
+IO.enumCase(Value, "class", FormatStyle::TAS_Class);
+  }

HazardyKnusperkeks wrote:
> This is what is printed in the documentation, if generated automatically.
The documentation is currently automatically generated. I used the additional 
comments in `Format.h` to overwrite the "in configuration" values:

```
/// ...
TAS_Leave,
/// ...
TAS_Typename, // typename
/// ...
TAS_Class // class
```

Note the additional comments after `TAS_Typename` and `TAS_Class`. They 
overwrite the "in configuration" names used by dump_format_style.py. The same 
trick is used, e.g., for the `LanguageStandard` enum.

Given that using lowercase `typename` and `class` still allow for 
auto-generated documentation, do yous still want me to switch to uppercase 
`Typename`/`Class`?

Personally, I do prefer lowercase here because the language keywords are also 
lowercase. Are there any other reasons except for auto-generated documentation 
why you would prefer uppercase config values?



Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:45
+  StringRef ReplacementStr;
+  tok::TokenKind BadToken;
+  bool KeepTemplateTemplateKW = false;

HazardyKnusperkeks wrote:
> I think you should rename it to TokenKindToReplace or something like that.
> A `is(BadToken)` can be misleading.
Good point! Done.



Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:49
+  case FormatStyle::TAS_Leave:
+return {Fixes, 0};
+  case FormatStyle::TAS_Typename:

HazardyKnusperkeks wrote:
> Should never happen, right? Assert on that.
yes, this should be unreachable. I added an assert.

Out of curiosity/slightly off-topic: where does clang-format stand re 
"defensive programming"? "Fail fast, fail often", or rather "don't trust your 
callers and handle as many situations as possible as gracefully as possible"?



Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:51
+  case FormatStyle::TAS_Typename:
+assert(Style.Standard != FormatStyle::LS_Auto);
+KeepTemplateTemplateKW = Style.Standard < FormatStyle::LS_Cpp17;

HazardyKnusperkeks wrote:
> Why is that? It is certainly possible to have that configuration and we 
> should not assert on possible user input. Either error out or handle it. I 
> think we should be on the safe side an pretend auto is before 17. Add a note 
> to the documentation about that.
I was under the false impression that `deriveLocalStyle` in `Format.cpp` would 
always replace `auto` by one of the actual versions. I was wrong about that.

Thanks for catching this!

I updated the code and added a test case for this particular case



Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:80
+}
+FormatToken *EndTok = Tok->MatchingParen;
+

HazardyKnusperkeks wrote:
> assert on non nullness
nullness was checked already in line 75:

```
if (Tok->isNot(TT_TemplateOpener) || !Tok->MatchingParen) {
```

Do you still want me to add this `assert`? Should I maybe restructure the code 
somehow to make it easier to understand? Or are you fine with leaving it as is?



Comment at: clang/lib/Format/TemplateArgumentKeywordFixer.cpp:93
+  bool StartedDefaultArg = Tok->is(tok::equal);
+  auto advanceTok = [&]() {
+Tok = Tok->Next;

HazardyKnusperkeks wrote:
> Could be located outside of the loop.
I agree it could be moved outside the loop. Should it, though?

Having it inside the loop makes the code slightly easier to read by keeping the 
definition closer to its usage. And performancewise, I do trust the compiler to 
do a decent job here.

Or am I somehow misguided in my judgement here?



Comment at: clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp:167
+}
+
+} // namespace

HazardyKnusperkeks wrote:
> Just to be sure add some tests with incomplete 

[PATCH] D116290: [clang-format] Add enforcement of consistent `class`/typename` keyword for template arguments

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 396361.
avogelsgesang marked 4 inline comments as done.
avogelsgesang added a comment.

Thank you for the quick review, @HazardyKnusperkeks!

I addressed most of your comments by fixing my code.
In two cases, I think there was a misunderstanding and I didn't apply your 
proposed changes, yet.
See my inline replies re "assert on non nullness" and "`typename` vs `Typename` 
in configuration".
If you still think, I should apply your proposed changes, please let me know. I 
don't have strong opinions on those points and am happy to do so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116290

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/Format.cpp
  clang/lib/Format/TemplateArgumentKeywordFixer.cpp
  clang/lib/Format/TemplateArgumentKeywordFixer.h
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp

Index: clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp
===
--- /dev/null
+++ clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp
@@ -0,0 +1,220 @@
+//===- unittest/Format/TemplateArgumentKeywordFixerTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Format/Format.h"
+
+#include "FormatTestUtils.h"
+#include "TestLexer.h"
+#include "gtest/gtest.h"
+
+#include "../../lib/Format/TemplateArgumentKeywordFixer.h"
+
+using testing::ScopedTrace;
+
+namespace clang {
+namespace format {
+namespace {
+
+class TemplateArgumentKeywordFixerTest : public ::testing::Test {
+protected:
+  std::string format(llvm::StringRef Code,
+ const std::vector ,
+ const FormatStyle ) {
+LLVM_DEBUG(llvm::errs() << "---\n");
+LLVM_DEBUG(llvm::errs() << Code << "\n\n");
+FormattingAttemptStatus Status;
+tooling::Replacements Replaces =
+reformat(Style, Code, Ranges, "", );
+EXPECT_TRUE(Status.FormatComplete);
+auto Result = applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(Result));
+LLVM_DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
+return *Result;
+  }
+
+  std::string format(llvm::StringRef Code, const FormatStyle ) {
+return format(Code,
+  /*Ranges=*/{1, tooling::Range(0, Code.size())}, Style);
+  }
+
+  void _verifyFormat(const char *File, int Line, llvm::StringRef Expected,
+ llvm::StringRef Code, const FormatStyle ) {
+ScopedTrace t(File, Line, ::testing::Message() << Code.str());
+EXPECT_EQ(Expected.str(), format(Expected, Style))
+<< "Expected code is not stable";
+EXPECT_EQ(Expected.str(), format(Code, Style));
+EXPECT_EQ(Expected.str(), format(test::messUp(Code), Style));
+  }
+};
+
+#define verifyFormat(...) _verifyFormat(__FILE__, __LINE__, __VA_ARGS__)
+
+TEST_F(TemplateArgumentKeywordFixerTest, DisabledByDefault) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.TemplateArgumentKeyword, FormatStyle::TAS_Leave);
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A",
+   "template  class A", Style);
+}
+
+TEST_F(TemplateArgumentKeywordFixerTest, FormatsAsClass) {
+  FormatStyle Style = getLLVMStyle();
+  Style.TemplateArgumentKeyword = FormatStyle::TAS_Class;
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A;",
+   "template  class A;", Style);
+  verifyFormat("template  class A;",
+   "template  class A;", Style);
+  verifyFormat("template  class Y> class A;",
+   "template  typename Y> class A;", Style);
+}
+
+TEST_F(TemplateArgumentKeywordFixerTest, ClassLeavesOtherTypenames) {
+  FormatStyle Style = getLLVMStyle();
+  Style.TemplateArgumentKeyword = FormatStyle::TAS_Class;
+  // `typename` outside of a template should stay unchanged
+  verifyFormat("typename T::nested", "typename T::nested", Style);
+  // `typename` inside a template should stay unchanged if it refers to a nested
+  // type
+  verifyFormat(
+  "template  class A;",
+  "template  class A;", Style);
+  // A legitimate `typename` might also occur in a nested list, directly after
+  // the `,` token It's not sufficient to just check for a comma in front of
+  // `typename`.
+  

[PATCH] D116290: [clang-format] Add enforcement of consistent `class`/typename` keyword for template arguments

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added reviewers: MyDeveloperDay, HazardyKnusperkeks, 
aaron.ballman, curdeius, sammccall, owenpan.
avogelsgesang added a comment.

Adding reviewers from D69764  to this change 
because I think it falls into the same category: It replaces tokens and could 
thereby potentially break code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116290

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

[PATCH] D116290: [clang-format] Add enforcement of consistent `class`/typename` keyword for template arguments

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 396237.
avogelsgesang added a comment.

Fix code formatting


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116290

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/Format.cpp
  clang/lib/Format/TemplateArgumentKeywordFixer.cpp
  clang/lib/Format/TemplateArgumentKeywordFixer.h
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp

Index: clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp
===
--- /dev/null
+++ clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp
@@ -0,0 +1,170 @@
+//===- unittest/Format/TemplateArgumentKeywordFixerTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Format/Format.h"
+
+#include "FormatTestUtils.h"
+#include "TestLexer.h"
+#include "gtest/gtest.h"
+
+#include "../../lib/Format/TemplateArgumentKeywordFixer.h"
+
+using testing::ScopedTrace;
+
+namespace clang {
+namespace format {
+namespace {
+
+class TemplateArgumentKeywordFixerTest : public ::testing::Test {
+protected:
+  std::string format(llvm::StringRef Code,
+ const std::vector ,
+ const FormatStyle ) {
+LLVM_DEBUG(llvm::errs() << "---\n");
+LLVM_DEBUG(llvm::errs() << Code << "\n\n");
+FormattingAttemptStatus Status;
+tooling::Replacements Replaces =
+reformat(Style, Code, Ranges, "", );
+EXPECT_TRUE(Status.FormatComplete);
+auto Result = applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(Result));
+LLVM_DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
+return *Result;
+  }
+
+  std::string format(llvm::StringRef Code, const FormatStyle ) {
+return format(Code,
+  /*Ranges=*/{1, tooling::Range(0, Code.size())}, Style);
+  }
+
+  void _verifyFormat(const char *File, int Line, llvm::StringRef Expected,
+ llvm::StringRef Code, const FormatStyle ) {
+ScopedTrace t(File, Line, ::testing::Message() << Code.str());
+EXPECT_EQ(Expected.str(), format(Expected, Style))
+<< "Expected code is not stable";
+EXPECT_EQ(Expected.str(), format(Code, Style));
+EXPECT_EQ(Expected.str(), format(test::messUp(Code), Style));
+  }
+};
+
+#define verifyFormat(...) _verifyFormat(__FILE__, __LINE__, __VA_ARGS__)
+
+TEST_F(TemplateArgumentKeywordFixerTest, DisabledByDefault) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.TemplateArgumentKeyword, FormatStyle::TAS_Leave);
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A",
+   "template  class A", Style);
+}
+
+TEST_F(TemplateArgumentKeywordFixerTest, FormatsAsClass) {
+  FormatStyle Style = getLLVMStyle();
+  Style.TemplateArgumentKeyword = FormatStyle::TAS_Class;
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A;",
+   "template  class A;", Style);
+  verifyFormat("template  class A;",
+   "template  class A;", Style);
+  verifyFormat("template  class Y> class A;",
+   "template  typename Y> class A;", Style);
+}
+
+TEST_F(TemplateArgumentKeywordFixerTest, ClassLeavesOtherTypenames) {
+  FormatStyle Style = getLLVMStyle();
+  Style.TemplateArgumentKeyword = FormatStyle::TAS_Class;
+  // `typename` outside of a template should stay unchanged
+  verifyFormat("typename T::nested", "typename T::nested", Style);
+  // `typename` inside a template should stay unchanged if it refers to a nested
+  // type
+  verifyFormat(
+  "template  class A;",
+  "template  class A;", Style);
+  // A legitimate `typename` might also occur in a nested list, directly after
+  // the `,` token It's not sufficient to just check for a comma in front of
+  // `typename`.
+  verifyFormat("template , class Z "
+   "= void> class A;",
+   "template , "
+   "typename Z = void> class A;",
+   Style);
+  // `typename` might also occur in a function call, not only in a type
+  verifyFormat("template (), class Z = "
+   "void> class A;",
+   "template (), "
+   "typename Z = void> class A;",
+   Style);
+  // `typename` might also occur in function call 

[PATCH] D116290: [clang-format] Add enforcement of consistent `class`/typename` keyword for template arguments

[Adrian Vogelsgesang via Phabricator via cfe-commits]

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

Template argument types can be introduced using either the `class` or
the `typename` argument. While there are good arguments for both styles,
usually one wants to use one style consistently across a project.

This commit adds a `TemplateArgumentKeyword` option which can be used
to format all template arguments consistently using either the `class`
or the `typename` argument.

Implementationwise, it closely follows in the footsteps of D69764 
, i.e.
it adds a new `TemplateArgumentKeywordFixer` pass which creates the
appropriate replacements.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116290

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/Format.cpp
  clang/lib/Format/TemplateArgumentKeywordFixer.cpp
  clang/lib/Format/TemplateArgumentKeywordFixer.h
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp

Index: clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp
===
--- /dev/null
+++ clang/unittests/Format/TemplateArgumentKeywordFixerTest.cpp
@@ -0,0 +1,170 @@
+//===- unittest/Format/TemplateArgumentKeywordFixerTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Format/Format.h"
+
+#include "FormatTestUtils.h"
+#include "TestLexer.h"
+#include "gtest/gtest.h"
+
+#include "../../lib/Format/TemplateArgumentKeywordFixer.h"
+
+using testing::ScopedTrace;
+
+namespace clang {
+namespace format {
+namespace {
+
+class TemplateArgumentKeywordFixerTest : public ::testing::Test {
+protected:
+  std::string format(llvm::StringRef Code,
+ const std::vector ,
+ const FormatStyle ) {
+LLVM_DEBUG(llvm::errs() << "---\n");
+LLVM_DEBUG(llvm::errs() << Code << "\n\n");
+FormattingAttemptStatus Status;
+tooling::Replacements Replaces =
+reformat(Style, Code, Ranges, "", );
+EXPECT_TRUE(Status.FormatComplete);
+auto Result = applyAllReplacements(Code, Replaces);
+EXPECT_TRUE(static_cast(Result));
+LLVM_DEBUG(llvm::errs() << "\n" << *Result << "\n\n");
+return *Result;
+  }
+
+  std::string format(llvm::StringRef Code, const FormatStyle ) {
+return format(Code,
+  /*Ranges=*/{1, tooling::Range(0, Code.size())}, Style);
+  }
+
+  void _verifyFormat(const char *File, int Line, llvm::StringRef Expected,
+ llvm::StringRef Code, const FormatStyle ) {
+ScopedTrace t(File, Line, ::testing::Message() << Code.str());
+EXPECT_EQ(Expected.str(), format(Expected, Style))
+<< "Expected code is not stable";
+EXPECT_EQ(Expected.str(), format(Code, Style));
+EXPECT_EQ(Expected.str(), format(test::messUp(Code), Style));
+  }
+};
+
+#define verifyFormat(...) _verifyFormat(__FILE__, __LINE__, __VA_ARGS__)
+
+TEST_F(TemplateArgumentKeywordFixerTest, DisabledByDefault) {
+  FormatStyle Style = getLLVMStyle();
+  EXPECT_EQ(Style.TemplateArgumentKeyword, FormatStyle::TAS_Leave);
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A",
+   "template  class A", Style);
+}
+
+TEST_F(TemplateArgumentKeywordFixerTest, FormatsAsClass) {
+  FormatStyle Style = getLLVMStyle();
+  Style.TemplateArgumentKeyword = FormatStyle::TAS_Class;
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A", "template  class A",
+   Style);
+  verifyFormat("template  class A;",
+   "template  class A;", Style);
+  verifyFormat("template  class A;",
+   "template  class A;", Style);
+  verifyFormat("template  class Y> class A;",
+   "template  typename Y> class A;", Style);
+}
+
+TEST_F(TemplateArgumentKeywordFixerTest, ClassLeavesOtherTypenames) {
+  FormatStyle Style = getLLVMStyle();
+  Style.TemplateArgumentKeyword = FormatStyle::TAS_Class;
+  // `typename` outside of a template should stay unchanged
+  verifyFormat("typename T::nested", "typename T::nested", Style);
+  // `typename` inside a template should stay unchanged if it refers to a nested
+  // type
+  verifyFormat(
+  "template  class A;",
+  "template  class A;", Style);
+  // A legitimate `typename` might also occur in a nested 

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

gentle ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

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

[PATCH] D113943: Add `withIntrospection` matcher.

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

Nit:
Personally, I would prefer the name `withDebugOutput` over `withIntrospection`, 
as it more clearly describes the intent of this matcher.
At least to me, "introspection" is usually something done programatically, i.e. 
the matcher somehow reflects about its own state, but in this case all we are 
doing is printing some debug output to stderr.

Code looks good to me, but I don't know the LLVM coding guidelines and hence 
can't provide an actual code review




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3000-3001
+  const internal::Matcher ) {
+  return internal::Matcher(new internal::WithIntrospectionMatcher(
+  BeforeTag, AfterTag, InnerMatcher));
+}

std::move(BeforeTag), std::move(AfterTag)

No need to create additional copies of those strings. Here and other places


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113943

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

[PATCH] D112648: [clang-tidy] Don't offer partial fix-its for `modernize-pass-by-value`

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

> What name and email address would you like me to use for patch attribution?

Adrian Vogelsgesang, avogelsges...@salesforce.com


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112648

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

[PATCH] D112648: [clang-tidy] Don't offer partial fix-its for `modernize-pass-by-value`

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

@aaron.ballman can you please commit this on my behalf? I don't have commit 
access


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112648

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

[PATCH] D112648: [clang-tidy] Don't offer partial fix-its for `modernize-pass-by-value`

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 391121.
avogelsgesang marked an inline comment as done.
avogelsgesang added a comment.

Add missing dot to comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112648

Files:
  clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
@@ -231,5 +231,18 @@
 
 struct U {
   U(const POD ) : M(M) {}
+  // CHECK-FIXES: U(const POD ) : M(M) {}
   POD M;
 };
+
+// The rewrite can't look through `typedefs` and `using`.
+// Test that we don't partially rewrite one decl without rewriting the other.
+using MovableConstRef = const Movable &;
+struct V {
+  V(MovableConstRef M);
+  // CHECK-FIXES: V(MovableConstRef M);
+  Movable M;
+};
+V::V(const Movable ) : M(M) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move
+// CHECK-FIXES: V::V(const Movable ) : M(M) {}
Index: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
@@ -190,24 +190,35 @@
 
   auto Diag = diag(ParamDecl->getBeginLoc(), "pass by value and use 
std::move");
 
-  // Iterate over all declarations of the constructor.
-  for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) {
-auto ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
-auto RefTL = ParamTL.getAs();
+  // If we received a `const&` type, we need to rewrite the function
+  // declarations.
+  if (ParamDecl->getType()->isLValueReferenceType()) {
+// Check if we can succesfully rewrite all declarations of the constructor.
+for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) {
+  TypeLoc ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
+  ReferenceTypeLoc RefTL = ParamTL.getAs();
+  if (RefTL.isNull()) {
+// We cannot rewrite this instance. The type is probably hidden behind
+// some `typedef`. Do not offer a fix-it in this case.
+return;
+  }
+}
+// Rewrite all declarations.
+for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) {
+  TypeLoc ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
+  ReferenceTypeLoc RefTL = ParamTL.getAs();
 
-// Do not replace if it is already a value, skip.
-if (RefTL.isNull())
-  continue;
-
-TypeLoc ValueTL = RefTL.getPointeeLoc();
-auto TypeRange = CharSourceRange::getTokenRange(ParmDecl->getBeginLoc(),
-ParamTL.getEndLoc());
-std::string ValueStr = Lexer::getSourceText(CharSourceRange::getTokenRange(
-ValueTL.getSourceRange()),
-SM, getLangOpts())
-   .str();
-ValueStr += ' ';
-Diag << FixItHint::CreateReplacement(TypeRange, ValueStr);
+  TypeLoc ValueTL = RefTL.getPointeeLoc();
+  CharSourceRange TypeRange = CharSourceRange::getTokenRange(
+  ParmDecl->getBeginLoc(), ParamTL.getEndLoc());
+  std::string ValueStr =
+  Lexer::getSourceText(
+  CharSourceRange::getTokenRange(ValueTL.getSourceRange()), SM,
+  getLangOpts())
+  .str();
+  ValueStr += ' ';
+  Diag << FixItHint::CreateReplacement(TypeRange, ValueStr);
+}
   }
 
   // Use std::move in the initialization list.


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
@@ -231,5 +231,18 @@
 
 struct U {
   U(const POD ) : M(M) {}
+  // CHECK-FIXES: U(const POD ) : M(M) {}
   POD M;
 };
+
+// The rewrite can't look through `typedefs` and `using`.
+// Test that we don't partially rewrite one decl without rewriting the other.
+using MovableConstRef = const Movable &;
+struct V {
+  V(MovableConstRef M);
+  // CHECK-FIXES: V(MovableConstRef M);
+  Movable M;
+};
+V::V(const Movable ) : M(M) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move
+// CHECK-FIXES: V::V(const Movable ) : M(M) {}
Index: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
+++ 

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

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

Thanks for the instructions on how to build the documentation!
I fixed a build issue in the docs (incorrect length of the table footer) and 
updated the wording slightly

I am not planning any additional changes. If there is anything else you would 
want me to change, please let me know.
Otherwise, in case you approve this change, can you please also merge it on my 
behalf? I do not have commit access


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

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

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 391075.
avogelsgesang added a comment.

Fix build error in documentation and update wording


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
@@ -0,0 +1,196 @@
+// RUN: %check_clang_tidy -std=c++20 %s readability-container-contains %t
+
+// Some *very* simplified versions of `map` etc.
+namespace std {
+
+template 
+struct map {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+  void *find(const Key );
+  void *end();
+};
+
+template 
+struct set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct unordered_set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct multimap {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+} // namespace std
+
+// Check that we detect various common ways to check for membership
+int testDifferentCheckTypes(std::map ) {
+  if (MyMap.count(0))
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use 'contains' to check for membership [readability-container-contains]
+// CHECK-FIXES: if (MyMap.contains(0))
+return 1;
+  bool C1 = MyMap.count(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = MyMap.contains(1);
+  auto C2 = static_cast(MyMap.count(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C2 = static_cast(MyMap.contains(1));
+  auto C3 = MyMap.count(2) != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C3 = MyMap.contains(2);
+  auto C4 = MyMap.count(3) > 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C4 = MyMap.contains(3);
+  auto C5 = MyMap.count(4) >= 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C5 = MyMap.contains(4);
+  auto C6 = MyMap.find(5) != MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C6 = MyMap.contains(5);
+  return C1 + C2 + C3 + C4 + C5 + C6;
+}
+
+// Check that we detect various common ways to check for non-membership
+int testNegativeChecks(std::map ) {
+  bool C1 = !MyMap.count(-1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = !MyMap.contains(-1);
+  auto C2 = MyMap.count(-2) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C2 = !MyMap.contains(-2);
+  auto C3 = MyMap.count(-3) <= 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C3 = !MyMap.contains(-3);
+  auto C4 = MyMap.count(-4) < 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C4 = !MyMap.contains(-4);
+  auto C5 = MyMap.find(-5) == MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C5 = !MyMap.contains(-5);
+  return C1 + C2 + C3 + C4 + C5;
+}
+
+// Check for various types
+int testDifferentTypes(std::map , std::unordered_set , std::set , std::multimap ) {
+  bool C1 = M.count(1001);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = M.contains(1001);
+  bool C2 = US.count(1002);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C2 = US.contains(1002);
+  bool 

[PATCH] D112648: [clang-tidy] Improve fix-its for `modernize-pass-by-value` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.



Comment at: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp:198
+for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) {
+  auto ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
+  auto RefTL = ParamTL.getAs();

aaron.ballman wrote:
> Please spell out this type (per our usual coding conventions). The use on the 
> next line is fine because the type name is spelled out in the initialization.
Thanks for the hint. I wasn't aware of that convention. I fixed it here.

Should I also fix the other violations in this file?

E.g., a couple of lines above, we have

```
auto Diag = diag(ParamDecl->getBeginLoc(), "pass by value and use std::move");
```

which I guess should be

```
clang::DiagnosticBuilder Diag = diag(ParamDecl->getBeginLoc(), "pass by value 
and use std::move");
```



Comment at: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp:220
 
-TypeLoc ValueTL = RefTL.getPointeeLoc();
-auto TypeRange = CharSourceRange::getTokenRange(ParmDecl->getBeginLoc(),
-ParamTL.getEndLoc());
-std::string ValueStr = Lexer::getSourceText(CharSourceRange::getTokenRange(
-ValueTL.getSourceRange()),
-SM, getLangOpts())
-   .str();
-ValueStr += ' ';
-Diag << FixItHint::CreateReplacement(TypeRange, ValueStr);
+  // Check if we need to append an additional whitespace
+  auto TypeRangeEnd =

aaron.ballman wrote:
> I don't think we should take these changes, unless they're strictly necessary 
> for program correctness. The rule of thumb in clang-tidy for fix-its is that 
> we do not make much attempt to have pretty formatting from the fix (the fix 
> needs to be correct, but doesn't need to be formatted); the user can run 
> clang-format after applying fixes. Someday, we'd like to consume clang-format 
> as a library and automatically reformat the code after applying fixes instead 
> of trying to catch every formatting concern in each individual check.
sounds good. Removed the whitespace related parts from this patch



Comment at: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp:221-225
+  auto TypeRangeEnd =
+  Lexer::getLocForEndOfToken(ParamTL.getEndLoc(), 0, SM, 
getLangOpts());
+  auto NextChar = Lexer::getSourceText(
+  CharSourceRange::getCharRange(TypeRangeEnd,
+TypeRangeEnd.getLocWithOffset(1)),

aaron.ballman wrote:
> Please spell out the types.
no longer applicable after removing the whitespace related part of this patch



Comment at: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp:227-229
+  if (NextChar != " ") {
+ValueStr += ' ';
+  }

aaron.ballman wrote:
> (Style guideline nit)
no longer applicable after removing the whitespace related part of this patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112648

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

[PATCH] D112648: [clang-tidy] Improve fix-its for `modernize-pass-by-value` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 391059.
avogelsgesang marked 4 inline comments as done.
avogelsgesang added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112648

Files:
  clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
@@ -231,5 +231,18 @@
 
 struct U {
   U(const POD ) : M(M) {}
+  // CHECK-FIXES: U(const POD ) : M(M) {}
   POD M;
 };
+
+// The rewrite can't look through `typedefs` and `using`.
+// Test that we don't partially rewrite one decl without rewriting the other.
+using MovableConstRef = const Movable &;
+struct V {
+  V(MovableConstRef M);
+  // CHECK-FIXES: V(MovableConstRef M);
+  Movable M;
+};
+V::V(const Movable ) : M(M) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move
+// CHECK-FIXES: V::V(const Movable ) : M(M) {}
Index: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
@@ -190,24 +190,35 @@
 
   auto Diag = diag(ParamDecl->getBeginLoc(), "pass by value and use 
std::move");
 
-  // Iterate over all declarations of the constructor.
-  for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) {
-auto ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
-auto RefTL = ParamTL.getAs();
+  // If we received a `const&` type, we need to rewrite the function
+  // declarations
+  if (ParamDecl->getType()->isLValueReferenceType()) {
+// Check if we can succesfully rewrite all declarations of the constructor.
+for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) {
+  TypeLoc ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
+  ReferenceTypeLoc RefTL = ParamTL.getAs();
+  if (RefTL.isNull()) {
+// We cannot rewrite this instance. The type is probably hidden behind
+// some `typedef`. Do not offer a fix-it in this case.
+return;
+  }
+}
+// Rewrite all declarations.
+for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) {
+  TypeLoc ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
+  ReferenceTypeLoc RefTL = ParamTL.getAs();
 
-// Do not replace if it is already a value, skip.
-if (RefTL.isNull())
-  continue;
-
-TypeLoc ValueTL = RefTL.getPointeeLoc();
-auto TypeRange = CharSourceRange::getTokenRange(ParmDecl->getBeginLoc(),
-ParamTL.getEndLoc());
-std::string ValueStr = Lexer::getSourceText(CharSourceRange::getTokenRange(
-ValueTL.getSourceRange()),
-SM, getLangOpts())
-   .str();
-ValueStr += ' ';
-Diag << FixItHint::CreateReplacement(TypeRange, ValueStr);
+  TypeLoc ValueTL = RefTL.getPointeeLoc();
+  CharSourceRange TypeRange = CharSourceRange::getTokenRange(
+  ParmDecl->getBeginLoc(), ParamTL.getEndLoc());
+  std::string ValueStr =
+  Lexer::getSourceText(
+  CharSourceRange::getTokenRange(ValueTL.getSourceRange()), SM,
+  getLangOpts())
+  .str();
+  ValueStr += ' ';
+  Diag << FixItHint::CreateReplacement(TypeRange, ValueStr);
+}
   }
 
   // Use std::move in the initialization list.


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
@@ -231,5 +231,18 @@
 
 struct U {
   U(const POD ) : M(M) {}
+  // CHECK-FIXES: U(const POD ) : M(M) {}
   POD M;
 };
+
+// The rewrite can't look through `typedefs` and `using`.
+// Test that we don't partially rewrite one decl without rewriting the other.
+using MovableConstRef = const Movable &;
+struct V {
+  V(MovableConstRef M);
+  // CHECK-FIXES: V(MovableConstRef M);
+  Movable M;
+};
+V::V(const Movable ) : M(M) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move
+// CHECK-FIXES: V::V(const Movable ) : M(M) {}
Index: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
+++ 

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:189
+// NO-WARNING.
+// CHECK-FIXES: if (MyMap.count(0))
+return nullptr;

whisperity wrote:
> If a fix is not generated, why is this line here?
I added this test expectation to make sure that the line is kept unchanged. 

I followed 
https://clang.llvm.org/extra/clang-tidy/Contributing.html#testing-checks:

> In particular, CHECK-FIXES: can be used to check that code was not modified 
> by fix-its, by checking that it is present unchanged in the fixed code.

Based on this, I thought it is best practice to also check that places which I 
expect to stay unmodified are actually left unmodified. But I am not sure what 
the expectations in the clang-tidy code base are and whether those "the code 
stays unmodified" checks are actually encouraged or discouraged.
So just say the word and I am happy to remove this test expectation...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

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

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 390085.
avogelsgesang marked 7 inline comments as done.
avogelsgesang added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
@@ -0,0 +1,196 @@
+// RUN: %check_clang_tidy -std=c++20 %s readability-container-contains %t
+
+// Some *very* simplified versions of `map` etc.
+namespace std {
+
+template 
+struct map {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+  void *find(const Key );
+  void *end();
+};
+
+template 
+struct set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct unordered_set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct multimap {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+} // namespace std
+
+// Check that we detect various common ways to check for membership
+int testDifferentCheckTypes(std::map ) {
+  if (MyMap.count(0))
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use 'contains' to check for membership [readability-container-contains]
+// CHECK-FIXES: if (MyMap.contains(0))
+return 1;
+  bool C1 = MyMap.count(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = MyMap.contains(1);
+  auto C2 = static_cast(MyMap.count(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C2 = static_cast(MyMap.contains(1));
+  auto C3 = MyMap.count(2) != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C3 = MyMap.contains(2);
+  auto C4 = MyMap.count(3) > 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C4 = MyMap.contains(3);
+  auto C5 = MyMap.count(4) >= 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C5 = MyMap.contains(4);
+  auto C6 = MyMap.find(5) != MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C6 = MyMap.contains(5);
+  return C1 + C2 + C3 + C4 + C5 + C6;
+}
+
+// Check that we detect various common ways to check for non-membership
+int testNegativeChecks(std::map ) {
+  bool C1 = !MyMap.count(-1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = !MyMap.contains(-1);
+  auto C2 = MyMap.count(-2) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C2 = !MyMap.contains(-2);
+  auto C3 = MyMap.count(-3) <= 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C3 = !MyMap.contains(-3);
+  auto C4 = MyMap.count(-4) < 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C4 = !MyMap.contains(-4);
+  auto C5 = MyMap.find(-5) == MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C5 = !MyMap.contains(-5);
+  return C1 + C2 + C3 + C4 + C5;
+}
+
+// Check for various types
+int testDifferentTypes(std::map , std::unordered_set , std::set , std::multimap ) {
+  bool C1 = M.count(1001);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = M.contains(1001);
+  bool C2 = US.count(1002);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C2 = 

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 387926.
avogelsgesang marked an inline comment as not done.
avogelsgesang added a comment.

Fix test case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
@@ -0,0 +1,192 @@
+// RUN: %check_clang_tidy -std=c++20 %s readability-container-contains %t
+
+// Some *very* simplified versions of `map` etc.
+namespace std {
+
+template 
+struct map {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+  void *find(const Key );
+  void *end();
+};
+
+template 
+struct set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct unordered_set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct multimap {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+} // namespace std
+
+// Check that we detect various common ways to check for membership
+int testDifferentCheckTypes(std::map ) {
+  if (MyMap.count(0))
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use 'contains' to check for membership [readability-container-contains]
+// CHECK-FIXES: if (MyMap.contains(0))
+return 1;
+  bool C1 = MyMap.count(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = MyMap.contains(1);
+  auto C2 = static_cast(MyMap.count(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C2 = static_cast(MyMap.contains(1));
+  auto C3 = MyMap.count(2) != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C3 = MyMap.contains(2);
+  auto C4 = MyMap.count(3) > 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C4 = MyMap.contains(3);
+  auto C5 = MyMap.count(4) >= 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C5 = MyMap.contains(4);
+  auto C6 = MyMap.find(5) != MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C6 = MyMap.contains(5);
+  return C1 + C2 + C3 + C4 + C5 + C6;
+}
+
+// Check that we detect various common ways to check for non-membership
+int testNegativeChecks(std::map ) {
+  bool C1 = !MyMap.count(-1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = !MyMap.contains(-1);
+  auto C2 = MyMap.count(-2) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C2 = !MyMap.contains(-2);
+  auto C3 = MyMap.count(-3) <= 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C3 = !MyMap.contains(-3);
+  auto C4 = MyMap.count(-4) < 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C4 = !MyMap.contains(-4);
+  auto C5 = MyMap.find(-5) == MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C5 = !MyMap.contains(-5);
+  return C1 + C2 + C3 + C4 + C5;
+}
+
+// Check for various types
+int testDifferentTypes(std::map , std::unordered_set , std::set , std::multimap ) {
+  bool C1 = M.count(1001);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = M.contains(1001);
+  bool C2 = US.count(1002);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C2 = 

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang marked 2 inline comments as not done.
avogelsgesang added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:105-108
+  const auto *PositiveCheck = Result.Nodes.getNodeAs("positive");
+  const auto *NegativeCheck = Result.Nodes.getNodeAs("negative");
+  bool Negated = NegativeCheck != nullptr;
+  const auto *Check = Negated ? NegativeCheck : PositiveCheck;

whisperity wrote:
> `Comparison` instead of `Check`? These should be matching the 
> `binaryOperator`, right?
In most cases, they match the `binaryOperator`. For the first pattern they 
match the `implicitCastExpr`, though

Given this is not always a `binaryOperator`, should I still rename it to 
`Comparison`?



Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:110
+
+  // Diagnose the issue
+  auto Diag =

whisperity wrote:
> I'm not sure if these comments are useful, though. The business logic flow of 
> the implementation is straightforward.
Agree, not sure how much value they provide.
Let me know if I should delete them, or if we want to keep them for the 
structure they introduce...



Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:113
+  diag(Call->getExprLoc(),
+   "use `contains` instead of `count` to check for containment");
+

whisperity wrote:
> whisperity wrote:
> > This might be a bit nitpicking, but `containment` sounds off here: it 
> > usually comes up with regards to superset/subset relationships. Perhaps 
> > phrasing in `membership` or `element` somehow would ease this.
> We use single apostrophe (`'`) instead of backtick (`) in the diagnostics for 
> symbol names.
> but containment sounds off here

Agree. Thanks for catching this!



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst:6
+
+Finds usages of `container.count()` and `container.find() == container.end()` 
which should be replaced by a call to the `container.contains()` method 
introduced in C++ 20.
+

whisperity wrote:
> whisperity wrote:
> > Same comment about the backtick count and how you would like the rendering 
> > to be. Please build the documentation locally and verify visually, as both 
> > ways most likely compile without warnings or errors.
> 
> Please build the documentation locally [...]

How do I actually do that?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:33-35
+
+using namespace std;
+

whisperity wrote:
> Tests are to guard our future selves from breaking the system, so perhaps two 
> tests that involve having `std::` typed out, and also using a different 
> container that's not `std::whatever` would be useful.
> 
> 
> 
> Do you think it would be worthwhile to add matching any user-defined object 
> which has a `count()` and a `contains()` method (with the appropriate number 
> of parameters) later? So match not only the few standard containers, but more 
> stuff?
> 
> It doesn't have to be done now, but having a test for `MyContainer` not in 
> `std::` being marked `// NO-WARNING.` or something could indicate that we 
> purposefully don't want to go down that road just yet.
> so perhaps two tests that involve having std:: typed out

rewrote the tests, such that most of them use fully-qualified types. Also added 
a few test cases involving type defs and namespace aliases (this actually 
uncovered a mistake in the matcher)

> Do you think it would be worthwhile to add matching any user-defined object 
> which has a count() and a contains() method (with the appropriate number of 
> parameters) later?

Not sure. At least not for the code base I wrote this check for...

> having a test for MyContainer not in std:: being marked // NO-WARNING. or 
> something could indicate that we purposefully don't want to go down that road 
> just yet

added such a test case and commented it as "not currently supported"




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:111
+  // CHECK-FIXES: return M.count(21);
+}

whisperity wrote:
> Similarly, the test file could use at least some //negative// examples. 
> Things like `count(X) >= 2` and such, to ensure that the matchers aren't 
> inadvertently broken by someone which would result in a lot of false 
> positives in production.
Added a couple of negative test cases. Please let me know if you have 
additional additional test cases in mind which I should also add


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

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

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 387698.
avogelsgesang marked 11 inline comments as done.
avogelsgesang added a comment.

Address review comments by @whisperity:

- "containment" -> "membership"
- "C++ 20" -> "C++20"
- double-backticks in rst files
- additonal test cases (both positive and negative)

Also:

- use `hasUnqualifiedDesugaredType` -> handle typedefs better


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
@@ -0,0 +1,192 @@
+// RUN: %check_clang_tidy -std=c++20 %s readability-container-contains %t
+
+// Some *very* simplified versions of `map` etc.
+namespace std {
+
+template 
+struct map {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+  void *find(const Key );
+  void *end();
+};
+
+template 
+struct set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct unordered_set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct multimap {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+} // namespace std
+
+// Check that we detect various common ways to check for membership
+int testDifferentCheckTypes(std::map ) {
+  if (MyMap.count(0))
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use 'contains' to check for membership [readability-container-contains]
+// CHECK-FIXES: if (MyMap.contains(0))
+return 1;
+  bool C1 = MyMap.count(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = MyMap.contains(1);
+  auto C2 = static_cast(MyMap.count(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C2 = static_cast(MyMap.contains(1));
+  auto C3 = MyMap.count(2) != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C3 = MyMap.contains(2);
+  auto C4 = MyMap.count(3) > 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C4 = MyMap.contains(3);
+  auto C5 = MyMap.count(4) >= 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C5 = MyMap.contains(4);
+  auto C6 = MyMap.find(5) != MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C6 = MyMap.contains(5);
+  return C1 + C2 + C3 + C4 + C5 + C6;
+}
+
+// Check that we detect various common ways to check for non-membership
+int testNegativeChecks(std::map ) {
+  bool C1 = !MyMap.count(-1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = !MyMap.contains(-1);
+  auto C2 = MyMap.count(-2) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C2 = !MyMap.contains(-2);
+  auto C3 = MyMap.count(-3) <= 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C3 = !MyMap.contains(-3);
+  auto C4 = MyMap.count(-4) < 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C4 = !MyMap.contains(-4);
+  auto C5 = MyMap.find(-5) == MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C5 = !MyMap.contains(-5);
+  return C1 + C2 + C3 + C4 + C5;
+}
+
+// Check for various types
+int testDifferentTypes(std::map , std::unordered_set , std::set , std::multimap ) {
+  bool C1 = M.count(1001);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use 'contains' to check for membership [readability-container-contains]
+  // 

[PATCH] D112648: [clang-tidy] Improve fix-its for `modernize-pass-by-value` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added subscribers: ymandel, whisperity, aaron.ballman.
avogelsgesang added a comment.

@ymandel,  @whisperity, @aaron.ballman could one of you review this/point me in 
the direction of a good reviewer for this change?
(Sorry for the spam - I am new to the LLVM project, and I guess I still have to 
learn how to navigate Phabricator/find the correct reviewers for my changes)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112648

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

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

@xazax.hun / @whisperity could you review this change for me, please :) 
Or, alternatively: can you direct me to somebody who could review?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

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

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 387318.
avogelsgesang added a comment.

Update docs to also mention `container.find() != container.end()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
@@ -0,0 +1,111 @@
+// RUN: %check_clang_tidy -std=c++20 %s readability-container-contains %t
+
+// Some *very* simplified versions of `map` etc.
+namespace std {
+
+template 
+struct map {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+  void *find(const Key );
+  void *end();
+};
+
+template 
+struct set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct unordered_set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct multimap {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+} // namespace std
+
+using namespace std;
+
+// Check that we detect various common ways to check for containment
+int testDifferentCheckTypes(map ) {
+  if (MyMap.count(0))
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+// CHECK-FIXES: if (MyMap.contains(0))
+return 1;
+  bool C1 = MyMap.count(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: bool C1 = MyMap.contains(1);
+  auto C2 = static_cast(MyMap.count(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C2 = static_cast(MyMap.contains(1));
+  auto C3 = MyMap.count(2) != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C3 = MyMap.contains(2);
+  auto C4 = MyMap.count(3) > 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C4 = MyMap.contains(3);
+  auto C5 = MyMap.count(4) >= 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C5 = MyMap.contains(4);
+  auto C6 = MyMap.find(5) != MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C6 = MyMap.contains(5);
+  return C1 + C2 + C3 + C4 + C5 + C6;
+}
+
+// Check that we detect various common ways to check for non-containment
+int testNegativeChecks(map ) {
+  bool C1 = !MyMap.count(-1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: bool C1 = !MyMap.contains(-1);
+  auto C2 = MyMap.count(-2) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C2 = !MyMap.contains(-2);
+  auto C3 = MyMap.count(-3) <= 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C3 = !MyMap.contains(-3);
+  auto C4 = MyMap.count(-4) < 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C4 = !MyMap.contains(-4);
+  auto C5 = MyMap.find(-5) == MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C5 = !MyMap.contains(-5);
+  return C1 + C2 + C3 + C4 + C5;
+}
+
+// Check for various types
+int testDifferentTypes(map , unordered_set , set , multimap ) {
+  bool C1 = M.count(1001);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use `contains` instead of `count` to check for containment 

[PATCH] D112647: [clang-apply-replacements] Correctly handle relative paths

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

Thanks for the reviews, @ymandel and @hokein!

What are the next steps to get this landed?
(I am a first time contributor and have no commit access)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112647

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

[PATCH] D112647: [clang-apply-replacements] Correctly handle relative paths

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 386485.
avogelsgesang added a comment.

Don't update WorkingDir through local reference; as requested @ymandel and 
@hokein


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112647

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/basic.h
  
clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file1.yaml
  
clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file2.yaml
  clang-tools-extra/test/clang-apply-replacements/relative-paths.cpp

Index: clang-tools-extra/test/clang-apply-replacements/relative-paths.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/relative-paths.cpp
@@ -0,0 +1,7 @@
+// RUN: mkdir -p %T/Inputs/relative-paths
+// RUN: mkdir -p %T/Inputs/relative-paths/subdir
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/relative-paths/basic.h > %T/Inputs/relative-paths/basic.h
+// RUN: sed "s#\$(path)#%/T/Inputs/relative-paths#" %S/Inputs/relative-paths/file1.yaml > %T/Inputs/relative-paths/file1.yaml
+// RUN: sed "s#\$(path)#%/T/Inputs/relative-paths#" %S/Inputs/relative-paths/file2.yaml > %T/Inputs/relative-paths/file2.yaml
+// RUN: clang-apply-replacements %T/Inputs/relative-paths
+// RUN: FileCheck -input-file=%T/Inputs/relative-paths/basic.h %S/Inputs/relative-paths/basic.h
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file2.yaml
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file2.yaml
@@ -0,0 +1,15 @@
+---
+MainSourceFile: source2.cpp
+Diagnostics:
+  - BuildDirectory: $(path)
+DiagnosticName: test-relative-path
+DiagnosticMessage:
+  Message: Fix
+  FilePath: ./basic.h
+  FileOffset: 148
+  Replacements:
+- FilePath:./basic.h
+  Offset:  298
+  Length:  1
+  ReplacementText: elem
+...
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file1.yaml
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file1.yaml
@@ -0,0 +1,27 @@
+---
+MainSourceFile: source1.cpp
+Diagnostics:
+  - BuildDirectory: $(path)/subdir/
+DiagnosticName: test-relative-path
+DiagnosticMessage:
+  Message: Fix
+  FilePath: ../relative-path.h
+  FileOffset: 242
+  Replacements:
+- FilePath:../basic.h
+  Offset:  242
+  Length:  26
+  ReplacementText: 'auto & elem : ints'
+- FilePath:$(path)/basic.h
+  Offset:  276
+  Length:  22
+  ReplacementText: ''
+- FilePath:../basic.h
+  Offset:  298
+  Length:  1
+  ReplacementText: elem
+- FilePath:../../relative-paths/basic.h
+  Offset:  148
+  Length:  0
+  ReplacementText: 'override '
+...
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/basic.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/basic.h
@@ -0,0 +1,32 @@
+#ifndef BASIC_H
+#define BASIC_H
+
+
+class Parent {
+public:
+  virtual void func() {}
+};
+
+class Derived : public Parent {
+public:
+  virtual void func() {}
+  // CHECK: virtual void func() override {}
+};
+
+extern void ext(int (&)[5], const Parent &);
+
+void func(int t) {
+  int ints[5];
+  for (unsigned i = 0; i < 5; ++i) {
+int  = ints[i];
+e = t;
+// CHECK: for (auto & elem : ints) {
+// CHECK-NEXT: elem = t;
+  }
+
+  Derived d;
+
+  ext(ints, d);
+}
+
+#endif // BASIC_H
Index: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -23,6 +23,7 @@
 #include "clang/Tooling/DiagnosticsYaml.h"
 #include "clang/Tooling/ReplacementsYaml.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
@@ -152,9 +153,13 @@
   DiagReplacements;
 
   auto AddToGroup = [&](const tooling::Replacement ,
-const tooling::TranslationUnitDiagnostics *SourceTU) {
+const tooling::TranslationUnitDiagnostics *SourceTU,
+const llvm::Optional 

[PATCH] D112647: [clang-apply-replacements] Correctly handle relative paths

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

@alexfh, @ymandel: Do you know who could review this change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112647

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

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

gentle ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

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

[PATCH] D112647: [clang-apply-replacements] Correctly handle relative paths

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 384239.
avogelsgesang marked 2 inline comments as done.
avogelsgesang added a comment.

Use `llvm::Optional` instead of pointer as suggested by @ymandel


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112647

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/basic.h
  
clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file1.yaml
  
clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file2.yaml
  clang-tools-extra/test/clang-apply-replacements/relative-paths.cpp

Index: clang-tools-extra/test/clang-apply-replacements/relative-paths.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/relative-paths.cpp
@@ -0,0 +1,7 @@
+// RUN: mkdir -p %T/Inputs/relative-paths
+// RUN: mkdir -p %T/Inputs/relative-paths/subdir
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/relative-paths/basic.h > %T/Inputs/relative-paths/basic.h
+// RUN: sed "s#\$(path)#%/T/Inputs/relative-paths#" %S/Inputs/relative-paths/file1.yaml > %T/Inputs/relative-paths/file1.yaml
+// RUN: sed "s#\$(path)#%/T/Inputs/relative-paths#" %S/Inputs/relative-paths/file2.yaml > %T/Inputs/relative-paths/file2.yaml
+// RUN: clang-apply-replacements %T/Inputs/relative-paths
+// RUN: FileCheck -input-file=%T/Inputs/relative-paths/basic.h %S/Inputs/relative-paths/basic.h
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file2.yaml
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file2.yaml
@@ -0,0 +1,15 @@
+---
+MainSourceFile: source2.cpp
+Diagnostics:
+  - BuildDirectory: $(path)
+DiagnosticName: test-relative-path
+DiagnosticMessage:
+  Message: Fix
+  FilePath: ./basic.h
+  FileOffset: 148
+  Replacements:
+- FilePath:./basic.h
+  Offset:  298
+  Length:  1
+  ReplacementText: elem
+...
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file1.yaml
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file1.yaml
@@ -0,0 +1,27 @@
+---
+MainSourceFile: source1.cpp
+Diagnostics:
+  - BuildDirectory: $(path)/subdir/
+DiagnosticName: test-relative-path
+DiagnosticMessage:
+  Message: Fix
+  FilePath: ../relative-path.h
+  FileOffset: 242
+  Replacements:
+- FilePath:../basic.h
+  Offset:  242
+  Length:  26
+  ReplacementText: 'auto & elem : ints'
+- FilePath:$(path)/basic.h
+  Offset:  276
+  Length:  22
+  ReplacementText: ''
+- FilePath:../basic.h
+  Offset:  298
+  Length:  1
+  ReplacementText: elem
+- FilePath:../../relative-paths/basic.h
+  Offset:  148
+  Length:  0
+  ReplacementText: 'override '
+...
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/basic.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/basic.h
@@ -0,0 +1,32 @@
+#ifndef BASIC_H
+#define BASIC_H
+
+
+class Parent {
+public:
+  virtual void func() {}
+};
+
+class Derived : public Parent {
+public:
+  virtual void func() {}
+  // CHECK: virtual void func() override {}
+};
+
+extern void ext(int (&)[5], const Parent &);
+
+void func(int t) {
+  int ints[5];
+  for (unsigned i = 0; i < 5; ++i) {
+int  = ints[i];
+e = t;
+// CHECK: for (auto & elem : ints) {
+// CHECK-NEXT: elem = t;
+  }
+
+  Derived d;
+
+  ext(ints, d);
+}
+
+#endif // BASIC_H
Index: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -23,6 +23,7 @@
 #include "clang/Tooling/DiagnosticsYaml.h"
 #include "clang/Tooling/ReplacementsYaml.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
@@ -152,9 +153,14 @@
   DiagReplacements;
 
   auto AddToGroup = [&](const tooling::Replacement ,
-const tooling::TranslationUnitDiagnostics *SourceTU) {
+const tooling::TranslationUnitDiagnostics *SourceTU,
+

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

> [...] If we want to follow container-size-empty's convention, we should 
> include the replaced method in the name [...]

Note that this commit is slightly different from `container-size-empty`: it 
doesn't only replace `count(...)` by `contains(...)` but also replaces `find() 
== end()` by `contains`. So I guess the name would have to be 
`container-count-begin-end-contains` or similar... which would be a bit much in 
my opinion


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

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

[PATCH] D112647: [clang-apply-replacements] Correctly handle relative paths

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a subscriber: alexfh.
avogelsgesang added a comment.

>> Those relative paths are meant to be resolved relative to the corresponding 
>> build directory.
>
> Is this behavior documented somewhere?

I couldn't find this documented anywhere. My assumption is based on behavior 
which I observed from clang-tidy. Without this patch, `clang-apply-fixes` 
failed to apply the changes exported by `clang-tidy --export-fixes` due to its 
inability to find some source files referred to through relative paths

It seems the `clang::tooling::Diagnostic::BuildDirectory` was introduced in 
https://reviews.llvm.org/D26137. Maybe @alexfh who reviewed that patch can 
provide some more context here?




Comment at: 
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:159
 // automatically canonicalized.
+auto  = SM.getFileManager().getFileSystemOpts().WorkingDir;
+auto PrevWorkingDir = WorkingDir;

ymandel wrote:
> Why are you capturing this as a reference? This is a subtle and IMO error 
> prone pattern, since it's not obvious in the code below that you're mutating 
> a deeply nested element of the filesystem.  Can  you instead use a local 
> variable and just set this right before you need it?
> Why are you capturing this as a reference?

Mostly to keep the code shorter. Otherwise, I would have to write

```
auto PrevWorkingDir = SM.getFileManager().getFileSystemOpts().WorkingDir;
if (buildDir)
  SM.getFileManager().getFileSystemOpts().WorkingDir = *buildDir;
// [...]
WorkingDir = SM.getFileManager().getFileSystemOpts().WorkingDir;
```

which isn't really DRY.

> Can you instead use a local variable and just set this right before you need 
> it?

I think I don't understand the alternative you are proposing here. Can you 
provide an example?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112647

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

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang marked 2 inline comments as done.
avogelsgesang added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:341-343
+   `cert-exp42-c `_, `bugprone-suspicious-memory-comparison 
`_,
`cert-fio38-c `_, `misc-non-copyable-objects 
`_,
+   `cert-flp37-c `_, `bugprone-suspicious-memory-comparison 
`_,

avogelsgesang wrote:
> whisperity wrote:
> > These are unrelated changes coming from a lack of rebase or a too eager 
> > diff.
> those changes were added by `add_new_check.py`.
> I guess `add_new_check.py` recreates the complete `list.rst` file, and the 
> previously checked in `list.rst` was not in sync with the remaining files...
> 
> Do you want me to remove those changes from this commit?
I removed the other changes from this commit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

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

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 382982.
avogelsgesang added a comment.

Fix formatting; remove unrelated changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
@@ -0,0 +1,111 @@
+// RUN: %check_clang_tidy -std=c++20 %s readability-container-contains %t
+
+// Some *very* simplified versions of `map` etc.
+namespace std {
+
+template 
+struct map {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+  void *find(const Key );
+  void *end();
+};
+
+template 
+struct set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct unordered_set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct multimap {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+} // namespace std
+
+using namespace std;
+
+// Check that we detect various common ways to check for containment
+int testDifferentCheckTypes(map ) {
+  if (MyMap.count(0))
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+// CHECK-FIXES: if (MyMap.contains(0))
+return 1;
+  bool C1 = MyMap.count(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: bool C1 = MyMap.contains(1);
+  auto C2 = static_cast(MyMap.count(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C2 = static_cast(MyMap.contains(1));
+  auto C3 = MyMap.count(2) != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C3 = MyMap.contains(2);
+  auto C4 = MyMap.count(3) > 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C4 = MyMap.contains(3);
+  auto C5 = MyMap.count(4) >= 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C5 = MyMap.contains(4);
+  auto C6 = MyMap.find(5) != MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C6 = MyMap.contains(5);
+  return C1 + C2 + C3 + C4 + C5 + C6;
+}
+
+// Check that we detect various common ways to check for non-containment
+int testNegativeChecks(map ) {
+  bool C1 = !MyMap.count(-1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: bool C1 = !MyMap.contains(-1);
+  auto C2 = MyMap.count(-2) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C2 = !MyMap.contains(-2);
+  auto C3 = MyMap.count(-3) <= 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C3 = !MyMap.contains(-3);
+  auto C4 = MyMap.count(-4) < 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C4 = !MyMap.contains(-4);
+  auto C5 = MyMap.find(-5) == MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto C5 = !MyMap.contains(-5);
+  return C1 + C2 + C3 + C4 + C5;
+}
+
+// Check for various types
+int testDifferentTypes(map , unordered_set , set , multimap ) {
+  bool C1 = M.count(1001);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // 

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:341-343
+   `cert-exp42-c `_, `bugprone-suspicious-memory-comparison 
`_,
`cert-fio38-c `_, `misc-non-copyable-objects 
`_,
+   `cert-flp37-c `_, `bugprone-suspicious-memory-comparison 
`_,

whisperity wrote:
> These are unrelated changes coming from a lack of rebase or a too eager diff.
those changes were added by `add_new_check.py`.
I guess `add_new_check.py` recreates the complete `list.rst` file, and the 
previously checked in `list.rst` was not in sync with the remaining files...

Do you want me to remove those changes from this commit?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

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

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

> Why readability-, if the intent is to make users move to a newer API?

My line of thinking was:

1. The very similar `readability-container-size-empty` pass is also a 
readability pass.
2. The main reason why I want people to use `contains` over `count` is because 
of readability. For this rule, I want to use modern C++20, not because my 
intent is to "modernize" per se, but because my intent is to make the C++ code 
more readable afterwards

That being said, I don't care strongly about `readability-` vs. `modernize-` 
and if you prefer, I can move the check to `modernize`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

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

[PATCH] D112648: [clang-tidy] Improve fix-its for `modernize-pass-by-value` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a reviewer: alexfh.
avogelsgesang added a subscriber: alexfh.
avogelsgesang added a comment.

not sure whom to add as a reviewer.  According to `git log` this check wasn't 
changed for a while...
Adding @alexfh based on `CODE_OWNERS.TXT`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112648

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

[PATCH] D112647: [clang-apply-replacements] Correctly handle relative paths

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added reviewers: ymandel, njames93.
avogelsgesang added a comment.

Adding reviewers based on `git log`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112647

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

[PATCH] D112648: [clang-tidy] Improve fix-its for `modernize-pass-by-value` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang created this revision.
Herald added subscribers: carlosgalvezp, xazax.hun.
avogelsgesang requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

This commit improves the fix-its of `modernize-pass-by-value` in two ways:

1. No longer propose partial fixes. In the presence of `using`/`typedef`, we 
failed to rewrite the function signature but still adjusted the function body. 
This led to incorrect, partial fix-its. Instead, the check now simply doesn't 
offer any fixes at all in such a situation
2. The check unconditionally added a whitespace after the new type name. This 
is necessary, e.g., for LLVM's code style where the `&` is directly attached to 
the variable name. For other code styles, we don't want this whitespace, 
though. The check now checks if a whitespace is already present and if so 
doesn't introduce a 2nd duplicated whitespace


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112648

Files:
  clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
@@ -231,5 +231,33 @@
 
 struct U {
   U(const POD ) : M(M) {}
+  // CHECK-FIXES: U(const POD ) : M(M) {}
   POD M;
 };
+
+// The rewrite can't look through `typedefs` and `using`.
+// Test that we don't partially rewrite one decl without rewriting the other.
+using MovableConstRef = const Movable &;
+struct V {
+  V(MovableConstRef M);
+  // CHECK-FIXES: V(MovableConstRef M);
+  Movable M;
+};
+V::V(const Movable ) : M(M) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move
+// CHECK-FIXES: V::V(const Movable ) : M(M) {}
+
+// Check that we don't insert an unnecessary whitespace if not required
+struct W {
+  // Here we need a whitespace so we don't write `MovableM`
+  W(const Movable );
+  // CHECK-FIXES: W(Movable M);
+  Movable M;
+};
+// Here we don't need an additional whitepace, as there already is a whitespace
+// between `&` and the variable name
+// clang-format off
+W::W(const Movable& M) : M(M) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move
+// CHECK-FIXES: W::W(Movable M) : M(std::move(M)) {}
+// clang-format on
Index: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
@@ -190,24 +190,46 @@
 
   auto Diag = diag(ParamDecl->getBeginLoc(), "pass by value and use std::move");
 
-  // Iterate over all declarations of the constructor.
-  for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) {
-auto ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
-auto RefTL = ParamTL.getAs();
+  // If we received a `const&` type, we need to rewrite the function
+  // declarations
+  if (ParamDecl->getType()->isLValueReferenceType()) {
+// Check if we can succesfully rewrite all declarations of the constructor.
+for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) {
+  auto ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
+  auto RefTL = ParamTL.getAs();
+  if (RefTL.isNull()) {
+// We cannot rewrite this instance. The type is probably hidden behind
+// some `typedef`. Do not offer a fix-it in this case.
+return;
+  }
+}
+// Rewrite all declarations.
+for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) {
+  auto ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
+  auto RefTL = ParamTL.getAs();
 
-// Do not replace if it is already a value, skip.
-if (RefTL.isNull())
-  continue;
+  TypeLoc ValueTL = RefTL.getPointeeLoc();
+  auto TypeRange = CharSourceRange::getTokenRange(ParmDecl->getBeginLoc(),
+  ParamTL.getEndLoc());
+  std::string ValueStr =
+  Lexer::getSourceText(
+  CharSourceRange::getTokenRange(ValueTL.getSourceRange()), SM,
+  getLangOpts())
+  .str();
 
-TypeLoc ValueTL = RefTL.getPointeeLoc();
-auto TypeRange = CharSourceRange::getTokenRange(ParmDecl->getBeginLoc(),
-ParamTL.getEndLoc());
-std::string ValueStr = Lexer::getSourceText(CharSourceRange::getTokenRange(
-ValueTL.getSourceRange()),
-SM, getLangOpts())
-   .str();
-ValueStr += ' ';
-Diag << 

[PATCH] D112647: [clang-apply-replacements] Correctly handle relative paths

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 382741.
avogelsgesang added a comment.

Remove unrelated changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112647

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/basic.h
  
clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file1.yaml
  
clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file2.yaml
  clang-tools-extra/test/clang-apply-replacements/relative-paths.cpp

Index: clang-tools-extra/test/clang-apply-replacements/relative-paths.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/relative-paths.cpp
@@ -0,0 +1,7 @@
+// RUN: mkdir -p %T/Inputs/relative-paths
+// RUN: mkdir -p %T/Inputs/relative-paths/subdir
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/relative-paths/basic.h > %T/Inputs/relative-paths/basic.h
+// RUN: sed "s#\$(path)#%/T/Inputs/relative-paths#" %S/Inputs/relative-paths/file1.yaml > %T/Inputs/relative-paths/file1.yaml
+// RUN: sed "s#\$(path)#%/T/Inputs/relative-paths#" %S/Inputs/relative-paths/file2.yaml > %T/Inputs/relative-paths/file2.yaml
+// RUN: clang-apply-replacements %T/Inputs/relative-paths
+// RUN: FileCheck -input-file=%T/Inputs/relative-paths/basic.h %S/Inputs/relative-paths/basic.h
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file2.yaml
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file2.yaml
@@ -0,0 +1,15 @@
+---
+MainSourceFile: source2.cpp
+Diagnostics:
+  - BuildDirectory: $(path)
+DiagnosticName: test-relative-path
+DiagnosticMessage:
+  Message: Fix
+  FilePath: ./basic.h
+  FileOffset: 148
+  Replacements:
+- FilePath:./basic.h
+  Offset:  298
+  Length:  1
+  ReplacementText: elem
+...
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file1.yaml
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file1.yaml
@@ -0,0 +1,27 @@
+---
+MainSourceFile: source1.cpp
+Diagnostics:
+  - BuildDirectory: $(path)/subdir/
+DiagnosticName: test-relative-path
+DiagnosticMessage:
+  Message: Fix
+  FilePath: ../relative-path.h
+  FileOffset: 242
+  Replacements:
+- FilePath:../basic.h
+  Offset:  242
+  Length:  26
+  ReplacementText: 'auto & elem : ints'
+- FilePath:$(path)/basic.h
+  Offset:  276
+  Length:  22
+  ReplacementText: ''
+- FilePath:../basic.h
+  Offset:  298
+  Length:  1
+  ReplacementText: elem
+- FilePath:../../relative-paths/basic.h
+  Offset:  148
+  Length:  0
+  ReplacementText: 'override '
+...
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/basic.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/basic.h
@@ -0,0 +1,32 @@
+#ifndef BASIC_H
+#define BASIC_H
+
+
+class Parent {
+public:
+  virtual void func() {}
+};
+
+class Derived : public Parent {
+public:
+  virtual void func() {}
+  // CHECK: virtual void func() override {}
+};
+
+extern void ext(int (&)[5], const Parent &);
+
+void func(int t) {
+  int ints[5];
+  for (unsigned i = 0; i < 5; ++i) {
+int  = ints[i];
+e = t;
+// CHECK: for (auto & elem : ints) {
+// CHECK-NEXT: elem = t;
+  }
+
+  Derived d;
+
+  ext(ints, d);
+}
+
+#endif // BASIC_H
Index: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -152,9 +152,15 @@
   DiagReplacements;
 
   auto AddToGroup = [&](const tooling::Replacement ,
-const tooling::TranslationUnitDiagnostics *SourceTU) {
+const tooling::TranslationUnitDiagnostics *SourceTU,
+const std::string *buildDir) {
 // Use the file manager to deduplicate paths. FileEntries are
 // automatically canonicalized.
+auto  = SM.getFileManager().getFileSystemOpts().WorkingDir;
+auto PrevWorkingDir = WorkingDir;
+if (buildDir) {
+  WorkingDir = *buildDir;
+}
 if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
 

[PATCH] D112647: [clang-apply-replacements] Correctly handle relative paths

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang created this revision.
Herald added a subscriber: carlosgalvezp.
avogelsgesang requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

The fixes from the YAML file can refer to relative paths.
Those relative paths are meant to be resolved relative to the
corresponding `build directory`.
However, `clang-apply-replacements` currently interprets all
paths relative to its own working directory. This causes issues,
e.g., when `clang-apply-replacements` is run from outside of
the original build directory.

This commit adjusts `clang-apply-replacements` to take the build
directory into account when resolving relative file paths.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112647

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
  clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/basic.h
  
clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file1.yaml
  
clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file2.yaml
  clang-tools-extra/test/clang-apply-replacements/relative-paths.cpp

Index: clang-tools-extra/test/clang-apply-replacements/relative-paths.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/relative-paths.cpp
@@ -0,0 +1,7 @@
+// RUN: mkdir -p %T/Inputs/relative-paths
+// RUN: mkdir -p %T/Inputs/relative-paths/subdir
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/relative-paths/basic.h > %T/Inputs/relative-paths/basic.h
+// RUN: sed "s#\$(path)#%/T/Inputs/relative-paths#" %S/Inputs/relative-paths/file1.yaml > %T/Inputs/relative-paths/file1.yaml
+// RUN: sed "s#\$(path)#%/T/Inputs/relative-paths#" %S/Inputs/relative-paths/file2.yaml > %T/Inputs/relative-paths/file2.yaml
+// RUN: clang-apply-replacements %T/Inputs/relative-paths
+// RUN: FileCheck -input-file=%T/Inputs/relative-paths/basic.h %S/Inputs/relative-paths/basic.h
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file2.yaml
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file2.yaml
@@ -0,0 +1,15 @@
+---
+MainSourceFile: source2.cpp
+Diagnostics:
+  - BuildDirectory: $(path)
+DiagnosticName: test-relative-path
+DiagnosticMessage:
+  Message: Fix
+  FilePath: ./basic.h
+  FileOffset: 148
+  Replacements:
+- FilePath:./basic.h
+  Offset:  298
+  Length:  1
+  ReplacementText: elem
+...
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file1.yaml
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/file1.yaml
@@ -0,0 +1,27 @@
+---
+MainSourceFile: source1.cpp
+Diagnostics:
+  - BuildDirectory: $(path)/subdir/
+DiagnosticName: test-relative-path
+DiagnosticMessage:
+  Message: Fix
+  FilePath: ../relative-path.h
+  FileOffset: 242
+  Replacements:
+- FilePath:../basic.h
+  Offset:  242
+  Length:  26
+  ReplacementText: 'auto & elem : ints'
+- FilePath:$(path)/basic.h
+  Offset:  276
+  Length:  22
+  ReplacementText: ''
+- FilePath:../basic.h
+  Offset:  298
+  Length:  1
+  ReplacementText: elem
+- FilePath:../../relative-paths/basic.h
+  Offset:  148
+  Length:  0
+  ReplacementText: 'override '
+...
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/basic.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/relative-paths/basic.h
@@ -0,0 +1,32 @@
+#ifndef BASIC_H
+#define BASIC_H
+
+
+class Parent {
+public:
+  virtual void func() {}
+};
+
+class Derived : public Parent {
+public:
+  virtual void func() {}
+  // CHECK: virtual void func() override {}
+};
+
+extern void ext(int (&)[5], const Parent &);
+
+void func(int t) {
+  int ints[5];
+  for (unsigned i = 0; i < 5; ++i) {
+int  = ints[i];
+e = t;
+// CHECK: for (auto & elem : ints) {
+// CHECK-NEXT: elem = t;
+  }
+
+  Derived d;
+
+  ext(ints, d);
+}
+
+#endif // BASIC_H
Index: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
@@ -196,8 +196,9 @@
 auto RefTL = ParamTL.getAs();
 
 // Do not replace if it is already a value, skip.

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang created this revision.
Herald added subscribers: carlosgalvezp, xazax.hun, mgorny.
avogelsgesang requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

This commit introduces a new check `readability-container-contains`
which finds usages of `container.count()` which should be replaced
by a call to the `container.contains()` method introduced in C++ 20.

For containers which permit multiple entries per key (`multimap`,
`multiset`, ...), `contains` is more efficient than `count` because
`count` has to do unnecessary additional work.

While this this performance difference does not exist for containers
with only a single entry per key (`map`, `unordered_map`, ...),
`contains` still conveys the intent better.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112646

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
@@ -0,0 +1,111 @@
+// RUN: %check_clang_tidy -std=c++20 %s readability-container-contains %t
+
+// Some *very* simplified versions of `map` etc.
+namespace std {
+
+template 
+struct map {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+  void *find(const Key );
+  void *end();
+};
+
+template 
+struct set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct unordered_set {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+template 
+struct multimap {
+  unsigned count(const Key ) const;
+  bool contains(const Key ) const;
+};
+
+} // namespace std
+
+using namespace std;
+
+// Check that we detect various common ways to check for containment
+int testDifferentCheckTypes(map ) {
+  if (myMap.count(0))
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: if (myMap.contains(0))
+return 1;
+  bool c1 = myMap.count(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: bool c1 = myMap.contains(1);
+  auto c2 = static_cast(myMap.count(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto c2 = static_cast(myMap.contains(1));
+  auto c3 = myMap.count(2) != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto c3 = myMap.contains(2);
+  auto c4 = myMap.count(3) > 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto c4 = myMap.contains(3);
+  auto c5 = myMap.count(4) >= 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto c5 = myMap.contains(4);
+  auto c6 = myMap.find(5) != myMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto c6 = myMap.contains(5);
+  return c1 + c2 + c3 + c4 + c5 + c6;
+}
+
+// Check that we detect various common ways to check for non-containment
+int testNegativeChecks(map ) {
+  bool c1 = !myMap.count(-1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: bool c1 = !myMap.contains(-1);
+  auto c2 = myMap.count(-2) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto c2 = !myMap.contains(-2);
+  auto c3 = myMap.count(-3) <= 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment [readability-container-contains]
+  // CHECK-FIXES: auto c3 = !myMap.contains(-3);
+  auto c4 = myMap.count(-4) < 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use `contains` instead of `count` to check for containment 

[PATCH] D108696: [Coroutines] [Frontend] Lookup in std namespace first

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added a comment.

Hi @ChuanqiXu

sorry for what might be naive questions, but just to make sure I understand the 
context of this patch correctly:

This patch fixes the look up of the `coroutine_traits`, so that clang now 
search both the `std` and the `std::experimental` namespace. As such, it must 
land before D109433  since otherwise the new 
`std::coroutine_traits` wouldn't be found by the compiler, correct?

Also, does this patch enable me to use `clang-cl` to build programs which use 
the coroutine header from Microsoft's STL 
? Or are there 
still other incompatibilities between clang and the Microsoft STL with regards 
to coroutines?


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

https://reviews.llvm.org/D108696

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

[PATCH] D108696: [Coroutines] [Frontend] Lookup in std namespace first

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11015
+  "coroutine_traits defined in std::experimental namespace is deprecated. "
+  "Considering update libcxx and include  instead of 
.">,
+  InGroup;

Considering update -> Consider updating


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

https://reviews.llvm.org/D108696

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