[PATCH] D24888: [clang-tidy] Use [[clang::suppress]] with cppcoreguidelines-pro-type-reinterpret-cast

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

In D24888#4025382 , @bam wrote:

> I'm sorry, could we bring it to the finish line?
> It's a pity the standard C++ Core Guidelines rule suppression syntax is still 
> not supported, as Clang-tidy is one of the main tools supporting the 
> Guidelines..
>
> Happy new year to all!

Hi @bam, this is currently not on my priority list. Feel free to take over the 
PR!


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

https://reviews.llvm.org/D24888

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

[PATCH] D139170: [X86][clang] Lift _BitInt() supported max width.

[Matthias Gehre via Phabricator via cfe-commits]

mgehre-amd added a comment.

In D139170#3965850 , @FreddyYe wrote:

> In D139170#3965814 , @mgehre-amd 
> wrote:
>
>> Do other targets not support > 128 bit integers, or is this PR only the 
>> first conservative step of lifting the limit?
>
> I temporary only enabled > 128 bit FP conversion for X86 in 
> https://reviews.llvm.org/D137241, since I highly relied on end-to-end tests 
> to implement that pass and I don't have other targets' environment to verify. 
> So same to this patch, I only lift the limit for X86. Hope other targets will 
> follow us to enable for themselves. WDYT?

A right, I was confused because I saw llvm/test/CodeGen/AArch64/O0-pipeline.ll 
running the new pass on AArch64, but actually it doesn't do anything unless 
it's enabled in llvm/lib/Target/*/*ISelLowering.cpp. Got it, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139170

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

[PATCH] D139170: [X86][clang] Lift _BitInt() supported max width.

[Matthias Gehre via Phabricator via cfe-commits]

mgehre-amd added a comment.

Do other targets not support > 128 bit integers, or is this PR only the first 
conservative step of lifting the limit?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139170

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

[PATCH] D122234: [clang] Link libbitint for large division of _BitInt

[Matthias Gehre via Phabricator via cfe-commits]

mgehre-amd abandoned this revision.
mgehre-amd added a comment.

Instead of linking to libbitint, I instead created a backend pass to transform 
div/rem instructions: https://reviews.llvm.org/D130076


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122234

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

[PATCH] D127287: clang: Introduce -fexperimental-max-bitint-width

[Matthias Gehre via Phabricator via cfe-commits]

mgehre-amd closed this revision.
mgehre-amd added a comment.

Merged


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127287

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

[PATCH] D127287: clang: Introduce -fexperimental-max-bitint-width

[Matthias Gehre via Phabricator via cfe-commits]

mgehre-amd added a comment.

Thanks for the quick review!




Comment at: clang/test/Sema/large-bit-int.c:1
+// RUN: %clang_cc1 -fexperimental-max-bitint-width=1024 -fsyntax-only -verify 
%s -Wno-unused -triple x86_64-gnu-linux
+

aaron.ballman wrote:
> Thoughts on adding a frontend diagnostic if the user specifies a value larger 
> than `llvm::IntegerType::MAX_INT_BITS`? I'm on the fence. OTOneH, this gives 
> a good user experience in the event of mistypes in the command line, 
> OTOtherH, it's a cc1-only option that we expect to remove in the (hopefully) 
> near future.
> 
> Also, I don't think the triple is needed and I'm pretty sure `-Wno-unused` is 
> the default already.
I was thinking that this is both only a cc1 option and at the same time 
MAX_INT_BITS is really big, so I don't imagine a practical case where one 
intends to have bigger _BitInts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127287

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

[PATCH] D127287: clang: Introduce -fexperimental-max-bitint-width

[Matthias Gehre via Phabricator via cfe-commits]

mgehre-amd updated this revision to Diff 435161.
mgehre-amd marked 4 inline comments as done.
mgehre-amd added a comment.

Used Optional
Added entry to release notes
Modifed help text according to suggestion
Added comment to LANGOPT that it will be removed in the future


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127287

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Basic/TargetInfo.h
  clang/include/clang/Driver/Options.td
  clang/lib/Basic/TargetInfo.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/test/Sema/large-bit-int.c

Index: clang/test/Sema/large-bit-int.c
===
--- /dev/null
+++ clang/test/Sema/large-bit-int.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -fexperimental-max-bitint-width=1024 -fsyntax-only -verify %s
+
+void f() {
+  _Static_assert(__BITINT_MAXWIDTH__ == 1024, "Macro value is unexpected.");
+
+  _BitInt(1024) a;
+  unsigned _BitInt(1024) b;
+
+  _BitInt(8388609) c;// expected-error {{signed _BitInt of bit sizes greater than 1024 not supported}}
+  unsigned _BitInt(0xFF) d; // expected-error {{unsigned _BitInt of bit sizes greater than 1024 not supported}}
+}
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -314,7 +314,7 @@
 
 #define BENIGN_LANGOPT(Name, Bits, Default, Description)
 #define BENIGN_ENUM_LANGOPT(Name, Type, Bits, Default, Description)
-#define BENIGN_VALUE_LANGOPT(Name, Type, Bits, Default, Description)
+#define BENIGN_VALUE_LANGOPT(Name, Bits, Default, Description)
 #include "clang/Basic/LangOptions.def"
 
   if (ExistingLangOpts.ModuleFeatures != LangOpts.ModuleFeatures) {
Index: clang/lib/Basic/TargetInfo.cpp
===
--- clang/lib/Basic/TargetInfo.cpp
+++ clang/lib/Basic/TargetInfo.cpp
@@ -150,6 +150,9 @@
   PlatformMinVersion = VersionTuple();
 
   MaxOpenCLWorkGroupSize = 1024;
+
+  MaxBitIntWidth.reset();
+
   ProgramAddrSpace = 0;
 }
 
@@ -478,6 +481,9 @@
 Diags.Report(diag::err_opt_not_valid_on_target) << "-fprotect-parens";
 Opts.ProtectParens = false;
   }
+
+  if (Opts.MaxBitIntWidth)
+MaxBitIntWidth = Opts.MaxBitIntWidth;
 }
 
 bool TargetInfo::initFeatureMap(
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -6082,6 +6082,12 @@
 def fobjc_gc : Flag<["-"], "fobjc-gc">, Group,
   HelpText<"Enable Objective-C garbage collection">;
 
+def fexperimental_max_bitint_width_EQ:
+  Joined<["-"], "fexperimental-max-bitint-width=">, Group,
+  MetaVarName<"">,
+  HelpText<"Set the maximum bitwidth for _BitInt (this option is expected to be removed in the future)">,
+  MarshallingInfoInt>;
+
 } // let Flags = [CC1Option, NoDriverOption]
 
 //===--===//
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -31,6 +31,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Frontend/OpenMP/OMPGridValues.h"
+#include "llvm/IR/DerivedTypes.h"
 #include "llvm/Support/DataTypes.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/VersionTuple.h"
@@ -235,6 +236,8 @@
 
   unsigned MaxOpenCLWorkGroupSize;
 
+  Optional MaxBitIntWidth;
+
   Optional DarwinTargetVariantTriple;
 
   // TargetInfo Constructor.  Default initializes all fields.
@@ -595,11 +598,16 @@
   // Different targets may support a different maximum width for the _BitInt
   // type, depending on what operations are supported.
   virtual size_t getMaxBitIntWidth() const {
+// Consider -fexperimental-max-bitint-width= first.
+if (MaxBitIntWidth)
+  return std::min(*MaxBitIntWidth, llvm::IntegerType::MAX_INT_BITS);
+
 // FIXME: this value should be llvm::IntegerType::MAX_INT_BITS, which is
 // maximum bit width that LLVM claims its IR can support. However, most
-// backends currently have a bug where they only support division
-// operations on types that are <= 128 bits and crash otherwise. We're
-// setting the max supported value to 128 to be conservative.
+// backends currently have a bug where they only support float to int
+// conversion (and vice versa) on types that are <= 128 bits and crash
+// otherwise. We're setting the max supported value to 128 to be
+// conservative.
 return 128;
   }
 
Index: clang/include/clang/Basic/LangOptions.def
===
--- 

[PATCH] D122234: [clang] Link libbitint for large division of _BitInt

[Matthias Gehre via Phabricator via cfe-commits]

mgehre-amd marked an inline comment as done.
mgehre-amd added a comment.

I split the introduction of -fexperimental-max-bitint-width into 
https://reviews.llvm.org/D127287 because there is still discussion about the 
bitint libraries in the backend PRs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122234

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

[PATCH] D127287: clang: Introduce -fexperimental-max-bitint-width

[Matthias Gehre via Phabricator via cfe-commits]

mgehre-amd created this revision.
mgehre-amd added a reviewer: aaron.ballman.
Herald added a project: All.
mgehre-amd requested review of this revision.
Herald added a project: clang.

This splits of the introduction of -fexperimental-max-bitint-width 
from https://reviews.llvm.org/D122234
because that PR is still blocked on discussions on the backend side.

I was asked [0] to upstream at least the flag.

[0] 
https://github.com/llvm/llvm-project/commit/09854f2af3b914b616f29cb640bede3a27cf7c4e#commitcomment-75116619


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D127287

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Basic/TargetInfo.h
  clang/include/clang/Driver/Options.td
  clang/lib/Basic/TargetInfo.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/test/Sema/large-bit-int.c

Index: clang/test/Sema/large-bit-int.c
===
--- /dev/null
+++ clang/test/Sema/large-bit-int.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -fexperimental-max-bitint-width=1024 -fsyntax-only -verify %s -Wno-unused -triple x86_64-gnu-linux
+
+void f() {
+  _Static_assert(__BITINT_MAXWIDTH__ == 1024, "Macro value is unexpected.");
+
+  _BitInt(1024) a;
+  unsigned _BitInt(1024) b;
+
+  _BitInt(8388609) c;// expected-error {{signed _BitInt of bit sizes greater than 1024 not supported}}
+  unsigned _BitInt(0xFF) d; // expected-error {{unsigned _BitInt of bit sizes greater than 1024 not supported}}
+}
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -314,7 +314,7 @@
 
 #define BENIGN_LANGOPT(Name, Bits, Default, Description)
 #define BENIGN_ENUM_LANGOPT(Name, Type, Bits, Default, Description)
-#define BENIGN_VALUE_LANGOPT(Name, Type, Bits, Default, Description)
+#define BENIGN_VALUE_LANGOPT(Name, Bits, Default, Description)
 #include "clang/Basic/LangOptions.def"
 
   if (ExistingLangOpts.ModuleFeatures != LangOpts.ModuleFeatures) {
Index: clang/lib/Basic/TargetInfo.cpp
===
--- clang/lib/Basic/TargetInfo.cpp
+++ clang/lib/Basic/TargetInfo.cpp
@@ -150,6 +150,9 @@
   PlatformMinVersion = VersionTuple();
 
   MaxOpenCLWorkGroupSize = 1024;
+
+  MaxBitIntWidth = 0;
+
   ProgramAddrSpace = 0;
 }
 
@@ -478,6 +481,9 @@
 Diags.Report(diag::err_opt_not_valid_on_target) << "-fprotect-parens";
 Opts.ProtectParens = false;
   }
+
+  if (Opts.MaxBitIntWidth)
+MaxBitIntWidth = Opts.MaxBitIntWidth;
 }
 
 bool TargetInfo::initFeatureMap(
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -6082,6 +6082,12 @@
 def fobjc_gc : Flag<["-"], "fobjc-gc">, Group,
   HelpText<"Enable Objective-C garbage collection">;
 
+def fexperimental_max_bitint_width_EQ:
+  Joined<["-"], "fexperimental-max-bitint-width=">, Group,
+  MetaVarName<"">,
+  HelpText<"Set the maximum bitwidth for _BitInt (experimental)">,
+  MarshallingInfoInt, "0">;
+
 } // let Flags = [CC1Option, NoDriverOption]
 
 //===--===//
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -31,6 +31,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Frontend/OpenMP/OMPGridValues.h"
+#include "llvm/IR/DerivedTypes.h"
 #include "llvm/Support/DataTypes.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/VersionTuple.h"
@@ -235,6 +236,8 @@
 
   unsigned MaxOpenCLWorkGroupSize;
 
+  unsigned MaxBitIntWidth;
+
   Optional DarwinTargetVariantTriple;
 
   // TargetInfo Constructor.  Default initializes all fields.
