[PATCH] D49518: [VFS] Emit an error when a file isn't located in any directory.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC339164: [VFS] Emit an error when entry at root level uses a 
relative path. (authored by vsapsai, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D49518?vs=157311&id=159567#toc

Repository:
  rC Clang

https://reviews.llvm.org/D49518

Files:
  lib/Basic/VirtualFileSystem.cpp
  unittests/Basic/VirtualFileSystemTest.cpp

Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -1468,3 +1468,35 @@
   }
   EXPECT_EQ(I, E);
 }
+
+TEST_F(VFSFromYAMLTest, RelativePaths) {
+  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  // Filename at root level without a parent directory.
+  IntrusiveRefCntPtr FS = getFromYAMLString(
+  "{ 'roots': [\n"
+  "  { 'type': 'file', 'name': 'file-not-in-directory.h',\n"
+  "'external-contents': '//root/external/file'\n"
+  "  }\n"
+  "] }", Lower);
+  EXPECT_EQ(nullptr, FS.get());
+
+  // Relative file path.
+  FS = getFromYAMLString(
+  "{ 'roots': [\n"
+  "  { 'type': 'file', 'name': 'relative/file/path.h',\n"
+  "'external-contents': '//root/external/file'\n"
+  "  }\n"
+  "] }", Lower);
+  EXPECT_EQ(nullptr, FS.get());
+
+  // Relative directory path.
+  FS = getFromYAMLString(
+   "{ 'roots': [\n"
+   "  { 'type': 'directory', 'name': 'relative/directory/path.h',\n"
+   "'contents': []\n"
+   "  }\n"
+   "] }", Lower);
+  EXPECT_EQ(nullptr, FS.get());
+
+  EXPECT_EQ(3, NumDiagnostics);
+}
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -1281,7 +1281,8 @@
 }
   }
 
