[PATCH] D74166: [AIX][Frontend] Static init implementation for AIX considering no priority

2020-06-13 Thread Hubert Tong via Phabricator via cfe-commits 
hubert.reinterpretcast added inline comments.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4482
+  llvm::Value *NeedsDestruct =
+  CGF.Builder.CreateIsNull(V, "needsDestruct");
+

There are uses of `CreateIsNull` with `snake_case`; this is the only 
`camelCase` instance.



Comment at: clang/test/CodeGen/static-init.cpp:14
+  } t1, t2;
+}; // namespace test1
 

Minor nit: Don't use a semicolon after a namespace definition. Apply throughout.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74166



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

[PATCH] D69764: [clang-format] Add East/West Const fixer capability

2020-06-13 Thread Stephen Kelly via Phabricator via cfe-commits 
steveire added a comment.

In D69764#2063798 , @MyDeveloperDay 
wrote:

> @steveire Thanks for the additional test cases, I'm reworking how I handle 
> the Macro case as I realized that I didn't actually even try to change the 
> case @rsmith came up with in the first place. I made a decision previous that 
> I couldn't handle any case without the *,& or && to determine that wasn't the 
> case
>
> Just ignoring upper case isn't a good idea either because of LONG,HRESULT and 
> LRESULT types.
>
> it may take me a couple of days, but I agree to have something akin to 
> FOREACH macros might be the way to go.


I think I've found more bugs in the current implementation, but I think you're 
working on a totally different implementation now, so there may not be so much 
value in reporting bugs with the current implementation, right?


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

https://reviews.llvm.org/D69764



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

[PATCH] D81315: [analyzer] Warning for default constructed unique pointer dereferences

2020-06-13 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added a comment.

Hey, that's a lot of progress already! =)




Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:124
+return;
+  updateTrackedRegion(Call, C, ThisValRegion);
+}

Not all constructors behave this way. In particular, if it's a copy/move 
constructor this function would track the value of the original smart pointer 
to this smart pointer, but what we need is to track the value of the raw 
pointer instead.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:175
+return;
+  updateTrackedRegion(Call, C, ThisValRegion);
+}

When you're doing `evalCall`, you're responsible for all aspects of the call - 
not just your private GDM map but also the Store and the Environment.

For `.reset()` the other important thing to do would be to invalidate the 
region in the Store so that the Store did not think that it contains the old 
value. We can't set the new value correctly but invalidating it would still be 
better than doing nothing - simply because "not being sure" is not as bad as 
"being confidently incorrect". That said, once you model *all* methods of the 
smart pointers, you would probably no longer need to invalidate the Store, 
because it will never be actually accessed during analysis. So my conclusion 
here is:
- For this first patch invalidating the Store is probably not worth it but 
let's add a FIXME.
- We should make sure that we eventually add the invalidation if we don't have 
time to model all methods.

Now, you're calling this same function for `.release()` as well. And for 
`.release()` there is another important thing that we're responsible for: 
//model the return value//. The `.release()` method returns the raw pointer - 
which is something this checker is accidentally an expert on! - so let's 
`BindExpr()` the value of the call-expression to the value of the raw pointer. 
If you want to delay this exercise until later patches, i recommend to 
temporarily modeling the return value as a conjured symbol instead, but we 
cannot completely drop this part.

Another useful thing that we should do with `.release()` in the future is to 
tell `MallocChecker` to start tracking the raw pointer. This will allow us to 
emit memory leak warnings against it. Let's add a TODO for that as well.


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

https://reviews.llvm.org/D81315



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

[PATCH] D69825: [Clang][Driver] Re-use the calling process instead of creating a new process for the cc1 invocation

2020-06-13 Thread Dimitry Andric via Phabricator via cfe-commits 
dim added a subscriber: asbirlea.
dim added a comment.

Okay, after a long while of attempting to make a reproduction scenario, I 
finally managed one that consistently worked. As BSD make puts in different 
redirections for stderr when running in `-j` mode, it turned out that I could 
simply run the compilation twice on a terminal, first normally, then with 
stderr redirected to `/dev/null`, e.g. my test script is:

  #!/bin/sh
  
  exec /dev/tty 2>&1
  
  ulimit -c 0
  trap "rm -f testcase[12].s testcase[12].s-* testcase-*.s.tmp" EXIT
  
  ${CLANG:-clang} -fintegrated-cc1 -Werror -O2 -std=gnu99 
-fstack-protector-strong -S testcase.c -o testcase1.s || exit 1
  ${CLANG:-clang} -fintegrated-cc1 -Werror -O2 -std=gnu99 
-fstack-protector-strong -S testcase.c -o testcase2.s 2>/dev/null || exit 1
  
  cmp -s testcase1.s testcase2.s
  test $? = 1 || exit 1
  
  exit 0

(Here `testcase.c` is a copy of the .c file in 
https://www.andric.com/freebsd/clang/bug246630-repro.tar.xz)

Using this script I could finally bisect, and arrived at the following commit 
which fixes the issue, i.e. it makes the outputs of both clang runs the same 
again, and it is rG0cecafd647cc 
 
("[BasicAA] Make BasicAA a cfg pass"). @asbirlea seems to indicate in the 
commit message that it influences the way the BasicAA pass is freed/invalidaed 
or not during runs?

So is this commit fixing it as an unintended side effect, or would it be 
according to expectation?  If the latter, we should merge it into 10.0.1, and I 
will create a PR for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69825



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

[PATCH] D81745: [analyzer][MallocChecker] PR46253: Correctly recognize standard realloc

2020-06-13 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Thanks! Yeah, that's a lot of annoying code to write that doesn't need to be 
imperative at all.




Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1074-1075
+  // function. This should be solved by making CallDescription smarter.
+  // Mind that this came from a bug report, and all other functions suffer from
+  // this.
+  // https://bugs.llvm.org/show_bug.cgi?id=46253

> https://bugs.llvm.org/show_bug.cgi?id=46253#c1
> Uh-oh, did we lose some sanity checking during CallDescription conversion?

While modeling of all functions is probably incorrect, the //crash// can be 
bisected down to D68165. I think other functions don't crash because they 
already have similar type checks in them, just more spread out around the code 
rather than concentrated in one place. It might still be worth it to try to 
figure out why exactly did D68165 cause it in order to double-check for more 
regressions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81745



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

[PATCH] D81794: [clang] Don't emit warn_cxx_ms_struct when MSBitfields is enabled globally

2020-06-13 Thread Martin Storsjö via Phabricator via cfe-commits 
mstorsjo updated this revision to Diff 270587.
mstorsjo added a comment.

Fixed the formatting


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

https://reviews.llvm.org/D81794

Files:
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/ms_struct.cpp


Index: clang/test/SemaCXX/ms_struct.cpp
===
--- clang/test/SemaCXX/ms_struct.cpp
+++ clang/test/SemaCXX/ms_struct.cpp
@@ -1,8 +1,11 @@
-// RUN: %clang_cc1 -fsyntax-only -Wno-error=incompatible-ms-struct -verify 
-triple i686-apple-darwin9 -std=c++11 %s
-// RUN: %clang_cc1 -fsyntax-only -Wno-error=incompatible-ms-struct -verify 
-triple armv7-apple-darwin9 -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -DTEST_FOR_WARNING 
-Wno-error=incompatible-ms-struct -verify -triple i686-apple-darwin9 -std=c++11 
%s
+// RUN: %clang_cc1 -fsyntax-only -DTEST_FOR_WARNING 
-Wno-error=incompatible-ms-struct -verify -triple armv7-apple-darwin9 
-std=c++11 %s
 // RUN: %clang_cc1 -fsyntax-only -DTEST_FOR_ERROR -verify -triple 
armv7-apple-darwin9 -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -DNO_PRAGMA -mms-bitfields -verify -triple 
armv7-apple-darwin9 -std=c++11 %s
 
+#ifndef NO_PRAGMA
 #pragma ms_struct on
+#endif
 
 struct A {
   unsigned long a:4;
@@ -12,7 +15,7 @@
 struct B : public A {
 #ifdef TEST_FOR_ERROR
   // expected-error@-2 {{ms_struct may not produce Microsoft-compatible 
layouts for classes with base classes or virtual functions}}
-#else
+#elif defined(TEST_FOR_WARNING)
   // expected-warning@-4 {{ms_struct may not produce Microsoft-compatible 
layouts for classes with base classes or virtual functions}}
 #endif
   unsigned long c:16;
@@ -27,7 +30,7 @@
 struct C {
 #ifdef TEST_FOR_ERROR
   // expected-error@-2 {{ms_struct may not produce Microsoft-compatible 
layouts for classes with base classes or virtual functions}}
-#else
+#elif defined(TEST_FOR_WARNING)
   // expected-warning@-4 {{ms_struct may not produce Microsoft-compatible 
layouts for classes with base classes or virtual functions}}
 #endif
   virtual void foo();
@@ -36,3 +39,6 @@
 
 static_assert(__builtin_offsetof(C, n) == 8,
   "long long field in ms_struct should be 8-byte aligned");
+#if !defined(TEST_FOR_ERROR) && !defined(TEST_FOR_WARNING)
+// expected-no-diagnostics
+#endif
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -6753,7 +6753,11 @@
   // headers, sweeping up a bunch of types that the project doesn't
   // really rely on MSVC-compatible layout for.  We must therefore
   // support "ms_struct except for C++ stuff" as a secondary ABI.
-  if (Record->isMsStruct(Context) &&
+  // Don't emit this diagnostic if the feature was enabled as a
+  // language option (as opposed to via a pragma or attribute), as
+  // the option -mms-bitfields otherwise essentially makes it impossible
+  // to build C++ code, unless this diagnostic is turned off.
+  if (Record->isMsStruct(Context) && !Context.getLangOpts().MSBitfields &&
   (Record->isPolymorphic() || Record->getNumBases())) {
 Diag(Record->getLocation(), diag::warn_cxx_ms_struct);
   }


Index: clang/test/SemaCXX/ms_struct.cpp
===
--- clang/test/SemaCXX/ms_struct.cpp
+++ clang/test/SemaCXX/ms_struct.cpp
@@ -1,8 +1,11 @@
-// RUN: %clang_cc1 -fsyntax-only -Wno-error=incompatible-ms-struct -verify -triple i686-apple-darwin9 -std=c++11 %s
-// RUN: %clang_cc1 -fsyntax-only -Wno-error=incompatible-ms-struct -verify -triple armv7-apple-darwin9 -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -DTEST_FOR_WARNING -Wno-error=incompatible-ms-struct -verify -triple i686-apple-darwin9 -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -DTEST_FOR_WARNING -Wno-error=incompatible-ms-struct -verify -triple armv7-apple-darwin9 -std=c++11 %s
 // RUN: %clang_cc1 -fsyntax-only -DTEST_FOR_ERROR -verify -triple armv7-apple-darwin9 -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -DNO_PRAGMA -mms-bitfields -verify -triple armv7-apple-darwin9 -std=c++11 %s
 
+#ifndef NO_PRAGMA
 #pragma ms_struct on
+#endif
 
 struct A {
   unsigned long a:4;
@@ -12,7 +15,7 @@
 struct B : public A {
 #ifdef TEST_FOR_ERROR
   // expected-error@-2 {{ms_struct may not produce Microsoft-compatible layouts for classes with base classes or virtual functions}}
-#else
+#elif defined(TEST_FOR_WARNING)
   // expected-warning@-4 {{ms_struct may not produce Microsoft-compatible layouts for classes with base classes or virtual functions}}
 #endif
   unsigned long c:16;
@@ -27,7 +30,7 @@
 struct C {
 #ifdef TEST_FOR_ERROR
   // expected-error@-2 {{ms_struct may not produce Microsoft-compatible layouts for classes with base classes or virtual functions}}
-#else
+#elif defined(TEST_FOR_WARNING)
   // expected-warning@-4 {{ms_struct may not produce Microsoft-compatible layouts for classes with base classes or

[PATCH] D81795: [clang] Enable -mms-bitfields by default for mingw targets

2020-06-13 Thread Martin Storsjö via Phabricator via cfe-commits 
mstorsjo created this revision.
mstorsjo added reviewers: rjmccall, rnk.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This matches GCC, which enabled -mms-bitfields by default for mingw targets in 
4.7 [1].

[1] https://www.gnu.org/software/gcc/gcc-4.7/changes.html


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81795

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/ms-bitfields.c


Index: clang/test/Driver/ms-bitfields.c
===
--- clang/test/Driver/ms-bitfields.c
+++ clang/test/Driver/ms-bitfields.c
@@ -1,4 +1,5 @@
-// RUN: %clang -### %s 2>&1 | FileCheck %s -check-prefix=NO-MSBITFIELDS
+// RUN: %clang -### -target x86_64-linux-gnu %s 2>&1 | FileCheck %s 
-check-prefix=NO-MSBITFIELDS
+// RUN: %clang -### -target x86_64-windows-gnu %s 2>&1 | FileCheck %s 
-check-prefix=MSBITFIELDS
 // RUN: %clang -### -mno-ms-bitfields -mms-bitfields %s 2>&1 | FileCheck %s 
-check-prefix=MSBITFIELDS
 // RUN: %clang -### -mms-bitfields -mno-ms-bitfields %s 2>&1 | FileCheck %s 
-check-prefix=NO-MSBITFIELDS
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4646,7 +4646,7 @@
 CmdArgs.push_back("-fforbid-guard-variables");
 
   if (Args.hasFlag(options::OPT_mms_bitfields, options::OPT_mno_ms_bitfields,
-   false)) {
+   Triple.isWindowsGNUEnvironment())) {
 CmdArgs.push_back("-mms-bitfields");
   }
 


Index: clang/test/Driver/ms-bitfields.c
===
--- clang/test/Driver/ms-bitfields.c
+++ clang/test/Driver/ms-bitfields.c
@@ -1,4 +1,5 @@
-// RUN: %clang -### %s 2>&1 | FileCheck %s -check-prefix=NO-MSBITFIELDS
+// RUN: %clang -### -target x86_64-linux-gnu %s 2>&1 | FileCheck %s -check-prefix=NO-MSBITFIELDS
+// RUN: %clang -### -target x86_64-windows-gnu %s 2>&1 | FileCheck %s -check-prefix=MSBITFIELDS
 // RUN: %clang -### -mno-ms-bitfields -mms-bitfields %s 2>&1 | FileCheck %s -check-prefix=MSBITFIELDS
 // RUN: %clang -### -mms-bitfields -mno-ms-bitfields %s 2>&1 | FileCheck %s -check-prefix=NO-MSBITFIELDS
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4646,7 +4646,7 @@
 CmdArgs.push_back("-fforbid-guard-variables");
 
   if (Args.hasFlag(options::OPT_mms_bitfields, options::OPT_mno_ms_bitfields,
-   false)) {
+   Triple.isWindowsGNUEnvironment())) {
 CmdArgs.push_back("-mms-bitfields");
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81794: [clang] Don't emit warn_cxx_ms_struct when MSBitfields is enabled globally

2020-06-13 Thread Martin Storsjö via Phabricator via cfe-commits 
mstorsjo created this revision.
mstorsjo added reviewers: rjmccall, rnk.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This diagnostic (which defaults to an error, added in 
95833f33bda6c92e746e0b0007b69c2c30bfc693) was (as far as I can see) intended to 
clearly point out cases where the C++ ABI won't match the Microsoft C++ ABI, 
for cases when this is enabled via a pragma over a region of code.

The MSVC compatible struct layout feature can also be enabled via a compiler 
option (-mms-bitfields). If enabled that way, one essentially can't compile any 
C++ code unless also building with -Wno-incompatible-ms-struct (which GCC 
doesn't support, and projects developed with GCC aren't setting).

For the MinGW target, it's expected that the C++ ABI won't match the MSVC one, 
if this option is used for getting the struct layout to match MSVC.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81794

Files:
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/ms_struct.cpp


Index: clang/test/SemaCXX/ms_struct.cpp
===
--- clang/test/SemaCXX/ms_struct.cpp
+++ clang/test/SemaCXX/ms_struct.cpp
@@ -1,8 +1,11 @@
-// RUN: %clang_cc1 -fsyntax-only -Wno-error=incompatible-ms-struct -verify 
-triple i686-apple-darwin9 -std=c++11 %s
-// RUN: %clang_cc1 -fsyntax-only -Wno-error=incompatible-ms-struct -verify 
-triple armv7-apple-darwin9 -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -DTEST_FOR_WARNING 
-Wno-error=incompatible-ms-struct -verify -triple i686-apple-darwin9 -std=c++11 
%s
+// RUN: %clang_cc1 -fsyntax-only -DTEST_FOR_WARNING 
-Wno-error=incompatible-ms-struct -verify -triple armv7-apple-darwin9 
-std=c++11 %s
 // RUN: %clang_cc1 -fsyntax-only -DTEST_FOR_ERROR -verify -triple 
armv7-apple-darwin9 -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -DNO_PRAGMA -mms-bitfields -verify -triple 
armv7-apple-darwin9 -std=c++11 %s
 
+#ifndef NO_PRAGMA
 #pragma ms_struct on
+#endif
 
 struct A {
   unsigned long a:4;
@@ -12,7 +15,7 @@
 struct B : public A {
 #ifdef TEST_FOR_ERROR
   // expected-error@-2 {{ms_struct may not produce Microsoft-compatible 
layouts for classes with base classes or virtual functions}}
-#else
+#elif defined(TEST_FOR_WARNING)
   // expected-warning@-4 {{ms_struct may not produce Microsoft-compatible 
layouts for classes with base classes or virtual functions}}
 #endif
   unsigned long c:16;
@@ -27,7 +30,7 @@
 struct C {
 #ifdef TEST_FOR_ERROR
   // expected-error@-2 {{ms_struct may not produce Microsoft-compatible 
layouts for classes with base classes or virtual functions}}
-#else
+#elif defined(TEST_FOR_WARNING)
   // expected-warning@-4 {{ms_struct may not produce Microsoft-compatible 
layouts for classes with base classes or virtual functions}}
 #endif
   virtual void foo();
@@ -36,3 +39,6 @@
 
 static_assert(__builtin_offsetof(C, n) == 8,
   "long long field in ms_struct should be 8-byte aligned");
+#if !defined(TEST_FOR_ERROR) && !defined(TEST_FOR_WARNING)
+// expected-no-diagnostics
+#endif
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -6753,7 +6753,12 @@
   // headers, sweeping up a bunch of types that the project doesn't
   // really rely on MSVC-compatible layout for.  We must therefore
   // support "ms_struct except for C++ stuff" as a secondary ABI.
+  // Don't emit this diagnostic if the feature was enabled as a
+  // language option (as opposed to via a pragma or attribute), as
+  // the option -mms-bitfields otherwise essentially makes it impossible
+  // to build C++ code, unless this diagnostic is turned off.
   if (Record->isMsStruct(Context) &&
+  !Context.getLangOpts().MSBitfields &&
   (Record->isPolymorphic() || Record->getNumBases())) {
 Diag(Record->getLocation(), diag::warn_cxx_ms_struct);
   }


Index: clang/test/SemaCXX/ms_struct.cpp
===
--- clang/test/SemaCXX/ms_struct.cpp
+++ clang/test/SemaCXX/ms_struct.cpp
@@ -1,8 +1,11 @@
-// RUN: %clang_cc1 -fsyntax-only -Wno-error=incompatible-ms-struct -verify -triple i686-apple-darwin9 -std=c++11 %s
-// RUN: %clang_cc1 -fsyntax-only -Wno-error=incompatible-ms-struct -verify -triple armv7-apple-darwin9 -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -DTEST_FOR_WARNING -Wno-error=incompatible-ms-struct -verify -triple i686-apple-darwin9 -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -DTEST_FOR_WARNING -Wno-error=incompatible-ms-struct -verify -triple armv7-apple-darwin9 -std=c++11 %s
 // RUN: %clang_cc1 -fsyntax-only -DTEST_FOR_ERROR -verify -triple armv7-apple-darwin9 -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -DNO_PRAGMA -mms-bitfields -verify -triple armv7-apple-darwin9 -std=c++11 %s
 
+#ifndef NO_PRAGMA
 #pragma ms_struct on
+#endif
 
 struct A {
   unsigned long a:4;
@@ -12,7 +15,7 @@
 struct B :

[PATCH] D81736: [openmp] Base of tablegen generated OpenMP common declaration

2020-06-13 Thread Johannes Doerfert via Phabricator via cfe-commits 
jdoerfert added reviewers: fghanim, ABataev, jdenny, hfinkel, jhuber6, 
kiranchandramohan, kiranktp.
jdoerfert added a comment.

This is really cool :)

In D81736#2090419 , @clementval wrote:

> @jdoerfert Some tests in clang relies on the position of the specific enum in 
> the declaration. TableGen is sorting the records so the enum is generated in 
> alphabetical order therefore the enum position is changed. Is it fine to 
> update the test with smith like `{{.*}}` instead of the specific id?


Yes, that's fine.

I left some comments and added some more reviewers.




Comment at: llvm/include/llvm/Frontend/Directive/DirectiveBase.td:69
+  list requiredClauses = ?;
+}

I really like we have this abstraction now and also a nice way to encode some 
constraints, e.g., required clauses, right away :)



Comment at: llvm/include/llvm/Frontend/OpenMP/OMP.td:496
+def OMP_EndDeclareVariant : Directive<"end declare variant"> {}
+def OMP_Unknown : Directive<"unknown"> {}

I guess we go over this in a follow up and use the `requiredClauses` and 
similar features, that seems reasonable to me. 



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:50
-#include "llvm/Frontend/OpenMP/OMPKinds.def"
-
 /// IDs for all omp runtime library (RTL) functions.

While the MACRO MAGIC worked surprisingly well, I'm glad it is going away.



Comment at: llvm/test/TableGen/directive.td:34
+// CHECK-NEXT: }
+// CHECK-NEXT: }

How does the `allowedClauses` affect the result?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81736



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

[PATCH] D81678: Introduce partialinit attribute at call sites for stricter poison analysis

2020-06-13 Thread Evgenii Stepanov via Phabricator via cfe-commits 
eugenis added a comment.

Positive attribute sounds good to me (frozen is not a bad name), but the tests 
update change is going to be huge. Any concerns about IR size bloat? The 
attribute will apply to the majority of function arguments, 8 bytes per 
instance as far as I can see.

Good point about uninitialized variables being undef, not poison. The meaning 
of the attribute should be "poison or undef (even partial) in this argument is 
UB". An extension that specifies which bytes, or even bits, of the argument are 
required to be undef-free would be nice, but seems too much at this point.

Another way to design this thing that would work for MSan, but not so much for 
general poison/undef analysis, is to emit an intrinsic call on the pre-coerced 
type to force an MSan check, and then an attribute ("partialinit") on the 
argument to skip checking the coerced value. This way the frontend has greater 
control over which parts of the arguments correspond to C++-things, and what 
does it mean for them to be initialized. I'm not sure I like this design though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81678



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

[PATCH] D81678: Introduce partialinit attribute at call sites for stricter poison analysis

2020-06-13 Thread Eli Friedman via Phabricator via cfe-commits 
efriedma added a comment.

In D81678#2089041 , @aqjune wrote:

> > @efriedma 
> >  The way that call argument coercion works is unsound in the presence of 
> > poison. An integer can't be partially poisoned: it's either poison, or not 
> > poison. We probably need to come up with some safer way to pass 
> > structs/unions.
>
> This is true, clang frontend may lower an argument with aggregate type into 
> one with large int type (such as i64).
>  However, can poison value be safely generated in C? Paddings or union with 
> different size may contain undef bits, but not poison. Signed overflow is UB.
>  Undef value can exist bitwisely, so I think this is an orthogonal issue.


In C semantics, an expression can't produce a poison value.  As long as 
variables and allocations are initialized to undef, not poison, there isn't any 
way to sneak poison into the padding of a variable, so argument passing is 
sound.  So I guess it's not an issue unless we start poisoning uninitialized 
variables.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81678



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

[PATCH] D81422: Change filecheck default to dump input on failure

2020-06-13 Thread Joel E. Denny via Phabricator via cfe-commits 
jdenny added a comment.

In D81422#2090425 , @mehdi_amini wrote:

> I'm thinking about a possible improvement here: we could have FileCheck dump 
> the input for the current CHECK-LABEL section only: it seems like it could 
> reduce the output drastically while still preserving how useful the 
> information is?


One FileCheck invocation can report multiple errors, one per CHECK-LABEL 
section, and I prefer to see all errors every time.

I also prefer to see the entire input dump (with annotations produced by -vv) 
as sometimes the error FileCheck reports is far away from the actual error.  
For example, maybe the CHECK-LABEL directives matched text at unexpected 
locations in the input, and then the directives in their sections failed as a 
result. It might be hard to determine that the CHECK-LABEL directives were at 
fault without seeing the text it should have matched outside of its (incorrect) 
section.

If you choose to limit the dump to one CHECK-LABEL section, please make that 
behavior optional via a command-line option (which can be set via 
FILECHECK_OPTS).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81422



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

[PATCH] D81422: Change filecheck default to dump input on failure

2020-06-13 Thread Mehdi AMINI via Phabricator via cfe-commits 
mehdi_amini added a comment.

I'm thinking about a possible improvement here: we could have FileCheck dump 
the input for the current CHECK-LABEL section only: it seems like it could 
reduce the output drastically while still preserving how useful the information 
is?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81422



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

[PATCH] D81422: Change filecheck default to dump input on failure

2020-06-13 Thread Joel E. Denny via Phabricator via cfe-commits 
jdenny added a comment.

In D81422#2090725 , @mehdi_amini wrote:

> In D81422#2090618 , @jdenny wrote:
>
> > In D81422#2090425 , @mehdi_amini 
> > wrote:
> >
> > > I'm thinking about a possible improvement here: we could have FileCheck 
> > > dump the input for the current CHECK-LABEL section only: it seems like it 
> > > could reduce the output drastically while still preserving how useful the 
> > > information is?
> >
> >
> > One FileCheck invocation can report multiple errors, one per CHECK-LABEL 
> > section, and I prefer to see all errors every time.
>
>
> Yeah I had in mind to show all the errors for the current section, then the 
> input for the section, before moving to the next section.


I'm not sure I understand. Currently, (1) FileCheck prints all errors up front 
in case that's sufficient without the dump, and then (2) FileCheck prints the 
dump with annotations showing the errors again (and successful directives, 
including labels, if -vv).  Are you talking about interleaving 1 and 2?  Does 
that actually make it easier to read?  Maybe just omit lines from the dump 
instead?

>> I also prefer to see the entire input dump (with annotations produced by 
>> -vv) as sometimes the error FileCheck reports is far away from the actual 
>> error.  For example, maybe the CHECK-LABEL directives matched text at 
>> unexpected locations in the input, and then the directives in their sections 
>> failed as a result. It might be hard to determine that the CHECK-LABEL 
>> directives were at fault without seeing the text it should have matched 
>> outside of its (incorrect) section.
> 
> Sure: but that seems like not the most common case to me: if we start by 
> showing the section we matched it provides already a pretty good indication.
> 
>> If you choose to limit the dump to one CHECK-LABEL section, please make that 
>> behavior optional via a command-line option (which can be set via 
>> FILECHECK_OPTS).
> 
> I thought it would be a better default actually, leaving the full-full as 
> opt-in

I'm not sure how common it is, and I don't know what the default should be.  
From the command line, I can always run again with different options to get 
missing info.  From the bots, it's harder to get missing info (which is why I 
suggested -vv for the bots).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81422



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

[PATCH] D81422: Change filecheck default to dump input on failure

2020-06-13 Thread Mehdi AMINI via Phabricator via cfe-commits 
mehdi_amini added a comment.

In D81422#2090618 , @jdenny wrote:

> In D81422#2090425 , @mehdi_amini 
> wrote:
>
> > I'm thinking about a possible improvement here: we could have FileCheck 
> > dump the input for the current CHECK-LABEL section only: it seems like it 
> > could reduce the output drastically while still preserving how useful the 
> > information is?
>
>
> One FileCheck invocation can report multiple errors, one per CHECK-LABEL 
> section, and I prefer to see all errors every time.


