[PATCH] D66919: Warn about zero-parameter K definitions in -Wstrict-prototypes

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Note that I'm just copying GCC, which seems the be the original intent behind 
the warning (https://bugs.llvm.org/show_bug.cgi?id=20796). So people who use 
both compilers will have seen that warning already. Note also that there is no 
warning if any declaration provides a prototype, so this is fine:

  void f(void);  // provides a prototype
  void f() {}// not a prototype, but we have one already



In D66919#1650174 , @dexonsmith wrote:

> We could carve out a `-W` flag (if it doesn't already exist) that warns if 
> you incorrectly pass parameters to a function whose definition has no 
> prototype


The warning exists, but there is no flag apparently.

  void f() {}
  void g() {
  f(0);
  }

spits out

  test.c:3:8: warning: too many arguments in call to 'f'
  f(0);
  ~  ^

But with `f(void)` we get

  test.c:3:7: error: too many arguments to function call, expected 0, have 1
  f(0);
  ~ ^
  test.c:1:1: note: 'f' declared here
  void f(void) {}
  ^


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66919



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

[PATCH] D66831: [ObjC] Fix type checking for qualified id block parameters.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Yes, that makes sense, thanks.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66831



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

[PATCH] D55326: [Driver] Fix incorrect GNU triplet for PowerPC on SUSE Linux

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert updated this revision to Diff 217757.
aaronpuchert added a comment.
Herald added a subscriber: kbarton.

Added a test case, verified that it fails before the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D55326

Files:
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/test/Driver/Inputs/opensuse_tumbleweed_ppc_tree/usr/lib/crt1.o
  clang/test/Driver/Inputs/opensuse_tumbleweed_ppc_tree/usr/lib/crti.o
  clang/test/Driver/Inputs/opensuse_tumbleweed_ppc_tree/usr/lib/crtn.o
  
clang/test/Driver/Inputs/opensuse_tumbleweed_ppc_tree/usr/lib/gcc/powerpc64-suse-linux/9/crtbegin.o
  
clang/test/Driver/Inputs/opensuse_tumbleweed_ppc_tree/usr/lib/gcc/powerpc64-suse-linux/9/crtend.o
  clang/test/Driver/linux-ld.c


Index: clang/test/Driver/linux-ld.c
===
--- clang/test/Driver/linux-ld.c
+++ clang/test/Driver/linux-ld.c
@@ -879,6 +879,21 @@
 // CHECK-OPENSUSE-TW-RISCV64: 
"{{.*}}/usr/lib64/gcc/riscv64-suse-linux/9{{/|}}crtend.o"
 // CHECK-OPENSUSE-TW-RISCV64: 
"{{.*}}/usr/lib64/gcc/riscv64-suse-linux/9/../../../../lib64{{/|}}crtn.o"
 //
+// Check openSUSE Tumbleweed on ppc
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=powerpc64-suse-linux -rtlib=platform -m32 \
+// RUN: --gcc-toolchain="" \
+// RUN: --sysroot=%S/Inputs/opensuse_tumbleweed_ppc_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-OPENSUSE-TW-PPC %s
+// CHECK-OPENSUSE-TW-PPC: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK-OPENSUSE-TW-PPC: 
"{{.*}}/usr/lib/gcc/powerpc64-suse-linux/9/../../..{{/|}}crt1.o"
+// CHECK-OPENSUSE-TW-PPC: 
"{{.*}}/usr/lib/gcc/powerpc64-suse-linux/9/../../..{{/|}}crti.o"
+// CHECK-OPENSUSE-TW-PPC: 
"{{.*}}/usr/lib/gcc/powerpc64-suse-linux/9{{/|}}crtbegin.o"
+// CHECK-OPENSUSE-TW-PPC: "-L[[SYSROOT]]/usr/lib/gcc/powerpc64-suse-linux/9"
+// CHECK-OPENSUSE-TW-PPC: 
"-L[[SYSROOT]]/usr/lib/gcc/powerpc64-suse-linux/9/../../.."
+// CHECK-OPENSUSE-TW-PPC: 
"{{.*}}/usr/lib/gcc/powerpc64-suse-linux/9{{/|}}crtend.o"
+// CHECK-OPENSUSE-TW-PPC: 
"{{.*}}/usr/lib/gcc/powerpc64-suse-linux/9/../../..{{/|}}crtn.o"
+//
 // Check dynamic-linker for different archs
 // RUN: %clang %s -### -o %t.o 2>&1 \
 // RUN: --target=arm-linux-gnueabi \
Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -2000,7 +2000,9 @@
   static const char *const PPCLibDirs[] = {"/lib32", "/lib"};
   static const char *const PPCTriples[] = {
   "powerpc-linux-gnu", "powerpc-unknown-linux-gnu", "powerpc-linux-gnuspe",
-  "powerpc-suse-linux", "powerpc-montavista-linuxspe"};
+  // On 32-bit PowerPC systems running SUSE Linux, gcc is configured as a
+  // 64-bit compiler which defaults to "-m32", hence 
"powerpc64-suse-linux".
+  "powerpc64-suse-linux", "powerpc-montavista-linuxspe"};
   static const char *const PPC64LibDirs[] = {"/lib64", "/lib"};
   static const char *const PPC64Triples[] = {
   "powerpc64-linux-gnu", "powerpc64-unknown-linux-gnu",


Index: clang/test/Driver/linux-ld.c
===
--- clang/test/Driver/linux-ld.c
+++ clang/test/Driver/linux-ld.c
@@ -879,6 +879,21 @@
 // CHECK-OPENSUSE-TW-RISCV64: "{{.*}}/usr/lib64/gcc/riscv64-suse-linux/9{{/|}}crtend.o"
 // CHECK-OPENSUSE-TW-RISCV64: "{{.*}}/usr/lib64/gcc/riscv64-suse-linux/9/../../../../lib64{{/|}}crtn.o"
 //
+// Check openSUSE Tumbleweed on ppc
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=powerpc64-suse-linux -rtlib=platform -m32 \
+// RUN: --gcc-toolchain="" \
+// RUN: --sysroot=%S/Inputs/opensuse_tumbleweed_ppc_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-OPENSUSE-TW-PPC %s
+// CHECK-OPENSUSE-TW-PPC: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK-OPENSUSE-TW-PPC: "{{.*}}/usr/lib/gcc/powerpc64-suse-linux/9/../../..{{/|}}crt1.o"
+// CHECK-OPENSUSE-TW-PPC: "{{.*}}/usr/lib/gcc/powerpc64-suse-linux/9/../../..{{/|}}crti.o"
+// CHECK-OPENSUSE-TW-PPC: "{{.*}}/usr/lib/gcc/powerpc64-suse-linux/9{{/|}}crtbegin.o"
+// CHECK-OPENSUSE-TW-PPC: "-L[[SYSROOT]]/usr/lib/gcc/powerpc64-suse-linux/9"
+// CHECK-OPENSUSE-TW-PPC: "-L[[SYSROOT]]/usr/lib/gcc/powerpc64-suse-linux/9/../../.."
+// CHECK-OPENSUSE-TW-PPC: "{{.*}}/usr/lib/gcc/powerpc64-suse-linux/9{{/|}}crtend.o"
+// CHECK-OPENSUSE-TW-PPC: "{{.*}}/usr/lib/gcc/powerpc64-suse-linux/9/../../..{{/|}}crtn.o"
+//
 // Check dynamic-linker for different archs
 // RUN: %clang %s -### -o %t.o 2>&1 \
 // RUN: --target=arm-linux-gnueabi \
Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ 

[PATCH] D66919: Warn about zero-parameter K definitions in -Wstrict-prototypes

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a reviewer: erik.pilkington.
dexonsmith added a comment.

This could cause a lot of churn in existing projects (especially with `static 
void foo()`), without giving Clang any new information.  I'm wary of this.

> Zero-parameter K definitions specify that the function has no
>  parameters, but they are still not prototypes, so calling the function
>  with the wrong number of parameters is just a warning, not an error.

Why not just directly give an error for the problematic case?  We could carve 
out a `-W` flag (if it doesn't already exist) that warns if you incorrectly 
pass parameters to a function whose definition has no prototype, and then make 
it `-Werror`-by-default.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66919



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

[PATCH] D66324: clang-misexpect: Profile Guided Validation of Performance Annotations in LLVM

[Nick Desaulniers via Phabricator via cfe-commits]

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

Ok, thanks for the iteration on code review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



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

[PATCH] D66710: ASTReader: Bypass overridden files when reading PCHs

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/Basic/FileManager.h:320
+  /// twice, you get two new file entries.
+  const FileEntry *getBypassFile(const FileEntry );
+

bruno wrote:
> Does it make sense to return/read from a FileEntryRef here? @arphaman, wdyt?
SGTM.


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

https://reviews.llvm.org/D66710



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

[PATCH] D66710: ASTReader: Bypass overridden files when reading PCHs

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith updated this revision to Diff 217754.
dexonsmith added a comment.

Added a FileManagerTest and changed FileManager::getBypassFile to use 
FileEntryRef.


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

https://reviews.llvm.org/D66710

Files:
  clang/include/clang/Basic/FileManager.h
  clang/include/clang/Basic/SourceManager.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/unittests/Basic/FileManagerTest.cpp

Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -397,4 +397,54 @@
   EXPECT_EQ((*file)->tryGetRealPathName(), ExpectedResult);
 }
 
+TEST_F(FileManagerTest, getBypassFile) {
+  SmallString<64> CustomWorkingDir;
+#ifdef _WIN32
+  CustomWorkingDir = "C:/";
+#else
+  CustomWorkingDir = "/";
+#endif
+
+  auto FS = IntrusiveRefCntPtr(
+  new llvm::vfs::InMemoryFileSystem);
+  // setCurrentworkingdirectory must finish without error.
+  ASSERT_TRUE(!FS->setCurrentWorkingDirectory(CustomWorkingDir));
+
+  FileSystemOptions Opts;
+  FileManager Manager(Opts, FS);
+
+  // Inject fake files into the file system.
+  auto Cache = std::make_unique();
+  Cache->InjectDirectory("/tmp", 42);
+  Cache->InjectFile("/tmp/test", 43);
+  Manager.setStatCache(std::move(Cache));
+
+  // Set up a virtual file with a different size than FakeStatCache uses.
+  const FileEntry *File = Manager.getVirtualFile("/tmp/test", /*Size=*/10, 0);
+  ASSERT_TRUE(File);
+  FileEntryRef Ref("/tmp/test", *File);
+  EXPECT_TRUE(Ref.isValid());
+  EXPECT_EQ(Ref.getSize(), 10);
+
+  // Calling a second time should not affect the UID or size.
+  unsigned VirtualUID = Ref.getUID();
+  EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
+  EXPECT_EQ(Ref.getUID(), VirtualUID);
+  EXPECT_EQ(Ref.getSize(), 10);
+
+  // Bypass the file.
+  llvm::Optional BypassRef = Manager.getBypassFile(Ref);
+  ASSERT_TRUE(BypassRef);
+  EXPECT_TRUE(BypassRef->isValid());
+  EXPECT_EQ(BypassRef->getName(), Ref.getName());
+
+  // Check that it's different in the right ways.
+  EXPECT_NE(>getFileEntry(), File);
+  EXPECT_NE(BypassRef->getUID(), VirtualUID);
+  EXPECT_NE(BypassRef->getSize(), Ref.getSize());
+
+  // The virtual file should still be returned when searching.
+  EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
+}
+
 } // anonymous namespace
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -2315,19 +2315,14 @@
   if ((!Overridden && !Transient) && SM.isFileOverridden(File)) {
 if (Complain)
   Error(diag::err_fe_pch_file_overridden, Filename);
-// After emitting the diagnostic, recover by disabling the override so
-// that the original file will be used.
-//
-// FIXME: This recovery is just as broken as the original state; there may
-// be another precompiled module that's using the overridden contents, or
-// we might be half way through parsing it. Instead, we should treat the
-// overridden contents as belonging to a separate FileEntry.
-SM.disableFileContentsOverride(File);
-// The FileEntry is a virtual file entry with the size of the contents
-// that would override the original contents. Set it to the original's
-// size/time.
-FileMgr.modifyFileEntry(const_cast(File),
-StoredSize, StoredTime);
+
+// After emitting the diagnostic, bypass the overriding file to recover
+// (this creates a separate FileEntry).
+File = SM.bypassFileContentsOverride(*File);
+if (!File) {
+  F.InputFilesLoaded[ID - 1] = InputFile::getNotFound();
+  return InputFile();
+}
   }
 
   bool IsOutOfDate = false;
Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -669,17 +669,19 @@
   getOverriddenFilesInfo().OverriddenFiles[SourceFile] = NewFile;
 }
 
-void SourceManager::disableFileContentsOverride(const FileEntry *File) {
-  if (!isFileOverridden(File))
-return;
-
-  const SrcMgr::ContentCache *IR = getOrCreateContentCache(File);
-  const_cast(IR)->replaceBuffer(nullptr);
-  const_cast(IR)->ContentsEntry = IR->OrigEntry;
+const FileEntry *
+SourceManager::bypassFileContentsOverride(const FileEntry ) {
+  assert(isFileOverridden());
+  llvm::Optional BypassFile =
+  FileMgr.getBypassFile(FileEntryRef(File.getName(), File));
+
+  // If the file can't be found in the FS, give up.
+  if (!BypassFile)
+return nullptr;
 
-  assert(OverriddenFilesInfo);
-  OverriddenFilesInfo->OverriddenFiles.erase(File);
-  OverriddenFilesInfo->OverriddenFilesWithBuffer.erase(File);
+  const 

[PATCH] D66324: clang-misexpect: Profile Guided Validation of Performance Annotations in LLVM

[Paul Kirth via Phabricator via cfe-commits]

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



Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:157-158
+// In these cases we should not emit any diagnostic related to misexpect.
+if (NOps < 3)
+  return;
+

nickdesaulniers wrote:
> would an `assert` be more appropriate then?  If cases 1/2/3 above are all 
> highly unlikely, I'd make this an assert rather than early return.
I'm not sure an assert is appropriate. There are several types of profiling 
metadata, such as the value profiling metadata, and the types other than branch 
weight will have different numbers of arguments.  

I'll look into the tags that fall under `MD_prof`to see if any have less than 3 
operands, but right now I'm not sure. I do know that other places that read the 
MD_prof tags use early return rather than an assert though.

The two examples I based my conventions on were:
- InstrProf.cpp:987 where VP metadata is extracted after first checking the 
number of operands.

- Metadata.cpp:1345 where branch weights are extracted using a less defensive 
approach, and do not check the number of operands before checking the metadata 
tag.



Comment at: llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll:79
+
+attributes #0 = { nounwind "correctly-rounded-divide-sqrt-fp-math"="false" 
"disable-tail-calls"="false" "frame-pointer"="none" 
"less-precise-fpmad"="false" "min-legal-vector-width"="0" 
"no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" 
"no-signed-zeros-fp-math"="false" "no-trapping-math"="false" 
"stack-protector-buffer-size"="8" "target-features"="+cx8,+mmx,+sse,+sse2,+x87" 
"unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { argmemonly nounwind willreturn }

nickdesaulniers wrote:
> does the debug info need to be in all of these unit tests?  If you still have 
> the C sources handy, `-g0` is extremely handy in generating more concise IR 
> for unit tests.
For this one, I think you're right, the debug info isn't needed, since we are 
checking that a diagnostic is never emitted.

In the other cases, I wanted to verify that the diagnostic flags the correct 
line number. If it is OK for the clang based tests to handle that aspect, then 
I can try to refactor the LLVM tests to be less precise about the output they 
are checking, and remove the debug data from the IR


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



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

[PATCH] D66919: Warn about zero-parameter K definitions in -Wstrict-prototypes

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert created this revision.
aaronpuchert added reviewers: arphaman, bruno, rsmith.
Herald added subscribers: cfe-commits, dexonsmith.
Herald added a project: clang.

Zero-parameter K definitions specify that the function has no
parameters, but they are still not prototypes, so calling the function
with the wrong number of parameters is just a warning, not an error.

The C11 standard doesn't seem to directly define what a prototype is,
but it can be inferred from 6.9.1p7: "If the declarator includes a
parameter type list, the list also specifies the types of all the
parameters; such a declarator also serves as a function prototype
for later calls to the same function in the same translation unit."
This refers to 6.7.6.3p5: "If, in the declaration “T D1 
”, D1  has
the form

  D(parameter-type-list)

or

  D(identifier-list_opt)

[...]". Later in 6.11.7 it also refers only to the parameter-type-list
variant as prototype: "The use of function definitions with separate
parameter identifier and declaration lists (not prototype-format
parameter type and identifier declarators) is an obsolescent feature."

We already correctly treat an empty parameter list as non-prototype
declaration, so we can just take that information.

GCC also warns about this with -Wstrict-prototypes.

This shouldn't affect C++, because there all FunctionType's are
FunctionProtoTypes. I added a simple test for that.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66919

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/Sema/warn-strict-prototypes.c
  clang/test/Sema/warn-strict-prototypes.cpp
  clang/test/Sema/warn-strict-prototypes.m


Index: clang/test/Sema/warn-strict-prototypes.m
===
--- clang/test/Sema/warn-strict-prototypes.m
+++ clang/test/Sema/warn-strict-prototypes.m
@@ -10,7 +10,7 @@
 
 @end
 
-void foo() {
+void foo() { // expected-warning {{this old-style function definition is not 
preceded by a prototype}}
   void (^block)() = // expected-warning {{this block declaration is not a 
prototype}}
 ^void(int arg) { // no warning
   };
Index: clang/test/Sema/warn-strict-prototypes.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-strict-prototypes.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -Wstrict-prototypes %s
+// expected-no-diagnostics
+
+void decl();
+void decl_void(void);
+
+void def() {}
+void def_void(void) {}
Index: clang/test/Sema/warn-strict-prototypes.c
===
--- clang/test/Sema/warn-strict-prototypes.c
+++ clang/test/Sema/warn-strict-prototypes.c
@@ -7,9 +7,9 @@
 // function declaration with 0 params
 void foo2(void);
 
-// function definition with 0 params(for both cases),
-// valid according to 6.7.5.3/14
-void foo1() {}
+// function definition with 0 params, no prototype.
+void foo1() {} // expected-warning {{this old-style function definition is not 
preceded by a prototype}}
+// function definition with 0 params, prototype.
 void foo2(void) {}
 
 // function type typedef unspecified params
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -13395,11 +13395,7 @@
   //   Warn if K function is defined without a previous declaration.
   //   This warning is issued only if the definition itself does not 
provide
   //   a prototype. Only K definitions do not provide a prototype.