-  std::unique_ptr parseEntry(yaml::Node *N, RedirectingFileSystem *FS) {
+  std::unique_ptr parseEntry(yaml::Node *N, RedirectingFileSystem *FS,
+bool IsRootEntry) {
 auto *M = dyn_cast(N);
 if (!M) {
   error(N, "expected mapping node for file or directory entry");
@@ -1302,6 +1303,7 @@
 std::vector> EntryArrayContents;
 std::string ExternalContentsPath;
 std::string Name;
+yaml::Node *NameValueNode;
 auto UseExternalName = RedirectingFileEntry::NK_NotSet;
 EntryKind Kind;
 
@@ -1321,6 +1323,7 @@
 if (!parseScalarString(I.getValue(), Value, Buffer))
   return nullptr;
 
+NameValueNode = I.getValue();
 if (FS->UseCanonicalizedPaths) {
   SmallString<256> Path(Value);
   // Guarantee that old YAML files containing paths with ".." and "."
@@ -1357,7 +1360,8 @@
 }
 
 for (auto &I : *Contents) {
-  if (std::unique_ptr E = parseEntry(&I, FS))
+  if (std::unique_ptr E =
+  parseEntry(&I, FS, /*IsRootEntry*/ false))
 EntryArrayContents.push_back(std::move(E));
   else
 return nullptr;
@@ -1418,6 +1422,13 @@
   return nullptr;
 }
 
+if (IsRootEntry && !sys::path::is_absolute(Name)) {
+  assert(NameValueNode && "Name presence should be checked earlier");
+  error(NameValueNode,
+"entry with relative path at the root level is not discoverable");
+  return nullptr;
+}
+
 // Remove trailing slash(es), being careful not to remove the root path
 StringRef Trimmed(Name);
 size_t RootPathLen = sys::path::root_path(Trimmed).size();
@@ -1500,7 +1511,8 @@
 }
 
 for (auto &I : *Roots) {
-  if (std::unique_ptr E = parseEntry(&I, FS))
+  if (std::unique_ptr E =
+  parseEntry(&I, FS, /*IsRootEntry*/ true))
 RootEntries.push_back(std::move(E));
   else
 return false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49518: [VFS] Emit an error when a file isn't located in any directory.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Thanks for the review, Bruno.


Repository:
  rC Clang

https://reviews.llvm.org/D49518



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

[PATCH] D50344: [libc++] Enable aligned allocation based on feature test macro, irrespective of standard

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: libcxx/include/__config:993
+!defined(_LIBCPP_BUILDING_LIBRARY) && \
+!defined(__cpp_aligned_new)
+#  define _LIBCPP_HAS_NO_ALIGNED_ALLOCATION

I think I'd rather keep `__cpp_aligned_new >= 201606` check. I don't know about 
cases where the smaller value is possible, Clang always had 201606 as far as I 
(and SVN) know. But it seems to be safer and more correct.

Don't have objective arguments for keeping the check, so if you think it's not 
worth it, I trust your judgement.



Comment at: libcxx/include/new:111-116
 #if !__has_builtin(__builtin_operator_new) || \
__has_builtin(__builtin_operator_new) < 201802L || \
defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) || \
!defined(__cpp_aligned_new) || __cpp_aligned_new < 201606
 #define _LIBCPP_HAS_NO_BUILTIN_ALIGNED_OPERATOR_NEW_DELETE
 #endif

Maybe move this to `__config` too? This way we'll have 
`__cpp_aligned_new`-related macros together.



Comment at: libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp:11
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11

Initially `with_system_cxx_lib` made me suspicious because macro 
`_LIBCPP_HAS_NO_ALIGNED_ALLOCATION` doesn't need specific dylib. And `XFAIL: 
availability=macosx10.12` seems to be working.

[Documentation](https://github.com/llvm-mirror/libcxx/blob/master/docs/DesignDocs/AvailabilityMarkup.rst#testing)
 describes which feature should be used in different cases but in this case I 
cannot definitely say if test uses unavailable feature. I think it is 
acceptable to stick with `with_system_cxx_lib` but I decided to brought to your 
attention the alternative.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50344



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

[PATCH] D50341: [libcxx] Mark aligned allocation tests as XFAIL on old OSX versions

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: 
libcxx/test/std/language.support/support.dynamic/new.delete/new.delete.array/new_size_align.fail.cpp:15
 
 // UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
 // UNSUPPORTED: clang-3.3, clang-3.4, clang-3.5, clang-3.6, clang-3.7, 
clang-3.8

In what cases are we supposed to run these tests? Such extensive collection of 
unsupported C++ standards looks suspicious and I guess that is the reason why I 
haven't seen test failures with older libc++ dylibs.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50341



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

[PATCH] D50118: [VFS] Unify iteration code for VFSFromYamlDirIterImpl, NFC intended.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC339199: [VFS] Unify iteration code for 
VFSFromYamlDirIterImpl, NFC intended. (authored by vsapsai, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D50118?vs=158420&id=159614#toc

Repository:
  rC Clang

https://reviews.llvm.org/D50118

Files:
  lib/Basic/VirtualFileSystem.cpp


Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -933,6 +933,8 @@
   RedirectingFileSystem &FS;
   RedirectingDirectoryEntry::iterator Current, End;
 
+  std::error_code incrementImpl();
+
 public:
   VFSFromYamlDirIterImpl(const Twine &Path, RedirectingFileSystem &FS,
  RedirectingDirectoryEntry::iterator Begin,
@@ -1997,36 +1999,25 @@
 RedirectingDirectoryEntry::iterator Begin,
 RedirectingDirectoryEntry::iterator End, std::error_code &EC)
 : Dir(_Path.str()), FS(FS), Current(Begin), End(End) {
-  while (Current != End) {
-SmallString<128> PathStr(Dir);
-llvm::sys::path::append(PathStr, (*Current)->getName());
-llvm::ErrorOr S = FS.status(PathStr);
-if (S) {
-  CurrentEntry = *S;
-  return;
-}
-// Skip entries which do not map to a reliable external content.
-if (FS.ignoreNonExistentContents() &&
-S.getError() == llvm::errc::no_such_file_or_directory) {
-  ++Current;
-  continue;
-} else {
-  EC = S.getError();
-  break;
-}
-  }
+  EC = incrementImpl();
 }
 
 std::error_code VFSFromYamlDirIterImpl::increment() {
   assert(Current != End && "cannot iterate past end");
-  while (++Current != End) {
+  ++Current;
+  return incrementImpl();
+}
+
+std::error_code VFSFromYamlDirIterImpl::incrementImpl() {
+  while (Current != End) {
 SmallString<128> PathStr(Dir);
 llvm::sys::path::append(PathStr, (*Current)->getName());
 llvm::ErrorOr S = FS.status(PathStr);
 if (!S) {
   // Skip entries which do not map to a reliable external content.
   if (FS.ignoreNonExistentContents() &&
   S.getError() == llvm::errc::no_such_file_or_directory) {
+++Current;
 continue;
   } else {
 return S.getError();


Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -933,6 +933,8 @@
   RedirectingFileSystem &FS;
   RedirectingDirectoryEntry::iterator Current, End;
 
+  std::error_code incrementImpl();
+
 public:
   VFSFromYamlDirIterImpl(const Twine &Path, RedirectingFileSystem &FS,
  RedirectingDirectoryEntry::iterator Begin,
@@ -1997,36 +1999,25 @@
 RedirectingDirectoryEntry::iterator Begin,
 RedirectingDirectoryEntry::iterator End, std::error_code &EC)
 : Dir(_Path.str()), FS(FS), Current(Begin), End(End) {
-  while (Current != End) {
-SmallString<128> PathStr(Dir);
-llvm::sys::path::append(PathStr, (*Current)->getName());
-llvm::ErrorOr S = FS.status(PathStr);
-if (S) {
-  CurrentEntry = *S;
-  return;
-}
-// Skip entries which do not map to a reliable external content.
-if (FS.ignoreNonExistentContents() &&
-S.getError() == llvm::errc::no_such_file_or_directory) {
-  ++Current;
-  continue;
-} else {
-  EC = S.getError();
-  break;
-}
-  }
+  EC = incrementImpl();
 }
 
 std::error_code VFSFromYamlDirIterImpl::increment() {
   assert(Current != End && "cannot iterate past end");
-  while (++Current != End) {
+  ++Current;
+  return incrementImpl();
+}
+
+std::error_code VFSFromYamlDirIterImpl::incrementImpl() {
+  while (Current != End) {
 SmallString<128> PathStr(Dir);
 llvm::sys::path::append(PathStr, (*Current)->getName());
 llvm::ErrorOr S = FS.status(PathStr);
 if (!S) {
   // Skip entries which do not map to a reliable external content.
   if (FS.ignoreNonExistentContents() &&
   S.getError() == llvm::errc::no_such_file_or_directory) {
+++Current;
 continue;
   } else {
 return S.getError();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50344: [libc++] Enable aligned allocation based on feature test macro, irrespective of standard

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai accepted this revision.
vsapsai added inline comments.
This revision is now accepted and ready to land.



Comment at: libcxx/include/new:111-116
 #if !__has_builtin(__builtin_operator_new) || \
__has_builtin(__builtin_operator_new) < 201802L || \
defined(_LIBCPP_HAS_NO_ALIGNED_ALLOCATION) || \
!defined(__cpp_aligned_new) || __cpp_aligned_new < 201606
 #define _LIBCPP_HAS_NO_BUILTIN_ALIGNED_OPERATOR_NEW_DELETE
 #endif

ldionne wrote:
> vsapsai wrote:
> > Maybe move this to `__config` too? This way we'll have 
> > `__cpp_aligned_new`-related macros together.
> The big difference is that 
> `_LIBCPP_HAS_NO_BUILTIN_ALIGNED_OPERATOR_NEW_DELETE` is only used in ``, 
> whereas `_LIBCPP_HAS_NO_ALIGNED_ALLOCATION` was used in other files as well. 
> Hence, I feel like it makes more sense to lift 
> `_LIBCPP_HAS_NO_ALIGNED_ALLOCATION` into `<__config>`, but not 
> `_LIBCPP_HAS_NO_BUILTIN_ALIGNED_OPERATOR_NEW_DELETE `.
Agree with your reasoning.



Comment at: libcxx/test/libcxx/memory/aligned_allocation_macro.pass.cpp:11
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11

ldionne wrote:
> vsapsai wrote:
> > Initially `with_system_cxx_lib` made me suspicious because macro 
> > `_LIBCPP_HAS_NO_ALIGNED_ALLOCATION` doesn't need specific dylib. And 
> > `XFAIL: availability=macosx10.12` seems to be working.
> > 
> > [Documentation](https://github.com/llvm-mirror/libcxx/blob/master/docs/DesignDocs/AvailabilityMarkup.rst#testing)
> >  describes which feature should be used in different cases but in this case 
> > I cannot definitely say if test uses unavailable feature. I think it is 
> > acceptable to stick with `with_system_cxx_lib` but I decided to brought to 
> > your attention the alternative.
> My understanding is that the `availability` feature may not be there if we're 
> not using availability macros, since they can be disabled entirely.
`availability` feature [will be 
present](https://github.com/llvm-mirror/libcxx/blob/5428c6b9d64dd4cc65fd5665c977c11dc6f8a547/utils/libcxx/test/config.py#L403-L426)
 when tests are invoked with `use_system_cxx_lib`. I suspect in this case both 
`with_system_cxx_lib` and `availability` will work, so let's keep 
`with_system_cxx_lib`.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50344



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

[PATCH] D50539: [VFS] Add property 'fallthrough' that controls fallback to real file system.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added reviewers: bruno, benlangmuir.
Herald added a subscriber: dexonsmith.

Default property value 'true' preserves current behavior. Value 'false' can be
used to create VFS "root", file system that gives better control over which
files compiler can use during compilation as there are no unpredictable
accesses to real file system.

Non-fallthrough use case changes how we treat multiple VFS overlay
files. Instead of all of them being at the same level just above a real
file system, now they are nested and subsequent overlays can refer to
files in previous overlays.

rdar://problem/39465552


https://reviews.llvm.org/D50539

Files:
  clang/lib/Basic/VirtualFileSystem.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/VFS/Inputs/Broken.framework/Headers/Error.h
  clang/test/VFS/Inputs/Broken.framework/Modules/module.modulemap
  clang/test/VFS/Inputs/Broken.framework/VFSHeaders/A.h
  clang/test/VFS/Inputs/vfsroot.yaml
  clang/test/VFS/vfsroot-include.c
  clang/test/VFS/vfsroot-module.m
  clang/test/VFS/vfsroot-with-overlay.c
  clang/unittests/Basic/VirtualFileSystemTest.cpp

Index: clang/unittests/Basic/VirtualFileSystemTest.cpp
===
--- clang/unittests/Basic/VirtualFileSystemTest.cpp
+++ clang/unittests/Basic/VirtualFileSystemTest.cpp
@@ -1500,3 +1500,89 @@
 
   EXPECT_EQ(3, NumDiagnostics);
 }
+
+TEST_F(VFSFromYAMLTest, NonFallthroughDirectoryIteration) {
+  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/");
+  Lower->addRegularFile("//root/a");
+  Lower->addRegularFile("//root/b");
+  IntrusiveRefCntPtr FS = getFromYAMLString(
+  "{ 'use-external-names': false,\n"
+  "  'fallthrough': false,\n"
+  "  'roots': [\n"
+  "{\n"
+  "  'type': 'directory',\n"
+  "  'name': '//root/',\n"
+  "  'contents': [ {\n"
+  "  'type': 'file',\n"
+  "  'name': 'c',\n"
+  "  'external-contents': '//root/a'\n"
+  "}\n"
+  "  ]\n"
+  "}\n"
+  "]\n"
+  "}",
+  Lower);
+  ASSERT_TRUE(FS.get() != nullptr);
+
+  std::error_code EC;
+  checkContents(FS->dir_begin("//root/", EC),
+{"//root/c"});
+}
+
+TEST_F(VFSFromYAMLTest, DirectoryIterationWithDuplicates) {
+  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/");
+  Lower->addRegularFile("//root/a");
+  Lower->addRegularFile("//root/b");
+  IntrusiveRefCntPtr FS = getFromYAMLString(
+  "{ 'use-external-names': false,\n"
+  "  'roots': [\n"
+  "{\n"
+  "  'type': 'directory',\n"
+  "  'name': '//root/',\n"
+  "  'contents': [ {\n"
+  "  'type': 'file',\n"
+  "  'name': 'a',\n"
+  "  'external-contents': '//root/a'\n"
+  "}\n"
+  "  ]\n"
+  "}\n"
+  "]\n"
+  "}",
+	  Lower);
+  ASSERT_TRUE(FS.get() != nullptr);
+
+  std::error_code EC;
+  checkContents(FS->dir_begin("//root/", EC),
+{"//root/a", "//root/b"});
+}
+
+TEST_F(VFSFromYAMLTest, DirectoryIterationErrorInVFSLayer) {
+  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  Lower->addDirectory("//root/");
+  Lower->addDirectory("//root/foo");
+  Lower->addRegularFile("//root/foo/a");
+  Lower->addRegularFile("//root/foo/b");
+  IntrusiveRefCntPtr FS = getFromYAMLString(
+  "{ 'use-external-names': false,\n"
+  "  'roots': [\n"
+  "{\n"
+  "  'type': 'directory',\n"
+  "  'name': '//root/',\n"
+  "  'contents': [ {\n"
+  "  'type': 'file',\n"
+  "  'name': 'bar/a',\n"
+  "  'external-contents': '//root/foo/a'\n"
+  "}\n"
+  "  ]\n"
+  "}\n"
+  "]\n"
+  "}",
+  Lower);
+  ASSERT_TRUE(FS.get() != nullptr);
+
+  std::error_code EC;
+  checkContents(FS->dir_begin("//root/foo", EC),
+{"//root/foo/a", "//root/foo/b"});
+}
Index: clang/test/VFS/vfsroot-with-overlay.c
===
--- /dev/null
+++ clang/test/VFS/vfsroot-with-overlay.c
@@ -0,0 +1,12 @@
+// REQUIRES: shell
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: sed -e "s:TEST_DIR:%S:g" -e "s:OUT_DIR:%t:g" %S/Inputs/vfsroot.yaml > %t.yaml
+// RUN: sed -e "s:INPUT_DIR:/indirect-vfs-root-files:g" -e "s:OUT_DIR:/overlay-dir:g" %S/Inputs/vfsoverlay.yaml > %t/vfsoverlay.yaml
+// RUN: %clang_cc1 -Werror -ivfsoverlay %t.yaml -ivfsoverlay /direct-vfs-root-files/vfsoverlay.yaml -I /overlay-dir -fsyntax-only /tests/vfsroot-with-overlay.c
+
+#include "not_real.h"
+
+void foo() {
+  bar();
+}
Index: clang/test/VFS/vfsroot-module.m
===
--- /dev/null
+++ clang/test/VFS/vfsroot-module.m
@@ -0,0 +1,10 @@
+// REQUIRES: shell
+// RUN: rm -rf %t
+// RUN: mkdir 

[PATCH] D50539: [VFS] Add property 'fallthrough' that controls fallback to real file system.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Known issues:

- No support for 'fallthrough' in crash reproducer.
- Current way of working with modules in VFS "root" is clunky and error-prone.

Also I don't like that VFSFromYamlDirIterImpl is similar to overlay iterator 
but doesn't share code. At the moment I think it's not worth it to rework those 
abstractions to make more code reusable. If you've noticed problems with those 
iterators before, maybe it makes sense to try to find a better approach.


https://reviews.llvm.org/D50539



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

[PATCH] D50543: [libcxx] Mark charconv tests as failing for previous libcxx versions.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added reviewers: mclow.lists, ldionne.
Herald added a reviewer: EricWF.
Herald added subscribers: dexonsmith, christof.

 was added in r338479. Previous libcxx versions don't have
this functionality and corresponding tests should be failing.


https://reviews.llvm.org/D50543

Files:
  libcxx/test/std/utilities/charconv/charconv.from.chars/integral.pass.cpp
  libcxx/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp


Index: libcxx/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp
===
--- libcxx/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp
+++ libcxx/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp
@@ -8,6 +8,15 @@
 
//===--===//
 
 // UNSUPPORTED: c++98, c++03, c++11
+
+// XFAIL: with_system_cxx_lib=macosx10.13
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
+
 // 
 
 // to_chars_result to_chars(char* first, char* last, Integral value,
Index: libcxx/test/std/utilities/charconv/charconv.from.chars/integral.pass.cpp
===
--- libcxx/test/std/utilities/charconv/charconv.from.chars/integral.pass.cpp
+++ libcxx/test/std/utilities/charconv/charconv.from.chars/integral.pass.cpp
@@ -8,6 +8,15 @@
 
//===--===//
 
 // UNSUPPORTED: c++98, c++03, c++11
+
+// XFAIL: with_system_cxx_lib=macosx10.13
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
+
 // 
 
 // from_chars_result from_chars(const char* first, const char* last,


Index: libcxx/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp
===
--- libcxx/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp
+++ libcxx/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp
@@ -8,6 +8,15 @@
 //===--===//
 
 // UNSUPPORTED: c++98, c++03, c++11
+
+// XFAIL: with_system_cxx_lib=macosx10.13
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
+
 // 
 
 // to_chars_result to_chars(char* first, char* last, Integral value,
Index: libcxx/test/std/utilities/charconv/charconv.from.chars/integral.pass.cpp
===
--- libcxx/test/std/utilities/charconv/charconv.from.chars/integral.pass.cpp
+++ libcxx/test/std/utilities/charconv/charconv.from.chars/integral.pass.cpp
@@ -8,6 +8,15 @@
 //===--===//
 
 // UNSUPPORTED: c++98, c++03, c++11
+
+// XFAIL: with_system_cxx_lib=macosx10.13
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
+
 // 
 
 // from_chars_result from_chars(const char* first, const char* last,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50543: [libcxx] Mark charconv tests as failing for previous libcxx versions.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL339451: [libcxx] Mark charconv tests as failing for previous 
libcxx versions. (authored by vsapsai, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50543?vs=160041&id=160132#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50543

Files:
  libcxx/trunk/test/std/utilities/charconv/charconv.from.chars/integral.pass.cpp
  libcxx/trunk/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp


Index: 
libcxx/trunk/test/std/utilities/charconv/charconv.from.chars/integral.pass.cpp
===
--- 
libcxx/trunk/test/std/utilities/charconv/charconv.from.chars/integral.pass.cpp
+++ 
libcxx/trunk/test/std/utilities/charconv/charconv.from.chars/integral.pass.cpp
@@ -8,6 +8,15 @@
 
//===--===//
 
 // UNSUPPORTED: c++98, c++03, c++11
+
+// XFAIL: with_system_cxx_lib=macosx10.13
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
+
 // 
 
 // from_chars_result from_chars(const char* first, const char* last,
Index: 
libcxx/trunk/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp
===
--- libcxx/trunk/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp
+++ libcxx/trunk/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp
@@ -8,6 +8,15 @@
 
//===--===//
 
 // UNSUPPORTED: c++98, c++03, c++11
+
+// XFAIL: with_system_cxx_lib=macosx10.13
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
+
 // 
 
 // to_chars_result to_chars(char* first, char* last, Integral value,


Index: libcxx/trunk/test/std/utilities/charconv/charconv.from.chars/integral.pass.cpp
===
--- libcxx/trunk/test/std/utilities/charconv/charconv.from.chars/integral.pass.cpp
+++ libcxx/trunk/test/std/utilities/charconv/charconv.from.chars/integral.pass.cpp
@@ -8,6 +8,15 @@
 //===--===//
 
 // UNSUPPORTED: c++98, c++03, c++11
+
+// XFAIL: with_system_cxx_lib=macosx10.13
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
+
 // 
 
 // from_chars_result from_chars(const char* first, const char* last,
Index: libcxx/trunk/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp
===
--- libcxx/trunk/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp
+++ libcxx/trunk/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp
@@ -8,6 +8,15 @@
 //===--===//
 
 // UNSUPPORTED: c++98, c++03, c++11
+
+// XFAIL: with_system_cxx_lib=macosx10.13
+// XFAIL: with_system_cxx_lib=macosx10.12
+// XFAIL: with_system_cxx_lib=macosx10.11
+// XFAIL: with_system_cxx_lib=macosx10.10
+// XFAIL: with_system_cxx_lib=macosx10.9
+// XFAIL: with_system_cxx_lib=macosx10.8
+// XFAIL: with_system_cxx_lib=macosx10.7
+
 // 
 
 // to_chars_result to_chars(char* first, char* last, Integral value,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50543: [libcxx] Mark charconv tests as failing for previous libcxx versions.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Thanks for the review.


Repository:
  rL LLVM

https://reviews.llvm.org/D50543



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

[PATCH] D50341: [libcxx] Fix XFAILs for aligned allocation tests on older OSX versions

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

What about defining a feature for unsupported configurations? I've tried

  if '__cpp_aligned_new' not in macros or \
  intMacroValue(macros['__cpp_aligned_new']) < 201606:
  self.config.available_features.add('libcpp-no-aligned-new')

and `// UNSUPPORTED: libcpp-no-aligned-new`. Seems to be working but it's not 
sufficient. For example, clang-6 for old macOS versions would define 
`__cpp_aligned_new` but a test would fail. Still, we can use another feature, 
not necessarily macro-based. Though probably something like 
`libcpp-no-aligned-new` can replace `// UNSUPPORTED: c++98, c++03, c++11, 
c++14`.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50341



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

[PATCH] D50341: [libcxx] Fix XFAILs for aligned allocation tests on older OSX versions

[Volodymyr Sapsai via Phabricator via cfe-commits]

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

I don't have any other comments. Looks good to me.


Repository:
  rCXX libc++

https://reviews.llvm.org/D50341



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

[PATCH] D50462: Try building complete AST after a fatal error was emitted if further diagnostics are expected

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Have you checked that produced AST is of sufficient quality to be used? 
Because, for example, for invalid code error recovery tries to keep going but 
the end result isn't always good.

In the test you verify that you can ignore bogus includes. Is this the 
real-world use case you want to address? Or something like "stddef.h not found"?


Repository:
  rC Clang

https://reviews.llvm.org/D50462



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

[PATCH] D50462: Try building complete AST after a fatal error was emitted if further diagnostics are expected

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.
Herald added a subscriber: kadircet.

Thanks, Dmitry, for your explanation. It does help to understand your use case 
better.

What about having a mode that treats missing header as non-fatal error? Because 
I believe there are cases where there is no sense to continue after a fatal 
error. For example, consider hitting the error limit. Showing too many errors 
isn't useful and also after 50 errors it is a little bit too optimistic to 
assume that AST is in a good shape. I don't know if there are other 
fatal-but-we-can-continue errors and if we need a more general solution. So far 
looks like missing include is the biggest blocker for IDE-like functionality.


Repository:
  rC Clang

https://reviews.llvm.org/D50462



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

[PATCH] D45470: Emit an error when mixing and

[Volodymyr Sapsai via Phabricator via cfe-commits]

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

`__ALLOW_STDC_ATOMICS_IN_CXX__` approach didn't work in practice, I've reverted 
all changes.


Repository:
  rC Clang

https://reviews.llvm.org/D45470



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

[PATCH] D45470: Emit an error when mixing and

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 145817.
vsapsai added a comment.

Here is another approach that should emit an error only when mixing headers
causes compilation problems.

Have no ideas how to test the change. `-verify` doesn't work with fatal errors
and libcxx doesn't use FileCheck. Performed only manual testing.


https://reviews.llvm.org/D45470

Files:
  libcxx/include/atomic


Index: libcxx/include/atomic
===
--- libcxx/include/atomic
+++ libcxx/include/atomic
@@ -555,6 +555,9 @@
 #if !defined(_LIBCPP_HAS_C_ATOMIC_IMP) && !defined(_LIBCPP_HAS_GCC_ATOMIC_IMP)
 #error  is not implemented
 #endif
+#ifdef kill_dependency
+#error C++ standard library is incompatible with 
+#endif
 
 #if _LIBCPP_STD_VER > 14
 # define __cpp_lib_atomic_is_always_lock_free 201603L


Index: libcxx/include/atomic
===
--- libcxx/include/atomic
+++ libcxx/include/atomic
@@ -555,6 +555,9 @@
 #if !defined(_LIBCPP_HAS_C_ATOMIC_IMP) && !defined(_LIBCPP_HAS_GCC_ATOMIC_IMP)
 #error  is not implemented
 #endif
+#ifdef kill_dependency
+#error C++ standard library is incompatible with 
+#endif
 
 #if _LIBCPP_STD_VER > 14
 # define __cpp_lib_atomic_is_always_lock_free 201603L
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45470: Emit an error when include after

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In https://reviews.llvm.org/D45470#1092260, @jfb wrote:

> In https://reviews.llvm.org/D45470#1092212, @vsapsai wrote:
>
> > Here is another approach that should emit an error only when mixing headers
> >  causes compilation problems.
> >
> > Have no ideas how to test the change. `-verify` doesn't work with fatal 
> > errors
> >  and libcxx doesn't use FileCheck. Performed only manual testing.
>
>
> This worked with `` before `` as well as with the order 
> reversed?




  #include 
  #include 

> fatal error: too many errors emitted, stopping now [-ferror-limit=]



  #include 
  #include 

> no errors

So when `` is //after// ``, I add one more error to 
existing. When `` is //before// ``, there are no errors 
and I don't add anything.


https://reviews.llvm.org/D45470



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

[PATCH] D46446: [c++17] Fix assertion on synthesizing deduction guides after a fatal error.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Ping.


https://reviews.llvm.org/D46446



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

[PATCH] D46446: [c++17] Fix assertion on synthesizing deduction guides after a fatal error.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL332307: [c++17] Fix assertion on synthesizing deduction 
guides after a fatal error. (authored by vsapsai, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D46446?vs=145251&id=146713#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D46446

Files:
  cfe/trunk/lib/Sema/SemaTemplate.cpp
  cfe/trunk/test/SemaTemplate/instantiate-after-fatal-cxx17.cpp


Index: cfe/trunk/lib/Sema/SemaTemplate.cpp
===
--- cfe/trunk/lib/Sema/SemaTemplate.cpp
+++ cfe/trunk/lib/Sema/SemaTemplate.cpp
@@ -1976,6 +1976,8 @@
   // FIXME: Add a kind for this to give more meaningful diagnostics. But can
   // this substitution process actually fail?
   InstantiatingTemplate BuildingDeductionGuides(*this, Loc, Template);
+  if (BuildingDeductionGuides.isInvalid())
+return;
 
   // Convert declared constructors into deduction guide templates.
   // FIXME: Skip constructors for which deduction must necessarily fail (those
Index: cfe/trunk/test/SemaTemplate/instantiate-after-fatal-cxx17.cpp
===
--- cfe/trunk/test/SemaTemplate/instantiate-after-fatal-cxx17.cpp
+++ cfe/trunk/test/SemaTemplate/instantiate-after-fatal-cxx17.cpp
@@ -0,0 +1,16 @@
+// RUN: not %clang_cc1 -std=c++17 -fsyntax-only -ferror-limit 1 %s 2>&1 | 
FileCheck %s
+
+#error Error 1
+#error Error 2
+// CHECK: fatal error: too many errors emitted, stopping now
+
+namespace rdar39051732 {
+
+  template struct A {
+template  A(T&, ...);
+  };
+  // Deduction guide triggers constructor instantiation.
+  template A(const T&, const T&) -> A;
+
+}
+


Index: cfe/trunk/lib/Sema/SemaTemplate.cpp
===
--- cfe/trunk/lib/Sema/SemaTemplate.cpp
+++ cfe/trunk/lib/Sema/SemaTemplate.cpp
@@ -1976,6 +1976,8 @@
   // FIXME: Add a kind for this to give more meaningful diagnostics. But can
   // this substitution process actually fail?
   InstantiatingTemplate BuildingDeductionGuides(*this, Loc, Template);
+  if (BuildingDeductionGuides.isInvalid())
+return;
 
   // Convert declared constructors into deduction guide templates.
   // FIXME: Skip constructors for which deduction must necessarily fail (those
Index: cfe/trunk/test/SemaTemplate/instantiate-after-fatal-cxx17.cpp
===
--- cfe/trunk/test/SemaTemplate/instantiate-after-fatal-cxx17.cpp
+++ cfe/trunk/test/SemaTemplate/instantiate-after-fatal-cxx17.cpp
@@ -0,0 +1,16 @@
+// RUN: not %clang_cc1 -std=c++17 -fsyntax-only -ferror-limit 1 %s 2>&1 | FileCheck %s
+
+#error Error 1
+#error Error 2
+// CHECK: fatal error: too many errors emitted, stopping now
+
+namespace rdar39051732 {
+
+  template struct A {
+template  A(T&, ...);
+  };
+  // Deduction guide triggers constructor instantiation.
+  template A(const T&, const T&) -> A;
+
+}
+
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46446: [c++17] Fix assertion on synthesizing deduction guides after a fatal error.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Thanks for the review.


Repository:
  rL LLVM

https://reviews.llvm.org/D46446



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

[PATCH] D46909: [Sema] Fix assertion when constructor is disabled with partially specialized template.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added reviewers: rsmith, arphaman.

The added test case was triggering assertion

> Assertion failed: 
> (!SpecializedTemplate.is() && "Already set 
> to a class template partial specialization!"), function setInstantiationOf, 
> file clang/include/clang/AST/DeclTemplate.h, line 1825.

It was happening with ClassTemplateSpecializationDecl
`enable_if_not_same`. Because this template is specialized for
equal types not to have a definition, it wasn't instantiated and its
specialization kind remained TSK_Undeclared. And because it was implicit
instantiation, we didn't mark the decl as invalid. So when we try to
find the best matching partial specialization the second time, we hit
the assertion as partial specialization is already set.

Fix by reusing stored partial specialization when available, instead of
looking for the best match every time.

rdar://problem/39524996


https://reviews.llvm.org/D46909

Files:
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaTemplate/partial-spec-instantiate.cpp

Index: clang/test/SemaTemplate/partial-spec-instantiate.cpp
===
--- clang/test/SemaTemplate/partial-spec-instantiate.cpp
+++ clang/test/SemaTemplate/partial-spec-instantiate.cpp
@@ -55,3 +55,46 @@
   // expected-no-diagnostics
 #endif
 }
+
+// rdar://problem/39524996
+namespace rdar39524996 {
+  template 
+  struct enable_if_not_same
+  {
+typedef void type;
+  };
+  template 
+  struct enable_if_not_same;
+
+  template 
+  struct Wrapper {
+// Assertion triggered on trying to set twice the same partial specialization
+// enable_if_not_same
+template 
+Wrapper(const Wrapper& other,
+typename enable_if_not_same::type* = 0) {}
+
+explicit Wrapper(int i) {}
+  };
+
+  template 
+  struct Container {
+// It is important that the struct has implicit copy and move constructors.
+Container() : x() {}
+
+template 
+Container(const Container& other) : x(static_cast(other.x)) {}
+
+// Implicit constructors are member-wise, so the field triggers instantiation
+// of T constructors and we instantiate all of them for overloading purposes.
+T x;
+  };
+
+  void takesWrapperInContainer(const Container< Wrapper >& c);
+  void test() {
+// Type mismatch triggers initialization with conversion which requires
+// implicit constructors to be instantiated.
+Container c;
+takesWrapperInContainer(c);
+  }
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2398,127 +2398,137 @@
   if (Inst.isInvalid() || Inst.isAlreadyInstantiating())
 return nullptr;
 
-  ClassTemplateDecl *Template = ClassTemplateSpec->getSpecializedTemplate();
-  CXXRecordDecl *Pattern = nullptr;
-
-  // C++ [temp.class.spec.match]p1:
-  //   When a class template is used in a context that requires an
-  //   instantiation of the class, it is necessary to determine
-  //   whether the instantiation is to be generated using the primary
-  //   template or one of the partial specializations. This is done by
-  //   matching the template arguments of the class template
-  //   specialization with the template argument lists of the partial
-  //   specializations.
-  typedef PartialSpecMatchResult MatchResult;
-  SmallVector Matched;
-  SmallVector PartialSpecs;
-  Template->getPartialSpecializations(PartialSpecs);
-  TemplateSpecCandidateSet FailedCandidates(PointOfInstantiation);
-  for (unsigned I = 0, N = PartialSpecs.size(); I != N; ++I) {
-ClassTemplatePartialSpecializationDecl *Partial = PartialSpecs[I];
-TemplateDeductionInfo Info(FailedCandidates.getLocation());
-if (Sema::TemplateDeductionResult Result = S.DeduceTemplateArguments(
-Partial, ClassTemplateSpec->getTemplateArgs(), Info)) {
-  // Store the failed-deduction information for use in diagnostics, later.
-  // TODO: Actually use the failed-deduction info?
-  FailedCandidates.addCandidate().set(
-  DeclAccessPair::make(Template, AS_public), Partial,
-  MakeDeductionFailureInfo(S.Context, Result, Info));
-  (void)Result;
-} else {
-  Matched.push_back(PartialSpecMatchResult());
-  Matched.back().Partial = Partial;
-  Matched.back().Args = Info.take();
+  llvm::PointerUnion
+  Specialized = ClassTemplateSpec->getSpecializedTemplateOrPartial();
+  if (!Specialized.is()) {
+// Find best matching specialization.
+ClassTemplateDecl *Template = ClassTemplateSpec->getSpecializedTemplate();
+
+// C++ [temp.class.spec.match]p1:
+//   When a class template is used in a context that requires an
+//   instantiation of the class, it is necessary to determine
+//   whether the instantiation is to be generated using the primary
+

[PATCH] D46909: [Sema] Fix assertion when constructor is disabled with partially specialized template.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/lib/Sema/SemaTemplate.cpp:3873-3881
   // Find the variable template specialization declaration that
   // corresponds to these arguments.
   void *InsertPos = nullptr;
   if (VarTemplateSpecializationDecl *Spec = Template->findSpecialization(
   Converted, InsertPos)) {
 checkSpecializationVisibility(TemplateNameLoc, Spec);
 // If we already have a variable template specialization, return it.

Code of interest (will refer to it later).


https://reviews.llvm.org/D46909



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

[PATCH] D46909: [Sema] Fix assertion when constructor is disabled with partially specialized template.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 146944.
vsapsai added a comment.

Potentially, VarTemplateDecl is susceptible to the same bug as
ClassTemplateSpecializationDecl. But I wasn't able to trigger it. And based on
code I've convinced myself that the mentioned early exit in
Sema::CheckVarTemplateId is equivalent to reusing available partial
specialization. So seems like no changes for VarTemplateDecl are required.


https://reviews.llvm.org/D46909

Files:
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaTemplate/partial-spec-instantiate.cpp

Index: clang/test/SemaTemplate/partial-spec-instantiate.cpp
===
--- clang/test/SemaTemplate/partial-spec-instantiate.cpp
+++ clang/test/SemaTemplate/partial-spec-instantiate.cpp
@@ -55,3 +55,46 @@
   // expected-no-diagnostics
 #endif
 }
+
+// rdar://problem/39524996
+namespace rdar39524996 {
+  template 
+  struct enable_if_not_same
+  {
+typedef void type;
+  };
+  template 
+  struct enable_if_not_same;
+
+  template 
+  struct Wrapper {
+// Assertion triggered on trying to set twice the same partial specialization
+// enable_if_not_same
+template 
+Wrapper(const Wrapper& other,
+typename enable_if_not_same::type* = 0) {}
+
+explicit Wrapper(int i) {}
+  };
+
+  template 
+  struct Container {
+// It is important that the struct has implicit copy and move constructors.
+Container() : x() {}
+
+template 
+Container(const Container& other) : x(static_cast(other.x)) {}
+
+// Implicit constructors are member-wise, so the field triggers instantiation
+// of T constructors and we instantiate all of them for overloading purposes.
+T x;
+  };
+
+  void takesWrapperInContainer(const Container< Wrapper >& c);
+  void test() {
+// Type mismatch triggers initialization with conversion which requires
+// implicit constructors to be instantiated.
+Container c;
+takesWrapperInContainer(c);
+  }
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2398,127 +2398,137 @@
   if (Inst.isInvalid() || Inst.isAlreadyInstantiating())
 return nullptr;
 
-  ClassTemplateDecl *Template = ClassTemplateSpec->getSpecializedTemplate();
-  CXXRecordDecl *Pattern = nullptr;
-
-  // C++ [temp.class.spec.match]p1:
-  //   When a class template is used in a context that requires an
-  //   instantiation of the class, it is necessary to determine
-  //   whether the instantiation is to be generated using the primary
-  //   template or one of the partial specializations. This is done by
-  //   matching the template arguments of the class template
-  //   specialization with the template argument lists of the partial
-  //   specializations.
-  typedef PartialSpecMatchResult MatchResult;
-  SmallVector Matched;
-  SmallVector PartialSpecs;
-  Template->getPartialSpecializations(PartialSpecs);
-  TemplateSpecCandidateSet FailedCandidates(PointOfInstantiation);
-  for (unsigned I = 0, N = PartialSpecs.size(); I != N; ++I) {
-ClassTemplatePartialSpecializationDecl *Partial = PartialSpecs[I];
-TemplateDeductionInfo Info(FailedCandidates.getLocation());
-if (Sema::TemplateDeductionResult Result = S.DeduceTemplateArguments(
-Partial, ClassTemplateSpec->getTemplateArgs(), Info)) {
-  // Store the failed-deduction information for use in diagnostics, later.
-  // TODO: Actually use the failed-deduction info?
-  FailedCandidates.addCandidate().set(
-  DeclAccessPair::make(Template, AS_public), Partial,
-  MakeDeductionFailureInfo(S.Context, Result, Info));
-  (void)Result;
-} else {
-  Matched.push_back(PartialSpecMatchResult());
-  Matched.back().Partial = Partial;
-  Matched.back().Args = Info.take();
+  llvm::PointerUnion
+  Specialized = ClassTemplateSpec->getSpecializedTemplateOrPartial();
+  if (!Specialized.is()) {
+// Find best matching specialization.
+ClassTemplateDecl *Template = ClassTemplateSpec->getSpecializedTemplate();
+
+// C++ [temp.class.spec.match]p1:
+//   When a class template is used in a context that requires an
+//   instantiation of the class, it is necessary to determine
+//   whether the instantiation is to be generated using the primary
+//   template or one of the partial specializations. This is done by
+//   matching the template arguments of the class template
+//   specialization with the template argument lists of the partial
+//   specializations.
+typedef PartialSpecMatchResult MatchResult;
+SmallVector Matched;
+SmallVector PartialSpecs;
+Template->getPartialSpecializations(PartialSpecs);
+TemplateSpecCandidateSet FailedCandidates(PointOfInstantiation);
+for (unsigned I = 0, N = PartialSpecs.size(); I != N; ++I) {
+  

[PATCH] D45470: Emit an error when include after

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL332413: Emit an error when include  after 
 (authored by vsapsai, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D45470?vs=145817&id=146946#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D45470

Files:
  libcxx/trunk/include/atomic


Index: libcxx/trunk/include/atomic
===
--- libcxx/trunk/include/atomic
+++ libcxx/trunk/include/atomic
@@ -555,6 +555,9 @@
 #if !defined(_LIBCPP_HAS_C_ATOMIC_IMP) && !defined(_LIBCPP_HAS_GCC_ATOMIC_IMP)
 #error  is not implemented
 #endif
+#ifdef kill_dependency
+#error C++ standard library is incompatible with 
+#endif
 
 #if _LIBCPP_STD_VER > 14
 # define __cpp_lib_atomic_is_always_lock_free 201603L


Index: libcxx/trunk/include/atomic
===
--- libcxx/trunk/include/atomic
+++ libcxx/trunk/include/atomic
@@ -555,6 +555,9 @@
 #if !defined(_LIBCPP_HAS_C_ATOMIC_IMP) && !defined(_LIBCPP_HAS_GCC_ATOMIC_IMP)
 #error  is not implemented
 #endif
+#ifdef kill_dependency
+#error C++ standard library is incompatible with 
+#endif
 
 #if _LIBCPP_STD_VER > 14
 # define __cpp_lib_atomic_is_always_lock_free 201603L
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45470: Emit an error when include after

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Thanks for review.


Repository:
  rL LLVM

https://reviews.llvm.org/D45470



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

[PATCH] D46909: [Sema] Fix assertion when constructor is disabled with partially specialized template.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Thanks for the quick review, Richard. I'll keep in mind the idea with comparing 
results of multiple lookups when I work on other partial specialization-related 
bugs.


https://reviews.llvm.org/D46909



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

[PATCH] D46909: [Sema] Fix assertion when constructor is disabled with partially specialized template.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL332509: [Sema] Fix assertion when constructor is disabled 
with partially specialized… (authored by vsapsai, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D46909?vs=146944&id=147144#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D46909

Files:
  cfe/trunk/lib/Sema/SemaTemplateInstantiate.cpp
  cfe/trunk/test/SemaTemplate/partial-spec-instantiate.cpp

Index: cfe/trunk/lib/Sema/SemaTemplateInstantiate.cpp
===
--- cfe/trunk/lib/Sema/SemaTemplateInstantiate.cpp
+++ cfe/trunk/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2398,127 +2398,137 @@
   if (Inst.isInvalid() || Inst.isAlreadyInstantiating())
 return nullptr;
 
-  ClassTemplateDecl *Template = ClassTemplateSpec->getSpecializedTemplate();
-  CXXRecordDecl *Pattern = nullptr;
-
-  // C++ [temp.class.spec.match]p1:
-  //   When a class template is used in a context that requires an
-  //   instantiation of the class, it is necessary to determine
-  //   whether the instantiation is to be generated using the primary
-  //   template or one of the partial specializations. This is done by
-  //   matching the template arguments of the class template
-  //   specialization with the template argument lists of the partial
-  //   specializations.
-  typedef PartialSpecMatchResult MatchResult;
-  SmallVector Matched;
-  SmallVector PartialSpecs;
-  Template->getPartialSpecializations(PartialSpecs);
-  TemplateSpecCandidateSet FailedCandidates(PointOfInstantiation);
-  for (unsigned I = 0, N = PartialSpecs.size(); I != N; ++I) {
-ClassTemplatePartialSpecializationDecl *Partial = PartialSpecs[I];
-TemplateDeductionInfo Info(FailedCandidates.getLocation());
-if (Sema::TemplateDeductionResult Result = S.DeduceTemplateArguments(
-Partial, ClassTemplateSpec->getTemplateArgs(), Info)) {
-  // Store the failed-deduction information for use in diagnostics, later.
-  // TODO: Actually use the failed-deduction info?
-  FailedCandidates.addCandidate().set(
-  DeclAccessPair::make(Template, AS_public), Partial,
-  MakeDeductionFailureInfo(S.Context, Result, Info));
-  (void)Result;
-} else {
-  Matched.push_back(PartialSpecMatchResult());
-  Matched.back().Partial = Partial;
-  Matched.back().Args = Info.take();
+  llvm::PointerUnion
+  Specialized = ClassTemplateSpec->getSpecializedTemplateOrPartial();
+  if (!Specialized.is()) {
+// Find best matching specialization.
+ClassTemplateDecl *Template = ClassTemplateSpec->getSpecializedTemplate();
+
+// C++ [temp.class.spec.match]p1:
+//   When a class template is used in a context that requires an
+//   instantiation of the class, it is necessary to determine
+//   whether the instantiation is to be generated using the primary
+//   template or one of the partial specializations. This is done by
+//   matching the template arguments of the class template
+//   specialization with the template argument lists of the partial
+//   specializations.
+typedef PartialSpecMatchResult MatchResult;
+SmallVector Matched;
+SmallVector PartialSpecs;
+Template->getPartialSpecializations(PartialSpecs);
+TemplateSpecCandidateSet FailedCandidates(PointOfInstantiation);
+for (unsigned I = 0, N = PartialSpecs.size(); I != N; ++I) {
+  ClassTemplatePartialSpecializationDecl *Partial = PartialSpecs[I];
+  TemplateDeductionInfo Info(FailedCandidates.getLocation());
+  if (Sema::TemplateDeductionResult Result = S.DeduceTemplateArguments(
+  Partial, ClassTemplateSpec->getTemplateArgs(), Info)) {
+// Store the failed-deduction information for use in diagnostics, later.
+// TODO: Actually use the failed-deduction info?
+FailedCandidates.addCandidate().set(
+DeclAccessPair::make(Template, AS_public), Partial,
+MakeDeductionFailureInfo(S.Context, Result, Info));
+(void)Result;
+  } else {
+Matched.push_back(PartialSpecMatchResult());
+Matched.back().Partial = Partial;
+Matched.back().Args = Info.take();
+  }
 }
-  }
 
-  // If we're dealing with a member template where the template parameters
-  // have been instantiated, this provides the original template parameters
-  // from which the member template's parameters were instantiated.
-
-  if (Matched.size() >= 1) {
-SmallVectorImpl::iterator Best = Matched.begin();
-if (Matched.size() == 1) {
-  //   -- If exactly one matching specialization is found, the
-  //  instantiation is generated from that specialization.
-  // We don't need to do anything for this.
-} else {
-  //   -- If more than one matching specialization is found, the
-  //  partial order rules (14.5.4.2) are used to determine

[PATCH] D45015: [Preprocessor] Allow libc++ to detect when aligned allocation is unavailable.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Eric, do you have more thoughts on this issue?


Repository:
  rC Clang

https://reviews.llvm.org/D45015



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

[PATCH] D45015: [Preprocessor] Allow libc++ to detect when aligned allocation is unavailable.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In https://reviews.llvm.org/D45015#1105314, @rsmith wrote:

> That is: on old Darwin, we should not define `__cpp_aligned_allocation` (even 
> in C++17), produce the "no aligned allocation support" warning in C++17 mode, 
> and then not try to call the aligned allocation function. But if 
> `-faligned-allocation` or `-fno-aligned-allocation` is specified explicitly, 
> then the user knows what they're doing and they get no warning.


What when compiler has `__builtin_operator_new`, `__builtin_operator_delete`? 
If I build libc++ tests with recent Clang which has these builtins and run 
tests with libc++.dylib from old Darwin, there are no linkage errors. Should we 
define `__cpp_aligned_allocation` in this case?


Repository:
  rC Clang

https://reviews.llvm.org/D45015



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

[PATCH] D45015: [Preprocessor] Allow libc++ to detect when aligned allocation is unavailable.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Somewhat tangential, in discussion with Duncan he mentioned that `-nostdinc++` 
should turn off assumptions about old Darwin. So if you build libc++ yourself, 
you don't care what does the system stdlib support. I agree with that and think 
it doesn't interfere with the latest proposal but adds more to it.


Repository:
  rC Clang

https://reviews.llvm.org/D45015



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

[PATCH] D47341: [Sema] Disable creating new delayed typos while correcting existing.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added reviewers: arphaman, majnemer.

NumTypos guard value ~0U doesn't prevent from creating new delayed
typos. When you create new delayed typos during typo correction, value
~0U wraps around to 0. This state is inconsistent and depending on total
number of typos you can hit the assertion

> Assertion failed: (DelayedTypos.empty() && "Uncorrected typos!"), function 
> ~Sema, file clang/lib/Sema/Sema.cpp, line 366.

or have infinite typo correction loop.

Fix by disabling typo correction during performing typo correcting
transform. It disables the feature of having typo corrections on top of
other typo corrections because that feature is unreliable.

rdar://problem/38642201


https://reviews.llvm.org/D47341

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/Sema/typo-correction.c
  clang/test/SemaCXX/typo-correction-delayed.cpp

Index: clang/test/SemaCXX/typo-correction-delayed.cpp
===
--- clang/test/SemaCXX/typo-correction-delayed.cpp
+++ clang/test/SemaCXX/typo-correction-delayed.cpp
@@ -137,13 +137,12 @@
 
 namespace PR21925 {
 struct X {
-  int get() { return 7; }  // expected-note {{'get' declared here}}
+  int get() { return 7; }
 };
 void test() {
-  X variable;  // expected-note {{'variable' declared here}}
+  X variable;
 
-  // expected-error@+2 {{use of undeclared identifier 'variableX'; did you mean 'variable'?}}
-  // expected-error@+1 {{no member named 'getX' in 'PR21925::X'; did you mean 'get'?}}
+  // expected-error@+1 {{use of undeclared identifier 'variableX'}}
   int x = variableX.getX();
 }
 }
Index: clang/test/Sema/typo-correction.c
===
--- clang/test/Sema/typo-correction.c
+++ clang/test/Sema/typo-correction.c
@@ -87,3 +87,16 @@
 void overloadable_callexpr(int arg) {
 	func_overloadable(ar); //expected-error{{use of undeclared identifier}}
 }
+
+// rdar://problem/38642201
+struct rdar38642201 {
+  int fieldName;
+};
+
+void rdar38642201_callee(int x, int y);
+void rdar38642201_caller() {
+  struct rdar38642201 structVar;
+  rdar38642201_callee(
+  structVarTypo.fieldNameTypo, //expected-error{{use of undeclared identifier 'structVarTypo'}}
+  structVarTypo2.fieldNameTypo2); //expected-error{{use of undeclared identifier 'structVarTypo2'}}
+}
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -7588,7 +7588,7 @@
 // handling potentially ambiguous typo corrections as any new TypoExprs will
 // have been introduced by the application of one of the correction
 // candidates and add little to no value if corrected.
-SemaRef.DisableTypoCorrection = true;
+Sema::DisableTypoCorrectionScope Scope(SemaRef);
 while (!AmbiguousTypoExprs.empty()) {
   auto TE  = AmbiguousTypoExprs.back();
   auto Cached = TransformCache[TE];
@@ -7605,7 +7605,6 @@
   State.Consumer->restoreSavedPosition();
   TransformCache[TE] = Cached;
 }
-SemaRef.DisableTypoCorrection = false;
 
 // Ensure that all of the TypoExprs within the current Expr have been found.
 if (!Res.isUsable())
@@ -7668,11 +7667,14 @@
   if (E && !ExprEvalContexts.empty() && ExprEvalContexts.back().NumTypos &&
   (E->isTypeDependent() || E->isValueDependent() ||
E->isInstantiationDependent())) {
+DisableTypoCorrectionScope DisableRecurrentCorrectionScope(*this);
 auto TyposInContext = ExprEvalContexts.back().NumTypos;
 assert(TyposInContext < ~0U && "Recursive call of CorrectDelayedTyposInExpr");
 ExprEvalContexts.back().NumTypos = ~0U;
 auto TyposResolved = DelayedTypos.size();
 auto Result = TransformTypos(*this, InitDecl, Filter).Transform(E);
+assert(ExprEvalContexts.back().NumTypos == ~0U &&
+   "Unexpected NumTypos modification");
 ExprEvalContexts.back().NumTypos = TyposInContext;
 TyposResolved -= DelayedTypos.size();
 if (Result.isInvalid() || Result.get() != E) {
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -7561,6 +7561,22 @@
 }
   };
 
+  /// RAII class used to disable typo correction temporarily.
+  class DisableTypoCorrectionScope {
+Sema &SemaRef;
+bool PrevDisableTypoCorrection;
+
+  public:
+explicit DisableTypoCorrectionScope(Sema &SemaRef)
+: SemaRef(SemaRef),
+  PrevDisableTypoCorrection(SemaRef.DisableTypoCorrection) {
+  SemaRef.DisableTypoCorrection = true;
+}
+~DisableTypoCorrectionScope() {
+  SemaRef.DisableTypoCorrection = PrevDisableTypoCorrection;
+}
+  };
+
   /// The current instantiation scope used to store local
   /// variables.
   LocalInstantiationScope *CurrentInstantiationScope

[PATCH] D47301: Warning for framework include violation from Headers to PrivateHeaders

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: include/clang/Basic/DiagnosticGroups.td:34-35
 def AutoImport : DiagGroup<"auto-import">;
 def FrameworkHdrQuotedInclude : 
DiagGroup<"quoted-include-in-framework-header">;
+def FrameworkIncludePrivateFromPublic : 
DiagGroup<"framework-include-private-from-public">;
 def CXX14BinaryLiteral : DiagGroup<"c++14-binary-literal">;

It might be convenient for users to have a warning group that will cover 
different framework warnings, something like -Wframework-hygiene. But it's out 
of scope for this review, more as an idea for future improvements.



Comment at: lib/Lex/HeaderSearch.cpp:625
+static bool isFrameworkStylePath(StringRef Path, bool &IsPrivateHeader,
+ SmallVectorImpl &FrameworkName) {
   using namespace llvm::sys;

In this function we accept some paths that aren't valid framework paths. Need 
to think more if it is OK or if we want to be stricter.



Comment at: lib/Lex/HeaderSearch.cpp:893
+  // from Foo.framework/PrivateHeaders, since this violates public/private
+  // api boundaries and can cause modular dependency cycles.
+  SmallString<128> ToFramework;

s/api/API/



Comment at: lib/Lex/HeaderSearch.cpp:895
+  SmallString<128> ToFramework;
+  bool IncludeIsFrameworkPrivateHeader = false;
+  if (IsIncluderFromFramework && !IsFrameworkPrivateHeader &&

My opinion on readability is personal, so take it with a grain of salt. What if 
you make variable names more consistent, like
* IsIncluderPrivateHeader
* IsIncluderFromFramework
* IsIncludeePrivateHeader



Comment at: test/Modules/framework-public-includes-private.m:33
+
+int bar() { return foo(); }
+

I'm not entirely sure it's not covered by existing test. It might be worth 
testing private header including public header and private header including 
another private header.


https://reviews.llvm.org/D47301



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

[PATCH] D44568: Fix emission of phony dependency targets when adding extra deps

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: lib/Frontend/DependencyFile.cpp:182-185
 for (const auto &ExtraDep : Opts.ExtraDeps) {
   AddFilename(ExtraDep);
+  ++InputFileIndex;
 }

This is incorrect if there are duplicates in `Opts.ExtraDeps`. Also please 
update the test to have duplicate extra dependencies.



Comment at: test/Frontend/dependency-gen-extradeps-phony.c:6-7
+
+// CHECK: dependency-gen-extradeps-phony.o: 1.extra 2.extra
+// CHECK-NOT: .c:
+// CHECK: 1.extra:

I think it would be better to have a check

// CHECK:   dependency-gen-extradeps-phony.c

Because you expect it to be there and it's not that simple to notice the colon 
in `.c:`, so it's not immediately clear how CHECK-NOT is applied here.



Comment at: test/Frontend/dependency-gen-extradeps-phony.c:9-11
+// CHECK-NOT: .c:
+// CHECK: 2.extra:
+// CHECK-NOT: .c:

For these repeated CHECK-NOT please consider using `FileCheck 
--implicit-check-not`. In this case it's not that important as the test is 
small but it can still help to capture your intention more clearly.


Repository:
  rC Clang

https://reviews.llvm.org/D44568



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

[PATCH] D47341: [Sema] Disable creating new delayed typos while correcting existing.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai planned changes to this revision.
vsapsai added a comment.

After looking into this more, I think there are 2 different bugs: one with 
infinite loop and another with `DelayedTypos.empty() && "Uncorrected typos!"` 
assertion. And disabling typo correction happened to fix both of them.

Infinite loop seems to be avoidable by not using ~0U as the guard value, need 
to investigate further.

And uncorrected dangling delayed typo bug has a different cause. When we are 
checking potential corrections, we have roughly the following behaviour

  1. structVarTypo -> structVar; structVarTypo2 -> structVar;
   after correction discover more typos
   fieldNameTypo -> fieldName; fieldNameTypo2 -> fieldName;  // Overall 
correction fails but newly discovered typos are processed and removed.
  
  2. structVarTypo -> ; structVarTypo2 -> structVar;
   correction fails early, don't discover more typos
  
  3. structVarTypo -> structVar; structVarTypo2 -> ;
   correction fails but we discover fieldNameTypo and the way correction 
fails we don't attempt to correct this new delayed typo
  
  4. structVarTypo -> ; structVarTypo2 -> ;
   correction fails early, don't discover more typos

So the typo from step 3 is the one that remains till ~Sema.

I've spent some time looking into Richard's suggestion to correct typos 
immediately. That gets more involved than I expected and I want to finish 
investigating my other ideas. Now I think that my original approach with 
disabling typo correction just hides the issue instead of fixing it. And I feel 
like immediate typo correction can be hiding the actual issue too but it is too 
early to say.


https://reviews.llvm.org/D47341



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

[PATCH] D44568: Fix emission of phony dependency targets when adding extra deps

[Volodymyr Sapsai via Phabricator via cfe-commits]

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

Looks good to me. Just watch the build bots in case some of them are strict 
with warnings and require `(void)AddFilename(Filename)`.

As for the case when the input file is also an extra dependency, I think we can 
ignore that for now because extra dependencies are supposed to be sanitizer 
blacklists. Even if that changes in the future, I expect extra dependencies to 
stay orthogonal to the input.




Comment at: test/Frontend/dependency-gen-extradeps-phony.c:6-7
+
+// CHECK: dependency-gen-extradeps-phony.o: 1.extra 2.extra
+// CHECK-NOT: .c:
+// CHECK: 1.extra:

dstenb wrote:
> vsapsai wrote:
> > I think it would be better to have a check
> > 
> > // CHECK:   dependency-gen-extradeps-phony.c
> > 
> > Because you expect it to be there and it's not that simple to notice the 
> > colon in `.c:`, so it's not immediately clear how CHECK-NOT is applied here.
> Do you mean replace the two checks with that? Isn't there a risk that that 
> would match with `dependency-gen-extradeps-phony.c:`, which the not-checks 
> would not pick up then?
> 
> I added a CHECK-NEXT check for the input file so that we match that whole 
> dependency entry at least.
You are right, you need to be careful to make sure that we match .c file only 
as a dependency and not as a target. Good solution with CHECK-NEXT. Now I'm 
happy with the test as expected output is clearly visible and we protect from 
undesired output.


https://reviews.llvm.org/D44568



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

[PATCH] D47341: [Sema] Disable creating new delayed typos while correcting existing.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 148841.
vsapsai added a comment.

- Fix only infinite loop and don't touch the assertion failure.

New commit message should be:

[Sema] Fix infinite typo correction loop.

NumTypos guard value ~0U doesn't prevent from creating new delayed typos. When
you create new delayed typos during typo correction, value ~0U wraps around to
0. When NumTypos is 0 we can miss some typos and treat an expression as it can
be typo-corrected. But if the expression is still invalid after correction, we
can get stuck in infinite loop trying to correct it.

Fix by not using value ~0U so that NumTypos correctly reflects the number of
typos.

rdar://problem/38642201


https://reviews.llvm.org/D47341

Files:
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/Sema/typo-correction.c


Index: clang/test/Sema/typo-correction.c
===
--- clang/test/Sema/typo-correction.c
+++ clang/test/Sema/typo-correction.c
@@ -87,3 +87,16 @@
 void overloadable_callexpr(int arg) {
func_overloadable(ar); //expected-error{{use of undeclared identifier}}
 }
+
+// rdar://problem/38642201
+struct rdar38642201 {
+  int fieldName;
+};
+
+void rdar38642201_callee(int x, int y);
+void rdar38642201_caller() {
+  struct rdar38642201 structVar;
+  rdar38642201_callee(
+  structVar1.fieldName1.member1, //expected-error{{use of undeclared 
identifier 'structVar1'}}
+  structVar2.fieldName2.member2); //expected-error{{use of undeclared 
identifier 'structVar2'}}
+}
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -7668,12 +7668,8 @@
   if (E && !ExprEvalContexts.empty() && ExprEvalContexts.back().NumTypos &&
   (E->isTypeDependent() || E->isValueDependent() ||
E->isInstantiationDependent())) {
-auto TyposInContext = ExprEvalContexts.back().NumTypos;
-assert(TyposInContext < ~0U && "Recursive call of 
CorrectDelayedTyposInExpr");
-ExprEvalContexts.back().NumTypos = ~0U;
 auto TyposResolved = DelayedTypos.size();
 auto Result = TransformTypos(*this, InitDecl, Filter).Transform(E);
-ExprEvalContexts.back().NumTypos = TyposInContext;
 TyposResolved -= DelayedTypos.size();
 if (Result.isInvalid() || Result.get() != E) {
   ExprEvalContexts.back().NumTypos -= TyposResolved;


Index: clang/test/Sema/typo-correction.c
===
--- clang/test/Sema/typo-correction.c
+++ clang/test/Sema/typo-correction.c
@@ -87,3 +87,16 @@
 void overloadable_callexpr(int arg) {
 	func_overloadable(ar); //expected-error{{use of undeclared identifier}}
 }
+
+// rdar://problem/38642201
+struct rdar38642201 {
+  int fieldName;
+};
+
+void rdar38642201_callee(int x, int y);
+void rdar38642201_caller() {
+  struct rdar38642201 structVar;
+  rdar38642201_callee(
+  structVar1.fieldName1.member1, //expected-error{{use of undeclared identifier 'structVar1'}}
+  structVar2.fieldName2.member2); //expected-error{{use of undeclared identifier 'structVar2'}}
+}
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -7668,12 +7668,8 @@
   if (E && !ExprEvalContexts.empty() && ExprEvalContexts.back().NumTypos &&
   (E->isTypeDependent() || E->isValueDependent() ||
E->isInstantiationDependent())) {
-auto TyposInContext = ExprEvalContexts.back().NumTypos;
-assert(TyposInContext < ~0U && "Recursive call of CorrectDelayedTyposInExpr");
-ExprEvalContexts.back().NumTypos = ~0U;
 auto TyposResolved = DelayedTypos.size();
 auto Result = TransformTypos(*this, InitDecl, Filter).Transform(E);
-ExprEvalContexts.back().NumTypos = TyposInContext;
 TyposResolved -= DelayedTypos.size();
 if (Result.isInvalid() || Result.get() != E) {
   ExprEvalContexts.back().NumTypos -= TyposResolved;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47341: [Sema] Disable creating new delayed typos while correcting existing.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Some debugging details that can be useful but too specific for the commit 
message. When we have infinite loop, the steps are:

1. Detect typos `structVar1`, `structVar2`
2. Correct typos `structVar1`, `structVar2`
  1. Replace `structVar1` with `structVar`. Add corresponding object to 
`TransformTypos::TypoExprs`.
  2. Detect typo `fieldName1`. `NumTypos` becomes 0.
  3. Try to `CorrectDelayedTyposInExpr` for `fieldName1` TypoExpr. As there are 
no typos, no expression transformation.
  4. Replace `structVar2` with `structVar`. Add corresponding object to 
`TransformTypos::TypoExprs`.
  5. Detect typo `fieldName2`. `NumTypos` becomes 1.
1. Replace `fieldName2` with `fieldName`
  6. Figure out that `structVar.fieldName.member2` is invalid, 
CheckAndAdvanceTypoExprCorrectionStreams
3. Try different TypoCorrection for `structVar1`. As it is empty, the 
correction fails.
4. CheckAndAdvanceTypoExprCorrectionStreams. Reset correction stream for 
`structVar1` and keep going with typo correction because `structVar2` typo 
correction stream isn't finished.
5. Try different TypoCorrection for `structVar1`.
  1. Replace `structVar1` with `structVar`
  2. Detect typo `fieldName1`
1. Replace `fieldName1` with `fieldName`
  3. Figure out that `structVar.fieldName.member1` is invalid, 
CheckAndAdvanceTypoExprCorrectionStreams
6. Try different TypoCorrection for `structVar1`. As it is empty, the 
correction fails.
7. CheckAndAdvanceTypoExprCorrectionStreams over and over again.

After my fix the steps are:

1. Detect typos `structVar1`, `structVar2`
2. Correct typos `structVar1`, `structVar2`
  1. Replace `structVar1` with `structVar`. Add corresponding object to 
`TransformTypos::TypoExprs`.
  2. Detect typo `fieldName1`. `NumTypos` becomes 3.
1. Replace `fieldName1` with `fieldName`
  3. Figure out that `structVar.fieldName.member1` is invalid, 
CheckAndAdvanceTypoExprCorrectionStreams
3. Try different TypoCorrection for `structVar1`. As it is empty, the 
correction fails.
4. All available typo corrections were tried because 
`TransformTypos::TypoExprs` contains only `structVar1`. Complete the typo 
correction.


https://reviews.llvm.org/D47341



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

[PATCH] D38327: [Sema] Put nullability fix-it after the end of the pointer.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.

Fixes nullability fix-it for `id`. With this change
nullability specifier is inserted after ">" instead of between
"id" and "<".

rdar://problem/34260995


https://reviews.llvm.org/D38327

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaType.cpp
  clang/test/FixIt/Inputs/nullability-objc.h
  clang/test/FixIt/nullability.mm
  clang/test/SemaObjCXX/Inputs/nullability-consistency-2.h

Index: clang/test/SemaObjCXX/Inputs/nullability-consistency-2.h
===
--- clang/test/SemaObjCXX/Inputs/nullability-consistency-2.h
+++ clang/test/SemaObjCXX/Inputs/nullability-consistency-2.h
@@ -6,9 +6,9 @@
 
 void g3(const
 id // expected-warning{{missing a nullability type specifier}}
+volatile
 // expected-note@-1 {{insert '_Nullable' if the pointer may be null}}
 // expected-note@-2 {{insert '_Nonnull' if the pointer should never be null}}
-volatile
 * // expected-warning{{missing a nullability type specifier}}
 // expected-note@-1 {{insert '_Nullable' if the pointer may be null}}
 // expected-note@-2 {{insert '_Nonnull' if the pointer should never be null}}
Index: clang/test/FixIt/nullability.mm
===
--- clang/test/FixIt/nullability.mm
+++ clang/test/FixIt/nullability.mm
@@ -2,8 +2,10 @@
 // RUN: not %clang_cc1 -fdiagnostics-parseable-fixits -fblocks -std=gnu++11 -I %S/Inputs %s >%t.txt 2>&1
 // RUN: FileCheck %s < %t.txt
 // RUN: FileCheck %S/Inputs/nullability.h < %t.txt
+// RUN: FileCheck %S/Inputs/nullability-objc.h < %t.txt
 
 #include "nullability.h"
+#include "nullability-objc.h"
 
 #pragma clang assume_nonnull begin
 
Index: clang/test/FixIt/Inputs/nullability-objc.h
===
--- /dev/null
+++ clang/test/FixIt/Inputs/nullability-objc.h
@@ -0,0 +1,48 @@
+@class Item;
+@class Container;
+@protocol Protocol;
+
+// rdar://problem/34260995
+// The first pointer in the file is handled in a different way so need
+// a separate test for this case even if the parameter type is the same as in
+// objcIdParameterWithProtocol.
+void objcIdParameterWithProtocolFirstInFile(id i); // expected-warning {{pointer is missing a nullability type specifier}}
+// expected-note@-1 {{insert '_Nullable'}}
+// expected-note@-2 {{insert '_Nonnull'}}
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-3]]:57-[[@LINE-3]]:57}:" _Nullable"
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-4]]:57-[[@LINE-4]]:57}:" _Nonnull"
+
+int * _Nonnull forceNullabilityWarningsObjC(void);
+
+void objcClassParameter(Item *i); // expected-warning {{pointer is missing a nullability type specifier}}
+// expected-note@-1 {{insert '_Nullable'}}
+// expected-note@-2 {{insert '_Nonnull'}}
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-3]]:31-[[@LINE-3]]:31}:" _Nullable "
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-4]]:31-[[@LINE-4]]:31}:" _Nonnull "
+
+void objcClassParameterWithProtocol(Item *i); // expected-warning {{pointer is missing a nullability type specifier}}
+// expected-note@-1 {{insert '_Nullable'}}
+// expected-note@-2 {{insert '_Nonnull'}}
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-3]]:53-[[@LINE-3]]:53}:" _Nullable "
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-4]]:53-[[@LINE-4]]:53}:" _Nonnull "
+
+// rdar://problem/34260995
+void objcIdParameterWithProtocol(id i); // expected-warning {{pointer is missing a nullability type specifier}}
+// expected-note@-1 {{insert '_Nullable'}}
+// expected-note@-2 {{insert '_Nonnull'}}
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-3]]:46-[[@LINE-3]]:46}:" _Nullable"
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-4]]:46-[[@LINE-4]]:46}:" _Nonnull"
+
+// Class parameters don't have nullability type specifier.
+void objcParameterizedClassParameter(Container *c); // expected-warning {{pointer is missing a nullability type specifier}}
+// expected-note@-1 {{insert '_Nullable'}}
+// expected-note@-2 {{insert '_Nonnull'}}
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-3]]:57-[[@LINE-3]]:57}:" _Nullable "
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-4]]:57-[[@LINE-4]]:57}:" _Nonnull "
+
+// Class parameters don't have nullability type specifier.
+void objcParameterizedClassParameterWithProtocol(Container> *c); // expected-warning {{pointer is missing a nullability type specifier}}
+// expected-note@-1 {{insert '_Nullable'}}
+// expected-note@-2 {{insert '_Nonnull'}}
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-3]]:75-[[@LINE-3]]:75}:" _Nullable "
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-4]]:75-[[@LINE-4]]:75}:" _Nonnull "
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -3500,7 +3500,8 @@
 
 static void emitNullabilityConsistencyWarning(Sema &S,

[PATCH] D38327: [Sema] Put nullability fix-it after the end of the pointer.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

To preempt some of review feedback here are attempted and rejected approaches:

- Pass `pointerLoc` and `pointerEndLoc` as `pointerRange`. Such source range 
can be misleading as `pointerLoc` doesn't necesserily point at the beginning. 
For example, for `int *x` pointer range would be "*" and I would expect it to 
be "int *". So it's not really a range but 2 related locations.

- Use `D.getDeclSpec().getLocEnd()` instead of 
`D.getDeclSpec().getTypeSpecTypeLoc()`. In this case warning location points at 
the closing angle bracket and that can be confusing to developers. It looks like

  ./test.h:14:3: warning: pointer is missing a nullability type specifier 
(_Nonnull, _Nullable, or _Null_unspecified)
id thingies;
   ^



- Use `pointerLoc` for insert note instead of `pointerEndLoc`. It looks like

  ./test.h:14:3: note: insert '_Nullable' if the pointer may be null
id thingies;
^
 _Nullable

compared to suggested

  ./test.h:14:18: note: insert '_Nullable' if the pointer may be null
id thingies;
   ^
 _Nullable

I don't expect developers to know that they should match whitespace preceding 
_Nullable to calculate where in the line it should be inserted. And I think 
developers shouldn't care about it. So put the cursor where you expect the text 
to be inserted.


https://reviews.llvm.org/D38327



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

[PATCH] D38327: [Sema] Put nullability fix-it after the end of the pointer.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL314473: [Sema] Put nullability fix-it after the end of the 
pointer. (authored by vsapsai).

Changed prior to commit:
  https://reviews.llvm.org/D38327?vs=116861&id=117076#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D38327

Files:
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/lib/Sema/SemaType.cpp
  cfe/trunk/test/FixIt/Inputs/nullability-objc.h
  cfe/trunk/test/FixIt/nullability.mm
  cfe/trunk/test/SemaObjCXX/Inputs/nullability-consistency-2.h

Index: cfe/trunk/include/clang/Sema/Sema.h
===
--- cfe/trunk/include/clang/Sema/Sema.h
+++ cfe/trunk/include/clang/Sema/Sema.h
@@ -229,6 +229,10 @@
   /// not have a corresponding nullability annotation.
   SourceLocation PointerLoc;
 
+  /// The end location for the first pointer declarator in the file. Used for
+  /// placing fix-its.
+  SourceLocation PointerEndLoc;
+
   /// Which kind of pointer declarator we saw.
   uint8_t PointerKind;
 
Index: cfe/trunk/test/SemaObjCXX/Inputs/nullability-consistency-2.h
===
--- cfe/trunk/test/SemaObjCXX/Inputs/nullability-consistency-2.h
+++ cfe/trunk/test/SemaObjCXX/Inputs/nullability-consistency-2.h
@@ -6,9 +6,9 @@
 
 void g3(const
 id // expected-warning{{missing a nullability type specifier}}
+volatile
 // expected-note@-1 {{insert '_Nullable' if the pointer may be null}}
 // expected-note@-2 {{insert '_Nonnull' if the pointer should never be null}}
-volatile
 * // expected-warning{{missing a nullability type specifier}}
 // expected-note@-1 {{insert '_Nullable' if the pointer may be null}}
 // expected-note@-2 {{insert '_Nonnull' if the pointer should never be null}}
Index: cfe/trunk/test/FixIt/nullability.mm
===
--- cfe/trunk/test/FixIt/nullability.mm
+++ cfe/trunk/test/FixIt/nullability.mm
@@ -2,8 +2,10 @@
 // RUN: not %clang_cc1 -fdiagnostics-parseable-fixits -fblocks -std=gnu++11 -I %S/Inputs %s >%t.txt 2>&1
 // RUN: FileCheck %s < %t.txt
 // RUN: FileCheck %S/Inputs/nullability.h < %t.txt
+// RUN: FileCheck %S/Inputs/nullability-objc.h < %t.txt
 
 #include "nullability.h"
+#include "nullability-objc.h"
 
 #pragma clang assume_nonnull begin
 
Index: cfe/trunk/test/FixIt/Inputs/nullability-objc.h
===
--- cfe/trunk/test/FixIt/Inputs/nullability-objc.h
+++ cfe/trunk/test/FixIt/Inputs/nullability-objc.h
@@ -0,0 +1,48 @@
+@class Item;
+@class Container;
+@protocol Protocol;
+
+// rdar://problem/34260995
+// The first pointer in the file is handled in a different way so need
+// a separate test for this case even if the parameter type is the same as in
+// objcIdParameterWithProtocol.
+void objcIdParameterWithProtocolFirstInFile(id i); // expected-warning {{pointer is missing a nullability type specifier}}
+// expected-note@-1 {{insert '_Nullable'}}
+// expected-note@-2 {{insert '_Nonnull'}}
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-3]]:57-[[@LINE-3]]:57}:" _Nullable"
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-4]]:57-[[@LINE-4]]:57}:" _Nonnull"
+
+int * _Nonnull forceNullabilityWarningsObjC(void);
+
+void objcClassParameter(Item *i); // expected-warning {{pointer is missing a nullability type specifier}}
+// expected-note@-1 {{insert '_Nullable'}}
+// expected-note@-2 {{insert '_Nonnull'}}
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-3]]:31-[[@LINE-3]]:31}:" _Nullable "
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-4]]:31-[[@LINE-4]]:31}:" _Nonnull "
+
+void objcClassParameterWithProtocol(Item *i); // expected-warning {{pointer is missing a nullability type specifier}}
+// expected-note@-1 {{insert '_Nullable'}}
+// expected-note@-2 {{insert '_Nonnull'}}
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-3]]:53-[[@LINE-3]]:53}:" _Nullable "
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-4]]:53-[[@LINE-4]]:53}:" _Nonnull "
+
+// rdar://problem/34260995
+void objcIdParameterWithProtocol(id i); // expected-warning {{pointer is missing a nullability type specifier}}
+// expected-note@-1 {{insert '_Nullable'}}
+// expected-note@-2 {{insert '_Nonnull'}}
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-3]]:46-[[@LINE-3]]:46}:" _Nullable"
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-4]]:46-[[@LINE-4]]:46}:" _Nonnull"
+
+// Class parameters don't have nullability type specifier.
+void objcParameterizedClassParameter(Container *c); // expected-warning {{pointer is missing a nullability type specifier}}
+// expected-note@-1 {{insert '_Nullable'}}
+// expected-note@-2 {{insert '_Nonnull'}}
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-3]]:57-[[@LINE-3]]:57}:" _Nullable "
+// CHECK: fix-it:"{{.*}}nullability-objc.h":{[[@LINE-4]]:57-[[@LINE-4]]:57}:" _Nonnull "
+
+// Class param

[PATCH] D38327: [Sema] Put nullability fix-it after the end of the pointer.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In https://reviews.llvm.org/D38327#882540, @jordan_rose wrote:

> Nice catch, Volodymyr! Looks good to me.


Thanks for the quick review, Jordan.


Repository:
  rL LLVM

https://reviews.llvm.org/D38327



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

[PATCH] D38773: [Sema] Add support for flexible array members in Obj-C.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.

Allow Obj-C ivars with incomplete array type but only as the last ivar.
Also add a requirement for ivars that contain a flexible array member to
be at the end of class too. It is possible to add in a subclass another
ivar at the end but we'll emit a warning in this case. Also we'll emit a
warning if a variable sized ivar is declared in class extension or in
implementation because subclasses won't know they should avoid adding
new ivars.

In ARC incomplete array objects are treated as __unsafe_unretained so
require them to be marked as such.

Prohibit synthesizing ivars with flexible array members because order of
synthesized ivars is not obvious and tricky to control. Spelling out
ivar explicitly gives control to developers and helps to avoid surprises
with unexpected ivar ordering.

For C and C++ changed diagnostic to tell explicitly a field is not the
last one and point to the next field. It is not as useful as in Obj-C
but it is an improvement and it is consistent with Obj-C. For C for
unions emit more specific err_flexible_array_union instead of generic
err_field_incomplete.

rdar://problem/21054495


https://reviews.llvm.org/D38773

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclObjC.cpp
  clang/lib/Sema/SemaObjCProperty.cpp
  clang/test/Sema/transparent-union.c
  clang/test/SemaCXX/flexible-array-test.cpp
  clang/test/SemaObjC/flexible-array-arc.m
  clang/test/SemaObjC/flexible-array.m
  clang/test/SemaObjC/ivar-sem-check-1.m
  clang/test/SemaObjCXX/flexible-array.mm

Index: clang/test/SemaObjCXX/flexible-array.mm
===
--- /dev/null
+++ clang/test/SemaObjCXX/flexible-array.mm
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
+
+// Test only flexible array member functionality specific to C++.
+
+union VariableSizeUnion {
+  int s;
+  char c[];
+};
+
+@interface LastUnionIvar {
+  VariableSizeUnion flexible;
+}
+@end
+
+@interface NotLastUnionIvar {
+  VariableSizeUnion flexible; // expected-error {{field 'flexible' with variable sized type 'VariableSizeUnion' is not at the end of class}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
+
+
+class VariableSizeClass {
+public:
+  int s;
+  char c[];
+};
+
+@interface LastClassIvar {
+  VariableSizeClass flexible;
+}
+@end
+
+@interface NotLastClassIvar {
+  VariableSizeClass flexible; // expected-error {{field 'flexible' with variable sized type 'VariableSizeClass' is not at the end of class}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
Index: clang/test/SemaObjC/ivar-sem-check-1.m
===
--- clang/test/SemaObjC/ivar-sem-check-1.m
+++ clang/test/SemaObjC/ivar-sem-check-1.m
@@ -6,14 +6,15 @@
 @interface INTF
 {
 	struct F {} JJ;
-	int arr[];  // expected-error {{field has incomplete type}}
+	int arr[];  // expected-error {{flexible array member 'arr' with type 'int []' is not at the end of class}}
 	struct S IC;  // expected-error {{field has incomplete type}}
+	  // expected-note@-1 {{next instance variable declaration is here}}
 	struct T { // expected-note {{previous definition is here}}
 	  struct T {} X;  // expected-error {{nested redefinition of 'T'}}
 	}YYY; 
 	FOOBADFUNC;  // expected-error {{field 'BADFUNC' declared as a function}}
 	int kaka;	// expected-note {{previous declaration is here}}
 	int kaka;	// expected-error {{duplicate member 'kaka'}}
-	char ch[];	// expected-error {{field has incomplete type}}
+	char ch[];
 }
 @end
Index: clang/test/SemaObjC/flexible-array.m
===
--- /dev/null
+++ clang/test/SemaObjC/flexible-array.m
@@ -0,0 +1,288 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
+
+// # Flexible array member.
+// ## Instance variables only in interface.
+@interface LastIvar {
+  char flexible[];
+}
+@end
+
+@interface NotLastIvar {
+  char flexible[]; // expected-error {{flexible array member 'flexible' with type 'char []' is not at the end of class}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
+
+// ## Instance variables in implementation.
+@interface LastIvarInImpl
+@end
+@implementation LastIvarInImpl {
+  char flexible[]; // expected-warning {{field 'flexible' with variable sized type 'char []' is not visible to subclasses and can conflict with their instance variables}}
+}
+@end
+
+@interface NotLastIvarInImpl
+@end
+@implementation NotLastIvarInImpl {
+  char flexible[]; // expected-error {{flexible array member 'flexible' with type 'char []' is not at the end of class}}
+  // expected-warning@-1 {{field 'flexible' with variable sized type 'char []' is not visible to subclasses and can conflict with their in

[PATCH] D38774: [CodeGen] Add support for IncompleteArrayType in Obj-C ivars.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.

Fixes an assertion failure when ivar is a struct containing incomplete
array. Also completes support for direct flexible array members.

rdar://problem/21054495


https://reviews.llvm.org/D38774

Files:
  clang/lib/CodeGen/CGObjCMac.cpp
  clang/test/CodeGenObjC/ivar-layout-flexible-array.m


Index: clang/test/CodeGenObjC/ivar-layout-flexible-array.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/ivar-layout-flexible-array.m
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -Wno-objc-root-class -fobjc-arc 
-emit-llvm -o - %s | FileCheck %s
+
+// rdar://problem/21054495
+@interface FlexibleArrayMember {
+  char flexible_array[];
+}
+@end
+@implementation FlexibleArrayMember
+@end
+// CHECK: @OBJC_METH_VAR_NAME_{{.*}} = private unnamed_addr constant {{.*}} 
c"flexible_array\00"
+// CHECK-NEXT: @OBJC_METH_VAR_TYPE_{{.*}} = private unnamed_addr constant 
{{.*}} c"^c\00"
+
+
+struct Packet {
+  int size;
+  char data[];
+};
+
+@interface VariableSizeIvar {
+  struct Packet flexible_struct;
+}
+@end
+@implementation VariableSizeIvar
+@end
+// CHECK: @OBJC_METH_VAR_NAME_{{.*}} = private unnamed_addr constant {{.*}} 
c"flexible_struct\00"
+// CHECK-NEXT: @OBJC_METH_VAR_TYPE_{{.*}} = private unnamed_addr constant 
{{.*}} c"{Packet=\22size\22i\22data\22[0c]}\00"
Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -5089,6 +5089,11 @@
 fieldType = arrayType->getElementType();
   }
 
+  if (isa(fieldType)) {
+numElts = 0;
+fieldType = fieldType->getAsArrayTypeUnsafe()->getElementType();
+  }
+
   assert(!fieldType->isArrayType() && "ivar of non-constant array type?");
 
   // If we ended up with a zero-sized array, we've done what we can do within


Index: clang/test/CodeGenObjC/ivar-layout-flexible-array.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/ivar-layout-flexible-array.m
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -Wno-objc-root-class -fobjc-arc -emit-llvm -o - %s | FileCheck %s
+
+// rdar://problem/21054495
+@interface FlexibleArrayMember {
+  char flexible_array[];
+}
+@end
+@implementation FlexibleArrayMember
+@end
+// CHECK: @OBJC_METH_VAR_NAME_{{.*}} = private unnamed_addr constant {{.*}} c"flexible_array\00"
+// CHECK-NEXT: @OBJC_METH_VAR_TYPE_{{.*}} = private unnamed_addr constant {{.*}} c"^c\00"
+
+
+struct Packet {
+  int size;
+  char data[];
+};
+
+@interface VariableSizeIvar {
+  struct Packet flexible_struct;
+}
+@end
+@implementation VariableSizeIvar
+@end
+// CHECK: @OBJC_METH_VAR_NAME_{{.*}} = private unnamed_addr constant {{.*}} c"flexible_struct\00"
+// CHECK-NEXT: @OBJC_METH_VAR_TYPE_{{.*}} = private unnamed_addr constant {{.*}} c"{Packet=\22size\22i\22data\22[0c]}\00"
Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -5089,6 +5089,11 @@
 fieldType = arrayType->getElementType();
   }
 
+  if (isa(fieldType)) {
+numElts = 0;
+fieldType = fieldType->getAsArrayTypeUnsafe()->getElementType();
+  }
+
   assert(!fieldType->isArrayType() && "ivar of non-constant array type?");
 
   // If we ended up with a zero-sized array, we've done what we can do within
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38773: [Sema] Add support for flexible array members in Obj-C.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Previous discussion on the mailing list can be found at 
http://lists.llvm.org/pipermail/cfe-dev/2017-September/055548.html The 
implementation is not exactly like it was discussed, it has some changes.

There is another case when extra ivar is added at the end, it is for bitfields. 
But bitfields aren't compatible with flexible array members so I decided not to 
include them in tests.

I separated CodeGen change in a separate patch that can be found here 
.


https://reviews.llvm.org/D38773



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

[PATCH] D38774: [CodeGen] Add support for IncompleteArrayType in Obj-C ivars.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

This patch depends on sema change https://reviews.llvm.org/D38773


https://reviews.llvm.org/D38774



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

[PATCH] D38774: [CodeGen] Add support for IncompleteArrayType in Obj-C ivars.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

To save reviewers some time. Incomplete array type is not the same as for 
zero-sized array, it is `c"^c\00"` while for zero-sized array it is 
`c"[0c]\00"`. Don't know if it matters, I haven't found any difference in 
behaviour. Though maybe I wasn't looking in the right place.


https://reviews.llvm.org/D38774



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

[PATCH] D38773: [Sema] Add support for flexible array members in Obj-C.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5226
+def err_objc_variable_sized_type_not_at_end : Error<
+  "field %0 with variable sized type %1 is not at the end of class">;
+def note_next_field_declaration : Note<

rjmccall wrote:
> "Variable sized type" is a bit too close to the C99 variably-sized array type 
> extension.  Maybe "unbounded array type" if you're trying to cover both "int 
> x[];" and "int x[0];"?
> 
> Well, I guess there's some precedent for using this terminology, but ugh.
I took "variable sized type" entirely from

```
def ext_variable_sized_type_in_struct : ExtWarn<
  "field %0 with variable sized type %1 not at the end of a struct or class is"
  " a GNU extension">, InGroup;
```

I'm not covering `int x[0];`. All the changes are for `int x[];` and `struct { 
int x[]; }`



Comment at: clang/lib/Sema/SemaDecl.cpp:15055
   }
+  // If it is the last field is checked elsewhere.
 }

rjmccall wrote:
> "Whether" rather than "If", please.  You should also leave a comment about 
> *why* we can't check this here — I assume because you also want to complain 
> about the last explicit ivar if there are synthesized ivars?  I think we 
> could at least still check this for `@interface` ivars.
Will change s/If/Whether/

Main reason for checking elsewhere is to check after ivars are synthesized, you 
are right. At some point I had this check done here but for detecting 
ivar-after-flexible-array on interface/extension, interface/implementation 
border I am relying on chained ObjCIvarDecl. But here I have `ArrayRef 
Fields` so implementation will be different. I decided that it would be cleaner 
to perform the check only in DiagnoseVariableSizedIvars.


https://reviews.llvm.org/D38773



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

[PATCH] D38773: [Sema] Add support for flexible array members in Obj-C.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:15055
   }
+  // If it is the last field is checked elsewhere.
 }

rjmccall wrote:
> vsapsai wrote:
> > rjmccall wrote:
> > > "Whether" rather than "If", please.  You should also leave a comment 
> > > about *why* we can't check this here — I assume because you also want to 
> > > complain about the last explicit ivar if there are synthesized ivars?  I 
> > > think we could at least still check this for `@interface` ivars.
> > Will change s/If/Whether/
> > 
> > Main reason for checking elsewhere is to check after ivars are synthesized, 
> > you are right. At some point I had this check done here but for detecting 
> > ivar-after-flexible-array on interface/extension, interface/implementation 
> > border I am relying on chained ObjCIvarDecl. But here I have `ArrayRef > *> Fields` so implementation will be different. I decided that it would be 
> > cleaner to perform the check only in DiagnoseVariableSizedIvars.
> Is there a reason to do any of the checking here, then?
No objective reason. I updated isIncompleteArrayType branch to avoid flexible 
array members rejected at line 15023

```lang=c++
} else if (!FDTy->isDependentType() &&
   RequireCompleteType(FD->getLocation(), FD->getType(),
   diag::err_field_incomplete)) {
```

and here I've added it for consistency. Will move to DiagnoseVariableSizedIvars 
and see if it works fine, don't expect any problems.


https://reviews.llvm.org/D38773



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

[PATCH] D38773: [Sema] Add support for flexible array members in Obj-C.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 118990.
vsapsai added a comment.

- Address rjmccall review comment, move warn_variable_sized_ivar_visibility to 
DiagnoseVariableSizedIvars.


https://reviews.llvm.org/D38773

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclObjC.cpp
  clang/lib/Sema/SemaObjCProperty.cpp
  clang/test/Sema/transparent-union.c
  clang/test/SemaCXX/flexible-array-test.cpp
  clang/test/SemaObjC/flexible-array-arc.m
  clang/test/SemaObjC/flexible-array.m
  clang/test/SemaObjC/ivar-sem-check-1.m
  clang/test/SemaObjCXX/flexible-array.mm

Index: clang/test/SemaObjCXX/flexible-array.mm
===
--- /dev/null
+++ clang/test/SemaObjCXX/flexible-array.mm
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
+
+// Test only flexible array member functionality specific to C++.
+
+union VariableSizeUnion {
+  int s;
+  char c[];
+};
+
+@interface LastUnionIvar {
+  VariableSizeUnion flexible;
+}
+@end
+
+@interface NotLastUnionIvar {
+  VariableSizeUnion flexible; // expected-error {{field 'flexible' with variable sized type 'VariableSizeUnion' is not at the end of class}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
+
+
+class VariableSizeClass {
+public:
+  int s;
+  char c[];
+};
+
+@interface LastClassIvar {
+  VariableSizeClass flexible;
+}
+@end
+
+@interface NotLastClassIvar {
+  VariableSizeClass flexible; // expected-error {{field 'flexible' with variable sized type 'VariableSizeClass' is not at the end of class}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
Index: clang/test/SemaObjC/ivar-sem-check-1.m
===
--- clang/test/SemaObjC/ivar-sem-check-1.m
+++ clang/test/SemaObjC/ivar-sem-check-1.m
@@ -6,14 +6,15 @@
 @interface INTF
 {
 	struct F {} JJ;
-	int arr[];  // expected-error {{field has incomplete type}}
+	int arr[];  // expected-error {{flexible array member 'arr' with type 'int []' is not at the end of class}}
 	struct S IC;  // expected-error {{field has incomplete type}}
+	  // expected-note@-1 {{next instance variable declaration is here}}
 	struct T { // expected-note {{previous definition is here}}
 	  struct T {} X;  // expected-error {{nested redefinition of 'T'}}
 	}YYY; 
 	FOOBADFUNC;  // expected-error {{field 'BADFUNC' declared as a function}}
 	int kaka;	// expected-note {{previous declaration is here}}
 	int kaka;	// expected-error {{duplicate member 'kaka'}}
-	char ch[];	// expected-error {{field has incomplete type}}
+	char ch[];
 }
 @end
Index: clang/test/SemaObjC/flexible-array.m
===
--- /dev/null
+++ clang/test/SemaObjC/flexible-array.m
@@ -0,0 +1,288 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
+
+// # Flexible array member.
+// ## Instance variables only in interface.
+@interface LastIvar {
+  char flexible[];
+}
+@end
+
+@interface NotLastIvar {
+  char flexible[]; // expected-error {{flexible array member 'flexible' with type 'char []' is not at the end of class}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
+
+// ## Instance variables in implementation.
+@interface LastIvarInImpl
+@end
+@implementation LastIvarInImpl {
+  char flexible[]; // expected-warning {{field 'flexible' with variable sized type 'char []' is not visible to subclasses and can conflict with their instance variables}}
+}
+@end
+
+@interface NotLastIvarInImpl
+@end
+@implementation NotLastIvarInImpl {
+  char flexible[]; // expected-error {{flexible array member 'flexible' with type 'char []' is not at the end of class}}
+  // expected-warning@-1 {{field 'flexible' with variable sized type 'char []' is not visible to subclasses and can conflict with their instance variables}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
+
+@implementation NotLastIvarInImplWithoutInterface { // expected-warning {{cannot find interface declaration for 'NotLastIvarInImplWithoutInterface'}}
+  char flexible[]; // expected-error {{flexible array member 'flexible' with type 'char []' is not at the end of class}}
+  // expected-warning@-1 {{field 'flexible' with variable sized type 'char []' is not visible to subclasses and can conflict with their instance variables}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
+
+@interface LastIvarInClass_OtherIvarInImpl {
+  char flexible[]; // expected-error {{flexible array member 'flexible' with type 'char []' is not at the end of class}}
+}
+@end
+@implementation LastIvarInClass_OtherIvarInImpl {
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
+
+// ## Non-instance variables i

[PATCH] D39133: [Sema] Better fix for tags defined inside an enumeration (PR28903).

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.

When we encounter an opening parenthesis and parse the rest as type
cast, use DeclSpecContext DSC_type_specifier so we hit the existing check
that prevents defining tags in contexts where type specifier is expected.

This reverts implementation done in r313386, r313894, and keeps the tests.

rdar://problem/28530809


https://reviews.llvm.org/D39133

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CXX/drs/dr5xx.cpp
  clang/test/SemaCXX/enum.cpp


Index: clang/test/SemaCXX/enum.cpp
===
--- clang/test/SemaCXX/enum.cpp
+++ clang/test/SemaCXX/enum.cpp
@@ -114,7 +114,7 @@
 // PR28903
 struct PR28903 {
   enum {
-PR28903_A = (enum { // expected-error-re {{'PR28903::(anonymous enum at 
{{.*}})' cannot be defined in an enumeration}}
+PR28903_A = (enum { // expected-error-re {{'PR28903::(anonymous enum at 
{{.*}})' cannot be defined in a type specifier}}
   PR28903_B,
   PR28903_C = PR28903_B
 })
Index: clang/test/CXX/drs/dr5xx.cpp
===
--- clang/test/CXX/drs/dr5xx.cpp
+++ clang/test/CXX/drs/dr5xx.cpp
@@ -424,7 +424,7 @@
   while (const n = 0) {} // expected-error {{requires a type specifier}}
   for (const n = 0; // expected-error {{requires a type specifier}}
const m = 0; ) {} // expected-error {{requires a type specifier}}
-  sizeof(const); // expected-error {{requires a type specifier}}
+  sizeof(const); // expected-error {{expected a type}}
   struct S {
 const n; // expected-error {{requires a type specifier}}
 operator const(); // expected-error {{expected a type}}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -14012,13 +14012,6 @@
 Invalid = true;
   }
 
-  if (!Invalid && getLangOpts().CPlusPlus && TUK == TUK_Definition &&
-  DC->getDeclKind() == Decl::Enum) {
-Diag(New->getLocation(), diag::err_type_defined_in_enum)
-  << Context.getTagDeclType(New);
-Invalid = true;
-  }
-
   // Maybe add qualifier info.
   if (SS.isNotEmpty()) {
 if (SS.isSet()) {
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2373,7 +2373,7 @@
 
 // Parse the type declarator.
 DeclSpec DS(AttrFactory);
-ParseSpecifierQualifierList(DS);
+ParseSpecifierQualifierList(DS, AS_none, DSC_type_specifier);
 Declarator DeclaratorInfo(DS, Declarator::TypeNameContext);
 ParseDeclarator(DeclaratorInfo);
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1349,8 +1349,6 @@
   "%0 cannot be defined in a type alias template">;
 def err_type_defined_in_condition : Error<
   "%0 cannot be defined in a condition">;
-def err_type_defined_in_enum : Error<
-  "%0 cannot be defined in an enumeration">;
 
 def note_pure_virtual_function : Note<
   "unimplemented pure virtual method %0 in %1">;


Index: clang/test/SemaCXX/enum.cpp
===
--- clang/test/SemaCXX/enum.cpp
+++ clang/test/SemaCXX/enum.cpp
@@ -114,7 +114,7 @@
 // PR28903
 struct PR28903 {
   enum {
-PR28903_A = (enum { // expected-error-re {{'PR28903::(anonymous enum at {{.*}})' cannot be defined in an enumeration}}
+PR28903_A = (enum { // expected-error-re {{'PR28903::(anonymous enum at {{.*}})' cannot be defined in a type specifier}}
   PR28903_B,
   PR28903_C = PR28903_B
 })
Index: clang/test/CXX/drs/dr5xx.cpp
===
--- clang/test/CXX/drs/dr5xx.cpp
+++ clang/test/CXX/drs/dr5xx.cpp
@@ -424,7 +424,7 @@
   while (const n = 0) {} // expected-error {{requires a type specifier}}
   for (const n = 0; // expected-error {{requires a type specifier}}
const m = 0; ) {} // expected-error {{requires a type specifier}}
-  sizeof(const); // expected-error {{requires a type specifier}}
+  sizeof(const); // expected-error {{expected a type}}
   struct S {
 const n; // expected-error {{requires a type specifier}}
 operator const(); // expected-error {{expected a type}}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -14012,13 +14012,6 @@
 Invalid = true;
   }
 
-  if (!Invalid && getLangOpts().CPlusPlus && TUK == TUK_Definition &&
-  DC->getDeclKind() == Decl::Enum) {
-Diag(New->getLocation(), diag::err_type_defined_in_enum)
-  << Context.getTagDeclType(New);
-Inva

[PATCH] D38773: [Sema] Add support for flexible array members in Obj-C.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL316381: [Sema] Add support for flexible array members in 
Obj-C. (authored by vsapsai).

Changed prior to commit:
  https://reviews.llvm.org/D38773?vs=118990&id=119943#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D38773

Files:
  cfe/trunk/include/clang/Basic/DiagnosticGroups.td
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/Sema/SemaDecl.cpp
  cfe/trunk/lib/Sema/SemaDeclObjC.cpp
  cfe/trunk/lib/Sema/SemaObjCProperty.cpp
  cfe/trunk/test/Sema/transparent-union.c
  cfe/trunk/test/SemaCXX/flexible-array-test.cpp
  cfe/trunk/test/SemaObjC/flexible-array-arc.m
  cfe/trunk/test/SemaObjC/flexible-array.m
  cfe/trunk/test/SemaObjC/ivar-sem-check-1.m
  cfe/trunk/test/SemaObjCXX/flexible-array.mm

Index: cfe/trunk/lib/Sema/SemaObjCProperty.cpp
===
--- cfe/trunk/lib/Sema/SemaObjCProperty.cpp
+++ cfe/trunk/lib/Sema/SemaObjCProperty.cpp
@@ -1290,6 +1290,14 @@
 // An abstract type is as bad as an incomplete type.
 CompleteTypeErr = true;
   }
+  if (!CompleteTypeErr) {
+const RecordType *RecordTy = PropertyIvarType->getAs();
+if (RecordTy && RecordTy->getDecl()->hasFlexibleArrayMember()) {
+  Diag(PropertyIvarLoc, diag::err_synthesize_variable_sized_ivar)
+<< PropertyIvarType;
+  CompleteTypeErr = true; // suppress later diagnostics about the ivar
+}
+  }
   if (CompleteTypeErr)
 Ivar->setInvalidDecl();
   ClassImpDecl->addDecl(Ivar);
Index: cfe/trunk/lib/Sema/SemaDecl.cpp
===
--- cfe/trunk/lib/Sema/SemaDecl.cpp
+++ cfe/trunk/lib/Sema/SemaDecl.cpp
@@ -14997,67 +14997,78 @@
 //   possibly recursively, a member that is such a structure)
 //   shall not be a member of a structure or an element of an
 //   array.
+bool IsLastField = (i + 1 == Fields.end());
 if (FDTy->isFunctionType()) {
   // Field declared as a function.
   Diag(FD->getLocation(), diag::err_field_declared_as_function)
 << FD->getDeclName();
   FD->setInvalidDecl();
   EnclosingDecl->setInvalidDecl();
   continue;
-} else if (FDTy->isIncompleteArrayType() && Record &&
-   ((i + 1 == Fields.end() && !Record->isUnion()) ||
-((getLangOpts().MicrosoftExt ||
-  getLangOpts().CPlusPlus) &&
- (i + 1 == Fields.end() || Record->isUnion() {
-  // Flexible array member.
-  // Microsoft and g++ is more permissive regarding flexible array.
-  // It will accept flexible array in union and also
-  // as the sole element of a struct/class.
-  unsigned DiagID = 0;
-  if (Record->isUnion())
-DiagID = getLangOpts().MicrosoftExt
- ? diag::ext_flexible_array_union_ms
- : getLangOpts().CPlusPlus
-   ? diag::ext_flexible_array_union_gnu
-   : diag::err_flexible_array_union;
-  else if (NumNamedMembers < 1)
-DiagID = getLangOpts().MicrosoftExt
- ? diag::ext_flexible_array_empty_aggregate_ms
- : getLangOpts().CPlusPlus
-   ? diag::ext_flexible_array_empty_aggregate_gnu
-   : diag::err_flexible_array_empty_aggregate;
-
-  if (DiagID)
-Diag(FD->getLocation(), DiagID) << FD->getDeclName()
-<< Record->getTagKind();
-  // While the layout of types that contain virtual bases is not specified
-  // by the C++ standard, both the Itanium and Microsoft C++ ABIs place
-  // virtual bases after the derived members.  This would make a flexible
-  // array member declared at the end of an object not adjacent to the end
-  // of the type.
-  if (CXXRecordDecl *RD = dyn_cast(Record))
-if (RD->getNumVBases() != 0)
-  Diag(FD->getLocation(), diag::err_flexible_array_virtual_base)
+} else if (FDTy->isIncompleteArrayType() &&
+   (Record || isa(EnclosingDecl))) {
+  if (Record) {
+// Flexible array member.
+// Microsoft and g++ is more permissive regarding flexible array.
+// It will accept flexible array in union and also
+// as the sole element of a struct/class.
+unsigned DiagID = 0;
+if (!Record->isUnion() && !IsLastField) {
+  Diag(FD->getLocation(), diag::err_flexible_array_not_at_end)
+<< FD->getDeclName() << FD->getType() << Record->getTagKind();
+  Diag((*(i + 1))->getLocation(), diag::note_next_field_declaration);
+  FD->setInvalidDecl();
+  EnclosingDecl->setInvalidDecl();
+  continue;
+} else if (Record->isUnion())
+  DiagID = getLangOpts().MicrosoftExt
+   ? diag::ext_fle

[PATCH] D38774: [CodeGen] Add support for IncompleteArrayType in Obj-C ivars.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/lib/CodeGen/CGObjCMac.cpp:5095
+fieldType = fieldType->getAsArrayTypeUnsafe()->getElementType();
+  }
+

rjmccall wrote:
> You can't just use isa<> here; there can be typedefs of incomplete array type.
Thanks for catching it.


https://reviews.llvm.org/D38774



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

[PATCH] D38774: [CodeGen] Add support for IncompleteArrayType in Obj-C ivars.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 119951.
vsapsai added a comment.

- Address rjmccall review comment about isa<>.

Decided to keep in test only cases with typedefs because test coverage is the
same and there is less similar code.


https://reviews.llvm.org/D38774

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGObjCMac.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclObjC.cpp
  clang/lib/Sema/SemaObjCProperty.cpp
  clang/test/CodeGenObjC/ivar-layout-flexible-array.m
  clang/test/Sema/transparent-union.c
  clang/test/SemaCXX/flexible-array-test.cpp
  clang/test/SemaObjC/flexible-array-arc.m
  clang/test/SemaObjC/flexible-array.m
  clang/test/SemaObjC/ivar-sem-check-1.m
  clang/test/SemaObjCXX/flexible-array.mm

Index: clang/test/SemaObjCXX/flexible-array.mm
===
--- /dev/null
+++ clang/test/SemaObjCXX/flexible-array.mm
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
+
+// Test only flexible array member functionality specific to C++.
+
+union VariableSizeUnion {
+  int s;
+  char c[];
+};
+
+@interface LastUnionIvar {
+  VariableSizeUnion flexible;
+}
+@end
+
+@interface NotLastUnionIvar {
+  VariableSizeUnion flexible; // expected-error {{field 'flexible' with variable sized type 'VariableSizeUnion' is not at the end of class}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
+
+
+class VariableSizeClass {
+public:
+  int s;
+  char c[];
+};
+
+@interface LastClassIvar {
+  VariableSizeClass flexible;
+}
+@end
+
+@interface NotLastClassIvar {
+  VariableSizeClass flexible; // expected-error {{field 'flexible' with variable sized type 'VariableSizeClass' is not at the end of class}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
Index: clang/test/SemaObjC/ivar-sem-check-1.m
===
--- clang/test/SemaObjC/ivar-sem-check-1.m
+++ clang/test/SemaObjC/ivar-sem-check-1.m
@@ -6,14 +6,15 @@
 @interface INTF
 {
 	struct F {} JJ;
-	int arr[];  // expected-error {{field has incomplete type}}
+	int arr[];  // expected-error {{flexible array member 'arr' with type 'int []' is not at the end of class}}
 	struct S IC;  // expected-error {{field has incomplete type}}
+	  // expected-note@-1 {{next instance variable declaration is here}}
 	struct T { // expected-note {{previous definition is here}}
 	  struct T {} X;  // expected-error {{nested redefinition of 'T'}}
 	}YYY; 
 	FOOBADFUNC;  // expected-error {{field 'BADFUNC' declared as a function}}
 	int kaka;	// expected-note {{previous declaration is here}}
 	int kaka;	// expected-error {{duplicate member 'kaka'}}
-	char ch[];	// expected-error {{field has incomplete type}}
+	char ch[];
 }
 @end
Index: clang/test/SemaObjC/flexible-array.m
===
--- /dev/null
+++ clang/test/SemaObjC/flexible-array.m
@@ -0,0 +1,288 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
+
+// # Flexible array member.
+// ## Instance variables only in interface.
+@interface LastIvar {
+  char flexible[];
+}
+@end
+
+@interface NotLastIvar {
+  char flexible[]; // expected-error {{flexible array member 'flexible' with type 'char []' is not at the end of class}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
+
+// ## Instance variables in implementation.
+@interface LastIvarInImpl
+@end
+@implementation LastIvarInImpl {
+  char flexible[]; // expected-warning {{field 'flexible' with variable sized type 'char []' is not visible to subclasses and can conflict with their instance variables}}
+}
+@end
+
+@interface NotLastIvarInImpl
+@end
+@implementation NotLastIvarInImpl {
+  char flexible[]; // expected-error {{flexible array member 'flexible' with type 'char []' is not at the end of class}}
+  // expected-warning@-1 {{field 'flexible' with variable sized type 'char []' is not visible to subclasses and can conflict with their instance variables}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
+
+@implementation NotLastIvarInImplWithoutInterface { // expected-warning {{cannot find interface declaration for 'NotLastIvarInImplWithoutInterface'}}
+  char flexible[]; // expected-error {{flexible array member 'flexible' with type 'char []' is not at the end of class}}
+  // expected-warning@-1 {{field 'flexible' with variable sized type 'char []' is not visible to subclasses and can conflict with their instance variables}}
+  int last; // expected-note {{next instance variable declaration is here}}
+}
+@end
+
+@interface LastIvarInClass_OtherIvarInImpl {
+  char flexible[]; // expected-error {{flexible array member 'flexible' with type 'char []' is not at the end of class}}
+}
+@end
+@implementation LastIvarIn

[PATCH] D38774: [CodeGen] Add support for IncompleteArrayType in Obj-C ivars.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 119952.
vsapsai added a comment.

- Resubmit my last change without files from underlying branch.


https://reviews.llvm.org/D38774

Files:
  clang/lib/CodeGen/CGObjCMac.cpp
  clang/test/CodeGenObjC/ivar-layout-flexible-array.m


Index: clang/test/CodeGenObjC/ivar-layout-flexible-array.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/ivar-layout-flexible-array.m
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -Wno-objc-root-class -fobjc-arc 
-emit-llvm -o - %s | FileCheck %s
+
+// rdar://problem/21054495
+typedef char FlexibleArray[];
+
+@interface FlexibleArrayMember {
+  FlexibleArray flexible_array;
+}
+@end
+@implementation FlexibleArrayMember
+@end
+// CHECK: @OBJC_METH_VAR_NAME_{{.*}} = private unnamed_addr constant {{.*}} 
c"flexible_array\00"
+// CHECK-NEXT: @OBJC_METH_VAR_TYPE_{{.*}} = private unnamed_addr constant 
{{.*}} c"^c\00"
+
+
+struct Packet {
+  int size;
+  FlexibleArray data;
+};
+
+@interface VariableSizeIvar {
+  struct Packet flexible_struct;
+}
+@end
+@implementation VariableSizeIvar
+@end
+// CHECK: @OBJC_METH_VAR_NAME_{{.*}} = private unnamed_addr constant {{.*}} 
c"flexible_struct\00"
+// CHECK-NEXT: @OBJC_METH_VAR_TYPE_{{.*}} = private unnamed_addr constant 
{{.*}} c"{Packet=\22size\22i\22data\22[0c]}\00"
Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -5089,6 +5089,11 @@
 fieldType = arrayType->getElementType();
   }
 
+  if (auto arrayType = CGM.getContext().getAsIncompleteArrayType(fieldType)) {
+numElts = 0;
+fieldType = arrayType->getElementType();
+  }
+
   assert(!fieldType->isArrayType() && "ivar of non-constant array type?");
 
   // If we ended up with a zero-sized array, we've done what we can do within


Index: clang/test/CodeGenObjC/ivar-layout-flexible-array.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/ivar-layout-flexible-array.m
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -Wno-objc-root-class -fobjc-arc -emit-llvm -o - %s | FileCheck %s
+
+// rdar://problem/21054495
+typedef char FlexibleArray[];
+
+@interface FlexibleArrayMember {
+  FlexibleArray flexible_array;
+}
+@end
+@implementation FlexibleArrayMember
+@end
+// CHECK: @OBJC_METH_VAR_NAME_{{.*}} = private unnamed_addr constant {{.*}} c"flexible_array\00"
+// CHECK-NEXT: @OBJC_METH_VAR_TYPE_{{.*}} = private unnamed_addr constant {{.*}} c"^c\00"
+
+
+struct Packet {
+  int size;
+  FlexibleArray data;
+};
+
+@interface VariableSizeIvar {
+  struct Packet flexible_struct;
+}
+@end
+@implementation VariableSizeIvar
+@end
+// CHECK: @OBJC_METH_VAR_NAME_{{.*}} = private unnamed_addr constant {{.*}} c"flexible_struct\00"
+// CHECK-NEXT: @OBJC_METH_VAR_TYPE_{{.*}} = private unnamed_addr constant {{.*}} c"{Packet=\22size\22i\22data\22[0c]}\00"
Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -5089,6 +5089,11 @@
 fieldType = arrayType->getElementType();
   }
 
+  if (auto arrayType = CGM.getContext().getAsIncompleteArrayType(fieldType)) {
+numElts = 0;
+fieldType = arrayType->getElementType();
+  }
+
   assert(!fieldType->isArrayType() && "ivar of non-constant array type?");
 
   // If we ended up with a zero-sized array, we've done what we can do within
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51867: [Diagnostics][NFC] Add error handling to FormatDiagnostic()

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Regarding the asserts to catch potential problems, seems most of them are for 
buffer overflows. Aren't sanitizers catching those cases, specifically Address 
Sanitizer? I haven't checked, just seems it would be good to check buffer 
overflow automatically instead of using explicit asserts.

Also there are a few changes I wouldn't call NFC. Those change loop iteration 
from "iterator != end" to "iterator < end". As it is functionality change, I'd 
like to have tests to cover that. Also I've fixed a few bugs with going past 
the end of buffer and bugs were actually inside the loop, not with buffer range 
check. It is tempting to play safe but it has a risk of hiding real bugs.




Comment at: lib/Basic/Diagnostic.cpp:804
+
+  while (DiagStr < DiagEnd) {
 if (DiagStr[0] != '%') {

For example, I wouldn't call this one NFC.


Repository:
  rC Clang

https://reviews.llvm.org/D51867



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

[PATCH] D50539: [VFS] Add property 'fallthrough' that controls fallback to real file system.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Rebased on top of trunk and checked that this is still working. Please take a 
look.


https://reviews.llvm.org/D50539



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

[PATCH] D50462: Try building complete AST after a fatal error was emitted if further diagnostics are expected

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Agree that fatal/non-fatal error is too coarse and tooling/IDEs need more 
details and more control to provide better experience. But I don't think we are 
in a state to claim that all errors are recoverable (in theory and in current 
implementation). Instead of continuing on all errors, I prefer to select errors 
that are important for tooling and improve those first.

Regarding the current patch, I don't like creating coupling between 
`hasFatalErrorOccurred` and `shouldRecoverAfterFatalErrors`. Looks like after 
this patch you'll need to call these methods together in many cases. For 
example, probably `Sema::makeTypoCorrectionConsumer` in

  if (Diags.hasFatalErrorOccurred() || !getLangOpts().SpellChecking ||
  DisableTypoCorrection)
return nullptr;

should check `shouldRecoverAfterFatalErrors` too.


Repository:
  rC Clang

https://reviews.llvm.org/D50462



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

[PATCH] D30882: Add a callback for __has_include and use it for dependency scanning

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai commandeered this revision.
vsapsai added a reviewer: pete.
vsapsai added a comment.

Taking over the change, I'll address the reviewers' comments.


https://reviews.llvm.org/D30882



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

[PATCH] D30882: Add a callback for __has_include and use it for dependency scanning

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 165610.
vsapsai added a comment.

- Improve tests, fix -MMD, address comments.


https://reviews.llvm.org/D30882

Files:
  clang/include/clang/Lex/PPCallbacks.h
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Lex/PPMacroExpansion.cpp
  clang/test/Frontend/dependency-gen-has-include.c

Index: clang/test/Frontend/dependency-gen-has-include.c
===
--- /dev/null
+++ clang/test/Frontend/dependency-gen-has-include.c
@@ -0,0 +1,40 @@
+// REQUIRES: shell
+
+// Basic test
+// RUN: rm -rf %t.dir
+// RUN: mkdir %t.dir
+// RUN: mkdir %t.dir/a
+// RUN: echo "#ifndef HEADER_A" > %t.dir/a/header.h
+// RUN: echo "#define HEADER_A" >> %t.dir/a/header.h
+// RUN: echo "#endif" >> %t.dir/a/header.h
+// RUN: mkdir %t.dir/system
+// RUN: echo "#define SYSTEM_HEADER" > %t.dir/system/system-header.h
+// RUN: mkdir %t.dir/next-a
+// RUN: echo "#if __has_include_next()" > %t.dir/next-a/next-header.h
+// RUN: echo "#endif" >> %t.dir/next-a/next-header.h
+// RUN: mkdir %t.dir/next-b
+// RUN: echo "#define NEXT_HEADER" > %t.dir/next-b/next-header.h
+
+// RUN: %clang -MD -MF %t.dir/file.deps %s -fsyntax-only -I %t.dir -isystem %t.dir/system -I %t.dir/next-a -I %t.dir/next-b
+// RUN: FileCheck -input-file=%t.dir/file.deps %s
+// CHECK: dependency-gen-has-include.o
+// CHECK: dependency-gen-has-include.c
+// CHECK: a/header.h
+// CHECK-NOT: missing/file.h
+// CHECK: system/system-header.h
+// CHECK: next-a/next-header.h
+// CHECK: next-b/next-header.h
+
+// Verify that we ignore system headers in user-only headers mode.
+// RUN: %clang -MMD -MF %t.dir/user-headers.deps %s -fsyntax-only -I %t.dir -isystem %t.dir/system -I %t.dir/next-a -I %t.dir/next-b
+// RUN: FileCheck -input-file=%t.dir/user-headers.deps --check-prefix CHECK-USER-HEADER %s
+// CHECK-USER-HEADER-NOT: system/system-header.h
+
+#if __has_include("a/header.h")
+#endif
+#if __has_include("missing/file.h")
+#endif
+#if __has_include()
+#endif
+
+#include 
Index: clang/lib/Lex/PPMacroExpansion.cpp
===
--- clang/lib/Lex/PPMacroExpansion.cpp
+++ clang/lib/Lex/PPMacroExpansion.cpp
@@ -23,6 +23,7 @@
 #include "clang/Lex/CodeCompletionHandler.h"
 #include "clang/Lex/DirectoryLookup.h"
 #include "clang/Lex/ExternalPreprocessorSource.h"
+#include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/LexDiagnostic.h"
 #include "clang/Lex/MacroArgs.h"
 #include "clang/Lex/MacroInfo.h"
@@ -1242,6 +1243,13 @@
   PP.LookupFile(FilenameLoc, Filename, isAngled, LookupFrom, LookupFromFile,
 CurDir, nullptr, nullptr, nullptr, nullptr);
 
+  if (PPCallbacks *Callbacks = PP.getPPCallbacks()) {
+SrcMgr::CharacteristicKind FileType = SrcMgr::C_User;
+if (File)
+  FileType = PP.getHeaderSearchInfo().getFileDirFlavor(File);
+Callbacks->HasInclude(FilenameLoc, Filename, isAngled, File, FileType);
+  }
+
   // Get the result value.  A result of true means the file exists.
   return File != nullptr;
 }
Index: clang/lib/Frontend/DependencyFile.cpp
===
--- clang/lib/Frontend/DependencyFile.cpp
+++ clang/lib/Frontend/DependencyFile.cpp
@@ -200,6 +200,10 @@
   const Module *Imported,
   SrcMgr::CharacteristicKind FileType) override;
 
+  void HasInclude(SourceLocation Loc, StringRef SpelledFilename, bool IsAngled,
+  const FileEntry *File,
+  SrcMgr::CharacteristicKind FileType) override;
+
   void EndOfMainFile() override {
 OutputDependencyFile();
   }
@@ -328,6 +332,17 @@
   }
 }
 
+void DFGImpl::HasInclude(SourceLocation Loc, StringRef SpelledFilename,
+ bool IsAngled, const FileEntry *File,
+ SrcMgr::CharacteristicKind FileType) {
+  if (!File)
+return;
+  StringRef Filename = File->getName();
+  if (!FileMatchesDepCriteria(Filename.data(), FileType))
+return;
+  AddFilename(llvm::sys::path::remove_leading_dotslash(Filename));
+}
+
 bool DFGImpl::AddFilename(StringRef Filename) {
   if (FilesSet.insert(Filename).second) {
 Files.push_back(Filename);
Index: clang/include/clang/Lex/PPCallbacks.h
===
--- clang/include/clang/Lex/PPCallbacks.h
+++ clang/include/clang/Lex/PPCallbacks.h
@@ -276,6 +276,12 @@
SourceRange Range) {
   }
 
+  /// Hook called when a '__has_include' or '__has_include_next' directive is
+  /// read.
+  virtual void HasInclude(SourceLocation Loc, StringRef FileName, bool IsAngled,
+  const FileEntry *File,
+  SrcMgr::CharacteristicKind FileType) {}
+
   /// Hook called when a source range is skipped.
   /// \param Range The SourceRange that was skipped. The range begins at the
   /// \#if/\#else directive and ends after the \#endif/\#else direct

[PATCH] D30882: Add a callback for __has_include and use it for dependency scanning

[Volodymyr Sapsai via Phabricator via cfe-commits]

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



Comment at: include/clang/Lex/PPCallbacks.h:263
+  /// \brief Callback invoked when a has_include directive is read.
+  virtual void HasInclude(SourceLocation Loc, const FileEntry *File) {
+  }

rsmith wrote:
> This callback seems pretty unhelpful in the case where lookup of the file 
> failed (you just get a source location). Could we pass in the `FileName` and 
> `IsAngled` flag too (like we do for `InclusionDirective`)? I think that's 
> what (eg) a callback tracking anti-dependencies (for a more sophisticated 
> build system that can handle them) would want.
Added. Also added `FileType` so we can ignore system headers for `-MMD`. And 
tweaked doxygen comment to be more consistent with other callbacks.



Comment at: test/Frontend/dependency-gen-has-include.c:11-15
+// RUN: %clang -MD -MF %t.dir/file.deps %s -fsyntax-only -I %t.dir
+// RUN: FileCheck -input-file=%t.dir/file.deps %s
+// CHECK: dependency-gen-has-include.o
+// CHECK: dependency-gen-has-include.c
+// CHECK: a/header.h

dexonsmith wrote:
> This covers quoted includes when there is a single `-I`.  I think there are a 
> couple of other cases worth testing:
> - Multiple `-I` arguments: all should be listed.
> - Angle includes: all `-isystem` should be listed too.
> - Also `-F` and `-iframework`.
I've added a test with system headers. Overall, my testing strategy was to test 
all cases for the following dimensions:
* file exists/file doesn't exist;
* file is user/system header;
* `__has_include`/`__has_include_next` is used.

But I don't test the full matrix as I don't think it provides enough value.



Comment at: test/Frontend/dependency-gen-has-include.c:17
+#if __has_include("a/header.h")
+#endif

bruno wrote:
> Can you also add a testcase for `__has_include_next`?
Added.


https://reviews.llvm.org/D30882



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

[PATCH] D30882: Add a callback for __has_include and use it for dependency scanning

[Volodymyr Sapsai via Phabricator via cfe-commits]

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



Comment at: lib/Frontend/DependencyFile.cpp:325
+void DFGImpl::HasInclude(SourceLocation Loc, const FileEntry *File) {
+  if (!File)
+return;

rsmith wrote:
> Have you thought about whether we should add a dependency even for a missing 
> file under `-MG` (`AddMissingHeaderDeps`) mode? I think it's probably better 
> to not do so (ie, the behavior in this patch), but it seems worth considering.
Do you know how Make uses these missing files? Or maybe where I can find more. 
The only documentation I found says

> This feature is used in automatic updating of makefiles.

Which is not particularly illuminating.

Currently I prefer not to include not found `__has_include` files because they 
aren't really missing, it's OK if they aren't there and nothing has to be done 
to fix that. But I'd like to confirm if my understanding aligns with Make 
behaviour.


https://reviews.llvm.org/D30882



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

[PATCH] D48786: [Preprocessor] Stop entering included files after hitting a fatal error.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/lib/Lex/PPDirectives.cpp:1875
+  // Stop further preprocessing if a fatal error has occurred. Any diagnostics
+  // we might have raised will not be visible.
+  if (ShouldEnter && Diags->hasFatalErrorOccurred())

jkorous wrote:
> vsapsai wrote:
> > jkorous wrote:
> > > I am not sure I understand this - does that mean that we are not 
> > > displaying diagnostics that were produced before "now"?
> > I can see how the wording can cause the confusion. But I'm not entirely 
> > sure it is misguiding, I've copy-pasted it from 
> > [Sema::InstantiatingTemplate::InstantiatingTemplate 
> > ](https://github.com/llvm-mirror/clang/blob/580f7daabc7696d50ad09d9643b2afeadbd387d8/lib/Sema/SemaTemplateInstantiate.cpp#L218-L220).
> >  Let me explain my reasoning in a different way to see if it makes sense. 
> > Entering a file is observable if it produces diagnostics or some other 
> > build artifact (object file in most cases). So when we encounter a fatal 
> > error, there is no visible indication of entering subsequent files. That's 
> > why we can skip entering those files: no difference in output and less work 
> > to do.
> Thanks for the explanation! I was not sure whether "only subsequent" OR "both 
> subsequent and some/all prior" raised diagnostics would be not visible (could 
> be just my shitty English though).
> 
> Maybe something along this line "any eventual subsequent diagnostics will not 
> be visible" would be more clear?
Curiously, the comment kinda means both. Some prior diagnostics might be 
invisible but in this case we care about subsequent diagnostics. I'll update 
the comment to make that more clear.


https://reviews.llvm.org/D48786



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

[PATCH] D48786: [Preprocessor] Stop entering included files after hitting a fatal error.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 156911.
vsapsai added a comment.

- Tweak the comment according to review comments.

Undiscussed changes: we don't stop preprocessing entirely, only in included
files; not using better-sounding "skip" deliberately as it might be confused
with `FileSkipped` API.


https://reviews.llvm.org/D48786

Files:
  clang/lib/Lex/PPDirectives.cpp
  clang/test/Preprocessor/Inputs/cycle/a.h
  clang/test/Preprocessor/Inputs/cycle/b.h
  clang/test/Preprocessor/Inputs/cycle/c.h
  clang/test/Preprocessor/include-cycle.c


Index: clang/test/Preprocessor/include-cycle.c
===
--- /dev/null
+++ clang/test/Preprocessor/include-cycle.c
@@ -0,0 +1,5 @@
+// RUN: not %clang_cc1 -E -I%S/Inputs -ferror-limit 20 %s
+
+// Test that preprocessing terminates even if we have inclusion cycles.
+
+#include "cycle/a.h"
Index: clang/test/Preprocessor/Inputs/cycle/c.h
===
--- /dev/null
+++ clang/test/Preprocessor/Inputs/cycle/c.h
@@ -0,0 +1 @@
+#include "a.h"
Index: clang/test/Preprocessor/Inputs/cycle/b.h
===
--- /dev/null
+++ clang/test/Preprocessor/Inputs/cycle/b.h
@@ -0,0 +1 @@
+#include "a.h"
Index: clang/test/Preprocessor/Inputs/cycle/a.h
===
--- /dev/null
+++ clang/test/Preprocessor/Inputs/cycle/a.h
@@ -0,0 +1,8 @@
+// Presence of 2 inclusion cycles
+//b.h -> a.h -> b.h -> ...
+//c.h -> a.h -> c.h -> ...
+// makes it unfeasible to reach max inclusion depth in all possible ways. Need
+// to stop earlier.
+
+#include "b.h"
+#include "c.h"
Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -1871,6 +1871,12 @@
   if (PPOpts->SingleFileParseMode)
 ShouldEnter = false;
 
+  // Any diagnostics after the fatal error will not be visible. As the
+  // compilation failed already and errors in subsequently included files won't
+  // be visible, avoid preprocessing those files.
+  if (ShouldEnter && Diags->hasFatalErrorOccurred())
+ShouldEnter = false;
+
   // Determine whether we should try to import the module for this #include, if
   // there is one. Don't do so if precompiled module support is disabled or we
   // are processing this module textually (because we're building the module).


Index: clang/test/Preprocessor/include-cycle.c
===
--- /dev/null
+++ clang/test/Preprocessor/include-cycle.c
@@ -0,0 +1,5 @@
+// RUN: not %clang_cc1 -E -I%S/Inputs -ferror-limit 20 %s
+
+// Test that preprocessing terminates even if we have inclusion cycles.
+
+#include "cycle/a.h"
Index: clang/test/Preprocessor/Inputs/cycle/c.h
===
--- /dev/null
+++ clang/test/Preprocessor/Inputs/cycle/c.h
@@ -0,0 +1 @@
+#include "a.h"
Index: clang/test/Preprocessor/Inputs/cycle/b.h
===
--- /dev/null
+++ clang/test/Preprocessor/Inputs/cycle/b.h
@@ -0,0 +1 @@
+#include "a.h"
Index: clang/test/Preprocessor/Inputs/cycle/a.h
===
--- /dev/null
+++ clang/test/Preprocessor/Inputs/cycle/a.h
@@ -0,0 +1,8 @@
+// Presence of 2 inclusion cycles
+//b.h -> a.h -> b.h -> ...
+//c.h -> a.h -> c.h -> ...
+// makes it unfeasible to reach max inclusion depth in all possible ways. Need
+// to stop earlier.
+
+#include "b.h"
+#include "c.h"
Index: clang/lib/Lex/PPDirectives.cpp
===
--- clang/lib/Lex/PPDirectives.cpp
+++ clang/lib/Lex/PPDirectives.cpp
@@ -1871,6 +1871,12 @@
   if (PPOpts->SingleFileParseMode)
 ShouldEnter = false;
 
+  // Any diagnostics after the fatal error will not be visible. As the
+  // compilation failed already and errors in subsequently included files won't
+  // be visible, avoid preprocessing those files.
+  if (ShouldEnter && Diags->hasFatalErrorOccurred())
+ShouldEnter = false;
+
   // Determine whether we should try to import the module for this #include, if
   // there is one. Don't do so if precompiled module support is disabled or we
   // are processing this module textually (because we're building the module).
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49518: [VFS] Emit an error when a file isn't located in any directory.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In https://reviews.llvm.org/D49518#1168038, @bruno wrote:

> Hi Volodymyr, thanks for improving this.
>
> > Need to double check what tests we have when using relative path names at 
> > the root level.
>
> Another interesting place to look at is 
> `unittests/Basic/VirtualFileSystemTest.cpp`


Thanks for the pointer. Probably I'll move the added test there as it doesn't 
need full file system interaction and doesn't need to execute entire clang_cc1.

>> I'd like to make the behavior consistent because a file name is a specific 
>> case of relative paths. So far there are no assertions and no errors but 
>> file lookup doesn't seem to be working.
> 
> Makes sense. My general impression is that relative paths don't make sense 
> here either, but you can address that in a follow up patch (giving time for 
> any potential counterexample on this).

Based on the code, relative paths won't work for file lookup. That is so 
because in `RedirectingFileSystem::lookupPath` we convert paths to absolute

  if (std::error_code EC = makeAbsolute(Path))
return EC;
  // ...
  sys::path::const_iterator Start = sys::path::begin(Path);
  sys::path::const_iterator End = sys::path::end(Path);

and textually compare them with VFS entry name 
.

Having an error for relative paths makes the change cleaner, so I'll include it 
in this patch.


https://reviews.llvm.org/D49518



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

[PATCH] D49518: [VFS] Emit an error when a file isn't located in any directory.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 157311.
vsapsai added a comment.

- Make diagnostics more general, use unit tests.


https://reviews.llvm.org/D49518

Files:
  clang/lib/Basic/VirtualFileSystem.cpp
  clang/unittests/Basic/VirtualFileSystemTest.cpp

Index: clang/unittests/Basic/VirtualFileSystemTest.cpp
===
--- clang/unittests/Basic/VirtualFileSystemTest.cpp
+++ clang/unittests/Basic/VirtualFileSystemTest.cpp
@@ -1429,3 +1429,35 @@
   }
   EXPECT_EQ(I, E);
 }
+
+TEST_F(VFSFromYAMLTest, RelativePaths) {
+  IntrusiveRefCntPtr Lower(new DummyFileSystem());
+  // Filename at root level without a parent directory.
+  IntrusiveRefCntPtr FS = getFromYAMLString(
+  "{ 'roots': [\n"
+  "  { 'type': 'file', 'name': 'file-not-in-directory.h',\n"
+  "'external-contents': '//root/external/file'\n"
+  "  }\n"
+  "] }", Lower);
+  EXPECT_EQ(nullptr, FS.get());
+
+  // Relative file path.
+  FS = getFromYAMLString(
+  "{ 'roots': [\n"
+  "  { 'type': 'file', 'name': 'relative/file/path.h',\n"
+  "'external-contents': '//root/external/file'\n"
+  "  }\n"
+  "] }", Lower);
+  EXPECT_EQ(nullptr, FS.get());
+
+  // Relative directory path.
+  FS = getFromYAMLString(
+   "{ 'roots': [\n"
+   "  { 'type': 'directory', 'name': 'relative/directory/path.h',\n"
+   "'contents': []\n"
+   "  }\n"
+   "] }", Lower);
+  EXPECT_EQ(nullptr, FS.get());
+
+  EXPECT_EQ(3, NumDiagnostics);
+}
Index: clang/lib/Basic/VirtualFileSystem.cpp
===
--- clang/lib/Basic/VirtualFileSystem.cpp
+++ clang/lib/Basic/VirtualFileSystem.cpp
@@ -1244,7 +1244,8 @@
 }
   }
 
-  std::unique_ptr parseEntry(yaml::Node *N, RedirectingFileSystem *FS) {
+  std::unique_ptr parseEntry(yaml::Node *N, RedirectingFileSystem *FS,
+bool IsRootEntry) {
 auto *M = dyn_cast(N);
 if (!M) {
   error(N, "expected mapping node for file or directory entry");
@@ -1265,6 +1266,7 @@
 std::vector> EntryArrayContents;
 std::string ExternalContentsPath;
 std::string Name;
+yaml::Node *NameValueNode;
 auto UseExternalName = RedirectingFileEntry::NK_NotSet;
 EntryKind Kind;
 
@@ -1284,6 +1286,7 @@
 if (!parseScalarString(I.getValue(), Value, Buffer))
   return nullptr;
 
+NameValueNode = I.getValue();
 if (FS->UseCanonicalizedPaths) {
   SmallString<256> Path(Value);
   // Guarantee that old YAML files containing paths with ".." and "."
@@ -1320,7 +1323,8 @@
 }
 
 for (auto &I : *Contents) {
-  if (std::unique_ptr E = parseEntry(&I, FS))
+  if (std::unique_ptr E =
+  parseEntry(&I, FS, /*IsRootEntry*/ false))
 EntryArrayContents.push_back(std::move(E));
   else
 return nullptr;
@@ -1381,6 +1385,13 @@
   return nullptr;
 }
 
+if (IsRootEntry && !sys::path::is_absolute(Name)) {
+  assert(NameValueNode && "Name presence should be checked earlier");
+  error(NameValueNode,
+"entry with relative path at the root level is not discoverable");
+  return nullptr;
+}
+
 // Remove trailing slash(es), being careful not to remove the root path
 StringRef Trimmed(Name);
 size_t RootPathLen = sys::path::root_path(Trimmed).size();
@@ -1463,7 +1474,8 @@
 }
 
 for (auto &I : *Roots) {
-  if (std::unique_ptr E = parseEntry(&I, FS))
+  if (std::unique_ptr E =
+  parseEntry(&I, FS, /*IsRootEntry*/ true))
 RootEntries.push_back(std::move(E));
   else
 return false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48786: [Preprocessor] Stop entering included files after hitting a fatal error.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Thanks everyone for reviewing the change.


Repository:
  rL LLVM

https://reviews.llvm.org/D48786



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

[PATCH] D48786: [Preprocessor] Stop entering included files after hitting a fatal error.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL337953: [Preprocessor] Stop entering included files after 
hitting a fatal error. (authored by vsapsai, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D48786?vs=156911&id=157336#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D48786

Files:
  cfe/trunk/lib/Lex/PPDirectives.cpp
  cfe/trunk/test/Preprocessor/Inputs/cycle/a.h
  cfe/trunk/test/Preprocessor/Inputs/cycle/b.h
  cfe/trunk/test/Preprocessor/Inputs/cycle/c.h
  cfe/trunk/test/Preprocessor/include-cycle.c


Index: cfe/trunk/test/Preprocessor/Inputs/cycle/c.h
===
--- cfe/trunk/test/Preprocessor/Inputs/cycle/c.h
+++ cfe/trunk/test/Preprocessor/Inputs/cycle/c.h
@@ -0,0 +1 @@
+#include "a.h"
Index: cfe/trunk/test/Preprocessor/Inputs/cycle/b.h
===
--- cfe/trunk/test/Preprocessor/Inputs/cycle/b.h
+++ cfe/trunk/test/Preprocessor/Inputs/cycle/b.h
@@ -0,0 +1 @@
+#include "a.h"
Index: cfe/trunk/test/Preprocessor/Inputs/cycle/a.h
===
--- cfe/trunk/test/Preprocessor/Inputs/cycle/a.h
+++ cfe/trunk/test/Preprocessor/Inputs/cycle/a.h
@@ -0,0 +1,8 @@
+// Presence of 2 inclusion cycles
+//b.h -> a.h -> b.h -> ...
+//c.h -> a.h -> c.h -> ...
+// makes it unfeasible to reach max inclusion depth in all possible ways. Need
+// to stop earlier.
+
+#include "b.h"
+#include "c.h"
Index: cfe/trunk/test/Preprocessor/include-cycle.c
===
--- cfe/trunk/test/Preprocessor/include-cycle.c
+++ cfe/trunk/test/Preprocessor/include-cycle.c
@@ -0,0 +1,5 @@
+// RUN: not %clang_cc1 -E -I%S/Inputs -ferror-limit 20 %s
+
+// Test that preprocessing terminates even if we have inclusion cycles.
+
+#include "cycle/a.h"
Index: cfe/trunk/lib/Lex/PPDirectives.cpp
===
--- cfe/trunk/lib/Lex/PPDirectives.cpp
+++ cfe/trunk/lib/Lex/PPDirectives.cpp
@@ -1896,6 +1896,12 @@
   if (PPOpts->SingleFileParseMode)
 ShouldEnter = false;
 
+  // Any diagnostics after the fatal error will not be visible. As the
+  // compilation failed already and errors in subsequently included files won't
+  // be visible, avoid preprocessing those files.
+  if (ShouldEnter && Diags->hasFatalErrorOccurred())
+ShouldEnter = false;
+
   // Determine whether we should try to import the module for this #include, if
   // there is one. Don't do so if precompiled module support is disabled or we
   // are processing this module textually (because we're building the module).


Index: cfe/trunk/test/Preprocessor/Inputs/cycle/c.h
===
--- cfe/trunk/test/Preprocessor/Inputs/cycle/c.h
+++ cfe/trunk/test/Preprocessor/Inputs/cycle/c.h
@@ -0,0 +1 @@
+#include "a.h"
Index: cfe/trunk/test/Preprocessor/Inputs/cycle/b.h
===
--- cfe/trunk/test/Preprocessor/Inputs/cycle/b.h
+++ cfe/trunk/test/Preprocessor/Inputs/cycle/b.h
@@ -0,0 +1 @@
+#include "a.h"
Index: cfe/trunk/test/Preprocessor/Inputs/cycle/a.h
===
--- cfe/trunk/test/Preprocessor/Inputs/cycle/a.h
+++ cfe/trunk/test/Preprocessor/Inputs/cycle/a.h
@@ -0,0 +1,8 @@
+// Presence of 2 inclusion cycles
+//b.h -> a.h -> b.h -> ...
+//c.h -> a.h -> c.h -> ...
+// makes it unfeasible to reach max inclusion depth in all possible ways. Need
+// to stop earlier.
+
+#include "b.h"
+#include "c.h"
Index: cfe/trunk/test/Preprocessor/include-cycle.c
===
--- cfe/trunk/test/Preprocessor/include-cycle.c
+++ cfe/trunk/test/Preprocessor/include-cycle.c
@@ -0,0 +1,5 @@
+// RUN: not %clang_cc1 -E -I%S/Inputs -ferror-limit 20 %s
+
+// Test that preprocessing terminates even if we have inclusion cycles.
+
+#include "cycle/a.h"
Index: cfe/trunk/lib/Lex/PPDirectives.cpp
===
--- cfe/trunk/lib/Lex/PPDirectives.cpp
+++ cfe/trunk/lib/Lex/PPDirectives.cpp
@@ -1896,6 +1896,12 @@
   if (PPOpts->SingleFileParseMode)
 ShouldEnter = false;
 
+  // Any diagnostics after the fatal error will not be visible. As the
+  // compilation failed already and errors in subsequently included files won't
+  // be visible, avoid preprocessing those files.
+  if (ShouldEnter && Diags->hasFatalErrorOccurred())
+ShouldEnter = false;
+
   // Determine whether we should try to import the module for this #include, if
   // there is one. Don't do so if precompiled module support is disabled or we
   // are processing this module textually (because we're building the module).
___
cfe-commits

[PATCH] D48753: [libcxx] Use custom allocator's `construct` in C++03 when available.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Ping.


https://reviews.llvm.org/D48753



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

[PATCH] D49119: [Sema][ObjC] Issue a warning when a method declared in a protocol is non-escaping but the corresponding method in the implementation is escaping.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In https://reviews.llvm.org/D49119#1176047, @ahatanak wrote:

> In https://reviews.llvm.org/D49119#1164285, @vsapsai wrote:
>
> > Also I had a few ideas for tests when the warning isn't required and it is 
> > absent. But I'm not sure they are actually valuable. If you are interested, 
> > we can discuss it in more details.
>
>
> Could you elaborate on what kind of tests you have in mind?




- declaring `noescape` on implementation when nothing like that was mentioned 
in interface or protocols;
- have class method and instance method with the same name, only one of them is 
`noescape`, test that we don't show spurious warning in this case;
- try selector names that look similar but are different, like `-foo`, `-foo:`, 
`-foo::` This test suggestion isn't really related to `noescape`, I just got 
carried away.


Repository:
  rC Clang

https://reviews.llvm.org/D49119



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

[PATCH] D49119: [Sema][ObjC] Issue a warning when a method declared in a protocol is non-escaping but the corresponding method in the implementation is escaping.

[Volodymyr Sapsai via Phabricator via cfe-commits]

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

Looks good to me. Any further improvements are up to you.


Repository:
  rC Clang

https://reviews.llvm.org/D49119



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

[PATCH] D50118: [VFS] Unify iteration code for VFSFromYamlDirIterImpl, NFC intended.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added reviewers: bruno, benlangmuir.
Herald added a subscriber: dexonsmith.

First and subsequent iteration steps are similar, capture this similarity.


https://reviews.llvm.org/D50118

Files:
  clang/lib/Basic/VirtualFileSystem.cpp


Index: clang/lib/Basic/VirtualFileSystem.cpp
===
--- clang/lib/Basic/VirtualFileSystem.cpp
+++ clang/lib/Basic/VirtualFileSystem.cpp
@@ -896,6 +896,8 @@
   RedirectingFileSystem &FS;
   RedirectingDirectoryEntry::iterator Current, End;
 
+  std::error_code incrementImpl();
+
 public:
   VFSFromYamlDirIterImpl(const Twine &Path, RedirectingFileSystem &FS,
  RedirectingDirectoryEntry::iterator Begin,
@@ -1948,36 +1950,25 @@
 RedirectingDirectoryEntry::iterator Begin,
 RedirectingDirectoryEntry::iterator End, std::error_code &EC)
 : Dir(_Path.str()), FS(FS), Current(Begin), End(End) {
-  while (Current != End) {
-SmallString<128> PathStr(Dir);
-llvm::sys::path::append(PathStr, (*Current)->getName());
-llvm::ErrorOr S = FS.status(PathStr);
-if (S) {
-  CurrentEntry = *S;
-  return;
-}
-// Skip entries which do not map to a reliable external content.
-if (FS.ignoreNonExistentContents() &&
-S.getError() == llvm::errc::no_such_file_or_directory) {
-  ++Current;
-  continue;
-} else {
-  EC = S.getError();
-  break;
-}
-  }
+  EC = incrementImpl();
 }
 
 std::error_code VFSFromYamlDirIterImpl::increment() {
   assert(Current != End && "cannot iterate past end");
-  while (++Current != End) {
+  ++Current;
+  return incrementImpl();
+}
+
+std::error_code VFSFromYamlDirIterImpl::incrementImpl() {
+  while (Current != End) {
 SmallString<128> PathStr(Dir);
 llvm::sys::path::append(PathStr, (*Current)->getName());
 llvm::ErrorOr S = FS.status(PathStr);
 if (!S) {
   // Skip entries which do not map to a reliable external content.
   if (FS.ignoreNonExistentContents() &&
   S.getError() == llvm::errc::no_such_file_or_directory) {
+++Current;
 continue;
   } else {
 return S.getError();


Index: clang/lib/Basic/VirtualFileSystem.cpp
===
--- clang/lib/Basic/VirtualFileSystem.cpp
+++ clang/lib/Basic/VirtualFileSystem.cpp
@@ -896,6 +896,8 @@
   RedirectingFileSystem &FS;
   RedirectingDirectoryEntry::iterator Current, End;
 
+  std::error_code incrementImpl();
+
 public:
   VFSFromYamlDirIterImpl(const Twine &Path, RedirectingFileSystem &FS,
  RedirectingDirectoryEntry::iterator Begin,
@@ -1948,36 +1950,25 @@
 RedirectingDirectoryEntry::iterator Begin,
 RedirectingDirectoryEntry::iterator End, std::error_code &EC)
 : Dir(_Path.str()), FS(FS), Current(Begin), End(End) {
-  while (Current != End) {
-SmallString<128> PathStr(Dir);
-llvm::sys::path::append(PathStr, (*Current)->getName());
-llvm::ErrorOr S = FS.status(PathStr);
-if (S) {
-  CurrentEntry = *S;
-  return;
-}
-// Skip entries which do not map to a reliable external content.
-if (FS.ignoreNonExistentContents() &&
-S.getError() == llvm::errc::no_such_file_or_directory) {
-  ++Current;
-  continue;
-} else {
-  EC = S.getError();
-  break;
-}
-  }
+  EC = incrementImpl();
 }
 
 std::error_code VFSFromYamlDirIterImpl::increment() {
   assert(Current != End && "cannot iterate past end");
-  while (++Current != End) {
+  ++Current;
+  return incrementImpl();
+}
+
+std::error_code VFSFromYamlDirIterImpl::incrementImpl() {
+  while (Current != End) {
 SmallString<128> PathStr(Dir);
 llvm::sys::path::append(PathStr, (*Current)->getName());
 llvm::ErrorOr S = FS.status(PathStr);
 if (!S) {
   // Skip entries which do not map to a reliable external content.
   if (FS.ignoreNonExistentContents() &&
   S.getError() == llvm::errc::no_such_file_or_directory) {
+++Current;
 continue;
   } else {
 return S.getError();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50160: [c-index-test] Use correct executable path to discover resource directory.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added reviewers: nathawes, akyrtzi, bob.wilson.
Herald added a subscriber: dexonsmith.

Driver builds resource directory path based on provided executable path.
Instead of string "clang" use actual executable path.

rdar://problem/42699514


https://reviews.llvm.org/D50160

Files:
  clang/tools/c-index-test/core_main.cpp


Index: clang/tools/c-index-test/core_main.cpp
===
--- clang/tools/c-index-test/core_main.cpp
+++ clang/tools/c-index-test/core_main.cpp
@@ -20,6 +20,7 @@
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Serialization/ASTReader.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/PrettyStackTrace.h"
@@ -202,11 +203,13 @@
   });
 }
 
-static bool printSourceSymbols(ArrayRef Args,
-   bool dumpModuleImports,
-   bool indexLocals) {
+static bool printSourceSymbols(const char *ProgName,
+   ArrayRef Args,
+   bool dumpModuleImports, bool indexLocals) {
+  void *P = (void*) (intptr_t) indextest_core_main;
+  std::string Executable = llvm::sys::fs::getMainExecutable(ProgName, P);
   SmallVector ArgsWithProgName;
-  ArgsWithProgName.push_back("clang");
+  ArgsWithProgName.push_back(Executable.c_str());
   ArgsWithProgName.append(Args.begin(), Args.end());
   IntrusiveRefCntPtr
 Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions));
@@ -314,6 +317,7 @@
 int indextest_core_main(int argc, const char **argv) {
   sys::PrintStackTraceOnErrorSignal(argv[0]);
   PrettyStackTraceProgram X(argc, argv);
+  const char *ProgName = argv[0];
 
   assert(argv[1] == StringRef("core"));
   ++argv;
@@ -343,7 +347,8 @@
   errs() << "error: missing compiler args; pass '-- '\n";
   return 1;
 }
-return printSourceSymbols(CompArgs, options::DumpModuleImports, 
options::IncludeLocals);
+return printSourceSymbols(ProgName, CompArgs, options::DumpModuleImports,
+  options::IncludeLocals);
   }
 
   return 0;


Index: clang/tools/c-index-test/core_main.cpp
===
--- clang/tools/c-index-test/core_main.cpp
+++ clang/tools/c-index-test/core_main.cpp
@@ -20,6 +20,7 @@
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Serialization/ASTReader.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/PrettyStackTrace.h"
@@ -202,11 +203,13 @@
   });
 }
 
-static bool printSourceSymbols(ArrayRef Args,
-   bool dumpModuleImports,
-   bool indexLocals) {
+static bool printSourceSymbols(const char *ProgName,
+   ArrayRef Args,
+   bool dumpModuleImports, bool indexLocals) {
+  void *P = (void*) (intptr_t) indextest_core_main;
+  std::string Executable = llvm::sys::fs::getMainExecutable(ProgName, P);
   SmallVector ArgsWithProgName;
-  ArgsWithProgName.push_back("clang");
+  ArgsWithProgName.push_back(Executable.c_str());
   ArgsWithProgName.append(Args.begin(), Args.end());
   IntrusiveRefCntPtr
 Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions));
@@ -314,6 +317,7 @@
 int indextest_core_main(int argc, const char **argv) {
   sys::PrintStackTraceOnErrorSignal(argv[0]);
   PrettyStackTraceProgram X(argc, argv);
+  const char *ProgName = argv[0];
 
   assert(argv[1] == StringRef("core"));
   ++argv;
@@ -343,7 +347,8 @@
   errs() << "error: missing compiler args; pass '-- '\n";
   return 1;
 }
-return printSourceSymbols(CompArgs, options::DumpModuleImports, options::IncludeLocals);
+return printSourceSymbols(ProgName, CompArgs, options::DumpModuleImports,
+  options::IncludeLocals);
   }
 
   return 0;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50160: [c-index-test] Use correct executable path to discover resource directory.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 158658.
vsapsai added a comment.

- Address review comment: move getMainExecutable to indextest_core_main.


https://reviews.llvm.org/D50160

Files:
  clang/tools/c-index-test/core_main.cpp


Index: clang/tools/c-index-test/core_main.cpp
===
--- clang/tools/c-index-test/core_main.cpp
+++ clang/tools/c-index-test/core_main.cpp
@@ -20,6 +20,7 @@
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Serialization/ASTReader.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/PrettyStackTrace.h"
@@ -202,11 +203,11 @@
   });
 }
 
-static bool printSourceSymbols(ArrayRef Args,
-   bool dumpModuleImports,
-   bool indexLocals) {
+static bool printSourceSymbols(const char *Executable,
+   ArrayRef Args,
+   bool dumpModuleImports, bool indexLocals) {
   SmallVector ArgsWithProgName;
-  ArgsWithProgName.push_back("clang");
+  ArgsWithProgName.push_back(Executable);
   ArgsWithProgName.append(Args.begin(), Args.end());
   IntrusiveRefCntPtr
 Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions));
@@ -314,6 +315,8 @@
 int indextest_core_main(int argc, const char **argv) {
   sys::PrintStackTraceOnErrorSignal(argv[0]);
   PrettyStackTraceProgram X(argc, argv);
+  void *MainAddr = (void*) (intptr_t) indextest_core_main;
+  std::string Executable = llvm::sys::fs::getMainExecutable(argv[0], MainAddr);
 
   assert(argv[1] == StringRef("core"));
   ++argv;
@@ -343,7 +346,9 @@
   errs() << "error: missing compiler args; pass '-- '\n";
   return 1;
 }
-return printSourceSymbols(CompArgs, options::DumpModuleImports, 
options::IncludeLocals);
+return printSourceSymbols(Executable.c_str(), CompArgs,
+  options::DumpModuleImports,
+  options::IncludeLocals);
   }
 
   return 0;


Index: clang/tools/c-index-test/core_main.cpp
===
--- clang/tools/c-index-test/core_main.cpp
+++ clang/tools/c-index-test/core_main.cpp
@@ -20,6 +20,7 @@
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Serialization/ASTReader.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/PrettyStackTrace.h"
@@ -202,11 +203,11 @@
   });
 }
 
-static bool printSourceSymbols(ArrayRef Args,
-   bool dumpModuleImports,
-   bool indexLocals) {
+static bool printSourceSymbols(const char *Executable,
+   ArrayRef Args,
+   bool dumpModuleImports, bool indexLocals) {
   SmallVector ArgsWithProgName;
-  ArgsWithProgName.push_back("clang");
+  ArgsWithProgName.push_back(Executable);
   ArgsWithProgName.append(Args.begin(), Args.end());
   IntrusiveRefCntPtr
 Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions));
@@ -314,6 +315,8 @@
 int indextest_core_main(int argc, const char **argv) {
   sys::PrintStackTraceOnErrorSignal(argv[0]);
   PrettyStackTraceProgram X(argc, argv);
+  void *MainAddr = (void*) (intptr_t) indextest_core_main;
+  std::string Executable = llvm::sys::fs::getMainExecutable(argv[0], MainAddr);
 
   assert(argv[1] == StringRef("core"));
   ++argv;
@@ -343,7 +346,9 @@
   errs() << "error: missing compiler args; pass '-- '\n";
   return 1;
 }
-return printSourceSymbols(CompArgs, options::DumpModuleImports, options::IncludeLocals);
+return printSourceSymbols(Executable.c_str(), CompArgs,
+  options::DumpModuleImports,
+  options::IncludeLocals);
   }
 
   return 0;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50160: [c-index-test] Use correct executable path to discover resource directory.

[Volodymyr Sapsai via Phabricator via cfe-commits]

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



Comment at: clang/tools/c-index-test/core_main.cpp:210
+  void *P = (void*) (intptr_t) indextest_core_main;
+  std::string Executable = llvm::sys::fs::getMainExecutable(ProgName, P);
   SmallVector ArgsWithProgName;

akyrtzi wrote:
> Could you move this up to `indextest_core_main` and have 
> `printSourceSymbols()` accept the executable path directly ?
> This would come in handy to avoid duplication if we later on add another 
> function that also needs the executable path.
Done. Thanks for the suggestion, I think the code looks better now.


https://reviews.llvm.org/D50160



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

[PATCH] D50160: [c-index-test] Use correct executable path to discover resource directory.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
vsapsai marked an inline comment as done.
Closed by commit rL338741: [c-index-test] Use correct executable path to 
discover resource directory. (authored by vsapsai, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50160?vs=158658&id=158794#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50160

Files:
  cfe/trunk/tools/c-index-test/core_main.cpp


Index: cfe/trunk/tools/c-index-test/core_main.cpp
===
--- cfe/trunk/tools/c-index-test/core_main.cpp
+++ cfe/trunk/tools/c-index-test/core_main.cpp
@@ -20,6 +20,7 @@
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Serialization/ASTReader.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/PrettyStackTrace.h"
@@ -202,11 +203,11 @@
   });
 }
 
-static bool printSourceSymbols(ArrayRef Args,
-   bool dumpModuleImports,
-   bool indexLocals) {
+static bool printSourceSymbols(const char *Executable,
+   ArrayRef Args,
+   bool dumpModuleImports, bool indexLocals) {
   SmallVector ArgsWithProgName;
-  ArgsWithProgName.push_back("clang");
+  ArgsWithProgName.push_back(Executable);
   ArgsWithProgName.append(Args.begin(), Args.end());
   IntrusiveRefCntPtr
 Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions));
@@ -314,6 +315,8 @@
 int indextest_core_main(int argc, const char **argv) {
   sys::PrintStackTraceOnErrorSignal(argv[0]);
   PrettyStackTraceProgram X(argc, argv);
+  void *MainAddr = (void*) (intptr_t) indextest_core_main;
+  std::string Executable = llvm::sys::fs::getMainExecutable(argv[0], MainAddr);
 
   assert(argv[1] == StringRef("core"));
   ++argv;
@@ -343,7 +346,9 @@
   errs() << "error: missing compiler args; pass '-- '\n";
   return 1;
 }
-return printSourceSymbols(CompArgs, options::DumpModuleImports, 
options::IncludeLocals);
+return printSourceSymbols(Executable.c_str(), CompArgs,
+  options::DumpModuleImports,
+  options::IncludeLocals);
   }
 
   return 0;


Index: cfe/trunk/tools/c-index-test/core_main.cpp
===
--- cfe/trunk/tools/c-index-test/core_main.cpp
+++ cfe/trunk/tools/c-index-test/core_main.cpp
@@ -20,6 +20,7 @@
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Serialization/ASTReader.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/PrettyStackTrace.h"
@@ -202,11 +203,11 @@
   });
 }
 
-static bool printSourceSymbols(ArrayRef Args,
-   bool dumpModuleImports,
-   bool indexLocals) {
+static bool printSourceSymbols(const char *Executable,
+   ArrayRef Args,
+   bool dumpModuleImports, bool indexLocals) {
   SmallVector ArgsWithProgName;
-  ArgsWithProgName.push_back("clang");
+  ArgsWithProgName.push_back(Executable);
   ArgsWithProgName.append(Args.begin(), Args.end());
   IntrusiveRefCntPtr
 Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions));
@@ -314,6 +315,8 @@
 int indextest_core_main(int argc, const char **argv) {
   sys::PrintStackTraceOnErrorSignal(argv[0]);
   PrettyStackTraceProgram X(argc, argv);
+  void *MainAddr = (void*) (intptr_t) indextest_core_main;
+  std::string Executable = llvm::sys::fs::getMainExecutable(argv[0], MainAddr);
 
   assert(argv[1] == StringRef("core"));
   ++argv;
@@ -343,7 +346,9 @@
   errs() << "error: missing compiler args; pass '-- '\n";
   return 1;
 }
-return printSourceSymbols(CompArgs, options::DumpModuleImports, options::IncludeLocals);
+return printSourceSymbols(Executable.c_str(), CompArgs,
+  options::DumpModuleImports,
+  options::IncludeLocals);
   }
 
   return 0;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50160: [c-index-test] Use correct executable path to discover resource directory.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Thanks for the review!


Repository:
  rL LLVM

https://reviews.llvm.org/D50160



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

[PATCH] D45015: [Preprocessor] Allow libc++ to detect when aligned allocation is unavailable.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Eric, will you have time to commit this patch? If you don't have time and don't 
have objections, I plan to land this change on your behalf.


https://reviews.llvm.org/D45015



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

[PATCH] D45015: [Preprocessor] Allow libc++ to detect when aligned allocation is unavailable.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL338934: [Preprocessor] Allow libc++ to detect when aligned 
allocation is unavailable. (authored by vsapsai, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D45015?vs=150635&id=159121#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D45015

Files:
  cfe/trunk/include/clang/Basic/DiagnosticGroups.td
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/Driver/ToolChains/Darwin.cpp
  cfe/trunk/lib/Frontend/InitPreprocessor.cpp
  cfe/trunk/lib/Sema/SemaExprCXX.cpp
  cfe/trunk/test/Driver/unavailable_aligned_allocation.cpp
  cfe/trunk/test/Lexer/aligned-allocation.cpp

Index: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticGroups.td
+++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td
@@ -364,7 +364,6 @@
 def NullPointerArithmetic : DiagGroup<"null-pointer-arithmetic">;
 def : DiagGroup<"effc++", [NonVirtualDtor]>;
 def OveralignedType : DiagGroup<"over-aligned">;
-def AlignedAllocationUnavailable : DiagGroup<"aligned-allocation-unavailable">;
 def OldStyleCast : DiagGroup<"old-style-cast">;
 def : DiagGroup<"old-style-definition">;
 def OutOfLineDeclaration : DiagGroup<"out-of-line-declaration">;
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6465,12 +6465,12 @@
   "type %0 requires %1 bytes of alignment and the default allocator only "
   "guarantees %2 bytes">,
   InGroup, DefaultIgnore;
-def warn_aligned_allocation_unavailable :Warning<
+def err_aligned_allocation_unavailable : Error<
   "aligned %select{allocation|deallocation}0 function of type '%1' is only "
-  "available on %2 %3 or newer">, InGroup, DefaultError;
+  "available on %2 %3 or newer">;
 def note_silence_unligned_allocation_unavailable : Note<
   "if you supply your own aligned allocation functions, use "
-  "-Wno-aligned-allocation-unavailable to silence this diagnostic">;
+  "-faligned-allocation to silence this diagnostic">;
 
 def err_conditional_void_nonvoid : Error<
   "%select{left|right}1 operand to ? is void, but %select{right|left}1 operand "
Index: cfe/trunk/test/Lexer/aligned-allocation.cpp
===
--- cfe/trunk/test/Lexer/aligned-allocation.cpp
+++ cfe/trunk/test/Lexer/aligned-allocation.cpp
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.12.0 -fexceptions -std=c++17 -verify %s \
+// RUN:   -DEXPECT_DEFINED
+//
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.12.0 -fexceptions -std=c++17 -verify %s \
+// RUN:   -faligned-alloc-unavailable
+//
+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.12.0 -fexceptions -std=c++17 -verify %s \
+// RUN:   -faligned-allocation -faligned-alloc-unavailable
+
+// Test that __cpp_aligned_new is not defined when CC1 is passed
+// -faligned-alloc-unavailable by the Darwin driver, even when aligned
+// allocation is actually enabled.
+
+// expected-no-diagnostics
+#ifdef EXPECT_DEFINED
+# ifndef __cpp_aligned_new
+#   error "__cpp_aligned_new" should be defined
+# endif
+#else
+# ifdef __cpp_aligned_new
+#   error "__cpp_aligned_new" should not be defined
+# endif
+#endif
Index: cfe/trunk/test/Driver/unavailable_aligned_allocation.cpp
===
--- cfe/trunk/test/Driver/unavailable_aligned_allocation.cpp
+++ cfe/trunk/test/Driver/unavailable_aligned_allocation.cpp
@@ -51,4 +51,13 @@
 // RUN: -c -### %s 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=AVAILABLE
 //
+// Check that passing -faligned-allocation or -fno-aligned-allocation stops the
+// driver from passing -faligned-alloc-unavailable to cc1.
+//
+// RUN: %clang -target x86_64-apple-macosx10.12 -faligned-allocation -c -### %s 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=AVAILABLE
+//
+// RUN: %clang -target x86_64-apple-macosx10.12 -fno-aligned-allocation -c -### %s 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=AVAILABLE
+
 // AVAILABLE-NOT: "-faligned-alloc-unavailable"
Index: cfe/trunk/lib/Driver/ToolChains/Darwin.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Darwin.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Darwin.cpp
@@ -2035,7 +2035,11 @@
 void Darwin::addClangTargetOptions(const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args,
Action::OffloadKind DeviceOffloadKind) const {
-  if (isAlignedAllocationUnavailable())
+  // Pass "-faligned-alloc-unavailable" only when the user hasn't manually
+  // enabled or disabled aligned allocations.
+  if (!DriverArgs.hasArgNoClaim(op

[PATCH] D45015: [Preprocessor] Allow libc++ to detect when aligned allocation is unavailable.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In https://reviews.llvm.org/D45015#1188141, @EricWF wrote:

> Hi, I'm in the process of moving cities. please take over.
>
> Thanks, and sorry


No worries. Thanks for spending time on this issue and making the fix, 
committing is the easy part. Good luck with the move.


Repository:
  rL LLVM

https://reviews.llvm.org/D45015



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

[PATCH] D42938: [Sema] Emit -Winteger-overflow for arguments in function calls, ObjC messages.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added reviewers: ahatanak, nicholas, rsmith.
Herald added a subscriber: jkorous-apple.

Objective-C properties aren't handled on purpose as they require
different approach.

rdar://problem/35539384


https://reviews.llvm.org/D42938

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/integer-overflow.c
  clang/test/SemaCXX/integer-overflow.cpp
  clang/test/SemaObjC/integer-overflow.m


Index: clang/test/SemaObjC/integer-overflow.m
===
--- /dev/null
+++ clang/test/SemaObjC/integer-overflow.m
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -Wno-objc-root-class -fsyntax-only -verify %s
+
+@interface Foo
+@end
+
+@implementation Foo
+- (int)add:(int)a with:(int)b {
+  return a + b;
+}
+
+- (void)testIntegerOverflows {
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 
'int'}}
+  (void)[self add:0 with:4608 * 1024 * 1024];
+}
+@end
Index: clang/test/SemaCXX/integer-overflow.cpp
===
--- clang/test/SemaCXX/integer-overflow.cpp
+++ clang/test/SemaCXX/integer-overflow.cpp
@@ -169,3 +169,15 @@
 // expected-warning@+1 2{{overflow in expression; result is 536870912 with 
type 'int'}}
   return ((4608 * 1024 * 1024) + ((uint64_t)(4608 * 1024 * 1024)));
 }
+
+void check_integer_overflows_in_function_calls() {
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 
'int'}}
+  (void)f0(4608 * 1024 * 1024);
+
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 
'int'}}
+  uint64_t x = f0(4608 * 1024 * 1024);
+
+// expected-warning@+2 {{overflow in expression; result is 536870912 with type 
'int'}}
+  uint64_t (*f0_ptr)(uint64_t) = &f0;
+  (void)(*f0_ptr)(4608 * 1024 * 1024);
+}
Index: clang/test/Sema/integer-overflow.c
===
--- clang/test/Sema/integer-overflow.c
+++ clang/test/Sema/integer-overflow.c
@@ -158,6 +158,18 @@
   return ((4608 * 1024 * 1024) + ((uint64_t)(4608 * 1024 * 1024)));
 }
 
+void check_integer_overflows_in_function_calls() {
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 
'int'}}
+  (void)f0(4608 * 1024 * 1024);
+
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 
'int'}}
+  uint64_t x = f0(4608 * 1024 * 1024);
+
+// expected-warning@+2 {{overflow in expression; result is 536870912 with type 
'int'}}
+  uint64_t (*f0_ptr)(uint64_t) = &f0;
+  (void)(*f0_ptr)(4608 * 1024 * 1024);
+}
+
 struct s {
   unsigned x;
   unsigned y;
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -10201,18 +10201,22 @@
   SmallVector Exprs(1, E);
 
   do {
-Expr *E = Exprs.pop_back_val();
+Expr *OriginalE = Exprs.pop_back_val();
+Expr *E = OriginalE->IgnoreParenCasts();
 
-if (isa(E->IgnoreParenCasts())) {
-  E->IgnoreParenCasts()->EvaluateForOverflow(Context);
+if (isa(E)) {
+  E->EvaluateForOverflow(Context);
   continue;
 }
 
-if (auto InitList = dyn_cast(E))
+if (auto InitList = dyn_cast(OriginalE))
   Exprs.append(InitList->inits().begin(), InitList->inits().end());
-
-if (isa(E))
-  E->IgnoreParenCasts()->EvaluateForOverflow(Context);
+else if (isa(OriginalE))
+  E->EvaluateForOverflow(Context);
+else if (auto Call = dyn_cast(E))
+  Exprs.append(Call->arg_begin(), Call->arg_end());
+else if (auto Message = dyn_cast(E))
+  Exprs.append(Message->arg_begin(), Message->arg_end());
   } while (!Exprs.empty());
 }
 


Index: clang/test/SemaObjC/integer-overflow.m
===
--- /dev/null
+++ clang/test/SemaObjC/integer-overflow.m
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -Wno-objc-root-class -fsyntax-only -verify %s
+
+@interface Foo
+@end
+
+@implementation Foo
+- (int)add:(int)a with:(int)b {
+  return a + b;
+}
+
+- (void)testIntegerOverflows {
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
+  (void)[self add:0 with:4608 * 1024 * 1024];
+}
+@end
Index: clang/test/SemaCXX/integer-overflow.cpp
===
--- clang/test/SemaCXX/integer-overflow.cpp
+++ clang/test/SemaCXX/integer-overflow.cpp
@@ -169,3 +169,15 @@
 // expected-warning@+1 2{{overflow in expression; result is 536870912 with type 'int'}}
   return ((4608 * 1024 * 1024) + ((uint64_t)(4608 * 1024 * 1024)));
 }
+
+void check_integer_overflows_in_function_calls() {
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
+  (void)f0(4608 * 1024 * 1024);
+
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
+  uint64_t x = f0(4608 * 1024 * 1024);
+
+// expected-warning@+2 {{overflow i

[PATCH] D42938: [Sema] Emit -Winteger-overflow for arguments in function calls, ObjC messages.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 132917.
vsapsai added a comment.

- Improve tests: check when function argument is a function call itself.


https://reviews.llvm.org/D42938

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/integer-overflow.c
  clang/test/SemaCXX/integer-overflow.cpp
  clang/test/SemaObjC/integer-overflow.m

Index: clang/test/SemaObjC/integer-overflow.m
===
--- /dev/null
+++ clang/test/SemaObjC/integer-overflow.m
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -Wno-objc-root-class -fsyntax-only -verify %s
+
+@interface Foo
+@end
+
+@implementation Foo
+- (int)add:(int)a with:(int)b {
+  return a + b;
+}
+
+- (void)testIntegerOverflows {
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
+  (void)[self add:0 with:4608 * 1024 * 1024];
+
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
+  (void)[self add:0 with:[self add:4608 * 1024 * 1024 with:0]];
+}
+@end
Index: clang/test/SemaCXX/integer-overflow.cpp
===
--- clang/test/SemaCXX/integer-overflow.cpp
+++ clang/test/SemaCXX/integer-overflow.cpp
@@ -169,3 +169,18 @@
 // expected-warning@+1 2{{overflow in expression; result is 536870912 with type 'int'}}
   return ((4608 * 1024 * 1024) + ((uint64_t)(4608 * 1024 * 1024)));
 }
+
+void check_integer_overflows_in_function_calls() {
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
+  (void)f0(4608 * 1024 * 1024);
+
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
+  uint64_t x = f0(4608 * 1024 * 1024);
+
+// expected-warning@+2 {{overflow in expression; result is 536870912 with type 'int'}}
+  uint64_t (*f0_ptr)(uint64_t) = &f0;
+  (void)(*f0_ptr)(4608 * 1024 * 1024);
+
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
+  (void)f2(0, f0(4608 * 1024 * 1024));
+}
Index: clang/test/Sema/integer-overflow.c
===
--- clang/test/Sema/integer-overflow.c
+++ clang/test/Sema/integer-overflow.c
@@ -158,6 +158,21 @@
   return ((4608 * 1024 * 1024) + ((uint64_t)(4608 * 1024 * 1024)));
 }
 
+void check_integer_overflows_in_function_calls() {
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
+  (void)f0(4608 * 1024 * 1024);
+
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
+  uint64_t x = f0(4608 * 1024 * 1024);
+
+// expected-warning@+2 {{overflow in expression; result is 536870912 with type 'int'}}
+  uint64_t (*f0_ptr)(uint64_t) = &f0;
+  (void)(*f0_ptr)(4608 * 1024 * 1024);
+
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
+  (void)f2(0, f0(4608 * 1024 * 1024));
+}
+
 struct s {
   unsigned x;
   unsigned y;
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -10201,18 +10201,22 @@
   SmallVector Exprs(1, E);
 
   do {
-Expr *E = Exprs.pop_back_val();
+Expr *OriginalE = Exprs.pop_back_val();
+Expr *E = OriginalE->IgnoreParenCasts();
 
-if (isa(E->IgnoreParenCasts())) {
-  E->IgnoreParenCasts()->EvaluateForOverflow(Context);
+if (isa(E)) {
+  E->EvaluateForOverflow(Context);
   continue;
 }
 
-if (auto InitList = dyn_cast(E))
+if (auto InitList = dyn_cast(OriginalE))
   Exprs.append(InitList->inits().begin(), InitList->inits().end());
-
-if (isa(E))
-  E->IgnoreParenCasts()->EvaluateForOverflow(Context);
+else if (isa(OriginalE))
+  E->EvaluateForOverflow(Context);
+else if (auto Call = dyn_cast(E))
+  Exprs.append(Call->arg_begin(), Call->arg_end());
+else if (auto Message = dyn_cast(E))
+  Exprs.append(Message->arg_begin(), Message->arg_end());
   } while (!Exprs.empty());
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41834: [Lex] Fix handling numerical literals ending with ' and signed exponent.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Thanks for the review.


https://reviews.llvm.org/D41834



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

[PATCH] D41834: [Lex] Fix handling numerical literals ending with ' and signed exponent.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL324419: [Lex] Fix handling numerical literals ending with 
' and signed exponent. (authored by vsapsai, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D41834?vs=129125&id=133084#toc

Repository:
  rC Clang

https://reviews.llvm.org/D41834

Files:
  cfe/trunk/lib/Lex/LiteralSupport.cpp
  cfe/trunk/test/Lexer/cxx1y_digit_separators.cpp


Index: cfe/trunk/test/Lexer/cxx1y_digit_separators.cpp
===
--- cfe/trunk/test/Lexer/cxx1y_digit_separators.cpp
+++ cfe/trunk/test/Lexer/cxx1y_digit_separators.cpp
@@ -51,6 +51,8 @@
   float u = 0x.'p1f; // expected-error {{hexadecimal floating literal requires 
a significand}}
   float v = 0e'f; // expected-error {{exponent has no digits}}
   float w = 0x0p'f; // expected-error {{exponent has no digits}}
+  float x = 0'e+1; // expected-error {{digit separator cannot appear at end of 
digit sequence}}
+  float y = 0x0'p+1; // expected-error {{digit separator cannot appear at end 
of digit sequence}}
 }
 
 #line 123'456
Index: cfe/trunk/lib/Lex/LiteralSupport.cpp
===
--- cfe/trunk/lib/Lex/LiteralSupport.cpp
+++ cfe/trunk/lib/Lex/LiteralSupport.cpp
@@ -738,15 +738,17 @@
 s++;
 radix = 10;
 saw_exponent = true;
-if (*s == '+' || *s == '-')  s++; // sign
+if (s != ThisTokEnd && (*s == '+' || *s == '-'))  s++; // sign
 const char *first_non_digit = SkipDigits(s);
 if (containsDigits(s, first_non_digit)) {
   checkSeparator(TokLoc, s, CSK_BeforeDigits);
   s = first_non_digit;
 } else {
-  PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, Exponent-ThisTokBegin),
-  diag::err_exponent_has_no_digits);
-  hadError = true;
+  if (!hadError) {
+PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, Exponent-ThisTokBegin),
+diag::err_exponent_has_no_digits);
+hadError = true;
+  }
   return;
 }
   }
@@ -787,10 +789,12 @@
   } else if (Pos == ThisTokEnd)
 return;
 
-  if (isDigitSeparator(*Pos))
+  if (isDigitSeparator(*Pos)) {
 PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, Pos - ThisTokBegin),
 diag::err_digit_separator_not_between_digits)
   << IsAfterDigits;
+hadError = true;
+  }
 }
 
 /// ParseNumberStartingWithZero - This method is called when the first 
character
@@ -840,12 +844,14 @@
   const char *Exponent = s;
   s++;
   saw_exponent = true;
-  if (*s == '+' || *s == '-')  s++; // sign
+  if (s != ThisTokEnd && (*s == '+' || *s == '-'))  s++; // sign
   const char *first_non_digit = SkipDigits(s);
   if (!containsDigits(s, first_non_digit)) {
-PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, Exponent-ThisTokBegin),
-diag::err_exponent_has_no_digits);
-hadError = true;
+if (!hadError) {
+  PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, Exponent-ThisTokBegin),
+  diag::err_exponent_has_no_digits);
+  hadError = true;
+}
 return;
   }
   checkSeparator(TokLoc, s, CSK_BeforeDigits);


Index: cfe/trunk/test/Lexer/cxx1y_digit_separators.cpp
===
--- cfe/trunk/test/Lexer/cxx1y_digit_separators.cpp
+++ cfe/trunk/test/Lexer/cxx1y_digit_separators.cpp
@@ -51,6 +51,8 @@
   float u = 0x.'p1f; // expected-error {{hexadecimal floating literal requires a significand}}
   float v = 0e'f; // expected-error {{exponent has no digits}}
   float w = 0x0p'f; // expected-error {{exponent has no digits}}
+  float x = 0'e+1; // expected-error {{digit separator cannot appear at end of digit sequence}}
+  float y = 0x0'p+1; // expected-error {{digit separator cannot appear at end of digit sequence}}
 }
 
 #line 123'456
Index: cfe/trunk/lib/Lex/LiteralSupport.cpp
===
--- cfe/trunk/lib/Lex/LiteralSupport.cpp
+++ cfe/trunk/lib/Lex/LiteralSupport.cpp
@@ -738,15 +738,17 @@
 s++;
 radix = 10;
 saw_exponent = true;
-if (*s == '+' || *s == '-')  s++; // sign
+if (s != ThisTokEnd && (*s == '+' || *s == '-'))  s++; // sign
 const char *first_non_digit = SkipDigits(s);
 if (containsDigits(s, first_non_digit)) {
   checkSeparator(TokLoc, s, CSK_BeforeDigits);
   s = first_non_digit;
 } else {
-  PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, Exponent-ThisTokBegin),
-  diag::err_exponent_has_no_digits);
-  hadError = true;
+  if (!hadError) {
+PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, Exponent-ThisTokBegin),
+diag::err_exponent_has_no_digits);
+hadError = true;
+  }
   return;
 }
   }
@@ -787,10 +789,12 @@
   } else if (Pos == ThisTokEnd)
 return;
 
-  if (isDigitSeparator(*Pos))
+  i

[PATCH] D41834: [Lex] Fix handling numerical literals ending with ' and signed exponent.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC324419: [Lex] Fix handling numerical literals ending with 
' and signed exponent. (authored by vsapsai, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D41834?vs=129125&id=133085#toc

Repository:
  rC Clang

https://reviews.llvm.org/D41834

Files:
  lib/Lex/LiteralSupport.cpp
  test/Lexer/cxx1y_digit_separators.cpp


Index: lib/Lex/LiteralSupport.cpp
===
--- lib/Lex/LiteralSupport.cpp
+++ lib/Lex/LiteralSupport.cpp
@@ -738,15 +738,17 @@
 s++;
 radix = 10;
 saw_exponent = true;
-if (*s == '+' || *s == '-')  s++; // sign
+if (s != ThisTokEnd && (*s == '+' || *s == '-'))  s++; // sign
 const char *first_non_digit = SkipDigits(s);
 if (containsDigits(s, first_non_digit)) {
   checkSeparator(TokLoc, s, CSK_BeforeDigits);
   s = first_non_digit;
 } else {
-  PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, Exponent-ThisTokBegin),
-  diag::err_exponent_has_no_digits);
-  hadError = true;
+  if (!hadError) {
+PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, Exponent-ThisTokBegin),
+diag::err_exponent_has_no_digits);
+hadError = true;
+  }
   return;
 }
   }
@@ -787,10 +789,12 @@
   } else if (Pos == ThisTokEnd)
 return;
 
-  if (isDigitSeparator(*Pos))
+  if (isDigitSeparator(*Pos)) {
 PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, Pos - ThisTokBegin),
 diag::err_digit_separator_not_between_digits)
   << IsAfterDigits;
+hadError = true;
+  }
 }
 
 /// ParseNumberStartingWithZero - This method is called when the first 
character
@@ -840,12 +844,14 @@
   const char *Exponent = s;
   s++;
   saw_exponent = true;
-  if (*s == '+' || *s == '-')  s++; // sign
+  if (s != ThisTokEnd && (*s == '+' || *s == '-'))  s++; // sign
   const char *first_non_digit = SkipDigits(s);
   if (!containsDigits(s, first_non_digit)) {
-PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, Exponent-ThisTokBegin),
-diag::err_exponent_has_no_digits);
-hadError = true;
+if (!hadError) {
+  PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, Exponent-ThisTokBegin),
+  diag::err_exponent_has_no_digits);
+  hadError = true;
+}
 return;
   }
   checkSeparator(TokLoc, s, CSK_BeforeDigits);
Index: test/Lexer/cxx1y_digit_separators.cpp
===
--- test/Lexer/cxx1y_digit_separators.cpp
+++ test/Lexer/cxx1y_digit_separators.cpp
@@ -51,6 +51,8 @@
   float u = 0x.'p1f; // expected-error {{hexadecimal floating literal requires 
a significand}}
   float v = 0e'f; // expected-error {{exponent has no digits}}
   float w = 0x0p'f; // expected-error {{exponent has no digits}}
+  float x = 0'e+1; // expected-error {{digit separator cannot appear at end of 
digit sequence}}
+  float y = 0x0'p+1; // expected-error {{digit separator cannot appear at end 
of digit sequence}}
 }
 
 #line 123'456


Index: lib/Lex/LiteralSupport.cpp
===
--- lib/Lex/LiteralSupport.cpp
+++ lib/Lex/LiteralSupport.cpp
@@ -738,15 +738,17 @@
 s++;
 radix = 10;
 saw_exponent = true;
-if (*s == '+' || *s == '-')  s++; // sign
+if (s != ThisTokEnd && (*s == '+' || *s == '-'))  s++; // sign
 const char *first_non_digit = SkipDigits(s);
 if (containsDigits(s, first_non_digit)) {
   checkSeparator(TokLoc, s, CSK_BeforeDigits);
   s = first_non_digit;
 } else {
-  PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, Exponent-ThisTokBegin),
-  diag::err_exponent_has_no_digits);
-  hadError = true;
+  if (!hadError) {
+PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, Exponent-ThisTokBegin),
+diag::err_exponent_has_no_digits);
+hadError = true;
+  }
   return;
 }
   }
@@ -787,10 +789,12 @@
   } else if (Pos == ThisTokEnd)
 return;
 
-  if (isDigitSeparator(*Pos))
+  if (isDigitSeparator(*Pos)) {
 PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, Pos - ThisTokBegin),
 diag::err_digit_separator_not_between_digits)
   << IsAfterDigits;
+hadError = true;
+  }
 }
 
 /// ParseNumberStartingWithZero - This method is called when the first character
@@ -840,12 +844,14 @@
   const char *Exponent = s;
   s++;
   saw_exponent = true;
-  if (*s == '+' || *s == '-')  s++; // sign
+  if (s != ThisTokEnd && (*s == '+' || *s == '-'))  s++; // sign
   const char *first_non_digit = SkipDigits(s);
   if (!containsDigits(s, first_non_digit)) {
-PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, Exponent-ThisTokBegin),
-diag::err_exponent_has_no_digits);
-hadError = true;
+if (!hadError) {
+  PP.Diag(PP.AdvanceToTokenCharacter(

[PATCH] D42498: [ExprConstant] Fix crash when initialize an indirect field with another field.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.
Herald added a subscriber: jkorous-apple.

Ping.


https://reviews.llvm.org/D42498



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

[PATCH] D42755: [libcxx] Fix last_write_time tests for filesystems that don't support very small times.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: 
libcxx/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp:360
-
-ec = GetTestEC();
-last_write_time(p, Clock::now());

EricWF wrote:
> I would really love to keep this test. I think it has the ability to detect 
> incorrect integer arithmetic in `last_write_time` when UBSAN is enabled.
> 
> Could we maybe add another check like `SupportsAlmostMinTime` where that 
> checks `::min() + MicroSec(1)`?
What about nesting removed part in `TimeIsRepresentableByFilesystem` like
```lang=c++
if (TimeIsRepresentableByFilesystem(new_time)) {
TEST_CHECK(!ec);
TEST_CHECK(tt >= new_time);
TEST_CHECK(tt < new_time + Sec(1));

ec = GetTestEC();
last_write_time(p, Clock::now());

new_time = file_time_type::min() + MicroSec(1);

last_write_time(p, new_time, ec);
tt = last_write_time(p);

if (TimeIsRepresentableByFilesystem(new_time)) {
TEST_CHECK(!ec);
TEST_CHECK(tt >= new_time);
TEST_CHECK(tt < new_time + Sec(1));
}
}
```

As for me, I don't like how it looks. So maybe the same approach but with a 
flag like
```lang=c++
bool supports_min_time = false;
if (TimeIsRepresentableByFilesystem(new_time)) {
TEST_CHECK(!ec);
TEST_CHECK(tt >= new_time);
TEST_CHECK(tt < new_time + Sec(1));
supports_min_time = true;
}

ec = GetTestEC();
last_write_time(p, Clock::now());

new_time = file_time_type::min() + MicroSec(1);

last_write_time(p, new_time, ec);
tt = last_write_time(p);

if (supports_min_time && TimeIsRepresentableByFilesystem(new_time)) {
TEST_CHECK(!ec);
TEST_CHECK(tt >= new_time);
TEST_CHECK(tt < new_time + Sec(1));
}
```

Yet another option is to leverage that APFS truncates time to supported value, 
something like
```if ((old_tt < new_time + Sec(1)) && 
TimeIsRepresentableByFilesystem(new_time))```
I don't like it because it implicitly relies on time truncation which is not 
guaranteed for all filesystems and it is slightly harder to understand than 
boolean.

How do you like these ideas? `SupportsAlmostMinTime` can also work but I'd like 
to avoid repeating `+ MicroSec(1)` part in 2 places.


https://reviews.llvm.org/D42755



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

[PATCH] D43494: [Modules] Fix creating fake definition data for lambdas.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added reviewers: rsmith, bruno.
Herald added a subscriber: jkorous-apple.

During reading C++ definition data for lambda we can access
CXXRecordDecl representing lambda before we finished reading the
definition data. This can happen by reading a captured variable which is
VarDecl, then reading its decl context which is CXXMethodDecl `operator()`,
then trying to merge redeclarable methods and accessing
enclosing CXXRecordDecl. The call stack looks roughly like

  VisitCXXRecordDecl
ReadCXXRecordDefinition
  VisitVarDecl
VisitCXXMethodDecl
  mergeRedeclarable
getPrimaryContextForMerging

If we add fake definition data at this point, later we'll hit the assertion

  Assertion failed: (!DD.IsLambda && !MergeDD.IsLambda && "faked up lambda 
definition?"), function MergeDefinitionData, file 
clang/lib/Serialization/ASTReaderDecl.cpp, line 1675.

The fix is to check if we are still reading real definition data and to
avoid adding a fake one in this case. Fixes PR32556.

rdar://problem/37461072


https://reviews.llvm.org/D43494

Files:
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/Inputs/self-referencing-lambda/a.h
  clang/test/Modules/Inputs/self-referencing-lambda/module.modulemap
  clang/test/Modules/self-referencing-lambda.cpp


Index: clang/test/Modules/self-referencing-lambda.cpp
===
--- /dev/null
+++ clang/test/Modules/self-referencing-lambda.cpp
@@ -0,0 +1,5 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I 
%S/Inputs/self-referencing-lambda %s -verify -emit-obj -o %t2.o
+// expected-no-diagnostics
+
+#include "a.h"
Index: clang/test/Modules/Inputs/self-referencing-lambda/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/self-referencing-lambda/module.modulemap
@@ -0,0 +1,4 @@
+module "a.h" {
+  header "a.h"
+  export *
+}
Index: clang/test/Modules/Inputs/self-referencing-lambda/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/self-referencing-lambda/a.h
@@ -0,0 +1,4 @@
+void f() {
+  int x = 0;
+  auto q = [xm = x]{};
+}
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -1782,7 +1782,10 @@
   else
 DD = new (C) struct CXXRecordDecl::DefinitionData(D);
 
+  assert(!D->IsBeingDefined && "Should not overwrite definition in progress");
+  D->IsBeingDefined = true;
   ReadCXXDefinitionData(*DD, D);
+  D->IsBeingDefined = false;
 
   // We might already have a definition for this record. This can happen either
   // because we're reading an update record, or because we've already done some
@@ -2963,6 +2966,9 @@
 if (!DD)
   DD = RD->getCanonicalDecl()->DefinitionData;
 
+if (!DD && RD->isBeingDefined())
+  return nullptr;
+
 // If there's no definition yet, then DC's definition is added by an update
 // record, but we've not yet loaded that update record. In this case, we
 // commit to DC being the canonical definition now, and will fix this when


Index: clang/test/Modules/self-referencing-lambda.cpp
===
--- /dev/null
+++ clang/test/Modules/self-referencing-lambda.cpp
@@ -0,0 +1,5 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/self-referencing-lambda %s -verify -emit-obj -o %t2.o
+// expected-no-diagnostics
+
+#include "a.h"
Index: clang/test/Modules/Inputs/self-referencing-lambda/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/self-referencing-lambda/module.modulemap
@@ -0,0 +1,4 @@
+module "a.h" {
+  header "a.h"
+  export *
+}
Index: clang/test/Modules/Inputs/self-referencing-lambda/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/self-referencing-lambda/a.h
@@ -0,0 +1,4 @@
+void f() {
+  int x = 0;
+  auto q = [xm = x]{};
+}
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -1782,7 +1782,10 @@
   else
 DD = new (C) struct CXXRecordDecl::DefinitionData(D);
 
+  assert(!D->IsBeingDefined && "Should not overwrite definition in progress");
+  D->IsBeingDefined = true;
   ReadCXXDefinitionData(*DD, D);
+  D->IsBeingDefined = false;
 
   // We might already have a definition for this record. This can happen either
   // because we're reading an update record, or because we've already done some
@@ -2963,6 +2966,9 @@
 if (!DD)
   DD

[PATCH] D42755: [libcxx] Fix last_write_time tests for filesystems that don't support very small times.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 135003.
vsapsai added a comment.
Herald added a subscriber: christof.

- Add back existing test but run it only when file system supports min time.


https://reviews.llvm.org/D42755

Files:
  
libcxx/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp


Index: 
libcxx/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
===
--- 
libcxx/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
+++ 
libcxx/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
@@ -116,12 +116,31 @@
 return !ec && new_write_time > max_sec - 1;
 }
 
+bool TestSupportsMinTime() {
+using namespace std::chrono;
+using Lim = std::numeric_limits;
+auto min_sec = 
duration_cast(file_time_type::min().time_since_epoch()).count();
+if (min_sec < Lim::min()) return false;
+std::error_code ec;
+std::time_t old_write_time, new_write_time;
+{ // WARNING: Do not assert in this scope.
+  scoped_test_env env;
+  const path file = env.create_file("file", 42);
+  old_write_time = LastWriteTime(file);
+  file_time_type tp = file_time_type::min();
+  fs::last_write_time(file, tp, ec);
+  new_write_time = LastWriteTime(file);
+}
+return !ec && new_write_time < min_sec + 1;
+}
+
 #if defined(__clang__)
 #pragma clang diagnostic pop
 #endif
 
 static const bool SupportsNegativeTimes = TestSupportsNegativeTimes();
 static const bool SupportsMaxTime = TestSupportsMaxTime();
+static const bool SupportsMinTime = TestSupportsMinTime();
 
 } // end namespace
 
@@ -140,14 +159,17 @@
 // (B) 'tp' is non-negative or the filesystem supports negative times.
 // (C) 'tp' is not 'file_time_type::max()' or the filesystem supports the max
 // value.
+// (D) 'tp' is not 'file_time_type::min()' or the filesystem supports the min
+// value.
 inline bool TimeIsRepresentableByFilesystem(file_time_type tp) {
 using namespace std::chrono;
 using Lim = std::numeric_limits;
 auto sec = duration_cast(tp.time_since_epoch()).count();
 auto microsec = duration_cast(tp.time_since_epoch()).count();
 if (sec < Lim::min() || sec > Lim::max())   return false;
 else if (microsec < 0 && !SupportsNegativeTimes) return false;
 else if (tp == file_time_type::max() && !SupportsMaxTime) return false;
+else if (tp == file_time_type::min() && !SupportsMinTime) return false;
 return true;
 }
 
@@ -355,20 +377,20 @@
 TEST_CHECK(!ec);
 TEST_CHECK(tt >= new_time);
 TEST_CHECK(tt < new_time + Sec(1));
-}
 
-ec = GetTestEC();
-last_write_time(p, Clock::now());
+ec = GetTestEC();
+last_write_time(p, Clock::now());
 
-new_time = file_time_type::min() + MicroSec(1);
+new_time = file_time_type::min() + MicroSec(1);
 
-last_write_time(p, new_time, ec);
-tt = last_write_time(p);
+last_write_time(p, new_time, ec);
+tt = last_write_time(p);
 
-if (TimeIsRepresentableByFilesystem(new_time)) {
-TEST_CHECK(!ec);
-TEST_CHECK(tt >= new_time);
-TEST_CHECK(tt < new_time + Sec(1));
+if (TimeIsRepresentableByFilesystem(new_time)) {
+TEST_CHECK(!ec);
+TEST_CHECK(tt >= new_time);
+TEST_CHECK(tt < new_time + Sec(1));
+}
 }
 }
 


Index: libcxx/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
===
--- libcxx/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
+++ libcxx/test/std/experimental/filesystem/fs.op.funcs/fs.op.last_write_time/last_write_time.pass.cpp
@@ -116,12 +116,31 @@
 return !ec && new_write_time > max_sec - 1;
 }
 
+bool TestSupportsMinTime() {
+using namespace std::chrono;
+using Lim = std::numeric_limits;
+auto min_sec = duration_cast(file_time_type::min().time_since_epoch()).count();
+if (min_sec < Lim::min()) return false;
+std::error_code ec;
+std::time_t old_write_time, new_write_time;
+{ // WARNING: Do not assert in this scope.
+  scoped_test_env env;
+  const path file = env.create_file("file", 42);
+  old_write_time = LastWriteTime(file);
+  file_time_type tp = file_time_type::min();
+  fs::last_write_time(file, tp, ec);
+  new_write_time = LastWriteTime(file);
+}
+return !ec && new_write_time < min_sec + 1;
+}
+
 #if defined(__clang__)
 #pragma clang diagnostic pop
 #endif
 
 static const bool SupportsNegativeTimes = TestSupportsNegativeTimes();
 static const bool SupportsMaxTime = TestSupportsMaxTime();
+static const bool SupportsMinTime = TestSupportsMinTime();
 
 } // end namespace
 
@@ -140,14 +159,17 @@
 // (B) 'tp' is non-negative or the filesystem supports 

[PATCH] D43494: [Modules] Fix creating fake definition data for lambdas.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:2969
 
+if (!DD && RD->isBeingDefined())
+  return nullptr;

aprantl wrote:
> Perhaps add a comment explaining what's going on in this early exit?
While I was trying to explain this early exit, I realized one of my assumptions 
was incorrect. Now I'm not sure returning nullptr is the right fix, maybe 
assigning incomplete definition data like

```lang=c++
D->DefinitionData = DD;
ReadCXXDefinitionData(*DD, D);
```
is better.

I am trying to come up with a test case showing different behaviour for these 2 
different fixes. But so far nothing yet. If anybody can tell which one is 
correct: null context for merging or not-yet-populated definition data, that 
would be helpful.


https://reviews.llvm.org/D43494



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

[PATCH] D43494: [Modules] Fix creating fake definition data for lambdas.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:2969
 
+if (!DD && RD->isBeingDefined())
+  return nullptr;

vsapsai wrote:
> aprantl wrote:
> > Perhaps add a comment explaining what's going on in this early exit?
> While I was trying to explain this early exit, I realized one of my 
> assumptions was incorrect. Now I'm not sure returning nullptr is the right 
> fix, maybe assigning incomplete definition data like
> 
> ```lang=c++
> D->DefinitionData = DD;
> ReadCXXDefinitionData(*DD, D);
> ```
> is better.
> 
> I am trying to come up with a test case showing different behaviour for these 
> 2 different fixes. But so far nothing yet. If anybody can tell which one is 
> correct: null context for merging or not-yet-populated definition data, that 
> would be helpful.
Looks like both fix approaches should work as in 
`ASTDeclReader::ReadCXXDefinitionData` we read decl only for lambda. If I'm not 
mistaken, for lambdas merging doesn't really make sense, so no context for 
merging doesn't have repercussions.


https://reviews.llvm.org/D43494



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

[PATCH] D43494: [Modules] Fix creating fake definition data for lambdas.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 135179.
vsapsai added a comment.

- Set `D->DefinitionData` early.

We already call `MergeDefinitionData` after definition data deserialization is
complete. And I removed previously added `IsBeingDefined`, so left the "all the
bits must match" checks in `MergeDefinitionData` as-is.

As for testing, requirement for `Canon->DefinitionData != DD` check is already
enforced by existing tests.


https://reviews.llvm.org/D43494

Files:
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/Inputs/self-referencing-lambda/a.h
  clang/test/Modules/Inputs/self-referencing-lambda/module.modulemap
  clang/test/Modules/self-referencing-lambda.cpp


Index: clang/test/Modules/self-referencing-lambda.cpp
===
--- /dev/null
+++ clang/test/Modules/self-referencing-lambda.cpp
@@ -0,0 +1,5 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I 
%S/Inputs/self-referencing-lambda %s -verify -emit-obj -o %t2.o
+// expected-no-diagnostics
+
+#include "a.h"
Index: clang/test/Modules/Inputs/self-referencing-lambda/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/self-referencing-lambda/module.modulemap
@@ -0,0 +1,4 @@
+module "a.h" {
+  header "a.h"
+  export *
+}
Index: clang/test/Modules/Inputs/self-referencing-lambda/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/self-referencing-lambda/a.h
@@ -0,0 +1,4 @@
+void f() {
+  int x = 0;
+  auto q = [xm = x]{};
+}
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -1782,21 +1782,24 @@
   else
 DD = new (C) struct CXXRecordDecl::DefinitionData(D);
 
+  // Set definition data before reading it, so that during deserialization when
+  // we read CXXRecordDecl, it already has definition data and we don't set 
fake
+  // one.
+  D->DefinitionData = DD;
   ReadCXXDefinitionData(*DD, D);
 
   // We might already have a definition for this record. This can happen either
   // because we're reading an update record, or because we've already done some
   // merging. Either way, just merge into it.
   CXXRecordDecl *Canon = D->getCanonicalDecl();
-  if (Canon->DefinitionData) {
+  if (Canon->DefinitionData && Canon->DefinitionData != DD) {
 MergeDefinitionData(Canon, std::move(*DD));
 D->DefinitionData = Canon->DefinitionData;
 return;
   }
 
   // Mark this declaration as being a definition.
   D->IsCompleteDefinition = true;
-  D->DefinitionData = DD;
 
   // If this is not the first declaration or is an update record, we can have
   // other redeclarations already. Make a note that we need to propagate the


Index: clang/test/Modules/self-referencing-lambda.cpp
===
--- /dev/null
+++ clang/test/Modules/self-referencing-lambda.cpp
@@ -0,0 +1,5 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/self-referencing-lambda %s -verify -emit-obj -o %t2.o
+// expected-no-diagnostics
+
+#include "a.h"
Index: clang/test/Modules/Inputs/self-referencing-lambda/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/self-referencing-lambda/module.modulemap
@@ -0,0 +1,4 @@
+module "a.h" {
+  header "a.h"
+  export *
+}
Index: clang/test/Modules/Inputs/self-referencing-lambda/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/self-referencing-lambda/a.h
@@ -0,0 +1,4 @@
+void f() {
+  int x = 0;
+  auto q = [xm = x]{};
+}
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -1782,21 +1782,24 @@
   else
 DD = new (C) struct CXXRecordDecl::DefinitionData(D);
 
+  // Set definition data before reading it, so that during deserialization when
+  // we read CXXRecordDecl, it already has definition data and we don't set fake
+  // one.
+  D->DefinitionData = DD;
   ReadCXXDefinitionData(*DD, D);
 
   // We might already have a definition for this record. This can happen either
   // because we're reading an update record, or because we've already done some
   // merging. Either way, just merge into it.
   CXXRecordDecl *Canon = D->getCanonicalDecl();
-  if (Canon->DefinitionData) {
+  if (Canon->DefinitionData && Canon->DefinitionData != DD) {
 MergeDefinitionData(Canon, std::move(*DD));
 D->DefinitionData = Canon->DefinitionData;
 return;
   }
 
   // Mark this declaration as being a definition.
   D->IsCompleteDef

[PATCH] D43590: [clang-format] Fix regression when getStyle() called with empty filename

[Volodymyr Sapsai via Phabricator via cfe-commits]

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

I'm not familiar with this code, so post-commit review by someone more 
knowledgeable might be a good idea.

Code-wise it looks good. I feel uneasy about using fake file name, but it is 
exactly the case being tested, so that's not a problem.


Repository:
  rC Clang

https://reviews.llvm.org/D43590



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

[PATCH] D42498: [ExprConstant] Fix crash when initialize an indirect field with another field.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Thanks for response, Richard. I'll look into using `CXXDefaultInitExpr`.

As for

In https://reviews.llvm.org/D42498#1007028, @rsmith wrote:

> […] your approach will still go wrong if the `This` pointer is used in other 
> ways, such as an explicit mention of `this` or a member function call.


I wasn't able to find a failing test case. Maybe I misunderstood you but in my 
testing functions declared in named structs had correct `This` pointer and in 
anonymous structs functions cannot be declared.


https://reviews.llvm.org/D42498



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

[PATCH] D42498: [ExprConstant] Fix crash when initialize an indirect field with another field.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 135708.
vsapsai added a comment.

- Override `This` for indirect field initializer more like it is done for 
`InitListExpr`.

I rebased my changes, so the diff between different versions can be noisy.


https://reviews.llvm.org/D42498

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/constant-expression-cxx1y.cpp

Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1044,3 +1044,44 @@
   constexpr B b;
   constexpr int p = b.n; // expected-error {{constant expression}} expected-note {{mutable}}
 }
+
+namespace IndirectFields {
+
+// Reference indirect field.
+struct A {
+  struct {
+union {
+  int x = x = 3; // expected-note {{outside its lifetime}}
+};
+  };
+  constexpr A() {}
+};
+static_assert(A().x == 3, ""); // expected-error{{not an integral constant expression}} expected-note{{in call to 'A()'}}
+
+// Reference another indirect field, with different 'this'.
+struct B {
+  struct {
+union {
+  int x = 3;
+};
+int y = x;
+  };
+  constexpr B() {}
+};
+static_assert(B().y == 3, "");
+
+// Nested evaluation of indirect field initializers.
+struct C {
+  union {
+int x = 1;
+  };
+};
+struct D {
+  struct {
+C c;
+int y = c.x + 1;
+  };
+};
+static_assert(D().y == 2, "");
+
+} // namespace IndirectFields
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4383,6 +4383,7 @@
 #endif
   for (const auto *I : Definition->inits()) {
 LValue Subobject = This;
+LValue SubobjectParent = This;
 APValue *Value = &Result;
 
 // Determine the subobject to initialize.
@@ -4413,7 +4414,8 @@
 } else if (IndirectFieldDecl *IFD = I->getIndirectMember()) {
   // Walk the indirect field decl's chain to find the object to initialize,
   // and make sure we've initialized every step along it.
-  for (auto *C : IFD->chain()) {
+  auto IndirectFieldChain = IFD->chain();
+  for (auto *C : IndirectFieldChain) {
 FD = cast(C);
 CXXRecordDecl *CD = cast(FD->getParent());
 // Switch the union field if it differs. This happens if we had
@@ -4429,6 +4431,10 @@
 *Value = APValue(APValue::UninitStruct(), CD->getNumBases(),
  std::distance(CD->field_begin(), CD->field_end()));
 }
+// Store Subobject as its parent before updating it for the last element
+// in the chain.
+if (C == IndirectFieldChain.back())
+  SubobjectParent = Subobject;
 if (!HandleLValueMember(Info, I->getInit(), Subobject, FD))
   return false;
 if (CD->isUnion())
@@ -4440,10 +4446,16 @@
   llvm_unreachable("unknown base initializer kind");
 }
 
+// Need to override This for implicit field initializers as in this case
+// This refers to innermost anonymous struct/union containing initializer,
+// not to currently constructed class.
+const Expr *Init = I->getInit();
+ThisOverrideRAII ThisOverride(*Info.CurrentCall, &SubobjectParent,
+  isa(Init));
 FullExpressionRAII InitScope(Info);
-if (!EvaluateInPlace(*Value, Info, Subobject, I->getInit()) ||
-(FD && FD->isBitField() && !truncateBitfieldValue(Info, I->getInit(),
-  *Value, FD))) {
+if (!EvaluateInPlace(*Value, Info, Subobject, Init) ||
+(FD && FD->isBitField() &&
+ !truncateBitfieldValue(Info, Init, *Value, FD))) {
   // If we're checking for a potential constant expression, evaluate all
   // initializers even if some of them fail.
   if (!Info.noteFailure())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42498: [ExprConstant] Fix crash when initialize an indirect field with another field.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC325997: [ExprConstant] Fix crash when initialize an indirect 
field with another field. (authored by vsapsai, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D42498?vs=135708&id=135725#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D42498

Files:
  lib/AST/ExprConstant.cpp
  test/SemaCXX/constant-expression-cxx1y.cpp

Index: test/SemaCXX/constant-expression-cxx1y.cpp
===
--- test/SemaCXX/constant-expression-cxx1y.cpp
+++ test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1045,3 +1045,57 @@
   constexpr B b;
   constexpr int p = b.n; // expected-error {{constant expression}} expected-note {{mutable}}
 }
+
+namespace IndirectFields {
+
+// Reference indirect field.
+struct A {
+  struct {
+union {
+  int x = x = 3; // expected-note {{outside its lifetime}}
+};
+  };
+  constexpr A() {}
+};
+static_assert(A().x == 3, ""); // expected-error{{not an integral constant expression}} expected-note{{in call to 'A()'}}
+
+// Reference another indirect field, with different 'this'.
+struct B {
+  struct {
+union {
+  int x = 3;
+};
+int y = x;
+  };
+  constexpr B() {}
+};
+static_assert(B().y == 3, "");
+
+// Nested evaluation of indirect field initializers.
+struct C {
+  union {
+int x = 1;
+  };
+};
+struct D {
+  struct {
+C c;
+int y = c.x + 1;
+  };
+};
+static_assert(D().y == 2, "");
+
+// Explicit 'this'.
+struct E {
+  int n = 0;
+  struct {
+void *x = this;
+  };
+  void *y = this;
+};
+constexpr E e1 = E();
+static_assert(e1.x != e1.y, "");
+constexpr E e2 = E{0};
+static_assert(e2.x != e2.y, "");
+
+} // namespace IndirectFields
Index: lib/AST/ExprConstant.cpp
===
--- lib/AST/ExprConstant.cpp
+++ lib/AST/ExprConstant.cpp
@@ -4383,6 +4383,7 @@
 #endif
   for (const auto *I : Definition->inits()) {
 LValue Subobject = This;
+LValue SubobjectParent = This;
 APValue *Value = &Result;
 
 // Determine the subobject to initialize.
@@ -4413,7 +4414,8 @@
 } else if (IndirectFieldDecl *IFD = I->getIndirectMember()) {
   // Walk the indirect field decl's chain to find the object to initialize,
   // and make sure we've initialized every step along it.
-  for (auto *C : IFD->chain()) {
+  auto IndirectFieldChain = IFD->chain();
+  for (auto *C : IndirectFieldChain) {
 FD = cast(C);
 CXXRecordDecl *CD = cast(FD->getParent());
 // Switch the union field if it differs. This happens if we had
@@ -4429,6 +4431,10 @@
 *Value = APValue(APValue::UninitStruct(), CD->getNumBases(),
  std::distance(CD->field_begin(), CD->field_end()));
 }
+// Store Subobject as its parent before updating it for the last element
+// in the chain.
+if (C == IndirectFieldChain.back())
+  SubobjectParent = Subobject;
 if (!HandleLValueMember(Info, I->getInit(), Subobject, FD))
   return false;
 if (CD->isUnion())
@@ -4440,10 +4446,16 @@
   llvm_unreachable("unknown base initializer kind");
 }
 
+// Need to override This for implicit field initializers as in this case
+// This refers to innermost anonymous struct/union containing initializer,
+// not to currently constructed class.
+const Expr *Init = I->getInit();
+ThisOverrideRAII ThisOverride(*Info.CurrentCall, &SubobjectParent,
+  isa(Init));
 FullExpressionRAII InitScope(Info);
-if (!EvaluateInPlace(*Value, Info, Subobject, I->getInit()) ||
-(FD && FD->isBitField() && !truncateBitfieldValue(Info, I->getInit(),
-  *Value, FD))) {
+if (!EvaluateInPlace(*Value, Info, Subobject, Init) ||
+(FD && FD->isBitField() &&
+ !truncateBitfieldValue(Info, Init, *Value, FD))) {
   // If we're checking for a potential constant expression, evaluate all
   // initializers even if some of them fail.
   if (!Info.noteFailure())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42498: [ExprConstant] Fix crash when initialize an indirect field with another field.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL325997: [ExprConstant] Fix crash when initialize an indirect 
field with another field. (authored by vsapsai, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D42498?vs=135708&id=135726#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D42498

Files:
  cfe/trunk/lib/AST/ExprConstant.cpp
  cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp

Index: cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp
+++ cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1045,3 +1045,57 @@
   constexpr B b;
   constexpr int p = b.n; // expected-error {{constant expression}} expected-note {{mutable}}
 }
+
+namespace IndirectFields {
+
+// Reference indirect field.
+struct A {
+  struct {
+union {
+  int x = x = 3; // expected-note {{outside its lifetime}}
+};
+  };
+  constexpr A() {}
+};
+static_assert(A().x == 3, ""); // expected-error{{not an integral constant expression}} expected-note{{in call to 'A()'}}
+
+// Reference another indirect field, with different 'this'.
+struct B {
+  struct {
+union {
+  int x = 3;
+};
+int y = x;
+  };
+  constexpr B() {}
+};
+static_assert(B().y == 3, "");
+
+// Nested evaluation of indirect field initializers.
+struct C {
+  union {
+int x = 1;
+  };
+};
+struct D {
+  struct {
+C c;
+int y = c.x + 1;
+  };
+};
+static_assert(D().y == 2, "");
+
+// Explicit 'this'.
+struct E {
+  int n = 0;
+  struct {
+void *x = this;
+  };
+  void *y = this;
+};
+constexpr E e1 = E();
+static_assert(e1.x != e1.y, "");
+constexpr E e2 = E{0};
+static_assert(e2.x != e2.y, "");
+
+} // namespace IndirectFields
Index: cfe/trunk/lib/AST/ExprConstant.cpp
===
--- cfe/trunk/lib/AST/ExprConstant.cpp
+++ cfe/trunk/lib/AST/ExprConstant.cpp
@@ -4383,6 +4383,7 @@
 #endif
   for (const auto *I : Definition->inits()) {
 LValue Subobject = This;
+LValue SubobjectParent = This;
 APValue *Value = &Result;
 
 // Determine the subobject to initialize.
@@ -4413,7 +4414,8 @@
 } else if (IndirectFieldDecl *IFD = I->getIndirectMember()) {
   // Walk the indirect field decl's chain to find the object to initialize,
   // and make sure we've initialized every step along it.
-  for (auto *C : IFD->chain()) {
+  auto IndirectFieldChain = IFD->chain();
+  for (auto *C : IndirectFieldChain) {
 FD = cast(C);
 CXXRecordDecl *CD = cast(FD->getParent());
 // Switch the union field if it differs. This happens if we had
@@ -4429,6 +4431,10 @@
 *Value = APValue(APValue::UninitStruct(), CD->getNumBases(),
  std::distance(CD->field_begin(), CD->field_end()));
 }
+// Store Subobject as its parent before updating it for the last element
+// in the chain.
+if (C == IndirectFieldChain.back())
+  SubobjectParent = Subobject;
 if (!HandleLValueMember(Info, I->getInit(), Subobject, FD))
   return false;
 if (CD->isUnion())
@@ -4440,10 +4446,16 @@
   llvm_unreachable("unknown base initializer kind");
 }
 
+// Need to override This for implicit field initializers as in this case
+// This refers to innermost anonymous struct/union containing initializer,
+// not to currently constructed class.
+const Expr *Init = I->getInit();
+ThisOverrideRAII ThisOverride(*Info.CurrentCall, &SubobjectParent,
+  isa(Init));
 FullExpressionRAII InitScope(Info);
-if (!EvaluateInPlace(*Value, Info, Subobject, I->getInit()) ||
-(FD && FD->isBitField() && !truncateBitfieldValue(Info, I->getInit(),
-  *Value, FD))) {
+if (!EvaluateInPlace(*Value, Info, Subobject, Init) ||
+(FD && FD->isBitField() &&
+ !truncateBitfieldValue(Info, Init, *Value, FD))) {
   // If we're checking for a potential constant expression, evaluate all
   // initializers even if some of them fail.
   if (!Info.noteFailure())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42498: [ExprConstant] Fix crash when initialize an indirect field with another field.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Thanks for the review. Explicit `this` usage is a good test, added it.

Checked that it happened to work with my previous approach because initializer 
was `CXXDefaultInitExpr` with expression

  ImplicitCastExpr 0x7fe966808c40 'void *' 
  `-CXXThisExpr 0x7fe966808c28 'struct A::(anonymous at file.cc:3:5) *' this


Repository:
  rL LLVM

https://reviews.llvm.org/D42498



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

[PATCH] D43787: Fix which Darwin versions have ObjC runtime with full subscripting support.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added reviewers: vsk, kubamracek.
Herald added a subscriber: jkorous-apple.

Update min deployment target in some tests so that they don't try
to link against libarclite and don't fail when it's not available.

rdar://problem/29253617


https://reviews.llvm.org/D43787

Files:
  clang/include/clang/Basic/ObjCRuntime.h
  clang/test/Driver/arclite-link.c
  compiler-rt/test/lit.common.cfg
  compiler-rt/test/tsan/Darwin/norace-objcxx-run-time.mm


Index: compiler-rt/test/tsan/Darwin/norace-objcxx-run-time.mm
===
--- compiler-rt/test/tsan/Darwin/norace-objcxx-run-time.mm
+++ compiler-rt/test/tsan/Darwin/norace-objcxx-run-time.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_tsan %s -lc++ -fobjc-arc -lobjc -o %t -framework Foundation
+// RUN: %clang_tsan %s -lc++ -fobjc-arc -lobjc -o %t -framework Foundation 
%darwin_min_target_with_full_runtime_arc_support
 // RUN: %run %t 2>&1 | FileCheck %s
 
 // Check that we do not report races between:
Index: compiler-rt/test/lit.common.cfg
===
--- compiler-rt/test/lit.common.cfg
+++ compiler-rt/test/lit.common.cfg
@@ -197,8 +197,14 @@
 pass
 
   config.substitutions.append( ("%macos_min_target_10_11", 
"-mmacosx-version-min=10.11") )
+
+  isIOS = getattr(config, 'ios', False)
+  # rdar://problem/22207160
+  config.substitutions.append( 
("%darwin_min_target_with_full_runtime_arc_support",
+  "-miphoneos-version-min=9.0" if isIOS else "-mmacosx-version-min=10.11") 
)
 else:
   config.substitutions.append( ("%macos_min_target_10_11", "") )
+  config.substitutions.append( 
("%darwin_min_target_with_full_runtime_arc_support", "") )
 
 if config.android:
   adb = os.environ.get('ADB', 'adb')
Index: clang/test/Driver/arclite-link.c
===
--- clang/test/Driver/arclite-link.c
+++ clang/test/Driver/arclite-link.c
@@ -1,6 +1,6 @@
 // RUN: touch %t.o
-// RUN: %clang -### -target x86_64-apple-darwin10 -fobjc-link-runtime -lfoo 
-mmacosx-version-min=10.7 %t.o 2>&1 | FileCheck -check-prefix=CHECK-ARCLITE-OSX 
%s
-// RUN: %clang -### -target x86_64-apple-darwin10 -fobjc-link-runtime 
-mmacosx-version-min=10.8 %t.o 2>&1 | FileCheck -check-prefix=CHECK-NOARCLITE %s
+// RUN: %clang -### -target x86_64-apple-darwin10 -fobjc-link-runtime -lfoo 
-mmacosx-version-min=10.10 %t.o 2>&1 | FileCheck 
-check-prefix=CHECK-ARCLITE-OSX %s
+// RUN: %clang -### -target x86_64-apple-darwin10 -fobjc-link-runtime 
-mmacosx-version-min=10.11 %t.o 2>&1 | FileCheck -check-prefix=CHECK-NOARCLITE 
%s
 // RUN: %clang -### -target i386-apple-darwin10 -fobjc-link-runtime 
-mmacosx-version-min=10.7 %t.o 2>&1 | FileCheck -check-prefix=CHECK-NOARCLITE %s
 // RUN: %clang -### -target x86_64-apple-darwin10 -fobjc-link-runtime 
-nostdlib %t.o 2>&1 | FileCheck -check-prefix=CHECK-NOSTDLIB %s
 
@@ -12,6 +12,6 @@
 // CHECK-NOARCLITE-NOT: libarclite
 // CHECK-NOSTDLIB-NOT: -lobjc
 
-// RUN: %clang -### -target x86_64-apple-darwin10 -fobjc-link-runtime 
-fobjc-arc -mmacosx-version-min=10.7 %s 2>&1 | FileCheck 
-check-prefix=CHECK-UNUSED %s
+// RUN: %clang -### -target x86_64-apple-darwin10 -fobjc-link-runtime 
-fobjc-arc -mmacosx-version-min=10.10 %s 2>&1 | FileCheck 
-check-prefix=CHECK-UNUSED %s
 
 // CHECK-UNUSED-NOT: warning: argument unused during compilation: 
'-fobjc-link-runtime'
Index: clang/include/clang/Basic/ObjCRuntime.h
===
--- clang/include/clang/Basic/ObjCRuntime.h
+++ clang/include/clang/Basic/ObjCRuntime.h
@@ -207,8 +207,8 @@
   bool hasSubscripting() const {
 switch (getKind()) {
 case FragileMacOSX: return false;
-case MacOSX: return getVersion() >= VersionTuple(10, 8);
-case iOS: return getVersion() >= VersionTuple(6);
+case MacOSX: return getVersion() >= VersionTuple(10, 11);
+case iOS: return getVersion() >= VersionTuple(9);
 case WatchOS: return true;
 
 // This is really a lie, because some implementations and versions


Index: compiler-rt/test/tsan/Darwin/norace-objcxx-run-time.mm
===
--- compiler-rt/test/tsan/Darwin/norace-objcxx-run-time.mm
+++ compiler-rt/test/tsan/Darwin/norace-objcxx-run-time.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_tsan %s -lc++ -fobjc-arc -lobjc -o %t -framework Foundation
+// RUN: %clang_tsan %s -lc++ -fobjc-arc -lobjc -o %t -framework Foundation %darwin_min_target_with_full_runtime_arc_support
 // RUN: %run %t 2>&1 | FileCheck %s
 
 // Check that we do not report races between:
Index: compiler-rt/test/lit.common.cfg
===
--- compiler-rt/test/lit.common.cfg
+++ compiler-rt/test/lit.common.cfg
@@ -197,8 +197,14 @@
 pass
 
   config.substitutions.append( ("%macos_min_target_10_11", "-mmacosx-version-min=10.11") )
+
+  isIOS = getattr(config, '

[PATCH] D43787: Fix which Darwin versions have ObjC runtime with full subscripting support.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In https://reviews.llvm.org/D43787#1019886, @kubamracek wrote:

> Great! So the versions in `ObjCRuntime.h` didn't really match what the iOS 
> and macOS supported?


Yes, support for assigning nil for a key for NSMutableDictionary was added in 
macOS 10.11, iOS 9. Earlier versions require libarclite.


https://reviews.llvm.org/D43787



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