[PATCH] D55710: add pragmas to control Software Pipelining optimisation

2018-12-24 Thread Alexey Lapshin via Phabricator via cfe-commits 
alexey.lapshin marked 2 inline comments as done.
alexey.lapshin added inline comments.



Comment at: include/clang/Basic/AttrDocs.td:2655
+  dependence violated. This constant interval(in cycles) between the start
+  of iterations called initiation interval. Cycles number of one iteration
+  of newly generated loop matches with Initiation Interval. For further

aaron.ballman wrote:
> alexey.lapshin wrote:
> > aaron.ballman wrote:
> > > is called the initiation interval.
> > > 
> > > I can't quite parse the second sentence, though. Is it trying to say that 
> > > the initiation interval is the number of cycles for a single iteration of 
> > > the optimized loop?
> > Not quite right : initiation interval is not a number of cycles for a 
> > single iteration of the optimized loop. But initiation interval matches 
> > with number of cycles for a single iteration of the optimized loop. 
> > Initiation interval is the number of cycles between next and previous 
> > iteration of original loop. New loop created so that single iteration of 
> > the optimized loop has the same number cycles as initiation interval( thus 
> > every new iteration of original loop started each time when new iteration 
> > of optimized loop started - difference between iterations is initiation 
> > interval). 
> > 
> > trying to rephrase : number of cycles for a single iteration of the 
> > optimized loop matches with number of cycles of initiation interval.
> I am still a bit fuzzy on the details (optimizations are not my area of 
> expertise). Is this an accurate what to rephrase it?
> 
> "The initiation interval is the number of cycles between two iterations of an 
> unoptimized loop. These pragmas cause a new. optimized loop to be created 
> such that a single iteration of the loop executes in the same number of 
> cycles as the initiation interval."
I will take it with small modifications.



Comment at: include/clang/Basic/AttrDocs.td:2676
+  the software pipeliner to try the specified initiation interval.
+  If schedule found the resulting loop iteration would have specified
+  cycle count. The positive number greater than zero can be specified:

aaron.ballman wrote:
> alexey.lapshin wrote:
> > aaron.ballman wrote:
> > > I'm can't quite parse this sentence either. Is it trying to say that the 
> > > scheduler will try to find a loop iteration cycle count that most-closely 
> > > matches the specified initiation interval?
> > I wanted to say that pipeliner will try to find a schedule that exactly 
> > matches the specified initiation interval. And if schedule would be found 
> > then single iteration of the optimized loop would have exactly the same 
> > amount of cycles as initiation interval. trying to rephrase :
> > 
> > If schedule would be found then single iteration of the optimized loop 
> > would have exactly the same amount of cycles as initiation interval has. 
> Ah, okay! So what happens if an exact match cannot be found? Does it behave 
> as though the pragma was not specified at all (as in, the pragma is ignored)?
yes. if schedule was not found then the loop would stay unchanged.


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

https://reviews.llvm.org/D55710



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

[PATCH] D55710: add pragmas to control Software Pipelining optimisation

2018-12-24 Thread Alexey Lapshin via Phabricator via cfe-commits 
alexey.lapshin updated this revision to Diff 179459.
alexey.lapshin added a comment.

please consider changes in AttrDocs.td


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

https://reviews.llvm.org/D55710

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticParseKinds.td
  lib/CodeGen/CGLoopInfo.cpp
  lib/CodeGen/CGLoopInfo.h
  lib/Parse/ParsePragma.cpp
  lib/Sema/SemaStmtAttr.cpp
  test/CodeGenCXX/pragma-pipeline.cpp
  test/Parser/pragma-loop.cpp
  test/Parser/pragma-pipeline.cpp
  test/Parser/pragma-unroll-and-jam.cpp

Index: test/Parser/pragma-unroll-and-jam.cpp
===
--- test/Parser/pragma-unroll-and-jam.cpp
+++ test/Parser/pragma-unroll-and-jam.cpp
@@ -67,7 +67,7 @@
   }
 
 // pragma clang unroll_and_jam is disabled for the moment
-/* expected-error {{invalid option 'unroll_and_jam'; expected vectorize, vectorize_width, interleave, interleave_count, unroll, unroll_count, or distribute}} */ #pragma clang loop unroll_and_jam(4)
+/* expected-error {{invalid option 'unroll_and_jam'; expected vectorize, vectorize_width, interleave, interleave_count, unroll, unroll_count, pipeline, pipeline_initiation_interval or distribute}} */ #pragma clang loop unroll_and_jam(4)
   for (int i = 0; i < Length; i++) {
 for (int j = 0; j < Length; j++) {
   List[i * Length + j] = Value;
Index: test/Parser/pragma-pipeline.cpp
===
--- /dev/null
+++ test/Parser/pragma-pipeline.cpp
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -std=c++11 -verify %s
+
+// Note that this puts the expected lines before the directives to work around
+// limitations in the -verify mode.
+
+void test(int *List, int Length, int Value) {
+  int i = 0;
+
+#pragma clang loop pipeline(disable)
+  for (int i = 0; i < Length; i++) {
+List[i] = Value;
+  }
+
+#pragma clang loop pipeline_initiation_interval(10)
+  for (int i = 0; i < Length; i++) {
+List[i] = Value;
+  }
+
+/* expected-error {{expected ')'}} */ #pragma clang loop pipeline(disable
+/* expected-error {{invalid argument; expected 'disable'}} */ #pragma clang loop pipeline(enable)
+/* expected-error {{invalid argument; expected 'disable'}} */ #pragma clang loop pipeline(error)
+/* expected-error {{expected '('}} */ #pragma clang loop pipeline disable
+/* expected-error {{missing argument; expected an integer value}} */ #pragma clang loop pipeline_initiation_interval()
+/* expected-error {{use of undeclared identifier 'error'}} */ #pragma clang loop pipeline_initiation_interval(error)
+/* expected-error {{expected '('}} */ #pragma clang loop pipeline_initiation_interval 1 2
+/* expected-error {{expected ')'}} */ #pragma clang loop pipeline_initiation_interval(1
+/* expected-error {{invalid argument of type 'double'; expected an integer type}} */ #pragma clang loop pipeline_initiation_interval(1.0)
+/* expected-error {{invalid value '0'; must be positive}} */ #pragma clang loop pipeline_initiation_interval(0)
+  for (int i = 0; i < Length; i++) {
+for (int j = 0; j < Length; j++) {
+  List[i * Length + j] = Value;
+}
+  }
+
+#pragma clang loop pipeline(disable) 
+/* expected-error {{expected a for, while, or do-while loop to follow '#pragma clang loop'}} */ int j = Length;
+#pragma clang loop pipeline_initiation_interval(4)
+/* expected-error {{expected a for, while, or do-while loop to follow '#pragma clang loop'}} */ int k = Length;
+
+#pragma clang loop pipeline(disable)
+#pragma clang loop pipeline_initiation_interval(4) /* expected-error {{incompatible directives 'pipeline(disable)' and 'pipeline_initiation_interval(4)'}} */
+  for (int i = 0; i < Length; i++) {
+List[i] = Value;
+  }
+
+#pragma clang loop pipeline(disable)
+/* expected-error {{expected statement}} */ }
Index: test/Parser/pragma-loop.cpp
===
--- test/Parser/pragma-loop.cpp
+++ test/Parser/pragma-loop.cpp
@@ -147,7 +147,7 @@
 /* expected-error {{missing argument; expected 'enable', 'full' or 'disable'}} */ #pragma clang loop unroll()
 /* expected-error {{missing argument; expected 'enable' or 'disable'}} */ #pragma clang loop distribute()
 
-/* expected-error {{missing option; expected vectorize, vectorize_width, interleave, interleave_count, unroll, unroll_count, or distribute}} */ #pragma clang loop
+/* expected-error {{missing option; expected vectorize, vectorize_width, interleave, interleave_count, unroll, unroll_count, pipeline, pipeline_initiation_interval or distribute}} */ #pragma clang loop
 /* expected-error {{invalid option 'badkeyword'}} */ #pragma clang loop badkeyword
 /* expected-error {{invalid option 'badkeyword'}} */ #pragma clang loop badkeyword(enable)
 /* expected-error {{invalid option 'badkeyword'}} */ #pragma clang loop vectorize(enable) badkeyword(4)
Index: test/CodeGenCXX/pragma-pipeline.cpp
===

[PATCH] D56059: [gn build] Add build files for clang/tools/{c-arcmt-test, c-index-test} and their dependency clang/tools/libclang

2018-12-24 Thread Petr Hosek via Phabricator via cfe-commits 
phosek accepted this revision.
phosek marked an inline comment as done.
phosek added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: llvm/utils/gn/secondary/clang/tools/libclang/BUILD.gn:16-17
+if (host_os == "linux") {
+  # Linux needs -fPIC to build shared libs but they aren't on by default.
+  # For now, make libclang a static lib there.
+  libclang_target_type = "static_library"

We can solve this with toolchains, i.e. have a variant of the default toolchain 
that includes `-fPIC` which will be used to build `libClang.so`. Not something 
to do in this change, but definitely something we should look into in the 
future.


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

https://reviews.llvm.org/D56059



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

[PATCH] D47817: [compiler-rt] [sanitizer_common] Fix using libtirpc on Linux

2018-12-24 Thread Michał Górny via Phabricator via cfe-commits 
mgorny updated this revision to Diff 179460.
mgorny added a comment.
Herald added subscribers: srhines, emaste.

Updated as requested by @Lekensteyn


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

https://reviews.llvm.org/D47817

Files:
  cmake/Modules/FindLibtirpc.cmake
  lib/sanitizer_common/CMakeLists.txt
  lib/sanitizer_common/sanitizer_platform.h
  lib/sanitizer_common/sanitizer_platform_limits_freebsd.cc
  lib/sanitizer_common/sanitizer_platform_limits_posix.cc

Index: lib/sanitizer_common/sanitizer_platform_limits_posix.cc
===
--- lib/sanitizer_common/sanitizer_platform_limits_posix.cc
+++ lib/sanitizer_common/sanitizer_platform_limits_posix.cc
@@ -112,10 +112,8 @@
 #include 
 #include 
 #include 
-#if HAVE_RPC_XDR_H
+#ifdef HAVE_RPC_XDR_H
 # include 
-#elif HAVE_TIRPC_RPC_XDR_H
-# include 
 #endif
 #include 
 #include 
@@ -1210,7 +1208,7 @@
 CHECK_SIZE_AND_OFFSET(group, gr_gid);
 CHECK_SIZE_AND_OFFSET(group, gr_mem);
 
-#if HAVE_RPC_XDR_H || HAVE_TIRPC_RPC_XDR_H
+#ifdef HAVE_RPC_XDR_H
 CHECK_TYPE_SIZE(XDR);
 CHECK_SIZE_AND_OFFSET(XDR, x_op);
 CHECK_SIZE_AND_OFFSET(XDR, x_ops);
Index: lib/sanitizer_common/sanitizer_platform_limits_freebsd.cc
===
--- lib/sanitizer_common/sanitizer_platform_limits_freebsd.cc
+++ lib/sanitizer_common/sanitizer_platform_limits_freebsd.cc
@@ -500,7 +500,7 @@
 CHECK_SIZE_AND_OFFSET(group, gr_gid);
 CHECK_SIZE_AND_OFFSET(group, gr_mem);
 
-#if HAVE_RPC_XDR_H || HAVE_TIRPC_RPC_XDR_H
+#ifdef HAVE_RPC_XDR_H
 CHECK_TYPE_SIZE(XDR);
 CHECK_SIZE_AND_OFFSET(XDR, x_op);
 CHECK_SIZE_AND_OFFSET(XDR, x_ops);
Index: lib/sanitizer_common/sanitizer_platform.h
===
--- lib/sanitizer_common/sanitizer_platform.h
+++ lib/sanitizer_common/sanitizer_platform.h
@@ -275,12 +275,6 @@
 # define SANITIZER_POINTER_FORMAT_LENGTH FIRST_32_SECOND_64(8, 12)
 #endif
 
-// Assume obsolete RPC headers are available by default
-#if !defined(HAVE_RPC_XDR_H) && !defined(HAVE_TIRPC_RPC_XDR_H)
-# define HAVE_RPC_XDR_H (SANITIZER_LINUX && !SANITIZER_ANDROID)
-# define HAVE_TIRPC_RPC_XDR_H 0
-#endif
-
 /// \macro MSC_PREREQ
 /// \brief Is the compiler MSVC of at least the specified version?
 /// The common \param version values to check for are:
Index: lib/sanitizer_common/CMakeLists.txt
===
--- lib/sanitizer_common/CMakeLists.txt
+++ lib/sanitizer_common/CMakeLists.txt
@@ -193,9 +193,12 @@
 
 set(SANITIZER_COMMON_DEFINITIONS)
 
-include(CheckIncludeFile)
-append_have_file_definition(rpc/xdr.h HAVE_RPC_XDR_H SANITIZER_COMMON_DEFINITIONS)
-append_have_file_definition(tirpc/rpc/xdr.h HAVE_TIRPC_RPC_XDR_H SANITIZER_COMMON_DEFINITIONS)
+find_package(Libtirpc)
+
+if (Libtirpc_FOUND)
+  include_directories(${Libtirpc_INCLUDE_DIRS})
+  list(APPEND SANITIZER_COMMON_DEFINITIONS HAVE_RPC_XDR_H=1)
+endif()
 
 set(SANITIZER_CFLAGS ${SANITIZER_COMMON_CFLAGS})
 append_rtti_flag(OFF SANITIZER_CFLAGS)
Index: cmake/Modules/FindLibtirpc.cmake
===
--- /dev/null
+++ cmake/Modules/FindLibtirpc.cmake
@@ -0,0 +1,22 @@
+# CMake find_package() Module for libtirpc
+#
+# Example usage:
+#
+# find_package(Libtirpc)
+#
+# If successful the following variables will be defined
+# Libtirpc_FOUND
+# Libtirpc_INCLUDE_DIRS
+
+find_path(Libtirpc_INCLUDE_DIR
+  NAMES rpc/xdr.h
+  PATH_SUFFIXES tirpc
+)
+
+include(FindPackageHandleStandardArgs)
+find_package_handle_standard_args(Libtirpc
+	REQUIRED_VARS Libtirpc_INCLUDE_DIR)
+
+if(Libtirpc_FOUND)
+  set(Libtirpc_INCLUDE_DIRS ${Libtirpc_INCLUDE_DIR})
+endif()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47817: [compiler-rt] [sanitizer_common] Fix using libtirpc on Linux

2018-12-24 Thread Kamil Rytarowski via Phabricator via cfe-commits 
krytarowski added a comment.

If libtirpc is from an external library we shall not sanitize it. If it is a 
part of libc (on glibc? on solaris?) it shall be sanitized only there.


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

https://reviews.llvm.org/D47817



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

[PATCH] D56025: [clang-tidy] add IgnoreMacros option to readability-uppercase-literal-suffix

2018-12-24 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

LG i guess.


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

https://reviews.llvm.org/D56025



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

[PATCH] D55948: Modify DeclaratorChuck::getFunction to use DeclSpec for qualifiers

2018-12-24 Thread Anastasia Stulova via Phabricator via cfe-commits 
Anastasia updated this revision to Diff 179469.
Anastasia added a comment.

- Changed to store DeclSpec only when quals are present.
- Renamed to MethodQualifiers.


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

https://reviews.llvm.org/D55948

Files:
  include/clang/Sema/DeclSpec.h
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseDeclCXX.cpp
  lib/Parse/ParseExpr.cpp
  lib/Parse/ParseExprCXX.cpp
  lib/Sema/DeclSpec.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaType.cpp

Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -716,6 +716,8 @@
 
   // ...and *prepend* it to the declarator.
   SourceLocation NoLoc;
+  AttributeFactory attrFactory;
+  DeclSpec NoDS(attrFactory);
   declarator.AddInnermostTypeInfo(DeclaratorChunk::getFunction(
   /*HasProto=*/true,
   /*IsAmbiguous=*/false,
@@ -724,12 +726,8 @@
   /*NumArgs=*/0,
   /*EllipsisLoc=*/NoLoc,
   /*RParenLoc=*/NoLoc,
-  /*TypeQuals=*/0,
   /*RefQualifierIsLvalueRef=*/true,
   /*RefQualifierLoc=*/NoLoc,
-  /*ConstQualifierLoc=*/NoLoc,
-  /*VolatileQualifierLoc=*/NoLoc,
-  /*RestrictQualifierLoc=*/NoLoc,
   /*MutableLoc=*/NoLoc, EST_None,
   /*ESpecRange=*/SourceRange(),
   /*Exceptions=*/nullptr,
@@ -737,8 +735,7 @@
   /*NumExceptions=*/0,
   /*NoexceptExpr=*/nullptr,
   /*ExceptionSpecTokens=*/nullptr,
-  /*DeclsInPrototype=*/None,
-  loc, loc, declarator));
+  /*DeclsInPrototype=*/None, loc, loc, declarator, NoDS));
 
   // For consistency, make sure the state still has us as processing
   // the decl spec.
@@ -4460,7 +4457,9 @@
   // does not have a K&R-style identifier list), then the arguments are part
   // of the type, otherwise the argument list is ().
   const DeclaratorChunk::FunctionTypeInfo &FTI = DeclType.Fun;
-  IsQualifiedFunction = FTI.TypeQuals || FTI.hasRefQualifier();
+  IsQualifiedFunction =
+  (FTI.MethodQualifiers && FTI.MethodQualifiers->getTypeQualifiers()) ||
+  FTI.hasRefQualifier();
 
   // Check for auto functions and trailing return type and adjust the
   // return type accordingly.
@@ -4698,7 +4697,9 @@
 EPI.ExtInfo = EI;
 EPI.Variadic = FTI.isVariadic;
 EPI.HasTrailingReturn = FTI.hasTrailingReturnType();
-EPI.TypeQuals.addCVRUQualifiers(FTI.TypeQuals);
+EPI.TypeQuals.addCVRUQualifiers(
+FTI.MethodQualifiers ? FTI.MethodQualifiers->getTypeQualifiers()
+ : 0);
 EPI.RefQualifier = !FTI.hasRefQualifier()? RQ_None
 : FTI.RefQualifierIsLValueRef? RQ_LValue
 : RQ_RValue;
@@ -5026,11 +5027,11 @@
 assert(Chunk.Kind == DeclaratorChunk::Function);
 if (Chunk.Fun.hasRefQualifier())
   RemovalLocs.push_back(Chunk.Fun.getRefQualifierLoc());
-if (Chunk.Fun.TypeQuals & Qualifiers::Const)
+if (Chunk.Fun.hasConstQualifier())
   RemovalLocs.push_back(Chunk.Fun.getConstQualifierLoc());
-if (Chunk.Fun.TypeQuals & Qualifiers::Volatile)
+if (Chunk.Fun.hasVolatileQualifier())
   RemovalLocs.push_back(Chunk.Fun.getVolatileQualifierLoc());
-if (Chunk.Fun.TypeQuals & Qualifiers::Restrict)
+if (Chunk.Fun.hasRestrictQualifier())
   RemovalLocs.push_back(Chunk.Fun.getRestrictQualifierLoc());
 if (!RemovalLocs.empty()) {
   llvm::sort(RemovalLocs,
Index: lib/Sema/SemaLambda.cpp
===
--- lib/Sema/SemaLambda.cpp
+++ lib/Sema/SemaLambda.cpp
@@ -884,8 +884,11 @@
 //   This function call operator is declared const (9.3.1) if and only if
 //   the lambda-expression's parameter-declaration-clause is not followed
 //   by mutable. It is neither virtual nor declared volatile. [...]
-if (!FTI.hasMutableQualifier())
-  FTI.TypeQuals |= DeclSpec::TQ_const;
+if (!FTI.hasMutableQualifier()) {
+  if (!FTI.MethodQualifiers)
+FTI.createMethodQualifiers();
+  FTI.MethodQualifiers->SetTypeQual(DeclSpec::TQ_const, SourceLocation());
+}
 
 MethodTyInfo = GetTypeForDeclarator(ParamInfo, CurScope);
 assert(MethodTyInfo && "no type from lambda-declarator");
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -8172,16 +8172,16 @@
   }
 
   DeclaratorChunk::FunctionTypeInfo &FTI = D.getFunctionTypeInfo();
-  if (FTI.TypeQuals != 0) {
-if (FTI.TypeQuals & Qualifiers::Const)
+  if (FTI.MethodQualifiers && FTI.MethodQualifiers->getTypeQualifiers() != 0) {
+if (FTI.hasConstQualifier())
   Diag(D.getIdentifierLoc(), diag::err_invalid_qualified_constructor)
-<< "const" << SourceRange(D.getIdentifierLoc());
-if (FTI.Type

[PATCH] D55948: Modify DeclaratorChuck::getFunction to use DeclSpec for qualifiers

2018-12-24 Thread Anastasia Stulova via Phabricator via cfe-commits 
Anastasia marked 3 inline comments as done.
Anastasia added inline comments.



Comment at: lib/Parse/ParseDecl.cpp:6129
+  // create DeclSpec here to be populated later.
+  DS = new DeclSpec(AttrFactory);
+

rjmccall wrote:
> Is it possible to just build into a local DS and then move that DS to the 
> heap if it contains qualifiers or attributes?  It'd be nice to not do a heap 
> allocation every time we parse a function declarator in C++ since the vast 
> majority of them don't have qualifiers.  (It probably has to be a move to 
> handle attributes and `AttributeFactory` correctly.)
> 
> That would also side-step any concerns about memory management.
I found `DeclSpec` a bit copy-unfriednly... not sure it was better to modify 
its interface. For now I just provide custom impl for function quals.


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

https://reviews.llvm.org/D55948



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

[PATCH] D56025: [clang-tidy] add IgnoreMacros option to readability-uppercase-literal-suffix

2018-12-24 Thread Jonas Toth via Phabricator via cfe-commits 
JonasToth accepted this revision.
JonasToth added a comment.

LGTM, do you have commit rights?


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

https://reviews.llvm.org/D56025



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

[PATCH] D55433: [clang-tidy] Adding a new modernize use nodiscard checker

2018-12-24 Thread Jonas Toth via Phabricator via cfe-commits 
JonasToth added a comment.

Sorry for my absence the last week, holidays came in between.
I will take a look at the code again and try to find a robuster way of 
template-type detection :) I do have an idea, but not sure if that works as 
expected (within this week).

Sorry again!


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

https://reviews.llvm.org/D55433



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

[PATCH] D56059: [gn build] Add build files for clang/tools/{c-arcmt-test, c-index-test} and their dependency clang/tools/libclang

2018-12-24 Thread Phabricator via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL350054: [gn build] Add build files for 
clang/tools/{c-arcmt-test,c-index-test} and… (authored by nico, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56059?vs=179446&id=179472#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56059

Files:
  llvm/trunk/utils/gn/secondary/BUILD.gn
  llvm/trunk/utils/gn/secondary/clang/tools/c-arcmt-test/BUILD.gn
  llvm/trunk/utils/gn/secondary/clang/tools/c-index-test/BUILD.gn
  llvm/trunk/utils/gn/secondary/clang/tools/libclang/BUILD.gn

Index: llvm/trunk/utils/gn/secondary/clang/tools/libclang/BUILD.gn
===
--- llvm/trunk/utils/gn/secondary/clang/tools/libclang/BUILD.gn
+++ llvm/trunk/utils/gn/secondary/clang/tools/libclang/BUILD.gn
@@ -0,0 +1,84 @@
+import("//clang/lib/ARCMigrate/enable.gni")
+import("//llvm/version.gni")
+
+# This build file is just enough to get check-clang to pass, it's missing
+# several things from the CMake build:
+# - linking in clangTidyPlugin and clangIncludeFixerPlugin from
+#   clang-tools-extra (which doesn't have any GN build files yet)
+# - using libclang.exports
+# - an build target copying the Python bindings
+# - the GN linux build always builds without -fPIC (as if LLVM_ENABLE_PIC=OFF
+#   in the CMake build), so libclang is always a static library on linux
+# - the GN build doesn't have LIBCLANG_BUILD_STATIC
+
+libclang_target_type = "shared_library"
+if (host_os == "linux") {
+  # Linux needs -fPIC to build shared libs but they aren't on by default.
+  # For now, make libclang a static lib there.
+  libclang_target_type = "static_library"
+}
+
+target(libclang_target_type, "libclang") {
+  configs += [ "//llvm/utils/gn/build:clang_code" ]
+  deps = [
+"//clang/include/clang/Config",
+"//clang/lib/AST",
+"//clang/lib/Basic",
+"//clang/lib/Frontend",
+"//clang/lib/Headers",
+"//clang/lib/Index",
+"//clang/lib/Lex",
+"//clang/lib/Sema",
+"//clang/lib/Tooling",
+"//llvm/include/llvm/Config:llvm-config",
+"//llvm/lib/IR",
+"//llvm/lib/Support",
+"//llvm/lib/Target:TargetsToBuild",
+  ]
+  if (clang_enable_arcmt) {
+deps += [ "//clang/lib/ARCMigrate" ]
+  }
+
+  if (host_os == "win") {
+defines = [ "_CINDEX_LIB_" ]
+  }
+
+  sources = [
+"ARCMigrate.cpp",
+"BuildSystem.cpp",
+"CIndex.cpp",
+"CIndexCXX.cpp",
+"CIndexCodeCompletion.cpp",
+"CIndexDiagnostic.cpp",
+"CIndexDiagnostic.h",
+"CIndexHigh.cpp",
+"CIndexInclusionStack.cpp",
+"CIndexUSRs.cpp",
+"CIndexer.cpp",
+"CIndexer.h",
+"CXComment.cpp",
+"CXCompilationDatabase.cpp",
+"CXCursor.cpp",
+"CXCursor.h",
+"CXIndexDataConsumer.cpp",
+"CXLoadedDiagnostic.cpp",
+"CXLoadedDiagnostic.h",
+"CXSourceLocation.cpp",
+"CXSourceLocation.h",
+"CXStoredDiagnostic.cpp",
+"CXString.cpp",
+"CXString.h",
+"CXTranslationUnit.h",
+"CXType.cpp",
+"CXType.h",
+"Index_Internal.h",
+"Indexing.cpp",
+  ]
+  if (host_os == "mac") {
+ldflags = [
+  "-Wl,-compatibility_version,1",
+  "-Wl,-current_version,$llvm_version",
+  "-Wl,-install_name,@rpath/libclang.dylib",
+]
+  }
+}
Index: llvm/trunk/utils/gn/secondary/clang/tools/c-index-test/BUILD.gn
===
--- llvm/trunk/utils/gn/secondary/clang/tools/c-index-test/BUILD.gn
+++ llvm/trunk/utils/gn/secondary/clang/tools/c-index-test/BUILD.gn
@@ -0,0 +1,28 @@
+executable("c-index-test") {
+  configs += [ "//llvm/utils/gn/build:clang_code" ]
+  deps = [
+"//clang/include/clang/Config",
+"//clang/lib/AST",
+"//clang/lib/Basic",
+"//clang/lib/CodeGen",
+"//clang/lib/Frontend",
+"//clang/lib/Index",
+"//clang/lib/Serialization",
+"//clang/tools/libclang",
+"//llvm/lib/Support",
+"//llvm/utils/gn/build/libs/xml",
+  ]
+  if (host_os != "win") {
+cflags_c = [ "-std=gnu89" ]
+  }
+  sources = [
+"c-index-test.c",
+"core_main.cpp",
+  ]
+
+  # See comment at top of clang/tools/libclang/BUILD.gn for why this isn't
+  # needed on Linux.
+  if (host_os == "mac") {
+ldflags = [ "-Wl,-rpath,@loader_path/../lib" ]
+  }
+}
Index: llvm/trunk/utils/gn/secondary/clang/tools/c-arcmt-test/BUILD.gn
===
--- llvm/trunk/utils/gn/secondary/clang/tools/c-arcmt-test/BUILD.gn
+++ llvm/trunk/utils/gn/secondary/clang/tools/c-arcmt-test/BUILD.gn
@@ -0,0 +1,15 @@
+executable("c-arcmt-test") {
+  configs += [ "//llvm/utils/gn/build:clang_code" ]
+  deps = [
+"//clang/tools/libclang",
+  ]
+  sources = [
+"c-arcmt-test.c",
+  ]
+
+  # See comment at top of clang/tools/libclang/BUILD.gn for why this isn't
+  # needed on Linux.
+  if (host_o

[PATCH] D56059: [gn build] Add build files for clang/tools/{c-arcmt-test, c-index-test} and their dependency clang/tools/libclang

2018-12-24 Thread Nico Weber via Phabricator via cfe-commits 
thakis marked an inline comment as done.
thakis added a comment.

Thanks




Comment at: llvm/utils/gn/secondary/clang/tools/libclang/BUILD.gn:16-17
+if (host_os == "linux") {
+  # Linux needs -fPIC to build shared libs but they aren't on by default.
+  # For now, make libclang a static lib there.
+  libclang_target_type = "static_library"

phosek wrote:
> We can solve this with toolchains, i.e. have a variant of the default 
> toolchain that includes `-fPIC` which will be used to build `libClang.so`. 
> Not something to do in this change, but definitely something we should look 
> into in the future.
Yup! I think we want several mechanisms:
- llvm_enable_pic, like the cmake build. It should probably default to false
- a toolchain setup, for people who want to redist clang without pic but 
libclang with pic. This makes building that convenient, but the pic difference 
means the one build dir will have to build more or less everything twice (once 
per toolchain), which for regular development is a bit annoying because it 
takes a bit longer.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56059



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

[PATCH] D56065: [gn build] Make NOSORT line actually work

2018-12-24 Thread Nico Weber via Phabricator via cfe-commits 
thakis created this revision.
thakis added a reviewer: phosek.

GN wants the NOSORT line to be the first line of a comment block, not the last 
line.

I sent https://gn-review.googlesource.com/c/gn/+/3560 to support having it in 
the last line too, but since it will be a while until everyone has that change 
even if it's expected, use the form that works today.


https://reviews.llvm.org/D56065

Files:
  llvm/utils/gn/secondary/clang/lib/Headers/BUILD.gn


Index: llvm/utils/gn/secondary/clang/lib/Headers/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/lib/Headers/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/Headers/BUILD.gn
@@ -34,9 +34,9 @@
 ":arm_headers",
   ]
 
+  # NOSORT
   # Tell `gn format` to not reorder the sources list: Its order matches CMake,
   # and the ordering is alphabetical but ignores leading underscores.
-  # NOSORT
   sources = [
 "adxintrin.h",
 "altivec.h",


Index: llvm/utils/gn/secondary/clang/lib/Headers/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/lib/Headers/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/Headers/BUILD.gn
@@ -34,9 +34,9 @@
 ":arm_headers",
   ]
 
+  # NOSORT
   # Tell `gn format` to not reorder the sources list: Its order matches CMake,
   # and the ordering is alphabetical but ignores leading underscores.
-  # NOSORT
   sources = [
 "adxintrin.h",
 "altivec.h",
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55955: Properly diagnose [[nodiscard]] on the body of a range-based for loop

2018-12-24 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman updated this revision to Diff 179475.
aaron.ballman marked 3 inline comments as done.
aaron.ballman added a comment.

Updated based on review feedback.

The lookahead wasn't too annoying to thread through, but I did run into 
surprises in TreeTransform where I can't use the lookahead to determine whether 
the expression statement should be warned on or not. The solution I have is 
serviceable, but perhaps there's a better approach to be taken.

The changes introduce warnings where they were previously missed, such as in 
init-statements. The only unfortunate behavioral side effect comes from 
range-based for loops that have an expression as the first statement -- in 
addition to the error, we now trigger an unused expression result warning, but 
restructuring the code to avoid it would be challenging for such an edge case 
(and the warning behavior seems reasonable).


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

https://reviews.llvm.org/D55955

Files:
  include/clang/Parse/Parser.h
  include/clang/Sema/Sema.h
  lib/Parse/ParseObjc.cpp
  lib/Parse/ParseOpenMP.cpp
  lib/Parse/ParseStmt.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaOpenMP.cpp
  lib/Sema/SemaStmt.cpp
  lib/Sema/TreeTransform.h
  test/CXX/stmt.stmt/stmt.select/p3.cpp
  test/CodeCompletion/pragma-macro-token-caching.c
  test/Parser/cxx1z-init-statement.cpp
  test/Parser/switch-recovery.cpp
  test/SemaCXX/cxx1z-init-statement.cpp
  test/SemaCXX/for-range-examples.cpp
  test/SemaCXX/warn-unused-result.cpp
  test/SemaObjCXX/foreach.mm

Index: test/SemaObjCXX/foreach.mm
===
--- test/SemaObjCXX/foreach.mm
+++ test/SemaObjCXX/foreach.mm
@@ -6,8 +6,8 @@
 void f(NSArray *a) {
 id keys;
 for (int i : a); // expected-error{{selector element type 'int' is not a valid object}} 
-for ((id)2 : a);  // expected-error {{for range declaration must declare a variable}}
-for (2 : a); // expected-error {{for range declaration must declare a variable}}
+for ((id)2 : a);  // expected-error {{for range declaration must declare a variable}} expected-warning {{expression result unused}}
+for (2 : a); // expected-error {{for range declaration must declare a variable}} expected-warning {{expression result unused}}
   
   for (id thisKey : keys);
 
@@ -71,7 +71,7 @@
 @end
 void test2(NSObject *collection) {
   Test2 *obj;
-  for (obj.prop : collection) { // expected-error {{for range declaration must declare a variable}}
+  for (obj.prop : collection) { // expected-error {{for range declaration must declare a variable}} expected-warning {{property access result unused}}
   }
 }
 
Index: test/SemaCXX/warn-unused-result.cpp
===
--- test/SemaCXX/warn-unused-result.cpp
+++ test/SemaCXX/warn-unused-result.cpp
@@ -33,6 +33,36 @@
   const S &s4 = g1();
 }
 
+void testSubstmts(int i) {
+  switch (i) {
+  case 0:
+f(); // expected-warning {{ignoring return value}}
+  default:
+f(); // expected-warning {{ignoring return value}}
+  }
+
+  if (i)
+f(); // expected-warning {{ignoring return value}}
+  else
+f(); // expected-warning {{ignoring return value}}
+
+  while (i)
+f(); // expected-warning {{ignoring return value}}
+
+  do
+f(); // expected-warning {{ignoring return value}}
+  while (i);
+
+  for (f(); // expected-warning {{ignoring return value}}
+   ;
+   f() // expected-warning {{ignoring return value}}
+  )
+f(); // expected-warning {{ignoring return value}}
+
+  f(),  // expected-warning {{ignoring return value}}
+  (void)f();
+}
+
 struct X {
  int foo() __attribute__((warn_unused_result));
 };
@@ -206,3 +236,13 @@
   (void)++p;
 }
 } // namespace
+
+namespace PR39837 {
+[[clang::warn_unused_result]] int f(int);
+
+void g() {
+  int a[2];
+  for (int b : a)
+f(b); // expected-warning {{ignoring return value}}
+}
+} // namespace PR39837
Index: test/SemaCXX/for-range-examples.cpp
===
--- test/SemaCXX/for-range-examples.cpp
+++ test/SemaCXX/for-range-examples.cpp
@@ -176,9 +176,9 @@
 
 // Make sure these don't crash. Better diagnostics would be nice.
 for (: {1, 2, 3}) {} // expected-error {{expected expression}} expected-error {{expected ';'}}
-for (1 : {1, 2, 3}) {} // expected-error {{must declare a variable}}
+for (1 : {1, 2, 3}) {} // expected-error {{must declare a variable}} expected-warning {{expression result unused}}
 for (+x : {1, 2, 3}) {} // expected-error {{undeclared identifier}} expected-error {{expected ';'}}
-for (+y : {1, 2, 3}) {} // expected-error {{must declare a variable}}
+for (+y : {1, 2, 3}) {} // expected-error {{must declare a variable}} expected-warning {{expression result unused}}
   }
 }
 
@@ -244,7 +244,7 @@
 { 
   int b = 1, a[b];
   a[0] = 0;
-  [&] { for (int c : a) 0; } ();
+  [&] { for (int c : a) 0; } (); // expected-wa

[clang-tools-extra] r350056 - [clang-tidy] add IgnoreMacros option to readability-uppercase-literal-suffix

2018-12-24 Thread Miklos Vajna via cfe-commits 
Author: vmiklos
Date: Mon Dec 24 09:47:32 2018
New Revision: 350056

URL: http://llvm.org/viewvc/llvm-project?rev=350056&view=rev
Log:
[clang-tidy] add IgnoreMacros option to readability-uppercase-literal-suffix

And also enable it by default to be consistent with e.g.
modernize-use-using.

This helps e.g. when running this check on client code where the macro
is provided by the system, so there is no easy way to modify it.

Reviewed By: JonasToth, lebedev.ri

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

Modified:

clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.h
clang-tools-extra/trunk/docs/ReleaseNotes.rst

clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uppercase-literal-suffix.rst

clang-tools-extra/trunk/test/clang-tidy/readability-uppercase-literal-suffix-integer-macro.cpp

clang-tools-extra/trunk/test/clang-tidy/readability-uppercase-literal-suffix-integer.cpp

Modified: 
clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp?rev=350056&r1=350055&r2=350056&view=diff
==
--- 
clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp 
(original)
+++ 
clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp 
Mon Dec 24 09:47:32 2018
@@ -183,12 +183,14 @@ UppercaseLiteralSuffixCheck::UppercaseLi
 StringRef Name, ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
   NewSuffixes(
-  utils::options::parseStringList(Options.get("NewSuffixes", ""))) {}
+  utils::options::parseStringList(Options.get("NewSuffixes", ""))),
+  IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", 1) != 0) {}
 
 void UppercaseLiteralSuffixCheck::storeOptions(
 ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "NewSuffixes",
 utils::options::serializeStringList(NewSuffixes));
+  Options.store(Opts, "IgnoreMacros", IgnoreMacros);
 }
 
 void UppercaseLiteralSuffixCheck::registerMatchers(MatchFinder *Finder) {
@@ -216,6 +218,8 @@ bool UppercaseLiteralSuffixCheck::checkB
   // We might have a suffix that is already uppercase.
   if (auto Details = shouldReplaceLiteralSuffix(
   *Literal, NewSuffixes, *Result.SourceManager, getLangOpts())) {
+if (Details->LiteralLocation.getBegin().isMacroID() && IgnoreMacros)
+  return true;
 auto Complaint = diag(Details->LiteralLocation.getBegin(),
   "%0 literal has suffix '%1', which is not uppercase")
  << LiteralType::Name << Details->OldSuffix;

Modified: 
clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.h?rev=350056&r1=350055&r2=350056&view=diff
==
--- 
clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.h 
(original)
+++ 
clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.h 
Mon Dec 24 09:47:32 2018
@@ -35,6 +35,7 @@ private:
   bool checkBoundMatch(const ast_matchers::MatchFinder::MatchResult &Result);
 
   const std::vector NewSuffixes;
+  const bool IgnoreMacros;
 };
 
 } // namespace readability

Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=350056&r1=350055&r2=350056&view=diff
==
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Mon Dec 24 09:47:32 2018
@@ -236,6 +236,10 @@ Improvements to clang-tidy
   ` check does not warn
   about calls inside macros anymore by default.
 
+- The :doc:`readability-uppercase-literal-suffix
+  ` check does not warn
+  about literal suffixes inside macros anymore by default.
+
 - The :doc:`cppcoreguidelines-narrowing-conversions
   ` check now
   detects more narrowing conversions:

Modified: 
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uppercase-literal-suffix.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uppercase-literal-suffix.rst?rev=350056&r1=350055&r2=350056&view=diff
==
--- 
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uppercase-literal-suffix.rst
 (original)
+++ 
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uppercase-literal-suffix.rst
 Mon Dec 24 09:47:32 2018
@@ -49,3 +49,8 @@ Given a list `L;uL`:
 * ``uL`` will be kept as is.
 * `

[PATCH] D56025: [clang-tidy] add IgnoreMacros option to readability-uppercase-literal-suffix

2018-12-24 Thread Miklos Vajna via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
vmiklos marked an inline comment as done.
Closed by commit rL350056: [clang-tidy] add IgnoreMacros option to 
readability-uppercase-literal-suffix (authored by vmiklos, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56025?vs=179371&id=179476#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56025

Files:
  clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
  clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uppercase-literal-suffix.rst
  
clang-tools-extra/trunk/test/clang-tidy/readability-uppercase-literal-suffix-integer-macro.cpp
  
clang-tools-extra/trunk/test/clang-tidy/readability-uppercase-literal-suffix-integer.cpp


Index: 
clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
===
--- 
clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
+++ 
clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
@@ -183,12 +183,14 @@
 StringRef Name, ClangTidyContext *Context)
 : ClangTidyCheck(Name, Context),
   NewSuffixes(
-  utils::options::parseStringList(Options.get("NewSuffixes", ""))) {}
+  utils::options::parseStringList(Options.get("NewSuffixes", ""))),
+  IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", 1) != 0) {}
 
 void UppercaseLiteralSuffixCheck::storeOptions(
 ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "NewSuffixes",
 utils::options::serializeStringList(NewSuffixes));
+  Options.store(Opts, "IgnoreMacros", IgnoreMacros);
 }
 
 void UppercaseLiteralSuffixCheck::registerMatchers(MatchFinder *Finder) {
@@ -216,6 +218,8 @@
   // We might have a suffix that is already uppercase.
   if (auto Details = shouldReplaceLiteralSuffix(
   *Literal, NewSuffixes, *Result.SourceManager, getLangOpts())) {
+if (Details->LiteralLocation.getBegin().isMacroID() && IgnoreMacros)
+  return true;
 auto Complaint = diag(Details->LiteralLocation.getBegin(),
   "%0 literal has suffix '%1', which is not uppercase")
  << LiteralType::Name << Details->OldSuffix;
Index: 
clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.h
+++ clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.h
@@ -35,6 +35,7 @@
   bool checkBoundMatch(const ast_matchers::MatchFinder::MatchResult &Result);
 
   const std::vector NewSuffixes;
+  const bool IgnoreMacros;
 };
 
 } // namespace readability
Index: clang-tools-extra/trunk/docs/ReleaseNotes.rst
===
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst
@@ -236,6 +236,10 @@
   ` check does not warn
   about calls inside macros anymore by default.
 
+- The :doc:`readability-uppercase-literal-suffix
+  ` check does not warn
+  about literal suffixes inside macros anymore by default.
+
 - The :doc:`cppcoreguidelines-narrowing-conversions
   ` check now
   detects more narrowing conversions:
Index: 
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uppercase-literal-suffix.rst
===
--- 
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uppercase-literal-suffix.rst
+++ 
clang-tools-extra/trunk/docs/clang-tidy/checks/readability-uppercase-literal-suffix.rst
@@ -49,3 +49,8 @@
 * ``uL`` will be kept as is.
 * ``ull`` will be kept as is, since it is not in the list
 * and so on.
+
+.. option:: IgnoreMacros
+
+   If this option is set to non-zero (default is `1`), the check will not warn
+   about literal suffixes inside macros.
Index: 
clang-tools-extra/trunk/test/clang-tidy/readability-uppercase-literal-suffix-integer-macro.cpp
===
--- 
clang-tools-extra/trunk/test/clang-tidy/readability-uppercase-literal-suffix-integer-macro.cpp
+++ 
clang-tools-extra/trunk/test/clang-tidy/readability-uppercase-literal-suffix-integer-macro.cpp
@@ -1,4 +1,6 @@
-// RUN: %check_clang_tidy %s readability-uppercase-literal-suffix %t -- -- -I 
%S
+// RUN: %check_clang_tidy %s readability-uppercase-literal-suffix %t -- \
+// RUN:   -config="{CheckOptions: [{key: 
readability-uppercase-literal-suffix.IgnoreMacros, value: 0}]}" \
+// RUN:   -- -I %S
 
 void macros() {
 #define INMACRO(X) 1.f
Index: 
clang-tools-extra/trunk/test/clang-tidy/readability-upp

[PATCH] D56025: [clang-tidy] add IgnoreMacros option to readability-uppercase-literal-suffix

2018-12-24 Thread Miklos Vajna via Phabricator via cfe-commits 
vmiklos added a comment.

In D56025#1340618 , @JonasToth wrote:

> LGTM, do you have commit rights?


Yes, thanks. Committed now.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56025



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

[PATCH] D55955: Properly diagnose [[nodiscard]] on the body of a range-based for loop

2018-12-24 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added inline comments.



Comment at: include/clang/Sema/Sema.h:5337
   ExprResult ActOnFinishFullExpr(Expr *Expr, SourceLocation CC,
- bool DiscardedValue = false,
+ bool WarnOnDiscardedValue = false,
  bool IsConstexpr = false);

Why "WarnOn"? Shouldn't this flag simply indicate whether the expression is a 
discarded-value expression?



Comment at: lib/Parse/ParseStmt.cpp:23
 #include "clang/Sema/Scope.h"
+#include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/TypoCorrection.h"

The parser shouldn't be looking inside Sema's data structures. Can you add a 
method to the Sema interface for the query you make below instead?



Comment at: lib/Sema/SemaExprCXX.cpp:7785
 
-  if (DiscardedValue) {
+  if (WarnOnDiscardedValue) {
 // Top-level expressions default to 'id' when we're in a debugger.

It doesn't make sense for a WarnOn flag to control how we build the AST like 
this.



Comment at: lib/Sema/TreeTransform.h:3297
 
-  return getSema().ActOnExprStmt(E);
+  return getSema().ActOnExprStmt(E, SuppressWarnOnUnusedExpr == 0);
 }

What happens if the last statement in an expression statement contains a 
lambda? Will we consider all expression statements in it as not being 
discarded-value expressions? Alternate suggestion below.



Comment at: lib/Sema/TreeTransform.h:6529
+  --SuppressWarnOnUnusedExpr;
+
 if (Result.isInvalid()) {

Instead of this counter mechanism (which doesn't seem reliable), can you just 
pass a flag to TransformStmt?


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

https://reviews.llvm.org/D55955



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

[PATCH] D56066: [OpenCL] Address space for default class members

2018-12-24 Thread Anastasia Stulova via Phabricator via cfe-commits 
Anastasia created this revision.
Anastasia added reviewers: rjmccall, ebevhan.
Herald added a subscriber: yaxunl.

Fixed several issues reported in https://reviews.llvm.org/D55656 and 
https://reviews.llvm.org/D54862 - mainly removed addr space from the `QualType` 
of the method itself.

This should make all implicitly and explicity defaulted special class members 
work now!

Since Mikael is on holiday and I have no permission to upload to his review, I 
have to create a new one.


https://reviews.llvm.org/D56066

Files:
  include/clang/Sema/Sema.h
  lib/CodeGen/CGCall.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaInit.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenOpenCLCXX/addrspace-of-this.cl
  test/SemaOpenCLCXX/address-space-templates.cl

Index: test/SemaOpenCLCXX/address-space-templates.cl
===
--- test/SemaOpenCLCXX/address-space-templates.cl
+++ test/SemaOpenCLCXX/address-space-templates.cl
@@ -2,11 +2,10 @@
 
 template 
 struct S {
-  T a;// expected-error{{field may not be qualified with an address space}}
-  T f1(); // expected-error{{function type may not be qualified with an address space}}
+  T a;// expected-error{{field may not be qualified with an address space}}
+  T f1(); // expected-error{{function type may not be qualified with an address space}}
   // FIXME: Should only get the error message once.
-  void f2(T); // expected-error{{parameter may not be qualified with an address space}} expected-error{{parameter may not be qualified with an address space}}
-
+  void f2(T); // expected-error{{parameter may not be qualified with an address space}}
 };
 
 template 
Index: test/CodeGenOpenCLCXX/addrspace-of-this.cl
===
--- test/CodeGenOpenCLCXX/addrspace-of-this.cl
+++ test/CodeGenOpenCLCXX/addrspace-of-this.cl
@@ -9,18 +9,21 @@
 public:
   int v;
   C() { v = 2; }
-  // FIXME: Does not work yet.
-  // C(C &&c) { v = c.v; }
+  C(C &&c) { v = c.v; }
   C(const C &c) { v = c.v; }
   C &operator=(const C &c) {
 v = c.v;
 return *this;
   }
-  // FIXME: Does not work yet.
-  //C &operator=(C&& c) & {
-  //  v = c.v;
-  //  return *this;
-  //}
+  C &operator=(C &&c) & {
+v = c.v;
+return *this;
+  }
+
+  C operator+(const C &c) {
+v += c.v;
+return *this;
+  }
 
   int get() { return v; }
 
@@ -41,15 +44,13 @@
   C c1(c);
   C c2;
   c2 = c1;
-  // FIXME: Does not work yet.
-  // C c3 = c1 + c2;
-  // C c4(foo());
-  // C c5 = foo();
-
+  C c3 = c1 + c2;
+  C c4(foo());
+  C c5 = foo();
 }
 
 // CHECK-LABEL: @__cxx_global_var_init()
-// CHECK: call void @_ZNU3AS41CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*)) #4
+// CHECK: call void @_ZNU3AS41CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
 
 // Test that the address space is __generic for the constructor
 // CHECK-LABEL: @_ZNU3AS41CC1Ev(%class.C addrspace(4)* %this)
@@ -57,7 +58,7 @@
 // CHECK:   %this.addr = alloca %class.C addrspace(4)*, align 4
 // CHECK:   store %class.C addrspace(4)* %this, %class.C addrspace(4)** %this.addr, align 4
 // CHECK:   %this1 = load %class.C addrspace(4)*, %class.C addrspace(4)** %this.addr, align 4
-// CHECK:   call void @_ZNU3AS41CC2Ev(%class.C addrspace(4)* %this1) #4
+// CHECK:   call void @_ZNU3AS41CC2Ev(%class.C addrspace(4)* %this1)
 // CHECK:   ret void
 
 // CHECK-LABEL: @_Z12test__globalv()
@@ -74,13 +75,28 @@
 
 // Test the address space of 'this' when invoking a constructor.
 // CHECK:   %1 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
-// CHECK:   call void @_ZNU3AS41CC1Ev(%class.C addrspace(4)* %1) #4
+// CHECK:   call void @_ZNU3AS41CC1Ev(%class.C addrspace(4)* %1)
 
 // Test the address space of 'this' when invoking assignment operator.
 // CHECK:   %2 = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
 // CHECK:   %3 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
 // CHECK:   %call2 = call dereferenceable(4) %class.C addrspace(4)* @_ZNU3AS41CaSERU3AS4KS_(%class.C addrspace(4)* %3, %class.C addrspace(4)* dereferenceable(4) %2)
 
+// Test the address space of 'this' when invoking the operator+
+// CHECK:   %4 = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
+// CHECK:   %5 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK:   call void @_ZNU3AS41CplERU3AS4KS_(%class.C* sret %c3, %class.C addrspace(4)* %4, %class.C addrspace(4)* dereferenceable(4) %5)
+
+// Test the address space of 'this' when invoking the move constructor
+// CHECK:   %6 = addrspacecast %class.C* %c4 to %class.C addrspace(4)*
+// CHECK:   %call3 = call spir_func dereferenceable(4) %class.C addrspace(4)* @_Z3foov()
+// CHECK:   call void @_ZNU3AS41CC1EOU3AS4S_(%class.C addrspace(4)* %6, %class.C addrspace(4)* dereferenceable(4) %call3)
+
+// Test the address space of 'this' when invoking the move assignm

[PATCH] D55656: [OpenCL] Address space for default class members

2018-12-24 Thread Anastasia Stulova via Phabricator via cfe-commits 
Anastasia added inline comments.



Comment at: lib/AST/ASTContext.cpp:2781
+
+  return getAddrSpaceQualType(NewT, Orig.getAddressSpace());
 }

ebevhan wrote:
> rjmccall wrote:
> > Anastasia wrote:
> > > rjmccall wrote:
> > > > You're trying to handle a method qualifier, not a type a functions that 
> > > > are themselves in some non-standard address space, right?  The method 
> > > > qualifier should already be part of `Proto->getExtProtoInfo()`, so if 
> > > > there's an address space qualifier out here, something is very wrong.
> > > As far as I understand the new design, we have an address space qualifier 
> > > on a method and as a part of the function prototype too. Are you saying 
> > > that we need to make sure the prototype has an address space too?
> > The address-space `this` qualifiers are a part of `FunctionProtoType` 
> > that's totally independent from the storage of top-level qualifiers that's 
> > part of `QualType`.  `Orig.getAddressSpace()` is asking about the top-level 
> > qualifiers, not the `this` qualifiers.  Generally, function types should 
> > not have top-level qualifiers at all (although of course a member pointer 
> > can have both top-level qualifiers and `this` qualifiers, with the former 
> > meaning something completely different from the latter).
> I suspect the reason there has to be qualifiers on the function type itself 
> is because of the `getType` mistake I mentioned in the previous patch. If the 
> `this` address space qualifiers aren't also on the function type, then 
> `GetThisType` will not produce the correctly qualified type.
> 
> If the `getType` in `GetThisType` is changed to `getTypeQualifiers`, then the 
> function type should not need any qualifiers.
I fixed the issues reported here and in https://reviews.llvm.org/D54862 on the 
following review: https://reviews.llvm.org/D56066.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55656



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

[PATCH] D55656: [OpenCL] Address space for default class members

2018-12-24 Thread Anastasia Stulova via Phabricator via cfe-commits 
Anastasia requested changes to this revision.
Anastasia added a comment.
This revision now requires changes to proceed.

@mikael can you please close this review, because I opened another one to 
continue this: https://reviews.llvm.org/D56066.

Thanks for your work!


Repository:
  rC Clang

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

https://reviews.llvm.org/D55656



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

r350057 - [analyzer] [NFC] Clean up the mess of constructing argument effects in RetainCountChecker

2018-12-24 Thread George Karpenkov via cfe-commits 
Author: george.karpenkov
Date: Mon Dec 24 10:45:18 2018
New Revision: 350057

URL: http://llvm.org/viewvc/llvm-project?rev=350057&view=rev
Log:
[analyzer] [NFC] Clean up the mess of constructing argument effects in 
RetainCountChecker

Previously, argument effects were stored in a method variable, which was
effectively global.
The global state was reset at each (hopefully) entrance point to the
summary construction,
and every function could modify it.

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

Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp

Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/RetainSummaryManager.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/RetainSummaryManager.h?rev=350057&r1=350056&r2=350057&view=diff
==
--- cfe/trunk/include/clang/StaticAnalyzer/Core/RetainSummaryManager.h 
(original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/RetainSummaryManager.h Mon Dec 
24 10:45:18 2018
@@ -508,9 +508,6 @@ class RetainSummaryManager {
   /// AF - A factory for ArgEffects objects.
   ArgEffects::Factory AF;
 
-  /// ScratchArgs - A holding buffer for construct ArgEffects.
-  ArgEffects ScratchArgs;
-
   /// ObjCAllocRetE - Default return effect for methods returning Objective-C
   ///  objects.
   RetEffect ObjCAllocRetE;
@@ -523,10 +520,6 @@ class RetainSummaryManager {
   /// effects.
   llvm::FoldingSet SimpleSummaries;
 
-  /// getArgEffects - Returns a persistent ArgEffects object based on the
-  ///  data in ScratchArgs.
-  ArgEffects getArgEffects();
-
   /// Create an OS object at +1.
   const RetainSummary *getOSSummaryCreateRule(const FunctionDecl *FD);
 
@@ -554,25 +547,30 @@ class RetainSummaryManager {
   const RetainSummary *getPersistentSummary(const RetainSummary &OldSumm);
 
   const RetainSummary *getPersistentSummary(RetEffect RetEff,
+ArgEffects ScratchArgs,
 ArgEffect ReceiverEff = DoNothing,
 ArgEffect DefaultEff = MayEscape,
 ArgEffect ThisEff = DoNothing) {
-RetainSummary Summ(getArgEffects(), RetEff, DefaultEff, ReceiverEff,
+RetainSummary Summ(ScratchArgs, RetEff, DefaultEff, ReceiverEff,
ThisEff);
 return getPersistentSummary(Summ);
   }
 
   const RetainSummary *getDoNothingSummary() {
-return getPersistentSummary(RetEffect::MakeNoRet(), DoNothing, DoNothing);
+return getPersistentSummary(RetEffect::MakeNoRet(),
+ArgEffects(AF.getEmptyMap()),
+DoNothing, DoNothing);
   }
 
   const RetainSummary *getDefaultSummary() {
 return getPersistentSummary(RetEffect::MakeNoRet(),
+ArgEffects(AF.getEmptyMap()),
 DoNothing, MayEscape);
   }
 
   const RetainSummary *getPersistentStopSummary() {
 return getPersistentSummary(RetEffect::MakeNoRet(),
+ArgEffects(AF.getEmptyMap()),
 StopTracking, StopTracking);
   }
 
@@ -649,7 +647,6 @@ class RetainSummaryManager {
   bool applyFunctionParamAnnotationEffect(const ParmVarDecl *pd,
 unsigned parm_idx,
 const FunctionDecl *FD,
-ArgEffects::Factory &AF,
 RetainSummaryTemplate &Template);
 
 public:
@@ -661,7 +658,7 @@ public:
  ARCEnabled(usesARC),
  TrackObjCAndCFObjects(trackObjCAndCFObjects),
  TrackOSObjects(trackOSObjects),
- AF(BPAlloc), ScratchArgs(AF.getEmptyMap()),
+ AF(BPAlloc),
  ObjCAllocRetE(usesARC ? RetEffect::MakeNotOwned(RetEffect::ObjC)
: RetEffect::MakeOwned(RetEffect::ObjC)),
  ObjCInitRetE(usesARC ? RetEffect::MakeNotOwned(RetEffect::ObjC)

Modified: cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp?rev=350057&r1=350056&r2=350057&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp Mon Dec 24 
10:45:18 2018
@@ -49,12 +49,6 @@ template  bool RetainSummaryMan
   llvm_unreachable("Unexpected attribute passed");
 }
 
-ArgEffects RetainSummaryManager::getArgEffects() {
-  ArgEffects AE = ScratchArgs;
-  ScratchArgs = AF.getEmptyMap();
-  return AE;
-}
-
 const RetainSummary *
 RetainSummaryManager::getPersistentSummary(const RetainSummary &OldSumm) {
   // Unique "simple" summaries -- thos

[PATCH] D55955: Properly diagnose [[nodiscard]] on the body of a range-based for loop

2018-12-24 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman marked 3 inline comments as done.
aaron.ballman added inline comments.



Comment at: include/clang/Sema/Sema.h:5337
   ExprResult ActOnFinishFullExpr(Expr *Expr, SourceLocation CC,
- bool DiscardedValue = false,
+ bool WarnOnDiscardedValue = false,
  bool IsConstexpr = false);

rsmith wrote:
> Why "WarnOn"? Shouldn't this flag simply indicate whether the expression is a 
> discarded-value expression?
It probably can; but then it feels like the logic is backwards from the 
suggested changes as I understood them. If it's a discarded value expression, 
then the value being unused should *not* be diagnosed because the expression 
only exists for its side effects (not its value computations), correct?



Comment at: lib/Parse/ParseStmt.cpp:23
 #include "clang/Sema/Scope.h"
+#include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/TypoCorrection.h"

rsmith wrote:
> The parser shouldn't be looking inside Sema's data structures. Can you add a 
> method to the Sema interface for the query you make below instead?
Can do.



Comment at: lib/Sema/TreeTransform.h:6529
+  --SuppressWarnOnUnusedExpr;
+
 if (Result.isInvalid()) {

rsmith wrote:
> Instead of this counter mechanism (which doesn't seem reliable), can you just 
> pass a flag to TransformStmt?
I'll give it a shot, it seems plausible.


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

https://reviews.llvm.org/D55955



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

[PATCH] D56067: Make test/Driver/darwin-sdk-version.c pass if the host triple is 32-bit

2018-12-24 Thread Nico Weber via Phabricator via cfe-commits 
thakis created this revision.
thakis added a reviewer: arphaman.

For some reason, the cmake build on my macbook has 
LLVM_HOST_TRIPLE:STRING=i386-apple-darwin16.7.0 . 
test/Driver/darwin-sdk-version.c assumed that the host triple is 64-bit, so 
make it resilient against 32-bit host triples.


https://reviews.llvm.org/D56067

Files:
  test/Driver/darwin-sdk-version.c


Index: test/Driver/darwin-sdk-version.c
===
--- test/Driver/darwin-sdk-version.c
+++ test/Driver/darwin-sdk-version.c
@@ -5,10 +5,10 @@
 //
 // RUN: rm -rf %t/SDKs/MacOSX10.10.sdk
 // RUN: mkdir -p %t/SDKs/MacOSX10.10.sdk
-// RUN: %clang -isysroot %t/SDKs/MacOSX10.10.sdk -c -### %s 2>&1 \
+// RUN: %clang -m64 -isysroot %t/SDKs/MacOSX10.10.sdk -c -### %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=INFER_SDK_VERSION %s
 // RUN: sed -e 's/10\.14/10\.8/g' %S/Inputs/MacOSX10.14.sdk/SDKSettings.json > 
%t/SDKs/MacOSX10.10.sdk/SDKSettings.json
-// RUN: %clang -isysroot %t/SDKs/MacOSX10.10.sdk -c -### %s 2>&1 \
+// RUN: %clang -m64 -isysroot %t/SDKs/MacOSX10.10.sdk -c -### %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=INFER_DEPLOYMENT_TARGET_VERSION %s
 // REQUIRES: system-darwin && native
 //


Index: test/Driver/darwin-sdk-version.c
===
--- test/Driver/darwin-sdk-version.c
+++ test/Driver/darwin-sdk-version.c
@@ -5,10 +5,10 @@
 //
 // RUN: rm -rf %t/SDKs/MacOSX10.10.sdk
 // RUN: mkdir -p %t/SDKs/MacOSX10.10.sdk
-// RUN: %clang -isysroot %t/SDKs/MacOSX10.10.sdk -c -### %s 2>&1 \
+// RUN: %clang -m64 -isysroot %t/SDKs/MacOSX10.10.sdk -c -### %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=INFER_SDK_VERSION %s
 // RUN: sed -e 's/10\.14/10\.8/g' %S/Inputs/MacOSX10.14.sdk/SDKSettings.json > %t/SDKs/MacOSX10.10.sdk/SDKSettings.json
-// RUN: %clang -isysroot %t/SDKs/MacOSX10.10.sdk -c -### %s 2>&1 \
+// RUN: %clang -m64 -isysroot %t/SDKs/MacOSX10.10.sdk -c -### %s 2>&1 \
 // RUN:   | FileCheck --check-prefix=INFER_DEPLOYMENT_TARGET_VERSION %s
 // REQUIRES: system-darwin && native
 //
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55955: Properly diagnose [[nodiscard]] on the body of a range-based for loop

2018-12-24 Thread Arthur O'Dwyer via Phabricator via cfe-commits 
Quuxplusone added inline comments.



Comment at: include/clang/Sema/Sema.h:5337
   ExprResult ActOnFinishFullExpr(Expr *Expr, SourceLocation CC,
- bool DiscardedValue = false,
+ bool WarnOnDiscardedValue = false,
  bool IsConstexpr = false);

aaron.ballman wrote:
> rsmith wrote:
> > Why "WarnOn"? Shouldn't this flag simply indicate whether the expression is 
> > a discarded-value expression?
> It probably can; but then it feels like the logic is backwards from the 
> suggested changes as I understood them. If it's a discarded value expression, 
> then the value being unused should *not* be diagnosed because the expression 
> only exists for its side effects (not its value computations), correct?
Peanut gallery says: There are at least three things that need to be computed 
somewhere: (1) Is this expression's value discarded? (2) Is this expression the 
result of a `[[nodiscard]]` function? (3) Is the diagnostic enabled? It is 
unclear to me who's responsible for computing which of these things. I.e., it 
is unclear to me whether `WarnOnDiscardedValue=true` means "Hey 
`ActOnFinishFullExpr`, please give a warning //because// this value is being 
discarded" (conjunction of 1,2, and maybe 3) or "Hey `ActOnFinishFullExpr`, 
please give a warning //if// this value is being discarded" (conjunction of 2 
and maybe 3).

I also think it is needlessly confusing that `ActOnFinishFullExpr` gives 
`WarnOnDiscardedValue` a defaulted value of `false` but `ActOnExprStmt` gives 
`WarnOnDiscardedValue` a defaulted value of `true`. Defaulted values 
(especially of boolean type) are horrible, but context-dependent defaulted 
values are even worse.


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

https://reviews.llvm.org/D55955



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

[PATCH] D56065: [gn build] Make NOSORT line actually work

2018-12-24 Thread Petr Hosek via Phabricator via cfe-commits 
phosek accepted this revision.
phosek added a comment.
This revision is now accepted and ready to land.

LGTM


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

https://reviews.llvm.org/D56065



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

[PATCH] D56065: [gn build] Make NOSORT line actually work

2018-12-24 Thread Phabricator via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL350060: [gn build] Make NOSORT line actually work (authored 
by nico, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D56065?vs=179474&id=179480#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56065

Files:
  llvm/trunk/utils/gn/secondary/clang/lib/Headers/BUILD.gn


Index: llvm/trunk/utils/gn/secondary/clang/lib/Headers/BUILD.gn
===
--- llvm/trunk/utils/gn/secondary/clang/lib/Headers/BUILD.gn
+++ llvm/trunk/utils/gn/secondary/clang/lib/Headers/BUILD.gn
@@ -34,9 +34,9 @@
 ":arm_headers",
   ]
 
+  # NOSORT
   # Tell `gn format` to not reorder the sources list: Its order matches CMake,
   # and the ordering is alphabetical but ignores leading underscores.
-  # NOSORT
   sources = [
 "adxintrin.h",
 "altivec.h",


Index: llvm/trunk/utils/gn/secondary/clang/lib/Headers/BUILD.gn
===
--- llvm/trunk/utils/gn/secondary/clang/lib/Headers/BUILD.gn
+++ llvm/trunk/utils/gn/secondary/clang/lib/Headers/BUILD.gn
@@ -34,9 +34,9 @@
 ":arm_headers",
   ]
 
+  # NOSORT
   # Tell `gn format` to not reorder the sources list: Its order matches CMake,
   # and the ordering is alphabetical but ignores leading underscores.
-  # NOSORT
   sources = [
 "adxintrin.h",
 "altivec.h",
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits