r336137 - [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Mon Jul  2 12:21:36 2018
New Revision: 336137

URL: http://llvm.org/viewvc/llvm-project?rev=336137=rev
Log:
[CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers

Summary:
Emmiting new intrinsic that strips invariant.groups to make
devirtulization sound, as described in RFC: Devirtualization v2.

Reviewers: rjmccall, rsmith, amharc, kuhar

Subscribers: llvm-commits, cfe-commits

Differential Revision: https://reviews.llvm.org/D47299

Co-authored-by: Krzysztof Pszeniczny 

Modified:
cfe/trunk/include/clang/AST/DeclCXX.h
cfe/trunk/include/clang/AST/Type.h
cfe/trunk/lib/AST/Type.cpp
cfe/trunk/lib/CodeGen/CGExpr.cpp
cfe/trunk/lib/CodeGen/CGExprScalar.cpp
cfe/trunk/test/CodeGenCXX/strict-vtable-pointers.cpp

Modified: cfe/trunk/include/clang/AST/DeclCXX.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=336137=336136=336137=diff
==
--- cfe/trunk/include/clang/AST/DeclCXX.h (original)
+++ cfe/trunk/include/clang/AST/DeclCXX.h Mon Jul  2 12:21:36 2018
@@ -790,6 +790,18 @@ public:
 return data().Polymorphic || data().NumVBases != 0;
   }
 
+  /// @returns true if class is dynamic or might be dynamic because the
+  /// definition is incomplete of dependent.
+  bool mayBeDynamicClass() const {
+return !hasDefinition() || isDynamicClass() || hasAnyDependentBases();
+  }
+
+  /// @returns true if class is non dynamic or might be non dynamic because the
+  /// definition is incomplete of dependent.
+  bool mayBeNonDynamicClass() const {
+return !hasDefinition() || !isDynamicClass() || hasAnyDependentBases();
+  }
+
   void setIsParsingBaseSpecifiers() { data().IsParsingBaseSpecifiers = true; }
 
   bool isParsingBaseSpecifiers() const {

Modified: cfe/trunk/include/clang/AST/Type.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Type.h?rev=336137=336136=336137=diff
==
--- cfe/trunk/include/clang/AST/Type.h (original)
+++ cfe/trunk/include/clang/AST/Type.h Mon Jul  2 12:21:36 2018
@@ -810,6 +810,13 @@ public:
   /// Return true if this is a trivially copyable type (C++0x [basic.types]p9)
   bool isTriviallyCopyableType(const ASTContext ) const;
 
+
+  /// Returns true if it is a class and it might be dynamic.
+  bool mayBeDynamicClass() const;
+
+  /// Returns true if it is not a class or if the class might not be dynamic.
+  bool mayBeNotDynamicClass() const;
+
   // Don't promise in the API that anything besides 'const' can be
   // easily added.
 

Modified: cfe/trunk/lib/AST/Type.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Type.cpp?rev=336137=336136=336137=diff
==
--- cfe/trunk/lib/AST/Type.cpp (original)
+++ cfe/trunk/lib/AST/Type.cpp Mon Jul  2 12:21:36 2018
@@ -88,6 +88,16 @@ const IdentifierInfo* QualType::getBaseT
   return nullptr;
 }
 
+bool QualType::mayBeDynamicClass() const {
+  const auto *ClassDecl = getTypePtr()->getPointeeCXXRecordDecl();
+  return ClassDecl && ClassDecl->mayBeDynamicClass();
+}
+
+bool QualType::mayBeNotDynamicClass() const {
+  const auto *ClassDecl = getTypePtr()->getPointeeCXXRecordDecl();
+  return !ClassDecl || ClassDecl->mayBeNonDynamicClass();
+}
+
 bool QualType::isConstant(QualType T, const ASTContext ) {
   if (T.isConstQualified())
 return true;

Modified: cfe/trunk/lib/CodeGen/CGExpr.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExpr.cpp?rev=336137=336136=336137=diff
==
--- cfe/trunk/lib/CodeGen/CGExpr.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExpr.cpp Mon Jul  2 12:21:36 2018
@@ -3870,6 +3870,18 @@ LValue CodeGenFunction::EmitLValueForFie
   }
 
   Address addr = base.getAddress();
+  if (auto *ClassDef = dyn_cast(rec)) {
+if (CGM.getCodeGenOpts().StrictVTablePointers &&
+ClassDef->isDynamicClass()) {
+  // Getting to any field of dynamic object requires stripping dynamic
+  // information provided by invariant.group.  This is because accessing
+  // fields may leak the real address of dynamic object, which could result
+  // in miscompilation when leaked pointer would be compared.
+  auto *stripped = Builder.CreateStripInvariantGroup(addr.getPointer());
+  addr = Address(stripped, addr.getAlignment());
+}
+  }
+
   unsigned RecordCVR = base.getVRQualifiers();
   if (rec->isUnion()) {
 // For unions, there is no pointer adjustment.

Modified: cfe/trunk/lib/CodeGen/CGExprScalar.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprScalar.cpp?rev=336137=336136=336137=diff
==
--- cfe/trunk/lib/CodeGen/CGExprScalar.cpp (original)
+++ 

r334600 - Add -fforce-emit-vtables

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Wed Jun 13 06:55:42 2018
New Revision: 334600

URL: http://llvm.org/viewvc/llvm-project?rev=334600=rev
Log:
Add -fforce-emit-vtables

Summary:
 In many cases we can't devirtualize
 because definition of vtable is not present. Most of the
 time it is caused by inline virtual function not beeing
 emitted. Forcing emitting of vtable adds a reference of these
 inline virtual functions.
 Note that GCC was always doing it.

Reviewers: rjmccall, rsmith, amharc, kuhar

Subscribers: llvm-commits, cfe-commits

Differential Revision: https://reviews.llvm.org/D47108

Co-authored-by: Krzysztof Pszeniczny 

Modified:
cfe/trunk/docs/ClangCommandLineReference.rst
cfe/trunk/docs/ReleaseNotes.rst
cfe/trunk/docs/UsersManual.rst
cfe/trunk/include/clang/Basic/LangOptions.def
cfe/trunk/include/clang/Driver/Options.td
cfe/trunk/include/clang/Frontend/CodeGenOptions.def
cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
cfe/trunk/lib/Frontend/CompilerInvocation.cpp
cfe/trunk/lib/Sema/SemaDeclCXX.cpp
cfe/trunk/test/CodeGenCXX/vtable-available-externally.cpp

Modified: cfe/trunk/docs/ClangCommandLineReference.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ClangCommandLineReference.rst?rev=334600=334599=334600=diff
==
--- cfe/trunk/docs/ClangCommandLineReference.rst (original)
+++ cfe/trunk/docs/ClangCommandLineReference.rst Wed Jun 13 06:55:42 2018
@@ -1934,6 +1934,12 @@ Set the default symbol visibility for al
 
 Enables whole-program vtable optimization. Requires -flto
 
+.. option:: -fforce-emit-vtables, -fno-force-emit-vtables
+
+In order to improve devirtualization, forces emitting of vtables even in
+modules where it isn't necessary. It causes more inline virtual functions
+to be emitted.
+
 .. option:: -fwrapv, -fno-wrapv
 
 Treat signed integer overflow as two's complement

Modified: cfe/trunk/docs/ReleaseNotes.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=334600=334599=334600=diff
==
--- cfe/trunk/docs/ReleaseNotes.rst (original)
+++ cfe/trunk/docs/ReleaseNotes.rst Wed Jun 13 06:55:42 2018
@@ -112,6 +112,12 @@ New Compiler Flags
   'no-strict' option, Clang attempts to match the overflowing behavior of the
   target's native float-to-int conversion instructions.
 
+- :option: `-fforce-emit-vtables` and `-fno-force-emit-vtables`.
+
+   In order to improve devirtualization, forces emitting of vtables even in
+   modules where it isn't necessary. It causes more inline virtual functions
+   to be emitted.
+
 - ...
 
 Deprecated Compiler Flags

Modified: cfe/trunk/docs/UsersManual.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/UsersManual.rst?rev=334600=334599=334600=diff
==
--- cfe/trunk/docs/UsersManual.rst (original)
+++ cfe/trunk/docs/UsersManual.rst Wed Jun 13 06:55:42 2018
@@ -1269,6 +1269,12 @@ are listed below.
devirtualization and virtual constant propagation, for classes with
:doc:`hidden LTO visibility `. Requires ``-flto``.
 
+.. option:: -fforce-emit-vtables
+
+   In order to improve devirtualization, forces emitting of vtables even in
+   modules where it isn't necessary. It causes more inline virtual functions
+   to be emitted.
+
 .. option:: -fno-assume-sane-operator-new
 
Don't assume that the C++'s new operator is sane.

Modified: cfe/trunk/include/clang/Basic/LangOptions.def
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/LangOptions.def?rev=334600=334599=334600=diff
==
--- cfe/trunk/include/clang/Basic/LangOptions.def (original)
+++ cfe/trunk/include/clang/Basic/LangOptions.def Wed Jun 13 06:55:42 2018
@@ -293,6 +293,8 @@ LANGOPT(XRayAlwaysEmitTypedEvents, 1, 0,
 "controls whether to always emit intrinsic calls to "
 "__xray_typedevent(...) builtin.")
 
+LANGOPT(ForceEmitVTables, 1, 0, "whether to emit all vtables")
+
 BENIGN_LANGOPT(AllowEditorPlaceholders, 1, 0,
"allow editor placeholders in source")
 

Modified: cfe/trunk/include/clang/Driver/Options.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=334600=334599=334600=diff
==
--- cfe/trunk/include/clang/Driver/Options.td (original)
+++ cfe/trunk/include/clang/Driver/Options.td Wed Jun 13 06:55:42 2018
@@ -1688,6 +1688,11 @@ def fwhole_program_vtables : Flag<["-"],
   HelpText<"Enables whole-program vtable optimization. Requires -flto">;
 def fno_whole_program_vtables : Flag<["-"], "fno-whole-program-vtables">, 
Group,
   Flags<[CoreOption]>;
+def fforce_emit_vtables : Flag<["-"], "fforce-emit-vtables">, Group,
+

r331448 - Rename invariant.group.barrier to launder.invariant.group

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Thu May  3 04:03:01 2018
New Revision: 331448

URL: http://llvm.org/viewvc/llvm-project?rev=331448=rev
Log:
Rename invariant.group.barrier to launder.invariant.group

Summary:
This is one of the initial commit of "RFC: Devirtualization v2" proposal:
https://docs.google.com/document/d/16GVtCpzK8sIHNc2qZz6RN8amICNBtvjWUod2SujZVEo/edit?usp=sharing

Reviewers: rsmith, amharc, kuhar, sanjoy

Subscribers: arsenm, nhaehnle, javed.absar, hiraditya, llvm-commits

Differential Revision: https://reviews.llvm.org/D45111

Modified:
cfe/trunk/lib/CodeGen/CGClass.cpp
cfe/trunk/lib/CodeGen/CGExpr.cpp
cfe/trunk/lib/CodeGen/CGExprCXX.cpp
cfe/trunk/test/CodeGenCXX/invariant.group-for-vptrs.cpp
cfe/trunk/test/CodeGenCXX/strict-vtable-pointers.cpp

Modified: cfe/trunk/lib/CodeGen/CGClass.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGClass.cpp?rev=331448=331447=331448=diff
==
--- cfe/trunk/lib/CodeGen/CGClass.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGClass.cpp Thu May  3 04:03:01 2018
@@ -1276,7 +1276,7 @@ void CodeGenFunction::EmitCtorPrologue(c
 if (CGM.getCodeGenOpts().StrictVTablePointers &&
 CGM.getCodeGenOpts().OptimizationLevel > 0 &&
 isInitializerOfDynamicClass(*B))
-  CXXThisValue = Builder.CreateInvariantGroupBarrier(LoadCXXThis());
+  CXXThisValue = Builder.CreateLaunderInvariantGroup(LoadCXXThis());
 EmitBaseInitializer(*this, ClassDecl, *B, CtorType);
   }
 
@@ -1293,7 +1293,7 @@ void CodeGenFunction::EmitCtorPrologue(c
 if (CGM.getCodeGenOpts().StrictVTablePointers &&
 CGM.getCodeGenOpts().OptimizationLevel > 0 &&
 isInitializerOfDynamicClass(*B))
-  CXXThisValue = Builder.CreateInvariantGroupBarrier(LoadCXXThis());
+  CXXThisValue = Builder.CreateLaunderInvariantGroup(LoadCXXThis());
 EmitBaseInitializer(*this, ClassDecl, *B, CtorType);
   }
 
@@ -1477,11 +1477,11 @@ void CodeGenFunction::EmitDestructorBody
 
 // Initialize the vtable pointers before entering the body.
 if (!CanSkipVTablePointerInitialization(*this, Dtor)) {
-  // Insert the llvm.invariant.group.barrier intrinsic before initializing
+  // Insert the llvm.launder.invariant.group intrinsic before initializing
   // the vptrs to cancel any previous assumptions we might have made.
   if (CGM.getCodeGenOpts().StrictVTablePointers &&
   CGM.getCodeGenOpts().OptimizationLevel > 0)
-CXXThisValue = Builder.CreateInvariantGroupBarrier(LoadCXXThis());
+CXXThisValue = Builder.CreateLaunderInvariantGroup(LoadCXXThis());
   InitializeVTablePointers(Dtor->getParent());
 }
 

Modified: cfe/trunk/lib/CodeGen/CGExpr.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExpr.cpp?rev=331448=331447=331448=diff
==
--- cfe/trunk/lib/CodeGen/CGExpr.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExpr.cpp Thu May  3 04:03:01 2018
@@ -3850,7 +3850,7 @@ LValue CodeGenFunction::EmitLValueForFie
 hasAnyVptr(FieldType, getContext()))
   // Because unions can easily skip invariant.barriers, we need to add
   // a barrier every time CXXRecord field with vptr is referenced.
-  addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()),
+  addr = Address(Builder.CreateLaunderInvariantGroup(addr.getPointer()),
  addr.getAlignment());
   } else {
 // For structs, we GEP to the field that the record layout suggests.

Modified: cfe/trunk/lib/CodeGen/CGExprCXX.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprCXX.cpp?rev=331448=331447=331448=diff
==
--- cfe/trunk/lib/CodeGen/CGExprCXX.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExprCXX.cpp Thu May  3 04:03:01 2018
@@ -1696,13 +1696,13 @@ llvm::Value *CodeGenFunction::EmitCXXNew
   llvm::Type *elementTy = ConvertTypeForMem(allocType);
   Address result = Builder.CreateElementBitCast(allocation, elementTy);
 
-  // Passing pointer through invariant.group.barrier to avoid propagation of
+  // Passing pointer through launder.invariant.group to avoid propagation of
   // vptrs information which may be included in previous type.
   // To not break LTO with different optimizations levels, we do it regardless
   // of optimization level.
   if (CGM.getCodeGenOpts().StrictVTablePointers &&
   allocator->isReservedGlobalPlacementOperator())
-result = Address(Builder.CreateInvariantGroupBarrier(result.getPointer()),
+result = Address(Builder.CreateLaunderInvariantGroup(result.getPointer()),
  result.getAlignment());
 
   EmitNewInitializer(*this, E, allocType, elementTy, result, numElements,

Modified: cfe/trunk/test/CodeGenCXX/invariant.group-for-vptrs.cpp
URL: 

r313278 - Enable __declspec(selectany) on any platform

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Thu Sep 14 10:33:08 2017
New Revision: 313278

URL: http://llvm.org/viewvc/llvm-project?rev=313278=rev
Log:
Enable __declspec(selectany) on any platform

Summary:
This feature was disabled probably by mistake in rL300562
This fixes bug https://bugs.llvm.org/show_bug.cgi?id=33285

Reviewers: davide, rnk

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D33852

Modified:
cfe/trunk/include/clang/Basic/Attr.td
cfe/trunk/include/clang/Basic/AttrDocs.td
cfe/trunk/test/Sema/attr-selectany.c
cfe/trunk/test/SemaCXX/attr-selectany.cpp

Modified: cfe/trunk/include/clang/Basic/Attr.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=313278=313277=313278=diff
==
--- cfe/trunk/include/clang/Basic/Attr.td (original)
+++ cfe/trunk/include/clang/Basic/Attr.td Thu Sep 14 10:33:08 2017
@@ -2472,9 +2472,9 @@ def DLLImport : InheritableAttr, TargetS
   let Documentation = [DLLImportDocs];
 }
 
-def SelectAny : InheritableAttr, TargetSpecificAttr {
+def SelectAny : InheritableAttr {
   let Spellings = [Declspec<"selectany">, GCC<"selectany">];
-  let Documentation = [Undocumented];
+  let Documentation = [SelectAnyDocs];
 }
 
 def Thread : Attr {

Modified: cfe/trunk/include/clang/Basic/AttrDocs.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/AttrDocs.td?rev=313278=313277=313278=diff
==
--- cfe/trunk/include/clang/Basic/AttrDocs.td (original)
+++ cfe/trunk/include/clang/Basic/AttrDocs.td Thu Sep 14 10:33:08 2017
@@ -3192,3 +3192,18 @@ This attribute can be added to an Object
 ensure that this class cannot be subclassed.
   }];
 }
+
+
+def SelectAnyDocs : Documentation {
+  let Category = DocCatType;
+  let Content = [{
+This attribute appertains to a global symbol, causing it to have a weak
+definition (
+`linkonce `_
+), allowing the linker to select any definition.
+
+For more information see
+`gcc documentation 
`_
+or `msvc documentation `_.
+}];
+}

Modified: cfe/trunk/test/Sema/attr-selectany.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/attr-selectany.c?rev=313278=313277=313278=diff
==
--- cfe/trunk/test/Sema/attr-selectany.c (original)
+++ cfe/trunk/test/Sema/attr-selectany.c Thu Sep 14 10:33:08 2017
@@ -1,5 +1,7 @@
 // RUN: %clang_cc1 -triple x86_64-win32 -fdeclspec -verify %s
 // RUN: %clang_cc1 -triple x86_64-mingw32 -verify %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -verify -fdeclspec %s
+// RUN: %clang_cc1 -triple x86_64-win32-macho -verify -fdeclspec %s
 
 extern __declspec(selectany) const int x1 = 1; // no warning, const means we 
need extern in C++
 

Modified: cfe/trunk/test/SemaCXX/attr-selectany.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/attr-selectany.cpp?rev=313278=313277=313278=diff
==
--- cfe/trunk/test/SemaCXX/attr-selectany.cpp (original)
+++ cfe/trunk/test/SemaCXX/attr-selectany.cpp Thu Sep 14 10:33:08 2017
@@ -1,4 +1,7 @@
 // RUN: %clang_cc1 -triple x86_64-win32 -fms-compatibility -fms-extensions 
-fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux -fms-compatibility 
-fms-extensions -fsyntax-only -verify -std=c++11 %s
+// RUN: %clang_cc1 -triple x86_64-win32-macho -fms-compatibility 
-fms-extensions -fsyntax-only -verify -std=c++11 %s
+
 // MSVC produces similar diagnostics.
 
 __declspec(selectany) void foo() { } // expected-error{{'selectany' can only 
be applied to data items with external linkage}}


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

r304455 - Fixed broken test (strict-vtable-pointers)

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Thu Jun  1 14:08:05 2017
New Revision: 304455

URL: http://llvm.org/viewvc/llvm-project?rev=304455=rev
Log:
Fixed broken test (strict-vtable-pointers)

Modified:
cfe/trunk/test/CodeGenCXX/strict-vtable-pointers.cpp

Modified: cfe/trunk/test/CodeGenCXX/strict-vtable-pointers.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/strict-vtable-pointers.cpp?rev=304455=304454=304455=diff
==
--- cfe/trunk/test/CodeGenCXX/strict-vtable-pointers.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/strict-vtable-pointers.cpp Thu Jun  1 14:08:05 
2017
@@ -209,7 +209,7 @@ void UnionsBarriers(U *u) {
   // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
   // CHECK-NEW: call void @_Z2g2P1A(%struct.A*
   g2(>b);
-  // CHECK-NEW: call void @_Z9changeToAP1U(%union.U* %6)
+  // CHECK-NEW: call void @_Z9changeToAP1U(%union.U* 
   changeToA(u);
   // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
   // call void @_Z2g2P1A(%struct.A* %a)


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

r304448 - Emit invariant.group.barrier when using union field

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Thu Jun  1 13:39:34 2017
New Revision: 304448

URL: http://llvm.org/viewvc/llvm-project?rev=304448=rev
Log:
Emit invariant.group.barrier when using union field

Summary:
We need to emit barrier if the union field
is CXXRecordDecl because it might have vptrs. The testcode
was wrongly devirtualized. It also proves that having different
groups for different dynamic types is not sufficient.

Reviewers: rjmccall, rsmith, mehdi_amini

Subscribers: amharc, cfe-commits

Differential Revision: https://reviews.llvm.org/D31830

Modified:
cfe/trunk/lib/CodeGen/CGExpr.cpp
cfe/trunk/test/CodeGenCXX/strict-vtable-pointers.cpp

Modified: cfe/trunk/lib/CodeGen/CGExpr.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExpr.cpp?rev=304448=304447=304448=diff
==
--- cfe/trunk/lib/CodeGen/CGExpr.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExpr.cpp Thu Jun  1 13:39:34 2017
@@ -3530,6 +3530,25 @@ static Address emitAddrOfFieldStorage(Co
   return CGF.Builder.CreateStructGEP(base, idx, offset, field->getName());
 }
 
+static bool hasAnyVptr(const QualType Type, const ASTContext ) {
+  const auto *RD = Type.getTypePtr()->getAsCXXRecordDecl();
+  if (!RD)
+return false;
+
+  if (RD->isDynamicClass())
+return true;
+
+  for (const auto  : RD->bases())
+if (hasAnyVptr(Base.getType(), Context))
+  return true;
+
+  for (const FieldDecl *Field : RD->fields())
+if (hasAnyVptr(Field->getType(), Context))
+  return true;
+
+  return false;
+}
+
 LValue CodeGenFunction::EmitLValueForField(LValue base,
const FieldDecl *field) {
   LValueBaseInfo BaseInfo = base.getBaseInfo();
@@ -3572,6 +3591,14 @@ LValue CodeGenFunction::EmitLValueForFie
 assert(!type->isReferenceType() && "union has reference member");
 // TODO: handle path-aware TBAA for union.
 TBAAPath = false;
+
+const auto FieldType = field->getType();
+if (CGM.getCodeGenOpts().StrictVTablePointers &&
+hasAnyVptr(FieldType, getContext()))
+  // Because unions can easily skip invariant.barriers, we need to add
+  // a barrier every time CXXRecord field with vptr is referenced.
+  addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()),
+ addr.getAlignment());
   } else {
 // For structs, we GEP to the field that the record layout suggests.
 addr = emitAddrOfFieldStorage(*this, addr, field);

Modified: cfe/trunk/test/CodeGenCXX/strict-vtable-pointers.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/strict-vtable-pointers.cpp?rev=304448=304447=304448=diff
==
--- cfe/trunk/test/CodeGenCXX/strict-vtable-pointers.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/strict-vtable-pointers.cpp Thu Jun  1 13:39:34 
2017
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 
-fstrict-vtable-pointers -disable-llvm-passes -O2 -emit-llvm -o %t.ll
+// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 
-fstrict-vtable-pointers -std=c++11 -disable-llvm-passes -O2 -emit-llvm -o %t.ll
 // RUN: FileCheck --check-prefix=CHECK-CTORS %s < %t.ll
 // RUN: FileCheck --check-prefix=CHECK-NEW %s < %t.ll
 // RUN: FileCheck --check-prefix=CHECK-DTORS %s < %t.ll
@@ -180,6 +180,119 @@ struct DynamicFromStatic;
 // CHECK-CTORS-NOT: @llvm.invariant.group.barrier(
 // CHECK-CTORS-LABEL: {{^}}}
 
+struct A {
+  virtual void foo();
+};
+struct B : A {
+  virtual void foo();
+};
+
+union U {
+  A a;
+  B b;
+};
+
+void changeToB(U *u);
+void changeToA(U *u);
+
+void g2(A *a) {
+  a->foo();
+}
+// We have to guard access to union fields with invariant.group, because
+// it is very easy to skip the barrier with unions. In this example the inlined
+// g2 will produce loads with the same !invariant.group metadata, and
+// u->a and u->b would use the same pointer.
+// CHECK-NEW-LABEL: define void @_Z14UnionsBarriersP1U
+void UnionsBarriers(U *u) {
+  // CHECK-NEW: call void @_Z9changeToBP1U(
+  changeToB(u);
+  // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+  // CHECK-NEW: call void @_Z2g2P1A(%struct.A*
+  g2(>b);
+  // CHECK-NEW: call void @_Z9changeToAP1U(%union.U* %6)
+  changeToA(u);
+  // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+  // call void @_Z2g2P1A(%struct.A* %a)
+  g2(>a);
+  // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+}
+
+struct HoldingVirtuals {
+  A a;
+};
+
+struct Empty {};
+struct AnotherEmpty {
+  Empty e;
+};
+union NoVptrs {
+  int a;
+  AnotherEmpty empty;
+};
+void take(AnotherEmpty &);
+
+// CHECK-NEW-LABEL: noBarriers
+void noBarriers(NoVptrs ) {
+  // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+  // CHECK-NEW: 42
+  noVptrs.a += 42;
+  // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+  // CHECK-NEW: call void @_Z4takeR12AnotherEmpty(
+  

r304397 - Fixed warnings

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Thu Jun  1 04:24:36 2017
New Revision: 304397

URL: http://llvm.org/viewvc/llvm-project?rev=304397=rev
Log:
Fixed warnings

Modified:
cfe/trunk/include/clang/AST/VTableBuilder.h
cfe/trunk/lib/CodeGen/CodeGenModule.cpp

Modified: cfe/trunk/include/clang/AST/VTableBuilder.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/VTableBuilder.h?rev=304397=304396=304397=diff
==
--- cfe/trunk/include/clang/AST/VTableBuilder.h (original)
+++ cfe/trunk/include/clang/AST/VTableBuilder.h Thu Jun  1 04:24:36 2017
@@ -173,6 +173,7 @@ public:
 case CK_UnusedFunctionPointer:
   llvm_unreachable("Only function pointers kinds");
 }
+llvm_unreachable("Should already return");
   }
 
 private:

Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=304397=304396=304397=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Thu Jun  1 04:24:36 2017
@@ -1393,8 +1393,8 @@ void CodeGenModule::EmitVTablesOpportuni
   // is not allowed to create new references to things that need to be emitted
   // lazily. Note that it also uses fact that we eagerly emitting RTTI.
 
-  assert(OpportunisticVTables.empty() || shouldOpportunisticallyEmitVTables() 
&&
-   "Only emit opportunistic vtables with optimizations");
+  assert((OpportunisticVTables.empty() || shouldOpportunisticallyEmitVTables())
+ && "Only emit opportunistic vtables with optimizations");
 
   for (const CXXRecordDecl *RD : OpportunisticVTables) {
 assert(getVTables().isVTableExternal(RD) &&


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

r304394 - Emit available_externally vtables opportunistically

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Thu Jun  1 03:04:05 2017
New Revision: 304394

URL: http://llvm.org/viewvc/llvm-project?rev=304394=rev
Log:
Emit available_externally vtables opportunistically

Summary:
We can emit vtable definition having inline function
if they are all emitted.

Reviewers: rjmccall, rsmith

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D33437

Modified:
cfe/trunk/include/clang/AST/VTableBuilder.h
cfe/trunk/lib/CodeGen/CGVTables.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.cpp
cfe/trunk/lib/CodeGen/CodeGenModule.h
cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
cfe/trunk/test/CodeGenCXX/vtable-available-externally.cpp
cfe/trunk/test/CodeGenCXX/vtable-linkage.cpp

Modified: cfe/trunk/include/clang/AST/VTableBuilder.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/VTableBuilder.h?rev=304394=304393=304394=diff
==
--- cfe/trunk/include/clang/AST/VTableBuilder.h (original)
+++ cfe/trunk/include/clang/AST/VTableBuilder.h Thu Jun  1 03:04:05 2017
@@ -154,6 +154,27 @@ public:
 
   bool isRTTIKind() const { return isRTTIKind(getKind()); }
 
+  GlobalDecl getGlobalDecl() const {
+assert(isUsedFunctionPointerKind() &&
+   "GlobalDecl can be created only from virtual function");
+
+auto *DtorDecl = dyn_cast(getFunctionDecl());
+switch (getKind()) {
+case CK_FunctionPointer:
+  return GlobalDecl(getFunctionDecl());
+case CK_CompleteDtorPointer:
+  return GlobalDecl(DtorDecl, CXXDtorType::Dtor_Complete);
+case CK_DeletingDtorPointer:
+  return GlobalDecl(DtorDecl, CXXDtorType::Dtor_Deleting);
+case CK_VCallOffset:
+case CK_VBaseOffset:
+case CK_OffsetToTop:
+case CK_RTTI:
+case CK_UnusedFunctionPointer:
+  llvm_unreachable("Only function pointers kinds");
+}
+  }
+
 private:
   static bool isFunctionPointerKind(Kind ComponentKind) {
 return isUsedFunctionPointerKind(ComponentKind) ||

Modified: cfe/trunk/lib/CodeGen/CGVTables.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGVTables.cpp?rev=304394=304393=304394=diff
==
--- cfe/trunk/lib/CodeGen/CGVTables.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGVTables.cpp Thu Jun  1 03:04:05 2017
@@ -901,6 +901,8 @@ void CodeGenModule::EmitDeferredVTables(
   for (const CXXRecordDecl *RD : DeferredVTables)
 if (shouldEmitVTableAtEndOfTranslationUnit(*this, RD))
   VTables.GenerateClassData(RD);
+else if (shouldOpportunisticallyEmitVTables())
+  OpportunisticVTables.push_back(RD);
 
   assert(savedSize == DeferredVTables.size() &&
  "deferred extra vtables during vtable emission?");

Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=304394=304393=304394=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Thu Jun  1 03:04:05 2017
@@ -382,6 +382,7 @@ void InstrProfStats::reportDiagnostics(D
 
 void CodeGenModule::Release() {
   EmitDeferred();
+  EmitVTablesOpportunistically();
   applyGlobalValReplacements();
   applyReplacements();
   checkAliases();
@@ -1386,6 +1387,24 @@ void CodeGenModule::EmitDeferred() {
   }
 }
 
+void CodeGenModule::EmitVTablesOpportunistically() {
+  // Try to emit external vtables as available_externally if they have emitted
+  // all inlined virtual functions.  It runs after EmitDeferred() and therefore
+  // is not allowed to create new references to things that need to be emitted
+  // lazily. Note that it also uses fact that we eagerly emitting RTTI.
+
+  assert(OpportunisticVTables.empty() || shouldOpportunisticallyEmitVTables() 
&&
+   "Only emit opportunistic vtables with optimizations");
+
+  for (const CXXRecordDecl *RD : OpportunisticVTables) {
+assert(getVTables().isVTableExternal(RD) &&
+   "This queue should only contain external vtables");
+if (getCXXABI().canSpeculativelyEmitVTable(RD))
+  VTables.GenerateClassData(RD);
+  }
+  OpportunisticVTables.clear();
+}
+
 void CodeGenModule::EmitGlobalAnnotations() {
   if (Annotations.empty())
 return;
@@ -1906,6 +1925,10 @@ bool CodeGenModule::shouldEmitFunction(G
   return !isTriviallyRecursive(F);
 }
 
+bool CodeGenModule::shouldOpportunisticallyEmitVTables() {
+  return CodeGenOpts.OptimizationLevel > 0;
+}
+
 void CodeGenModule::EmitGlobalDefinition(GlobalDecl GD, llvm::GlobalValue *GV) 
{
   const auto *D = cast(GD.getDecl());
 

Modified: cfe/trunk/lib/CodeGen/CodeGenModule.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.h?rev=304394=304393=304394=diff
==
--- 

r303488 - [Devirtualization] insert placement new barrier with -O0

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Sat May 20 03:56:18 2017
New Revision: 303488

URL: http://llvm.org/viewvc/llvm-project?rev=303488=rev
Log:
[Devirtualization] insert placement new barrier with -O0

Summary:
To not break LTO with different optimizations levels, we should insert
the barrier regardles of optimization level.

Reviewers: rjmccall, rsmith, mehdi_amini

Reviewed By: mehdi_amini

Subscribers: mehdi_amini, cfe-commits

Differential Revision: https://reviews.llvm.org/D32401

Modified:
cfe/trunk/lib/CodeGen/CGExprCXX.cpp

Modified: cfe/trunk/lib/CodeGen/CGExprCXX.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprCXX.cpp?rev=303488=303487=303488=diff
==
--- cfe/trunk/lib/CodeGen/CGExprCXX.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExprCXX.cpp Sat May 20 03:56:18 2017
@@ -1658,8 +1658,9 @@ llvm::Value *CodeGenFunction::EmitCXXNew
 
   // Passing pointer through invariant.group.barrier to avoid propagation of
   // vptrs information which may be included in previous type.
+  // To not break LTO with different optimizations levels, we do it regardless
+  // of optimization level.
   if (CGM.getCodeGenOpts().StrictVTablePointers &&
-  CGM.getCodeGenOpts().OptimizationLevel > 0 &&
   allocator->isReservedGlobalPlacementOperator())
 result = Address(Builder.CreateInvariantGroupBarrier(result.getPointer()),
  result.getAlignment());


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

r301178 - [Devirtualization] Emit invariant.group loads with empty group md

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Mon Apr 24 07:58:43 2017
New Revision: 301178

URL: http://llvm.org/viewvc/llvm-project?rev=301178=rev
Log:
[Devirtualization] Emit invariant.group loads with empty group md

Summary:
As discussed here
http://lists.llvm.org/pipermail/llvm-dev/2017-January/109332.html
having different groups doesn't solve the problem entirly.

Reviewers: rjmccall, rsmith

Subscribers: amharc, cfe-commits

Differential Revision: https://reviews.llvm.org/D32110

Modified:
cfe/trunk/lib/CodeGen/CodeGenModule.cpp
cfe/trunk/test/CodeGenCXX/invariant.group-for-vptrs.cpp

Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=301178=301177=301178=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Mon Apr 24 07:58:43 2017
@@ -565,12 +565,8 @@ void CodeGenModule::DecorateInstructionW
 
 void CodeGenModule::DecorateInstructionWithInvariantGroup(
 llvm::Instruction *I, const CXXRecordDecl *RD) {
-  llvm::Metadata *MD = 
CreateMetadataIdentifierForType(QualType(RD->getTypeForDecl(), 0));
-  auto *MetaDataNode = dyn_cast(MD);
-  // Check if we have to wrap MDString in MDNode.
-  if (!MetaDataNode)
-MetaDataNode = llvm::MDNode::get(getLLVMContext(), MD);
-  I->setMetadata(llvm::LLVMContext::MD_invariant_group, MetaDataNode);
+  I->setMetadata(llvm::LLVMContext::MD_invariant_group,
+ llvm::MDNode::get(getLLVMContext(), {}));
 }
 
 void CodeGenModule::Error(SourceLocation loc, StringRef message) {

Modified: cfe/trunk/test/CodeGenCXX/invariant.group-for-vptrs.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/invariant.group-for-vptrs.cpp?rev=301178=301177=301178=diff
==
--- cfe/trunk/test/CodeGenCXX/invariant.group-for-vptrs.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/invariant.group-for-vptrs.cpp Mon Apr 24 07:58:43 
2017
@@ -12,15 +12,15 @@ struct D : A {
 void testExternallyVisible() {
   A *a = new A;
 
-  // CHECK: load {{.*}} !invariant.group ![[A_MD:[0-9]+]]
+  // CHECK: load {{.*}} !invariant.group ![[MD:[0-9]+]]
   a->foo();
 
   D *d = new D;
   // CHECK: call void @_ZN1DC1Ev(
-  // CHECK: load {{.*}} !invariant.group ![[D_MD:[0-9]+]]
+  // CHECK: load {{.*}} !invariant.group ![[MD]]
   d->foo();
   A *a2 = d;
-  // CHECK: load {{.*}} !invariant.group ![[A_MD]]
+  // CHECK: load {{.*}} !invariant.group ![[MD]]
   a2->foo();
 }
 // CHECK-LABEL: {{^}}}
@@ -40,35 +40,32 @@ struct C : B {
 // CHECK-LABEL: define void @_Z21testInternallyVisibleb(
 void testInternallyVisible(bool p) {
   B *b = new B;
-  // CHECK: = load {{.*}}, !invariant.group ![[B_MD:[0-9]+]]
+  // CHECK: = load {{.*}}, !invariant.group ![[MD]]
   b->bar();
 
   // CHECK: call void @_ZN12_GLOBAL__N_11CC1Ev(
   C *c = new C;
-  // CHECK: = load {{.*}}, !invariant.group ![[C_MD:[0-9]+]]
+  // CHECK: = load {{.*}}, !invariant.group ![[MD]]
   c->bar();
 }
 
 // Checking A::A()
 // CHECK-LABEL: define linkonce_odr void @_ZN1AC2Ev(
-// CHECK: store {{.*}}, !invariant.group ![[A_MD]]
+// CHECK: store {{.*}}, !invariant.group ![[MD]]
 // CHECK-LABEL: {{^}}}
 
 // Checking D::D()
 // CHECK-LABEL: define linkonce_odr void @_ZN1DC2Ev(
 // CHECK:  = call i8* @llvm.invariant.group.barrier(i8*
 // CHECK:  call void @_ZN1AC2Ev(%struct.A*
-// CHECK: store {{.*}} !invariant.group ![[D_MD]]
+// CHECK: store {{.*}} !invariant.group ![[MD]]
 
 // Checking B::B()
 // CHECK-LABEL: define internal void @_ZN12_GLOBAL__N_11BC2Ev(
-// CHECK:  store {{.*}}, !invariant.group ![[B_MD]]
+// CHECK:  store {{.*}}, !invariant.group ![[MD]]
 
 // Checking C::C()
 // CHECK-LABEL: define internal void @_ZN12_GLOBAL__N_11CC2Ev(
-// CHECK:  store {{.*}}, !invariant.group ![[C_MD]]
+// CHECK:  store {{.*}}, !invariant.group ![[MD]]
 
-// CHECK: ![[A_MD]] = !{!"_ZTS1A"}
-// CHECK: ![[D_MD]] = !{!"_ZTS1D"}
-// CHECK: ![[B_MD]] = distinct !{}
-// CHECK: ![[C_MD]] = distinct !{}
+// CHECK: ![[MD]] = !{}


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

[clang-tools-extra] r296888 - [clang-tidy] Yet another docs fixes

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Fri Mar  3 11:16:11 2017
New Revision: 296888

URL: http://llvm.org/viewvc/llvm-project?rev=296888=rev
Log:
[clang-tidy] Yet another docs fixes

Modified:
clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst

Modified: 
clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst?rev=296888=296887=296888=diff
==
--- clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst 
(original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst 
Fri Mar  3 11:16:11 2017
@@ -69,7 +69,7 @@ exception safe. In this case the calls o
 
 This is because replacing it with ``emplace_back`` could cause a leak of this
 pointer if ``emplace_back`` would throw exception before emplacement (e.g. not
-enough memory to add new element).
+enough memory to add a new element).
 
 For more info read item 42 - "Consider emplacement instead of insertion." of
 Scott Meyers "Effective Modern C++".
@@ -79,14 +79,15 @@ The default smart pointers that are cons
 other classes use the :option:`SmartPointers` option.
 
 
-Check also fires if any argument of constructor call would be:
+Check also doesn't fire if any argument of the constructor call would be:
 
-  - bitfield (bitfields can't bind to rvalue/universal reference)
+  - a bit-field (bit-fields can't bind to rvalue/universal reference)
 
-  - ``new`` expression (to avoid leak) or if the argument would be converted 
via
-derived-to-base cast.
+  - a ``new`` expression (to avoid leak)
 
-This check requires C++11 of higher to run.
+  - if the argument would be converted via derived-to-base cast.
+
+This check requires C++11 or higher to run.
 
 Options
 ---


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

[clang-tools-extra] r296867 - [clang-tidy] Fix modernize-use-emplace docs

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Fri Mar  3 06:42:22 2017
New Revision: 296867

URL: http://llvm.org/viewvc/llvm-project?rev=296867=rev
Log:
[clang-tidy] Fix modernize-use-emplace docs

Modified:
clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst

Modified: 
clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst?rev=296867=296866=296867=diff
==
--- clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst 
(original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst 
Fri Mar  3 06:42:22 2017
@@ -36,7 +36,7 @@ After:
 
 std::vector> w;
 w.emplace_back(21, 37);
-// This will be fixed to w.push_back(21, 37); in next version
+// This will be fixed to w.emplace_back(21L, 37L); in next version
 w.emplace_back(std::make_pair(21L, 37L);
 
 The other situation is when we pass arguments that will be converted to a type


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

r292112 - Add -fstrict-vtable-pointers to UsersManual

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Mon Jan 16 07:20:08 2017
New Revision: 292112

URL: http://llvm.org/viewvc/llvm-project?rev=292112=rev
Log:
Add -fstrict-vtable-pointers to UsersManual

Summary: Add missing flag to UsersManual
It would be good to merge it to 4.0 branch.

Reviewers: hans

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D28727

Modified:
cfe/trunk/docs/UsersManual.rst

Modified: cfe/trunk/docs/UsersManual.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/UsersManual.rst?rev=292112=292111=292112=diff
==
--- cfe/trunk/docs/UsersManual.rst (original)
+++ cfe/trunk/docs/UsersManual.rst Mon Jan 16 07:20:08 2017
@@ -1097,6 +1097,13 @@ are listed below.
the behavior of sanitizers in the ``cfi`` group to allow checking
of cross-DSO virtual and indirect calls.
 
+
+.. option:: -fstrict-vtable-pointers
+   Enable optimizations based on the strict rules for overwriting polymorphic
+   C++ objects, i.e. the vptr is invariant during an object's lifetime.
+   This enables better devirtualization. Turned off by default, because it is
+   still experimental.
+
 .. option:: -ffast-math
 
Enable fast-math mode. This defines the ``__FAST_MATH__`` preprocessor


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

r290677 - [ItaniumABI] NFC changes

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Wed Dec 28 12:26:08 2016
New Revision: 290677

URL: http://llvm.org/viewvc/llvm-project?rev=290677=rev
Log:
[ItaniumABI] NFC changes

Modified:
cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp

Modified: cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp?rev=290677=290676=290677=diff
==
--- cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp (original)
+++ cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp Wed Dec 28 12:26:08 2016
@@ -366,11 +366,12 @@ public:
   void emitCXXStructor(const CXXMethodDecl *MD, StructorType Type) override;
 
  private:
-   bool hasAnyUsedVirtualInlineFunction(const CXXRecordDecl *RD) const {
+   bool hasAnyVirtualInlineFunction(const CXXRecordDecl *RD) const {
 const auto  =
 CGM.getItaniumVTableContext().getVTableLayout(RD);
 
 for (const auto  : VtableLayout.vtable_components()) {
+  // Skip empty slot.
   if (!VtableComponent.isUsedFunctionPointerKind())
 continue;
 
@@ -1687,7 +1688,7 @@ bool ItaniumCXXABI::canSpeculativelyEmit
   // then we are safe to emit available_externally copy of vtable.
   // FIXME we can still emit a copy of the vtable if we
   // can emit definition of the inline functions.
-  return !hasAnyUsedVirtualInlineFunction(RD) && !isVTableHidden(RD);
+  return !hasAnyVirtualInlineFunction(RD) && !isVTableHidden(RD);
 }
 static llvm::Value *performTypeAdjustment(CodeGenFunction ,
   Address InitialPtr,


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

r290676 - Revert "Mention devirtualization in release notes"

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Wed Dec 28 12:25:30 2016
New Revision: 290676

URL: http://llvm.org/viewvc/llvm-project?rev=290676=rev
Log:
Revert "Mention devirtualization in release notes"

Accidental commit. LLVM changes have not been pushed yet
This reverts commit 592453413690a2d16784667d1644758b9af700c1.

Modified:
cfe/trunk/docs/ReleaseNotes.rst

Modified: cfe/trunk/docs/ReleaseNotes.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=290676=290675=290676=diff
==
--- cfe/trunk/docs/ReleaseNotes.rst (original)
+++ cfe/trunk/docs/ReleaseNotes.rst Wed Dec 28 12:25:30 2016
@@ -47,24 +47,6 @@ sections with improvements to Clang's su
 Major New Features
 --
 
-- Enhanced devirtualization with `-fstrict-vtable-pointers`. Clang 
devirtualizes
-across different basic blocks, like loops:
-
-.. code-block:: c++
-   struct A {
-   virtual void foo() {}
-   };
-   void indirect(A , int n) {
-   for (int i = 0 ; i < n; i++)
-   a.foo();
-
-   }
-   void test(int n) {
-   A a;
-   indirect(a);
-   }
-
-
 -  ...
 
 Improvements to Clang's diagnostics


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

r290675 - Mention devirtualization in release notes

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Wed Dec 28 12:23:23 2016
New Revision: 290675

URL: http://llvm.org/viewvc/llvm-project?rev=290675=rev
Log:
Mention devirtualization in release notes

Modified:
cfe/trunk/docs/ReleaseNotes.rst

Modified: cfe/trunk/docs/ReleaseNotes.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=290675=290674=290675=diff
==
--- cfe/trunk/docs/ReleaseNotes.rst (original)
+++ cfe/trunk/docs/ReleaseNotes.rst Wed Dec 28 12:23:23 2016
@@ -47,6 +47,24 @@ sections with improvements to Clang's su
 Major New Features
 --
 
+- Enhanced devirtualization with `-fstrict-vtable-pointers`. Clang 
devirtualizes
+across different basic blocks, like loops:
+
+.. code-block:: c++
+   struct A {
+   virtual void foo() {}
+   };
+   void indirect(A , int n) {
+   for (int i = 0 ; i < n; i++)
+   a.foo();
+
+   }
+   void test(int n) {
+   A a;
+   indirect(a);
+   }
+
+
 -  ...
 
 Improvements to Clang's diagnostics


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

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Piotr Padlewski via cfe-commits]

Firstly, please respond in phabricator if it is possible. When you send
email it doesn't appear in phabricator, it's probably a bug.

2016-12-19 8:00 GMT+01:00 Mads Ravn :

> Hi Piotr,
>
> Thank you for your detailed comments :)
>
> I would love some help with the other fixit. I have some notes on it at
> home. But my main catch is that is an implicit cast to boolean from
> str.compare(str) with maybe an ! in front of it. And I need to fix that to
> str.compare(str) == 0 or str.compare(str) != 0.
>
> Why fix it to something, that you will want to fix it again to str == str
and str != str?
I guess you just have to match parent of this expr is negation or anything,
bind negation to some name and then check if you matched to the negation or
not.


> Where should I put the warning in a static const global variable? Is it
> still in StringCompare.cpp or do we have a  joint files for these?
>
> Yep, StringCompare.cpp, just like in other checks.

> Best regards,
> Mads Ravn
>
> On Sun, Dec 18, 2016 at 11:26 PM Piotr Padlewski via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> Prazek added a comment.
>>
>> Do you need some help with implementing the other fixit, or you just need
>> some extra time? It seems to be almost the same as your second fixit
>>
>>
>>
>> 
>> Comment at: clang-tidy/misc/StringCompareCheck.cpp:69-70
>> +diag(Matched->getLocStart(),
>> + "do not use 'compare' to test equality of strings; "
>> + "use the string equality operator instead");
>> +
>> 
>> Take this warning to some static const global variable
>>
>>
>> 
>> Comment at: clang-tidy/misc/StringCompareCheck.cpp:71
>> + "use the string equality operator instead");
>> +
>> +  if (const auto *Matched = Result.Nodes.getNodeAs("match2")) {
>> 
>> match1 and match2 are in different matchers (passed to register matchers)?
>>
>> If so put return here after diag to finish control flow for this case.
>>
>>
>> 
>> Comment at: clang-tidy/misc/StringCompareCheck.cpp:81
>> +  auto Diag = diag(Matched->getLocStart(),
>> +   "do not use 'compare' to test equality of
>> strings; "
>> +   "use the string equality operator instead");
>> 
>> and use it here
>>
>>
>> 
>> Comment at: clang-tidy/misc/StringCompareCheck.h:10-11
>> +
>> +#ifndef STRING_COMPARE_CHECK_H
>> +#define STRING_COMPARE_CHECK_H
>> +
>> 
>> This should be much longer string. Do you know about ./add_new_check?
>>
>> Please make one similar to other checks
>>
>>
>> 
>> Comment at: clang-tidy/misc/StringCompareCheck.h:36
>> +
>> +#endif // STRING_COMPARE_CHECK_H
>> 
>> DITTO
>>
>>
>> 
>> Comment at: test/clang-tidy/misc-string-compare.cpp:35-40
>> +  if (str1.compare(str2)) {
>> +  }
>> +  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to
>> test equality of strings; use the string equality operator instead
>> [misc-string-compare]
>> +  if (!str1.compare(str2)) {
>> +  }
>> +  // CHECK-MESSAGES: [[@LINE-2]]:8: warning: do not use 'compare' to
>> test equality of strings; use the string equality operator instead
>> [misc-string-compare]
>> 
>> Why this one doesn't have fixit hint?
>>
>>
>> 
>> Comment at: test/clang-tidy/misc-string-compare.cpp:70
>> +  }
>> +  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to
>> test equality of strings;
>> +  if (str3->compare(str2) == 0) {
>> 
>> no fixit?
>>
>>
>> https://reviews.llvm.org/D27210
>>
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r289930 - [clang-tidy] fix missing anchor for MPI Module

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Fri Dec 16 03:14:47 2016
New Revision: 289930

URL: http://llvm.org/viewvc/llvm-project?rev=289930=rev
Log:
[clang-tidy] fix missing anchor for MPI Module

Summary: MPIModule was not linked to plugins

Reviewers: alexfh, Alexander_Droste, hokein

Subscribers: JDevlieghere, cfe-commits

Differential Revision: https://reviews.llvm.org/D27813

Modified:
clang-tools-extra/trunk/clang-tidy/plugin/ClangTidyPlugin.cpp

Modified: clang-tools-extra/trunk/clang-tidy/plugin/ClangTidyPlugin.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/plugin/ClangTidyPlugin.cpp?rev=289930=289929=289930=diff
==
--- clang-tools-extra/trunk/clang-tidy/plugin/ClangTidyPlugin.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/plugin/ClangTidyPlugin.cpp Fri Dec 16 
03:14:47 2016
@@ -108,6 +108,11 @@ extern volatile int ModernizeModuleAncho
 static int LLVM_ATTRIBUTE_UNUSED ModernizeModuleAnchorDestination =
 ModernizeModuleAnchorSource;
 
+// This anchor is used to force the linker to link the MPIModule.
+extern volatile int MPIModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED MPIModuleAnchorDestination =
+  MPIModuleAnchorSource;
+
 // This anchor is used to force the linker to link the PerformanceModule.
 extern volatile int PerformanceModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED PerformanceModuleAnchorDestination =


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

Re: [PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Piotr Padlewski via cfe-commits]

I think that the feature I mentioned is right thing to put in this check,
however you don't have to implement it right now, just leave FIXIT comment

2016-12-15 20:55 GMT+01:00 Mads Ravn :

> Hi Piotr,
>
> That is a good point. Because it is not always -1 or 1 that determines
> lexicographical higher or lower.
>
> However, I don't think that is in the scope of this check. This check
> checks for string comparison (equality or inequality). Adding a match for
> if the user is using the compare function semantically wrong might make the
> check too ambiguous. Since str.compare(str) == 0 will check for equality
> and str.compare(str) != 0 will check for inequality. But str.compare(str)
> == 1 will check whether one string is lexicographically smaller than the
> other (and == -1 also). What do you think?
>
> Best regards,
> Mads Ravn
>
> On Thu, Dec 15, 2016 at 8:17 PM Piotr Padlewski via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> Prazek added a comment.
>>
>> Good job.
>> I think it is resonable to warn in cases like:
>>
>>   if (str.compare(str2) == 1)
>>
>> or even
>>
>>   if(str.compare(str2) == -1)
>>
>> Sometimes people check for 1 or -1 instead of > 0 and < 0. If I remember
>> corectly PVS studio found some bugs like this.
>>
>>
>>
>> 
>> Comment at: clang-tidy/misc/StringCompareCheck.cpp:27
>> +   hasName("::std::basic_string"),
>> +  hasArgument(0, declRefExpr()), callee(memberExpr()));
>> +
>> 
>> malcolm.parsons wrote:
>> > I don't think you need to check what the first argument is.
>> +1, just check if you are calling function with 1 argument.
>> you can still use hasArgument(0, expr().bind("str2")) to bind argument
>>
>>
>> 
>> Comment at: clang-tidy/misc/StringCompareCheck.cpp:25
>> +return;
>> +  const auto strCompare = cxxMemberCallExpr(
>> +  callee(cxxMethodDecl(hasName("compare"),
>> 
>> Start with upper case
>>
>>
>> https://reviews.llvm.org/D27210
>>
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clang-tools-extra] r289656 - modernize-use-auto NFC fixes

[Piotr Padlewski via cfe-commits]

Sure, I am planning to do so. I just wanted to firstly make all auto's and
the change was small enough that I though you would not mind pushing it
without review.
BTW the same thing in clang needs review: https://reviews.llvm.org/D27767

2016-12-15 17:08 GMT+01:00 Alexander Kornienko <ale...@google.com>:

> For most loop changes here applying modernize-loop-convert would be better
> ;)
>
> On Wed, Dec 14, 2016 at 4:29 PM, Piotr Padlewski via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: prazek
>> Date: Wed Dec 14 09:29:23 2016
>> New Revision: 289656
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=289656=rev
>> Log:
>> modernize-use-auto NFC fixes
>>
>> Modified:
>> clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
>> clang-tools-extra/trunk/clang-apply-replacements/lib/Tooling
>> /ApplyReplacements.cpp
>> clang-tools-extra/trunk/clang-query/Query.cpp
>> clang-tools-extra/trunk/clang-query/QueryParser.cpp
>> clang-tools-extra/trunk/clang-query/tool/ClangQuery.cpp
>> clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProType
>> MemberInitCheck.cpp
>> clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/Special
>> MemberFunctionsCheck.cpp
>> clang-tools-extra/trunk/clang-tidy/google/ExplicitConstructo
>> rCheck.cpp
>> clang-tools-extra/trunk/clang-tidy/google/NonConstReferences.cpp
>> clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.cpp
>> clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.cpp
>> clang-tools-extra/trunk/clang-tidy/misc/MoveForwardingRefere
>> nceCheck.cpp
>> clang-tools-extra/trunk/clang-tidy/misc/MultipleStatementMac
>> roCheck.cpp
>> clang-tools-extra/trunk/clang-tidy/misc/ThrowByValueCatchByR
>> eferenceCheck.cpp
>> clang-tools-extra/trunk/clang-tidy/modernize/UseNullptrCheck.cpp
>> clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp
>> clang-tools-extra/trunk/clang-tidy/mpi/TypeMismatchCheck.cpp
>> clang-tools-extra/trunk/clang-tidy/readability/ImplicitBoolC
>> astCheck.cpp
>> clang-tools-extra/trunk/clang-tidy/readability/NamespaceComm
>> entCheck.cpp
>> clang-tools-extra/trunk/clang-tidy/readability/RedundantDecl
>> arationCheck.cpp
>> clang-tools-extra/trunk/clang-tidy/readability/RedundantSmar
>> tptrGetCheck.cpp
>> clang-tools-extra/trunk/include-fixer/find-all-symbols/
>> FindAllSymbols.cpp
>> clang-tools-extra/trunk/modularize/CoverageChecker.cpp
>> clang-tools-extra/trunk/modularize/Modularize.cpp
>> clang-tools-extra/trunk/modularize/ModularizeUtilities.cpp
>> clang-tools-extra/trunk/modularize/ModuleAssistant.cpp
>> clang-tools-extra/trunk/modularize/PreprocessorTracker.cpp
>> clang-tools-extra/trunk/pp-trace/PPTrace.cpp
>> clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyDiagno
>> sticConsumerTest.cpp
>> clang-tools-extra/trunk/unittests/clang-tidy/NamespaceAliaserTest.cpp
>> clang-tools-extra/trunk/unittests/clang-tidy/UsingInserterTest.cpp
>>
>> Modified: clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
>> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/
>> change-namespace/ChangeNamespace.cpp?rev=289656=289655&
>> r2=289656=diff
>> 
>> ==
>> --- clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
>> (original)
>> +++ clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp Wed Dec
>> 14 09:29:23 2016
>> @@ -709,7 +709,7 @@ void ChangeNamespaceTool::fixTypeLoc(
>>return;
>>}
>>
>> -  const Decl *DeclCtx = Result.Nodes.getNodeAs("dc");
>> +  const auto *DeclCtx = Result.Nodes.getNodeAs("dc");
>>assert(DeclCtx && "Empty decl context.");
>>replaceQualifiedSymbolInDeclContext(Result,
>> DeclCtx->getDeclContext(), Start,
>>End, FromDecl);
>>
>> Modified: clang-tools-extra/trunk/clang-apply-replacements/lib/Tooling
>> /ApplyReplacements.cpp
>> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/
>> clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp?
>> rev=289656=289655=289656=diff
>> 
>> ==
>> --- 
>> clang-tools-extra/trunk/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
>> (original)
>> +++ 
>> clang-tools-e

[clang-tools-extra] r289658 - Deleted unused typedef

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Wed Dec 14 09:42:23 2016
New Revision: 289658

URL: http://llvm.org/viewvc/llvm-project?rev=289658=rev
Log:
Deleted unused typedef

Modified:
clang-tools-extra/trunk/modularize/ModularizeUtilities.cpp

Modified: clang-tools-extra/trunk/modularize/ModularizeUtilities.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/modularize/ModularizeUtilities.cpp?rev=289658=289657=289658=diff
==
--- clang-tools-extra/trunk/modularize/ModularizeUtilities.cpp (original)
+++ clang-tools-extra/trunk/modularize/ModularizeUtilities.cpp Wed Dec 14 
09:42:23 2016
@@ -75,7 +75,6 @@ ModularizeUtilities *ModularizeUtilities
 
 // Load all header lists and dependencies.
 std::error_code ModularizeUtilities::loadAllHeaderListsAndDependencies() {
-  typedef std::vector::iterator Iter;
   // For each input file.
   for (auto I = InputFilePaths.begin(), E = InputFilePaths.end(); I != E; ++I) 
{
 llvm::StringRef InputPath = *I;


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

[clang-tools-extra] r289656 - modernize-use-auto NFC fixes

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Wed Dec 14 09:29:23 2016
New Revision: 289656

URL: http://llvm.org/viewvc/llvm-project?rev=289656=rev
Log:
modernize-use-auto NFC fixes

Modified:
clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp

clang-tools-extra/trunk/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
clang-tools-extra/trunk/clang-query/Query.cpp
clang-tools-extra/trunk/clang-query/QueryParser.cpp
clang-tools-extra/trunk/clang-query/tool/ClangQuery.cpp

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
clang-tools-extra/trunk/clang-tidy/google/ExplicitConstructorCheck.cpp
clang-tools-extra/trunk/clang-tidy/google/NonConstReferences.cpp
clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.cpp
clang-tools-extra/trunk/clang-tidy/misc/ArgumentCommentCheck.cpp
clang-tools-extra/trunk/clang-tidy/misc/MoveForwardingReferenceCheck.cpp
clang-tools-extra/trunk/clang-tidy/misc/MultipleStatementMacroCheck.cpp

clang-tools-extra/trunk/clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp
clang-tools-extra/trunk/clang-tidy/modernize/UseNullptrCheck.cpp
clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp
clang-tools-extra/trunk/clang-tidy/mpi/TypeMismatchCheck.cpp
clang-tools-extra/trunk/clang-tidy/readability/ImplicitBoolCastCheck.cpp
clang-tools-extra/trunk/clang-tidy/readability/NamespaceCommentCheck.cpp
clang-tools-extra/trunk/clang-tidy/readability/RedundantDeclarationCheck.cpp
clang-tools-extra/trunk/clang-tidy/readability/RedundantSmartptrGetCheck.cpp
clang-tools-extra/trunk/include-fixer/find-all-symbols/FindAllSymbols.cpp
clang-tools-extra/trunk/modularize/CoverageChecker.cpp
clang-tools-extra/trunk/modularize/Modularize.cpp
clang-tools-extra/trunk/modularize/ModularizeUtilities.cpp
clang-tools-extra/trunk/modularize/ModuleAssistant.cpp
clang-tools-extra/trunk/modularize/PreprocessorTracker.cpp
clang-tools-extra/trunk/pp-trace/PPTrace.cpp

clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp
clang-tools-extra/trunk/unittests/clang-tidy/NamespaceAliaserTest.cpp
clang-tools-extra/trunk/unittests/clang-tidy/UsingInserterTest.cpp

Modified: clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp?rev=289656=289655=289656=diff
==
--- clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp (original)
+++ clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp Wed Dec 14 
09:29:23 2016
@@ -709,7 +709,7 @@ void ChangeNamespaceTool::fixTypeLoc(
   return;
   }
 
-  const Decl *DeclCtx = Result.Nodes.getNodeAs("dc");
+  const auto *DeclCtx = Result.Nodes.getNodeAs("dc");
   assert(DeclCtx && "Empty decl context.");
   replaceQualifiedSymbolInDeclContext(Result, DeclCtx->getDeclContext(), Start,
   End, FromDecl);

Modified: 
clang-tools-extra/trunk/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp?rev=289656=289655=289656=diff
==
--- 
clang-tools-extra/trunk/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
 (original)
+++ 
clang-tools-extra/trunk/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
 Wed Dec 14 09:29:23 2016
@@ -124,9 +124,7 @@ static void reportConflict(
 bool applyAllReplacements(const std::vector ,
   Rewriter ) {
   bool Result = true;
-  for (std::vector::const_iterator I = Replaces.begin(),
- E = Replaces.end();
-   I != E; ++I) {
+  for (auto I = Replaces.begin(), E = Replaces.end(); I != E; ++I) {
 if (I->isApplicable()) {
   Result = I->apply(Rewrite) && Result;
 } else {
@@ -293,8 +291,7 @@ RangeVector calculateChangedRanges(
 
 bool writeFiles(const clang::Rewriter ) {
 
-  for (Rewriter::const_buffer_iterator BufferI = Rewrites.buffer_begin(),
-   BufferE = Rewrites.buffer_end();
+  for (auto BufferI = Rewrites.buffer_begin(), BufferE = Rewrites.buffer_end();
BufferI != BufferE; ++BufferI) {
 StringRef FileName =
 Rewrites.getSourceMgr().getFileEntryForID(BufferI->first)->getName();

Modified: clang-tools-extra/trunk/clang-query/Query.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-query/Query.cpp?rev=289656=289655=289656=diff
==
--- clang-tools-extra/trunk/clang-query/Query.cpp (original)
+++ 

Re: [clang-tools-extra] r286222 - [clang-tidy] clang-analyzer-alpha* checks are not registered, so there's no need to disable them

[Piotr Padlewski via cfe-commits]

Hmm, right now there is no way to use alpha checks in clang tidy?
Piotr

On Nov 10, 2016 10:57, "Malcolm Parsons via cfe-commits" <
cfe-commits@lists.llvm.org> wrote:

> On 9 November 2016 at 18:50, Devin Coughlin  wrote:
>
> > We agree that this is a valuable checker and are committed to getting it
> out of alpha. This check is in alpha because:
> >
> > a) The diagnostic experience is not very good. It reports a call path
> directly in the diagnostic message (for example “Call path: foo <— bar” for
> a call to foo() in bar()) rather than as a path diagnostic.
>
> Agreed.
>
> > b) The lack of path-sensitive reasoning may result in false positives
> when a called function uses a member variable flag to track whether
> initialization is complete and does not call the virtual member function
> during initialization.
>
> Right, we're not doing this.
>
> > c) The check issues a warning for both calls to pure virtual functions
> (which is always an error) and non-pure virtual functions (which is more of
> a code smell and may be a false positive).
>
> I'm using static analysis to find code smells.
>
> > I’ll commit to doing Step 1) in the immediate future and Step 2)
> eventually. Once the checker is on by default we’ll need to assess whether
> the false positive rate from c) is too high — if so, we’ll need to turn the
> non-pure-virtual case off by default.
>
> LGTM.
>
> Thanks,
> --
> Malcolm Parsons
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D21298: [Clang-tidy] delete null check

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.



Comment at: test/clang-tidy/misc-delete-null-pointer.cpp:11
+  }
+  // CHECK-FIXES: delete p;
+  int *p3 = new int[3];

Is there check-fixes-not? This seems to be required here, because even if the 
fixit won't happen here, the test will pass.


https://reviews.llvm.org/D21298



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

[PATCH] D26511: [clang-tidy] Rename modernize-use-default to modernize-use-equals-default

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

I think this change is not required at first place.
It is introduced because of "modernize-use-delete" was too ambiguous because of 
operator delete, so it was changed to "modernize-use-equals-delete". But this 
case is not ambiguous at all, so I don't see point changing that.
Maybe it would be better to find better name for the 
modernize-use-equals-delete to be modernize-delete-function or something before 
4.0 is released.


https://reviews.llvm.org/D26511



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

[PATCH] D26117: [Devirtualization] Decorate vfunction load with invariant.load

[Piotr Padlewski via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL285497: [Devirtualization] Decorate vfunction load with 
invariant.load (authored by Prazek).

Changed prior to commit:
  https://reviews.llvm.org/D26117?vs=76303=76306#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D26117

Files:
  cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
  cfe/trunk/test/CodeGenCXX/virtual-function-calls.cpp


Index: cfe/trunk/test/CodeGenCXX/virtual-function-calls.cpp
===
--- cfe/trunk/test/CodeGenCXX/virtual-function-calls.cpp
+++ cfe/trunk/test/CodeGenCXX/virtual-function-calls.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -emit-llvm -o - | 
FileCheck %s
+// RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -emit-llvm -o - 
-fstrict-vtable-pointers -O1 | FileCheck --check-prefix=CHECK-INVARIANT %s
 
 // PR5021
 namespace PR5021 {
@@ -42,10 +43,14 @@
 [[noreturn]] virtual void f();
   };
 
-  // CHECK: @_ZN15VirtualNoreturn1f
+  // CHECK-LABEL: @_ZN15VirtualNoreturn1f
+  // CHECK-INVARIANT-LABEL: define void @_ZN15VirtualNoreturn1f
   void f(A *p) {
 p->f();
 // CHECK: call {{.*}}void %{{[^#]*$}}
 // CHECK-NOT: unreachable
+// CHECK-INVARIANT: load {{.*}} !invariant.load ![[EMPTY_NODE:[0-9]+]]
   }
 }
+
+// CHECK-INVARIANT: ![[EMPTY_NODE]] = !{}
Index: cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
===
--- cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
+++ cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
@@ -1623,7 +1623,22 @@
 
 llvm::Value *VFuncPtr =
 CGF.Builder.CreateConstInBoundsGEP1_64(VTable, VTableIndex, "vfn");
-VFunc = CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
+auto *VFuncLoad =
+CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
+
+// Add !invariant.load md to virtual function load to indicate that
+// function didn't change inside vtable.
+// It's safe to add it without -fstrict-vtable-pointers, but it would not
+// help in devirtualization because it will only matter if we will have 2
+// the same virtual function loads from the same vtable load, which won't
+// happen without enabled devirtualization with -fstrict-vtable-pointers.
+if (CGM.getCodeGenOpts().OptimizationLevel > 0 &&
+CGM.getCodeGenOpts().StrictVTablePointers)
+  VFuncLoad->setMetadata(
+  llvm::LLVMContext::MD_invariant_load,
+  llvm::MDNode::get(CGM.getLLVMContext(),
+llvm::ArrayRef()));
+VFunc = VFuncLoad;
   }
 
   CGCallee Callee(MethodDecl, VFunc);


Index: cfe/trunk/test/CodeGenCXX/virtual-function-calls.cpp
===
--- cfe/trunk/test/CodeGenCXX/virtual-function-calls.cpp
+++ cfe/trunk/test/CodeGenCXX/virtual-function-calls.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -emit-llvm -o - -fstrict-vtable-pointers -O1 | FileCheck --check-prefix=CHECK-INVARIANT %s
 
 // PR5021
 namespace PR5021 {
@@ -42,10 +43,14 @@
 [[noreturn]] virtual void f();
   };
 
-  // CHECK: @_ZN15VirtualNoreturn1f
+  // CHECK-LABEL: @_ZN15VirtualNoreturn1f
+  // CHECK-INVARIANT-LABEL: define void @_ZN15VirtualNoreturn1f
   void f(A *p) {
 p->f();
 // CHECK: call {{.*}}void %{{[^#]*$}}
 // CHECK-NOT: unreachable
+// CHECK-INVARIANT: load {{.*}} !invariant.load ![[EMPTY_NODE:[0-9]+]]
   }
 }
+
+// CHECK-INVARIANT: ![[EMPTY_NODE]] = !{}
Index: cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
===
--- cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
+++ cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
@@ -1623,7 +1623,22 @@
 
 llvm::Value *VFuncPtr =
 CGF.Builder.CreateConstInBoundsGEP1_64(VTable, VTableIndex, "vfn");
-VFunc = CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
+auto *VFuncLoad =
+CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
+
+// Add !invariant.load md to virtual function load to indicate that
+// function didn't change inside vtable.
+// It's safe to add it without -fstrict-vtable-pointers, but it would not
+// help in devirtualization because it will only matter if we will have 2
+// the same virtual function loads from the same vtable load, which won't
+// happen without enabled devirtualization with -fstrict-vtable-pointers.
+if (CGM.getCodeGenOpts().OptimizationLevel > 0 &&
+CGM.getCodeGenOpts().StrictVTablePointers)
+  VFuncLoad->setMetadata(
+  llvm::LLVMContext::MD_invariant_load,
+  llvm::MDNode::get(CGM.getLLVMContext(),
+llvm::ArrayRef()));
+VFunc = VFuncLoad;
   }
 
   CGCallee Callee(MethodDecl, VFunc);

r285497 - [Devirtualization] Decorate vfunction load with invariant.load

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Sat Oct 29 10:28:30 2016
New Revision: 285497

URL: http://llvm.org/viewvc/llvm-project?rev=285497=rev
Log:
[Devirtualization] Decorate vfunction load with invariant.load

Summary:
This patch was introduced one year ago, but because my google account
was disabled, I didn't get email with failing buildbot and I missed
revert of this commit. There was small but in test regex.
I am back.

Reviewers: rsmith, rengolin

Subscribers: nlewycky, rjmccall, cfe-commits

Differential Revision: https://reviews.llvm.org/D26117

Modified:
cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
cfe/trunk/test/CodeGenCXX/virtual-function-calls.cpp

Modified: cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp?rev=285497=285496=285497=diff
==
--- cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp (original)
+++ cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp Sat Oct 29 10:28:30 2016
@@ -1623,7 +1623,22 @@ CGCallee ItaniumCXXABI::getVirtualFuncti
 
 llvm::Value *VFuncPtr =
 CGF.Builder.CreateConstInBoundsGEP1_64(VTable, VTableIndex, "vfn");
-VFunc = CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
+auto *VFuncLoad =
+CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
+
+// Add !invariant.load md to virtual function load to indicate that
+// function didn't change inside vtable.
+// It's safe to add it without -fstrict-vtable-pointers, but it would not
+// help in devirtualization because it will only matter if we will have 2
+// the same virtual function loads from the same vtable load, which won't
+// happen without enabled devirtualization with -fstrict-vtable-pointers.
+if (CGM.getCodeGenOpts().OptimizationLevel > 0 &&
+CGM.getCodeGenOpts().StrictVTablePointers)
+  VFuncLoad->setMetadata(
+  llvm::LLVMContext::MD_invariant_load,
+  llvm::MDNode::get(CGM.getLLVMContext(),
+llvm::ArrayRef()));
+VFunc = VFuncLoad;
   }
 
   CGCallee Callee(MethodDecl, VFunc);

Modified: cfe/trunk/test/CodeGenCXX/virtual-function-calls.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/virtual-function-calls.cpp?rev=285497=285496=285497=diff
==
--- cfe/trunk/test/CodeGenCXX/virtual-function-calls.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/virtual-function-calls.cpp Sat Oct 29 10:28:30 
2016
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -emit-llvm -o - | 
FileCheck %s
+// RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -emit-llvm -o - 
-fstrict-vtable-pointers -O1 | FileCheck --check-prefix=CHECK-INVARIANT %s
 
 // PR5021
 namespace PR5021 {
@@ -42,10 +43,14 @@ namespace VirtualNoreturn {
 [[noreturn]] virtual void f();
   };
 
-  // CHECK: @_ZN15VirtualNoreturn1f
+  // CHECK-LABEL: @_ZN15VirtualNoreturn1f
+  // CHECK-INVARIANT-LABEL: define void @_ZN15VirtualNoreturn1f
   void f(A *p) {
 p->f();
 // CHECK: call {{.*}}void %{{[^#]*$}}
 // CHECK-NOT: unreachable
+// CHECK-INVARIANT: load {{.*}} !invariant.load ![[EMPTY_NODE:[0-9]+]]
   }
 }
+
+// CHECK-INVARIANT: ![[EMPTY_NODE]] = !{}


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

r285496 - NFC small format

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Sat Oct 29 10:28:25 2016
New Revision: 285496

URL: http://llvm.org/viewvc/llvm-project?rev=285496=rev
Log:
NFC small format

Modified:
cfe/trunk/lib/Analysis/UninitializedValues.cpp

Modified: cfe/trunk/lib/Analysis/UninitializedValues.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/UninitializedValues.cpp?rev=285496=285495=285496=diff
==
--- cfe/trunk/lib/Analysis/UninitializedValues.cpp (original)
+++ cfe/trunk/lib/Analysis/UninitializedValues.cpp Sat Oct 29 10:28:25 2016
@@ -348,7 +348,8 @@ public:
 }
 
 static const DeclRefExpr *getSelfInitExpr(VarDecl *VD) {
-  if (VD->getType()->isRecordType()) return nullptr;
+  if (VD->getType()->isRecordType())
+return nullptr;
   if (Expr *Init = VD->getInit()) {
 const DeclRefExpr *DRE
   = dyn_cast(stripCasts(VD->getASTContext(), Init));


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

[PATCH] D26117: [Devirtualization] Decorate vfunction load with invariant.load

[Piotr Padlewski via cfe-commits]

Prazek updated this revision to Diff 76303.
Prazek added a comment.

Updated comment and reformatted


https://reviews.llvm.org/D26117

Files:
  lib/CodeGen/ItaniumCXXABI.cpp
  test/CodeGenCXX/virtual-function-calls.cpp


Index: test/CodeGenCXX/virtual-function-calls.cpp
===
--- test/CodeGenCXX/virtual-function-calls.cpp
+++ test/CodeGenCXX/virtual-function-calls.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -emit-llvm -o - | 
FileCheck %s
+// RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -emit-llvm -o - 
-fstrict-vtable-pointers -O1 | FileCheck --check-prefix=CHECK-INVARIANT %s
 
 // PR5021
 namespace PR5021 {
@@ -42,10 +43,14 @@
 [[noreturn]] virtual void f();
   };
 
-  // CHECK: @_ZN15VirtualNoreturn1f
+  // CHECK-LABEL: @_ZN15VirtualNoreturn1f
+  // CHECK-INVARIANT-LABEL: define void @_ZN15VirtualNoreturn1f
   void f(A *p) {
 p->f();
 // CHECK: call {{.*}}void %{{[^#]*$}}
 // CHECK-NOT: unreachable
+// CHECK-INVARIANT: load {{.*}} !invariant.load ![[EMPTY_NODE:[0-9]+]]
   }
 }
+
+// CHECK-INVARIANT: ![[EMPTY_NODE]] = !{}
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -1623,7 +1623,22 @@
 
 llvm::Value *VFuncPtr =
 CGF.Builder.CreateConstInBoundsGEP1_64(VTable, VTableIndex, "vfn");
-VFunc = CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
+auto *VFuncLoad =
+CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
+
+// Add !invariant.load md to virtual function load to indicate that
+// function didn't change inside vtable.
+// It's safe to add it without -fstrict-vtable-pointers, but it would not
+// help in devirtualization because it will only matter if we will have 2
+// the same virtual function loads from the same vtable load, which won't
+// happen without enabled devirtualization with -fstrict-vtable-pointers.
+if (CGM.getCodeGenOpts().OptimizationLevel > 0 &&
+CGM.getCodeGenOpts().StrictVTablePointers)
+  VFuncLoad->setMetadata(
+  llvm::LLVMContext::MD_invariant_load,
+  llvm::MDNode::get(CGM.getLLVMContext(),
+llvm::ArrayRef()));
+VFunc = VFuncLoad;
   }
 
   CGCallee Callee(MethodDecl, VFunc);


Index: test/CodeGenCXX/virtual-function-calls.cpp
===
--- test/CodeGenCXX/virtual-function-calls.cpp
+++ test/CodeGenCXX/virtual-function-calls.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -emit-llvm -o - -fstrict-vtable-pointers -O1 | FileCheck --check-prefix=CHECK-INVARIANT %s
 
 // PR5021
 namespace PR5021 {
@@ -42,10 +43,14 @@
 [[noreturn]] virtual void f();
   };
 
-  // CHECK: @_ZN15VirtualNoreturn1f
+  // CHECK-LABEL: @_ZN15VirtualNoreturn1f
+  // CHECK-INVARIANT-LABEL: define void @_ZN15VirtualNoreturn1f
   void f(A *p) {
 p->f();
 // CHECK: call {{.*}}void %{{[^#]*$}}
 // CHECK-NOT: unreachable
+// CHECK-INVARIANT: load {{.*}} !invariant.load ![[EMPTY_NODE:[0-9]+]]
   }
 }
+
+// CHECK-INVARIANT: ![[EMPTY_NODE]] = !{}
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -1623,7 +1623,22 @@
 
 llvm::Value *VFuncPtr =
 CGF.Builder.CreateConstInBoundsGEP1_64(VTable, VTableIndex, "vfn");
-VFunc = CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
+auto *VFuncLoad =
+CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
+
+// Add !invariant.load md to virtual function load to indicate that
+// function didn't change inside vtable.
+// It's safe to add it without -fstrict-vtable-pointers, but it would not
+// help in devirtualization because it will only matter if we will have 2
+// the same virtual function loads from the same vtable load, which won't
+// happen without enabled devirtualization with -fstrict-vtable-pointers.
+if (CGM.getCodeGenOpts().OptimizationLevel > 0 &&
+CGM.getCodeGenOpts().StrictVTablePointers)
+  VFuncLoad->setMetadata(
+  llvm::LLVMContext::MD_invariant_load,
+  llvm::MDNode::get(CGM.getLLVMContext(),
+llvm::ArrayRef()));
+VFunc = VFuncLoad;
   }
 
   CGCallee Callee(MethodDecl, VFunc);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26117: [Devirtualization] Decorate vfunction load with invariant.load

[Piotr Padlewski via cfe-commits]

Prazek updated this revision to Diff 76302.
Prazek added a comment.

rebae


https://reviews.llvm.org/D26117

Files:
  lib/CodeGen/ItaniumCXXABI.cpp
  test/CodeGenCXX/virtual-function-calls.cpp


Index: test/CodeGenCXX/virtual-function-calls.cpp
===
--- test/CodeGenCXX/virtual-function-calls.cpp
+++ test/CodeGenCXX/virtual-function-calls.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -emit-llvm -o - | 
FileCheck %s
+// RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -emit-llvm -o - 
-fstrict-vtable-pointers -O1 | FileCheck --check-prefix=CHECK-INVARIANT %s
 
 // PR5021
 namespace PR5021 {
@@ -42,10 +43,14 @@
 [[noreturn]] virtual void f();
   };
 
-  // CHECK: @_ZN15VirtualNoreturn1f
+  // CHECK-LABEL: @_ZN15VirtualNoreturn1f
+  // CHECK-INVARIANT-LABEL: define void @_ZN15VirtualNoreturn1f
   void f(A *p) {
 p->f();
 // CHECK: call {{.*}}void %{{[^#]*$}}
 // CHECK-NOT: unreachable
+// CHECK-INVARIANT: load {{.*}} !invariant.load ![[EMPTY_NODE:[0-9]+]]
   }
 }
+
+// CHECK-INVARIANT: ![[EMPTY_NODE]] = !{}
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -1623,7 +1623,17 @@
 
 llvm::Value *VFuncPtr =
 CGF.Builder.CreateConstInBoundsGEP1_64(VTable, VTableIndex, "vfn");
-VFunc = CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
+auto *VFuncLoad = CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF
+  .getPointerAlign());
+
+// It's safe to add "invariant.load" without -fstrict-vtable-pointers, but
+// it would not help in devirtualization.
+if (CGM.getCodeGenOpts().OptimizationLevel > 0 &&
+CGM.getCodeGenOpts().StrictVTablePointers)
+  VFuncLoad->setMetadata(llvm::LLVMContext::MD_invariant_load,
+llvm::MDNode::get(CGM.getLLVMContext(),
+  llvm::ArrayRef()));
+VFunc = VFuncLoad;
   }
 
   CGCallee Callee(MethodDecl, VFunc);


Index: test/CodeGenCXX/virtual-function-calls.cpp
===
--- test/CodeGenCXX/virtual-function-calls.cpp
+++ test/CodeGenCXX/virtual-function-calls.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -emit-llvm -o - -fstrict-vtable-pointers -O1 | FileCheck --check-prefix=CHECK-INVARIANT %s
 
 // PR5021
 namespace PR5021 {
@@ -42,10 +43,14 @@
 [[noreturn]] virtual void f();
   };
 
-  // CHECK: @_ZN15VirtualNoreturn1f
+  // CHECK-LABEL: @_ZN15VirtualNoreturn1f
+  // CHECK-INVARIANT-LABEL: define void @_ZN15VirtualNoreturn1f
   void f(A *p) {
 p->f();
 // CHECK: call {{.*}}void %{{[^#]*$}}
 // CHECK-NOT: unreachable
+// CHECK-INVARIANT: load {{.*}} !invariant.load ![[EMPTY_NODE:[0-9]+]]
   }
 }
+
+// CHECK-INVARIANT: ![[EMPTY_NODE]] = !{}
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -1623,7 +1623,17 @@
 
 llvm::Value *VFuncPtr =
 CGF.Builder.CreateConstInBoundsGEP1_64(VTable, VTableIndex, "vfn");
-VFunc = CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
+auto *VFuncLoad = CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF
+  .getPointerAlign());
+
+// It's safe to add "invariant.load" without -fstrict-vtable-pointers, but
+// it would not help in devirtualization.
+if (CGM.getCodeGenOpts().OptimizationLevel > 0 &&
+CGM.getCodeGenOpts().StrictVTablePointers)
+  VFuncLoad->setMetadata(llvm::LLVMContext::MD_invariant_load,
+llvm::MDNode::get(CGM.getLLVMContext(),
+  llvm::ArrayRef()));
+VFunc = VFuncLoad;
   }
 
   CGCallee Callee(MethodDecl, VFunc);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26117: [Devirtualization] Decorate vfunction load with invariant.load

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

merging with master. Gonna update patch in a minute.


https://reviews.llvm.org/D26117



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

[PATCH] D26117: [Devirtualization] Decorate vfunction load with invariant.load

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

Here is commit from last year
https://reviews.llvm.org/D13279


https://reviews.llvm.org/D26117



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

[PATCH] D26117: [Devirtualization] Decorate vfunction load with invariant.load

[Piotr Padlewski via cfe-commits]

Prazek created this revision.
Prazek added reviewers: rsmith, rengolin.
Prazek added subscribers: cfe-commits, rjmccall, nlewycky.

This patch was introduced one year ago, but because my google account
was disabled, I didn't get email with failing buildbot and I missed
revert of this commit. There was small but in test regex.
I am back.


https://reviews.llvm.org/D26117

Files:
  lib/CodeGen/ItaniumCXXABI.cpp
  test/CodeGenCXX/virtual-function-calls.cpp


Index: test/CodeGenCXX/virtual-function-calls.cpp
===
--- test/CodeGenCXX/virtual-function-calls.cpp
+++ test/CodeGenCXX/virtual-function-calls.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -emit-llvm -o - | 
FileCheck %s
+// RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -emit-llvm -o - 
-fstrict-vtable-pointers -O1 | FileCheck --check-prefix=CHECK-INVARIANT %s
 
 // PR5021
 namespace PR5021 {
@@ -42,10 +43,14 @@
 [[noreturn]] virtual void f();
   };
 
-  // CHECK: @_ZN15VirtualNoreturn1f
+  // CHECK-LABEL: @_ZN15VirtualNoreturn1f
+  // CHECK-INVARIANT-LABEL: define void @_ZN15VirtualNoreturn1f
   void f(A *p) {
 p->f();
 // CHECK: call {{.*}}void %{{[^#]*$}}
 // CHECK-NOT: unreachable
+// CHECK-INVARIANT: load {{.*}} !invariant.load ![[EMPTY_NODE:[0-9]+]]
   }
 }
+
+// CHECK-INVARIANT: ![[EMPTY_NODE]] = !{}
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -1618,7 +1618,16 @@
 
 llvm::Value *VFuncPtr =
 CGF.Builder.CreateConstInBoundsGEP1_64(VTable, VTableIndex, "vfn");
-return CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
+auto *Inst = CGF.Builder.CreateAlignedLoad(VFuncPtr, 
CGF.getPointerAlign());
+
+// It's safe to add "invariant.load" without -fstrict-vtable-pointers, but
+// it would not help in devirtualization.
+if (CGM.getCodeGenOpts().OptimizationLevel > 0 &&
+CGM.getCodeGenOpts().StrictVTablePointers)
+  Inst->setMetadata(llvm::LLVMContext::MD_invariant_load,
+llvm::MDNode::get(CGM.getLLVMContext(),
+  llvm::ArrayRef()));
+return Inst;
   }
 }
 


Index: test/CodeGenCXX/virtual-function-calls.cpp
===
--- test/CodeGenCXX/virtual-function-calls.cpp
+++ test/CodeGenCXX/virtual-function-calls.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple %itanium_abi_triple -std=c++11 -emit-llvm -o - -fstrict-vtable-pointers -O1 | FileCheck --check-prefix=CHECK-INVARIANT %s
 
 // PR5021
 namespace PR5021 {
@@ -42,10 +43,14 @@
 [[noreturn]] virtual void f();
   };
 
-  // CHECK: @_ZN15VirtualNoreturn1f
+  // CHECK-LABEL: @_ZN15VirtualNoreturn1f
+  // CHECK-INVARIANT-LABEL: define void @_ZN15VirtualNoreturn1f
   void f(A *p) {
 p->f();
 // CHECK: call {{.*}}void %{{[^#]*$}}
 // CHECK-NOT: unreachable
+// CHECK-INVARIANT: load {{.*}} !invariant.load ![[EMPTY_NODE:[0-9]+]]
   }
 }
+
+// CHECK-INVARIANT: ![[EMPTY_NODE]] = !{}
Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -1618,7 +1618,16 @@
 
 llvm::Value *VFuncPtr =
 CGF.Builder.CreateConstInBoundsGEP1_64(VTable, VTableIndex, "vfn");
-return CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
+auto *Inst = CGF.Builder.CreateAlignedLoad(VFuncPtr, CGF.getPointerAlign());
+
+// It's safe to add "invariant.load" without -fstrict-vtable-pointers, but
+// it would not help in devirtualization.
+if (CGM.getCodeGenOpts().OptimizationLevel > 0 &&
+CGM.getCodeGenOpts().StrictVTablePointers)
+  Inst->setMetadata(llvm::LLVMContext::MD_invariant_load,
+llvm::MDNode::get(CGM.getLLVMContext(),
+  llvm::ArrayRef()));
+return Inst;
   }
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25898: [clang-tidy] Enhance modernize-make-unique to handle unique_ptr.reset()

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.



Comment at: test/clang-tidy/modernize-make-shared.cpp:122
+  Pderived = std::shared_ptr(new Derived());
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: use std::make_shared instead
+  // CHECK-FIXES: Pderived = std::make_shared();

malcolm.parsons wrote:
> Prazek wrote:
> > I think the warning here could be better. The user is using make_shared 
> > here.
> > Maybe ' warning: use std::make_shared with zero arguments ...', but only in 
> > this case
> The user isn't using make_shared.
true, I've read that incorectly



Comment at: test/clang-tidy/modernize-make-shared.cpp:129
+  // FIXME: OK to replace when auto is not used
+  std::shared_ptr PBase = std::shared_ptr(new Derived());
+

malcolm.parsons wrote:
> Prazek wrote:
> > I think it is good to replace it even with auto, like
> > auto PBase = std::make_shared(new Derived());
> > 
> > For shared_ptr we can even do better, that we can't do for unique_ptr - we
> > coud change it to
> > auto PBase = std::make_shared();
> > because all conversions works.
> > Of course not in this patch, but it would be good to leave a comment about 
> > this here.
> A smart pointer to Derived cannot be reset with a pointer to Base.
oh yea, I was only thinking about downcasting smart pointer


https://reviews.llvm.org/D25898



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

[PATCH] D21502: Fix heuristics skipping invalid ctor-initializers with C++11

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

Richard?


https://reviews.llvm.org/D21502



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

[PATCH] D24894: [clang-tidy] Prefer transparent functors to non-transparent one.

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.



Comment at: docs/clang-tidy/checks/modernize-use-transparent-functors.rst:12
+  .. code-block:: c++
+
+// Non-transparent functor  

Say somewhere that you also handle cases like
std::less(arg1, arg2)
because from this documentation I didn't know about it.


https://reviews.llvm.org/D24894



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

[PATCH] D24894: [clang-tidy] Prefer transparent functors to non-transparent one.

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.



Comment at: clang-tidy/modernize/UseTransparentFunctorsCheck.cpp:70
+static const StringRef Message =
+"prefer transparent functors (aka diamond operators)";
+

The message would be much better if you would put the name of this functor, like
"prefer transparent functor (%0)" where %0 would be evaluated to 
'std::greater<>" etc.



Comment at: clang-tidy/modernize/UseTransparentFunctorsCheck.cpp:89-109
+  for (; ArgNum < FunctorParentLoc.getNumArgs(); ++ArgNum) {
+const TemplateArgument  =
+FunctorParentLoc.getArgLoc(ArgNum).getArgument();
+if (Arg.getKind() != TemplateArgument::Type)
+  continue;
+QualType ParentArgType = Arg.getAsType();
+if (ParentArgType->isRecordType() &&

This can be moved to one or 2 functions, returning FunctorTypeLoc or 
llvm::Optional



Comment at: docs/clang-tidy/checks/modernize-use-transparent-functors.rst:32-33
+
+   If the option is set to non-zero (default is `0`), the check will not
+   warn on those cases where automatic FIXIT is not safe to apply.

I think
... will not warn on these cases as shown above, where automatic FIXIT...
would have been better.



Comment at: docs/clang-tidy/checks/modernize-use-transparent-functors.rst:33
+   If the option is set to non-zero (default is `0`), the check will not
+   warn on those cases where automatic FIXIT is not safe to apply.

Add a note
This check requires C++14 or higher to run.


https://reviews.llvm.org/D24894



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

[PATCH] D25898: [clang-tidy] Enhance modernize-make-unique to handle unique_ptr.reset()

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

Besides this looks good




Comment at: test/clang-tidy/modernize-make-shared.cpp:122
+  Pderived = std::shared_ptr(new Derived());
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: use std::make_shared instead
+  // CHECK-FIXES: Pderived = std::make_shared();

I think the warning here could be better. The user is using make_shared here.
Maybe ' warning: use std::make_shared with zero arguments ...', but only in 
this case



Comment at: test/clang-tidy/modernize-make-shared.cpp:129
+  // FIXME: OK to replace when auto is not used
+  std::shared_ptr PBase = std::shared_ptr(new Derived());
+

I think it is good to replace it even with auto, like
auto PBase = std::make_shared(new Derived());

For shared_ptr we can even do better, that we can't do for unique_ptr - we
coud change it to
auto PBase = std::make_shared();
because all conversions works.
Of course not in this patch, but it would be good to leave a comment about this 
here.


https://reviews.llvm.org/D25898



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

[PATCH] D25316: [clang-tidy] Fix PR25499: Enhance modernize-use-auto to casts

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

BTW I think changing the commit name by removing bug ID would be good, because 
it would be more clear that this is a feature commit, not a bug fix. 
You can move t he bug id, or the link to bug to the summary section.


https://reviews.llvm.org/D25316



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

[PATCH] D25316: [clang-tidy] Fix PR25499: Enhance modernize-use-auto to casts

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.



Comment at: test/clang-tidy/modernize-use-auto-cast-remove-stars.cpp:25
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with 
a cast to avoid duplicating the type name
+  // CHECK-FIXES: auto  ll = static_cast(l);
+  unsigned long long ull = static_cast(l);

malcolm.parsons wrote:
> Prazek wrote:
> > malcolm.parsons wrote:
> > > Prazek wrote:
> > > > Is it possible to simply fix the double spaces?
> > > The existing modernize-use-auto tests have the same problem.
> > > My codebase is clang-formatted regularly so it doesn't bother me.
> > I agree that it is not huge problem with clang-format and of course it is 
> > not required to fix it to push this patch upstream. It would be good to 
> > leave a comment somewhere with FIXME: about this, so it would be easier for 
> > some other contributor to fix it (or if it simple, just fix it in this 
> > patch :))
> There's a FIXME comment in one of the existing modernize-use-auto test files.
Yes, but I am talking about FIXME comment in check code. IF someone reads the 
code, it is much more clear for them that this thing doesn't work completely, 
than if the programmer would see the error and would have to look at tests or 
execute clang-tidy to make sure that he understand code well.


https://reviews.llvm.org/D25316



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

[PATCH] D25316: [clang-tidy] Fix PR25499: Enhance modernize-use-auto to casts

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

In https://reviews.llvm.org/D25316#565574, @malcolm.parsons wrote:

> In https://reviews.llvm.org/D25316#565567, @Prazek wrote:
>
> >   functionDecl(hasName(LEXICAL_CAST_NAME),
>
>
> I was trying to avoid specifying the name of the function so that it works 
> for any templated function/member function that returns its first template 
> type.


Good idea. I am not sure how it will work when you will run it on big codebase, 
so I wanted to make it configurable for any function name. But if it will work 
for 99% of useful cases, then it will be great!


https://reviews.llvm.org/D25316



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

[PATCH] D25316: [clang-tidy] Fix PR25499: Enhance modernize-use-auto to casts

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

Awesome to see this patch. After this one will make it to upstream, it will be 
much easier for me to do same with template functions.




Comment at: test/clang-tidy/modernize-use-auto-cast-remove-stars.cpp:2
+// RUN: %check_clang_tidy %s modernize-use-auto %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-auto.RemoveStars, 
value: '1'}]}" \
+// RUN:   -- -std=c++11

What is the difference between this test, and next test?
The name indicate that it removes star, but I see a lot of test that doesn't 
use star
and that seem to be duplicated with the next one.



Comment at: test/clang-tidy/modernize-use-auto-cast-remove-stars.cpp:25
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with 
a cast to avoid duplicating the type name
+  // CHECK-FIXES: auto  ll = static_cast(l);
+  unsigned long long ull = static_cast(l);

Is it possible to simply fix the double spaces?



Comment at: test/clang-tidy/modernize-use-auto-cast.cpp:14
+  long l = 1;
+  int i1 = static_cast(l);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with 
a cast to avoid duplicating the type name

How do you handle cases like

  long l = static_cast(i1);

I would expect it to produce warning, but not produce fixit (because it might 
change the behavior of program), but I still would like to get information 
about it because it seems like a bug.


https://reviews.llvm.org/D25316



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

[PATCH] D25363: Store a SourceRange for multi-token builtin types

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

Thanks for the patch! I think some unit test should be added.

Do you also handle cases like

  unsigned long long
  unsigned volatile long const long static

etc.
The problem here is that the whole type like "unsigned long long" could be in 
other tokens. 
I talked with Richard Smith while ago and one of the solutions proposed was to 
have "isValid()" for source range
returning false for cases like this

  unsigned const long




Comment at: include/clang/AST/TypeLoc.h:529
+  void setBuiltinLocStart(SourceLocation Loc) {
+if (getLocalData()->BuiltinRange.getEnd().isValid()) {
+  getLocalData()->BuiltinRange.setBegin(Loc);

Can you add a comment here? It might be because I don't know the AST API, but 
it is not clear for me what does it do.



Comment at: lib/Sema/DeclSpec.cpp:621
   TypeSpecWidth = W;
+  // Remember location of the last 'long'
+  TSTLoc = Loc;

What about cases like 
  unsigned int
  unsigned long

This is not only about long.


https://reviews.llvm.org/D25363



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

[PATCH] D25316: [clang-tidy] Fix PR25499: Enhance modernize-use-auto to casts

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

In https://reviews.llvm.org/D25316#564217, @malcolm.parsons wrote:

> In https://reviews.llvm.org/D25316#563930, @Prazek wrote:
>
> > Please add tests with
> >
> >   long long p = static_cast(4);
> >   
> >
> > and the same with const at beginning. I remember I had problems with this 
> > last time (Type->SourceRange was returning only source range for the first 
> > token.
>
>
> BuiltinTypeLoc only returns the first token:
>
>   SourceRange getLocalSourceRange() const {
> return SourceRange(getBuiltinLoc(), getBuiltinLoc());
>   }
>   
>
> The existing check fails too:
>
>   -long long *ll = new long long();
>   +auto long *ll = new long long();


Interesting! I remember checking it half year ago, and it was working 
(SourceRange was returning all tokens from first one
to asterisk). I remember there was some small features introduced, like instead 
of
auto p = long long new;
to produce
auto *p = long long new;

Maybe it was introduced in that patch. Anyway I think this have to be fixed 
somehow. Either by playing with lexer, or by fixing sourceRange


https://reviews.llvm.org/D25316



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

[PATCH] D25316: [clang-tidy] Fix PR25499: Enhance modernize-use-auto to casts

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

Please add tests with

  long long p = static_cast(4);

and the same with const at beginning. I remember I had problems with this last 
time (Type->SourceRange was returning only source range for the first token.
I will review patch later.


https://reviews.llvm.org/D25316



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

Re: [PATCH] D24439: [Clang] Fix some Clang-tidy modernize-use-bool-literals and Include What You Use warnings; other minor fixes

[Piotr Padlewski via cfe-commits]

Prazek added a subscriber: Prazek.
Prazek accepted this revision.
Prazek added a reviewer: Prazek.
Prazek added a comment.
This revision is now accepted and ready to land.

Lgtm


Repository:
  rL LLVM

https://reviews.llvm.org/D24439



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

Re: [PATCH] D24349: [clang-tidy] Extend readability-container-size-empty to arbitrary class with size() and empty()

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: clang-tidy/readability/ContainerSizeEmptyCheck.cpp:34
@@ +33,3 @@
+  has(functionDecl(
+  isPublic(), hasName("size"), returns(isInteger()),
+  unless(anyOf(returns(isAnyCharacter()), returns(booleanType()),

Would be nice to have 'isStrictlyInteger' matcher to do this thing.


Repository:
  rL LLVM

https://reviews.llvm.org/D24349



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

Re: [PATCH] D24339: [clang-tidy] Add check 'readability-redundant-member-init'

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: clang-tidy/readability/RedundantMemberInitCheck.cpp:34
@@ +33,3 @@
+  const auto *Construct = 
Result.Nodes.getNodeAs("construct");
+  const auto arguments = Construct->arguments();
+

Arguments (upper case)


Repository:
  rL LLVM

https://reviews.llvm.org/D24339



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

Re: [PATCH] D23343: [clang-tidy] modernize-make-{smart_ptr} private ctor bugfix

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

Sorry for long delay. I had some issues with git-svn on mac.


Repository:
  rL LLVM

https://reviews.llvm.org/D23343



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

Re: [PATCH] D23343: [clang-tidy] modernize-make-{smart_ptr} private ctor bugfix

[Piotr Padlewski via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL280180: [clang-tidy] modernize-make-{smart_ptr} private ctor 
bugfix (authored by Prazek).

Changed prior to commit:
  https://reviews.llvm.org/D23343?vs=68824=69787#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D23343

Files:
  clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/test/clang-tidy/modernize-make-shared.cpp
  clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique.cpp

Index: clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique.cpp
@@ -103,6 +103,38 @@
   std::unique_ptr Placement = std::unique_ptr(new (PInt) int{3});
 }
 
+// Calling make_smart_ptr from within a member function of a type with a
+// private or protected constructor would be ill-formed.
+class Private {
+private:
+  Private(int z) {}
+
+public:
+  Private() {}
+  void create() {
+auto callsPublic = std::unique_ptr(new Private);
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_unique instead
+// CHECK-FIXES: auto callsPublic = std::make_unique();
+auto ptr = std::unique_ptr(new Private(42));
+  }
+
+  virtual ~Private();
+};
+
+class Protected {
+protected:
+  Protected() {}
+
+public:
+  Protected(int, int) {}
+  void create() {
+auto callsPublic = std::unique_ptr(new Protected(1, 2));
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_unique instead
+// CHECK-FIXES: auto callsPublic = std::make_unique(1, 2);
+auto ptr = std::unique_ptr(new Protected);
+  }
+};
+
 void initialization(int T, Base b) {
   // Test different kinds of initialization of the pointee.
 
Index: clang-tools-extra/trunk/test/clang-tidy/modernize-make-shared.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/modernize-make-shared.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-make-shared.cpp
@@ -100,6 +100,38 @@
   std::shared_ptr Placement = std::shared_ptr(new (PInt) int{3});
 }
 
+// Calling make_smart_ptr from within a member function of a type with a
+// private or protected constructor would be ill-formed.
+class Private {
+private:
+  Private(int z) {}
+
+public:
+  Private() {}
+  void create() {
+auto callsPublic = std::shared_ptr(new Private);
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_shared instead
+// CHECK-FIXES: auto callsPublic = std::make_shared();
+auto ptr = std::shared_ptr(new Private(42));
+  }
+
+  virtual ~Private();
+};
+
+class Protected {
+protected:
+  Protected() {}
+
+public:
+  Protected(int, int) {}
+  void create() {
+auto callsPublic = std::shared_ptr(new Protected(1, 2));
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_shared instead
+// CHECK-FIXES: auto callsPublic = std::make_shared(1, 2);
+auto ptr = std::shared_ptr(new Protected);
+  }
+};
+
 void initialization(int T, Base b) {
   // Test different kinds of initialization of the pointee.
 
Index: clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp
@@ -29,13 +29,19 @@
   if (!getLangOpts().CPlusPlus11)
 return;
 
+  // Calling make_smart_ptr from within a member function of a type with a
+  // private or protected constructor would be ill-formed.
+  auto CanCallCtor = unless(has(ignoringImpCasts(cxxConstructExpr(
+  hasDeclaration(decl(unless(isPublic(;
+
   Finder->addMatcher(
   cxxBindTemporaryExpr(has(ignoringParenImpCasts(
   cxxConstructExpr(
   hasType(getSmartPointerTypeMatcher()), argumentCountIs(1),
   hasArgument(0,
   cxxNewExpr(hasType(pointsTo(qualType(hasCanonicalType(
- equalsBoundNode(PointerType))
+ equalsBoundNode(PointerType),
+ CanCallCtor)
   .bind(NewExpression)))
   .bind(ConstructorCall,
   this);
Index: clang-tools-extra/trunk/docs/ReleaseNotes.rst
===
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst
@@ -97,6 +97,14 @@
   Flags function parameters of a pointer type that could be changed to point to
   a constant type instead.
 
+Fixed bugs:
+- `modernize-make-unique
+  `_
+  and `modernize-make-shared

[clang-tools-extra] r280180 - [clang-tidy] modernize-make-{smart_ptr} private ctor bugfix

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Tue Aug 30 19:06:55 2016
New Revision: 280180

URL: http://llvm.org/viewvc/llvm-project?rev=280180=rev
Log:
[clang-tidy] modernize-make-{smart_ptr} private ctor bugfix

Summary:
Bugfix for 27321. When the constructor of stored pointer
type is private then it is invalid to change it to
make_shared or make_unique.

Reviewers: alexfh, aaron.ballman, hokein

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D23343

Modified:
clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/test/clang-tidy/modernize-make-shared.cpp
clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique.cpp

Modified: clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp?rev=280180=280179=280180=diff
==
--- clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp Tue Aug 
30 19:06:55 2016
@@ -29,13 +29,19 @@ void MakeSmartPtrCheck::registerMatchers
   if (!getLangOpts().CPlusPlus11)
 return;
 
+  // Calling make_smart_ptr from within a member function of a type with a
+  // private or protected constructor would be ill-formed.
+  auto CanCallCtor = unless(has(ignoringImpCasts(cxxConstructExpr(
+  hasDeclaration(decl(unless(isPublic(;
+
   Finder->addMatcher(
   cxxBindTemporaryExpr(has(ignoringParenImpCasts(
   cxxConstructExpr(
   hasType(getSmartPointerTypeMatcher()), argumentCountIs(1),
   hasArgument(0,
   
cxxNewExpr(hasType(pointsTo(qualType(hasCanonicalType(
- equalsBoundNode(PointerType))
+ equalsBoundNode(PointerType),
+ CanCallCtor)
   .bind(NewExpression)))
   .bind(ConstructorCall,
   this);

Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=280180=280179=280180=diff
==
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Tue Aug 30 19:06:55 2016
@@ -97,6 +97,14 @@ Improvements to clang-tidy
   Flags function parameters of a pointer type that could be changed to point to
   a constant type instead.
 
+Fixed bugs:
+- `modernize-make-unique
+  `_
+  and `modernize-make-shared
+  `_
+  Calling ``make_{unique|shared}`` from within a member function of a type
+  with a private or protected constructor would be ill-formed.
+
 Improvements to include-fixer
 -
 

Modified: clang-tools-extra/trunk/test/clang-tidy/modernize-make-shared.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/modernize-make-shared.cpp?rev=280180=280179=280180=diff
==
--- clang-tools-extra/trunk/test/clang-tidy/modernize-make-shared.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-make-shared.cpp Tue Aug 
30 19:06:55 2016
@@ -100,6 +100,38 @@ void basic() {
   std::shared_ptr Placement = std::shared_ptr(new (PInt) int{3});
 }
 
+// Calling make_smart_ptr from within a member function of a type with a
+// private or protected constructor would be ill-formed.
+class Private {
+private:
+  Private(int z) {}
+
+public:
+  Private() {}
+  void create() {
+auto callsPublic = std::shared_ptr(new Private);
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_shared instead
+// CHECK-FIXES: auto callsPublic = std::make_shared();
+auto ptr = std::shared_ptr(new Private(42));
+  }
+
+  virtual ~Private();
+};
+
+class Protected {
+protected:
+  Protected() {}
+
+public:
+  Protected(int, int) {}
+  void create() {
+auto callsPublic = std::shared_ptr(new Protected(1, 2));
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_shared instead
+// CHECK-FIXES: auto callsPublic = std::make_shared(1, 2);
+auto ptr = std::shared_ptr(new Protected);
+  }
+};
+
 void initialization(int T, Base b) {
   // Test different kinds of initialization of the pointee.
 

Modified: clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique.cpp?rev=280180=280179=280180=diff

Re: [PATCH] D23343: [clang-tidy] modernize-make-{smart_ptr} private ctor bugfix

[Piotr Padlewski via cfe-commits]

Prazek updated this revision to Diff 68824.
Prazek added a comment.

- fixes


https://reviews.llvm.org/D23343

Files:
  clang-tidy/modernize/MakeSmartPtrCheck.cpp
  docs/ReleaseNotes.rst
  test/clang-tidy/modernize-make-shared.cpp
  test/clang-tidy/modernize-make-unique.cpp

Index: test/clang-tidy/modernize-make-unique.cpp
===
--- test/clang-tidy/modernize-make-unique.cpp
+++ test/clang-tidy/modernize-make-unique.cpp
@@ -103,6 +103,38 @@
   std::unique_ptr Placement = std::unique_ptr(new (PInt) int{3});
 }
 
+// Calling make_smart_ptr from within a member function of a type with a
+// private or protected constructor would be ill-formed.
+class Private {
+private:
+  Private(int z) {}
+
+public:
+  Private() {}
+  void create() {
+auto callsPublic = std::unique_ptr(new Private);
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_unique instead
+// CHECK-FIXES: auto callsPublic = std::make_unique();
+auto ptr = std::unique_ptr(new Private(42));
+  }
+
+  virtual ~Private();
+};
+
+class Protected {
+protected:
+  Protected() {}
+
+public:
+  Protected(int, int) {}
+  void create() {
+auto callsPublic = std::unique_ptr(new Protected(1, 2));
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_unique instead
+// CHECK-FIXES: auto callsPublic = std::make_unique(1, 2);
+auto ptr = std::unique_ptr(new Protected);
+  }
+};
+
 void initialization(int T, Base b) {
   // Test different kinds of initialization of the pointee.
 
Index: test/clang-tidy/modernize-make-shared.cpp
===
--- test/clang-tidy/modernize-make-shared.cpp
+++ test/clang-tidy/modernize-make-shared.cpp
@@ -100,6 +100,38 @@
   std::shared_ptr Placement = std::shared_ptr(new (PInt) int{3});
 }
 
+// Calling make_smart_ptr from within a member function of a type with a
+// private or protected constructor would be ill-formed.
+class Private {
+private:
+  Private(int z) {}
+
+public:
+  Private() {}
+  void create() {
+auto callsPublic = std::shared_ptr(new Private);
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_shared instead
+// CHECK-FIXES: auto callsPublic = std::make_shared();
+auto ptr = std::shared_ptr(new Private(42));
+  }
+
+  virtual ~Private();
+};
+
+class Protected {
+protected:
+  Protected() {}
+
+public:
+  Protected(int, int) {}
+  void create() {
+auto callsPublic = std::shared_ptr(new Protected(1, 2));
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_shared instead
+// CHECK-FIXES: auto callsPublic = std::make_shared(1, 2);
+auto ptr = std::shared_ptr(new Protected);
+  }
+};
+
 void initialization(int T, Base b) {
   // Test different kinds of initialization of the pointee.
 
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -85,6 +85,15 @@
   Warns about the performance overhead arising from concatenating strings using
   the ``operator+``, instead of ``operator+=``.
 
+
+Fixed bugs:
+- `modernize-make-unique
+  `_
+  and `modernize-make-shared
+  `_
+  Calling ``make_{unique|shared}`` from within a member function of a type
+  with a private or protected constructor would be ill-formed.
+
 Improvements to include-fixer
 -
 
Index: clang-tidy/modernize/MakeSmartPtrCheck.cpp
===
--- clang-tidy/modernize/MakeSmartPtrCheck.cpp
+++ clang-tidy/modernize/MakeSmartPtrCheck.cpp
@@ -29,13 +29,19 @@
   if (!getLangOpts().CPlusPlus11)
 return;
 
+  // Calling make_smart_ptr from within a member function of a type with a
+  // private or protected constructor would be ill-formed.
+  auto CanCallCtor = unless(has(ignoringImpCasts(cxxConstructExpr(
+  hasDeclaration(decl(unless(isPublic(;
+
   Finder->addMatcher(
   cxxBindTemporaryExpr(has(ignoringParenImpCasts(
   cxxConstructExpr(
   hasType(getSmartPointerTypeMatcher()), argumentCountIs(1),
   hasArgument(0,
   cxxNewExpr(hasType(pointsTo(qualType(hasCanonicalType(
- equalsBoundNode(PointerType))
+ equalsBoundNode(PointerType),
+ CanCallCtor)
   .bind(NewExpression)))
   .bind(ConstructorCall,
   this);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23343: [clang-tidy] modernize-make-{smart_ptr} private ctor bugfix

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:35
@@ +34,3 @@
+  auto CanCallCtor = unless(has(ignoringImpCasts(cxxConstructExpr(
+  hasDeclaration(decl(anyOf(isPrivate(), isProtected(;
+

alexfh wrote:
> Prazek wrote:
> > alexfh wrote:
> > > Prazek wrote:
> > > > aaron.ballman wrote:
> > > > > Prazek wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > Perhaps: `unless(isPublic())` instead of `anyOf(isPrivate(), 
> > > > > > > isProtected())`?
> > > > > > POD types doesn't have public constructors so it will fail :)
> > > > > Don't they have an implicit one for the purposes of a 
> > > > > CXXConstructExpr? Looking at an AST dump for:
> > > > > ```
> > > > > struct S {
> > > > >   int i;
> > > > > };
> > > > > ```
> > > > > yields:
> > > > > ```
> > > > > |-CXXRecordDecl 0x26d74ae5348  line:25:8 
> > > > > referenced struct S definition
> > > > > | |-CXXRecordDecl 0x26d74ae5460  col:8 implicit struct S
> > > > > | |-FieldDecl 0x26d74ae7880  col:7 i 'int'
> > > > > | |-CXXConstructorDecl 0x26d74ae87b8  col:8 implicit used 
> > > > > S 'void (void) noexcept' inline
> > > > > | | `-CompoundStmt 0x26d74ae3850 
> > > > > | |-CXXConstructorDecl 0x26d74ae34a8  col:8 implicit constexpr 
> > > > > S 'void (const struct S &)' inline noexcept-unevaluated 0x26d74ae34a8
> > > > > | | `-ParmVarDecl 0x26d74ae35e0  col:8 'const struct S &'
> > > > > | `-CXXConstructorDecl 0x26d74ae3678  col:8 implicit constexpr 
> > > > > S 'void (struct S &&)' inline noexcept-unevaluated 0x26d74ae3678
> > > > > |   `-ParmVarDecl 0x26d74ae37b0  col:8 'struct S &&'
> > > > > ```
> > > > what about std::shared_ptr x = std::shared_ptr(new int); ?
> > > > If I recall correctly, it didn't work on this test.
> > > There's no `CXXConstructExpr` for primitive types:
> > > ```
> > > $ cat /tmp/qq.cc 
> > > #include 
> > > 
> > > struct S { S(); };
> > > void () {
> > >   std::shared_ptr p1 = std::shared_ptr(new int);
> > >   std::shared_ptr p2 = std::shared_ptr(new S);
> > > }
> > > $ clang-check -ast-dump -ast-dump-filter= /tmp/qq.cc -- -std=c++11
> > > Dumping :
> > > FunctionDecl 0x7f48270fb6e0  line:4:6  
> > > 'void (void)'
> > > `-CompoundStmt 0x7f48271582a8 
> > >   |-DeclStmt 0x7f4827131000 
> > >   | `-VarDecl 0x7f48270fb9c8  col:24 p1 
> > > 'std::shared_ptr':'class std::shared_ptr' cinit
> > >   |   `-ExprWithCleanups 0x7f4827130fe8  
> > > 'std::shared_ptr':'class std::shared_ptr'
> > >   | `-CXXConstructExpr 0x7f4827130fb0  
> > > 'std::shared_ptr':'class std::shared_ptr' 'void (class 
> > > std::shared_ptr &&) noexcept' elidable
> > >   |   `-MaterializeTemporaryExpr 0x7f4827130f98  
> > > 'class std::shared_ptr' xvalue
> > >   | `-CXXFunctionalCastExpr 0x7f48271303c8  
> > > 'std::shared_ptr':'class std::shared_ptr' functional cast to 
> > > std::shared_ptr 
> > >   |   `-CXXBindTemporaryExpr 0x7f48271303a8  
> > > 'std::shared_ptr':'class std::shared_ptr' (CXXTemporary 
> > > 0x7f48271303a0)
> > >   | `-CXXConstructExpr 0x7f4827130338  
> > > 'std::shared_ptr':'class std::shared_ptr' 'void (int *)'
> > >   |   `-CXXNewExpr 0x7f48270fbb58  'int *' 
> > > Function 0x7f4826a1c000 'operator new' 'void *(std::size_t)'
> > >   `-DeclStmt 0x7f4827158290 
> > > `-VarDecl 0x7f4827131238  col:22 p2 
> > > 'std::shared_ptr':'class std::shared_ptr' cinit
> > >   `-ExprWithCleanups 0x7f4827158278  
> > > 'std::shared_ptr':'class std::shared_ptr'
> > > `-CXXConstructExpr 0x7f4827158240  
> > > 'std::shared_ptr':'class std::shared_ptr' 'void (class 
> > > std::shared_ptr &&) noexcept' elidable
> > >   `-MaterializeTemporaryExpr 0x7f4827158228  
> > > 'class std::shared_ptr' xvalue
> > > `-CXXFunctionalCastExpr 0x7f4827157658  
> > > 'std::shared_ptr':'class std::shared_ptr' functional cast to 
> > > std::shared_ptr 
> > >   `-CXXBindTemporaryExpr 0x7f4827157638  
> > > 'std::shared_ptr':'class std::shared_ptr' (CXXTemporary 
> > > 0x7f4827157630)
> > > `-CXXConstructExpr 0x7f48271575c8  
> > > 'std::shared_ptr':'class std::shared_ptr' 'void (struct S *)'
> > >   `-CXXNewExpr 0x7f4827131838  'struct S 
> > > *' Function 0x7f4826a1c000 'operator new' 'void *(std::size_t)'
> > > `-CXXConstructExpr 0x7f4827131808  'struct S' 
> > > 'void (void)'
> > > ```
> > exactly.
> But this means that `unless(isPublic())` should work, IIUC.
Oh I see.
Yea I remember right now what I did:
has(ignoringImpCasts(cxxConstructExpr(
  

Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

In https://reviews.llvm.org/D23353#516314, @mboehme wrote:

> In https://reviews.llvm.org/D23353#511362, @Prazek wrote:
>
> > I will review it later, but my first thoughts:
> >
> > 1. I think we should make some other group, because misc seems to be 
> > overloaded. I discussed it with Alex months ago - something like bugprone 
> > would be good.
>
>
> Agree that "misc" seems pretty overcrowded. I'll defer to those who have been 
> working on clang-tidy longer than me to make this call.
>
> > 2. Also it would be good to make link in cppcoreguidelines.
>
>
> How exactly would I create such a "link"? Are you just thinking of a link in 
> the documentation, or is there a way to have one clang-tidy check activate 
> another (and is this what you're thinking of)?


I am not sure if there is any other mechanism than just links in documentation. 
In the perfect word it would be nice to invoke this check using 
cppcoreguidelines-use-after-move also with some special options like Pedantic=1 
(That would warn about any use after move, even after reinitialization - this 
is what cppcoreguidelines says)



Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:134
@@ +133,3 @@
+/// various internal helper functions).
+class UseAfterMoveFinder {
+public:

What do you think about moving this, and maybe other things to some different 
header file to make it not so scary?


Comment at: clang-tidy/misc/UseAfterMoveCheck.cpp:649-652
@@ +648,6 @@
+FunctionBody = ContainingFunc->getBody();
+  }
+
+  if (!FunctionBody)
+return;
+

you can replace it with 
  else
return;


Comment at: test/clang-tidy/misc-use-after-move.cpp:504-505
@@ +503,4 @@
+std::move(a);
+a = A();
+a.foo();
+  }

I would like to mark it as use after move with some pedantic flag


https://reviews.llvm.org/D23353



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

Re: [PATCH] D23343: [clang-tidy] modernize-make-{smart_ptr} private ctor bugfix

[Piotr Padlewski via cfe-commits]

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

https://reviews.llvm.org/D23343



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

Re: [PATCH] D23343: [clang-tidy] modernize-make-{smart_ptr} private ctor bugfix

[Piotr Padlewski via cfe-commits]

Prazek marked 8 inline comments as done.


Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:35
@@ +34,3 @@
+  auto CanCallCtor = unless(has(ignoringImpCasts(cxxConstructExpr(
+  hasDeclaration(decl(anyOf(isPrivate(), isProtected(;
+

alexfh wrote:
> Prazek wrote:
> > aaron.ballman wrote:
> > > Prazek wrote:
> > > > aaron.ballman wrote:
> > > > > Perhaps: `unless(isPublic())` instead of `anyOf(isPrivate(), 
> > > > > isProtected())`?
> > > > POD types doesn't have public constructors so it will fail :)
> > > Don't they have an implicit one for the purposes of a CXXConstructExpr? 
> > > Looking at an AST dump for:
> > > ```
> > > struct S {
> > >   int i;
> > > };
> > > ```
> > > yields:
> > > ```
> > > |-CXXRecordDecl 0x26d74ae5348  line:25:8 referenced 
> > > struct S definition
> > > | |-CXXRecordDecl 0x26d74ae5460  col:8 implicit struct S
> > > | |-FieldDecl 0x26d74ae7880  col:7 i 'int'
> > > | |-CXXConstructorDecl 0x26d74ae87b8  col:8 implicit used S 
> > > 'void (void) noexcept' inline
> > > | | `-CompoundStmt 0x26d74ae3850 
> > > | |-CXXConstructorDecl 0x26d74ae34a8  col:8 implicit constexpr S 
> > > 'void (const struct S &)' inline noexcept-unevaluated 0x26d74ae34a8
> > > | | `-ParmVarDecl 0x26d74ae35e0  col:8 'const struct S &'
> > > | `-CXXConstructorDecl 0x26d74ae3678  col:8 implicit constexpr S 
> > > 'void (struct S &&)' inline noexcept-unevaluated 0x26d74ae3678
> > > |   `-ParmVarDecl 0x26d74ae37b0  col:8 'struct S &&'
> > > ```
> > what about std::shared_ptr x = std::shared_ptr(new int); ?
> > If I recall correctly, it didn't work on this test.
> There's no `CXXConstructExpr` for primitive types:
> ```
> $ cat /tmp/qq.cc 
> #include 
> 
> struct S { S(); };
> void () {
>   std::shared_ptr p1 = std::shared_ptr(new int);
>   std::shared_ptr p2 = std::shared_ptr(new S);
> }
> $ clang-check -ast-dump -ast-dump-filter= /tmp/qq.cc -- -std=c++11
> Dumping :
> FunctionDecl 0x7f48270fb6e0  line:4:6  'void 
> (void)'
> `-CompoundStmt 0x7f48271582a8 
>   |-DeclStmt 0x7f4827131000 
>   | `-VarDecl 0x7f48270fb9c8  col:24 p1 
> 'std::shared_ptr':'class std::shared_ptr' cinit
>   |   `-ExprWithCleanups 0x7f4827130fe8  
> 'std::shared_ptr':'class std::shared_ptr'
>   | `-CXXConstructExpr 0x7f4827130fb0  
> 'std::shared_ptr':'class std::shared_ptr' 'void (class 
> std::shared_ptr &&) noexcept' elidable
>   |   `-MaterializeTemporaryExpr 0x7f4827130f98  'class 
> std::shared_ptr' xvalue
>   | `-CXXFunctionalCastExpr 0x7f48271303c8  
> 'std::shared_ptr':'class std::shared_ptr' functional cast to 
> std::shared_ptr 
>   |   `-CXXBindTemporaryExpr 0x7f48271303a8  
> 'std::shared_ptr':'class std::shared_ptr' (CXXTemporary 
> 0x7f48271303a0)
>   | `-CXXConstructExpr 0x7f4827130338  
> 'std::shared_ptr':'class std::shared_ptr' 'void (int *)'
>   |   `-CXXNewExpr 0x7f48270fbb58  'int *' 
> Function 0x7f4826a1c000 'operator new' 'void *(std::size_t)'
>   `-DeclStmt 0x7f4827158290 
> `-VarDecl 0x7f4827131238  col:22 p2 
> 'std::shared_ptr':'class std::shared_ptr' cinit
>   `-ExprWithCleanups 0x7f4827158278  
> 'std::shared_ptr':'class std::shared_ptr'
> `-CXXConstructExpr 0x7f4827158240  
> 'std::shared_ptr':'class std::shared_ptr' 'void (class 
> std::shared_ptr &&) noexcept' elidable
>   `-MaterializeTemporaryExpr 0x7f4827158228  'class 
> std::shared_ptr' xvalue
> `-CXXFunctionalCastExpr 0x7f4827157658  
> 'std::shared_ptr':'class std::shared_ptr' functional cast to 
> std::shared_ptr 
>   `-CXXBindTemporaryExpr 0x7f4827157638  
> 'std::shared_ptr':'class std::shared_ptr' (CXXTemporary 
> 0x7f4827157630)
> `-CXXConstructExpr 0x7f48271575c8  
> 'std::shared_ptr':'class std::shared_ptr' 'void (struct S *)'
>   `-CXXNewExpr 0x7f4827131838  'struct S *' 
> Function 0x7f4826a1c000 'operator new' 'void *(std::size_t)'
> `-CXXConstructExpr 0x7f4827131808  'struct S' 
> 'void (void)'
> ```
exactly.


https://reviews.llvm.org/D23343



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

Re: [PATCH] D23343: [clang-tidy] modernize-make-{smart_ptr} private ctor bugfix

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:32-33
@@ -31,1 +31,4 @@
 
+  // Calling make_smart_ptr from within a member function of a type with a
+  // private or protected constructor would be ill-formed.
+  auto CanCallCtor = unless(has(ignoringImpCasts(cxxConstructExpr(

alexfh wrote:
> malcolm.parsons wrote:
> > The private constructor could also be called from a friend class.
> It might only be relevant, if `std::make_(shared|unique)` is declared as a 
> friend of the class in question, which is unlikely to be a common case and I 
> don't think we need to focus on this now.
I am totally aware of it, but I never saw a friend delcaration for any 
class/function from libc++.


https://reviews.llvm.org/D23343



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

Re: [PATCH] D23343: [clang-tidy] modernize-make-{smart_ptr} private ctor bugfix

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:35
@@ +34,3 @@
+  auto CanCallCtor = unless(has(ignoringImpCasts(cxxConstructExpr(
+  hasDeclaration(decl(anyOf(isPrivate(), isProtected(;
+

aaron.ballman wrote:
> Prazek wrote:
> > aaron.ballman wrote:
> > > Perhaps: `unless(isPublic())` instead of `anyOf(isPrivate(), 
> > > isProtected())`?
> > POD types doesn't have public constructors so it will fail :)
> Don't they have an implicit one for the purposes of a CXXConstructExpr? 
> Looking at an AST dump for:
> ```
> struct S {
>   int i;
> };
> ```
> yields:
> ```
> |-CXXRecordDecl 0x26d74ae5348  line:25:8 referenced 
> struct S definition
> | |-CXXRecordDecl 0x26d74ae5460  col:8 implicit struct S
> | |-FieldDecl 0x26d74ae7880  col:7 i 'int'
> | |-CXXConstructorDecl 0x26d74ae87b8  col:8 implicit used S 'void 
> (void) noexcept' inline
> | | `-CompoundStmt 0x26d74ae3850 
> | |-CXXConstructorDecl 0x26d74ae34a8  col:8 implicit constexpr S 'void 
> (const struct S &)' inline noexcept-unevaluated 0x26d74ae34a8
> | | `-ParmVarDecl 0x26d74ae35e0  col:8 'const struct S &'
> | `-CXXConstructorDecl 0x26d74ae3678  col:8 implicit constexpr S 'void 
> (struct S &&)' inline noexcept-unevaluated 0x26d74ae3678
> |   `-ParmVarDecl 0x26d74ae37b0  col:8 'struct S &&'
> ```
what about std::shared_ptr x = std::shared_ptr(new int); ?
If I recall correctly, it didn't work on this test.


https://reviews.llvm.org/D23343



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

Re: [PATCH] D23343: [clang-tidy] modernize-make-{smart_ptr} private ctor bugfix

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:35
@@ +34,3 @@
+  auto CanCallCtor = unless(has(ignoringImpCasts(cxxConstructExpr(
+  hasDeclaration(decl(anyOf(isPrivate(), isProtected(;
+

aaron.ballman wrote:
> Perhaps: `unless(isPublic())` instead of `anyOf(isPrivate(), isProtected())`?
POD types doesn't have public constructors so it will fail :)


https://reviews.llvm.org/D23343



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

Re: [PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:22-23
@@ +21,4 @@
+  Finder->addMatcher(
+  declRefExpr(hasDeclaration(functionDecl(namedDecl(hasName("::rand")),
+  parameterCountIs(0
+  .bind("randomGenerator"),

aaron.ballman wrote:
> Prazek wrote:
> > aaron.ballman wrote:
> > > This should be looking at a callExpr() rather than a declRefExpr(), 
> > > should it not?
> > I was also thinking about this, but this is actually better, because it 
> > will also match with binding rand with function pointer.
> True, but a DeclRefExpr doesn't mean it's a function call. Binding the 
> function is not contrary to the CERT rule, just calling it. For instance, the 
> following pathological case will be caught by this check:
> ```
> if (std::rand) {}
> ```
> The behavior of this check should be consistent with cert-env33-c, which only 
> looks at calls. (If we really care about bound functions, we'd need flow 
> control analysis, and I think that's overkill for both of those checks, but 
> wouldn't be opposed to someone writing the flow analysis if they really 
> wanted to.)
It would indeed fire on this pathological case, but I don't think we should 
care about cases like this, because no one is writing code like this (and if he 
would then it would probably be a bug).
I don't think that there is much code that binds pointer to std::rand either, 
but I think it would be good to display warning for this, because even if the 
function would be never called, then it means that this is a bug, and if it 
would be called then it would be nice to tell user that rand might be used here.

Anyway I don't oppose for changing it to callExpr, but I think it is better 
this way.


Repository:
  rL LLVM

https://reviews.llvm.org/D22346



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

Re: [PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:22-23
@@ +21,4 @@
+  Finder->addMatcher(
+  declRefExpr(hasDeclaration(functionDecl(namedDecl(hasName("::rand")),
+  parameterCountIs(0
+  .bind("randomGenerator"),

aaron.ballman wrote:
> This should be looking at a callExpr() rather than a declRefExpr(), should it 
> not?
I was also thinking about this, but this is actually better, because it will 
also match with binding rand with function pointer.


Repository:
  rL LLVM

https://reviews.llvm.org/D22346



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

Re: [PATCH] D22346: [Clang-tidy] CERT-MSC50-CPP (std:rand() )

[Piotr Padlewski via cfe-commits]

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

LGTM with the fixes of docs.



Comment at: clang-tidy/cert/LimitedRandomnessCheck.cpp:31
@@ +30,3 @@
+  Result.Nodes.getNodeAs("randomGenerator");
+  diag(MatchedDecl->getLocation(), "rand() function has limited randomness, "
+   "use C++11 random library instead");

I am not sure about the diagnostics convention, it is possible that you should 
replace comma with semicolon.


Repository:
  rL LLVM

https://reviews.llvm.org/D22346



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

Re: [PATCH] D23343: [clang-tidy] modernize-make-{smart_ptr} private ctor bugfix

[Piotr Padlewski via cfe-commits]

Prazek updated this revision to Diff 67967.
Prazek added a comment.

- fixes


https://reviews.llvm.org/D23343

Files:
  clang-tidy/modernize/MakeSmartPtrCheck.cpp
  docs/ReleaseNotes.rst
  test/clang-tidy/modernize-make-shared.cpp
  test/clang-tidy/modernize-make-unique.cpp

Index: test/clang-tidy/modernize-make-unique.cpp
===
--- test/clang-tidy/modernize-make-unique.cpp
+++ test/clang-tidy/modernize-make-unique.cpp
@@ -103,6 +103,38 @@
   std::unique_ptr Placement = std::unique_ptr(new (PInt) int{3});
 }
 
+// Calling make_smart_ptr from within a member function of a type with a
+// private or protected constructor would be ill-formed.
+class Private {
+private:
+  Private(int z) {}
+
+public:
+  Private() {}
+  void create() {
+auto callsPublic = std::unique_ptr(new Private);
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_unique instead
+// CHECK-FIXES: auto callsPublic = std::make_unique();
+auto ptr = std::unique_ptr(new Private(42));
+  }
+
+  virtual ~Private();
+};
+
+class Protected {
+protected:
+  Protected() {}
+
+public:
+  Protected(int, int) {}
+  void create() {
+auto callsPublic = std::unique_ptr(new Protected(1, 2));
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_unique instead
+// CHECK-FIXES: auto callsPublic = std::make_unique(1, 2);
+auto ptr = std::unique_ptr(new Protected);
+  }
+};
+
 void initialization(int T, Base b) {
   // Test different kinds of initialization of the pointee.
 
Index: test/clang-tidy/modernize-make-shared.cpp
===
--- test/clang-tidy/modernize-make-shared.cpp
+++ test/clang-tidy/modernize-make-shared.cpp
@@ -100,6 +100,38 @@
   std::shared_ptr Placement = std::shared_ptr(new (PInt) int{3});
 }
 
+// Calling make_smart_ptr from within a member function of a type with a
+// private or protected constructor would be ill-formed.
+class Private {
+private:
+  Private(int z) {}
+
+public:
+  Private() {}
+  void create() {
+auto callsPublic = std::shared_ptr(new Private);
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_shared instead
+// CHECK-FIXES: auto callsPublic = std::make_shared();
+auto ptr = std::shared_ptr(new Private(42));
+  }
+
+  virtual ~Private();
+};
+
+class Protected {
+protected:
+  Protected() {}
+
+public:
+  Protected(int, int) {}
+  void create() {
+auto callsPublic = std::shared_ptr(new Protected(1, 2));
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_shared instead
+// CHECK-FIXES: auto callsPublic = std::make_shared(1, 2);
+auto ptr = std::shared_ptr(new Protected);
+  }
+};
+
 void initialization(int T, Base b) {
   // Test different kinds of initialization of the pointee.
 
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -85,6 +85,15 @@
   Warns about the performance overhead arising from concatenating strings using
   the ``operator+``, instead of ``operator+=``.
 
+
+Fixed bugs:
+- `modernize-make-unique
+  `_
+  and `modernize-make-shared
+  `_
+  Calling ``make_{unique|shared}`` from within a member function of a type
+  with a private or protected constructor would be ill-formed.
+
 Improvements to include-fixer
 -
 
Index: clang-tidy/modernize/MakeSmartPtrCheck.cpp
===
--- clang-tidy/modernize/MakeSmartPtrCheck.cpp
+++ clang-tidy/modernize/MakeSmartPtrCheck.cpp
@@ -29,13 +29,19 @@
   if (!getLangOpts().CPlusPlus11)
 return;
 
+  // Calling make_smart_ptr from within a member function of a type with a
+  // private or protected constructor would be ill-formed.
+  auto CanCallCtor = unless(has(ignoringImpCasts(cxxConstructExpr(
+  hasDeclaration(decl(anyOf(isPrivate(), isProtected(;
+
   Finder->addMatcher(
   cxxBindTemporaryExpr(has(ignoringParenImpCasts(
   cxxConstructExpr(
   hasType(getSmartPointerTypeMatcher()), argumentCountIs(1),
   hasArgument(0,
   cxxNewExpr(hasType(pointsTo(qualType(hasCanonicalType(
- equalsBoundNode(PointerType))
+ equalsBoundNode(PointerType),
+ CanCallCtor)
   .bind(NewExpression)))
   .bind(ConstructorCall,
   this);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23343: [clang-tidy] modernize-make-{smart_ptr} private ctor bugfix

[Piotr Padlewski via cfe-commits]

Prazek updated this revision to Diff 67966.
Prazek marked 6 inline comments as done.
Prazek added a comment.

- fixes


https://reviews.llvm.org/D23343

Files:
  clang-tidy/modernize/MakeSmartPtrCheck.cpp
  docs/ReleaseNotes.rst
  test/clang-tidy/modernize-make-shared.cpp
  test/clang-tidy/modernize-make-unique.cpp

Index: test/clang-tidy/modernize-make-unique.cpp
===
--- test/clang-tidy/modernize-make-unique.cpp
+++ test/clang-tidy/modernize-make-unique.cpp
@@ -103,6 +103,36 @@
   std::unique_ptr Placement = std::unique_ptr(new (PInt) int{3});
 }
 
+// Calling make_smart_ptr from within a member function of a type with a
+// private or protected constructor would be ill-formed.
+class Private {
+private:
+  Private(int z) {}
+public:
+  Private() {}
+  void create() {
+auto callsPublic = std::unique_ptr(new Private);
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_unique instead
+// CHECK-FIXES: auto callsPublic = std::make_unique();
+auto ptr = std::unique_ptr(new Private(42));
+  }
+
+  virtual ~Private();
+};
+
+class Protected {
+protected:
+  Protected() {}
+public:
+  Protected(int, int) {}
+  void create() {
+auto callsPublic = std::unique_ptr(new Protected(1, 2));
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_unique instead
+// CHECK-FIXES: auto callsPublic = std::make_unique(1, 2);
+auto ptr = std::unique_ptr(new Protected);
+  }
+};
+
 void initialization(int T, Base b) {
   // Test different kinds of initialization of the pointee.
 
Index: test/clang-tidy/modernize-make-shared.cpp
===
--- test/clang-tidy/modernize-make-shared.cpp
+++ test/clang-tidy/modernize-make-shared.cpp
@@ -100,6 +100,36 @@
   std::shared_ptr Placement = std::shared_ptr(new (PInt) int{3});
 }
 
+// Calling make_smart_ptr from within a member function of a type with a
+// private or protected constructor would be ill-formed.
+class Private {
+private:
+  Private(int z) {}
+public:
+  Private() {}
+  void create() {
+auto callsPublic = std::shared_ptr(new Private);
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_shared instead
+// CHECK-FIXES: auto callsPublic = std::make_shared();
+auto ptr = std::shared_ptr(new Private(42));
+  }
+
+  virtual ~Private();
+};
+
+class Protected {
+protected:
+  Protected() {}
+public:
+  Protected(int, int) {}
+  void create() {
+auto callsPublic = std::shared_ptr(new Protected(1, 2));
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use std::make_shared instead
+// CHECK-FIXES: auto callsPublic = std::make_shared(1, 2);
+auto ptr = std::shared_ptr(new Protected);
+  }
+};
+
 void initialization(int T, Base b) {
   // Test different kinds of initialization of the pointee.
 
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -85,6 +85,15 @@
   Warns about the performance overhead arising from concatenating strings using
   the ``operator+``, instead of ``operator+=``.
 
+
+Fixed bugs:
+- `modernize-make-unique
+  `_
+  and `modernize-make-shared
+  `_
+  Calling ``make_{unique|shared}`` from within a member function of a type
+  with a private or protected constructor would be ill-formed.
+
 Improvements to include-fixer
 -
 
Index: clang-tidy/modernize/MakeSmartPtrCheck.cpp
===
--- clang-tidy/modernize/MakeSmartPtrCheck.cpp
+++ clang-tidy/modernize/MakeSmartPtrCheck.cpp
@@ -29,13 +29,21 @@
   if (!getLangOpts().CPlusPlus11)
 return;
 
+  // Calling make_smart_ptr from within a member function of a type with a
+  // private or protected constructor would be ill-formed.
+  auto CanCallCtor = unless(has(
+  ignoringImpCasts(cxxConstructExpr(hasDeclaration(decl(anyOf(isPrivate()
+, isProtected(
+  ;
+
   Finder->addMatcher(
   cxxBindTemporaryExpr(has(ignoringParenImpCasts(
   cxxConstructExpr(
   hasType(getSmartPointerTypeMatcher()), argumentCountIs(1),
   hasArgument(0,
   cxxNewExpr(hasType(pointsTo(qualType(hasCanonicalType(
- equalsBoundNode(PointerType))
+ equalsBoundNode(PointerType),
+ CanCallCtor)
   .bind(NewExpression)))
   .bind(ConstructorCall,
   this);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15332: new clang-tidy checker readability-non-const-parameter

[Piotr Padlewski via cfe-commits]

Prazek added a subscriber: Prazek.


Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:95-98
@@ +94,6 @@
+const QualType T = VD->getType();
+if (T->isPointerType() && !T->getPointeeType().isConstQualified())
+  markCanNotBeConst(VD->getInit(), true);
+else if (T->isArrayType())
+  markCanNotBeConst(VD->getInit(), true);
+  }

This looks like it could be in the same if.


https://reviews.llvm.org/D15332



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

Re: [PATCH] D23343: [clang-tidy] modernize-make-{smart_ptr} private ctor bugfix

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: test/clang-tidy/modernize-make-shared.cpp:109
@@ +108,3 @@
+  void create() {
+auto ptr = std::shared_ptr(new Private(42));
+  }

aaron.ballman wrote:
> Add comments explaining why make_shared is not correct. Also, please add a 
> case showing that protected constructors still get the proper fix applied.
You mean I should also not warn when the constructor is protected? Make sense.


https://reviews.llvm.org/D23343



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

Re: [PATCH] D23353: [clang-tidy] Add check 'misc-use-after-move'

[Piotr Padlewski via cfe-commits]

Prazek added a subscriber: Prazek.
Prazek added a reviewer: Prazek.
Prazek added a comment.

I will review it later, but my first thoughts:

1. I think we should make some other group, because misc seems to be 
overloaded. I discussed it with Alex months ago - something like bugprone would 
be good.
2. Also it would be good to make link in cppcoreguidelines.


https://reviews.llvm.org/D23353



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

Re: [PATCH] D23343: [clang-tidy] modernize-make-{smart_ptr} private ctor bugfix

[Piotr Padlewski via cfe-commits]

Prazek updated this revision to Diff 67460.
Prazek added a comment.

I hate it when arc do this thing. When origin/master is not the trunk...


https://reviews.llvm.org/D23343

Files:
  clang-tidy/modernize/MakeSmartPtrCheck.cpp
  docs/ReleaseNotes.rst
  test/clang-tidy/modernize-make-shared.cpp
  test/clang-tidy/modernize-make-unique.cpp

Index: test/clang-tidy/modernize-make-unique.cpp
===
--- test/clang-tidy/modernize-make-unique.cpp
+++ test/clang-tidy/modernize-make-unique.cpp
@@ -103,6 +103,16 @@
   std::unique_ptr Placement = std::unique_ptr(new (PInt) int{3});
 }
 
+class Private {
+private:
+  Private(int z) {}
+
+public:
+  void create() {
+auto ptr = std::unique_ptr(new Private(42));
+  }
+};
+
 void initialization(int T, Base b) {
   // Test different kinds of initialization of the pointee.
 
Index: test/clang-tidy/modernize-make-shared.cpp
===
--- test/clang-tidy/modernize-make-shared.cpp
+++ test/clang-tidy/modernize-make-shared.cpp
@@ -100,6 +100,18 @@
   std::shared_ptr Placement = std::shared_ptr(new (PInt) int{3});
 }
 
+class Private {
+private:
+  Private(int z) {}
+
+public:
+  void create() {
+auto ptr = std::shared_ptr(new Private(42));
+  }
+
+  ~Private();
+};
+
 void initialization(int T, Base b) {
   // Test different kinds of initialization of the pointee.
 
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,13 @@
   This check warns about the performance overhead arising from concatenating
   strings using the ``operator+``, instead of ``operator+=``.
 
+- Bugfix for `modernize-make-unique
+  `_
+  and `modernize-make-shared
+  `_
+  checks
+  It is invalid to use ``make_{smart_ptr}`` when the called constructor is
+  private.
 
 Improvements to include-fixer
 -
Index: clang-tidy/modernize/MakeSmartPtrCheck.cpp
===
--- clang-tidy/modernize/MakeSmartPtrCheck.cpp
+++ clang-tidy/modernize/MakeSmartPtrCheck.cpp
@@ -29,13 +29,19 @@
   if (!getLangOpts().CPlusPlus11)
 return;
 
+  // It is possible to make smart ptr calling private ctor inside of a member
+  // function. Change to make_smart_ptr will be invalid.
+  auto CallsPrivateCtor = has(
+  ignoringImpCasts(cxxConstructExpr(hasDeclaration(decl(isPrivate());
+
   Finder->addMatcher(
   cxxBindTemporaryExpr(has(ignoringParenImpCasts(
   cxxConstructExpr(
   hasType(getSmartPointerTypeMatcher()), argumentCountIs(1),
   hasArgument(0,
   
cxxNewExpr(hasType(pointsTo(qualType(hasCanonicalType(
- equalsBoundNode(PointerType))
+ equalsBoundNode(PointerType),
+ unless(CallsPrivateCtor))
   .bind(NewExpression)))
   .bind(ConstructorCall,
   this);


Index: test/clang-tidy/modernize-make-unique.cpp
===
--- test/clang-tidy/modernize-make-unique.cpp
+++ test/clang-tidy/modernize-make-unique.cpp
@@ -103,6 +103,16 @@
   std::unique_ptr Placement = std::unique_ptr(new (PInt) int{3});
 }
 
+class Private {
+private:
+  Private(int z) {}
+
+public:
+  void create() {
+auto ptr = std::unique_ptr(new Private(42));
+  }
+};
+
 void initialization(int T, Base b) {
   // Test different kinds of initialization of the pointee.
 
Index: test/clang-tidy/modernize-make-shared.cpp
===
--- test/clang-tidy/modernize-make-shared.cpp
+++ test/clang-tidy/modernize-make-shared.cpp
@@ -100,6 +100,18 @@
   std::shared_ptr Placement = std::shared_ptr(new (PInt) int{3});
 }
 
+class Private {
+private:
+  Private(int z) {}
+
+public:
+  void create() {
+auto ptr = std::shared_ptr(new Private(42));
+  }
+
+  ~Private();
+};
+
 void initialization(int T, Base b) {
   // Test different kinds of initialization of the pointee.
 
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,13 @@
   This check warns about the performance overhead arising from concatenating
   strings using the ``operator+``, instead of ``operator+=``.
 
+- Bugfix for `modernize-make-unique
+  `_
+  and `modernize-make-shared
+  `_
+  checks
+  It is invalid to use ``make_{smart_ptr}`` when 

[PATCH] D23343: [clang-tidy] modernize-make-{smart_ptr} private ctor bugfix

[Piotr Padlewski via cfe-commits]

Prazek created this revision.
Prazek added reviewers: alexfh, aaron.ballman, hokein.
Prazek added a subscriber: cfe-commits.

Bugfix for 27321. When the constructor of stored pointer
type is private then it is invalid to change it to
make_shared or make_unique.

https://reviews.llvm.org/D23343

Files:
  clang-tidy/modernize/MakeSmartPtrCheck.cpp
  test/clang-tidy/modernize-make-shared.cpp
  test/clang-tidy/modernize-make-unique.cpp

Index: test/clang-tidy/modernize-make-unique.cpp
===
--- test/clang-tidy/modernize-make-unique.cpp
+++ test/clang-tidy/modernize-make-unique.cpp
@@ -103,6 +103,17 @@
   std::unique_ptr Placement = std::unique_ptr(new (PInt) int{3});
 }
 
+class Private {
+private:
+  Private(int z) {}
+
+public:
+  void create() {
+auto ptr = std::unique_ptr(new Private(42));
+  }
+};
+
+
 void initialization(int T, Base b) {
   // Test different kinds of initialization of the pointee.
 
Index: test/clang-tidy/modernize-make-shared.cpp
===
--- test/clang-tidy/modernize-make-shared.cpp
+++ test/clang-tidy/modernize-make-shared.cpp
@@ -100,6 +100,18 @@
   std::shared_ptr Placement = std::shared_ptr(new (PInt) int{3});
 }
 
+class Private {
+private:
+  Private(int z) {}
+
+public:
+  void create() {
+auto ptr = std::shared_ptr(new Private(42));
+  }
+
+  ~Private();
+};
+
 void initialization(int T, Base b) {
   // Test different kinds of initialization of the pointee.
 
Index: clang-tidy/modernize/MakeSmartPtrCheck.cpp
===
--- clang-tidy/modernize/MakeSmartPtrCheck.cpp
+++ clang-tidy/modernize/MakeSmartPtrCheck.cpp
@@ -29,13 +29,19 @@
   if (!getLangOpts().CPlusPlus11)
 return;
 
+  // It is possible to make smart ptr calling private ctor inside of a member
+  // function. Change to make_smart_ptr will be invalid.
+  auto CallsPrivateCtor = has(ignoringImpCasts(cxxConstructExpr(
+hasDeclaration(decl(isPrivate());
+
   Finder->addMatcher(
   cxxBindTemporaryExpr(has(ignoringParenImpCasts(
   cxxConstructExpr(
   hasType(getSmartPointerTypeMatcher()), argumentCountIs(1),
   hasArgument(0,
   
cxxNewExpr(hasType(pointsTo(qualType(hasCanonicalType(
- equalsBoundNode(PointerType))
+ equalsBoundNode(PointerType),
+ unless(CallsPrivateCtor))
   .bind(NewExpression)))
   .bind(ConstructorCall,
   this);


Index: test/clang-tidy/modernize-make-unique.cpp
===
--- test/clang-tidy/modernize-make-unique.cpp
+++ test/clang-tidy/modernize-make-unique.cpp
@@ -103,6 +103,17 @@
   std::unique_ptr Placement = std::unique_ptr(new (PInt) int{3});
 }
 
+class Private {
+private:
+  Private(int z) {}
+
+public:
+  void create() {
+auto ptr = std::unique_ptr(new Private(42));
+  }
+};
+
+
 void initialization(int T, Base b) {
   // Test different kinds of initialization of the pointee.
 
Index: test/clang-tidy/modernize-make-shared.cpp
===
--- test/clang-tidy/modernize-make-shared.cpp
+++ test/clang-tidy/modernize-make-shared.cpp
@@ -100,6 +100,18 @@
   std::shared_ptr Placement = std::shared_ptr(new (PInt) int{3});
 }
 
+class Private {
+private:
+  Private(int z) {}
+
+public:
+  void create() {
+auto ptr = std::shared_ptr(new Private(42));
+  }
+
+  ~Private();
+};
+
 void initialization(int T, Base b) {
   // Test different kinds of initialization of the pointee.
 
Index: clang-tidy/modernize/MakeSmartPtrCheck.cpp
===
--- clang-tidy/modernize/MakeSmartPtrCheck.cpp
+++ clang-tidy/modernize/MakeSmartPtrCheck.cpp
@@ -29,13 +29,19 @@
   if (!getLangOpts().CPlusPlus11)
 return;
 
+  // It is possible to make smart ptr calling private ctor inside of a member
+  // function. Change to make_smart_ptr will be invalid.
+  auto CallsPrivateCtor = has(ignoringImpCasts(cxxConstructExpr(
+hasDeclaration(decl(isPrivate());
+
   Finder->addMatcher(
   cxxBindTemporaryExpr(has(ignoringParenImpCasts(
   cxxConstructExpr(
   hasType(getSmartPointerTypeMatcher()), argumentCountIs(1),
   hasArgument(0,
   cxxNewExpr(hasType(pointsTo(qualType(hasCanonicalType(
- equalsBoundNode(PointerType))
+ equalsBoundNode(PointerType),
+ unless(CallsPrivateCtor))
   .bind(NewExpression)))
   .bind(ConstructorCall,
   this);
___

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Piotr Padlewski via cfe-commits]

2016-08-09 5:49 GMT-07:00 Aaron Ballman :
>
> I think this boils down to personal preference, which is why I'm
> concerned about the check. Either mechanism is correct, so this is
> purely a stylistic check in many regards.
>
> About warnings - well, if someone choose this check to be run, then he
> > probably wants to get warnings instead of notes.
>
> The problem is that people don't always choose this check to be run,
> they choose to run clang-tidy and this check is enabled by default. Or
> they choose to run modernize and this check is enabled by default.
>
> As with most checks. We can either be "perfect" and add only checks that
we consider very useful, or we can be open
and let people choose checks that they want from bigger set. After I used
clang-tidy first time, I know what checks are not something that I want, so
I just didn't use it.
I think we can work out on other solution. Maybe we should make some levels
of recomendation, so the user would use some option like --level=recommend
or something,
and we would have to somehow split the checks into groups. This would be
still problematic, but I know that it would be nice to have analyzer-alpha
checks into not recommend mode
(I don't know why they are used on default. They have so many bugs).

Anyway as you can see the matter of usefulness and very subjective. I can
argue that this is should belong to modernize, because I think that this is
modernization (changing code from
bugprone C ways to slightly better C++ ways), but of course everyone will
have own opinion.


Piotr

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

Re: [PATCH] D23257: Fix clang-tidy crash when a single fix is applied on multiple files.

[Piotr Padlewski via cfe-commits]

Prazek added a subscriber: Prazek.
Prazek added a comment.

I remember that it was pissing me off when I used clang-tidy first time. Thanks 
for fixing that!


https://reviews.llvm.org/D23257



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

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Piotr Padlewski via cfe-commits]

2016-08-08 8:33 GMT-07:00 Aaron Ballman :

> aaron.ballman added inline comments.
>
> 
> Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:59-61
> @@ +58,5 @@
> +  IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
> +  Options.get("IncludeStyle", "llvm"))) {
> +
> +  for (const auto  :
> +   {std::make_pair("memcpy", "std::copy"), {"memset", "std::fill"}}) {
> 
> alexfh wrote:
> > I'm not sure this works on MSVC2013. Could someone try this before
> submitting?
> It does not work in MSVC 2013.
>
> What is the best way to fix it and still have a nice code?


> 
> Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:102
> @@ +101,3 @@
> +  assert(it != Replacements.end() &&
> + "Replacements list does not match list of registered matcher
> names");
> +  const std::string ReplacedName = it->second;
> 
> alexfh wrote:
> > Notes are treated completely differently - each note has to be attached
> to a warning.
> >
> > Clang-tidy can only deal with two severity levels of diagnostics:
> "warning" and "error", but it's better to let them be controlled by the
> user via `-warnings-as-errors=` command-line option or the
> `WarningsAsErrors` configuration file option.
> Drat. I am not keen on this being a warning (let alone an error) because
> it really doesn't warn the user against anything bad (the type safety
> argument is tenuous at best), and it's arguable whether this actually
> modernizes code. Do you think there's sufficient utility to this check to
> warrant it being default-enabled as part of the modernize suite? For
> instance, we have 368 instances of memcpy() and 171 instances of
> std::copy() in the LLVM code base, which is an example of a very modern C++
> code base. I'm not opposed to the check, just worried that it will drown
> out diagnostics that have more impact to the user.
>
> I consider memcpy and memset very bugprone. It is very easy to swap
arguments by mistake or pass something with a different type etc. Also it
is easier to understand fill than a memset, so it is easier for
C++ programmers who see any of it first time to get it.
If I would see someone using memcpy or memset in C++ code on the review,
asking to change for the one from the algorithm would be my first comment.

About warnings - well, if someone choose this check to be run, then he
probably wants to get warnings instead of notes. I understand your point,
that maybe we should use something lighter for the "light" mistakes, but
the user is the one that feels if something is bad or not, and he just
choose the check that he thinks it is resonable to run.
I would like to see some proposal how you exactly you would imagine good
warnings, but I don't think we should discuss it in this review. We can
change it after it will be in the trunk.

Also, could you respond in the phabricator? I am not sure how does it work,
but sometimes responding in a mail works fine and there is a comment in
phab, but many times it doesn't appear there.

Piotr


> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D22725
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r277923 - [ASTReader] Use real move semantics instead of emulating them in the copy ctor.

[Piotr Padlewski via cfe-commits]

2016-08-06 5:45 GMT-07:00 Benjamin Kramer via cfe-commits <
cfe-commits@lists.llvm.org>:

> Author: d0k
> Date: Sat Aug  6 07:45:16 2016
> New Revision: 277923
>
> URL: http://llvm.org/viewvc/llvm-project?rev=277923=rev
> Log:
> [ASTReader] Use real move semantics instead of emulating them in the copy
> ctor.
>
> No functionality change intended.
>
> Modified:
> cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
>
> Modified: cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/
> Serialization/ASTReaderDecl.cpp?rev=277923=277922=277923=diff
> 
> ==
> --- cfe/trunk/lib/Serialization/ASTReaderDecl.cpp (original)
> +++ cfe/trunk/lib/Serialization/ASTReaderDecl.cpp Sat Aug  6 07:45:16 2016
> @@ -170,12 +170,12 @@ namespace clang {
>ASTReader 
>NamedDecl *New;
>NamedDecl *Existing;
> -  mutable bool AddResult;
> +  bool AddResult;
>
>unsigned AnonymousDeclNumber;
>IdentifierInfo *TypedefNameForLinkage;
>
> -  void operator=(FindExistingResult&) = delete;
> +  void operator=(FindExistingResult &&) = delete;
>
>  public:
>FindExistingResult(ASTReader )
> @@ -189,7 +189,7 @@ namespace clang {
>  AnonymousDeclNumber(AnonymousDeclNumber),
>  TypedefNameForLinkage(TypedefNameForLinkage) {}
>
> -  FindExistingResult(const FindExistingResult )
> +  FindExistingResult(FindExistingResult &)
>: Reader(Other.Reader), New(Other.New),
> Existing(Other.Existing),
>  AddResult(Other.AddResult),
>  AnonymousDeclNumber(Other.AnonymousDeclNumber),
>
> Shouldn't these lines have std::move() everywhere to make them real move
ctors?



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

Re: [PATCH] D23243: [clang-tidy] enhance modernize-use-bool-literals to check ternary operator

[Piotr Padlewski via cfe-commits]

2016-08-07 15:38 GMT-07:00 Aaron Ballman :

On Sun, Aug 7, 2016 at 6:33 PM, Piotr Padlewski
>  wrote:
> > Prazek added a comment.
> >
> > Yea, I also have never heard of it. I don't think it is worth even
> discussing
>
Just because you've never heard of a compiler extension that gets
> pointed out during review does not mean it's not worth discussing. Not
> everyone has heard of every compiler vendor extension we support,
> that's why we do public review in a wide audience. :-) Again, I am not
> suggesting the OP do this as part of this patch.
>
> GCC has many extensions. My point is that I also don't know any user that
use it.
Of course it doesn't imply that no one is using it, but I strongly belive
that
There are so few user in the word that I think it is much better to spend
time doing something other useful in clang-tidy than to even discuss this
topic.
It is good that you have mentioned it, at least I learned something new :)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23243: [clang-tidy] enhance modernize-use-bool-literals to check ternary operator

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

Yea, I also have never heard of it. I don't think it is worth even discussing


https://reviews.llvm.org/D23243



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

Re: [PATCH] D23243: [clang-tidy] enhance modernize-use-bool-literals to check ternary operator

[Piotr Padlewski via cfe-commits]

Prazek accepted this revision.
Prazek added a reviewer: Prazek.
Prazek added a comment.
This revision is now accepted and ready to land.

LGTM, but wait a day, maybe someone will have other comments.


https://reviews.llvm.org/D23243



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

Re: [PATCH] D23243: [clang-tidy] enhance modernize-use-bool-literals to check ternary operator

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: clang-tidy/modernize/UseBoolLiteralsCheck.cpp:38-43
@@ +37,8 @@
+  unless(isInTemplateInstantiation(,
+  anyOf(hasTrueExpression(ignoringParenImpCasts(
+integerLiteral().bind("trueBranchLiteral"))),
+anything()),
+  anyOf(hasFalseExpression(ignoringParenImpCasts(
+integerLiteral().bind("falseBranchLiteral"))),
+anything())),
+  this);

Do I understand it correctly, that you have to have this 2 anyOf matchers, 
because if you would have it in one then because anyOf is lazy, it would not 
bind to the second branch?

I think it would be good to write some comment about it, because on the first 
sight it looks like it could be simplified.


Comment at: test/clang-tidy/modernize-use-bool-literals.cpp:119
@@ -118,1 +118,3 @@
 }
+
+static int Value = 1;

Please add a test with only one branch with constant, and none of them.
Also one test like
bool Result = Value == 1 ? true : 0;
or
bool Result = Value == 1 ? false : bool(0);


Repository:
  rL LLVM

https://reviews.llvm.org/D23243



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

Re: [PATCH] D23130: [Clang-tidy] Add a check for definitions in the global namespace.

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: clang-tidy/misc/GlobalNamespaceCheck.cpp:46
@@ +45,3 @@
+// extern "C" globals need to be in the global namespace.
+if (VDecl->isExternC())
+  return;

I think it would be better to check it in matcher.
I see that there is isExternC, but it only works for FunctionDecl, but I think 
that adding isExternC for VarDecl would be good


Comment at: clang-tidy/misc/GlobalNamespaceCheck.cpp:67
@@ +66,3 @@
+  }
+
+  // FIXME: If we want to be really fancy we could move declaration-less

What about macros? I think you should check isMacroId on location here (don't 
do fixit when it is in macro, but print warning)

Also please add tests for it.


Repository:
  rL LLVM

https://reviews.llvm.org/D23130



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

Re: [PATCH] D23135: [clang-tidy] misc-argument-comment non-strict mode

[Piotr Padlewski via cfe-commits]

Prazek added a subscriber: Prazek.
Prazek added a comment.

LG(TM)


https://reviews.llvm.org/D23135



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

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

In https://reviews.llvm.org/D22725#505020, @JDevlieghere wrote:

> Addresses comments from Aaron Ballman
>
> @aaron.ballman Thanks for the thorough review! Can you check whether the 
> tests I added address your concerns? Could you also elaborate on the case 
> with the C-function pointer? Unless I explicitly cast it to void* the 
> compiler rejects will reject it as an argument to memcpy. Am I missing a case 
> where this could go wrong? I still added it to the pointer arithmetic check 
> though, just to be sure.


Did you manage to see what was wrong in the transformation that you did on LLVM?

I have one idea, which should be fixed. Memset takes char as an argument, so if 
you would fill the array of ints with memset(arr, 0, sizeof(arr)), then it will 
set all
the elements to zero. But if you would do it with with set say 1, you will end 
up having

  int val = 1 | (1 << 8) | (1 << 16) | (1 << 24)

for each elemnt of the array.

I don't think tho that any LLVM code depend od this, you usually set everything 
to zero, or to another value if you operate on char type.

So allow transformation and diagnostic if the argument equals 0, or when the 
type is char.


Repository:
  rL LLVM

https://reviews.llvm.org/D22725



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

Re: [PATCH] D20196: [clang-tidy] Inefficient string operation

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

In https://reviews.llvm.org/D20196#504397, @bittnerbarni wrote:

> I'm planning to submit more patches in the future, as I have time for them. 
> So it wouldn't be in vain :)


http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access


https://reviews.llvm.org/D20196



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

Re: [PATCH] D20196: [clang-tidy] Inefficient string operation

[Piotr Padlewski via cfe-commits]

Prazek added a subscriber: Prazek.
Prazek added a comment.

In https://reviews.llvm.org/D20196#504394, @bittnerbarni wrote:

> Thank you for all the assistance. Could you please do that?


btw obtaining commit access and commiting is very simple, so if you are 
planning to send us some more cool patches then I think you should get the 
commit access :)


https://reviews.llvm.org/D20196



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

Re: [PATCH] D23008: [clang-tidy] fix segfault in cppcore-guidelines-special-member-functions check

[Piotr Padlewski via cfe-commits]

Prazek added a subscriber: Prazek.
Prazek accepted this revision.
Prazek added a reviewer: Prazek.
Prazek added a comment.
This revision is now accepted and ready to land.

LGTM


https://reviews.llvm.org/D23008



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

Re: [PATCH] D22208: [clang-tidy] Fixes to modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek marked 5 inline comments as done.


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:115
@@ -95,1 +114,3 @@
+  auto CtorCallSourceRange = CharSourceRange::getTokenRange(
+  InnerCtorCall->getExprLoc(), CallParensRange.getBegin());
 

alexfh wrote:
> Prazek wrote:
> > alexfh wrote:
> > > Prazek wrote:
> > > > alexfh wrote:
> > > > > This doesn't seem to be an issue, since expression 
> > > > > `v.push_back(obj.member())` won't trigger this check: it expects that 
> > > > > the argument of the `push_back` call is a `cxxConstructExpr` or a 
> > > > > `cxxFunctionalCastExpr`.
> > > > what about the implicit conversion? What if obj.member would return 
> > > > object that is different from the one that v stores, but it is 
> > > > convertible to it?
> > > Sounds almost like a recipe for a test case ;) Have you tried to 
> > > construct it this way?
> > Yes, I think I emulated this case in line 190. I tried to hit this problem 
> > but could not find the test case.
> >> if The Call->getArg(0) will be a memberExpr (call to member function)
> > what about the implicit conversion?
> 
> I think, I know what happens in this case. When an implicit conversion 
> happens in `v.push_back(obj.member())`, then `Call->getArg(0)` is an implicit 
> `CXXConstructExpr`, not a `MemberExpr` (`MemberExpr` will be its indirect 
> child). The case should be handled correctly.
That might be it. I remember that in some early version I was biding to 
MemberExpr somewhere, which might cause it.


https://reviews.llvm.org/D22208



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

Re: [PATCH] D22208: [clang-tidy] Fixes to modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

In https://reviews.llvm.org/D22208#503194, @alexfh wrote:

> Please add revision number (this can be automated, if include differential 
> revision URL in your commit message as described in 
> http://llvm.org/docs/Phabricator.html#committing-a-change).


I normally use arc, but I was working in half from my laptop and in half from 
workstation, and I am not sure if arc would handle this.

I added the url in the patch, but I didn't know that "Differential Revision:" 
is required. I will keep it in mind next time.


https://reviews.llvm.org/D22208



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

Re: [PATCH] D22208: [clang-tidy] Fixes to modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:115
@@ -95,1 +114,3 @@
+  auto CtorCallSourceRange = CharSourceRange::getTokenRange(
+  InnerCtorCall->getExprLoc(), CallParensRange.getBegin());
 

alexfh wrote:
> Prazek wrote:
> > alexfh wrote:
> > > This doesn't seem to be an issue, since expression 
> > > `v.push_back(obj.member())` won't trigger this check: it expects that the 
> > > argument of the `push_back` call is a `cxxConstructExpr` or a 
> > > `cxxFunctionalCastExpr`.
> > what about the implicit conversion? What if obj.member would return object 
> > that is different from the one that v stores, but it is convertible to it?
> Sounds almost like a recipe for a test case ;) Have you tried to construct it 
> this way?
Yes, I think I emulated this case in line 190. I tried to hit this problem but 
could not find the test case.


https://reviews.llvm.org/D22208



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

Re: [PATCH] D22208: [clang-tidy] Fixes to modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: clang-tidy/modernize/UseEmplaceCheck.h:36-37
@@ -32,1 +35,4 @@
+private:
+  std::vector ContainersWithPushBack;
+  std::vector SmartPointers;
 };

aaron.ballman wrote:
> What about `llvm::make_range()`?
llvm::make_range do pretty much the same thing. The patch is in upstream, so 
feel free to try it, but either I am doing something wrong, or just 
SmallVector/ArrayRef have this limitations.


https://reviews.llvm.org/D22208



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

Re: [PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

I see you solved the void and conmpatible types problems. Excellent!

Can you post a patch with changes after running LLVM? I would wait for Alex or 
Aaron to accept it.



Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:58-60
@@ +57,5 @@
+
+  for (const auto  :
+   std::vector>(
+   {{"memcpy", "std::copy"}, {"memset", "std::fill"}})) {
+Replacements.insert(KeyValue);

Try just initializer list instead of creating vector here.
Just 
for (const auto  : {std::make_pair("memcpy", "std::copy"), {"memset", 
"std::fill"}}

You just have to specify the type of the first argument and it will try to 
convert each next to it.


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:98-99
@@ +97,4 @@
+  const auto it = Replacements.find(MatchedName);
+  if (it == Replacements.end())
+return;
+  const auto ReplacedName = it->second;

this should not happen. You can change it to assert


Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:139
@@ +138,3 @@
+// Cannot do pointer arithmetic on void type.
+if (getStrippedType(Arg1Type)->isVoidType())
+  return;

Would it be the same to check here for the first argument instead of second?
So then you could take this if before the if(MatchedName == "memset")


Comment at: docs/clang-tidy/checks/modernize-use-algorithm.rst:37
@@ +36,2 @@
+std::fill(dest, dest + count, ch)
+

You should add 2 notes here
1. The check runs just with C++.
2. There are some rare cases that require adding parens (and put example), so 
the check could right now generate wrong output.
 (which is something that I would not require to have, but it is good to leave 
not about it in documentation)

Also leave Fixit comment in the check.


Repository:
  rL LLVM

https://reviews.llvm.org/D22725



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

[clang-tools-extra] r277097 - [clang-tidy] Fixes to modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Author: prazek
Date: Thu Jul 28 21:10:23 2016
New Revision: 277097

URL: http://llvm.org/viewvc/llvm-project?rev=277097=rev
Log:
[clang-tidy] Fixes to modernize-use-emplace

Not everything is valid, but it should works for 99.8% cases

https://reviews.llvm.org/D22208

Modified:
clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.h
clang-tools-extra/trunk/clang-tidy/utils/Matchers.h
clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-emplace.rst
clang-tools-extra/trunk/test/clang-tidy/modernize-use-emplace.cpp

Modified: clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp?rev=277097=277096=277097=diff
==
--- clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseEmplaceCheck.cpp Thu Jul 28 
21:10:23 2016
@@ -8,14 +8,25 @@
 
//===--===//
 
 #include "UseEmplaceCheck.h"
-#include "../utils/Matchers.h"
-
+#include "../utils/OptionsUtils.h"
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
+static const auto DefaultContainersWithPushBack =
+"::std::vector; ::std::list; ::std::deque";
+static const auto DefaultSmartPointers =
+"::std::shared_ptr; ::std::unique_ptr; ::std::auto_ptr; ::std::weak_ptr";
+
+UseEmplaceCheck::UseEmplaceCheck(StringRef Name, ClangTidyContext *Context)
+: ClangTidyCheck(Name, Context),
+  ContainersWithPushBack(utils::options::parseStringList(Options.get(
+  "ContainersWithPushBack", DefaultContainersWithPushBack))),
+  SmartPointers(utils::options::parseStringList(
+  Options.get("SmartPointers", DefaultSmartPointers))) {}
+
 void UseEmplaceCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus11)
 return;
@@ -31,40 +42,51 @@ void UseEmplaceCheck::registerMatchers(M
   // + match for make_pair calls.
   auto callPushBack = cxxMemberCallExpr(
   hasDeclaration(functionDecl(hasName("push_back"))),
-  on(hasType(cxxRecordDecl(hasAnyName("std::vector", "llvm::SmallVector",
-  "std::list", "std::deque");
+  on(hasType(cxxRecordDecl(hasAnyName(SmallVector(
+  ContainersWithPushBack.begin(), ContainersWithPushBack.end()));
 
   // We can't replace push_backs of smart pointer because
   // if emplacement fails (f.e. bad_alloc in vector) we will have leak of
   // passed pointer because smart pointer won't be constructed
   // (and destructed) as in push_back case.
-  auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(
-  ofClass(hasAnyName("std::shared_ptr", "std::unique_ptr", "std::auto_ptr",
- "std::weak_ptr";
+  auto isCtorOfSmartPtr = hasDeclaration(cxxConstructorDecl(ofClass(hasAnyName(
+  SmallVector(SmartPointers.begin(), 
SmartPointers.end());
 
   // Bitfields binds only to consts and emplace_back take it by universal ref.
-  auto bitFieldAsArgument = hasAnyArgument(ignoringParenImpCasts(
-  memberExpr(hasDeclaration(fieldDecl(matchers::isBitfield());
+  auto bitFieldAsArgument = hasAnyArgument(
+  ignoringImplicit(memberExpr(hasDeclaration(fieldDecl(isBitField());
+
+  // Initializer list can't be passed to universal reference.
+  auto initializerListAsArgument = hasAnyArgument(
+  ignoringImplicit(cxxConstructExpr(isListInitialization(;
 
   // We could have leak of resource.
-  auto newExprAsArgument = hasAnyArgument(ignoringParenImpCasts(cxxNewExpr()));
+  auto newExprAsArgument = hasAnyArgument(ignoringImplicit(cxxNewExpr()));
+  // We would call another constructor.
   auto constructingDerived =
   hasParent(implicitCastExpr(hasCastKind(CastKind::CK_DerivedToBase)));
 
-  auto hasInitList = has(ignoringParenImpCasts(initListExpr()));
+  // emplace_back can't access private constructor.
+  auto isPrivateCtor = hasDeclaration(cxxConstructorDecl(isPrivate()));
+
+  auto hasInitList = has(ignoringImplicit(initListExpr()));
+  // FIXME: Discard 0/NULL (as nullptr), static inline const data members,
+  // overloaded functions and template names.
   auto soughtConstructExpr =
   cxxConstructExpr(
   unless(anyOf(isCtorOfSmartPtr, hasInitList, bitFieldAsArgument,
-   newExprAsArgument, constructingDerived,
-   has(materializeTemporaryExpr(hasInitList)
+   initializerListAsArgument, newExprAsArgument,
+   constructingDerived, isPrivateCtor)))
   .bind("ctor");
-  auto hasConstructExpr = has(ignoringParenImpCasts(soughtConstructExpr));
+  auto hasConstructExpr = 

Re: [llvm-dev] [PATCH] Add support for the 'unless' matcher in the dynamic layer.

[Piotr Padlewski via cfe-commits]

Is, but it is still a lot of typing and we are talking about debuging.

2016-07-27 3:40 GMT-07:00 Manuel Klimek :

> On Wed, Jul 27, 2016 at 1:06 AM Piotr Padlewski via llvm-dev <
> llvm-...@lists.llvm.org> wrote:
>
>> We could also just add nothing() matcher, so debugging would be much
>> easier, just add anything() or nothing() matcher as extra argument.
>>
>> The other pros of it is that new developers won't send the patches that
>> uses those variadic matchers with only one argument.
>>
>
> We already have anything() and unless(anything()).
>
>
>>
>> 2016-07-26 16:02 GMT-07:00 Zac Hansen via llvm-dev <
>> llvm-...@lists.llvm.org>:
>>
>>> Even if it still did add overhead, it seems perfectly reasonable, from a
>>> user's perspective (namely mine), that if I introduce unnecessary narrowing
>>> matchers to my chain that there may be a performance penalty.
>>>
>>> The ability to do the following easily outweighs any performance issues
>>> for me:
>>>
>>>
>>> anyOf (
>>> /*hasName("..."), */
>>> hasName("...")
>>>
>>> )
>>>
>>> though C++ not allowing trailing commas makes this not quite as great.
>>>
>>>
>>> *However, without help, I would not be able to put forward a patch with
>>> anything more than simply removing the minimums.*
>>>
>>> Would this be acceptable or would someone be able to point me at what it
>>> would take to do it the "smart way" in less time than it would take them to
>>> make the change themselves?
>>>
>>> On Tue, Jul 26, 2016 at 3:29 PM, Samuel Benzaquen 
>>> wrote:
>>>
 One of the reasons we added the minimum was because these nodes added
 overhead to the matching that was not unnecessary when they only had a
 single node.
 On the current implementation we could actually get rid of the node
 completely for the one argument calls.
 I would be ok with removing the lower bound.


 On Tue, Jul 26, 2016 at 6:20 PM, Zac Hansen  wrote:

> I was wondering if there is any objection to removing the 2-element
> minimum on the eachOf, anyOf and allOf matchers.
>
> It is frustrating when playing with matchers to have to edit
> significant amounts of code to be able to temporarily go from 2 to 1
> matcher inside an any- or allOf matcher.
>
> And overall it feels very "un-set-theory"-like.
>
> The change was made here:
> https://github.com/llvm-mirror/clang/commit/674e54c167eab0be7a54bca7082c07d2f1d0c8cc
>
>
> Thank you and apologies if I sent this to the wrong lists/people.
>
> --Zac
>


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

Re: [llvm-dev] [PATCH] Add support for the 'unless' matcher in the dynamic layer.

[Piotr Padlewski via cfe-commits]

We could also just add nothing() matcher, so debugging would be much
easier, just add anything() or nothing() matcher as extra argument.

The other pros of it is that new developers won't send the patches that
uses those variadic matchers with only one argument.

2016-07-26 16:02 GMT-07:00 Zac Hansen via llvm-dev 
:

> Even if it still did add overhead, it seems perfectly reasonable, from a
> user's perspective (namely mine), that if I introduce unnecessary narrowing
> matchers to my chain that there may be a performance penalty.
>
> The ability to do the following easily outweighs any performance issues
> for me:
>
>
> anyOf (
> /*hasName("..."), */
> hasName("...")
>
> )
>
> though C++ not allowing trailing commas makes this not quite as great.
>
>
> *However, without help, I would not be able to put forward a patch with
> anything more than simply removing the minimums.*
>
> Would this be acceptable or would someone be able to point me at what it
> would take to do it the "smart way" in less time than it would take them to
> make the change themselves?
>
> On Tue, Jul 26, 2016 at 3:29 PM, Samuel Benzaquen 
> wrote:
>
>> One of the reasons we added the minimum was because these nodes added
>> overhead to the matching that was not unnecessary when they only had a
>> single node.
>> On the current implementation we could actually get rid of the node
>> completely for the one argument calls.
>> I would be ok with removing the lower bound.
>>
>>
>> On Tue, Jul 26, 2016 at 6:20 PM, Zac Hansen  wrote:
>>
>>> I was wondering if there is any objection to removing the 2-element
>>> minimum on the eachOf, anyOf and allOf matchers.
>>>
>>> It is frustrating when playing with matchers to have to edit significant
>>> amounts of code to be able to temporarily go from 2 to 1 matcher inside an
>>> any- or allOf matcher.
>>>
>>> And overall it feels very "un-set-theory"-like.
>>>
>>> The change was made here:
>>> https://github.com/llvm-mirror/clang/commit/674e54c167eab0be7a54bca7082c07d2f1d0c8cc
>>>
>>>
>>> Thank you and apologies if I sent this to the wrong lists/people.
>>>
>>> --Zac
>>>
>>
>>
>
> ___
> LLVM Developers mailing list
> llvm-...@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22208: [clang-tidy] Fixes to modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: clang-tidy/modernize/UseEmplaceCheck.cpp:115
@@ -95,1 +114,3 @@
+  auto CtorCallSourceRange = CharSourceRange::getTokenRange(
+  InnerCtorCall->getExprLoc(), CallParensRange.getBegin());
 

alexfh wrote:
> This doesn't seem to be an issue, since expression 
> `v.push_back(obj.member())` won't trigger this check: it expects that the 
> argument of the `push_back` call is a `cxxConstructExpr` or a 
> `cxxFunctionalCastExpr`.
what about the implicit conversion? What if obj.member would return object that 
is different from the one that v stores, but it is convertible to it?


https://reviews.llvm.org/D22208



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

Re: [PATCH] D22725: [clang-tidy] Add check 'misc-replace-memcpy'

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

In https://reviews.llvm.org/D22725#495329, @etienneb wrote:

> In https://reviews.llvm.org/D22725#494167, @Prazek wrote:
>
> > hmm It seems that I mislead you, I suck at C api - memmove source and 
> > destination can overlap, but std::move can't. So I guess you have to remove 
> > the memmove support. Sorry.
>
>
> On windows (MSVC CRT), both function are the same : memmove.


It is possible that the call to move with some trivialy copyable object would 
end up calling memmove, even if the call is wrong.

quote from http://en.cppreference.com/w/cpp/algorithm/move

> d_first   -   the beginning of the destination range. If d_first is 
> within [first, last), std::move_backward must be used instead of std::move.


But this is only the special ability of memmove, so we should not introduce 
stl-implementation dependent bugs


https://reviews.llvm.org/D22725



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

Re: [PATCH] D22507: Clang-tidy - Enum misuse check

[Piotr Padlewski via cfe-commits]

Prazek added a subscriber: Prazek.


Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:19
@@ +18,3 @@
+
+// Stores a min and a max value which describe an interval.
+struct ValueRange {

s/\/\//\/\/\/

In other words, change // to /// (doxygen comment)


Comment at: clang-tidy/misc/EnumMisuseCheck.cpp:192
@@ +191,3 @@
+  const auto *LhsConstant =
+  LhsDecExpr ? dyn_cast(LhsDecExpr->getDecl()) : nullptr;
+  const auto *RhsConstant =

If the pointer returned from dyn_cast(LhsDeclExpr->getDecl()) 
is assumed to be always not null, thn you can change it to cast<> (It will 
assert if null instead of returning nullptr)


Comment at: docs/clang-tidy/checks/misc-enum-misuse.rst:29
@@ +28,3 @@
+
+.. code:: c++
+

I think all the code here needs to be indented by one or more to get into 
..code:: block.


https://reviews.llvm.org/D22507



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

Re: [PATCH] D22725: [clang-tidy] Add check 'misc-replace-memcpy'

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

In https://reviews.llvm.org/D22725#494168, @JDevlieghere wrote:

> In https://reviews.llvm.org/D22725#494167, @Prazek wrote:
>
> > hmm It seems that I mislead you, I suck at C api - memmove source and 
> > destination can overlap, but std::move can't. So I guess you have to remove 
> > the memmove support. Sorry.
>
>
> No problem, I wasn't aware of the issue either, so thanks for letting me know!
>
> While running my check on the LLVM repo I ran into another issue: can't do 
> pointer arithmetic when the arguments to memcpy are void pointers. I will 
> look into updating my matchers to exclude this particular case.


Yep, developing clang-tidy checks is mostly about trying to find out all the 
cases, and then finding that there are 3x more corner cases after running it on 
LLVM :)
btw I recommend compiling on Release with enabled assertions 
(-DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON). Usually you don't 
need debug info while developing clang-tidy because it doesn't crash that often 
and Release is at least 4x faster than RelWithDebInfo (and even more than 
Debug), so it actually make sense to recompile everything and run it than to 
wait for clang-tidy compiled with debug.


https://reviews.llvm.org/D22725



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

Re: [PATCH] D22725: [clang-tidy] Add check 'misc-replace-memcpy'

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

hmm It seems that I mislead you, I suck at C api - memmove source and 
destination can overlap, but std::move can't. So I guess you have to remove the 
memmove support. Sorry.


https://reviews.llvm.org/D22725



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

Re: [PATCH] D22725: [clang-tidy] Add check 'misc-replace-memcpy'

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

In https://reviews.llvm.org/D22725#493970, @JDevlieghere wrote:

> In https://reviews.llvm.org/D22725#493947, @Prazek wrote:
>
> > Thanks for the contribution. Is it your first check?
>
>
> Yes, it is! :-)


Cool! hope to see some other cool checks. You could probably use pretty much 
the same code, and make modernize-use-fill for replacing memset and
modernize-use-move to change memmove -> move. The other thing that I can think 
of, is also to detect when you could use std::copy_n istead.

Maybe the right way would be to have check called 
'modernize-use-sequence-algorithm' or just 'modernize-use-algorithm' that would 
basically do all those stuff. It would be good to not introduce 5 new check 
that pretty much do the same thing and also the user would want to have them 
all or none.
You can look how I did it with modernize-use-make-shared, it would be pretty 
much the same.

> I have included an example in the documentation with the ternary conditional 
> operator. I agree that it's not ideal, but I tried to be defensive. Basically 
> anything with lower precedence than the + operator might cause an issue, and 
> this is quite a lot...


So It should not be very hard. You would just have to check if the second or 
third argument to memcpy is one of [list here] of the things that 
would make it bad. And you can probably write easy matcher for this.
I understand that it probably will take the double of time that you have spend 
on this check, but this will actually is the thing here - you want your check 
to produce the fixes that makes sense. When you will run it on llvm code base, 
I guess you will see that over 90% of the cases didn't require coma. I 
understand that you want to make it accurate, but the fix should not also 
intoruce some code that is not tidy enough.
So I guess either fix it, or wait what Alex will say about it. It might make it 
to upstream like this, but I hope you would fix it in some time :)

Anyway good job, hope to see some more checks from you!

> > 4. Have you run the check on LLVM code base? It uses std::copy in many 
> > places. If after running with -fix all the memcpys will be gone (check if 
> > if finds anything after second run) and the code will compile and tests 
> > will not fail then it means that your check fully works!

> 

> 

> Doing this as we speak!


When you will be finished just post a diff of changes after running it on llvm 
on phabricator and add me as subscriber :)



Comment at: clang-tidy/modernize/UseCopyCheck.cpp:38
@@ +37,3 @@
+  // Match calls to memcpy with 3 arguments
+  Finder->addMatcher(callExpr(allOf(callee(functionDecl(hasName("memcpy"))),
+argumentCountIs(3)))

change memcpy to ::std::memcpy, and also make a test with some memcpy that is 
outside of std namespace.
You don't want to change someone else code because it uses the same name :)


Repository:
  rL LLVM

https://reviews.llvm.org/D22725



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

Re: [PATCH] D22725: [clang-tidy] Add check 'misc-replace-memcpy'

[Piotr Padlewski via cfe-commits]

Prazek added inline comments.


Comment at: clang-tidy/modernize/UseCopyCheck.cpp:49
@@ +48,3 @@
+  if (getLangOpts().CPlusPlus) {
+Inserter.reset(new utils::IncludeInserter(
+Compiler.getSourceManager(), Compiler.getLangOpts(), IncludeStyle));

there is actually llvm::make_shared somewhere


Comment at: clang-tidy/modernize/UseCopyCheck.cpp:59-61
@@ +58,5 @@
+
+  // Don't make replacements in macro
+  if (Loc.isMacroID())
+return;
+

What you should do is you should print out the warning, but not make the 
replacement for macro (because you can't get the exact location)


So just move auto Diag = ... above this line.

Also finish comments with coma.


Comment at: test/clang-tidy/modernize-use-copy.cpp:17
@@ +16,3 @@
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: Use std::copy instead of memcpy
+  // [modernize-use-copy]
+  // CHECK-FIXES: std::copy(foo, (foo) + (sizeof bar), bar);

This won't be checked. The long checking comments are ok in tests.


Comment at: test/clang-tidy/modernize-use-copy.cpp:34
@@ +33,3 @@
+
+#define memcpy(dest, src, len) std::memcpy((dest), (src), (len))
+void g() {

change it to MEMCPY, and maybe remove the last argument (so it will be set to 
sizeof dest), because this macro doesn't make much sense right now.


Comment at: test/clang-tidy/modernize-use-copy.cpp:37
@@ +36,3 @@
+  char foo[] = "foo", bar[3];
+  memcpy(bar, foo, sizeof bar);
+}

so ter should be a warning here.


Repository:
  rL LLVM

https://reviews.llvm.org/D22725



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

Re: [PATCH] D22208: [clang-tidy] Fixes to modernize-use-emplace

[Piotr Padlewski via cfe-commits]

Prazek removed rL LLVM as the repository for this revision.
Prazek updated this revision to Diff 65243.
Prazek added a comment.

Ok works right now. I don't know why but I could not reproduce the error in 
test file, but I manged to fix it.


https://reviews.llvm.org/D22208

Files:
  clang-tidy/modernize/UseEmplaceCheck.cpp
  clang-tidy/modernize/UseEmplaceCheck.h
  clang-tidy/utils/Matchers.h
  docs/clang-tidy/checks/modernize-use-emplace.rst
  test/clang-tidy/modernize-use-emplace.cpp

Index: test/clang-tidy/modernize-use-emplace.cpp
===
--- test/clang-tidy/modernize-use-emplace.cpp
+++ test/clang-tidy/modernize-use-emplace.cpp
@@ -1,4 +1,7 @@
-// RUN: %check_clang_tidy %s modernize-use-emplace %t
+// RUN: %check_clang_tidy %s modernize-use-emplace %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN: [{key: modernize-use-emplace.ContainersWithPushBack, \
+// RUN:   value: '::std::vector; ::std::list; ::std::deque; llvm::LikeASmallVector'}]}" -- -std=c++11
 
 namespace std {
 template 
@@ -9,6 +12,7 @@
 
   template 
   void emplace_back(Args &&... args){};
+  ~vector();
 };
 template 
 class list {
@@ -18,6 +22,7 @@
 
   template 
   void emplace_back(Args &&... args){};
+  ~list();
 };
 
 template 
@@ -28,6 +33,7 @@
 
   template 
   void emplace_back(Args &&... args){};
+  ~deque();
 };
 
 template 
@@ -54,10 +60,24 @@
 template 
 class unique_ptr {
 public:
-  unique_ptr(T *) {}
+  explicit unique_ptr(T *) {}
+  ~unique_ptr();
 };
 } // namespace std
 
+namespace llvm {
+template 
+class LikeASmallVector {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args){};
+};
+
+} // llvm
+
 void testInts() {
   std::vector v;
   v.push_back(42);
@@ -72,6 +92,7 @@
   Something(int a, int b = 41) {}
   Something() {}
   void push_back(Something);
+  int getInt() { return 42; }
 };
 
 struct Convertable {
@@ -103,6 +124,15 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: v.emplace_back();
 
+  Something Different;
+  v.push_back(Something(Different.getInt(), 42));
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(Different.getInt(), 42);
+
+  v.push_back(Different.getInt());
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(Different.getInt());
+
   v.push_back(42);
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: v.emplace_back(42);
@@ -117,6 +147,23 @@
   v.push_back(s);
 }
 
+template 
+void dependOnElem() {
+  std::vector v;
+  v.push_back(ElemType(42));
+}
+
+template 
+void dependOnContainer() {
+  ContainerType v;
+  v.push_back(Something(42));
+}
+
+void callDependent() {
+  dependOnElem();
+  dependOnContainer();
+}
+
 void test2() {
   std::vector v;
   v.push_back(Zoz(Something(21, 37)));
@@ -130,12 +177,20 @@
   v.push_back(getZoz(Something(1, 2)));
 }
 
+struct GetPair {
+  std::pair getPair();
+};
 void testPair() {
   std::vector> v;
   v.push_back(std::pair(1, 2));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: v.emplace_back(1, 2);
 
+  GetPair g;
+  v.push_back(g.getPair());
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
+  // CHECK-FIXES: v.emplace_back(g.getPair());
+
   std::vector> v2;
   v2.push_back(std::pair(Something(42, 42), Zoz(Something(21, 37;
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
@@ -206,14 +261,14 @@
   v.push_back(new int(5));
 
   std::vector v2;
-  v2.push_back(new int(42));
+  v2.push_back(std::unique_ptr(new int(42)));
   // This call can't be replaced with emplace_back.
   // If emplacement will fail (not enough memory to add to vector)
   // we will have leak of int because unique_ptr won't be constructed
   // (and destructed) as in push_back case.
 
   auto *ptr = new int;
-  v2.push_back(ptr);
+  v2.push_back(std::unique_ptr(ptr));
   // Same here
 }
 
@@ -240,6 +295,11 @@
   d.push_back(Something(42));
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: d.emplace_back(42);
+
+  llvm::LikeASmallVector ls;
+  ls.push_back(Something(42));
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use emplace_back
+  // CHECK-FIXES: ls.emplace_back(42);
 }
 
 class IntWrapper {
@@ -336,3 +396,29 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use emplace_back
   // CHECK-FIXES: v.emplace_back(42, var);
 }
+
+class PrivateCtor {
+  PrivateCtor(int z);
+
+public:
+  void doStuff() {
+std::vector v;
+// This should not change it because emplace back doesn't have permission.
+// Check currently doesn't support friend delcarations because pretty much
+// nobody would want to be friend with std::vector :(.
+

Re: [PATCH] D22725: [clang-tidy] Add check 'misc-replace-memcpy'

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

In https://reviews.llvm.org/D22725#493942, @JDevlieghere wrote:

> In https://reviews.llvm.org/D22725#493940, @aaron.ballman wrote:
>
> > > Using std::copy opens up the possibility of type-aware optimizations 
> > > which are not possible with memcpy.
> >
> >
> > To my knowledge, those type-aware optimizations are for transforming copies 
> > involving trivially-copyable types into calls to memcpy(). What other 
> > optimizations does this check enable?
>
>
> I might be mistaken, but I understood that a call to std::copy (whether or 
> not specialized for certain types) might get inlined, while a call to memcpy 
> is typically not.


So if it is something trivially copyable (just copying bytes) then std::copy 
would probably call memcpy inside - so it won't be optimization in our case 
(even if it will get inlined).

But it will definitelly make code cleaner and won't be slower, which would be 
good to mention in docs.


Repository:
  rL LLVM

https://reviews.llvm.org/D22725



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