-  //   An empty list in a function declarator that is part of a definition
-  //   of that function specifies that the function has no parameters
-  //   (C99 6.7.5.3p14)
-  if (!FD->hasWrittenPrototype() && FD->getNumParams() > 0 &&
-  !LangOpts.CPlusPlus) {
+  if (!FD->hasWrittenPrototype()) {
 TypeSourceInfo *TI = FD->getTypeSourceInfo();
 TypeLoc TL = TI->getTypeLoc();
 FunctionTypeLoc FTL = TL.getAsAdjusted();


Index: clang/test/Sema/warn-strict-prototypes.m
===
--- clang/test/Sema/warn-strict-prototypes.m
+++ clang/test/Sema/warn-strict-prototypes.m
@@ -10,7 +10,7 @@
 
 @end
 
-void foo() {
+void foo() { // expected-warning {{this old-style function definition is not preceded by a prototype}}
   void (^block)() = // expected-warning {{this block declaration is not a prototype}}
 ^void(int arg) { // no warning
   };
Index: clang/test/Sema/warn-strict-prototypes.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-strict-prototypes.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -verify -fsyntax-only -Wstrict-prototypes %s
+// expected-no-diagnostics
+
+void decl();
+void decl_void(void);
+
+void def() {}
+void 

[PATCH] D52524: Add -Wpoison-system-directories warning

[Denis Nikitin via Phabricator via cfe-commits]

denik added a comment.

Hi Aaron,

Thank you for your review. I updated the diff with suggested changes.




Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1072
+// cross-compiling.
+def PoisonSystemDirectories : DiagGroup<"poison-system-directories">;
+

aaron.ballman wrote:
> Do you envision more warnings being added to this group? If not, I would 
> recommend dropping this change and instead using 
> `InGroup>` in the diagnostic.
Added the change. Thanks.



Comment at: clang/lib/Frontend/InitHeaderSearch.cpp:145
+  Headers.getDiags().Report(diag::warn_poison_system_directories)
+  << MappedPathStr.str();
+}

aaron.ballman wrote:
> I don't think you need the `.str()` here.
Thank you for pointing out. Fixed.


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

https://reviews.llvm.org/D52524



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

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

[David Majnemer via Phabricator via cfe-commits]

majnemer added inline comments.



Comment at: test/libcxx/numerics/truncating_cast.pass.cpp:12
+// Test the conversion function that truncates floating point types to the
+// closes representable value for the specified integer type, or
+// numeric_limits::max()/min() if the value isn't representable.

closes -> closest


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

https://reviews.llvm.org/D66836



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

[PATCH] D52524: Add -Wpoison-system-directories warning

[Denis Nikitin via Phabricator via cfe-commits]

denik updated this revision to Diff 217746.
denik marked 4 inline comments as done.

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

https://reviews.llvm.org/D52524

Files:
  clang/include/clang/Basic/DiagnosticCommonKinds.td
  clang/lib/Frontend/InitHeaderSearch.cpp
  clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/lib/.keep
  
clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/include/c++/.keep
  clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/lib/gcc/.keep
  
clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/local/include/.keep
  clang/test/Frontend/Inputs/sysroot_x86_64_cross_linux_tree/usr/local/lib/.keep
  clang/test/Frontend/warning-poison-system-directories.c


Index: clang/test/Frontend/warning-poison-system-directories.c
===
--- /dev/null
+++ clang/test/Frontend/warning-poison-system-directories.c
@@ -0,0 +1,27 @@
+// System directory and sysroot option causes warning.
+// RUN: %clang -Wpoison-system-directories -target x86_64 -I/usr/include 
--sysroot %S/Inputs/sysroot_x86_64_cross_linux_tree -c -o - %s 2> %t.1.stderr
+// RUN: FileCheck -check-prefix=WARN < %t.1.stderr %s
+// RUN: %clang -Wpoison-system-directories -target x86_64 
-cxx-isystem/usr/include --sysroot %S/Inputs/sysroot_x86_64_cross_linux_tree -c 
-o - %s 2> %t.1.stderr
+// RUN: FileCheck -check-prefix=WARN < %t.1.stderr %s
+// RUN: %clang -Wpoison-system-directories -target x86_64 
-iquote/usr/local/include --sysroot %S/Inputs/sysroot_x86_64_cross_linux_tree 
-c -o - %s 2> %t.1.stderr
+// RUN: FileCheck -check-prefix=WARN < %t.1.stderr %s
+// RUN: %clang -Wpoison-system-directories -target x86_64 
-isystem/usr/local/include --sysroot %S/Inputs/sysroot_x86_64_cross_linux_tree 
-c -o - %s 2> %t.1.stderr
+// RUN: FileCheck -check-prefix=WARN < %t.1.stderr %s
+
+// Missing target but included sysroot still causes the warning.
+// RUN: %clang -Wpoison-system-directories -I/usr/include --sysroot 
%S/Inputs/sysroot_x86_64_cross_linux_tree -c -o - %s 2> %t.2.stderr
+// RUN: FileCheck -check-prefix=WARN < %t.2.stderr %s
+
+// With -Werror the warning causes the failure.
+// RUN: not %clang -Werror=poison-system-directories -target x86_64 
-I/usr/include --sysroot %S/Inputs/sysroot_x86_64_cross_linux_tree -c -o - %s 
2> %t.3.stderr
+// RUN: FileCheck -check-prefix=ERROR < %t.3.stderr %s
+
+// Cros target without sysroot causes no warning.
+// RUN: %clang -Wpoison-system-directories -Werror -target x86_64 
-I/usr/include -c -o - %s
+
+// By default the warning is off.
+// RUN: %clang -Werror -target x86_64 -I/usr/include --sysroot 
%S/Inputs/sysroot_x86_64_cross_linux_tree -c -o - %s
+
+// WARN: warning: include location {{[^ ]+}} is unsafe for cross-compilation 
[-Wpoison-system-directories]
+
+// ERROR: error: include location {{[^ ]+}} is unsafe for cross-compilation 
[-Werror,-Wpoison-system-directories]
Index: clang/lib/Frontend/InitHeaderSearch.cpp
===
--- clang/lib/Frontend/InitHeaderSearch.cpp
+++ clang/lib/Frontend/InitHeaderSearch.cpp
@@ -137,6 +137,15 @@
   SmallString<256> MappedPathStorage;
   StringRef MappedPathStr = Path.toStringRef(MappedPathStorage);
 
+  // If use system headers while cross-compiling, emit the warning.
+  if (HasSysroot) {
+if (MappedPathStr.startswith("/usr/include") ||
+MappedPathStr.startswith("/usr/local/include")) {
+  Headers.getDiags().Report(diag::warn_poison_system_directories)
+  << MappedPathStr;
+}
+  }
+
   // Compute the DirectoryLookup type.
   SrcMgr::CharacteristicKind Type;
   if (Group == Quoted || Group == Angled || Group == IndexHeaderMap) {
Index: clang/include/clang/Basic/DiagnosticCommonKinds.td
===
--- clang/include/clang/Basic/DiagnosticCommonKinds.td
+++ clang/include/clang/Basic/DiagnosticCommonKinds.td
@@ -309,4 +309,9 @@
 "no analyzer checkers or packages are associated with '%0'">;
 def note_suggest_disabling_all_checkers : Note<
 "use -analyzer-disable-all-checks to disable all static analyzer 
checkers">;
+
+// Poison system directories.
+def warn_poison_system_directories : Warning <
+  "include location '%0' is unsafe for cross-compilation">,
+  InGroup>, DefaultIgnore;
 }


Index: clang/test/Frontend/warning-poison-system-directories.c
===
--- /dev/null
+++ clang/test/Frontend/warning-poison-system-directories.c
@@ -0,0 +1,27 @@
+// System directory and sysroot option causes warning.
+// RUN: %clang -Wpoison-system-directories -target x86_64 -I/usr/include --sysroot %S/Inputs/sysroot_x86_64_cross_linux_tree -c -o - %s 2> %t.1.stderr
+// RUN: FileCheck -check-prefix=WARN < %t.1.stderr %s
+// RUN: %clang -Wpoison-system-directories -target x86_64 -cxx-isystem/usr/include --sysroot 

[PATCH] D66873: [Test][Time profiler] Fix test for python3

[Nathan Chancellor via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66873



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

[PATCH] D66710: ASTReader: Bypass overridden files when reading PCHs

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Nice! Is this something that can be tested for in 
`unittests/Basic/FileManagerTest.cpp`?




Comment at: clang/include/clang/Basic/FileManager.h:320
+  /// twice, you get two new file entries.
+  const FileEntry *getBypassFile(const FileEntry );
+

Does it make sense to return/read from a FileEntryRef here? @arphaman, wdyt?


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

https://reviews.llvm.org/D66710



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

[PATCH] D66907: [Modules] Fix rebuilding an updated module for each of its consumers.

[Volodymyr Sapsai via Phabricator via cfe-commits]

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



Comment at: clang/lib/Serialization/ModuleManager.cpp:211
-if (!getModuleCache().tryToDropPCM(NewModule->FileName))
-  FileMgr.invalidateCache(NewModule->File);
 return OutOfDate;

dexonsmith wrote:
> This is the only caller of FileManager::invalidateCache.  I suggest deleting 
> it in a follow-up, in order to reduce complexity in the FileManager.
Good suggestion. Agree it will be better to keep the FileManager simpler.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66907



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

[PATCH] D66831: [ObjC] Fix type checking for qualified id block parameters.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In D66831#1650015 , @thakis wrote:

> We're getting a bunch of errors looking like 
> `../../AppsListViewController.m:11:37: error: incompatible block pointer 
> types assigning to 'void (^)(__strong id)' from 'void 
> (^)(AppCollectionViewCell *__strong)'` on code that looks fairly harmless to 
> me. It looks something like this:
>
>   @protocol AppCellProtocol  ... @end
>   @interface AppCollectionViewCell : NSObject  ...@enderby 
>  
>   @interface Cell : NSObject
>   @property(nonatomic, copy) void (^buttonPressed)(id cell);
>   @end
>  
>   @implementation Bar
>   - (void) f {
> __weak __typeof(self) weakSelf = self;
> cell.buttonPressed = ^(AppCollectionViewCell *pressedCell) {
>   // ...
> };
>   }
>   @end
>
>
> The code doesn't say `__strong` anywhere as far as I can tell; it looks like 
> regular protocol code.
>
> Is this expected?


That's the intention of the change. When you call `void 
(^buttonPressed)(id cell)`, it is only enforced that the 
argument conforms to `AppCellProtocol`. But you can do

  cell.buttonPressed = ^(AppCollectionViewCell *pressedCell) {
// call some AppCollectionViewCell method not present in AppCellProtocol
  };

and get unrecognized selector error. The change should help to avoid such 
situations.

Does my reasoning look correct to you?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66831



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

[PATCH] D66324: clang-misexpect: Profile Guided Validation of Performance Annotations in LLVM

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

Looks good, will likely approve after these 2 questions.




Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:157-158
+// In these cases we should not emit any diagnostic related to misexpect.
+if (NOps < 3)
+  return;
+

would an `assert` be more appropriate then?  If cases 1/2/3 above are all 
highly unlikely, I'd make this an assert rather than early return.



Comment at: llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll:79
+
+attributes #0 = { nounwind "correctly-rounded-divide-sqrt-fp-math"="false" 
"disable-tail-calls"="false" "frame-pointer"="none" 
"less-precise-fpmad"="false" "min-legal-vector-width"="0" 
"no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" 
"no-signed-zeros-fp-math"="false" "no-trapping-math"="false" 
"stack-protector-buffer-size"="8" "target-features"="+cx8,+mmx,+sse,+sse2,+x87" 
"unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { argmemonly nounwind willreturn }

does the debug info need to be in all of these unit tests?  If you still have 
the C sources handy, `-g0` is extremely handy in generating more concise IR for 
unit tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324



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

[PATCH] D66907: [Modules] Fix rebuilding an updated module for each of its consumers.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370274: [Modules] Fix rebuilding an updated module for each 
of its consumers. (authored by vsapsai, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66907?vs=217729=217740#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66907

Files:
  cfe/trunk/lib/Serialization/ModuleManager.cpp
  cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/A.h
  cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/B.h
  cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/Common.h
  cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/module.modulemap
  cfe/trunk/test/Modules/implicit-invalidate-common.c


Index: cfe/trunk/lib/Serialization/ModuleManager.cpp
===
--- cfe/trunk/lib/Serialization/ModuleManager.cpp
+++ cfe/trunk/lib/Serialization/ModuleManager.cpp
@@ -204,13 +204,8 @@
   // Read the signature eagerly now so that we can check it.  Avoid calling
   // ReadSignature unless there's something to check though.
   if (ExpectedSignature && checkSignature(ReadSignature(NewModule->Data),
-  ExpectedSignature, ErrorStr)) {
-// Try to remove the buffer.  If it can't be removed, then it was already
-// validated by this process.
-if (!getModuleCache().tryToDropPCM(NewModule->FileName))
-  FileMgr.invalidateCache(NewModule->File);
+  ExpectedSignature, ErrorStr))
 return OutOfDate;
-  }
 
   // We're keeping this module.  Store it everywhere.
   Module = Modules[Entry] = NewModule.get();
Index: cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/module.modulemap
===
--- cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/module.modulemap
+++ cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/module.modulemap
@@ -0,0 +1,3 @@
+module A { header "A.h" }
+module B { header "B.h" }
+module Common { header "Common.h" }
Index: cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/B.h
===
--- cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/B.h
+++ cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/B.h
@@ -0,0 +1,2 @@
+// B
+#include "Common.h"
Index: cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/A.h
===
--- cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/A.h
+++ cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/A.h
@@ -0,0 +1,2 @@
+// A
+#include "Common.h"
Index: cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/Common.h
===
--- cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/Common.h
+++ cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/Common.h
@@ -0,0 +1 @@
+// Common
Index: cfe/trunk/test/Modules/implicit-invalidate-common.c
===
--- cfe/trunk/test/Modules/implicit-invalidate-common.c
+++ cfe/trunk/test/Modules/implicit-invalidate-common.c
@@ -0,0 +1,36 @@
+// REQUIRES: shell
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/implicit-invalidate-common
+// RUN: cp -r %S/Inputs/implicit-invalidate-common %t/
+// RUN: echo '#include "A.h"' > %t/A.c
+// RUN: echo '#include "B.h"' > %t/B.c
+
+// Build with an empty module cache. Module 'Common' should be built only once.
+//
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps 
-fmodules-cache-path=%t/Cache \
+// RUN: -fsyntax-only -I %t/implicit-invalidate-common -Rmodule-build \
+// RUN: %t/A.c 2> %t/initial_build.txt
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps 
-fmodules-cache-path=%t/Cache \
+// RUN: -fsyntax-only -I %t/implicit-invalidate-common -Rmodule-build \
+// RUN: %t/B.c 2>> %t/initial_build.txt
+// RUN: FileCheck %s --implicit-check-not "remark:" --input-file 
%t/initial_build.txt
+
+// Update module 'Common' and build with the populated module cache. Module
+// 'Common' still should be built only once. Note that we are using the same
+// flags for A.c and B.c to avoid building Common.pcm at different paths.
+//
+// RUN: echo ' // ' >> %t/implicit-invalidate-common/Common.h
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps 
-fmodules-cache-path=%t/Cache \
+// RUN: -fsyntax-only -I %t/implicit-invalidate-common -Rmodule-build \
+// RUN: %t/A.c 2> %t/incremental_build.txt
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps 
-fmodules-cache-path=%t/Cache \
+// RUN: -fsyntax-only -I %t/implicit-invalidate-common -Rmodule-build \
+// RUN: %t/B.c 2>> %t/incremental_build.txt
+// RUN: FileCheck %s --implicit-check-not 

[PATCH] D66831: [ObjC] Fix type checking for qualified id block parameters.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

We're getting a bunch of errors looking like 
`../../AppsListViewController.m:11:37: error: incompatible block pointer types 
assigning to 'void (^)(__strong id)' from 'void 
(^)(AppCollectionViewCell *__strong)'` on code that looks fairly harmless to 
me. It looks something like this:

  @protocol AppCellProtocol  ... @end
  @interface AppCollectionViewCell : NSObject  ...@enderby 
  
  @interface Cell : NSObject
  @property(nonatomic, copy) void (^buttonPressed)(id cell);
  @end
  
  @implementation Bar
  - (void) f {
__weak __typeof(self) weakSelf = self;
cell.buttonPressed = ^(AppCollectionViewCell *pressedCell) {
  // ...
};
  }
  @end

The code doesn't say `__strong` anywhere as far as I can tell; it looks like 
regular protocol code.

Is this expected?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66831



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

r370274 - [Modules] Fix rebuilding an updated module for each of its consumers.

[Volodymyr Sapsai via cfe-commits]

Author: vsapsai
Date: Wed Aug 28 16:31:32 2019
New Revision: 370274

URL: http://llvm.org/viewvc/llvm-project?rev=370274=rev
Log:
[Modules] Fix rebuilding an updated module for each of its consumers.

Marking a module for a rebuild when its signature differs from the
expected one causes redundant module rebuilds for incremental builds.
When a module is updated, its signature changes. But its consumers still
have the old signature and loading them will result in signature
mismatches. It will correctly cause the rebuilds for the consumers but
we don't need to rebuild the common module for each of them as it is
already up to date.

In practice this bug causes longer build times. We are doing more work
than required and only a single process can build a module, so parallel
builds degrade to a single-process mode where extra processes are just
waiting on a file lock.

Fix by not marking a module dependency for a rebuild on signature
mismatch. We'll check if it is up to date when we load it.

rdar://problem/50212358

Reviewers: dexonsmith, bruno, rsmith

Reviewed By: dexonsmith, bruno

Subscribers: jkorous, ributzka, cfe-commits, aprantl

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

Added:
cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/
cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/A.h
cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/B.h
cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/Common.h
cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/module.modulemap
cfe/trunk/test/Modules/implicit-invalidate-common.c
Modified:
cfe/trunk/lib/Serialization/ModuleManager.cpp

Modified: cfe/trunk/lib/Serialization/ModuleManager.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ModuleManager.cpp?rev=370274=370273=370274=diff
==
--- cfe/trunk/lib/Serialization/ModuleManager.cpp (original)
+++ cfe/trunk/lib/Serialization/ModuleManager.cpp Wed Aug 28 16:31:32 2019
@@ -204,13 +204,8 @@ ModuleManager::addModule(StringRef FileN
   // Read the signature eagerly now so that we can check it.  Avoid calling
   // ReadSignature unless there's something to check though.
   if (ExpectedSignature && checkSignature(ReadSignature(NewModule->Data),
-  ExpectedSignature, ErrorStr)) {
-// Try to remove the buffer.  If it can't be removed, then it was already
-// validated by this process.
-if (!getModuleCache().tryToDropPCM(NewModule->FileName))
-  FileMgr.invalidateCache(NewModule->File);
+  ExpectedSignature, ErrorStr))
 return OutOfDate;
-  }
 
   // We're keeping this module.  Store it everywhere.
   Module = Modules[Entry] = NewModule.get();

Added: cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/A.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/A.h?rev=370274=auto
==
--- cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/A.h (added)
+++ cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/A.h Wed Aug 28 
16:31:32 2019
@@ -0,0 +1,2 @@
+// A
+#include "Common.h"

Added: cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/B.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/B.h?rev=370274=auto
==
--- cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/B.h (added)
+++ cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/B.h Wed Aug 28 
16:31:32 2019
@@ -0,0 +1,2 @@
+// B
+#include "Common.h"

Added: cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/Common.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/Common.h?rev=370274=auto
==
--- cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/Common.h (added)
+++ cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/Common.h Wed Aug 
28 16:31:32 2019
@@ -0,0 +1 @@
+// Common

Added: cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/module.modulemap
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/module.modulemap?rev=370274=auto
==
--- cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/module.modulemap 
(added)
+++ cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/module.modulemap 
Wed Aug 28 16:31:32 2019
@@ -0,0 +1,3 @@
+module A { header "A.h" }
+module B { header "B.h" }
+module Common { header "Common.h" }

Added: cfe/trunk/test/Modules/implicit-invalidate-common.c
URL: 

[PATCH] D66910: [clangd] Fix ExtractFunction dependencies

[Heejin Ahn via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370273: [clangd] Fix ExtractFunction dependencies (authored 
by aheejin, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66910?vs=217733=217736#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66910

Files:
  clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/trunk/clangd/tool/CMakeLists.txt
  clang-tools-extra/trunk/clangd/unittests/CMakeLists.txt


Index: clang-tools-extra/trunk/clangd/tool/CMakeLists.txt
===
--- clang-tools-extra/trunk/clangd/tool/CMakeLists.txt
+++ clang-tools-extra/trunk/clangd/tool/CMakeLists.txt
@@ -26,6 +26,7 @@
   clangSema
   clangTooling
   clangToolingCore
+  clangToolingRefactoring
   clangToolingSyntax
   ${CLANGD_XPC_LIBS}
   )
Index: clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt
===
--- clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt
+++ clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt
@@ -26,5 +26,6 @@
   clangBasic
   clangDaemon
   clangToolingCore
+  clangToolingRefactoring
   clangToolingSyntax
   )
Index: clang-tools-extra/trunk/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/trunk/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/trunk/clangd/unittests/CMakeLists.txt
@@ -90,6 +90,7 @@
   clangTooling
   clangToolingCore
   clangToolingInclusions
+  clangToolingRefactoring
   clangToolingSyntax
   LLVMSupport
   LLVMTestingSupport


Index: clang-tools-extra/trunk/clangd/tool/CMakeLists.txt
===
--- clang-tools-extra/trunk/clangd/tool/CMakeLists.txt
+++ clang-tools-extra/trunk/clangd/tool/CMakeLists.txt
@@ -26,6 +26,7 @@
   clangSema
   clangTooling
   clangToolingCore
+  clangToolingRefactoring
   clangToolingSyntax
   ${CLANGD_XPC_LIBS}
   )
Index: clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt
===
--- clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt
+++ clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt
@@ -26,5 +26,6 @@
   clangBasic
   clangDaemon
   clangToolingCore
+  clangToolingRefactoring
   clangToolingSyntax
   )
Index: clang-tools-extra/trunk/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/trunk/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/trunk/clangd/unittests/CMakeLists.txt
@@ -90,6 +90,7 @@
   clangTooling
   clangToolingCore
   clangToolingInclusions
+  clangToolingRefactoring
   clangToolingSyntax
   LLVMSupport
   LLVMTestingSupport
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r370273 - [clangd] Fix ExtractFunction dependencies

[Heejin Ahn via cfe-commits]

Author: aheejin
Date: Wed Aug 28 16:17:38 2019
New Revision: 370273

URL: http://llvm.org/viewvc/llvm-project?rev=370273=rev
Log:
[clangd] Fix ExtractFunction dependencies

Summary: Without these dependencies, builds with `-DBUILD_SHARED_LIBS=ON` fail.

Reviewers: SureYeaah

Subscribers: mgorny, ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, 
cfe-commits

Tags: #clang

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

Modified:
clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt
clang-tools-extra/trunk/clangd/tool/CMakeLists.txt
clang-tools-extra/trunk/clangd/unittests/CMakeLists.txt

Modified: clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt?rev=370273=370272=370273=diff
==
--- clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt Wed Aug 28 
16:17:38 2019
@@ -26,5 +26,6 @@ add_clang_library(clangDaemonTweaks OBJE
   clangBasic
   clangDaemon
   clangToolingCore
+  clangToolingRefactoring
   clangToolingSyntax
   )

Modified: clang-tools-extra/trunk/clangd/tool/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/tool/CMakeLists.txt?rev=370273=370272=370273=diff
==
--- clang-tools-extra/trunk/clangd/tool/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clangd/tool/CMakeLists.txt Wed Aug 28 16:17:38 2019
@@ -26,6 +26,7 @@ target_link_libraries(clangd
   clangSema
   clangTooling
   clangToolingCore
+  clangToolingRefactoring
   clangToolingSyntax
   ${CLANGD_XPC_LIBS}
   )

Modified: clang-tools-extra/trunk/clangd/unittests/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/CMakeLists.txt?rev=370273=370272=370273=diff
==
--- clang-tools-extra/trunk/clangd/unittests/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clangd/unittests/CMakeLists.txt Wed Aug 28 16:17:38 
2019
@@ -90,6 +90,7 @@ target_link_libraries(ClangdTests
   clangTooling
   clangToolingCore
   clangToolingInclusions
+  clangToolingRefactoring
   clangToolingSyntax
   LLVMSupport
   LLVMTestingSupport


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

[PATCH] D66907: [Modules] Fix rebuilding an updated module for each of its consumers.

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno accepted this revision.
bruno added a comment.

Thanks for fixing this, LGTM!


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

https://reviews.llvm.org/D66907



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

[PATCH] D66907: [Modules] Fix rebuilding an updated module for each of its consumers.

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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

This LGTM.

The key insight you're making is that when you're importing B into A and B's 
signature doesn't match what A expects, that does not mean that B is 
out-of-date.  B may have just been built and be totally correct.  It only means 
that A is out-of-date.  I had considered removing this call, but was confused 
about exactly what this would mean.




Comment at: clang/lib/Serialization/ModuleManager.cpp:211
-if (!getModuleCache().tryToDropPCM(NewModule->FileName))
-  FileMgr.invalidateCache(NewModule->File);
 return OutOfDate;

This is the only caller of FileManager::invalidateCache.  I suggest deleting it 
in a follow-up, in order to reduce complexity in the FileManager.


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

https://reviews.llvm.org/D66907



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

[PATCH] D66910: [clangd] Fix ExtractFunction dependencies

[Heejin Ahn via Phabricator via cfe-commits]

aheejin created this revision.
aheejin added a reviewer: SureYeaah.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ilya-biryukov, mgorny.
Herald added a project: clang.

Without these dependencies, builds with `-DBUILD_SHARED_LIBS=ON` fail.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66910

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/tool/CMakeLists.txt
  clang-tools-extra/clangd/unittests/CMakeLists.txt


Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -90,6 +90,7 @@
   clangTooling
   clangToolingCore
   clangToolingInclusions
+  clangToolingRefactoring
   clangToolingSyntax
   LLVMSupport
   LLVMTestingSupport
Index: clang-tools-extra/clangd/tool/CMakeLists.txt
===
--- clang-tools-extra/clangd/tool/CMakeLists.txt
+++ clang-tools-extra/clangd/tool/CMakeLists.txt
@@ -26,6 +26,7 @@
   clangSema
   clangTooling
   clangToolingCore
+  clangToolingRefactoring
   clangToolingSyntax
   ${CLANGD_XPC_LIBS}
   )
Index: clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
===
--- clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
+++ clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
@@ -26,5 +26,6 @@
   clangBasic
   clangDaemon
   clangToolingCore
+  clangToolingRefactoring
   clangToolingSyntax
   )


Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -90,6 +90,7 @@
   clangTooling
   clangToolingCore
   clangToolingInclusions
+  clangToolingRefactoring
   clangToolingSyntax
   LLVMSupport
   LLVMTestingSupport
Index: clang-tools-extra/clangd/tool/CMakeLists.txt
===
--- clang-tools-extra/clangd/tool/CMakeLists.txt
+++ clang-tools-extra/clangd/tool/CMakeLists.txt
@@ -26,6 +26,7 @@
   clangSema
   clangTooling
   clangToolingCore
+  clangToolingRefactoring
   clangToolingSyntax
   ${CLANGD_XPC_LIBS}
   )
Index: clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
===
--- clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
+++ clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
@@ -26,5 +26,6 @@
   clangBasic
   clangDaemon
   clangToolingCore
+  clangToolingRefactoring
   clangToolingSyntax
   )
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r370270 - Fix a passing XFAIL test

[Erik Pilkington via cfe-commits]

Author: epilk
Date: Wed Aug 28 15:38:36 2019
New Revision: 370270

URL: http://llvm.org/viewvc/llvm-project?rev=370270=rev
Log:
Fix a passing XFAIL test

Now that we can gracefully handle stack exhaustion, this test was passing in
darwin && asan. Instead, just unsupport it when threading is unavailable.

Modified:
cfe/trunk/test/CodeGenCXX/castexpr-basepathsize-threshold.cpp

Modified: cfe/trunk/test/CodeGenCXX/castexpr-basepathsize-threshold.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/castexpr-basepathsize-threshold.cpp?rev=370270=370269=370270=diff
==
--- cfe/trunk/test/CodeGenCXX/castexpr-basepathsize-threshold.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/castexpr-basepathsize-threshold.cpp Wed Aug 28 
15:38:36 2019
@@ -3,9 +3,9 @@
 // https://bugs.llvm.org/show_bug.cgi?id=38356
 // We only check that we do not crash.
 
-// ASAN increases stack usage, so we are hitting stack overflow before reaching
-// recursive template instantiation limit.
-// XFAIL: darwin && asan
+// This test can exceed stack usage in some configurations, so unless we can
+// properly handle that don't run it.
+// REQUIRES: thread_support
 
 template 
 struct d : d {};


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

[PATCH] D66907: [Modules] Fix rebuilding an updated module for each of its consumers.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Earlier `tryToDropPCM` was called 

 `tryToRemoveBuffer` and it was added in r298278 
.
 According to my investigation and understanding it was used to remove 
tentatively loaded dependencies. Otherwise we would finalize them 

 before validating if they are up to date. But now we are tracking a state of 
each module (Unknown, Tentative, ToBuild, Final) individually and don't need 
`tryToDropPCM`.


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

https://reviews.llvm.org/D66907



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

[PATCH] D66850: [AST][JSON] Avoid crash when dumping NULL Type as JSON

[Bert Belder via Phabricator via cfe-commits]

piscisaureus updated this revision to Diff 217730.
piscisaureus added a comment.

Add missing CHECK_NEXT line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66850

Files:
  clang/lib/AST/JSONNodeDumper.cpp
  clang/test/AST/ast-dump-types-json.cpp
  clang/test/AST/gen_ast_dump_json_test.py

Index: clang/test/AST/gen_ast_dump_json_test.py
===
--- clang/test/AST/gen_ast_dump_json_test.py
+++ clang/test/AST/gen_ast_dump_json_test.py
@@ -20,7 +20,7 @@
 normalize(e)
 elif type(v) is unicode:
 st = v.encode('utf-8')
-if re.match(r"0x[0-9A-Fa-f]+", v):
+if v != "0x0" and re.match(r"0x[0-9A-Fa-f]+", v):
 dict_var[k] = u'0x{{.*}}'
 elif os.path.isfile(v):
 dict_var[k] = u'{{.*}}'
Index: clang/test/AST/ast-dump-types-json.cpp
===
--- clang/test/AST/ast-dump-types-json.cpp
+++ clang/test/AST/ast-dump-types-json.cpp
@@ -20,6 +20,11 @@
 
 typedef int TestQualTypePrinting(const char* c);
 
+typedef int TestUsingShadowDeclType;
+namespace TestNamespaceWithUsingShadowType {
+using ::TestUsingShadowDeclType;
+}
+
 // NOTE: CHECK lines have been autogenerated by gen_ast_dump_json_test.py
 
 
@@ -422,3 +427,75 @@
 // CHECK-NEXT:   }
 // CHECK-NEXT:  ]
 // CHECK-NEXT: }
+
+// CHECK:  "kind": "NamespaceDecl",
+// CHECK-NEXT:  "loc": {
+// CHECK-NEXT:   "file": "{{.*}}",
+// CHECK-NEXT:   "line": 24,
+// CHECK-NEXT:   "col": 11,
+// CHECK-NEXT:   "tokLen": 32
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "range": {
+// CHECK-NEXT:   "begin": {
+// CHECK-NEXT:"col": 1,
+// CHECK-NEXT:"tokLen": 9
+// CHECK-NEXT:   },
+// CHECK-NEXT:   "end": {
+// CHECK-NEXT:"line": 26,
+// CHECK-NEXT:"col": 1,
+// CHECK-NEXT:"tokLen": 1
+// CHECK-NEXT:   }
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "name": "TestNamespaceWithUsingShadowType",
+// CHECK-NEXT:  "inner": [
+// CHECK-NEXT:   {
+// CHECK-NEXT:"id": "0x{{.*}}",
+// CHECK-NEXT:"kind": "UsingDecl",
+// CHECK-NEXT:"loc": {
+// CHECK-NEXT: "line": 25,
+// CHECK-NEXT: "col": 9,
+// CHECK-NEXT: "tokLen": 23
+// CHECK-NEXT:},
+// CHECK-NEXT:"range": {
+// CHECK-NEXT: "begin": {
+// CHECK-NEXT:  "col": 1,
+// CHECK-NEXT:  "tokLen": 5
+// CHECK-NEXT: },
+// CHECK-NEXT: "end": {
+// CHECK-NEXT:  "col": 9,
+// CHECK-NEXT:  "tokLen": 23
+// CHECK-NEXT: }
+// CHECK-NEXT:},
+// CHECK-NEXT:"name": "::TestUsingShadowDeclType"
+// CHECK-NEXT:   },
+// CHECK-NEXT:   {
+// CHECK-NEXT:"id": "0x{{.*}}",
+// CHECK-NEXT:"kind": "UsingShadowDecl",
+// CHECK-NEXT:"loc": {
+// CHECK-NEXT: "col": 9,
+// CHECK-NEXT: "tokLen": 23
+// CHECK-NEXT:},
+// CHECK-NEXT:"range": {
+// CHECK-NEXT: "begin": {
+// CHECK-NEXT:  "col": 9,
+// CHECK-NEXT:  "tokLen": 23
+// CHECK-NEXT: },
+// CHECK-NEXT: "end": {
+// CHECK-NEXT:  "col": 9,
+// CHECK-NEXT:  "tokLen": 23
+// CHECK-NEXT: }
+// CHECK-NEXT:},
+// CHECK-NEXT:"isImplicit": true,
+// CHECK-NEXT:"target": {
+// CHECK-NEXT: "id": "0x{{.*}}",
+// CHECK-NEXT: "kind": "TypedefDecl",
+// CHECK-NEXT: "name": "TestUsingShadowDeclType"
+// CHECK-NEXT:},
+// CHECK-NEXT:"inner": [
+// CHECK-NEXT: {
+// CHECK-NEXT:  "id": "0x0"
+// CHECK-NEXT: }
+// CHECK-NEXT:]
+// CHECK-NEXT:   }
+// CHECK-NEXT:  ]
+// CHECK-NEXT: }
Index: clang/lib/AST/JSONNodeDumper.cpp
===
--- clang/lib/AST/JSONNodeDumper.cpp
+++ clang/lib/AST/JSONNodeDumper.cpp
@@ -66,6 +66,10 @@
 
 void JSONNodeDumper::Visit(const Type *T) {
   JOS.attribute("id", createPointerRepresentation(T));
+
+  if (!T)
+return;
+
   JOS.attribute("kind", (llvm::Twine(T->getTypeClassName()) + "Type").str());
   JOS.attribute("type", createQualType(QualType(T, 0), /*Desugar*/ false));
   attributeOnlyIfTrue("isDependent", T->isDependentType());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66907: [Modules] Fix rebuilding an updated module for each of its consumers.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added reviewers: dexonsmith, bruno, rsmith.
Herald added subscribers: ributzka, jkorous.

Marking a module for a rebuild when its signature differs from the
expected one causes redundant module rebuilds for incremental builds.
When a module is updated, its signature changes. But its consumers still
have the old signature and loading them will result in signature
mismatches. It will correctly cause the rebuilds for the consumers but
we don't need to rebuild the common module for each of them as it is
already up to date.

In practice this bug causes longer build times. We are doing more work
than required and only a single process can build a module, so parallel
builds degrade to a single-process mode where extra processes are just
waiting on a file lock.

Fix by not marking a module dependency for a rebuild on signature
mismatch. We'll check if it is up to date when we load it.

rdar://problem/50212358


https://reviews.llvm.org/D66907

Files:
  clang/lib/Serialization/ModuleManager.cpp
  clang/test/Modules/Inputs/implicit-invalidate-common/A.h
  clang/test/Modules/Inputs/implicit-invalidate-common/B.h
  clang/test/Modules/Inputs/implicit-invalidate-common/Common.h
  clang/test/Modules/Inputs/implicit-invalidate-common/module.modulemap
  clang/test/Modules/implicit-invalidate-common.c


Index: clang/test/Modules/implicit-invalidate-common.c
===
--- /dev/null
+++ clang/test/Modules/implicit-invalidate-common.c
@@ -0,0 +1,36 @@
+// REQUIRES: shell
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/implicit-invalidate-common
+// RUN: cp -r %S/Inputs/implicit-invalidate-common %t/
+// RUN: echo '#include "A.h"' > %t/A.c
+// RUN: echo '#include "B.h"' > %t/B.c
+
+// Build with an empty module cache. Module 'Common' should be built only once.
+//
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps 
-fmodules-cache-path=%t/Cache \
+// RUN: -fsyntax-only -I %t/implicit-invalidate-common -Rmodule-build \
+// RUN: %t/A.c 2> %t/initial_build.txt
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps 
-fmodules-cache-path=%t/Cache \
+// RUN: -fsyntax-only -I %t/implicit-invalidate-common -Rmodule-build \
+// RUN: %t/B.c 2>> %t/initial_build.txt
+// RUN: FileCheck %s --implicit-check-not "remark:" --input-file 
%t/initial_build.txt
+
+// Update module 'Common' and build with the populated module cache. Module
+// 'Common' still should be built only once. Note that we are using the same
+// flags for A.c and B.c to avoid building Common.pcm at different paths.
+//
+// RUN: echo ' // ' >> %t/implicit-invalidate-common/Common.h
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps 
-fmodules-cache-path=%t/Cache \
+// RUN: -fsyntax-only -I %t/implicit-invalidate-common -Rmodule-build \
+// RUN: %t/A.c 2> %t/incremental_build.txt
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps 
-fmodules-cache-path=%t/Cache \
+// RUN: -fsyntax-only -I %t/implicit-invalidate-common -Rmodule-build \
+// RUN: %t/B.c 2>> %t/incremental_build.txt
+// RUN: FileCheck %s --implicit-check-not "remark:" --input-file 
%t/incremental_build.txt
+
+// CHECK: remark: building module 'A'
+// CHECK: remark: building module 'Common'
+// CHECK: remark: finished building module 'Common'
+// CHECK: remark: finished building module 'A'
+// CHECK: remark: building module 'B'
+// CHECK: remark: finished building module 'B'
Index: clang/test/Modules/Inputs/implicit-invalidate-common/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/implicit-invalidate-common/module.modulemap
@@ -0,0 +1,3 @@
+module A { header "A.h" }
+module B { header "B.h" }
+module Common { header "Common.h" }
Index: clang/test/Modules/Inputs/implicit-invalidate-common/Common.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/implicit-invalidate-common/Common.h
@@ -0,0 +1 @@
+// Common
Index: clang/test/Modules/Inputs/implicit-invalidate-common/B.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/implicit-invalidate-common/B.h
@@ -0,0 +1,2 @@
+// B
+#include "Common.h"
Index: clang/test/Modules/Inputs/implicit-invalidate-common/A.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/implicit-invalidate-common/A.h
@@ -0,0 +1,2 @@
+// A
+#include "Common.h"
Index: clang/lib/Serialization/ModuleManager.cpp
===
--- clang/lib/Serialization/ModuleManager.cpp
+++ clang/lib/Serialization/ModuleManager.cpp
@@ -204,13 +204,8 @@
   // Read the signature eagerly now so that we can check it.  Avoid calling
   // ReadSignature unless there's something to check though.
   if (ExpectedSignature && checkSignature(ReadSignature(NewModule->Data),
-

[PATCH] D66850: [AST][JSON] Avoid crash when dumping NULL Type as JSON

[Bert Belder via Phabricator via cfe-commits]

piscisaureus added a comment.

In D66850#1649865 , @aaron.ballman 
wrote:

> In D66850#1649708 , @piscisaureus 
> wrote:
>
> > In D66850#1648776 , @aaron.ballman 
> > wrote:
> >
> > > LGTM, but missing a test case.
> > >
> > > In D66850#1648557 , @sidorovd 
> > > wrote:
> > >
> > > > LGTM. I'm not an expert in JSON, but may be it makes sense to move the 
> > > > change a line earlier before creation of pointer representation?
> > >
> > >
> > > I would prefer it remains where it is -- having the `0x0` in the output 
> > > for a null pointer is a good thing because it conveys more information 
> > > than a totally empty `Type` object. We're accidentally inconsistent about 
> > > this currently (Decl prints 0x0 but Stmt gives an empty object).
> >
> >
> > I don't disagree, but I would argue that "0x0" is a rather poor choice, 
> > since now the consumer has to treat the "id" field as an opaque value, 
> > **except** when it's "0x0". A better choice would be to use JavaScript 
> > `null` to represent null pointers.
>
>
> Normally, I'd say yes, but in this case, we have to represent the pointer as 
> a string because the number type in JSON is a signed value. Rather than 
> "you'll either get a string or null", it seemed a bit more friendly to say 
> "you'll always get a string" and allow the consumer to decide how to handle 
> degenerate values. That said, I don't feel super strongly about this.


I agree using numbers isn't feasible. Not because javascript numbers are 
signed, but because they're 64-bit floating point values, which means they 
can't faithfully store 64-bit int values. The latest versions of ECMAScript 
support arbitrary-precision integers a.k.a. BigInt (written as 
`0xABCABCABCABCABCABCn`) which **can** represent 64-bit values, but support 
isn't currently widespread.

The reason I suggest to use `null` (which is not a number nor a string, but a 
type of its own) is that users need to treat the `id` as an opaque value 
(essentially they're only usable to compare nodes and use it as a lookup key in 
a map), with the exception of `"0x0"` which has a specific meaning. As a 
frequent JavaScript/TypeScript user, I would say that using different types is 
actually more ergonomic than using a string and having to know about certain 
strings that indicate a degenerate case.

> 
> 
> In D66850#1649804 , @piscisaureus 
> wrote:
> 
>> Add test
> 
> 
> Thank you!
> 
> LGTM, do you need someone to commit on your behalf?

Yes, please; I have no idea how to commit things to the LLVM tree.
BTW: I haven't ran the full test suite, I suppose some CI system will do that 
before this gets landed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66850



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

[PATCH] D66873: [Test][Time profiler] Fix test for python3

[Anton Afanasyev via Phabricator via cfe-commits]

anton-afanasyev added a comment.

Could you please give LGTM?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66873



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

r370263 - [analyzer] Fix more analyzer warnings on analyzer and libAnalysis.

[Artem Dergachev via cfe-commits]

Author: dergachev
Date: Wed Aug 28 14:19:58 2019
New Revision: 370263

URL: http://llvm.org/viewvc/llvm-project?rev=370263=rev
Log:
[analyzer] Fix more analyzer warnings on analyzer and libAnalysis.

Modified:
cfe/trunk/lib/Analysis/BodyFarm.cpp
cfe/trunk/lib/Analysis/CFG.cpp
cfe/trunk/lib/Analysis/CocoaConventions.cpp
cfe/trunk/lib/Analysis/RetainSummaryManager.cpp

Modified: cfe/trunk/lib/Analysis/BodyFarm.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/BodyFarm.cpp?rev=370263=370262=370263=diff
==
--- cfe/trunk/lib/Analysis/BodyFarm.cpp (original)
+++ cfe/trunk/lib/Analysis/BodyFarm.cpp Wed Aug 28 14:19:58 2019
@@ -408,8 +408,8 @@ static Stmt *create_call_once(ASTContext
   // reference.
   for (unsigned int ParamIdx = 2; ParamIdx < D->getNumParams(); ParamIdx++) {
 const ParmVarDecl *PDecl = D->getParamDecl(ParamIdx);
-if (PDecl &&
-CallbackFunctionType->getParamType(ParamIdx - 2)
+assert(PDecl);
+if (CallbackFunctionType->getParamType(ParamIdx - 2)
 .getNonReferenceType()
 .getCanonicalType() !=
 PDecl->getType().getNonReferenceType().getCanonicalType()) {

Modified: cfe/trunk/lib/Analysis/CFG.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CFG.cpp?rev=370263=370262=370263=diff
==
--- cfe/trunk/lib/Analysis/CFG.cpp (original)
+++ cfe/trunk/lib/Analysis/CFG.cpp Wed Aug 28 14:19:58 2019
@@ -2480,10 +2480,8 @@ CFGBlock *CFGBuilder::VisitBreakStmt(Bre
 
 static bool CanThrow(Expr *E, ASTContext ) {
   QualType Ty = E->getType();
-  if (Ty->isFunctionPointerType())
-Ty = Ty->getAs()->getPointeeType();
-  else if (Ty->isBlockPointerType())
-Ty = Ty->getAs()->getPointeeType();
+  if (Ty->isFunctionPointerType() || Ty->isBlockPointerType())
+Ty = Ty->getPointeeType();
 
   const FunctionType *FT = Ty->getAs();
   if (FT) {
@@ -4906,9 +4904,13 @@ CFGImplicitDtor::getDestructorDecl(ASTCo
   while (const ArrayType *arrayType = astContext.getAsArrayType(ty)) {
 ty = arrayType->getElementType();
   }
-  const RecordType *recordType = ty->getAs();
-  const CXXRecordDecl *classDecl =
-  cast(recordType->getDecl());
+
+  // The situation when the type of the lifetime-extending reference
+  // does not correspond to the type of the object is supposed
+  // to be handled by now. In particular, 'ty' is now the unwrapped
+  // record type.
+  const CXXRecordDecl *classDecl = ty->getAsCXXRecordDecl();
+  assert(classDecl);
   return classDecl->getDestructor();
 }
 case CFGElement::DeleteDtor: {
@@ -4933,12 +4935,6 @@ CFGImplicitDtor::getDestructorDecl(ASTCo
   llvm_unreachable("getKind() returned bogus value");
 }
 
-bool CFGImplicitDtor::isNoReturn(ASTContext ) const {
-  if (const CXXDestructorDecl *DD = getDestructorDecl(astContext))
-return DD->isNoReturn();
-  return false;
-}
-
 
//===--===//
 // CFGBlock operations.
 
//===--===//

Modified: cfe/trunk/lib/Analysis/CocoaConventions.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CocoaConventions.cpp?rev=370263=370262=370263=diff
==
--- cfe/trunk/lib/Analysis/CocoaConventions.cpp (original)
+++ cfe/trunk/lib/Analysis/CocoaConventions.cpp Wed Aug 28 14:19:58 2019
@@ -38,8 +38,8 @@ bool cocoa::isRefType(QualType RetTy, St
 return false;
 
   // Is the type void*?
-  const PointerType* PT = RetTy->getAs();
-  if (!(PT->getPointeeType().getUnqualifiedType()->isVoidType()))
+  const PointerType* PT = RetTy->castAs();
+  if (!PT || !PT->getPointeeType().getUnqualifiedType()->isVoidType())
 return false;
 
   // Does the name start with the prefix?

Modified: cfe/trunk/lib/Analysis/RetainSummaryManager.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/RetainSummaryManager.cpp?rev=370263=370262=370263=diff
==
--- cfe/trunk/lib/Analysis/RetainSummaryManager.cpp (original)
+++ cfe/trunk/lib/Analysis/RetainSummaryManager.cpp Wed Aug 28 14:19:58 2019
@@ -504,7 +504,7 @@ RetainSummaryManager::generateSummary(co
   FName = FName.substr(FName.find_first_not_of('_'));
 
   // Inspect the result type. Strip away any typedefs.
-  const auto *FT = FD->getType()->getAs();
+  const auto *FT = FD->getType()->castAs();
   QualType RetTy = FT->getReturnType();
 
   if (TrackOSObjects)


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

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

[Zoe Carver via Phabricator via cfe-commits]

zoecarver added a comment.

Yes, that sounds right. I can't think of any reason that the condition couldn't 
be `if (__r >  static_cast(numeric_limits::max()))`. The 
information lost from shifting the value around is never more than the 
information lost from static_casting the value (as far as I have been able to 
reason and test).


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

https://reviews.llvm.org/D66836



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

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

[Steve Canon via Phabricator via cfe-commits]

scanon added a comment.

In D66836#1649846 , @zoecarver wrote:

> > Dead link.
>
> Here: https://godbolt.org/z/AjBHYq


Yes, conversion of `numeric_limits::max` to `double` rounds to a 
value out of range for `long long`. That's not what I'm talking about. Very 
specifically, in this line:

`if (__r >= ::nextafter(static_cast<_RealT>(_MaxVal), INFINITY))`

`_MaxVal`, by construction, is representable both as `_RealT` and as `_IntT`, 
so the static_cast does not change the value (so the rounding demonstrated in 
your godbolt link doesn't create a bug). `a >= nextafter(b, INFINITY)` is 
equivalent to `a > b` for any finite floating-point `a` and `b`. So this 
condition can simply be `if (__r > static_cast<_RealT>(_MaxVal))`.


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

https://reviews.llvm.org/D66836



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

[clang-tools-extra] r370262 - Revert "[clangd] Migrate last tweak tests to TweakTesting.h and remove old helpers. NFC"

[Sam McCall via cfe-commits]

Author: sammccall
Date: Wed Aug 28 14:05:49 2019
New Revision: 370262

URL: http://llvm.org/viewvc/llvm-project?rev=370262=rev
Log:
Revert "[clangd] Migrate last tweak tests to TweakTesting.h and remove old 
helpers. NFC"

This reverts commit 8f85685b5cf57eddea11fa444503ade220c724e4, which
breaks on old gcc that have the macro + raw strings bug.

Modified:
clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp

Modified: clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp?rev=370262=370261=370262=diff
==
--- clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp (original)
+++ clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp Wed Aug 28 14:05:49 
2019
@@ -23,6 +23,8 @@
 #include "gtest/gtest.h"
 #include 
 
+using llvm::Failed;
+using llvm::Succeeded;
 using ::testing::AllOf;
 using ::testing::HasSubstr;
 using ::testing::StartsWith;
@@ -31,6 +33,100 @@ namespace clang {
 namespace clangd {
 namespace {
 
+// FIXME(sammccall): migrate the rest of the tests to use TweakTesting.h and
+// remove these helpers.
+std::string markRange(llvm::StringRef Code, Range R) {
+  size_t Begin = llvm::cantFail(positionToOffset(Code, R.start));
+  size_t End = llvm::cantFail(positionToOffset(Code, R.end));
+  assert(Begin <= End);
+  if (Begin == End) // Mark a single point.
+return (Code.substr(0, Begin) + "^" + Code.substr(Begin)).str();
+  // Mark a range.
+  return (Code.substr(0, Begin) + "[[" + Code.substr(Begin, End - Begin) +
+  "]]" + Code.substr(End))
+  .str();
+}
+
+void checkAvailable(StringRef ID, llvm::StringRef Input, bool Available) {
+  Annotations Code(Input);
+  ASSERT_TRUE(0 < Code.points().size() || 0 < Code.ranges().size())
+  << "no points of interest specified";
+  TestTU TU;
+  TU.Filename = "foo.cpp";
+  TU.Code = Code.code();
+
+  ParsedAST AST = TU.build();
+
+  auto CheckOver = [&](Range Selection) {
+unsigned Begin = cantFail(positionToOffset(Code.code(), Selection.start));
+unsigned End = cantFail(positionToOffset(Code.code(), Selection.end));
+auto T = prepareTweak(ID, Tweak::Selection(AST, Begin, End));
+if (Available)
+  EXPECT_THAT_EXPECTED(T, Succeeded())
+  << "code is " << markRange(Code.code(), Selection);
+else
+  EXPECT_THAT_EXPECTED(T, Failed())
+  << "code is " << markRange(Code.code(), Selection);
+  };
+  for (auto P : Code.points())
+CheckOver(Range{P, P});
+  for (auto R : Code.ranges())
+CheckOver(R);
+}
+
+/// Checks action is available at every point and range marked in \p Input.
+void checkAvailable(StringRef ID, llvm::StringRef Input) {
+  return checkAvailable(ID, Input, /*Available=*/true);
+}
+
+/// Same as checkAvailable, but checks the action is not available.
+void checkNotAvailable(StringRef ID, llvm::StringRef Input) {
+  return checkAvailable(ID, Input, /*Available=*/false);
+}
+
+llvm::Expected apply(StringRef ID, llvm::StringRef Input) {
+  Annotations Code(Input);
+  Range SelectionRng;
+  if (Code.points().size() != 0) {
+assert(Code.ranges().size() == 0 &&
+   "both a cursor point and a selection range were specified");
+SelectionRng = Range{Code.point(), Code.point()};
+  } else {
+SelectionRng = Code.range();
+  }
+  TestTU TU;
+  TU.Filename = "foo.cpp";
+  TU.Code = Code.code();
+
+  ParsedAST AST = TU.build();
+  unsigned Begin = cantFail(positionToOffset(Code.code(), SelectionRng.start));
+  unsigned End = cantFail(positionToOffset(Code.code(), SelectionRng.end));
+  Tweak::Selection S(AST, Begin, End);
+
+  auto T = prepareTweak(ID, S);
+  if (!T)
+return T.takeError();
+  return (*T)->apply(S);
+}
+
+llvm::Expected applyEdit(StringRef ID, llvm::StringRef Input) {
+  auto Effect = apply(ID, Input);
+  if (!Effect)
+return Effect.takeError();
+  if (!Effect->ApplyEdit)
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+   "No replacements");
+  Annotations Code(Input);
+  return applyAllReplacements(Code.code(), *Effect->ApplyEdit);
+}
+
+void checkTransform(llvm::StringRef ID, llvm::StringRef Input,
+std::string Output) {
+  auto Result = applyEdit(ID, Input);
+  ASSERT_TRUE(bool(Result)) << llvm::toString(Result.takeError()) << Input;
+  EXPECT_EQ(Output, std::string(*Result)) << Input;
+}
+
 TWEAK_TEST(SwapIfBranches);
 TEST_F(SwapIfBranchesTest, Test) {
   Context = Function;
@@ -119,9 +215,9 @@ TEST_F(DumpRecordLayoutTest, Test) {
   AllOf(StartsWith("message:"), HasSubstr("0 |   int x")));
 }
 
-TWEAK_TEST(ExtractVariable);
-TEST_F(ExtractVariableTest, Test) {
-  EXPECT_AVAILABLE(R"cpp(
+TEST(TweaksTest, ExtractVariable) {
+  llvm::StringLiteral ID = "ExtractVariable";
+  checkAvailable(ID, R"cpp(
 int xyz(int a = 1) {
   struct T {
 int bar(int a = 1);
@@ -161,14 

[PATCH] D66850: [AST][JSON] Avoid crash when dumping NULL Type as JSON

[Aaron Ballman via Phabricator via cfe-commits]

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

In D66850#1649708 , @piscisaureus 
wrote:

> In D66850#1648776 , @aaron.ballman 
> wrote:
>
> > LGTM, but missing a test case.
> >
> > In D66850#1648557 , @sidorovd 
> > wrote:
> >
> > > LGTM. I'm not an expert in JSON, but may be it makes sense to move the 
> > > change a line earlier before creation of pointer representation?
> >
> >
> > I would prefer it remains where it is -- having the `0x0` in the output for 
> > a null pointer is a good thing because it conveys more information than a 
> > totally empty `Type` object. We're accidentally inconsistent about this 
> > currently (Decl prints 0x0 but Stmt gives an empty object).
>
>
> I don't disagree, but I would argue that "0x0" is a rather poor choice, since 
> now the consumer has to treat the "id" field as an opaque value, **except** 
> when it's "0x0". A better choice would be to use JavaScript `null` to 
> represent null pointers.


Normally, I'd say yes, but in this case, we have to represent the pointer as a 
string because the number type in JSON is a signed value. Rather than "you'll 
either get a string or null", it seemed a bit more friendly to say "you'll 
always get a string" and allow the consumer to decide how to handle degenerate 
values. That said, I don't feel super strongly about this.

In D66850#1649804 , @piscisaureus 
wrote:

> Add test


Thank you!

LGTM, do you need someone to commit on your behalf?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66850



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

r370261 - Fix always_inline 'target' compatibility check code for Lambdas

[Erich Keane via cfe-commits]

Author: erichkeane
Date: Wed Aug 28 13:59:25 2019
New Revision: 370261

URL: http://llvm.org/viewvc/llvm-project?rev=370261=rev
Log:
Fix always_inline 'target' compatibility check code for Lambdas

The previous version of this used CurFuncDecl in CodeGenFunction,
however this doesn't include lambdas.  However, CurCodeDecl DOES. Switch
the check to use CurCodeDecl so that the actual function being emitted
gets checked, preventing an error in ISEL.

Modified:
cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
cfe/trunk/test/CodeGenCXX/target-features-error.cpp

Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.cpp?rev=370261=370260=370261=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenFunction.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenFunction.cpp Wed Aug 28 13:59:25 2019
@@ -2203,7 +2203,7 @@ void CodeGenFunction::checkTargetFeature
 
   // Get the current enclosing function if it exists. If it doesn't
   // we can't check the target features anyhow.
-  const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl);
+  const FunctionDecl *FD = dyn_cast_or_null(CurCodeDecl);
   if (!FD)
 return;
 

Modified: cfe/trunk/test/CodeGenCXX/target-features-error.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/target-features-error.cpp?rev=370261=370260=370261=diff
==
--- cfe/trunk/test/CodeGenCXX/target-features-error.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/target-features-error.cpp Wed Aug 28 13:59:25 2019
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -S -verify -o -
+// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -S -verify -o - -DTEST1
+// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -S -verify -o - -DTEST2
+// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -S -verify -o - -DTEST3
 
 struct S {
   __attribute__((always_inline, target("avx512f")))
@@ -9,9 +11,51 @@ struct S {
   void operator()(){ }
 
 };
+__attribute__((always_inline, target("avx512f")))
+void free_func(){}
 
+
+#ifdef TEST1
 void usage(S & s) {
   s.foo(); // expected-error {{'foo' requires target feature 'avx512f'}}
   (void)(int)s; // expected-error {{'operator int' requires target feature 
'avx512f'}}
   s(); // expected-error {{'operator()' requires target feature 'avx512f'}}
+  free_func(); // expected-error{{'free_func' requires target feature 
'avx512f'}}
+
+}
+#endif
+
+#ifdef TEST2
+__attribute__((target("avx512f")))
+void usage(S & s) {
+  s.foo();
+  (void)(int)s;
+  s();
+
+  [] {
+s.foo();   // expected-error {{'foo' requires target feature 
'avx512f'}}
+(void)(int) s; // expected-error {{'operator int' requires target feature 
'avx512f'}}
+s();   // expected-error {{'operator()' requires target feature 
'avx512f'}}
+free_func();   // expected-error{{'free_func' requires target feature 
'avx512f'}}
+  }();
+}
+#endif
+
+#ifdef TEST3
+void usage(S & s) {
+
+  [] () __attribute__((target("avx512f"))) {
+s.foo();
+(void)(int) s;
+s();
+free_func();
+  }();
+
+  [] {
+s.foo();   // expected-error {{'foo' requires target feature 
'avx512f'}}
+(void)(int) s; // expected-error {{'operator int' requires target feature 
'avx512f'}}
+s();   // expected-error {{'operator()' requires target feature 
'avx512f'}}
+free_func();   // expected-error{{'free_func' requires target feature 
'avx512f'}}
+  }();
 }
+#endif


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

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

[Zoe Carver via Phabricator via cfe-commits]

zoecarver added a comment.

> Dead link.

Here: https://godbolt.org/z/AjBHYq


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

https://reviews.llvm.org/D66836



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

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

[Steve Canon via Phabricator via cfe-commits]

scanon added a comment.

> Eric showed me this link https://godbolt.org/z/AjBHYqv

Dead link.


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

https://reviews.llvm.org/D66836



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

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

[Zoe Carver via Phabricator via cfe-commits]

zoecarver added inline comments.



Comment at: include/math.h:1582
+  const _RealT __trunc_r = __builtin_trunc(__r);
+  if (__trunc_r >= ::nextafter(static_cast<_RealT>(_MaxVal), INFINITY)) {
+return _Lim::max();

scanon wrote:
> EricWF wrote:
> > scanon wrote:
> > > zoecarver wrote:
> > > > Maybe change `INFINITY` to `std::numeric_limits< _RealT >::infinity()`
> > > Why isn't this just `__trunc_r > _MaxVal`?
> > Consider `long long` and `double`. `MaxVal - numeric_limits > long>::max() == 1024`, and we want values between `MaxVal` and `::max()` to 
> > round down. So instead we essentially check for `__r >= numeric_limits > long>::max() + 1`. This approach seems more accurate.
> > Consider long long and double. MaxVal - numeric_limits::max() == 
> > 1024, and we want values between MaxVal and ::max() to round down. So 
> > instead we essentially check for __r >= numeric_limits::max() + 1
> 
> Yes, but there are no values between MaxVal and ::max() in the floating-point 
> format; if there were, they would be MaxVal instead. So you can ditch the 
> nextafter and just use `> static_cast<_FloatT>(MaxVal)`.
I thought the same thing, but that isn't necessarily true. Eric showed me this 
link https://godbolt.org/z/AjBHYqv which does a good job showing what happens 
when trying to compare an integer value and float value. See the precision loss:
> warning: implicit conversion from 'long long' to 'double' changes value from 
> 9223372036854775807 to 9223372036854775808 


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

https://reviews.llvm.org/D66836



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

[PATCH] D66850: [AST][JSON] Avoid crash when dumping NULL Type as JSON

[Bert Belder via Phabricator via cfe-commits]

piscisaureus updated this revision to Diff 217713.
piscisaureus added a comment.

Add test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66850

Files:
  clang/lib/AST/JSONNodeDumper.cpp
  clang/test/AST/ast-dump-types-json.cpp
  clang/test/AST/gen_ast_dump_json_test.py

Index: clang/test/AST/gen_ast_dump_json_test.py
===
--- clang/test/AST/gen_ast_dump_json_test.py
+++ clang/test/AST/gen_ast_dump_json_test.py
@@ -20,7 +20,7 @@
 normalize(e)
 elif type(v) is unicode:
 st = v.encode('utf-8')
-if re.match(r"0x[0-9A-Fa-f]+", v):
+if v != "0x0" and re.match(r"0x[0-9A-Fa-f]+", v):
 dict_var[k] = u'0x{{.*}}'
 elif os.path.isfile(v):
 dict_var[k] = u'{{.*}}'
Index: clang/test/AST/ast-dump-types-json.cpp
===
--- clang/test/AST/ast-dump-types-json.cpp
+++ clang/test/AST/ast-dump-types-json.cpp
@@ -20,6 +20,11 @@
 
 typedef int TestQualTypePrinting(const char* c);
 
+typedef int TestUsingShadowDeclType;
+namespace TestNamespaceWithUsingShadowType {
+using ::TestUsingShadowDeclType;
+}
+
 // NOTE: CHECK lines have been autogenerated by gen_ast_dump_json_test.py
 
 
@@ -422,3 +427,74 @@
 // CHECK-NEXT:   }
 // CHECK-NEXT:  ]
 // CHECK-NEXT: }
+
+// CHECK:  "kind": "NamespaceDecl",
+// CHECK-NEXT:  "loc": {
+// CHECK-NEXT:   "line": 24,
+// CHECK-NEXT:   "col": 11,
+// CHECK-NEXT:   "tokLen": 32
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "range": {
+// CHECK-NEXT:   "begin": {
+// CHECK-NEXT:"col": 1,
+// CHECK-NEXT:"tokLen": 9
+// CHECK-NEXT:   },
+// CHECK-NEXT:   "end": {
+// CHECK-NEXT:"line": 26,
+// CHECK-NEXT:"col": 1,
+// CHECK-NEXT:"tokLen": 1
+// CHECK-NEXT:   }
+// CHECK-NEXT:  },
+// CHECK-NEXT:  "name": "TestNamespaceWithUsingShadowType",
+// CHECK-NEXT:  "inner": [
+// CHECK-NEXT:   {
+// CHECK-NEXT:"id": "0x{{.*}}",
+// CHECK-NEXT:"kind": "UsingDecl",
+// CHECK-NEXT:"loc": {
+// CHECK-NEXT: "line": 25,
+// CHECK-NEXT: "col": 9,
+// CHECK-NEXT: "tokLen": 23
+// CHECK-NEXT:},
+// CHECK-NEXT:"range": {
+// CHECK-NEXT: "begin": {
+// CHECK-NEXT:  "col": 1,
+// CHECK-NEXT:  "tokLen": 5
+// CHECK-NEXT: },
+// CHECK-NEXT: "end": {
+// CHECK-NEXT:  "col": 9,
+// CHECK-NEXT:  "tokLen": 23
+// CHECK-NEXT: }
+// CHECK-NEXT:},
+// CHECK-NEXT:"name": "::TestUsingShadowDeclType"
+// CHECK-NEXT:   },
+// CHECK-NEXT:   {
+// CHECK-NEXT:"id": "0x{{.*}}",
+// CHECK-NEXT:"kind": "UsingShadowDecl",
+// CHECK-NEXT:"loc": {
+// CHECK-NEXT: "col": 9,
+// CHECK-NEXT: "tokLen": 23
+// CHECK-NEXT:},
+// CHECK-NEXT:"range": {
+// CHECK-NEXT: "begin": {
+// CHECK-NEXT:  "col": 9,
+// CHECK-NEXT:  "tokLen": 23
+// CHECK-NEXT: },
+// CHECK-NEXT: "end": {
+// CHECK-NEXT:  "col": 9,
+// CHECK-NEXT:  "tokLen": 23
+// CHECK-NEXT: }
+// CHECK-NEXT:},
+// CHECK-NEXT:"isImplicit": true,
+// CHECK-NEXT:"target": {
+// CHECK-NEXT: "id": "0x{{.*}}",
+// CHECK-NEXT: "kind": "TypedefDecl",
+// CHECK-NEXT: "name": "TestUsingShadowDeclType"
+// CHECK-NEXT:},
+// CHECK-NEXT:"inner": [
+// CHECK-NEXT: {
+// CHECK-NEXT:  "id": "0x0"
+// CHECK-NEXT: }
+// CHECK-NEXT:]
+// CHECK-NEXT:   }
+// CHECK-NEXT:  ]
+// CHECK-NEXT: }
Index: clang/lib/AST/JSONNodeDumper.cpp
===
--- clang/lib/AST/JSONNodeDumper.cpp
+++ clang/lib/AST/JSONNodeDumper.cpp
@@ -66,6 +66,10 @@
 
 void JSONNodeDumper::Visit(const Type *T) {
   JOS.attribute("id", createPointerRepresentation(T));
+
+  if (!T)
+return;
+
   JOS.attribute("kind", (llvm::Twine(T->getTypeClassName()) + "Type").str());
   JOS.attribute("type", createQualType(QualType(T, 0), /*Desugar*/ false));
   attributeOnlyIfTrue("isDependent", T->isDependentType());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66559: [OPENMP] Update the diagnosis message for canonical loop form

[Chi Chun Chen via Phabricator via cfe-commits]

cchen added a comment.

@ABataev thanks so much for your instruction, I'll look into that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66559



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

[PATCH] D66559: [OPENMP] Update the diagnosis message for canonical loop form

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D66559#1649775 , @cchen wrote:

> Haven't updated the lit test for OpenMP 5.0 yet since I'm not able to check 
> OPENMP version by using _OPENMP with preprocessor condition checking.


You don't need `_OPENMP` macro for this. There a lot of tests in the OpenMP 
directory for OpenMP 5.0 already, look for `-fopenmp-version=50` option in the 
tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66559



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

[PATCH] D66559: [OPENMP] Update the diagnosis message for canonical loop form

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

What about the check for `!=`? it would be good to allow it only for OpenMP 5.0.
Also, add/update the tests.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9027-9032
+def err_omp_before_50_loop_not_canonical_cond : Error<
   "condition of OpenMP for loop must be a relational comparison "
-  "('<', '<=', '>', or '>=') of loop variable %0">;
+   "('<', '<=', '>', or '>=') of loop variable %0">;
+def err_omp_after_50_loop_not_canonical_cond : Error<
+  "condition of OpenMP for loop must be a relational comparison "
+   "('<', '<=', '>', '>=', or '!=') of loop variable %0">;

You can merge these 2 messages into one using something like 
`"condition of OpenMP for loop must be a relational comparison ('<', '<=', '>', 
%select{or '>='|'>=', or '!='}0) of loop variable %1"`



Comment at: clang/lib/Sema/SemaOpenMP.cpp:5422
+   if (SemaRef.getLangOpts().OpenMP < 50)
+   SemaRef.Diag(DefaultLoc, 
diag::err_omp_before_50_loop_not_canonical_cond) << LCDecl;
+   else

If you merge 2 messages into one, you can diagnose it this way:
```
SemaRef.Diag(DefaultLoc, diag::err_omp_loop_not_canonical_cond) << 
(SemaRef.getLangOpts().OpenMP <= 45 ? 0 : 1) << LCDecl;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66559



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

[PATCH] D66559: [OPENMP] Update the diagnosis message for canonical loop form

[Chi Chun Chen via Phabricator via cfe-commits]

cchen added a comment.

Haven't updated the lit test for OpenMP 5.0 yet since I'm not able to check 
OPENMP version by using _OPENMP with preprocessor condition checking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66559



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

[PATCH] D66559: [OPENMP] Update the diagnosis message for canonical loop form

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

`!=` is still not being diagnosed in pre-5.0 mode though?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66559



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

[PATCH] D66559: [OPENMP] Update the diagnosis message for canonical loop form

[Chi Chun Chen via Phabricator via cfe-commits]

cchen updated this revision to Diff 217710.
cchen added a comment.

Update the diagnosis message for canonical loop form based on OpenMP version


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66559

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaOpenMP.cpp


Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -5413,12 +5413,15 @@
 bool OpenMPIterationSpaceChecker::checkAndSetCond(Expr *S) {
   // Check test-expr for canonical form, save upper-bound UB, flags for
   // less/greater and for strict/non-strict comparison.
-  // OpenMP [2.6] Canonical loop form. Test-expr may be one of the following:
+  // OpenMP [2.9] Canonical loop form. Test-expr may be one of the following:
   //   var relational-op b
   //   b relational-op var
   //
   if (!S) {
-SemaRef.Diag(DefaultLoc, diag::err_omp_loop_not_canonical_cond) << LCDecl;
+   if (SemaRef.getLangOpts().OpenMP < 50)
+   SemaRef.Diag(DefaultLoc, 
diag::err_omp_before_50_loop_not_canonical_cond) << LCDecl;
+   else
+   SemaRef.Diag(DefaultLoc, 
diag::err_omp_after_50_loop_not_canonical_cond) << LCDecl;
 return true;
   }
   Condition = S;
@@ -5474,8 +5477,12 @@
   }
   if (dependent() || SemaRef.CurContext->isDependentContext())
 return false;
-  SemaRef.Diag(CondLoc, diag::err_omp_loop_not_canonical_cond)
-  << S->getSourceRange() << LCDecl;
+   if (SemaRef.getLangOpts().OpenMP < 50)
+   SemaRef.Diag(CondLoc, 
diag::err_omp_before_50_loop_not_canonical_cond)
+   << S->getSourceRange() << LCDecl;
+   else
+   SemaRef.Diag(CondLoc, 
diag::err_omp_after_50_loop_not_canonical_cond)
+   << S->getSourceRange() << LCDecl;
   return true;
 }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9024,9 +9024,12 @@
 def ext_omp_loop_not_canonical_init : ExtWarn<
   "initialization clause of OpenMP for loop is not in canonical form "
   "('var = init' or 'T var = init')">, InGroup;
-def err_omp_loop_not_canonical_cond : Error<
+def err_omp_before_50_loop_not_canonical_cond : Error<
   "condition of OpenMP for loop must be a relational comparison "
-  "('<', '<=', '>', or '>=') of loop variable %0">;
+   "('<', '<=', '>', or '>=') of loop variable %0">;
+def err_omp_after_50_loop_not_canonical_cond : Error<
+  "condition of OpenMP for loop must be a relational comparison "
+   "('<', '<=', '>', '>=', or '!=') of loop variable %0">;
 def err_omp_loop_not_canonical_incr : Error<
   "increment clause of OpenMP for loop must perform simple addition "
   "or subtraction on loop variable %0">;


Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -5413,12 +5413,15 @@
 bool OpenMPIterationSpaceChecker::checkAndSetCond(Expr *S) {
   // Check test-expr for canonical form, save upper-bound UB, flags for
   // less/greater and for strict/non-strict comparison.
-  // OpenMP [2.6] Canonical loop form. Test-expr may be one of the following:
+  // OpenMP [2.9] Canonical loop form. Test-expr may be one of the following:
   //   var relational-op b
   //   b relational-op var
   //
   if (!S) {
-SemaRef.Diag(DefaultLoc, diag::err_omp_loop_not_canonical_cond) << LCDecl;
+		if (SemaRef.getLangOpts().OpenMP < 50)
+			SemaRef.Diag(DefaultLoc, diag::err_omp_before_50_loop_not_canonical_cond) << LCDecl;
+		else
+			SemaRef.Diag(DefaultLoc, diag::err_omp_after_50_loop_not_canonical_cond) << LCDecl;
 return true;
   }
   Condition = S;
@@ -5474,8 +5477,12 @@
   }
   if (dependent() || SemaRef.CurContext->isDependentContext())
 return false;
-  SemaRef.Diag(CondLoc, diag::err_omp_loop_not_canonical_cond)
-  << S->getSourceRange() << LCDecl;
+	if (SemaRef.getLangOpts().OpenMP < 50)
+		SemaRef.Diag(CondLoc, diag::err_omp_before_50_loop_not_canonical_cond)
+<< S->getSourceRange() << LCDecl;
+	else
+		SemaRef.Diag(CondLoc, diag::err_omp_after_50_loop_not_canonical_cond)
+<< S->getSourceRange() << LCDecl;
   return true;
 }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9024,9 +9024,12 @@
 def ext_omp_loop_not_canonical_init : ExtWarn<
   "initialization clause of OpenMP for loop is not in canonical form "
   "('var = init' or 'T var = init')">, InGroup;
-def 

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

[Eric Fiselier via Phabricator via cfe-commits]

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



Comment at: include/math.h:1573
+  enum { _Bits = numeric_limits<_IntT>::digits - 
numeric_limits<_FloatT>::digits };
+  static const _IntT value = numeric_limits<_IntT>::max() >> _Bits << _Bits;
+};

scanon wrote:
> zoecarver wrote:
> > What's the reasoning behind shifting something forward and back? Shouldn't 
> > this always negate the other operation? 
> This function doesn't quite do what it says on the tin; it considers the 
> number of significand bits used for the floating-point type, but not the 
> exponent range. This doesn't matter for double, because double's exponent 
> range is much, much larger than any integer type, but it does matter for 
> types like float16 (largest representable value is 65504)--when it's added as 
> a standard floating-point type at some future point, this will introduce 
> subtle bugs.
> 
> You should be able to work around this by converting `value` to  `_FloatT`, 
> taking the minimum of the result and numeric_limits::max, and converting back.
> 
> This also assumes that _FloatT has radix == 2, which I do not believe is 
> actually implied by `is_floating_point == true`. Please add a static assert 
> for that so that future decimal types don't use this template.
Very good point. I've added the static assertions and limited the function to 
`float`, `double`, and `long double` so the `fp16` case won't bite us anytime 
soon.



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

https://reviews.llvm.org/D66836



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

[PATCH] D66903: [OPENMP] Update the diagnosis message for canonical loop form

[Chi Chun Chen via Phabricator via cfe-commits]

cchen added a comment.

I should update a revision instead of creating this new one, so I'll just 
remove the subscriber. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66903



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

[PATCH] D66903: [OPENMP] Update the diagnosis message for canonical loop form

[Chi Chun Chen via Phabricator via cfe-commits]

cchen created this revision.
Herald added subscribers: cfe-commits, guansong.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.

The previous patch (https://reviews.llvm.org/D54441) support the
relational-op != very well for openmp canonical loop form, however,
it didn't update the diagnosis message. So this patch is simply
update the diagnosis message by adding != for OpenMP 5.0.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66903

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaOpenMP.cpp


Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -5413,12 +5413,15 @@
 bool OpenMPIterationSpaceChecker::checkAndSetCond(Expr *S) {
   // Check test-expr for canonical form, save upper-bound UB, flags for
   // less/greater and for strict/non-strict comparison.
-  // OpenMP [2.6] Canonical loop form. Test-expr may be one of the following:
+  // OpenMP [2.9] Canonical loop form. Test-expr may be one of the following:
   //   var relational-op b
   //   b relational-op var
   //
   if (!S) {
-SemaRef.Diag(DefaultLoc, diag::err_omp_loop_not_canonical_cond) << LCDecl;
+   if (SemaRef.getLangOpts().OpenMP < 50)
+   SemaRef.Diag(DefaultLoc, 
diag::err_omp_before_50_loop_not_canonical_cond) << LCDecl;
+   else
+   SemaRef.Diag(DefaultLoc, 
diag::err_omp_after_50_loop_not_canonical_cond) << LCDecl;
 return true;
   }
   Condition = S;
@@ -5474,8 +5477,12 @@
   }
   if (dependent() || SemaRef.CurContext->isDependentContext())
 return false;
-  SemaRef.Diag(CondLoc, diag::err_omp_loop_not_canonical_cond)
-  << S->getSourceRange() << LCDecl;
+   if (SemaRef.getLangOpts().OpenMP < 50)
+   SemaRef.Diag(CondLoc, 
diag::err_omp_before_50_loop_not_canonical_cond)
+   << S->getSourceRange() << LCDecl;
+   else
+   SemaRef.Diag(CondLoc, 
diag::err_omp_after_50_loop_not_canonical_cond)
+   << S->getSourceRange() << LCDecl;
   return true;
 }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9024,9 +9024,12 @@
 def ext_omp_loop_not_canonical_init : ExtWarn<
   "initialization clause of OpenMP for loop is not in canonical form "
   "('var = init' or 'T var = init')">, InGroup;
-def err_omp_loop_not_canonical_cond : Error<
+def err_omp_before_50_loop_not_canonical_cond : Error<
   "condition of OpenMP for loop must be a relational comparison "
-  "('<', '<=', '>', or '>=') of loop variable %0">;
+   "('<', '<=', '>', or '>=') of loop variable %0">;
+def err_omp_after_50_loop_not_canonical_cond : Error<
+  "condition of OpenMP for loop must be a relational comparison "
+   "('<', '<=', '>', '>=', or '!=') of loop variable %0">;
 def err_omp_loop_not_canonical_incr : Error<
   "increment clause of OpenMP for loop must perform simple addition "
   "or subtraction on loop variable %0">;


Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -5413,12 +5413,15 @@
 bool OpenMPIterationSpaceChecker::checkAndSetCond(Expr *S) {
   // Check test-expr for canonical form, save upper-bound UB, flags for
   // less/greater and for strict/non-strict comparison.
-  // OpenMP [2.6] Canonical loop form. Test-expr may be one of the following:
+  // OpenMP [2.9] Canonical loop form. Test-expr may be one of the following:
   //   var relational-op b
   //   b relational-op var
   //
   if (!S) {
-SemaRef.Diag(DefaultLoc, diag::err_omp_loop_not_canonical_cond) << LCDecl;
+		if (SemaRef.getLangOpts().OpenMP < 50)
+			SemaRef.Diag(DefaultLoc, diag::err_omp_before_50_loop_not_canonical_cond) << LCDecl;
+		else
+			SemaRef.Diag(DefaultLoc, diag::err_omp_after_50_loop_not_canonical_cond) << LCDecl;
 return true;
   }
   Condition = S;
@@ -5474,8 +5477,12 @@
   }
   if (dependent() || SemaRef.CurContext->isDependentContext())
 return false;
-  SemaRef.Diag(CondLoc, diag::err_omp_loop_not_canonical_cond)
-  << S->getSourceRange() << LCDecl;
+	if (SemaRef.getLangOpts().OpenMP < 50)
+		SemaRef.Diag(CondLoc, diag::err_omp_before_50_loop_not_canonical_cond)
+<< S->getSourceRange() << LCDecl;
+	else
+		SemaRef.Diag(CondLoc, diag::err_omp_after_50_loop_not_canonical_cond)
+<< S->getSourceRange() << LCDecl;
   return true;
 }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9024,9 +9024,12 @@
 def 

[PATCH] D66856: [Sema] Suppress -Wformat diagnostics for bool types when printed using %hhd

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

In D66856#1648809 , @aaron.ballman 
wrote:

> I'm wondering whether this should even warn pedantically. There are no format 
> specifiers that apply directly to a `_Bool` datatype, so the user is left 
> making a choice as to what specifier fits best. I think `hhd` and `hhu` are 
> both equally reasonable types, as are the `d` and `u` groups directly -- I 
> don't think we should warn on either of those specifiers with a `_Bool` 
> argument. I could maybe see a case for pedantically diagnosing `h`, `l`, and 
> `ll` prefixes with `_Bool` because that seems a bit type-confused to me. `c` 
> as a specifier seems weird (given that false values will potentially 
> terminate the string suddenly depending on terminal behavior, IIRC), but `lc` 
> seems like type confusion again.


Hmm... on second though I think the `l` and `ll` prefixes are worth -Wformat 
proper, since that can result in printing a half-initialized integer when the 
argument ends up on the stack (we generate `movl $0,(%rsp)` for the first stack 
argument). I think `%c` is probably also worth a -Wformat, it seems like it 
would almost certainly be a mistake. I suppose we could pedantically warn on 
`%hd`, but I'm not sure. Who is the audience for -Wformat-pedantic? Users that 
expect it to diagnose any pedantically-UB printf calls will already be 
disappointed, since the C standard seems to be quite a lot more restrictive 
then us (7.21.6.9p9 says: "If any argument is not the correct type for the 
corresponding conversion specification, the behavior is undefined." (I'm 
assuming "correct type" means "same type")). So I think we should just concern 
ourselves with calls that would lead to actual problems at runtime, or are 
likely programmer errors, and `printf("%hd", (_Bool)0)` doesn't seem to meet 
either of those bars.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66856



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

[PATCH] D66850: [AST][JSON] Avoid crash when dumping NULL Type as JSON

[Bert Belder via Phabricator via cfe-commits]

piscisaureus added a comment.

In D66850#1648776 , @aaron.ballman 
wrote:

> LGTM, but missing a test case.
>
> In D66850#1648557 , @sidorovd wrote:
>
> > LGTM. I'm not an expert in JSON, but may be it makes sense to move the 
> > change a line earlier before creation of pointer representation?
>
>
> I would prefer it remains where it is -- having the `0x0` in the output for a 
> null pointer is a good thing because it conveys more information than a 
> totally empty `Type` object. We're accidentally inconsistent about this 
> currently (Decl prints 0x0 but Stmt gives an empty object).


I don't disagree, but I would argue that "0x0" is a rather poor choice, since 
now the consumer has to treat the "id" field as an opaque value, **except** 
when it's "0x0". A better choice would be to use JavaScript `null` to represent 
null pointers.
That said, I've simply followed the pattern established in `visit(Decl*)` here; 
switching to `null` would warrant a separate patch IMO.


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

https://reviews.llvm.org/D66850



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

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

[Steve Canon via Phabricator via cfe-commits]

scanon requested changes to this revision.
scanon added inline comments.
This revision now requires changes to proceed.



Comment at: include/math.h:1586
+return _Lim::min();
+  }
+  return static_cast<_IntT>(__trunc_r);

EricWF wrote:
> scanon wrote:
> > If I'm reading right, NaNs will fall through the above two comparisons and 
> > invoke UB on the static_cast below. I suspect that's not the desired 
> > behavior. What is the intended result for NaN?
> I didn't want to treat `NaN` as a valid input, so I want to allow UBSAN to 
> catch it rather than to provide a valid output.
Please document this clearly; otherwise someone will assume that this is a 
UB-free conversion and use it for that purpose.


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

https://reviews.llvm.org/D66836



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

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

[Steve Canon via Phabricator via cfe-commits]

scanon added inline comments.



Comment at: include/math.h:1582
+  const _RealT __trunc_r = __builtin_trunc(__r);
+  if (__trunc_r >= ::nextafter(static_cast<_RealT>(_MaxVal), INFINITY)) {
+return _Lim::max();

EricWF wrote:
> scanon wrote:
> > zoecarver wrote:
> > > Maybe change `INFINITY` to `std::numeric_limits< _RealT >::infinity()`
> > Why isn't this just `__trunc_r > _MaxVal`?
> Consider `long long` and `double`. `MaxVal - numeric_limits::max() 
> == 1024`, and we want values between `MaxVal` and `::max()` to round down. So 
> instead we essentially check for `__r >= numeric_limits::max() + 
> 1`. This approach seems more accurate.
> Consider long long and double. MaxVal - numeric_limits::max() == 
> 1024, and we want values between MaxVal and ::max() to round down. So instead 
> we essentially check for __r >= numeric_limits::max() + 1

Yes, but there are no values between MaxVal and ::max() in the floating-point 
format; if there were, they would be MaxVal instead. So you can ditch the 
nextafter and just use `> static_cast<_FloatT>(MaxVal)`.


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

https://reviews.llvm.org/D66836



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

[PATCH] D65526: [Clangd] First version of ExtractFunction

[Shaurya Gupta via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370249: [Clangd] Initial version of ExtractFunction 
(authored by SureYeaah, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65526?vs=217701=217702#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65526

Files:
  clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp
  clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp
@@ -500,6 +500,112 @@
 R"cpp(const char * x = "test")cpp");
 }
 
+TWEAK_TEST(ExtractFunction);
+TEST_F(ExtractFunctionTest, FunctionTest) {
+  Context = Function;
+
+  // Root statements should have common parent.
+  EXPECT_EQ(apply("for(;;) [[1+2; 1+2;]]"), "unavailable");
+  // Expressions aren't extracted.
+  EXPECT_EQ(apply("int x = 0; [[x++;]]"), "unavailable");
+  // We don't support extraction from lambdas.
+  EXPECT_EQ(apply("auto lam = [](){ [[int x;]] }; "), "unavailable");
+
+  // Ensure that end of Zone and Beginning of PostZone being adjacent doesn't
+  // lead to break being included in the extraction zone.
+  EXPECT_THAT(apply("for(;;) { [[int x;]]break; }"), HasSubstr("extracted"));
+  // FIXME: This should be unavailable since partially selected but
+  // selectionTree doesn't always work correctly for VarDecls.
+  EXPECT_THAT(apply("int [[x = 0]];"), HasSubstr("extracted"));
+  // FIXME: ExtractFunction should be unavailable inside loop construct
+  // initalizer/condition.
+  EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("extracted"));
+  // Don't extract because needs hoisting.
+  EXPECT_THAT(apply(" [[int a = 5;]] a++; "), StartsWith("fail"));
+  // Don't extract return
+  EXPECT_THAT(apply(" if(true) [[return;]] "), StartsWith("fail"));
+  // Don't extract break and continue.
+  // FIXME: We should be able to extract this since it's non broken.
+  EXPECT_THAT(apply(" [[for(;;) break;]] "), StartsWith("fail"));
+  EXPECT_THAT(apply(" for(;;) [[continue;]] "), StartsWith("fail"));
+}
+
+TEST_F(ExtractFunctionTest, FileTest) {
+  // Check all parameters are in order
+  std::string ParameterCheckInput = R"cpp(
+struct Foo {
+  int x;
+};
+void f(int a) {
+  int b;
+  int *ptr = 
+  Foo foo;
+  [[a += foo.x + b;
+  *ptr++;]]
+})cpp";
+  std::string ParameterCheckOutput = R"cpp(
+struct Foo {
+  int x;
+};
+void extracted(int , int , int * , Foo ) {
+a += foo.x + b;
+  *ptr++;
+}
+void f(int a) {
+  int b;
+  int *ptr = 
+  Foo foo;
+  extracted(a, b, ptr, foo);
+})cpp";
+  EXPECT_EQ(apply(ParameterCheckInput), ParameterCheckOutput);
+
+  // Check const qualifier
+  std::string ConstCheckInput = R"cpp(
+void f(const int c) {
+  [[while(c) {}]]
+})cpp";
+  std::string ConstCheckOutput = R"cpp(
+void extracted(const int ) {
+while(c) {}
+}
+void f(const int c) {
+  extracted(c);
+})cpp";
+  EXPECT_EQ(apply(ConstCheckInput), ConstCheckOutput);
+
+  // Don't extract when we need to make a function as a parameter.
+  EXPECT_THAT(apply("void f() { [[int a; f();]] }"), StartsWith("fail"));
+
+  // We don't extract from methods for now since they may involve multi-file
+  // edits
+  std::string MethodFailInput = R"cpp(
+class T {
+  void f() {
+[[int x;]]
+  }
+};
+  )cpp";
+  EXPECT_EQ(apply(MethodFailInput), "unavailable");
+
+  // We don't extract from templated functions for now as templates are hard
+  // to deal with.
+  std::string TemplateFailInput = R"cpp(
+template
+void f() {
+  [[int x;]]
+}
+  )cpp";
+  EXPECT_EQ(apply(TemplateFailInput), "unavailable");
+
+  // FIXME: This should be extractable after selectionTree works correctly for
+  // macros (currently it doesn't select anything for the following case)
+  std::string MacroFailInput = R"cpp(
+#define F(BODY) void f() { BODY }
+F ([[int x = 0;]])
+  )cpp";
+  EXPECT_EQ(apply(MacroFailInput), "unavailable");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp
===
--- clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp
+++ clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -0,0 +1,605 @@
+//===--- ExtractFunction.cpp -*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//

[PATCH] D65526: [Clangd] First version of ExtractFunction

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:119
+  case SelectionTree::Selection::Partial:
+// Treat Partially selected VarDecl as completely selected since
+// SelectionTree doesn't always select VarDecls correctly.

sammccall wrote:
> D66872 has the fix if you'd rather avoid these workarounds
Will remove this once D66872 is committed.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:150
+  }
+  bool isRootStmt(const Stmt *S) const;
+  // The last root statement is important to decide where we need to insert a

kadircet wrote:
> it seems like you are rather using it to decide whether a statement is inside 
> the zone or not?
> 
> Could you rather rename it to reflect that and add some comments?
if extracting 
```
for(;;)
  int x = 0;
```
the DeclStmt is inside the zone but not a rootstmt.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:172
+  // FIXME: Support extraction from templated functions.
+  if (CurNode->Parent->ASTNode.get())
+return nullptr;

kadircet wrote:
> nit:
> ```
> if(isa(Func))
> ```
Using `Func->isTemplated()`



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:346
+ZoneRelative DeclaredIn;
+// index of the declaration or first reference
+unsigned DeclIndex;

kadircet wrote:
> i think you mean `index of the first reference to this decl` ?
It can be the index of declaration or the first reference. Since the visitor 
Visits Decls as well, we will encounter a reference before the Decl only if the 
Decl is outside the enclosing function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65526



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

[PATCH] D65526: [Clangd] First version of ExtractFunction

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah updated this revision to Diff 217701.
SureYeaah marked 14 inline comments as done.
SureYeaah added a comment.

Addressed review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65526

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -599,6 +599,112 @@
 R"cpp(const char * x = "test")cpp");
 }
 
+TWEAK_TEST(ExtractFunction);
+TEST_F(ExtractFunctionTest, FunctionTest) {
+  Context = Function;
+
+  // Root statements should have common parent.
+  EXPECT_EQ(apply("for(;;) [[1+2; 1+2;]]"), "unavailable");
+  // Expressions aren't extracted.
+  EXPECT_EQ(apply("int x = 0; [[x++;]]"), "unavailable");
+  // We don't support extraction from lambdas.
+  EXPECT_EQ(apply("auto lam = [](){ [[int x;]] }; "), "unavailable");
+
+  // Ensure that end of Zone and Beginning of PostZone being adjacent doesn't
+  // lead to break being included in the extraction zone.
+  EXPECT_THAT(apply("for(;;) { [[int x;]]break; }"), HasSubstr("extracted"));
+  // FIXME: This should be unavailable since partially selected but
+  // selectionTree doesn't always work correctly for VarDecls.
+  EXPECT_THAT(apply("int [[x = 0]];"), HasSubstr("extracted"));
+  // FIXME: ExtractFunction should be unavailable inside loop construct
+  // initalizer/condition.
+  EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("extracted"));
+  // Don't extract because needs hoisting.
+  EXPECT_THAT(apply(" [[int a = 5;]] a++; "), StartsWith("fail"));
+  // Don't extract return
+  EXPECT_THAT(apply(" if(true) [[return;]] "), StartsWith("fail"));
+  // Don't extract break and continue.
+  // FIXME: We should be able to extract this since it's non broken.
+  EXPECT_THAT(apply(" [[for(;;) break;]] "), StartsWith("fail"));
+  EXPECT_THAT(apply(" for(;;) [[continue;]] "), StartsWith("fail"));
+}
+
+TEST_F(ExtractFunctionTest, FileTest) {
+  // Check all parameters are in order
+  std::string ParameterCheckInput = R"cpp(
+struct Foo {
+  int x;
+};
+void f(int a) {
+  int b;
+  int *ptr = 
+  Foo foo;
+  [[a += foo.x + b;
+  *ptr++;]]
+})cpp";
+  std::string ParameterCheckOutput = R"cpp(
+struct Foo {
+  int x;
+};
+void extracted(int , int , int * , Foo ) {
+a += foo.x + b;
+  *ptr++;
+}
+void f(int a) {
+  int b;
+  int *ptr = 
+  Foo foo;
+  extracted(a, b, ptr, foo);
+})cpp";
+  EXPECT_EQ(apply(ParameterCheckInput), ParameterCheckOutput);
+
+  // Check const qualifier
+  std::string ConstCheckInput = R"cpp(
+void f(const int c) {
+  [[while(c) {}]]
+})cpp";
+  std::string ConstCheckOutput = R"cpp(
+void extracted(const int ) {
+while(c) {}
+}
+void f(const int c) {
+  extracted(c);
+})cpp";
+  EXPECT_EQ(apply(ConstCheckInput), ConstCheckOutput);
+
+  // Don't extract when we need to make a function as a parameter.
+  EXPECT_THAT(apply("void f() { [[int a; f();]] }"), StartsWith("fail"));
+
+  // We don't extract from methods for now since they may involve multi-file
+  // edits
+  std::string MethodFailInput = R"cpp(
+class T {
+  void f() {
+[[int x;]]
+  }
+};
+  )cpp";
+  EXPECT_EQ(apply(MethodFailInput), "unavailable");
+
+  // We don't extract from templated functions for now as templates are hard
+  // to deal with.
+  std::string TemplateFailInput = R"cpp(
+template
+void f() {
+  [[int x;]]
+}
+  )cpp";
+  EXPECT_EQ(apply(TemplateFailInput), "unavailable");
+
+  // FIXME: This should be extractable after selectionTree works correctly for
+  // macros (currently it doesn't select anything for the following case)
+  std::string MacroFailInput = R"cpp(
+#define F(BODY) void f() { BODY }
+F ([[int x = 0;]])
+  )cpp";
+  EXPECT_EQ(apply(MacroFailInput), "unavailable");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -0,0 +1,604 @@
+//===--- ExtractFunction.cpp -*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Extracts statements to a new function and replaces the statements with a
+// call to the new function.
+// Before:
+//   void f(int a) {
+// [[if(a < 5)
+//   a = 5;]]
+//   }
+// After:
+//   

[clang-tools-extra] r370249 - [Clangd] Initial version of ExtractFunction

[Shaurya Gupta via cfe-commits]

Author: sureyeaah
Date: Wed Aug 28 12:34:17 2019
New Revision: 370249

URL: http://llvm.org/viewvc/llvm-project?rev=370249=rev
Log:
[Clangd] Initial version of ExtractFunction

Summary:
- Only works for extraction from free functions
- Basic analysis of the code being extracted.
- Extract to void function
- Bail out if extracting a return, continue or break.
- Doesn't hoist decls yet

Reviewers: kadircet, sammccall

Subscribers: mgorny, ilya-biryukov, MaskRay, jkorous, arphaman, cfe-commits

Tags: #clang

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

Added:
clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp
Modified:
clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt
clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp

Modified: clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt?rev=370249=370248=370249=diff
==
--- clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clangd/refactor/tweaks/CMakeLists.txt Wed Aug 28 
12:34:17 2019
@@ -16,6 +16,7 @@ add_clang_library(clangDaemonTweaks OBJE
   DumpAST.cpp
   ExpandAutoType.cpp
   ExpandMacro.cpp
+  ExtractFunction.cpp
   ExtractVariable.cpp
   RawStringLiteral.cpp
   SwapIfBranches.cpp

Added: clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp?rev=370249=auto
==
--- clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp (added)
+++ clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp Wed Aug 
28 12:34:17 2019
@@ -0,0 +1,605 @@
+//===--- ExtractFunction.cpp -*- 
C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Extracts statements to a new function and replaces the statements with a
+// call to the new function.
+// Before:
+//   void f(int a) {
+// [[if(a < 5)
+//   a = 5;]]
+//   }
+// After:
+//   void extracted(int ) {
+// if(a < 5)
+//   a = 5;
+//   }
+//   void f(int a) {
+// extracted(a);
+//   }
+//
+// - Only extract statements
+// - Extracts from non-templated free functions only.
+// - Parameters are const only if the declaration was const
+//   - Always passed by l-value reference
+// - Void return type
+// - Cannot extract declarations that will be needed in the original function
+//   after extraction.
+// - Doesn't check for broken control flow (break/continue without loop/switch)
+//
+// 1. ExtractFunction is the tweak subclass
+//- Prepare does basic analysis of the selection and is therefore fast.
+//  Successful prepare doesn't always mean we can apply the tweak.
+//- Apply does a more detailed analysis and can be slower. In case of
+//  failure, we let the user know that we are unable to perform extraction.
+// 2. ExtractionZone store information about the range being extracted and the
+//enclosing function.
+// 3. NewFunction stores properties of the extracted function and provides
+//methods for rendering it.
+// 4. CapturedZoneInfo uses a RecursiveASTVisitor to capture information about
+//the extraction like declarations, existing return statements, etc.
+// 5. getExtractedFunction is responsible for analyzing the CapturedZoneInfo 
and
+//creating a NewFunction.
+//===--===//
+
+#include "AST.h"
+#include "ClangdUnit.h"
+#include "Logger.h"
+#include "Selection.h"
+#include "SourceCode.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclTemplate.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "clang/Tooling/Refactoring/Extract/SourceExtraction.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/iterator_range.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using Node = SelectionTree::Node;
+
+// ExtractionZone is the part of code that is being extracted.
+// EnclosingFunction is the function/method inside which the zone lies.
+// We split the 

[PATCH] D66879: [Index] Added a ShouldSkipFunctionBody callback to libIndex, and refactored clients to use it instead of inventing their own solution

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/include/clang/Index/IndexingAction.h:60
+
+inline std::unique_ptr createIndexingASTConsumer(
+std::shared_ptr DataConsumer,

gribozavr wrote:
> jkorous wrote:
> > Why not use default argument instead of overloading?
> Writing a whole lambda in a default argument seemed to be a bit too much. I 
> can change to a default argument if you think it is better. WDYT?
Seems like we do that in couple places (`VisibleModuleSet::setVisible` or 
`FinishTemplateArgumentDeduction`). Anyway, it's no big deal, I'm fine with 
either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66879



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

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

[Eric Fiselier via Phabricator via cfe-commits]

EricWF updated this revision to Diff 217700.
EricWF added a comment.

Address review comments. I think this is good to go.


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

https://reviews.llvm.org/D66836

Files:
  include/math.h
  test/libcxx/numerics/clamp_to_integral.pass.cpp

Index: test/libcxx/numerics/clamp_to_integral.pass.cpp
===
--- /dev/null
+++ test/libcxx/numerics/clamp_to_integral.pass.cpp
@@ -0,0 +1,57 @@
+//===--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+// __clamp_to_integral(RealT)
+
+// Test the conversion function that truncates floating point types to the
+// closes representable value for the specified integer type, or
+// numeric_limits::max()/min() if the value isn't representable.
+
+#include 
+#include 
+#include 
+
+template 
+void test() {
+  typedef std::numeric_limits Lim;
+  const bool MaxIsRepresentable = sizeof(IntT) < 8;
+  const bool IsSigned = std::is_signed::value;
+  struct TestCase {
+double Input;
+IntT Expect;
+bool IsRepresentable;
+  } TestCases[] = {
+  {0, 0, true},
+  {1, 1, true},
+  {IsSigned ? static_cast(-1) : 0,
+   IsSigned ? static_cast(-1) : 0, true},
+  {Lim::lowest(), Lim::lowest(), true},
+  {static_cast(Lim::max()), Lim::max(), MaxIsRepresentable},
+  {static_cast(Lim::max()) + 1, Lim::max(), false},
+  {static_cast(Lim::max()) + 1024, Lim::max(), false},
+  {nextafter(static_cast(Lim::max()), INFINITY), Lim::max(), false},
+  };
+  for (TestCase TC : TestCases) {
+auto res = std::__clamp_to_integral(TC.Input);
+assert(res == TC.Expect);
+if (TC.IsRepresentable) {
+  auto other = static_cast(std::trunc(TC.Input));
+  assert(res == other);
+} else
+  assert(res == Lim::min() || res == Lim::max());
+  }
+}
+
+int main() {
+  test();
+  test();
+  test();
+  test();
+  test();
+  test();
+}
Index: include/math.h
===
--- include/math.h
+++ include/math.h
@@ -1553,6 +1553,48 @@
 typename std::enable_if::value, double>::type
 trunc(_A1 __lcpp_x) _NOEXCEPT {return ::trunc((double)__lcpp_x);}
 
+_LIBCPP_BEGIN_NAMESPACE_STD
+template ::digits > numeric_limits<_IntT>::digits)>
+struct __max_representable_int_for_float;
+
+template 
+struct __max_representable_int_for_float<_IntT, _FloatT, true> {
+  static_assert(is_floating_point<_FloatT>::value, "must be a floating point type");
+  static_assert(is_integral<_IntT>::value, "must be an integral type");
+  static_assert(numeric_limits<_FloatT>::radix == 2, "FloatT has incorrect radix");
+  static _LIBCPP_CONSTEXPR _IntT __value() _NOEXCEPT {
+return numeric_limits<_IntT>::max();
+  }
+};
+
+template 
+struct __max_representable_int_for_float<_IntT, _FloatT, false> {
+  static_assert(is_floating_point<_FloatT>::value, "must be a floating point type");
+  static_assert(is_integral<_IntT>::value, "must be an integral type");
+  static_assert(numeric_limits<_FloatT>::radix == 2, "FloatT has incorrect radix");
+  enum { _Bits = numeric_limits<_IntT>::digits - numeric_limits<_FloatT>::digits };
+  // This also assumes two's compliment, which is fine because that's the only
+  // representation we support.
+  static _LIBCPP_CONSTEXPR _IntT __value() _NOEXCEPT {
+return numeric_limits<_IntT>::max() >> _Bits << _Bits;
+  }
+};
+
+template 
+_LIBCPP_INLINE_VISIBILITY
+_IntT __clamp_to_integral(_RealT __r) _NOEXCEPT {
+  using _Lim = std::numeric_limits<_IntT>;
+  const _IntT _MaxVal = __max_representable_int_for_float<_IntT, _RealT>::__value();
+  if (__r >= ::nextafter(static_cast<_RealT>(_MaxVal), INFINITY)) {
+return _Lim::max();
+  } else if (__r <= _Lim::lowest()) {
+return _Lim::min();
+  }
+  return static_cast<_IntT>(__r);
+}
+_LIBCPP_END_NAMESPACE_STD
+
 } // extern "C++"
 
 #endif // __cplusplus
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

[Eric Fiselier via Phabricator via cfe-commits]

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



Comment at: include/math.h:1556
 
+_LIBCPP_BEGIN_NAMESPACE_STD
+template  Seems odd this is the only thing in this file inside the standard namespace. 
> Are we moving towards writing `std::__helper`  instead of `__libcpp_helper`? 
> It seems like the other helper functions in this file use the `__libcpp` 
> prefix and aren't in the standard namespace. 
We shouldn't put things in the global namespace generally. It's fine that this 
is the only thing in namespace `std` is this file.



Comment at: include/math.h:1558
+template ::digits > 
numeric_limits<_IntT>::digits)>
+struct __max_representable_int_for_float;

zoecarver wrote:
> Nit: maybe qualify all the uses of `numeric_limits` and similar?
No need to qualify types. The lookup will be unambiguous. 



Comment at: include/math.h:1572
+  static_assert(is_integral<_IntT>::value, "must be an integral type");
+  enum { _Bits = numeric_limits<_IntT>::digits - 
numeric_limits<_FloatT>::digits };
+  static const _IntT value = numeric_limits<_IntT>::max() >> _Bits << _Bits;

zoecarver wrote:
> What is the enum providing for you? Couldn't this just be `static const int 
> _Bits  = ...`?
It subtly enforces that the argument is a compile time constant without 
introducing a global variable declaration.



Comment at: include/math.h:1579
+_IntT __truncating_cast(_RealT __r) _NOEXCEPT {
+  using _Lim = std::numeric_limits<_IntT>;
+  const _IntT _MaxVal = __max_representable_int_for_float<_IntT, 
_RealT>::value;

zoecarver wrote:
> This will not work before C++11.
Using statements? Yes it will, but only because we require Clang in C++03 now. 
And Clang allows using statements as an extension.



Comment at: include/math.h:1582
+  const _RealT __trunc_r = __builtin_trunc(__r);
+  if (__trunc_r >= ::nextafter(static_cast<_RealT>(_MaxVal), INFINITY)) {
+return _Lim::max();

scanon wrote:
> zoecarver wrote:
> > Maybe change `INFINITY` to `std::numeric_limits< _RealT >::infinity()`
> Why isn't this just `__trunc_r > _MaxVal`?
Consider `long long` and `double`. `MaxVal - numeric_limits::max() 
== 1024`, and we want values between `MaxVal` and `::max()` to round down. So 
instead we essentially check for `__r >= numeric_limits::max() + 1`. 
This approach seems more accurate.



Comment at: include/math.h:1584
+return _Lim::max();
+  } else if (__trunc_r <= _Lim::lowest()) {
+return _Lim::min();

scanon wrote:
> This has a subtle assumption that `_IntT` is two's-complement and `_FloatT` 
> has `radix=2`, so that the implicit conversion that occurs in the comparison 
> is exact. The radix should be a static assert; does libc++ care about 
> non-two's-complement at all?
> 
> Just from a clarity perspective, I would personally make the conversion 
> explicit.
I'll static assert the radix, but I think it's safe to assume twos compliment. 
According to p0907, none of MSVC, GCC, or Clang support other representations 
[1].

How would you make this explicit?

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0907r0.html



Comment at: include/math.h:1586
+return _Lim::min();
+  }
+  return static_cast<_IntT>(__trunc_r);

scanon wrote:
> If I'm reading right, NaNs will fall through the above two comparisons and 
> invoke UB on the static_cast below. I suspect that's not the desired 
> behavior. What is the intended result for NaN?
I didn't want to treat `NaN` as a valid input, so I want to allow UBSAN to 
catch it rather than to provide a valid output.



Comment at: test/libcxx/numerics/truncating_cast.pass.cpp:10
+// __truncating_cast(RealT)
+
+// Test the conversion function that truncates floating point types to the

zoecarver wrote:
> Is this supposed to work in C++03? If so, update this test and 
> `__truncating_cast`. Otherwise, add an `#if` and a `// UNSUPPORTED: C++98, 
> C++03`
The test compiles and passes with C++03.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D66836



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

[PATCH] D65361: [analyzer] Trust global initializers when analyzing main().

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Np, post-commit review is always welcome (:


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65361



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

[PATCH] D65361: [analyzer] Trust global initializers when analyzing main().

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Sorry for not accepting this -- I actually read the code multiple times and 
didn't see anything that stood out! I didn't have the time to dig too deep, but 
if the tests are anything to go by, its gotta be alright.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65361



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

[PATCH] D66873: [Test][Time profiler] Fix test for python3

[Nathan Chancellor via Phabricator via cfe-commits]

nathanchance added a comment.

The test case passes for me after this, thanks for the fix!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66873



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

[PATCH] D66879: [Index] Added a ShouldSkipFunctionBody callback to libIndex, and refactored clients to use it instead of inventing their own solution

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang/include/clang/Index/IndexingAction.h:60
+
+inline std::unique_ptr createIndexingASTConsumer(
+std::shared_ptr DataConsumer,

jkorous wrote:
> Why not use default argument instead of overloading?
Writing a whole lambda in a default argument seemed to be a bit too much. I can 
change to a default argument if you think it is better. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66879



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

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Aside from the few remaining nits, I think this is almost ready to go.




Comment at: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp:32-33
+"llvm-prefer-register-over-unsigned");
+CheckFactories.registerCheck(
+"llvm-namespace-comment");
 CheckFactories.registerCheck("llvm-twine-local");

Spurious change that moved `llvm-namespace-comment` to be out of alphabetical 
order?



Comment at: 
clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp:60
+}
+} // end namespace llvm

dsanders wrote:
> aaron.ballman wrote:
> > I'd also like to see some tests like:
> > ```
> > void func(unsigned Reg);
> > 
> > struct S {
> >   unsigned Reg;
> > 
> >   S(unsigned Reg) : Reg(Reg) {}
> > };
> > 
> > void other_func() {
> >   func(getReg());
> >   S s{getReg()};
> >   s.Reg = getReg();
> > }
> > ```
> Added tests for the following cases that do not make changes*
> 1. Similar interface but not called Register
> 2. Register class not inside llvm namespace
> 3. Calling a function that takes an llvm::Register with a llvm::Register 
> argument
> 4. Calling a function that takes an unsigned and is given an llvm::Register 
> argument
> 5. Initializing an llvm::Register using an llvm::Register argument
> 6. Initializing any other object using a constructor parameter that's 
> unsigned and a llvm::Register argument
> 7. Assigning to a member of llvm::Register
> 8. Assigning to a unsigned member of any other object
> 
> *4, 6, and 8 should eventually but don't yet
Nice, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65919



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

[PATCH] D66404: [CFG] Make destructor calls more accurate

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370247: [CFG] Make representation of destructor calls more 
accurate. (authored by dergachev, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66404?vs=216545=217699#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66404

Files:
  cfe/trunk/lib/Analysis/CFG.cpp
  cfe/trunk/test/Analysis/auto-obj-dtors-cfg-output.cpp
  cfe/trunk/test/Analysis/cfg-rich-constructors.cpp
  cfe/trunk/test/Analysis/cfg-rich-constructors.mm
  cfe/trunk/test/Analysis/cfg.cpp
  cfe/trunk/test/Analysis/missing-bind-temporary.cpp
  cfe/trunk/test/Analysis/more-dtors-cfg-output.cpp
  cfe/trunk/test/Analysis/scopes-cfg-output.cpp
  cfe/trunk/test/Analysis/temporaries.cpp

Index: cfe/trunk/test/Analysis/cfg.cpp
===
--- cfe/trunk/test/Analysis/cfg.cpp
+++ cfe/trunk/test/Analysis/cfg.cpp
@@ -203,7 +203,7 @@
 // CHECK-LABEL: int test1(int *x)
 // CHECK: 1: 1
 // CHECK-NEXT: 2: return
-// CHECK-NEXT: ~B() (Implicit destructor)
+// CHECK-NEXT: ~NoReturnSingleSuccessor::B() (Implicit destructor)
 // CHECK-NEXT: Preds (1)
 // CHECK-NEXT: Succs (1): B0
   int test1(int *x) {
@@ -477,7 +477,7 @@
 // WARNINGS-NEXT:1:  (CXXConstructExpr, struct pr37688_deleted_union_destructor::A)
 // ANALYZER-NEXT:1:  (CXXConstructExpr, [B1.2], struct pr37688_deleted_union_destructor::A)
 // CHECK-NEXT:2: pr37688_deleted_union_destructor::A a;
-// CHECK-NEXT:3: [B1.2].~A() (Implicit destructor)
+// CHECK-NEXT:3: [B1.2].~pr37688_deleted_union_destructor::A() (Implicit destructor)
 // CHECK-NEXT:Preds (1): B2
 // CHECK-NEXT:Succs (1): B0
 // CHECK:  [B0 (EXIT)]
Index: cfe/trunk/test/Analysis/cfg-rich-constructors.cpp
===
--- cfe/trunk/test/Analysis/cfg-rich-constructors.cpp
+++ cfe/trunk/test/Analysis/cfg-rich-constructors.cpp
@@ -412,7 +412,6 @@
   ~D();
 };
 
-// FIXME: There should be no temporary destructor in C++17.
 // CHECK:  return_stmt_with_dtor::D returnTemporary()
 // CXX11-ELIDE:  1: return_stmt_with_dtor::D() (CXXConstructExpr, [B1.2], [B1.4], [B1.5], class return_stmt_with_dtor::D)
 // CXX11-NOELIDE:  1: return_stmt_with_dtor::D() (CXXConstructExpr, [B1.2], [B1.4], class return_stmt_with_dtor::D)
@@ -422,15 +421,13 @@
 // CXX11-NEXT: 5: [B1.4] (CXXConstructExpr, [B1.7], class return_stmt_with_dtor::D)
 // CXX11-NEXT: 6: ~return_stmt_with_dtor::D() (Temporary object destructor)
 // CXX11-NEXT: 7: return [B1.5];
-// CXX17:  1: return_stmt_with_dtor::D() (CXXConstructExpr, [B1.4], [B1.2], class return_stmt_w
+// CXX17:  1: return_stmt_with_dtor::D() (CXXConstructExpr, [B1.3], [B1.2], class return_stmt_w
 // CXX17-NEXT: 2: [B1.1] (BindTemporary)
-// CXX17-NEXT: 3: ~return_stmt_with_dtor::D() (Temporary object destructor)
-// CXX17-NEXT: 4: return [B1.2];
+// CXX17-NEXT: 3: return [B1.2];
 D returnTemporary() {
   return D();
 }
 
-// FIXME: There should be no temporary destructor in C++17.
 // CHECK: void returnByValueIntoVariable()
 // CHECK:  1: returnTemporary
 // CHECK-NEXT: 2: [B1.1] (ImplicitCastExpr, FunctionToPointerDecay, class return_stmt_with_dtor::D (*)(void))
@@ -442,12 +439,10 @@
 // CXX11-NEXT: 7: [B1.6] (CXXConstructExpr, [B1.8], class return_stmt_with_dtor::D)
 // CXX11-NEXT: 8: return_stmt_with_dtor::D d = returnTemporary();
 // CXX11-NEXT: 9: ~return_stmt_with_dtor::D() (Temporary object destructor)
-// CXX11-NEXT:10: [B1.8].~D() (Implicit destructor)
+// CXX11-NEXT:10: [B1.8].~return_stmt_with_dtor::D() (Implicit destructor)
 // CXX17-NEXT: 3: [B1.2]() (CXXRecordTypedCall, [B1.5], [B1.4])
 // CXX17-NEXT: 4: [B1.3] (BindTemporary)
 // CXX17-NEXT: 5: return_stmt_with_dtor::D d = returnTemporary();
-// CXX17-NEXT: 6: ~return_stmt_with_dtor::D() (Temporary object destructor)
-// CXX17-NEXT: 7: [B1.5].~D() (Implicit destructor)
 void returnByValueIntoVariable() {
   D d = returnTemporary();
 }
@@ -602,7 +597,7 @@
 // CHECK-NEXT: 3: [B1.2] (BindTemporary)
 // CHECK-NEXT: 4: [B1.3]
 // CHECK-NEXT: 5: const temporary_object_expr_with_dtors::D (0);
-// CHECK-NEXT: 6: [B1.5].~D() (Implicit destructor)
+// CHECK-NEXT: 6: [B1.5].~temporary_object_expr_with_dtors::D() (Implicit destructor)
 void referenceVariableWithConstructor() {
   const D (0);
 }
@@ -613,14 +608,14 @@
 // CHECK-NEXT: 3: [B1.2] (ImplicitCastExpr, NoOp, const class temporary_object_expr_with_dtors::D)
 // CHECK-NEXT: 4: [B1.3]
 // CHECK-NEXT: 5: const temporary_object_expr_with_dtors::D  = temporary_object_expr_with_dtors::D();
-// CHECK-NEXT: 6: [B1.5].~D() (Implicit destructor)
+// CHECK-NEXT: 6: 

[PATCH] D66847: [analyzer] Fix analyzer warnings.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370246: [analyzer] Fix analyzer warnings on analyzer. 
(authored by dergachev, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66847?vs=217537=217698#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66847

Files:
  cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  cfe/trunk/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/MallocSizeofChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
  
cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
  cfe/trunk/test/Analysis/cstring-syntax-weird.c
  cfe/trunk/test/Analysis/cstring-syntax-weird2.c
  cfe/trunk/test/Analysis/cstring-syntax.c

Index: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -803,9 +803,8 @@
   if (CNE->isArray()) {
 // FIXME: allocating an array requires simulating the constructors.
 // For now, just return a symbolicated region.
-if (const SubRegion *NewReg =
-dyn_cast_or_null(symVal.getAsRegion())) {
-  QualType ObjTy = CNE->getType()->getAs()->getPointeeType();
+if (const auto *NewReg = cast_or_null(symVal.getAsRegion())) {
+  QualType ObjTy = CNE->getType()->getPointeeType();
   const ElementRegion *EleReg =
   getStoreManager().GetElementZeroRegion(NewReg, ObjTy);
   Result = loc::MemRegionVal(EleReg);
Index: cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -1075,7 +1075,7 @@
  const SubRegion *Super,
  bool IsVirtual) {
   if (isa(Super)) {
-assert(isValidBaseClass(RD, dyn_cast(Super), IsVirtual));
+assert(isValidBaseClass(RD, cast(Super), IsVirtual));
 (void)
 
 if (IsVirtual) {
@@ -1426,6 +1426,7 @@
 case MemRegion::FieldRegionKind: {
   const auto *FR = cast(R);
   R = FR->getSuperRegion();
+  assert(R);
 
   const RecordDecl *RD = FR->getDecl()->getParent();
   if (RD->isUnion() || !RD->isCompleteDefinition()) {
Index: cfe/trunk/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -91,7 +91,7 @@
   } else if (auto PD = dyn_cast_or_null(S)) {
 // Initialization
 assert(PD->isSingleDecl() && "We process decls one by one");
-VD = dyn_cast_or_null(PD->getSingleDecl());
+VD = cast(PD->getSingleDecl());
 RHS = VD->getAnyInitializer();
   }
 
Index: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -850,8 +850,7 @@
   if (OOE->EvaluateAsInt(Result, getContext())) {
 APSInt IV = Result.Val.getInt();
 assert(IV.getBitWidth() == getContext().getTypeSize(OOE->getType()));
-assert(OOE->getType()->isBuiltinType());
-assert(OOE->getType()->getAs()->isInteger());
+assert(OOE->getType()->castAs()->isInteger());
 assert(IV.isSigned() == OOE->getType()->isSignedIntegerType());
 SVal X = svalBuilder.makeIntVal(IV);
 B.generateNode(OOE, Pred,
Index: cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2291,8 +2291,7 @@
  const TypedValueRegion* R,

[PATCH] D65361: [analyzer] Trust global initializers when analyzing main().

[Phabricator via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL370244: [analyzer] Trust global initializers when analyzing 
main(). (authored by dergachev, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65361?vs=215274=217696#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65361

Files:
  cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
  cfe/trunk/test/Analysis/main.c
  cfe/trunk/test/Analysis/main.cpp

Index: cfe/trunk/test/Analysis/main.c
===
--- cfe/trunk/test/Analysis/main.c
+++ cfe/trunk/test/Analysis/main.c
@@ -0,0 +1,32 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+int x = 1;
+
+struct {
+  int a, b;
+} s = {2, 3};
+
+int arr[] = {4, 5, 6};
+
+void clang_analyzer_eval(int);
+
+int main() {
+  // In main() we know that the initial values are still valid.
+  clang_analyzer_eval(x == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(s.a == 2); // expected-warning{{TRUE}}
+  clang_analyzer_eval(s.b == 3); // expected-warning{{TRUE}}
+  clang_analyzer_eval(arr[0] == 4); // expected-warning{{TRUE}}
+  clang_analyzer_eval(arr[1] == 5); // expected-warning{{TRUE}}
+  clang_analyzer_eval(arr[2] == 6); // expected-warning{{TRUE}}
+  return 0;
+}
+
+void foo() {
+  // In other functions these values may already be overwritten.
+  clang_analyzer_eval(x == 1); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(s.a == 2); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(s.b == 3); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(arr[0] == 4); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(arr[1] == 5); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(arr[2] == 6); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+}
Index: cfe/trunk/test/Analysis/main.cpp
===
--- cfe/trunk/test/Analysis/main.cpp
+++ cfe/trunk/test/Analysis/main.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+int x = 1;
+
+struct {
+  int a, b;
+} s = {2, 3};
+
+int arr[] = {4, 5, 6};
+
+void clang_analyzer_eval(int);
+
+int main() {
+  // Do not trust global initializers in C++.
+  clang_analyzer_eval(x == 1); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(s.a == 2); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(s.b == 3); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(arr[0] == 4); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(arr[1] == 5); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  clang_analyzer_eval(arr[2] == 6); // expected-warning{{TRUE}} // expected-warning{{FALSE}}
+  return 0;
+}
Index: cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -155,28 +155,42 @@
  ClusterBindings> {
   ClusterBindings::Factory *CBFactory;
 
+  // This flag indicates whether the current bindings are within the analysis
+  // that has started from main(). It affects how we perform loads from
+  // global variables that have initializers: if we have observed the
+  // program execution from the start and we know that these variables
+  // have not been overwritten yet, we can be sure that their initializers
+  // are still relevant. This flag never gets changed when the bindings are
+  // updated, so it could potentially be moved into RegionStoreManager
+  // (as if it's the same bindings but a different loading procedure)
+  // however that would have made the manager needlessly stateful.
+  bool IsMainAnalysis;
+
 public:
   typedef llvm::ImmutableMapRef
   ParentTy;
 
   RegionBindingsRef(ClusterBindings::Factory ,
 const RegionBindings::TreeTy *T,
-RegionBindings::TreeTy::Factory *F)
+RegionBindings::TreeTy::Factory *F,
+bool IsMainAnalysis)
   : llvm::ImmutableMapRef(T, F),
-CBFactory() {}
+CBFactory(), IsMainAnalysis(IsMainAnalysis) {}
 
-  RegionBindingsRef(const ParentTy , ClusterBindings::Factory )
+  RegionBindingsRef(const ParentTy ,
+ClusterBindings::Factory ,
+bool IsMainAnalysis)
   : llvm::ImmutableMapRef(P),
-CBFactory() {}
+CBFactory(), 

[PATCH] D66565: [analyzer] pr43036: Fix support for operator `sizeof...'.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370245: [analyzer] pr43036: Fix support for operator 
sizeof (authored by dergachev, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66565?vs=216517=217697#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66565

Files:
  cfe/trunk/lib/StaticAnalyzer/Core/Environment.cpp
  cfe/trunk/test/Analysis/sizeofpack.cpp


Index: cfe/trunk/test/Analysis/sizeofpack.cpp
===
--- cfe/trunk/test/Analysis/sizeofpack.cpp
+++ cfe/trunk/test/Analysis/sizeofpack.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection \
+// RUN:-verify %s
+
+typedef __typeof(sizeof(int)) size_t;
+
+void clang_analyzer_eval(bool);
+
+template  size_t foo() {
+  return sizeof...(N);
+}
+
+void bar() {
+  clang_analyzer_eval(foo<>() == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(foo<1, 2, 3>() == 3); // expected-warning{{TRUE}}
+}
Index: cfe/trunk/lib/StaticAnalyzer/Core/Environment.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/Environment.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/Environment.cpp
@@ -108,6 +108,7 @@
   case Stmt::ObjCStringLiteralClass:
   case Stmt::StringLiteralClass:
   case Stmt::TypeTraitExprClass:
+  case Stmt::SizeOfPackExprClass:
 // Known constants; defer to SValBuilder.
 return svalBuilder.getConstantVal(cast(S)).getValue();
 


Index: cfe/trunk/test/Analysis/sizeofpack.cpp
===
--- cfe/trunk/test/Analysis/sizeofpack.cpp
+++ cfe/trunk/test/Analysis/sizeofpack.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection \
+// RUN:-verify %s
+
+typedef __typeof(sizeof(int)) size_t;
+
+void clang_analyzer_eval(bool);
+
+template  size_t foo() {
+  return sizeof...(N);
+}
+
+void bar() {
+  clang_analyzer_eval(foo<>() == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(foo<1, 2, 3>() == 3); // expected-warning{{TRUE}}
+}
Index: cfe/trunk/lib/StaticAnalyzer/Core/Environment.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/Environment.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/Environment.cpp
@@ -108,6 +108,7 @@
   case Stmt::ObjCStringLiteralClass:
   case Stmt::StringLiteralClass:
   case Stmt::TypeTraitExprClass:
+  case Stmt::SizeOfPackExprClass:
 // Known constants; defer to SValBuilder.
 return svalBuilder.getConstantVal(cast(S)).getValue();
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52524: Add -Wpoison-system-directories warning

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1072
+// cross-compiling.
+def PoisonSystemDirectories : DiagGroup<"poison-system-directories">;
+

Do you envision more warnings being added to this group? If not, I would 
recommend dropping this change and instead using 
`InGroup>` in the diagnostic.



Comment at: clang/lib/Frontend/InitHeaderSearch.cpp:145
+  Headers.getDiags().Report(diag::warn_poison_system_directories)
+  << MappedPathStr.str();
+}

I don't think you need the `.str()` here.


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

https://reviews.llvm.org/D52524



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

r370247 - [CFG] Make representation of destructor calls more accurate.

[Artem Dergachev via cfe-commits]

Author: dergachev
Date: Wed Aug 28 11:44:42 2019
New Revision: 370247

URL: http://llvm.org/viewvc/llvm-project?rev=370247=rev
Log:
[CFG] Make representation of destructor calls more accurate.

Respect C++17 copy elision; previously it would generate destructor calls
for elided temporaries, including in initialization and return statements.

Don't generate duplicate destructor calls for statement expressions.

Fix destructors in initialization lists and comma operators.

Improve printing of implicit destructors.

Patch by Nicholas Allegra!

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

Added:
cfe/trunk/test/Analysis/more-dtors-cfg-output.cpp
Modified:
cfe/trunk/lib/Analysis/CFG.cpp
cfe/trunk/test/Analysis/auto-obj-dtors-cfg-output.cpp
cfe/trunk/test/Analysis/cfg-rich-constructors.cpp
cfe/trunk/test/Analysis/cfg-rich-constructors.mm
cfe/trunk/test/Analysis/cfg.cpp
cfe/trunk/test/Analysis/missing-bind-temporary.cpp
cfe/trunk/test/Analysis/scopes-cfg-output.cpp
cfe/trunk/test/Analysis/temporaries.cpp

Modified: cfe/trunk/lib/Analysis/CFG.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CFG.cpp?rev=370247=370246=370247=diff
==
--- cfe/trunk/lib/Analysis/CFG.cpp (original)
+++ cfe/trunk/lib/Analysis/CFG.cpp Wed Aug 28 11:44:42 2019
@@ -525,7 +525,7 @@ private:
   CFGBlock *VisitCallExpr(CallExpr *C, AddStmtChoice asc);
   CFGBlock *VisitCaseStmt(CaseStmt *C);
   CFGBlock *VisitChooseExpr(ChooseExpr *C, AddStmtChoice asc);
-  CFGBlock *VisitCompoundStmt(CompoundStmt *C);
+  CFGBlock *VisitCompoundStmt(CompoundStmt *C, bool ExternallyDestructed);
   CFGBlock *VisitConditionalOperator(AbstractConditionalOperator *C,
  AddStmtChoice asc);
   CFGBlock *VisitContinueStmt(ContinueStmt *C);
@@ -546,7 +546,8 @@ private:
   CFGBlock *VisitDeclSubExpr(DeclStmt *DS);
   CFGBlock *VisitDefaultStmt(DefaultStmt *D);
   CFGBlock *VisitDoStmt(DoStmt *D);
-  CFGBlock *VisitExprWithCleanups(ExprWithCleanups *E, AddStmtChoice asc);
+  CFGBlock *VisitExprWithCleanups(ExprWithCleanups *E,
+  AddStmtChoice asc, bool 
ExternallyDestructed);
   CFGBlock *VisitForStmt(ForStmt *F);
   CFGBlock *VisitGotoStmt(GotoStmt *G);
   CFGBlock *VisitGCCAsmStmt(GCCAsmStmt *G, AddStmtChoice asc);
@@ -585,7 +586,8 @@ private:
   CFGBlock *VisitUnaryOperator(UnaryOperator *U, AddStmtChoice asc);
   CFGBlock *VisitWhileStmt(WhileStmt *W);
 
-  CFGBlock *Visit(Stmt *S, AddStmtChoice asc = AddStmtChoice::NotAlwaysAdd);
+  CFGBlock *Visit(Stmt *S, AddStmtChoice asc = AddStmtChoice::NotAlwaysAdd,
+  bool ExternallyDestructed = false);
   CFGBlock *VisitStmt(Stmt *S, AddStmtChoice asc);
   CFGBlock *VisitChildren(Stmt *S);
   CFGBlock *VisitNoRecurse(Expr *E, AddStmtChoice asc);
@@ -656,15 +658,17 @@ private:
 
   // Visitors to walk an AST and generate destructors of temporaries in
   // full expression.
-  CFGBlock *VisitForTemporaryDtors(Stmt *E, bool BindToTemporary,
+  CFGBlock *VisitForTemporaryDtors(Stmt *E, bool ExternallyDestructed,
TempDtorContext );
-  CFGBlock *VisitChildrenForTemporaryDtors(Stmt *E, TempDtorContext );
+  CFGBlock *VisitChildrenForTemporaryDtors(Stmt *E,  bool ExternallyDestructed,
+   TempDtorContext );
   CFGBlock *VisitBinaryOperatorForTemporaryDtors(BinaryOperator *E,
+ bool ExternallyDestructed,
  TempDtorContext );
   CFGBlock *VisitCXXBindTemporaryExprForTemporaryDtors(
-  CXXBindTemporaryExpr *E, bool BindToTemporary, TempDtorContext );
+  CXXBindTemporaryExpr *E, bool ExternallyDestructed, TempDtorContext 
);
   CFGBlock *VisitConditionalOperatorForTemporaryDtors(
-  AbstractConditionalOperator *E, bool BindToTemporary,
+  AbstractConditionalOperator *E, bool ExternallyDestructed,
   TempDtorContext );
   void InsertTempDtorDecisionBlock(const TempDtorContext ,
CFGBlock *FalseSucc = nullptr);
@@ -1575,7 +1579,7 @@ CFGBlock *CFGBuilder::addInitializer(CXX
   // Generate destructors for temporaries in initialization expression.
   TempDtorContext Context;
   VisitForTemporaryDtors(cast(Init)->getSubExpr(),
- /*BindToTemporary=*/false, Context);
+ /*ExternallyDestructed=*/false, Context);
 }
   }
 
@@ -2051,7 +2055,8 @@ CFGBuilder::prependAutomaticObjScopeEndW
 /// Visit - Walk the subtree of a statement and add extra
 ///   blocks for ternary operators, &&, and ||.  We also process "," and
 ///   DeclStmts (which may contain nested control-flow).
-CFGBlock *CFGBuilder::Visit(Stmt * S, AddStmtChoice asc) {
+CFGBlock *CFGBuilder::Visit(Stmt * S, AddStmtChoice asc,
+bool 

r370244 - [analyzer] Trust global initializers when analyzing main().

[Artem Dergachev via cfe-commits]

Author: dergachev
Date: Wed Aug 28 11:44:32 2019
New Revision: 370244

URL: http://llvm.org/viewvc/llvm-project?rev=370244=rev
Log:
[analyzer] Trust global initializers when analyzing main().

If the global variable has an initializer, we'll ignore it because we're usually
not analyzing the program from the beginning, which means that the global
variable may have changed before we start our analysis.

However when we're analyzing main() as the top-level function, we can rely
on global initializers to still be valid. At least in C; in C++ we have global
constructors that can still break this logic.

This patch allows the Static Analyzer to load constant initializers from
global variables if the top-level function of the current analysis is main().

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

Added:
cfe/trunk/test/Analysis/main.c
cfe/trunk/test/Analysis/main.cpp
Modified:
cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp?rev=370244=370243=370244=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp Wed Aug 28 11:44:32 2019
@@ -155,28 +155,42 @@ class RegionBindingsRef : public llvm::I
  ClusterBindings> {
   ClusterBindings::Factory *CBFactory;
 
+  // This flag indicates whether the current bindings are within the analysis
+  // that has started from main(). It affects how we perform loads from
+  // global variables that have initializers: if we have observed the
+  // program execution from the start and we know that these variables
+  // have not been overwritten yet, we can be sure that their initializers
+  // are still relevant. This flag never gets changed when the bindings are
+  // updated, so it could potentially be moved into RegionStoreManager
+  // (as if it's the same bindings but a different loading procedure)
+  // however that would have made the manager needlessly stateful.
+  bool IsMainAnalysis;
+
 public:
   typedef llvm::ImmutableMapRef
   ParentTy;
 
   RegionBindingsRef(ClusterBindings::Factory ,
 const RegionBindings::TreeTy *T,
-RegionBindings::TreeTy::Factory *F)
+RegionBindings::TreeTy::Factory *F,
+bool IsMainAnalysis)
   : llvm::ImmutableMapRef(T, F),
-CBFactory() {}
+CBFactory(), IsMainAnalysis(IsMainAnalysis) {}
 
-  RegionBindingsRef(const ParentTy , ClusterBindings::Factory )
+  RegionBindingsRef(const ParentTy ,
+ClusterBindings::Factory ,
+bool IsMainAnalysis)
   : llvm::ImmutableMapRef(P),
-CBFactory() {}
+CBFactory(), IsMainAnalysis(IsMainAnalysis) {}
 
   RegionBindingsRef add(key_type_ref K, data_type_ref D) const {
 return RegionBindingsRef(static_cast(this)->add(K, D),
- *CBFactory);
+ *CBFactory, IsMainAnalysis);
   }
 
   RegionBindingsRef remove(key_type_ref K) const {
 return RegionBindingsRef(static_cast(this)->remove(K),
- *CBFactory);
+ *CBFactory, IsMainAnalysis);
   }
 
   RegionBindingsRef addBinding(BindingKey K, SVal V) const;
@@ -206,7 +220,13 @@ public:
 
   /// Return the internal tree as a Store.
   Store asStore() const {
-return asImmutableMap().getRootWithoutRetain();
+llvm::PointerIntPair Ptr = {
+asImmutableMap().getRootWithoutRetain(), IsMainAnalysis};
+return reinterpret_cast(Ptr.getOpaqueValue());
+  }
+
+  bool isMainAnalysis() const {
+return IsMainAnalysis;
   }
 
   void printJson(raw_ostream , const char *NL = "\n",
@@ -381,8 +401,15 @@ public:
   ///  casts from arrays to pointers.
   SVal ArrayToPointer(Loc Array, QualType ElementTy) override;
 
+  /// Creates the Store that correctly represents memory contents before
+  /// the beginning of the analysis of the given top-level stack frame.
   StoreRef getInitialStore(const LocationContext *InitLoc) override {
-return StoreRef(RBFactory.getEmptyMap().getRootWithoutRetain(), *this);
+bool IsMainAnalysis = false;
+if (const auto *FD = dyn_cast(InitLoc->getDecl()))
+  IsMainAnalysis = FD->isMain() && !Ctx.getLangOpts().CPlusPlus;
+return StoreRef(RegionBindingsRef(
+RegionBindingsRef::ParentTy(RBFactory.getEmptyMap(), RBFactory),
+CBFactory, IsMainAnalysis).asStore(), *this);
   }
 
   //===---===//
@@ -608,9 +635,13 @@ public: // Part of public interface to c
   //===--===//
 
   RegionBindingsRef getRegionBindings(Store store) const {
-return 

r370245 - [analyzer] pr43036: Fix support for operator 'sizeof...'.

[Artem Dergachev via cfe-commits]

Author: dergachev
Date: Wed Aug 28 11:44:35 2019
New Revision: 370245

URL: http://llvm.org/viewvc/llvm-project?rev=370245=rev
Log:
[analyzer] pr43036: Fix support for operator 'sizeof...'.

It was known to be a compile-time constant so it wasn't evaluated during
symbolic execution, but it wasn't evaluated as a compile-time constant either.

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

Added:
cfe/trunk/test/Analysis/sizeofpack.cpp
Modified:
cfe/trunk/lib/StaticAnalyzer/Core/Environment.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Core/Environment.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/Environment.cpp?rev=370245=370244=370245=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/Environment.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/Environment.cpp Wed Aug 28 11:44:35 2019
@@ -108,6 +108,7 @@ SVal Environment::getSVal(const Environm
   case Stmt::ObjCStringLiteralClass:
   case Stmt::StringLiteralClass:
   case Stmt::TypeTraitExprClass:
+  case Stmt::SizeOfPackExprClass:
 // Known constants; defer to SValBuilder.
 return svalBuilder.getConstantVal(cast(S)).getValue();
 

Added: cfe/trunk/test/Analysis/sizeofpack.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/sizeofpack.cpp?rev=370245=auto
==
--- cfe/trunk/test/Analysis/sizeofpack.cpp (added)
+++ cfe/trunk/test/Analysis/sizeofpack.cpp Wed Aug 28 11:44:35 2019
@@ -0,0 +1,15 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection \
+// RUN:-verify %s
+
+typedef __typeof(sizeof(int)) size_t;
+
+void clang_analyzer_eval(bool);
+
+template  size_t foo() {
+  return sizeof...(N);
+}
+
+void bar() {
+  clang_analyzer_eval(foo<>() == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(foo<1, 2, 3>() == 3); // expected-warning{{TRUE}}
+}


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

r370246 - [analyzer] Fix analyzer warnings on analyzer.

[Artem Dergachev via cfe-commits]

Author: dergachev
Date: Wed Aug 28 11:44:38 2019
New Revision: 370246

URL: http://llvm.org/viewvc/llvm-project?rev=370246=rev
Log:
[analyzer] Fix analyzer warnings on analyzer.

Write tests for the actual crash that was found. Write comments and refactor
code around 17 style bugs and suppress 3 false positives.

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

Added:
cfe/trunk/test/Analysis/cstring-syntax-weird.c
cfe/trunk/test/Analysis/cstring-syntax-weird2.c
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
cfe/trunk/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocSizeofChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp

cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp
cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
cfe/trunk/test/Analysis/cstring-syntax.c

Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h?rev=370246=370245=370246=diff
==
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h 
(original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h Wed 
Aug 28 11:44:38 2019
@@ -169,6 +169,7 @@ public:
   Kind getKind() const { return kind; }
 
   template const RegionTy* getAs() const;
+  template const RegionTy* castAs() const;
 
   virtual bool isBoundable() const { return false; }
 
@@ -1231,6 +1232,11 @@ const RegionTy* MemRegion::getAs() const
   return nullptr;
 }
 
+template
+const RegionTy* MemRegion::castAs() const {
+  return cast(this);
+}
+
 
//===--===//
 // MemRegionManager - Factory object for creating regions.
 
//===--===//

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp?rev=370246=370245=370246=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp Wed Aug 28 
11:44:38 2019
@@ -156,14 +156,21 @@ bool WalkAST::containsBadStrlcpyStrlcatP
   const Expr *DstArg = CE->getArg(0);
   const Expr *LenArg = CE->getArg(2);
 
-  const auto *DstArgDecl = 
dyn_cast(DstArg->IgnoreParenImpCasts());
-  const auto *LenArgDecl = 
dyn_cast(LenArg->IgnoreParenLValueCasts());
+  const auto *DstArgDRE = dyn_cast(DstArg->IgnoreParenImpCasts());
+  const auto *LenArgDRE =
+  dyn_cast(LenArg->IgnoreParenLValueCasts());
   uint64_t DstOff = 0;
   if (isSizeof(LenArg, DstArg))
 return false;
+
   // - size_t dstlen = sizeof(dst)
-  if (LenArgDecl) {
-const auto *LenArgVal = dyn_cast(LenArgDecl->getDecl());
+  if (LenArgDRE) {
+const auto *LenArgVal = dyn_cast(LenArgDRE->getDecl());
+// If it's an EnumConstantDecl instead, then we're missing out on 
something.
+if (!LenArgVal) {
+  assert(isa(LenArgDRE->getDecl()));
+  return false;
+}
 if (LenArgVal->getInit())
   LenArg = LenArgVal->getInit();
   }
@@ -177,9 +184,10 @@ bool WalkAST::containsBadStrlcpyStrlcatP
 // Case when there is pointer arithmetic on the destination buffer
 // especially when we offset from the base decreasing the
 // buffer length accordingly.
-if (!DstArgDecl) {
-  if (const auto *BE = 
dyn_cast(DstArg->IgnoreParenImpCasts())) {
-DstArgDecl = 
dyn_cast(BE->getLHS()->IgnoreParenImpCasts());
+if (!DstArgDRE) {
+  if (const auto *BE =
+  dyn_cast(DstArg->IgnoreParenImpCasts())) {
+DstArgDRE = dyn_cast(BE->getLHS()->IgnoreParenImpCasts());
 if (BE->getOpcode() == BO_Add) {
   if ((IL = 
dyn_cast(BE->getRHS()->IgnoreParenImpCasts( {
 DstOff = IL->getValue().getZExtValue();
@@ -187,8 +195,9 @@ bool 

[PATCH] D66397: [Diagnostics] Improve -Wxor-used-as-pow

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D66397#1649353 , @xbolva00 wrote:

> In D66397#1647455 , @aaron.ballman 
> wrote:
>
> > Adding @rsmith to see if we can make forward progress on this patch again.
>
>
> On the other side, I don't want to waste Richard's time since I dont want to 
> extend (variables and macros are controversal topic anyway) it more now. I 
> promised to @jfb to handle (2 ^ 64) - 1 as follow up patch and the promised 
> patch is here..


It looks like your patch changed the behavior involving macros though, so this 
isn't just about handling (2 ^ 64) - 1. It's hard to tell due to the way the 
patch is formatted though. tbh, I would appreciate if you would leave the 
definition of `diagnoseXorMisusedAsPow()` where it is and add a forward declare 
of the function earlier in the file. It would make spotting the differences in 
the function much easier.


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

https://reviews.llvm.org/D66397



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

[PATCH] D63134: [clang] improving diagnotics for invalid constexpr defaulted special membres

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 217694.
Tyker added a comment.

Rebased on current master

@rsmith Ping


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

https://reviews.llvm.org/D63134

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclCXX.cpp

Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -6483,24 +6483,29 @@
 
 /// Is the special member function which would be selected to perform the
 /// specified operation on the specified class type a constexpr constructor?
-static bool
-specialMemberIsConstexpr(Sema , CXXRecordDecl *ClassDecl,
- Sema::CXXSpecialMember CSM, unsigned Quals,
- bool ConstRHS,
- CXXConstructorDecl *InheritedCtor = nullptr,
- Sema::InheritedConstructorInfo *Inherited = nullptr) {
+static bool specialMemberIsConstexpr(
+Sema , CXXRecordDecl *ClassDecl, Sema::CXXSpecialMember CSM,
+unsigned Quals, bool ConstRHS, CXXConstructorDecl *InheritedCtor = nullptr,
+Sema::InheritedConstructorInfo *Inherited = nullptr,
+SourceLocation *SMLoc = nullptr) {
   // If we're inheriting a constructor, see if we need to call it for this base
   // class.
   if (InheritedCtor) {
 assert(CSM == Sema::CXXDefaultConstructor);
 auto BaseCtor =
 Inherited->findConstructorForBase(ClassDecl, InheritedCtor).first;
-if (BaseCtor)
+if (BaseCtor) {
+  if (SMLoc && !BaseCtor->isImplicit())
+*SMLoc = BaseCtor->getLocation();
   return BaseCtor->isConstexpr();
+}
   }
 
-  if (CSM == Sema::CXXDefaultConstructor)
-return ClassDecl->hasConstexprDefaultConstructor();
+  if (CSM == Sema::CXXDefaultConstructor) {
+bool IsConstexprCtor = ClassDecl->hasConstexprDefaultConstructor();
+if (IsConstexprCtor || !SMLoc)
+  return IsConstexprCtor;
+  }
 
   Sema::SpecialMemberOverloadResult SMOR =
   lookupCallFromSpecialMember(S, ClassDecl, CSM, Quals, ConstRHS);
@@ -6508,21 +6513,96 @@
 // A constructor we wouldn't select can't be "involved in initializing"
 // anything.
 return true;
+  if (SMLoc && !SMOR.getMethod()->isImplicit())
+*SMLoc = SMOR.getMethod()->getLocation();
   return SMOR.getMethod()->isConstexpr();
 }
 
 /// Determine whether the specified special member function would be constexpr
 /// if it were implicitly defined.
 static bool defaultedSpecialMemberIsConstexpr(
-Sema , CXXRecordDecl *ClassDecl, Sema::CXXSpecialMember CSM,
+Sema , const CXXRecordDecl *ClassDecl, Sema::CXXSpecialMember CSM,
 bool ConstArg, CXXConstructorDecl *InheritedCtor = nullptr,
-Sema::InheritedConstructorInfo *Inherited = nullptr) {
+Sema::InheritedConstructorInfo *Inherited = nullptr,
+SmallVectorImpl *DiagNotes = nullptr) {
   if (!S.getLangOpts().CPlusPlus11)
 return false;
 
+  SourceLocation SMLoc;
+  SourceLocation DiagLoc;
+  enum class FailKind {
+Subobject,
+Union,
+VirtualBase,
+NonLiteral,
+  };
+  /// This lambda will gathering all information and add notes for diagnostics.
+  auto Fail = [&](FailKind FailureKind, const Decl *FaillingDecl = nullptr) {
+if (DiagNotes) {
+  switch (FailureKind) {
+  case FailKind::Subobject: {
+PartialDiagnostic PDiag(diag::note_constexpr_defaulted_special_member,
+S.Context.getDiagAllocator());
+auto *Field = dyn_cast(FaillingDecl);
+auto *Record = dyn_cast(FaillingDecl);
+SourceLocation Loc = FaillingDecl->getLocation();
+if (!Record)
+  Record = Field->getType()->getAsCXXRecordDecl();
+if (DiagLoc.isValid())
+  Loc = DiagLoc;
+PDiag << CSM << isa(FaillingDecl);
+if (Field)
+  PDiag << Field;
+else
+  PDiag << Record;
+PDiag << ClassDecl << SMLoc.isInvalid()
+  << (Field ? Field->getDeclName().isEmpty() : false);
+DiagNotes->emplace_back(Loc, PDiag);
+if (SMLoc.isValid()) {
+  DiagNotes->emplace_back(
+  SMLoc, PartialDiagnostic(diag::note_special_member_declared_here,
+   S.Context.getDiagAllocator())
+ << CSM);
+  return false;
+}
+if (!Record)
+  return false;
+defaultedSpecialMemberIsConstexpr(S, Record, CSM, ConstArg, nullptr,
+  nullptr, DiagNotes);
+return false;
+  }
+  case FailKind::VirtualBase: {
+PartialDiagnostic PDiag(diag::note_constexpr_defaulted_virtual_base,
+S.Context.getDiagAllocator());
+const CXXBaseSpecifier  = *ClassDecl->bases_begin();
+PDiag << ClassDecl << Base.getType();
+DiagNotes->emplace_back(Base.getBeginLoc(), PDiag);
+

[PATCH] D66715: [CFG] Add dumps for CFGElement and CFGElementRef

[Artem Dergachev via Phabricator via cfe-commits]

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

Thanks!!

In D66715#1644395 , @Szelethus wrote:

> Mind that I'll probably commit without the unit test, last time I learned the 
> hard way that the AST's lifetime ends by the time we retrieve the CFG.


Ouch. We'll need to fix that eventually because the CFG is, by design, filled 
with AST pointers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66715



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

[PATCH] D66460: [analyzer] Remove BugReporter.BugTypes.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ closed this revision.
NoQ added a comment.

Committed as rC369451  but forgot to add a 
phabricator link.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66460



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

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.

Excellent detective work! Thanks! I'll do the honors.


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

https://reviews.llvm.org/D66733



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

[PATCH] D66879: [Index] Added a ShouldSkipFunctionBody callback to libIndex, and refactored clients to use it instead of inventing their own solution

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Otherwise LGTM. Thanks for refactoring this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66879



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

[PATCH] D65130: [clang][OpenMP] Add clang-offload-wrapper tool

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev updated this revision to Diff 217682.
sdmitriev added a comment.

Rebased and synced up changes with https://reviews.llvm.org/D64943


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

https://reviews.llvm.org/D65130

Files:
  clang/test/Driver/clang-offload-wrapper.c
  clang/tools/CMakeLists.txt
  clang/tools/clang-offload-wrapper/CMakeLists.txt
  clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp

Index: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
===
--- /dev/null
+++ clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
@@ -0,0 +1,360 @@
+//===-- clang-offload-wrapper/ClangOffloadWrapper.cpp ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+///
+/// \file
+/// Implememtation of the offload wrapper tool. It takes offload target binaries
+/// as input and creates wrapper bitcode from them which registers given
+/// binaries in offload runtime.
+///
+//===--===//
+
+#include "clang/Basic/Version.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Bitcode/BitcodeWriter.h"
+#include "llvm/IR/Constants.h"
+#include "llvm/IR/GlobalVariable.h"
+#include "llvm/IR/IRBuilder.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Signals.h"
+#include "llvm/Support/ToolOutputFile.h"
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
+#include "llvm/Transforms/Utils/ModuleUtils.h"
+#include 
+#include 
+
+using namespace llvm;
+
+static cl::opt Help("h", cl::desc("Alias for -help"), cl::Hidden);
+
+// Mark all our options with this category, everything else (except for -version
+// and -help) will be hidden.
+static cl::OptionCategory
+ClangOffloadWrapperCategory("clang-offload-wrapper options");
+
+static cl::opt Output("o", cl::Required,
+   cl::desc("Output filename"),
+   cl::value_desc("filename"),
+   cl::cat(ClangOffloadWrapperCategory));
+
+static cl::list Inputs(cl::Positional, cl::OneOrMore,
+cl::desc(""),
+cl::cat(ClangOffloadWrapperCategory));
+
+static cl::opt Target("target", cl::Required,
+   cl::desc("Target triple"),
+   cl::value_desc("triple"),
+   cl::cat(ClangOffloadWrapperCategory));
+
+namespace {
+
+class BinaryWrapper {
+  LLVMContext C;
+  Module M;
+
+  StructType *EntryTy = nullptr;
+  StructType *ImageTy = nullptr;
+  StructType *DescTy = nullptr;
+
+private:
+  IntegerType *getSizeTTy() {
+switch (M.getDataLayout().getPointerTypeSize(Type::getInt8PtrTy(C))) {
+case 4u:
+  return Type::getInt32Ty(C);
+case 8u:
+  return Type::getInt64Ty(C);
+}
+llvm_unreachable("unsupported pointer type size");
+  }
+
+  // struct __tgt_offload_entry {
+  //   void *addr;
+  //   char *name;
+  //   size_t size;
+  //   int32_t flags;
+  //   int32_t reserved;
+  // };
+  StructType *getEntryTy() {
+if (!EntryTy)
+  EntryTy = StructType::create("__tgt_offload_entry", Type::getInt8PtrTy(C),
+   Type::getInt8PtrTy(C), getSizeTTy(),
+   Type::getInt32Ty(C), Type::getInt32Ty(C));
+return EntryTy;
+  }
+
+  PointerType *getEntryPtrTy() { return PointerType::getUnqual(getEntryTy()); }
+
+  // struct __tgt_device_image {
+  //   void *ImageStart;
+  //   void *ImageEnd;
+  //   __tgt_offload_entry *EntriesBegin;
+  //   __tgt_offload_entry *EntriesEnd;
+  // };
+  StructType *getDeviceImageTy() {
+if (!ImageTy)
+  ImageTy = StructType::create("__tgt_device_image", Type::getInt8PtrTy(C),
+   Type::getInt8PtrTy(C), getEntryPtrTy(),
+   getEntryPtrTy());
+return ImageTy;
+  }
+
+  PointerType *getDeviceImagePtrTy() {
+return PointerType::getUnqual(getDeviceImageTy());
+  }
+
+  // struct __tgt_bin_desc {
+  //   int32_t NumDeviceImages;
+  //   __tgt_device_image *DeviceImages;
+  //   __tgt_offload_entry *HostEntriesBegin;
+  //   __tgt_offload_entry *HostEntriesEnd;
+  // };
+  StructType *getBinDescTy() {
+if (!DescTy)
+  DescTy = StructType::create("__tgt_bin_desc", Type::getInt32Ty(C),
+  getDeviceImagePtrTy(), 

[PATCH] D66879: [Index] Added a ShouldSkipFunctionBody callback to libIndex, and refactored clients to use it instead of inventing their own solution

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/include/clang/Index/IndexingAction.h:60
+
+inline std::unique_ptr createIndexingASTConsumer(
+std::shared_ptr DataConsumer,

Why not use default argument instead of overloading?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66879



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

[PATCH] D66877: Moved the IndexDataConsumer::finish call into the IndexASTConsumer from IndexAction

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clang/lib/Index/IndexingAction.cpp:77
+IndexCtx->getDataConsumer().setPreprocessor(PP);
+PP->addPPCallbacks(std::make_unique(IndexCtx));
+  }

jkorous wrote:
> gribozavr wrote:
> > ilya-biryukov wrote:
> > > The fact that we call `addPPCallbacks` **after** creating `ASTContext` is 
> > > a bit concerning.
> > > 
> > > This wouldn't probably cause any problems in practice, but generally 
> > > changing preprocessor by the time `ASTContext` is already created seems 
> > > dangerous (what if something there ever starts depending on the state of 
> > > the preprocessor?)
> > > 
> > > I think this concern is easily elevated if we change Preprocessor and 
> > > call addPPCallbacks on `CreateASTConsumer(CompilerInvocation& CI)`. That 
> > > would mean we have a separate function that sets up the preprocessor and 
> > > creates `IndexASTConsumer` and it should be exposed to the clients.
> > > 
> > > Have you considered this approach? Would it work?
> > It does feel a bit weird, but we shouldn't have started parsing before 
> > calling `Initialize` on the `ASTConsumer`. Therefore I agree with you that 
> > it won't cause problems in practice.
> > 
> > Calling `addPPCallbacks` in `FrontendAction` is against the goal of this 
> > patch set. The goal is to encapsulate as much as possible in the 
> > `IndexASTConsumer`, because they compose well, unlike `FrontendAction`s. 
> > Therefore, requiring customization in `FrontendAction` is not possible.
> We could also move the preprocessor setup to constructor and check/add 
> asserts that `ASTContext` has been provided to `IndexingContext` and 
> `IndexDataConsumer` implementations.
LG, let's keep it like this and hope this won't break in the future too.
And if it will, we would probably be able to fix this by setting up 
`PPCallbacks` at the time when we create the `ASTConsumer`, i.e.
```
std::unique_ptr createIndexASTConsumer(CompilerInvocation ) {
  CI.getPreprocessor().addPPCallback();
  return llvm::make_unique(...);
}
```

That would reduce the gap between `FrontendAction::BeginSourceFile` and 
`addPPCallbacks` to just a few instructions, and the chances of breaking stuff 
would be even less.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66877



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

[PATCH] D66877: Moved the IndexDataConsumer::finish call into the IndexASTConsumer from IndexAction

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/lib/Index/IndexingAction.cpp:77
+IndexCtx->getDataConsumer().setPreprocessor(PP);
+PP->addPPCallbacks(std::make_unique(IndexCtx));
+  }

gribozavr wrote:
> ilya-biryukov wrote:
> > The fact that we call `addPPCallbacks` **after** creating `ASTContext` is a 
> > bit concerning.
> > 
> > This wouldn't probably cause any problems in practice, but generally 
> > changing preprocessor by the time `ASTContext` is already created seems 
> > dangerous (what if something there ever starts depending on the state of 
> > the preprocessor?)
> > 
> > I think this concern is easily elevated if we change Preprocessor and call 
> > addPPCallbacks on `CreateASTConsumer(CompilerInvocation& CI)`. That would 
> > mean we have a separate function that sets up the preprocessor and creates 
> > `IndexASTConsumer` and it should be exposed to the clients.
> > 
> > Have you considered this approach? Would it work?
> It does feel a bit weird, but we shouldn't have started parsing before 
> calling `Initialize` on the `ASTConsumer`. Therefore I agree with you that it 
> won't cause problems in practice.
> 
> Calling `addPPCallbacks` in `FrontendAction` is against the goal of this 
> patch set. The goal is to encapsulate as much as possible in the 
> `IndexASTConsumer`, because they compose well, unlike `FrontendAction`s. 
> Therefore, requiring customization in `FrontendAction` is not possible.
We could also move the preprocessor setup to constructor and check/add asserts 
that `ASTContext` has been provided to `IndexingContext` and 
`IndexDataConsumer` implementations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66877



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

[PATCH] D66878: [Index] Stopped wrapping FrontendActions in libIndex and its users

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clang-tools-extra/clangd/index/IndexAction.cpp:217
   std::unique_ptr Includes;
+  index::IndexingOptions Opts;
   std::unique_ptr PragmaHandler;

gribozavr wrote:
> ilya-biryukov wrote:
> > Are these option ever used? Do we need to keep them alive for the lifetime 
> > of the action?
> > Might be worth a comment.
> They are passed in through the constructor, and consumed by 
> `index::createIndexingASTConsumer` in `CreateASTConsumer`. So they need to be 
> stored in a member variable between those two calls.
Ah, missed that at first glance. Thanks for the explanation, LG.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66878



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

[clang-tools-extra] r370229 - [clangd] Migrate last tweak tests to TweakTesting.h and remove old helpers. NFC

[Sam McCall via cfe-commits]

Author: sammccall
Date: Wed Aug 28 09:41:22 2019
New Revision: 370229

URL: http://llvm.org/viewvc/llvm-project?rev=370229=rev
Log:
[clangd] Migrate last tweak tests to TweakTesting.h and remove old helpers. NFC

Modified:
clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp

Modified: clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp?rev=370229=370228=370229=diff
==
--- clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp (original)
+++ clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp Wed Aug 28 09:41:22 
2019
@@ -23,8 +23,6 @@
 #include "gtest/gtest.h"
 #include 
 
-using llvm::Failed;
-using llvm::Succeeded;
 using ::testing::AllOf;
 using ::testing::HasSubstr;
 using ::testing::StartsWith;
@@ -33,100 +31,6 @@ namespace clang {
 namespace clangd {
 namespace {
 
-// FIXME(sammccall): migrate the rest of the tests to use TweakTesting.h and
-// remove these helpers.
-std::string markRange(llvm::StringRef Code, Range R) {
-  size_t Begin = llvm::cantFail(positionToOffset(Code, R.start));
-  size_t End = llvm::cantFail(positionToOffset(Code, R.end));
-  assert(Begin <= End);
-  if (Begin == End) // Mark a single point.
-return (Code.substr(0, Begin) + "^" + Code.substr(Begin)).str();
-  // Mark a range.
-  return (Code.substr(0, Begin) + "[[" + Code.substr(Begin, End - Begin) +
-  "]]" + Code.substr(End))
-  .str();
-}
-
-void checkAvailable(StringRef ID, llvm::StringRef Input, bool Available) {
-  Annotations Code(Input);
-  ASSERT_TRUE(0 < Code.points().size() || 0 < Code.ranges().size())
-  << "no points of interest specified";
-  TestTU TU;
-  TU.Filename = "foo.cpp";
-  TU.Code = Code.code();
-
-  ParsedAST AST = TU.build();
-
-  auto CheckOver = [&](Range Selection) {
-unsigned Begin = cantFail(positionToOffset(Code.code(), Selection.start));
-unsigned End = cantFail(positionToOffset(Code.code(), Selection.end));
-auto T = prepareTweak(ID, Tweak::Selection(AST, Begin, End));
-if (Available)
-  EXPECT_THAT_EXPECTED(T, Succeeded())
-  << "code is " << markRange(Code.code(), Selection);
-else
-  EXPECT_THAT_EXPECTED(T, Failed())
-  << "code is " << markRange(Code.code(), Selection);
-  };
-  for (auto P : Code.points())
-CheckOver(Range{P, P});
-  for (auto R : Code.ranges())
-CheckOver(R);
-}
-
-/// Checks action is available at every point and range marked in \p Input.
-void checkAvailable(StringRef ID, llvm::StringRef Input) {
-  return checkAvailable(ID, Input, /*Available=*/true);
-}
-
-/// Same as checkAvailable, but checks the action is not available.
-void checkNotAvailable(StringRef ID, llvm::StringRef Input) {
-  return checkAvailable(ID, Input, /*Available=*/false);
-}
-
-llvm::Expected apply(StringRef ID, llvm::StringRef Input) {
-  Annotations Code(Input);
-  Range SelectionRng;
-  if (Code.points().size() != 0) {
-assert(Code.ranges().size() == 0 &&
-   "both a cursor point and a selection range were specified");
-SelectionRng = Range{Code.point(), Code.point()};
-  } else {
-SelectionRng = Code.range();
-  }
-  TestTU TU;
-  TU.Filename = "foo.cpp";
-  TU.Code = Code.code();
-
-  ParsedAST AST = TU.build();
-  unsigned Begin = cantFail(positionToOffset(Code.code(), SelectionRng.start));
-  unsigned End = cantFail(positionToOffset(Code.code(), SelectionRng.end));
-  Tweak::Selection S(AST, Begin, End);
-
-  auto T = prepareTweak(ID, S);
-  if (!T)
-return T.takeError();
-  return (*T)->apply(S);
-}
-
-llvm::Expected applyEdit(StringRef ID, llvm::StringRef Input) {
-  auto Effect = apply(ID, Input);
-  if (!Effect)
-return Effect.takeError();
-  if (!Effect->ApplyEdit)
-return llvm::createStringError(llvm::inconvertibleErrorCode(),
-   "No replacements");
-  Annotations Code(Input);
-  return applyAllReplacements(Code.code(), *Effect->ApplyEdit);
-}
-
-void checkTransform(llvm::StringRef ID, llvm::StringRef Input,
-std::string Output) {
-  auto Result = applyEdit(ID, Input);
-  ASSERT_TRUE(bool(Result)) << llvm::toString(Result.takeError()) << Input;
-  EXPECT_EQ(Output, std::string(*Result)) << Input;
-}
-
 TWEAK_TEST(SwapIfBranches);
 TEST_F(SwapIfBranchesTest, Test) {
   Context = Function;
@@ -215,9 +119,9 @@ TEST_F(DumpRecordLayoutTest, Test) {
   AllOf(StartsWith("message:"), HasSubstr("0 |   int x")));
 }
 
-TEST(TweaksTest, ExtractVariable) {
-  llvm::StringLiteral ID = "ExtractVariable";
-  checkAvailable(ID, R"cpp(
+TWEAK_TEST(ExtractVariable);
+TEST_F(ExtractVariableTest, Test) {
+  EXPECT_AVAILABLE(R"cpp(
 int xyz(int a = 1) {
   struct T {
 int bar(int a = 1);
@@ -257,14 +161,14 @@ TEST(TweaksTest, ExtractVariable) {
 }
   )cpp");
   // Should not crash.
-  checkNotAvailable(ID, R"cpp(
+  

[PATCH] D66397: [Diagnostics] Improve -Wxor-used-as-pow

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

In D66397#1647455 , @aaron.ballman 
wrote:

> Adding @rsmith to see if we can make forward progress on this patch again.


On the other side, I don't want to waste Richard's time since I dont want to 
extend (variables and macros are controversal topic anyway) it more now. I 
promised to @jfb to handle (2 ^ 64) - 1 as follow up patch and the promised 
patch is here..


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

https://reviews.llvm.org/D66397



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

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

[Sergey Dmitriev via Phabricator via cfe-commits]

sdmitriev added a comment.

Looks like there will be no more comments. If so, I will update the first part 
https://reviews.llvm.org/D65130 which adds clang-offload-wrapper tool with the 
latest changes.


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

https://reviews.llvm.org/D64943



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

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

[Balázs Benics via Phabricator via cfe-commits]

steakhal updated this revision to Diff 217665.
steakhal added a comment.

Changes:

- Flag option marked as 'enabled by default'.
- Reformat all the test cases for C, C++ and Obj C.
- Now uses `-verify=tags` approach.
- Fixes checker documentation.


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

https://reviews.llvm.org/D66733

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/dead-stores.c
  clang/test/Analysis/dead-stores.cpp
  clang/test/Analysis/dead-stores.m

Index: clang/test/Analysis/dead-stores.m
===
--- clang/test/Analysis/dead-stores.m
+++ clang/test/Analysis/dead-stores.m
@@ -1,5 +1,4 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core -analyzer-checker=deadcode.DeadStores,osx.cocoa.RetainCount -fblocks -verify -Wno-objc-root-class %s
-// expected-no-diagnostics
 
 typedef signed char BOOL;
 typedef unsigned int NSUInteger;
@@ -72,7 +71,8 @@
 
 @implementation Rdar7947686_B
 - (id) init {
-  id x = (self = [super init]); // no-warning
+  id x = (self = [super init]);
+  // expected-warning@-1 {{Although the value stored to 'self'}}
   return x;
 }
 @end
Index: clang/test/Analysis/dead-stores.cpp
===
--- clang/test/Analysis/dead-stores.cpp
+++ clang/test/Analysis/dead-stores.cpp
@@ -1,15 +1,26 @@
-// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11 -analyzer-checker=deadcode.DeadStores -verify -Wno-unreachable-code %s
-// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11 -analyzer-store=region -analyzer-checker=deadcode.DeadStores -verify -Wno-unreachable-code %s
+// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11\
+// RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code\
+// RUN:  -analyzer-config deadcode.DeadStores:WarnForDeadNestedAssignments=false\
+// RUN:  -verify=non-nested %s
+//
+// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11\
+// RUN:  -analyzer-store=region -analyzer-checker=deadcode.DeadStores   \
+// RUN:  -analyzer-config deadcode.DeadStores:WarnForDeadNestedAssignments=false\
+// RUN:  -Wno-unreachable-code -verify=non-nested %s
+//
+// RUN: %clang_analyze_cc1 -fcxx-exceptions -fexceptions -fblocks -std=c++11\
+// RUN:  -analyzer-checker=deadcode.DeadStores -Wno-unreachable-code\
+// RUN:  -verify=non-nested,nested %s
 
 //===--===//
 // Basic dead store checking (but in C++ mode).
 //===--===//
 
 int j;
+int make_int();
 void test1() {
   int x = 4;
-
-  x = x + 1; // expected-warning{{never read}}
+  x = x + 1; // non-nested-warning {{never read}}
 
   switch (j) {
   case 1:
@@ -17,6 +28,11 @@
 (void)x;
 break;
   }
+
+  int y;
+  (void)y;
+  if ((y = make_int())) // nested-warning {{Although the value stored}}
+return;
 }
 
 //===--===//
@@ -25,6 +41,7 @@
 
 class Test2 {
   int 
+
 public:
   Test2(int ) : x(y) {}
   ~Test2() { ++x; }
@@ -66,17 +83,17 @@
 //===--===//
 
 void test3_a(int x) {
-   x = x + 1; // expected-warning{{never read}}
+  x = x + 1; // non-nested-warning {{never read}}
 }
 
 void test3_b(int ) {
-  x = x + 1; // no-warninge
+  x = x + 1; // no-warning
 }
 
 void test3_c(int x) {
   int  = x;
-  // Shows the limitation of dead stores tracking.  The write is really
-  // dead since the value cannot escape the function.
+  // Shows the limitation of dead stores tracking. The write is really dead
+  // since the value cannot escape the function.
   ++y; // no-warning
 }
 
@@ -94,7 +111,7 @@
 //===--===//
 
 static void test_new(unsigned n) {
-  char **p = new char* [n]; // expected-warning{{never read}}
+  char **p = new char *[n]; // non-nested-warning {{never read}}
 }
 
 //===--===//
@@ -102,11 +119,11 @@
 //===--===//
 
 namespace foo {
-  int test_4(int x) {
-x = 2; // expected-warning{{Value stored to 'x' is never read}}
-x = 2;
-return x;
-  }
+int test_4(int x) {
+  x = 2; // non-nested-warning {{Value stored to 'x' is never read}}
+  x = 2;
+  return x;
+}
 }
 
 //===--===//
@@ -119,42 +136,39 @@
   try {
 x = 2; // no-warning
 test_5_Aux();
-  }
-  catch (int z) {
+  } catch (int z) {
 return x 

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

[Steve Canon via Phabricator via cfe-commits]

scanon added inline comments.



Comment at: test/libcxx/numerics/truncating_cast.pass.cpp:36
+  {static_cast(Lim::max()) + 1, Lim::max(), false},
+  {static_cast(Lim::max()) + 1024, Lim::max(), false},
+  };

Probably should test `nextafter(static_cast(Lim::max()), INFINITY)` 
here instead.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D66836



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

[PATCH] D66836: [libc++] Add `__truncating_cast` for safely casting float types to integers

[Steve Canon via Phabricator via cfe-commits]

scanon added a comment.

I would tend to write this function in the following form:

  // set up lower bound and upper bound
  if (r > upperBound) r = upperBound;
  if (!(r >= lowerBound)) r = lowerBound; // NaN is mapped to l.b.
  return static_cast(r);

I prefer to avoid the explicit trunc call, since that's the defined behavior of 
the `static_cast` once the value is in-range, anyway.




Comment at: include/math.h:1573
+  enum { _Bits = numeric_limits<_IntT>::digits - 
numeric_limits<_FloatT>::digits };
+  static const _IntT value = numeric_limits<_IntT>::max() >> _Bits << _Bits;
+};

zoecarver wrote:
> What's the reasoning behind shifting something forward and back? Shouldn't 
> this always negate the other operation? 
This function doesn't quite do what it says on the tin; it considers the number 
of significand bits used for the floating-point type, but not the exponent 
range. This doesn't matter for double, because double's exponent range is much, 
much larger than any integer type, but it does matter for types like float16 
(largest representable value is 65504)--when it's added as a standard 
floating-point type at some future point, this will introduce subtle bugs.

You should be able to work around this by converting `value` to  `_FloatT`, 
taking the minimum of the result and numeric_limits::max, and converting back.

This also assumes that _FloatT has radix == 2, which I do not believe is 
actually implied by `is_floating_point == true`. Please add a static assert for 
that so that future decimal types don't use this template.



Comment at: include/math.h:1582
+  const _RealT __trunc_r = __builtin_trunc(__r);
+  if (__trunc_r >= ::nextafter(static_cast<_RealT>(_MaxVal), INFINITY)) {
+return _Lim::max();

zoecarver wrote:
> Maybe change `INFINITY` to `std::numeric_limits< _RealT >::infinity()`
Why isn't this just `__trunc_r > _MaxVal`?



Comment at: include/math.h:1584
+return _Lim::max();
+  } else if (__trunc_r <= _Lim::lowest()) {
+return _Lim::min();

This has a subtle assumption that `_IntT` is two's-complement and `_FloatT` has 
`radix=2`, so that the implicit conversion that occurs in the comparison is 
exact. The radix should be a static assert; does libc++ care about 
non-two's-complement at all?

Just from a clarity perspective, I would personally make the conversion 
explicit.



Comment at: include/math.h:1586
+return _Lim::min();
+  }
+  return static_cast<_IntT>(__trunc_r);

If I'm reading right, NaNs will fall through the above two comparisons and 
invoke UB on the static_cast below. I suspect that's not the desired behavior. 
What is the intended result for NaN?


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D66836



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

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

[Balázs Benics via Phabricator via cfe-commits]

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

Fixes for @NoQ's comments.
I will update the patch.


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

https://reviews.llvm.org/D66733



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

[PATCH] D66556: [clang-scan-deps] Minimizer: Correctly handle multi-line content with CR+LF line endings

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

rL370219  and rG3c307370c8f8 



Repository:
  rL LLVM

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

https://reviews.llvm.org/D66556



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

[PATCH] D65433: [clangd] DefineInline action availability checks

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:59
+
+void checkAvailable(StringRef ID, llvm::StringRef Input, bool Available,
+const llvm::StringMap ) {

This function, and all the other helpers in this file, are actually imminently 
going away in favor of TweakTesting.h.
Sorry it's been stuck in my backlog for a couple of weeks...

We should work out how to do that stuff there.
One approach is to give TweakTest a `StringMap ExtraFiles` and 
`StringMap EditedFiles` that excludes main, so you'd write
```
ExtraFiles["foo.h"] = "...";
EXPECT_EQ(apply("..."), "...");
EXPECT_THAT(EditedFiles, ElementsAre(Pair(testPath("foo.h"), "...");
```
or something like this.

(Whatever we do, we should bear in mind the fact that out-line will mean we 
need to supply an index too)



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:631
+
+  // Should it also trigger on NestedNameSpecifier? i.e "Bar::"
+  [[void [[Bar::[[b^a^z() [[{

I don't think so



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:653
+
+TEST_F(DefineInlineTest, NoDecl) {
+  checkNotAvailable(TweakID, R"cpp(

nit: NoForwardDecl



Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:683
+})cpp",
+ {{"a.h", "void bar(); void foo(int test);"}});
+

FWIW, I find these params rather cryptic - it'd be nice to have an API for the 
helpers that is explicit about this being files/filenames


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65433



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

r370219 - Try fixing CRLF issues in Git with [clang-scan-deps] Minimizer: Correctly handle multi-line content with CR+LF line endings

[Alexandre Ganea via cfe-commits]

Author: aganea
Date: Wed Aug 28 08:14:37 2019
New Revision: 370219

URL: http://llvm.org/viewvc/llvm-project?rev=370219=rev
Log:
Try fixing CRLF issues in Git with [clang-scan-deps] Minimizer: Correctly 
handle multi-line content with CR+LF line endings

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

Added:

cfe/trunk/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c   
(with props)
Removed:
cfe/trunk/.gitattributes

Removed: cfe/trunk/.gitattributes
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/.gitattributes?rev=370218=auto
==
--- cfe/trunk/.gitattributes (original)
+++ cfe/trunk/.gitattributes (removed)
@@ -1,3 +0,0 @@
-# Windows line ending tests
-test/Lexer/minimize_source_to_dependency_directives_invalid_error.c text 
eol=crlf
-test/FixIt/fixit-newline-style.c text eol=crlf

Added: 
cfe/trunk/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c?rev=370219=auto
==
--- 
cfe/trunk/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c 
(added)
+++ 
cfe/trunk/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c 
Wed Aug 28 08:14:37 2019
@@ -0,0 +1,16 @@
+// Test CF+LF are properly handled along with quoted, multi-line #error
+// RUN: %clang_cc1 -DOTHER -print-dependency-directives-minimized-source %s 
2>&1 | FileCheck %s
+
+#ifndef TEST
+#error "message \
+   more message \
+   even more"
+#endif
+
+#ifdef OTHER
+#include 
+#endif
+
+// CHECK:  #ifdef OTHER
+// CHECK-NEXT: #include 
+// CHECK-NEXT: #endif

Propchange: 
cfe/trunk/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
--
svn:eol-style = CRLF


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

[PATCH] D66881: [clangd][vscode] Don't leak the resources

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL370218: [clangd][vscode] Dont leak the resources 
(authored by hokein, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66881?vs=217657=217659#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66881

Files:
  clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts


Index: clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
===
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
@@ -111,6 +111,8 @@
 serverOptions, clientOptions);
   const semanticHighlightingFeature =
   new semanticHighlighting.SemanticHighlightingFeature();
+  context.subscriptions.push(
+  vscode.Disposable.from(semanticHighlightingFeature));
   clangdClient.registerFeature(semanticHighlightingFeature);
   console.log('Clang Language Server is now active!');
   context.subscriptions.push(clangdClient.start());
@@ -133,9 +135,10 @@
 vscode.window.showTextDocument(doc);
   }));
   const status = new FileStatus();
+  context.subscriptions.push(vscode.Disposable.from(status));
   context.subscriptions.push(vscode.window.onDidChangeActiveTextEditor(
   () => { status.updateStatus(); }));
-  clangdClient.onDidChangeState(({newState}) => {
+  context.subscriptions.push(clangdClient.onDidChangeState(({newState}) => {
 if (newState == vscodelc.State.Running) {
   // clangd starts or restarts after crash.
   clangdClient.onNotification(
@@ -150,7 +153,7 @@
   status.clear();
   semanticHighlightingFeature.dispose();
 }
-  })
+  }));
   // An empty place holder for the activate command, otherwise we'll get an
   // "command is not registered" error.
   context.subscriptions.push(vscode.commands.registerCommand(


Index: clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
===
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
@@ -111,6 +111,8 @@
 serverOptions, clientOptions);
   const semanticHighlightingFeature =
   new semanticHighlighting.SemanticHighlightingFeature();
+  context.subscriptions.push(
+  vscode.Disposable.from(semanticHighlightingFeature));
   clangdClient.registerFeature(semanticHighlightingFeature);
   console.log('Clang Language Server is now active!');
   context.subscriptions.push(clangdClient.start());
@@ -133,9 +135,10 @@
 vscode.window.showTextDocument(doc);
   }));
   const status = new FileStatus();
+  context.subscriptions.push(vscode.Disposable.from(status));
   context.subscriptions.push(vscode.window.onDidChangeActiveTextEditor(
   () => { status.updateStatus(); }));
-  clangdClient.onDidChangeState(({newState}) => {
+  context.subscriptions.push(clangdClient.onDidChangeState(({newState}) => {
 if (newState == vscodelc.State.Running) {
   // clangd starts or restarts after crash.
   clangdClient.onNotification(
@@ -150,7 +153,7 @@
   status.clear();
   semanticHighlightingFeature.dispose();
 }
-  })
+  }));
   // An empty place holder for the activate command, otherwise we'll get an
   // "command is not registered" error.
   context.subscriptions.push(vscode.commands.registerCommand(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/SourceCode.cpp:601
+  llvm::inconvertibleErrorCode(),
+  "File contents differ on disk for %s, please save", FilePath.data());
+}

you seem to be checking this both here and in clangdlspserver. Why?



Comment at: clang-tools-extra/clangd/SourceCode.h:42
 
+/// Represents a set of edits generated for a single file.
+struct Edit {

nit: drop "represents"



Comment at: clang-tools-extra/clangd/SourceCode.h:42
 
+/// Represents a set of edits generated for a single file.
+struct Edit {

sammccall wrote:
> nit: drop "represents"
nit: this could also describe Replacements, vector, etc. Motivate 
this class a little more?



Comment at: clang-tools-extra/clangd/SourceCode.h:215
 
+/// Formats the edits in \p ApplyEdits and generates TextEdits from those.
+/// Ensures the files in FS are consistent with the files used while generating

not sure what "generates TextEdits from those" refers to.

Could this function be called "reformatEdits" or "formatAroundEdits"?



Comment at: clang-tools-extra/clangd/SourceCode.h:220
+llvm::Error formatAndGenerateEdits(llvm::vfs::FileSystem *FS,
+   llvm::StringMap ,
+   llvm::StringRef MainFilePath,

This signature is confusing: we pass code in *three* different ways (FS, Edits, 
and MainFileCode). Much of this is because we put the loop (and therefore all 
special cases) inside this function.
The logic around the way the VFS is mapped for the main file vs others doesn't 
really belong in this layer. Neither does "please save"...

It seems this wants to be something simpler/smaller like `reformatEdit(const 
Edit&, const Style&)` that could be called from ClangdServer. There's probably 
another helper like `checkApplies(const Edit&, VFS*)` that would go in 
ClangdLSPServer.




Comment at: clang-tools-extra/clangd/refactor/Tweak.h:74
+/// A mapping from absolute file path to edits.
+llvm::Optional> ApplyEdits;
 

what's the difference between `None` and an empty map?



Comment at: clang-tools-extra/clangd/refactor/Tweak.h:76
 
-static Effect applyEdit(tooling::Replacements R) {
+void addEdit(StringRef FilePath, Edit Ed) {
+  assert(ApplyEdits);

(if the null map case goes away, this can too)



Comment at: clang-tools-extra/clangd/refactor/Tweak.h:81
+
+static Effect applyEdit(StringRef FilePath, Edit Ed) {
   Effect E;

This greatly increases the burden of callers of this function, who mostly want 
to edit the current file.
 - need to provide a fully-qualified name (I don't think the implementations in 
this patch actually do that)
 - need to construct an Edit

can we provide an `Effect mainFileEdit(const SourceManager&, Replacements)`? 
and maybe `Effect fileEdit(const SourceManager&, FileID, Replacements)` though 
maybe the latter doesn't cover enough cases to pull its weight.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66637



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

[clang-tools-extra] r370218 - [clangd][vscode] Don't leak the resources

[Haojian Wu via cfe-commits]

Author: hokein
Date: Wed Aug 28 08:09:04 2019
New Revision: 370218

URL: http://llvm.org/viewvc/llvm-project?rev=370218=rev
Log:
[clangd][vscode] Don't leak the resources

Summary: We miss a few places where we need to add them to the subscriptions.

Reviewers: jvikstrom

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits

Tags: #clang

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

Modified:
clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts

Modified: clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts?rev=370218=370217=370218=diff
==
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts 
(original)
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts Wed 
Aug 28 08:09:04 2019
@@ -111,6 +111,8 @@ export function activate(context: vscode
 serverOptions, clientOptions);
   const semanticHighlightingFeature =
   new semanticHighlighting.SemanticHighlightingFeature();
+  context.subscriptions.push(
+  vscode.Disposable.from(semanticHighlightingFeature));
   clangdClient.registerFeature(semanticHighlightingFeature);
   console.log('Clang Language Server is now active!');
   context.subscriptions.push(clangdClient.start());
@@ -133,9 +135,10 @@ export function activate(context: vscode
 vscode.window.showTextDocument(doc);
   }));
   const status = new FileStatus();
+  context.subscriptions.push(vscode.Disposable.from(status));
   context.subscriptions.push(vscode.window.onDidChangeActiveTextEditor(
   () => { status.updateStatus(); }));
-  clangdClient.onDidChangeState(({newState}) => {
+  context.subscriptions.push(clangdClient.onDidChangeState(({newState}) => {
 if (newState == vscodelc.State.Running) {
   // clangd starts or restarts after crash.
   clangdClient.onNotification(
@@ -150,7 +153,7 @@ export function activate(context: vscode
   status.clear();
   semanticHighlightingFeature.dispose();
 }
-  })
+  }));
   // An empty place holder for the activate command, otherwise we'll get an
   // "command is not registered" error.
   context.subscriptions.push(vscode.commands.registerCommand(


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

[PATCH] D66881: [clangd][vscode] Don't leak the resources

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 217657.
hokein added a comment.

dispose the semanticHighlighting feature as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66881

Files:
  clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts


Index: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
@@ -111,6 +111,8 @@
 serverOptions, clientOptions);
   const semanticHighlightingFeature =
   new semanticHighlighting.SemanticHighlightingFeature();
+  context.subscriptions.push(
+  vscode.Disposable.from(semanticHighlightingFeature));
   clangdClient.registerFeature(semanticHighlightingFeature);
   console.log('Clang Language Server is now active!');
   context.subscriptions.push(clangdClient.start());
@@ -133,9 +135,10 @@
 vscode.window.showTextDocument(doc);
   }));
   const status = new FileStatus();
+  context.subscriptions.push(vscode.Disposable.from(status));
   context.subscriptions.push(vscode.window.onDidChangeActiveTextEditor(
   () => { status.updateStatus(); }));
-  clangdClient.onDidChangeState(({newState}) => {
+  context.subscriptions.push(clangdClient.onDidChangeState(({newState}) => {
 if (newState == vscodelc.State.Running) {
   // clangd starts or restarts after crash.
   clangdClient.onNotification(
@@ -150,7 +153,7 @@
   status.clear();
   semanticHighlightingFeature.dispose();
 }
-  })
+  }));
   // An empty place holder for the activate command, otherwise we'll get an
   // "command is not registered" error.
   context.subscriptions.push(vscode.commands.registerCommand(


Index: clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/extension.ts
@@ -111,6 +111,8 @@
 serverOptions, clientOptions);
   const semanticHighlightingFeature =
   new semanticHighlighting.SemanticHighlightingFeature();
+  context.subscriptions.push(
+  vscode.Disposable.from(semanticHighlightingFeature));
   clangdClient.registerFeature(semanticHighlightingFeature);
   console.log('Clang Language Server is now active!');
   context.subscriptions.push(clangdClient.start());
@@ -133,9 +135,10 @@
 vscode.window.showTextDocument(doc);
   }));
   const status = new FileStatus();
+  context.subscriptions.push(vscode.Disposable.from(status));
   context.subscriptions.push(vscode.window.onDidChangeActiveTextEditor(
   () => { status.updateStatus(); }));
-  clangdClient.onDidChangeState(({newState}) => {
+  context.subscriptions.push(clangdClient.onDidChangeState(({newState}) => {
 if (newState == vscodelc.State.Running) {
   // clangd starts or restarts after crash.
   clangdClient.onNotification(
@@ -150,7 +153,7 @@
   status.clear();
   semanticHighlightingFeature.dispose();
 }
-  })
+  }));
   // An empty place holder for the activate command, otherwise we'll get an
   // "command is not registered" error.
   context.subscriptions.push(vscode.commands.registerCommand(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits