date:20190514

[PATCH] D61350: [clang-tidy] New check calling out uses of +new in Objective-C code

2019-05-14 Thread Michael Wyman via Phabricator via cfe-commits

mwyman updated this revision to Diff 199375.
mwyman added a comment.

Bah, previous changes not caught in Git commit; switching back and forth 
between Git/Mercurial makes for some mix-ups, I guess.


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

https://reviews.llvm.org/D61350

Files:
  clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.cpp
  clang-tools-extra/clang-tidy/google/AvoidNSObjectNewCheck.h
  clang-tools-extra/clang-tidy/google/CMakeLists.txt
  clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/google-objc-avoid-nsobject-new.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/google-objc-avoid-nsobject-new.m

Index: clang-tools-extra/test/clang-tidy/google-objc-avoid-nsobject-new.m
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/google-objc-avoid-nsobject-new.m
@@ -0,0 +1,80 @@
+// RUN: %check_clang_tidy %s google-objc-avoid-nsobject-new %t
+
+@interface NSObject
++ (instancetype)new;
++ (instancetype)alloc;
+- (instancetype)init;
+@end
+
+@interface NSProxy  // Root class with no -init method.
+@end
+
+// NSDate provides a specific factory method.
+@interface NSDate : NSObject
++ (instancetype)date;
+@end
+
+// For testing behavior with Objective-C Generics.
+@interface NSMutableDictionary<__covariant KeyType, __covariant ObjectType> : NSObject
+@end
+
+@class NSString;
+
+#define ALLOCATE_OBJECT(_Type) [_Type new]
+
+void CheckSpecificInitRecommendations(void) {
+  NSObject *object = [NSObject new];
+  // CHECK-MESSAGES: [[@LINE-1]]:22: warning: do not create objects with +new [google-objc-avoid-nsobject-new]
+  // CHECK-FIXES: [NSObject alloc] init];
+
+  NSDate *correctDate = [NSDate date];
+  NSDate *incorrectDate = [NSDate new];
+  // CHECK-MESSAGES: [[@LINE-1]]:27: warning: do not create objects with +new [google-objc-avoid-nsobject-new]
+  // CHECK-FIXES: [NSDate date];
+
+  NSObject *macroCreated = ALLOCATE_OBJECT(NSObject);  // Shouldn't warn on macros.
+
+  NSMutableDictionary *dict = [NSMutableDictionary new];
+  // CHECK-MESSAGES: [[@LINE-1]]:31: warning: do not create objects with +new [google-objc-avoid-nsobject-new]
+  // CHECK-FIXES: [NSMutableDictionary alloc] init];
+}
+
+@interface Foo : NSObject
++ (instancetype)new; // Declare again to suppress warning.
+- (instancetype)initWithInt:(int)anInt;
+- (instancetype)init __attribute__((unavailable));
+
+- (id)new;
+@end
+
+@interface Baz : Foo // Check unavailable -init through inheritance.
+@end
+
+@interface ProxyFoo : NSProxy
++ (instancetype)new;
+@end
+
+void CallNewWhenInitUnavailable(void) {
+  Foo *foo = [Foo new];
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: do not create objects with +new [google-objc-avoid-nsobject-new]
+
+  Baz *baz = [Baz new];
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: do not create objects with +new [google-objc-avoid-nsobject-new]
+
+  // Instance method -new calls may be weird, but are not strictly forbidden.
+  Foo *bar = [[Foo alloc] initWithInt:4];
+  [bar new];
+
+  ProxyFoo *proxy = [ProxyFoo new];
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: do not create objects with +new [google-objc-avoid-nsobject-new]
+}
+
+@interface HasNewOverride : NSObject
+@end
+
+@implementation HasNewOverride
++ (instancetype)new {
+  return [[self alloc] init];
+}
+// CHECK-MESSAGES: [[@LINE-3]]:1: warning: classes should not override +new [google-objc-avoid-nsobject-new]
+@end
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -133,6 +133,7 @@
google-default-arguments
google-explicit-constructor
google-global-names-in-headers
+   google-objc-avoid-nsobject-new
google-objc-avoid-throwing-exception
google-objc-function-naming
google-objc-global-variable-declaration
Index: clang-tools-extra/docs/clang-tidy/checks/google-objc-avoid-nsobject-new.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/google-objc-avoid-nsobject-new.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - google-objc-avoid-nsobject-new
+
+google-objc-avoid-nsobject-new
+==
+
+Finds calls to ``+new`` or overrides of it, which are prohibited by the
+Google Objective-C style guide.
+
+The Google Objective-C style guide forbids calling ``+new`` or overriding it in
+class implementations, preferring ``+alloc`` and ``-init`` methods to
+instantiate objects.
+
+An example:
+
+.. code-block:: objc
+
+  NSDate *now = [NSDate new];
+  Foo *bar = [Foo new];
+
+Instead, code should use ``+alloc``/``-init`` or class factory methods.
+
+.. code-block:: objc
+
+  NSDate *now = [NSDate date];
+  Foo *bar = [[Foo alloc] i

[PATCH] D61861: DeleteNullPointerCheck now deletes until the end brace of the condition

2019-05-14 Thread Jonathan Camilleri via Phabricator via cfe-commits

J-Camilleri added a comment.

In D61861#1500868 , @madsravn wrote:

> Looks good to me :)


Thanks for the review and suggestion, who should I contact now in order to push 
the patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61861



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

[PATCH] D61861: DeleteNullPointerCheck now deletes until the end brace of the condition

2019-05-14 Thread Mads Ravn via Phabricator via cfe-commits

madsravn added a comment.

In D61861#1500900 , @J-Camilleri wrote:

> In D61861#1500868 , @madsravn wrote:
>
> > Looks good to me :)
>
>
> Thanks for the review and suggestion, who should I contact now in order to 
> push the patch?

I asked around and I was told that we should just leave a comment and then 
somebody will come around. I was also told that after a few patches you can 
obtain commit access if you want to push stuff yourself.

someone please commit this.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61861

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

[PATCH] D36357: Added a better diagnostic when using the delete operator with lambdas

2019-05-14 Thread Nicolas Lesser via Phabricator via cfe-commits

Rakete marked 3 inline comments as done.
Rakete added a comment.

> we could perform a tentative parse and skip to the } of the lambda.

How should I do this? Do I just skip to the next `}`, or also take into account 
 any additional scopes? Also does this mean that I skip and then revert, 
because that seems pretty expensive?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36357



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

[PATCH] D61851: [clang-tidy] New option for misc-throw-by-value-catch-by-reference

2019-05-14 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added inline comments.



Comment at: docs/ReleaseNotes.rst:182-184
+- New options `WarnOnLargeObject` and `MaxSize` for check
+  :doc:`misc-throw-by-value-catch-by-reference
+  ` check to warn

for check :doc:`..`<..> check

(repeated 'check' word)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D61851



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

[PATCH] D36357: Added a better diagnostic when using the delete operator with lambdas

2019-05-14 Thread Nicolas Lesser via Phabricator via cfe-commits

Rakete updated this revision to Diff 199380.
Rakete added a comment.

- Addressed review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36357

Files:
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseExprCXX.cpp
  clang/test/FixIt/fixit-cxx0x.cpp
  clang/test/Parser/cxx0x-lambda-expressions.cpp
  clang/test/SemaCXX/new-delete-0x.cpp

Index: clang/test/SemaCXX/new-delete-0x.cpp
===
--- clang/test/SemaCXX/new-delete-0x.cpp
+++ clang/test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,6 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 }
+
Index: clang/test/Parser/cxx0x-lambda-expressions.cpp
===
--- clang/test/Parser/cxx0x-lambda-expressions.cpp
+++ clang/test/Parser/cxx0x-lambda-expressions.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify -std=c++2a %s
 
 enum E { e };
 
@@ -53,8 +54,14 @@
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 delete [&] { return new int; } (); // ok, lambda
+
+delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+delete [](E Enum) { return new int((int)Enum); }(e); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+#if __cplusplus > 201703L
+delete [] { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+#endif
   }
 
   // We support init-captures in C++11 as an extension.
Index: clang/test/FixIt/fixit-cxx0x.cpp
===
--- clang/test/FixIt/fixit-cxx0x.cpp
+++ clang/test/FixIt/fixit-cxx0x.cpp
@@ -58,6 +58,9 @@
   (void)[&, i, i]{ }; // expected-error{{'i' can appear only once in a capture list}}
   (void)[] mutable { }; // expected-error{{lambda requires '()' before 'mutable'}}
   (void)[] -> int { }; // expected-error{{lambda requires '()' before return type}}
+
+  delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+  delete [] { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
 }
 
 #define bar "bar"
Index: clang/lib/Parse/ParseExprCXX.cpp
===
--- clang/lib/Parse/ParseExprCXX.cpp
+++ clang/lib/Parse/ParseExprCXX.cpp
@@ -2984,8 +2984,38 @@
 //   [Footnote: A lambda expression with a lambda-introducer that consists
 //  of empty square brackets can follow the delete keyword if
 //  the lambda expression is enclosed in parentheses.]