@@ -595,11 +598,16 @@
   // Different targets may support a different maximum width for the _BitInt
   // type, depending on what operations are supported.
   virtual size_t getMaxBitIntWidth() const {
+// Consider -fexperimental-max-bitint-width= first.
+if (MaxBitIntWidth)
+  return std::min(MaxBitIntWidth, llvm::IntegerType::MAX_INT_BITS);
+
 // FIXME: this value should be llvm::IntegerType::MAX_INT_BITS, which is
 // maximum bit width that LLVM claims its IR can support. However, most
-// backends currently have a bug where they only support division
-// operations on types that are <= 128 bits and crash otherwise. We're
-// setting the max supported value to 128 to be conservative.
+// backends currently have a bug where they only support float to int
+// conversion (and vice versa) on types that are <= 128 bits and crash
+// otherwise. We're setting the max supported value to 128 to be
+// conservative.
 

[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size

[Matthias Gehre via Phabricator via cfe-commits]

mgehre-amd added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:315
 #define BENIGN_ENUM_LANGOPT(Name, Type, Bits, Default, Description)
-#define BENIGN_VALUE_LANGOPT(Name, Type, Bits, Default, Description)
+#define BENIGN_VALUE_LANGOPT(Name, Bits, Default, Description)
 #include "clang/Basic/LangOptions.def"

This macro was taking the wrong number of arguments, but nobody noticed because 
there was no `BENIGN_VALUE_LANGOPT` before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122234

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

[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size

[Matthias Gehre via Phabricator via cfe-commits]

mgehre-amd updated this revision to Diff 418861.
mgehre-amd added a comment.
Herald added a subscriber: dexonsmith.

- Remove diagnostics for float-to-bitint
- Remove lexer changes (will be another PR)
- Add -fexperimental-max-bitint-width=N


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122234

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Basic/TargetInfo.h
  clang/include/clang/Driver/Options.td
  clang/lib/Basic/TargetInfo.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/Hexagon.cpp
  clang/lib/Driver/ToolChains/MinGW.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/test/Driver/linux-ld.c
  clang/test/Sema/large-bit-int.c

Index: clang/test/Sema/large-bit-int.c
===
--- /dev/null
+++ clang/test/Sema/large-bit-int.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -fexperimental-max-bitint-width=1024 -fsyntax-only -verify %s -Wno-unused -triple x86_64-gnu-linux
+
+void f() {
+  _Static_assert(__BITINT_MAXWIDTH__ == 1024, "Macro value is unexpected.");
+
+  _BitInt(1024) a;
+  unsigned _BitInt(1024) b;
+  
+  _BitInt(8388609) c;// expected-error {{signed _BitInt of bit sizes greater than 1024 not supported}}
+  unsigned _BitInt(0xFF) d; // expected-error {{unsigned _BitInt of bit sizes greater than 1024 not supported}}
+}
Index: clang/test/Driver/linux-ld.c
===
--- clang/test/Driver/linux-ld.c
+++ clang/test/Driver/linux-ld.c
@@ -174,7 +174,7 @@
 // RUN: --sysroot=%S/Inputs/basic_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-CLANG-NO-LIBGCC-STATIC %s
 // CHECK-CLANG-NO-LIBGCC-STATIC: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
-// CHECK-CLANG-NO-LIBGCC-STATIC: "--start-group" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
+// CHECK-CLANG-NO-LIBGCC-STATIC: "--start-group" "{{[^"]*}}/libclang_rt.bitint{{[^"]*}}" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
 //
 // RUN: %clang -static-pie -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: --target=x86_64-unknown-linux -rtlib=platform --unwindlib=platform \
@@ -189,7 +189,7 @@
 // CHECK-CLANG-LD-STATIC-PIE: "text"
 // CHECK-CLANG-LD-STATIC-PIE: "-m" "elf_x86_64"
 // CHECK-CLANG-LD-STATIC-PIE: "{{.*}}rcrt1.o"
-// CHECK-CLANG-LD-STATIC-PIE: "--start-group" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
+// CHECK-CLANG-LD-STATIC-PIE: "--start-group" "{{[^"]*}}/libclang_rt.bitint{{[^"]*}}" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
 //
 // RUN: %clang -static-pie -pie -no-canonical-prefixes %s -no-pie -### -o %t.o 2>&1 \
 // RUN: --target=x86_64-unknown-linux -rtlib=platform --unwindlib=platform \
@@ -204,7 +204,7 @@
 // CHECK-CLANG-LD-STATIC-PIE-PIE: "text"
 // CHECK-CLANG-LD-STATIC-PIE-PIE: "-m" "elf_x86_64"
 // CHECK-CLANG-LD-STATIC-PIE-PIE: "{{.*}}rcrt1.o"
-// CHECK-CLANG-LD-STATIC-PIE-PIE: "--start-group" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
+// CHECK-CLANG-LD-STATIC-PIE-PIE: "--start-group" "{{[^"]*}}/libclang_rt.bitint{{[^"]*}}" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
 //
 // RUN: %clang -static-pie -static -no-canonical-prefixes %s -no-pie -### -o %t.o 2>&1 \
 // RUN: --target=x86_64-unknown-linux -rtlib=platform --unwindlib=platform \
@@ -219,7 +219,7 @@
 // CHECK-CLANG-LD-STATIC-PIE-STATIC: "text"
 // CHECK-CLANG-LD-STATIC-PIE-STATIC: "-m" "elf_x86_64"
 // CHECK-CLANG-LD-STATIC-PIE-STATIC: "{{.*}}rcrt1.o"
-// CHECK-CLANG-LD-STATIC-PIE-STATIC: "--start-group" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
+// CHECK-CLANG-LD-STATIC-PIE-STATIC: "--start-group" "{{[^"]*}}/libclang_rt.bitint{{[^"]*}}" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
 //
 // RUN: %clang -static-pie -nopie -no-canonical-prefixes %s -no-pie -### -o %t.o 2>&1 \
 // RUN: --target=x86_64-unknown-linux -rtlib=platform \
@@ -318,7 +318,7 @@
 // CHECK-LD-64-STATIC: "-L[[SYSROOT]]/usr/lib/gcc/x86_64-unknown-linux/10.2.0/../../../../x86_64-unknown-linux/lib"
 // CHECK-LD-64-STATIC: "-L[[SYSROOT]]/lib"
 // CHECK-LD-64-STATIC: "-L[[SYSROOT]]/usr/lib"
-// CHECK-LD-64-STATIC: "--start-group" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
+// CHECK-LD-64-STATIC: "--start-group" "{{[^"]*}}/libclang_rt.bitint{{[^"]*}}" "-lgcc" "-lgcc_eh" "-lc" "--end-group"
 
 // RUN: %clang -no-pie -### %s --target=x86_64-unknown-linux -rtlib=platform --unwindlib=platform -shared -static \
 // RUN:   --gcc-toolchain= --sysroot=%S/Inputs/basic_linux_tree 2>&1 | FileCheck --check-prefix=CHECK-LD-SHARED-STATIC %s
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -312,7 +312,7 @@
 
 #define BENIGN_LANGOPT(Name, Bits, Default, Description)
 #define BENIGN_ENUM_LANGOPT(Name, Type, Bits, Default, Description)

[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size

[Matthias Gehre via Phabricator via cfe-commits]

mgehre-amd marked 2 inline comments as done.
mgehre-amd added inline comments.
Herald added a subscriber: MaskRay.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8422
 
+def err_int_to_float_bit_int_max_size : Error<
+  "cannot convert '_BitInt' operands of more than %0 bits to floating point">;

aaron.ballman wrote:
> mgehre-amd wrote:
> > aaron.ballman wrote:
> > > mgehre-amd wrote:
> > > > erichkeane wrote:
> > > > > Can you explain the issue here?  This is supposed to be well-defined 
> > > > > behavior.
> > > > I saw `Assertion LC != RTLIB::UNKNOWN_LIBCALL && "Don't know how to 
> > > > expand this SINT_TO_FP!"' failed.` in the backend. I think we would 
> > > > need to add library calls for floating to bitint (and vice versa) to 
> > > > the bitint library to enable that.
> > > Good catch! I think we'll need to solve that before we can enable wide 
> > > bit-width support (users are going to expect assignment and 
> > > initialization to not crash as those are basic builtin operators). FWIW, 
> > > this is a reproducer from Clang 13 where we still allowed large widths: 
> > > https://godbolt.org/z/hvzWq1KTK
> > I don't think I agree. 
> > 
> > Right now users will get a diagnostic that _BitInt > 128 bit cannot be 
> > used. With this PR, they can use them but get a diagnostic when they try to 
> > convert large _BitInts to floats or back.
> > I think there is huge value in allowing large _BitInts even when float 
> > to/from conversion will cause a clang diagnostic because at least in my use 
> > case that is pretty uncommon,
> > so I propose to move forward with this PR (lifting the limit on _BitInt 
> > width) without having the compiler-rt support for float conversions.
> > Right now users will get a diagnostic that _BitInt > 128 bit cannot be 
> > used. With this PR, they can use them but get a diagnostic when they try to 
> > convert large _BitInts to floats or back.
> 
> This is certainly better than crashing, no doubt.
> 
> > I think there is huge value in allowing large _BitInts even when float 
> > to/from conversion will cause a clang diagnostic because at least in my use 
> > case that is pretty uncommon,
> 
> That's the problem -- this is a new basic datatype; we can't make assumptions 
> that our constituent uses cases are the common pattern. My experience thus 
> far with this feature has been that people are using it for all sorts of 
> reasons in ways I wouldn't have expected, like as a big int, as a regular int 
> that doesn't integer promote, as bit-fields, etc.
> 
> To me, the bar for whether we allow large bit widths than we do today is: do 
> all basic mathematical operators on any bit-width work correctly at runtime 
> without crashing or major compile-time or runtime performance issues for the 
> given target you want to change the limit for? Assignment and conversion are 
> basic mathematical operators I'd definitely expect to work; these aren't 
> weird situations -- assigning a float to an integer happens with some 
> regularity, so there's no reason to assume it won't happen when the integer 
> is a larger bit-precise integer: https://godbolt.org/z/hx5sYbjGK.
> 
> I'd *much* rather slow-roll the feature than have people's first impression 
> be that they can't trust the feature because it's too flaky. The whole point 
> to the `BITINT_MAXWIDTH` macro is to give the user a reliable way to know 
> what bit widths are supported; lying will not do us any favors.
Ok, then let me propose the following:
- Keep the default max. _BitInt size limit at 128 with this PR
- Remove the code that emits the float-to-int/int-to-float warnings for bit 
_BitInts
- Add a cc1 option -fmax-bitint-size=N to allow overwriting the max. _BitInt 
size for tests (so we can test big division even when float-to-int isn't there 
yet)

What do you think?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122234

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

[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size

[Matthias Gehre via Phabricator via cfe-commits]

mgehre-amd marked 3 inline comments as done.
mgehre-amd added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8422
 
+def err_int_to_float_bit_int_max_size : Error<
+  "cannot convert '_BitInt' operands of more than %0 bits to floating point">;

aaron.ballman wrote:
> mgehre-amd wrote:
> > erichkeane wrote:
> > > Can you explain the issue here?  This is supposed to be well-defined 
> > > behavior.
> > I saw `Assertion LC != RTLIB::UNKNOWN_LIBCALL && "Don't know how to expand 
> > this SINT_TO_FP!"' failed.` in the backend. I think we would need to add 
> > library calls for floating to bitint (and vice versa) to the bitint library 
> > to enable that.
> Good catch! I think we'll need to solve that before we can enable wide 
> bit-width support (users are going to expect assignment and initialization to 
> not crash as those are basic builtin operators). FWIW, this is a reproducer 
> from Clang 13 where we still allowed large widths: 
> https://godbolt.org/z/hvzWq1KTK
I don't think I agree. 

Right now users will get a diagnostic that _BitInt > 128 bit cannot be used. 
With this PR, they can use them but get a diagnostic when they try to convert 
large _BitInts to floats or back.
I think there is huge value in allowing large _BitInts even when float to/from 
conversion will cause a clang diagnostic because at least in my use case that 
is pretty uncommon,
so I propose to move forward with this PR (lifting the limit on _BitInt width) 
without having the compiler-rt support for float conversions.



Comment at: clang/lib/Lex/LiteralSupport.cpp:1232
 
+  if (llvm::isPowerOf2_32(radix)) {
+unsigned BitsPerDigit = llvm::Log2(radix);

erichkeane wrote:
> mgehre-amd wrote:
> > erichkeane wrote:
> > > Can you explain what is going on here?  This isn't at all obvious to me.
> > When I adapted the test case  clang/test/Lexer/bitint-constants.c 
> > https://reviews.llvm.org/D122234#change-GjEFAAe2jzfe to test literals with 
> > 2097152 hex digits, clang spend minutes in the code below to try to turn 
> > that literal into a ApInt of 8388608 bits.
> > 
> > That is because the code below does a generic `Val *= Radix` and `Val += 
> > Digit`. The `Val *= Radix` is very slow on a ApInt with 8388608 bits, and 
> > it's also not needed when
> > we know that `Radix` is a power of two. Then one can just copy the bits 
> > from each Digit into the right position in Val.
> > 
> > If the integer literals can just be 128 bits or something, it doesn't 
> > matter, but now we need to support really big integer literals with having 
> > increased the limit on _BitInt width.
> Hmm, so this is only really an issue with base-10 integer values?  Is the 
> same 'clang spent minutes in...' still a problem for base 10 literals?  It 
> would be unfortunate if we cannot just fix that.
> 
> Additionally, I'd like some comment to explain this, it isn't particularly 
> clear what it is doing.
Yes, clang is still slow if a user writes a decimal literal to initialize a 
8388608  bit ApInt, but maybe that is just what the called for.
You can also have a very slow compilation when you create complicated recursive 
templates or huge macros; that isn't inherently the compilers fault.

I mainly fixed it for the hex case to be able to write a test case which says 
`this literal is still accepted` and `this literal is too big and cannot be 
represented`.

I will add comments to make the code clearer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122234

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

[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size

[Matthias Gehre via Phabricator via cfe-commits]

mgehre-amd marked an inline comment as done.
mgehre-amd added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8422
 
+def err_int_to_float_bit_int_max_size : Error<
+  "cannot convert '_BitInt' operands of more than %0 bits to floating point">;

erichkeane wrote:
> Can you explain the issue here?  This is supposed to be well-defined behavior.
I saw `Assertion LC != RTLIB::UNKNOWN_LIBCALL && "Don't know how to expand this 
SINT_TO_FP!"' failed.` in the backend. I think we would need to add library 
calls for floating to bitint (and vice versa) to the bitint library to enable 
that.



Comment at: clang/lib/Lex/LiteralSupport.cpp:1232
 
+  if (llvm::isPowerOf2_32(radix)) {
+unsigned BitsPerDigit = llvm::Log2(radix);

erichkeane wrote:
> Can you explain what is going on here?  This isn't at all obvious to me.
When I adapted the test case  clang/test/Lexer/bitint-constants.c 
https://reviews.llvm.org/D122234#change-GjEFAAe2jzfe to test literals with 
2097152 hex digits, clang spend minutes in the code below to try to turn that 
literal into a ApInt of 8388608 bits.

That is because the code below does a generic `Val *= Radix` and `Val += 
Digit`. The `Val *= Radix` is very slow on a ApInt with 8388608 bits, and it's 
also not needed when
we know that `Radix` is a power of two. Then one can just copy the bits from 
each Digit into the right position in Val.

If the integer literals can just be 128 bits or something, it doesn't matter, 
but now we need to support really big integer literals with having increased 
the limit on _BitInt width.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122234

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

[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size

[Matthias Gehre via Phabricator via cfe-commits]

mgehre-amd created this revision.
mgehre-amd added reviewers: aaron.ballman, erichkeane.
Herald added subscribers: luke957, s.egerton, mstorsjo, simoncook, 
fedor.sergeev, dschuff.
Herald added a project: All.
mgehre-amd requested review of this revision.
Herald added subscribers: pcwang-thead, aheejin.
Herald added a project: clang.

According to the RFC [0], this review contains the clang parts of large integer 
divison for _BitInt.

It contains the following parts:

- Driver: Gnu, MinGW: Link libbitint when available
- clang/Basic/TargetInfo: Increase getMaxBitIntWidth to 
llvm::IntegerType::MAX_INT_BITS
- Sema: Diagnose when converting _BitInt to/from floating point for bit width > 
128 because that breaks in the backend
- Lex: Speedup parsing of large integer literals with a power-of-two radix, so 
parsing a hex literal with 2097152 digits (=MAX_INT_BITS) doesn't take forever

[0] 
https://discourse.llvm.org/t/rfc-add-support-for-division-of-large-bitint-builtins-selectiondag-globalisel-clang/60329


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122234

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/TargetInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CommonArgs.h
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/MinGW.cpp
  clang/lib/Lex/LiteralSupport.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/test/CodeGen/ext-int-cc.c
  clang/test/Driver/linux-ld.c
  clang/test/Lexer/bitint-constants.c
  clang/test/Preprocessor/init-aarch64.c
  clang/test/Preprocessor/init.c
  clang/test/Sema/ext-int.c
  clang/test/SemaCXX/ext-int.cpp

Index: clang/test/SemaCXX/ext-int.cpp
===
--- clang/test/SemaCXX/ext-int.cpp
+++ clang/test/SemaCXX/ext-int.cpp
@@ -17,7 +17,7 @@
   unsigned _BitInt(5) e = 5;
   _BitInt(5) unsigned f;
 
-  _BitInt(-3) g; // expected-error{{signed _BitInt of bit sizes greater than 128 not supported}}
+  _BitInt(-3) g; // expected-error{{signed _BitInt of bit sizes greater than 8388608 not supported}}
   _BitInt(0) h; // expected-error{{signed _BitInt must have a bit size of at least 2}}
   _BitInt(1) i; // expected-error{{signed _BitInt must have a bit size of at least 2}}
   _BitInt(2) j;;
@@ -29,12 +29,12 @@
   constexpr _BitInt(7) o = 33;
 
   // Check imposed max size.
-  _BitInt(129) p;   // expected-error {{signed _BitInt of bit sizes greater than 128 not supported}}
-  unsigned _BitInt(0xFF) q; // expected-error {{unsigned _BitInt of bit sizes greater than 128 not supported}}
+  _BitInt(8388609) p;// expected-error {{signed _BitInt of bit sizes greater than 8388608 not supported}}
+  unsigned _BitInt(0xFF) q; // expected-error {{unsigned _BitInt of bit sizes greater than 8388608 not supported}}
 
 // Ensure template params are instantiated correctly.
-  // expected-error@5{{signed _BitInt of bit sizes greater than 128 not supported}}
-  // expected-error@6{{unsigned _BitInt of bit sizes greater than 128 not supported}}
+  // expected-error@5{{signed _BitInt of bit sizes greater than 8388608 not supported}}
+  // expected-error@6{{unsigned _BitInt of bit sizes greater than 8388608 not supported}}
   // expected-note@+1{{in instantiation of template class }}
   HasExtInt<-1> r;
   // expected-error@5{{signed _BitInt must have a bit size of at least 2}}
@@ -285,3 +285,16 @@
 void FromPaper2(_BitInt(8) a1, _BitInt(24) a2) {
   static_assert(is_same::value, "");
 }
+
+void LargeBitIntToFloat(_BitInt(129) a) {
+  float fa = a; // expected-error {{cannot convert '_BitInt' operands of more than 128 bits to floating point}}
+  fa = static_cast(a); // expected-error {{cannot convert '_BitInt' operands of more than 128 bits to floating point}}
+  fa = a + 0.1; // expected-error {{cannot convert '_BitInt' operands of more than 128 bits to floating point}}
+}
+
+_BitInt(129) LargeBitIntFromFloat(float f) {
+  _BitInt(129) a = f; // expected-error {{cannot convert floating point operands to a '_BitInt' of more than 128 bits}}
+  a = f; // expected-error {{cannot convert floating point operands to a '_BitInt' of more than 128 bits}}
+  a = static_cast<_BitInt(129)>(f); // expected-error {{cannot convert floating point operands to a '_BitInt' of more than 128 bits}}
+  return f; // expected-error {{cannot convert floating point operands to a '_BitInt' of more than 128 bits}}
+}
Index: clang/test/Sema/ext-int.c
===
--- clang/test/Sema/ext-int.c
+++ clang/test/Sema/ext-int.c
@@ -73,3 +73,16 @@
 void FromPaper2(_BitInt(8) a1, _BitInt(24) a2) {
   _Static_assert(EXPR_HAS_TYPE(a1 * (_BitInt(32))a2, _BitInt(32)), "");
 }
+
+
+void LargeBitIntToFloat(_BitInt(129) a) {
+  float fa = a; // expected-error {{cannot convert '_BitInt' operands of more than 128 bits to floating point}}

[PATCH] D122227: Fix _BitInt suffix width calculation

[Matthias Gehre via Phabricator via cfe-commits]

mgehre-amd accepted this revision.
mgehre-amd added a comment.

Thanks a lot!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D17

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

[PATCH] D122075: [clang-tidy] Skip parentheses in `readability-make-member-function-const`

[Matthias Gehre via Phabricator via cfe-commits]

mgehre-amd added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122075

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

[PATCH] D77572: [clang-tidy] add new check readability-use-anyofallof

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

Sorry for breaking the build and thanks for the fixes!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77572



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

[PATCH] D77572: [clang-tidy] add new check readability-use-anyofallof

[Matthias Gehre via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGadd51e152aa6: [clang-tidy] add new check 
readability-use-anyofallof (authored by mgehre).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77572

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp
  clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-use-anyofallof.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof-cpp20.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof.cpp
@@ -0,0 +1,183 @@
+// RUN: %check_clang_tidy -std=c++14,c++17 %s readability-use-anyofallof %t
+
+bool good_any_of() {
+  int v[] = {1, 2, 3};
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: replace loop by 'std::any_of()' [readability-use-anyofallof]
+  for (int i : v)
+if (i)
+  return true;
+  return false;
+}
+
+bool cond(int i);
+
+bool good_any_of2() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()'
+int k = i / 2;
+if (cond(k))
+  return true;
+  }
+  return false;
+}
+
+bool good_any_of3() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()'
+if (i == 3)
+  continue;
+if (i)
+  return true;
+  }
+
+  return false;
+}
+
+bool good_any_of_use_external(int comp) {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()'
+if (i == comp)
+  return true;
+  }
+
+  return false;
+}
+
+bool good_any_of_no_cond() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()'
+return true; // Not a real loop, but technically can become any_of.
+  }
+
+  return false;
+}
+
+bool good_any_of_local_modification() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+int j = i;
+j++; // FIXME: Any non-const use disables check.
+if (j > 3)
+  return true;
+  }
+
+  return false;
+}
+
+bool good_any_of_throw() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()'
+if (i > 3)
+  return true;
+if (i == 42)
+  throw 0;
+  }
+
+  return false;
+}
+
+bool bad_any_of1() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+if (i)
+  return false; // bad constant
+  }
+  return false;
+}
+
+bool bad_any_of2() {
+  int v[] = {1, 2, 3};
+  for (int i : v)
+if (i)
+  return true;
+
+  return true; // bad return
+}
+
+bool bad_any_of3() {
+  int v[] = {1, 2, 3};
+  for (int i : v)
+if (i)
+  return true;
+else
+  return i / 2; // bad return
+
+  return false;
+}
+
+bool bad_any_of_control_flow1() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+break; // bad control flow
+if (i)
+  return true;
+  }
+
+  return false;
+}
+
+bool bad_any_of_control_flow2() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+goto end; // bad control flow
+if (i)
+  return true;
+  }
+
+  end:
+  return false;
+}
+
+bool bad_any_of4() {
+  return false; // wrong order
+
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+if (i)
+  return true;
+  }
+}
+
+bool bad_any_of5() {
+  int v[] = {1, 2, 3};
+  int j = 0;
+  for (int i : v) {
+j++; // modifications
+if (i)
+  return true;
+  }
+  return false;
+}
+
+bool bad_any_of6() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+if (i)
+  return true;
+  }
+  int j = 0; // Statements between loop and return
+  j++;
+  return false;
+}
+
+bool bad_any_of7() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+i; // No 'return true' in body.
+  }
+  return false;
+}
+
+bool good_all_of() {
+  int v[] = {1, 2, 3};
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: replace loop by 'std::all_of()' [readability-use-anyofallof]
+  for (int i : v)
+if (i)
+  return false;
+  return true;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof-cpp20.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof-cpp20.cpp
@@ -0,0 +1,19 @@
+// RUN: %check_clang_tidy -std=c++2a-or-later %s readability-use-anyofallof %t
+
+bool good_any_of() {
+  int v[] = {1, 2, 3};

[PATCH] D77572: [clang-tidy] add new check readability-use-anyofallof

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 268120.
mgehre added a comment.

Implemented njames93's comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77572

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp
  clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-use-anyofallof.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof-cpp20.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof.cpp
@@ -0,0 +1,183 @@
+// RUN: %check_clang_tidy -std=c++14,c++17 %s readability-use-anyofallof %t
+
+bool good_any_of() {
+  int v[] = {1, 2, 3};
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: replace loop by 'std::any_of()' [readability-use-anyofallof]
+  for (int i : v)
+if (i)
+  return true;
+  return false;
+}
+
+bool cond(int i);
+
+bool good_any_of2() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()'
+int k = i / 2;
+if (cond(k))
+  return true;
+  }
+  return false;
+}
+
+bool good_any_of3() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()'
+if (i == 3)
+  continue;
+if (i)
+  return true;
+  }
+
+  return false;
+}
+
+bool good_any_of_use_external(int comp) {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()'
+if (i == comp)
+  return true;
+  }
+
+  return false;
+}
+
+bool good_any_of_no_cond() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()'
+return true; // Not a real loop, but technically can become any_of.
+  }
+
+  return false;
+}
+
+bool good_any_of_local_modification() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+int j = i;
+j++; // FIXME: Any non-const use disables check.
+if (j > 3)
+  return true;
+  }
+
+  return false;
+}
+
+bool good_any_of_throw() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()'
+if (i > 3)
+  return true;
+if (i == 42)
+  throw 0;
+  }
+
+  return false;
+}
+
+bool bad_any_of1() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+if (i)
+  return false; // bad constant
+  }
+  return false;
+}
+
+bool bad_any_of2() {
+  int v[] = {1, 2, 3};
+  for (int i : v)
+if (i)
+  return true;
+
+  return true; // bad return
+}
+
+bool bad_any_of3() {
+  int v[] = {1, 2, 3};
+  for (int i : v)
+if (i)
+  return true;
+else
+  return i / 2; // bad return
+
+  return false;
+}
+
+bool bad_any_of_control_flow1() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+break; // bad control flow
+if (i)
+  return true;
+  }
+
+  return false;
+}
+
+bool bad_any_of_control_flow2() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+goto end; // bad control flow
+if (i)
+  return true;
+  }
+
+  end:
+  return false;
+}
+
+bool bad_any_of4() {
+  return false; // wrong order
+
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+if (i)
+  return true;
+  }
+}
+
+bool bad_any_of5() {
+  int v[] = {1, 2, 3};
+  int j = 0;
+  for (int i : v) {
+j++; // modifications
+if (i)
+  return true;
+  }
+  return false;
+}
+
+bool bad_any_of6() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+if (i)
+  return true;
+  }
+  int j = 0; // Statements between loop and return
+  j++;
+  return false;
+}
+
+bool bad_any_of7() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+i; // No 'return true' in body.
+  }
+  return false;
+}
+
+bool good_all_of() {
+  int v[] = {1, 2, 3};
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: replace loop by 'std::all_of()' [readability-use-anyofallof]
+  for (int i : v)
+if (i)
+  return false;
+  return true;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof-cpp20.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof-cpp20.cpp
@@ -0,0 +1,19 @@
+// RUN: %check_clang_tidy -std=c++2a-or-later %s readability-use-anyofallof %t
+
+bool good_any_of() {
+  int v[] = {1, 2, 3};
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: replace loop by 

[PATCH] D80301: [yaml][clang-tidy] Fix new line YAML serialization

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added inline comments.



Comment at: llvm/lib/Support/YAMLTraits.cpp:904
+   std::string ) {
+  Val.clear();
+  size_t CurrentPos = 0;

DmitryPolukhin wrote:
> mgehre wrote:
> > I wonder whether using StringRef::split() would lead to an easier 
> > implementation 
> > (https://llvm.org/doxygen/classllvm_1_1StringRef.html#af0284e4c41c0e09c0bc4767bc77a899d)
> I'm not sure that it will be easier to read or more efficient 
> (`StringRef::split` will require additional vector).
I don't expect the difference in efficiency to be noticable, but the code would 
look like
```
SmallVector Lines;
StringRef(Val).split(Lines, '\n');
bool First = true;
for (StringRef Line: Lines) {
  if (First)
First = false;
  else
Out << '\n';
  Out << Line;
}
```
which I personally find easier to follow than `CurrentPos`, `LineBreakPos` and 
their arithmetic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80301



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

[PATCH] D77572: [clang-tidy] add new check readability-use-anyofallof

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 266083.
mgehre marked an inline comment as done.
mgehre added a comment.

Fix comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77572

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp
  clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-use-anyofallof.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof-cpp20.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof.cpp
@@ -0,0 +1,183 @@
+// RUN: %check_clang_tidy -std=c++14,c++17 %s readability-use-anyofallof %t
+
+bool good_any_of() {
+  int v[] = {1, 2, 3};
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: replace loop by 'std::any_of()' [readability-use-anyofallof]
+  for (int i : v)
+if (i)
+  return true;
+  return false;
+}
+
+bool cond(int i);
+
+bool good_any_of2() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()'
+int k = i / 2;
+if (cond(k))
+  return true;
+  }
+  return false;
+}
+
+bool good_any_of3() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()'
+if (i == 3)
+  continue;
+if (i)
+  return true;
+  }
+
+  return false;
+}
+
+bool good_any_of_use_external(int comp) {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()'
+if (i == comp)
+  return true;
+  }
+
+  return false;
+}
+
+bool good_any_of_no_cond() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()'
+return true; // Not a real loop, but technically can become any_of.
+  }
+
+  return false;
+}
+
+bool good_any_of_local_modification() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+int j = i;
+j++; // FIXME: Any non-const use disables check.
+if (j > 3)
+  return true;
+  }
+
+  return false;
+}
+
+bool good_any_of_throw() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()'
+if (i > 3)
+  return true;
+if (i == 42)
+  throw 0;
+  }
+
+  return false;
+}
+
+bool bad_any_of1() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+if (i)
+  return false; // bad constant
+  }
+  return false;
+}
+
+bool bad_any_of2() {
+  int v[] = {1, 2, 3};
+  for (int i : v)
+if (i)
+  return true;
+
+  return true; // bad return
+}
+
+bool bad_any_of3() {
+  int v[] = {1, 2, 3};
+  for (int i : v)
+if (i)
+  return true;
+else
+  return i / 2; // bad return
+
+  return false;
+}
+
+bool bad_any_of_control_flow1() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+break; // bad control flow
+if (i)
+  return true;
+  }
+
+  return false;
+}
+
+bool bad_any_of_control_flow2() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+goto end; // bad control flow
+if (i)
+  return true;
+  }
+
+  end:
+  return false;
+}
+
+bool bad_any_of4() {
+  return false; // wrong order
+
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+if (i)
+  return true;
+  }
+}
+
+bool bad_any_of5() {
+  int v[] = {1, 2, 3};
+  int j = 0;
+  for (int i : v) {
+j++; // modifications
+if (i)
+  return true;
+  }
+  return false;
+}
+
+bool bad_any_of6() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+if (i)
+  return true;
+  }
+  int j = 0; // Statements between loop and return
+  j++;
+  return false;
+}
+
+bool bad_any_of7() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+i; // No 'return true' in body.
+  }
+  return false;
+}
+
+bool good_all_of() {
+  int v[] = {1, 2, 3};
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: replace loop by 'std::all_of()' [readability-use-anyofallof]
+  for (int i : v)
+if (i)
+  return false;
+  return true;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof-cpp20.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof-cpp20.cpp
@@ -0,0 +1,19 @@
+// RUN: %check_clang_tidy -std=c++2a-or-later %s readability-use-anyofallof %t
+
+bool good_any_of() {
+  int v[] = {1, 2, 3};
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: replace 

[PATCH] D77572: [clang-tidy] add new check readability-use-anyofallof

[Matthias Gehre via Phabricator via cfe-commits]

mgehre marked 5 inline comments as done.
mgehre added a comment.

Aaron, thank you very much for taking the time to review this PR.




Comment at: clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp:61
+  hasBody(allOf(hasDescendant(returns(true)),
+unless(anyOf(hasDescendant(breakStmt()),
+ hasDescendant(returnsButNotTrue))

aaron.ballman wrote:
> Should we reject other ways to break out of the loop, like `goto` or `throw`?
I think throw statements still can be transformed. We cannot transform `break` 
because the loop is gone and we cannot transform `goto` because we cannot jump 
from the lambda into its caller.
But we can keep `throw` statements because exceptions can propagate from the 
lambda through the algorithm back into the original caller. If we could not 
allow `throw` statements, we would also have to disallow any other kind of call 
statements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77572



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

[PATCH] D80301: [yaml][clang-tidy] Fix new line YAML serialization

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

Thanks for doing this! I didn't work on the YAML parser before, so I cannot 
give formal approval.




Comment at: llvm/lib/Support/YAMLTraits.cpp:904
+   std::string ) {
+  Val.clear();
+  size_t CurrentPos = 0;

I wonder whether using StringRef::split() would lead to an easier 
implementation 
(https://llvm.org/doxygen/classllvm_1_1StringRef.html#af0284e4c41c0e09c0bc4767bc77a899d)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80301



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

[PATCH] D77572: [clang-tidy] add new check readability-use-anyofallof

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

Ping :-)
I'm looking for directions on what the next steps are.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77572



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

[PATCH] D63482: [clang-tidy] Fix the YAML created for checks like modernize-pass-by-value

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

Could be - did it handle it correctly for your include case here?

And if this is a general yaml string thing, shouldn't the replacement of 
newlines (for both ways) happen in the yaml parser/writer instead of 
clang/Tooling?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63482



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

[PATCH] D63482: [clang-tidy] Fix the YAML created for checks like modernize-pass-by-value

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

This change breaks `modernize-use-using` fixits.
The code
`typedef int A, B;`
is replaced into

  using A = int;
  using B = int;

when directly applying fixits with clang-tidy. But it is replaced into

  using A = int;
  
  using B = int;

when applying fixits with `clang-apply-replacements`. 
This is because the check creates two replacements,
the first one being `typedef int A` -> `using A = int`
and the second one being `, B` -> `;\nusing B = int`.

With this change, the newline is duplicated, which introduces an unwanted 
newline.
Any advice how to fix this properly?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63482



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

[PATCH] D77979: [clang-tidy] modernize-use-using: Fix broken fixit with InjectedClassName

[Matthias Gehre via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG145dcef8bdf9: [clang-tidy] modernize-use-using: Fix broken 
fixit with InjectedClassName (authored by mgehre).

Changed prior to commit:
  https://reviews.llvm.org/D77979?vs=256872=260294#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77979

Files:
  clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
  clang/include/clang/AST/PrettyPrinter.h
  clang/lib/AST/TypePrinter.cpp


Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1329,7 +1329,12 @@
 
 void TypePrinter::printInjectedClassNameBefore(const InjectedClassNameType *T,
raw_ostream ) {
-  printTemplateSpecializationBefore(T->getInjectedTST(), OS);
+  if (Policy.PrintInjectedClassNameWithArguments)
+return printTemplateSpecializationBefore(T->getInjectedTST(), OS);
+
+  IncludeStrongLifetimeRAII Strong(Policy);
+  T->getTemplateName().print(OS, Policy);
+  spaceBeforePlaceHolder(OS);
 }
 
 void TypePrinter::printInjectedClassNameAfter(const InjectedClassNameType *T,
Index: clang/include/clang/AST/PrettyPrinter.h
===
--- clang/include/clang/AST/PrettyPrinter.h
+++ clang/include/clang/AST/PrettyPrinter.h
@@ -63,7 +63,7 @@
 MSWChar(LO.MicrosoftExt && !LO.WChar), IncludeNewlines(true),
 MSVCFormatting(false), ConstantsAsWritten(false),
 SuppressImplicitBase(false), FullyQualifiedName(false),
-PrintCanonicalTypes(false) {}
+PrintCanonicalTypes(false), PrintInjectedClassNameWithArguments(true) 
{}
 
   /// Adjust this printing policy for cases where it's known that we're
   /// printing C++ code (for instance, if AST dumping reaches a C++-only
@@ -244,6 +244,11 @@
   /// Whether to print types as written or canonically.
   unsigned PrintCanonicalTypes : 1;
 
+  /// Whether to print an InjectedClassNameType with template arguments or as
+  /// written. When a template argument is unnamed, printing it results in
+  /// invalid C++ code.
+  unsigned PrintInjectedClassNameWithArguments : 1;
+
   /// Callbacks to use to allow the behavior of printing to be customized.
   const PrintingCallbacks *Callbacks = nullptr;
 };
Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
@@ -289,3 +289,16 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: using EnumT2_CheckTypedefImpactFromAnotherFile = enum { ea2, 
eb2 };
 
+template 
+struct InjectedClassName {
+  typedef InjectedClassName b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef'
+  // CHECK-FIXES: using b = InjectedClassName;
+};
+
+template 
+struct InjectedClassNameWithUnnamedArgument {
+  typedef InjectedClassNameWithUnnamedArgument b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef'
+  // CHECK-FIXES: using b = InjectedClassNameWithUnnamedArgument;
+};
Index: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
@@ -58,6 +58,7 @@
   printPolicy.SuppressScope = true;
   printPolicy.ConstantArraySizeAsWritten = true;
   printPolicy.UseVoidForZeroParams = false;
+  printPolicy.PrintInjectedClassNameWithArguments = false;
 
   std::string Type = MatchedDecl->getUnderlyingType().getAsString(printPolicy);
   std::string Name = MatchedDecl->getNameAsString();


Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1329,7 +1329,12 @@
 
 void TypePrinter::printInjectedClassNameBefore(const InjectedClassNameType *T,
raw_ostream ) {
-  printTemplateSpecializationBefore(T->getInjectedTST(), OS);
+  if (Policy.PrintInjectedClassNameWithArguments)
+return printTemplateSpecializationBefore(T->getInjectedTST(), OS);
+
+  IncludeStrongLifetimeRAII Strong(Policy);
+  T->getTemplateName().print(OS, Policy);
+  spaceBeforePlaceHolder(OS);
 }
 
 void TypePrinter::printInjectedClassNameAfter(const InjectedClassNameType *T,
Index: clang/include/clang/AST/PrettyPrinter.h
===
--- clang/include/clang/AST/PrettyPrinter.h
+++ 

[PATCH] D78139: [clang-tidy] modernize-use-using: Fix broken fixit with 'template' keyword

[Matthias Gehre via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0642e5e7a7e5: [clang-tidy] modernize-use-using: Fix broken 
fixit with template keyword (authored by mgehre).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78139

Files:
  clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
  clang/lib/AST/NestedNameSpecifier.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp


Index: clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp
@@ -109,9 +109,9 @@
 struct G { template static T temp; };
 
 template requires requires { typename T::template temp; }
-// expected-note@-1{{because 'typename T::temp' would be invalid: type 
'int' cannot be used prior to '::' because it has no members}}
-// expected-note@-2{{because 'typename T::temp' would be invalid: no 
member named 'temp' in 'D'}}
-// expected-note@-3{{because 'typename T::temp' would be invalid: 
template name refers to non-type template 'G::template temp'}}
+// expected-note@-1{{because 'typename T::template temp' would be 
invalid: type 'int' cannot be used prior to '::' because it has no members}}
+// expected-note@-2{{because 'typename T::template temp' would be 
invalid: no member named 'temp' in 'D'}}
+// expected-note@-3{{because 'typename T::template temp' would be 
invalid: template name refers to non-type template 'G::template temp'}}
 struct r7 {};
 
 using r7i1 = r7; // expected-error{{constraints not satisfied for class 
template 'r7' [with T = int]}}
Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1388,7 +1388,7 @@
 
   if (T->getQualifier())
 T->getQualifier()->print(OS, Policy);
-  OS << T->getIdentifier()->getName();
+  OS << "template " << T->getIdentifier()->getName();
   printTemplateArgumentList(OS, T->template_arguments(), Policy);
   spaceBeforePlaceHolder(OS);
 }
Index: clang/lib/AST/NestedNameSpecifier.cpp
===
--- clang/lib/AST/NestedNameSpecifier.cpp
+++ clang/lib/AST/NestedNameSpecifier.cpp
@@ -311,6 +311,14 @@
   // Print the template argument list.
   printTemplateArgumentList(OS, SpecType->template_arguments(),
 InnerPolicy);
+} else if (const auto *DepSpecType =
+   dyn_cast(T)) {
+  // Print the template name without its corresponding
+  // nested-name-specifier.
+  OS << DepSpecType->getIdentifier()->getName();
+  // Print the template argument list.
+  printTemplateArgumentList(OS, DepSpecType->template_arguments(),
+InnerPolicy);
 } else {
   // Print the type normally
   QualType(T, 0).print(OS, InnerPolicy);
Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
@@ -249,6 +249,17 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: using Nested_t = TwoArgTemplate>, S<(0 < 0), Q>>;
 
+template 
+class TemplateKeyword {
+  typedef typename a::template b<> d;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef'
+  // CHECK-FIXES: using d = typename a::template b<>;
+
+  typedef typename a::template b<>::c d2;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef'
+  // CHECK-FIXES: using d2 = typename a::template b<>::c;
+};
+
 template 
 class Variadic {};
 


Index: clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp
@@ -109,9 +109,9 @@
 struct G { template static T temp; };
 
 template requires requires { typename T::template temp; }
-// expected-note@-1{{because 'typename T::temp' would be invalid: type 'int' cannot be used prior to '::' because it has no members}}
-// expected-note@-2{{because 'typename T::temp' would be invalid: no member named 'temp' in 'D'}}
-// expected-note@-3{{because 'typename T::temp' would be invalid: template name refers to non-type template 'G::template temp'}}
+// expected-note@-1{{because 'typename T::template temp' would be invalid: type 'int' cannot be used prior to '::' because it has no members}}
+// expected-note@-2{{because 'typename 

[PATCH] D46317: [clang-tidy] New check bugprone-map-subscript-operator-lookup

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added inline comments.



Comment at: test/clang-tidy/bugprone-map-subscript-operator-lookup.cpp:60
+}
+
+void noWarning() {

We often have code like
```
auto  = Map[Idx];
if (!V) {
  V = init();
}
use(V);
```
Would that be flagged by this check? Could you add a test for it (possibly with 
`FIXME`)?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D46317



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

[PATCH] D78139: [clang-tidy] modernize-use-using: Fix broken fixit with 'template' keyword

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 257774.
mgehre marked an inline comment as done.
mgehre added a comment.

Implement review comments (Thanks!)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78139

Files:
  clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
  clang/lib/AST/NestedNameSpecifier.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp


Index: clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp
@@ -109,9 +109,9 @@
 struct G { template static T temp; };
 
 template requires requires { typename T::template temp; }
-// expected-note@-1{{because 'typename T::temp' would be invalid: type 
'int' cannot be used prior to '::' because it has no members}}
-// expected-note@-2{{because 'typename T::temp' would be invalid: no 
member named 'temp' in 'D'}}
-// expected-note@-3{{because 'typename T::temp' would be invalid: 
template name refers to non-type template 'G::template temp'}}
+// expected-note@-1{{because 'typename T::template temp' would be 
invalid: type 'int' cannot be used prior to '::' because it has no members}}
+// expected-note@-2{{because 'typename T::template temp' would be 
invalid: no member named 'temp' in 'D'}}
+// expected-note@-3{{because 'typename T::template temp' would be 
invalid: template name refers to non-type template 'G::template temp'}}
 struct r7 {};
 
 using r7i1 = r7; // expected-error{{constraints not satisfied for class 
template 'r7' [with T = int]}}
Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1388,7 +1388,7 @@
 
   if (T->getQualifier())
 T->getQualifier()->print(OS, Policy);
-  OS << T->getIdentifier()->getName();
+  OS << "template " << T->getIdentifier()->getName();
   printTemplateArgumentList(OS, T->template_arguments(), Policy);
   spaceBeforePlaceHolder(OS);
 }
Index: clang/lib/AST/NestedNameSpecifier.cpp
===
--- clang/lib/AST/NestedNameSpecifier.cpp
+++ clang/lib/AST/NestedNameSpecifier.cpp
@@ -311,6 +311,14 @@
   // Print the template argument list.
   printTemplateArgumentList(OS, SpecType->template_arguments(),
 InnerPolicy);
+} else if (const auto *DepSpecType =
+   dyn_cast(T)) {
+  // Print the template name without its corresponding
+  // nested-name-specifier.
+  OS << DepSpecType->getIdentifier()->getName();
+  // Print the template argument list.
+  printTemplateArgumentList(OS, DepSpecType->template_arguments(),
+InnerPolicy);
 } else {
   // Print the type normally
   QualType(T, 0).print(OS, InnerPolicy);
Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
@@ -249,6 +249,17 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: using Nested_t = TwoArgTemplate>, S<(0 < 0), Q>>;
 
+template 
+class TemplateKeyword {
+  typedef typename a::template b<> d;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef'
+  // CHECK-FIXES: using d = typename a::template b<>;
+
+  typedef typename a::template b<>::c d2;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef'
+  // CHECK-FIXES: using d2 = typename a::template b<>::c;
+};
+
 template 
 class Variadic {};
 


Index: clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp
@@ -109,9 +109,9 @@
 struct G { template static T temp; };
 
 template requires requires { typename T::template temp; }
-// expected-note@-1{{because 'typename T::temp' would be invalid: type 'int' cannot be used prior to '::' because it has no members}}
-// expected-note@-2{{because 'typename T::temp' would be invalid: no member named 'temp' in 'D'}}
-// expected-note@-3{{because 'typename T::temp' would be invalid: template name refers to non-type template 'G::template temp'}}
+// expected-note@-1{{because 'typename T::template temp' would be invalid: type 'int' cannot be used prior to '::' because it has no members}}
+// expected-note@-2{{because 'typename T::template temp' would be invalid: no member named 'temp' 

[PATCH] D77572: [clang-tidy] add new check readability-use-anyofallof

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

Thanks for the comments so far.
I'm a bit lost now. Which changes that cannot wait for the next PR do you see 
necessary to get this check merged?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77572



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

[PATCH] D78139: [clang-tidy] modernize-use-using: Fix broken fixit with 'template' keyword

[Matthias Gehre via Phabricator via cfe-commits]

mgehre created this revision.
mgehre added reviewers: aaron.ballman, alexfh, hokein, njames93.
Herald added a subscriber: xazax.hun.
Herald added a project: clang.

Before this PR, `modernize-use-using` would transform the typedef in

  template  class TemplateKeyword {
typedef typename a::template f<> e;
typedef typename a::template f<>::d e2;
  };

into

  template  class TemplateKeyword {
using d = typename a::b<>;
using d2 = typename a::template a::b<>::c;
  };

The first one is missing the `template` keyword,
the second one has an extra `a::` scope. Both result
in compilation errors.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78139

Files:
  clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
  clang/lib/AST/NestedNameSpecifier.cpp
  clang/lib/AST/TypePrinter.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp


Index: clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp
@@ -109,9 +109,9 @@
 struct G { template static T temp; };
 
 template requires requires { typename T::template temp; }
-// expected-note@-1{{because 'typename T::temp' would be invalid: type 
'int' cannot be used prior to '::' because it has no members}}
-// expected-note@-2{{because 'typename T::temp' would be invalid: no 
member named 'temp' in 'D'}}
-// expected-note@-3{{because 'typename T::temp' would be invalid: 
template name refers to non-type template 'G::template temp'}}
+// expected-note@-1{{because 'typename T::template temp' would be 
invalid: type 'int' cannot be used prior to '::' because it has no members}}
+// expected-note@-2{{because 'typename T::template temp' would be 
invalid: no member named 'temp' in 'D'}}
+// expected-note@-3{{because 'typename T::template temp' would be 
invalid: template name refers to non-type template 'G::template temp'}}
 struct r7 {};
 
 using r7i1 = r7; // expected-error{{constraints not satisfied for class 
template 'r7' [with T = int]}}
Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1388,6 +1388,7 @@
 
   if (T->getQualifier())
 T->getQualifier()->print(OS, Policy);
+  OS << "template ";
   OS << T->getIdentifier()->getName();
   printTemplateArgumentList(OS, T->template_arguments(), Policy);
   spaceBeforePlaceHolder(OS);
Index: clang/lib/AST/NestedNameSpecifier.cpp
===
--- clang/lib/AST/NestedNameSpecifier.cpp
+++ clang/lib/AST/NestedNameSpecifier.cpp
@@ -308,6 +308,14 @@
   // nested-name-specifier.
   SpecType->getTemplateName().print(OS, InnerPolicy, true);
 
+  // Print the template argument list.
+  printTemplateArgumentList(OS, SpecType->template_arguments(),
+InnerPolicy);
+} else if (const auto *SpecType =
+   dyn_cast(T)) {
+  // Print the template name without its corresponding
+  // nested-name-specifier.
+  OS << SpecType->getIdentifier()->getName();
   // Print the template argument list.
   printTemplateArgumentList(OS, SpecType->template_arguments(),
 InnerPolicy);
Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
@@ -249,6 +249,17 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: using Nested_t = TwoArgTemplate>, S<(0 < 0), Q>>;
 
+template 
+class TemplateKeyword {
+  typedef typename a::template b<> d;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef'
+  // CHECK-FIXES: using d = typename a::template b<>;
+
+  typedef typename a::template b<>::c d2;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef'
+  // CHECK-FIXES: using d2 = typename a::template b<>::c;
+};
+
 template 
 class Variadic {};
 


Index: clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp
===
--- clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp
+++ clang/test/CXX/expr/expr.prim/expr.prim.req/type-requirement.cpp
@@ -109,9 +109,9 @@
 struct G { template static T temp; };
 
 template requires requires { typename T::template temp; }
-// expected-note@-1{{because 'typename T::temp' would be invalid: type 'int' cannot be used prior to '::' because it has no members}}
-// expected-note@-2{{because 'typename T::temp' would be invalid: no member named 'temp' in 'D'}}
-// 

[PATCH] D77979: [clang-tidy] modernize-use-using: Fix broken fixit with InjectedClassName

[Matthias Gehre via Phabricator via cfe-commits]

mgehre created this revision.
mgehre added reviewers: aaron.ballman, alexfh, hokein, njames93.
Herald added a subscriber: xazax.hun.
Herald added a project: clang.

Before this PR, `modernize-use-using` would transform the typedef in

  template 
  struct InjectedClassName {
typedef InjectedClassName b;
  };

into `using b = InjectedClassName;` and

  template 
  struct InjectedClassNameWithUnnamedArgument {
typedef InjectedClassNameWithUnnamedArgument b;
  };

into `using b = InjectedClassNameWithUnnamedArgument<>;`.
The first fixit is surprising because its different than the code
before, but the second fixit doesn't even compile.

This PR adds an option to the TypePrinter to print InjectedClassNameType without
template parameters (i.e. as written).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77979

Files:
  clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
  clang/include/clang/AST/PrettyPrinter.h
  clang/lib/AST/TypePrinter.cpp


Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1305,7 +1305,12 @@
 
 void TypePrinter::printInjectedClassNameBefore(const InjectedClassNameType *T,
raw_ostream ) {
-  printTemplateSpecializationBefore(T->getInjectedTST(), OS);
+  if (Policy.PrintInjectedClassNameWithArguments)
+return printTemplateSpecializationBefore(T->getInjectedTST(), OS);
+
+  IncludeStrongLifetimeRAII Strong(Policy);
+  T->getTemplateName().print(OS, Policy);
+  spaceBeforePlaceHolder(OS);
 }
 
 void TypePrinter::printInjectedClassNameAfter(const InjectedClassNameType *T,
Index: clang/include/clang/AST/PrettyPrinter.h
===
--- clang/include/clang/AST/PrettyPrinter.h
+++ clang/include/clang/AST/PrettyPrinter.h
@@ -63,7 +63,7 @@
 MSWChar(LO.MicrosoftExt && !LO.WChar), IncludeNewlines(true),
 MSVCFormatting(false), ConstantsAsWritten(false),
 SuppressImplicitBase(false), FullyQualifiedName(false),
-PrintCanonicalTypes(false) {}
+PrintCanonicalTypes(false), PrintInjectedClassNameWithArguments(true) 
{}
 
   /// Adjust this printing policy for cases where it's known that we're
   /// printing C++ code (for instance, if AST dumping reaches a C++-only
@@ -244,6 +244,11 @@
   /// Whether to print types as written or canonically.
   unsigned PrintCanonicalTypes : 1;
 
+  /// Whether to print an InjectedClassNameType with template arguments or as
+  /// written. When a template arguments are unnamed, printing them results in
+  /// invalid C++ code.
+  unsigned PrintInjectedClassNameWithArguments : 1;
+
   /// Callbacks to use to allow the behavior of printing to be customized.
   const PrintingCallbacks *Callbacks = nullptr;
 };
Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
@@ -278,3 +278,16 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: using EnumT2_CheckTypedefImpactFromAnotherFile = enum { ea2, 
eb2 };
 
+template 
+struct InjectedClassName {
+  typedef InjectedClassName b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef'
+  // CHECK-FIXES: using b = InjectedClassName;
+};
+
+template 
+struct InjectedClassNameWithUnnamedArgument {
+  typedef InjectedClassNameWithUnnamedArgument b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef'
+  // CHECK-FIXES: using b = InjectedClassNameWithUnnamedArgument;
+};
Index: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
@@ -58,6 +58,7 @@
   printPolicy.SuppressScope = true;
   printPolicy.ConstantArraySizeAsWritten = true;
   printPolicy.UseVoidForZeroParams = false;
+  printPolicy.PrintInjectedClassNameWithArguments = false;
 
   std::string Type = MatchedDecl->getUnderlyingType().getAsString(printPolicy);
   std::string Name = MatchedDecl->getNameAsString();


Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1305,7 +1305,12 @@
 
 void TypePrinter::printInjectedClassNameBefore(const InjectedClassNameType *T,
raw_ostream ) {
-  printTemplateSpecializationBefore(T->getInjectedTST(), OS);
+  if (Policy.PrintInjectedClassNameWithArguments)
+return 

[PATCH] D77572: [clang-tidy] add new check readability-use-anyofallof

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

Thanks @njames93, this is a good examples which I did not consider yet. I agree 
that only transforming
the second loop would make the code worse.

I would argue that a human seeing a warning on the second loop might also 
realize
that the first loop can be transformed in a similar way, and possibly combine 
them into

  return llvm::all_of(getConds(), [](const Init *Cond) { return Val->Cond(); }) 
&& llvm::all_of(getVals(), [](const Init *Val) { return Val->isConcrete(); });

(I was wondering whether the check should have an option to suggest 
`llvm::all_of` (or `boost::algorithm::all_of`, ...) instead of `std::all_of`, 
but
I thought that this could go into another PR.)

I have the feeling that extracting code into algorithms is generally a hard 
topic,
and automatic fixits would possible give a false sense of automatism (at least 
at the current point).
Your example is a good reminder of that.

Personally, clang-tidy has been a good source of learning C++ best practices. 
And I hope that this clang-tidy check would help
me and my coworkers to remember using those algorithms.

Are you saying that this check should not land unless its scope is extended? 
What would be the minimal scope making this check
worth-while to land?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77572



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

[PATCH] D77680: [clang-tidy] misc-unused-parameters: Don't remove parameter from lambda

[Matthias Gehre via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGeaa55590945a: [clang-tidy] misc-unused-parameters: 
Dont remove parameter from lambda (authored by mgehre).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77680

Files:
  clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
@@ -276,3 +276,13 @@
 // CHECK-FIXES: {{^}}  B(int  /*i*/) : A() {}{{$}}
 };
 } // namespace strict_mode_off
+
+namespace lambda {
+using fn = void(int);
+void f(fn *);
+void test() {
+  // CHECK-MESSAGES: :[[@LINE+2]]:12: warning: parameter 'I' is unused
+  // CHECK-FIXES: {{^}}  f([](int  /*I*/) {
+  f([](int I) { return; });
+}
+} // namespace lambda
Index: clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
@@ -8,6 +8,7 @@
 
 #include "UnusedParametersCheck.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTLambda.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
@@ -141,7 +142,8 @@
   // Cannot remove parameter for non-local functions.
   if (Function->isExternallyVisible() ||
   !Result.SourceManager->isInMainFile(Function->getLocation()) ||
-  !Indexer->getOtherRefs(Function).empty() || isOverrideMethod(Function)) {
+  !Indexer->getOtherRefs(Function).empty() || isOverrideMethod(Function) ||
+  isLambdaCallOperator(Function)) {
 
 // It is illegal to omit parameter name here in C code, so early-out.
 if (!Result.Context->getLangOpts().CPlusPlus)


Index: clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
@@ -276,3 +276,13 @@
 // CHECK-FIXES: {{^}}  B(int  /*i*/) : A() {}{{$}}
 };
 } // namespace strict_mode_off
+
+namespace lambda {
+using fn = void(int);
+void f(fn *);
+void test() {
+  // CHECK-MESSAGES: :[[@LINE+2]]:12: warning: parameter 'I' is unused
+  // CHECK-FIXES: {{^}}  f([](int  /*I*/) {
+  f([](int I) { return; });
+}
+} // namespace lambda
Index: clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
@@ -8,6 +8,7 @@
 
 #include "UnusedParametersCheck.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTLambda.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
@@ -141,7 +142,8 @@
   // Cannot remove parameter for non-local functions.
   if (Function->isExternallyVisible() ||
   !Result.SourceManager->isInMainFile(Function->getLocation()) ||
-  !Indexer->getOtherRefs(Function).empty() || isOverrideMethod(Function)) {
+  !Indexer->getOtherRefs(Function).empty() || isOverrideMethod(Function) ||
+  isLambdaCallOperator(Function)) {
 
 // It is illegal to omit parameter name here in C code, so early-out.
 if (!Result.Context->getLangOpts().CPlusPlus)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77680: [clang-tidy] misc-unused-parameters: Don't remove parameter from lambda

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

In D77680#1968860 , @njames93 wrote:

> Outright disabling for lambdas probably isn't the ideal solution, however 
> it's at least a good stepping stone til a better solution can be found.


I fully agree!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77680



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

[PATCH] D77680: [clang-tidy] misc-unused-parameters: Don't remove parameter from lambda

[Matthias Gehre via Phabricator via cfe-commits]

mgehre created this revision.
mgehre added reviewers: aaron.ballman, alexfh, hokein, njames93.
Herald added a subscriber: xazax.hun.
Herald added a project: clang.
mgehre added a project: clang-tools-extra.
mgehre edited the summary of this revision.

Previously, the check would fix

  using fn = void(int);
  void f(fn *);
  void test() {
// CHECK-MESSAGES: :[[@LINE+2]]:12: warning: parameter 'I' is unused
// CHECK-FIXES: {{^}}  f([](int  /*I*/) {
f([](int I) { return; });
  }

into
`f([]() { return; });` which breaks compilation. Now the check is disabled for 
lambdas.

The AST is not so easy to use. For

  auto l = [](int) {  return;  };
  f(l);

one gets

  `-CallExpr  'void'
   |-ImplicitCastExpr  'void (*)(fn *)' 
   | `-DeclRefExpr  'void (fn *)' lvalue Function 0x55a91a545e28 'f' 
'void (fn *)'
   `-ImplicitCastExpr  'void (*)(int)' 
 `-CXXMemberCallExpr  'void (*)(int)'
   `-MemberExpr  '' .operator void 
(*)(int) 0x55a91a546850
 `-ImplicitCastExpr  'const (lambda at line:6:14)' lvalue 

   `-DeclRefExpr  '(lambda at line:6:14)':'(lambda at 
line:6:14)' lvalue Var 0x55a91a5461c0 'l' '(lambda at line:6:14)':'(lambda at 
line:6:14)'

There is no direct use of the `operator()(int I)` of the lambda, so the 
`!Indexer->getOtherRefs(Function).empty()`
does not fire. In the future, we might be able to use the conversion operator 
`operator void (*)(int)` to mark
the call operator as having an "other ref".


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77680

Files:
  clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
@@ -276,3 +276,13 @@
 // CHECK-FIXES: {{^}}  B(int  /*i*/) : A() {}{{$}}
 };
 } // namespace strict_mode_off
+
+namespace lambda {
+using fn = void(int);
+void f(fn *);
+void test() {
+  // CHECK-MESSAGES: :[[@LINE+2]]:12: warning: parameter 'I' is unused
+  // CHECK-FIXES: {{^}}  f([](int  /*I*/) {
+  f([](int I) { return; });
+}
+} // namespace lambda
Index: clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
@@ -8,6 +8,7 @@
 
 #include "UnusedParametersCheck.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTLambda.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
@@ -141,7 +142,8 @@
   // Cannot remove parameter for non-local functions.
   if (Function->isExternallyVisible() ||
   !Result.SourceManager->isInMainFile(Function->getLocation()) ||
-  !Indexer->getOtherRefs(Function).empty() || isOverrideMethod(Function)) {
+  !Indexer->getOtherRefs(Function).empty() || isOverrideMethod(Function) ||
+  isLambdaCallOperator(Function)) {
 
 // It is illegal to omit parameter name here in C code, so early-out.
 if (!Result.Context->getLangOpts().CPlusPlus)


Index: clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
@@ -276,3 +276,13 @@
 // CHECK-FIXES: {{^}}  B(int  /*i*/) : A() {}{{$}}
 };
 } // namespace strict_mode_off
+
+namespace lambda {
+using fn = void(int);
+void f(fn *);
+void test() {
+  // CHECK-MESSAGES: :[[@LINE+2]]:12: warning: parameter 'I' is unused
+  // CHECK-FIXES: {{^}}  f([](int  /*I*/) {
+  f([](int I) { return; });
+}
+} // namespace lambda
Index: clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
===
--- clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp
@@ -8,6 +8,7 @@
 
 #include "UnusedParametersCheck.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTLambda.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
@@ -141,7 +142,8 @@
   // Cannot remove parameter for non-local functions.
   if (Function->isExternallyVisible() ||
   !Result.SourceManager->isInMainFile(Function->getLocation()) ||
-  !Indexer->getOtherRefs(Function).empty() || isOverrideMethod(Function)) {
+  !Indexer->getOtherRefs(Function).empty() || isOverrideMethod(Function) ||
+  isLambdaCallOperator(Function)) {
 
 // It is illegal to omit parameter name here in C code, 

[PATCH] D77572: [clang-tidy] add new check readability-use-anyofallof

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 255814.
mgehre added a comment.

Review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77572

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp
  clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-use-anyofallof.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof-cpp20.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof.cpp
@@ -0,0 +1,158 @@
+// RUN: %check_clang_tidy -std=c++14,c++17 %s readability-use-anyofallof %t
+
+bool good_any_of() {
+  int v[] = {1, 2, 3};
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: Replace loop by std::any_of() from  [readability-use-anyofallof]
+  for (int i : v)
+if (i)
+  return true;
+  return false;
+}
+
+bool cond(int i);
+
+bool good_any_of2() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Replace loop by std::any_of()
+int k = i / 2;
+if (cond(k))
+  return true;
+  }
+  return false;
+}
+
+bool good_any_of3() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Replace loop by std::any_of()
+if (i == 3)
+  continue;
+if (i)
+  return true;
+  }
+
+  return false;
+}
+
+bool good_any_of_use_external(int comp) {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Replace loop by std::any_of()
+if (i == comp)
+  return true;
+  }
+
+  return false;
+}
+
+bool good_any_of_no_cond() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Replace loop by std::any_of()
+return true; // Not a real loop, but technically can become any_of.
+  }
+
+  return false;
+}
+
+bool good_any_of_local_modification() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+int j = i;
+j++; // FIXME: Any non-const use disables check.
+if (j > 3)
+  return true;
+  }
+
+  return false;
+}
+
+bool bad_any_of1() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+if (i)
+  return false; // bad constant
+  }
+  return false;
+}
+
+bool bad_any_of2() {
+  int v[] = {1, 2, 3};
+  for (int i : v)
+if (i)
+  return true;
+
+  return true; // bad return
+}
+
+bool bad_any_of3() {
+  int v[] = {1, 2, 3};
+  for (int i : v)
+if (i)
+  return true;
+else
+  return i / 2; // bad return
+
+  return false;
+}
+
+bool bad_any_of_control_flow1() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+break; // bad control flow
+if (i)
+  return true;
+  }
+
+  return false;
+}
+
+bool bad_any_of4() {
+  return false; // wrong order
+
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+if (i)
+  return true;
+  }
+}
+
+bool bad_any_of5() {
+  int v[] = {1, 2, 3};
+  int j = 0;
+  for (int i : v) {
+j++; // modifications
+if (i)
+  return true;
+  }
+  return false;
+}
+
+bool bad_any_of6() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+if (i)
+  return true;
+  }
+  int j = 0; // Statements between loop and return
+  j++;
+  return false;
+}
+
+bool bad_any_of7() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+i; // No 'return true' in body.
+  }
+  return false;
+}
+
+bool good_all_of() {
+  int v[] = {1, 2, 3};
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: Replace loop by std::all_of() from  [readability-use-anyofallof]
+  for (int i : v)
+if (i)
+  return false;
+  return true;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof-cpp20.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof-cpp20.cpp
@@ -0,0 +1,19 @@
+// RUN: %check_clang_tidy -std=c++2a-or-later %s readability-use-anyofallof %t
+
+bool good_any_of() {
+  int v[] = {1, 2, 3};
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: Replace loop by std::ranges::any_of() from 
+  for (int i : v)
+if (i)
+  return true;
+  return false;
+}
+
+bool good_all_of() {
+  int v[] = {1, 2, 3};
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: Replace loop by std::ranges::all_of() from 
+  for (int i : v)
+if (i)
+  return false;
+  return true;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-use-anyofallof.rst
===
--- /dev/null

[PATCH] D77572: [clang-tidy] add new check readability-use-anyofallof

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 255812.
mgehre added a comment.

Review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77572

Files:
  clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp
  clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst

Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -132,8 +132,8 @@
 - New :doc:`readability-use-anyofallof
   ` check.
 
-  Finds range-based for loops that can be replaced by a call to std::any_of or
-  std::all_of.
+  Finds range-based for loops that can be replaced by a call to ``std::any_of``
+  or ``std::all_of``.
 
 New check aliases
 ^
Index: clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.h
===
--- clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.h
+++ clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.h
@@ -24,19 +24,14 @@
 /// http://clang.llvm.org/extra/clang-tidy/checks/readability-use-anyofallof.html
 class UseAnyOfAllOfCheck : public ClangTidyCheck {
 public:
-  UseAnyOfAllOfCheck(StringRef Name, ClangTidyContext *Context)
-  : ClangTidyCheck(Name, Context),
-IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
-Options.getLocalOrGlobal("IncludeStyle", "llvm"))) {}
+  using ClangTidyCheck::ClangTidyCheck;
+
+  bool isLanguageVersionSupported(const LangOptions ) const override {
+return LangOpts.CPlusPlus;
+  }
 
-  void registerPPCallbacks(const SourceManager , Preprocessor *PP,
-   Preprocessor *ModuleExpanderPP) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult ) override;
-
-private:
-  std::unique_ptr IncludeInserter;
-  const utils::IncludeSorter::IncludeStyle IncludeStyle;
 };
 
 } // namespace readability
Index: clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp
@@ -18,12 +18,13 @@
 using namespace clang::ast_matchers;
 
 namespace clang {
-namespace ast_matchers {
+namespace {
 /// Matches a Stmt whose parent is a CompoundStmt,
 /// and which is directly followed by
 /// a Stmt matching the inner matcher.
-AST_MATCHER_P(Stmt, nextStmt, internal::Matcher, InnerMatcher) {
-  const auto  = Finder->getASTContext().getParents(Node);
+AST_MATCHER_P(Stmt, nextStmt, ast_matchers::internal::Matcher,
+  InnerMatcher) {
+  DynTypedNodeList Parents = Finder->getASTContext().getParents(Node);
   if (Parents.size() != 1)
 return false;
 
@@ -31,30 +32,19 @@
   if (!C)
 return false;
 
-  const auto *I = std::find(C->body_begin(), C->body_end(), );
+  const auto *I = llvm::find(C->body(), );
   assert(I != C->body_end()); // C is parent of Node.
   if (++I == C->body_end())
 return false; // Node is last statement.
 
   return InnerMatcher.matches(**I, Finder, Builder);
 }
-} // namespace ast_matchers
+} // namespace
 
 namespace tidy {
 namespace readability {
 
-void UseAnyOfAllOfCheck::registerPPCallbacks(const SourceManager ,
- Preprocessor *PP,
- Preprocessor *ModuleExpanderPP) {
-  IncludeInserter =
-  std::make_unique(SM, getLangOpts(), IncludeStyle);
-  PP->addPPCallbacks(IncludeInserter->CreatePPCallbacks());
-}
-
 void UseAnyOfAllOfCheck::registerMatchers(MatchFinder *Finder) {
-  if (!getLangOpts().CPlusPlus)
-return;
-
   auto returns = [](bool V) {
 return returnStmt(hasReturnValue(cxxBoolLiteral(equals(V;
   };
@@ -107,7 +97,6 @@
 
 diag(S->getForLoc(), "Replace loop by std%0::any_of() from ")
 << Ranges;
-
   } else if (const auto *S =
  Result.Nodes.getNodeAs("all_of_loop")) {
 if (!isViableLoop(*S, *Result.Context))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77572: [clang-tidy] add new check readability-use-anyofallof

[Matthias Gehre via Phabricator via cfe-commits]

mgehre marked 6 inline comments as done.
mgehre added a comment.

In D77572#1965143 , @njames93 wrote:

> I'm struggling to see the value of this check though. If it was reworked to 
> check for loop in the middle of a function it would have a lot more value.


This is (just) a first step. The next step is to detect the local variable case 
as you also described it. Note that this also catches functions
that do some preprocessing and end with a any_of-style loop.
I also have a local branch that generates fixits, but they add quite some code, 
so I wanted to put them in a separate PR.

In LLVM & clang, the check in this PR already emits 370 unique warnings. 
F11685854: readability-use-anyofallof.log 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77572



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

[PATCH] D77572: [clang-tidy] add new check readability-use-anyofallof

[Matthias Gehre via Phabricator via cfe-commits]

mgehre created this revision.
mgehre added reviewers: aaron.ballman, alexfh, hokein.
Herald added subscribers: xazax.hun, mgorny.
Herald added a project: clang.

Finds range-based for loops that can be replaced by a call to ``std::any_of`` or
``std::all_of``. In C++ 20 mode, suggests ``std::ranges::any_of`` or
``std::ranges::all_of``.
For now, no fixits are produced.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77572

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp
  clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-use-anyofallof.rst
  
clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof-cpp20.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof.cpp
@@ -0,0 +1,158 @@
+// RUN: %check_clang_tidy -std=c++14,c++17 %s readability-use-anyofallof %t
+
+bool good_any_of() {
+  int v[] = {1, 2, 3};
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: Replace loop by std::any_of() from  [readability-use-anyofallof]
+  for (int i : v)
+if (i)
+  return true;
+  return false;
+}
+
+bool cond(int i);
+
+bool good_any_of2() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Replace loop by std::any_of()
+int k = i / 2;
+if (cond(k))
+  return true;
+  }
+  return false;
+}
+
+bool good_any_of3() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Replace loop by std::any_of()
+if (i == 3)
+  continue;
+if (i)
+  return true;
+  }
+
+  return false;
+}
+
+bool good_any_of_use_external(int comp) {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Replace loop by std::any_of()
+if (i == comp)
+  return true;
+  }
+
+  return false;
+}
+
+bool good_any_of_no_cond() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Replace loop by std::any_of()
+return true; // Not a real loop, but technically can become any_of.
+  }
+
+  return false;
+}
+
+bool good_any_of_local_modification() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+int j = i;
+j++; // FIXME: Any non-const use disables check.
+if (j > 3)
+  return true;
+  }
+
+  return false;
+}
+
+bool bad_any_of1() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+if (i)
+  return false; // bad constant
+  }
+  return false;
+}
+
+bool bad_any_of2() {
+  int v[] = {1, 2, 3};
+  for (int i : v)
+if (i)
+  return true;
+
+  return true; // bad return
+}
+
+bool bad_any_of3() {
+  int v[] = {1, 2, 3};
+  for (int i : v)
+if (i)
+  return true;
+else
+  return i / 2; // bad return
+
+  return false;
+}
+
+bool bad_any_of_control_flow1() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+break; // bad control flow
+if (i)
+  return true;
+  }
+
+  return false;
+}
+
+bool bad_any_of4() {
+  return false; // wrong order
+
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+if (i)
+  return true;
+  }
+}
+
+bool bad_any_of5() {
+  int v[] = {1, 2, 3};
+  int j = 0;
+  for (int i : v) {
+j++; // modifications
+if (i)
+  return true;
+  }
+  return false;
+}
+
+bool bad_any_of6() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+if (i)
+  return true;
+  }
+  int j = 0; // Statements between loop and return
+  j++;
+  return false;
+}
+
+bool bad_any_of7() {
+  int v[] = {1, 2, 3};
+  for (int i : v) {
+i; // No 'return true' in body.
+  }
+  return false;
+}
+
+bool good_all_of() {
+  int v[] = {1, 2, 3};
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: Replace loop by std::all_of() from  [readability-use-anyofallof]
+  for (int i : v)
+if (i)
+  return false;
+  return true;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof-cpp20.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof-cpp20.cpp
@@ -0,0 +1,19 @@
+// RUN: %check_clang_tidy -std=c++2a-or-later %s readability-use-anyofallof %t
+
+bool good_any_of() {
+  int v[] = {1, 2, 3};
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: Replace loop by std::ranges::any_of() from 
+  for (int i : v)
+if (i)
+  return true;
+  return false;
+}
+
+bool good_all_of() {
+  int v[] = {1, 2, 3};
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: Replace loop by std::ranges::all_of() from 
+  for (int i 

[PATCH] D72380: [DataFlow] Factor two worklist implementations out

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowWorklist.h:20
+namespace clang {
+template  class DataflowWorklistBase {
+  llvm::BitVector EnqueuedBlocks;

xazax.hun wrote:
> xazax.hun wrote:
> > mgehre wrote:
> > > Should this class have a bit of doxygen and a unit test?
> > We have two users and both users have regression tests. More tests are 
> > always good, but I am not sure if we would get much value in this case. 
> > Having some comments sound very useful though :)
> Added a unit test anyway :)
;-)



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowWorklist.h:20
+namespace clang {
+/// A worklist implementation where the enqueued blocks wwill be dequeued based
+/// on the order defined by 'Comp'.

nit: wwill


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

https://reviews.llvm.org/D72380



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

[PATCH] D72380: [DataFlow] Factor two worklist implementations out

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added inline comments.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowWorklist.h:20
+namespace clang {
+template  class DataflowWorklistBase {
+  llvm::BitVector EnqueuedBlocks;

Should this class have a bit of doxygen and a unit test?


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

https://reviews.llvm.org/D72380



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

[PATCH] D70052: [clang-tidy] Add misc-mutating-copy check

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

Did you consider to warn on copy constructors/assignment operators take a their 
arguments by non-`const` reference, and suggest the user to turn them into 
const references? This seems like a more useful (and easier) check to me.
The link above contains `Ideally, the copy operator should have an idiomatic 
signature. For copy constructors, that is T(const T&); and for copy assignment 
operators, that is T& operator=(const T&);.`.

The only remaining case are then `mutable` members which are quite rare.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70052



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

[PATCH] D70052: [clang-tidy] Add misc-mutating-copy check

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

Have you run your check over the LLVM/clang source base and seen true 
positives/false positives?




Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-mutating-copy.rst:6
+
+Finds assignments to and to direct or indirect members of the copied object 
+in copy constructors and copy assignment operators.

`to and to`


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70052



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

[PATCH] D68074: [clang-tidy] Add readability-make-member-function-const

[Matthias Gehre via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG24130d661ed4: [clang-tidy] Add 
readability-make-member-function-const (authored by mgehre).

Changed prior to commit:
  https://reviews.llvm.org/D68074?vs=225804=228006#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68074

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.cpp
  clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.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-make-member-function-const.rst
  clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp

Index: clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp
@@ -0,0 +1,332 @@
+// RUN: %check_clang_tidy %s readability-make-member-function-const %t
+
+struct Str {
+  void const_method() const;
+  void non_const_method();
+};
+
+namespace Diagnose {
+struct A;
+
+void free_const_use(const A *);
+void free_const_use(const A &);
+
+struct A {
+  int M;
+  const int ConstM;
+  struct {
+int M;
+  } Struct;
+  Str S;
+  Str 
+
+  void already_const() const;
+
+  int read_field() {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_field' can be made const
+// CHECK-FIXES: {{^}}  int read_field() const {
+return M;
+  }
+
+  int read_struct_field() {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_struct_field' can be made const
+// CHECK-FIXES: {{^}}  int read_struct_field() const {
+return Struct.M;
+  }
+
+  int read_const_field() {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_const_field' can be made const
+// CHECK-FIXES: {{^}}  int read_const_field() const {
+return ConstM;
+  }
+
+  void call_const_member() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'call_const_member' can be made const
+// CHECK-FIXES: {{^}}  void call_const_member() const {
+already_const();
+  }
+
+  void call_const_member_on_public_field() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'call_const_member_on_public_field' can be made const
+// CHECK-FIXES: {{^}}  void call_const_member_on_public_field() const {
+S.const_method();
+  }
+
+  void call_const_member_on_public_field_ref() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'call_const_member_on_public_field_ref' can be made const
+// CHECK-FIXES: {{^}}  void call_const_member_on_public_field_ref() const {
+Sref.const_method();
+  }
+
+  const Str _public_field_ref() {
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: method 'return_public_field_ref' can be made const
+// CHECK-FIXES: {{^}}  const Str _public_field_ref() const {
+return S;
+  }
+
+  const A *return_this_const() {
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: method 'return_this_const' can be made const
+// CHECK-FIXES: {{^}}  const A *return_this_const() const {
+return this;
+  }
+
+  const A _this_const_ref() {
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: method 'return_this_const_ref' can be made const
+// CHECK-FIXES: {{^}}  const A _this_const_ref() const {
+return *this;
+  }
+
+  void const_use() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'const_use' can be made const
+// CHECK-FIXES: {{^}}  void const_use() const {
+free_const_use(this);
+  }
+
+  void const_use_ref() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'const_use_ref' can be made const
+// CHECK-FIXES: {{^}}  void const_use_ref() const {
+free_const_use(*this);
+  }
+
+  auto trailingReturn() -> int {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'trailingReturn' can be made const
+// CHECK-FIXES: {{^}}  auto trailingReturn() const -> int {
+return M;
+  }
+
+  int volatileFunction() volatile {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'volatileFunction' can be made const
+// CHECK-FIXES: {{^}}  int volatileFunction() const volatile {
+return M;
+  }
+
+  int restrictFunction() __restrict {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'restrictFunction' can be made const
+// CHECK-FIXES: {{^}}  int restrictFunction() const __restrict {
+return M;
+  }
+
+  int refFunction() & {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'refFunction' can be made const
+// CHECK-FIXES: {{^}}  int refFunction() const & {
+return M;
+  }
+
+  void out_of_line_call_const();
+  // CHECK-FIXES: {{^}}  void out_of_line_call_const() const;
+};
+
+void 

[PATCH] D68074: [clang-tidy] Add readability-make-member-function-const

[Matthias Gehre via Phabricator via cfe-commits]

mgehre marked 8 inline comments as done.
mgehre added a comment.

Thanks, fixed!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68074



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

[PATCH] D68074: [clang-tidy] Add readability-make-member-function-const

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

Friendly Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68074



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

[PATCH] D68074: [clang-tidy] Add readability-make-member-function-const

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 225804.
mgehre marked 6 inline comments as done.
mgehre added a comment.

- Update documentation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68074

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.cpp
  clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.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-make-member-function-const.rst
  clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp

Index: clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp
@@ -0,0 +1,321 @@
+// RUN: %check_clang_tidy %s readability-make-member-function-const %t
+
+struct Str {
+  void const_method() const;
+  void non_const_method();
+};
+
+namespace Diagnose {
+struct A;
+
+void free_const_use(const A *);
+void free_const_use(const A &);
+
+struct A {
+  int M;
+  const int ConstM;
+  struct {
+int M;
+  } Struct;
+  Str S;
+  Str Sref;
+
+  void already_const() const;
+
+  int read_field() {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_field' can be made const
+// CHECK-FIXES: {{^}}  int read_field() const {
+return M;
+  }
+
+  int read_struct_field() {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_struct_field' can be made const
+// CHECK-FIXES: {{^}}  int read_struct_field() const {
+return Struct.M;
+  }
+
+  int read_const_field() {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_const_field' can be made const
+// CHECK-FIXES: {{^}}  int read_const_field() const {
+return ConstM;
+  }
+
+  void call_const_member() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'call_const_member' can be made const
+// CHECK-FIXES: {{^}}  void call_const_member() const {
+already_const();
+  }
+
+  void call_const_member_on_public_field() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'call_const_member_on_public_field' can be made const
+// CHECK-FIXES: {{^}}  void call_const_member_on_public_field() const {
+S.const_method();
+  }
+
+  void call_const_member_on_public_field_ref() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'call_const_member_on_public_field_ref' can be made const
+// CHECK-FIXES: {{^}}  void call_const_member_on_public_field_ref() const {
+Sref.const_method();
+  }
+
+  const Str _public_field_ref() {
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: method 'return_public_field_ref' can be made const
+// CHECK-FIXES: {{^}}  const Str _public_field_ref() const {
+return S;
+  }
+
+  const A *return_this_const() {
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: method 'return_this_const' can be made const
+// CHECK-FIXES: {{^}}  const A *return_this_const() const {
+return this;
+  }
+
+  const A _this_const_ref() {
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: method 'return_this_const_ref' can be made const
+// CHECK-FIXES: {{^}}  const A _this_const_ref() const {
+return *this;
+  }
+
+  void const_use() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'const_use' can be made const
+// CHECK-FIXES: {{^}}  void const_use() const {
+free_const_use(this);
+  }
+
+  void const_use_ref() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'const_use_ref' can be made const
+// CHECK-FIXES: {{^}}  void const_use_ref() const {
+free_const_use(*this);
+  }
+
+  auto trailingReturn() -> int {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'trailingReturn' can be made const
+// CHECK-FIXES: {{^}}  auto trailingReturn() const -> int {
+return M;
+  }
+
+  int volatileFunction() volatile {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'volatileFunction' can be made const
+// CHECK-FIXES: {{^}}  int volatileFunction() const volatile {
+return M;
+  }
+
+  int restrictFunction() __restrict {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'restrictFunction' can be made const
+// CHECK-FIXES: {{^}}  int restrictFunction() const __restrict {
+return M;
+  }
+
+  int refFunction() & {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'refFunction' can be made const
+// CHECK-FIXES: {{^}}  int refFunction() const & {
+return M;
+  }
+
+  void out_of_line_call_const();
+  // CHECK-FIXES: {{^}}  void out_of_line_call_const() const;
+};
+
+void A::out_of_line_call_const() {
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: method 'out_of_line_call_const' can be made const
+  // 

[PATCH] D69145: Give readability-redundant-member-init an option IgnoreBaseInCopyConstructors to avoid breaking code with gcc -Werror=extra

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

Exactly due to the issue you are fixing here, we ended up disabling the 
complete check because we didn't want to live with the warnings it produced on 
-Wextra.
Therefore, I'm actually strongly in favor to enable the option by default.

When others see that clang-tidy fixits introduce warnings (with -Wextra) or 
even break their build (with -Werror), they might not look into check options, 
but just disable the check directly.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D69145



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

[PATCH] D69145: Give readability-redundant-member-init an option IgnoreBaseInCopyConstructors to avoid breaking code with gcc -Werror=extra

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/RedundantMemberInitCheck.cpp:55
   const auto *Construct = 
Result.Nodes.getNodeAs("construct");
+  const auto* ConstructorDecl = Result.Nodes.getNodeAs( 
"constructor" );
+

Extra space around string literal - did you run clang-format on your changes?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-redundant-member-init.rst:30
+within copy constructors. Some compilers issue warnings requiring copy
+constructors to explicitly initialize their base classes.
+

I'd might be helpful to mention `gcc` and `-Wextra` here so users can easily 
identify whether this case applies to them.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D69145



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

[PATCH] D68074: [clang-tidy] Add readability-make-member-function-const

[Matthias Gehre via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-make-member-function-const.rst:10
+The check conservatively tries to preserve logical costness in favor of
+physical costness. The only operations on ``this`` that this check considers
+to preserve logical constness are

gribozavr wrote:
> Sorry, it is unclear to me what it means: "the check [...] tries to do X in 
> favor of Y"
> 
> Also unclear what logical/physical constness mean.
I guess it should read `tries to preserve logical constness instead of physical 
constness.`

logical/physical constness is from here: 
https://isocpp.org/wiki/faq/const-correctness#logical-vs-physical-state
Are there more common terms for this or should I link or copy the explanation?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-make-member-function-const.rst:17
+* returning const-qualified ``this``
+* passing const-qualified ``this`` as a parameter.
+

gribozavr wrote:
> These rules need a justification; if not for the users, but for future 
> maintainers.
> 
> For example, why isn't reading a private member variable OK? Why isn't 
> calling a private member function OK?
> 
Sure! Let's first see if you agree to the rules based on the explanation in my 
other comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68074



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

[PATCH] D68074: [clang-tidy] Add readability-make-member-function-const

[Matthias Gehre via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp:311
+  // This member function must stay non-const, even though
+  // it only calls other private const member functions.
+  int () {

gribozavr wrote:
> Is it because the const version already exists? Then that should be the rule 
> (not calling a private helper function).
Let me try to explain. If it make senses what I say, I'll then update the 
documentation.
The terms logical const and physical const are from here: 
https://isocpp.org/wiki/faq/const-correctness#logical-vs-physical-const

The const version is just here to illustrate how this pattern works. The 
important fact is that a private member function is not part of the public 
interface of the class. 
I.e. users of the class were not able to call `data()` with a const object 
before (even though data() is declared as const) because `data()` is at the 
same time `private`.
We thus cannot assume that the `constness` of `data()` reflects the interface 
contract the the author of the class had in mind.

Here, e.g. the author clearly wanted to only give access to a non-const `int&` 
to users that possses a non-const version of *this.

The rule "don't make the method const if it already has a const overload" seems 
appealing, but its neither sufficient (we still need the rule we currently have 
about private data)
nor is is necessary once we have all other rules. I have successfully run this 
check on the LLVM code base, and there were not issues with clashing overloads 
after fix-its. (It found multiple hundred of valid fixits)

The second example how to see the issue with private data members is the Pimpl 
idiom:
```
class S {
  struct Impl {
   int d;
  };
  Impl *I;
public:
  void set(int v) {
I->d = v;
  }
};
```
A human would not make the `set()` function const because we consider the value 
of `Impl->d` to be part of the value of the instance. Technically, we can make 
`set()` const because
it does not modify `I`. But that is not the author's intention. I try to have 
no false-positives in this check,
so I conservatively assume that an access to a private member that is not of 
builtin type does not preserve logical constness.
Note that the same happens when the type of I is changed to `unique_ptr`. 
`unique_ptr::operator->` is a const member function but returns a reference to 
non-const `Impl`,
and so does not preserve logical constness

We might be able to extend the check by tracking pointer use, but that's pretty 
difficult.
The only extension I did is for builtin types, i.e.`int`. When we read an int 
(in the AST that's an LvalueToRvalue conversion), then there is no way that 
this can eventually lead to a modification
of the state of the int, so it preserves logical constness.

Const use of public members is also ok because the user of the class already 
has access to them. We are not widening the contract by making a member 
function const that
(only) contains const use of public members.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68074



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

[PATCH] D68074: [clang-tidy] Add readability-make-member-function-const

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

In D68074#1683770 , @lebedev.ri wrote:

> Awesome!
>  I believe i have asked this question in the convert-to-static diff - can 
> ExprMutAnalyzer be used here?


I believe in this case, we can get away with a simpler analysis. Because `this` 
is an prvalue,
there are less operations that can be done to it, and it seems easy to just 
list those here. Also, we want
to conservatively disallow "const" access to non-public member 
variables/functions, and I don't see
how this would elegantly integrate with using the ExprMutationAnalyzer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68074



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

[PATCH] D68074: [clang-tidy] Add readability-make-member-function-const

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 222331.
mgehre marked 6 inline comments as done.
mgehre added a comment.

- Implement review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68074

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.cpp
  clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.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-make-member-function-const.rst
  clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp

Index: clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp
@@ -0,0 +1,321 @@
+// RUN: %check_clang_tidy %s readability-make-member-function-const %t
+
+struct Str {
+  void const_method() const;
+  void non_const_method();
+};
+
+namespace Diagnose {
+struct A;
+
+void free_const_use(const A *);
+void free_const_use(const A &);
+
+struct A {
+  int M;
+  const int ConstM;
+  struct {
+int M;
+  } Struct;
+  Str S;
+  Str Sref;
+
+  void already_const() const;
+
+  int read_field() {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_field' can be made const
+// CHECK-FIXES: {{^}}  int read_field() const {
+return M;
+  }
+
+  int read_struct_field() {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_struct_field' can be made const
+// CHECK-FIXES: {{^}}  int read_struct_field() const {
+return Struct.M;
+  }
+
+  int read_const_field() {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_const_field' can be made const
+// CHECK-FIXES: {{^}}  int read_const_field() const {
+return ConstM;
+  }
+
+  void call_const_member() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'call_const_member' can be made const
+// CHECK-FIXES: {{^}}  void call_const_member() const {
+already_const();
+  }
+
+  void call_const_member_on_public_field() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'call_const_member_on_public_field' can be made const
+// CHECK-FIXES: {{^}}  void call_const_member_on_public_field() const {
+S.const_method();
+  }
+
+  void call_const_member_on_public_field_ref() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'call_const_member_on_public_field_ref' can be made const
+// CHECK-FIXES: {{^}}  void call_const_member_on_public_field_ref() const {
+Sref.const_method();
+  }
+
+  const Str _public_field_ref() {
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: method 'return_public_field_ref' can be made const
+// CHECK-FIXES: {{^}}  const Str _public_field_ref() const {
+return S;
+  }
+
+  const A *return_this_const() {
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: method 'return_this_const' can be made const
+// CHECK-FIXES: {{^}}  const A *return_this_const() const {
+return this;
+  }
+
+  const A _this_const_ref() {
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: method 'return_this_const_ref' can be made const
+// CHECK-FIXES: {{^}}  const A _this_const_ref() const {
+return *this;
+  }
+
+  void const_use() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'const_use' can be made const
+// CHECK-FIXES: {{^}}  void const_use() const {
+free_const_use(this);
+  }
+
+  void const_use_ref() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'const_use_ref' can be made const
+// CHECK-FIXES: {{^}}  void const_use_ref() const {
+free_const_use(*this);
+  }
+
+  auto trailingReturn() -> int {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'trailingReturn' can be made const
+// CHECK-FIXES: {{^}}  auto trailingReturn() const -> int {
+return M;
+  }
+
+  int volatileFunction() volatile {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'volatileFunction' can be made const
+// CHECK-FIXES: {{^}}  int volatileFunction() const volatile {
+return M;
+  }
+
+  int restrictFunction() __restrict {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'restrictFunction' can be made const
+// CHECK-FIXES: {{^}}  int restrictFunction() const __restrict {
+return M;
+  }
+
+  int refFunction() & {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'refFunction' can be made const
+// CHECK-FIXES: {{^}}  int refFunction() const & {
+return M;
+  }
+
+  void out_of_line_call_const();
+  // CHECK-FIXES: {{^}}  void out_of_line_call_const() const;
+};
+
+void A::out_of_line_call_const() {
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: method 'out_of_line_call_const' can be made const
+  // 

[PATCH] D68074: [clang-tidy] Add readability-make-member-function-const

[Matthias Gehre via Phabricator via cfe-commits]

mgehre created this revision.
mgehre added reviewers: aaron.ballman, gribozavr, hokein, alexfh.
mgehre added a project: clang.
Herald added subscribers: xazax.hun, mgorny.

Finds non-static member functions that can be made ``const``
because the functions don't use ``this`` in a non-const way.

The check conservatively tries to preserve logical costness in favor of
physical costness. See readability-make-member-function-const.rst for more
details.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68074

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.cpp
  clang-tools-extra/clang-tidy/readability/MakeMemberFunctionConstCheck.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-make-member-function-const.rst
  clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp

Index: clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-make-member-function-const.cpp
@@ -0,0 +1,321 @@
+// RUN: %check_clang_tidy %s readability-make-member-function-const %t
+
+struct Str {
+  void const_method() const;
+  void non_const_method();
+};
+
+namespace Diagnose {
+struct A;
+
+void free_const_use(const A *);
+void free_const_use(const A &);
+
+struct A {
+  int M;
+  const int ConstM;
+  struct {
+int M;
+  } Struct;
+  Str S;
+  Str Sref;
+
+  void already_const() const;
+
+  int read_field() {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_field' can be made const
+// CHECK-FIXES: {{^}}  int read_field() const {
+return M;
+  }
+
+  int read_struct_field() {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_struct_field' can be made const
+// CHECK-FIXES: {{^}}  int read_struct_field() const {
+return Struct.M;
+  }
+
+  int read_const_field() {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'read_const_field' can be made const
+// CHECK-FIXES: {{^}}  int read_const_field() const {
+return ConstM;
+  }
+
+  void call_const_member() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'call_const_member' can be made const
+// CHECK-FIXES: {{^}}  void call_const_member() const {
+already_const();
+  }
+
+  void call_const_member_on_public_field() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'call_const_member_on_public_field' can be made const
+// CHECK-FIXES: {{^}}  void call_const_member_on_public_field() const {
+S.const_method();
+  }
+
+  void call_const_member_on_public_field_ref() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'call_const_member_on_public_field_ref' can be made const
+// CHECK-FIXES: {{^}}  void call_const_member_on_public_field_ref() const {
+Sref.const_method();
+  }
+
+  const Str _public_field_ref() {
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: method 'return_public_field_ref' can be made const
+// CHECK-FIXES: {{^}}  const Str _public_field_ref() const {
+return S;
+  }
+
+  const A *return_this_const() {
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: method 'return_this_const' can be made const
+// CHECK-FIXES: {{^}}  const A *return_this_const() const {
+return this;
+  }
+
+  const A _this_const_ref() {
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: method 'return_this_const_ref' can be made const
+// CHECK-FIXES: {{^}}  const A _this_const_ref() const {
+return *this;
+  }
+
+  void const_use() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'const_use' can be made const
+// CHECK-FIXES: {{^}}  void const_use() const {
+free_const_use(this);
+  }
+
+  void const_use_ref() {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'const_use_ref' can be made const
+// CHECK-FIXES: {{^}}  void const_use_ref() const {
+free_const_use(*this);
+  }
+
+  auto trailingReturn() -> int {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: method 'trailingReturn' can be made const
+// CHECK-FIXES: {{^}}  auto trailingReturn() const -> int {
+return M;
+  }
+
+  int volatileFunction() volatile {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'volatileFunction' can be made const
+// CHECK-FIXES: {{^}}  int volatileFunction() const volatile {
+return M;
+  }
+
+  int restrictFunction() __restrict {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'restrictFunction' can be made const
+// CHECK-FIXES: {{^}}  int restrictFunction() const __restrict {
+return M;
+  }
+
+  int refFunction() & {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'refFunction' can be made const
+// CHECK-FIXES: {{^}}  int refFunction() const & {
+return M;
+  }
+

[PATCH] D66365: [clang-tidy] [readability-convert-member-functions-to-static] ignore functions that hide base class methods

[Matthias Gehre via Phabricator via cfe-commits]

mgehre abandoned this revision.
mgehre added a comment.

The discussions on the bug did not provide further insight (until now).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66365



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

[PATCH] D66806: [LifetimeAnalysis] Fix some false positives

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

Nice! Having no false-positives is most important because this is enabled by 
default.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66806



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

[PATCH] D63325: [Support][Time profiler] Make FE codegen blocks to be inside frontend blocks

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

I'm seeing the same issue `Not all CodeGen sections are inside any Frontend 
section!` with python 3.7.1. Json: F9863382: check-time-trace-sections.json 



Repository:
  rL LLVM

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

https://reviews.llvm.org/D63325



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

[PATCH] D66686: [LifetimeAnalysis] Make it possible to disable the new warnings

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:6651
   visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding,
-Visit);
+Visit, true);
 else

Could we have argument comments here `/*EnableLifetimeWarnings=*/true,
and especially below where multiple bool literals are passed?


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

https://reviews.llvm.org/D66686



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

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

Yes, it means that the automatic inference of Pointer/Owner from base classes 
has the same issues as all of those automatic inferences: Once can construct a 
case where they are not true.
So for having no false-positives at all, we should avoid this inference by 
default and not consider MutableArrayRef to be a gsl::Pointer.
Instead, we disable the analysis when it sees an unannotated class (like here, 
MutableArrayRef).

Eventually, we can explicitly add the correct annotations to the 
MutableArrayRef and OwningArrayRef  source code,
and provide a command line option to opt into this kind of inference with 
possible false-positives.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66486



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

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

When I understand you correctly, you are thinking about
the cases

  OwningArrayRef getRef();
  
  ArrayRef f() {
ArrayRef r = getRef(); // warning: r points into temporary owner
return r;
  }

and

  ArrayRef getRef() {
OwningArrayRef r;
return r;  // warning: returning reference to local variable
  }

which we will both diagnose correctly with the current analysis. In which case 
would our analysis have problems with the fact that ArrayRef is a Pointer and 
the derived OwningArrayRef is a Owner?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66486



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

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

> Also it feels a bit weird to change the ownership semantics in a derived 
> class, I bet that would violate the Liskov substitution principle.

And then we see that llvm::OwningArrayRef inherits from llvm::ArrayRef ... But 
maybe this direction (strengthening a Pointer into an Owner)
is okay. We'll need to go though the call rules, and see if something breaks 
when the caller passes an Owner to a function that takes a base class (i.e. 
Pointer).
My initial reaction is that this should work in general. An Owner is like a 
Pointer that points to {static}.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66486



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

[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

[Matthias Gehre via Phabricator via cfe-commits]

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

Yes, for  libc++ we could add the annotations to its source code, and 
eventually this would be the cleaner solution.
Currently, that is neither sufficient (because one could use libstdc++, MSVC or 
an older libc++ version) nor necessary (the inference we need for libstdc++ / 
MSVC also works for libc++),
so it's currently low on our priority list.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66179



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

[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

[Matthias Gehre via Phabricator via cfe-commits]

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



Comment at: cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp:9
+
+#include "../ASTMatchers/ASTMatchersTest.h"
+#include "clang/ASTMatchers/ASTMatchers.h"

thakis wrote:
> mgehre wrote:
> > thakis wrote:
> > > This weird relative include path is a hint that this isn't the intended 
> > > use :)
> > True. But ...
> > I thought about copying the `matches` function from STMatchersTest.h, but 
> > together with all callees, that's many lines of code.
> > Alternatively, I could implement my own AST walking and property checking, 
> > but then the test would be much less readable.
> > As yet another alternative, I though about moving the 
> > `GslOwnerPointerInference.cpp` test itself to `unittests/ASTMatchers`,
> > but that would also be strange because it actually tests Sema functionality.
> > 
> > What would be your suggestion? (It's good that we discuss this now because 
> > I'm planning to extend this unit test further).
> Do you need the full matches() function? With the current test, it's not even 
> clear to me what exactly it tests since it looks very integration-testy and 
> not very unit-testy. Maybe if you make the test narrower, you don't that much 
> scaffolding? (Integration-type tests are supposed to be lit tests, and unit 
> tests are supposed to test small details that are difficult to test via lit.)
The purpose of the test is to verify that the ClassTemplateSpecialization gets 
the Owner attribute, independent of which of the redeclarations of the template 
had the attribute attached.
The second thing that I want to test here in the future is that the inference 
for std:: types leads to correct Owner/Pointer attributes on the 
`ClassTemplateSpecialization`.,
i.e. a type named `std::vector` should have OwnerAttr on each of its 
`ClassTemplateSpecialization`.

We currently do this in a lit test here 
https://reviews.llvm.org/differential/changeset/?ref=1574879 via checking the 
AST dump, but that is really hard to debug in cases of failures,
and not entirely clear that it checks exactly what we want. (E.g. we can check 
that a `ClassTemplateSpecializationDecl` line is followed by a `PointerAttr` 
line, but we don't actually know if that attribute hierarchically belongs to 
that decl or a another one).

So @gribozavr suggested to me to instead break this integration tests into 
smaller chunks using the ASTMatchers, which at least solves that problem.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66179



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

[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

[Matthias Gehre via Phabricator via cfe-commits]

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



Comment at: cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp:9
+
+#include "../ASTMatchers/ASTMatchersTest.h"
+#include "clang/ASTMatchers/ASTMatchers.h"

thakis wrote:
> This weird relative include path is a hint that this isn't the intended use :)
True. But ...
I thought about copying the `matches` function from STMatchersTest.h, but 
together with all callees, that's many lines of code.
Alternatively, I could implement my own AST walking and property checking, but 
then the test would be much less readable.
As yet another alternative, I though about moving the 
`GslOwnerPointerInference.cpp` test itself to `unittests/ASTMatchers`,
but that would also be strange because it actually tests Sema functionality.

What would be your suggestion? (It's good that we discuss this now because I'm 
planning to extend this unit test further).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66179



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

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

In the false-positive example, after the `DerivedToBase`, we see a constructor 
call which I think is the copy constructor.

1. We should consider `MutableArrayRef` to be a gsl::Pointer according to the 
paper, because it publicly derives from one.
2. Also in the paper, the copy constructor does should copies the pset of the 
argument instead of making the pointer point at the argument.

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66486



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

[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

[Matthias Gehre via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
mgehre marked an inline comment as done.
Closed by commit rL369591: [LifetimeAnalysis] Support more STL idioms (template 
forward declaration and… (authored by mgehre, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66179?vs=216247=216498#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66179

Files:
  cfe/trunk/lib/Sema/SemaAttr.cpp
  cfe/trunk/lib/Sema/SemaDeclAttr.cpp
  cfe/trunk/lib/Sema/SemaInit.cpp
  cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
  cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
  cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp
  cfe/trunk/unittests/Sema/CMakeLists.txt
  cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp

Index: cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -552,6 +552,18 @@
   continue;
 }
 
+if (auto *A = dyn_cast(TmplAttr)) {
+  if (!New->hasAttr())
+New->addAttr(A->clone(Context));
+  continue;
+}
+
+if (auto *A = dyn_cast(TmplAttr)) {
+  if (!New->hasAttr())
+New->addAttr(A->clone(Context));
+  continue;
+}
+
 assert(!TmplAttr->isPackExpansion());
 if (TmplAttr->isLateParsed() && LateAttrs) {
   // Late parsed attributes must be instantiated and attached after the
@@ -711,6 +723,9 @@
 
   SemaRef.InstantiateAttrs(TemplateArgs, D, Typedef);
 
+  if (D->getUnderlyingType()->getAs())
+SemaRef.inferGslPointerAttribute(Typedef);
+
   Typedef->setAccess(D->getAccess());
 
   return Typedef;
Index: cfe/trunk/lib/Sema/SemaInit.cpp
===
--- cfe/trunk/lib/Sema/SemaInit.cpp
+++ cfe/trunk/lib/Sema/SemaInit.cpp
@@ -6561,7 +6561,7 @@
 
 template  static bool isRecordWithAttr(QualType Type) {
   if (auto *RD = Type->getAsCXXRecordDecl())
-return RD->getCanonicalDecl()->hasAttr();
+return RD->hasAttr();
   return false;
 }
 
@@ -6672,7 +6672,7 @@
 
   if (auto *CCE = dyn_cast(Call)) {
 const auto *Ctor = CCE->getConstructor();
-const CXXRecordDecl *RD = Ctor->getParent()->getCanonicalDecl();
+const CXXRecordDecl *RD = Ctor->getParent();
 if (CCE->getNumArgs() > 0 && RD->hasAttr())
   VisitPointerArg(Ctor->getParamDecl(0), CCE->getArgs()[0]);
   }
Index: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
===
--- cfe/trunk/lib/Sema/SemaDeclAttr.cpp
+++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp
@@ -4592,9 +4592,11 @@
   }
   return;
 }
-D->addAttr(::new (S.Context)
-   OwnerAttr(AL.getRange(), S.Context, DerefTypeLoc,
- AL.getAttributeSpellingListIndex()));
+for (Decl *Redecl : D->redecls()) {
+  Redecl->addAttr(::new (S.Context)
+  OwnerAttr(AL.getRange(), S.Context, DerefTypeLoc,
+AL.getAttributeSpellingListIndex()));
+}
   } else {
 if (checkAttrMutualExclusion(S, D, AL))
   return;
@@ -4609,9 +4611,11 @@
   }
   return;
 }
-D->addAttr(::new (S.Context)
-   PointerAttr(AL.getRange(), S.Context, DerefTypeLoc,
-   AL.getAttributeSpellingListIndex()));
+for (Decl *Redecl : D->redecls()) {
+  Redecl->addAttr(::new (S.Context)
+  PointerAttr(AL.getRange(), S.Context, DerefTypeLoc,
+  AL.getAttributeSpellingListIndex()));
+}
   }
 }
 
Index: cfe/trunk/lib/Sema/SemaAttr.cpp
===
--- cfe/trunk/lib/Sema/SemaAttr.cpp
+++ cfe/trunk/lib/Sema/SemaAttr.cpp
@@ -88,13 +88,11 @@
 template 
 static void addGslOwnerPointerAttributeIfNotExisting(ASTContext ,
  CXXRecordDecl *Record) {
-  CXXRecordDecl *Canonical = Record->getCanonicalDecl();
-  if (Canonical->hasAttr() || Canonical->hasAttr())
+  if (Record->hasAttr() || Record->hasAttr())
 return;
 
-  Canonical->addAttr(::new (Context) Attribute(SourceRange{}, Context,
-   /*DerefType*/ nullptr,
-   /*Spelling=*/0));
+  for (Decl *Redecl : Record->redecls())
+Redecl->addAttr(Attribute::CreateImplicit(Context, /*DerefType=*/nullptr));
 }
 
 void Sema::inferGslPointerAttribute(NamedDecl *ND,
@@ -189,8 +187,7 @@
 
   // Handle classes that directly appear in std namespace.
   if (Record->isInStdNamespace()) {
-CXXRecordDecl *Canonical = Record->getCanonicalDecl();
-if (Canonical->hasAttr() || Canonical->hasAttr())
+if 

[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

[Matthias Gehre via Phabricator via cfe-commits]

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



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4596
+for (Decl *Redecl : D->redecls()) {
+  Redecl->addAttr(::new (S.Context)
+  OwnerAttr(AL.getRange(), S.Context, DerefTypeLoc,

gribozavr wrote:
> Add it only if it does not already exist?
If a previous decl had that Attribute, then we would have inherited it, and the 
check above for an existing attribute would have led to an early return.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66179



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

[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

This change might be the cause for the false-positive in 
https://github.com/mgehre/llvm-project/issues/45. Could you check?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66486



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

[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 216247.
mgehre marked 6 inline comments as done.
mgehre added a comment.
Herald added a subscriber: mgorny.

- Put OwnerAttr/PointerAttr on all redeclarations
- clang/lib/Sema/SemaAttr.cpp: Use Attribute::CreateImplicit as requested in 
review


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66179

Files:
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
  clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
  clang/unittests/Sema/CMakeLists.txt
  clang/unittests/Sema/GslOwnerPointerInference.cpp

Index: clang/unittests/Sema/GslOwnerPointerInference.cpp
===
--- /dev/null
+++ clang/unittests/Sema/GslOwnerPointerInference.cpp
@@ -0,0 +1,61 @@
+//== unittests/Sema/GslOwnerPointerInference.cpp - gsl::Owner/Pointer //
+//
+// 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 "../ASTMatchers/ASTMatchersTest.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+using namespace ast_matchers;
+
+TEST(OwnerPointer, BothHaveAttributes) {
+  EXPECT_TRUE(matches(
+R"cpp(
+  template
+  class [[gsl::Owner]] C;
+
+  template
+  class [[gsl::Owner]] C {};
+
+  C c;
+  )cpp",
+classTemplateSpecializationDecl(hasName("C"), hasAttr(clang::attr::Owner;
+}
+
+TEST(OwnerPointer, ForwardDeclOnly) {
+  EXPECT_TRUE(matches(
+R"cpp(
+  template
+  class [[gsl::Owner]] C;
+
+  template
+  class C {};
+
+  C c;
+  )cpp",
+classTemplateSpecializationDecl(hasName("C"), hasAttr(clang::attr::Owner;
+}
+
+TEST(OwnerPointer, LateForwardDeclOnly) {
+  EXPECT_TRUE(matches(
+R"cpp(
+  template
+  class C;
+
+  template
+  class C {};
+
+  template
+  class [[gsl::Owner]] C;
+
+  C c;
+  )cpp",
+classTemplateSpecializationDecl(hasName("C"), hasAttr(clang::attr::Owner;
+}
+
+} // namespace clang
Index: clang/unittests/Sema/CMakeLists.txt
===
--- clang/unittests/Sema/CMakeLists.txt
+++ clang/unittests/Sema/CMakeLists.txt
@@ -5,11 +5,13 @@
 add_clang_unittest(SemaTests
   ExternalSemaSourceTest.cpp
   CodeCompleteTest.cpp
+  GslOwnerPointerInference.cpp
   )
 
 clang_target_link_libraries(SemaTests
   PRIVATE
   clangAST
+  clangASTMatchers
   clangBasic
   clangFrontend
   clangParse
Index: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
===
--- clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
+++ clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
@@ -105,3 +105,20 @@
 class [[gsl::Owner(int)]] AddTheSameLater;
 // CHECK: CXXRecordDecl {{.*}} prev {{.*}} AddTheSameLater
 // CHECK: OwnerAttr {{.*}} int
+
+template 
+class [[gsl::Owner]] ForwardDeclared;
+// CHECK: ClassTemplateDecl {{.*}} ForwardDeclared
+// CHECK: OwnerAttr {{.*}}
+// CHECK: ClassTemplateSpecializationDecl {{.*}} ForwardDeclared
+// CHECK: TemplateArgument type 'int'
+// CHECK: OwnerAttr {{.*}}
+
+template 
+class [[gsl::Owner]] ForwardDeclared {
+// CHECK: ClassTemplateDecl {{.*}} ForwardDeclared
+// CHECK: CXXRecordDecl {{.*}} ForwardDeclared definition
+// CHECK: OwnerAttr {{.*}}
+};
+
+static_assert(sizeof(ForwardDeclared), ""); // Force instantiation.
Index: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
===
--- clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
+++ clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
@@ -92,6 +92,59 @@
 static_assert(sizeof(unordered_map::iterator), ""); // Force instantiation.
 } // namespace inlinens
 
+// The iterator typedef is a DependentNameType.
+template 
+class __unordered_multimap_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __unordered_multimap_iterator
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __unordered_multimap_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class __unordered_multimap_base {
+public:
+  using iterator = __unordered_multimap_iterator;
+};
+
+template 
+class unordered_multimap {
+  // CHECK: ClassTemplateDecl {{.*}} unordered_multimap
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} unordered_multimap
+  // CHECK: OwnerAttr {{.*}}
+public:
+  using _Mybase = __unordered_multimap_base;
+  using iterator = typename _Mybase::iterator;
+};
+static_assert(sizeof(unordered_multimap::iterator), ""); // Force instantiation.
+
+// The canonical 

[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

I now add the attributes to all redeclarations to make the logic a bit easier 
to follow.




Comment at: clang/lib/Sema/SemaAttr.cpp:94
 
-  Canonical->addAttr(::new (Context) Attribute(SourceRange{}, Context,
-   /*DerefType*/ nullptr,
-   /*Spelling=*/0));
+  Record->addAttr(::new (Context) Attribute(SourceRange{}, Context,
+/*DerefType*/ nullptr,

xazax.hun wrote:
> Doesn't the attribute have a `CreateImplicit` static method? If so, we could 
> use that :)
Nice, didn't know that!



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp:95
 
+// The iterator typedef is a DependentNameType.
+template 

gribozavr wrote:
> mgehre wrote:
> > gribozavr wrote:
> > > This test file is getting pretty long and subtle, with lots of things are 
> > > being mixed into one file without isolation.
> > > 
> > > WDYT about refactoring it into a unit test that uses AST matchers to 
> > > verify attribute presence?
> > > 
> > > See clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp for examples.
> > > 
> > > Each test case would look approximately like this:
> > > 
> > > ```
> > > EXPECT_TRUE(matches(
> > >   "template class ...",
> > >   classTemplateDecl(hasName("foo"), hasAttr(clang::attr::GslPointer)));
> > > ```
> > > 
> > > Each test case would be isolated from other tests, each test would have a 
> > > name (and optionally a comment) that will make it obvious what exactly is 
> > > being tested.
> > > 
> > > It would be also possible to verify things like "The iterator typedef is 
> > > a DependentNameType" to ensure that we're testing exactly what we want to 
> > > test.
> > I like the AST matcher approach! This test file is really hard to debug - I 
> > usually copy a test to its own file for debugging. 
> > Would you be okay with deferring the testing change to the next PR?
> Of course! Thanks!
I added the some tests for this particular change to a new unittest and will 
migrate the rest of the tests later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66179



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

[PATCH] D66365: [clang-tidy] [readability-convert-member-functions-to-static] ignore functions that hide base class methods

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

I struggle myself to fully understand the motivation behind the bug report. 
This is certainly not code that I would write.
So I don't really care about that kind of code and I didn't want to be in the 
way of other people's way of writing C++.

Alternatively, we could ask the bug reporter to use `// NOLINT` to support his 
specific case. I will try to get some clarification
from the reporter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66365



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

[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

[Matthias Gehre via Phabricator via cfe-commits]

mgehre marked 4 inline comments as done.
mgehre added a comment.

I now start to wonder if we should instead put the attribute on all 
declarations (instead of just the canonical). Later redeclarations 
automatically inherit the attributes.
This would make both the checking easier (just check any declaration, no need 
to obtain the canonical first), and remove the special
case for adding to the definition of templated record for ClassTemplateDecls.




Comment at: clang/lib/Sema/SemaAttr.cpp:200
 CXXRecordDecl *Canonical = Record->getCanonicalDecl();
 if (Canonical->hasAttr() || Canonical->hasAttr())
   return;

gribozavr wrote:
> mgehre wrote:
> > gribozavr wrote:
> > > Should this code not do this short-circuit check? It is prone to going 
> > > out of sync with the code in `addGslOwnerPointerAttributeIfNotExisting`, 
> > > like it did just now.
> > > 
> > > If you want to check for attribute before doing the string search, you 
> > > could pass the string set into 
> > > `addGslOwnerPointerAttributeIfNotExisting`, and let that decide if it 
> > > should infer the attribute or not.
> > Yes, the `hasAttr` check here is an optimization to avoid the string 
> > search. I don't think I can move the string search into 
> > `addGslOwnerPointerAttributeIfNotExisting`, because that function is called 
> > from other call sites that don't care about a set of names.
> Sorry I wasn't clear enough -- the issue that I noticed is that this check 
> looks at the canonical decl, while `addGslOwnerPointerAttributeIfNotExisting` 
> attaches the attribute to the decl itself -- that's what I meant by "out of 
> sync".
I make sure that `addGslOwnerPointerAttributeIfNotExisting` is only called with 
the canonical decl as argument, which the caller usually has around. The only 
exception to this is when addGslOwnerPointerAttributeIfNotExisting calls itself 
to attach an attribute to the definition of templated class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66179



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

[PATCH] D66365: [clang-tidy] [readability-convert-member-functions-to-static] ignore functions that hide base class methods

[Matthias Gehre via Phabricator via cfe-commits]

mgehre created this revision.
mgehre added reviewers: aaron.ballman, Eugene.Zelenko.
Herald added a subscriber: xazax.hun.
Herald added a project: clang.

Fixes bug https://bugs.llvm.org/show_bug.cgi?id=43002


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66365

Files:
  clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp
  
clang-tools-extra/test/clang-tidy/readability-convert-member-functions-to-static.cpp


Index: 
clang-tools-extra/test/clang-tidy/readability-convert-member-functions-to-static.cpp
===
--- 
clang-tools-extra/test/clang-tidy/readability-convert-member-functions-to-static.cpp
+++ 
clang-tools-extra/test/clang-tidy/readability-convert-member-functions-to-static.cpp
@@ -216,3 +216,21 @@
 return;
   }
 };
+
+class HiddingBase {
+  int f();
+
+  static int g();
+};
+
+class HiddingDerived : public HiddingBase {
+  int f() {
+return 0;
+  }
+
+  int g() {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'g' can be made static
+// CHECK-FIXES: {{^}}  static int g() {
+return 0;
+  }
+};
Index: 
clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp
===
--- clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp
+++ clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp
@@ -71,6 +71,31 @@
   return UsageOfThis.Used;
 }
 
+/// Does any base class have a non-static method with the same name?
+AST_MATCHER(CXXMethodDecl, isHiding) {
+  const IdentifierInfo *ThisIdentifier = Node.getIdentifier();
+  if (!ThisIdentifier)
+return false;
+
+  const CXXRecordDecl *Record = Node.getParent();
+
+  auto StaticOrDifferentName = [ThisIdentifier](const CXXMethodDecl *Method) {
+  if (Method->isStatic())
+return true;
+
+  const IdentifierInfo *Ident = 
Method->getIdentifier();
+  if (!Ident)
+return true;
+
+  return Ident->getName() != ThisIdentifier->getName();
+};
+
+  return !Record->forallBases([StaticOrDifferentName](const CXXRecordDecl 
*Base) {
+return llvm::all_of(Base->methods(),
+StaticOrDifferentName);
+  });
+}
+
 void ConvertMemberFunctionsToStatic::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
   cxxMethodDecl(
@@ -86,7 +111,8 @@
   // depending on template base class.
   ),
   isInsideMacroDefinition(),
-  hasCanonicalDecl(isInsideMacroDefinition()), usesThis(
+  hasCanonicalDecl(isInsideMacroDefinition()), isHiding(),
+  usesThis(
   .bind("x"),
   this);
 }


Index: clang-tools-extra/test/clang-tidy/readability-convert-member-functions-to-static.cpp
===
--- clang-tools-extra/test/clang-tidy/readability-convert-member-functions-to-static.cpp
+++ clang-tools-extra/test/clang-tidy/readability-convert-member-functions-to-static.cpp
@@ -216,3 +216,21 @@
 return;
   }
 };
+
+class HiddingBase {
+  int f();
+
+  static int g();
+};
+
+class HiddingDerived : public HiddingBase {
+  int f() {
+return 0;
+  }
+
+  int g() {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: method 'g' can be made static
+// CHECK-FIXES: {{^}}  static int g() {
+return 0;
+  }
+};
Index: clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp
===
--- clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp
+++ clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp
@@ -71,6 +71,31 @@
   return UsageOfThis.Used;
 }
 
+/// Does any base class have a non-static method with the same name?
+AST_MATCHER(CXXMethodDecl, isHiding) {
+  const IdentifierInfo *ThisIdentifier = Node.getIdentifier();
+  if (!ThisIdentifier)
+return false;
+
+  const CXXRecordDecl *Record = Node.getParent();
+
+  auto StaticOrDifferentName = [ThisIdentifier](const CXXMethodDecl *Method) {
+  if (Method->isStatic())
+return true;
+
+  const IdentifierInfo *Ident = Method->getIdentifier();
+  if (!Ident)
+return true;
+
+  return Ident->getName() != ThisIdentifier->getName();
+};
+
+  return !Record->forallBases([StaticOrDifferentName](const CXXRecordDecl *Base) {
+return llvm::all_of(Base->methods(),
+StaticOrDifferentName);
+  });
+}
+
 void ConvertMemberFunctionsToStatic::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
   

[PATCH] D66303: [LifetimeAnalysis] Add support for free functions

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:6622
+return false;
+  const auto *RD = FD->getParamDecl(0)->getType()->getPointeeCXXRecordDecl();
+  if (!FD->isInStdNamespace() || !RD || !RD->isInStdNamespace())

Maybe move the `Callee->getNumParams() == 1` check from the caller into this 
function so it's obvious that `getParamDecl(0)` is allowed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66303



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

[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

[Matthias Gehre via Phabricator via cfe-commits]

mgehre marked 6 inline comments as done.
mgehre added inline comments.



Comment at: clang/lib/Sema/SemaAttr.cpp:102
+  dyn_cast_or_null(Record->getDescribedTemplate())) 
{
+if (auto *Def = Record->getDefinition())
+  addGslOwnerPointerAttributeIfNotExisting(Context, Def);

gribozavr wrote:
> I wonder why this is necessary. Sema should call inference on every 
> CXXRecordDecl. Is it because of incorrect short-circuiting in 
> `inferGslPointerAttribute` that I'm pointing out below?
The contract is: A CXXRecordDecl is a Pointer/Owner if and only if its 
canonical declaration has the Pointer/Owner attribute.

The analysis only checks this on concrete classes (excuse me if I'm using the 
wrong term here), i.e. non-template classes or instantiation of template 
classes.

During the inference, we might also put the attributes onto the templated 
declaration of a ClassTemplateDecl. The Pointer/Owner attributes here will not 
be used by the analysis.
We just put them there so clang will copy them to each instantiation, where 
they will be used by the analysis.

From what I understand, clang copies the attributes of the definition of the 
templated declaration to the instantiation.
In addition, when clang sees a redeclaration (e.g. of a template), it will also 
copy the attributes from the previous/canonical (?) declaration.

In the problematic case
```
// The canonical declaration of the iterator template is not its definition.
template 
class __unordered_multiset_iterator;

template 
class __unordered_multiset_iterator {
};

template 
class unordered_multiset {
public:
  using iterator = __unordered_multiset_iterator;
};

static_assert(sizeof(unordered_multiset::iterator), ""); // Force 
instantiation.
```
clang would (before this PR):
1. Create a `ClassTemplateDecl` with an `CXXRecordDecl` for 
`__unordered_multiset_iterator` (this is the canonical declaration, but not a 
definition)
2. Create a `ClassTemplateDecl` with an `CXXRecordDecl` for 
`__unordered_multiset_iterator` (this is not the canonical declaration, but its 
definition)
3. While processing the `using iterator` line, add a PointerAttr to the 
canonical declaration (from point 1)
4. While instantiating `__unordered_multiset_iterator`, copy attributes 
from the template definition (from point 2), which are none
Result: The `ClassTemplateSpecializationDecl` for 
`__unordered_multiset_iterator` would not have the `PointerAttr`.

On the other hand, a swapped example
```
template 
class __unordered_multiset_iterator;

template 
class unordered_multiset {
public:
  using iterator = __unordered_multiset_iterator;
};

template 
class __unordered_multiset_iterator {
};

static_assert(sizeof(unordered_multiset::iterator), ""); // Force 
instantiation.
```
would work because clang would (before this PR):
1. Create a `ClassTemplateDecl` with an `CXXRecordDecl` for 
`__unordered_multiset_iterator` (this is the canonical declaration, but not a 
definition)
2. While processing the `using iterator` line, add a PointerAttr to the 
canonical declaration (from point 1)
3. Create a `ClassTemplateDecl` with an `CXXRecordDecl` for 
`__unordered_multiset_iterator` (this is not the canonical declaration, but its 
definition); copy all attributes from a previous declaration, i.e. the 
PointerAttr from point 1
4. While instantiating `__unordered_multiset_iterator`, copy PointerAttr 
from the template definition (from point 3)

So we cannot just put the PointerAttr on the definition of the class template 
because there might be none yet (like in the second example). We need to put it 
on the definition in case
it was already parsed.



Comment at: clang/lib/Sema/SemaAttr.cpp:200
 CXXRecordDecl *Canonical = Record->getCanonicalDecl();
 if (Canonical->hasAttr() || Canonical->hasAttr())
   return;

gribozavr wrote:
> Should this code not do this short-circuit check? It is prone to going out of 
> sync with the code in `addGslOwnerPointerAttributeIfNotExisting`, like it did 
> just now.
> 
> If you want to check for attribute before doing the string search, you could 
> pass the string set into `addGslOwnerPointerAttributeIfNotExisting`, and let 
> that decide if it should infer the attribute or not.
Yes, the `hasAttr` check here is an optimization to avoid the string search. I 
don't think I can move the string search into 
`addGslOwnerPointerAttributeIfNotExisting`, because that function is called 
from other call sites that don't care about a set of names.



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp:95
 
+// The iterator typedef is a DependentNameType.
+template 

gribozavr wrote:
> This test file is getting pretty long and subtle, with lots of things are 
> being mixed into one file without isolation.
> 
> WDYT about refactoring it into a unit test that uses AST matchers to verify 
> attribute presence?
> 
> See 

[PATCH] D66164: [LifetimeAnalysis] Support std::stack::top() and std::optional::value()

[Matthias Gehre via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368929: [LifetimeAnalysis] Support std::stack::top() and 
std::optional::value() (authored by mgehre, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66164?vs=214954=215259#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66164

Files:
  cfe/trunk/lib/Sema/SemaInit.cpp
  cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp


Index: cfe/trunk/lib/Sema/SemaInit.cpp
===
--- cfe/trunk/lib/Sema/SemaInit.cpp
+++ cfe/trunk/lib/Sema/SemaInit.cpp
@@ -6610,7 +6610,7 @@
  OO == OverloadedOperatorKind::OO_Star;
 }
 return llvm::StringSwitch(Callee->getName())
-.Cases("front", "back", "at", true)
+.Cases("front", "back", "at", "top", "value", true)
 .Default(false);
   }
   return false;
Index: cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -170,7 +170,15 @@
 struct optional {
   optional();
   optional(const T&);
-  T *();
+  T *() &;
+  T &*() &&;
+  T () &;
+  T &() &&;
+};
+
+template
+struct stack {
+  T ();
 };
 }
 
@@ -188,6 +196,16 @@
   return s.c_str(); // expected-warning {{address of stack memory associated 
with local variable 's' returned}}
 }
 
+int () {
+  std::optional o;
+  return o.value(); // expected-warning {{reference to stack memory associated 
with local variable 'o' returned}}
+}
+
+int () {
+  std::optional o;
+  return *o; // expected-warning {{reference to stack memory associated with 
local variable 'o' returned}}
+}
+
 const char *danglingRawPtrFromTemp() {
   return std::basic_string().c_str(); // expected-warning {{returning 
address of local temporary object}}
 }
@@ -203,9 +221,10 @@
 }
 
 void danglingReferenceFromTempOwner() {
-  int  = *std::optional(); // expected-warning {{object backing the 
pointer will be destroyed at the end of the full-expression}}
-  int  = *std::optional(5); // expected-warning {{object backing the 
pointer will be destroyed at the end of the full-expression}}
-  int  = std::vector().at(3); // expected-warning {{object backing the 
pointer will be destroyed at the end of the full-expression}}
+  int & = *std::optional();  // expected-warning {{object 
backing the pointer will be destroyed at the end of the full-expression}}
+  int & = *std::optional(5);// expected-warning {{object 
backing the pointer will be destroyed at the end of the full-expression}}
+  int & = std::optional(5).value(); // expected-warning {{object 
backing the pointer will be destroyed at the end of the full-expression}}
+  int  = std::vector().at(3);   // expected-warning {{object 
backing the pointer will be destroyed at the end of the full-expression}}
 }
 
 std::vector getTempVec();


