[PATCH] D36527: Implemented P0428R2 - Familiar template syntax for generic lambdas

[Hamza Sood via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL359967: [c++20] Implement P0428R2 - Familiar template syntax 
for generic lambdas (authored by hamzasood, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D36527?vs=177221=198134#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D36527

Files:
  cfe/trunk/include/clang/AST/DeclCXX.h
  cfe/trunk/include/clang/AST/DeclTemplate.h
  cfe/trunk/include/clang/AST/ExprCXX.h
  cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
  cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
  cfe/trunk/include/clang/Parse/Parser.h
  cfe/trunk/include/clang/Sema/ScopeInfo.h
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/lib/AST/DeclCXX.cpp
  cfe/trunk/lib/AST/DeclPrinter.cpp
  cfe/trunk/lib/AST/ExprCXX.cpp
  cfe/trunk/lib/AST/ItaniumMangle.cpp
  cfe/trunk/lib/AST/StmtPrinter.cpp
  cfe/trunk/lib/AST/TypePrinter.cpp
  cfe/trunk/lib/Parse/ParseExprCXX.cpp
  cfe/trunk/lib/Sema/Sema.cpp
  cfe/trunk/lib/Sema/SemaLambda.cpp
  cfe/trunk/lib/Sema/SemaType.cpp
  cfe/trunk/test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  cfe/trunk/test/CodeGenCXX/mangle-lambda-explicit-template-params.cpp
  cfe/trunk/test/Index/print-display-names.cpp
  cfe/trunk/test/PCH/cxx11-lambdas.mm
  cfe/trunk/test/PCH/cxx1y-lambdas.mm
  cfe/trunk/test/PCH/cxx2a-template-lambdas.cpp
  cfe/trunk/test/Parser/cxx2a-template-lambdas.cpp
  cfe/trunk/test/SemaCXX/cxx2a-template-lambdas.cpp
  cfe/trunk/unittests/AST/StmtPrinterTest.cpp
  cfe/trunk/unittests/Tooling/RecursiveASTVisitorTests/LambdaTemplateParams.cpp
  cfe/trunk/www/cxx_status.html

Index: cfe/trunk/test/PCH/cxx2a-template-lambdas.cpp
===
--- cfe/trunk/test/PCH/cxx2a-template-lambdas.cpp
+++ cfe/trunk/test/PCH/cxx2a-template-lambdas.cpp
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -std=c++2a -emit-pch %s -o %t
+// RUN: %clang_cc1 -std=c++2a -include-pch %t -verify %s
+
+// expected-no-diagnostics
+
+#ifndef HEADER
+#define HEADER
+
+auto l1 = []() constexpr -> int {
+return I;
+};
+
+auto l2 = []() constexpr -> decltype(I) {
+return I;
+};
+
+auto l3 = [](auto i) constexpr -> T {
+  return T(i);
+};
+
+auto l4 = [] class T, class U>(T, auto i) constexpr -> U {
+  return U(i);
+};
+
+#else /*included pch*/
+
+static_assert(l1.operator()<5>() == 5);
+static_assert(l1.operator()<6>() == 6);
+
+static_assert(l2.operator()<7>() == 7);
+static_assert(l2.operator()() == nullptr);
+
+static_assert(l3.operator()(8.4) == 8);
+static_assert(l3.operator()(9.9) == 9);
+
+template
+struct DummyTemplate { };
+
+static_assert(l4(DummyTemplate(), 12) == 12.0);
+static_assert(l4(DummyTemplate(), 19.8) == 19);
+
+#endif // HEADER
Index: cfe/trunk/test/PCH/cxx11-lambdas.mm
===
--- cfe/trunk/test/PCH/cxx11-lambdas.mm
+++ cfe/trunk/test/PCH/cxx11-lambdas.mm
@@ -54,7 +54,7 @@
 }
 
 // CHECK-PRINT: inline int add_int_slowly_twice 
-// CHECK-PRINT: lambda = [&] (int z)
+// CHECK-PRINT: lambda = [&](int z)
 
 // CHECK-PRINT: init_capture
 // CHECK-PRINT: [&, x(t)]
Index: cfe/trunk/test/PCH/cxx1y-lambdas.mm
===
--- cfe/trunk/test/PCH/cxx1y-lambdas.mm
+++ cfe/trunk/test/PCH/cxx1y-lambdas.mm
@@ -50,7 +50,7 @@
 }
 
 // CHECK-PRINT: inline int add_int_slowly_twice 
-// CHECK-PRINT: lambda = [] (type-parameter-0-0 z
+// CHECK-PRINT: lambda = [](auto z
 
 // CHECK-PRINT: init_capture
 // CHECK-PRINT: [&, x(t)]
Index: cfe/trunk/test/SemaCXX/cxx2a-template-lambdas.cpp
===
--- cfe/trunk/test/SemaCXX/cxx2a-template-lambdas.cpp
+++ cfe/trunk/test/SemaCXX/cxx2a-template-lambdas.cpp
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -std=c++2a -verify %s
+
+template
+constexpr bool is_same = false;
+
+template
+constexpr bool is_same = true;
+
+template
+struct DummyTemplate { };
+
+void func() {
+  auto L0 = [](T arg) {
+static_assert(is_same); // expected-error {{static_assert failed}}
+  };
+  L0(0);
+  L0(0.0); // expected-note {{in instantiation}}
+
+  auto L1 = [] {
+static_assert(I == 5); // expected-error {{static_assert failed}}
+  };
+  L1.operator()<5>();
+  L1.operator()<6>(); // expected-note {{in instantiation}}
+
+  auto L2 = [] class T, class U>(T &) {
+static_assert(is_same, DummyTemplate>); // // expected-error {{static_assert failed}}
+  };
+  L2(DummyTemplate());
+  L2(DummyTemplate()); // expected-note {{in instantiation}}
+}
+
+template // expected-note {{declared here}}
+struct ShadowMe {
+  void member_func() {
+auto L = [] { }; // expected-error {{'T' shadows template parameter}}
+  }
+};
+
+template
+constexpr T outer() {
+  return []() { return x; }.template operator()<123>(); // expected-error {{no matching member 

[PATCH] D36527: Implemented P0428R2 - Familiar template syntax for generic lambdas

[Hamza Sood via Phabricator via cfe-commits]

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



Comment at: lib/AST/DeclPrinter.cpp:107-108
 
-void printTemplateParameters(const TemplateParameterList *Params);
+void printTemplateParameters(const TemplateParameterList *Params,
+ bool OmitTemplateKW = false);
 void printTemplateArguments(const TemplateArgumentList ,

rsmith wrote:
> Nit: I'd prefer splitting this into two functions (one that prints 
> 'template', calls the other, then prints a space, and the other to print the 
> actual template parameters) rather than passing a boolean flag.
Do you reckon that the boolean flag is okay for `TemplateParameterList::print`? 
I've left this change for now until that has been decided.


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

https://reviews.llvm.org/D36527



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

[PATCH] D36527: Implemented P0428R2 - Familiar template syntax for generic lambdas

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 177221.
hamzasood added a comment.

- Committed the test framework changes separately as r348589 and  r348603.
- Correctly cleanup the lambda scope if an error in encountered while parsing 
the template parameters.
- Replaced getExplicitTemplateParameterCount with 
getExplicitTemplateParameters, which returns an ArrayRef. This simplifies the 
handling of the parameters in a few places.
- Replaced a linear search over the template parameters with a binary search.
- Added expected error cases to the template lambda usage tests to ensure that 
the static assertions are correctly instantiated.
- Added a test for templated lambdas that're used within another template; 
TreeTransform already transforms the template parameter list so no change was 
needed to make the test pass.


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

https://reviews.llvm.org/D36527

Files:
  include/clang/AST/DeclCXX.h
  include/clang/AST/DeclTemplate.h
  include/clang/AST/ExprCXX.h
  include/clang/AST/RecursiveASTVisitor.h
  include/clang/Basic/DiagnosticParseKinds.td
  include/clang/Parse/Parser.h
  include/clang/Sema/ScopeInfo.h
  include/clang/Sema/Sema.h
  lib/AST/DeclCXX.cpp
  lib/AST/DeclPrinter.cpp
  lib/AST/ExprCXX.cpp
  lib/AST/ItaniumMangle.cpp
  lib/AST/StmtPrinter.cpp
  lib/AST/TypePrinter.cpp
  lib/Parse/ParseExprCXX.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaType.cpp
  test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  test/CodeGenCXX/mangle-lambda-explicit-template-params.cpp
  test/Index/print-display-names.cpp
  test/PCH/cxx11-lambdas.mm
  test/PCH/cxx1y-lambdas.mm
  test/PCH/cxx2a-template-lambdas.cpp
  test/Parser/cxx2a-template-lambdas.cpp
  test/SemaCXX/cxx2a-template-lambdas.cpp
  unittests/AST/StmtPrinterTest.cpp
  unittests/Tooling/RecursiveASTVisitorTests/LambdaTemplateParams.cpp
  www/cxx_status.html

Index: www/cxx_status.html
===
--- www/cxx_status.html
+++ www/cxx_status.html
@@ -860,7 +860,7 @@
 
   template-parameter-list for generic lambdas
   http://wg21.link/p0428r2;>P0428R2
-  No
+  SVN
 
 
   Concepts
Index: unittests/Tooling/RecursiveASTVisitorTests/LambdaTemplateParams.cpp
===
--- unittests/Tooling/RecursiveASTVisitorTests/LambdaTemplateParams.cpp
+++ unittests/Tooling/RecursiveASTVisitorTests/LambdaTemplateParams.cpp
@@ -0,0 +1,53 @@
+//===- unittest/Tooling/RecursiveASTVisitorTests/LambdaTemplateParams.cpp -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "TestVisitor.h"
+
+using namespace clang;
+
+namespace {
+
+// Matches (optional) explicit template parameters.
+class LambdaTemplateParametersVisitor
+  : public ExpectedLocationVisitor {
+public:
+  bool shouldVisitImplicitCode() const { return false; }
+
+  bool VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
+EXPECT_FALSE(D->isImplicit());
+Match(D->getName(), D->getLocStart());
+return true;
+  }
+
+  bool VisitNonTypeTemplateParmDecl(NonTypeTemplateParmDecl *D) {
+EXPECT_FALSE(D->isImplicit());
+Match(D->getName(), D->getLocStart());
+return true;
+  }
+
+  bool VisitTemplateTemplateParmDecl(TemplateTemplateParmDecl *D) {
+EXPECT_FALSE(D->isImplicit());
+Match(D->getName(), D->getLocStart());
+return true;
+  }
+};
+
+TEST(RecursiveASTVisitor, VisitsLambdaExplicitTemplateParameters) {
+  LambdaTemplateParametersVisitor Visitor;
+  Visitor.ExpectMatch("T",  2, 15);
+  Visitor.ExpectMatch("I",  2, 24);
+  Visitor.ExpectMatch("TT", 2, 31);
+  EXPECT_TRUE(Visitor.runOver(
+  "void f() { \n"
+  "  auto l = [] class TT>(auto p) { }; \n"
+  "}",
+  LambdaTemplateParametersVisitor::Lang_CXX2a));
+}
+
+} // end anonymous namespace
Index: unittests/AST/StmtPrinterTest.cpp
===
--- unittests/AST/StmtPrinterTest.cpp
+++ unittests/AST/StmtPrinterTest.cpp
@@ -232,6 +232,43 @@
 // WRONG; Should be: (a & b).operator void *()
 }
 
+TEST(StmtPrinter, TestCXXLamda) {
+  ASSERT_TRUE(PrintedStmtCXXMatches(StdVer::CXX11,
+"void A() {"
+"  auto l = [] { };"
+"}",
+lambdaExpr(anything()).bind("id"),
+"[] {\n"
+"}"));
+
+  ASSERT_TRUE(PrintedStmtCXXMatches(StdVer::CXX11,
+"void A() {"
+"  int a = 0, b = 1;"
+"  auto l = [a,b](int c, float d) { };"
+"}",
+lambdaExpr(anything()).bind("id"),
+"[a, b](int c, float d) {\n"
+"}"));
+
+  ASSERT_TRUE(PrintedStmtCXXMatches(StdVer::CXX14,
+"void A() {"
+"  auto l = [](auto a, int b, auto c, int, auto) { };"
+"}",
+lambdaExpr(anything()).bind("id"),
+"[](auto a, int 

[PATCH] D51868: [libcxx] Build and test fixes for Windows

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added inline comments.



Comment at: CMakeLists.txt:550
+endif()
 add_compile_flags_if_supported(
+-Wextra -W -Wwrite-strings

EricWF wrote:
> Couldn't we keep the "-Wall" here? And just add something else that adds 
> "/W4"?
> 
> Also, we don't yet support building with MSVC's frontend. Only clang-cl. Does 
> clang-cl not like this existing code?
The `-Wall` flag gets converted to `-Weverything` by Clang-CL; I don’t think we 
should keep it (it causes thousands of warnings per header).

`/W4` is the equivalent flag on Windows.



Comment at: include/filesystem:1396
 
-  _LIBCPP_FUNC_VIS
   void __create_what(int __num_paths);

EricWF wrote:
> hamzasood wrote:
> > compnerd wrote:
> > > This possibly changes the meaning on other targets.  What was the error 
> > > that this triggered?
> > I've re-uploaded the patch with full context to make this clearer.
> > 
> > That's a member function on an exported type, which I don't think should 
> > have any visibility attributes. The specific issue in this case is that 
> > both the class type and the member function are marked as dllimport, which 
> > causes a compilation error.
> Perhaps part of the problem here is that filesystem builds to a static 
> library, not a shared one.
> 
> None the less, this macro is important once we move the definitions from 
> libc++fs.a to libc++.so.
I don’t think member functions are meant to have function visibility, and I 
wasn’t able to find any other such cases in libc++. That’ll always be a hard 
error on Windows because a dllimport class can’t have dllimport member 
functions.



Comment at: test/support/test_macros.h:147
-#  elif defined(_WIN32)
-#if defined(_MSC_VER) && !defined(__MINGW32__)
-#  define TEST_HAS_C11_FEATURES // Using Microsoft's C Runtime library

EricWF wrote:
> hamzasood wrote:
> > hamzasood wrote:
> > > STL_MSFT wrote:
> > > > compnerd wrote:
> > > > > I think that the condition here is inverted, and should be enabled 
> > > > > for MinGW32.
> > > > What error prompted this? It hasn't caused problems when running 
> > > > libc++'s tests against MSVC's STL (with both C1XX and Clang).
> > > The comment above this says that it's copied from `__config`, but they 
> > > must've gone out of sync at some point because `__config` doesn't have 
> > > that extra section that I deleted.
> > > 
> > > This causes lots of errors when testing libc++. E.g. libc++ isn't 
> > > declaring `std::timespec` on Windows because `_LIBCPP_HAS_C11_FEATURES` 
> > > isn't defined, but the tests think that it's available so they try to use 
> > > it (which causes compilation failures in lots of tests).
> > > 
> > > The better solution here might be to update `__config` to match this, but 
> > > I'm not familiar with some of those platforms so this seemed like the 
> > > safest approach for now.
> > This is a workaround for a libc++ issue rather than an MSVC one. See the 
> > response to @compnerd for the full details.
> 
> What tests are failing?
Any test that requires `TEST_HAS_C11_FEATURES` or `TEST_HAS_TIMESPEC_GET`. 
Those test macros aren’t in sync with the corresponding `__config` ones.



Comment at: utils/libcxx/test/config.py:518
 self.cxx.compile_flags += ['-DNOMINMAX']
+# Disable auto-linking; the library is linked manually when
+# configuring the linker flags.

EricWF wrote:
> I think the correct fix here is to disabling linking libc++ when auto linking 
> is enabled by the headers. Because we want to test that the auto link pragmas 
> work.
In that case, it might be worth adding separate auto-linking tests that 
specifically use Clang-CL. Making it work here would involve defining `_DLL` 
for dynamic builds, but I’m not sure if that would interfere with the CRT.


https://reviews.llvm.org/D51868



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

[PATCH] D51868: [libcxx] Build and test fixes for Windows

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added inline comments.



Comment at: test/support/verbose_assert.h:26
   static_assert(ST == -1, "specialization required for ST != -1");
   static void Print(Tp const&) { std::clog << "Value Not Streamable!\n"; }
 };

mclow.lists wrote:
> > The renaming is to clarify that an `stderr` stream is being selected.
> And yet, there is `clog`, right there.
> 
I guess because that’s logging a programmer error (trying to print a type that 
isn’t streamable) whereas the others are test-related diagnostics? Not 
completely sure though.


https://reviews.llvm.org/D51868



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

[PATCH] D51868: [libcxx] Build and test fixes for Windows

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added inline comments.



Comment at: include/filesystem:1396
 
-  _LIBCPP_FUNC_VIS
   void __create_what(int __num_paths);

compnerd wrote:
> This possibly changes the meaning on other targets.  What was the error that 
> this triggered?
I've re-uploaded the patch with full context to make this clearer.

That's a member function on an exported type, which I don't think should have 
any visibility attributes. The specific issue in this case is that both the 
class type and the member function are marked as dllimport, which causes a 
compilation error.



Comment at: test/support/test_macros.h:147
-#  elif defined(_WIN32)
-#if defined(_MSC_VER) && !defined(__MINGW32__)
-#  define TEST_HAS_C11_FEATURES // Using Microsoft's C Runtime library

STL_MSFT wrote:
> compnerd wrote:
> > I think that the condition here is inverted, and should be enabled for 
> > MinGW32.
> What error prompted this? It hasn't caused problems when running libc++'s 
> tests against MSVC's STL (with both C1XX and Clang).
The comment above this says that it's copied from `__config`, but they must've 
gone out of sync at some point because `__config` doesn't have that extra 
section that I deleted.

This causes lots of errors when testing libc++. E.g. libc++ isn't declaring 
`std::timespec` on Windows because `_LIBCPP_HAS_C11_FEATURES` isn't defined, 
but the tests think that it's available so they try to use it (which causes 
compilation failures in lots of tests).

The better solution here might be to update `__config` to match this, but I'm 
not familiar with some of those platforms so this seemed like the safest 
approach for now.



Comment at: test/support/test_macros.h:147
-#  elif defined(_WIN32)
-#if defined(_MSC_VER) && !defined(__MINGW32__)
-#  define TEST_HAS_C11_FEATURES // Using Microsoft's C Runtime library

hamzasood wrote:
> STL_MSFT wrote:
> > compnerd wrote:
> > > I think that the condition here is inverted, and should be enabled for 
> > > MinGW32.
> > What error prompted this? It hasn't caused problems when running libc++'s 
> > tests against MSVC's STL (with both C1XX and Clang).
> The comment above this says that it's copied from `__config`, but they 
> must've gone out of sync at some point because `__config` doesn't have that 
> extra section that I deleted.
> 
> This causes lots of errors when testing libc++. E.g. libc++ isn't declaring 
> `std::timespec` on Windows because `_LIBCPP_HAS_C11_FEATURES` isn't defined, 
> but the tests think that it's available so they try to use it (which causes 
> compilation failures in lots of tests).
> 
> The better solution here might be to update `__config` to match this, but I'm 
> not familiar with some of those platforms so this seemed like the safest 
> approach for now.
This is a workaround for a libc++ issue rather than an MSVC one. See the 
response to @compnerd for the full details.



Comment at: test/support/verbose_assert.h:24
 : (IsStreamable::value ? 2 : -1))>
-struct SelectStream {
+struct SelectErrStream {
   static_assert(ST == -1, "specialization required for ST != -1");

mclow.lists wrote:
> Why the renaming here? 
> 
> What's the deal with sometimes using `clog` and sometimes `cerr`?
> (I know, you didn't do this, but ???)
> 
Some of the casting that goes on in that template triggers an error under 
`-fno-ms-extensions` when given a function pointer (in this case: `std::endl`). 
Selecting between a standard/wide stream isn't needed for `std::endl` as it can 
go to either, so I've bypassed the selection by sending it straight to `cerr`.

The renaming is to clarify that an `stderr` stream (and nothing else) is being 
selected, which sort of justifies going directly to `cerr`.


https://reviews.llvm.org/D51868



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

[PATCH] D51868: [libcxx] Build and test fixes for Windows

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 166124.
hamzasood added a comment.
Herald added a subscriber: christof.

I've added a fix for another related issue: the tests would fail to link if 
libc++ is built only as a DLL.

The auto-linking doesn't look for a dynamic library unless `_DLL` is defined 
(which I think is done by passing `/MD` to CL), but the test runner doesn't use 
CL-style commands. This could either be solved by either manually defining that 
macro or just disabling auto-linking. The test runner already configures the 
linker correctly so disabling auto-linking seemed like the simplest option.


https://reviews.llvm.org/D51868

Files:
  CMakeLists.txt
  include/filesystem
  test/std/strings/c.strings/cuchar.pass.cpp
  test/support/test_macros.h
  test/support/verbose_assert.h
  utils/libcxx/test/config.py

Index: utils/libcxx/test/config.py
===
--- utils/libcxx/test/config.py
+++ utils/libcxx/test/config.py
@@ -515,6 +515,10 @@
 # and so that those tests don't have to be changed to tolerate
 # this insanity.
 self.cxx.compile_flags += ['-DNOMINMAX']
+# Disable auto-linking; the library is linked manually when
+# configuring the linker flags.
+if self.cxx_stdlib_under_test == 'libc++':
+self.cxx.compile_flags += ['-D_LIBCPP_NO_AUTO_LINK']
 additional_flags = self.get_lit_conf('test_compiler_flags')
 if additional_flags:
 self.cxx.compile_flags += shlex.split(additional_flags)
Index: test/support/verbose_assert.h
===
--- test/support/verbose_assert.h
+++ test/support/verbose_assert.h
@@ -21,18 +21,18 @@
 
 template ::value ? 1
 : (IsStreamable::value ? 2 : -1))>
-struct SelectStream {
+struct SelectErrStream {
   static_assert(ST == -1, "specialization required for ST != -1");
   static void Print(Tp const&) { std::clog << "Value Not Streamable!\n"; }
 };
 
 template 
-struct SelectStream {
+struct SelectErrStream {
   static void Print(Tp const& val) { std::cerr << val; }
 };
 
 template 
-struct SelectStream {
+struct SelectErrStream {
   static void Print(Tp const& val) { std::wcerr << val; }
 };
 
@@ -86,14 +86,14 @@
 template 
 friend LogType& operator<<(LogType& log, Tp const& value) {
   if (!log.is_disabled) {
-SelectStream::Print(value);
+SelectErrStream::Print(value);
   }
   return log;
 }
 
 friend LogType& operator<<(LogType& log, EndLType* m) {
   if (!log.is_disabled) {
-SelectStream::Print(m);
+std::cerr << m;
   }
   return log;
 }
Index: test/support/test_macros.h
===
--- test/support/test_macros.h
+++ test/support/test_macros.h
@@ -143,11 +143,6 @@
 #  define TEST_HAS_C11_FEATURES
 #  define TEST_HAS_TIMESPEC_GET
 #endif
-#  elif defined(_WIN32)
-#if defined(_MSC_VER) && !defined(__MINGW32__)
-#  define TEST_HAS_C11_FEATURES // Using Microsoft's C Runtime library
-#  define TEST_HAS_TIMESPEC_GET
-#endif
 #  endif
 #endif
 
Index: test/std/strings/c.strings/cuchar.pass.cpp
===
--- test/std/strings/c.strings/cuchar.pass.cpp
+++ test/std/strings/c.strings/cuchar.pass.cpp
@@ -8,6 +8,9 @@
 //===--===//
 //
 // XFAIL: *
+//
+// The MSVC stdlib has cuchar, which causes this test to pass unexpectedly.
+// UNSUPPORTED: windows
 
 // 
 
Index: include/filesystem
===
--- include/filesystem
+++ include/filesystem
@@ -1390,7 +1390,6 @@
 return __storage_->__what_.c_str();
   }
 
-  _LIBCPP_FUNC_VIS
   void __create_what(int __num_paths);
 
 private:
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -67,7 +67,7 @@
 option(LIBCXX_ENABLE_ASSERTIONS "Enable assertions independent of build mode." OFF)
 option(LIBCXX_ENABLE_SHARED "Build libc++ as a shared library." ON)
 option(LIBCXX_ENABLE_STATIC "Build libc++ as a static library." ON)
-option(LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY "Build libc++experimental.a" ON)
+option(LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY "Build libc++experimental.a" $)
 set(ENABLE_FILESYSTEM_DEFAULT ${LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY})
 if (WIN32)
   set(ENABLE_FILESYSTEM_DEFAULT OFF)
@@ -542,8 +542,13 @@
 
 # Warning flags ===
 add_definitions(-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+if (MSVC)
+add_compile_flags(/W4)
+else()
+add_compile_flags_if_supported(-Wall)
+endif()
 add_compile_flags_if_supported(
--Wall -Wextra -W -Wwrite-strings
+-Wextra -W -Wwrite-strings
 -Wno-unused-parameter -Wno-long-long
 

[PATCH] D51868: [libcxx] Build and test fixes for Windows

[Hamza Sood via Phabricator via cfe-commits]

hamzasood created this revision.
hamzasood added reviewers: rnk, EricWF, compnerd, smeenai.
Herald added subscribers: cfe-commits, ldionne, mgorny.

The patch fixes a few issues that arise when using libc++ on Windows.
Specifically:

1. The CMake configuration added `-Wall` to the compilation, which actually 
means `-Weverything` in MSVC. This led to several thousand warnings per file.
2. The experimental library was enabled by default. It doesn't really work 
(yet) on Windows, and in fact the build docs suggest to disable it, so I don't 
think that was a sensible default.
3. There were some other errors that caused compilation errors in some of the 
tests.

I've identified a few other issues, but I'm not sure how best to fix them:

1. `support/win32/locale_win32.h` includes `xlocinfo.h`, which ends up 
including `yvals_core.h`. That MSVC header defines feature test macros, among 
other things, that clash with macros defined in libc++. This is causing lots of 
test errors.
2. `` includes the ucrt header `new.h` in the hope that it'll declare 
`get_new_handler` etc. but `new.h` really just goes back and includes ``. 
The end result is that nothing actually gets declared and so we're missing a 
few declarations, which causes lots of compilation errors.


Repository:
  rCXX libc++

https://reviews.llvm.org/D51868

Files:
  CMakeLists.txt
  include/filesystem
  test/std/strings/c.strings/cuchar.pass.cpp
  test/support/test_macros.h
  test/support/verbose_assert.h

Index: test/support/verbose_assert.h
===
--- test/support/verbose_assert.h
+++ test/support/verbose_assert.h
@@ -21,18 +21,18 @@
 
 template ::value ? 1
 : (IsStreamable::value ? 2 : -1))>
-struct SelectStream {
+struct SelectErrStream {
   static_assert(ST == -1, "specialization required for ST != -1");
   static void Print(Tp const&) { std::clog << "Value Not Streamable!\n"; }
 };
 
 template 
-struct SelectStream {
+struct SelectErrStream {
   static void Print(Tp const& val) { std::cerr << val; }
 };
 
 template 
-struct SelectStream {
+struct SelectErrStream {
   static void Print(Tp const& val) { std::wcerr << val; }
 };
 
@@ -86,14 +86,14 @@
 template 
 friend LogType& operator<<(LogType& log, Tp const& value) {
   if (!log.is_disabled) {
-SelectStream::Print(value);
+SelectErrStream::Print(value);
   }
   return log;
 }
 
 friend LogType& operator<<(LogType& log, EndLType* m) {
   if (!log.is_disabled) {
-SelectStream::Print(m);
+std::cerr << m;
   }
   return log;
 }
Index: test/support/test_macros.h
===
--- test/support/test_macros.h
+++ test/support/test_macros.h
@@ -143,11 +143,6 @@
 #  define TEST_HAS_C11_FEATURES
 #  define TEST_HAS_TIMESPEC_GET
 #endif
-#  elif defined(_WIN32)
-#if defined(_MSC_VER) && !defined(__MINGW32__)
-#  define TEST_HAS_C11_FEATURES // Using Microsoft's C Runtime library
-#  define TEST_HAS_TIMESPEC_GET
-#endif
 #  endif
 #endif
 
Index: test/std/strings/c.strings/cuchar.pass.cpp
===
--- test/std/strings/c.strings/cuchar.pass.cpp
+++ test/std/strings/c.strings/cuchar.pass.cpp
@@ -8,6 +8,9 @@
 //===--===//
 //
 // XFAIL: *
+//
+// The MSVC stdlib has cuchar, which causes this test to pass unexpectedly.
+// UNSUPPORTED: windows
 
 // 
 
Index: include/filesystem
===
--- include/filesystem
+++ include/filesystem
@@ -1393,7 +1393,6 @@
 return __storage_->__what_.c_str();
   }
 
-  _LIBCPP_FUNC_VIS
   void __create_what(int __num_paths);
 
 private:
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -67,7 +67,7 @@
 option(LIBCXX_ENABLE_ASSERTIONS "Enable assertions independent of build mode." OFF)
 option(LIBCXX_ENABLE_SHARED "Build libc++ as a shared library." ON)
 option(LIBCXX_ENABLE_STATIC "Build libc++ as a static library." ON)
-option(LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY "Build libc++experimental.a" ON)
+option(LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY "Build libc++experimental.a" $)
 set(ENABLE_FILESYSTEM_DEFAULT ${LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY})
 if (WIN32)
   set(ENABLE_FILESYSTEM_DEFAULT OFF)
@@ -542,8 +542,13 @@
 
 # Warning flags ===
 add_definitions(-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+if (MSVC)
+add_compile_flags(/W4)
+else()
+add_compile_flags_if_supported(-Wall)
+endif()
 add_compile_flags_if_supported(
--Wall -Wextra -W -Wwrite-strings
+-Wextra -W -Wwrite-strings
 -Wno-unused-parameter -Wno-long-long
 -Werror=return-type)
 if ("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")

[PATCH] D51321: [Tooling] Improve handling of CL-style options

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added a comment.

Thanks for the help with this.
I reverted most of my changes (including the parameterised tests) before 
committing; I think I implemented all of your suggestions.


Repository:
  rL LLVM

https://reviews.llvm.org/D51321



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

[PATCH] D51321: [Tooling] Improve handling of CL-style options

[Hamza Sood via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL341760: [Tooling] Improve handling of CL-style options 
(authored by hamzasood, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D51321?vs=163098=164585#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D51321

Files:
  cfe/trunk/lib/Tooling/InterpolatingCompilationDatabase.cpp
  cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp

Index: cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp
===
--- cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp
+++ cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -648,14 +648,17 @@
 protected:
   // Adds an entry to the underlying compilation database.
   // A flag is injected: -D , so the command used can be identified.
-  void add(llvm::StringRef File, llvm::StringRef Flags = "") {
-llvm::SmallVector Argv = {"clang", File, "-D", File};
+  void add(StringRef File, StringRef Clang, StringRef Flags) {
+SmallVector Argv = {Clang, File, "-D", File};
 llvm::SplitString(Flags, Argv);
-llvm::SmallString<32> Dir;
+
+SmallString<32> Dir;
 llvm::sys::path::system_temp_directory(false, Dir);
+
 Entries[path(File)].push_back(
 {Dir, path(File), {Argv.begin(), Argv.end()}, "foo.o"});
   }
+  void add(StringRef File, StringRef Flags = "") { add(File, "clang", Flags); }
 
   // Turn a unix path fragment (foo/bar.h) into a native path (C:\tmp\foo\bar.h)
   std::string path(llvm::SmallString<32> File) {
@@ -739,6 +742,30 @@
   EXPECT_EQ(getCommand("foo/bar/baz/shout.C"), "clang -D FOO/BAR/BAZ/SHOUT.cc");
 }
 
+TEST_F(InterpolateTest, Aliasing) {
+  add("foo.cpp", "-faligned-new");
+
+  // The interpolated command should keep the given flag as written, even though
+  // the flag is internally represented as an alias.
+  EXPECT_EQ(getCommand("foo.hpp"), "clang -D foo.cpp -faligned-new");
+}
+
+TEST_F(InterpolateTest, ClangCL) {
+  add("foo.cpp", "clang-cl", "/W4");
+
+  // Language flags should be added with CL syntax.
+  EXPECT_EQ(getCommand("foo.h"), "clang-cl -D foo.cpp /W4 /TP");
+}
+
+TEST_F(InterpolateTest, DriverModes) {
+  add("foo.cpp", "clang-cl", "--driver-mode=gcc");
+  add("bar.cpp", "clang", "--driver-mode=cl");
+
+  // --driver-mode overrides should be respected.
+  EXPECT_EQ(getCommand("foo.h"), "clang-cl -D foo.cpp --driver-mode=gcc -x c++-header");
+  EXPECT_EQ(getCommand("bar.h"), "clang -D bar.cpp --driver-mode=cl /TP");
+}
+
 TEST(CompileCommandTest, EqualityOperator) {
   CompileCommand CCRef("/foo/bar", "hello.c", {"a", "b"}, "hello.o");
   CompileCommand CCTest = CCRef;
Index: cfe/trunk/lib/Tooling/InterpolatingCompilationDatabase.cpp
===
--- cfe/trunk/lib/Tooling/InterpolatingCompilationDatabase.cpp
+++ cfe/trunk/lib/Tooling/InterpolatingCompilationDatabase.cpp
@@ -48,6 +48,7 @@
 #include "clang/Frontend/LangStandard.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Option/ArgList.h"
@@ -127,47 +128,68 @@
   Optional Type;
   // Standard specified by -std.
   LangStandard::Kind Std = LangStandard::lang_unspecified;
+  // Whether the command line is for the cl-compatible driver.
+  bool ClangCLMode;
 
   TransferableCommand(CompileCommand C)
-  : Cmd(std::move(C)), Type(guessType(Cmd.Filename)) {
-std::vector NewArgs = {Cmd.CommandLine.front()};
+  : Cmd(std::move(C)), Type(guessType(Cmd.Filename)),
+ClangCLMode(checkIsCLMode(Cmd.CommandLine)) {
+std::vector OldArgs = std::move(Cmd.CommandLine);
+Cmd.CommandLine.clear();
+
+// Wrap the old arguments in an InputArgList.
+llvm::opt::InputArgList ArgList;
+{
+  SmallVector TmpArgv;
+  for (const std::string  : OldArgs)
+TmpArgv.push_back(S.c_str());
+  ArgList = {TmpArgv.begin(), TmpArgv.end()};
+}
+
 // Parse the old args in order to strip out and record unwanted flags.
+// We parse each argument individually so that we can retain the exact
+// spelling of each argument; re-rendering is lossy for aliased flags.
+// E.g. in CL mode, /W4 maps to -Wall.
 auto OptTable = clang::driver::createDriverOptTable();
-std::vector Argv;
-for (unsigned I = 1; I < Cmd.CommandLine.size(); ++I)
-  Argv.push_back(Cmd.CommandLine[I].c_str());
-unsigned MissingI, MissingC;
-auto ArgList = OptTable->ParseArgs(Argv, MissingI, MissingC);
-for (const auto *Arg : ArgList) {
-  const auto  = Arg->getOption();
+Cmd.CommandLine.emplace_back(OldArgs.front());
+for (unsigned Pos = 1; Pos < OldArgs.size();) {
+  using namespace driver::options;
+
+  const unsigned OldPos = Pos;
+  std::unique_ptr Arg(OptTable->ParseOneArg(
+ 

[PATCH] D51321: [Tooling] Improve handling of CL-style options

[Hamza Sood via Phabricator via cfe-commits]

hamzasood marked 10 inline comments as done.
hamzasood added inline comments.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:124
+// Determine whether the given file path is the clang-cl executable.
+static bool checkIsCLMode(const std::vector ,
+  const llvm::opt::OptTable ) {

hamzasood wrote:
> sammccall wrote:
> > nit: a two-value enum would be clearer and allow for terser variable names 
> > (`Mode`)
> The advantage of a Boolean is that it makes the checks simpler. E.g. `if 
> (isCL)` instead of `if (mode == DriverMode::CL)` or something.
> 
> Happy to change it though if you still disagree.
Also, there're more than just 2 driver modes. But we only care about cl and not 
cl.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:156
+
+// Transform the command line to an llvm ArgList.
+// Make sure that OldArgs lives for at least as long as this variable as 
the

hamzasood wrote:
> sammccall wrote:
> > would you mind reverting this change and just wrapping the old Argv in an 
> > InputArgList?
> > I'm not sure the lifetime comments and std::transform aid readability here.
> The change was more about safety than readability. The old approach left an 
> InputArgList of dangling pointers after moving the new args into the cmd 
> object. This way there’s no way to accidentally access deallocated memory by 
> using the InputArgList.
> 
> As above, happy to change if you still disagree.
I've restructured it so hopefully this is less of a concern.



Comment at: unittests/Tooling/CompilationDatabaseTest.cpp:652
+
+class InterpolateTest
+  : public ::testing::TestWithParam> {

hamzasood wrote:
> sammccall wrote:
> > I'm not sure the parameterized test is the right model here.
> > The `INSTANTIATE_TEST_P` and friends makes the test fixture pretty 
> > complicated, and failure messages hard to relate to input conditions.
> > Then the actual tests have a bunch of assertions conditional on which mode 
> > we're in.
> > Finally, we don't actually test any mixed-mode database.
> > 
> > Could we write this a little more directly?:
> >  - pass the driver mode flag to `add()` in the normal way, in the Flags 
> > param
> >  - require callers to "add" to pass "clang" or "clang-cl". (It's OK that 
> > this makes existing cases a bit more verbose)
> >  - explicitly test the clang-cl and --driver-mode cases we care about, 
> > rather than running every assertion in every configuration
> > 
> > e.g.
> > ```
> > TEST_F(InterpolateTest, ClangCL) {
> >   add("foo.cc", "clang");
> >   add("bar.cc", "clang-cl");
> >   add("baz.cc", "clang --driver-mode=clang-cl");
> > 
> >   EXPECT_EQ(getCommand("a/bar.h"), "clang-cl -D a/baz.cc /TP");
> > }
> > ```
> The intent was to make sure that the existing cases (that shouldn’t depend on 
> the driver mode) don’t break, which admittedly happened while I was working 
> on the patch.
> 
> Most of the tests aren’t conditional on the mode, and I think it’s important 
> that they’re tested in all configurations. The parameterisation also lets us 
> test as `clang-cl`, `clang-cl —driver-mode=cl`, and `clang —driver-mode=cl`. 
> Which are all valid ways of using CL mode.
To clarify: the parameterisation runs the test 6 times. The `isTestingCL()` 
check narrows that down to 3.


https://reviews.llvm.org/D51321



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

[PATCH] D51321: [Tooling] Improve handling of CL-style options

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 163098.
hamzasood added a comment.

I've addressed most of your comments and resolved the merge conflicts 
introduced in r340838.


https://reviews.llvm.org/D51321

Files:
  lib/Tooling/InterpolatingCompilationDatabase.cpp
  unittests/Tooling/CompilationDatabaseTest.cpp

Index: unittests/Tooling/CompilationDatabaseTest.cpp
===
--- unittests/Tooling/CompilationDatabaseTest.cpp
+++ unittests/Tooling/CompilationDatabaseTest.cpp
@@ -14,6 +14,8 @@
 #include "clang/Tooling/JSONCompilationDatabase.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/Path.h"
+
+#define GTEST_HAS_COMBINE 1
 #include "gtest/gtest.h"
 
 namespace clang {
@@ -644,15 +646,53 @@
   }
 };
 
-class InterpolateTest : public ::testing::Test {
+enum class DriverExe { Clang, ClangCL };
+enum class DriverMode { Implicit, GCC, CL };
+
+class InterpolateTest
+: public ::testing::TestWithParam> {
+
+  StringRef getClangExe() const {
+switch (std::get<0>(GetParam())) {
+case DriverExe::Clang:   return "clang";
+case DriverExe::ClangCL: return "clang-cl";
+}
+llvm_unreachable("Invalid DriverExe");
+  }
+
+  StringRef getDriverModeFlag() const {
+switch (std::get<1>(GetParam())) {
+case DriverMode::Implicit: return StringRef();
+case DriverMode::GCC: return "--driver-mode=gcc";
+case DriverMode::CL:  return "--driver-mode=cl";
+}
+llvm_unreachable("Invalid DriverMode");
+  }
+
 protected:
+  bool isTestingCL() const {
+switch (std::get<1>(GetParam())) {
+case DriverMode::Implicit:
+  return std::get<0>(GetParam()) == DriverExe::ClangCL;
+case DriverMode::GCC: return false;
+case DriverMode::CL:  return true;
+}
+llvm_unreachable("Invalid DriverMode");
+  }
+
   // Adds an entry to the underlying compilation database.
   // A flag is injected: -D , so the command used can be identified.
   void add(llvm::StringRef File, llvm::StringRef Flags = "") {
-llvm::SmallVector Argv = {"clang", File, "-D", File};
+llvm::SmallVector Argv;
+Argv.push_back(getClangExe());
+if (!getDriverModeFlag().empty())
+  Argv.push_back(getDriverModeFlag());
+Argv.append({"-D", File});
 llvm::SplitString(Flags, Argv);
+
 llvm::SmallString<32> Dir;
 llvm::sys::path::system_temp_directory(false, Dir);
+
 Entries[path(File)].push_back(
 {Dir, path(File), {Argv.begin(), Argv.end()}, "foo.o"});
   }
@@ -675,69 +715,124 @@
 ->getCompileCommands(path(F));
 if (Results.empty())
   return "none";
-// drop the input file argument, so tests don't have to deal with path().
-EXPECT_EQ(Results[0].CommandLine.back(), path(F))
-<< "Last arg should be the file";
-Results[0].CommandLine.pop_back();
-return llvm::join(Results[0].CommandLine, " ");
+
+// To simply the test checks, we're building a new command line that doesn't
+// contain the uninteresting stuff (exe name etc.).
+ArrayRef CmdLine = Results[0].CommandLine;
+
+// Drop the clang executable path.
+EXPECT_EQ(CmdLine.front(), getClangExe()) << "First arg should be clang";
+CmdLine = CmdLine.drop_front();
+
+// Drop the --driver-mode flag.
+if (!getDriverModeFlag().empty()) {
+  EXPECT_EQ(CmdLine.front(), getDriverModeFlag())
+<< "Incorrect driver mode flag";
+  CmdLine = CmdLine.drop_front();
+}
+
+// Drop the input file.
+EXPECT_EQ(CmdLine.back(), path(F)) << "Last arg should be the file";
+CmdLine = CmdLine.drop_back();
+
+return llvm::join(CmdLine, " ");
   }
 
   MemCDB::EntryMap Entries;
 };
 
-TEST_F(InterpolateTest, Nearby) {
+TEST_P(InterpolateTest, Nearby) {
   add("dir/foo.cpp");
   add("dir/bar.cpp");
   add("an/other/foo.cpp");
 
   // great: dir and name both match (prefix or full, case insensitive)
-  EXPECT_EQ(getCommand("dir/f.cpp"), "clang -D dir/foo.cpp");
-  EXPECT_EQ(getCommand("dir/FOO.cpp"), "clang -D dir/foo.cpp");
+  EXPECT_EQ(getCommand("dir/f.cpp"), "-D dir/foo.cpp");
+  EXPECT_EQ(getCommand("dir/FOO.cpp"), "-D dir/foo.cpp");
   // no name match. prefer matching dir, break ties by alpha
-  EXPECT_EQ(getCommand("dir/a.cpp"), "clang -D dir/bar.cpp");
+  EXPECT_EQ(getCommand("dir/a.cpp"), "-D dir/bar.cpp");
   // an exact name match beats one segment of directory match
   EXPECT_EQ(getCommand("some/other/bar.h"),
-"clang -D dir/bar.cpp -x c++-header");
+isTestingCL()
+? "-D dir/bar.cpp /TP"
+: "-D dir/bar.cpp -x c++-header");
   // two segments of directory match beat a prefix name match
-  EXPECT_EQ(getCommand("an/other/b.cpp"), "clang -D an/other/foo.cpp");
+  EXPECT_EQ(getCommand("an/other/b.cpp"), "-D an/other/foo.cpp");
   // if nothing matches at all, we still get the closest alpha match
   EXPECT_EQ(getCommand("below/some/obscure/path.cpp"),
-"clang -D an/other/foo.cpp");
+"-D 

[PATCH] D51321: [Tooling] Improve handling of CL-style options

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added a comment.

Thanks for the detailed feedback; I’ll try to get a new patch up in a few hours.

However I disagree with some of them (see replies).




Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:124
+// Determine whether the given file path is the clang-cl executable.
+static bool checkIsCLMode(const std::vector ,
+  const llvm::opt::OptTable ) {

sammccall wrote:
> nit: a two-value enum would be clearer and allow for terser variable names 
> (`Mode`)
The advantage of a Boolean is that it makes the checks simpler. E.g. `if 
(isCL)` instead of `if (mode == DriverMode::CL)` or something.

Happy to change it though if you still disagree.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:156
+
+// Transform the command line to an llvm ArgList.
+// Make sure that OldArgs lives for at least as long as this variable as 
the

sammccall wrote:
> would you mind reverting this change and just wrapping the old Argv in an 
> InputArgList?
> I'm not sure the lifetime comments and std::transform aid readability here.
The change was more about safety than readability. The old approach left an 
InputArgList of dangling pointers after moving the new args into the cmd 
object. This way there’s no way to accidentally access deallocated memory by 
using the InputArgList.

As above, happy to change if you still disagree.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:178
+  {
+unsigned Included, Excluded;
+if (IsCLMode) {

sammccall wrote:
> This seems a bit verbose.
> 
> First, do things actually break if we just use the default 0/0 masks? We're 
> not trying to interpret all the flags, just a few and pass the rest through.
> 
> Failing that, it seems clearer to just use a ternary to initialize 
> Included/Excluded, or inline them completely.
> (Please also drop the extra scope here).
Theoretically it could break without the flags. We need to recognise input 
files and strip them from the command line. If someone on a UNIX platform has 
an input file called `/W3`, that’d instead be interpreted as a warning flag and 
it’ll be left in. Likewise for any file in the root directory with the same 
spelling as a CL-only argument.

But yeah, I’ll inline the flags with a ternary.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:205
+  if (const auto GivenStd = isStdArg(*Arg)) {
+if (*GivenStd != LangStandard::lang_unspecified)
+  Std = *GivenStd;

sammccall wrote:
> prefer *either* optional<> or allowing the function to return 
> lang_unspecified, but not both. (There's no way a user can *explicitly* pass 
> a flag saying the language is unspecified, right?)
Kind of: `-std=hello_there` is parsed as an unspecified language. We still want 
to strip the flag in this case, which won’t be possible if we also use the 
unspecified value to denote a lack of an `-std` flag.



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:231
: Type;
-  Result.CommandLine.push_back("-x");
-  Result.CommandLine.push_back(types::getTypeName(TargetType));
+  if (IsCLMode) {
+const StringRef Flag = toCLFlag(TargetType);

sammccall wrote:
> can we unify as `addTypeFlag(TargetType, Mode, Result.CommandLine)`?
To clarify, you mean extract this into a helper function?



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:258
+
+if (IsCLMode)
+  return Opt.matches(driver::options::OPT__SLASH_Fa) ||

sammccall wrote:
> Why do we need to take IsClMode into account while reading flags?
> How would `OPT__SLASH_Fa` get parsed if we weren't in CL mode? Would we care?
> 
> (and below)
It was an optimisation attempt in that we can do 5 less comparisons in the case 
where we know that there aren’t going to be any CL flags. Maybe I was trying 
too hard to micro-optimise...



Comment at: unittests/Tooling/CompilationDatabaseTest.cpp:652
+
+class InterpolateTest
+  : public ::testing::TestWithParam> {

sammccall wrote:
> I'm not sure the parameterized test is the right model here.
> The `INSTANTIATE_TEST_P` and friends makes the test fixture pretty 
> complicated, and failure messages hard to relate to input conditions.
> Then the actual tests have a bunch of assertions conditional on which mode 
> we're in.
> Finally, we don't actually test any mixed-mode database.
> 
> Could we write this a little more directly?:
>  - pass the driver mode flag to `add()` in the normal way, in the Flags param
>  - require callers to "add" to pass "clang" or "clang-cl". (It's OK that this 
> makes existing cases a bit more verbose)
>  - explicitly test the clang-cl and --driver-mode cases we care about, rather 
> than running every assertion in every 

[PATCH] D51321: [Tooling] Improve handling of CL-style options

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 162872.
hamzasood added a comment.

- Re-uploaded with full context.

Yeah, I encountered these issues while using clangd on Windows. I haven't run 
into any clangd issues after applying this patch, but admittedly my usage is 
very basic (pretty much just code completion). It may well be subtly broken in 
other places.


https://reviews.llvm.org/D51321

Files:
  lib/Tooling/InterpolatingCompilationDatabase.cpp
  unittests/Tooling/CompilationDatabaseTest.cpp

Index: unittests/Tooling/CompilationDatabaseTest.cpp
===
--- unittests/Tooling/CompilationDatabaseTest.cpp
+++ unittests/Tooling/CompilationDatabaseTest.cpp
@@ -14,6 +14,8 @@
 #include "clang/Tooling/JSONCompilationDatabase.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/Path.h"
+
+#define GTEST_HAS_COMBINE 1
 #include "gtest/gtest.h"
 
 namespace clang {
@@ -644,15 +646,50 @@
   }
 };
 
-class InterpolateTest : public ::testing::Test {
+enum class DriverExe { Clang, ClangCL };
+enum class DriverMode { None, GCC, CL };
+
+class InterpolateTest
+  : public ::testing::TestWithParam> {
+
 protected:
+  bool isTestingCL() const {
+switch (std::get<1>(GetParam())) {
+case DriverMode::None: return std::get<0>(GetParam()) == DriverExe::ClangCL;
+case DriverMode::GCC:  return false;
+case DriverMode::CL:   return true;
+default: llvm_unreachable("Invalid DriverMode");
+}
+  }
+  StringRef getClangExe() const {
+switch (std::get<0>(GetParam())) {
+case DriverExe::Clang:   return "clang";
+case DriverExe::ClangCL: return "clang-cl";
+default: llvm_unreachable("Invalid DriverExe");
+}
+  }
+  StringRef getDriverModeArg() const {
+switch (std::get<1>(GetParam())) {
+case DriverMode::None: return StringRef();
+case DriverMode::GCC:  return "--driver-mode=gcc";
+case DriverMode::CL:   return "--driver-mode=cl";
+default: llvm_unreachable("Invalid DriverMode");
+}
+  }
+
   // Adds an entry to the underlying compilation database.
   // A flag is injected: -D , so the command used can be identified.
   void add(llvm::StringRef File, llvm::StringRef Flags = "") {
-llvm::SmallVector Argv = {"clang", File, "-D", File};
+llvm::SmallVector Argv;
+Argv.push_back(getClangExe());
+if (!getDriverModeArg().empty())
+  Argv.push_back(getDriverModeArg());
+Argv.append({"-D", File});
 llvm::SplitString(Flags, Argv);
+
 llvm::SmallString<32> Dir;
 llvm::sys::path::system_temp_directory(false, Dir);
+
 Entries[path(File)].push_back(
 {Dir, path(File), {Argv.begin(), Argv.end()}, "foo.o"});
   }
@@ -675,67 +712,101 @@
 ->getCompileCommands(path(F));
 if (Results.empty())
   return "none";
-// drop the input file argument, so tests don't have to deal with path().
-EXPECT_EQ(Results[0].CommandLine.back(), path(F))
-<< "Last arg should be the file";
-Results[0].CommandLine.pop_back();
-return llvm::join(Results[0].CommandLine, " ");
+
+ArrayRef CmdLine = Results[0].CommandLine;
+
+// Drop the clang executable path.
+EXPECT_EQ(CmdLine.front(), getClangExe()) << "First arg should be clang";
+CmdLine = CmdLine.drop_front();
+
+// Drop the --driver-mode flag.
+if (!getDriverModeArg().empty()) {
+  EXPECT_EQ(CmdLine.front(), getDriverModeArg())
+  << "Expected driver mode arg";
+  CmdLine = CmdLine.drop_front();
+}
+
+// Drop the input file.
+EXPECT_EQ(CmdLine.back(), path(F)) << "Last arg should be the file";
+CmdLine = CmdLine.drop_back();
+
+return llvm::join(CmdLine, " ");
   }
 
   MemCDB::EntryMap Entries;
 };
 
-TEST_F(InterpolateTest, Nearby) {
+TEST_P(InterpolateTest, Nearby) {
   add("dir/foo.cpp");
   add("dir/bar.cpp");
   add("an/other/foo.cpp");
 
   // great: dir and name both match (prefix or full, case insensitive)
-  EXPECT_EQ(getCommand("dir/f.cpp"), "clang -D dir/foo.cpp");
-  EXPECT_EQ(getCommand("dir/FOO.cpp"), "clang -D dir/foo.cpp");
+  EXPECT_EQ(getCommand("dir/f.cpp"), "-D dir/foo.cpp");
+  EXPECT_EQ(getCommand("dir/FOO.cpp"), "-D dir/foo.cpp");
   // no name match. prefer matching dir, break ties by alpha
-  EXPECT_EQ(getCommand("dir/a.cpp"), "clang -D dir/bar.cpp");
+  EXPECT_EQ(getCommand("dir/a.cpp"), "-D dir/bar.cpp");
   // an exact name match beats one segment of directory match
   EXPECT_EQ(getCommand("some/other/bar.h"),
-"clang -D dir/bar.cpp -x c++-header");
+isTestingCL()
+? "-D dir/bar.cpp /TP"
+: "-D dir/bar.cpp -x c++-header");
   // two segments of directory match beat a prefix name match
-  EXPECT_EQ(getCommand("an/other/b.cpp"), "clang -D an/other/foo.cpp");
+  EXPECT_EQ(getCommand("an/other/b.cpp"), "-D an/other/foo.cpp");
   // if nothing matches at all, we still get the closest alpha match
   EXPECT_EQ(getCommand("below/some/obscure/path.cpp"),
-

[PATCH] D51321: [Tooling] Improve handling of CL-style options

[Hamza Sood via Phabricator via cfe-commits]

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



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:136
+  // Otherwise just check the clang file name.
+  return llvm::sys::path::stem(CmdLine.front()).endswith_lower("clang-cl");
+}

rnk wrote:
> We support being called "CL.exe", but with our new VS integration, I don't 
> think it matters to check for this case anymore. We may remove it over time.
Should I add the check anyway?



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:140
-unsigned MissingI, MissingC;
-auto ArgList = OptTable->ParseArgs(Argv, MissingI, MissingC);
-for (const auto *Arg : ArgList) {

rnk wrote:
> Can you not keep using ParseArgs? It takes FlagsToInclude and FlagsToExclude, 
> which you can compute outside the loop now.
Good point, I forgot to move the flags out of the loop when moving the 
—driver-mode check.

But I don’t think ParseArgs is suitable (see the other comment response).



Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:156-169
+// Transform the command line to an llvm ArgList.
+// Make sure that OldArgs lives for at least as long as this variable as 
the
+// arg list contains pointers to the OldArgs strings.
+llvm::opt::InputArgList ArgList;
+{
+  // Unfortunately InputArgList requires an array of c-strings whereas we
+  // have an array of std::string; we'll need an intermediate vector.

rnk wrote:
> I think the old for loop was nicer. I don't think this code needs to change, 
> you should be able to use ParseArgs with the extra flag args.
The problem with the old loop is that we lose aliasing information (e.g. `/W3` 
becomes `-Wall`). While `ParseOneArg` also performs alias conversion, it gives 
us indices for each individual argument. The last line of the new loop copies 
arguments by using that index information to read from `OldArgs`, which gives 
us the original spelling.


https://reviews.llvm.org/D51321



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

[PATCH] D51321: [Tooling] Improve handling of CL-style options

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 162762.
hamzasood added a comment.

Thanks, I've changed it so that the driver mode is calculated first.
I've also extended the unit test to include the various `--driver-mode` 
combinations.


https://reviews.llvm.org/D51321

Files:
  lib/Tooling/InterpolatingCompilationDatabase.cpp
  unittests/Tooling/CompilationDatabaseTest.cpp

Index: unittests/Tooling/CompilationDatabaseTest.cpp
===
--- unittests/Tooling/CompilationDatabaseTest.cpp
+++ unittests/Tooling/CompilationDatabaseTest.cpp
@@ -14,6 +14,8 @@
 #include "clang/Tooling/JSONCompilationDatabase.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/Path.h"
+
+#define GTEST_HAS_COMBINE 1
 #include "gtest/gtest.h"
 
 namespace clang {
@@ -644,15 +646,50 @@
   }
 };
 
-class InterpolateTest : public ::testing::Test {
+enum class DriverExe { Clang, ClangCL };
+enum class DriverMode { None, GCC, CL };
+
+class InterpolateTest
+  : public ::testing::TestWithParam> {
+
 protected:
+  bool isTestingCL() const {
+switch (std::get<1>(GetParam())) {
+case DriverMode::None: return std::get<0>(GetParam()) == DriverExe::ClangCL;
+case DriverMode::GCC:  return false;
+case DriverMode::CL:   return true;
+default: llvm_unreachable("Invalid DriverMode");
+}
+  }
+  StringRef getClangExe() const {
+switch (std::get<0>(GetParam())) {
+case DriverExe::Clang:   return "clang";
+case DriverExe::ClangCL: return "clang-cl";
+default: llvm_unreachable("Invalid DriverExe");
+}
+  }
+  StringRef getDriverModeArg() const {
+switch (std::get<1>(GetParam())) {
+case DriverMode::None: return StringRef();
+case DriverMode::GCC:  return "--driver-mode=gcc";
+case DriverMode::CL:   return "--driver-mode=cl";
+default: llvm_unreachable("Invalid DriverMode");
+}
+  }
+
   // Adds an entry to the underlying compilation database.
   // A flag is injected: -D , so the command used can be identified.
   void add(llvm::StringRef File, llvm::StringRef Flags = "") {
-llvm::SmallVector Argv = {"clang", File, "-D", File};
+llvm::SmallVector Argv;
+Argv.push_back(getClangExe());
+if (!getDriverModeArg().empty())
+  Argv.push_back(getDriverModeArg());
+Argv.append({"-D", File});
 llvm::SplitString(Flags, Argv);
+
 llvm::SmallString<32> Dir;
 llvm::sys::path::system_temp_directory(false, Dir);
+
 Entries[path(File)].push_back(
 {Dir, path(File), {Argv.begin(), Argv.end()}, "foo.o"});
   }
@@ -675,67 +712,101 @@
 ->getCompileCommands(path(F));
 if (Results.empty())
   return "none";
-// drop the input file argument, so tests don't have to deal with path().
-EXPECT_EQ(Results[0].CommandLine.back(), path(F))
-<< "Last arg should be the file";
-Results[0].CommandLine.pop_back();
-return llvm::join(Results[0].CommandLine, " ");
+
+ArrayRef CmdLine = Results[0].CommandLine;
+
+// Drop the clang executable path.
+EXPECT_EQ(CmdLine.front(), getClangExe()) << "First arg should be clang";
+CmdLine = CmdLine.drop_front();
+
+// Drop the --driver-mode flag.
+if (!getDriverModeArg().empty()) {
+  EXPECT_EQ(CmdLine.front(), getDriverModeArg())
+  << "Expected driver mode arg";
+  CmdLine = CmdLine.drop_front();
+}
+
+// Drop the input file.
+EXPECT_EQ(CmdLine.back(), path(F)) << "Last arg should be the file";
+CmdLine = CmdLine.drop_back();
+
+return llvm::join(CmdLine, " ");
   }
 
   MemCDB::EntryMap Entries;
 };
 
-TEST_F(InterpolateTest, Nearby) {
+TEST_P(InterpolateTest, Nearby) {
   add("dir/foo.cpp");
   add("dir/bar.cpp");
   add("an/other/foo.cpp");
 
   // great: dir and name both match (prefix or full, case insensitive)
-  EXPECT_EQ(getCommand("dir/f.cpp"), "clang -D dir/foo.cpp");
-  EXPECT_EQ(getCommand("dir/FOO.cpp"), "clang -D dir/foo.cpp");
+  EXPECT_EQ(getCommand("dir/f.cpp"), "-D dir/foo.cpp");
+  EXPECT_EQ(getCommand("dir/FOO.cpp"), "-D dir/foo.cpp");
   // no name match. prefer matching dir, break ties by alpha
-  EXPECT_EQ(getCommand("dir/a.cpp"), "clang -D dir/bar.cpp");
+  EXPECT_EQ(getCommand("dir/a.cpp"), "-D dir/bar.cpp");
   // an exact name match beats one segment of directory match
   EXPECT_EQ(getCommand("some/other/bar.h"),
-"clang -D dir/bar.cpp -x c++-header");
+isTestingCL()
+? "-D dir/bar.cpp /TP"
+: "-D dir/bar.cpp -x c++-header");
   // two segments of directory match beat a prefix name match
-  EXPECT_EQ(getCommand("an/other/b.cpp"), "clang -D an/other/foo.cpp");
+  EXPECT_EQ(getCommand("an/other/b.cpp"), "-D an/other/foo.cpp");
   // if nothing matches at all, we still get the closest alpha match
   EXPECT_EQ(getCommand("below/some/obscure/path.cpp"),
-"clang -D an/other/foo.cpp");
+"-D an/other/foo.cpp");
 }
 
-TEST_F(InterpolateTest, Language) {
-  

[PATCH] D51321: [Tooling] Improve handling of CL-style options

[Hamza Sood via Phabricator via cfe-commits]

hamzasood created this revision.
hamzasood added reviewers: rnk, hokein, sammccall.
Herald added a subscriber: cfe-commits.

This patch fixes the handling of clang-cl options in 
`InterpolatingCompilationDatabase`.
They were previously ignored completely, which led to a lot of bugs:

- Additional options were being added with the wrong syntax. E.g. a file was 
specified as C++ by adding `-x c++`, which causes an error in CL mode.
- The args were parsed and then rendered, which means that the aliasing 
information was lost. E.g. `/W3` was rendered to `-Wall`, which in CL mode 
means `-Weverything`.
- CL options were ignored when checking things like `-std=`, so a lot of logic 
was being bypassed.


Repository:
  rC Clang

https://reviews.llvm.org/D51321

Files:
  lib/Tooling/InterpolatingCompilationDatabase.cpp
  unittests/Tooling/CompilationDatabaseTest.cpp

Index: unittests/Tooling/CompilationDatabaseTest.cpp
===
--- unittests/Tooling/CompilationDatabaseTest.cpp
+++ unittests/Tooling/CompilationDatabaseTest.cpp
@@ -644,12 +644,15 @@
   }
 };
 
-class InterpolateTest : public ::testing::Test {
+class InterpolateTest : public ::testing::TestWithParam {
 protected:
+  bool isTestingCL() const { return GetParam(); }
+  StringRef getClangExe() const { return isTestingCL() ? "clang-cl" : "clang"; }
+
   // Adds an entry to the underlying compilation database.
   // A flag is injected: -D , so the command used can be identified.
   void add(llvm::StringRef File, llvm::StringRef Flags = "") {
-llvm::SmallVector Argv = {"clang", File, "-D", File};
+llvm::SmallVector Argv = {getClangExe(), File, "-D", File};
 llvm::SplitString(Flags, Argv);
 llvm::SmallString<32> Dir;
 llvm::sys::path::system_temp_directory(false, Dir);
@@ -675,67 +678,92 @@
 ->getCompileCommands(path(F));
 if (Results.empty())
   return "none";
-// drop the input file argument, so tests don't have to deal with path().
-EXPECT_EQ(Results[0].CommandLine.back(), path(F))
-<< "Last arg should be the file";
-Results[0].CommandLine.pop_back();
-return llvm::join(Results[0].CommandLine, " ");
+
+ArrayRef CmdLine = Results[0].CommandLine;
+
+// drop the clang path, so tests don't have to deal with getClangExe().
+EXPECT_EQ(CmdLine.front(), getClangExe()) << "First arg should be clang";
+CmdLine = CmdLine.drop_front();
+
+// Drop the input file argument, so tests don't have to deal with path().
+EXPECT_EQ(CmdLine.back(), path(F)) << "Last arg should be the file";
+CmdLine = CmdLine.drop_back();
+
+return llvm::join(CmdLine, " ");
   }
 
   MemCDB::EntryMap Entries;
 };
 
-TEST_F(InterpolateTest, Nearby) {
+TEST_P(InterpolateTest, Nearby) {
   add("dir/foo.cpp");
   add("dir/bar.cpp");
   add("an/other/foo.cpp");
 
   // great: dir and name both match (prefix or full, case insensitive)
-  EXPECT_EQ(getCommand("dir/f.cpp"), "clang -D dir/foo.cpp");
-  EXPECT_EQ(getCommand("dir/FOO.cpp"), "clang -D dir/foo.cpp");
+  EXPECT_EQ(getCommand("dir/f.cpp"), "-D dir/foo.cpp");
+  EXPECT_EQ(getCommand("dir/FOO.cpp"), "-D dir/foo.cpp");
   // no name match. prefer matching dir, break ties by alpha
-  EXPECT_EQ(getCommand("dir/a.cpp"), "clang -D dir/bar.cpp");
+  EXPECT_EQ(getCommand("dir/a.cpp"), "-D dir/bar.cpp");
   // an exact name match beats one segment of directory match
   EXPECT_EQ(getCommand("some/other/bar.h"),
-"clang -D dir/bar.cpp -x c++-header");
+isTestingCL()
+? "-D dir/bar.cpp /TP"
+: "-D dir/bar.cpp -x c++-header");
   // two segments of directory match beat a prefix name match
-  EXPECT_EQ(getCommand("an/other/b.cpp"), "clang -D an/other/foo.cpp");
+  EXPECT_EQ(getCommand("an/other/b.cpp"), "-D an/other/foo.cpp");
   // if nothing matches at all, we still get the closest alpha match
   EXPECT_EQ(getCommand("below/some/obscure/path.cpp"),
-"clang -D an/other/foo.cpp");
+"-D an/other/foo.cpp");
 }
 
-TEST_F(InterpolateTest, Language) {
-  add("dir/foo.cpp", "-std=c++17");
-  add("dir/baz.cee", "-x c");
+TEST_P(InterpolateTest, Language) {
+  add("dir/foo.cpp", isTestingCL() ? "/std:c++17" : "-std=c++17");
+  add("dir/baz.cee", isTestingCL() ? "/TC" : "-x c");
 
   // .h is ambiguous, so we add explicit language flags
   EXPECT_EQ(getCommand("foo.h"),
-"clang -D dir/foo.cpp -x c++-header -std=c++17");
+isTestingCL()
+? "-D dir/foo.cpp /TP /std:c++17"
+: "-D dir/foo.cpp -x c++-header -std=c++17");
   // and don't add -x if the inferred language is correct.
-  EXPECT_EQ(getCommand("foo.hpp"), "clang -D dir/foo.cpp -std=c++17");
+  EXPECT_EQ(getCommand("foo.hpp"),
+isTestingCL()
+? "-D dir/foo.cpp /std:c++17"
+: "-D dir/foo.cpp -std=c++17");
   // respect -x if it's already there.
-  EXPECT_EQ(getCommand("baz.h"), "clang 

[PATCH] D50764: [AST] Make NullStmt final and give it factory functions

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added a comment.

Thanks. I'm not planning to move forward with this until the contracts patch is 
ready (it might change enough to not even require this); that should leave 
plenty of time for others to take a look.


Repository:
  rC Clang

https://reviews.llvm.org/D50764



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

[PATCH] D50662: Add dump() method for SourceRange

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added a comment.

This seems like a reasonable addition. Maybe it could also provide `print` and 
`printToString` to more closely match the `SourceLocation` API?


Repository:
  rC Clang

https://reviews.llvm.org/D50662



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

[PATCH] D50764: [AST] Make NullStmt final and give it factory functions

[Hamza Sood via Phabricator via cfe-commits]

hamzasood created this revision.
hamzasood added reviewers: klimek, steveire, bkramer.
Herald added a subscriber: cfe-commits.

I've submitted a patch for contracts (review linked to this) that adds trailing 
objects to NullStmt. This patch contains the changes to make that possible.


Repository:
  rC Clang

https://reviews.llvm.org/D50764

Files:
  include/clang/AST/Stmt.h
  lib/AST/ASTImporter.cpp
  lib/Sema/SemaStmt.cpp
  lib/Sema/TreeTransform.h
  lib/Serialization/ASTReaderStmt.cpp

Index: lib/Serialization/ASTReaderStmt.cpp
===
--- lib/Serialization/ASTReaderStmt.cpp
+++ lib/Serialization/ASTReaderStmt.cpp
@@ -3141,7 +3141,7 @@
   break;
 
 case STMT_NULL:
-  S = new (Context) NullStmt(Empty);
+  S = NullStmt::CreateEmpty(Context);
   break;
 
 case STMT_COMPOUND:
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -6650,7 +6650,7 @@
 if (Then.isInvalid())
   return StmtError();
   } else {
-Then = new (getSema().Context) NullStmt(S->getThen()->getBeginLoc());
+Then = NullStmt::Create(getSema().Context, S->getThen()->getBeginLoc());
   }
 
   // Transform the "else" branch.
@@ -7511,13 +7511,13 @@
 if (S->isIfExists())
   break;
 
-return new (getSema().Context) NullStmt(S->getKeywordLoc());
+return NullStmt::Create(getSema().Context, S->getKeywordLoc());
 
   case Sema::IER_DoesNotExist:
 if (S->isIfNotExists())
   break;
 
-return new (getSema().Context) NullStmt(S->getKeywordLoc());
+return NullStmt::Create(getSema().Context, S->getKeywordLoc());
 
   case Sema::IER_Dependent:
 Dependent = true;
Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -67,7 +67,7 @@
 
 StmtResult Sema::ActOnNullStmt(SourceLocation SemiLoc,
bool HasLeadingEmptyMacro) {
-  return new (Context) NullStmt(SemiLoc, HasLeadingEmptyMacro);
+  return NullStmt::Create(Context, SemiLoc, HasLeadingEmptyMacro);
 }
 
 StmtResult Sema::ActOnDeclStmt(DeclGroupPtrTy dg, SourceLocation StartLoc,
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -5044,8 +5044,8 @@
 
 Stmt *ASTNodeImporter::VisitNullStmt(NullStmt *S) {
   SourceLocation ToSemiLoc = Importer.Import(S->getSemiLoc());
-  return new (Importer.getToContext()) NullStmt(ToSemiLoc,
-S->hasLeadingEmptyMacro());
+  return NullStmt::Create(Importer.getToContext(), ToSemiLoc,
+  S->hasLeadingEmptyMacro());
 }
 
 Stmt *ASTNodeImporter::VisitCompoundStmt(CompoundStmt *S) {
Index: include/clang/AST/Stmt.h
===
--- include/clang/AST/Stmt.h
+++ include/clang/AST/Stmt.h
@@ -587,26 +587,36 @@
 
 /// NullStmt - This is the null statement ";": C99 6.8.3p3.
 ///
-class NullStmt : public Stmt {
+class NullStmt final : public Stmt {
   SourceLocation SemiLoc;
 
   /// True if the null statement was preceded by an empty macro, e.g:
   /// @code
   ///   #define CALL(x)
   ///   CALL(0);
   /// @endcode
-  bool HasLeadingEmptyMacro = false;
+  bool HasLeadingEmptyMacro;
+
+  NullStmt(SourceLocation L, bool hasLeadingEmptyMacro)
+  : Stmt(NullStmtClass), SemiLoc(L),
+HasLeadingEmptyMacro(hasLeadingEmptyMacro) {}
+
+  explicit NullStmt(EmptyShell Empty)
+  : Stmt(NullStmtClass, Empty), HasLeadingEmptyMacro(false) {}
 
 public:
   friend class ASTStmtReader;
   friend class ASTStmtWriter;
 
-  NullStmt(SourceLocation L, bool hasLeadingEmptyMacro = false)
-  : Stmt(NullStmtClass), SemiLoc(L),
-HasLeadingEmptyMacro(hasLeadingEmptyMacro) {}
+  static NullStmt *Create(const ASTContext , SourceLocation L,
+  bool hasLeadingEmptyMacro = false) {
+return new (C) NullStmt(L, hasLeadingEmptyMacro);
+  }
 
   /// Build an empty null statement.
-  explicit NullStmt(EmptyShell Empty) : Stmt(NullStmtClass, Empty) {}
+  static NullStmt *CreateEmpty(const ASTContext ) {
+return new (C) NullStmt(EmptyShell{});
+  }
 
   SourceLocation getSemiLoc() const { return SemiLoc; }
   void setSemiLoc(SourceLocation L) { SemiLoc = L; }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50763: [Parser] Refactor and fix bugs in late-parsing

[Hamza Sood via Phabricator via cfe-commits]

hamzasood created this revision.
hamzasood added reviewers: erichkeane, v.g.vassilev, malcolm.parsons, rsmith.
Herald added a reviewer: javed.absar.
Herald added subscribers: cfe-commits, kristof.beyls, eraman.

This patch extracts the eof/stop token pattern used in late-parsing to a 
re-usable RAII class. This simplifies a lot of the late-parsing code.

A few related bugs were fixed in the process:

- Late-parsing a method with default arguments but without an exception 
specification would result in an invalid union access. It was mostly harmless 
but it's technically undefined behaviour.
- When an inherited late-parsed parameter is encountered, the associated 
declaration is force-casted to FunctionDecl. This caused a crash when the 
declaration is a template.


Repository:
  rC Clang

https://reviews.llvm.org/D50763

Files:
  include/clang/Parse/Parser.h
  include/clang/Sema/Sema.h
  lib/Parse/ParseCXXInlineMethods.cpp
  lib/Parse/ParseDeclCXX.cpp
  lib/Sema/SemaExprCXX.cpp
  test/CXX/dcl.decl/dcl.meaning/dcl.fct.default/p4.cpp

Index: test/CXX/dcl.decl/dcl.meaning/dcl.fct.default/p4.cpp
===
--- test/CXX/dcl.decl/dcl.meaning/dcl.fct.default/p4.cpp
+++ test/CXX/dcl.decl/dcl.meaning/dcl.fct.default/p4.cpp
@@ -61,15 +61,24 @@
 struct A {
   struct B {
 static void Foo (int = 0);
+
+template
+static void TFoo (T = 0);
   };
   
   // should not hide default args
   friend void B::Foo (int);
+
+  // Same as above, but with templates!
+  // Should not crash the compiler.
+  template
+  friend void B::TFoo (T);
 };
 
 void Test ()
 {
   A::B::Foo ();
+  A::B::TFoo ();
 }
 
 } // namespace
Index: lib/Sema/SemaExprCXX.cpp
===
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -1116,10 +1116,10 @@
   this->Enabled = true;
 }
 
-
-Sema::CXXThisScopeRAII::~CXXThisScopeRAII() {
+void Sema::CXXThisScopeRAII::Exit() {
   if (Enabled) {
 S.CXXThisTypeOverride = OldCXXThisTypeOverride;
+Enabled = false;
   }
 }
 
Index: lib/Parse/ParseDeclCXX.cpp
===
--- lib/Parse/ParseDeclCXX.cpp
+++ lib/Parse/ParseDeclCXX.cpp
@@ -2097,43 +2097,42 @@
 /// delayed, e.g., default arguments or an exception-specification, create a
 /// late-parsed method declaration record to handle the parsing at the end of
 /// the class definition.
-void Parser::HandleMemberFunctionDeclDelays(Declarator& DeclaratorInfo,
+void Parser::HandleMemberFunctionDeclDelays(Declarator ,
 Decl *ThisDecl) {
-  DeclaratorChunk::FunctionTypeInfo 
-= DeclaratorInfo.getFunctionTypeInfo();
-  // If there was a late-parsed exception-specification, we'll need a
-  // late parse
-  bool NeedLateParse = FTI.getExceptionSpecType() == EST_Unparsed;
-
-  if (!NeedLateParse) {
-// Look ahead to see if there are any default args
-for (unsigned ParamIdx = 0; ParamIdx < FTI.NumParams; ++ParamIdx) {
-  auto Param = cast(FTI.Params[ParamIdx].Param);
-  if (Param->hasUnparsedDefaultArg()) {
-NeedLateParse = true;
-break;
-  }
-}
-  }
+  DeclaratorChunk::FunctionTypeInfo  = DeclaratorInfo.getFunctionTypeInfo();
+  auto ParamInfoRange = llvm::make_range(FTI.Params,
+ FTI.Params + FTI.NumParams);
 
-  if (NeedLateParse) {
-// Push this method onto the stack of late-parsed method
-// declarations.
-auto LateMethod = new LateParsedMethodDeclaration(this, ThisDecl);
-getCurrentClass().LateParsedDeclarations.push_back(LateMethod);
-LateMethod->TemplateScope = getCurScope()->isTemplateParamScope();
+  const bool LateParseDefaultArgs =
+  llvm::any_of(ParamInfoRange, [](const DeclaratorChunk::ParamInfo )
+{ return cast(PI.Param)->hasUnparsedDefaultArg(); });
 
-// Stash the exception-specification tokens in the late-pased method.
-LateMethod->ExceptionSpecTokens = FTI.ExceptionSpecTokens;
-FTI.ExceptionSpecTokens = nullptr;
+  const bool LateParseExceptionSpec =
+  FTI.getExceptionSpecType() == EST_Unparsed;
+
+  // If we didn't delay parsing for any parts of this function, then we're done.
+  if (!LateParseDefaultArgs && !LateParseExceptionSpec)
+return;
 
-// Push tokens for each parameter.  Those that do not have
-// defaults will be NULL.
-LateMethod->DefaultArgs.reserve(FTI.NumParams);
-for (unsigned ParamIdx = 0; ParamIdx < FTI.NumParams; ++ParamIdx)
-  LateMethod->DefaultArgs.push_back(LateParsedDefaultArgument(
-  FTI.Params[ParamIdx].Param,
-  std::move(FTI.Params[ParamIdx].DefaultArgTokens)));
+  // Push this method onto the stack of late-parsed method declarations.
+  auto LateMethod = new LateParsedMethodDeclaration(this, ThisDecl);
+  getCurrentClass().LateParsedDeclarations.push_back(LateMethod);
+  LateMethod->TemplateScope = 

[PATCH] D49647: [libcxx] Library support for contracts (C++2a)

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 156857.
hamzasood added a comment.

- Made the suggested test changes.

The compiler is meant to create these; Clang doesn't do this yet but I'm 
working on the implementation.

I was under the impression that the compiler support would require the library 
stuff to land first, but I just checked Clang's coroutine tests and it looks 
like they include their own coroutine header so it doesn't depend on the 
library. I suppose the same thing can happen here, in which case I agree it's 
probably best to put this on hold in the meantime.


https://reviews.llvm.org/D49647

Files:
  include/contract
  include/module.modulemap
  test/libcxx/double_include.sh.cpp
  test/libcxx/language.support/support.contract/version.pass.cpp
  test/std/language.support/support.contract/contract_violation.pass.cpp

Index: test/std/language.support/support.contract/contract_violation.pass.cpp
===
--- test/std/language.support/support.contract/contract_violation.pass.cpp
+++ test/std/language.support/support.contract/contract_violation.pass.cpp
@@ -0,0 +1,44 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
+
+// 
+// class contract_violation
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+using CV = std::contract_violation;
+
+// Check for noexcept.
+static_assert(noexcept(std::declval().line_number()));
+static_assert(noexcept(std::declval().file_name()));
+static_assert(noexcept(std::declval().function_name()));
+static_assert(noexcept(std::declval().comment()));
+static_assert(noexcept(std::declval().assertion_level()));
+
+// Check return types.
+static_assert(std::is_same_v().line_number()),
+ std::uint_least32_t>);
+static_assert(std::is_same_v().file_name()),
+ std::string_view>);
+static_assert(std::is_same_v().function_name()),
+ std::string_view>);
+static_assert(std::is_same_v().comment()),
+ std::string_view>);
+static_assert(std::is_same_v().assertion_level()),
+ std::string_view>);
+
+int main()
+{
+}
Index: test/libcxx/language.support/support.contract/version.pass.cpp
===
--- test/libcxx/language.support/support.contract/version.pass.cpp
+++ test/libcxx/language.support/support.contract/version.pass.cpp
@@ -0,0 +1,22 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
+// 
+
+#include 
+
+#ifndef _LIBCPP_VERSION
+#error _LIBCPP_VERSION not defined
+#endif
+
+int main()
+{
+}
Index: test/libcxx/double_include.sh.cpp
===
--- test/libcxx/double_include.sh.cpp
+++ test/libcxx/double_include.sh.cpp
@@ -45,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
Index: include/module.modulemap
===
--- include/module.modulemap
+++ include/module.modulemap
@@ -255,6 +255,10 @@
 header "condition_variable"
 export *
   }
+  module contract {
+header "contract"
+export *
+  }
   module deque {
 header "deque"
 export initializer_list
Index: include/contract
===
--- include/contract
+++ include/contract
@@ -0,0 +1,67 @@
+// -*- C++ -*-
+//===-- contract --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef _LIBCPP_CONTRACT
+#define _LIBCPP_CONTRACT
+
+/*
+contract synopsis
+
+namespace std {
+  class contract_violation {
+  public:
+uint_least32_t line_number() const noexcept;
+string_view file_name() const noexcept;
+string_view function_name() const noexcept;
+string_view comment() const noexcept;
+string_view assertion_level() const noexcept;
+  };
+}
+*/
+
+#include <__config>
+#include 
+#include 
+
+#ifndef 

[PATCH] D49647: [libcxx] Library support for contracts (C++2a)

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 156808.
hamzasood added a comment.

- Added a missing visibility attribute.
- Added a simple test that type-checks the expected public interface.

Testing whether it's actually hooked up correctly is quite problematic; it 
isn't meant to be instantiable. What's the best way to proceed with this?


https://reviews.llvm.org/D49647

Files:
  include/contract
  include/module.modulemap
  test/libcxx/double_include.sh.cpp
  test/libcxx/language.support/support.contract/version.pass.cpp
  test/std/language.support/support.contract/contract_violation.pass.cpp

Index: test/std/language.support/support.contract/contract_violation.pass.cpp
===
--- test/std/language.support/support.contract/contract_violation.pass.cpp
+++ test/std/language.support/support.contract/contract_violation.pass.cpp
@@ -0,0 +1,42 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
+
+// 
+// class contract_violation
+
+#include 
+#include 
+#include 
+
+#include "test_macros.h"
+
+// Check that the expected public members exist and have the required type.
+// The contract_violation is taken as a const reference, which ensures that the
+// member functions we access are marked as const.
+void check_public_member_types(const std::contract_violation ) {
+  (void)violation;
+
+  ASSERT_NOEXCEPT(violation.line_number());
+  ASSERT_NOEXCEPT(violation.file_name());
+  ASSERT_NOEXCEPT(violation.function_name());
+  ASSERT_NOEXCEPT(violation.comment());
+  ASSERT_NOEXCEPT(violation.assertion_level());
+
+  ASSERT_SAME_TYPE(decltype(violation.line_number()), std::uint_least32_t);
+  ASSERT_SAME_TYPE(decltype(violation.file_name()), std::string_view);
+  ASSERT_SAME_TYPE(decltype(violation.function_name()), std::string_view);
+  ASSERT_SAME_TYPE(decltype(violation.comment()), std::string_view);
+  ASSERT_SAME_TYPE(decltype(violation.assertion_level()), std::string_view);
+}
+
+int main()
+{
+}
Index: test/libcxx/language.support/support.contract/version.pass.cpp
===
--- test/libcxx/language.support/support.contract/version.pass.cpp
+++ test/libcxx/language.support/support.contract/version.pass.cpp
@@ -0,0 +1,20 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// 
+
+#include 
+
+#ifndef _LIBCPP_VERSION
+#error _LIBCPP_VERSION not defined
+#endif
+
+int main()
+{
+}
Index: test/libcxx/double_include.sh.cpp
===
--- test/libcxx/double_include.sh.cpp
+++ test/libcxx/double_include.sh.cpp
@@ -45,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
Index: include/module.modulemap
===
--- include/module.modulemap
+++ include/module.modulemap
@@ -255,6 +255,10 @@
 header "condition_variable"
 export *
   }
+  module contract {
+header "contract"
+export *
+  }
   module deque {
 header "deque"
 export initializer_list
Index: include/contract
===
--- include/contract
+++ include/contract
@@ -0,0 +1,67 @@
+// -*- C++ -*-
+//===-- contract --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef _LIBCPP_CONTRACT
+#define _LIBCPP_CONTRACT
+
+/*
+contract synopsis
+
+namespace std {
+  class contract_violation {
+  public:
+uint_least32_t line_number() const noexcept;
+string_view file_name() const noexcept;
+string_view function_name() const noexcept;
+string_view comment() const noexcept;
+string_view assertion_level() const noexcept;
+  };
+}
+*/
+
+#include <__config>
+#include 
+#include 
+
+#ifndef _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER
+#pragma GCC system_header
+#endif
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+#if _LIBCPP_STD_VER > 17
+
+class _LIBCPP_TYPE_VIS contract_violation {
+public:
+  _LIBCPP_INLINE_VISIBILITY uint_least32_t line_number() const noexcept { 

[PATCH] D49647: [libcxx] Library support for contracts (C++2a)

[Hamza Sood via Phabricator via cfe-commits]

hamzasood created this revision.
hamzasood added reviewers: EricWF, mclow.lists, rsmith.
Herald added subscribers: cfe-commits, ldionne, christof.

This patch adds the library components needed for contracts in C++2a.

The wording says that a contract_violation object is populated in an 
implementation-defined manner. A private constructor that the compiler can call 
seems like the most sensible (and obvious) solution.


Repository:
  rCXX libc++

https://reviews.llvm.org/D49647

Files:
  include/contract
  include/module.modulemap
  test/libcxx/double_include.sh.cpp
  test/libcxx/language.support/support.contract/version.pass.cpp

Index: test/libcxx/language.support/support.contract/version.pass.cpp
===
--- test/libcxx/language.support/support.contract/version.pass.cpp
+++ test/libcxx/language.support/support.contract/version.pass.cpp
@@ -0,0 +1,20 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+// 
+
+#include 
+
+#ifndef _LIBCPP_VERSION
+#error _LIBCPP_VERSION not defined
+#endif
+
+int main()
+{
+}
Index: test/libcxx/double_include.sh.cpp
===
--- test/libcxx/double_include.sh.cpp
+++ test/libcxx/double_include.sh.cpp
@@ -45,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
Index: include/module.modulemap
===
--- include/module.modulemap
+++ include/module.modulemap
@@ -255,6 +255,10 @@
 header "condition_variable"
 export *
   }
+  module contract {
+header "contract"
+export *
+  }
   module deque {
 header "deque"
 export initializer_list
Index: include/contract
===
--- include/contract
+++ include/contract
@@ -0,0 +1,67 @@
+// -*- C++ -*-
+//===-- contract --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef _LIBCPP_CONTRACT
+#define _LIBCPP_CONTRACT
+
+/*
+contract synopsis
+
+namespace std {
+  class contract_violation {
+  public:
+uint_least32_t line_number() const noexcept;
+string_view file_name() const noexcept;
+string_view function_name() const noexcept;
+string_view comment() const noexcept;
+string_view assertion_level() const noexcept;
+  };
+}
+*/
+
+#include <__config>
+#include 
+#include 
+
+#ifndef _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER
+#pragma GCC system_header
+#endif
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+#if _LIBCPP_STD_VER > 17
+
+class contract_violation {
+public:
+  _LIBCPP_INLINE_VISIBILITY uint_least32_t line_number() const noexcept { return __line_number; }
+  _LIBCPP_INLINE_VISIBILITY string_view file_name() const noexcept { return __file_name; }
+  _LIBCPP_INLINE_VISIBILITY string_view function_name() const noexcept { return __function_name; }
+  _LIBCPP_INLINE_VISIBILITY string_view comment() const noexcept { return __comment; }
+  _LIBCPP_INLINE_VISIBILITY string_view assertion_level() const noexcept { return __assertion_level; }
+
+private:
+  _LIBCPP_INLINE_VISIBILITY
+  constexpr contract_violation(uint_least32_t __line, string_view __file, string_view __fn,
+   string_view __comment, string_view __lvl) noexcept
+  : __line_number(__line), __file_name(__file), __function_name(__fn),
+__comment(__comment), __assertion_level(__lvl) {}
+
+  uint_least32_t __line_number;
+  string_view __file_name;
+  string_view __function_name;
+  string_view __comment;
+  string_view __assertion_level;
+};
+
+#endif // _LIBCPP_STD_VER > 17
+
+_LIBCPP_END_NAMESPACE_STD
+
+#endif // _LIBCPP_CONTRACT
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36527: Implemented P0428R2 - Familiar template syntax for generic lambdas

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 156693.
hamzasood added a comment.

- Resolved merge conflicts


https://reviews.llvm.org/D36527

Files:
  include/clang/AST/DeclCXX.h
  include/clang/AST/DeclTemplate.h
  include/clang/AST/ExprCXX.h
  include/clang/AST/RecursiveASTVisitor.h
  include/clang/Basic/DiagnosticParseKinds.td
  include/clang/Parse/Parser.h
  include/clang/Sema/ScopeInfo.h
  include/clang/Sema/Sema.h
  lib/AST/DeclCXX.cpp
  lib/AST/DeclPrinter.cpp
  lib/AST/ExprCXX.cpp
  lib/AST/ItaniumMangle.cpp
  lib/AST/StmtPrinter.cpp
  lib/AST/TypePrinter.cpp
  lib/Parse/ParseExprCXX.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaType.cpp
  test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  test/CodeGenCXX/mangle-lambda-explicit-template-params.cpp
  test/Index/print-display-names.cpp
  test/PCH/cxx11-lambdas.mm
  test/PCH/cxx1y-lambdas.mm
  test/PCH/cxx2a-template-lambdas.cpp
  test/Parser/cxx2a-template-lambdas.cpp
  test/SemaCXX/cxx2a-template-lambdas.cpp
  unittests/AST/StmtPrinterTest.cpp
  unittests/Tooling/RecursiveASTVisitorTests/LambdaTemplateParams.cpp
  unittests/Tooling/TestVisitor.h
  www/cxx_status.html

Index: www/cxx_status.html
===
--- www/cxx_status.html
+++ www/cxx_status.html
@@ -860,7 +860,7 @@
 
   template-parameter-list for generic lambdas
   http://wg21.link/p0428r2;>P0428R2
-  No
+  SVN
 
 
   Concepts
Index: unittests/Tooling/TestVisitor.h
===
--- unittests/Tooling/TestVisitor.h
+++ unittests/Tooling/TestVisitor.h
@@ -44,6 +44,8 @@
 Lang_CXX98,
 Lang_CXX11,
 Lang_CXX14,
+Lang_CXX17,
+Lang_CXX2a,
 Lang_OBJC,
 Lang_OBJCXX11,
 Lang_CXX = Lang_CXX98
@@ -60,6 +62,8 @@
   case Lang_CXX98: Args.push_back("-std=c++98"); break;
   case Lang_CXX11: Args.push_back("-std=c++11"); break;
   case Lang_CXX14: Args.push_back("-std=c++14"); break;
+  case Lang_CXX17: Args.push_back("-std=c++17"); break;
+  case Lang_CXX2a: Args.push_back("-std=c++2a"); break;
   case Lang_OBJC:
 Args.push_back("-ObjC");
 Args.push_back("-fobjc-runtime=macosx-10.12.0");
Index: unittests/Tooling/RecursiveASTVisitorTests/LambdaTemplateParams.cpp
===
--- unittests/Tooling/RecursiveASTVisitorTests/LambdaTemplateParams.cpp
+++ unittests/Tooling/RecursiveASTVisitorTests/LambdaTemplateParams.cpp
@@ -0,0 +1,53 @@
+//===- unittest/Tooling/RecursiveASTVisitorTests/LambdaTemplateParams.cpp -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "TestVisitor.h"
+
+using namespace clang;
+
+namespace {
+
+// Matches (optional) explicit template parameters.
+class LambdaTemplateParametersVisitor
+  : public ExpectedLocationVisitor {
+public:
+  bool shouldVisitImplicitCode() const { return false; }
+
+  bool VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
+EXPECT_FALSE(D->isImplicit());
+Match(D->getName(), D->getLocStart());
+return true;
+  }
+
+  bool VisitNonTypeTemplateParmDecl(NonTypeTemplateParmDecl *D) {
+EXPECT_FALSE(D->isImplicit());
+Match(D->getName(), D->getLocStart());
+return true;
+  }
+
+  bool VisitTemplateTemplateParmDecl(TemplateTemplateParmDecl *D) {
+EXPECT_FALSE(D->isImplicit());
+Match(D->getName(), D->getLocStart());
+return true;
+  }
+};
+
+TEST(RecursiveASTVisitor, VisitsLambdaExplicitTemplateParameters) {
+  LambdaTemplateParametersVisitor Visitor;
+  Visitor.ExpectMatch("T",  2, 15);
+  Visitor.ExpectMatch("I",  2, 24);
+  Visitor.ExpectMatch("TT", 2, 31);
+  EXPECT_TRUE(Visitor.runOver(
+  "void f() { \n"
+  "  auto l = [] class TT>(auto p) { }; \n"
+  "}",
+  LambdaTemplateParametersVisitor::Lang_CXX2a));
+}
+
+} // end anonymous namespace
Index: unittests/AST/StmtPrinterTest.cpp
===
--- unittests/AST/StmtPrinterTest.cpp
+++ unittests/AST/StmtPrinterTest.cpp
@@ -106,78 +106,73 @@
   return ::testing::AssertionSuccess();
 }
 
-::testing::AssertionResult
-PrintedStmtCXX98Matches(StringRef Code, const StatementMatcher ,
-StringRef ExpectedPrinted) {
-  std::vector Args;
-  Args.push_back("-std=c++98");
-  Args.push_back("-Wno-unused-value");
-  return PrintedStmtMatches(Code, Args, NodeMatch, ExpectedPrinted);
-}
-
-::testing::AssertionResult PrintedStmtCXX98Matches(
-  StringRef Code,
-  StringRef ContainingFunction,
-  StringRef ExpectedPrinted) {
-  std::vector Args;
-  

[PATCH] D41627: [Modules TS] Fix namespace visibility

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 156689.
hamzasood added a comment.

- Don't assume that -fmodules-ts implies that we're in a TS module.
- Don't ignore private namespaces when calculating the ownership kind.


https://reviews.llvm.org/D41627

Files:
  include/clang/AST/DeclBase.h
  lib/AST/DeclBase.cpp
  lib/Sema/SemaDeclCXX.cpp
  test/CXX/modules-ts/basic/basic.namespace/p1.cpp


Index: test/CXX/modules-ts/basic/basic.namespace/p1.cpp
===
--- test/CXX/modules-ts/basic/basic.namespace/p1.cpp
+++ test/CXX/modules-ts/basic/basic.namespace/p1.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fmodules-ts -DMODULE -emit-module-interface %s -o %t
+// RUN: %clang_cc1 -fmodules-ts -DUSER -fmodule-file=%t %s -verify
+// expected-no-diagnostics
+
+#ifdef MODULE
+export module A;
+namespace ns {
+  export class A { };
+}
+#endif
+
+#ifdef USER
+import A;
+ns::A a;
+#endif
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -8866,6 +8866,21 @@
   // for the namespace has the declarations that showed up in that particular
   // namespace definition.
   PushDeclContext(NamespcScope, Namespc);
+
+  if (const Module *CurrentModule = getCurrentModule()) {
+// Under the Modules TS, a namespace with external linkage should always be
+// visible when imported, regardless of whether it's explicitly exported.
+const bool IsTSModule = CurrentModule->Kind == Module::ModuleInterfaceUnit 
||
+CurrentModule->Kind == 
Module::GlobalModuleFragment;
+if (IsTSModule &&
+!Namespc->isAnonymousNamespace() && 
!Namespc->isInAnonymousNamespace()) {
+  Namespc->setModuleOwnershipKind(
+CurrentModule->Kind == Module::ModuleKind::GlobalModuleFragment
+? Decl::ModuleOwnershipKind::Visible
+: Decl::ModuleOwnershipKind::VisibleWhenImported);
+}
+  }
+
   return Namespc;
 }
 
Index: lib/AST/DeclBase.cpp
===
--- lib/AST/DeclBase.cpp
+++ lib/AST/DeclBase.cpp
@@ -292,6 +292,23 @@
 // Out-of-line virtual method providing a home for Decl.
 Decl::~Decl() = default;
 
+Decl::ModuleOwnershipKind Decl::getModuleOwnershipKindForChildOf(DeclContext 
*DC) {
+  // Ignore public namespaces because they might be visible by default.
+  while (DC && DC->isNamespace() && !cast(DC)->isModulePrivate())
+DC = DC->getParent();
+
+  if (auto *D = cast_or_null(DC)) {
+auto MOK = D->getModuleOwnershipKind();
+if (MOK != ModuleOwnershipKind::Unowned &&
+(!D->isFromASTFile() || D->hasLocalOwningModuleStorage()))
+  return MOK;
+// If D is not local and we have no local module storage, then we don't
+// need to track module ownership at all.
+  }
+
+  return ModuleOwnershipKind::Unowned;
+}
+
 void Decl::setDeclContext(DeclContext *DC) {
   DeclCtx = DC;
 }
Index: include/clang/AST/DeclBase.h
===
--- include/clang/AST/DeclBase.h
+++ include/clang/AST/DeclBase.h
@@ -350,18 +350,7 @@
 
   /// Get the module ownership kind to use for a local lexical child of \p DC,
   /// which may be either a local or (rarely) an imported declaration.
-  static ModuleOwnershipKind getModuleOwnershipKindForChildOf(DeclContext *DC) 
{
-if (DC) {
-  auto *D = cast(DC);
-  auto MOK = D->getModuleOwnershipKind();
-  if (MOK != ModuleOwnershipKind::Unowned &&
-  (!D->isFromASTFile() || D->hasLocalOwningModuleStorage()))
-return MOK;
-  // If D is not local and we have no local module storage, then we don't
-  // need to track module ownership at all.
-}
-return ModuleOwnershipKind::Unowned;
-  }
+  static ModuleOwnershipKind getModuleOwnershipKindForChildOf(DeclContext *DC);
 
 protected:
   Decl(Kind DK, DeclContext *DC, SourceLocation L)


Index: test/CXX/modules-ts/basic/basic.namespace/p1.cpp
===
--- test/CXX/modules-ts/basic/basic.namespace/p1.cpp
+++ test/CXX/modules-ts/basic/basic.namespace/p1.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fmodules-ts -DMODULE -emit-module-interface %s -o %t
+// RUN: %clang_cc1 -fmodules-ts -DUSER -fmodule-file=%t %s -verify
+// expected-no-diagnostics
+
+#ifdef MODULE
+export module A;
+namespace ns {
+  export class A { };
+}
+#endif
+
+#ifdef USER
+import A;
+ns::A a;
+#endif
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -8866,6 +8866,21 @@
   // for the namespace has the declarations that showed up in that particular
   // namespace definition.
   PushDeclContext(NamespcScope, Namespc);
+
+  if (const Module *CurrentModule = getCurrentModule()) {
+// Under the Modules TS, a namespace with external linkage should 

[PATCH] D41566: [Modules TS] Diagnose exported internal linkage declarations

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 156681.
hamzasood added a comment.

- Fixed the premature loop termination issue pointed out by @rsmith (and added 
a test case for it).
- Fixed a few small issues regarding diagnosing exported enum/struct/union 
declarations without a name (especially anonymous unions at namespace-scope).
- Factored out the loop body into a separate function to keep things tidy.


https://reviews.llvm.org/D41566

Files:
  include/clang/AST/DeclBase.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDecl.cpp
  test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp
  test/CXX/modules-ts/basic/basic.def.odr/p4/module.cppm
  test/CXX/modules-ts/basic/basic.def.odr/p4/user.cpp
  test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p2.cpp
  test/SemaCXX/anonymous-union-export.cpp

Index: test/SemaCXX/anonymous-union-export.cpp
===
--- test/SemaCXX/anonymous-union-export.cpp
+++ test/SemaCXX/anonymous-union-export.cpp
@@ -1,6 +1,14 @@
 // RUN: %clang_cc1 -std=c++17 -fmodules-ts -emit-obj -verify -o %t.pcm %s
 
 export module M;
+
 export {
-union { bool a; }; // expected-error{{anonymous unions at namespace or global scope must be declared 'static'}}
+union { bool a; }; // expected-error{{anonymous unions at namespace or global scope must be declared 'static'}} \
+   // expected-error{{anonymous unions at namespace or global scope cannot be exported}}
+
+static union { bool a; }; // expected-error{{anonymous unions at namespace or global scope cannot be exported}}
+}
+
+namespace {
+export union { bool a; }; // expected-error{{anonymous unions at namespace or global scope cannot be exported}}
 }
Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p2.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p2.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p2.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -fmodules-ts %s -emit-module-interface -verify -o /dev/null
+
+export module A;
+
+export namespace { } // expected-error {{unnamed namespace}}
+
+export static int n = 5; // expected-error {{internal linkage}}
+
+namespace { // expected-note 5 {{in this}}
+  export {
+int a = 1;// expected-error {{internal linkage}}
+void f() { }  // expected-error {{internal linkage}}
+class B { };  // expected-error {{internal linkage}}
+struct { } x; // expected-error {{internal linkage}}
+
+extern "C++" void g() { } // expected-error {{internal linkage}}
+  }
+}
+
+export namespace a {
+  namespace b {
+namespace c {
+  static int i = 3; // expected-error {{internal linkage}}
+}
+  }
+}
Index: test/CXX/modules-ts/basic/basic.def.odr/p4/user.cpp
===
--- test/CXX/modules-ts/basic/basic.def.odr/p4/user.cpp
+++ test/CXX/modules-ts/basic/basic.def.odr/p4/user.cpp
@@ -3,7 +3,6 @@
 
 // CHECK-DAG: @extern_var_exported = external {{(dso_local )?}}global
 // CHECK-DAG: @inline_var_exported = linkonce_odr {{(dso_local )?}}global
-// CHECK-DAG: @_ZW6ModuleE19static_var_exported = available_externally {{(dso_local )?}}global i32 0
 // CHECK-DAG: @const_var_exported = available_externally {{(dso_local )?}}constant i32 3
 
 import Module;
@@ -16,7 +15,6 @@
 
   (void)_var_exported;
   (void)_var_exported;
-  (void)_var_exported;
   (void)_var_exported;
 
   // Module-linkage declarations are not visible here.
Index: test/CXX/modules-ts/basic/basic.def.odr/p4/module.cppm
===
--- test/CXX/modules-ts/basic/basic.def.odr/p4/module.cppm
+++ test/CXX/modules-ts/basic/basic.def.odr/p4/module.cppm
@@ -11,7 +11,6 @@
 // can discard this global and its initializer (if any), and other TUs are not
 // permitted to run the initializer for this variable.
 // CHECK-DAG: @inline_var_exported = linkonce_odr {{(dso_local )?}}global
-// CHECK-DAG: @_ZW6ModuleE19static_var_exported = {{(dso_local )?}}global
 // CHECK-DAG: @const_var_exported = {{(dso_local )?}}constant
 //
 // CHECK-DAG: @_ZW6ModuleE25extern_var_module_linkage = external {{(dso_local )?}}global
@@ -58,32 +57,20 @@
 export module Module;
 
 export {
-  // FIXME: These should be ill-formed: you can't export an internal linkage
-  // symbol, per [dcl.module.interface]p2.
-  // CHECK: define {{(dso_local )?}}void {{.*}}@_ZW6ModuleE22unused_static_exportedv
-  static void unused_static_exported() {}
-  // CHECK: define {{(dso_local )?}}void {{.*}}@_ZW6ModuleE20used_static_exportedv
-  static void used_static_exported() {}
-
   inline void unused_inline_exported() {}
   inline void used_inline_exported() {}
 
   extern int extern_var_exported;
   inline int inline_var_exported;
-  // FIXME: This should be ill-formed: you can't export an internal linkage
-  // symbol.
-  static int 

[PATCH] D40443: [Modules TS] Make imports from an interface unit visible to its implementation units

[Hamza Sood via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaDecl.cpp:16185-16194
+  if (getLangOpts().ModulesTS) {
+Module *CurrentModule = getCurrentModule();
+assert(CurrentModule && "Expected to be in a module scope");
+
+// If the current module has been loaded from disk, then this is an
+// implementation unit and hence we shouldn't modify the module.
+// FIXME: Is that a hacky assumption? We can't just check

rsmith wrote:
> This is not appropriate; generally whether we serialize to an AST file should 
> be treated as orthogonal to whether we're in / importing a module.
> 
> The right check here is probably `getLangOpts().getCompilingModule() == 
> CMK_ModuleInterface`.
Thanks! I completely missed that lang opt.


https://reviews.llvm.org/D40443



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

[PATCH] D40443: [Modules TS] Make imports from an interface unit visible to its implementation units

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 150649.
hamzasood added a comment.

Addressed Richard's comments.


https://reviews.llvm.org/D40443

Files:
  include/clang/Basic/Module.h
  lib/Basic/Module.cpp
  lib/Sema/SemaDecl.cpp
  test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
  test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
  
test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp

Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
@@ -1,28 +0,0 @@
-// RUN: %clang_cc1 -fmodules-ts %s -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DINTERFACE -verify -emit-module-interface -o %t
-// RUN: %clang_cc1 -fmodules-ts %s -DIMPLEMENTATION -verify -fmodule-file=%t -o /dev/null
-//
-// RUN: %clang_cc1 -fmodules-ts %s -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DINTERFACE -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DIMPLEMENTATION -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-
-#if INTERFACE
-// expected-no-diagnostics
-export module A;
-#elif IMPLEMENTATION
-module A;
- #ifdef BUILT_AS_INTERFACE
-  // expected-error@-2 {{missing 'export' specifier in module declaration while building module interface}}
-  #define INTERFACE
- #endif
-#else
- #ifdef BUILT_AS_INTERFACE
-  // expected-error@1 {{missing 'export module' declaration in module interface unit}}
- #endif
-#endif
-
-#ifndef INTERFACE
-export int b; // expected-error {{export declaration can only be used within a module interface unit}}
-#else
-export int a;
-#endif
Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
@@ -0,0 +1,40 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: echo 'export module a; export class A { };' > %t/a.cppm
+// RUN: echo 'export module b; export class B { };' > %t/b.cppm
+//
+// RUN: %clang_cc1 -fmodules-ts -emit-module-interface %t/a.cppm -o %t/a.pcm
+// RUN: %clang_cc1 -fmodules-ts -emit-module-interface %t/b.cppm -o %t/b.pcm
+// RUN: %clang_cc1 -fmodules-ts -fprebuilt-module-path=%t -emit-module-interface %s -o %t/test.pcm -DTEST_INTERFACE
+//
+// RUN: %clang_cc1 -fmodules-ts -I%t -fmodule-file=%t/test.pcm %s -verify -DTEST_IMPLEMENTATION
+// RUN: %clang_cc1 -fmodules-ts -I%t -fmodule-file=%t/test.pcm %s -verify -DOTHER_TU
+
+
+#ifdef TEST_INTERFACE
+import a;
+export module test;
+import b;
+#endif
+
+#ifdef TEST_IMPLEMENTATION
+module test;
+
+A a; // expected-error {{must be imported from module 'a'}}
+ // expected-n...@a.cppm:1 {{here}}
+
+// Module b is imported within the purview of this module's interface unit.
+// So its exported definitions should be visible here.
+B b;
+#endif
+
+
+#ifdef OTHER_TU
+import test;
+
+A a; // expected-error {{must be imported from module 'a'}}
+ // expected-n...@a.cppm:1 {{here}}
+
+B b; // expected-error {{must be imported from module 'b'}}
+ // expected-n...@b.cppm:1 {{here}}
+#endif
Index: test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
===
--- test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
+++ test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
@@ -10,9 +10,7 @@
 //
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/M.pcm -emit-module-interface -o %t/N.pcm -DMODULE_INTERFACE -DNO_ERRORS
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -verify
-// FIXME: Once we start importing "import" declarations properly, this should
-// be rejected (-verify should be added to the following line).
-// RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -DNO_IMPORT
+// RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -DNO_IMPORT -verify
 //
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/M.pcm -emit-module-interface -o %t/N-no-M.pcm -DMODULE_INTERFACE -DNO_ERRORS -DNO_IMPORT
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N-no-M.pcm -verify
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -16570,6 +16570,17 @@
   TU->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModulePrivate);
   TU->setLocalOwningModule(Mod);
 
+  // Modules TS + p0731r0 [dcl.module.interface]p1:
+  //   Every name of an entity with linkage other than internal linkage made
+  //   visible in the 

[PATCH] D41627: [Modules TS] Fix namespace visibility

[Hamza Sood via Phabricator via cfe-commits]

hamzasood created this revision.
hamzasood added reviewers: rsmith, bruno, boris.

Namespaces without an explicit `export` were being mistakenly marked as module 
private.
This patch fixes the visibility as per `[basic.namespace]p1` and adds a test 
case with previously rejected (yet valid) code.


https://reviews.llvm.org/D41627

Files:
  include/clang/AST/DeclBase.h
  lib/AST/DeclBase.cpp
  lib/Sema/SemaDeclCXX.cpp
  test/CXX/modules-ts/basic/basic.namespace/p1.cpp


Index: test/CXX/modules-ts/basic/basic.namespace/p1.cpp
===
--- test/CXX/modules-ts/basic/basic.namespace/p1.cpp
+++ test/CXX/modules-ts/basic/basic.namespace/p1.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fmodules-ts -DMODULE -emit-module-interface %s -o %t
+// RUN: %clang_cc1 -fmodules-ts -DUSER -fmodule-file=%t %s -verify
+// expected-no-diagnostics
+
+#ifdef MODULE
+export module A;
+namespace ns {
+  export class A { };
+}
+#endif
+
+#ifdef USER
+import A;
+ns::A a;
+#endif
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -8623,6 +8623,19 @@
   // for the namespace has the declarations that showed up in that particular
   // namespace definition.
   PushDeclContext(NamespcScope, Namespc);
+
+  // A namespace with external linkage should always be visible when imported,
+  // regardless of whether or not it's explicitly exported.
+  if (getLangOpts().ModulesTS &&
+  !Namespc->isAnonymousNamespace() && !Namespc->isInAnonymousNamespace()) {
+const Module *CurrentModule = getCurrentModule();
+assert(CurrentModule);
+Namespc->setModuleOwnershipKind(
+CurrentModule->Kind == Module::ModuleKind::GlobalModuleFragment
+? Decl::ModuleOwnershipKind::Visible
+: Decl::ModuleOwnershipKind::VisibleWhenImported);
+  }
+
   return Namespc;
 }
 
Index: lib/AST/DeclBase.cpp
===
--- lib/AST/DeclBase.cpp
+++ lib/AST/DeclBase.cpp
@@ -281,6 +281,23 @@
 // Out-of-line virtual method providing a home for Decl.
 Decl::~Decl() = default;
 
+Decl::ModuleOwnershipKind Decl::getModuleOwnershipKindForChildOf(DeclContext 
*DC) {
+  // Ignore namespaces because they're visible by default.
+  while (DC && DC->isNamespace())
+DC = DC->getParent();
+
+  if (auto *D = cast_or_null(DC)) {
+auto MOK = D->getModuleOwnershipKind();
+if (MOK != ModuleOwnershipKind::Unowned &&
+(!D->isFromASTFile() || D->hasLocalOwningModuleStorage()))
+  return MOK;
+// If D is not local and we have no local module storage, then we don't
+// need to track module ownership at all.
+  }
+
+  return ModuleOwnershipKind::Unowned;
+}
+
 void Decl::setDeclContext(DeclContext *DC) {
   DeclCtx = DC;
 }
Index: include/clang/AST/DeclBase.h
===
--- include/clang/AST/DeclBase.h
+++ include/clang/AST/DeclBase.h
@@ -349,18 +349,7 @@
 
   /// Get the module ownership kind to use for a local lexical child of \p DC,
   /// which may be either a local or (rarely) an imported declaration.
-  static ModuleOwnershipKind getModuleOwnershipKindForChildOf(DeclContext *DC) 
{
-if (DC) {
-  auto *D = cast(DC);
-  auto MOK = D->getModuleOwnershipKind();
-  if (MOK != ModuleOwnershipKind::Unowned &&
-  (!D->isFromASTFile() || D->hasLocalOwningModuleStorage()))
-return MOK;
-  // If D is not local and we have no local module storage, then we don't
-  // need to track module ownership at all.
-}
-return ModuleOwnershipKind::Unowned;
-  }
+  static ModuleOwnershipKind getModuleOwnershipKindForChildOf(DeclContext *DC);
 
 protected:
   Decl(Kind DK, DeclContext *DC, SourceLocation L)


Index: test/CXX/modules-ts/basic/basic.namespace/p1.cpp
===
--- test/CXX/modules-ts/basic/basic.namespace/p1.cpp
+++ test/CXX/modules-ts/basic/basic.namespace/p1.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fmodules-ts -DMODULE -emit-module-interface %s -o %t
+// RUN: %clang_cc1 -fmodules-ts -DUSER -fmodule-file=%t %s -verify
+// expected-no-diagnostics
+
+#ifdef MODULE
+export module A;
+namespace ns {
+  export class A { };
+}
+#endif
+
+#ifdef USER
+import A;
+ns::A a;
+#endif
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -8623,6 +8623,19 @@
   // for the namespace has the declarations that showed up in that particular
   // namespace definition.
   PushDeclContext(NamespcScope, Namespc);
+
+  // A namespace with external linkage should always be visible when imported,
+  // regardless of whether or not it's explicitly exported.
+  if (getLangOpts().ModulesTS &&
+  !Namespc->isAnonymousNamespace() && !Namespc->isInAnonymousNamespace()) {
+

[PATCH] D41566: [Modules TS] Diagnose exported internal linkage declarations

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 128302.
hamzasood added a comment.

I've removed the `isExported` fix for namespaces as it's somewhat unrelated to 
this patch.
I'll do a separate patch for fixing namespaces (which will include the stuff 
removed from here and a bit more)


https://reviews.llvm.org/D41566

Files:
  include/clang/AST/DeclBase.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDecl.cpp
  test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp
  test/CXX/modules-ts/basic/basic.def.odr/p4/module.cppm
  test/CXX/modules-ts/basic/basic.def.odr/p4/user.cpp
  test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p2.cpp

Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p2.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p2.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p2.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -fmodules-ts %s -emit-module-interface -verify
+
+export module A;
+
+export namespace { } // expected-error {{unnamed namespace}}
+
+export static int n = 5; // expected-error {{internal linkage}}
+
+namespace { // expected-note 3{{in this}}
+  export {
+int a = 1;   // expected-error {{internal linkage}}
+void f() { } // expected-error {{internal linkage}}
+class B { }; // expected-error {{internal linkage}}
+  }
+}
+
+export namespace a {
+  namespace b {
+namespace c {
+  static int i = 3; // expected-error {{internal linkage}}
+}
+  }
+}
Index: test/CXX/modules-ts/basic/basic.def.odr/p4/user.cpp
===
--- test/CXX/modules-ts/basic/basic.def.odr/p4/user.cpp
+++ test/CXX/modules-ts/basic/basic.def.odr/p4/user.cpp
@@ -3,7 +3,6 @@
 
 // CHECK-DAG: @extern_var_exported = external global
 // CHECK-DAG: @inline_var_exported = linkonce_odr global
-// CHECK-DAG: @_ZW6ModuleE19static_var_exported = available_externally global i32 0
 // CHECK-DAG: @const_var_exported = available_externally constant i32 3
 
 import Module;
@@ -16,7 +15,6 @@
 
   (void)_var_exported;
   (void)_var_exported;
-  (void)_var_exported;
   (void)_var_exported;
 
   // Module-linkage declarations are not visible here.
Index: test/CXX/modules-ts/basic/basic.def.odr/p4/module.cppm
===
--- test/CXX/modules-ts/basic/basic.def.odr/p4/module.cppm
+++ test/CXX/modules-ts/basic/basic.def.odr/p4/module.cppm
@@ -11,7 +11,6 @@
 // can discard this global and its initializer (if any), and other TUs are not
 // permitted to run the initializer for this variable.
 // CHECK-DAG: @inline_var_exported = linkonce_odr global
-// CHECK-DAG: @_ZW6ModuleE19static_var_exported = global
 // CHECK-DAG: @const_var_exported = constant
 //
 // CHECK-DAG: @_ZW6ModuleE25extern_var_module_linkage = external global
@@ -58,32 +57,20 @@
 export module Module;
 
 export {
-  // FIXME: These should be ill-formed: you can't export an internal linkage
-  // symbol, per [dcl.module.interface]p2.
-  // CHECK: define void {{.*}}@_ZW6ModuleE22unused_static_exportedv
-  static void unused_static_exported() {}
-  // CHECK: define void {{.*}}@_ZW6ModuleE20used_static_exportedv
-  static void used_static_exported() {}
-
   inline void unused_inline_exported() {}
   inline void used_inline_exported() {}
 
   extern int extern_var_exported;
   inline int inline_var_exported;
-  // FIXME: This should be ill-formed: you can't export an internal linkage
-  // symbol.
-  static int static_var_exported;
   const int const_var_exported = 3;
 
   // CHECK: define void {{.*}}@_Z18noninline_exportedv
   void noninline_exported() {
-used_static_exported();
 // CHECK: define linkonce_odr {{.*}}@_Z20used_inline_exportedv
 used_inline_exported();
 
 (void)_var_exported;
 (void)_var_exported;
-(void)_var_exported;
 (void)_var_exported;
   }
 }
Index: test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp
===
--- test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp
+++ test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp
@@ -3,12 +3,10 @@
 
 // CHECK-DAG: @extern_var_exported = external global
 // CHECK-DAG: @inline_var_exported = linkonce_odr global
-// CHECK-DAG: @_ZW6ModuleE19static_var_exported = available_externally global i32 0,
 // CHECK-DAG: @const_var_exported = available_externally constant i32 3,
 //
 // CHECK-DAG: @_ZW6ModuleE25extern_var_module_linkage = external global
 // CHECK-DAG: @_ZW6ModuleE25inline_var_module_linkage = linkonce_odr global
-// CHECK-DAG: @_ZW6ModuleE25static_var_module_linkage = available_externally global i32 0,
 // CHECK-DAG: @_ZW6ModuleE24const_var_module_linkage = available_externally constant i32 3,
 
 module Module;
@@ -21,7 +19,6 @@
 
   (void)_var_exported;
   (void)_var_exported;
-  (void)_var_exported; // FIXME: Should not be exported.
   

[PATCH] D40443: [Modules TS] Make imports from an interface unit visible to its implementation units

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added inline comments.



Comment at: lib/Basic/Module.cpp:349
 
+  // Everything visible to the interface unit's global module fragment is
+  // visible to the interface unit.

boris wrote:
> This comment (not sure about the code)  sounds wrong to me: names from the 
> interface unit before the module purview (I assume this is what "global 
> module fragment" refers to) should not be visible in the implementation units.
Yes, the documentation comment for this function (in the header) notes that 
things visible to the interface unit aren’t necessarily visible to the 
implementation units.

There’re a few reasons why I chose for the function to count modules that’re 
only visible from the interface unit:


  # The `Module::Kind` for a C++ module is `ModuleInterfaceUnit`. Since the 
module identifies itself as an interface unit, I figured it’d make sense for 
queries such as this to return what’s relevant for the interface unit.
  # I think it’s just more useful that way. I can’t think of any situation 
where a module would need to know what’s visible to implementation units of 
other modules (and I couldn’t find any such examples in Clang). However 
there’re plenty of reasons why one would want to know about visibility to an 
interface unit (synthesising default members, instantiating templates etc.)




https://reviews.llvm.org/D40443



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

[PATCH] D40443: [Modules TS] Make imports from an interface unit visible to its implementation units

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 128284.
hamzasood added a comment.

I've been investigating an issue regarding visibility during code synthesis, 
and I noticed that this patch half fixes it. I've added the rest of the fix 
since it's related to what's going on here, and I guess this is now a 
"correctly handle imports from an interface unit" patch.

I believe this fixes this bug  
posted on the LLVM bug tracker.

@boris if this is still okay with you after seeing the changes I've made, would 
you mind accepting the revision so I can commit this without Phabricator 
shouting at me?


https://reviews.llvm.org/D40443

Files:
  include/clang/Basic/Module.h
  lib/Basic/Module.cpp
  lib/Sema/SemaDecl.cpp
  test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
  test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
  test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/export-kw.cpp
  
test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp

Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
@@ -1,28 +0,0 @@
-// RUN: %clang_cc1 -fmodules-ts %s -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DINTERFACE -verify -emit-module-interface -o %t
-// RUN: %clang_cc1 -fmodules-ts %s -DIMPLEMENTATION -verify -fmodule-file=%t -o /dev/null
-//
-// RUN: %clang_cc1 -fmodules-ts %s -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DINTERFACE -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DIMPLEMENTATION -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-
-#if INTERFACE
-// expected-no-diagnostics
-export module A;
-#elif IMPLEMENTATION
-module A;
- #ifdef BUILT_AS_INTERFACE
-  // expected-error@-2 {{missing 'export' specifier in module declaration while building module interface}}
-  #define INTERFACE
- #endif
-#else
- #ifdef BUILT_AS_INTERFACE
-  // expected-error@1 {{missing 'export module' declaration in module interface unit}}
- #endif
-#endif
-
-#ifndef INTERFACE
-export int b; // expected-error {{export declaration can only be used within a module interface unit}}
-#else
-export int a;
-#endif
Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
@@ -0,0 +1,40 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: echo 'export module a; export class A { };' > %t/a.cppm
+// RUN: echo 'export module b; export class B { };' > %t/b.cppm
+//
+// RUN: %clang_cc1 -fmodules-ts -emit-module-interface %t/a.cppm -o %t/a.pcm
+// RUN: %clang_cc1 -fmodules-ts -emit-module-interface %t/b.cppm -o %t/b.pcm
+// RUN: %clang_cc1 -fmodules-ts -fprebuilt-module-path=%t -emit-module-interface %s -o %t/test.pcm -DTEST_INTERFACE
+//
+// RUN: %clang_cc1 -fmodules-ts -I%t -fmodule-file=%t/test.pcm %s -verify -DTEST_IMPLEMENTATION
+// RUN: %clang_cc1 -fmodules-ts -I%t -fmodule-file=%t/test.pcm %s -verify -DOTHER_TU
+
+
+#ifdef TEST_INTERFACE
+import a;
+export module test;
+import b;
+#endif
+
+#ifdef TEST_IMPLEMENTATION
+module test;
+
+A a; // expected-error {{must be imported from module 'a'}}
+ // expected-n...@a.cppm:1 {{here}}
+
+// Module b is imported within the purview of this module's interface unit.
+// So its exported definitions should be visible here.
+B b;
+#endif
+
+
+#ifdef OTHER_TU
+import test;
+
+A a; // expected-error {{must be imported from module 'a'}}
+ // expected-n...@a.cppm:1 {{here}}
+
+B b; // expected-error {{must be imported from module 'b'}}
+ // expected-n...@b.cppm:1 {{here}}
+#endif
Index: test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
===
--- test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
+++ test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
@@ -10,9 +10,7 @@
 //
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/M.pcm -emit-module-interface -o %t/N.pcm -DMODULE_INTERFACE -DNO_ERRORS
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -verify
-// FIXME: Once we start importing "import" declarations properly, this should
-// be rejected (-verify should be added to the following line).
-// RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -DNO_IMPORT
+// RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -DNO_IMPORT -verify
 //
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s 

[PATCH] D41566: [Modules TS] Diagnose exported internal linkage declarations

[Hamza Sood via Phabricator via cfe-commits]

hamzasood created this revision.
hamzasood added reviewers: rsmith, bruno, boris.

Diagnose attempts to export declarations with internal linkage, as mentioned in 
`[dcl.module.interface]p2` in the Modules TS.

I would've liked to add a FixIt hint to remove the static keyword if present, 
but I couldn't work out how to get the `SourceRange` that covers it (it's 
accessible from the `Declarator`, but seemingly not after the actual node has 
been created).
I've left a fixme comment in there in case anyone else can figure it out.


https://reviews.llvm.org/D41566

Files:
  include/clang/AST/DeclBase.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/DeclBase.cpp
  lib/Sema/SemaDecl.cpp
  test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp
  test/CXX/modules-ts/basic/basic.def.odr/p4/module.cppm
  test/CXX/modules-ts/basic/basic.def.odr/p4/user.cpp
  test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p2.cpp

Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p2.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p2.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p2.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -fmodules-ts %s -emit-module-interface -verify
+
+export module A;
+
+export namespace { } // expected-error {{unnamed namespace}}
+
+export static int n = 5; // expected-error {{internal linkage}}
+
+namespace { // expected-note 3{{in this}}
+  export {
+int a = 1;   // expected-error {{internal linkage}}
+void f() { } // expected-error {{internal linkage}}
+class B { }; // expected-error {{internal linkage}}
+  }
+}
+
+export namespace a {
+  namespace b {
+namespace c {
+  static int i = 3; // expected-error {{internal linkage}}
+}
+  }
+}
Index: test/CXX/modules-ts/basic/basic.def.odr/p4/user.cpp
===
--- test/CXX/modules-ts/basic/basic.def.odr/p4/user.cpp
+++ test/CXX/modules-ts/basic/basic.def.odr/p4/user.cpp
@@ -3,7 +3,6 @@
 
 // CHECK-DAG: @extern_var_exported = external global
 // CHECK-DAG: @inline_var_exported = linkonce_odr global
-// CHECK-DAG: @_ZW6ModuleE19static_var_exported = available_externally global i32 0
 // CHECK-DAG: @const_var_exported = available_externally constant i32 3
 
 import Module;
@@ -16,7 +15,6 @@
 
   (void)_var_exported;
   (void)_var_exported;
-  (void)_var_exported;
   (void)_var_exported;
 
   // Module-linkage declarations are not visible here.
Index: test/CXX/modules-ts/basic/basic.def.odr/p4/module.cppm
===
--- test/CXX/modules-ts/basic/basic.def.odr/p4/module.cppm
+++ test/CXX/modules-ts/basic/basic.def.odr/p4/module.cppm
@@ -11,7 +11,6 @@
 // can discard this global and its initializer (if any), and other TUs are not
 // permitted to run the initializer for this variable.
 // CHECK-DAG: @inline_var_exported = linkonce_odr global
-// CHECK-DAG: @_ZW6ModuleE19static_var_exported = global
 // CHECK-DAG: @const_var_exported = constant
 //
 // CHECK-DAG: @_ZW6ModuleE25extern_var_module_linkage = external global
@@ -58,32 +57,20 @@
 export module Module;
 
 export {
-  // FIXME: These should be ill-formed: you can't export an internal linkage
-  // symbol, per [dcl.module.interface]p2.
-  // CHECK: define void {{.*}}@_ZW6ModuleE22unused_static_exportedv
-  static void unused_static_exported() {}
-  // CHECK: define void {{.*}}@_ZW6ModuleE20used_static_exportedv
-  static void used_static_exported() {}
-
   inline void unused_inline_exported() {}
   inline void used_inline_exported() {}
 
   extern int extern_var_exported;
   inline int inline_var_exported;
-  // FIXME: This should be ill-formed: you can't export an internal linkage
-  // symbol.
-  static int static_var_exported;
   const int const_var_exported = 3;
 
   // CHECK: define void {{.*}}@_Z18noninline_exportedv
   void noninline_exported() {
-used_static_exported();
 // CHECK: define linkonce_odr {{.*}}@_Z20used_inline_exportedv
 used_inline_exported();
 
 (void)_var_exported;
 (void)_var_exported;
-(void)_var_exported;
 (void)_var_exported;
   }
 }
Index: test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp
===
--- test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp
+++ test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp
@@ -3,12 +3,10 @@
 
 // CHECK-DAG: @extern_var_exported = external global
 // CHECK-DAG: @inline_var_exported = linkonce_odr global
-// CHECK-DAG: @_ZW6ModuleE19static_var_exported = available_externally global i32 0,
 // CHECK-DAG: @const_var_exported = available_externally constant i32 3,
 //
 // CHECK-DAG: @_ZW6ModuleE25extern_var_module_linkage = external global
 // CHECK-DAG: @_ZW6ModuleE25inline_var_module_linkage = linkonce_odr global
-// CHECK-DAG: @_ZW6ModuleE25static_var_module_linkage = 

[PATCH] D40443: [Modules TS] Make imports from an interface unit visible to its implementation units

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added a comment.

Ping


https://reviews.llvm.org/D40443



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

[PATCH] D40443: [Modules TS] Make imports from an interface unit visible to its implementation units

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 124231.
hamzasood added a comment.

Good idea, I've added that to the test.
I'll give Richard some time to look over this before committing.


https://reviews.llvm.org/D40443

Files:
  lib/Sema/SemaDecl.cpp
  test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
  test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
  test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/export-kw.cpp
  
test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp

Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
@@ -1,28 +0,0 @@
-// RUN: %clang_cc1 -fmodules-ts %s -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DINTERFACE -verify -emit-module-interface -o %t
-// RUN: %clang_cc1 -fmodules-ts %s -DIMPLEMENTATION -verify -fmodule-file=%t -o /dev/null
-//
-// RUN: %clang_cc1 -fmodules-ts %s -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DINTERFACE -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DIMPLEMENTATION -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-
-#if INTERFACE
-// expected-no-diagnostics
-export module A;
-#elif IMPLEMENTATION
-module A;
- #ifdef BUILT_AS_INTERFACE
-  // expected-error@-2 {{missing 'export' specifier in module declaration while building module interface}}
-  #define INTERFACE
- #endif
-#else
- #ifdef BUILT_AS_INTERFACE
-  // expected-error@1 {{missing 'export module' declaration in module interface unit}}
- #endif
-#endif
-
-#ifndef INTERFACE
-export int b; // expected-error {{export declaration can only be used within a module interface unit}}
-#else
-export int a;
-#endif
Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
@@ -0,0 +1,40 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: echo 'export module a; export class A { };' > %t/a.cppm
+// RUN: echo 'export module b; export class B { };' > %t/b.cppm
+//
+// RUN: %clang_cc1 -fmodules-ts -emit-module-interface %t/a.cppm -o %t/a.pcm
+// RUN: %clang_cc1 -fmodules-ts -emit-module-interface %t/b.cppm -o %t/b.pcm
+// RUN: %clang_cc1 -fmodules-ts -fprebuilt-module-path=%t -emit-module-interface %s -o %t/test.pcm -DTEST_INTERFACE
+//
+// RUN: %clang_cc1 -fmodules-ts -I%t -fmodule-file=%t/test.pcm %s -verify -DTEST_IMPLEMENTATION
+// RUN: %clang_cc1 -fmodules-ts -I%t -fmodule-file=%t/test.pcm %s -verify -DOTHER_TU
+
+
+#ifdef TEST_INTERFACE
+import a;
+export module test;
+import b;
+#endif
+
+#ifdef TEST_IMPLEMENTATION
+module test;
+
+A a; // expected-error {{must be imported from module 'a'}}
+ // expected-n...@a.cppm:1 {{here}}
+
+// Module b is imported within the purview of this module's interface unit.
+// So its exported definitions should be visible here.
+B b;
+#endif
+
+
+#ifdef OTHER_TU
+import test;
+
+A a; // expected-error {{must be imported from module 'a'}}
+ // expected-n...@a.cppm:1 {{here}}
+
+B b; // expected-error {{must be imported from module 'b'}}
+ // expected-n...@b.cppm:1 {{here}}
+#endif
Index: test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
===
--- test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
+++ test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
@@ -10,9 +10,7 @@
 //
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/M.pcm -emit-module-interface -o %t/N.pcm -DMODULE_INTERFACE -DNO_ERRORS
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -verify
-// FIXME: Once we start importing "import" declarations properly, this should
-// be rejected (-verify should be added to the following line).
-// RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -DNO_IMPORT
+// RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -DNO_IMPORT -verify
 //
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/M.pcm -emit-module-interface -o %t/N-no-M.pcm -DMODULE_INTERFACE -DNO_ERRORS -DNO_IMPORT
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N-no-M.pcm -verify
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -16308,6 +16308,15 @@
   TU->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModulePrivate);
   TU->setLocalOwningModule(Mod);
 
+  // Modules TS + p0731r0 [dcl.module.interface]p1:
+  

[PATCH] D40443: [Modules TS] Make imports from an interface unit visible to its implementation units

[Hamza Sood via Phabricator via cfe-commits]

hamzasood created this revision.

This provides an implementation for the changes outlined in section 4.1 of 
P0731r0, which clarifies the intended behaviour regarding implementation units 
being able to see imports made within their corresponding interface unit.


https://reviews.llvm.org/D40443

Files:
  lib/Sema/SemaDecl.cpp
  test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
  test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
  test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/export-kw.cpp
  
test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp

Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1.cpp
@@ -1,28 +0,0 @@
-// RUN: %clang_cc1 -fmodules-ts %s -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DINTERFACE -verify -emit-module-interface -o %t
-// RUN: %clang_cc1 -fmodules-ts %s -DIMPLEMENTATION -verify -fmodule-file=%t -o /dev/null
-//
-// RUN: %clang_cc1 -fmodules-ts %s -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DINTERFACE -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-// RUN: %clang_cc1 -fmodules-ts %s -DIMPLEMENTATION -DBUILT_AS_INTERFACE -emit-module-interface -verify -o /dev/null
-
-#if INTERFACE
-// expected-no-diagnostics
-export module A;
-#elif IMPLEMENTATION
-module A;
- #ifdef BUILT_AS_INTERFACE
-  // expected-error@-2 {{missing 'export' specifier in module declaration while building module interface}}
-  #define INTERFACE
- #endif
-#else
- #ifdef BUILT_AS_INTERFACE
-  // expected-error@1 {{missing 'export module' declaration in module interface unit}}
- #endif
-#endif
-
-#ifndef INTERFACE
-export int b; // expected-error {{export declaration can only be used within a module interface unit}}
-#else
-export int a;
-#endif
Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.interface/p1/interface-imports.cpp
@@ -0,0 +1,29 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: echo 'export module a; export class A { };' > %t/a.cppm
+// RUN: echo 'export module b; export class B { };' > %t/b.cppm
+//
+// RUN: %clang_cc1 -fmodules-ts -emit-module-interface %t/a.cppm -o %t/a.pcm
+// RUN: %clang_cc1 -fmodules-ts -emit-module-interface %t/b.cppm -o %t/b.pcm
+//
+// RUN: %clang_cc1 -fmodules-ts -fprebuilt-module-path=%t -emit-module-interface %s -o %t/test.pcm -DINTERFACE
+// RUN: %clang_cc1 -fmodules-ts -I%t -fmodule-file=%t/test.pcm %s -verify
+
+#ifdef INTERFACE
+
+import a;
+export module test;
+import b;
+
+#else // IMPLEMENTATION
+
+module test;
+
+A a; // expected-error {{must be imported from module 'a'}}
+ // expected-n...@a.cppm:1 {{here}}
+
+// Module b is imported within the purview of this module's interface unit.
+// So its exported definitions should be visible here.
+B b;
+
+#endif
Index: test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
===
--- test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
+++ test/CXX/modules-ts/basic/basic.def.odr/p6/module-vs-module.cpp
@@ -10,9 +10,7 @@
 //
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/M.pcm -emit-module-interface -o %t/N.pcm -DMODULE_INTERFACE -DNO_ERRORS
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -verify
-// FIXME: Once we start importing "import" declarations properly, this should
-// be rejected (-verify should be added to the following line).
-// RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -DNO_IMPORT
+// RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N.pcm -DNO_IMPORT -verify
 //
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/M.pcm -emit-module-interface -o %t/N-no-M.pcm -DMODULE_INTERFACE -DNO_ERRORS -DNO_IMPORT
 // RUN: %clang_cc1 -fmodules-ts -std=c++17 %s -fmodule-file=%t/N-no-M.pcm -verify
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -16308,6 +16308,15 @@
   TU->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModulePrivate);
   TU->setLocalOwningModule(Mod);
 
+  // Modules TS + p0731r0 [dcl.module.interface]p1:
+  //   Every name of an entity with linkage other than internal linkage made
+  //   visible in the purview of the module interface unit of a module M is
+  //   visible in the purview of all module implementation units of M.
+  if (MDK == ModuleDeclKind::Implementation) {
+for (Module 

[PATCH] D40270: [Modules TS] Added re-export support

[Hamza Sood via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL318744: [Modules TS] Added module re-export support. 
(authored by hamzasood).

Changed prior to commit:
  https://reviews.llvm.org/D40270?vs=123666=123736#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D40270

Files:
  cfe/trunk/include/clang/Parse/Parser.h
  cfe/trunk/lib/Parse/ParseObjc.cpp
  cfe/trunk/lib/Parse/Parser.cpp
  cfe/trunk/lib/Sema/SemaDecl.cpp
  cfe/trunk/test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.export/p1.cpp
  cfe/trunk/test/SemaCXX/modules-ts.cppm

Index: cfe/trunk/test/SemaCXX/modules-ts.cppm
===
--- cfe/trunk/test/SemaCXX/modules-ts.cppm
+++ cfe/trunk/test/SemaCXX/modules-ts.cppm
@@ -52,10 +52,6 @@
 export { ; }
 export { static_assert(true); }
 
-// FIXME: These diagnostics are not very good.
-export import foo; // expected-error {{expected unqualified-id}}
-export { import foo; } // expected-error {{expected unqualified-id}}
-
 int use_b = b;
 int use_n = n; // FIXME: this should not be visible, because it is not exported
 
Index: cfe/trunk/test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.export/p1.cpp
===
--- cfe/trunk/test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.export/p1.cpp
+++ cfe/trunk/test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.export/p1.cpp
@@ -0,0 +1,40 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+//
+// RUN: echo 'export module a; export class A{};' | %clang_cc1 -x c++ -fmodules-ts -emit-module-interface - -o %t/a.pcm
+// RUN: echo 'export module b; export class B{};' | %clang_cc1 -x c++ -fmodules-ts -emit-module-interface - -o %t/b.pcm
+// RUN: echo 'export module c; export class C{};' | %clang_cc1 -x c++ -fmodules-ts -emit-module-interface - -o %t/c.pcm
+//
+// RUN: %clang_cc1 -fmodules-ts -fprebuilt-module-path=%t -emit-module-interface %s -o %t/aggregate.internal.pcm -DAGGREGATE_INTERNAL
+// RUN: %clang_cc1 -fmodules-ts -fprebuilt-module-path=%t -emit-module-interface %s -o %t/aggregate.pcm -DAGGREGATE
+//
+// RUN: %clang_cc1 -fmodules-ts -fprebuilt-module-path=%t %s -verify -DTEST
+// expected-no-diagnostics
+
+
+#ifdef AGGREGATE_INTERNAL
+export module aggregate.internal;
+export import a;
+export {
+  import b;
+  import c;
+}
+#endif
+
+
+// Export the above aggregate module.
+// This is done to ensure that re-exports are transitive.
+#ifdef AGGREGATE
+export module aggregate;
+export import aggregate.internal;
+#endif
+
+
+// For the actual test, just try using the classes from the exported modules
+// and hope that they're accessible.
+#ifdef TEST
+import aggregate;
+A a;
+B b;
+C c;
+#endif
Index: cfe/trunk/lib/Sema/SemaDecl.cpp
===
--- cfe/trunk/lib/Sema/SemaDecl.cpp
+++ cfe/trunk/lib/Sema/SemaDecl.cpp
@@ -16171,7 +16171,7 @@
 DC = LSD->getParent();
   }
 
-  while (isa(DC))
+  while (isa(DC) || isa(DC))
 DC = DC->getParent();
 
   if (!isa(DC)) {
@@ -16349,12 +16349,17 @@
 IdentifierLocs.push_back(Path[I].second);
   }
 
-  TranslationUnitDecl *TU = getASTContext().getTranslationUnitDecl();
-  ImportDecl *Import = ImportDecl::Create(Context, TU, StartLoc,
+  ImportDecl *Import = ImportDecl::Create(Context, CurContext, StartLoc,
   Mod, IdentifierLocs);
   if (!ModuleScopes.empty())
 Context.addModuleInitializer(ModuleScopes.back().Module, Import);
-  TU->addDecl(Import);
+  CurContext->addDecl(Import);
+
+  // Re-export the module if needed.
+  if (Import->isExported() &&
+  !ModuleScopes.empty() && ModuleScopes.back().ModuleInterface)
+getCurrentModule()->Exports.emplace_back(Mod, false);
+
   return Import;
 }
 
Index: cfe/trunk/lib/Parse/ParseObjc.cpp
===
--- cfe/trunk/lib/Parse/ParseObjc.cpp
+++ cfe/trunk/lib/Parse/ParseObjc.cpp
@@ -81,8 +81,10 @@
 SingleDecl = ParseObjCPropertyDynamic(AtLoc);
 break;
   case tok::objc_import:
-if (getLangOpts().Modules || getLangOpts().DebuggerSupport)
-  return ParseModuleImport(AtLoc);
+if (getLangOpts().Modules || getLangOpts().DebuggerSupport) {
+  SingleDecl = ParseModuleImport(AtLoc);
+  break;
+}
 Diag(AtLoc, diag::err_atimport);
 SkipUntil(tok::semi);
 return Actions.ConvertDeclToDeclGroup(nullptr);
Index: cfe/trunk/lib/Parse/Parser.cpp
===
--- cfe/trunk/lib/Parse/Parser.cpp
+++ cfe/trunk/lib/Parse/Parser.cpp
@@ -556,10 +556,6 @@
 HandlePragmaUnused();
 return false;
 
-  case tok::kw_import:
-Result = ParseModuleImport(SourceLocation());
-return false;
-
   case tok::kw_export:
 if (NextToken().isNot(tok::kw_module))
   break;
@@ -637,6 +633,9 @@
 ///   ';'
 ///
 /// [C++0x/GNU] 'extern' 'template' declaration
+///
+/// [Modules-TS] 

[PATCH] D40270: [Modules TS] Added re-export support

[Hamza Sood via Phabricator via cfe-commits]

hamzasood created this revision.

This patch adds support for re-exporting modules as described in 
`[dcl.modules.export]`


https://reviews.llvm.org/D40270

Files:
  include/clang/Parse/Parser.h
  lib/Parse/ParseObjc.cpp
  lib/Parse/Parser.cpp
  lib/Sema/SemaDecl.cpp
  test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.export/p1.cpp
  test/SemaCXX/modules-ts.cppm

Index: test/SemaCXX/modules-ts.cppm
===
--- test/SemaCXX/modules-ts.cppm
+++ test/SemaCXX/modules-ts.cppm
@@ -52,10 +52,6 @@
 export { ; }
 export { static_assert(true); }
 
-// FIXME: These diagnostics are not very good.
-export import foo; // expected-error {{expected unqualified-id}}
-export { import foo; } // expected-error {{expected unqualified-id}}
-
 int use_b = b;
 int use_n = n; // FIXME: this should not be visible, because it is not exported
 
Index: test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.export/p1.cpp
===
--- test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.export/p1.cpp
+++ test/CXX/modules-ts/dcl.dcl/dcl.module/dcl.module.export/p1.cpp
@@ -0,0 +1,40 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+//
+// RUN: echo 'export module a; export class A{};' | %clang_cc1 -x c++ -fmodules-ts -emit-module-interface - -o %t/a.pcm
+// RUN: echo 'export module b; export class B{};' | %clang_cc1 -x c++ -fmodules-ts -emit-module-interface - -o %t/b.pcm
+// RUN: echo 'export module c; export class C{};' | %clang_cc1 -x c++ -fmodules-ts -emit-module-interface - -o %t/c.pcm
+//
+// RUN: %clang_cc1 -fmodules-ts -fprebuilt-module-path=%t -emit-module-interface %s -o %t/aggregate.internal.pcm -DAGGREGATE_INTERNAL
+// RUN: %clang_cc1 -fmodules-ts -fprebuilt-module-path=%t -emit-module-interface %s -o %t/aggregate.pcm -DAGGREGATE
+//
+// RUN: %clang_cc1 -fmodules-ts -fprebuilt-module-path=%t %s -verify -DTEST
+// expected-no-diagnostics
+
+
+#ifdef AGGREGATE_INTERNAL
+export module aggregate.internal;
+export import a;
+export {
+  import b;
+  import c;
+}
+#endif
+
+
+// Export the above aggregate module.
+// This is done to ensure that re-exports are transitive.
+#ifdef AGGREGATE
+export module aggregate;
+export import aggregate.internal;
+#endif
+
+
+// For the actual test, just try using the classes from the exported modules
+// and hope that they're accessible.
+#ifdef TEST
+import aggregate;
+A a;
+B b;
+C c;
+#endif
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -16171,7 +16171,7 @@
 DC = LSD->getParent();
   }
 
-  while (isa(DC))
+  while (isa(DC) || isa(DC))
 DC = DC->getParent();
 
   if (!isa(DC)) {
@@ -16349,12 +16349,17 @@
 IdentifierLocs.push_back(Path[I].second);
   }
 
-  TranslationUnitDecl *TU = getASTContext().getTranslationUnitDecl();
-  ImportDecl *Import = ImportDecl::Create(Context, TU, StartLoc,
+  ImportDecl *Import = ImportDecl::Create(Context, CurContext, StartLoc,
   Mod, IdentifierLocs);
   if (!ModuleScopes.empty())
 Context.addModuleInitializer(ModuleScopes.back().Module, Import);
-  TU->addDecl(Import);
+  CurContext->addDecl(Import);
+
+  // Re-export the module if needed.
+  if (Import->isExported() &&
+  !ModuleScopes.empty() && ModuleScopes.back().ModuleInterface)
+getCurrentModule()->Exports.emplace_back(Mod, false);
+
   return Import;
 }
 
Index: lib/Parse/Parser.cpp
===
--- lib/Parse/Parser.cpp
+++ lib/Parse/Parser.cpp
@@ -556,10 +556,6 @@
 HandlePragmaUnused();
 return false;
 
-  case tok::kw_import:
-Result = ParseModuleImport(SourceLocation());
-return false;
-
   case tok::kw_export:
 if (NextToken().isNot(tok::kw_module))
   break;
@@ -637,6 +633,9 @@
 ///   ';'
 ///
 /// [C++0x/GNU] 'extern' 'template' declaration
+///
+/// [Modules-TS] module-import-declaration
+///
 Parser::DeclGroupPtrTy
 Parser::ParseExternalDeclaration(ParsedAttributesWithRange ,
  ParsingDeclSpec *DS) {
@@ -764,6 +763,9 @@
 CurParsedObjCImpl ? Sema::PCC_ObjCImplementation : Sema::PCC_Namespace);
 cutOffParsing();
 return nullptr;
+  case tok::kw_import:
+SingleDecl = ParseModuleImport(SourceLocation());
+break;
   case tok::kw_export:
 if (getLangOpts().ModulesTS) {
   SingleDecl = ParseExportDeclaration();
@@ -2092,7 +2094,7 @@
 ///   '@' 'import' module-name ';'
 /// [ModTS] module-import-declaration:
 ///   'import' module-name attribute-specifier-seq[opt] ';'
-Parser::DeclGroupPtrTy Parser::ParseModuleImport(SourceLocation AtLoc) {
+Decl *Parser::ParseModuleImport(SourceLocation AtLoc) {
   assert((AtLoc.isInvalid() ? Tok.is(tok::kw_import)
 : Tok.isObjCAtKeyword(tok::objc_import)) &&
  "Improper start to module import");

[PATCH] D36527: Implemented P0428R2 - Familiar template syntax for generic lambdas

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 123015.
hamzasood added a comment.

Resolved merge conflicts from the last month of changes 
(`unittests/AST/StmtPrinterTest.cpp` and `lib/AST/DeclCXX.cpp`)


https://reviews.llvm.org/D36527

Files:
  include/clang/AST/DeclCXX.h
  include/clang/AST/DeclTemplate.h
  include/clang/AST/ExprCXX.h
  include/clang/AST/RecursiveASTVisitor.h
  include/clang/Basic/DiagnosticParseKinds.td
  include/clang/Parse/Parser.h
  include/clang/Sema/ScopeInfo.h
  include/clang/Sema/Sema.h
  lib/AST/DeclCXX.cpp
  lib/AST/DeclPrinter.cpp
  lib/AST/ExprCXX.cpp
  lib/AST/ItaniumMangle.cpp
  lib/AST/StmtPrinter.cpp
  lib/AST/TypePrinter.cpp
  lib/Parse/ParseExprCXX.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaType.cpp
  test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  test/CodeGenCXX/mangle-lambda-explicit-template-params.cpp
  test/PCH/cxx11-lambdas.mm
  test/PCH/cxx1y-lambdas.mm
  test/PCH/cxx2a-template-lambdas.cpp
  test/Parser/cxx2a-template-lambdas.cpp
  test/SemaCXX/cxx2a-template-lambdas.cpp
  unittests/AST/StmtPrinterTest.cpp
  unittests/Tooling/RecursiveASTVisitorTest.cpp
  unittests/Tooling/TestVisitor.h
  www/cxx_status.html

Index: www/cxx_status.html
===
--- www/cxx_status.html
+++ www/cxx_status.html
@@ -823,7 +823,7 @@
 
   template-parameter-list for generic lambdas
   http://wg21.link/p0428r2;>P0428R2
-  No
+  SVN
 
 
   Initializer list constructors in class template argument deduction
Index: unittests/Tooling/TestVisitor.h
===
--- unittests/Tooling/TestVisitor.h
+++ unittests/Tooling/TestVisitor.h
@@ -44,6 +44,8 @@
 Lang_CXX98,
 Lang_CXX11,
 Lang_CXX14,
+Lang_CXX17,
+Lang_CXX2a,
 Lang_OBJC,
 Lang_OBJCXX11,
 Lang_CXX = Lang_CXX98
@@ -60,6 +62,8 @@
   case Lang_CXX98: Args.push_back("-std=c++98"); break;
   case Lang_CXX11: Args.push_back("-std=c++11"); break;
   case Lang_CXX14: Args.push_back("-std=c++14"); break;
+  case Lang_CXX17: Args.push_back("-std=c++17"); break;
+  case Lang_CXX2a: Args.push_back("-std=c++2a"); break;
   case Lang_OBJC:
 Args.push_back("-ObjC");
 Args.push_back("-fobjc-runtime=macosx-10.12.0");
Index: unittests/Tooling/RecursiveASTVisitorTest.cpp
===
--- unittests/Tooling/RecursiveASTVisitorTest.cpp
+++ unittests/Tooling/RecursiveASTVisitorTest.cpp
@@ -79,6 +79,41 @@
   LambdaDefaultCaptureVisitor::Lang_CXX11));
 }
 
+// Matches (optional) explicit template parameters.
+class LambdaTemplateParametersVisitor
+  : public ExpectedLocationVisitor {
+public:
+  bool shouldVisitImplicitCode() const { return false; }
+
+  bool VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
+EXPECT_FALSE(D->isImplicit());
+Match(D->getName(), D->getLocStart());
+return true;
+  }
+  bool VisitNonTypeTemplateParmDecl(NonTypeTemplateParmDecl *D) {
+EXPECT_FALSE(D->isImplicit());
+Match(D->getName(), D->getLocStart());
+return true;
+  }
+  bool VisitTemplateTemplateParmDecl(TemplateTemplateParmDecl *D) {
+EXPECT_FALSE(D->isImplicit());
+Match(D->getName(), D->getLocStart());
+return true;
+  }
+};
+
+TEST(RecursiveASTVisitor, VisitsLambdaExplicitTemplateParameters) {
+  LambdaTemplateParametersVisitor Visitor;
+  Visitor.ExpectMatch("T",  2, 15);
+  Visitor.ExpectMatch("I",  2, 24);
+  Visitor.ExpectMatch("TT", 2, 31);
+  EXPECT_TRUE(Visitor.runOver(
+  "void f() { \n"
+  "  auto l = [] class TT>(auto p) { }; \n"
+  "}",
+  LambdaTemplateParametersVisitor::Lang_CXX2a));
+}
+
 // Checks for lambda classes that are not marked as implicitly-generated.
 // (There should be none.)
 class ClassVisitor : public ExpectedLocationVisitor {
Index: unittests/AST/StmtPrinterTest.cpp
===
--- unittests/AST/StmtPrinterTest.cpp
+++ unittests/AST/StmtPrinterTest.cpp
@@ -106,78 +106,73 @@
   return ::testing::AssertionSuccess();
 }
 
-::testing::AssertionResult
-PrintedStmtCXX98Matches(StringRef Code, const StatementMatcher ,
-StringRef ExpectedPrinted) {
-  std::vector Args;
-  Args.push_back("-std=c++98");
-  Args.push_back("-Wno-unused-value");
-  return PrintedStmtMatches(Code, Args, NodeMatch, ExpectedPrinted);
-}
-
-::testing::AssertionResult PrintedStmtCXX98Matches(
-  StringRef Code,
-  StringRef ContainingFunction,
-  StringRef ExpectedPrinted) {
-  std::vector Args;
-  Args.push_back("-std=c++98");
-  Args.push_back("-Wno-unused-value");
-  return PrintedStmtMatches(Code,
-Args,
-

[PATCH] D38919: [libcxx] Implemented from Library Fundamentals V2

[Hamza Sood via Phabricator via cfe-commits]

hamzasood created this revision.

This patch implements make_array and to_array from the Library Fundamentals V2 
TS.


https://reviews.llvm.org/D38919

Files:
  include/experimental/array
  test/std/experimental/container/array/creation/make_array.fail.cpp
  test/std/experimental/container/array/creation/make_array.pass.cpp
  test/std/experimental/container/array/creation/to_array.pass.cpp

Index: test/std/experimental/container/array/creation/to_array.pass.cpp
===
--- test/std/experimental/container/array/creation/to_array.pass.cpp
+++ test/std/experimental/container/array/creation/to_array.pass.cpp
@@ -0,0 +1,46 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+//
+
+// UNSUPPORTED: c++98, c++03, c++11
+
+// 
+#include 
+#include 
+
+int main() {
+  using std::experimental::to_array;
+
+  {
+constexpr int raw[4] = {0, 1, 2, 3};
+constexpr auto a = to_array(raw);
+
+static_assert(std::is_same>::value,
+  "Incorrect type");
+static_assert(a[0] == 0 && a[1] == 1 && a[2] == 2 && a[3] == 3,
+  "Incorrect values");
+  }
+
+  {
+struct S { char c; };
+
+constexpr S raw[6] = { {'a'}, {'b'}, {'c'}, {'d'}, {'e'}, {'f'}, };
+constexpr auto a = to_array(raw);
+
+static_assert(std::is_same>::value,
+  "Incorrect type");
+static_assert(a[0].c == 'a' &&
+  a[1].c == 'b' &&
+  a[2].c == 'c' &&
+  a[3].c == 'd' &&
+  a[4].c == 'e' &&
+  a[5].c == 'f',
+  "Incorrect values");
+  }
+}
Index: test/std/experimental/container/array/creation/make_array.pass.cpp
===
--- test/std/experimental/container/array/creation/make_array.pass.cpp
+++ test/std/experimental/container/array/creation/make_array.pass.cpp
@@ -0,0 +1,61 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+//
+
+// UNSUPPORTED: c++98, c++03, c++11
+
+// 
+#include 
+#include 
+#include 
+#include 
+
+#define CHECK_ARRAY(a, Ty, ...)   \
+do {  \
+  static_assert(std::is_same::value, \
+#a"'s type is incorrect");\
+  if (a != Ty{{__VA_ARGS__}}) \
+assert(false && #a"'s values are incorrect"); \
+} while(0)
+
+#define STD_ARRAY(T, N) ::std::array
+
+int main() {
+  using std::experimental::make_array;
+
+  // This is the example given in the specification.
+  {
+int i = 1; int  = i;
+auto a1 = make_array(i, ri);
+auto a2 = make_array(i, ri, 42L);
+auto a3 = make_array(i, ri);
+auto a4 = make_array();
+
+CHECK_ARRAY(a1, STD_ARRAY(int,  2), 1,  1);
+CHECK_ARRAY(a2, STD_ARRAY(long, 3), 1L, 1L, 42L);
+CHECK_ARRAY(a3, STD_ARRAY(long, 2), 1L, 1L);
+CHECK_ARRAY(a4, STD_ARRAY(long, 0));
+  }
+
+  // Make sure that reference_wrappers can be used when an explicit element
+  // type is given.
+  {
+int i = 1;
+auto with_ref_wrapper = make_array(0, std::reference_wrapper(i));
+CHECK_ARRAY(with_ref_wrapper, STD_ARRAY(int, 2), 0, 1);
+  }
+
+  // Make sure that it works correctly with constexpr.
+  {
+constexpr auto a = make_array(0, 1, 2);
+static_assert(std::is_same>::value
+  && a[0] == 0 && a[1] == 1 && a[2] == 2,
+  "constexpr array was made incorrectly.");
+  }
+}
Index: test/std/experimental/container/array/creation/make_array.fail.cpp
===
--- test/std/experimental/container/array/creation/make_array.fail.cpp
+++ test/std/experimental/container/array/creation/make_array.fail.cpp
@@ -0,0 +1,27 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+//
+
+// UNSUPPORTED: c++98, c++03, c++11
+
+// 
+#include 
+#include 
+
+static int i = 

[PATCH] D37182: [libcxx] Special visibility macros for the experimental library

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added a comment.

I have access now, so I'm able to commit this myself.
However it's been a while since it was approved, so I'd be grateful if someone 
could take another look to make sure nothing has changed in the meantime 
(besides potentially needing to re-tag some new APIs).


https://reviews.llvm.org/D37182



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

[PATCH] D36527: Implemented P0428R2 - Familiar template syntax for generic lambdas

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 118261.
hamzasood added a comment.

- Updated lambda mangling to include explicit template parameters
- Allow explicit template parameter lists on lambdas pre-c++2a as an extension.
- Improved the somewhat fragile template depth handling.
- Reformatted some asserts.

Could you expand on your first point a bit more? Do you have an example that 
shows the issue?


https://reviews.llvm.org/D36527

Files:
  include/clang/AST/DeclCXX.h
  include/clang/AST/DeclTemplate.h
  include/clang/AST/ExprCXX.h
  include/clang/AST/RecursiveASTVisitor.h
  include/clang/Basic/DiagnosticParseKinds.td
  include/clang/Parse/Parser.h
  include/clang/Sema/ScopeInfo.h
  include/clang/Sema/Sema.h
  lib/AST/DeclCXX.cpp
  lib/AST/DeclPrinter.cpp
  lib/AST/ExprCXX.cpp
  lib/AST/ItaniumMangle.cpp
  lib/AST/StmtPrinter.cpp
  lib/AST/TypePrinter.cpp
  lib/Parse/ParseExprCXX.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaType.cpp
  test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  test/CodeGenCXX/mangle-lambda-explicit-template-params.cpp
  test/PCH/cxx11-lambdas.mm
  test/PCH/cxx1y-lambdas.mm
  test/PCH/cxx2a-template-lambdas.cpp
  test/Parser/cxx2a-template-lambdas.cpp
  test/SemaCXX/cxx2a-template-lambdas.cpp
  unittests/AST/StmtPrinterTest.cpp
  unittests/Tooling/RecursiveASTVisitorTest.cpp
  unittests/Tooling/TestVisitor.h
  www/cxx_status.html

Index: www/cxx_status.html
===
--- www/cxx_status.html
+++ www/cxx_status.html
@@ -823,7 +823,7 @@
 
   template-parameter-list for generic lambdas
   http://wg21.link/p0428r2;>P0428R2
-  No
+  SVN
 
 
   Initializer list constructors in class template argument deduction
Index: unittests/Tooling/TestVisitor.h
===
--- unittests/Tooling/TestVisitor.h
+++ unittests/Tooling/TestVisitor.h
@@ -44,6 +44,8 @@
 Lang_CXX98,
 Lang_CXX11,
 Lang_CXX14,
+Lang_CXX17,
+Lang_CXX2a,
 Lang_OBJC,
 Lang_OBJCXX11,
 Lang_CXX = Lang_CXX98
@@ -60,6 +62,8 @@
   case Lang_CXX98: Args.push_back("-std=c++98"); break;
   case Lang_CXX11: Args.push_back("-std=c++11"); break;
   case Lang_CXX14: Args.push_back("-std=c++14"); break;
+  case Lang_CXX17: Args.push_back("-std=c++17"); break;
+  case Lang_CXX2a: Args.push_back("-std=c++2a"); break;
   case Lang_OBJC:
 Args.push_back("-ObjC");
 Args.push_back("-fobjc-runtime=macosx-10.12.0");
Index: unittests/Tooling/RecursiveASTVisitorTest.cpp
===
--- unittests/Tooling/RecursiveASTVisitorTest.cpp
+++ unittests/Tooling/RecursiveASTVisitorTest.cpp
@@ -79,6 +79,41 @@
   LambdaDefaultCaptureVisitor::Lang_CXX11));
 }
 
+// Matches (optional) explicit template parameters.
+class LambdaTemplateParametersVisitor
+  : public ExpectedLocationVisitor {
+public:
+  bool shouldVisitImplicitCode() const { return false; }
+
+  bool VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
+EXPECT_FALSE(D->isImplicit());
+Match(D->getName(), D->getLocStart());
+return true;
+  }
+  bool VisitNonTypeTemplateParmDecl(NonTypeTemplateParmDecl *D) {
+EXPECT_FALSE(D->isImplicit());
+Match(D->getName(), D->getLocStart());
+return true;
+  }
+  bool VisitTemplateTemplateParmDecl(TemplateTemplateParmDecl *D) {
+EXPECT_FALSE(D->isImplicit());
+Match(D->getName(), D->getLocStart());
+return true;
+  }
+};
+
+TEST(RecursiveASTVisitor, VisitsLambdaExplicitTemplateParameters) {
+  LambdaTemplateParametersVisitor Visitor;
+  Visitor.ExpectMatch("T",  2, 15);
+  Visitor.ExpectMatch("I",  2, 24);
+  Visitor.ExpectMatch("TT", 2, 31);
+  EXPECT_TRUE(Visitor.runOver(
+  "void f() { \n"
+  "  auto l = [] class TT>(auto p) { }; \n"
+  "}",
+  LambdaTemplateParametersVisitor::Lang_CXX2a));
+}
+
 // Checks for lambda classes that are not marked as implicitly-generated.
 // (There should be none.)
 class ClassVisitor : public ExpectedLocationVisitor {
Index: unittests/AST/StmtPrinterTest.cpp
===
--- unittests/AST/StmtPrinterTest.cpp
+++ unittests/AST/StmtPrinterTest.cpp
@@ -67,9 +67,8 @@
 
 template 
 ::testing::AssertionResult
-PrintedStmtMatches(StringRef Code, const std::vector ,
-   const T , StringRef ExpectedPrinted) {
-
+PrintedStmtMatchesInternal(StringRef Code, const std::vector ,
+   const T , StringRef ExpectedPrinted) {
   PrintMatch Printer;
   MatchFinder Finder;
   Finder.addMatcher(NodeMatch, );
@@ -97,65 +96,52 @@
   return ::testing::AssertionSuccess();
 }
 
+enum class StdVer { CXX98, CXX11, CXX14, CXX17, CXX2a };
+
+DeclarationMatcher FunctionBodyMatcher(StringRef ContainingFunction) {
+  return functionDecl(hasName(ContainingFunction),
+  

[PATCH] D37376: [libcxx] Fix libc++experimental build on Windows

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 114784.
hamzasood added a comment.

Changed a CMake macro to a function.


https://reviews.llvm.org/D37376

Files:
  CMakeLists.txt
  lib/CMakeLists.txt

Index: lib/CMakeLists.txt
===
--- lib/CMakeLists.txt
+++ lib/CMakeLists.txt
@@ -186,31 +186,37 @@
 split_list(LIBCXX_COMPILE_FLAGS)
 split_list(LIBCXX_LINK_FLAGS)
 
+function(add_msvcrt_defs_if_needed target)
+  if(WIN32 AND NOT MINGW)
+target_compile_definitions(${target}
+   PRIVATE
+ # Ignore the -MSC_VER mismatch, as we may build
+ # with a different compatibility version.
+ _ALLOW_MSC_VER_MISMATCH
+ # Don't check the msvcprt iterator debug levels
+ # as we will define the iterator types; libc++
+ # uses a different macro to identify the debug
+ # level.
+ _ALLOW_ITERATOR_DEBUG_LEVEL_MISMATCH
+ # We are building the c++ runtime, don't pull in
+ # msvcprt.
+ _CRTBLD
+ # Don't warn on the use of "deprecated"
+ # "insecure" functions which are standards
+ # specified.
+ _CRT_SECURE_NO_WARNINGS
+ # Use the ISO conforming behaviour for conversion
+ # in printf, scanf.
+ _CRT_STDIO_ISO_WIDE_SPECIFIERS)
+  endif()
+endfunction()
+
 # Add an object library that contains the compiled source files.
 add_library(cxx_objects OBJECT ${exclude_from_all} ${LIBCXX_SOURCES} ${LIBCXX_HEADERS})
-if(WIN32 AND NOT MINGW)
-  target_compile_definitions(cxx_objects
- PRIVATE
-   # Ignore the -MSC_VER mismatch, as we may build
-   # with a different compatibility version.
-   _ALLOW_MSC_VER_MISMATCH
-   # Don't check the msvcprt iterator debug levels
-   # as we will define the iterator types; libc++
-   # uses a different macro to identify the debug
-   # level.
-   _ALLOW_ITERATOR_DEBUG_LEVEL_MISMATCH
-   # We are building the c++ runtime, don't pull in
-   # msvcprt.
-   _CRTBLD
-   # Don't warn on the use of "deprecated"
-   # "insecure" functions which are standards
-   # specified.
-   _CRT_SECURE_NO_WARNINGS
-   # Use the ISO conforming behaviour for conversion
-   # in printf, scanf.
-   _CRT_STDIO_ISO_WIDE_SPECIFIERS)
-endif()
-
+target_compile_definitions(cxx_objects
+   PRIVATE
+ _LIBCPP_BUILDING_LIBRARY)
+add_msvcrt_defs_if_needed(cxx_objects)
 set_target_properties(cxx_objects
   PROPERTIES
 COMPILE_FLAGS "${LIBCXX_COMPILE_FLAGS}"
@@ -285,7 +291,13 @@
   if (LIBCXX_ENABLE_FILESYSTEM)
 file(GLOB LIBCXX_FILESYSTEM_SOURCES ../src/experimental/filesystem/*.cpp)
   endif()
+
   add_library(cxx_experimental STATIC ${LIBCXX_EXPERIMENTAL_SOURCES} ${LIBCXX_FILESYSTEM_SOURCES})
+  target_compile_definitions(cxx_experimental
+ PRIVATE
+   _LIBCPPX_BUILDING_LIBRARY)
+  add_msvcrt_defs_if_needed(cxx_experimental)
+
   if (LIBCXX_ENABLE_SHARED)
 target_link_libraries(cxx_experimental cxx_shared)
   else()
@@ -313,6 +325,9 @@
 add_library(cxx_external_threads STATIC ${LIBCXX_EXTERNAL_THREADING_SUPPORT_SOURCES})
   endif()
 
+  target_compile_definitions(cxx_external_threads
+ PRIVATE
+   _LIBCPP_BUILDING_LIBRARY)
   set_target_properties(cxx_external_threads
 PROPERTIES
   LINK_FLAGS"${LIBCXX_LINK_FLAGS}"
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -465,10 +465,6 @@
   add_compile_flags_if_supported(-fcoroutines-ts)
 endif()
 
-# Let the library headers know they are currently being used to build the
-# library.
-add_definitions(-D_LIBCPP_BUILDING_LIBRARY)
-
 if (NOT LIBCXX_ENABLE_NEW_DELETE_DEFINITIONS)
   add_definitions(-D_LIBCPP_DISABLE_NEW_DELETE_DEFINITIONS)
 endif()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D36527: Implemented P0428R2 - Familiar template syntax for generic lambdas

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 114550.
hamzasood added a comment.

- Replaced an unneeded `STLExtras.h` include with ``.
- Assert that the lambda template scope has a parent before accessing it.
- Added an equals in a braced init as per the LLVM coding standards.

Thanks @Rakete


https://reviews.llvm.org/D36527

Files:
  include/clang/AST/DeclTemplate.h
  include/clang/AST/ExprCXX.h
  include/clang/AST/RecursiveASTVisitor.h
  include/clang/Basic/DiagnosticParseKinds.td
  include/clang/Sema/ScopeInfo.h
  include/clang/Sema/Sema.h
  lib/AST/DeclPrinter.cpp
  lib/AST/ExprCXX.cpp
  lib/AST/StmtPrinter.cpp
  lib/AST/TypePrinter.cpp
  lib/Parse/ParseExprCXX.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaType.cpp
  test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  test/PCH/cxx11-lambdas.mm
  test/PCH/cxx1y-lambdas.mm
  test/PCH/cxx2a-template-lambdas.cpp
  test/Parser/cxx2a-template-lambdas.cpp
  test/SemaCXX/cxx2a-template-lambdas.cpp
  unittests/AST/StmtPrinterTest.cpp
  unittests/Tooling/RecursiveASTVisitorTest.cpp
  unittests/Tooling/TestVisitor.h
  www/cxx_status.html

Index: www/cxx_status.html
===
--- www/cxx_status.html
+++ www/cxx_status.html
@@ -823,7 +823,7 @@
 
   template-parameter-list for generic lambdas
   http://wg21.link/p0428r2;>P0428R2
-  No
+  SVN
 
 
   Initializer list constructors in class template argument deduction
Index: unittests/Tooling/TestVisitor.h
===
--- unittests/Tooling/TestVisitor.h
+++ unittests/Tooling/TestVisitor.h
@@ -44,6 +44,8 @@
 Lang_CXX98,
 Lang_CXX11,
 Lang_CXX14,
+Lang_CXX17,
+Lang_CXX2a,
 Lang_OBJC,
 Lang_OBJCXX11,
 Lang_CXX = Lang_CXX98
@@ -60,6 +62,8 @@
   case Lang_CXX98: Args.push_back("-std=c++98"); break;
   case Lang_CXX11: Args.push_back("-std=c++11"); break;
   case Lang_CXX14: Args.push_back("-std=c++14"); break;
+  case Lang_CXX17: Args.push_back("-std=c++17"); break;
+  case Lang_CXX2a: Args.push_back("-std=c++2a"); break;
   case Lang_OBJC:
 Args.push_back("-ObjC");
 Args.push_back("-fobjc-runtime=macosx-10.12.0");
Index: unittests/Tooling/RecursiveASTVisitorTest.cpp
===
--- unittests/Tooling/RecursiveASTVisitorTest.cpp
+++ unittests/Tooling/RecursiveASTVisitorTest.cpp
@@ -79,6 +79,41 @@
   LambdaDefaultCaptureVisitor::Lang_CXX11));
 }
 
+// Matches (optional) explicit template parameters.
+class LambdaTemplateParametersVisitor
+  : public ExpectedLocationVisitor {
+public:
+  bool shouldVisitImplicitCode() const { return false; }
+
+  bool VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
+EXPECT_FALSE(D->isImplicit());
+Match(D->getName(), D->getLocStart());
+return true;
+  }
+  bool VisitNonTypeTemplateParmDecl(NonTypeTemplateParmDecl *D) {
+EXPECT_FALSE(D->isImplicit());
+Match(D->getName(), D->getLocStart());
+return true;
+  }
+  bool VisitTemplateTemplateParmDecl(TemplateTemplateParmDecl *D) {
+EXPECT_FALSE(D->isImplicit());
+Match(D->getName(), D->getLocStart());
+return true;
+  }
+};
+
+TEST(RecursiveASTVisitor, VisitsLambdaExplicitTemplateParameters) {
+  LambdaTemplateParametersVisitor Visitor;
+  Visitor.ExpectMatch("T",  2, 15);
+  Visitor.ExpectMatch("I",  2, 24);
+  Visitor.ExpectMatch("TT", 2, 31);
+  EXPECT_TRUE(Visitor.runOver(
+  "void f() { \n"
+  "  auto l = [] class TT>(auto p) { }; \n"
+  "}",
+  LambdaTemplateParametersVisitor::Lang_CXX2a));
+}
+
 // Checks for lambda classes that are not marked as implicitly-generated.
 // (There should be none.)
 class ClassVisitor : public ExpectedLocationVisitor {
Index: unittests/AST/StmtPrinterTest.cpp
===
--- unittests/AST/StmtPrinterTest.cpp
+++ unittests/AST/StmtPrinterTest.cpp
@@ -67,9 +67,8 @@
 
 template 
 ::testing::AssertionResult
-PrintedStmtMatches(StringRef Code, const std::vector ,
-   const T , StringRef ExpectedPrinted) {
-
+PrintedStmtMatchesInternal(StringRef Code, const std::vector ,
+   const T , StringRef ExpectedPrinted) {
   PrintMatch Printer;
   MatchFinder Finder;
   Finder.addMatcher(NodeMatch, );
@@ -97,65 +96,52 @@
   return ::testing::AssertionSuccess();
 }
 
+enum class StdVer { CXX98, CXX11, CXX14, CXX17, CXX2a };
+
+DeclarationMatcher FunctionBodyMatcher(StringRef ContainingFunction) {
+  return functionDecl(hasName(ContainingFunction),
+  has(compoundStmt(has(stmt().bind("id");
+}
+
+template 
 ::testing::AssertionResult
-PrintedStmtCXX98Matches(StringRef Code, const StatementMatcher ,
-StringRef ExpectedPrinted) {
-  std::vector Args;
-  Args.push_back("-std=c++98");
-  

[PATCH] D36527: Implemented P0428R2 - Familiar template syntax for generic lambdas

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added a comment.

Ping


https://reviews.llvm.org/D36527



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

[PATCH] D37182: [libcxx] Special visibility macros for the experimental library

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added a comment.

Thanks. Could someone commit this for me please?




Comment at: include/experimental/__config:57
 
+#if defined(_LIBCPP_OBJECT_FORMAT_COFF)
+# define _LIBCPPX_TYPE_VIS

compnerd wrote:
> hamzasood wrote:
> > smeenai wrote:
> > > I think it would make more sense to make these macros empty on all 
> > > platforms, not just Windows. It's true that they'll only cause link 
> > > errors on Windows (since you'll attempt to dllimport functions from a 
> > > static library), but on ELF and Mach-O, the visibility annotations would 
> > > cause these functions to be exported from whatever library 
> > > c++experimental gets linked into, which is probably not desirable either.
> > > 
> > > The exception (if you'll pardon the pun) is `_LIBCPPX_EXCEPTION_ABI`, 
> > > which will still need to expand to at least 
> > > `__type_visibility__((default))` for non-COFF in order for throwing and 
> > > catching those types to work correctly.
> > I might be mistaken, but I think the regular libc++ library still uses 
> > visibility annotations when it's being built as a static library on 
> > non-Windows platforms. So I figured it'd be best to match that behaviour.
> It's not about matching that behaviour, libc++ is built shared, this is built 
> static.  The static link marked default visibility would make the functions 
> be re-exported.  However, if I'm not mistaken, this already happens today, so 
> fixing that in a follow-up is fine as the status quo is maintained.
But isn't it possible to build libc++ as static instead of shared? 
(LIBCXX_ENABLE_STATIC=YES and LIBCXX_ENABLE_SHARED=NO).
I'm pretty sure that in that case, the visibility annotations are left active.


https://reviews.llvm.org/D37182



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

[PATCH] D37376: [libcxx] Fix libc++experimental build on Windows

[Hamza Sood via Phabricator via cfe-commits]

hamzasood created this revision.
Herald added a subscriber: mgorny.

This patch fixes a few problems with building libc++experimental on Windows.

Previously _LIBCPP_BUILDING_LIBRARY was defined for every source file, 
including files destined for the experimental library. As a result of this, 
non-experimental APIs were marked as dllexport when compiling the experimental 
library (which led to link errors whenever experimental code uses symbols from 
the regular libc++ library).
I've changed it so that _LIBCPP_BUILDING_LIBRARY is only defined for 
non-experimental source files, and _LIBCPPX_BUILDING_LIBRARY is defined for 
experimental source files (that macro isn't currently used for anything, it's 
mainly there for consistency).

I've also added the needed MSVCRT defines to the experimental code too, so that 
there won't be a bunch of warnings about using "deprecated" functions etc.


https://reviews.llvm.org/D37376

Files:
  CMakeLists.txt
  lib/CMakeLists.txt

Index: lib/CMakeLists.txt
===
--- lib/CMakeLists.txt
+++ lib/CMakeLists.txt
@@ -186,31 +186,37 @@
 split_list(LIBCXX_COMPILE_FLAGS)
 split_list(LIBCXX_LINK_FLAGS)
 
+macro(add_msvcrt_defs_if_needed target)
+  if(WIN32 AND NOT MINGW)
+target_compile_definitions(${target}
+   PRIVATE
+ # Ignore the -MSC_VER mismatch, as we may build
+ # with a different compatibility version.
+ _ALLOW_MSC_VER_MISMATCH
+ # Don't check the msvcprt iterator debug levels
+ # as we will define the iterator types; libc++
+ # uses a different macro to identify the debug
+ # level.
+ _ALLOW_ITERATOR_DEBUG_LEVEL_MISMATCH
+ # We are building the c++ runtime, don't pull in
+ # msvcprt.
+ _CRTBLD
+ # Don't warn on the use of "deprecated"
+ # "insecure" functions which are standards
+ # specified.
+ _CRT_SECURE_NO_WARNINGS
+ # Use the ISO conforming behaviour for conversion
+ # in printf, scanf.
+ _CRT_STDIO_ISO_WIDE_SPECIFIERS)
+  endif()
+endmacro()
+
 # Add an object library that contains the compiled source files.
 add_library(cxx_objects OBJECT ${exclude_from_all} ${LIBCXX_SOURCES} ${LIBCXX_HEADERS})
-if(WIN32 AND NOT MINGW)
-  target_compile_definitions(cxx_objects
- PRIVATE
-   # Ignore the -MSC_VER mismatch, as we may build
-   # with a different compatibility version.
-   _ALLOW_MSC_VER_MISMATCH
-   # Don't check the msvcprt iterator debug levels
-   # as we will define the iterator types; libc++
-   # uses a different macro to identify the debug
-   # level.
-   _ALLOW_ITERATOR_DEBUG_LEVEL_MISMATCH
-   # We are building the c++ runtime, don't pull in
-   # msvcprt.
-   _CRTBLD
-   # Don't warn on the use of "deprecated"
-   # "insecure" functions which are standards
-   # specified.
-   _CRT_SECURE_NO_WARNINGS
-   # Use the ISO conforming behaviour for conversion
-   # in printf, scanf.
-   _CRT_STDIO_ISO_WIDE_SPECIFIERS)
-endif()
-
+target_compile_definitions(cxx_objects
+   PRIVATE
+ _LIBCPP_BUILDING_LIBRARY)
+add_msvcrt_defs_if_needed(cxx_objects)
 set_target_properties(cxx_objects
   PROPERTIES
 COMPILE_FLAGS "${LIBCXX_COMPILE_FLAGS}"
@@ -285,7 +291,13 @@
   if (LIBCXX_ENABLE_FILESYSTEM)
 file(GLOB LIBCXX_FILESYSTEM_SOURCES ../src/experimental/filesystem/*.cpp)
   endif()
+
   add_library(cxx_experimental STATIC ${LIBCXX_EXPERIMENTAL_SOURCES} ${LIBCXX_FILESYSTEM_SOURCES})
+  target_compile_definitions(cxx_experimental
+ PRIVATE
+   _LIBCPPX_BUILDING_LIBRARY)
+  add_msvcrt_defs_if_needed(cxx_experimental)
+
   if (LIBCXX_ENABLE_SHARED)
 target_link_libraries(cxx_experimental cxx_shared)
   else()
@@ -313,6 +325,9 @@
 add_library(cxx_external_threads STATIC ${LIBCXX_EXTERNAL_THREADING_SUPPORT_SOURCES})
   endif()
 
+  

[PATCH] D37375: [libcxx] Test case for the experimental library visibility macros

[Hamza Sood via Phabricator via cfe-commits]

hamzasood created this revision.

This patch adds a test case to ensure that every library header uses the 
correct visibility macros.
libc++ headers should use _LIBCPP_* macros whereas libc++experimental headers 
should use _LIBCPPX_* macros.

This was originally part of https://reviews.llvm.org/D37182, but it's been 
taken out for separate review as requested.
It's dependent on the aforementioned patch; the test will fail until that patch 
has been applied.


https://reviews.llvm.org/D37375

Files:
  test/libcxx/selftest/experimental_visibility.sh.cpp
  utils/check_experimental_visibility.py
  utils/libcxx/test/config.py

Index: utils/libcxx/test/config.py
===
--- utils/libcxx/test/config.py
+++ utils/libcxx/test/config.py
@@ -1056,6 +1056,13 @@
 not_py = os.path.join(self.libcxx_src_root, 'utils', 'not.py')
 not_str = '%s %s ' % (pipes.quote(sys.executable), pipes.quote(not_py))
 sub.append(('not ', not_str))
+# Configure check_experimental_visibility program substitutions
+check_experimental_vis_py = os.path.join(self.libcxx_src_root,
+ 'utils',
+ 'check_experimental_visibility.py')
+check_experimental_vis_str = '%s %s' % (pipes.quote(sys.executable),
+pipes.quote(check_experimental_vis_py))
+sub.append(('check_experimental_visibility', check_experimental_vis_str))
 
 def can_use_deployment(self):
 # Check if the host is on an Apple platform using clang.
Index: utils/check_experimental_visibility.py
===
--- utils/check_experimental_visibility.py
+++ utils/check_experimental_visibility.py
@@ -0,0 +1,98 @@
+#!/usr/bin/env python
+#===--===##
+#
+# The LLVM Compiler Infrastructure
+#
+# This file is dual licensed under the MIT and the University of Illinois Open
+# Source Licenses. See LICENSE.TXT for details.
+#
+#===--===##
+
+# USAGE: check_experimental_visibility 
+# This script checks if each header in the given include dir use the correct
+# visibility macros. Experimental headers should use _LIBCPPX macros whereas
+# non-experimental headers should use the regular _LIBCPP macros.
+
+import itertools
+import os
+import sys
+
+visibility_macros = [
+"HIDDEN",
+"FUNC_VIS",
+"EXTERN_VIS",
+"OVERRIDABLE_FUNC_VIS",
+"INLINE_VISIBILITY",
+"ALWAYS_INLINE",
+"TYPE_VIS",
+"TEMPLATE_VIS",
+"ENUM_VIS",
+"EXTERN_TEMPLATE_TYPE_VIS",
+"CLASS_TEMPLATE_INSTANTIATION_VIS",
+"METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS",
+"EXTERN_TEMPLATE_INLINE_VISIBILITY",
+"EXCEPTION_ABI",
+]
+
+regular_visibility_macros  = ["_LIBCPP_{}".format(m)  for m in visibility_macros]
+experimental_visibility_macros = ["_LIBCPPX_{}".format(m) for m in visibility_macros]
+
+def main():
+if len(sys.argv) != 2:
+sys.stderr.write("Expected only one argument: the libcxx include path\n")
+return 1
+
+include_dir = sys.argv[1]
+if not os.path.isdir(include_dir):
+sys.stderr.write("Given include path isn't a directory\n")
+return 1
+
+# [(rel_file_path, line_number)]
+invalid_regular_files = []
+invalid_experimental_files = []
+
+files_to_check = itertools.chain.from_iterable((
+((dir, fname) for fname in filenames if fname != "__config")
+for (dir, _, filenames) in os.walk(include_dir)
+))
+for (dir, fname) in files_to_check:
+parent_path = os.path.relpath(dir, include_dir)
+
+header_path = os.path.join(parent_path, fname) if parent_path != '.' else fname
+experimental = os.path.split(header_path)[0] == "experimental"
+
+# List of macros that we shouldn't see in this file.
+problem_macros = regular_visibility_macros if experimental \
+ else experimental_visibility_macros
+
+with open(os.path.join(dir, fname), 'r') as f:
+problem_lines = (i for (i, line) in enumerate(f, 1)
+ if any(m in line for m in problem_macros))
+(invalid_experimental_files if experimental else invalid_regular_files) \
+.extend((header_path, l) for l in problem_lines)
+
+if not invalid_regular_files and not invalid_experimental_files:
+# No problems found
+return 0
+
+if invalid_regular_files:
+sys.stderr.write(
+"Found usage of experimental visibility macros in non-experimental files.\n"
+"These should be changed to the corresponding _LIBCPP macros.\n")
+for (path, line_num) in invalid_regular_files:
+sys.stderr.write("  

[PATCH] D36527: Implemented P0428R2 - Familiar template syntax for generic lambdas

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 113271.
hamzasood added a comment.
Herald added a subscriber: klimek.

Implemented pretty printing and recursive AST visiting (both with appropriate 
unit tests).

The pretty printing implementation includes fixing a few printing bugs (which I 
needed fixed to write decent tests);

- Unnamed template parameters now print without a trailing space (e.g.  
instead of ).
- C++14 generic lambda parameters are now printed as `auto` as opposed to 
`typeparameter-depth-index`.

RecursiveASTVisitor now visits each explicit template parameter, and also the 
implicit ones if shouldVisitImplicitCode is set.
However there is a problem with this implementation that I'm not sure about.
Consider a TU that is simply: `auto l = []() { };`
If shouldVisitImplicitCode is set, the template parameter will be visited 
twice, Once when visiting the lambda (which is what I added) and once when 
visiting the implicit lambda class type (existing behaviour). Is that a problem?

With regards to serialisation, I don't think any changes are needed. However 
I've added a PCH test to be sure (which passes).


https://reviews.llvm.org/D36527

Files:
  include/clang/AST/DeclTemplate.h
  include/clang/AST/ExprCXX.h
  include/clang/AST/RecursiveASTVisitor.h
  include/clang/Basic/DiagnosticParseKinds.td
  include/clang/Sema/ScopeInfo.h
  include/clang/Sema/Sema.h
  lib/AST/DeclPrinter.cpp
  lib/AST/ExprCXX.cpp
  lib/AST/StmtPrinter.cpp
  lib/AST/TypePrinter.cpp
  lib/Parse/ParseExprCXX.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaType.cpp
  test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  test/PCH/cxx11-lambdas.mm
  test/PCH/cxx1y-lambdas.mm
  test/PCH/cxx2a-template-lambdas.cpp
  test/Parser/cxx2a-template-lambdas.cpp
  test/SemaCXX/cxx2a-template-lambdas.cpp
  unittests/AST/StmtPrinterTest.cpp
  unittests/Tooling/RecursiveASTVisitorTest.cpp
  unittests/Tooling/TestVisitor.h
  www/cxx_status.html

Index: www/cxx_status.html
===
--- www/cxx_status.html
+++ www/cxx_status.html
@@ -822,7 +822,7 @@
 
   template-parameter-list for generic lambdas
   http://wg21.link/p0428r2;>P0428R2
-  No
+  SVN
 
 
   Initializer list constructors in class template argument deduction
Index: unittests/Tooling/TestVisitor.h
===
--- unittests/Tooling/TestVisitor.h
+++ unittests/Tooling/TestVisitor.h
@@ -44,6 +44,8 @@
 Lang_CXX98,
 Lang_CXX11,
 Lang_CXX14,
+Lang_CXX17,
+Lang_CXX2a,
 Lang_OBJC,
 Lang_OBJCXX11,
 Lang_CXX = Lang_CXX98
@@ -60,6 +62,8 @@
   case Lang_CXX98: Args.push_back("-std=c++98"); break;
   case Lang_CXX11: Args.push_back("-std=c++11"); break;
   case Lang_CXX14: Args.push_back("-std=c++14"); break;
+  case Lang_CXX17: Args.push_back("-std=c++17"); break;
+  case Lang_CXX2a: Args.push_back("-std=c++2a"); break;
   case Lang_OBJC:
 Args.push_back("-ObjC");
 Args.push_back("-fobjc-runtime=macosx-10.12.0");
Index: unittests/Tooling/RecursiveASTVisitorTest.cpp
===
--- unittests/Tooling/RecursiveASTVisitorTest.cpp
+++ unittests/Tooling/RecursiveASTVisitorTest.cpp
@@ -79,6 +79,41 @@
   LambdaDefaultCaptureVisitor::Lang_CXX11));
 }
 
+// Matches (optional) explicit template parameters.
+class LambdaTemplateParametersVisitor
+  : public ExpectedLocationVisitor {
+public:
+  bool shouldVisitImplicitCode() const { return false; }
+
+  bool VisitTemplateTypeParmDecl(TemplateTypeParmDecl *D) {
+EXPECT_FALSE(D->isImplicit());
+Match(D->getName(), D->getLocStart());
+return true;
+  }
+  bool VisitNonTypeTemplateParmDecl(NonTypeTemplateParmDecl *D) {
+EXPECT_FALSE(D->isImplicit());
+Match(D->getName(), D->getLocStart());
+return true;
+  }
+  bool VisitTemplateTemplateParmDecl(TemplateTemplateParmDecl *D) {
+EXPECT_FALSE(D->isImplicit());
+Match(D->getName(), D->getLocStart());
+return true;
+  }
+};
+
+TEST(RecursiveASTVisitor, VisitsLambdaExplicitTemplateParameters) {
+  LambdaTemplateParametersVisitor Visitor;
+  Visitor.ExpectMatch("T",  2, 15);
+  Visitor.ExpectMatch("I",  2, 24);
+  Visitor.ExpectMatch("TT", 2, 31);
+  EXPECT_TRUE(Visitor.runOver(
+  "void f() { \n"
+  "  auto l = [] class TT>(auto p) { }; \n"
+  "}",
+  LambdaTemplateParametersVisitor::Lang_CXX2a));
+}
+
 // Checks for lambda classes that are not marked as implicitly-generated.
 // (There should be none.)
 class ClassVisitor : public ExpectedLocationVisitor {
Index: unittests/AST/StmtPrinterTest.cpp
===
--- unittests/AST/StmtPrinterTest.cpp
+++ unittests/AST/StmtPrinterTest.cpp
@@ -67,9 +67,8 @@
 
 template 
 ::testing::AssertionResult
-PrintedStmtMatches(StringRef Code, const std::vector ,
-

[PATCH] D37182: [libcxx] Special visibility macros for the experimental library

[Hamza Sood via Phabricator via cfe-commits]

hamzasood added a comment.

In https://reviews.llvm.org/D37182#855262, @smeenai wrote:

> The other question, of course, is why the experimental library needs to be 
> static. If it were built shared, the annotations would just work on Windows 
> in theory (though I'm sure there are other issues there).


Even if libc++experimental is no longer forced to be static, it'd probably be 
too restrictive to force libc++ and libc++experimental to be the same library 
type. E.g. someone might want a dynamic libc++ but a static libc++experimental. 
If that's possible (even if not by default), different visibility macros will 
be needed.




Comment at: include/experimental/__config:57
 
+#if defined(_LIBCPP_OBJECT_FORMAT_COFF)
+# define _LIBCPPX_TYPE_VIS

smeenai wrote:
> I think it would make more sense to make these macros empty on all platforms, 
> not just Windows. It's true that they'll only cause link errors on Windows 
> (since you'll attempt to dllimport functions from a static library), but on 
> ELF and Mach-O, the visibility annotations would cause these functions to be 
> exported from whatever library c++experimental gets linked into, which is 
> probably not desirable either.
> 
> The exception (if you'll pardon the pun) is `_LIBCPPX_EXCEPTION_ABI`, which 
> will still need to expand to at least `__type_visibility__((default))` for 
> non-COFF in order for throwing and catching those types to work correctly.
I might be mistaken, but I think the regular libc++ library still uses 
visibility annotations when it's being built as a static library on non-Windows 
platforms. So I figured it'd be best to match that behaviour.


https://reviews.llvm.org/D37182



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

[PATCH] D36527: Implemented P0428R2 - Familiar template syntax for generic lambdas

[Hamza Sood via Phabricator via cfe-commits]

hamzasood updated this revision to Diff 112867.
hamzasood added a comment.

Hi Richard, thanks for your feedback. This update makes the changes that you 
requested.
It turns out that the casting is no longer an issue since 
`ParseTemplateParameters` has been refactored to use `NamedDecl`.


https://reviews.llvm.org/D36527

Files:
  include/clang/AST/ExprCXX.h
  include/clang/Basic/DiagnosticParseKinds.td
  include/clang/Sema/ScopeInfo.h
  include/clang/Sema/Sema.h
  lib/AST/ExprCXX.cpp
  lib/Parse/ParseExprCXX.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaType.cpp
  test/CXX/temp/temp.decls/temp.variadic/p4.cpp
  test/Parser/cxx2a-template-lambdas.cpp
  test/SemaCXX/cxx2a-template-lambdas.cpp
  www/cxx_status.html

Index: www/cxx_status.html
===
--- www/cxx_status.html
+++ www/cxx_status.html
@@ -822,7 +822,7 @@
 
   template-parameter-list for generic lambdas
   http://wg21.link/p0428r2;>P0428R2
-  No
+  SVN
 
 
   Initializer list constructors in class template argument deduction
Index: test/SemaCXX/cxx2a-template-lambdas.cpp
===
--- test/SemaCXX/cxx2a-template-lambdas.cpp
+++ test/SemaCXX/cxx2a-template-lambdas.cpp
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -std=c++2a -verify %s
+
+template
+constexpr bool is_same = false;
+
+template
+constexpr bool is_same = true;
+
+template
+struct DummyTemplate { };
+
+void func() {
+  auto L0 = [](T arg) {
+static_assert(is_same);
+  };
+  L0(0);
+
+  auto L1 = [] {
+static_assert(I == 5);
+  };
+  L1.operator()<5>();
+
+  auto L2 = []