-// FIXME: Produce a better diagnostic if the '[]' is unambiguously a
-//lambda-introducer.
+
+const Token Next = GetLookAheadToken(2);
+
+// Basic lookahead to check if we have a lambda expression.
+if (Next.isOneOf(tok::l_brace, tok::less) ||
+(Next.is(tok::l_paren) &&
+ (GetLookAheadToken(3).is(tok::r_paren) ||
+  (GetLookAheadToken(3).is(tok::identifier) &&
+   GetLookAheadToken(4).is(tok::identifier) {
+  SourceLocation RSquareLock = NextToken().getLocation();
+  // Warn that the non-capturing lambda isn't surrounded by parentheses
+  // to disambiguate it from 'delete[]'.
+  ExprResult Lambda = ParseLambdaExpression();
+  if (Lambda.isInvalid())
+return ExprError();
+
+  SourceLocation StartLoc = Lambda.get()->getBeginLoc();
+  Diag(Start, diag::err_lambda_after_delete)
+  << SourceRange(Start, RSquareLock)
+  << FixItHint::CreateInsertion(StartLoc, "(")
+  << FixItHint::CreateInsertion(
+ Lexer::getLocForEndOfToken(Lambda.get()->getEndLoc(), 0,
+Actions.getSourceManager(),
+getLangOpts()),
+ ")");
+
+  // Evaluate any postfix expressions used on the lambda.
+  Lambda = ParsePostfixExpressionSuffix(Lambda);
+  return Actions.ActOnCXXDelete(Start, UseGlobal, /*ArrayForm=*/false,
+Lambda.get());
+}
+
 ArrayDelete = true;
 BalancedDelimiterTracker T(*this, tok::l_square);
 
Index: clang/include/clang/Basic/DiagnosticParseKinds.td

[PATCH] D61845: [builtin] Fixed definitions of builtins that rely on the int/long long type is 32/64 bits

2019-05-14 Thread Karl-Johan Karlsson via Phabricator via cfe-commits

Ka-Ka updated this revision to Diff 199385.
Ka-Ka marked an inline comment as done.
Ka-Ka added a comment.

Added a new C++ testcase.
Removed the REQUIRES: avr-registered-target in the avr-builtins.c testcase.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61845

Files:
  include/clang/Basic/Builtins.def
  lib/AST/ASTContext.cpp
  test/CodeGen/avr-builtins.c
  test/CodeGen/builtins.cpp

Index: test/CodeGen/builtins.cpp
===
--- /dev/null
+++ test/CodeGen/builtins.cpp
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-linux-gnu -verify %s
+// RUN: %clang_cc1 -std=c++11 -triple i686-pc-linux-gnu -verify %s
+// RUN: %clang_cc1 -std=c++11 -triple avr-unknown-unknown -verify %s
+// RUN: %clang_cc1 -std=c++11 -triple powerpc-pc-linux -verify %s
+// RUN: %clang_cc1 -std=c++11 -triple arm-linux-gnueabi -verify %s
+
+// Test that checks that the builtins return the same type as the stdint.h type
+// withe the same witdh. This is done by first declaring a variable of a stdint
+// type of the correct width and the redeclaring the variable with the type that
+// the builting return. If the types are different you should the an error from
+// clang of the form:
+// "redefinition of '' with a different type: '' vs ''
+// (with gcc you get an error message
+// "conflicting declaration ' '").
+// If the types are the same you only get an error message of the form
+// "redefinition of ''"
+// with both clang and gcc.
+
+#include 
+
+uint16_t bswap16; // expected-note{{previous definition is here}}
+decltype(__builtin_bswap16(0)) bswap16 = 42; // expected-error-re{{redefinition of 'bswap16'{{$
+uint32_t bswap32; // expected-note{{previous definition is here}}
+decltype(__builtin_bswap32(0)) bswap32 = 42; // expected-error-re{{redefinition of 'bswap32'{{$
+uint64_t bswap64; // expected-note{{previous definition is here}}
+decltype(__builtin_bswap64(0)) bswap64 = 42; // expected-error-re{{redefinition of 'bswap64'{{$
+
+#ifdef __clang__
+uint8_t bitrev8; // expected-note{{previous definition is here}}
+decltype(__builtin_bitreverse8(0)) bitrev8 = 42; // expected-error-re{{redefinition of 'bitrev8'{{$
+uint16_t bitrev16; // expected-note{{previous definition is here}}
+decltype(__builtin_bitreverse16(0)) bitrev16 = 42; // expected-error-re{{redefinition of 'bitrev16'{{$
+uint32_t bitrev32; // expected-note{{previous definition is here}}
+decltype(__builtin_bitreverse32(0)) bitrev32 = 42; // expected-error-re{{redefinition of 'bitrev32'{{$
+uint64_t bitrev64; // expected-note{{previous definition is here}}
+decltype(__builtin_bitreverse64(0)) bitrev64 = 42; // expected-error-re{{redefinition of 'bitrev64'{{$
+
+uint8_t rotl8; // expected-note{{previous definition is here}}
+decltype(__builtin_rotateleft8(0,0)) rotl8 = 42; // expected-error-re{{redefinition of 'rotl8'{{$
+uint16_t rotl16; // expected-note{{previous definition is here}}
+decltype(__builtin_rotateleft16(0,0)) rotl16 = 42; // expected-error-re{{redefinition of 'rotl16'{{$
+uint32_t rotl32; // expected-note{{previous definition is here}}
+decltype(__builtin_rotateleft32(0,0)) rotl32 = 42; // expected-error-re{{redefinition of 'rotl32'{{$
+uint64_t rotl64; // expected-note{{previous definition is here}}
+decltype(__builtin_rotateleft64(0,0)) rotl64 = 42; // expected-error-re{{redefinition of 'rotl64'{{$
+
+uint8_t rotr8; // expected-note{{previous definition is here}}
+decltype(__builtin_rotateright8(0,0)) rotr8 = 42; // expected-error-re{{redefinition of 'rotr8'{{$
+uint16_t rotr16; // expected-note{{previous definition is here}}
+decltype(__builtin_rotateright16(0,0)) rotr16 = 42; // expected-error-re{{redefinition of 'rotr16'{{$
+uint32_t rotr32; // expected-note{{previous definition is here}}
+decltype(__builtin_rotateright32(0,0)) rotr32 = 42; // expected-error-re{{redefinition of 'rotr32'{{$
+uint64_t rotr64; // expected-note{{previous definition is here}}
+decltype(__builtin_rotateright64(0,0)) rotr64 = 42; // expected-error-re{{redefinition of 'rotr64'{{$
+#endif
Index: test/CodeGen/avr-builtins.c
===
--- /dev/null
+++ test/CodeGen/avr-builtins.c
@@ -0,0 +1,102 @@
+// RUN: %clang_cc1 -triple avr-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+
+unsigned char bitrev8(unsigned char data) {
+return __builtin_bitreverse8(data);
+}
+
+// CHECK: define zeroext i8 @bitrev8
+// CHECK: i8 @llvm.bitreverse.i8(i8
+
+unsigned int bitrev16(unsigned int data) {
+return __builtin_bitreverse16(data);
+}
+
+// CHECK: define i16 @bitrev16
+// CHECK: i16 @llvm.bitreverse.i16(i16
+
+unsigned long bitrev32(unsigned long data) {
+return __builtin_bitreverse32(data);
+}
+// CHECK: define i32 @bitrev32
+// CHECK: i32 @llvm.bitreverse.i32(i32
+
+unsigned long long bitrev64(unsigned long long data) {
+

[PATCH] D61643: [PragmaHandler][NFC] Expose `#pragma` location

2019-05-14 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added inline comments.



Comment at: clang/docs/ClangPlugins.rst:58
 ExamplePragmaHandler() : PragmaHandler("example_pragma") { }
-void HandlePragma(Preprocessor &PP, PragmaIntroducerKind Introducer,
+void HandlePragma(Preprocessor &PP, PragmaIntroducer Introducer,
   Token &PragmaTok) {

jdenny wrote:
> lebedev.ri wrote:
> > Hmm.
> > This will have effects on out-of-tree plugins that define pragmas.
> > I'm not sure how to proceed here, just notify cfe-dev and move on?
> We could do something like this in `PragmaHandler`:
> 
> ```
>   virtual void HandlePragma(Preprocessor &PP, PragmaIntroducerKind Introducer,
> Token &FirstToken) {
> llvm_unreachable("deprecated HandlePragma unexpectedly called");
>   }
>   virtual void HandlePragma(Preprocessor &PP, PragmaIntroducer Introducer,
> Token &FirstToken) {
> HandlePragma(PP, Introducer.Kind, FirstToken);
>   }
> ```
> 
> The derived class could override either one.  Unfortunately, if it didn't 
> override either, the error is then at run time instead of compile time as it 
> is now.
> 
> Whether this is worth it, I don't know.  Who's the right person to answer 
> this question?
I think mailing cfe-dev will get this question the most visibility.
Though i //think// the solution will be to go with the current patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61643



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

r360657 - Revert r360637 "PR41817: Fix regression in r359260 that caused the MS compatibility"

2019-05-14 Thread Hans Wennborg via cfe-commits

Author: hans
Date: Tue May 14 03:11:33 2019
New Revision: 360657

URL: http://llvm.org/viewvc/llvm-project?rev=360657&view=rev
Log:
Revert r360637 "PR41817: Fix regression in r359260 that caused the MS 
compatibility"

> extension allowing a "static" declaration to follow an "extern"
> declaration to stop working.

It introduced asserts for some "static-following-extern" cases, breaking the
Chromium build. See the cfe-commits thread for reproducer.

Removed:
cfe/trunk/test/CodeGen/ms-compat-extern-static.c
Modified:
cfe/trunk/lib/AST/Decl.cpp

Modified: cfe/trunk/lib/AST/Decl.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Decl.cpp?rev=360657&r1=360656&r2=360657&view=diff
==
--- cfe/trunk/lib/AST/Decl.cpp (original)
+++ cfe/trunk/lib/AST/Decl.cpp Tue May 14 03:11:33 2019
@@ -613,41 +613,12 @@ static LinkageInfo getExternalLinkageFor
 static StorageClass getStorageClass(const Decl *D) {
   if (auto *TD = dyn_cast(D))
 D = TD->getTemplatedDecl();
-  if (!D)
-return SC_None;
-
-  if (auto *VD = dyn_cast(D)) {
-// Generally, the storage class is determined by the first declaration.
-auto SC = VD->getCanonicalDecl()->getStorageClass();
-
-// ... except that MSVC permits an 'extern' declaration to be redeclared
-// 'static' as an extension.
-if (SC == SC_Extern) {
-  for (auto *Redecl : VD->redecls()) {
-if (Redecl->getStorageClass() == SC_Static)
-  return SC_Static;
-if (Redecl->getStorageClass() != SC_Extern &&
-!Redecl->isLocalExternDecl() && !Redecl->getFriendObjectKind())
-  break;
-  }
-}
-return SC;
+  if (D) {
+if (auto *VD = dyn_cast(D))
+  return VD->getStorageClass();
+if (auto *FD = dyn_cast(D))
+  return FD->getStorageClass();
   }
-
-  if (auto *FD = dyn_cast(D)) {
-auto SC = FD->getCanonicalDecl()->getStorageClass();
-if (SC == SC_Extern) {
-  for (auto *Redecl : FD->redecls()) {
-if (Redecl->getStorageClass() == SC_Static)
-  return SC_Static;
-if (Redecl->getStorageClass() != SC_Extern &&
-!Redecl->isLocalExternDecl() && !Redecl->getFriendObjectKind())
-  break;
-  }
-}
-return SC;
-  }
-
   return SC_None;
 }
 
@@ -663,7 +634,7 @@ LinkageComputer::getLVForNamespaceScopeD
   //   A name having namespace scope (3.3.6) has internal linkage if it
   //   is the name of
 
-  if (getStorageClass(D) == SC_Static) {
+  if (getStorageClass(D->getCanonicalDecl()) == SC_Static) {
 // - a variable, variable template, function, or function template
 //   that is explicitly declared static; or
 // (This bullet corresponds to C99 6.2.2p3.)

Removed: cfe/trunk/test/CodeGen/ms-compat-extern-static.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/ms-compat-extern-static.c?rev=360656&view=auto
==
--- cfe/trunk/test/CodeGen/ms-compat-extern-static.c (original)
+++ cfe/trunk/test/CodeGen/ms-compat-extern-static.c (removed)
@@ -1,11 +0,0 @@
-// RUN: %clang_cc1 -emit-llvm %s -o - -fms-extensions -triple x86_64-windows | 
FileCheck %s
-
-// CHECK: @n = internal global i32 1
-extern int n;
-static int n = 1;
-int *use = &n;
-
-// CHECK: define internal void @f(
-extern void f();
-static void f() {}
-void g() { return f(); }


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

Re: r360637 - PR41817: Fix regression in r359260 that caused the MS compatibility

2019-05-14 Thread Hans Wennborg via cfe-commits

Actually, we didn't notice r359260 in Chromium, however this one
(r360637) caused Clang to start asserting during our builds
(https://bugs.chromium.org/p/chromium/issues/detail?id=962840), here's
a reduced test case:

--
extern int a;
static int *use1 = &a;
int **use2 = &use1;
static int a = 0;
--

$ clang.bad -cc1 -triple i386-pc-windows-msvc19.11.0 -emit-obj
-fms-extensions /tmp/a.cc
clang.bad: /work/llvm.monorepo/clang/lib/AST/Decl.cpp:1494:
clang::LinkageInfo clang::LinkageComputer::getLVForDecl(const
clang::NamedDecl*, clang::LVComputationKind): Assertion
`D->getCachedLinkage() == LV.getLinkage()' failed.


I've reverted this one in r360657 in the meantime.

From: Richard Smith via cfe-commits 
Date: Tue, May 14, 2019 at 2:24 AM
To: 

> Author: rsmith
> Date: Mon May 13 17:27:16 2019
> New Revision: 360637
>
> URL: http://llvm.org/viewvc/llvm-project?rev=360637&view=rev
> Log:
> PR41817: Fix regression in r359260 that caused the MS compatibility
> extension allowing a "static" declaration to follow an "extern"
> declaration to stop working.
>
> Added:
> cfe/trunk/test/CodeGen/ms-compat-extern-static.c
> Modified:
> cfe/trunk/lib/AST/Decl.cpp
>
> Modified: cfe/trunk/lib/AST/Decl.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Decl.cpp?rev=360637&r1=360636&r2=360637&view=diff
> ==
> --- cfe/trunk/lib/AST/Decl.cpp (original)
> +++ cfe/trunk/lib/AST/Decl.cpp Mon May 13 17:27:16 2019
> @@ -613,12 +613,41 @@ static LinkageInfo getExternalLinkageFor
>  static StorageClass getStorageClass(const Decl *D) {
>if (auto *TD = dyn_cast(D))
>  D = TD->getTemplatedDecl();
> -  if (D) {
> -if (auto *VD = dyn_cast(D))
> -  return VD->getStorageClass();
> -if (auto *FD = dyn_cast(D))
> -  return FD->getStorageClass();
> +  if (!D)
> +return SC_None;
> +
> +  if (auto *VD = dyn_cast(D)) {
> +// Generally, the storage class is determined by the first declaration.
> +auto SC = VD->getCanonicalDecl()->getStorageClass();
> +
> +// ... except that MSVC permits an 'extern' declaration to be redeclared
> +// 'static' as an extension.
> +if (SC == SC_Extern) {
> +  for (auto *Redecl : VD->redecls()) {
> +if (Redecl->getStorageClass() == SC_Static)
> +  return SC_Static;
> +if (Redecl->getStorageClass() != SC_Extern &&
> +!Redecl->isLocalExternDecl() && !Redecl->getFriendObjectKind())
> +  break;
> +  }
> +}
> +return SC;
>}
> +
> +  if (auto *FD = dyn_cast(D)) {
> +auto SC = FD->getCanonicalDecl()->getStorageClass();
> +if (SC == SC_Extern) {
> +  for (auto *Redecl : FD->redecls()) {
> +if (Redecl->getStorageClass() == SC_Static)
> +  return SC_Static;
> +if (Redecl->getStorageClass() != SC_Extern &&
> +!Redecl->isLocalExternDecl() && !Redecl->getFriendObjectKind())
> +  break;
> +  }
> +}
> +return SC;
> +  }
> +
>return SC_None;
>  }
>
> @@ -634,7 +663,7 @@ LinkageComputer::getLVForNamespaceScopeD
>//   A name having namespace scope (3.3.6) has internal linkage if it
>//   is the name of
>
> -  if (getStorageClass(D->getCanonicalDecl()) == SC_Static) {
> +  if (getStorageClass(D) == SC_Static) {
>  // - a variable, variable template, function, or function template
>  //   that is explicitly declared static; or
>  // (This bullet corresponds to C99 6.2.2p3.)
>
> Added: cfe/trunk/test/CodeGen/ms-compat-extern-static.c
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/ms-compat-extern-static.c?rev=360637&view=auto
> ==
> --- cfe/trunk/test/CodeGen/ms-compat-extern-static.c (added)
> +++ cfe/trunk/test/CodeGen/ms-compat-extern-static.c Mon May 13 17:27:16 2019
> @@ -0,0 +1,11 @@
> +// RUN: %clang_cc1 -emit-llvm %s -o - -fms-extensions -triple x86_64-windows 
> | FileCheck %s
> +
> +// CHECK: @n = internal global i32 1
> +extern int n;
> +static int n = 1;
> +int *use = &n;
> +
> +// CHECK: define internal void @f(
> +extern void f();
> +static void f() {}
> +void g() { return f(); }
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

2019-05-14 Thread Gabor Marton via Phabricator via cfe-commits

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

Comment at: clang/include/clang/AST/ASTImporter.h:203
 /// context, or the import error.
-llvm::Expected Import_New(TypeSourceInfo *FromTSI);
-// FIXME: Remove this version.
-TypeSourceInfo *Import(TypeSourceInfo *FromTSI);
+llvm::Expected Import(TypeSourceInfo *FromTSI);

aprantl wrote:
> martong wrote:
> > aprantl wrote:
> > > Wouldn't it make more sense to return `Expected`?
> > That would not be consistent with the other changes. In this patch we 
> > change the signature of each functions similarly:
> > From `T foo(...)` to `Expected foo(...)`.
> > Also, `TypeSourceInfo` factory functions in `ASTContext` do return with a 
> > pointer. For example:
> > ```
> >   TypeSourceInfo *CreateTypeSourceInfo(QualType T, unsigned Size = 0) const;
> > 
> >   /// Allocate a TypeSourceInfo where all locations have been
> >   /// initialized to a given location, which defaults to the empty
> >   /// location.
> >   TypeSourceInfo *
> >   getTrivialTypeSourceInfo(QualType T,
> >SourceLocation Loc = SourceLocation()) const;
> > 
> > ```
> I believe that LLVM's practice of passing non-null pointers around is bad API 
> design because it's never quite clear from looking at an API which pointer 
> parameters are nullable, so I was hoping that we could correct some of that 
> at least in the cases where we introduce API that return Expected<> objects 
> to make it obvious that this is either an error or a valid object. If you 
> that the perceived inconsistency weighs stronger than the readability 
> improvements let me know and I can reconsider.
I couldn't agree more that having non-null-able pointers is a bad API, and we'd 
be better to have references in these cases. However, I think the situation is 
more complex than that.

If we changed `TypeSourceInfo *` to `TypeSourceInfo &` in this patch, that 
would involve to make similar changes with other importer functions 
(NestedNameSpecifier *, Decl *, Expr *, etc.). That would result in a **huge** 
patch and I am afraid we could not follow the incremental development 
suggestion in the LLVM dev policy. Thus, even if we decide to change to 
references I propose to do that in a separate patch.

Also, some AST nodes are indeed null-able. E.g. the body of a FunctionDecl 
might be null. Or the described class template of a CXXRecordDecl may be null 
(and we rely on this, because first we create the CXXRecordDecl then we import 
the ClassTemplateDecl and only then we set the relation).
The point is that we should identify those AST nodes which are really 
non-nullable in the AST. This does not seem trivial.
Besides, since the ASTImporter class is part of the AST lib, it would look 
strange if we changed to use references only in the ASTImporter. Perhaps, we 
should change in the whole AST API, but that might broke too many dependencies 
I guess. Still, this may worth an RFC on cfe-dev.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61438

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

[PATCH] D57858: [analyzer] Add a new frontend flag to display all checker options

2019-05-14 Thread Daniel Krupp via Phabricator via cfe-commits

dkrupp added a comment.

In D57858#1500635 , @NoQ wrote:

> In D57858#146 , @dkrupp wrote:
>
> > Some alpha checkers are considerably more mature than others and are quite 
> > usable. In our experience, there are some users who are keen to run these 
> > checkers on their code and report back any false positives to us. So in 
> > this sense these are not "developer only" checkers. So I think we should 
> > let the users list them, read their descriptions and try them out. Some of 
> > them will come back with useful feedback as to how to improve them further.
>
>
> What are such checkers currently? Like, the ones that aren't clearly "missing 
> limbs" and that have somebody happy to //address// feedback sent against them?
>
> Do you have a chance to call out to your users for testing the checker and 
> actively request feedback, as @Szelethus did on the mailing list?
>
> I feel that we could do some sort of "early access checkers" programme, but i 
> believe this would require a more careful PR than just dumping a list of 
> alpha checkers on our users' heads.
>
> In D57858#146 , @dkrupp wrote:
>
> > Some users would not care if the checker gives some more false positives 
> > than the "mature" checkers if they can catch some true positives with them.
>
>
> Yeah, and these are pretty much the users we're trying to protect from 
> themselves :)


These are the alpha checkers that we are testing in Ericsson:
 alpha.core.BoolAssignment
 alpha.core.CastSize
 alpha.core.Conversion
 alpha.core.DynamicTypeChecker
 alpha.core.SizeofPtr
 alpha.core.TestAfterDivZero
 alpha.cplusplus.DeleteWithNonVirtualDtor
 alpha.cplusplus.MisusedMovedObject
 alpha.cplusplus.UninitializedObject
 alpha.security.MallocOverflow
 alpha.security.MmapWriteExec
 alpha.security.ReturnPtrRange
 alpha.security.taint.TaintPropagation
 alpha.unix.BlockInCriticalSection
 alpha.unix.Chroot
 alpha.unix.PthreadLock
 alpha.unix.SimpleStream
 alpha.unix.Stream
 alpha.unix.cstring.NotNullTerminated
 alpha.unix.cstring.OutOfBounds

This 2 have just been moved out of alpha lately:
 alpha.cplusplus.MisusedMovedObject
 alpha.cplusplus.UninitializedObject

According to our tests these checkers do not crash and do not give a large 
number of reports (<~50)  even on large code base.
So we can check for false positives in them one by one. 
Probably these are the closest to come out from alpha. 
We could maybe try to test these checkers one-by-one on large open source code 
bases and move them out from alpha when we are confident enough.


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

https://reviews.llvm.org/D57858



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

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

2019-05-14 Thread Guillaume Chatelet via Phabricator via cfe-commits

gchatelet added a subscriber: tejohnson.
gchatelet added a comment.

In D61634#1500453 , @efriedma wrote:

> My main blocker is that I want to make sure we're moving in the right 
> direction: towards LLVM IR with clear semantics, towards straightforward 
> rules for writing freestanding C code, and towards solutions which behave 
> appropriately for all targets.  There's clearly a problem here that's worth 
> solving, but I want to make sure we're adding small features that interact 
> with existing features in an obvious way.  The patch as written is adding a 
> new attribute that changes the semantics of C and LLVM IR in a subtle way 
> that interacts with existing optimizations and lowering in ways that are 
> complex and hard to understand.


This makes a lot of sense, I'm totally on board to reduce entropy and confusion 
along the way.

> I don't want to mix general restrictions on calling C library functions, with 
> restrictions that apply specifically to memcpy: clang generally works on the 
> assumption that a "memcpy" symbol is available in freestanding environments, 
> and we don't really want to change that.
> 
> With -fno-builtin, specifically, we currently apply the restriction that 
> optimizations will not introduce memcpy calls that would not exist with 
> optimization disabled.  This is sort of artificial, and and maybe a bit 
> confusing, but it seems to work well enough in practice.  gcc does something 
> similar.
> 
> I don't really want to add an attribute that has a different meaning from 
> -fno-builtin.  An attribute that has the same meaning is a lot less 
> problematic: it reduces potential confusion, and solves a related problem 
> that -fno-builtin currently doesn't really work with LTO, because we don't 
> encode it into the IR.

Adding @tejohnson to the conversation.

IIUC you're offering to introduce something like 
`__attribute__((no-builtin-memcpy))` or `__attribute__((no-builtin("memcpy")))`.
As attributes they would compose nicely with (Thin)LTO.

I believe we still want to turn `-fno-builtin` flags into attributes so there's 
no impedance mismatch between the flag and the attribute right?

> Your current patch is using the "AlwaysInline" flag for 
> SelectionDAG::getMemcpy, which forces a memcpy to be lowered to an inline 
> implementation.  In general, this flag is only supported for constant-size 
> memcpy calls; otherwise, on targets where EmitTargetCodeForMemcpy does not 
> have some special lowering, like the x86 "rep;movs", you'll hit an assertion 
> failure.  And we don't really want to add an implementation of 
> variable-length memcpy for a lot of targets; it's very inefficient on targets 
> which don't have unaligned memory access.  You could try to narrowly fix this 
> to only apply "AlwaysInline" if the size is a constant integer, but there's a 
> related problem: even if a memcpy is constant size in C, optimizations don't 
> promise to preserve that, in general.  We could theoretically add such a 
> promise, I guess, but that's awkward: it would conditionally apply to 
> llvm.memcpy where the parameter is already const, so it's not something we 
> can easily verify.

Fair enough. This patch was really to get the conversation started : I was 
myself not especially convinced about the approach. Hijacking the 
`AlwaysInline` parameter was a shortcut that would not work for other mem 
function anyways.

> If we added a new builtin `llvm.memcpy.inline`, or something like that, the 
> end result ends up being simpler in many ways. It has obvious rules for both 
> generating it and lowering it, which don't depend on attributes in the parent 
> function.  We could mark the size as "immarg", so we don't have to add 
> special optimization rules for it.   And if we have it, we have a "complete" 
> solution for writing memcpy implementations in C: we make `__builtin_memcpy`, 
> or a new `__builtin_inline_memcpy`, produce it, and it can be combined with 
> -fno-builtin to ensure we don't produce any calls to the "real" memcpy.  The 
> downside of a new builtin, of course, is that we now have two functions with 
> similar semantics, and potentially have to fix a bunch of places to check for 
> both of them.

This was one of the approaches I envisioned, it's much cleaner and it's also a 
lot more work : )
I'm willing to go that route knowing that it would also work for @theraven's 
use case.

> MemCpyOpt has been mentioned (the pass which transforms memcpy-like loops 
> into llvm.memcpy).  If we want it to perform some transform in circumstances 
> where the call "memcpy" isn't available, we have to make sure to restrict it 
> based on the target: in the worst case, on some targets without unaligned 
> memory access, it could just act as a low-quality loop unroller.  This 
> applies no matter how the result is actually represented in IR.

Yes if we have to generate loops it need to happen before SelectionDAG.



In this frame

r360667 - Add a new language mode for C2x; enable [[attribute]] support by default in C2x.

2019-05-14 Thread Aaron Ballman via cfe-commits

Author: aaronballman
Date: Tue May 14 05:09:55 2019
New Revision: 360667

URL: http://llvm.org/viewvc/llvm-project?rev=360667&view=rev
Log:
Add a new language mode for C2x; enable [[attribute]] support by default in C2x.

Modified:
cfe/trunk/include/clang/Basic/LangOptions.def
cfe/trunk/include/clang/Frontend/LangStandard.h
cfe/trunk/include/clang/Frontend/LangStandards.def
cfe/trunk/lib/Frontend/CompilerInvocation.cpp
cfe/trunk/test/Driver/unknown-std.c
cfe/trunk/test/Parser/c2x-attributes.c
cfe/trunk/test/Sema/attr-cx2.c
cfe/trunk/test/Sema/attr-deprecated-c2x.c
cfe/trunk/test/Sema/c2x-maybe_unused-errors.c
cfe/trunk/test/Sema/c2x-maybe_unused.c
cfe/trunk/test/Sema/c2x-nodiscard.c

Modified: cfe/trunk/include/clang/Basic/LangOptions.def
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/LangOptions.def?rev=360667&r1=360666&r2=360667&view=diff
==
--- cfe/trunk/include/clang/Basic/LangOptions.def (original)
+++ cfe/trunk/include/clang/Basic/LangOptions.def Tue May 14 05:09:55 2019
@@ -82,6 +82,7 @@
 LANGOPT(C99   , 1, 0, "C99")
 LANGOPT(C11   , 1, 0, "C11")
 LANGOPT(C17   , 1, 0, "C17")
+LANGOPT(C2x   , 1, 0, "C2x")
 LANGOPT(MSVCCompat, 1, 0, "Microsoft Visual C++ full compatibility 
mode")
 LANGOPT(MicrosoftExt  , 1, 0, "Microsoft C++ extensions")
 LANGOPT(AsmBlocks , 1, 0, "Microsoft inline asm blocks")

Modified: cfe/trunk/include/clang/Frontend/LangStandard.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/LangStandard.h?rev=360667&r1=360666&r2=360667&view=diff
==
--- cfe/trunk/include/clang/Frontend/LangStandard.h (original)
+++ cfe/trunk/include/clang/Frontend/LangStandard.h Tue May 14 05:09:55 2019
@@ -22,16 +22,17 @@ enum LangFeatures {
   C99 = (1 << 1),
   C11 = (1 << 2),
   C17 = (1 << 3),
-  CPlusPlus = (1 << 4),
-  CPlusPlus11 = (1 << 5),
-  CPlusPlus14 = (1 << 6),
-  CPlusPlus17 = (1 << 7),
-  CPlusPlus2a = (1 << 8),
-  Digraphs = (1 << 9),
-  GNUMode = (1 << 10),
-  HexFloat = (1 << 11),
-  ImplicitInt = (1 << 12),
-  OpenCL = (1 << 13)
+  C2x = (1 << 4),
+  CPlusPlus = (1 << 5),
+  CPlusPlus11 = (1 << 6),
+  CPlusPlus14 = (1 << 7),
+  CPlusPlus17 = (1 << 8),
+  CPlusPlus2a = (1 << 9),
+  Digraphs = (1 << 10),
+  GNUMode = (1 << 11),
+  HexFloat = (1 << 12),
+  ImplicitInt = (1 << 13),
+  OpenCL = (1 << 14)
 };
 
 }
@@ -73,6 +74,9 @@ public:
   /// isC17 - Language is a superset of C17.
   bool isC17() const { return Flags & frontend::C17; }
 
+  /// isC2x - Language is a superset of C2x.
+  bool isC2x() const { return Flags & frontend::C2x; }
+
   /// isCPlusPlus - Language is a C++ variant.
   bool isCPlusPlus() const { return Flags & frontend::CPlusPlus; }
 

Modified: cfe/trunk/include/clang/Frontend/LangStandards.def
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/LangStandards.def?rev=360667&r1=360666&r2=360667&view=diff
==
--- cfe/trunk/include/clang/Frontend/LangStandards.def (original)
+++ cfe/trunk/include/clang/Frontend/LangStandards.def Tue May 14 05:09:55 2019
@@ -88,6 +88,14 @@ LANGSTANDARD(gnu17, "gnu17",
  LineComment | C99 | C11 | C17 | Digraphs | GNUMode | HexFloat)
 LANGSTANDARD_ALIAS(gnu17, "gnu18")
 
+// C2x modes
+LANGSTANDARD(c2x, "c2x",
+ C, "Working Draft for ISO C2x",
+ LineComment | C99 | C11 | C17 | C2x | Digraphs | HexFloat)
+LANGSTANDARD(gnu2x, "gnu2x",
+ C, "Working Draft for ISO C2x with GNU extensions",
+ LineComment | C99 | C11 | C17 | C2x | Digraphs | GNUMode | 
HexFloat)
+
 // C++ modes
 LANGSTANDARD(cxx98, "c++98",
  CXX, "ISO C++ 1998 with amendments",

Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=360667&r1=360666&r2=360667&view=diff
==
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Tue May 14 05:09:55 2019
@@ -2134,6 +2134,7 @@ void CompilerInvocation::setLangDefaults
   Opts.C99 = Std.isC99();
   Opts.C11 = Std.isC11();
   Opts.C17 = Std.isC17();
+  Opts.C2x = Std.isC2x();
   Opts.CPlusPlus = Std.isCPlusPlus();
   Opts.CPlusPlus11 = Std.isCPlusPlus11();
   Opts.CPlusPlus14 = Std.isCPlusPlus14();
@@ -2200,6 +2201,9 @@ void CompilerInvocation::setLangDefaults
   Opts.AlignedAllocation = Opts.CPlusPlus17;
 
   Opts.DollarIdents = !Opts.AsmPreprocessor;
+
+  // Enable [[]] attributes in C++11 and C2x by default.
+  Opts.DoubleSquareBracketAttributes = Opts.CPlusPlus11 || Opts.C2x;
 }
 
 /// Attempt to parse a visibility value out of the giv

[PATCH] D61370: Add a C2x mode and allow attributes in it

2019-05-14 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman closed this revision.
aaron.ballman marked an inline comment as done.
aaron.ballman added a comment.

I've commit in r360667, thanks!




Comment at: lib/Frontend/CompilerInvocation.cpp:2593-2596
+  // Enable [[]] attributes in C++11 and C2x by default.
+  Opts.DoubleSquareBracketAttributes = Args.hasFlag(
+  OPT_fdouble_square_bracket_attributes,
+  OPT_fno_double_square_bracket_attributes, Opts.CPlusPlus11 || Opts.C2x);

rsmith wrote:
> (We get this wrong in a bunch of other places, but...)
> 
> The default from the language mode should be set by `setLangDefault`, and 
> here we should override it if suitable flags are set.
Ah, good to know. I went ahead and fixed this for the commit.


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

https://reviews.llvm.org/D61370



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

[PATCH] D61788: Make getObjCEncodingForTypeImpl() take a bitmask instead of 8 bools

2019-05-14 Thread Nico Weber via Phabricator via cfe-commits

thakis updated this revision to Diff 199408.
thakis added a comment.

minor tweaks for landing


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

https://reviews.llvm.org/D61788

Files:
  clang/include/clang/AST/ASTContext.h
  clang/lib/AST/ASTContext.cpp

Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -6311,13 +6311,13 @@
   // Encode type qualifer, 'in', 'inout', etc. for the parameter.
   getObjCEncodingForTypeQualifier(QT, S);
   // Encode parameter type.
-  getObjCEncodingForTypeImpl(T, S, /*ExpandPointedToStructures=*/true,
- /*ExpandStructures=*/true, /*Field=*/nullptr,
- /*OutermostType=*/true,
- /*EncodingProperty=*/false,
- /*StructField=*/false,
- /*EncodeBlockParameters=*/Extended,
- /*EncodeClassNames=*/Extended);
+  ObjCEncOptions Options = ObjCEncOptions()
+   .setExpandPointedToStructures()
+   .setExpandStructures()
+   .setIsOutermostType();
+  if (Extended)
+Options.setEncodeBlockParameters().setEncodeClassNames();
+  getObjCEncodingForTypeImpl(T, S, Options, /*Field=*/nullptr);
 }
 
 /// getObjCEncodingForMethodDecl - Return the encoded type for this method
@@ -6509,13 +6509,12 @@
   // directly pointed to, and expanding embedded structures. Note that
   // these rules are sufficient to prevent recursive encoding of the
   // same type.
-  getObjCEncodingForTypeImpl(T, S, /*ExpandPointedToStructures=*/true,
- /*ExpandStructures=*/true, Field,
- /*OutermostType=*/true, /*EncodingProperty=*/false,
- /*StructField=*/false,
- /*EncodeBlockParameters=*/false,
- /*EncodeClassNames=*/false,
- /*EncodePointerToObjCTypedef=*/false, NotEncodedT);
+  getObjCEncodingForTypeImpl(T, S,
+ ObjCEncOptions()
+ .setExpandPointedToStructures()
+ .setExpandStructures()
+ .setIsOutermostType(),
+ Field, NotEncodedT);
 }
 
 void ASTContext::getObjCEncodingForPropertyType(QualType T,
@@ -6523,9 +6522,13 @@
   // Encode result type.
   // GCC has some special rules regarding encoding of properties which
   // closely resembles encoding of ivars.
-  getObjCEncodingForTypeImpl(
-  T, S, /*ExpandPointedToStructures=*/true, /*ExpandStructures=*/true,
-  /*Field=*/nullptr, /*OutermostType=*/true, /*EncodingProperty=*/true);
+  getObjCEncodingForTypeImpl(T, S,
+ ObjCEncOptions()
+ .setExpandPointedToStructures()
+ .setExpandStructures()
+ .setIsOutermostType()
+ .setEncodingProperty(),
+ /*Field=*/nullptr);
 }
 
 static char getObjCEncodingForPrimitiveKind(const ASTContext *C,
@@ -6672,16 +6675,9 @@
 }
 
 // FIXME: Use SmallString for accumulating string.
-void ASTContext::getObjCEncodingForTypeImpl(QualType T, std::string& S,
-bool ExpandPointedToStructures,
-bool ExpandStructures,
+void ASTContext::getObjCEncodingForTypeImpl(QualType T, std::string &S,
+const ObjCEncOptions Options,
 const FieldDecl *FD,
-bool OutermostType,
-bool EncodingProperty,
-bool StructField,
-bool EncodeBlockParameters,
-bool EncodeClassNames,
-bool EncodePointerToObjCTypedef,
 QualType *NotEncodedT) const {
   CanQualType CT = getCanonicalType(T);
   switch (CT->getTypeClass()) {
@@ -6698,18 +6694,16 @@
   case Type::Complex: {
 const auto *CT = T->castAs();
 S += 'j';
-getObjCEncodingForTypeImpl(CT->getElementType(), S,
-   /*ExpandPointedToStructures=*/false,
-   /*ExpandStructures=*/false, /*Field=*/nullptr);
+getObjCEncodingForTypeImpl(CT->getElementType(), S, ObjCEncOptions(),
+   /*Field=*/nullptr);
 return;
   }
 
   case Type::Atomic: {
 const auto *AT = T->castAs();
 S += 'A';
-getObjCEncodingForTypeImpl(AT->getValueType(), S,
-

r360668 - Make getObjCEncodingForTypeImpl() take a bitmask instead of 8 bools

2019-05-14 Thread Nico Weber via cfe-commits

Author: nico
Date: Tue May 14 05:32:37 2019
New Revision: 360668

URL: http://llvm.org/viewvc/llvm-project?rev=360668&view=rev
Log:
Make getObjCEncodingForTypeImpl() take a bitmask instead of 8 bools

Slightly easier to read, uses slightly less stack space, and makes it
impossible to mix up the order of all those bools.

No behavior change.

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

Modified:
cfe/trunk/include/clang/AST/ASTContext.h
cfe/trunk/lib/AST/ASTContext.cpp

Modified: cfe/trunk/include/clang/AST/ASTContext.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTContext.h?rev=360668&r1=360667&r2=360668&view=diff
==
--- cfe/trunk/include/clang/AST/ASTContext.h (original)
+++ cfe/trunk/include/clang/AST/ASTContext.h Tue May 14 05:32:37 2019
@@ -2864,18 +2864,53 @@ public:
 private:
   void InitBuiltinType(CanQualType &R, BuiltinType::Kind K);
 
+  class ObjCEncOptions {
+unsigned Bits;
+
+ObjCEncOptions(unsigned Bits) : Bits(Bits) {}
+
+  public:
+ObjCEncOptions() : Bits(0) {}
+ObjCEncOptions(const ObjCEncOptions &RHS) : Bits(RHS.Bits) {}
+
+#define OPT_LIST(V)
\
+  V(ExpandPointedToStructures, 0)  
\
+  V(ExpandStructures, 1)   
\
+  V(IsOutermostType, 2)
\
+  V(EncodingProperty, 3)   
\
+  V(IsStructField, 4)  
\
+  V(EncodeBlockParameters, 5)  
\
+  V(EncodeClassNames, 6)   
\
+  V(EncodePointerToObjCTypedef, 7)
+
+#define V(N,I) ObjCEncOptions& set##N() { Bits |= 1 << I; return *this; }
+OPT_LIST(V)
+#undef V
+
+#define V(N,I) bool N() const { return Bits & 1 << I; }
+OPT_LIST(V)
+#undef V
+
+#undef OPT_LIST
+
+LLVM_NODISCARD ObjCEncOptions keepingOnly(ObjCEncOptions Mask) const {
+  return Bits & Mask.Bits;
+}
+
+LLVM_NODISCARD ObjCEncOptions forComponentType() const {
+  ObjCEncOptions Mask = ObjCEncOptions()
+.setIsOutermostType()
+.setIsStructField()
+.setEncodePointerToObjCTypedef();
+  return Bits & ~Mask.Bits;
+}
+  };
+
   // Return the Objective-C type encoding for a given type.
   void getObjCEncodingForTypeImpl(QualType t, std::string &S,
-  bool ExpandPointedToStructures,
-  bool ExpandStructures,
+  ObjCEncOptions Options,
   const FieldDecl *Field,
-  bool OutermostType = false,
-  bool EncodingProperty = false,
-  bool StructField = false,
-  bool EncodeBlockParameters = false,
-  bool EncodeClassNames = false,
-  bool EncodePointerToObjCTypedef = false,
-  QualType *NotEncodedT=nullptr) const;
+  QualType *NotEncodedT = nullptr) const;
 
   // Adds the encoding of the structure's members.
   void getObjCEncodingForStructureImpl(RecordDecl *RD, std::string &S,

Modified: cfe/trunk/lib/AST/ASTContext.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTContext.cpp?rev=360668&r1=360667&r2=360668&view=diff
==
--- cfe/trunk/lib/AST/ASTContext.cpp (original)
+++ cfe/trunk/lib/AST/ASTContext.cpp Tue May 14 05:32:37 2019
@@ -6311,13 +6311,13 @@ void ASTContext::getObjCEncodingForMetho
   // Encode type qualifer, 'in', 'inout', etc. for the parameter.
   getObjCEncodingForTypeQualifier(QT, S);
   // Encode parameter type.
-  getObjCEncodingForTypeImpl(T, S, /*ExpandPointedToStructures=*/true,
- /*ExpandStructures=*/true, /*Field=*/nullptr,
- /*OutermostType=*/true,
- /*EncodingProperty=*/false,
- /*StructField=*/false,
- /*EncodeBlockParameters=*/Extended,
- /*EncodeClassNames=*/Extended);
+  ObjCEncOptions Options = ObjCEncOptions()
+   .setExpandPointedToStructures()
+   .setExpandStructures()
+   .setIsOutermostType();
+  if (Extended)
+Options.setEncodeBlockParameters().setEncodeClassNames();
+  getObjCEncodingForTypeImpl(T, S, Options, /*Field=*/nullptr);
 }
 
 /// getObjCEncodingForMethodDecl - Return the

[PATCH] D61788: Make getObjCEncodingForTypeImpl() take a bitmask instead of 8 bools

2019-05-14 Thread 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 rL360668: Make getObjCEncodingForTypeImpl() take a bitmask 
instead of 8 bools (authored by nico, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D61788?vs=199408&id=199409#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D61788

Files:
  cfe/trunk/include/clang/AST/ASTContext.h
  cfe/trunk/lib/AST/ASTContext.cpp

Index: cfe/trunk/include/clang/AST/ASTContext.h
===
--- cfe/trunk/include/clang/AST/ASTContext.h
+++ cfe/trunk/include/clang/AST/ASTContext.h
@@ -2864,18 +2864,53 @@
 private:
   void InitBuiltinType(CanQualType &R, BuiltinType::Kind K);
 
+  class ObjCEncOptions {
+unsigned Bits;
+
+ObjCEncOptions(unsigned Bits) : Bits(Bits) {}
+
+  public:
+ObjCEncOptions() : Bits(0) {}
+ObjCEncOptions(const ObjCEncOptions &RHS) : Bits(RHS.Bits) {}
+
+#define OPT_LIST(V)\
+  V(ExpandPointedToStructures, 0)  \
+  V(ExpandStructures, 1)   \
+  V(IsOutermostType, 2)\
+  V(EncodingProperty, 3)   \
+  V(IsStructField, 4)  \
+  V(EncodeBlockParameters, 5)  \
+  V(EncodeClassNames, 6)   \
+  V(EncodePointerToObjCTypedef, 7)
+
+#define V(N,I) ObjCEncOptions& set##N() { Bits |= 1 << I; return *this; }
+OPT_LIST(V)
+#undef V
+
+#define V(N,I) bool N() const { return Bits & 1 << I; }
+OPT_LIST(V)
+#undef V
+
+#undef OPT_LIST
+
+LLVM_NODISCARD ObjCEncOptions keepingOnly(ObjCEncOptions Mask) const {
+  return Bits & Mask.Bits;
+}
+
+LLVM_NODISCARD ObjCEncOptions forComponentType() const {
+  ObjCEncOptions Mask = ObjCEncOptions()
+.setIsOutermostType()
+.setIsStructField()
+.setEncodePointerToObjCTypedef();
+  return Bits & ~Mask.Bits;
+}
+  };
+
   // Return the Objective-C type encoding for a given type.
   void getObjCEncodingForTypeImpl(QualType t, std::string &S,
-  bool ExpandPointedToStructures,
-  bool ExpandStructures,
+  ObjCEncOptions Options,
   const FieldDecl *Field,
-  bool OutermostType = false,
-  bool EncodingProperty = false,
-  bool StructField = false,
-  bool EncodeBlockParameters = false,
-  bool EncodeClassNames = false,
-  bool EncodePointerToObjCTypedef = false,
-  QualType *NotEncodedT=nullptr) const;
+  QualType *NotEncodedT = nullptr) const;
 
   // Adds the encoding of the structure's members.
   void getObjCEncodingForStructureImpl(RecordDecl *RD, std::string &S,
Index: cfe/trunk/lib/AST/ASTContext.cpp
===
--- cfe/trunk/lib/AST/ASTContext.cpp
+++ cfe/trunk/lib/AST/ASTContext.cpp
@@ -6311,13 +6311,13 @@
   // Encode type qualifer, 'in', 'inout', etc. for the parameter.
   getObjCEncodingForTypeQualifier(QT, S);
   // Encode parameter type.
-  getObjCEncodingForTypeImpl(T, S, /*ExpandPointedToStructures=*/true,
- /*ExpandStructures=*/true, /*Field=*/nullptr,
- /*OutermostType=*/true,
- /*EncodingProperty=*/false,
- /*StructField=*/false,
- /*EncodeBlockParameters=*/Extended,
- /*EncodeClassNames=*/Extended);
+  ObjCEncOptions Options = ObjCEncOptions()
+   .setExpandPointedToStructures()
+   .setExpandStructures()
+   .setIsOutermostType();
+  if (Extended)
+Options.setEncodeBlockParameters().setEncodeClassNames();
+  getObjCEncodingForTypeImpl(T, S, Options, /*Field=*/nullptr);
 }
 
 /// getObjCEncodingForMethodDecl - Return the encoded type for this method
@@ -6509,13 +6509,12 @@
   // directly pointed to, and expanding embedded structures. Note that
   // these rules are sufficient to prevent recursive encoding of the
   // same type.
-  getObjCEncodingForTypeI

[PATCH] D61874: [clang-tidy] Fix invalid fixit for readability-static-accessed-through-instance (bug 40544)

2019-05-14 Thread 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.

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61874



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

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

2019-05-14 Thread Teresa Johnson via Phabricator via cfe-commits

tejohnson added a comment.

In D61634#1501066 , @gchatelet wrote:

> In D61634#1500453 , @efriedma wrote:
>
> > My main blocker is that I want to make sure we're moving in the right 
> > direction: towards LLVM IR with clear semantics, towards straightforward 
> > rules for writing freestanding C code, and towards solutions which behave 
> > appropriately for all targets.  There's clearly a problem here that's worth 
> > solving, but I want to make sure we're adding small features that interact 
> > with existing features in an obvious way.  The patch as written is adding a 
> > new attribute that changes the semantics of C and LLVM IR in a subtle way 
> > that interacts with existing optimizations and lowering in ways that are 
> > complex and hard to understand.
>
>
> This makes a lot of sense, I'm totally on board to reduce entropy and 
> confusion along the way.
>
> > I don't want to mix general restrictions on calling C library functions, 
> > with restrictions that apply specifically to memcpy: clang generally works 
> > on the assumption that a "memcpy" symbol is available in freestanding 
> > environments, and we don't really want to change that.
> > 
> > With -fno-builtin, specifically, we currently apply the restriction that 
> > optimizations will not introduce memcpy calls that would not exist with 
> > optimization disabled.  This is sort of artificial, and and maybe a bit 
> > confusing, but it seems to work well enough in practice.  gcc does 
> > something similar.
> > 
> > I don't really want to add an attribute that has a different meaning from 
> > -fno-builtin.  An attribute that has the same meaning is a lot less 
> > problematic: it reduces potential confusion, and solves a related problem 
> > that -fno-builtin currently doesn't really work with LTO, because we don't 
> > encode it into the IR.
>
> Adding @tejohnson to the conversation.
>
> IIUC you're offering to introduce something like 
> `__attribute__((no-builtin-memcpy))` or 
> `__attribute__((no-builtin("memcpy")))`.
>  As attributes they would compose nicely with (Thin)LTO.
>
> I believe we still want to turn `-fno-builtin` flags into attributes so 
> there's no impedance mismatch between the flag and the attribute right?


I have a related patch that turns -fno-builtin* options into module flags, and 
uses them in the LTO backends. This addresses current issues with these flags 
not working well with LTO.
See https://reviews.llvm.org/D60162


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61634



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

r360674 - [Driver][Windows] Add dependent lib argument for profile instr generate

2019-05-14 Thread Russell Gallop via cfe-commits

Author: russell_gallop
Date: Tue May 14 07:01:40 2019
New Revision: 360674

URL: http://llvm.org/viewvc/llvm-project?rev=360674&view=rev
Log:
[Driver][Windows] Add dependent lib argument for profile instr generate

This is needed so lld-link can find clang_rt.profile when self hosting
on Windows with PGO. Using clang-cl as a linker knows to add the library
but self hosting, using -DCMAKE_LINKER=<...>/lld-link.exe doesn't.

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

Modified:
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
cfe/trunk/test/Driver/cl-options.c
cfe/trunk/test/Driver/instrprof-ld.c

Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=360674&r1=360673&r2=360674&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Tue May 14 07:01:40 2019
@@ -731,8 +731,9 @@ static void appendUserToPath(SmallVector
   Result.append(UID.begin(), UID.end());
 }
 
-static void addPGOAndCoverageFlags(Compilation &C, const Driver &D,
-   const InputInfo &Output, const ArgList 
&Args,
+static void addPGOAndCoverageFlags(const ToolChain &TC, Compilation &C,
+   const Driver &D, const InputInfo &Output,
+   const ArgList &Args,
ArgStringList &CmdArgs) {
 
   auto *PGOGenerateArg = Args.getLastArg(options::OPT_fprofile_generate,
@@ -783,6 +784,11 @@ static void addPGOAndCoverageFlags(Compi
ProfileGenerateArg->getValue()));
 // The default is to use Clang Instrumentation.
 CmdArgs.push_back("-fprofile-instrument=clang");
+if (TC.getTriple().isWindowsMSVCEnvironment()) {
+  // Add dependent lib for clang_rt.profile
+  CmdArgs.push_back(Args.MakeArgString("--dependent-lib=" +
+   TC.getCompilerRT(Args, "profile")));
+}
   }
 
   Arg *PGOGenArg = nullptr;
@@ -4176,7 +4182,7 @@ void Clang::ConstructJob(Compilation &C,
   // sampling, overhead of call arc collection is way too high and there's no
   // way to collect the output.
   if (!Triple.isNVPTX())
-addPGOAndCoverageFlags(C, D, Output, Args, CmdArgs);
+addPGOAndCoverageFlags(TC, C, D, Output, Args, CmdArgs);
 
   if (auto *ABICompatArg = Args.getLastArg(options::OPT_fclang_abi_compat_EQ))
 ABICompatArg->render(Args, CmdArgs);

Modified: cfe/trunk/test/Driver/cl-options.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/cl-options.c?rev=360674&r1=360673&r2=360674&view=diff
==
--- cfe/trunk/test/Driver/cl-options.c (original)
+++ cfe/trunk/test/Driver/cl-options.c Tue May 14 07:01:40 2019
@@ -66,7 +66,7 @@
 // RUN: %clang_cl -### /FA -fprofile-instr-generate=/tmp/somefile.profraw -- 
%s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-GENERATE-FILE %s
 // RUN: %clang_cl -### /FA -fprofile-instr-generate -fprofile-instr-use -- %s 
2>&1 | FileCheck -check-prefix=CHECK-NO-MIX-GEN-USE %s
 // RUN: %clang_cl -### /FA -fprofile-instr-generate -fprofile-instr-use=file 
-- %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MIX-GEN-USE %s
-// CHECK-PROFILE-GENERATE: "-fprofile-instrument=clang"
+// CHECK-PROFILE-GENERATE: "-fprofile-instrument=clang" 
"--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.lib"
 // CHECK-PROFILE-GENERATE-FILE: 
"-fprofile-instrument-path=/tmp/somefile.profraw"
 // CHECK-NO-MIX-GEN-USE: '{{[a-z=-]*}}' not allowed with '{{[a-z=-]*}}'
 

Modified: cfe/trunk/test/Driver/instrprof-ld.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/instrprof-ld.c?rev=360674&r1=360673&r2=360674&view=diff
==
--- cfe/trunk/test/Driver/instrprof-ld.c (original)
+++ cfe/trunk/test/Driver/instrprof-ld.c Tue May 14 07:01:40 2019
@@ -129,3 +129,17 @@
 //
 // CHECK-MINGW-X86-64: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
 // CHECK-MINGW-X86-64: 
"{{.*}}/Inputs/resource_dir{{/|}}lib{{/|}}windows{{/|}}libclang_rt.profile-x86_64.a"
+
+// Test instrumented profiling dependent-lib flags
+//
+// RUN: %clang %s -### -o %t.o -target x86_64-pc-win32 \
+// RUN: -fprofile-instr-generate 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-WINDOWS-X86-64-DEPENDENT-LIB %s
+//
+// CHECK-WINDOWS-X86-64-DEPENDENT-LIB: 
"--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.lib"
+//
+// RUN: %clang %s -### -o %t.o -target x86_64-mingw32 \
+// RUN: -fprofile-instr-generate 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-MINGW-X86-64-DEPENDENT-LIB %s
+//
+// CHECK-MINGW-X86-64-DEPENDENT-LIB-NOT: 
"--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.a"


___
cfe-commits mailing list
cfe-co

[PATCH] D61742: [Driver][Windows] Add dependent lib argument for profile instr generate

2019-05-14 Thread Phabricator via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rC360674: [Driver][Windows] Add dependent lib argument for 
profile instr generate (authored by russell_gallop, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D61742?vs=199016&id=199436#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D61742

Files:
  lib/Driver/ToolChains/Clang.cpp
  test/Driver/cl-options.c
  test/Driver/instrprof-ld.c


Index: test/Driver/instrprof-ld.c
===
--- test/Driver/instrprof-ld.c
+++ test/Driver/instrprof-ld.c
@@ -129,3 +129,17 @@
 //
 // CHECK-MINGW-X86-64: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
 // CHECK-MINGW-X86-64: 
"{{.*}}/Inputs/resource_dir{{/|}}lib{{/|}}windows{{/|}}libclang_rt.profile-x86_64.a"
+
+// Test instrumented profiling dependent-lib flags
+//
+// RUN: %clang %s -### -o %t.o -target x86_64-pc-win32 \
+// RUN: -fprofile-instr-generate 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-WINDOWS-X86-64-DEPENDENT-LIB %s
+//
+// CHECK-WINDOWS-X86-64-DEPENDENT-LIB: 
"--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.lib"
+//
+// RUN: %clang %s -### -o %t.o -target x86_64-mingw32 \
+// RUN: -fprofile-instr-generate 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-MINGW-X86-64-DEPENDENT-LIB %s
+//
+// CHECK-MINGW-X86-64-DEPENDENT-LIB-NOT: 
"--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.a"
Index: test/Driver/cl-options.c
===
--- test/Driver/cl-options.c
+++ test/Driver/cl-options.c
@@ -66,7 +66,7 @@
 // RUN: %clang_cl -### /FA -fprofile-instr-generate=/tmp/somefile.profraw -- 
%s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-GENERATE-FILE %s
 // RUN: %clang_cl -### /FA -fprofile-instr-generate -fprofile-instr-use -- %s 
2>&1 | FileCheck -check-prefix=CHECK-NO-MIX-GEN-USE %s
 // RUN: %clang_cl -### /FA -fprofile-instr-generate -fprofile-instr-use=file 
-- %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MIX-GEN-USE %s
-// CHECK-PROFILE-GENERATE: "-fprofile-instrument=clang"
+// CHECK-PROFILE-GENERATE: "-fprofile-instrument=clang" 
"--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.lib"
 // CHECK-PROFILE-GENERATE-FILE: 
"-fprofile-instrument-path=/tmp/somefile.profraw"
 // CHECK-NO-MIX-GEN-USE: '{{[a-z=-]*}}' not allowed with '{{[a-z=-]*}}'
 
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -731,8 +731,9 @@
   Result.append(UID.begin(), UID.end());
 }
 
-static void addPGOAndCoverageFlags(Compilation &C, const Driver &D,
-   const InputInfo &Output, const ArgList 
&Args,
+static void addPGOAndCoverageFlags(const ToolChain &TC, Compilation &C,
+   const Driver &D, const InputInfo &Output,
+   const ArgList &Args,
ArgStringList &CmdArgs) {
 
   auto *PGOGenerateArg = Args.getLastArg(options::OPT_fprofile_generate,
@@ -783,6 +784,11 @@
ProfileGenerateArg->getValue()));
 // The default is to use Clang Instrumentation.
 CmdArgs.push_back("-fprofile-instrument=clang");
+if (TC.getTriple().isWindowsMSVCEnvironment()) {
+  // Add dependent lib for clang_rt.profile
+  CmdArgs.push_back(Args.MakeArgString("--dependent-lib=" +
+   TC.getCompilerRT(Args, "profile")));
+}
   }
 
   Arg *PGOGenArg = nullptr;
@@ -4176,7 +4182,7 @@
   // sampling, overhead of call arc collection is way too high and there's no
   // way to collect the output.
   if (!Triple.isNVPTX())
-addPGOAndCoverageFlags(C, D, Output, Args, CmdArgs);
+addPGOAndCoverageFlags(TC, C, D, Output, Args, CmdArgs);
 
   if (auto *ABICompatArg = Args.getLastArg(options::OPT_fclang_abi_compat_EQ))
 ABICompatArg->render(Args, CmdArgs);


Index: test/Driver/instrprof-ld.c
===
--- test/Driver/instrprof-ld.c
+++ test/Driver/instrprof-ld.c
@@ -129,3 +129,17 @@
 //
 // CHECK-MINGW-X86-64: "{{(.*[^.0-9A-Z_a-z])?}}ld{{(.exe)?}}"
 // CHECK-MINGW-X86-64: "{{.*}}/Inputs/resource_dir{{/|}}lib{{/|}}windows{{/|}}libclang_rt.profile-x86_64.a"
+
+// Test instrumented profiling dependent-lib flags
+//
+// RUN: %clang %s -### -o %t.o -target x86_64-pc-win32 \
+// RUN: -fprofile-instr-generate 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-WINDOWS-X86-64-DEPENDENT-LIB %s
+//
+// CHECK-WINDOWS-X86-64-DEPENDENT-LIB: "--dependent-lib={{[^"]*}}clang_rt.profile-{{[^"]*}}.lib"
+//
+// RUN: %clang %s -### -o %t.o -target x86_64-mingw32 \
+// RUN: -fprofile-instr-generate 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-MINGW-X86-64-DEPENDENT-LIB %s
+//
+// C

r360681 - [Sema] CodeSynthesisContext - add missing variable initialization to constructor. NFCI.

2019-05-14 Thread Simon Pilgrim via cfe-commits

Author: rksimon
Date: Tue May 14 07:58:47 2019
New Revision: 360681

URL: http://llvm.org/viewvc/llvm-project?rev=360681&view=rev
Log:
[Sema] CodeSynthesisContext - add missing variable initialization to 
constructor. NFCI.

SavedInNonInstantiationSFINAEContext isn't used outside of specific contexts 
but this fixes cppcheck and scan-build warnings.

Modified:
cfe/trunk/include/clang/Sema/Sema.h

Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=360681&r1=360680&r2=360681&view=diff
==
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Tue May 14 07:58:47 2019
@@ -7420,8 +7420,10 @@ public:
 SourceRange InstantiationRange;
 
 CodeSynthesisContext()
-  : Kind(TemplateInstantiation), Entity(nullptr), Template(nullptr),
-TemplateArgs(nullptr), NumTemplateArgs(0), DeductionInfo(nullptr) {}
+: Kind(TemplateInstantiation),
+  SavedInNonInstantiationSFINAEContext(false), Entity(nullptr),
+  Template(nullptr), TemplateArgs(nullptr), NumTemplateArgs(0),
+  DeductionInfo(nullptr) {}
 
 /// Determines whether this template is an actual instantiation
 /// that should be counted toward the maximum instantiation depth.


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

RE: r360452 - Replace 'REQUIRES: nozlib' with '!zlib' because we don't need two ways

2019-05-14 Thread via cfe-commits

There's no practical difference.  I view it as a matter of documentation of 
intent, see my commit log comment for r360603.
Arguably we could eliminate UNSUPPORTED and move all the expressions into 
REQUIRES (appropriately negated), but I'm not sure that's a net win for 
readability.
--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Monday, May 13, 2019 10:48 AM
To: Robinson, Paul
Cc: cfe-commits
Subject: Re: r360452 - Replace 'REQUIRES: nozlib' with '!zlib' because we don't 
need two ways

What's the practical difference between "UNSUPPORTED: foo" and "REQUIRES: 
!foo"? (I see some of the fixes you've made go one way, some the other way)

On Fri, May 10, 2019 at 11:30 AM Paul Robinson via cfe-commits 
mailto:cfe-commits@lists.llvm.org>> wrote:
Author: probinson
Date: Fri May 10 11:32:53 2019
New Revision: 360452

URL: http://llvm.org/viewvc/llvm-project?rev=360452&view=rev
Log:
Replace 'REQUIRES: nozlib' with '!zlib' because we don't need two ways
to say the same thing.

Modified:
cfe/trunk/test/Driver/nozlibcompress.c

Modified: cfe/trunk/test/Driver/nozlibcompress.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/nozlibcompress.c?rev=360452&r1=360451&r2=360452&view=diff
==
--- cfe/trunk/test/Driver/nozlibcompress.c (original)
+++ cfe/trunk/test/Driver/nozlibcompress.c Fri May 10 11:32:53 2019
@@ -1,4 +1,4 @@
-// REQUIRES: nozlib
+// REQUIRES: !zlib

 // RUN: %clang -### -fintegrated-as -gz -c %s 2>&1 | FileCheck %s 
-check-prefix CHECK-WARN
 // RUN: %clang -### -fintegrated-as -gz=none -c %s 2>&1 | FileCheck 
-allow-empty -check-prefix CHECK-NOWARN %s


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

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

2019-05-14 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:196
+/// Joins multiple rules into a single rule that applies the first rule in
+/// `Rules` whose pattern matches a given node. All of the rules must use the
+/// same kind of matcher (that is, share a base class in the AST hierarchy).

> All of the rules must use the same kind of matcher

Could you elaborate why we want this restriction?
Why they can't be of different kinds? 

It feels ok to have transformations to completely unrelated nodes and still 
apply the first one.




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:224
+//
+// RewriteRule R = makeOrderedRule({makeRule(two_calls, two_calls_action)},
+// {makeRule(left_call, left_call_action)},

Why would we need braces around each call? Aren't they a rewrite rule in 
themselves?



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:232
+// `makeOrderedRule({makeRule(m1, action1), makeRule(m2, action2), ...});`
+RewriteRule makeOrderedRule(ArrayRef Rules);
+

Any ideas for a better name? `pickFirst`  or `applyFirst`?



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:240
 
+/// The following three functions function are a low-level part of the API,
+/// provided to support interpretation of a \c RewriteRule in a tool, like \c

Could they be made private? If not, why?



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:247
+
+/// Returns the \c Action of \c Rule that was selected in the match result.
+const RewriteRule::Case &

s/Action/Case? Or am I missing something?



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:182
+// Determines whether A is higher than B in the class hierarchy.
+static bool isHigher(ASTNodeKind A, ASTNodeKind B) {
+  static auto QualKind = ASTNodeKind::getFromNodeKind();

NIT: maybe name it `isBaseOf`? When talking about class hierarchies, using 
'base' and 'derived' is a common convention.



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:250
+  assert(!CommonKind.isNone() && "Cases must have compatible matchers.");
+  return DynTypedMatcher::constructVariadic(
+  DynTypedMatcher::VO_AnyOf, CommonKind, taggedMatchers("Tag", 
Rule.Cases));

I wonder if there a better way to construct an `anyOf` matcher that can tell 
which case matched...
It looks to be the reason for a more complicated interface on the transformer 
side, but I'm not sure there's a way out of it.

Any ideas?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61335



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

[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

2019-05-14 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added a comment.

@aaron.ballman, would be a good reviewer for this? I'm happy to look at the 
transformer bits, but I'm definitely the wrong one to asses it from the  
`clang-tidy` perspective.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386



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

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

2019-05-14 Thread Yitzhak Mandelbaum via Phabricator via cfe-commits

ymandel marked an inline comment as done.
ymandel added a comment.

Agreed on all the comments -- just one question:




Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:250
+  assert(!CommonKind.isNone() && "Cases must have compatible matchers.");
+  return DynTypedMatcher::constructVariadic(
+  DynTypedMatcher::VO_AnyOf, CommonKind, taggedMatchers("Tag", 
Rule.Cases));

ilya-biryukov wrote:
> I wonder if there a better way to construct an `anyOf` matcher that can tell 
> which case matched...
> It looks to be the reason for a more complicated interface on the transformer 
> side, but I'm not sure there's a way out of it.
> 
> Any ideas?
I don't quite follow. Which interface complication are you referring to?  FWIW, 
the reason the code here doesn't just use the anyOf() matches is that it 
doesn't take a vector -- it only has a variadic form.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61335



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

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

2019-05-14 Thread Adrian Prantl via Phabricator via cfe-commits

aprantl accepted this revision.
aprantl added a comment.

Alright, thanks for taking the time to walk me through the thought process!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61438



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

[PATCH] D61909: Add Clang shared library with C++ exports

2019-05-14 Thread Chris Bieneman via Phabricator via cfe-commits

beanz created this revision.
beanz added reviewers: tstellar, winksaville.
Herald added a subscriber: mgorny.
Herald added a project: clang.

This patch adds a libClang_shared library on *nix systems which exports the 
entire C++ API. In order to support this on Windows we should really refactor 
llvm-shlib and share code between the two.

This also uses a slightly different method for generating the shared library, 
which I should back-port to llvm-shlib. Instead of linking the static archives 
and passing linker flags to force loading the whole libraries, this patch 
creates object libraries for every library (which has no cost in the build 
system), and link the object libraries.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61909

Files:
  clang/cmake/modules/AddClang.cmake
  clang/tools/CMakeLists.txt
  clang/tools/clang-shlib/CMakeLists.txt
  clang/tools/clang-shlib/clang-shlib.cpp


Index: clang/tools/clang-shlib/clang-shlib.cpp
===
--- /dev/null
+++ clang/tools/clang-shlib/clang-shlib.cpp
@@ -0,0 +1 @@
+// Intentionally empty source file to make CMake happy
Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- /dev/null
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -0,0 +1,13 @@
+get_property(clang_libs GLOBAL PROPERTY CLANG_STATIC_LIBS)
+
+foreach (lib ${clang_libs})
+  list(APPEND _OBJECTS $)
+  list(APPEND _DEPS $)
+endforeach ()
+
+add_clang_library(Clang_shared
+  SHARED
+  clang-shlib.cpp
+  ${_OBJECTS}
+  LINK_LIBS
+  ${_DEPS})
Index: clang/tools/CMakeLists.txt
===
--- clang/tools/CMakeLists.txt
+++ clang/tools/CMakeLists.txt
@@ -13,6 +13,9 @@
 
 add_clang_subdirectory(clang-rename)
 add_clang_subdirectory(clang-refactor)
+if(UNIX)
+  add_clang_subdirectory(clang-shlib)
+endif()
 
 if(CLANG_ENABLE_ARCMT)
   add_clang_subdirectory(arcmt-test)
Index: clang/cmake/modules/AddClang.cmake
===
--- clang/cmake/modules/AddClang.cmake
+++ clang/cmake/modules/AddClang.cmake
@@ -81,9 +81,12 @@
   )
   endif()
   if(ARG_SHARED)
-set(ARG_ENABLE_SHARED SHARED)
+set(LIBTYPE SHARED)
+  else()
+set(LIBTYPE STATIC OBJECT)
+set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
   endif()
-  llvm_add_library(${name} ${ARG_ENABLE_SHARED} ${ARG_UNPARSED_ARGUMENTS} 
${srcs})
+  llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
 
   if(TARGET ${name})
 target_link_libraries(${name} INTERFACE ${LLVM_COMMON_LIBS})


Index: clang/tools/clang-shlib/clang-shlib.cpp
===
--- /dev/null
+++ clang/tools/clang-shlib/clang-shlib.cpp
@@ -0,0 +1 @@
+// Intentionally empty source file to make CMake happy
Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- /dev/null
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -0,0 +1,13 @@
+get_property(clang_libs GLOBAL PROPERTY CLANG_STATIC_LIBS)
+
+foreach (lib ${clang_libs})
+  list(APPEND _OBJECTS $)
+  list(APPEND _DEPS $)
+endforeach ()
+
+add_clang_library(Clang_shared
+  SHARED
+  clang-shlib.cpp
+  ${_OBJECTS}
+  LINK_LIBS
+  ${_DEPS})
Index: clang/tools/CMakeLists.txt
===
--- clang/tools/CMakeLists.txt
+++ clang/tools/CMakeLists.txt
@@ -13,6 +13,9 @@
 
 add_clang_subdirectory(clang-rename)
 add_clang_subdirectory(clang-refactor)
+if(UNIX)
+  add_clang_subdirectory(clang-shlib)
+endif()
 
 if(CLANG_ENABLE_ARCMT)
   add_clang_subdirectory(arcmt-test)
Index: clang/cmake/modules/AddClang.cmake
===
--- clang/cmake/modules/AddClang.cmake
+++ clang/cmake/modules/AddClang.cmake
@@ -81,9 +81,12 @@
   )
   endif()
   if(ARG_SHARED)
-set(ARG_ENABLE_SHARED SHARED)
+set(LIBTYPE SHARED)
+  else()
+set(LIBTYPE STATIC OBJECT)
+set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
   endif()
-  llvm_add_library(${name} ${ARG_ENABLE_SHARED} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
+  llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
 
   if(TARGET ${name})
 target_link_libraries(${name} INTERFACE ${LLVM_COMMON_LIBS})
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

2019-05-14 Thread Yitzhak Mandelbaum via Phabricator via cfe-commits

ymandel updated this revision to Diff 199465.
ymandel edited the summary of this revision.
ymandel added a comment.

Response to comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61335

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -116,7 +116,8 @@
 };
   }
 
-  void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) {
+  template 
+  void testRule(R Rule, StringRef Input, StringRef Expected) {
 Transformer T(std::move(Rule), consumer());
 T.registerMatchers(&MatchFinder);
 compareSnippets(Expected, rewrite(Input));
@@ -147,7 +148,7 @@
  .bind(StringExpr)),
   callee(cxxMethodDecl(hasName("c_str")),
   change("REPLACED"));
-  R.Explanation = text("Use size() method directly on string.");
+  R.Cases[0].Explanation = text("Use size() method directly on string.");
   return R;
 }
 
@@ -375,6 +376,92 @@
Input, Expected);
 }
 
+TEST_F(TransformerTest, OrderedRuleUnrelated) {
+  StringRef Flag = "flag";
+  RewriteRule FlagRule = makeRule(
+  cxxMemberCallExpr(on(expr(hasType(cxxRecordDecl(
+hasName("proto::ProtoCommandLineFlag"
+   .bind(Flag)),
+unless(callee(cxxMethodDecl(hasName("GetProto"),
+  change(Flag, "PROTO"));
+
+  std::string Input = R"cc(
+proto::ProtoCommandLineFlag flag;
+int x = flag.foo();
+int y = flag.GetProto().foo();
+int f(string s) { return strlen(s.c_str()); }
+  )cc";
+  std::string Expected = R"cc(
+proto::ProtoCommandLineFlag flag;
+int x = PROTO.foo();
+int y = flag.GetProto().foo();
+int f(string s) { return REPLACED; }
+  )cc";
+
+  testRule(applyFirst({ruleStrlenSize(), FlagRule}), Input, Expected);
+}
+
+// Version of ruleStrlenSizeAny that inserts a method with a different name than
+// ruleStrlenSize, so we can tell their effect apart.
+RewriteRule ruleStrlenSizeDistinct() {
+  StringRef S;
+  return makeRule(
+  callExpr(callee(functionDecl(hasName("strlen"))),
+   hasArgument(0, cxxMemberCallExpr(
+  on(expr().bind(S)),
+  callee(cxxMethodDecl(hasName("c_str")),
+  change("DISTINCT"));
+}
+
+TEST_F(TransformerTest, OrderedRuleRelated) {
+  std::string Input = R"cc(
+namespace foo {
+struct mystring {
+  char* c_str();
+};
+int f(mystring s) { return strlen(s.c_str()); }
+}  // namespace foo
+int g(string s) { return strlen(s.c_str()); }
+  )cc";
+  std::string Expected = R"cc(
+namespace foo {
+struct mystring {
+  char* c_str();
+};
+int f(mystring s) { return DISTINCT; }
+}  // namespace foo
+int g(string s) { return REPLACED; }
+  )cc";
+
+  testRule(applyFirst({ruleStrlenSize(), ruleStrlenSizeDistinct()}), Input,
+   Expected);
+}
+
+// Change the order of the rules to get a different result.
+TEST_F(TransformerTest, OrderedRuleRelatedSwapped) {
+  std::string Input = R"cc(
+namespace foo {
+struct mystring {
+  char* c_str();
+};
+int f(mystring s) { return strlen(s.c_str()); }
+}  // namespace foo
+int g(string s) { return strlen(s.c_str()); }
+  )cc";
+  std::string Expected = R"cc(
+namespace foo {
+struct mystring {
+  char* c_str();
+};
+int f(mystring s) { return DISTINCT; }
+}  // namespace foo
+int g(string s) { return DISTINCT; }
+  )cc";
+
+  testRule(applyFirst({ruleStrlenSizeDistinct(), ruleStrlenSize()}), Input,
+   Expected);
+}
+
 //
 // Negative tests (where we expect no transformation to occur).
 //
Index: clang/lib/Tooling/Refactoring/Transformer.cpp
===
--- clang/lib/Tooling/Refactoring/Transformer.cpp
+++ clang/lib/Tooling/Refactoring/Transformer.cpp
@@ -28,6 +28,7 @@
 using namespace tooling;
 
 using ast_matchers::MatchFinder;
+using ast_matchers::internal::DynTypedMatcher;
 using ast_type_traits::ASTNodeKind;
 using ast_type_traits::DynTypedNode;
 using llvm::Error;
@@ -171,18 +172,113 @@
   return Transformations;
 }
 
-RewriteRule tooling::makeRule(ast_matchers::internal::DynTypedMatcher M,
+RewriteRule tooling::makeRule(DynTypedMatcher M,
   SmallVector Edits) {
+  return RewriteRule{
+  {RewriteRule::Case{std::move(M), std::move(Edits), nullptr}}};
+}
+
+// Determines whether A is a base type of B in the class hierarchy, including
+// the implicit relationship of Type and

[PATCH] D61804: Support building shared and static libraries simultaneously

2019-05-14 Thread Chris Bieneman via Phabricator via cfe-commits

beanz added a comment.

@winksaville that was my bad. I left off the new files in the commit because I 
forgot `git-diff` doesn't grab untracked files...

I've uploaded the patch as a D61909 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61804



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

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

2019-05-14 Thread Yitzhak Mandelbaum via Phabricator via cfe-commits

ymandel marked 9 inline comments as done and an inline comment as not done.
ymandel added a comment.

Thanks for the review. PTAL.




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:196
+/// Joins multiple rules into a single rule that applies the first rule in
+/// `Rules` whose pattern matches a given node. All of the rules must use the
+/// same kind of matcher (that is, share a base class in the AST hierarchy).

ilya-biryukov wrote:
> > All of the rules must use the same kind of matcher
> 
> Could you elaborate why we want this restriction?
> Why they can't be of different kinds? 
> 
> It feels ok to have transformations to completely unrelated nodes and still 
> apply the first one.
> 
Agreed. The problem is purely from the implementation perspective. Since 
anyOf() enforces this restriction, I need to either
1. pass it on to the user
2. infer the base kind of each matcher and place them in the appropriate bucket.

I figured for a first pass, we'd go with 1. if you disagree, I can add the 
logic to bucket them and thereby support arbitrary combinations. FWIW, I think 
it's a desirable feature, so its not wasted work. its just a matter of 
splitting up the work.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:224
+//
+// RewriteRule R = makeOrderedRule({makeRule(two_calls, two_calls_action)},
+// {makeRule(left_call, left_call_action)},

ilya-biryukov wrote:
> Why would we need braces around each call? Aren't they a rewrite rule in 
> themselves?
typo. thanks!



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:232
+// `makeOrderedRule({makeRule(m1, action1), makeRule(m2, action2), ...});`
+RewriteRule makeOrderedRule(ArrayRef Rules);
+

ilya-biryukov wrote:
> Any ideas for a better name? `pickFirst`  or `applyFirst`?
Yes, those are both better. Went w/ `applyFirst`.  Also considered 
`applyFirstMatch` but not sure that buys much clarity



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:240
 
+/// The following three functions function are a low-level part of the API,
+/// provided to support interpretation of a \c RewriteRule in a tool, like \c

ilya-biryukov wrote:
> Could they be made private? If not, why?
I tried to clarify this. But, I'm also fine splitting these three into a 
separate header file or moving them to the bottom of this one.  They're only 
exposed at all because TransformerTidy lives in a different directory and 
namespace.  WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61335



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

[PATCH] D61790: [C++20] add consteval specifier

2019-05-14 Thread Eli Friedman via Phabricator via cfe-commits

efriedma added a comment.

"complex" in this context is the C99 `_Complex`, which is supported in C++ as 
an extension, using the same syntax as C. You can just declare a variable with 
type `_Complex float`.  If you need to manipulate the real and imaginary parts 
of a variable, you can use the gcc extension `__real__`/`__imag__`, although I 
don't think you need to.


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

https://reviews.llvm.org/D61790



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

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

2019-05-14 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added a comment.

The implementation LG, thanks! Just a few NITs and comments about the public 
interface and the comments




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:196
+/// Joins multiple rules into a single rule that applies the first rule in
+/// `Rules` whose pattern matches a given node. All of the rules must use the
+/// same kind of matcher (that is, share a base class in the AST hierarchy).

ymandel wrote:
> ilya-biryukov wrote:
> > > All of the rules must use the same kind of matcher
> > 
> > Could you elaborate why we want this restriction?
> > Why they can't be of different kinds? 
> > 
> > It feels ok to have transformations to completely unrelated nodes and still 
> > apply the first one.
> > 
> Agreed. The problem is purely from the implementation perspective. Since 
> anyOf() enforces this restriction, I need to either
> 1. pass it on to the user
> 2. infer the base kind of each matcher and place them in the appropriate 
> bucket.
> 
> I figured for a first pass, we'd go with 1. if you disagree, I can add the 
> logic to bucket them and thereby support arbitrary combinations. FWIW, I 
> think it's a desirable feature, so its not wasted work. its just a matter of 
> splitting up the work.
Ah, ok, so this is the restriction of `anyOf`.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:232
+// `makeOrderedRule({makeRule(m1, action1), makeRule(m2, action2), ...});`
+RewriteRule makeOrderedRule(ArrayRef Rules);
+

ymandel wrote:
> ilya-biryukov wrote:
> > Any ideas for a better name? `pickFirst`  or `applyFirst`?
> Yes, those are both better. Went w/ `applyFirst`.  Also considered 
> `applyFirstMatch` but not sure that buys much clarity
`applyFirst` looks good, thanks!



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:240
 
+/// The following three functions function are a low-level part of the API,
+/// provided to support interpretation of a \c RewriteRule in a tool, like \c

ymandel wrote:
> ilya-biryukov wrote:
> > Could they be made private? If not, why?
> I tried to clarify this. But, I'm also fine splitting these three into a 
> separate header file or moving them to the bottom of this one.  They're only 
> exposed at all because TransformerTidy lives in a different directory and 
> namespace.  WDYT?
I see. Maybe additionally put them into `namespace detail`?
A good way to make sure that any use is a red herring. (Otherwise clients would 
need to read the code to realize it's an internal function.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:195
 
+/// Joins multiple rules into a single rule that applies the first rule in
+/// `Rules` whose pattern matches a given node.

NIT: Maybe shorten a bit? Something like

```
Applies the first rule whose pattern matches, other rules are ignored
```


The current version has a bit too many details, so it's hard to grasp on a 
first read.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:215
+//
+// Ordered rules allow us to specify each independently:
+// ```

NIT: s/Ordered rules allow us/applyFirst allows to... ?

With a new name for the function, "ordered" rules can be confusing



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:231
+// ```
+// More generally, anywhere you'd use anyOf(m1.bind("m1"), m2.bind("m2")) and
+// then dispatch on those tags in your code to decide what to do, we'll lift

Maybe start with this? It's a great analogy. Something like this at the start 
of the comment would be great:
```
`applyFirst` is similar to `anyOf` matcher with a replace action attached to 
each of its cases...
```



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:239
+  for (auto &Rule : Rules) {
+R.Cases.append(Rule.Cases.begin(), Rule.Cases.end());
+  }

NIT: remove redundant `{}`



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:262
+
+// Finds the rule that was "selected" -- that is, whose matcher triggered the
+// `MatchResult`.

NIT:s/Finds the rule/Finds the case
Would help avoid potential confusion...



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:250
+  assert(!CommonKind.isNone() && "Cases must have compatible matchers.");
+  return DynTypedMatcher::constructVariadic(
+  DynTypedMatcher::VO_AnyOf, CommonKind, taggedMatchers("Tag", 
Rule.Cases));

ymandel wrote:
> ilya-biryukov wrote:
> > I wonder if there a better way to construct an `anyOf` matcher that can 
> > tell which case matched...
> > It looks to be the reason for a more complicated interface on the 
> > transformer side, but I'm not sure there's a way out of it.
> > 
> > Any ideas?
> I don't quite follow. Which interfac

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

2019-05-14 Thread Gabor Marton via Phabricator via cfe-commits

martong added a comment.

Thank you guys for the review!

In D61438#1501457 , @aprantl wrote:

> Alright, thanks for taking the time to walk me through the thought process!





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61438



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

[PATCH] D60162: [ThinLTO] Add module flags for TargetLibraryInfoImpl and use in LTO backends

2019-05-14 Thread Steven Wu via Phabricator via cfe-commits

steven_wu added a comment.

Thanks for doing this. I think module flag is a good idea. Some comments inline.




Comment at: clang/test/CodeGen/svml-calls.ll:16
+
+define void @sin_f64(double* nocapture %varray) {
+; CHECK-LABEL: @sin_f64(

Personally, I think codegen tests like this will be cleaner to keep in LLVM. 
Clang tests just test the IRGen of the module flag and LLVM tests check that 
those flags are respected and module flag merge is respected.



Comment at: llvm/lib/LTO/LTOBackend.cpp:221
 
+static TargetLibraryInfoImpl *createTLII(Module &Mod, TargetMachine *TM) {
+  TargetLibraryInfoImpl *TLII =

Should this be done not just for LTOBackend but for regular compilation as 
well? LegacyCodegenerator and llc can all be benefit from a matching 
TargetLibraryInfo?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60162



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

Re: r360637 - PR41817: Fix regression in r359260 that caused the MS compatibility

2019-05-14 Thread Richard Smith via cfe-commits

On Tue, 14 May 2019, 03:09 Hans Wennborg via cfe-commits, <
cfe-commits@lists.llvm.org> wrote:

> Actually, we didn't notice r359260 in Chromium, however this one
> (r360637) caused Clang to start asserting during our builds
> (https://bugs.chromium.org/p/chromium/issues/detail?id=962840), here's
> a reduced test case:
>
> --
> extern int a;
> static int *use1 = &a;
> int **use2 = &use1;
> static int a = 0;
> --
>
> $ clang.bad -cc1 -triple i386-pc-windows-msvc19.11.0 -emit-obj
> -fms-extensions /tmp/a.cc
> clang.bad: /work/llvm.monorepo/clang/lib/AST/Decl.cpp:1494:
> clang::LinkageInfo clang::LinkageComputer::getLVForDecl(const
> clang::NamedDecl*, clang::LVComputationKind): Assertion
> `D->getCachedLinkage() == LV.getLinkage()' failed.
>
>
> I've reverted this one in r360657 in the meantime.
>

Yep, I'm not at all surprised. Perhaps we should stop claiming to support
this extension, given that it fundamentally violates assumptions made by
clang (that linkage doesn't change after the first declaration). Presumably
we instead used to miscompile the example you give above (and after the
revert, miscompile it again now)?

From: Richard Smith via cfe-commits 
> Date: Tue, May 14, 2019 at 2:24 AM
> To: 
>
> > Author: rsmith
> > Date: Mon May 13 17:27:16 2019
> > New Revision: 360637
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=360637&view=rev
> > Log:
> > PR41817: Fix regression in r359260 that caused the MS compatibility
> > extension allowing a "static" declaration to follow an "extern"
> > declaration to stop working.
> >
> > Added:
> > cfe/trunk/test/CodeGen/ms-compat-extern-static.c
> > Modified:
> > cfe/trunk/lib/AST/Decl.cpp
> >
> > Modified: cfe/trunk/lib/AST/Decl.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Decl.cpp?rev=360637&r1=360636&r2=360637&view=diff
> >
> ==
> > --- cfe/trunk/lib/AST/Decl.cpp (original)
> > +++ cfe/trunk/lib/AST/Decl.cpp Mon May 13 17:27:16 2019
> > @@ -613,12 +613,41 @@ static LinkageInfo getExternalLinkageFor
> >  static StorageClass getStorageClass(const Decl *D) {
> >if (auto *TD = dyn_cast(D))
> >  D = TD->getTemplatedDecl();
> > -  if (D) {
> > -if (auto *VD = dyn_cast(D))
> > -  return VD->getStorageClass();
> > -if (auto *FD = dyn_cast(D))
> > -  return FD->getStorageClass();
> > +  if (!D)
> > +return SC_None;
> > +
> > +  if (auto *VD = dyn_cast(D)) {
> > +// Generally, the storage class is determined by the first
> declaration.
> > +auto SC = VD->getCanonicalDecl()->getStorageClass();
> > +
> > +// ... except that MSVC permits an 'extern' declaration to be
> redeclared
> > +// 'static' as an extension.
> > +if (SC == SC_Extern) {
> > +  for (auto *Redecl : VD->redecls()) {
> > +if (Redecl->getStorageClass() == SC_Static)
> > +  return SC_Static;
> > +if (Redecl->getStorageClass() != SC_Extern &&
> > +!Redecl->isLocalExternDecl() &&
> !Redecl->getFriendObjectKind())
> > +  break;
> > +  }
> > +}
> > +return SC;
> >}
> > +
> > +  if (auto *FD = dyn_cast(D)) {
> > +auto SC = FD->getCanonicalDecl()->getStorageClass();
> > +if (SC == SC_Extern) {
> > +  for (auto *Redecl : FD->redecls()) {
> > +if (Redecl->getStorageClass() == SC_Static)
> > +  return SC_Static;
> > +if (Redecl->getStorageClass() != SC_Extern &&
> > +!Redecl->isLocalExternDecl() &&
> !Redecl->getFriendObjectKind())
> > +  break;
> > +  }
> > +}
> > +return SC;
> > +  }
> > +
> >return SC_None;
> >  }
> >
> > @@ -634,7 +663,7 @@ LinkageComputer::getLVForNamespaceScopeD
> >//   A name having namespace scope (3.3.6) has internal linkage if it
> >//   is the name of
> >
> > -  if (getStorageClass(D->getCanonicalDecl()) == SC_Static) {
> > +  if (getStorageClass(D) == SC_Static) {
> >  // - a variable, variable template, function, or function template
> >  //   that is explicitly declared static; or
> >  // (This bullet corresponds to C99 6.2.2p3.)
> >
> > Added: cfe/trunk/test/CodeGen/ms-compat-extern-static.c
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/ms-compat-extern-static.c?rev=360637&view=auto
> >
> ==
> > --- cfe/trunk/test/CodeGen/ms-compat-extern-static.c (added)
> > +++ cfe/trunk/test/CodeGen/ms-compat-extern-static.c Mon May 13 17:27:16
> 2019
> > @@ -0,0 +1,11 @@
> > +// RUN: %clang_cc1 -emit-llvm %s -o - -fms-extensions -triple
> x86_64-windows | FileCheck %s
> > +
> > +// CHECK: @n = internal global i32 1
> > +extern int n;
> > +static int n = 1;
> > +int *use = &n;
> > +
> > +// CHECK: define internal void @f(
> > +extern void f();
> > +static void f() {}
> > +void g() { return f(); }
> >
> >
> > ___
> > cfe-commits mailing list
> > cfe-

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

2019-05-14 Thread Shafik Yaghmour via Phabricator via cfe-commits

shafik added a comment.

Done with first round.




Comment at: lib/AST/ASTImporter.cpp:1643
+if (!ImportedOrErr) {
+  // For RecordDecls, failed import of a field will break the layout of the
+  // structure. Handle it as an error.

What cases are failed import of a field ok? Is that because we will get the 
field later on by another path?



Comment at: lib/AST/ASTImporter.cpp:1655
+  // Reorder declarations in RecordDecls because they may have another
+  // order. Keeping field order is vitable because it determines structure
+  // layout.

vitable?



Comment at: lib/AST/ASTImporter.cpp:1663
+
+
+  // NOTE: Here and below, we cannot call field_begin() method and its callers

Maybe a comment here explaining the purpose of the loops below. IIUC removing 
fields since they may have been imported in the wrong order and then adding 
them back in what should be the correct order now.



Comment at: lib/AST/ASTImporter.cpp:1674
+  Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+  assert(ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD));
+  ToRD->removeDecl(ToD);

Are we sure `ToD` is never a `nullptr` b/c you are unconditionally derefenecing 
it here.



Comment at: lib/AST/ASTImporter.cpp:1679
+
+  if (!ToRD->hasExternalLexicalStorage())
+assert(ToRD->field_empty());

Can you add a comment explaining why if the Decl has ExternalLexicalStorage the 
fields might not be empty even though we just removed them?


Repository:
  rC Clang

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

https://reviews.llvm.org/D44100



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

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

2019-05-14 Thread Yitzhak Mandelbaum via Phabricator via cfe-commits

ymandel added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:196
+/// Joins multiple rules into a single rule that applies the first rule in
+/// `Rules` whose pattern matches a given node. All of the rules must use the
+/// same kind of matcher (that is, share a base class in the AST hierarchy).

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > > All of the rules must use the same kind of matcher
> > > 
> > > Could you elaborate why we want this restriction?
> > > Why they can't be of different kinds? 
> > > 
> > > It feels ok to have transformations to completely unrelated nodes and 
> > > still apply the first one.
> > > 
> > Agreed. The problem is purely from the implementation perspective. Since 
> > anyOf() enforces this restriction, I need to either
> > 1. pass it on to the user
> > 2. infer the base kind of each matcher and place them in the appropriate 
> > bucket.
> > 
> > I figured for a first pass, we'd go with 1. if you disagree, I can add the 
> > logic to bucket them and thereby support arbitrary combinations. FWIW, I 
> > think it's a desirable feature, so its not wasted work. its just a matter 
> > of splitting up the work.
> Ah, ok, so this is the restriction of `anyOf`.
indeed -- and all the other variadoc operations.



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:250
+  assert(!CommonKind.isNone() && "Cases must have compatible matchers.");
+  return DynTypedMatcher::constructVariadic(
+  DynTypedMatcher::VO_AnyOf, CommonKind, taggedMatchers("Tag", 
Rule.Cases));

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > I wonder if there a better way to construct an `anyOf` matcher that can 
> > > tell which case matched...
> > > It looks to be the reason for a more complicated interface on the 
> > > transformer side, but I'm not sure there's a way out of it.
> > > 
> > > Any ideas?
> > I don't quite follow. Which interface complication are you referring to?  
> > FWIW, the reason the code here doesn't just use the anyOf() matches is that 
> > it doesn't take a vector -- it only has a variadic form.
> E.g. the restriction that the matchers should have a common kind seems to 
> come from the fact that we need to later find out which case matched.
> 
> It this a limitation of the AST matchers design? E.g. I can't match both a 
> type and an expression and bind different subnodes in each submatcher, right?
> E.g. the restriction that the matchers should have a common kind seems to 
> come from the fact that we need to later find out which case matched.
>
> It this a limitation of the AST matchers design? E.g. I can't match both a 
> type and an expression and bind different subnodes in each submatcher, right?

Yes, as far as I can tell, but I don't think its connected to the binding -- 
every `DynTypedMatcher` needs to report what kind it supports.  IIRC, this is 
connected to the desire to specialize matchers to types to provide some static 
checking for matchers.  Since there is no universal/root AST type, there's no 
root kind, and we're stuck w/ this restriction.  If we were willing for the 
`Matcher` class to diverge from the AST, we could add a "universal" node kind, 
but that's another discussion...

In practice, this is mitigated in matchers by forcing the user to call a 
different `addMatcher` for each kind. But, that won't work for us since we want 
to (ultimately) support rules of different (base) kinds.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61335



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

[PATCH] D61335: [LibTooling] Add support to Transformer for composing rules as an ordered choice.

2019-05-14 Thread Yitzhak Mandelbaum via Phabricator via cfe-commits

ymandel updated this revision to Diff 199476.
ymandel marked 18 inline comments as done.
ymandel added a comment.
Herald added a subscriber: jfb.

responded to comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61335

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -116,7 +116,8 @@
 };
   }
 
-  void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) {
+  template 
+  void testRule(R Rule, StringRef Input, StringRef Expected) {
 Transformer T(std::move(Rule), consumer());
 T.registerMatchers(&MatchFinder);
 compareSnippets(Expected, rewrite(Input));
@@ -147,7 +148,7 @@
  .bind(StringExpr)),
   callee(cxxMethodDecl(hasName("c_str")),
   change("REPLACED"));
-  R.Explanation = text("Use size() method directly on string.");
+  R.Cases[0].Explanation = text("Use size() method directly on string.");
   return R;
 }
 
@@ -375,6 +376,92 @@
Input, Expected);
 }
 
+TEST_F(TransformerTest, OrderedRuleUnrelated) {
+  StringRef Flag = "flag";
+  RewriteRule FlagRule = makeRule(
+  cxxMemberCallExpr(on(expr(hasType(cxxRecordDecl(
+hasName("proto::ProtoCommandLineFlag"
+   .bind(Flag)),
+unless(callee(cxxMethodDecl(hasName("GetProto"),
+  change(Flag, "PROTO"));
+
+  std::string Input = R"cc(
+proto::ProtoCommandLineFlag flag;
+int x = flag.foo();
+int y = flag.GetProto().foo();
+int f(string s) { return strlen(s.c_str()); }
+  )cc";
+  std::string Expected = R"cc(
+proto::ProtoCommandLineFlag flag;
+int x = PROTO.foo();
+int y = flag.GetProto().foo();
+int f(string s) { return REPLACED; }
+  )cc";
+
+  testRule(applyFirst({ruleStrlenSize(), FlagRule}), Input, Expected);
+}
+
+// Version of ruleStrlenSizeAny that inserts a method with a different name than
+// ruleStrlenSize, so we can tell their effect apart.
+RewriteRule ruleStrlenSizeDistinct() {
+  StringRef S;
+  return makeRule(
+  callExpr(callee(functionDecl(hasName("strlen"))),
+   hasArgument(0, cxxMemberCallExpr(
+  on(expr().bind(S)),
+  callee(cxxMethodDecl(hasName("c_str")),
+  change("DISTINCT"));
+}
+
+TEST_F(TransformerTest, OrderedRuleRelated) {
+  std::string Input = R"cc(
+namespace foo {
+struct mystring {
+  char* c_str();
+};
+int f(mystring s) { return strlen(s.c_str()); }
+}  // namespace foo
+int g(string s) { return strlen(s.c_str()); }
+  )cc";
+  std::string Expected = R"cc(
+namespace foo {
+struct mystring {
+  char* c_str();
+};
+int f(mystring s) { return DISTINCT; }
+}  // namespace foo
+int g(string s) { return REPLACED; }
+  )cc";
+
+  testRule(applyFirst({ruleStrlenSize(), ruleStrlenSizeDistinct()}), Input,
+   Expected);
+}
+
+// Change the order of the rules to get a different result.
+TEST_F(TransformerTest, OrderedRuleRelatedSwapped) {
+  std::string Input = R"cc(
+namespace foo {
+struct mystring {
+  char* c_str();
+};
+int f(mystring s) { return strlen(s.c_str()); }
+}  // namespace foo
+int g(string s) { return strlen(s.c_str()); }
+  )cc";
+  std::string Expected = R"cc(
+namespace foo {
+struct mystring {
+  char* c_str();
+};
+int f(mystring s) { return DISTINCT; }
+}  // namespace foo
+int g(string s) { return DISTINCT; }
+  )cc";
+
+  testRule(applyFirst({ruleStrlenSizeDistinct(), ruleStrlenSize()}), Input,
+   Expected);
+}
+
 //
 // Negative tests (where we expect no transformation to occur).
 //
Index: clang/lib/Tooling/Refactoring/Transformer.cpp
===
--- clang/lib/Tooling/Refactoring/Transformer.cpp
+++ clang/lib/Tooling/Refactoring/Transformer.cpp
@@ -28,6 +28,7 @@
 using namespace tooling;
 
 using ast_matchers::MatchFinder;
+using ast_matchers::internal::DynTypedMatcher;
 using ast_type_traits::ASTNodeKind;
 using ast_type_traits::DynTypedNode;
 using llvm::Error;
@@ -144,9 +145,9 @@
   llvm_unreachable("Unexpected case in NodePart type.");
 }
 
-Expected>
-tooling::translateEdits(const MatchResult &Result,
-llvm::ArrayRef Edits) {
+Expected>
+tooling::detail::translateEdits(const MatchResult &Result,
+llvm::ArrayRef Edits) {
   SmallVector Transformations;
   auto &NodesMap = Result.Nodes.getMap();
   for (const auto &E

Re: r360452 - Replace 'REQUIRES: nozlib' with '!zlib' because we don't need two ways

2019-05-14 Thread David Blaikie via cfe-commits

Fair enough - since they're already there I don't feel super serious
about converging on one (though I probably wouldn't've been in favor
of adding a second spelling at the point it was proposed).

On Tue, May 14, 2019 at 8:03 AM  wrote:
>
> There's no practical difference.  I view it as a matter of documentation of 
> intent, see my commit log comment for r360603.
>
> Arguably we could eliminate UNSUPPORTED and move all the expressions into 
> REQUIRES (appropriately negated), but I'm not sure that's a net win for 
> readability.
>
> --paulr
>
>
>
> From: David Blaikie [mailto:dblai...@gmail.com]
> Sent: Monday, May 13, 2019 10:48 AM
> To: Robinson, Paul
> Cc: cfe-commits
> Subject: Re: r360452 - Replace 'REQUIRES: nozlib' with '!zlib' because we 
> don't need two ways
>
>
>
> What's the practical difference between "UNSUPPORTED: foo" and "REQUIRES: 
> !foo"? (I see some of the fixes you've made go one way, some the other way)
>
>
>
> On Fri, May 10, 2019 at 11:30 AM Paul Robinson via cfe-commits 
>  wrote:
>
> Author: probinson
> Date: Fri May 10 11:32:53 2019
> New Revision: 360452
>
> URL: http://llvm.org/viewvc/llvm-project?rev=360452&view=rev
> Log:
> Replace 'REQUIRES: nozlib' with '!zlib' because we don't need two ways
> to say the same thing.
>
> Modified:
> cfe/trunk/test/Driver/nozlibcompress.c
>
> Modified: cfe/trunk/test/Driver/nozlibcompress.c
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/nozlibcompress.c?rev=360452&r1=360451&r2=360452&view=diff
> ==
> --- cfe/trunk/test/Driver/nozlibcompress.c (original)
> +++ cfe/trunk/test/Driver/nozlibcompress.c Fri May 10 11:32:53 2019
> @@ -1,4 +1,4 @@
> -// REQUIRES: nozlib
> +// REQUIRES: !zlib
>
>  // RUN: %clang -### -fintegrated-as -gz -c %s 2>&1 | FileCheck %s 
> -check-prefix CHECK-WARN
>  // RUN: %clang -### -fintegrated-as -gz=none -c %s 2>&1 | FileCheck 
> -allow-empty -check-prefix CHECK-NOWARN %s
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61909: Add Clang shared library with C++ exports

2019-05-14 Thread Tom Stellard via Phabricator via cfe-commits

tstellar added a comment.

Thanks for working on this, I have wanted something like this for a while.

It would also be nice to have a CLANG_LINK_CLANG_DYLIB option like we have for 
llvm, but this can be a follow on patch, and I would be happy to help with this 
if needed.




Comment at: clang/tools/clang-shlib/CMakeLists.txt:8
+
+add_clang_library(Clang_shared
+  SHARED

Is this capital 'C' to distinguish it from the other libclang.so ?  I would 
prfer to use lowercase 'c' here instead, so it fit the naming convention for 
the other libraries.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61909



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

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

2019-05-14 Thread Torbjörn Klatt via Phabricator via cfe-commits

torbjoernk updated this revision to Diff 199483.
torbjoernk edited the summary of this revision.
torbjoernk added a comment.
Herald added a subscriber: arphaman.

Addressed Roman Lebedev's comment regarding reference to `NOLINT` in the docs.

As I don't have commit rights, I'd be grateful someone else could commit this 
patch once it's fully accepted.


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

https://reviews.llvm.org/D61827

Files:
  clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
  clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp

Index: clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
===
--- clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
+++ clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
@@ -776,17 +776,20 @@
   // CHECK-FIXES: for (auto P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
-  // V.begin() returns a user-defined type 'iterator' which, since it's
-  // different from const_iterator, disqualifies these loops from
-  // transformation.
   dependent V;
   for (dependent::const_iterator It = V.begin(); It != V.end(); ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 
   for (dependent::const_iterator It(V.begin()); It != V.end(); ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 }
 
 } // namespace SingleIterator
@@ -991,18 +994,26 @@
   // CHECK-FIXES: for (int & I : Dep)
   // CHECK-FIXES-NEXT: auto H2 = [&]() { int R = I + 2; };
 
-  // FIXME: It doesn't work with const iterators.
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H3 = [I]() { int R = *I; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : Dep)
+  // CHECK-FIXES-NEXT: auto H3 = [&I]() { int R = I; };
 
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H4 = [&]() { int R = *I + 1; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : Dep)
+  // CHECK-FIXES-NEXT: auto H4 = [&]() { int R = I + 1; };
 
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H5 = [=]() { int R = *I; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int R : Dep)
+  // CHECK-FIXES-NEXT: auto H5 = [=]() { };
 }
 
 void captureByValue() {
Index: clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
===
--- clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
+++ clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
@@ -369,7 +369,7 @@
 // CHECK-FIXES: for (auto Val_int_ptr : Val_int_ptrs)
   }
 
-  // This container uses an iterator where the derefence type is a typedef of
+  // This container uses an iterator where the dereference type is a typedef of
   // a reference type. Make sure non-const auto & is still used. A failure here
   // means canonical types aren't being tested.
   {
@@ -431,19 +431,22 @@
   // CHECK-FIXES: for (auto P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
-  // V.begin() returns a user-defined type 'iterator' which, since it's
-  // different from const_iterator, disqualifies these loops from
-  // transformation.
   dependent V;
   for (dependent::const_iterator It = V.begin(), E = V.end();
It != E; ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 
   for (dependent::const_iterator It(V.begin()), E = V.end();
It != E; ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 }
 
 // Tests to ensure that an implicit 'this' is picked up as the container.
Index: clang-tools-extra/docs/clang-tidy/index.rst
===
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rs

[PATCH] D61909: Add Clang shared library with C++ exports

2019-05-14 Thread Wink Saville via Phabricator via cfe-commits

winksaville added a comment.

You mention that you're using OBJECT libraries so objects aren't built 
mutliples times
and in my current tests the number of steps increased by only 3, it went from 
4353 to 4356,
when using this patch, which is great!

What I see in my testing of the patch is that cmake finds that the compilers 
support position independent code and use `-fPIC` as default:

  -- LLVM default target triple: x86_64-unknown-linux-gnu
  -- Performing Test C_SUPPORTS_FPIC
  -- Performing Test C_SUPPORTS_FPIC - Success
  -- Performing Test CXX_SUPPORTS_FPIC
  -- Performing Test CXX_SUPPORTS_FPIC - Success
  -- Building with -fPIC

And then, as expected, I see the compile command lines have `-fPIC`:

  [1/4356] /usr/bin/c++  -DGTEST_HAS_RTTI=0 ... -fPIC ...

Its my understanding that when creating shared libraries position independent 
code generation is required.
Is there a problem if in some scenario the OBJECT libraries were not compiled 
with `-fPIC`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61909



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

[clang-tools-extra] r360698 - [clang-tidy] Fix invalid fixit for readability-static-accessed-through-instance (bug 40544)

2019-05-14 Thread Matthias Gehre via cfe-commits

Author: mgehre
Date: Tue May 14 11:23:10 2019
New Revision: 360698

URL: http://llvm.org/viewvc/llvm-project?rev=360698&view=rev
Log:
[clang-tidy] Fix invalid fixit for readability-static-accessed-through-instance 
(bug 40544)

Summary:
Fixed https://bugs.llvm.org/show_bug.cgi?id=40544
Before, we would generate a fixit like `(anonymous namespace)::Foo::fun();` for
the added test case.

Reviewers: aaron.ballman, alexfh, xazax.hun

Subscribers: rnkovacs, cfe-commits

Tags: #clang, #clang-tools-extra

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

Modified:

clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp

clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp

Modified: 
clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp?rev=360698&r1=360697&r2=360698&view=diff
==
--- 
clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
 (original)
+++ 
clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
 Tue May 14 11:23:10 2019
@@ -67,6 +67,7 @@ void StaticAccessedThroughInstanceCheck:
   const ASTContext *AstContext = Result.Context;
   PrintingPolicy PrintingPolicyWithSupressedTag(AstContext->getLangOpts());
   PrintingPolicyWithSupressedTag.SuppressTagKeyword = true;
+  PrintingPolicyWithSupressedTag.SuppressUnwrittenScope = true;
   std::string BaseTypeName =
   BaseType.getAsString(PrintingPolicyWithSupressedTag);
 

Modified: 
clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp?rev=360698&r1=360697&r2=360698&view=diff
==
--- 
clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
 (original)
+++ 
clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
 Tue May 14 11:23:10 2019
@@ -220,3 +220,31 @@ int func(Qptr qp) {
   qp->y = 10; // OK, the overloaded operator might have side-effects.
   qp->K = 10; //
 }
+
+namespace {
+  struct Anonymous {
+static int I;
+  };
+}
+
+void use_anonymous() {
+  Anonymous Anon;
+  Anon.I;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  Anonymous::I;{{$}}
+}
+
+namespace Outer {
+  inline namespace Inline {
+struct S {
+  static int I;
+};
+  }
+}
+
+void use_inline() {
+  Outer::S V;
+  V.I;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  Outer::S::I;{{$}}
+}


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

[PATCH] D61874: [clang-tidy] Fix invalid fixit for readability-static-accessed-through-instance (bug 40544)

2019-05-14 Thread Matthias Gehre via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL360698: [clang-tidy] Fix invalid fixit for 
readability-static-accessed-through-instance… (authored by mgehre, committed by 
).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D61874?vs=199333&id=199484#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D61874

Files:
  
clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
  
clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp


Index: 
clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
===
--- 
clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
+++ 
clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
@@ -67,6 +67,7 @@
   const ASTContext *AstContext = Result.Context;
   PrintingPolicy PrintingPolicyWithSupressedTag(AstContext->getLangOpts());
   PrintingPolicyWithSupressedTag.SuppressTagKeyword = true;
+  PrintingPolicyWithSupressedTag.SuppressUnwrittenScope = true;
   std::string BaseTypeName =
   BaseType.getAsString(PrintingPolicyWithSupressedTag);
 
Index: 
clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
===
--- 
clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
+++ 
clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
@@ -220,3 +220,31 @@
   qp->y = 10; // OK, the overloaded operator might have side-effects.
   qp->K = 10; //
 }
+
+namespace {
+  struct Anonymous {
+static int I;
+  };
+}
+
+void use_anonymous() {
+  Anonymous Anon;
+  Anon.I;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  Anonymous::I;{{$}}
+}
+
+namespace Outer {
+  inline namespace Inline {
+struct S {
+  static int I;
+};
+  }
+}
+
+void use_inline() {
+  Outer::S V;
+  V.I;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  Outer::S::I;{{$}}
+}


Index: clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp
@@ -67,6 +67,7 @@
   const ASTContext *AstContext = Result.Context;
   PrintingPolicy PrintingPolicyWithSupressedTag(AstContext->getLangOpts());
   PrintingPolicyWithSupressedTag.SuppressTagKeyword = true;
+  PrintingPolicyWithSupressedTag.SuppressUnwrittenScope = true;
   std::string BaseTypeName =
   BaseType.getAsString(PrintingPolicyWithSupressedTag);
 
Index: clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
@@ -220,3 +220,31 @@
   qp->y = 10; // OK, the overloaded operator might have side-effects.
   qp->K = 10; //
 }
+
+namespace {
+  struct Anonymous {
+static int I;
+  };
+}
+
+void use_anonymous() {
+  Anonymous Anon;
+  Anon.I;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  Anonymous::I;{{$}}
+}
+
+namespace Outer {
+  inline namespace Inline {
+struct S {
+  static int I;
+};
+  }
+}
+
+void use_inline() {
+  Outer::S V;
+  V.I;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
+  // CHECK-FIXES: {{^}}  Outer::S::I;{{$}}
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

2019-05-14 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov updated this revision to Diff 199485.
ilya-biryukov added a comment.

- Use a flag inside clang::Token instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59885

Files:
  clang/include/clang/Lex/Preprocessor.h
  clang/include/clang/Lex/Token.h
  clang/include/clang/Lex/TokenLexer.h
  clang/lib/Lex/PPCaching.cpp
  clang/lib/Lex/Preprocessor.cpp

Index: clang/lib/Lex/Preprocessor.cpp
===
--- clang/lib/Lex/Preprocessor.cpp
+++ clang/lib/Lex/Preprocessor.cpp
@@ -635,8 +635,7 @@
   CurTokenLexer->Lex(Tok);
   break;
 case CLK_CachingLexer:
-  bool IsNewToken;
-  CachingLex(Tok, IsNewToken);
+  CachingLex(Tok);
   break;
 case CLK_LexAfterModuleImport:
   LexAfterModuleImport(Tok);
@@ -880,22 +879,27 @@
   ++LexLevel;
 
   // We loop here until a lex function returns a token; this avoids recursion.
+  // FIXME: IsNewToken should be set inside lexers.
   bool ReturnedToken;
-  bool IsNewToken = true;
   do {
 switch (CurLexerKind) {
 case CLK_Lexer:
   ReturnedToken = CurLexer->Lex(Result);
+  Result.setFlag(Token::IsNewToken);
   break;
-case CLK_TokenLexer:
+case CLK_TokenLexer: {
+  bool IsNewToken = CurTokenLexer->isMacroExpansion();
   ReturnedToken = CurTokenLexer->Lex(Result);
+  Result.setFlagValue(Token::IsNewToken, IsNewToken);
   break;
+}
 case CLK_CachingLexer:
-  CachingLex(Result, IsNewToken);
+  CachingLex(Result);
   ReturnedToken = true;
   break;
 case CLK_LexAfterModuleImport:
   ReturnedToken = LexAfterModuleImport(Result);
+  Result.setFlag(Token::IsNewToken);
   break;
 }
   } while (!ReturnedToken);
@@ -911,7 +915,8 @@
 
   // Update ImportSeqState to track our position within a C++20 import-seq
   // if this token is being produced as a result of phase 4 of translation.
-  if (getLangOpts().CPlusPlusModules && LexLevel == 1 && IsNewToken) {
+  if (getLangOpts().CPlusPlusModules && LexLevel == 1 &&
+  Result.getFlag(Token::IsNewToken)) {
 switch (Result.getKind()) {
 case tok::l_paren: case tok::l_square: case tok::l_brace:
   ImportSeqState.handleOpenBracket();
@@ -952,6 +957,8 @@
 
   LastTokenWasAt = Result.is(tok::at);
   --LexLevel;
+  if (OnToken && LexLevel == 0 && Result.getFlag(Token::IsNewToken))
+OnToken(Result);
 }
 
 /// Lex a header-name token (including one formed from header-name-tokens if
Index: clang/lib/Lex/PPCaching.cpp
===
--- clang/lib/Lex/PPCaching.cpp
+++ clang/lib/Lex/PPCaching.cpp
@@ -45,7 +45,7 @@
   recomputeCurLexerKind();
 }
 
-void Preprocessor::CachingLex(Token &Result, bool &IsNewToken) {
+void Preprocessor::CachingLex(Token &Result) {
   if (!InCachingLexMode())
 return;
 
@@ -55,7 +55,8 @@
 
   if (CachedLexPos < CachedTokens.size()) {
 Result = CachedTokens[CachedLexPos++];
-IsNewToken = false;
+// FIXME: do this flag when writing to CachedTokens.
+Result.clearFlag(Token::IsNewToken);
 return;
   }
 
Index: clang/include/clang/Lex/TokenLexer.h
===
--- clang/include/clang/Lex/TokenLexer.h
+++ clang/include/clang/Lex/TokenLexer.h
@@ -147,6 +147,10 @@
   /// preprocessor directive.
   bool isParsingPreprocessorDirective() const;
 
+  /// Returns true iff the TokenLexer is expanding a macro and not replaying a
+  /// stream of tokens.
+  bool isMacroExpansion() const { return Macro != nullptr; }
+
 private:
   void destroy();
 
Index: clang/include/clang/Lex/Token.h
===
--- clang/include/clang/Lex/Token.h
+++ clang/include/clang/Lex/Token.h
@@ -70,20 +70,22 @@
 public:
   // Various flags set per token:
   enum TokenFlags {
-StartOfLine   = 0x01,  // At start of line or only after whitespace
-   // (considering the line after macro expansion).
-LeadingSpace  = 0x02,  // Whitespace exists before this token (considering
-   // whitespace after macro expansion).
-DisableExpand = 0x04,  // This identifier may never be macro expanded.
-NeedsCleaning = 0x08,  // Contained an escaped newline or trigraph.
+StartOfLine = 0x01,   // At start of line or only after whitespace
+  // (considering the line after macro expansion).
+LeadingSpace = 0x02,  // Whitespace exists before this token (considering
+  // whitespace after macro expansion).
+DisableExpand = 0x04, // This identifier may never be macro expanded.
+NeedsCleaning = 0x08, // Contained an escaped newline or trigraph.
 LeadingEmptyMacro = 0x10, // Empty macro exists before this token.
-HasUDSuffix = 0x20,// This string or character literal has a ud-suffix.

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

2019-05-14 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov added a comment.

The suggested approach looks promising. The difference seems to be within the 
noise levels on my machine:

  Before the change (baseline upstream revision):
Time (mean ± σ): 159.1 ms ±   8.7 ms[User: 138.3 ms, System: 20.6 
ms]
Range (min … max):   150.4 ms … 196.2 ms100 runs
  
  After (no callback specified):
Time (mean ± σ): 160.4 ms ±   7.6 ms[User: 138.5 ms, System: 21.7 
ms]
Range (min … max):   151.0 ms … 191.5 ms100 runs

I'm sending out a prototype I used for measures for review. The flag is 
currently set by the preprocessor, but I guess it would make more sense to do 
the following before landing this:

- flip the flag, i.e. instead of reporting whether the token is "new" (the 
common case), report whether it's "cached", i.e. coming from `CachedTokens` or 
from a non-macro-expansion token stream.
- set this flag only inside `CachingLex` and `TokenLexer`.

The revision also removes `IsNewToken` from `CachingLex` and sets the flag in 
the token instead.
Any other suggestions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59885



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

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

2019-05-14 Thread Ilya Biryukov via Phabricator via cfe-commits

ilya-biryukov updated this revision to Diff 199486.
ilya-biryukov added a comment.

- Remove the now redundant 'public:' spec.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59885

Files:
  clang/include/clang/Lex/Preprocessor.h
  clang/include/clang/Lex/Token.h
  clang/include/clang/Lex/TokenLexer.h
  clang/lib/Lex/PPCaching.cpp
  clang/lib/Lex/Preprocessor.cpp

Index: clang/lib/Lex/Preprocessor.cpp
===
--- clang/lib/Lex/Preprocessor.cpp
+++ clang/lib/Lex/Preprocessor.cpp
@@ -635,8 +635,7 @@
   CurTokenLexer->Lex(Tok);
   break;
 case CLK_CachingLexer:
-  bool IsNewToken;
-  CachingLex(Tok, IsNewToken);
+  CachingLex(Tok);
   break;
 case CLK_LexAfterModuleImport:
   LexAfterModuleImport(Tok);
@@ -880,22 +879,27 @@
   ++LexLevel;
 
   // We loop here until a lex function returns a token; this avoids recursion.
+  // FIXME: IsNewToken should be set inside lexers.
   bool ReturnedToken;
-  bool IsNewToken = true;
   do {
 switch (CurLexerKind) {
 case CLK_Lexer:
   ReturnedToken = CurLexer->Lex(Result);
+  Result.setFlag(Token::IsNewToken);
   break;
-case CLK_TokenLexer:
+case CLK_TokenLexer: {
+  bool IsNewToken = CurTokenLexer->isMacroExpansion();
   ReturnedToken = CurTokenLexer->Lex(Result);
+  Result.setFlagValue(Token::IsNewToken, IsNewToken);
   break;
+}
 case CLK_CachingLexer:
-  CachingLex(Result, IsNewToken);
+  CachingLex(Result);
   ReturnedToken = true;
   break;
 case CLK_LexAfterModuleImport:
   ReturnedToken = LexAfterModuleImport(Result);
+  Result.setFlag(Token::IsNewToken);
   break;
 }
   } while (!ReturnedToken);
@@ -911,7 +915,8 @@
 
   // Update ImportSeqState to track our position within a C++20 import-seq
   // if this token is being produced as a result of phase 4 of translation.
-  if (getLangOpts().CPlusPlusModules && LexLevel == 1 && IsNewToken) {
+  if (getLangOpts().CPlusPlusModules && LexLevel == 1 &&
+  Result.getFlag(Token::IsNewToken)) {
 switch (Result.getKind()) {
 case tok::l_paren: case tok::l_square: case tok::l_brace:
   ImportSeqState.handleOpenBracket();
@@ -952,6 +957,8 @@
 
   LastTokenWasAt = Result.is(tok::at);
   --LexLevel;
+  if (OnToken && LexLevel == 0 && Result.getFlag(Token::IsNewToken))
+OnToken(Result);
 }
 
 /// Lex a header-name token (including one formed from header-name-tokens if
Index: clang/lib/Lex/PPCaching.cpp
===
--- clang/lib/Lex/PPCaching.cpp
+++ clang/lib/Lex/PPCaching.cpp
@@ -45,7 +45,7 @@
   recomputeCurLexerKind();
 }
 
-void Preprocessor::CachingLex(Token &Result, bool &IsNewToken) {
+void Preprocessor::CachingLex(Token &Result) {
   if (!InCachingLexMode())
 return;
 
@@ -55,7 +55,8 @@
 
   if (CachedLexPos < CachedTokens.size()) {
 Result = CachedTokens[CachedLexPos++];
-IsNewToken = false;
+// FIXME: do this flag when writing to CachedTokens.
+Result.clearFlag(Token::IsNewToken);
 return;
   }
 
Index: clang/include/clang/Lex/TokenLexer.h
===
--- clang/include/clang/Lex/TokenLexer.h
+++ clang/include/clang/Lex/TokenLexer.h
@@ -147,6 +147,10 @@
   /// preprocessor directive.
   bool isParsingPreprocessorDirective() const;
 
+  /// Returns true iff the TokenLexer is expanding a macro and not replaying a
+  /// stream of tokens.
+  bool isMacroExpansion() const { return Macro != nullptr; }
+
 private:
   void destroy();
 
Index: clang/include/clang/Lex/Token.h
===
--- clang/include/clang/Lex/Token.h
+++ clang/include/clang/Lex/Token.h
@@ -70,20 +70,22 @@
 public:
   // Various flags set per token:
   enum TokenFlags {
-StartOfLine   = 0x01,  // At start of line or only after whitespace
-   // (considering the line after macro expansion).
-LeadingSpace  = 0x02,  // Whitespace exists before this token (considering
-   // whitespace after macro expansion).
-DisableExpand = 0x04,  // This identifier may never be macro expanded.
-NeedsCleaning = 0x08,  // Contained an escaped newline or trigraph.
+StartOfLine = 0x01,   // At start of line or only after whitespace
+  // (considering the line after macro expansion).
+LeadingSpace = 0x02,  // Whitespace exists before this token (considering
+  // whitespace after macro expansion).
+DisableExpand = 0x04, // This identifier may never be macro expanded.
+NeedsCleaning = 0x08, // Contained an escaped newline or trigraph.
 LeadingEmptyMacro = 0x10, // Empty macro exists before this token.
-HasUDSuffix = 0x20,// This string or character literal has a ud-suffix

r360699 - Restore test files accidentally deleted in r354839

2019-05-14 Thread Reid Kleckner via cfe-commits

Author: rnk
Date: Tue May 14 11:51:07 2019
New Revision: 360699

URL: http://llvm.org/viewvc/llvm-project?rev=360699&view=rev
Log:
Restore test files accidentally deleted in r354839

I think there must be a bug in git-llvm causing parent directories to be
deleted when the diff deletes files in a subdirectory. Perhaps it is
Windows-only.

There has been a behavior change, so some of these tests now fail. I
have marked them XFAIL and will fix them in a follow-up to separate the
changes.

Added:
cfe/trunk/test/ASTMerge/anonymous-fields/
cfe/trunk/test/ASTMerge/anonymous-fields/Inputs/
cfe/trunk/test/ASTMerge/anonymous-fields/Inputs/anonymous-fields1.cpp
cfe/trunk/test/ASTMerge/anonymous-fields/Inputs/anonymous-fields2.cpp
cfe/trunk/test/ASTMerge/anonymous-fields/test.cpp
cfe/trunk/test/ASTMerge/asm/
cfe/trunk/test/ASTMerge/asm/Inputs/
cfe/trunk/test/ASTMerge/asm/Inputs/asm-function.cpp
cfe/trunk/test/ASTMerge/asm/test.cpp
cfe/trunk/test/ASTMerge/category/
cfe/trunk/test/ASTMerge/category/Inputs/
cfe/trunk/test/ASTMerge/category/Inputs/category1.m
cfe/trunk/test/ASTMerge/category/Inputs/category2.m
cfe/trunk/test/ASTMerge/category/test.m
cfe/trunk/test/ASTMerge/class/
cfe/trunk/test/ASTMerge/class-template/
cfe/trunk/test/ASTMerge/class-template-partial-spec/
cfe/trunk/test/ASTMerge/class-template-partial-spec/Inputs/

cfe/trunk/test/ASTMerge/class-template-partial-spec/Inputs/class-template-partial-spec1.cpp

cfe/trunk/test/ASTMerge/class-template-partial-spec/Inputs/class-template-partial-spec2.cpp
cfe/trunk/test/ASTMerge/class-template-partial-spec/test.cpp
cfe/trunk/test/ASTMerge/class-template/Inputs/
cfe/trunk/test/ASTMerge/class-template/Inputs/class-template1.cpp
cfe/trunk/test/ASTMerge/class-template/Inputs/class-template2.cpp
cfe/trunk/test/ASTMerge/class-template/test.cpp
cfe/trunk/test/ASTMerge/class/Inputs/
cfe/trunk/test/ASTMerge/class/Inputs/class1.cpp
cfe/trunk/test/ASTMerge/class/Inputs/class2.cpp
cfe/trunk/test/ASTMerge/class/test.cpp
cfe/trunk/test/ASTMerge/class2/
cfe/trunk/test/ASTMerge/class2/Inputs/
cfe/trunk/test/ASTMerge/class2/Inputs/class3.cpp
cfe/trunk/test/ASTMerge/class2/test.cpp
cfe/trunk/test/ASTMerge/codegen-body/
cfe/trunk/test/ASTMerge/codegen-body/Inputs/
cfe/trunk/test/ASTMerge/codegen-body/Inputs/body1.c
cfe/trunk/test/ASTMerge/codegen-body/Inputs/body2.c
cfe/trunk/test/ASTMerge/codegen-body/test.c
cfe/trunk/test/ASTMerge/codegen-exprs/
cfe/trunk/test/ASTMerge/codegen-exprs/Inputs/
cfe/trunk/test/ASTMerge/codegen-exprs/Inputs/exprs1.c
cfe/trunk/test/ASTMerge/codegen-exprs/Inputs/exprs2.c
cfe/trunk/test/ASTMerge/codegen-exprs/test.c
cfe/trunk/test/ASTMerge/enum/
cfe/trunk/test/ASTMerge/enum/Inputs/
cfe/trunk/test/ASTMerge/enum/Inputs/enum1.c
cfe/trunk/test/ASTMerge/enum/Inputs/enum2.c
cfe/trunk/test/ASTMerge/enum/test.c
cfe/trunk/test/ASTMerge/exprs/
cfe/trunk/test/ASTMerge/exprs-cpp/
cfe/trunk/test/ASTMerge/exprs-cpp/Inputs/
cfe/trunk/test/ASTMerge/exprs-cpp/Inputs/exprs3.cpp
cfe/trunk/test/ASTMerge/exprs-cpp/test.cpp
cfe/trunk/test/ASTMerge/exprs/Inputs/
cfe/trunk/test/ASTMerge/exprs/Inputs/exprs1.c
cfe/trunk/test/ASTMerge/exprs/Inputs/exprs2.c
cfe/trunk/test/ASTMerge/exprs/test.c
cfe/trunk/test/ASTMerge/function/
cfe/trunk/test/ASTMerge/function-cpp/
cfe/trunk/test/ASTMerge/function-cpp/Inputs/
cfe/trunk/test/ASTMerge/function-cpp/Inputs/function-1.cpp
cfe/trunk/test/ASTMerge/function-cpp/test.cpp
cfe/trunk/test/ASTMerge/function/Inputs/
cfe/trunk/test/ASTMerge/function/Inputs/function1.c
cfe/trunk/test/ASTMerge/function/Inputs/function2.c
cfe/trunk/test/ASTMerge/function/test.c
cfe/trunk/test/ASTMerge/inheritance/
cfe/trunk/test/ASTMerge/inheritance/Inputs/
cfe/trunk/test/ASTMerge/inheritance/Inputs/inheritance-base.cpp
cfe/trunk/test/ASTMerge/inheritance/test.cpp
cfe/trunk/test/ASTMerge/init-ctors/
cfe/trunk/test/ASTMerge/init-ctors/Inputs/
cfe/trunk/test/ASTMerge/init-ctors/Inputs/init-ctors-classes.cpp
cfe/trunk/test/ASTMerge/init-ctors/test.cpp
cfe/trunk/test/ASTMerge/injected-class-name-decl/
cfe/trunk/test/ASTMerge/injected-class-name-decl/Inputs/
cfe/trunk/test/ASTMerge/injected-class-name-decl/Inputs/inject1.cpp
cfe/trunk/test/ASTMerge/injected-class-name-decl/Inputs/inject2.cpp
cfe/trunk/test/ASTMerge/injected-class-name-decl/test.cpp
cfe/trunk/test/ASTMerge/interface/
cfe/trunk/test/ASTMerge/interface/Inputs/
cfe/trunk/test/ASTMerge/interface/Inputs/interface1.m
cfe/trunk/test/ASTMerge/interface/Inputs/interface2.m
cfe/trunk/test/ASTMerge/interface/test.m
cfe/trunk/test/ASTMerge/macro/
cfe/trunk/test/ASTMerge/macro/Inputs/
cfe/trunk/test/ASTMerge/macro/Inputs/macro.modulemap
cfe/trunk/test/ASTMerge/macro/Inputs/mac

Re: r359960 - Reduce amount of work ODR hashing does.

2019-05-14 Thread David Blaikie via cfe-commits

On Tue, May 7, 2019 at 7:40 PM Richard Trieu  wrote:
>
>
> From: David Blaikie 
> Date: Mon, May 6, 2019 at 4:39 PM
> To: Richard Trieu
> Cc: cfe-commits
>
>> On Mon, May 6, 2019 at 4:24 PM Richard Trieu  wrote:
>> >
>> > There was no cycle for this crash.
>>
>> Oh, yeah, didn't mean to imply there were - but that a system designed
>> to prevent cycles might also be used/help prevent redundant work like
>> this.
>>
>> > What happened is that an exponential runtime is reduced to a linear 
>> > runtime.  Without this revision, ODR hashing would have worked if the 
>> > machine had enough memory and the user waited long enough.
>> >
>> > void foo(int a, int b) {}
>> > When computing the ODR hash for function foo, it will visit the type int 
>> > twice, once per parameter.  In general, re-visiting types shouldn't be a 
>> > problem, and in most cases, should be pretty fast.
>>
>> It does mean some potentially problematic worst-case situations where
>> non-trivial types are mentioned more than once (eg: if, instead of
>> int, it was a complex struct type - it wouldn't cycle, but it would do
>> all that work twice (or many more times if it appears in more places
>> in the entity being hashed)
>
>
> See below in the answer to DWARF.  ODRHash did have a system, it worked for a 
> while until it didn't, and was since removed.
>>
>>
>> > class S {
>> >   void bar(S* s);
>> > };
>> > There's actually two ways to visit the Decl behind S, 
>> > ODR::AddCXXRecordDecl and ODR::AddDecl.  When computing the ODR hash of S, 
>> > ODR::AddCXXRecordDecl is used for a deep dive into the AST of S.  When 
>> > reaching S another way, (via FunctionDecl bar, parameter s, PointerType 
>> > S*, RecordType S), then the CXXRecordDecl gets processed through 
>> > ODR::AddDecl, which only processes enough information to identify S, but 
>> > not any of its deeper details.  This allows self-reference without 
>> > introducing cycles.
>>
>> Ah, OK - specifically to break the cycle.
>>
>> So the ODR hash of the function "void f(S*)" doesn't hash the
>> implementation of S, (it uses AddDecl, not AddCXXRecordDecl)? But if
>> it were "void f(S)" it would hash S? What about a member function that
>> takes a parameter by value? ("struct S { void bar(S); }")
>
>
> The three functions AddCXXRecordDecl, AddFunctionDecl, and AddEnumDecl are 
> the entry points from outside to use the ODRHash and nothing inside ODRHash 
> will call these functions.  That means hashing "class S {};"  
> AddCXXRecordDecl is called with S.  Every other example, "void f(S)", "void 
> bar(S);", etc will be called into AddDecl.  The next question is probably, 
> how do you know if two functions "void f(S)" in two files refer to same class 
> S?  The answer is, ODRHash doesn't know and doesn't care.  But when Clang 
> imports both "void f(S)" functions, it will also import both S classes.  
> Since Clang checks, ODRHash doesn't need to.

Clang checks this by entering ODRHash separately (via AddCXXRecordDecl).

So it's sort of an inductive proof based on the assumptions of the ODR
- >"void f(S)" is ODR compliant if S is ODR compliant<, etc.

That makes sense, though still does mean potentially a lot of
redundant work in cases like this bug, but never circular work -
because anonymous types can't form cycles (because they have no name
to do that).

I think it still /might/ be worth using a type stack/hash so there's
never redundant work, even if it's non-circular work.

>>
>>
>> > I think it would be possible to add some checks in debug mode to catch 
>> > cycles.  I'm not sure it can detect redundant work as the function foo 
>> > example above shows that visiting the same types over multiple times is 
>> > expected.
>>
>> Both for efficiency and to avoid these cycles, it might be worthwhile
>> to consider a different way to resolve this issue
>>
>> The reason these ideas come to my mind is that DWARF has a type hash
>> that works in a different way to avoid cycles and redundant work.
>>
>> http://dwarfstd.org/doc/DWARF5.pdf - 7.32, Type Signature Computation.
>> It works by assigning every type a number when it's first encountered
>> (even before its contents are hashed), and if it's ever encountered
>> again, hash the number again rather than going back into hashing the
>> implementation.
>>
> Originally, ODR hashing did have a system similar to what DWARF had.  
> Relevant portions of 7.32 are 1, 4.a, and 4.b.  Basically, maintain a list of 
> Type's, when first visiting a Type, add it to the list and process it, and if 
> the Type is ever seen again, use the index number instead reprocessing.  
> Worked well, and then the AST had a small change in it where now we needed 
> two different Type's to hash to the same thing.  
> https://reviews.llvm.org/rL335853 ripped this out.  It's possible to replace 
> it, but it needs to be better than what we currently have.

Ah - I think I understand/see. The uniqueness of Type*s is based on
the profile hashing and it varies in intere

r360701 - Update ASTMerge FileCheck test expectations

2019-05-14 Thread Reid Kleckner via cfe-commits

Author: rnk
Date: Tue May 14 12:02:39 2019
New Revision: 360701

URL: http://llvm.org/viewvc/llvm-project?rev=360701&view=rev
Log:
Update ASTMerge FileCheck test expectations

I belive many of these diagnostics changed from errors to warnings in
r357394. I've simply mechanically updated the tests, but whoever owns
this code should probably audit for unintented behavior changes. I
wasn't able to find a flag to make these warnings errors again.

Modified:
cfe/trunk/test/ASTMerge/category/test.m
cfe/trunk/test/ASTMerge/class-template-partial-spec/test.cpp
cfe/trunk/test/ASTMerge/class-template/test.cpp
cfe/trunk/test/ASTMerge/enum/test.c
cfe/trunk/test/ASTMerge/function/test.c
cfe/trunk/test/ASTMerge/interface/test.m
cfe/trunk/test/ASTMerge/namespace/test.cpp
cfe/trunk/test/ASTMerge/property/test.m
cfe/trunk/test/ASTMerge/struct/test.c
cfe/trunk/test/ASTMerge/typedef/test.c
cfe/trunk/test/ASTMerge/var/test.c

Modified: cfe/trunk/test/ASTMerge/category/test.m
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ASTMerge/category/test.m?rev=360701&r1=360700&r2=360701&view=diff
==
--- cfe/trunk/test/ASTMerge/category/test.m (original)
+++ cfe/trunk/test/ASTMerge/category/test.m Tue May 14 12:02:39 2019
@@ -1,13 +1,11 @@
-// FIXME: Errors are now warnings.
-// XFAIL: *
 // RUN: %clang_cc1 -emit-pch -o %t.1.ast %S/Inputs/category1.m
 // RUN: %clang_cc1 -emit-pch -o %t.2.ast %S/Inputs/category2.m
-// RUN: not %clang_cc1 -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only 
%s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only %s 
2>&1 | FileCheck %s
 
-// CHECK: category2.m:18:1: error: instance method 'method2' has incompatible 
result types in different translation units ('float' vs. 'int')
+// CHECK: category2.m:18:1: warning: instance method 'method2' has 
incompatible result types in different translation units ('float' vs. 'int')
 // CHECK: category1.m:16:1: note: instance method 'method2' also declared here
-// CHECK: category2.m:26:1: error: instance method 'method3' has incompatible 
result types in different translation units ('float' vs. 'int')
+// CHECK: category2.m:26:1: warning: instance method 'method3' has 
incompatible result types in different translation units ('float' vs. 'int')
 // CHECK: category1.m:24:1: note: instance method 'method3' also declared here
-// CHECK: category2.m:48:1: error: instance method 'blah' has incompatible 
result types in different translation units ('int' vs. 'float')
+// CHECK: category2.m:48:1: warning: instance method 'blah' has incompatible 
result types in different translation units ('int' vs. 'float')
 // CHECK: category1.m:46:1: note: instance method 'blah' also declared here
-// CHECK: 3 errors generated.
+// CHECK: 3 warnings generated.

Modified: cfe/trunk/test/ASTMerge/class-template-partial-spec/test.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ASTMerge/class-template-partial-spec/test.cpp?rev=360701&r1=360700&r2=360701&view=diff
==
--- cfe/trunk/test/ASTMerge/class-template-partial-spec/test.cpp (original)
+++ cfe/trunk/test/ASTMerge/class-template-partial-spec/test.cpp Tue May 14 
12:02:39 2019
@@ -1,8 +1,8 @@
-// FIXME: Errors are now warnings.
+// FIXME: Crashes after r357394
 // XFAIL: *
 // RUN: %clang_cc1 -emit-pch -std=c++1z -o %t.1.ast 
%S/Inputs/class-template-partial-spec1.cpp
 // RUN: %clang_cc1 -emit-pch -std=c++1z -o %t.2.ast 
%S/Inputs/class-template-partial-spec2.cpp
-// RUN: not %clang_cc1 -std=c++1z -ast-merge %t.1.ast -ast-merge %t.2.ast 
-fsyntax-only %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -std=c++1z -ast-merge %t.1.ast -ast-merge %t.2.ast 
-fsyntax-only %s 2>&1 | FileCheck %s
 
 static_assert(sizeof(**SingleSource.member) == sizeof(**SingleDest.member));
 static_assert(sizeof(SecondDoubleSource.member) == 
sizeof(SecondDoubleDest.member));
@@ -11,17 +11,17 @@ static_assert(sizeof(Z0Source.member) ==
 static_assert(sizeof(Dst::Z0Dst.member) == sizeof(double));
 static_assert(sizeof(One::Child1>::member) == sizeof(double));
 
-// CHECK: class-template-partial-spec2.cpp:21:32: error: external variable 
'X1' declared with incompatible types in different translation units 
('TwoOptionTemplate' vs. 'TwoOptionTemplate')
+// CHECK: class-template-partial-spec2.cpp:21:32: warning: external variable 
'X1' declared with incompatible types in different translation units 
('TwoOptionTemplate' vs. 'TwoOptionTemplate')
 // CHECK: class-template-partial-spec1.cpp:21:31: note: declared here with 
type 'TwoOptionTemplate'
 
-// CHECK: class-template-partial-spec2.cpp:24:29: error: external variable 
'X4' declared with incompatible types in different translation units 
('TwoOptionTemplate' vs. 'TwoOptionTemplate')
+// CHECK: class-template-partial-spec2.cpp:24:29: warning: external variable 
'X

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

2019-05-14 Thread Don Hinton via Phabricator via cfe-commits

hintonda added a comment.

I'll be happy to commit for you, but will give it till tomorrow just to make 
sure no one else has any additional comments.

Thanks again!




Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst:262
+not been used on code with a compatibility requirements of OpenMP prior to
+version 5. It is **intentional** that this check does not make any attempt to
+not issue diagnostics on OpenMP for loops.

Could you reword this last line to remove the double negative?  Perhaps 
something like:

```
This check makes no attempt to exclude incorrect diagnostics for OpenMP loops 
prior to OpenMP 5.
```


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

https://reviews.llvm.org/D61827



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

[PATCH] D61281: [clang-format] Fixed self assignment

2019-05-14 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay requested changes to this revision.
MyDeveloperDay added a comment.
This revision now requires changes to proceed.

please simply remove line 249


Repository:
  rC Clang

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

https://reviews.llvm.org/D61281



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

r360703 - Temporarily revert "Change -gz and -Wa, --compress-debug-sections to use gABI compression (SHF_COMPRESSED)"

2019-05-14 Thread Eric Christopher via cfe-commits

Author: echristo
Date: Tue May 14 12:40:42 2019
New Revision: 360703

URL: http://llvm.org/viewvc/llvm-project?rev=360703&view=rev
Log:
Temporarily revert "Change -gz and -Wa,--compress-debug-sections to use gABI 
compression (SHF_COMPRESSED)"

This affects users of older (pre 2.26) binutils in such a way that they can't 
necessarily
work around it as it doesn't support the compress option on the command line. 
Reverting
to unblock them and we can revisit whether to make this change now or fix how 
we want
to express the option.

This reverts commit bdb21337e6e1732c9895966449c33c408336d295/r360403.

Modified:
cfe/trunk/docs/ReleaseNotes.rst
cfe/trunk/lib/Frontend/CompilerInvocation.cpp
cfe/trunk/tools/driver/cc1as_main.cpp

Modified: cfe/trunk/docs/ReleaseNotes.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=360703&r1=360702&r2=360703&view=diff
==
--- cfe/trunk/docs/ReleaseNotes.rst (original)
+++ cfe/trunk/docs/ReleaseNotes.rst Tue May 14 12:40:42 2019
@@ -77,10 +77,7 @@ Modified Compiler Flags
 
 - `clang -dumpversion` now returns the version of Clang itself.
 
-- On ELF, ``-gz`` now defaults to ``-gz=zlib``. It produces ``SHF_COMPRESSED``
-  style compression of debug information. GNU binutils 2.26 or newer, or lld is
-  required to link produced object files. Use ``-gz=zlib-gnu`` to get the old
-  behavior.
+- ...
 
 New Pragmas in Clang
 

Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=360703&r1=360702&r2=360703&view=diff
==
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Tue May 14 12:40:42 2019
@@ -1052,7 +1052,8 @@ static bool ParseCodeGenArgs(CodeGenOpti
   if (const Arg *A = Args.getLastArg(OPT_compress_debug_sections,
  OPT_compress_debug_sections_EQ)) {
 if (A->getOption().getID() == OPT_compress_debug_sections) {
-  Opts.setCompressDebugSections(llvm::DebugCompressionType::Z);
+  // TODO: be more clever about the compression type auto-detection
+  Opts.setCompressDebugSections(llvm::DebugCompressionType::GNU);
 } else {
   auto DCT = llvm::StringSwitch(A->getValue())
  .Case("none", llvm::DebugCompressionType::None)

Modified: cfe/trunk/tools/driver/cc1as_main.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/driver/cc1as_main.cpp?rev=360703&r1=360702&r2=360703&view=diff
==
--- cfe/trunk/tools/driver/cc1as_main.cpp (original)
+++ cfe/trunk/tools/driver/cc1as_main.cpp Tue May 14 12:40:42 2019
@@ -221,7 +221,8 @@ bool AssemblerInvocation::CreateFromArgs
   if (const Arg *A = Args.getLastArg(OPT_compress_debug_sections,
  OPT_compress_debug_sections_EQ)) {
 if (A->getOption().getID() == OPT_compress_debug_sections) {
-  Opts.CompressDebugSections = llvm::DebugCompressionType::Z;
+  // TODO: be more clever about the compression type auto-detection
+  Opts.CompressDebugSections = llvm::DebugCompressionType::GNU;
 } else {
   Opts.CompressDebugSections =
   llvm::StringSwitch(A->getValue())


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

Re: r360403 - Change -gz and -Wa, --compress-debug-sections to use gABI compression (SHF_COMPRESSED)

2019-05-14 Thread Eric Christopher via cfe-commits

Hi Ray,

I've temporarily reverted this here:

echristo@jhereg ~/s/llvm-project> git llvm push
Pushing 1 commit:
  fda79815a33 Temporarily revert "Change -gz and
-Wa,--compress-debug-sections to use gABI compression
(SHF_COMPRESSED)"
Sendingcfe/trunk/docs/ReleaseNotes.rst
Sendingcfe/trunk/lib/Frontend/CompilerInvocation.cpp
Sendingcfe/trunk/tools/driver/cc1as_main.cpp
Transmitting file data ...done
Committing transaction...
Committed revision 360703.
Committed fda79815a33 to svn.

From my message:

This affects users of older (pre 2.26) binutils in such a way that
they can't necessarily
work around it as it doesn't support the compress option on the
command line. Reverting
to unblock them and we can revisit whether to make this change now
or fix how we want
to express the option.

I'm not sure what we want to do here, but wanted to unblock people in
the meantime. What do you think?

Thanks!

-eric


On Thu, May 9, 2019 at 7:05 PM Fangrui Song via cfe-commits
 wrote:
>
> Author: maskray
> Date: Thu May  9 19:08:21 2019
> New Revision: 360403
>
> URL: http://llvm.org/viewvc/llvm-project?rev=360403&view=rev
> Log:
> Change -gz and -Wa,--compress-debug-sections to use gABI compression 
> (SHF_COMPRESSED)
>
> Since July 15, 2015 (binutils-gdb commit
> 19a7fe52ae3d0971e67a134bcb1648899e21ae1c, included in 2.26), gas
> --compress-debug-sections=zlib (gcc -gz) means zlib-gabi:
> SHF_COMPRESSED. Before that it meant zlib-gnu (.zdebug).
>
> clang's -gz was introduced in rC306115 (Jun 2017) to indicate zlib-gnu. It
> is 2019 now and it is not unreasonable to assume users of the new
> feature to have new linkers (ld.bfd/gold >= 2.26, lld >= rLLD273661).
>
> Change clang's default accordingly to improve standard conformance.
> zlib-gnu becomes out of fashion and gets poorer toolchain support.
> Its mangled names confuse tools and are more likely to cause problems.
>
> Reviewed By: compnerd
>
> Differential Revision: https://reviews.llvm.org/D61689
>
> Modified:
> cfe/trunk/docs/ReleaseNotes.rst
> cfe/trunk/lib/Frontend/CompilerInvocation.cpp
> cfe/trunk/tools/driver/cc1as_main.cpp
>
> Modified: cfe/trunk/docs/ReleaseNotes.rst
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=360403&r1=360402&r2=360403&view=diff
> ==
> --- cfe/trunk/docs/ReleaseNotes.rst (original)
> +++ cfe/trunk/docs/ReleaseNotes.rst Thu May  9 19:08:21 2019
> @@ -77,7 +77,10 @@ Modified Compiler Flags
>
>  - `clang -dumpversion` now returns the version of Clang itself.
>
> -- ...
> +- On ELF, ``-gz`` now defaults to ``-gz=zlib``. It produces 
> ``SHF_COMPRESSED``
> +  style compression of debug information. GNU binutils 2.26 or newer, or lld 
> is
> +  required to link produced object files. Use ``-gz=zlib-gnu`` to get the old
> +  behavior.
>
>  New Pragmas in Clang
>  
>
> Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=360403&r1=360402&r2=360403&view=diff
> ==
> --- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)
> +++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Thu May  9 19:08:21 2019
> @@ -1052,8 +1052,7 @@ static bool ParseCodeGenArgs(CodeGenOpti
>if (const Arg *A = Args.getLastArg(OPT_compress_debug_sections,
>   OPT_compress_debug_sections_EQ)) {
>  if (A->getOption().getID() == OPT_compress_debug_sections) {
> -  // TODO: be more clever about the compression type auto-detection
> -  Opts.setCompressDebugSections(llvm::DebugCompressionType::GNU);
> +  Opts.setCompressDebugSections(llvm::DebugCompressionType::Z);
>  } else {
>auto DCT = 
> llvm::StringSwitch(A->getValue())
>   .Case("none", llvm::DebugCompressionType::None)
>
> Modified: cfe/trunk/tools/driver/cc1as_main.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/driver/cc1as_main.cpp?rev=360403&r1=360402&r2=360403&view=diff
> ==
> --- cfe/trunk/tools/driver/cc1as_main.cpp (original)
> +++ cfe/trunk/tools/driver/cc1as_main.cpp Thu May  9 19:08:21 2019
> @@ -221,8 +221,7 @@ bool AssemblerInvocation::CreateFromArgs
>if (const Arg *A = Args.getLastArg(OPT_compress_debug_sections,
>   OPT_compress_debug_sections_EQ)) {
>  if (A->getOption().getID() == OPT_compress_debug_sections) {
> -  // TODO: be more clever about the compression type auto-detection
> -  Opts.CompressDebugSections = llvm::DebugCompressionType::GNU;
> +  Opts.CompressDebugSections = llvm::DebugCompressionType::Z;
>  } else {
>Opts.CompressDebugSections =
>llvm::StringSwitch(A->getValue())
>
>
>

[PATCH] D61689: Change -gz and -Wa,--compress-debug-sections to use gABI compression (SHF_COMPRESSED)

2019-05-14 Thread Jordan Rupprecht via Phabricator via cfe-commits

rupprecht added a comment.

So, while I think this is an //entirely// reasonable assumption in most cases, 
it's also not one that provides any kind of workaround for the few cases where 
it's not universally true.

- As mentioned in the patch, this effectively changes the default from 
`-gz=zlib-gnu` to `-gz=zlib`. Anyone with an older toolchain that wants the old 
behavior can set `-gz=zlib-gnu`. Seems OK so far.
- However, passing the `-gz` flag //also// implies sending 
`--compress-debug-sections=XXX` to the assembler, which -- if someone is using 
`-no-integrated-as` has an older toolchain -- is not a supported option (at 
least the variant that takes a value).

In other works, for the few users that have an older toolchain, this provides 
literally no workaround. Not setting the flag causes failures with an older 
linker, and setting the flag causes failures with the assembler.

(echristo reverted this in rL360703 , I 
wanted to add some context)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61689



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

[PATCH] D61281: [clang-format] Fixed self assignment

2019-05-14 Thread Dávid Bolvanský via Phabricator via cfe-commits

xbolva00 updated this revision to Diff 199494.
xbolva00 added a comment.

removed line


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

https://reviews.llvm.org/D61281

Files:
  lib/Format/FormatTokenLexer.cpp


Index: lib/Format/FormatTokenLexer.cpp
===
--- lib/Format/FormatTokenLexer.cpp
+++ lib/Format/FormatTokenLexer.cpp
@@ -246,7 +246,6 @@
   StringRef(Identifier->TokenText.begin(),
 Question->TokenText.end() - Identifier->TokenText.begin());
   Identifier->ColumnWidth += Question->ColumnWidth;
-  Identifier->Type = Identifier->Type;
   Tokens.erase(Tokens.end() - 1);
   return true;
 }


Index: lib/Format/FormatTokenLexer.cpp
===
--- lib/Format/FormatTokenLexer.cpp
+++ lib/Format/FormatTokenLexer.cpp
@@ -246,7 +246,6 @@
   StringRef(Identifier->TokenText.begin(),
 Question->TokenText.end() - Identifier->TokenText.begin());
   Identifier->ColumnWidth += Question->ColumnWidth;
-  Identifier->Type = Identifier->Type;
   Tokens.erase(Tokens.end() - 1);
   return true;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61790: [C++20] add consteval specifier

2019-05-14 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added a comment.

Thanks for working on this!

Comments on the general approach:

- We should only evaluate each immediate invocation once (this will become 
essential once we start supporting reflection -- and particularly operations 
that mutate the AST -- inside `consteval` functions).
- Each time an immediate invocation is formed, you should create a 
`ConstantExpr` AST node wrapping that call to reflect that it is a constant.
- You should change `ConstantExpr` to store an `APValue` representing the 
evaluated value of the expression.

Most of the above should be done as a separate preparatory change; we should be 
able to remove a lot of the existing ad-hoc caching of evaluated values (eg, 
for enumerators and the initializers of variables) at the same time.

Please also split this into smaller pieces. If you split off a patch to just 
add the keyword, parsing, and AST representation for the `consteval` specifier 
(but treat it identically to `constexpr` for constant evaluation purposes), 
that'd be a good first patch for this feature.




Comment at: clang/include/clang/AST/Decl.h:2102
   /// Whether this is a (C++11) constexpr function or constexpr constructor.
   bool isConstexpr() const { return FunctionDeclBits.IsConstexpr; }
   void setConstexpr(bool IC) { FunctionDeclBits.IsConstexpr = IC; }

Please rename this to `isConstexprSpecified()` (compare this to how we model 
`inline`, which has similar behavior: it can be explicit, or can be implied by 
other properties of the function).



Comment at: clang/include/clang/AST/Decl.h:2109
+
+  bool isConstexprOrConsteval() const { return isConstexpr() || isConsteval(); 
}
+

Both the `constexpr` and `consteval` specifiers make a function a "constexpr 
function", so I think this should be called simply `isConstexpr`; callers that 
want to distinguish `constexpr` from `consteval` can use `isConsteval()` (or we 
can add a `getConstexprKind()` or similar function).



Comment at: clang/include/clang/AST/DeclCXX.h:2115
+QualType T, TypeSourceInfo *TInfo, StorageClass SC,
+bool isInline, bool isConstexpr, bool isConsteval,
+SourceLocation EndLocation)

Instead of the three bool flags in a row here (which will make call sites hard 
to read), consider using an enum, perhaps:

```
enum class ConstexprSpecifierKind { None, Constexpr, Consteval };
```



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2297-2300
+def err_consteval_cannot_be_constant_eval : Error<
+  "call to %0 cannot be constant evaluated">;
+def note_argument_n_cannot_be_constant_eval : Note<
+  "argument %0 cannot be constant evaluated">;

It would be useful in these diagnostics to explain why the call is required to 
be constant evaluated; please also use the standard terminology "constant 
expression" here. (Eg, "call to consteval function %0 is not a constant 
expression")



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2308-2314
 def err_constexpr_tag : Error<
   "%select{class|struct|interface|union|enum}0 cannot be marked constexpr">;
-def err_constexpr_dtor : Error<"destructor cannot be marked constexpr">;
+def err_constexpr_dtor : Error<"destructor cannot be marked %0">;
+def err_invalid_consteval_decl_kind : Error<
+  "operator %select{new|delete|new[]|delete[]}0 cannot be marked consteval">;
+def err_take_adress_of_consteval_decl : Error<
+  "taking address of a %0">;

In these diagnostics (including the existing one for tags), we should say 
"declared constexpr" not "marked constexpr".



Comment at: clang/include/clang/Basic/TokenKinds.def:390-391
 
+// C++ consteval proposal
+CXX2A_KEYWORD(consteval , 0)
+

This and `char8_t` are both adopted C++2a features now (they'e not just 
proposals any more); please change the comment to just "C++2a keywords".



Comment at: clang/include/clang/Sema/DeclSpec.h:404
   SourceLocation FS_forceinlineLoc;
-  SourceLocation FriendLoc, ModulePrivateLoc, ConstexprLoc;
+  SourceLocation FriendLoc, ModulePrivateLoc, ConstexprLoc, ConstevalLoc;
   SourceLocation TQ_pipeLoc;

I think it would be preferable to track only one location here (for the 
`constexpr` / `consteval` specifier) and store a constexpr specifier kind 
instead of `Constexpr_specified`, like we do for the other kinds of specifier 
for which we allow only one of a set of keywords.



Comment at: clang/include/clang/Sema/Sema.h:985-986
+///   cases in a switch statement).
+/// - "immediate function context" in C++2a terms, a call to a function
+///   marked as consteval
 ConstantEvaluated,

An "immediate function context" is also "potentially evaluated"; I think what 
we want to say here is something like "The current context

[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

2019-05-14 Thread Torbjörn Klatt via Phabricator via cfe-commits

torbjoernk updated this revision to Diff 199496.
torbjoernk added a comment.

minor rewording


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

https://reviews.llvm.org/D61827

Files:
  clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
  clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp

Index: clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
===
--- clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
+++ clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
@@ -776,17 +776,20 @@
   // CHECK-FIXES: for (auto P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
-  // V.begin() returns a user-defined type 'iterator' which, since it's
-  // different from const_iterator, disqualifies these loops from
-  // transformation.
   dependent V;
   for (dependent::const_iterator It = V.begin(); It != V.end(); ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 
   for (dependent::const_iterator It(V.begin()); It != V.end(); ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 }
 
 } // namespace SingleIterator
@@ -991,18 +994,26 @@
   // CHECK-FIXES: for (int & I : Dep)
   // CHECK-FIXES-NEXT: auto H2 = [&]() { int R = I + 2; };
 
-  // FIXME: It doesn't work with const iterators.
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H3 = [I]() { int R = *I; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : Dep)
+  // CHECK-FIXES-NEXT: auto H3 = [&I]() { int R = I; };
 
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H4 = [&]() { int R = *I + 1; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : Dep)
+  // CHECK-FIXES-NEXT: auto H4 = [&]() { int R = I + 1; };
 
   for (dependent::const_iterator I = Dep.begin(), E = Dep.end();
I != E; ++I)
 auto H5 = [=]() { int R = *I; };
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int R : Dep)
+  // CHECK-FIXES-NEXT: auto H5 = [=]() { };
 }
 
 void captureByValue() {
Index: clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
===
--- clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
+++ clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp
@@ -369,7 +369,7 @@
 // CHECK-FIXES: for (auto Val_int_ptr : Val_int_ptrs)
   }
 
-  // This container uses an iterator where the derefence type is a typedef of
+  // This container uses an iterator where the dereference type is a typedef of
   // a reference type. Make sure non-const auto & is still used. A failure here
   // means canonical types aren't being tested.
   {
@@ -431,19 +431,22 @@
   // CHECK-FIXES: for (auto P : *Ps)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", P.X);
 
-  // V.begin() returns a user-defined type 'iterator' which, since it's
-  // different from const_iterator, disqualifies these loops from
-  // transformation.
   dependent V;
   for (dependent::const_iterator It = V.begin(), E = V.end();
It != E; ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 
   for (dependent::const_iterator It(V.begin()), E = V.end();
It != E; ++It) {
 printf("Fibonacci number is %d\n", *It);
   }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int It : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", It);
 }
 
 // Tests to ensure that an implicit 'this' is picked up as the container.
Index: clang-tools-extra/docs/clang-tidy/index.rst
===
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rst
@@ -258,6 +258,8 @@
   value:   'some value'
   ...
 
+.. _clang-tidy-nolint:
+
 Suppressing Undesired Diagnostics
 =
 
Index: clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst
===

r360705 - Fix ASTMerge/namespace/test.cpp after r360701

2019-05-14 Thread Reid Kleckner via cfe-commits

Author: rnk
Date: Tue May 14 13:01:03 2019
New Revision: 360705

URL: http://llvm.org/viewvc/llvm-project?rev=360705&view=rev
Log:
Fix ASTMerge/namespace/test.cpp after r360701

Modified:
cfe/trunk/test/ASTMerge/namespace/test.cpp

Modified: cfe/trunk/test/ASTMerge/namespace/test.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ASTMerge/namespace/test.cpp?rev=360705&r1=360704&r2=360705&view=diff
==
--- cfe/trunk/test/ASTMerge/namespace/test.cpp (original)
+++ cfe/trunk/test/ASTMerge/namespace/test.cpp Tue May 14 13:01:03 2019
@@ -1,6 +1,6 @@
 // RUN: %clang_cc1 -emit-pch -std=c++1z -o %t.1.ast %S/Inputs/namespace1.cpp
 // RUN: %clang_cc1 -emit-pch -std=c++1z -o %t.2.ast %S/Inputs/namespace2.cpp
-// RUN: not %clang_cc1 -std=c++1z -ast-merge %t.1.ast -ast-merge %t.2.ast 
-fsyntax-only %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -std=c++1z -ast-merge %t.1.ast -ast-merge %t.2.ast 
-fsyntax-only %s 2>&1 | FileCheck %s
 
 static_assert(TestAliasName::z == 4);
 static_assert(ContainsInline::z == 10);


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

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

2019-05-14 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added a comment.

In D59885#1501774 , @ilya-biryukov 
wrote:

> The suggested approach looks promising. The difference seems to be within the 
> noise levels on my machine:

:) Awesome!

> I guess it would make more sense to do the following before landing this:
> 
> - flip the flag, i.e. instead of reporting whether the token is "new" (the 
> common case), report whether it's "cached", i.e. coming from `CachedTokens` 
> or from a non-macro-expansion token stream.
> - set this flag only inside `CachingLex` and `TokenLexer`.
> 
>   The revision also removes `IsNewToken` from `CachingLex` and sets the flag 
> in the token instead. Any other suggestions?

Notionally, I think we should separate out "produces new tokens" from "is macro 
expansion" in `TokenLexer`. Please take a quick look at the various callers of 
`EnterTokenStream` (particularly the ones inside `Lex`; I'm not too worried 
about the ones in `Parse`) and see which (if any) of them actually intend to 
inject new "real" phase-4-of-translation tokens. If there are any (and maybe 
even if there aren't?), please add a flag on `EnterTokenStream` to specify 
whether the `TokenLexer` believes it's creating new tokens or replaying tokens 
that have been seen before.

We'll need to decide what to do about the annotation tokens that we create when 
we enter / leave / import modules (`Preprocessor::EnterAnnotationToken`). In 
some sense, those are "new" tokens -- and that's how the parser will interpret 
them -- but they didn't come from the original source file. I think either 
answer could be OK there, but I'm inclined to treat them as "new" because that 
gives more information to the token consumer.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59885

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

[PATCH] D61914: [Support][Test] Time profiler: add regression test

2019-05-14 Thread Anton Afanasyev via Phabricator via cfe-commits

anton-afanasyev created this revision.
anton-afanasyev added a reviewer: thakis.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Add output to `llvm::errs()` when `-ftime-trace` option is enabled,
add regression test checking this option works as expected.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61914

Files:
  clang/tools/driver/cc1_main.cpp
  llvm/test/Support/check-time-trace.cxx


Index: llvm/test/Support/check-time-trace.cxx
===
--- /dev/null
+++ llvm/test/Support/check-time-trace.cxx
@@ -0,0 +1,11 @@
+// RUN: clang++ -ftime-trace %s 2>&1 | grep "Time trace json-file dumped to" \
+// RUN:   | awk '{print $NF}' | xargs cat | FileCheck %s
+
+// CHECK: "args":{"name":"clang"}
+
+#include 
+
+int main() {
+  std::cout << "Foo" << std::endl;
+  return 0;
+}
Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -241,6 +241,11 @@
 
 llvm::timeTraceProfilerWrite(*profilerOutput);
 llvm::timeTraceProfilerCleanup();
+
+llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
+llvm::errs()
+<< "Use chrome://tracing or Speedscope App "
+   "(https://www.speedscope.app) for flamegraph visualization\n";
   }
 
   // Our error handler depends on the Diagnostics object, which we're


Index: llvm/test/Support/check-time-trace.cxx
===
--- /dev/null
+++ llvm/test/Support/check-time-trace.cxx
@@ -0,0 +1,11 @@
+// RUN: clang++ -ftime-trace %s 2>&1 | grep "Time trace json-file dumped to" \
+// RUN:   | awk '{print $NF}' | xargs cat | FileCheck %s
+
+// CHECK: "args":{"name":"clang"}
+
+#include 
+
+int main() {
+  std::cout << "Foo" << std::endl;
+  return 0;
+}
Index: clang/tools/driver/cc1_main.cpp
===
--- clang/tools/driver/cc1_main.cpp
+++ clang/tools/driver/cc1_main.cpp
@@ -241,6 +241,11 @@
 
 llvm::timeTraceProfilerWrite(*profilerOutput);
 llvm::timeTraceProfilerCleanup();
+
+llvm::errs() << "Time trace json-file dumped to " << Path.str() << "\n";
+llvm::errs()
+<< "Use chrome://tracing or Speedscope App "
+   "(https://www.speedscope.app) for flamegraph visualization\n";
   }
 
   // Our error handler depends on the Diagnostics object, which we're
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61817: [analyzer] Add a prunable note for skipping virtual base initializers in subclasses.

2019-05-14 Thread Csaba Dabis via Phabricator via cfe-commits

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

What about "the most derived class" or "a superclass" instead of "the 
superclass"? (https://en.cppreference.com/w/cpp/language/derived_class) 
May the sentence is a little bit too long. It would be cool to say "by `A`" or 
something more simple and precise.




Comment at: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp:223
+// But only if we're actually skipping the virtual constructors.
+if (L.getDst() == *L.getSrc()->succ_begin()) {
+  ProgramPoint P = L.withTag(getNoteTags().makeNoteTag(

I think it is better to reduce the number of `if`s and merge the related 
comment as it emphasizes there is //only one thing// happen.


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

https://reviews.llvm.org/D61817



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

[PATCH] D36357: Added a better diagnostic when using the delete operator with lambdas

2019-05-14 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added a comment.

In D36357#1500949 , @Rakete wrote:

> How should I do this? Do I just skip to the next `}`, or also take into 
> account  any additional scopes? Also does this mean that I skip and then 
> revert, because that seems pretty expensive?

It would be a little expensive, yes, but we'd only be doing it on codepaths 
where we're producing an error -- for an ill-formed program, it's OK to take 
more time in order to produce a better diagnostic. Skipping to the next `}` 
won't work, because `SkipUntil` will skip over pairs of brace-balanced tokens 
(so you'll skip past the `}` you're looking for), but skipping until the next 
`{` and then skipping to the `}` after it should work.

Comment at: clang/lib/Parse/ParseExprCXX.cpp:2996
+   GetLookAheadToken(4).is(tok::identifier) {
+  SourceLocation RightBracketLock = NextToken().getLocation();
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis

rsmith wrote:
> `RightBracketLock` -> `RSquareLoc`
> 
> (Our convention is to use `Loc` for "location" and to avoid the word 
> "bracket" because it means different things in different English dialects -- 
> usually `[]` in US English and usually `()` in UK English.)
`Lock` -> `Loc`. There's no `k` in "location" =)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36357

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

[PATCH] D60162: [ThinLTO] Add module flags for TargetLibraryInfoImpl and use in LTO backends

2019-05-14 Thread Teresa Johnson via Phabricator via cfe-commits

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



Comment at: clang/test/CodeGen/svml-calls.ll:16
+
+define void @sin_f64(double* nocapture %varray) {
+; CHECK-LABEL: @sin_f64(

steven_wu wrote:
> Personally, I think codegen tests like this will be cleaner to keep in LLVM. 
> Clang tests just test the IRGen of the module flag and LLVM tests check that 
> those flags are respected and module flag merge is respected.
Ok. I originally was doing it here to ensure that ThinLTO distributed backends 
(which use clang) are fixed. But now that the approach no longer involves 
passing down additional info via that path into LTO, I don't think we need to 
test this explicitly but rather just as an LLVM LTO test. Will move.



Comment at: llvm/lib/LTO/LTOBackend.cpp:221
 
+static TargetLibraryInfoImpl *createTLII(Module &Mod, TargetMachine *TM) {
+  TargetLibraryInfoImpl *TLII =

steven_wu wrote:
> Should this be done not just for LTOBackend but for regular compilation as 
> well? LegacyCodegenerator and llc can all be benefit from a matching 
> TargetLibraryInfo?
Yeah, probably. I think the best way to have this utilized everywhere is to 
move the below code into the TargetLibraryInfoImpl itself - by having it also 
take the Module as a parameter). Probably as a required parameter, to ensure it 
is used consistently. WDYT? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60162



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

[PATCH] D60162: [ThinLTO] Add module flags for TargetLibraryInfoImpl and use in LTO backends

2019-05-14 Thread Teresa Johnson via Phabricator via cfe-commits

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



Comment at: llvm/lib/LTO/LTOBackend.cpp:221
 
+static TargetLibraryInfoImpl *createTLII(Module &Mod, TargetMachine *TM) {
+  TargetLibraryInfoImpl *TLII =

tejohnson wrote:
> steven_wu wrote:
> > Should this be done not just for LTOBackend but for regular compilation as 
> > well? LegacyCodegenerator and llc can all be benefit from a matching 
> > TargetLibraryInfo?
> Yeah, probably. I think the best way to have this utilized everywhere is to 
> move the below code into the TargetLibraryInfoImpl itself - by having it also 
> take the Module as a parameter). Probably as a required parameter, to ensure 
> it is used consistently. WDYT? 
I meant, "into the TargetLibraryInfoImpl constructor"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60162



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

[PATCH] D61814: [CFG] NFC: Remove implicit conversion from CFGTerminator to Stmt *, make it a variant class instead.

2019-05-14 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus accepted this revision.
Szelethus added a comment.

LGTM!




Comment at: clang/include/clang/Analysis/CFG.h:514
+  CFGTerminator() { assert(!isValid()); }
+  CFGTerminator(Stmt *S, Kind K = StmtBranch) : Data(S, K) {}
 

Can we add something to check that the integer part of `Data` is actually large 
enough to store an object of type `Kind`, even if we add more values to the 
enum?


Repository:
  rC Clang

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

https://reviews.llvm.org/D61814



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

[PATCH] D36357: Added a better diagnostic when using the delete operator with lambdas

2019-05-14 Thread Nicolas Lesser via Phabricator via cfe-commits

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

In D36357#1501961 , @rsmith wrote:

> In D36357#1500949 , @Rakete 
> wrote:
>
> > How should I do this? Do I just skip to the next `}`, or also take into 
> > account  any additional scopes? Also does this mean that I skip and then 
> > revert, because that seems pretty expensive?
>
>
> It would be a little expensive, yes, but we'd only be doing it on codepaths 
> where we're producing an error -- for an ill-formed program, it's OK to take 
> more time in order to produce a better diagnostic. Skipping to the next `}` 
> won't work, because `SkipUntil` will skip over pairs of brace-balanced tokens 
> (so you'll skip past the `}` you're looking for), but skipping until the next 
> `{` and then skipping to the `}` after it should work.


Hmm wouldn't this interact badly with `{}` in initializers?

  [] {};
  [](int = {0}) = {};






Comment at: clang/lib/Parse/ParseExprCXX.cpp:2996
+   GetLookAheadToken(4).is(tok::identifier) {
+  SourceLocation RightBracketLock = NextToken().getLocation();
+  // Warn if the non-capturing lambda isn't surrounded by parenthesis

rsmith wrote:
> rsmith wrote:
> > `RightBracketLock` -> `RSquareLoc`
> > 
> > (Our convention is to use `Loc` for "location" and to avoid the word 
> > "bracket" because it means different things in different English dialects 
> > -- usually `[]` in US English and usually `()` in UK English.)
> `Lock` -> `Loc`. There's no `k` in "location" =)
No idea how that k managed to sneak in :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36357



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

RE: r360452 - Replace 'REQUIRES: nozlib' with '!zlib' because we don't need two ways

2019-05-14 Thread via cfe-commits


> -Original Message-
> From: David Blaikie [mailto:dblai...@gmail.com]
> Sent: Tuesday, May 14, 2019 1:47 PM
> To: Robinson, Paul
> Cc: cfe-commits
> Subject: Re: r360452 - Replace 'REQUIRES: nozlib' with '!zlib' because we
> don't need two ways
> 
> Fair enough - since they're already there I don't feel super serious
> about converging on one (though I probably wouldn't've been in favor
> of adding a second spelling at the point it was proposed).

UNSUPPORTED came first, in 2014, followed by the rare REQUIRES-ANY
in 2016, followed by boolean expression syntax in 2017.  UNSUPPORTED
is particularly popular in the libcxx suite (over 2000 tests) so I
doubt there's enough motivation to remove it.  REQUIRES-ANY could be 
tossed though, 17 occurrences across clang and compiler-rt.

I do see one test in clang that uses UNSUPPORTED as a FileCheck prefix,
I'll make a note to change that next time I need a break from real work.
--paulr

> 
> On Tue, May 14, 2019 at 8:03 AM  wrote:
> >
> > There's no practical difference.  I view it as a matter of documentation
> of intent, see my commit log comment for r360603.
> >
> > Arguably we could eliminate UNSUPPORTED and move all the expressions
> into REQUIRES (appropriately negated), but I'm not sure that's a net win
> for readability.
> >
> > --paulr
> >
> >
> >
> > From: David Blaikie [mailto:dblai...@gmail.com]
> > Sent: Monday, May 13, 2019 10:48 AM
> > To: Robinson, Paul
> > Cc: cfe-commits
> > Subject: Re: r360452 - Replace 'REQUIRES: nozlib' with '!zlib' because
> we don't need two ways
> >
> >
> >
> > What's the practical difference between "UNSUPPORTED: foo" and
> "REQUIRES: !foo"? (I see some of the fixes you've made go one way, some
> the other way)
> >
> >
> >
> > On Fri, May 10, 2019 at 11:30 AM Paul Robinson via cfe-commits  comm...@lists.llvm.org> wrote:
> >
> > Author: probinson
> > Date: Fri May 10 11:32:53 2019
> > New Revision: 360452
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=360452&view=rev
> > Log:
> > Replace 'REQUIRES: nozlib' with '!zlib' because we don't need two ways
> > to say the same thing.
> >
> > Modified:
> > cfe/trunk/test/Driver/nozlibcompress.c
> >
> > Modified: cfe/trunk/test/Driver/nozlibcompress.c
> > URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/test/Driver/nozlibcompress.c?rev=360452&r1=360451&r2=360
> 452&view=diff
> >
> ==
> 
> > --- cfe/trunk/test/Driver/nozlibcompress.c (original)
> > +++ cfe/trunk/test/Driver/nozlibcompress.c Fri May 10 11:32:53 2019
> > @@ -1,4 +1,4 @@
> > -// REQUIRES: nozlib
> > +// REQUIRES: !zlib
> >
> >  // RUN: %clang -### -fintegrated-as -gz -c %s 2>&1 | FileCheck %s -
> check-prefix CHECK-WARN
> >  // RUN: %clang -### -fintegrated-as -gz=none -c %s 2>&1 | FileCheck -
> allow-empty -check-prefix CHECK-NOWARN %s
> >
> >
> > ___
> > cfe-commits mailing list
> > cfe-commits@lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61909: Add Clang shared library with C++ exports

2019-05-14 Thread Chris Bieneman via Phabricator via cfe-commits

beanz updated this revision to Diff 199508.
beanz added a comment.

Changed to lowercase 'c' to match other clang libraries.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61909

Files:
  clang/cmake/modules/AddClang.cmake
  clang/tools/CMakeLists.txt
  clang/tools/clang-shlib/CMakeLists.txt
  clang/tools/clang-shlib/clang-shlib.cpp


Index: clang/tools/clang-shlib/clang-shlib.cpp
===
--- /dev/null
+++ clang/tools/clang-shlib/clang-shlib.cpp
@@ -0,0 +1 @@
+// Intentionally empty source file to make CMake happy
Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- /dev/null
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -0,0 +1,13 @@
+get_property(clang_libs GLOBAL PROPERTY CLANG_STATIC_LIBS)
+
+foreach (lib ${clang_libs})
+  list(APPEND _OBJECTS $)
+  list(APPEND _DEPS $)
+endforeach ()
+
+add_clang_library(clang_shared
+  SHARED
+  clang-shlib.cpp
+  ${_OBJECTS}
+  LINK_LIBS
+  ${_DEPS})
Index: clang/tools/CMakeLists.txt
===
--- clang/tools/CMakeLists.txt
+++ clang/tools/CMakeLists.txt
@@ -13,6 +13,9 @@
 
 add_clang_subdirectory(clang-rename)
 add_clang_subdirectory(clang-refactor)
+if(UNIX)
+  add_clang_subdirectory(clang-shlib)
+endif()
 
 if(CLANG_ENABLE_ARCMT)
   add_clang_subdirectory(arcmt-test)
Index: clang/cmake/modules/AddClang.cmake
===
--- clang/cmake/modules/AddClang.cmake
+++ clang/cmake/modules/AddClang.cmake
@@ -81,9 +81,12 @@
   )
   endif()
   if(ARG_SHARED)
-set(ARG_ENABLE_SHARED SHARED)
+set(LIBTYPE SHARED)
+  else()
+set(LIBTYPE STATIC OBJECT)
+set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
   endif()
-  llvm_add_library(${name} ${ARG_ENABLE_SHARED} ${ARG_UNPARSED_ARGUMENTS} 
${srcs})
+  llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
 
   if(TARGET ${name})
 target_link_libraries(${name} INTERFACE ${LLVM_COMMON_LIBS})


Index: clang/tools/clang-shlib/clang-shlib.cpp
===
--- /dev/null
+++ clang/tools/clang-shlib/clang-shlib.cpp
@@ -0,0 +1 @@
+// Intentionally empty source file to make CMake happy
Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- /dev/null
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -0,0 +1,13 @@
+get_property(clang_libs GLOBAL PROPERTY CLANG_STATIC_LIBS)
+
+foreach (lib ${clang_libs})
+  list(APPEND _OBJECTS $)
+  list(APPEND _DEPS $)
+endforeach ()
+
+add_clang_library(clang_shared
+  SHARED
+  clang-shlib.cpp
+  ${_OBJECTS}
+  LINK_LIBS
+  ${_DEPS})
Index: clang/tools/CMakeLists.txt
===
--- clang/tools/CMakeLists.txt
+++ clang/tools/CMakeLists.txt
@@ -13,6 +13,9 @@
 
 add_clang_subdirectory(clang-rename)
 add_clang_subdirectory(clang-refactor)
+if(UNIX)
+  add_clang_subdirectory(clang-shlib)
+endif()
 
 if(CLANG_ENABLE_ARCMT)
   add_clang_subdirectory(arcmt-test)
Index: clang/cmake/modules/AddClang.cmake
===
--- clang/cmake/modules/AddClang.cmake
+++ clang/cmake/modules/AddClang.cmake
@@ -81,9 +81,12 @@
   )
   endif()
   if(ARG_SHARED)
-set(ARG_ENABLE_SHARED SHARED)
+set(LIBTYPE SHARED)
+  else()
+set(LIBTYPE STATIC OBJECT)
+set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
   endif()
-  llvm_add_library(${name} ${ARG_ENABLE_SHARED} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
+  llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
 
   if(TARGET ${name})
 target_link_libraries(${name} INTERFACE ${LLVM_COMMON_LIBS})
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60162: [ThinLTO] Add module flags for TargetLibraryInfoImpl and use in LTO backends

2019-05-14 Thread Steven Wu via Phabricator via cfe-commits

steven_wu added inline comments.



Comment at: llvm/lib/LTO/LTOBackend.cpp:221
 
+static TargetLibraryInfoImpl *createTLII(Module &Mod, TargetMachine *TM) {
+  TargetLibraryInfoImpl *TLII =

tejohnson wrote:
> tejohnson wrote:
> > steven_wu wrote:
> > > Should this be done not just for LTOBackend but for regular compilation 
> > > as well? LegacyCodegenerator and llc can all be benefit from a matching 
> > > TargetLibraryInfo?
> > Yeah, probably. I think the best way to have this utilized everywhere is to 
> > move the below code into the TargetLibraryInfoImpl itself - by having it 
> > also take the Module as a parameter). Probably as a required parameter, to 
> > ensure it is used consistently. WDYT? 
> I meant, "into the TargetLibraryInfoImpl constructor"
SGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60162



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

[PATCH] D61909: Add Clang shared library with C++ exports

2019-05-14 Thread Chris Bieneman via Phabricator via cfe-commits

beanz added a comment.

@winksaville whether or not PIC is required for shared libraries varies by 
platform. These days LLVM defaults to -fPIC, and I'm not sure we actually 
support running LLVM without -fPIC on systems that require shared libraries to 
be PIC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61909



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

r360707 - [NewPM] Port HWASan and Kernel HWASan

2019-05-14 Thread Leonard Chan via cfe-commits

Author: leonardchan
Date: Tue May 14 14:17:21 2019
New Revision: 360707

URL: http://llvm.org/viewvc/llvm-project?rev=360707&view=rev
Log:
[NewPM] Port HWASan and Kernel HWASan

Port hardware assisted address sanitizer to new PM following the same 
guidelines as msan and tsan.

Changes:
- Separate HWAddressSanitizer into a pass class and a sanitizer class.
- Create new PM wrapper pass for the sanitizer class.
- Use the getOrINsert pattern for some module level initialization declarations.
- Also enable kernel-kwasan in new PM
- Update llvm tests and add clang test.

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

Added:
cfe/trunk/test/CodeGen/hwasan-new-pm.c
Modified:
cfe/trunk/lib/CodeGen/BackendUtil.cpp

Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/BackendUtil.cpp?rev=360707&r1=360706&r2=360707&view=diff
==
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp (original)
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp Tue May 14 14:17:21 2019
@@ -57,6 +57,7 @@
 #include "llvm/Transforms/Instrumentation/AddressSanitizer.h"
 #include "llvm/Transforms/Instrumentation/BoundsChecking.h"
 #include "llvm/Transforms/Instrumentation/GCOVProfiler.h"
+#include "llvm/Transforms/Instrumentation/HWAddressSanitizer.h"
 #include "llvm/Transforms/Instrumentation/InstrProfiling.h"
 #include "llvm/Transforms/Instrumentation/MemorySanitizer.h"
 #include "llvm/Transforms/Instrumentation/ThreadSanitizer.h"
@@ -265,12 +266,13 @@ static void addHWAddressSanitizerPasses(
   static_cast(Builder);
   const CodeGenOptions &CGOpts = BuilderWrapper.getCGOpts();
   bool Recover = CGOpts.SanitizeRecover.has(SanitizerKind::HWAddress);
-  PM.add(createHWAddressSanitizerPass(/*CompileKernel*/ false, Recover));
+  PM.add(
+  createHWAddressSanitizerLegacyPassPass(/*CompileKernel*/ false, 
Recover));
 }
 
 static void addKernelHWAddressSanitizerPasses(const PassManagerBuilder 
&Builder,
 legacy::PassManagerBase &PM) {
-  PM.add(createHWAddressSanitizerPass(
+  PM.add(createHWAddressSanitizerLegacyPassPass(
   /*CompileKernel*/ true, /*Recover*/ true));
 }
 
@@ -962,6 +964,17 @@ static void addSanitizersAtO0(ModulePass
   if (LangOpts.Sanitize.has(SanitizerKind::Thread)) {
 MPM.addPass(createModuleToFunctionPassAdaptor(ThreadSanitizerPass()));
   }
+
+  if (LangOpts.Sanitize.has(SanitizerKind::HWAddress)) {
+bool Recover = CodeGenOpts.SanitizeRecover.has(SanitizerKind::HWAddress);
+MPM.addPass(createModuleToFunctionPassAdaptor(
+HWAddressSanitizerPass(/*CompileKernel=*/false, Recover)));
+  }
+
+  if (LangOpts.Sanitize.has(SanitizerKind::KernelHWAddress)) {
+MPM.addPass(createModuleToFunctionPassAdaptor(
+HWAddressSanitizerPass(/*CompileKernel=*/true, /*Recover=*/true)));
+  }
 }
 
 /// A clean version of `EmitAssembly` that uses the new pass manager.
@@ -1145,6 +1158,23 @@ void EmitAssemblyHelper::EmitAssemblyWit
   UseOdrIndicator));
 });
   }
+  if (LangOpts.Sanitize.has(SanitizerKind::HWAddress)) {
+bool Recover =
+CodeGenOpts.SanitizeRecover.has(SanitizerKind::HWAddress);
+PB.registerOptimizerLastEPCallback(
+[Recover](FunctionPassManager &FPM,
+  PassBuilder::OptimizationLevel Level) {
+  FPM.addPass(HWAddressSanitizerPass(
+  /*CompileKernel=*/false, Recover));
+});
+  }
+  if (LangOpts.Sanitize.has(SanitizerKind::KernelHWAddress)) {
+PB.registerOptimizerLastEPCallback(
+[](FunctionPassManager &FPM, PassBuilder::OptimizationLevel Level) 
{
+  FPM.addPass(HWAddressSanitizerPass(
+  /*CompileKernel=*/true, /*Recover=*/true));
+});
+  }
   if (Optional Options = getGCOVOptions(CodeGenOpts))
 PB.registerPipelineStartEPCallback([Options](ModulePassManager &MPM) {
   MPM.addPass(GCOVProfilerPass(*Options));

Added: cfe/trunk/test/CodeGen/hwasan-new-pm.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/hwasan-new-pm.c?rev=360707&view=auto
==
--- cfe/trunk/test/CodeGen/hwasan-new-pm.c (added)
+++ cfe/trunk/test/CodeGen/hwasan-new-pm.c Tue May 14 14:17:21 2019
@@ -0,0 +1,34 @@
+// Test that HWASan and KHWASan runs with the new pass manager.
+// We run them under different optimizations and LTOs to ensure the IR is still
+// being instrumented properly.
+
+// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=hwaddress %s | FileCheck %s 
--check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
+// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=hwaddress -flto %s | FileCheck %s 
--check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
+// RUN: %clang_cc1 -S -emit-llvm -o - -f

[PATCH] D61709: [NewPM] Port HWASan and Kernel HWASan

2019-05-14 Thread Leonard Chan via Phabricator via cfe-commits

leonardchan added inline comments.



Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:179
 
-  bool runOnFunction(Function &F) override;
-  bool doInitialization(Module &M) override;
+  bool instrumentFunction(Function &F);
+  void initializeWithModule(Module &M);

philip.pfaffe wrote:
> There are some naming clashes here. In the other sanitizers these functions 
> are called `sanitizeFunction` and `initializeModule`.
Renamed


Repository:
  rC Clang

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

https://reviews.llvm.org/D61709



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

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

2019-05-14 Thread Eli Friedman via Phabricator via cfe-commits

efriedma added a comment.

> I have a related patch that turns -fno-builtin* options into module flags

Do you have any opinion on representing -fno-builtin using a module flag vs. a 
function attribute in IR?  It seems generally more flexible and easier to 
reason about a function attribute from my perspective.  But I might be missing 
something about the semantics of -fno-builtin that would make that 
representation awkward.  Or I guess it might just be more work to implement, 
given we have some IPO passes that use TargetLibraryInfo?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61634



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

[PATCH] D61709: [NewPM] Port HWASan and Kernel HWASan

2019-05-14 Thread Leonard Chan via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
leonardchan marked 2 inline comments as done.
Closed by commit rC360707: [NewPM] Port HWASan and Kernel HWASan (authored by 
leonardchan, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D61709?vs=198889&id=199511#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D61709

Files:
  lib/CodeGen/BackendUtil.cpp
  test/CodeGen/hwasan-new-pm.c

Index: test/CodeGen/hwasan-new-pm.c
===
--- test/CodeGen/hwasan-new-pm.c
+++ test/CodeGen/hwasan-new-pm.c
@@ -0,0 +1,34 @@
+// Test that HWASan and KHWASan runs with the new pass manager.
+// We run them under different optimizations and LTOs to ensure the IR is still
+// being instrumented properly.
+
+// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager -fsanitize=hwaddress %s | FileCheck %s --check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
+// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager -fsanitize=hwaddress -flto %s | FileCheck %s --check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
+// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager -fsanitize=hwaddress -flto=thin %s | FileCheck %s --check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
+// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager -fsanitize=hwaddress %s | FileCheck %s --check-prefixes=CHECK,HWASAN
+// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager -fsanitize=hwaddress -flto %s | FileCheck %s --check-prefixes=CHECK,HWASAN
+// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager -fsanitize=hwaddress -flto=thin %s | FileCheck %s
+
+// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager -fsanitize=kernel-hwaddress %s | FileCheck %s --check-prefixes=CHECK,KHWASAN,KHWASAN-NOOPT
+// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager -fsanitize=kernel-hwaddress -flto %s | FileCheck %s --check-prefixes=CHECK,KHWASAN,KHWASAN-NOOPT
+// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager -fsanitize=kernel-hwaddress -flto=thin %s | FileCheck %s --check-prefixes=CHECK,KHWASAN,KHWASAN-NOOPT
+// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager -fsanitize=kernel-hwaddress %s | FileCheck %s --check-prefixes=CHECK,KHWASAN
+// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager -fsanitize=kernel-hwaddress -flto %s | FileCheck %s --check-prefixes=CHECK,KHWASAN
+// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager -fsanitize=kernel-hwaddress -flto=thin %s | FileCheck %s
+
+int foo(int *a) { return *a; }
+
+// All the cases above mark the function with sanitize_hwaddress.
+// CHECK-DAG: sanitize_hwaddress
+
+// Both sanitizers produce %hwasan.shadow without both thinlto and optimizations.
+// HWASAN-DAG: %hwasan.shadow
+// KHWASAN-DAG: %hwasan.shadow
+
+// Both sanitizers produce __hwasan_tls without both thinlto and optimizations.
+// HWASAN-DAG: __hwasan_tls
+// KHWASAN-DAG: __hwasan_tls
+
+// For unoptimized cases, both sanitizers produce different load functions.
+// HWASAN-NOOPT-DAG: __hwasan_loadN
+// KHWASAN-NOOPT-DAG: __hwasan_loadN_noabort
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -57,6 +57,7 @@
 #include "llvm/Transforms/Instrumentation/AddressSanitizer.h"
 #include "llvm/Transforms/Instrumentation/BoundsChecking.h"
 #include "llvm/Transforms/Instrumentation/GCOVProfiler.h"
+#include "llvm/Transforms/Instrumentation/HWAddressSanitizer.h"
 #include "llvm/Transforms/Instrumentation/InstrProfiling.h"
 #include "llvm/Transforms/Instrumentation/MemorySanitizer.h"
 #include "llvm/Transforms/Instrumentation/ThreadSanitizer.h"
@@ -265,12 +266,13 @@
   static_cast(Builder);
   const CodeGenOptions &CGOpts = BuilderWrapper.getCGOpts();
   bool Recover = CGOpts.SanitizeRecover.has(SanitizerKind::HWAddress);
-  PM.add(createHWAddressSanitizerPass(/*CompileKernel*/ false, Recover));
+  PM.add(
+  createHWAddressSanitizerLegacyPassPass(/*CompileKernel*/ false, Recover));
 }
 
 static void addKernelHWAddressSanitizerPasses(const PassManagerBuilder &Builder,
 legacy::PassManagerBase &PM) {
-  PM.add(createHWAddressSanitizerPass(
+  PM.add(createHWAddressSanitizerLegacyPassPass(
   /*CompileKernel*/ true, /*Recover*/ true));
 }
 
@@ -962,6 +964,17 @@
   if (LangOpts.Sanitize.has(SanitizerKind::Thread)) {
 MPM.addPass(createModuleToFunctionPassAdaptor(ThreadSanitizerPass()));
   }
+
+  if (LangOpts.Sanitize.has(SanitizerKind::HWAddress)) {
+bool Recover = CodeGenOpts.SanitizeRecover.has(SanitizerKind::HWAddress);
+MPM.addPass(createModuleToFunctionPassAdaptor(
+HWAddressSanitizerPass(/*CompileKernel=*/false, Recover)))

[PATCH] D57858: [analyzer] Add a new frontend flag to display all checker options

2019-05-14 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added a comment.

In D57858#1501065 , @dkrupp wrote:

> These are the alpha checkers that we are testing in Ericsson:


Hmm, if you don't mind i'll take this to cfe-dev, as it's an interesting topic 
:)


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

https://reviews.llvm.org/D57858



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

[PATCH] D61845: [builtin] Fixed definitions of builtins that rely on the int/long long type is 32/64 bits

2019-05-14 Thread Eli Friedman via Phabricator via cfe-commits

efriedma added inline comments.



Comment at: test/CodeGen/builtins.cpp:5
+// RUN: %clang_cc1 -std=c++11 -triple powerpc-pc-linux -verify %s
+// RUN: %clang_cc1 -std=c++11 -triple arm-linux-gnueabi -verify %s
+

You don't need quite so many targets on this list.  There are essentially three 
interesting kinds of targets: targets where int is 16 bits (like avr), targets 
where int is 32 bits and long is 64 bits (like x86_64 Linux), and targets where 
int and long are both 32 bits (like i686 Linux).

You might need to pass -ffreestanding to avoid including /usr/include/stdint.h 
on non-Linux systems.



Comment at: test/CodeGen/builtins.cpp:21
+
+uint16_t bswap16; // expected-note{{previous definition is here}}
+decltype(__builtin_bswap16(0)) bswap16 = 42; // 
expected-error-re{{redefinition of 'bswap16'{{$

If you write "extern uint16_t bswap16;", there won't be any error message when 
the types match.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61845



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

[PATCH] D61365: [libcxx] [test] Suppress float->int narrowing warning in vector range-construction test.

2019-05-14 Thread Eric Fiselier via Phabricator via cfe-commits

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

This LGTM after correctly wrapping it as Casey mentionse.


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

https://reviews.llvm.org/D61365



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

[PATCH] D61366: [libcxx] [test] Don't assert that moved-from containers with non-POCMA allocators are empty.

2019-05-14 Thread Eric Fiselier via Phabricator via cfe-commits

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

I'm not sure I agree with your design decision, but this patch LGTM.

Are you not allowed to move the containers elements in this case?


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

https://reviews.llvm.org/D61366



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

[PATCH] D61366: [libcxx] [test] Don't assert that moved-from containers with non-POCMA allocators are empty.

2019-05-14 Thread Billy Robert O'Neal III via Phabricator via cfe-commits

BillyONeal added a comment.

In D61366#1502073 , @EricWF wrote:

> I'm not sure I agree with your design decision, but this patch LGTM.

I wouldn't object to a standards change to make this the case; though it is 
suboptimal to destroy all the elements needlessly.

> Are you not allowed to move the containers elements in this case?

Correct. The allocator is not POCMA and not equal, so it's functionally the 
same as doing assign(make_move_iterator(begin()), make_move_iterator(end())).

Billy3

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

https://reviews.llvm.org/D61366

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

[PATCH] D61750: [Targets] Move soft-float-abi filtering to `initFeatureMap`

2019-05-14 Thread Eli Friedman via Phabricator via cfe-commits

efriedma added a comment.

> If we agree that this is a good way forward, there also appears to be 
> +neonfp/-neonfp additions happening in handleTargetFeatures that should 
> probably be happening in initFeatureMap instead?

neonfp isn't passed as a feature in the first place; there's a separate API 
setFPMath which is used for that.  We translate it into a target feature for 
the sake of the backend.  So I'm not sure what you're proposing.

--

What happens if someone specifies __attribute__((target("soft-float-abi"))) or 
something like that?  (There's an API isValidFeatureName to validate feature 
names, but we currently don't implement it.)


Repository:
  rC Clang

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

https://reviews.llvm.org/D61750



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

[PATCH] D61350: [clang-tidy] New check calling out uses of +new in Objective-C code

2019-05-14 Thread Stephane Moore via Phabricator via cfe-commits

stephanemoore accepted this revision.
stephanemoore marked an inline comment as done.
stephanemoore added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp:54
+CheckFactories.registerCheck(
+"google-objc-avoid-nsobject-new");
 CheckFactories.registerCheck(

It is a little odd that "google-objc-avoid-nsobject-new" warns on `+new` even 
for classes that are not derived from `NSObject`. In a sense, "nsobject-new" is 
being used as an identifier for any class method named "new". Interestingly, 
renaming to the more direct  "google-objc-avoid-new-class-method" is even more 
misleading. I have yet to think of a name that I think would actually be better.

While I think the current name is potentially misleading, I think I am okay 
with this name for the following reasons:
(1) I believe this will only be misleading in marginal scenarios. I believe 
that it will be exceedingly rare for a class method named `new` to exist in a 
class hierarchy where the root class is not `NSObject`.
(2) We can always amend the check to restrict it to `NSObject`. In the 
unexpected circumstance where someone does provide a legitimate use case for a 
class method named `new` in a class hierarchy where the root class is not 
`NSObject`, we can simply amend the check to restrict its enforcement to class 
hierarchies where the root class is `NSObject`.


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

https://reviews.llvm.org/D61350



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

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

2019-05-14 Thread Hal Finkel via Phabricator via cfe-commits

hfinkel added a comment.

In D61634#1502043 , @efriedma wrote:

> > I have a related patch that turns -fno-builtin* options into module flags
>
> Do you have any opinion on representing -fno-builtin using a module flag vs. 
> a function attribute in IR?  It seems generally more flexible and easier to 
> reason about a function attribute from my perspective.  But I might be 
> missing something about the semantics of -fno-builtin that would make that 
> representation awkward.  Or I guess it might just be more work to implement, 
> given we have some IPO passes that use TargetLibraryInfo?

I think that a function attribute would be better. We generally use these flags 
only in the context of certain translation units, and when we use LTO, it would 
be sad if we had to take the most-conservative settings across the entire 
application. When we insert new function call to a standard library, we always 
do it in the context of some function. We'd probably need to block inlining in 
some cases, but that's better than a global conservative setting.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61634

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

[PATCH] D61366: [libcxx] [test] Don't assert that moved-from containers with non-POCMA allocators are empty.

2019-05-14 Thread Arthur O'Dwyer via Phabricator via cfe-commits

Quuxplusone added a comment.

>> Are you not allowed to move the containers elements in this case?
> 
> Correct. The allocator is not POCMA and not equal, so it's functionally the 
> same as doing `assign(make_move_iterator(begin()), 
> make_move_iterator(end()))`.

In case the clarification helps some reader: When the allocator is 
not-POCMA-and-not-equal, then you are forbidden to //pilfer// the //pointer// 
from the source container, but you are indeed allowed to //move// the 
//elements//, and that's what Billy is describing. You could finish by having 
the source container destroy all its elements (those elements being now in a 
moved-from state) and become empty, but that's less efficient than keeping the 
moved-from elements around.

Unless of course you know that the element type is //trivially relocatable//... 
;)


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

https://reviews.llvm.org/D61366



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

[PATCH] D61364: [libcxx] [test] Fix incorrect allocator::construct assertions in associative container tests

2019-05-14 Thread Louis Dionne via Phabricator via cfe-commits

ldionne added a comment.

> When the insert(P&&) is called, it delegates to emplace, which only gets 
> Cpp17EmplaceConstructible from the supplied parameters, not 
> Cpp17EmplaceConstructible from add_const_t of the parameters. Thus, libcxx's 
> test is asserting a condition the standard explicitly does not allow at this 
> time. [...]

Agreed.

> Unfortunately with the tests fixed to assert the correct allocator::construct 
> call, libc++ will fail these tests. I'm looking for guidance on how libc++ 
> maintainers would like to handle this.

Do you know *why* the tests are failing with libc++? I see this overload for 
`insert` and it seems like it should be a better match?

  template ::value>::type>
  _LIBCPP_INLINE_VISIBILITY
  pair insert(_Pp&& __p);

I think we should just fix libc++ to do the right thing (according to the 
standard).

Separately, I'm not sure it's worth changing the Standard to ensure that the `T 
const&` overload is selected -- I don't see a huge benefit here.


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

https://reviews.llvm.org/D61364



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

r360720 - Fix bots by adding target triple to test.

2019-05-14 Thread Leonard Chan via cfe-commits

Author: leonardchan
Date: Tue May 14 15:37:34 2019
New Revision: 360720

URL: http://llvm.org/viewvc/llvm-project?rev=360720&view=rev
Log:
Fix bots by adding target triple to test.

Modified:
cfe/trunk/test/CodeGen/hwasan-new-pm.c

Modified: cfe/trunk/test/CodeGen/hwasan-new-pm.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/hwasan-new-pm.c?rev=360720&r1=360719&r2=360720&view=diff
==
--- cfe/trunk/test/CodeGen/hwasan-new-pm.c (original)
+++ cfe/trunk/test/CodeGen/hwasan-new-pm.c Tue May 14 15:37:34 2019
@@ -2,19 +2,19 @@
 // We run them under different optimizations and LTOs to ensure the IR is still
 // being instrumented properly.
 
-// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=hwaddress %s | FileCheck %s 
--check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
-// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=hwaddress -flto %s | FileCheck %s 
--check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
-// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=hwaddress -flto=thin %s | FileCheck %s 
--check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
-// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager 
-fsanitize=hwaddress %s | FileCheck %s --check-prefixes=CHECK,HWASAN
-// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager 
-fsanitize=hwaddress -flto %s | FileCheck %s --check-prefixes=CHECK,HWASAN
-// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager 
-fsanitize=hwaddress -flto=thin %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - 
-fexperimental-new-pass-manager -fsanitize=hwaddress %s | FileCheck %s 
--check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - 
-fexperimental-new-pass-manager -fsanitize=hwaddress -flto %s | FileCheck %s 
--check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - 
-fexperimental-new-pass-manager -fsanitize=hwaddress -flto=thin %s | FileCheck 
%s --check-prefixes=CHECK,HWASAN,HWASAN-NOOPT
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -O1 
-fexperimental-new-pass-manager -fsanitize=hwaddress %s | FileCheck %s 
--check-prefixes=CHECK,HWASAN
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -O1 
-fexperimental-new-pass-manager -fsanitize=hwaddress -flto %s | FileCheck %s 
--check-prefixes=CHECK,HWASAN
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -O1 
-fexperimental-new-pass-manager -fsanitize=hwaddress -flto=thin %s | FileCheck 
%s
 
-// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=kernel-hwaddress %s | FileCheck %s 
--check-prefixes=CHECK,KHWASAN,KHWASAN-NOOPT
-// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=kernel-hwaddress -flto %s | FileCheck %s 
--check-prefixes=CHECK,KHWASAN,KHWASAN-NOOPT
-// RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=kernel-hwaddress -flto=thin %s | FileCheck %s 
--check-prefixes=CHECK,KHWASAN,KHWASAN-NOOPT
-// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager 
-fsanitize=kernel-hwaddress %s | FileCheck %s --check-prefixes=CHECK,KHWASAN
-// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager 
-fsanitize=kernel-hwaddress -flto %s | FileCheck %s 
--check-prefixes=CHECK,KHWASAN
-// RUN: %clang_cc1 -S -emit-llvm -o - -O1 -fexperimental-new-pass-manager 
-fsanitize=kernel-hwaddress -flto=thin %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - 
-fexperimental-new-pass-manager -fsanitize=kernel-hwaddress %s | FileCheck %s 
--check-prefixes=CHECK,KHWASAN,KHWASAN-NOOPT
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - 
-fexperimental-new-pass-manager -fsanitize=kernel-hwaddress -flto %s | 
FileCheck %s --check-prefixes=CHECK,KHWASAN,KHWASAN-NOOPT
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - 
-fexperimental-new-pass-manager -fsanitize=kernel-hwaddress -flto=thin %s | 
FileCheck %s --check-prefixes=CHECK,KHWASAN,KHWASAN-NOOPT
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -O1 
-fexperimental-new-pass-manager -fsanitize=kernel-hwaddress %s | FileCheck %s 
--check-prefixes=CHECK,KHWASAN
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -O1 
-fexperimental-new-pass-manager -fsanitize=kernel-hwaddress -flto %s | 
FileCheck %s --check-prefixes=CHECK,KHWASAN
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -o - -O1 
-fexperimental-new-pass-manager -fsanitize=kernel-hwaddress -flto=thin %s | 
FileCheck %s
 
 int foo(int *a) { return *a; }
 


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

[PATCH] D61364: [libcxx] [test] Fix incorrect allocator::construct assertions in associative container tests

2019-05-14 Thread Billy Robert O'Neal III via Phabricator via cfe-commits

BillyONeal added a comment.

In D61364#1502172 , @ldionne wrote:

> Do you know *why* the tests are failing with libc++? I see this overload for 
> `insert` and it seems like it should be a better match?


No, I haven't investigated. I avoid looking at libc++ product code too much for 
now because I want to avoid any appearance or reality of licensing conflicts... 
for now  ;) .

In D61364#1502172 , @ldionne wrote:

> I think we should just fix libc++ to do the right thing (according to the 
> standard).


That's what I'd recommend :)

In D61364#1502172 , @ldionne wrote:

> Separately, I'm not sure it's worth changing the Standard to ensure that the 
> `T const&` overload is selected -- I don't see a huge benefit here.


Would be a code size win if both overloads are called in the same program.


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

https://reviews.llvm.org/D61364



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

[PATCH] D61366: [libcxx] [test] Don't assert that moved-from containers with non-POCMA allocators are empty.

2019-05-14 Thread Billy Robert O'Neal III via Phabricator via cfe-commits

BillyONeal added a comment.

In D61366#1502170 , @Quuxplusone wrote:

> you are indeed allowed to //move// the //elements//


And indeed we *must* do that :).


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

https://reviews.llvm.org/D61366



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

[PATCH] D61364: [libcxx] [test] Fix incorrect allocator::construct assertions in associative container tests

2019-05-14 Thread Eric Fiselier via Phabricator via cfe-commits

EricWF added a comment.

From Billy and my last discussion, I think we came to the agreement that it's 
not clear exactly what the "standard behavior" is.


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

https://reviews.llvm.org/D61364



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

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

2019-05-14 Thread Teresa Johnson via Phabricator via cfe-commits

tejohnson added a comment.

In D61634#1502138 , @hfinkel wrote:

> In D61634#1502043 , @efriedma wrote:
>
> > > I have a related patch that turns -fno-builtin* options into module flags
> >
> > Do you have any opinion on representing -fno-builtin using a module flag 
> > vs. a function attribute in IR?  It seems generally more flexible and 
> > easier to reason about a function attribute from my perspective.  But I 
> > might be missing something about the semantics of -fno-builtin that would 
> > make that representation awkward.  Or I guess it might just be more work to 
> > implement, given we have some IPO passes that use TargetLibraryInfo?
>
>
> I think that a function attribute would be better. We generally use these 
> flags only in the context of certain translation units, and when we use LTO, 
> it would be sad if we had to take the most-conservative settings across the 
> entire application. When we insert new function call to a standard library, 
> we always do it in the context of some function. We'd probably need to block 
> inlining in some cases, but that's better than a global conservative setting.


Using function level attributes instead of module flags does provide finer 
grained control and avoids the conservativeness when merging IR for LTO. The 
downsides I see, mostly just in terms of the engineering effort to get this to 
work, are:

- need to prevent inlining with different attributes
- currently the TargetLibraryInfo is constructed on a per-module basis. 
Presumably it would instead need to be created per Function - this one in 
particular seems like it would require fairly extensive changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61634



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

[PATCH] D61364: [libcxx] [test] Fix incorrect allocator::construct assertions in associative container tests

2019-05-14 Thread Billy Robert O'Neal III via Phabricator via cfe-commits

BillyONeal added a comment.

In D61364#1502200 , @EricWF wrote:

> From Billy and my last discussion, I think we came to the agreement that it's 
> not clear exactly what the "standard behavior" is.

No, I don't think so. I think there was agreement that the behavior in the 
standard may not have been what was intended by the original move semantics 
proposals, but I don't think there's any doubt about what the standard 
currently requires.

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

https://reviews.llvm.org/D61364

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

[PATCH] D61364: [libcxx] [test] Fix incorrect allocator::construct assertions in associative container tests

2019-05-14 Thread Eric Fiselier via Phabricator via cfe-commits

EricWF added inline comments.



Comment at: test/std/containers/map_allocator_requirement_test_templates.h:236
 const ValueTp v(42, 1);
-cc->expect();
+cc->expect();
 It ret = c.insert(c.end(), std::move(v));

I really think the current behavior libc++ has here is correct.




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

https://reviews.llvm.org/D61364



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

[PATCH] D36357: Added a better diagnostic when using the delete operator with lambdas

2019-05-14 Thread Nicolas Lesser via Phabricator via cfe-commits

Rakete updated this revision to Diff 199528.
Rakete marked an inline comment as done.
Rakete added a comment.

Nevermind, seems to be working fine even with.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36357

Files:
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseExprCXX.cpp
  clang/test/FixIt/fixit-cxx0x.cpp
  clang/test/Parser/cxx0x-lambda-expressions.cpp
  clang/test/SemaCXX/new-delete-0x.cpp

Index: clang/test/SemaCXX/new-delete-0x.cpp
===
--- clang/test/SemaCXX/new-delete-0x.cpp
+++ clang/test/SemaCXX/new-delete-0x.cpp
@@ -34,6 +34,6 @@
 void bad_deletes()
 {
   // 'delete []' is always array delete, per [expr.delete]p1.
-  // FIXME: Give a better diagnostic.
-  delete []{ return (int*)0; }(); // expected-error {{expected expression}}
+  delete []{ return (int*)0; }(); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 }
+
Index: clang/test/Parser/cxx0x-lambda-expressions.cpp
===
--- clang/test/Parser/cxx0x-lambda-expressions.cpp
+++ clang/test/Parser/cxx0x-lambda-expressions.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify -std=c++2a %s
 
 enum E { e };
 
@@ -43,31 +44,57 @@
 int a4[1] = {[&b] = 1 }; // expected-error{{integral constant expression must have integral or unscoped enumeration type, not 'const int *'}}
 int a5[3] = { []{return 0;}() };
 int a6[1] = {[this] = 1 }; // expected-error{{integral constant expression must have integral or unscoped enumeration type, not 'C *'}}
-int a7[1] = {[d(0)] { return d; } ()}; // expected-warning{{extension}}
-int a8[1] = {[d = 0] { return d; } ()}; // expected-warning{{extension}}
+int a7[1] = {[d(0)] { return d; } ()};
+int a8[1] = {[d = 0] { return d; } ()};
+int a10[1] = {[id(0)] { return id; } ()};
+#if __cplusplus <= 201103L
+// expected-warning@-4{{extension}}
+// expected-warning@-4{{extension}}
+// expected-warning@-4{{extension}}
+#endif
 int a9[1] = {[d = 0] = 1}; // expected-error{{is not an integral constant expression}}
-int a10[1] = {[id(0)] { return id; } ()}; // expected-warning{{extension}}
+#if __cplusplus >= 201402L
+// expected-note@-2{{constant expression cannot modify an object that is visible outside that expression}}
+#endif
 int a11[1] = {[id(0)] = 1};
   }
 
   void delete_lambda(int *p) {
 delete [] p;
 delete [] (int*) { new int }; // ok, compound-literal, not lambda
-delete [] { return new int; } (); // expected-error{{expected expression}}
+delete [] { return new int; } (); // expected-error {{'[]' after delete interpreted as 'delete[]'}}
 delete [&] { return new int; } (); // ok, lambda
+
+delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+delete [](E Enum) { return new int((int)Enum); }(e); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+#if __cplusplus > 201703L
+delete []() { return new int; }(); // expected-error{{'[]' after delete interpreted as 'delete[]'}}
+#endif
   }
 
   // We support init-captures in C++11 as an extension.
   int z;
   void init_capture() {
-[n(0)] () mutable -> int { return ++n; }; // expected-warning{{extension}}
-[n{0}] { return; }; // expected-warning{{extension}}
-[n = 0] { return ++n; }; // expected-error {{captured by copy in a non-mutable}} expected-warning{{extension}}
-[n = {0}] { return; }; // expected-error {{}} expected-warning{{extension}}
-[a([&b = z]{})](){}; // expected-warning 2{{extension}}
+[n(0)] () mutable -> int { return ++n; };
+[n{0}] { return; };
+[a([&b = z]{})](){};
+[n = 0] { return ++n; }; // expected-error {{captured by copy in a non-mutable}}
+[n = {0}] { return; }; // expected-error {{}}
+#if __cplusplus <= 201103L
+// expected-warning@-6{{extension}}
+// expected-warning@-6{{extension}}
+// expected-warning@-6{{extension}}
+// expected-warning@-7{{extension}}
+// expected-warning@-7{{extension}}
+// expected-warning@-7{{extension}}
+#endif
 
 int x = 4;
-auto y = [&r = x, x = x + 1]() -> int { // expected-warning 2{{extension}}
+auto y = [&r = x, x = x + 1]() -> int {
+#if __cplusplus <= 201103L
+  // expected-warning@-2{{extension}}
+  // expected-warning@-3{{extension}}
+#endif
   r += 2;
   return x + 2;
 } ();
Index: clang/test/FixIt/fixit-cxx0x.cpp
===
--- clang/test/FixIt/fixit-cxx0x.cpp
+++ clang/test/FixIt/fixit-cxx0x.cpp
@@ -58,6 +58,9 @@
   (void)[&, i, i]{ }; // expected-error{{'i' can appear only once in a capture list}}
   (void)[] mutable { }; // expected-error{{lam

[PATCH] D61364: [libcxx] [test] Fix incorrect allocator::construct assertions in associative container tests

2019-05-14 Thread Billy Robert O'Neal III via Phabricator via cfe-commits

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



Comment at: test/std/containers/map_allocator_requirement_test_templates.h:236
 const ValueTp v(42, 1);
-cc->expect();
+cc->expect();
 It ret = c.insert(c.end(), std::move(v));

EricWF wrote:
> I really think the current behavior libc++ has here is correct.
> 
> 
File a DR to retroactively make libc++'s behavior standard then? :)


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

https://reviews.llvm.org/D61364



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

[PATCH] D36357: Added a better diagnostic when using the delete operator with lambdas

2019-05-14 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added a comment.

In D36357#1501999 , @Rakete wrote:

> In D36357#1501961 , @rsmith wrote:
>
> > In D36357#1500949 , @Rakete 
> > wrote:
> >
> > > How should I do this? Do I just skip to the next `}`, or also take into 
> > > account  any additional scopes? Also does this mean that I skip and then 
> > > revert, because that seems pretty expensive?
> >
> >
> > It would be a little expensive, yes, but we'd only be doing it on codepaths 
> > where we're producing an error -- for an ill-formed program, it's OK to 
> > take more time in order to produce a better diagnostic. Skipping to the 
> > next `}` won't work, because `SkipUntil` will skip over pairs of 
> > brace-balanced tokens (so you'll skip past the `}` you're looking for), but 
> > skipping until the next `{` and then skipping to the `}` after it should 
> > work.
>
>
> Hmm wouldn't this interact badly with `{}` in initializers?
>
>   [] {};
>   [](int = {0}) {};
>


Ouch. The first of these two seems like it won't work, since `SkipUntil` has no 
idea the angles are brackets. =( Getting this right is ... really hairy. Eg:

  []{} // end of lambda here?
  >(){} // or here?

(The answer depends on whether `a` is a template.) And the same problem shows 
up in the //trailing-return-type// and the //requires-clause//.

How about this: `SkipUntil` an `{` or `<`. If you find a `<` first, don't 
attach a `FixItHint` to the diagnostic since we can't easily tell where to 
suggest putting the `)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36357



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

[PATCH] D60974: Clang IFSO driver action.

2019-05-14 Thread Saleem Abdulrasool via Phabricator via cfe-commits

compnerd added a comment.

How about some cases for:

- global variable which is `static` and `extern`'ed
- global variable which is `static` defined in a function which is `static`
- global variable which is `static` defined in a function which is *not* 
`inline`
- global variable which is `static` which is `static` `inline` (GNU inline 
semantics)
- global variable which is `static` which is `static` `inline` (C99 inline 
semantics)
- virtual functions
- class vtables
- tests for hidden class deriving from hidden class
- tests for hidden class deriving from public class
- tests for public class deriving from public class
- tests for public class deriving from hidden class
- `constexpr` cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60974



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

[PATCH] D61814: [CFG] NFC: Remove implicit conversion from CFGTerminator to Stmt *, make it a variant class instead.

2019-05-14 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ updated this revision to Diff 199531.
NoQ marked 2 inline comments as done.
NoQ added a comment.

Fxd.


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

https://reviews.llvm.org/D61814

Files:
  clang/include/clang/Analysis/CFG.h
  clang/include/clang/Analysis/ProgramPoint.h
  clang/lib/Analysis/CFG.cpp
  clang/lib/Analysis/CFGStmtMap.cpp
  clang/lib/Analysis/Consumed.cpp
  clang/lib/Analysis/LiveVariables.cpp
  clang/lib/Analysis/ProgramPoint.cpp
  clang/lib/Analysis/ReachableCode.cpp
  clang/lib/Analysis/ThreadSafety.cpp
  clang/lib/Analysis/UninitializedValues.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
  clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp

Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -794,7 +794,7 @@
   if (auto SP = P.getAs())
 return SP->getStmt();
   if (auto BE = P.getAs())
-return BE->getSrc()->getTerminator();
+return BE->getSrc()->getTerminatorStmt();
   if (auto CE = P.getAs())
 return CE->getCallExpr();
   if (auto CEE = P.getAs())
Index: clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
===
--- clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
+++ clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
@@ -234,7 +234,7 @@
 
 ProgramPoint P = N->getLocation();
 if (Optional BE = P.getAs())
-  S = BE->getBlock()->getTerminator();
+  S = BE->getBlock()->getTerminatorStmt();
 
 if (S == LoopStmt)
   return false;
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1861,7 +1861,7 @@
   // other constraints) then consider completely unrolling it.
   if(AMgr.options.ShouldUnrollLoops) {
 unsigned maxBlockVisitOnPath = AMgr.options.maxBlockVisitOnPath;
-const Stmt *Term = nodeBuilder.getContext().getBlock()->getTerminator();
+const Stmt *Term = nodeBuilder.getContext().getBlock()->getTerminatorStmt();
 if (Term) {
   ProgramStateRef NewState = updateLoopStack(Term, AMgr.getASTContext(),
  Pred, maxBlockVisitOnPath);
@@ -1882,7 +1882,7 @@
   unsigned int BlockCount = nodeBuilder.getContext().blockCount();
   if (BlockCount == AMgr.options.maxBlockVisitOnPath - 1 &&
   AMgr.options.ShouldWidenLoops) {
-const Stmt *Term = nodeBuilder.getContext().getBlock()->getTerminator();
+const Stmt *Term = nodeBuilder.getContext().getBlock()->getTerminatorStmt();
 if (!(Term &&
   (isa(Term) || isa(Term) || isa(Term
   return;
@@ -2007,8 +2007,8 @@
   if (!BO || !BO->isLogicalOp())
 return Condition;
 
-  assert(!B->getTerminator().isTemporaryDtorsBranch() &&
- "Temporary destructor branches handled by processBindTemporary.");
+  assert(B->getTerminator().isStmtBranch() &&
+ "Other kinds of branches are handled separately!");
 
   // For logical operations, we still have the case where some branches
   // use the traditional "merge" approach and others sink the branch
Index: clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -275,14 +275,14 @@
 }
 
 void CoreEngine::HandleBlockExit(const CFGBlock * B, ExplodedNode *Pred) {
-  if (const Stmt *Term = B->getTerminator()) {
+  if (const Stmt *Term = B->getTerminatorStmt()) {
 switch (Term->getStmtClass()) {
   default:
 llvm_unreachable("Analysis for this terminator not implemented.");
 
   case Stmt::CXXBindTemporaryExprClass:
 HandleCleanupTemporaryBranch(
-cast(B->getTerminator().getStmt()), B, Pred);
+cast(Term), B, Pred);
 return;
 
   // Model static initializers.
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1494,7 +1494,7 @@
 return nullptr;
 
   CFGStmtMap *Map = CurLC->getAnalysisDeclContext()->getCFGStmtMap();
-  CurTerminatorStmt = Map->getBlock(CurStmt)->getTerminator();
+  CurTerminatorStmt = Map->getBlock(CurStmt)->getTerminatorStmt();
 } else {
   return nullptr;
 }
@@ -1566,7 +1566,7 @@
   Pro

[PATCH] D61816: [CFG] [analyzer] pr41300: Add a branch to skip virtual base initializers when they are handled by the superclass.

2019-05-14 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ updated this revision to Diff 199532.
NoQ added a comment.

Rebase.


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

https://reviews.llvm.org/D61816

Files:
  clang/include/clang/Analysis/AnalysisDeclContext.h
  clang/include/clang/Analysis/CFG.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
  clang/lib/Analysis/AnalysisDeclContext.cpp
  clang/lib/Analysis/CFG.cpp
  clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp
  clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/test/Analysis/initializer.cpp
  clang/test/Analysis/initializers-cfg-output.cpp

Index: clang/test/Analysis/initializers-cfg-output.cpp
===
--- clang/test/Analysis/initializers-cfg-output.cpp
+++ clang/test/Analysis/initializers-cfg-output.cpp
@@ -30,21 +30,25 @@
 class B : public virtual A {
 public:
   // CHECK:   B()
-  // CHECK:[B2 (ENTRY)]
-  // CHECK-NEXT: Succs (1): B1
+  // CHECK:[B3 (ENTRY)]
+  // CHECK-NEXT: Succs (1): B2
   // CHECK:[B1]
   // WARNINGS-NEXT: 1:  (CXXConstructExpr, class A)
   // ANALYZER-NEXT: 1:  (CXXConstructExpr, A() (Base initializer), class A)
   // CHECK-NEXT: 2: A([B1.1]) (Base initializer)
   // CHECK-NEXT: Preds (1): B2
   // CHECK-NEXT: Succs (1): B0
+  // CHECK:[B2]
+  // CHECK-NEXT: T: (See if superclass ctor has already initialized vbases)
+  // CHECK-NEXT: Preds (1): B3
+  // CHECK-NEXT: Succs (2): B0 B1
   // CHECK:[B0 (EXIT)]
-  // CHECK-NEXT: Preds (1): B1
+  // CHECK-NEXT: Preds (2): B1 B2
   B() {}
 
   // CHECK:   B(int i)
-  // CHECK:[B2 (ENTRY)]
-  // CHECK-NEXT: Succs (1): B1
+  // CHECK:[B3 (ENTRY)]
+  // CHECK-NEXT: Succs (1): B2
   // CHECK:[B1]
   // CHECK-NEXT: 1: i
   // CHECK-NEXT: 2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
@@ -53,29 +57,37 @@
   // CHECK-NEXT: 4: A([B1.3]) (Base initializer)
   // CHECK-NEXT: Preds (1): B2
   // CHECK-NEXT: Succs (1): B0
+  // CHECK:[B2]
+  // CHECK-NEXT: T: (See if superclass ctor has already initialized vbases)
+  // CHECK-NEXT: Preds (1): B3
+  // CHECK-NEXT: Succs (2): B0 B1
   // CHECK:[B0 (EXIT)]
-  // CHECK-NEXT: Preds (1): B1
+  // CHECK-NEXT: Preds (2): B1 B2
   B(int i) : A(i) {}
 };
 
 class C : public virtual A {
 public:
   // CHECK:   C()
-  // CHECK:[B2 (ENTRY)]
-  // CHECK-NEXT: Succs (1): B1
+  // CHECK:[B3 (ENTRY)]
+  // CHECK-NEXT: Succs (1): B2
   // CHECK:[B1]
   // WARNINGS-NEXT: 1:  (CXXConstructExpr, class A)
   // ANALYZER-NEXT: 1:  (CXXConstructExpr, A() (Base initializer), class A)
   // CHECK-NEXT: 2: A([B1.1]) (Base initializer)
   // CHECK-NEXT: Preds (1): B2
   // CHECK-NEXT: Succs (1): B0
+  // CHECK:[B2]
+  // CHECK-NEXT: T: (See if superclass ctor has already initialized vbases)
+  // CHECK-NEXT: Preds (1): B3
+  // CHECK-NEXT: Succs (2): B0 B1
   // CHECK:[B0 (EXIT)]
-  // CHECK-NEXT: Preds (1): B1
+  // CHECK-NEXT: Preds (2): B1 B2
   C() {}
 
   // CHECK:   C(int i)
-  // CHECK:[B2 (ENTRY)]
-  // CHECK-NEXT: Succs (1): B1
+  // CHECK:[B3 (ENTRY)]
+  // CHECK-NEXT: Succs (1): B2
   // CHECK:[B1]
   // CHECK-NEXT: 1: i
   // CHECK-NEXT: 2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
@@ -84,8 +96,12 @@
   // CHECK-NEXT: 4: A([B1.3]) (Base initializer)
   // CHECK-NEXT: Preds (1): B2
   // CHECK-NEXT: Succs (1): B0
+  // CHECK:[B2]
+  // CHECK-NEXT: T: (See if superclass ctor has already initialized vbases)
+  // CHECK-NEXT: Preds (1): B3
+  // CHECK-NEXT: Succs (2): B0 B1
   // CHECK:[B0 (EXIT)]
-  // CHECK-NEXT: Preds (1): B1
+  // CHECK-NEXT: Preds (2): B1 B2
   C(int i) : A(i) {}
 };
 
@@ -98,31 +114,38 @@
 };
 
 // CHECK:   TestOrder::TestOrder()
-// CHECK:[B2 (ENTRY)]
-// CHECK-NEXT: Succs (1): B1
+// CHECK:[B4 (ENTRY)]
+// CHECK-NEXT: Succs (1): B3
 // CHECK:[B1]
+// WARNINGS-NEXT: 1:  (CXXConstructExpr, class C)
+// ANALYZER-NEXT: 1:  (CXXConstructExpr, C() (Base initializer), class C)
+// CHECK-NEXT: 2: C([B1.1]) (Base initializer)
+// WARNINGS-NEXT: 3:  (CXXConstructExpr, class B)
+// ANALYZER-NEXT: 3:  (CXXConstructExpr, B() (Base initializer), class B)
+// CHECK-NEXT: 4: B([B1.3]) (Base initializer)
+// WARNINGS-NEXT: 5:  (CXXConstructExpr, class A)
+// ANALYZER-NEXT: 5:  (CXXConstructExpr, A() (Base initializer), class A)
+// CHECK-NEXT: 6: A([B1.5]) (Base initializer)
+// CHECK-NEXT: 7: /*implicit*/(int)0
+// CHECK-NEXT: 8: i([B1.7]) (Member initializer)
+// CHECK-NEXT: 9: this
+// CHECK-NEXT:10: [B1.9]->i
+// CHECK-NEXT:11: r([B1.10]) (Member initializer)
+// WARNINGS-NEXT:12:  (CX

1 2 >

1 - 100 of 126 matches

Mail list logo