Yeah I had in mind to show all the errors for the current section, then the 
input for the section, before moving to the next section.

> I also prefer to see the entire input dump (with annotations produced by -vv) 
> as sometimes the error FileCheck reports is far away from the actual error.  
> For example, maybe the CHECK-LABEL directives matched text at unexpected 
> locations in the input, and then the directives in their sections failed as a 
> result. It might be hard to determine that the CHECK-LABEL directives were at 
> fault without seeing the text it should have matched outside of its 
> (incorrect) section.

Sure: but that seems like not the most common case to me: if we start by 
showing the section we matched it provides already a pretty good indication.

> If you choose to limit the dump to one CHECK-LABEL section, please make that 
> behavior optional via a command-line option (which can be set via 
> FILECHECK_OPTS).

I thought it would be a better default actually, leaving the full-full as opt-in


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81422



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

[PATCH] D81422: Change filecheck default to dump input on failure

2020-06-13 Thread Matt Arsenault via Phabricator via cfe-commits 
arsenm added a comment.

In D81422#2090425 , @mehdi_amini wrote:

> I'm thinking about a possible improvement here: we could have FileCheck dump 
> the input for the current CHECK-LABEL section only: it seems like it could 
> reduce the output drastically while still preserving how useful the 
> information is?


That would help some. In my situation I've been making a lot of changes that 
touch dozens of generated MIR tests, and change most of the functions. If the 
dump constrained itself to just the first -LABEL section it would be more 
manageable


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81422



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

[PATCH] D80514: [clang-tidy] modernize-use-trailing-return-type support for C++20 concepts and decltype

2020-06-13 Thread Bernhard Manfred Gruber via Phabricator via cfe-commits 
bernhardmgruber added a comment.

Ping.


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

https://reviews.llvm.org/D80514



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

[clang] c669a1e - [clang][NFC] Pack LambdaExpr

2020-06-13 Thread Bruno Ricci via cfe-commits 

Author: Bruno Ricci
Date: 2020-06-13T14:31:13+01:00
New Revision: c669a1ed6386d57a75a602b53266466dae1e1d84

URL: 
https://github.com/llvm/llvm-project/commit/c669a1ed6386d57a75a602b53266466dae1e1d84
DIFF: 
https://github.com/llvm/llvm-project/commit/c669a1ed6386d57a75a602b53266466dae1e1d84.diff

LOG: [clang][NFC] Pack LambdaExpr

This saves sizeof(void *) bytes per LambdaExpr.

Review-after-commit since this is a straightforward change similar
to the work done on other nodes. NFC.

Added: 


Modified: 
clang/include/clang/AST/ExprCXX.h
clang/include/clang/AST/Stmt.h
clang/lib/AST/ExprCXX.cpp
clang/lib/Serialization/ASTReaderStmt.cpp
clang/lib/Serialization/ASTWriterStmt.cpp

Removed: 




diff  --git a/clang/include/clang/AST/ExprCXX.h 
b/clang/include/clang/AST/ExprCXX.h
index 82036a295002..822025a7503f 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -1829,26 +1829,14 @@ Stmt **CXXConstructExpr::getTrailingArgs() {
 /// and which can never occur implicitly.
 class LambdaExpr final : public Expr,
  private llvm::TrailingObjects {
+  // LambdaExpr has some data stored in LambdaExprBits.
+
   /// The source range that covers the lambda introducer ([...]).
   SourceRange IntroducerRange;
 
   /// The source location of this lambda's capture-default ('=' or '&').
   SourceLocation CaptureDefaultLoc;
 
-  /// The number of captures.
-  unsigned NumCaptures : 16;
-
-  /// The default capture kind, which is a value of type
-  /// LambdaCaptureDefault.
-  unsigned CaptureDefault : 2;
-
-  /// Whether this lambda had an explicit parameter list vs. an
-  /// implicit (and empty) parameter list.
-  unsigned ExplicitParams : 1;
-
-  /// Whether this lambda had the result type explicitly specified.
-  unsigned ExplicitResultType : 1;
-
   /// The location of the closing brace ('}') that completes
   /// the lambda.
   ///
@@ -1867,15 +1855,9 @@ class LambdaExpr final : public Expr,
  SourceLocation ClosingBrace, bool 
ContainsUnexpandedParameterPack);
 
   /// Construct an empty lambda expression.
-  LambdaExpr(EmptyShell Empty, unsigned NumCaptures)
-  : Expr(LambdaExprClass, Empty), NumCaptures(NumCaptures),
-CaptureDefault(LCD_None), ExplicitParams(false),
-ExplicitResultType(false) {
-getStoredStmts()[NumCaptures] = nullptr;
-  }
+  LambdaExpr(EmptyShell Empty, unsigned NumCaptures);
 
   Stmt **getStoredStmts() { return getTrailingObjects(); }
-
   Stmt *const *getStoredStmts() const { return getTrailingObjects(); }
 
 public:
@@ -1898,13 +1880,11 @@ class LambdaExpr final : public Expr,
 
   /// Determine the default capture kind for this lambda.
   LambdaCaptureDefault getCaptureDefault() const {
-return static_cast(CaptureDefault);
+return static_cast(LambdaExprBits.CaptureDefault);
   }
 
   /// Retrieve the location of this lambda's capture-default, if any.
-  SourceLocation getCaptureDefaultLoc() const {
-return CaptureDefaultLoc;
-  }
+  SourceLocation getCaptureDefaultLoc() const { return CaptureDefaultLoc; }
 
   /// Determine whether one of this lambda's captures is an init-capture.
   bool isInitCapture(const LambdaCapture *Capture) const;
@@ -1927,7 +1907,7 @@ class LambdaExpr final : public Expr,
   capture_iterator capture_end() const;
 
   /// Determine the number of captures in this lambda.
-  unsigned capture_size() const { return NumCaptures; }
+  unsigned capture_size() const { return LambdaExprBits.NumCaptures; }
 
   /// Retrieve this lambda's explicit captures.
   capture_range explicit_captures() const;
@@ -1984,13 +1964,13 @@ class LambdaExpr final : public Expr,
   /// Retrieve the iterator pointing one past the last
   /// initialization argument for this lambda expression.
   capture_init_iterator capture_init_end() {
-return capture_init_begin() + NumCaptures;
+return capture_init_begin() + capture_size();
   }
 
   /// Retrieve the iterator pointing one past the last
   /// initialization argument for this lambda expression.
   const_capture_init_iterator capture_init_end() const {
-return capture_init_begin() + NumCaptures;
+return capture_init_begin() + capture_size();
   }
 
   /// Retrieve the source range covering the lambda introducer,
@@ -2033,10 +2013,12 @@ class LambdaExpr final : public Expr,
 
   /// Determine whether this lambda has an explicit parameter
   /// list vs. an implicit (empty) parameter list.
-  bool hasExplicitParameters() const { return ExplicitParams; }
+  bool hasExplicitParameters() const { return LambdaExprBits.ExplicitParams; }
 
   /// Whether this lambda had its result type explicitly specified.
-  bool hasExplicitResultType() const { return ExplicitResultType; }
+  bool hasExplicitResultType() const {
+return LambdaExprBits.ExplicitResultType;
+  }
 
   static bool classof(const Stmt *T) {
 return

Re: [PATCH] D15528: Teach clang-tidy how to -Werror checks.

2020-06-13 Thread Jon Roelofs via cfe-commits 
Oh, yeah, that should probably
be clamped.

I didn’t have a specific use case in mind, but in general I find that tools
that emit “richer” information can be used for more things, hence returning
the count, and not some fixed value.

On Sat, Jun 13, 2020 at 12:58 AM Nathan James via Phabricator <
revi...@reviews.llvm.org> wrote:

> njames93 added inline comments.
> Herald added a subscriber: arphaman.
>
>
> 
> Comment at: clang-tidy/tool/ClangTidyMain.cpp:362
> + << Plural << "\n";
> +return WErrorCount;
> +  }
> 
> Was there any specific reason for returning the error count instead of
> returning 1. It results in undefined behaviour on POSIX shells if a value
> is returned outside the range of 0<=n<=255. See
> https://bugs.llvm.org/show_bug.cgi?id=46305
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D15528/new/
>
> https://reviews.llvm.org/D15528
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71512: [clang-format] Fix short block when braking after control statement

2020-06-13 Thread MyDeveloperDay via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG0487f6f19cda: [clang-format] Fix short block when braking 
after control statement (authored by MyDeveloperDay).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71512

Files:
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -606,6 +606,12 @@
   verifyFormat("if CONSTEXPR (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
+  verifyFormat("if (true) { fff(); }",
+   AllowSimpleBracedStatements);
+  verifyFormat("if (true) {\n"
+   "  ();\n"
+   "}",
+   AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"
"  ff();\n"
"}",
@@ -681,6 +687,13 @@
   verifyFormat("if CONSTEXPR (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
+  verifyFormat("if (true) { fff(); }",
+   AllowSimpleBracedStatements);
+  verifyFormat("if (true)\n"
+   "{\n"
+   "  ();\n"
+   "}",
+   AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
"{\n"
"  ff();\n"
@@ -745,7 +758,9 @@
   Style.BreakBeforeBraces = FormatStyle::BS_Allman;
   EXPECT_EQ("#define A  \\\n"
 "  if (HANDLEwernufrnuLwrmviferuvnierv) \\\n"
-"  { RET_ERR1_ANUIREUINERUIFNIOAerwfwrvnuier; }\n"
+"  {\\\n"
+"RET_ERR1_ANUIREUINERUIFNIOAerwfwrvnuier;   \\\n"
+"  }\n"
 "X;",
 format("#define A \\\n"
"   if (HANDLEwernufrnuLwrmviferuvnierv) { \\\n"
Index: clang/lib/Format/UnwrappedLineFormatter.cpp
===
--- clang/lib/Format/UnwrappedLineFormatter.cpp
+++ clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -342,21 +342,6 @@
  ? 1
  : 0;
 }
-// Try to merge either empty or one-line block if is precedeed by control
-// statement token
-if (TheLine->First->is(tok::l_brace) && TheLine->First == TheLine->Last &&
-I != AnnotatedLines.begin() &&
-I[-1]->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for)) {
-  unsigned MergedLines = 0;
-  if (Style.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never) {
-MergedLines = tryMergeSimpleBlock(I - 1, E, Limit);
-// If we managed to merge the block, discard the first merged line
-// since we are merging starting from I.
-if (MergedLines > 0)
-  --MergedLines;
-  }
-  return MergedLines;
-}
 // Don't merge block with left brace wrapped after ObjC special blocks
 if (TheLine->First->is(tok::l_brace) && I != AnnotatedLines.begin() &&
 I[-1]->First->is(tok::at) && I[-1]->First->Next) {


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -606,6 +606,12 @@
   verifyFormat("if CONSTEXPR (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
+  verifyFormat("if (true) { fff(); }",
+   AllowSimpleBracedStatements);
+  verifyFormat("if (true) {\n"
+   "  ();\n"
+   "}",
+   AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"
"  ff();\n"
"}",
@@ -681,6 +687,13 @@
   verifyFormat("if CONSTEXPR (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
+  verifyFormat("if (true) { fff(); }",
+   AllowSimpleBracedStatements);
+  verifyFormat("if (true)\n"
+   "{\n"
+   "  ();\n"
+   "}",
+

[PATCH] D81786: [clang][utils] Modify make-ast-dump-check.sh to generate AST serialization dump tests

2020-06-13 Thread Bruno Ricci via Phabricator via cfe-commits 
riccibruno added a comment.

In D81786#2091387 , @lebedev.ri wrote:

> +1, thank you for looking into this!
>  I suspect there's a few issues to be discovered with this :)


Indeed; I already got one in D81787 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81786



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

[clang] 0487f6f - [clang-format] Fix short block when braking after control statement

2020-06-13 Thread via cfe-commits 

Author: mydeveloperday
Date: 2020-06-13T14:19:49+01:00
New Revision: 0487f6f19cdaaec81a5e2d6dfce7fd0cd3a9b6ca

URL: 
https://github.com/llvm/llvm-project/commit/0487f6f19cdaaec81a5e2d6dfce7fd0cd3a9b6ca
DIFF: 
https://github.com/llvm/llvm-project/commit/0487f6f19cdaaec81a5e2d6dfce7fd0cd3a9b6ca.diff

LOG: [clang-format] Fix short block when braking after control statement

Summary:
This patch fixes bug #44192

When clang-format is run with option AllowShortBlocksOnASingleLine, it is 
expected to either succeed in putting the short block with its control 
statement on a single line or fail and leave the block as is. When brace 
wrapping after control statement is activated, if the block + the control 
statement length is superior to column limit but the block alone is not, 
clang-format puts the block in two lines: one for the control statement and one 
for the block. This patch removes this unexpected behaviour. Current unittests 
are updated to check for this behaviour.

Patch By: Bouska

Reviewed By: MyDeveloperDay

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

Added: 


Modified: 
clang/lib/Format/UnwrappedLineFormatter.cpp
clang/unittests/Format/FormatTest.cpp

Removed: 




diff  --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index e86f9ecb4b90..3392a055c0c6 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -342,21 +342,6 @@ class LineJoiner {
  ? 1
  : 0;
 }
-// Try to merge either empty or one-line block if is precedeed by control
-// statement token
-if (TheLine->First->is(tok::l_brace) && TheLine->First == TheLine->Last &&
-I != AnnotatedLines.begin() &&
-I[-1]->First->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for)) {
-  unsigned MergedLines = 0;
-  if (Style.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never) {
-MergedLines = tryMergeSimpleBlock(I - 1, E, Limit);
-// If we managed to merge the block, discard the first merged line
-// since we are merging starting from I.
-if (MergedLines > 0)
-  --MergedLines;
-  }
-  return MergedLines;
-}
 // Don't merge block with left brace wrapped after ObjC special blocks
 if (TheLine->First->is(tok::l_brace) && I != AnnotatedLines.begin() &&
 I[-1]->First->is(tok::at) && I[-1]->First->Next) {

diff  --git a/clang/unittests/Format/FormatTest.cpp 
b/clang/unittests/Format/FormatTest.cpp
index 2befa3456ce0..dce71b0f0692 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -606,6 +606,12 @@ TEST_F(FormatTest, FormatShortBracedStatements) {
   verifyFormat("if CONSTEXPR (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
+  verifyFormat("if (true) { fff(); }",
+   AllowSimpleBracedStatements);
+  verifyFormat("if (true) {\n"
+   "  ();\n"
+   "}",
+   AllowSimpleBracedStatements);
   verifyFormat("if (true) {\n"
"  ff();\n"
"}",
@@ -681,6 +687,13 @@ TEST_F(FormatTest, FormatShortBracedStatements) {
   verifyFormat("if CONSTEXPR (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("while (true) { f(); }", AllowSimpleBracedStatements);
   verifyFormat("for (;;) { f(); }", AllowSimpleBracedStatements);
+  verifyFormat("if (true) { fff(); }",
+   AllowSimpleBracedStatements);
+  verifyFormat("if (true)\n"
+   "{\n"
+   "  ();\n"
+   "}",
+   AllowSimpleBracedStatements);
   verifyFormat("if (true)\n"
"{\n"
"  ff();\n"
@@ -745,7 +758,9 @@ TEST_F(FormatTest, 
ShortBlocksInMacrosDontMergeWithCodeAfterMacro) {
   Style.BreakBeforeBraces = FormatStyle::BS_Allman;
   EXPECT_EQ("#define A  \\\n"
 "  if (HANDLEwernufrnuLwrmviferuvnierv) \\\n"
-"  { RET_ERR1_ANUIREUINERUIFNIOAerwfwrvnuier; }\n"
+"  {\\\n"
+"RET_ERR1_ANUIREUINERUIFNIOAerwfwrvnuier;   \\\n"
+"  }\n"
 "X;",
 format("#define A \\\n"
"   if (HANDLEwernufrnuLwrmviferuvnierv) { \\\n"



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

[clang] f13d704 - [clang][NFC] Mark CWG 1443 (Default arguments and non-static data members)...

2020-06-13 Thread Bruno Ricci via cfe-commits 

Author: Bruno Ricci
Date: 2020-06-13T13:59:54+01:00
New Revision: f13d704a5014fa28d56240a6da7d1aa0b1c01f4d

URL: 
https://github.com/llvm/llvm-project/commit/f13d704a5014fa28d56240a6da7d1aa0b1c01f4d
DIFF: 
https://github.com/llvm/llvm-project/commit/f13d704a5014fa28d56240a6da7d1aa0b1c01f4d.diff

LOG: [clang][NFC] Mark CWG 1443 (Default arguments and non-static data 
members)...

...as done. This is a NAD which has always been implemented correctly.

Added: 


Modified: 
clang/test/CXX/drs/dr14xx.cpp

Removed: 




diff  --git a/clang/test/CXX/drs/dr14xx.cpp b/clang/test/CXX/drs/dr14xx.cpp
index d55427f5de8c..50b0396a4b79 100644
--- a/clang/test/CXX/drs/dr14xx.cpp
+++ b/clang/test/CXX/drs/dr14xx.cpp
@@ -4,10 +4,6 @@
 // RUN: %clang_cc1 -std=c++17 %s -verify -fexceptions -fcxx-exceptions 
-pedantic-errors
 // RUN: %clang_cc1 -std=c++2a %s -verify -fexceptions -fcxx-exceptions 
-pedantic-errors
 
-#if __cplusplus < 201103L
-// expected-no-diagnostics
-#endif
-
 namespace dr1423 { // dr1423: 11
 #if __cplusplus >= 201103L
   bool b1 = nullptr; // expected-error {{cannot initialize}}
@@ -17,6 +13,13 @@ namespace dr1423 { // dr1423: 11
 #endif
 }
 
+namespace dr1443 { // dr1443: yes
+struct A {
+  int i;
+  A() { void foo(int=i); } // expected-error {{default argument references 
'this'}}
+};
+}
+
 // dr1425: na abi
 
 namespace dr1460 { // dr1460: 3.5



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

[clang] 6a79f5a - [clang][NFC] Add an AST dump test for LambdaExpr

2020-06-13 Thread Bruno Ricci via cfe-commits 

Author: Bruno Ricci
Date: 2020-06-13T14:03:25+01:00
New Revision: 6a79f5aa5dbc2528444b4dfb92bb68039c5a32e9

URL: 
https://github.com/llvm/llvm-project/commit/6a79f5aa5dbc2528444b4dfb92bb68039c5a32e9
DIFF: 
https://github.com/llvm/llvm-project/commit/6a79f5aa5dbc2528444b4dfb92bb68039c5a32e9.diff

LOG: [clang][NFC] Add an AST dump test for LambdaExpr

This test illustrate the bug fixed in D81787.

Added: 
clang/test/AST/ast-dump-lambda.cpp

Modified: 


Removed: 




diff  --git a/clang/test/AST/ast-dump-lambda.cpp 
b/clang/test/AST/ast-dump-lambda.cpp
new file mode 100644
index ..3f10d12bdd6f
--- /dev/null
+++ b/clang/test/AST/ast-dump-lambda.cpp
@@ -0,0 +1,281 @@
+// Test without serialization:
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -Wno-unused-value 
-std=gnu++17 \
+// RUN: -ast-dump %s | FileCheck 
-strict-whitespace %s
+
+// Test with serialization:
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -Wno-unused-value 
-std=gnu++17 \
+// RUN:-emit-pch -o %t %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -Wno-unused-value 
-std=gnu++17 \
+// RUN:-x c++ -include-pch %t -ast-dump-all | FileCheck 
-strict-whitespace %s
+
+// XFAIL: *
+
+template  void test(Ts... a) {
+  struct V {
+void f() {
+  [this] {};
+  [*this] {};
+}
+  };
+  int b, c;
+  []() {};
+  [](int a, ...) {};
+  [a...] {};
+  [=] {};
+  [=] { return b; };
+  [&] {};
+  [&] { return c; };
+  [b, ] { return b + c; };
+  [a..., x = 12] {};
+  []() constexpr {};
+  []() mutable {};
+  []() noexcept {};
+  []() -> int { return 0; };
+}
+// CHECK: TranslationUnitDecl {{.*}} <> {{( 
)?}}
+// CHECK: `-FunctionTemplateDecl {{.*}} <{{.*}}ast-dump-lambda.cpp:13:1, 
line:34:1> line:13:32{{( imported)?}} test
+// CHECK-NEXT:   |-TemplateTypeParmDecl {{.*}}  col:23{{( 
imported)?}} referenced typename depth 0 index 0 ... Ts
+// CHECK-NEXT:   `-FunctionDecl {{.*}}  line:13:32{{( 
imported)?}} test 'void (Ts...)'
+// CHECK-NEXT: |-ParmVarDecl {{.*}}  col:43{{( 
imported)?}} referenced a 'Ts...' pack
+// CHECK-NEXT: `-CompoundStmt {{.*}} 
+// CHECK-NEXT:   |-DeclStmt {{.*}} 
+// CHECK-NEXT:   | `-CXXRecordDecl {{.*}}  
line:14:10{{( imported)?}}{{( )?}} struct V 
definition
+// CHECK-NEXT:   |   |-DefinitionData empty aggregate standard_layout 
trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor 
can_const_default_init
+// CHECK-NEXT:   |   | |-DefaultConstructor exists trivial constexpr 
needs_implicit defaulted_is_constexpr
+// CHECK-NEXT:   |   | |-CopyConstructor simple trivial has_const_param 
needs_implicit implicit_has_const_param
+// CHECK-NEXT:   |   | |-MoveConstructor exists simple trivial 
needs_implicit
+// CHECK-NEXT:   |   | |-CopyAssignment simple trivial has_const_param 
needs_implicit implicit_has_const_param
+// CHECK-NEXT:   |   | |-MoveAssignment exists simple trivial 
needs_implicit
+// CHECK-NEXT:   |   | `-Destructor simple irrelevant trivial 
needs_implicit
+// CHECK-NEXT:   |   |-CXXRecordDecl {{.*}}  col:10{{( 
imported)?}} implicit struct V
+// CHECK-NEXT:   |   `-CXXMethodDecl {{.*}}  
line:15:10{{( imported)?}} f 'void ()'
+// CHECK-NEXT:   | `-CompoundStmt {{.*}} 
+// CHECK-NEXT:   |   |-LambdaExpr {{.*}}  '(lambda 
at {{.*}}ast-dump-lambda.cpp:16:7)'
+// CHECK-NEXT:   |   | |-CXXRecordDecl {{.*}}  col:7{{( 
imported)?}} implicit{{( )?}} class definition
+// CHECK-NEXT:   |   | | |-DefinitionData lambda standard_layout 
trivially_copyable can_const_default_init
+// CHECK-NEXT:   |   | | | |-DefaultConstructor
+// CHECK-NEXT:   |   | | | |-CopyConstructor simple trivial 
has_const_param needs_implicit implicit_has_const_param
+// CHECK-NEXT:   |   | | | |-MoveConstructor exists simple trivial 
needs_implicit
+// CHECK-NEXT:   |   | | | |-CopyAssignment trivial has_const_param 
needs_implicit implicit_has_const_param
+// CHECK-NEXT:   |   | | | |-MoveAssignment
+// CHECK-NEXT:   |   | | | `-Destructor simple irrelevant trivial 
needs_implicit
+// CHECK-NEXT:   |   | | |-CXXMethodDecl {{.*}}  
col:7{{( imported)?}} operator() 'auto () const -> auto' inline
+// CHECK-NEXT:   |   | | | `-CompoundStmt {{.*}} 
+// CHECK-NEXT:   |   | | `-FieldDecl {{.*}}  col:8{{( 
imported)?}} implicit 'V *'
+// CHECK-NEXT:   |   | |-ParenListExpr {{.*}}  'NULL TYPE'
+// CHECK-NEXT:   |   | | `-CXXThisExpr {{.*}}  'V *' this
+// CHECK-NEXT:   |   | `-<<>>
+// CHECK-NEXT:   |   `-LambdaExpr {{.*}}  '(lambda 
at {{.*}}ast-dump-lambda.cpp:17:7)'
+// CHECK-NEXT:   | |-CXXRecordDecl {{.*}}  col:7{{( 
imported)?}} implicit{{( )?}} class definition
+// CHECK-NEXT:   | | |-DefinitionData lambda standard_layout 
trivially_copyable

[clang] eb614db - [clang][NFC] Mark CWG 974 and 1814 (default argument in a...

2020-06-13 Thread Bruno Ricci via cfe-commits 

Author: Bruno Ricci
Date: 2020-06-13T13:49:07+01:00
New Revision: eb614db0a0b41734ef52c2cdd87461f0ca62a900

URL: 
https://github.com/llvm/llvm-project/commit/eb614db0a0b41734ef52c2cdd87461f0ca62a900
DIFF: 
https://github.com/llvm/llvm-project/commit/eb614db0a0b41734ef52c2cdd87461f0ca62a900.diff

LOG: [clang][NFC] Mark CWG 974 and 1814 (default argument in a...

...lambda-expression) as done. They have been allowed since at least clang 3.3.

Added: 


Modified: 
clang/test/CXX/drs/dr18xx.cpp
clang/test/CXX/drs/dr9xx.cpp

Removed: 




diff  --git a/clang/test/CXX/drs/dr18xx.cpp b/clang/test/CXX/drs/dr18xx.cpp
index 33c0452b6c09..6cf526345af3 100644
--- a/clang/test/CXX/drs/dr18xx.cpp
+++ b/clang/test/CXX/drs/dr18xx.cpp
@@ -31,6 +31,14 @@ namespace dr1813 { // dr1813: 7
   static_assert(!__is_standard_layout(U), "");
 }
 
+namespace dr1814 { // dr1814: yes
+#if __cplusplus >= 201103L
+  void test() {
+auto lam = [](int x = 42) { return x; };
+  }
+#endif
+}
+
 namespace dr1815 { // dr1815: no
 #if __cplusplus >= 201402L
   // FIXME: needs codegen test

diff  --git a/clang/test/CXX/drs/dr9xx.cpp b/clang/test/CXX/drs/dr9xx.cpp
index b37e17d6b098..e8c22b2972ce 100644
--- a/clang/test/CXX/drs/dr9xx.cpp
+++ b/clang/test/CXX/drs/dr9xx.cpp
@@ -73,3 +73,11 @@ namespace dr948 { // dr948: 3.7
   }
 #endif
 }
+
+namespace dr974 { // dr974: yes
+#if __cplusplus >= 201103L
+  void test() {
+auto lam = [](int x = 42) { return x; };
+  }
+#endif
+}



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

[PATCH] D81786: [clang][utils] Modify make-ast-dump-check.sh to generate AST serialization dump tests

2020-06-13 Thread Roman Lebedev via Phabricator via cfe-commits 
lebedev.ri added a comment.

+1, thank you for looking into this!
I suspect there's a few issues to be discovered with this :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81786



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

[PATCH] D81787: [clang] Fix the serialization of LambdaExpr and the bogus mutation in LambdaExpr::getBody

2020-06-13 Thread Bruno Ricci via Phabricator via cfe-commits 
riccibruno created this revision.
riccibruno added reviewers: jyknight, GorNishanov, aaron.ballman.
riccibruno added a project: clang.
Herald added subscribers: cfe-commits, modocache.

The body of `LambdaExpr` is currently not properly serialized. Instead 
`LambdaExpr::getBody`
checks if the body has been already deserialized and if not mutates 
`LambdaExpr`. This can be
observed with an AST dump test, where the body of the `LambdaExpr` will be null.

The mutation in `LambdaExpr::getBody` was left because of another bug: it is 
not true that the body
of a `LambdaExpr` is always a `CompoundStmt`; it can also be a 
`CoroutineBodyStmt` wrapping a
`CompoundStmt`. This is fixed by returning a `Stmt *` from `getBody` and 
introducing a convenience
function `getCompoundStmtBody` which always returns a `CompoundStmt *`. This 
function can be
used by callers who do not care about the coroutine node.

Happily all but one user of `getBody` treat it as a `Stmt *` and so this change 
is non-intrusive.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81787

Files:
  clang/include/clang/AST/ExprCXX.h
  clang/lib/AST/ExprCXX.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/test/AST/ast-dump-lambda.cpp

Index: clang/test/AST/ast-dump-lambda.cpp
===
--- clang/test/AST/ast-dump-lambda.cpp
+++ clang/test/AST/ast-dump-lambda.cpp
@@ -8,7 +8,7 @@
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown -Wno-unused-value -std=gnu++17 \
 // RUN:-x c++ -include-pch %t -ast-dump-all | FileCheck -strict-whitespace %s
 
-// XFAIL: *
+
 
 template  void test(Ts... a) {
   struct V {
@@ -64,7 +64,7 @@
 // CHECK-NEXT:   |   | | `-FieldDecl {{.*}}  col:8{{( imported)?}} implicit 'V *'
 // CHECK-NEXT:   |   | |-ParenListExpr {{.*}}  'NULL TYPE'
 // CHECK-NEXT:   |   | | `-CXXThisExpr {{.*}}  'V *' this
-// CHECK-NEXT:   |   | `-<<>>
+// CHECK-NEXT:   |   | `-CompoundStmt {{.*}} 
 // CHECK-NEXT:   |   `-LambdaExpr {{.*}}  '(lambda at {{.*}}ast-dump-lambda.cpp:17:7)'
 // CHECK-NEXT:   | |-CXXRecordDecl {{.*}}  col:7{{( imported)?}} implicit{{( )?}} class definition
 // CHECK-NEXT:   | | |-DefinitionData lambda standard_layout trivially_copyable can_const_default_init
@@ -80,7 +80,7 @@
 // CHECK-NEXT:   | |-ParenListExpr {{.*}}  'NULL TYPE'
 // CHECK-NEXT:   | | `-UnaryOperator {{.*}}  '' prefix '*' cannot overflow
 // CHECK-NEXT:   | |   `-CXXThisExpr {{.*}}  'V *' this
-// CHECK-NEXT:   | `-<<>>
+// CHECK-NEXT:   | `-CompoundStmt {{.*}} 
 // CHECK-NEXT:   |-DeclStmt {{.*}} 
 // CHECK-NEXT:   | |-VarDecl {{.*}}  col:7{{( imported)?}} referenced b 'int'
 // CHECK-NEXT:   | `-VarDecl {{.*}}  col:10{{( imported)?}} referenced c 'int'
@@ -97,7 +97,7 @@
 // CHECK-NEXT:   | | | `-CompoundStmt {{.*}} 
 // CHECK-NEXT:   | | |-CXXConversionDecl {{.*}}  col:3{{( imported)?}} implicit constexpr operator auto (*)() 'auto (*() const noexcept)()' inline
 // CHECK-NEXT:   | | `-CXXMethodDecl {{.*}}  col:3{{( imported)?}} implicit __invoke 'auto ()' static inline
-// CHECK-NEXT:   | `-<<>>
+// CHECK-NEXT:   | `-CompoundStmt {{.*}} 
 // CHECK-NEXT:   |-LambdaExpr {{.*}}  '(lambda at {{.*}}ast-dump-lambda.cpp:22:3)'
 // CHECK-NEXT:   | |-CXXRecordDecl {{.*}}  col:3{{( imported)?}} implicit{{( )?}} class definition
 // CHECK-NEXT:   | | |-DefinitionData lambda empty standard_layout trivially_copyable literal can_const_default_init
@@ -113,7 +113,7 @@
 // CHECK-NEXT:   | | |-CXXConversionDecl {{.*}}  col:3{{( imported)?}} implicit constexpr operator auto (*)(int, ...) 'auto (*() const noexcept)(int, ...)' inline
 // CHECK-NEXT:   | | `-CXXMethodDecl {{.*}}  col:3{{( imported)?}} implicit __invoke 'auto (int, ...)' static inline
 // CHECK-NEXT:   | |   `-ParmVarDecl {{.*}}  col:10{{( imported)?}} a 'int'
-// CHECK-NEXT:   | `-<<>>
+// CHECK-NEXT:   | `-CompoundStmt {{.*}} 
 // CHECK-NEXT:   |-LambdaExpr {{.*}}  '(lambda at {{.*}}ast-dump-lambda.cpp:23:3)'
 // CHECK-NEXT:   | |-CXXRecordDecl {{.*}}  col:3{{( imported)?}} implicit{{( )?}} class definition
 // CHECK-NEXT:   | | |-DefinitionData lambda standard_layout trivially_copyable can_const_default_init
@@ -128,7 +128,7 @@
 // CHECK-NEXT:   | | `-FieldDecl {{.*}}  col:4{{( imported)?}} implicit 'Ts...'
 // CHECK-NEXT:   | |-ParenListExpr {{.*}}  'NULL TYPE'
 // CHECK-NEXT:   | | `-DeclRefExpr {{.*}}  'Ts...' lvalue ParmVar {{.*}} 'a' 'Ts...'
-// CHECK-NEXT:   | `-<<>>
+// CHECK-NEXT:   | `-CompoundStmt {{.*}} 
 // CHECK-NEXT:   |-LambdaExpr {{.*}}  '(lambda at {{.*}}ast-dump-lambda.cpp:24:3)'
 // CHECK-NEXT:   | |-CXXRecordDecl {{.*}}  col:3{{( imported)?}} implicit{{( )?}} class definition
 // CHECK-NEXT:

[PATCH] D81786: [clang][utils] Modify make-ast-dump-check.sh to generate AST serialization dump tests

2020-06-13 Thread Bruno Ricci via Phabricator via cfe-commits 
riccibruno created this revision.
riccibruno added reviewers: aaron.ballman, stephenkelly.
riccibruno added a project: clang.
Herald added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, sstefan1.
riccibruno added a comment.

Example of a test: F12153075: ast-dump-lambda.cpp 



An AST serialization dump test is a test which compares the output of 
`-ast-dump` on the source and of `-ast-dump-all` on a PCH generated from the 
source. Modulo a few differences the outputs should match.

This patch to `make-ast-dump-check.sh` enables automatically generating these 
tests.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81786

Files:
  clang/utils/make-ast-dump-check.sh


Index: clang/utils/make-ast-dump-check.sh
===
--- clang/utils/make-ast-dump-check.sh
+++ clang/utils/make-ast-dump-check.sh
@@ -3,12 +3,21 @@
 # This script is intended as a FileCheck replacement to update the test
 # expectations in a -ast-dump test.
 #
-# Usage:
+# Usage (to generate normal AST dump tests):
 #
 # $ lit -DFileCheck=$PWD/utils/make-ast-dump-check.sh 
test/AST/ast-dump-openmp-*
+#
+# Usage (to generate serialization AST dump tests):
+#
+# $ lit -DFileCheck="generate_serialization_test=1 
$PWD/utils/make-ast-dump-check.sh"
+# test/AST/ast-dump-openmp-*
 
 prefix=CHECK
 
+if [ -z ${generate_serialization_test+x} ];
+  then generate_serialization_test=0;
+fi
+
 while [[ "$#" -ne 0 ]]; do
   case "$1" in
   --check-prefix)
@@ -54,6 +63,10 @@
   s = \$0
   gsub("0x[0-9a-fA-F]+", "{{.*}}", s)
   gsub("$testdir/", "{{.*}}", s)
+  if ($generate_serialization_test == 1) {
+gsub(" imported", "{{( imported)?}}", s)
+gsub(" ", "{{( )?}}", s)
+  }
 }
 
 matched_last_line == 0 {


Index: clang/utils/make-ast-dump-check.sh
===
--- clang/utils/make-ast-dump-check.sh
+++ clang/utils/make-ast-dump-check.sh
@@ -3,12 +3,21 @@
 # This script is intended as a FileCheck replacement to update the test
 # expectations in a -ast-dump test.
 #
-# Usage:
+# Usage (to generate normal AST dump tests):
 #
 # $ lit -DFileCheck=$PWD/utils/make-ast-dump-check.sh test/AST/ast-dump-openmp-*
+#
+# Usage (to generate serialization AST dump tests):
+#
+# $ lit -DFileCheck="generate_serialization_test=1 $PWD/utils/make-ast-dump-check.sh"
+# test/AST/ast-dump-openmp-*
 
 prefix=CHECK
 
+if [ -z ${generate_serialization_test+x} ];
+  then generate_serialization_test=0;
+fi
+
 while [[ "$#" -ne 0 ]]; do
   case "$1" in
   --check-prefix)
@@ -54,6 +63,10 @@
   s = \$0
   gsub("0x[0-9a-fA-F]+", "{{.*}}", s)
   gsub("$testdir/", "{{.*}}", s)
+  if ($generate_serialization_test == 1) {
+gsub(" imported", "{{( imported)?}}", s)
+gsub(" ", "{{( )?}}", s)
+  }
 }
 
 matched_last_line == 0 {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81786: [clang][utils] Modify make-ast-dump-check.sh to generate AST serialization dump tests

2020-06-13 Thread Bruno Ricci via Phabricator via cfe-commits 
riccibruno added a comment.

Example of a test: F12153075: ast-dump-lambda.cpp 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81786



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

[PATCH] D81785: [clangd] Fix readability-else-after-return 'Adding a note without main diagnostic' crash

2020-06-13 Thread Nathan James via Phabricator via cfe-commits 
njames93 created this revision.
njames93 added reviewers: sammccall, hokein.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous, 
MaskRay, ilya-biryukov.
Herald added a project: clang.
njames93 added a comment.

The actual fix in `ElseAfterReturnCheck.cpp` is needed. 
However clangd's handling of Remarks is a little suspicious. Remarks are 
supposed to be different to notes in the sense they are designed to be stand 
alone, unlike notes which depend on a another diagnostic. see Add 'remark' 
diagnostic type in 'clang' 

This seems to be how clangd determines what a note is:

  bool isNote(DiagnosticsEngine::Level L) {
return L == DiagnosticsEngine::Note || L == DiagnosticsEngine::Remark;
  }


Fix a crash in clangd caused by an (admittidly incorrect) Remark diagnositic 
being emitted from readability-else-after-return.
This crash doesn't occur in clang-tidy so there are no tests there for this.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81785

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp


Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -299,6 +299,25 @@
   DiagSource(Diag::ClangTidy), DiagName("modernize-loop-convert";
 }
 
+TEST(DiagnosticTest, NoAddingANoteWithoutMainDiagnostic) {
+  Annotations Main(R"cc(
+void foo() {
+  if (int X = 0) {
+return;
+  } [[else]] {
+X++;
+  }
+}
+  )cc");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.ClangTidyChecks = "readability-else-after-return";
+  EXPECT_THAT(TU.build().getDiagnostics(),
+  UnorderedElementsAre(::testing::AllOf(
+  Diag(Main.range(), "do not use 'else' after 'return'"),
+  DiagSource(Diag::ClangTidy),
+  DiagName("readability-else-after-return";
+}
+
 TEST(DiagnosticTest, ClangTidySuppressionComment) {
   Annotations Main(R"cpp(
 int main() {
Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -188,9 +188,8 @@
 if (IsLastInScope) {
   // If the if statement is the last statement its enclosing statements
   // scope, we can pull the decl out of the if statement.
-  DiagnosticBuilder Diag =
-  diag(ElseLoc, WarningMessage, clang::DiagnosticIDs::Level::Remark)
-  << ControlFlowInterruptor;
+  DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
+   << ControlFlowInterruptor;
   if (checkInitDeclUsageInElse(If) != nullptr) {
 Diag << tooling::fixit::createReplacement(
 SourceRange(If->getIfLoc()),


Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -299,6 +299,25 @@
   DiagSource(Diag::ClangTidy), DiagName("modernize-loop-convert";
 }
 
+TEST(DiagnosticTest, NoAddingANoteWithoutMainDiagnostic) {
+  Annotations Main(R"cc(
+void foo() {
+  if (int X = 0) {
+return;
+  } [[else]] {
+X++;
+  }
+}
+  )cc");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.ClangTidyChecks = "readability-else-after-return";
+  EXPECT_THAT(TU.build().getDiagnostics(),
+  UnorderedElementsAre(::testing::AllOf(
+  Diag(Main.range(), "do not use 'else' after 'return'"),
+  DiagSource(Diag::ClangTidy),
+  DiagName("readability-else-after-return";
+}
+
 TEST(DiagnosticTest, ClangTidySuppressionComment) {
   Annotations Main(R"cpp(
 int main() {
Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -188,9 +188,8 @@
 if (IsLastInScope) {
   // If the if statement is the last statement its enclosing statements
   // scope, we can pull the decl out of the if statement.
-  DiagnosticBuilder Diag =
-  diag(ElseLoc, WarningMessage, clang::DiagnosticIDs::Level::Remark)
-  << ControlFlowInterruptor;
+  DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
+   << ControlFlowInterruptor;
   if

[PATCH] D81785: [clangd] Fix readability-else-after-return 'Adding a note without main diagnostic' crash

2020-06-13 Thread Nathan James via Phabricator via cfe-commits 
njames93 added a comment.

The actual fix in `ElseAfterReturnCheck.cpp` is needed. 
However clangd's handling of Remarks is a little suspicious. Remarks are 
supposed to be different to notes in the sense they are designed to be stand 
alone, unlike notes which depend on a another diagnostic. see Add 'remark' 
diagnostic type in 'clang' 

This seems to be how clangd determines what a note is:

  bool isNote(DiagnosticsEngine::Level L) {
return L == DiagnosticsEngine::Note || L == DiagnosticsEngine::Remark;
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81785



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

[libunwind] 0c1a135 - [libunwind][RISCV] Track PC separately from RA

2020-06-13 Thread Amanieu d'Antras via cfe-commits 

Author: Amanieu d'Antras
Date: 2020-06-13T08:15:40+01:00
New Revision: 0c1a135adae530b88f68c9425fb85bd8fb9152ca

URL: 
https://github.com/llvm/llvm-project/commit/0c1a135adae530b88f68c9425fb85bd8fb9152ca
DIFF: 
https://github.com/llvm/llvm-project/commit/0c1a135adae530b88f68c9425fb85bd8fb9152ca.diff

LOG: [libunwind][RISCV] Track PC separately from RA

Summary:
This allows unwinding to work across signal handler frames where the IP of the 
previous frame is not the same as the current value of the RA register. This is 
particularly useful for acquiring backtraces from signal handlers.

I kept the size of the context structure the same to avoid ABI breakage; the PC 
is stored in the previously unused slot for register 0.

Reviewers: #libunwind, mhorne, lenary, luismarques, arichardson, compnerd

Reviewed By: #libunwind, mhorne, lenary, compnerd

Subscribers: kamleshbhalui, jrtc27, bsdjhb, arichardson, compnerd, simoncook, 
kito-cheng, shiva0217, rogfer01, rkruppe, psnobl, benna, Jim, s.egerton, 
sameer.abuasal, evandro, llvm-commits, libcxx-commits

Tags: #libunwind, #llvm

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

Added: 


Modified: 
libunwind/src/Registers.hpp
libunwind/src/UnwindRegistersRestore.S
libunwind/src/UnwindRegistersSave.S

Removed: 




diff  --git a/libunwind/src/Registers.hpp b/libunwind/src/Registers.hpp
index 26a0fa8f338e..c76b05bf314e 100644
--- a/libunwind/src/Registers.hpp
+++ b/libunwind/src/Registers.hpp
@@ -3734,11 +3734,11 @@ class _LIBUNWIND_HIDDEN Registers_riscv {
 
   uint64_t  getSP() const { return _registers[2]; }
   void  setSP(uint64_t value) { _registers[2] = value; }
-  uint64_t  getIP() const { return _registers[1]; }
-  void  setIP(uint64_t value) { _registers[1] = value; }
+  uint64_t  getIP() const { return _registers[0]; }
+  void  setIP(uint64_t value) { _registers[0] = value; }
 
 private:
-
+  // _registers[0] holds the pc
   uint64_t _registers[32];
   double   _floats[32];
 };
@@ -3773,7 +3773,7 @@ inline bool Registers_riscv::validRegister(int regNum) 
const {
 
 inline uint64_t Registers_riscv::getRegister(int regNum) const {
   if (regNum == UNW_REG_IP)
-return _registers[1];
+return _registers[0];
   if (regNum == UNW_REG_SP)
 return _registers[2];
   if (regNum == UNW_RISCV_X0)
@@ -3785,7 +3785,7 @@ inline uint64_t Registers_riscv::getRegister(int regNum) 
const {
 
 inline void Registers_riscv::setRegister(int regNum, uint64_t value) {
   if (regNum == UNW_REG_IP)
-_registers[1] = value;
+_registers[0] = value;
   else if (regNum == UNW_REG_SP)
 _registers[2] = value;
   else if (regNum == UNW_RISCV_X0)

diff  --git a/libunwind/src/UnwindRegistersRestore.S 
b/libunwind/src/UnwindRegistersRestore.S
index 9ad521591918..5d5443215286 100644
--- a/libunwind/src/UnwindRegistersRestore.S
+++ b/libunwind/src/UnwindRegistersRestore.S
@@ -1117,7 +1117,7 @@ 
DEFINE_LIBUNWIND_FUNCTION(_ZN9libunwind15Registers_riscv6jumptoEv)
 #endif
 
   // x0 is zero
-  ldx1, (8 * 1)(a0)
+  ldx1, (8 * 0)(a0) // restore pc into ra
   ldx2, (8 * 2)(a0)
   ldx3, (8 * 3)(a0)
   ldx4, (8 * 4)(a0)

diff  --git a/libunwind/src/UnwindRegistersSave.S 
b/libunwind/src/UnwindRegistersSave.S
index 9e52c4c9b772..51bb9b0688fd 100644
--- a/libunwind/src/UnwindRegistersSave.S
+++ b/libunwind/src/UnwindRegistersSave.S
@@ -1030,7 +1030,7 @@ DEFINE_LIBUNWIND_FUNCTION(__unw_getcontext)
 #  thread_state pointer is in a0
 #
 DEFINE_LIBUNWIND_FUNCTION(__unw_getcontext)
-  // x0 is zero
+  sdx1, (8 * 0)(a0) // store ra as pc
   sdx1, (8 * 1)(a0)
   sdx2, (8 * 2)(a0)
   sdx3, (8 * 3)(a0)



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

[PATCH] D15528: Teach clang-tidy how to -Werror checks.

2020-06-13 Thread Nathan James via Phabricator via cfe-commits 
njames93 added inline comments.
Herald added a subscriber: arphaman.



Comment at: clang-tidy/tool/ClangTidyMain.cpp:362
+ << Plural << "\n";
+return WErrorCount;
+  }

Was there any specific reason for returning the error count instead of 
returning 1. It results in undefined behaviour on POSIX shells if a value is 
returned outside the range of 0<=n<=255. See 
https://bugs.llvm.org/show_bug.cgi?id=46305


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

https://reviews.llvm.org/D15528



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