Index: cfe/trunk/lib/Sema/SemaInit.cpp
===
--- cfe/trunk/lib/Sema/SemaInit.cpp
+++ cfe/trunk/lib/Sema/SemaInit.cpp
@@ -6610,7 +6610,7 @@
  OO == OverloadedOperatorKind::OO_Star;
 }
 return llvm::StringSwitch(Callee->getName())
-.Cases("front", "back", "at", true)
+.Cases("front", "back", "at", "top", "value", true)
 .Default(false);
   }
   return false;
Index: cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -170,7 +170,15 @@
 struct optional {
   optional();
   optional(const T&);
-  T *();
+  T *() &;
+  T &*() &&;
+  T () &;
+  T &() &&;
+};
+
+template
+struct stack {
+  T ();
 };
 }
 
@@ -188,6 +196,16 @@
   return s.c_str(); // expected-warning {{address of stack memory associated with local variable 's' returned}}
 }
 
+int () {
+  std::optional o;
+  return o.value(); // expected-warning {{reference to stack memory associated with local variable 'o' returned}}
+}
+
+int () {
+  std::optional o;
+  return *o; // expected-warning {{reference to stack memory associated with local variable 'o' returned}}
+}
+
 const char *danglingRawPtrFromTemp() {
   return std::basic_string().c_str(); // expected-warning {{returning address of local temporary object}}
 }
@@ -203,9 +221,10 @@
 }
 
 void danglingReferenceFromTempOwner() {
-  int  = *std::optional(); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
-  int  = *std::optional(5); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
-  int  = std::vector().at(3); // expected-warning {{object 

[PATCH] D66164: [LifetimeAnalysis] Support std::stack::top() and std::optional::value()

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 214954.
mgehre marked an inline comment as done.
mgehre added a comment.

Add tests for rvalue-ref overloads


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66164

Files:
  clang/lib/Sema/SemaInit.cpp
  clang/test/Sema/warn-lifetime-analysis-nocfg.cpp


Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -168,7 +168,15 @@
 struct optional {
   optional();
   optional(const T&);
-  T *();
+  T *() &;
+  T &*() &&;
+  T () &;
+  T &() &&;
+};
+
+template
+struct stack {
+  T ();
 };
 }
 
@@ -186,6 +194,16 @@
   return s.c_str(); // expected-warning {{address of stack memory associated 
with local variable 's' returned}}
 }
 
+int () {
+  std::optional o;
+  return o.value(); // expected-warning {{reference to stack memory associated 
with local variable 'o' returned}}
+}
+
+int () {
+  std::optional o;
+  return *o; // expected-warning {{reference to stack memory associated with 
local variable 'o' returned}}
+}
+
 const char *danglingRawPtrFromTemp() {
   return std::basic_string().c_str(); // expected-warning {{returning 
address of local temporary object}}
 }
@@ -201,9 +219,10 @@
 }
 
 void danglingReferenceFromTempOwner() {
-  int  = *std::optional(); // expected-warning {{object backing the 
pointer will be destroyed at the end of the full-expression}}
-  int  = *std::optional(5); // expected-warning {{object backing the 
pointer will be destroyed at the end of the full-expression}}
-  int  = std::vector().at(3); // expected-warning {{object backing the 
pointer will be destroyed at the end of the full-expression}}
+  int & = *std::optional();  // expected-warning {{object 
backing the pointer will be destroyed at the end of the full-expression}}
+  int & = *std::optional(5);// expected-warning {{object 
backing the pointer will be destroyed at the end of the full-expression}}
+  int & = std::optional(5).value(); // expected-warning {{object 
backing the pointer will be destroyed at the end of the full-expression}}
+  int  = std::vector().at(3);   // expected-warning {{object 
backing the pointer will be destroyed at the end of the full-expression}}
 }
 
 std::vector getTempVec();
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -6591,7 +6591,7 @@
  OO == OverloadedOperatorKind::OO_Star;
 }
 return llvm::StringSwitch(Callee->getName())
-.Cases("front", "back", "at", true)
+.Cases("front", "back", "at", "top", "value", true)
 .Default(false);
   }
   return false;


Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -168,7 +168,15 @@
 struct optional {
   optional();
   optional(const T&);
-  T *();
+  T *() &;
+  T &*() &&;
+  T () &;
+  T &() &&;
+};
+
+template
+struct stack {
+  T ();
 };
 }
 
@@ -186,6 +194,16 @@
   return s.c_str(); // expected-warning {{address of stack memory associated with local variable 's' returned}}
 }
 
+int () {
+  std::optional o;
+  return o.value(); // expected-warning {{reference to stack memory associated with local variable 'o' returned}}
+}
+
+int () {
+  std::optional o;
+  return *o; // expected-warning {{reference to stack memory associated with local variable 'o' returned}}
+}
+
 const char *danglingRawPtrFromTemp() {
   return std::basic_string().c_str(); // expected-warning {{returning address of local temporary object}}
 }
@@ -201,9 +219,10 @@
 }
 
 void danglingReferenceFromTempOwner() {
-  int  = *std::optional(); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
-  int  = *std::optional(5); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
-  int  = std::vector().at(3); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  int & = *std::optional();  // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  int & = *std::optional(5);// expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  int & = std::optional(5).value(); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  int  = std::vector().at(3);   // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
 }
 
 std::vector getTempVec();
Index: clang/lib/Sema/SemaInit.cpp

[PATCH] D66164: [LifetimeAnalysis] Support std::stack::top() and std::optional::value()

[Matthias Gehre via Phabricator via cfe-commits]

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



Comment at: clang/lib/Sema/SemaInit.cpp:6583
 .Cases("end", "rend", "cend", "crend", true)
-.Cases("c_str", "data", "get", true)
+.Cases("c_str", "data", "get", "value", true)
 // Map and set types.

xazax.hun wrote:
> Oh, this one needs to be updated, as `value` returns a reference not a 
> pointer. 
Yes, I noticed the same.



Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:172
   T *();
+  T ();
+};

xazax.hun wrote:
> mgehre wrote:
> > xazax.hun wrote:
> > > I actually was a bit lazy when I added these tests. Both `value` and 
> > > `operator*` is overloaded on `&&`. But if you do not feel like adjusting 
> > > the tests this is fine, I can do it myself later :)
> > I'll change it to use the `&` variant in the test - the `&&` cannot dangle 
> > as far as I understand.
> It can!
> 
> Consider the following code:
> 
> ```
> int & = *std::optional(5);
> // r dangles here.
> ```
Oh, sure!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66164



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

[PATCH] D66179: [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

[Matthias Gehre via Phabricator via cfe-commits]

mgehre created this revision.
mgehre added a reviewer: gribozavr.
Herald added a subscriber: Szelethus.
Herald added a project: clang.

This fixes inference of gsl::Pointer on std::set::iterator with libstdc++ (the 
typedef for iterator
on the template is a DependentNameType - we can only put the gsl::Pointer 
attribute
on the underlaying record after instantiation)

inference of gsl::Pointer on std::vector::iterator with libc++ (the class was 
forward-declared,
we added the gsl::Pointer on the canonical decl (the forward decl), and later 
when the
template was instantiated, there was no attribute on the definition so it was 
not instantiated).

and a duplicate gsl::Pointer on some class with libstdc++ (we first added an 
attribute to
a incomplete instantiation, and then another was copied from the template 
definition
when the instantiation was completed).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66179

Files:
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp

Index: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
===
--- clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
+++ clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
@@ -92,6 +92,59 @@
 static_assert(sizeof(unordered_map::iterator), ""); // Force instantiation.
 } // namespace inlinens
 
+// The iterator typedef is a DependentNameType.
+template 
+class __unordered_multimap_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __unordered_multimap_iterator
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __unordered_multimap_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class __unordered_multimap_base {
+public:
+  using iterator = __unordered_multimap_iterator;
+};
+
+template 
+class unordered_multimap {
+  // CHECK: ClassTemplateDecl {{.*}} unordered_multimap
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} unordered_multimap
+  // CHECK: OwnerAttr {{.*}}
+public:
+  using _Mybase = __unordered_multimap_base;
+  using iterator = typename _Mybase::iterator;
+};
+static_assert(sizeof(unordered_multimap::iterator), ""); // Force instantiation.
+
+// The canonical declaration of the iterator template is not its definition.
+template 
+class __unordered_multiset_iterator;
+// CHECK: ClassTemplateDecl {{.*}} __unordered_multiset_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __unordered_multiset_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class __unordered_multiset_iterator {
+  // CHECK: ClassTemplateDecl {{.*}} prev {{.*}} __unordered_multiset_iterator
+  // CHECK: PointerAttr
+};
+
+template 
+class unordered_multiset {
+  // CHECK: ClassTemplateDecl {{.*}} unordered_multiset
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} unordered_multiset
+  // CHECK: OwnerAttr {{.*}}
+public:
+  using iterator = __unordered_multiset_iterator;
+};
+
+static_assert(sizeof(unordered_multiset::iterator), ""); // Force instantiation.
+
 // std::list has an implicit gsl::Owner attribute,
 // but explicit attributes take precedence.
 template 
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -552,6 +552,18 @@
   continue;
 }
 
+if (auto *A = dyn_cast(TmplAttr)) {
+  if (!New->hasAttr())
+New->addAttr(A->clone(Context));
+  continue;
+}
+
+if (auto *A = dyn_cast(TmplAttr)) {
+  if (!New->hasAttr())
+New->addAttr(A->clone(Context));
+  continue;
+}
+
 assert(!TmplAttr->isPackExpansion());
 if (TmplAttr->isLateParsed() && LateAttrs) {
   // Late parsed attributes must be instantiated and attached after the
@@ -711,6 +723,9 @@
 
   SemaRef.InstantiateAttrs(TemplateArgs, D, Typedef);
 
+  if (D->getUnderlyingType()->getAs())
+SemaRef.inferGslPointerAttribute(Typedef);
+
   Typedef->setAccess(D->getAccess());
 
   return Typedef;
Index: clang/lib/Sema/SemaAttr.cpp
===
--- clang/lib/Sema/SemaAttr.cpp
+++ clang/lib/Sema/SemaAttr.cpp
@@ -88,13 +88,20 @@
 template 
 static void addGslOwnerPointerAttributeIfNotExisting(ASTContext ,
  CXXRecordDecl *Record) {
-  CXXRecordDecl *Canonical = Record->getCanonicalDecl();
-  if (Canonical->hasAttr() || Canonical->hasAttr())
+  if (Record->hasAttr() || Record->hasAttr())
 return;
 
-  Canonical->addAttr(::new (Context) Attribute(SourceRange{}, Context,
-   /*DerefType*/ nullptr,
-   /*Spelling=*/0));
+  Record->addAttr(::new (Context) 

[PATCH] D66164: [LifetimeAnalysis] Support std::stack::top() and std::optional::value()

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 214950.
mgehre marked 2 inline comments as done.
mgehre added a comment.

Fix commit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66164

Files:
  clang/lib/Sema/SemaInit.cpp
  clang/test/Sema/warn-lifetime-analysis-nocfg.cpp


Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -168,7 +168,14 @@
 struct optional {
   optional();
   optional(const T&);
-  T *();
+  T *() &;
+  T &*() &&;
+  T () &;
+};
+
+template
+struct stack {
+  T ();
 };
 }
 
@@ -186,6 +193,16 @@
   return s.c_str(); // expected-warning {{address of stack memory associated 
with local variable 's' returned}}
 }
 
+int () {
+  std::optional o;
+  return o.value(); // expected-warning {{reference to stack memory associated 
with local variable 'o' returned}}
+}
+
+int () {
+  std::optional o;
+  return *o; // expected-warning {{reference to stack memory associated with 
local variable 'o' returned}}
+}
+
 const char *danglingRawPtrFromTemp() {
   return std::basic_string().c_str(); // expected-warning {{returning 
address of local temporary object}}
 }
@@ -201,9 +218,8 @@
 }
 
 void danglingReferenceFromTempOwner() {
-  int  = *std::optional(); // expected-warning {{object backing the 
pointer will be destroyed at the end of the full-expression}}
-  int  = *std::optional(5); // expected-warning {{object backing the 
pointer will be destroyed at the end of the full-expression}}
-  int  = std::vector().at(3); // expected-warning {{object backing the 
pointer will be destroyed at the end of the full-expression}}
+  int  = std::vector().at(3); // expected-warning {{object backing the 
pointer will be destroyed at the end of the full-expression}}
+  int  = std::stack().top();  // expected-warning {{object backing the 
pointer will be destroyed at the end of the full-expression}}
 }
 
 std::vector getTempVec();
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -6591,7 +6591,7 @@
  OO == OverloadedOperatorKind::OO_Star;
 }
 return llvm::StringSwitch(Callee->getName())
-.Cases("front", "back", "at", true)
+.Cases("front", "back", "at", "top", "value", true)
 .Default(false);
   }
   return false;


Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -168,7 +168,14 @@
 struct optional {
   optional();
   optional(const T&);
-  T *();
+  T *() &;
+  T &*() &&;
+  T () &;
+};
+
+template
+struct stack {
+  T ();
 };
 }
 
@@ -186,6 +193,16 @@
   return s.c_str(); // expected-warning {{address of stack memory associated with local variable 's' returned}}
 }
 
+int () {
+  std::optional o;
+  return o.value(); // expected-warning {{reference to stack memory associated with local variable 'o' returned}}
+}
+
+int () {
+  std::optional o;
+  return *o; // expected-warning {{reference to stack memory associated with local variable 'o' returned}}
+}
+
 const char *danglingRawPtrFromTemp() {
   return std::basic_string().c_str(); // expected-warning {{returning address of local temporary object}}
 }
@@ -201,9 +218,8 @@
 }
 
 void danglingReferenceFromTempOwner() {
-  int  = *std::optional(); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
-  int  = *std::optional(5); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
-  int  = std::vector().at(3); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  int  = std::vector().at(3); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  int  = std::stack().top();  // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
 }
 
 std::vector getTempVec();
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -6591,7 +6591,7 @@
  OO == OverloadedOperatorKind::OO_Star;
 }
 return llvm::StringSwitch(Callee->getName())
-.Cases("front", "back", "at", true)
+.Cases("front", "back", "at", "top", "value", true)
 .Default(false);
   }
   return false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66164: [LifetimeAnalysis] Support std::stack::top() and std::optional::value()

[Matthias Gehre via Phabricator via cfe-commits]

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



Comment at: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp:172
   T *();
+  T ();
+};

xazax.hun wrote:
> I actually was a bit lazy when I added these tests. Both `value` and 
> `operator*` is overloaded on `&&`. But if you do not feel like adjusting the 
> tests this is fine, I can do it myself later :)
I'll change it to use the `&` variant in the test - the `&&` cannot dangle as 
far as I understand.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66164



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

[PATCH] D66164: [LifetimeAnalysis] Support std::stack::top() and std::optional::value()

[Matthias Gehre via Phabricator via cfe-commits]

mgehre created this revision.
mgehre added a reviewer: gribozavr.
Herald added a project: clang.

Diagnose dangling pointers that come from std::stack::top() and 
std::optional::value().


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66164

Files:
  clang/lib/Sema/SemaInit.cpp
  clang/test/Sema/warn-lifetime-analysis-nocfg.cpp


Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -169,6 +169,12 @@
   optional();
   optional(const T&);
   T *();
+  T ();
+};
+
+template 
+struct stack {
+  T ();
 };
 }
 
@@ -204,6 +210,8 @@
   int  = *std::optional(); // expected-warning {{object backing the 
pointer will be destroyed at the end of the full-expression}}
   int  = *std::optional(5); // expected-warning {{object backing the 
pointer will be destroyed at the end of the full-expression}}
   int  = std::vector().at(3); // expected-warning {{object backing the 
pointer will be destroyed at the end of the full-expression}}
+  int  = std::optional(5).value(); // expected-warning {{object 
backing the pointer will be destroyed at the end of the full-expression}}
+  int  = std::stack().top();   // expected-warning {{object 
backing the pointer will be destroyed at the end of the full-expression}}
 }
 
 std::vector getTempVec();
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -6580,7 +6580,7 @@
 return llvm::StringSwitch(Callee->getName())
 .Cases("begin", "rbegin", "cbegin", "crbegin", true)
 .Cases("end", "rend", "cend", "crend", true)
-.Cases("c_str", "data", "get", true)
+.Cases("c_str", "data", "get", "value", true)
 // Map and set types.
 .Cases("find", "equal_range", "lower_bound", "upper_bound", true)
 .Default(false);
@@ -6591,7 +6591,7 @@
  OO == OverloadedOperatorKind::OO_Star;
 }
 return llvm::StringSwitch(Callee->getName())
-.Cases("front", "back", "at", true)
+.Cases("front", "back", "at", "top", true)
 .Default(false);
   }
   return false;


Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -169,6 +169,12 @@
   optional();
   optional(const T&);
   T *();
+  T ();
+};
+
+template 
+struct stack {
+  T ();
 };
 }
 
@@ -204,6 +210,8 @@
   int  = *std::optional(); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
   int  = *std::optional(5); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
   int  = std::vector().at(3); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  int  = std::optional(5).value(); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  int  = std::stack().top();   // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
 }
 
 std::vector getTempVec();
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -6580,7 +6580,7 @@
 return llvm::StringSwitch(Callee->getName())
 .Cases("begin", "rbegin", "cbegin", "crbegin", true)
 .Cases("end", "rend", "cend", "crend", true)
-.Cases("c_str", "data", "get", true)
+.Cases("c_str", "data", "get", "value", true)
 // Map and set types.
 .Cases("find", "equal_range", "lower_bound", "upper_bound", true)
 .Default(false);
@@ -6591,7 +6591,7 @@
  OO == OverloadedOperatorKind::OO_Star;
 }
 return llvm::StringSwitch(Callee->getName())
-.Cases("front", "back", "at", true)
+.Cases("front", "back", "at", "top", true)
 .Default(false);
   }
   return false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Matthias Gehre via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368147: gsl::Owner/gsl::Pointer: Add implicit annotations 
for some std types (authored by mgehre, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64448?vs=212446=213836#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64448

Files:
  cfe/trunk/include/clang/Basic/AttrDocs.td
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/lib/Sema/SemaAttr.cpp
  cfe/trunk/lib/Sema/SemaDecl.cpp
  cfe/trunk/lib/Sema/SemaTemplate.cpp
  cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp

Index: cfe/trunk/include/clang/Sema/Sema.h
===
--- cfe/trunk/include/clang/Sema/Sema.h
+++ cfe/trunk/include/clang/Sema/Sema.h
@@ -6116,6 +6116,17 @@
   ClassTemplateSpecializationDecl *BaseTemplateSpec,
   SourceLocation BaseLoc);
 
+  /// Add gsl::Pointer attribute to std::container::iterator
+  /// \param ND The declaration that introduces the name
+  /// std::container::iterator. \param UnderlyingRecord The record named by ND.
+  void inferGslPointerAttribute(NamedDecl *ND, CXXRecordDecl *UnderlyingRecord);
+
+  /// Add [[gsl::Owner]] and [[gsl::Pointer]] attributes for std:: types.
+  void inferGslOwnerPointerAttribute(CXXRecordDecl *Record);
+
+  /// Add [[gsl::Pointer]] attributes for std:: types.
+  void inferGslPointerAttribute(TypedefNameDecl *TD);
+
   void CheckCompletedCXXClass(CXXRecordDecl *Record);
 
   /// Check that the C++ class annoated with "trivial_abi" satisfies all the
Index: cfe/trunk/include/clang/Basic/AttrDocs.td
===
--- cfe/trunk/include/clang/Basic/AttrDocs.td
+++ cfe/trunk/include/clang/Basic/AttrDocs.td
@@ -4249,7 +4249,7 @@
 int *getInt() { return  }
   };
 
-The argument ``T`` is optional and currently ignored.
+The argument ``T`` is optional and is ignored.
 This attribute may be used by analysis tools and has no effect on code
 generation.
 
@@ -4275,7 +4275,7 @@
 int *getInt() { return  }
   };
 
-The argument ``T`` is optional and currently ignored.
+The argument ``T`` is optional and is ignored.
 This attribute may be used by analysis tools and has no effect on code
 generation.
 
Index: cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
===
--- cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
+++ cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 -ast-dump %s | \
+// RUN: FileCheck --implicit-check-not OwnerAttr --implicit-check-not PointerAttr %s
+
+// Test attribute inference for types in the standard library.
+namespace std {
+// Attributes are inferred for a (complete) class.
+class any {
+  // CHECK: CXXRecordDecl {{.*}} any
+  // CHECK: OwnerAttr {{.*}}
+};
+
+// Attributes are inferred for instantiations of a complete template.
+template 
+class vector {
+public:
+  class iterator {};
+  // CHECK: ClassTemplateDecl {{.*}} vector
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} vector
+  // CHECK: TemplateArgument type 'int'
+  // CHECK: OwnerAttr
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+};
+static_assert(sizeof(vector), "");   // Force instantiation.
+static_assert(sizeof(vector::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a using declaration, attributes are inferred
+// for the underlying class.
+template 
+class __set_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __set_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __set_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class set {
+  // CHECK: ClassTemplateDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+public:
+  using iterator = __set_iterator;
+};
+static_assert(sizeof(set::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a typedef, attributes are inferred for the
+// underlying class.
+template 
+class __map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __map_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class map {
+  // CHECK: ClassTemplateDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __map_iterator iterator;
+};
+static_assert(sizeof(map::iterator), ""); // Force instantiation.
+
+// Inline namespaces are ignored when checking if

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 212446.
mgehre added a comment.

- Fix crash


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448

Files:
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp

Index: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 -ast-dump %s | \
+// RUN: FileCheck --implicit-check-not OwnerAttr --implicit-check-not PointerAttr %s
+
+// Test attribute inference for types in the standard library.
+namespace std {
+// Attributes are inferred for a (complete) class.
+class any {
+  // CHECK: CXXRecordDecl {{.*}} any
+  // CHECK: OwnerAttr {{.*}}
+};
+
+// Attributes are inferred for instantiations of a complete template.
+template 
+class vector {
+public:
+  class iterator {};
+  // CHECK: ClassTemplateDecl {{.*}} vector
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} vector
+  // CHECK: TemplateArgument type 'int'
+  // CHECK: OwnerAttr
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+};
+static_assert(sizeof(vector), "");   // Force instantiation.
+static_assert(sizeof(vector::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a using declaration, attributes are inferred
+// for the underlying class.
+template 
+class __set_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __set_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __set_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class set {
+  // CHECK: ClassTemplateDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+public:
+  using iterator = __set_iterator;
+};
+static_assert(sizeof(set::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a typedef, attributes are inferred for the
+// underlying class.
+template 
+class __map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __map_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class map {
+  // CHECK: ClassTemplateDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __map_iterator iterator;
+};
+static_assert(sizeof(map::iterator), ""); // Force instantiation.
+
+// Inline namespaces are ignored when checking if
+// the class lives in the std namespace.
+inline namespace inlinens {
+template 
+class __unordered_map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __unordered_map_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __unordered_map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class unordered_map {
+  // CHECK: ClassTemplateDecl {{.*}} unordered_map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} unordered_map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __unordered_map_iterator iterator;
+};
+static_assert(sizeof(unordered_map::iterator), ""); // Force instantiation.
+} // namespace inlinens
+
+// std::list has an implicit gsl::Owner attribute,
+// but explicit attributes take precedence.
+template 
+class [[gsl::Pointer]] list{};
+// CHECK: ClassTemplateDecl {{.*}} list
+// CHECK: PointerAttr {{.*}}
+// CHECK: ClassTemplateSpecializationDecl {{.*}} list
+// CHECK: PointerAttr {{.*}}
+
+static_assert(sizeof(list), ""); // Force instantiation.
+
+// Forward declared template (Owner).
+template <
+class CharT,
+class Traits>
+class basic_regex;
+// CHECK: ClassTemplateDecl {{.*}} basic_regex
+// CHECK: OwnerAttr {{.*}}
+
+// Forward declared template (Pointer).
+template 
+class reference_wrapper;
+// CHECK: ClassTemplateDecl {{.*}} reference_wrapper
+// CHECK: PointerAttr {{.*}}
+
+class some_unknown_type;
+// CHECK: CXXRecordDecl {{.*}} some_unknown_type
+
+} // namespace std
+
+namespace user {
+// If a class is not in the std namespace, we don't infer the attributes.
+class any {
+};
+} // namespace user
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -1690,6 +1690,7 @@
 mergeDeclAttributes(NewClass, PrevClassTemplate->getTemplatedDecl());
 
   AddPushedVisibilityAttribute(NewClass);
+  addDefaultGslOwnerPointerAttribute(NewClass);
 
  

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 212441.
mgehre added a comment.

- Add missing check


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448

Files:
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp

Index: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 -ast-dump %s | \
+// RUN: FileCheck --implicit-check-not OwnerAttr --implicit-check-not PointerAttr %s
+
+// Test attribute inference for types in the standard library.
+namespace std {
+// Attributes are inferred for a (complete) class.
+class any {
+  // CHECK: CXXRecordDecl {{.*}} any
+  // CHECK: OwnerAttr {{.*}}
+};
+
+// Attributes are inferred for instantiations of a complete template.
+template 
+class vector {
+public:
+  class iterator {};
+  // CHECK: ClassTemplateDecl {{.*}} vector
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} vector
+  // CHECK: TemplateArgument type 'int'
+  // CHECK: OwnerAttr
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+};
+static_assert(sizeof(vector), "");   // Force instantiation.
+static_assert(sizeof(vector::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a using declaration, attributes are inferred
+// for the underlying class.
+template 
+class __set_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __set_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __set_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class set {
+  // CHECK: ClassTemplateDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+public:
+  using iterator = __set_iterator;
+};
+static_assert(sizeof(set::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a typedef, attributes are inferred for the
+// underlying class.
+template 
+class __map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __map_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class map {
+  // CHECK: ClassTemplateDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __map_iterator iterator;
+};
+static_assert(sizeof(map::iterator), ""); // Force instantiation.
+
+// Inline namespaces are ignored when checking if
+// the class lives in the std namespace.
+inline namespace inlinens {
+template 
+class __unordered_map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __unordered_map_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __unordered_map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class unordered_map {
+  // CHECK: ClassTemplateDecl {{.*}} unordered_map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} unordered_map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __unordered_map_iterator iterator;
+};
+static_assert(sizeof(unordered_map::iterator), ""); // Force instantiation.
+} // namespace inlinens
+
+// std::list has an implicit gsl::Owner attribute,
+// but explicit attributes take precedence.
+template 
+class [[gsl::Pointer]] list{};
+// CHECK: ClassTemplateDecl {{.*}} list
+// CHECK: PointerAttr {{.*}}
+// CHECK: ClassTemplateSpecializationDecl {{.*}} list
+// CHECK: PointerAttr {{.*}}
+
+static_assert(sizeof(list), ""); // Force instantiation.
+
+// Forward declared template (Owner).
+template <
+class CharT,
+class Traits>
+class basic_regex;
+// CHECK: ClassTemplateDecl {{.*}} basic_regex
+// CHECK: OwnerAttr {{.*}}
+
+// Forward declared template (Pointer).
+template 
+class reference_wrapper;
+// CHECK: ClassTemplateDecl {{.*}} reference_wrapper
+// CHECK: PointerAttr {{.*}}
+
+class some_unknown_type;
+// CHECK: CXXRecordDecl {{.*}} some_unknown_type
+
+} // namespace std
+
+namespace user {
+// If a class is not in the std namespace, we don't infer the attributes.
+class any {
+};
+} // namespace user
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -1690,6 +1690,7 @@
 mergeDeclAttributes(NewClass, PrevClassTemplate->getTemplatedDecl());
 
   AddPushedVisibilityAttribute(NewClass);
+  

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added inline comments.



Comment at: clang/include/clang/Sema/Sema.h:6097
+
+  /// Add [[gsl::Owner]] and [[gsl::Pointer]] attributes for std:: types.
+  void addDefaultGslPointerAttribute(TypedefNameDecl *TD);

gribozavr wrote:
> It seems like this function does not add gsl::Owner.
Fixed comment



Comment at: clang/lib/Sema/SemaTemplate.cpp:1689
   AddPushedVisibilityAttribute(NewClass);
+  addDefaultGslOwnerPointerAttribute(NewClass);
 

gribozavr wrote:
> It shouldn't be necessary to perform inference here, instead, the attributes 
> should be instantiated, see `instantiateDependentAlignedAttr` in 
> SemaTemplateInstantiateDecl.cpp for an example.
From what I understand, here the attribute is put on the `ClassTemplateDecl`. 
Without this, there would be no attribute to instantiate from?
The instantiation of OwnerAttr/PointerAttr is auto-generated into 
instantiateTemplateAttribute() in 
`build/tools/clang/include/clang/Sema/AttrTemplateInstantiate.inc`.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:702
   SemaRef.InstantiateAttrs(TemplateArgs, D, Typedef);
+  SemaRef.addDefaultGslPointerAttribute(Typedef);
 

gribozavr wrote:
> Ditto, should not be necessary to perform inference here.
I'll move this to already act on the typedef in the ClassTemplateDecl (instead 
of the instantiation here).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448



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

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 212439.
mgehre marked 6 inline comments as done.
mgehre added a comment.

- Fix comments
- Add Pointer via typedef on ClassTemplateDecl


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448

Files:
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp

Index: clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 -ast-dump %s | \
+// RUN: FileCheck --implicit-check-not OwnerAttr --implicit-check-not PointerAttr %s
+
+// Test attribute inference for types in the standard library.
+namespace std {
+// Attributes are inferred for a (complete) class.
+class any {
+  // CHECK: CXXRecordDecl {{.*}} any
+  // CHECK: OwnerAttr {{.*}}
+};
+
+// Attributes are inferred for instantiations of a complete template.
+template 
+class vector {
+public:
+  class iterator {};
+  // CHECK: ClassTemplateDecl {{.*}} vector
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} vector
+  // CHECK: TemplateArgument type 'int'
+  // CHECK: OwnerAttr
+  // CHECK: CXXRecordDecl {{.*}} iterator
+  // CHECK: PointerAttr {{.*}}
+};
+static_assert(sizeof(vector), "");   // Force instantiation.
+static_assert(sizeof(vector::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a using declaration, attributes are inferred
+// for the underlying class.
+template 
+class __set_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __set_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __set_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class set {
+  // CHECK: ClassTemplateDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} set
+  // CHECK: OwnerAttr {{.*}}
+public:
+  using iterator = __set_iterator;
+};
+static_assert(sizeof(set::iterator), ""); // Force instantiation.
+
+// If std::container::iterator is a typedef, attributes are inferred for the
+// underlying class.
+template 
+class __map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __map_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class map {
+  // CHECK: ClassTemplateDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __map_iterator iterator;
+};
+static_assert(sizeof(map::iterator), ""); // Force instantiation.
+
+// Inline namespaces are ignored when checking if
+// the class lives in the std namespace.
+inline namespace inlinens {
+template 
+class __unordered_map_iterator {};
+// CHECK: ClassTemplateDecl {{.*}} __unordered_map_iterator
+// CHECK: PointerAttr
+// CHECK: ClassTemplateSpecializationDecl {{.*}} __unordered_map_iterator
+// CHECK: TemplateArgument type 'int'
+// CHECK: PointerAttr
+
+template 
+class unordered_map {
+  // CHECK: ClassTemplateDecl {{.*}} unordered_map
+  // CHECK: OwnerAttr {{.*}}
+  // CHECK: ClassTemplateSpecializationDecl {{.*}} unordered_map
+  // CHECK: OwnerAttr {{.*}}
+public:
+  typedef __unordered_map_iterator iterator;
+};
+static_assert(sizeof(unordered_map::iterator), ""); // Force instantiation.
+} // namespace inlinens
+
+// std::list has an implicit gsl::Owner attribute,
+// but explicit attributes take precedence.
+template 
+class [[gsl::Pointer]] list{};
+// CHECK: ClassTemplateDecl {{.*}} list
+// CHECK: PointerAttr {{.*}}
+// CHECK: ClassTemplateSpecializationDecl {{.*}} list
+// CHECK: PointerAttr {{.*}}
+
+static_assert(sizeof(list), ""); // Force instantiation.
+
+// Forward declared template (Owner).
+template <
+class CharT,
+class Traits>
+class basic_regex;
+// CHECK: ClassTemplateDecl {{.*}} basic_regex
+// CHECK: OwnerAttr {{.*}}
+
+// Forward declared template (Pointer).
+template 
+class reference_wrapper;
+// CHECK: ClassTemplateDecl {{.*}} reference_wrapper
+// CHECK: PointerAttr {{.*}}
+
+class some_unknown_type;
+// CHECK: CXXRecordDecl {{.*}} some_unknown_type
+
+} // namespace std
+
+namespace user {
+// If a class is not in the std namespace, we don't infer the attributes.
+class any {
+};
+} // namespace user
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -1690,6 +1690,7 @@
 mergeDeclAttributes(NewClass, PrevClassTemplate->getTemplatedDecl());
 
   

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

I will include your latest comments into D64448 



Repository:
  rL LLVM

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

https://reviews.llvm.org/D63954



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

[PATCH] D65127: Even more warnings utilizing gsl::Owner/gsl::Pointer annotations

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:6581
+if (!Callee->getIdentifier()) {
+  auto OO = Callee->getOverloadedOperator();
+  return OO == OverloadedOperatorKind::OO_Subscript ||

xazax.hun wrote:
> If we want to relax the warnings to give more results we could extend the 
> checking of these overloaded operators for annotated types. But this would 
> imply that the user need to have the expected semantics for those types and 
> can only suppress false positives by removing some gsl:::owner/poinnter 
> annotations.
I see those options:
- Either gsl::Owner implies some specific form of those operators (and if that 
does not hold for a class, then one should not annotate it with gsl::Owner)
- or gsl::Owner only implies some specific behavior for the "gsl::Pointer 
constructed from gsl::Owner" case and everything else requires additional 
annotation
I expect that we will experiment a bit in the future to see what works well for 
real-world code.


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

https://reviews.llvm.org/D65127



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

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

Thank you very much for dedicating your time for the review. It improved the 
code a lot!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63954



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

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL367040: Add lifetime categories attributes (authored by 
mgehre, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D63954?vs=211600=211792#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D63954

Files:
  cfe/trunk/include/clang/Basic/Attr.td
  cfe/trunk/include/clang/Basic/AttrDocs.td
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/Parse/ParseDecl.cpp
  cfe/trunk/lib/Sema/SemaDeclAttr.cpp
  cfe/trunk/test/AST/ast-dump-attr.cpp
  cfe/trunk/test/Misc/pragma-attribute-supported-attributes-list.test
  cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp
  cfe/trunk/test/SemaOpenCL/invalid-kernel-attrs.cl
  cfe/trunk/utils/TableGen/ClangAttrEmitter.cpp

Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2519,6 +2519,9 @@
   "'NSObject' attribute is for pointer types only">;
 def err_attributes_are_not_compatible : Error<
   "%0 and %1 attributes are not compatible">;
+def err_attribute_invalid_argument : Error<
+  "%select{'void'|a reference type|an array type|a non-vector or "
+  "non-vectorizable scalar type}0 is an invalid argument to attribute %1">;
 def err_attribute_wrong_number_arguments : Error<
   "%0 attribute %plural{0:takes no arguments|1:takes one argument|"
   ":requires exactly %1 arguments}1">;
@@ -2567,8 +2570,6 @@
 def err_init_priority_object_attr : Error<
   "can only use 'init_priority' attribute on file-scope definitions "
   "of objects of class type">;
-def err_attribute_argument_vec_type_hint : Error<
-  "invalid attribute argument %0 - expecting a vector or vectorizable scalar type">;
 def err_attribute_argument_out_of_bounds : Error<
   "%0 attribute parameter %1 is out of bounds">;
 def err_attribute_only_once_per_parameter : Error<
Index: cfe/trunk/include/clang/Basic/AttrDocs.td
===
--- cfe/trunk/include/clang/Basic/AttrDocs.td
+++ cfe/trunk/include/clang/Basic/AttrDocs.td
@@ -4230,3 +4230,71 @@
 the initializer on host side.
   }];
 }
+
+def LifetimeOwnerDocs : Documentation {
+  let Category = DocCatDecl;
+  let Content = [{
+.. Note:: This attribute is experimental and its effect on analysis is subject to change in
+  a future version of clang.
+
+The attribute ``[[gsl::Owner(T)]]`` applies to structs and classes that own an
+object of type ``T``:
+
+.. code-block:: c++
+
+  class [[gsl::Owner(int)]] IntOwner {
+  private:
+int value;
+  public:
+int *getInt() { return  }
+  };
+
+The argument ``T`` is optional and currently ignored.
+This attribute may be used by analysis tools and has no effect on code
+generation.
+
+See Pointer_ for an example.
+}];
+}
+
+def LifetimePointerDocs : Documentation {
+  let Category = DocCatDecl;
+  let Content = [{
+.. Note:: This attribute is experimental and its effect on analysis is subject to change in
+  a future version of clang.
+
+The attribute ``[[gsl::Pointer(T)]]`` applies to structs and classes that behave
+like pointers to an object of type ``T``:
+
+.. code-block:: c++
+
+  class [[gsl::Pointer(int)]] IntPointer {
+  private:
+int *valuePointer;
+  public:
+int *getInt() { return  }
+  };
+
+The argument ``T`` is optional and currently ignored.
+This attribute may be used by analysis tools and has no effect on code
+generation.
+
+Example:
+When constructing an instance of a class annotated like this (a Pointer) from
+an instance of a class annotated with ``[[gsl::Owner]]`` (an Owner),
+then the analysis will consider the Pointer to point inside the Owner.
+When the Owner's lifetime ends, it will consider the Pointer to be dangling.
+
+.. code-block:: c++
+
+  int f() {
+IntPointer P;
+if (true) {
+  IntOwner O(7);
+  P = IntPointer(O); // P "points into" O
+} // P is dangling
+return P.get(); // error: Using a dangling Pointer.
+  }
+
+}];
+}
Index: cfe/trunk/include/clang/Basic/Attr.td
===
--- cfe/trunk/include/clang/Basic/Attr.td
+++ cfe/trunk/include/clang/Basic/Attr.td
@@ -2796,6 +2796,20 @@
   let Documentation = [TypeTagForDatatypeDocs];
 }
 
+def Owner : InheritableAttr {
+  let Spellings = [CXX11<"gsl", "Owner">];
+  let Subjects = SubjectList<[Struct]>;
+  let Args = [TypeArgument<"DerefType", /*opt=*/1>];
+  let Documentation = [LifetimeOwnerDocs];
+}
+
+def Pointer : InheritableAttr {
+  let Spellings = [CXX11<"gsl", "Pointer">];
+  let Subjects = SubjectList<[Struct]>;
+  let Args = [TypeArgument<"DerefType", /*opt=*/1>];
+  let Documentation = [LifetimePointerDocs];
+}
+
 // 

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

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



Comment at: clang/test/SemaCXX/attr-gsl-owner-pointer-parsing.cpp:2
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int [[gsl::Owner]] i;

aaron.ballman wrote:
> This file tests part of parsing and part of Sema somewhat interchangeably. 
> I'm not strictly opposed to it being that way, but it would be a bit cleaner 
> to have the parsing tests in the Parser directory and the rest in SemaCXX 
> (that also reduces the confusion from the file name).
I had split the tests into two files due to a misconception that the ast-dump 
would not work when errors are emitted. It actually does work, and so I 
recombined the test cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



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

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 211600.
mgehre marked an inline comment as done.
mgehre added a comment.

- Fix all comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
  clang/test/SemaOpenCL/invalid-kernel-attrs.cl
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -303,6 +303,8 @@
 std::string getIsOmitted() const override {
   if (type == "IdentifierInfo *")
 return "!get" + getUpperName().str() + "()";
+  if (type == "TypeSourceInfo *")
+return "!get" + getUpperName().str() + "Loc()";
   if (type == "ParamIdx")
 return "!get" + getUpperName().str() + "().isValid()";
   return "false";
@@ -336,6 +338,8 @@
<< "  OS << \" \" << SA->get" << getUpperName()
<< "()->getName();\n";
   } else if (type == "TypeSourceInfo *") {
+if (isOptional())
+  OS << "if (SA->get" << getUpperName() << "Loc())";
 OS << "OS << \" \" << SA->get" << getUpperName()
<< "().getAsString();\n";
   } else if (type == "bool") {
Index: clang/test/SemaOpenCL/invalid-kernel-attrs.cl
===
--- clang/test/SemaOpenCL/invalid-kernel-attrs.cl
+++ clang/test/SemaOpenCL/invalid-kernel-attrs.cl
@@ -1,12 +1,12 @@
-// RUN: %clang_cc1 -verify %s 
+// RUN: %clang_cc1 -verify %s
 
 kernel __attribute__((vec_type_hint)) void kernel1() {} //expected-error{{'vec_type_hint' attribute takes one argument}}
 
 kernel __attribute__((vec_type_hint(not_type))) void kernel2() {} //expected-error{{unknown type name 'not_type'}}
 
-kernel __attribute__((vec_type_hint(void))) void kernel3() {} //expected-error{{invalid attribute argument 'void' - expecting a vector or vectorizable scalar type}}
+kernel __attribute__((vec_type_hint(void))) void kernel3() {} //expected-error{{a non-vector or non-vectorizable scalar type is an invalid argument to attribute 'vec_type_hint'}}
 
-kernel __attribute__((vec_type_hint(bool))) void kernel4() {} //expected-error{{invalid attribute argument 'bool' - expecting a vector or vectorizable scalar type}}
+kernel __attribute__((vec_type_hint(bool))) void kernel4() {} //expected-error{{a non-vector or non-vectorizable scalar type is an invalid argument to attribute 'vec_type_hint'}}
 
 kernel __attribute__((vec_type_hint(int))) __attribute__((vec_type_hint(float))) void kernel5() {} //expected-warning{{attribute 'vec_type_hint' is already applied with different parameters}}
 
Index: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
@@ -0,0 +1,107 @@
+// RUN: %clang_cc1 -verify -ast-dump %s | \
+// RUN: FileCheck --implicit-check-not OwnerAttr --implicit-check-not PointerAttr %s
+
+int [[gsl::Owner]] i;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+void [[gsl::Owner]] f();
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+[[gsl::Owner]] void f();
+// expected-warning@-1 {{'Owner' attribute only applies to structs}}
+
+union [[gsl::Owner(int)]] Union{};
+// expected-warning@-1 {{'Owner' attribute only applies to structs}}
+
+struct S {
+};
+
+S [[gsl::Owner]] Instance;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+class [[gsl::Owner(7)]] OwnerDerefNoType{};
+// expected-error@-1 {{expected a type}}
+
+class [[gsl::Pointer("int")]] PointerDerefNoType{};
+// expected-error@-1 {{expected a type}}
+
+class [[gsl::Owner(int)]] [[gsl::Pointer(int)]] BothOwnerPointer{};
+// expected-error@-1 {{'Pointer' and 'Owner' attributes are not compatible}}
+// expected-note@-2 {{conflicting attribute is here}}
+// CHECK: CXXRecordDecl {{.*}} BothOwnerPointer
+// CHECK: OwnerAttr {{.*}} int
+
+class [[gsl::Owner(void)]] OwnerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Owner'}}
+class [[gsl::Pointer(void)]] PointerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Pointer'}}
+
+class [[gsl::Pointer(int)]] AddConflictLater{};
+// CHECK: CXXRecordDecl {{.*}} AddConflictLater
+// CHECK: PointerAttr {{.*}} int
+class [[gsl::Owner(int)]] AddConflictLater;
+// expected-error@-1 {{'Owner' and 'Pointer' attributes are not compatible}}
+// expected-note@-5 

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

mgehre updated this revision to Diff 211370.
mgehre marked 4 inline comments as done.
mgehre added a comment.

- Diagnose unions; improve other diagnostics; fix all comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaCXX/attr-gsl-owner-pointer-parsing.cpp
  clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp

Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -303,6 +303,8 @@
 std::string getIsOmitted() const override {
   if (type == "IdentifierInfo *")
 return "!get" + getUpperName().str() + "()";
+  if (type == "TypeSourceInfo *")
+return "!get" + getUpperName().str() + "Loc()";
   if (type == "ParamIdx")
 return "!get" + getUpperName().str() + "().isValid()";
   return "false";
@@ -336,6 +338,8 @@
<< "  OS << \" \" << SA->get" << getUpperName()
<< "()->getName();\n";
   } else if (type == "TypeSourceInfo *") {
+if (isOptional())
+  OS << "if (SA->get" << getUpperName() << "Loc())";
 OS << "OS << \" \" << SA->get" << getUpperName()
<< "().getAsString();\n";
   } else if (type == "bool") {
Index: clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -ast-dump %s | \
+// RUN: FileCheck --implicit-check-not OwnerAttr --implicit-check-not PointerAttr %s
+
+class [[gsl::Owner]] OwnerMissingParameter{};
+// CHECK: CXXRecordDecl {{.*}} OwnerMissingParameter
+// CHECK: OwnerAttr
+
+class [[gsl::Pointer]] PointerMissingParameter{};
+// CHECK: CXXRecordDecl {{.*}} PointerMissingParameter
+// CHECK: PointerAttr
+
+class [[gsl::Owner()]] OwnerWithEmptyParameterList{};
+// CHECK: CXXRecordDecl {{.*}} OwnerWithEmptyParameterList
+// CHECK: OwnerAttr {{.*}}
+
+class [[gsl::Pointer()]] PointerWithEmptyParameterList{};
+// CHECK: CXXRecordDecl {{.*}} PointerWithEmptyParameterList
+// CHECK: PointerAttr {{.*}}
+
+class [[gsl::Owner(int)]] AnOwner{};
+// CHECK: CXXRecordDecl {{.*}} AnOwner
+// CHECK: OwnerAttr {{.*}} int
+
+struct S;
+class [[gsl::Pointer(S)]] APointer{};
+// CHECK: CXXRecordDecl {{.*}} APointer
+// CHECK: PointerAttr {{.*}} S
+
+class [[gsl::Owner(int)]] [[gsl::Owner(int)]] DuplicateOwner{};
+// CHECK: CXXRecordDecl {{.*}} DuplicateOwner
+// CHECK: OwnerAttr {{.*}} int
+
+class [[gsl::Pointer(int)]] [[gsl::Pointer(int)]] DuplicatePointer{};
+// CHECK: CXXRecordDecl {{.*}} DuplicatePointer
+// CHECK: PointerAttr {{.*}} int
+
+class [[gsl::Owner(int)]] AddTheSameLater{};
+// CHECK: CXXRecordDecl {{.*}} AddTheSameLater
+// CHECK: OwnerAttr {{.*}} int
+
+class [[gsl::Owner(int)]] AddTheSameLater;
+// CHECK: CXXRecordDecl {{.*}} prev {{.*}} AddTheSameLater
+// CHECK: OwnerAttr {{.*}} int
Index: clang/test/SemaCXX/attr-gsl-owner-pointer-parsing.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-gsl-owner-pointer-parsing.cpp
@@ -0,0 +1,53 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+int [[gsl::Owner]] i;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+void [[gsl::Owner]] f();
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+[[gsl::Owner]] void f();
+// expected-warning@-1 {{'Owner' attribute only applies to classes}}
+
+struct S {
+};
+
+S [[gsl::Owner]] Instance;
+// expected-error@-1 {{'Owner' attribute cannot be applied to types}}
+
+class [[gsl::Owner(7)]] OwnerDerefNoType{};
+// expected-error@-1 {{expected a type}}
+
+class [[gsl::Pointer("int")]] PointerDerefNoType{};
+// expected-error@-1 {{expected a type}}
+
+class [[gsl::Owner(int)]] [[gsl::Pointer(int)]] BothOwnerPointer{};
+// expected-error@-1 {{'Pointer' and 'Owner' attributes are not compatible}}
+// expected-note@-2 {{conflicting attribute is here}}
+
+class [[gsl::Owner(void)]] OwnerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Owner'}}
+class [[gsl::Pointer(void)]] PointerVoidDerefType{};
+// expected-error@-1 {{'void' is an invalid argument to attribute 'Pointer'}}
+
+class [[gsl::Pointer(int)]] AddConflictLater{};
+class [[gsl::Owner(int)]] AddConflictLater;
+// expected-error@-1 {{'Owner' and 'Pointer' attributes are not compatible}}
+// expected-note@-3 

[PATCH] D63954: Add lifetime categories attributes

[Matthias Gehre via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/Basic/AttrDocs.td:4167
+
+The attribute ``[[gsl::Owner(T)]]`` applies to structs and classes that own an
+object of type ``T``:

aaron.ballman wrote:
> Do either of these attributes make sense on a union type? If so, might be 
> worth mentioning unions here and below. If not, would it be worth diagnosing 
> on a union?
Good catch! I added diagnostics and a test.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2516
   "%0 and %1 attributes are not compatible">;
+def err_attribute_invalid_argument : Error<
+  "%0 is an invalid argument to attribute %1">;

aaron.ballman wrote:
> Can you combine this one with `err_attribute_argument_vec_type_hint`?
I'm not sure: vec_type_hint reads `"invalid attribute argument %0 - expecting a 
vector or vectorizable scalar type"` and here `""%0 is an invalid argument to 
attribute %1"`, i.e. one is positive ("expecting ...") and the other is 
negative ("%0 is an invalid argument").

I don't know how to describe "not void, not reference, not array type" in terms 
of "expecting ...", and I think that we should keep "expecting a vector or 
vectorizable scalar type" on the  VecTypeHint attribute diagnostic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63954



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

[PATCH] D15032: [clang-tidy] new checker cppcoreguidelines-pro-lifetime

[Matthias Gehre via Phabricator via cfe-commits]

mgehre abandoned this revision.
mgehre added a comment.
Herald added subscribers: wuzish, dmgreen.

Upstreaming of this checker will happen with D63954 
 and following revisions.


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

https://reviews.llvm.org/D15032



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

[PATCH] D64448: gsl::Owner/gsl::Pointer: Add implicit annotations for some std types

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

In D64448#1595017 , @lebedev.ri wrote:

> Just passing-by thought:
>  These attributes that are being added implicitly, it will be possible to 
> differentiate
>  whether an attribute was actually present in the code, or was added 
> implicitly,
>  say for clang-tidy check purposes?
>  Is there some 'implicit' bit on these, or they have a location not in the 
> source buffer?


Thanks for the question! The implicitly added attributes have an invalid source 
location.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64448



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