[PATCH] D49476: [Index] Set OrigD before D is changed.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D49476#1167294, @akyrtzi wrote:

> Is it possible to add a regression test case ? I assume this is fixing some 
> issue, we should make sure we don't regress again.


This fixes a downstream use case where we use `OrigD`.  AFAICT, `c-index-test` 
is used to tests the index library, but it doesn't seem to use `OrigD` at all. 
We could probably add a flag to `c-index-test` to optionally print out `OrigD`. 
I didn't go down this path because I thought this change is trivial and doesn't 
break any existing changes. If you think optionally checking `OrigD` is 
reasonable, I'd be happy to add that with a test case.


Repository:
  rC Clang

https://reviews.llvm.org/D49476



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

[PATCH] D48072: Sema: Fix PR12350 destructor name lookup, addressing (some of) DR244

[Jeremy Morse via Phabricator via cfe-commits]

jmorse added a comment.

Ping


Repository:
  rC Clang

https://reviews.llvm.org/D48072



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

[PATCH] D49417: [clangd] Implement trigram generation algorithm for new symbol index

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.
Herald added a subscriber: arphaman.

In https://reviews.llvm.org/D49417#1166538, @omtcyfz wrote:

> Addressed all comments submitted by Eric.
>
> As discussed internally, I should also exercise my naming skills and come up 
> with a better for the symbol index to substitute "Noctem" which doesn't point 
> to any project's feature.


Ooh, a bikeshed!
Suggestion: `dex` - short, pronouncable, suggests `index`, and is a trigram :-)


https://reviews.llvm.org/D49417



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

[PATCH] D49417: [clangd] Implement trigram generation algorithm for new symbol index

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.cpp:38
+
+// FIXME(kbobyrev): Deal with short symbol symbol names. A viable approach 
would
+// be generating unigrams and bigrams here, too. This would prevent symbol 
index

Please also add what the current behavior is for short names. Do we just ignore 
them?



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.cpp:44
+generateTrigrams(const std::vector> &Segments) {
+  llvm::DenseSet UniqueTrigrams;
+  std::vector Trigrams;

Could we replace `UniqaueTrigrams`+`Trigrams` with a dense map from hash to 
token?



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.cpp:47
+
+  // Extract trigrams consisting of first characters of tokens sorted by of
+  // token positions. Trigram generator is allowed to skip 1 word between each

nit: a redundant "of" EOL.



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.cpp:57
+  // this case.
+  for (auto FirstSegment = Segments.begin(); FirstSegment != Segments.end();
+   ++FirstSegment) {

nit: Maybe S1, S2, S3 instead of FirstSegment, ...?



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.cpp:68
+SearchToken Trigram(
+(*FirstSegment + *SecondSegment + *ThirdSegment).str(),
+SearchToken::Kind::Trigram);

This seems wrong... wouldn't this give you a concatenation of three segments?



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.cpp:68
+SearchToken Trigram(
+(*FirstSegment + *SecondSegment + *ThirdSegment).str(),
+SearchToken::Kind::Trigram);

ioeric wrote:
> This seems wrong... wouldn't this give you a concatenation of three segments?
For trigrams, it might make sense to put 3 chars into a `SmallVector<3>` (can 
be reused) and std::move it into the constructor. Might be cheaper than 
creating a std::string



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.cpp:87
+
+for (size_t Position = 0; Position + 2 < Segment.size(); ++Position)
+  Trigrams.push_back(

nit: `Position <  Segment.size() - 2` seems more commonly used.



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.cpp:92
+
+  for (auto FirstSegment = Segments.begin(); FirstSegment != Segments.end();
+   ++FirstSegment) {

Comment for this loop?



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.cpp:135
+  // Skip underscores at the beginning, e.g. "__builtin_popcount".
+  while (SymbolName[SegmentStart] == '_')
+++SegmentStart;

use `trim`?



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.cpp:138
+
+  for (size_t Index = SegmentStart; Index + 1 < SymbolName.size(); ++Index) {
+const char CurrentSymbol = SymbolName[Index];

Maybe first split name on `_` and them run further upper-lower segmentation on 
the split segments?



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:58
+  // Returns precomputed hash.
+  size_t operator()(const SearchToken &T) const { return Hash; }
+

Any reason this has to be `operator()` instead of a `hash` method? `operator()` 
for hash value is not trivial on the call site.



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:69
+private:
+  friend llvm::hash_code hash_value(const SearchToken &Token) {
+return Token.Hash;

Who's the user of this friend function? Could it just call `Token.hash()`?



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:80
+  /// * Trigram: 3 bytes containing trigram characters
+  /// * Scope: full scope name, e.g. "foo::bar::baz"
+  /// * Path: full or relative path to the directory

nit: scope in clangd world is "foo::bar::baz::", but the global scope is "".



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:159
+/// \brief Combines segmentIdentifier() and generateTrigrams(Segments).
+std::vector generateTrigrams(llvm::StringRef SymbolName);
+

This seems to be the same as just `generateTrigrams(segmentIdentifier(Name))`, 
so I'd drop it.


https://reviews.llvm.org/D49417



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

[PATCH] D49417: [clangd] Implement trigram generation algorithm for new symbol index

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

(just .h files. +1 to eric's comments except where noted)




Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:2
+//===--- SearchToken.h- Symbol Search primitive --*- C++
+//-*-===//
+//

nit: something went wrong here



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:28
+
+/// \brief Hashable SearchToken, which represents a search token primitive.
+///

nit: remove \brief



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:32
+///
+/// * Trigram for fuzzy search on unqualified symbol names.
+/// * Proximity path primitives, e.g. "symbol is defined in directory

maybe just giving one example here, and moving the concrete semantics to Kind?



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:34
+/// * Proximity path primitives, e.g. "symbol is defined in directory
+///   $HOME/dev/llvm or its prefix".
+/// * Scope primitives, e.g. "symbol belongs to namespace foo::bar or its

ITYM suffix
consider just "under directory"



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:35
+///   $HOME/dev/llvm or its prefix".
+/// * Scope primitives, e.g. "symbol belongs to namespace foo::bar or its
+///   prefix".

namespace tokens don't have prefix/suffix semantics, they're exact



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:44
+/// constructing complex iterator trees.
+class SearchToken {
+public:

nit: this is a really common type, and it's namespaced. `Token` is probably 
fine.



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:46
+public:
+  enum class Kind : short {
+Trigram,

doc: the fact that Kind is a namespace for data, and (on each enum value) the 
semantics



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:46
+public:
+  enum class Kind : short {
+Trigram,

sammccall wrote:
> doc: the fact that Kind is a namespace for data, and (on each enum value) the 
> semantics
nit: drop `: short` as you don't seem to make use of it anywhere. Can add it 
later if we want to optimize in-memory structure (currently it has no effect)



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:48
+Trigram,
+Scope,
+Path,

in the first patch, probably just want trigram and scope (to ensure generality) 
and drop the rest



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:52
+
+Custom,
+  };

what is this for?



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:61
+  bool operator==(const SearchToken &Other) const {
+return TokenKind == Other.TokenKind && Data == Other.Data;
+  }

(nit: since you have the hashcode, comparing it before the data is probably 
faster?)



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:64
+
+  const llvm::StringRef data() const { return Data; }
+

nit: first const is not useful



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:64
+
+  const llvm::StringRef data() const { return Data; }
+

sammccall wrote:
> nit: first const is not useful
instead of these accessors, can we just make it a struct with const members and 
a constructor that ensures the hash is correctly set?

That should reflect the lightweightness/lack of behavior in this object



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:69
+private:
+  friend llvm::hash_code hash_value(const SearchToken &Token) {
+return Token.Hash;

ioeric wrote:
> Who's the user of this friend function? Could it just call `Token.hash()`?
This is the LLVM convention for hashing, see `ADT/Hashing.h` 



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:76
+  ///
+  /// These are the examples of Data for different TokenKinds (Token
+  /// Namespaces):

I'd move these to the Kind enum - these shouldn't be examples but rather the 
canonical documentation for these



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:81
+  /// * Scope: full scope name, e.g. "foo::bar::baz"
+  /// * Path: full or relative path to the directory
+  /// * Type: full type name or the USR associated with this type

need to more fully spell this out: when is it full vs relative? is it a URI? Do 
directories have a trailing slash?

But really, leave this out for now.



Comment at: clang-tools-extra/clangd/index/noctem/SearchToken.h:83
+  /// * Type: full type name or the USR associated with this type
+  llvm::SmallString<3> Data;
+  /// \brief Precomputed

[PATCH] D49536: [Analyzer] Quick Fix for exponential execution time when simpilifying complex additive expressions

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware created this revision.
baloghadamsoftware added a reviewer: NoQ.
Herald added subscribers: mikhail.ramalho, a.sidorin, dkrupp, rnkovacs, szepet, 
xazax.hun, whisperity.
Herald added a reviewer: george.karpenkov.

Patch [[https://reviews.llvm.org/rC329780 | [Analyzer] SValBuilder Comparison 
Rearrangement (with Restrictions and Analyzer Option) ]] not only rearranges 
comparisons but also binary expressions. This latter behavior is not protected 
by the analyzer option. Hower, since no complexity threshold is enforced to the 
symbols this may result in exponential execution time if the expressions are 
too complex: Huge static analysis performance regression for very simple 
testcase  For a quick fix we 
extended the analyzer option to also cover the additive cases.

This is only a temporary fix, the final solution should be enforcing the 
complexity threshold to the symbols.


https://reviews.llvm.org/D49536

Files:
  include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  test/Analysis/PR38208.c
  test/Analysis/constraint_manager_negate_difference.c
  test/Analysis/iterator-range.cpp
  test/Analysis/plist-macros.cpp
  test/Analysis/svalbuilder-rearrange-comparisons.c

Index: test/Analysis/svalbuilder-rearrange-comparisons.c
===
--- test/Analysis/svalbuilder-rearrange-comparisons.c
+++ test/Analysis/svalbuilder-rearrange-comparisons.c
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection,core.builtin -analyzer-config aggressive-relational-comparison-simplification=true -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection,core.builtin -analyzer-config aggressive-binary-operation-simplification=true -verify %s
 
 void clang_analyzer_dump(int x);
 void clang_analyzer_eval(int x);
Index: test/Analysis/plist-macros.cpp
===
--- test/Analysis/plist-macros.cpp
+++ test/Analysis/plist-macros.cpp
@@ -637,6 +637,69 @@
 // CHECK-NEXT: end
 // CHECK-NEXT:  
 // CHECK-NEXT:   
+// CHECK-NEXT:line36
+// CHECK-NEXT:col7
+// CHECK-NEXT:file0
+// CHECK-NEXT:   
+// CHECK-NEXT:   
+// CHECK-NEXT:line36
+// CHECK-NEXT:col7
+// CHECK-NEXT:file0
+// CHECK-NEXT:   
+// CHECK-NEXT:  
+// CHECK-NEXT:
+// CHECK-NEXT:   
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT:  kindevent
+// CHECK-NEXT:  location
+// CHECK-NEXT:  
+// CHECK-NEXT:   line36
+// CHECK-NEXT:   col7
+// CHECK-NEXT:   file0
+// CHECK-NEXT:  
+// CHECK-NEXT:  ranges
+// CHECK-NEXT:  
+// CHECK-NEXT:
+// CHECK-NEXT: 
+// CHECK-NEXT:  line36
+// CHECK-NEXT:  col7
+// CHECK-NEXT:  file0
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT:  line36
+// CHECK-NEXT:  col25
+// CHECK-NEXT:  file0
+// CHECK-NEXT: 
+// CHECK-NEXT:
+// CHECK-NEXT:  
+// CHECK-NEXT:  depth0
+// CHECK-NEXT:  extended_message
+// CHECK-NEXT:  Assuming the condition is true
+// CHECK-NEXT:  message
+// CHECK-NEXT:  Assuming the condition is true
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT:  kindcontrol
+// CHECK-NEXT:  edges
+// CHECK-NEXT:   
+// CHECK-NEXT:
+// CHECK-NEXT: start
+// CHECK-NEXT:  
+// CHECK-NEXT:   
+// CHECK-NEXT:line36
+// CHECK-NEXT:col7
+// CHECK-NEXT:file0
+// CHECK-NEXT:   
+// CHECK-NEXT:   
+// CHECK-NEXT:line36
+// CHECK-NEXT:col7
+// CHECK-NEXT:file0
+// CHECK-NEXT:   
+// CHECK-NEXT:  
+// CHECK-NEXT: end
+// CHECK-NEXT:  
+// CHECK-NEXT:   
 // CHECK-NEXT:line37
 // CHECK-NEXT:col5
 // CHECK-NEXT:file0
Index: test/Analysis/iterator-range.cpp
===
--- test/Analysis/iterator-range.cpp
+++ test/Analysis/iterator-range.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-eagerly-assume -analyzer-config aggressive-relational-comparison-simplification=true -analyzer-config c++-container-inlining=false %s -verify
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-eagerly-assume -analyzer-config aggressive-relational-comparison-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-eagerly

[PATCH] D41938: [Analyzer] SValBuilder Comparison Rearrangement (with Restrictions and Analyzer Option)

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In https://reviews.llvm.org/D41938#1167639, @baloghadamsoftware wrote:

> The flag is off by default. Except the rearrangement of additive operations. 
> Should we put it also behind the flag?


I did it as a temporary quick fix: https://reviews.llvm.org/D49536.


Repository:
  rL LLVM

https://reviews.llvm.org/D41938



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

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

[Jan Korous via Phabricator via cfe-commits]

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

LGTM with just a nit about comment wording.

Thanks for the patch!




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

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

Maybe something along this line "any eventual subsequent diagnostics will not 
be visible" would be more clear?


https://reviews.llvm.org/D48786



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

[PATCH] D46190: For a used declaration, mark any associated usings as referenced.

[Carlos Alberto Enciso via Phabricator via cfe-commits]

CarlosAlbertoEnciso updated this revision to Diff 156234.
CarlosAlbertoEnciso marked 11 inline comments as done.
CarlosAlbertoEnciso added a comment.

Address review comments from @probinson


https://reviews.llvm.org/D46190

Files:
  include/clang/Sema/Lookup.h
  include/clang/Sema/Sema.h
  include/clang/Sema/SemaInternal.h
  lib/Sema/SemaCXXScopeSpec.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaLookup.cpp
  lib/Sema/SemaTemplate.cpp
  test/PCH/cxx-templates.cpp
  test/SemaCXX/referenced_alias_declaration_1.cpp
  test/SemaCXX/referenced_alias_declaration_2.cpp
  test/SemaCXX/referenced_using_all.cpp
  test/SemaCXX/referenced_using_declaration_1.cpp
  test/SemaCXX/referenced_using_declaration_2.cpp
  test/SemaCXX/referenced_using_directive.cpp

Index: test/SemaCXX/referenced_using_directive.cpp
===
--- test/SemaCXX/referenced_using_directive.cpp
+++ test/SemaCXX/referenced_using_directive.cpp
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -std=c++11 -ast-dump %s | FileCheck %s --strict-whitespace
+
+namespace N {
+  typedef int Integer;
+  int var;
+}
+
+void Fa() {
+  using namespace N;  // Referenced
+  var = 1;
+}
+
+void Fb() {
+  using namespace N;
+  N::var = 1;
+}
+
+void Fc() {
+  using namespace N;  // Referenced
+  Integer var = 1;
+}
+
+void Fd() {
+  using namespace N;
+  N::Integer var = 1;
+}
+
+//CHECK:  |-FunctionDecl {{.*}} Fa 'void ()'
+//CHECK-NEXT: | `-CompoundStmt {{.*}}
+//CHECK-NEXT: |   |-DeclStmt {{.*}}
+//CHECK-NEXT: |   | `-UsingDirectiveDecl {{.*}} referenced Namespace {{.*}} 'N'
+//CHECK-NEXT: |   `-BinaryOperator {{.*}} 'int' lvalue '='
+//CHECK-NEXT: | |-DeclRefExpr {{.*}} 'int' lvalue Var {{.*}} 'var' 'int'
+//CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 1
+//CHECK-NEXT: |-FunctionDecl {{.*}} Fb 'void ()'
+//CHECK-NEXT: | `-CompoundStmt {{.*}}
+//CHECK-NEXT: |   |-DeclStmt {{.*}}
+//CHECK-NEXT: |   | `-UsingDirectiveDecl {{.*}} Namespace {{.*}} 'N'
+//CHECK-NEXT: |   `-BinaryOperator {{.*}} 'int' lvalue '='
+//CHECK-NEXT: | |-DeclRefExpr {{.*}} 'int' lvalue Var {{.*}} 'var' 'int'
+//CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 1
+//CHECK-NEXT: |-FunctionDecl {{.*}} Fc 'void ()'
+//CHECK-NEXT: | `-CompoundStmt {{.*}}
+//CHECK-NEXT: |   |-DeclStmt {{.*}}
+//CHECK-NEXT: |   | `-UsingDirectiveDecl {{.*}} referenced Namespace {{.*}} 'N'
+//CHECK-NEXT: |   `-DeclStmt {{.*}}
+//CHECK-NEXT: | `-VarDecl {{.*}} var 'N::Integer':'int' cinit
+//CHECK-NEXT: |   `-IntegerLiteral {{.*}} 'int' 1
+//CHECK-NEXT: `-FunctionDecl {{.*}} Fd 'void ()'
+//CHECK-NEXT:   `-CompoundStmt {{.*}}
+//CHECK-NEXT: |-DeclStmt {{.*}}
+//CHECK-NEXT: | `-UsingDirectiveDecl {{.*}} Namespace {{.*}} 'N'
+//CHECK-NEXT: `-DeclStmt {{.*}}
+//CHECK-NEXT:   `-VarDecl {{.*}} var 'N::Integer':'int' cinit
+//CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1
Index: test/SemaCXX/referenced_using_declaration_2.cpp
===
--- test/SemaCXX/referenced_using_declaration_2.cpp
+++ test/SemaCXX/referenced_using_declaration_2.cpp
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -std=c++11 -ast-dump %s | FileCheck %s --strict-whitespace
+
+namespace N {
+  typedef int Integer;
+  typedef char Char;
+}
+
+using N::Integer;
+using N::Char;  // Referenced
+
+void Foo(int p1, N::Integer p2, Char p3) {
+  N::Integer var;
+  var = 0;
+}
+
+using N::Integer;   // Referenced
+Integer Bar() {
+  using N::Char;
+  return 0;
+}
+
+//CHECK:  |-UsingDecl {{.*}} N::Integer
+//CHECK-NEXT: |-UsingShadowDecl {{.*}} implicit Typedef {{.*}} 'Integer'
+//CHECK-NEXT: | `-TypedefType {{.*}} 'N::Integer' sugar
+//CHECK-NEXT: |   |-Typedef {{.*}} 'Integer'
+//CHECK-NEXT: |   `-BuiltinType {{.*}} 'int'
+//CHECK-NEXT: |-UsingDecl {{.*}} referenced N::Char
+//CHECK-NEXT: |-UsingShadowDecl {{.*}} implicit referenced Typedef {{.*}} 'Char'
+//CHECK-NEXT: | `-TypedefType {{.*}} 'N::Char' sugar
+//CHECK-NEXT: |   |-Typedef {{.*}} 'Char'
+//CHECK-NEXT: |   `-BuiltinType {{.*}} 'char'
+//CHECK-NEXT: |-FunctionDecl {{.*}} Foo 'void (int, N::Integer, N::Char)'
+//CHECK-NEXT: | |-ParmVarDecl {{.*}} p1 'int'
+//CHECK-NEXT: | |-ParmVarDecl {{.*}} p2 'N::Integer':'int'
+//CHECK-NEXT: | |-ParmVarDecl {{.*}} p3 'N::Char':'char'
+//CHECK-NEXT: | `-CompoundStmt {{.*}}
+//CHECK-NEXT: |   |-DeclStmt {{.*}}
+//CHECK-NEXT: |   | `-VarDecl {{.*}} used var 'N::Integer':'int'
+//CHECK-NEXT: |   `-BinaryOperator {{.*}} 'N::Integer':'int' lvalue '='
+//CHECK-NEXT: | |-DeclRefExpr {{.*}} 'N::Integer':'int' lvalue Var {{.*}} 'var' 'N::Integer':'int'
+//CHECK-NEXT: | `-IntegerLiteral {{.*}} 'int' 0
+//CHECK-NEXT: |-UsingDecl {{.*}} referenced N::Integer
+//CHECK-NEXT: |-UsingShadowDecl {{.*}} implicit referenced Typedef {{.*}} 'Integer'
+//CHECK-NEXT: | `-TypedefType {{.*}} 'N::Integer' sugar
+//CHECK-NEXT: |   |-Typedef {{.*}} 'Integer'
+//CHECK-NEXT: 

[PATCH] D46190: For a used declaration, mark any associated usings as referenced.

[Carlos Alberto Enciso via Phabricator via cfe-commits]

CarlosAlbertoEnciso added inline comments.



Comment at: include/clang/Sema/SemaInternal.h:91
   Var->markUsed(SemaRef.Context);
+  SemaRef.MarkUsingReferenced(Var, Loc, /*CXXScopeSpec*=*/nullptr, RefExpr);
 }

probinson wrote:
> The comments on a nullptr parameter usually use the formal parameter name, 
> not its type.
Replaced the type name with the formal parameter name.



Comment at: lib/Sema/SemaDeclCXX.cpp:15554
+
+  for (LookupResult::iterator I = R.begin(), E = R.end(); I != E; ++I) {
+NamedDecl *D = (*I)->getUnderlyingDecl();

probinson wrote:
> Could this be a range-based for loop?
Replaced by a range-based for loop.



Comment at: lib/Sema/SemaExpr.cpp:14460
   Func->markUsed(Context);
+  MarkUsingReferenced(Func, Loc, /*CXXScopeSpec*=*/nullptr, E);
 }

probinson wrote:
> Parameter comments usually use the formal parameter name, not its type.
Replaced the type name with the formal parameter name.



Comment at: lib/Sema/SemaExpr.cpp:15374
+  SemaRef.MarkAnyDeclReferenced(Loc, D, MightBeOdrUse,
+/*CXXScopeSpec*=*/nullptr, E);
 

probinson wrote:
> Parameter comments usually use the formal parameter name, not its type.
Replaced the type name with the formal parameter name.



Comment at: lib/Sema/SemaExpr.cpp:15396
+SemaRef.MarkAnyDeclReferenced(Loc, DM, MightBeOdrUse,
+  /*CXXScopeSpec*=*/nullptr, E);
 }

probinson wrote:
> Parameter comments usually use the formal parameter name, not its type.
Replaced the type name with the formal parameter name.



Comment at: lib/Sema/SemaLookup.cpp:98
 
+using UDirs = llvm::SmallPtrSet;
+UDirs usings;

As `UDirs` is used to reference an instance of `UnqualUsingDirectiveSet`, I 
have changed `UDirs` to `UsingDirs`.



Comment at: lib/Sema/SemaLookup.cpp:196
+void addUsingDirective(LookupResult &R) {
+  for (auto I = usings.begin(), E = usings.end(); I != E; ++I)
+R.addDecl((*I));

probinson wrote:
> Can this be a range-based for loop?
Replaced with a range-based for loop.



Comment at: lib/Sema/SemaLookup.cpp:1064
+static void
+AddUsingDirectives(LookupResult &R,UnqualUsingDirectiveSet &UDirs) {
+  if (R.isAddUsingDirectives())

probinson wrote:
> Space after the comma.
> `UDirs` has a different meaning elsewhere in this file, maybe `UsingDirs` 
> instead?
Preserved 'UDirs' in this function, but changed the 'UDirs' typedef into 
'UsingDirs'.


https://reviews.llvm.org/D46190



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

[PATCH] D46190: For a used declaration, mark any associated usings as referenced.

[Carlos Alberto Enciso via Phabricator via cfe-commits]

CarlosAlbertoEnciso added a comment.

In https://reviews.llvm.org/D46190#1166775, @probinson wrote:

> A bunch of style comments.  Maybe try clang-format-diff.


@probinson:

Thanks very much for your review.


https://reviews.llvm.org/D46190



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

[PATCH] D49508: [Sema] Expr::skipRValueSubobjectAdjustments(): record skipped NoOp casts.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 156235.
lebedev.ri retitled this revision from "[CodeGen] 
VisitMaterializeTemporaryExpr(): don't skip NoOp Casts." to "[Sema] 
Expr::skipRValueSubobjectAdjustments(): record skipped NoOp casts.".
lebedev.ri edited the summary of this revision.
lebedev.ri added a comment.

In https://reviews.llvm.org/D49508#1167177, @efriedma wrote:

> Would it be enough to accumulate the skipped casts into a SmallVector, like 
> we do for the skipped comma operators?


Duuuh, why did i not try this in the first place?!
That works, thanks!


Repository:
  rC Clang

https://reviews.llvm.org/D49508

Files:
  include/clang/AST/Expr.h
  lib/AST/Expr.cpp
  lib/AST/ExprConstant.cpp
  lib/Analysis/CFG.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGExprConstant.cpp
  lib/Sema/JumpDiagnostics.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp

Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -358,9 +358,10 @@
 
   SmallVector CommaLHSs;
   SmallVector Adjustments;
+  SmallVector SkippedCasts;
 
   const Expr *Init = InitWithAdjustments->skipRValueSubobjectAdjustments(
-  CommaLHSs, Adjustments);
+  CommaLHSs, Adjustments, SkippedCasts);
 
   // Take the region for Init, i.e. for the whole object. If we do not remember
   // the region in which the object originally was constructed, come up with
Index: lib/Sema/JumpDiagnostics.cpp
===
--- lib/Sema/JumpDiagnostics.cpp
+++ lib/Sema/JumpDiagnostics.cpp
@@ -531,9 +531,10 @@
 if (MTE->getStorageDuration() == SD_Automatic) {
   SmallVector CommaLHS;
   SmallVector Adjustments;
+  SmallVector SkippedCasts;
   const Expr *ExtendedObject =
   MTE->GetTemporaryExpr()->skipRValueSubobjectAdjustments(
-  CommaLHS, Adjustments);
+  CommaLHS, Adjustments, SkippedCasts);
   if (ExtendedObject->getType().isDestructedType()) {
 Scopes.push_back(GotoScope(ParentScope, 0,
diag::note_exits_temporary_dtor,
Index: lib/CodeGen/CGExprConstant.cpp
===
--- lib/CodeGen/CGExprConstant.cpp
+++ lib/CodeGen/CGExprConstant.cpp
@@ -1829,14 +1829,14 @@
   return CGM.GetAddrOfUuidDescriptor(E);
 }
 
-ConstantLValue
-ConstantLValueEmitter::VisitMaterializeTemporaryExpr(
-const MaterializeTemporaryExpr *E) {
+ConstantLValue ConstantLValueEmitter::VisitMaterializeTemporaryExpr(
+const MaterializeTemporaryExpr *E) {
   assert(E->getStorageDuration() == SD_Static);
   SmallVector CommaLHSs;
   SmallVector Adjustments;
-  const Expr *Inner = E->GetTemporaryExpr()
-  ->skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
+  SmallVector SkippedCasts;
+  const Expr *Inner = E->GetTemporaryExpr()->skipRValueSubobjectAdjustments(
+  CommaLHSs, Adjustments, SkippedCasts);
   return CGM.GetAddrOfGlobalTemporary(E, Inner);
 }
 
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -467,7 +467,8 @@
 
   SmallVector CommaLHSs;
   SmallVector Adjustments;
-  E = E->skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
+  SmallVector SkippedCasts;
+  E = E->skipRValueSubobjectAdjustments(CommaLHSs, Adjustments, SkippedCasts);
 
   for (const auto &Ignored : CommaLHSs)
 EmitIgnoredExpr(Ignored);
Index: lib/Analysis/CFG.cpp
===
--- lib/Analysis/CFG.cpp
+++ lib/Analysis/CFG.cpp
@@ -1539,8 +1539,9 @@
 // Skip sub-object accesses into rvalues.
 SmallVector CommaLHSs;
 SmallVector Adjustments;
-const Expr *SkippedInit =
-Init->skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
+SmallVector SkippedCasts;
+const Expr *SkippedInit = Init->skipRValueSubobjectAdjustments(
+CommaLHSs, Adjustments, SkippedCasts);
 if (SkippedInit != Init) {
   Init = SkippedInit;
   continue;
@@ -4384,11 +4385,12 @@
   BindToTemporary = (MTE->getStorageDuration() != SD_FullExpression);
   SmallVector CommaLHSs;
   SmallVector Adjustments;
+  SmallVector SkippedCasts;
   // Find the expression whose lifetime needs to be extended.
-  E = const_cast(
-  cast(E)
-  ->GetTemporaryExpr()
-  ->skipRValueSubobjectAdjustments(CommaLHSs, Adjustments));
+  E = const_cast(cast(E)
+ ->GetTemporaryExpr()
+ ->skipRValueSubobjectAdjustments(
+ CommaLHSs, Adjustments, SkippedCasts));
   // Visit the skipped comma operator left-hand sides for other temporaries.
   for (const Expr *CommaLHS : CommaLHSs) {
 VisitFo

[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 156237.
lebedev.ri added a comment.

Breakthrough: no more false-positives due to the `MaterializeTemporaryExpr` 
skipping over NoOp casts. (https://reviews.llvm.org/D49508)
Slight docs update.

Ping, please review!
We are so close :)


Repository:
  rC Clang

https://reviews.llvm.org/D48958

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGExprConstant.cpp
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -31,6 +31,21 @@
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
 // CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent),?){5}"}}
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-RECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fno-sanitize-recover=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-NORECOVER
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-cast -fsanitize-trap=implicit-cast %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-IMPLICIT-CAST,CHECK-IMPLICIT-CAST-TRAP
+// CHECK-IMPLICIT-CAST: "-fsanitize={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-RECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}} // ???
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-NORECOVER-NOT: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP: "-fsanitize-trap={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fsanitize-recover={{((implicit-integer-truncation),?){1}"}}
+// CHECK-IMPLICIT-CAST-TRAP-NOT: "-fno-sanitize-recover={{((implicit-integer-truncation),?){1}"}}
+
 // RUN: %clang -fsanitize=bounds -### -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDS
 // CHECK-BOUNDS: "-fsanitize={{((array-bounds|local-bounds),?){2}"}}
 
Index: test/CodeGenCXX/catch-implicit-integer-truncations.cpp
===
--- /dev/null
+++ test/CodeGenCXX/catch-implicit-integer-truncations.cpp
@@ -0,0 +1,254 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fno-sanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-NORECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-recover=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-ANYRECOVER,CHECK-SANITIZE-RECOVER
+// RUN: %clang_cc1 -fsanitize=implicit-integer-truncation -fsanitize-trap=implicit-integer-truncation -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,CHECK-SANITIZE,CHECK-SANITIZE-TRAP
+
+extern "C" { // Disable name mangling.
+
+// == //
+// Check that explicit cast does not interfere with implicit cast
+// == //
+// These contain one implicit truncating cast, and one explicit truncating cast.
+// We want to make sure that we still diagnose the implicit cast.
+
+// Implicit truncation after explicit truncation.
+// CHECK-LABEL: @explicit_cast_interference0
+unsigned char explicit_cast_interference0(unsigned int c) {
+  // CHECK-SANITIZE: %[[ANYEXT:.*]] = zext i8 %[[DST:.*]] to i16, !nosanitize
+  // CHECK-SANITIZE: call
+  // CHECK-SANITIZE-NOT: call
+  // CHECK: }
+  return (unsigned short)c;
+}
+
+// Implicit tru

[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:23
+
+const char DefaultIgnoredValues[] = "0;1;2;10;100;";
+

0x8000- wrote:
> aaron.ballman wrote:
> > lebedev.ri wrote:
> > > aaron.ballman wrote:
> > > > Why 2, 10, and 100?
> > > These really should be a config option.
> > These are the default values for the config option. I think 0 and 1 make a 
> > lot of sense to have in the default list given how prevalent they are in 
> > real world code. But the other three seem to be used far less often.
> > 
> > I think if we wanted to have any values other than 0 and 1 (and perhaps -1) 
> > as defaults, I'd want to see some data on large amounts of code to justify 
> > anything else.
> No need for -1, as it is covered by 1, plus unary operator.
> 
> I know 100 is used often for converting percentages.
> 
> 10 is used for number parsing (yes, many people still do that by hand instead 
> of libraries).
> 
> But this is the configuration - and I'm ok with reverting to 0 and 1 as I had 
> originally.
Good point on -1. I'd say let's revert until we have statistics to justify any 
other default values. That said, I'd still be curious to know how chatty this 
is over large code bases. How many times does this trigger in LLVM, for 
instance?



Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:30-32
+  for (const std::string &IgnoredValue : IngnoredValuesInput) {
+IngnoredValues.push_back(std::stoll(IgnoredValue));
+  }

0x8000- wrote:
> aaron.ballman wrote:
> > This can be replaced with `llvm::transform(IgnoredValuesInput, 
> > IgnoredValues.begin(), std::stoll);`
> /home/florin/tools/llvm/tools/clang/tools/extra/clang-tidy/readability/MagicNumbersCheck.cpp:30:3:
>  error: no matching function for call to 'transform'  
>   
>   llvm::transform(IgnoredValuesInput, IgnoredIntegerValues.begin(), 
> std::stoll);
>   ^~~
> /home/florin/tools/llvm/include/llvm/ADT/STLExtras.h:990:10: note: candidate 
> template ignored: couldn't infer template argument 'UnaryPredicate'   
>
> OutputIt transform(R &&Range, OutputIt d_first, UnaryPredicate P) {
>  ^
> 1 error generated.
> 
> Shall I wrap it in a lambda?
Yeah, I'd wrap in a lambda then.



Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:41-42
+void MagicNumbersCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(integerLiteral().bind("ii"), this);
+  Finder->addMatcher(floatLiteral().bind("ff"), this);
+}

0x8000- wrote:
> aaron.ballman wrote:
> > The names `ii` and `ff` could be a bit more user-friendly. Also, this can 
> > be written using a single matcher, I think.
> > `anyOf(integerLiteral().bind("integer"), floatLiteral().bind("float"))`
> addMatcher(anyOf(...), this) is ambiguous. Is there any benefit over the two 
> statements?
Better memoization, which may not really be critical given how trivial the 
matchers are.



Comment at: test/clang-tidy/readability-magic-numbers.cpp:16
+void BuggyFunction() {
+  int BadLocalInt = 6;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: magic number integer literal 6 
[readability-magic-numbers]

0x8000- wrote:
> 0x8000- wrote:
> > Quuxplusone wrote:
> > > Please add test cases immediately following this one, for
> > > 
> > > const int BadLocalConstInt = 6;
> > > constexpr int BadLocalConstexprInt = 6;
> > > static const int BadLocalStaticConstInt = 6;
> > > static constexpr int BadLocalStaticConstexprInt = 6;
> > > 
> > > (except of course changing "Bad" to "Good" in any cases where 6 is 
> > > acceptable as an initializer).
> > > 
> > > Also
> > > 
> > > std::vector BadLocalVector(6);
> > > const std::vector BadLocalConstVector(6);
> > > 
> > > etc. etc.
> > Again... all the "const .* (\d)+" patterns should be acceptable. We're 
> > initializing a constant. Would you prefer an explicit option?
> I have  template and constructor arguments already in the test. I have tried 
> including  but somehow it is not found and the std::vector is 
> reported as an error in itself.
Tests need to be hermetic and cannot rely on STL headers or other system 
headers. Basically, you have to replicate the contents of  (or 
whatever) within the test file for whatever you're trying to test.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114



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

[PATCH] D49540: [clangd] FuzzyMatch exposes an API for its word segmentation. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added reviewers: ioeric, omtcyfz.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, 
ilya-biryukov.

This is intended to be used for indexing, e.g. in 
https://reviews.llvm.org/D49417


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49540

Files:
  clangd/FuzzyMatch.cpp
  clangd/FuzzyMatch.h
  unittests/clangd/FuzzyMatchTests.cpp

Index: unittests/clangd/FuzzyMatchTests.cpp
===
--- unittests/clangd/FuzzyMatchTests.cpp
+++ unittests/clangd/FuzzyMatchTests.cpp
@@ -273,6 +273,29 @@
   EXPECT_THAT("Abs", matches("[abs]", 2.f));
 }
 
+// Returns pretty-printed segmentation of Text.
+// e.g. std::basic_string --> +--  + +-
+std::string segment(StringRef Text) {
+  std::vector Roles(Text.size());
+  calculateRoles(Text, Roles);
+  std::string Printed;
+  for (unsigned I = 0; I < Text.size(); ++I)
+Printed.push_back("?-+ "[static_cast(Roles[I])]);
+  return Printed;
+}
+
+// this is a no-op hack so clang-format will vertically align our testcases.
+StringRef returns(StringRef Text) { return Text; }
+
+TEST(FuzzyMatch, Segmentation) {
+  EXPECT_THAT(segment("std::basic_string"), //
+  returns("+--  + +-"));
+  EXPECT_THAT(segment("XMLHttpRequest"), //
+  returns("+--+---+--"));
+  EXPECT_THAT(segment("t3h PeNgU1N oF d00m"), //
+  returns("+-- +-+-+-+ ++ +---"));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/FuzzyMatch.h
===
--- clangd/FuzzyMatch.h
+++ clangd/FuzzyMatch.h
@@ -16,14 +16,57 @@
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FUZZYMATCH_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FUZZYMATCH_H
 
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 namespace clangd {
 
+// Utilities for word segmentation.
+// FuzzyMatcher already incorporates this logic, so most users don't need this.
+//
+// A name like "fooBar_baz" consists of several parts foo, bar, baz.
+// Aligning segmentation of word and pattern improves the fuzzy-match.
+// For example: [lol] matches "LaughingOutLoud" better than "LionPopulation"
+//
+// First we classify each character into types (uppercase, lowercase, etc).
+// Then we look at the sequence: e.g. [upper, lower] is the start of a segment.
+
+// We distinguish the types of characters that affect segmentation.
+// It's not obvious how to segment digits, we treat them as lowercase letters.
+// As we don't decode UTF-8, we treat bytes over 127 as lowercase too.
+// This means we require exact (case-sensitive) match for those characters.
+enum CharType : unsigned char {
+  Empty = 0,   // Before-the-start and after-the-end (and control chars).
+  Lower = 1,   // Lowercase letters, digits, and non-ASCII bytes.
+  Upper = 2,   // Uppercase letters.
+  Punctuation = 3, // ASCII punctuation (including Space)
+};
+// A CharTypeSet is a bitfield representing all the character types in a word.
+// Its bits are 1< Roles);
+
 // A matcher capable of matching and scoring strings against a single pattern.
 // It's optimized for matching against many strings - match() does not allocate.
 class FuzzyMatcher {
@@ -48,8 +91,6 @@
 private:
   // We truncate the pattern and the word to bound the cost of matching.
   constexpr static int MaxPat = 63, MaxWord = 127;
-  enum CharRole : unsigned char; // For segmentation.
-  enum CharType : unsigned char; // For segmentation.
   // Action describes how a word character was matched to the pattern.
   // It should be an enum, but this causes bitfield problems:
   //   - for MSVC the enum type must be explicitly unsigned for correctness
@@ -60,7 +101,6 @@
 
   bool init(llvm::StringRef Word);
   void buildGraph();
-  void calculateRoles(const char *Text, CharRole *Out, int &Types, int N);
   bool allowMatch(int P, int W, Action Last) const;
   int skipPenalty(int W, Action Last) const;
   int matchBonus(int P, int W, Action Last) const;
@@ -70,15 +110,15 @@
   int PatN; // Length
   char LowPat[MaxPat];  // Pattern in lowercase
   CharRole PatRole[MaxPat]; // Pattern segmentation info
-  int PatTypeSet;   // Bitmask of 1< 0)
-calculateRoles(Pat, PatRole, PatTypeSet, PatN);
+  PatTypeSet =
+  calculateRoles(StringRef(Pat, PatN), makeMutableArrayRef(PatRole, PatN));
 }
 
 Optional FuzzyMatcher::match(StringRef Word) {
@@ -110,25 +110,6 @@
   return Score;
 }
 
-// Segmentation of words and patterns.
-// A name like "fooBar_baz" consists of several parts foo, bar, baz.
-// Aligning segmentation of word and pattern improves the fuzzy-match.
-// For example: [lol] matches "LaughingOutLoud" better than "LionPopulation"
-//
-// First we classify each character into types (uppercase, l

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM as well


https://reviews.llvm.org/D48786



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

r337447 - [PCH+Modules] Load -fmodule-map-file content before including PCHs

[Bruno Cardoso Lopes via cfe-commits]

Author: bruno
Date: Thu Jul 19 05:32:06 2018
New Revision: 337447

URL: http://llvm.org/viewvc/llvm-project?rev=337447&view=rev
Log:
[PCH+Modules] Load -fmodule-map-file content before including PCHs

Consider:
1) Generate PCH with -fmodules and -fmodule-map-file
2) Use PCH with -fmodules and the same -fmodule-map-file

If we don't load -fmodule-map-file content before including PCHs,
the modules that are dependencies in PCHs cannot get loaded,
since there's no matching module map file when reading back the AST.

rdar://problem/40852867

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

Added:
cfe/trunk/test/Modules/module-imported-by-pch-with-modulemap.m
Modified:
cfe/trunk/lib/Frontend/FrontendAction.cpp

Modified: cfe/trunk/lib/Frontend/FrontendAction.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/FrontendAction.cpp?rev=337447&r1=337446&r2=337447&view=diff
==
--- cfe/trunk/lib/Frontend/FrontendAction.cpp (original)
+++ cfe/trunk/lib/Frontend/FrontendAction.cpp Thu Jul 19 05:32:06 2018
@@ -766,6 +766,22 @@ bool FrontendAction::BeginSourceFile(Com
   if (!BeginSourceFileAction(CI))
 goto failure;
 
+  // If we were asked to load any module map files, do so now.
+  for (const auto &Filename : CI.getFrontendOpts().ModuleMapFiles) {
+if (auto *File = CI.getFileManager().getFile(Filename))
+  CI.getPreprocessor().getHeaderSearchInfo().loadModuleMapFile(
+  File, /*IsSystem*/false);
+else
+  CI.getDiagnostics().Report(diag::err_module_map_not_found) << Filename;
+  }
+
+  // Add a module declaration scope so that modules from -fmodule-map-file
+  // arguments may shadow modules found implicitly in search paths.
+  CI.getPreprocessor()
+  .getHeaderSearchInfo()
+  .getModuleMap()
+  .finishModuleDeclarationScope();
+
   // Create the AST context and consumer unless this is a preprocessor only
   // action.
   if (!usesPreprocessorOnly()) {
@@ -855,22 +871,6 @@ bool FrontendAction::BeginSourceFile(Com
"doesn't support modules");
   }
 
-  // If we were asked to load any module map files, do so now.
-  for (const auto &Filename : CI.getFrontendOpts().ModuleMapFiles) {
-if (auto *File = CI.getFileManager().getFile(Filename))
-  CI.getPreprocessor().getHeaderSearchInfo().loadModuleMapFile(
-  File, /*IsSystem*/false);
-else
-  CI.getDiagnostics().Report(diag::err_module_map_not_found) << Filename;
-  }
-
-  // Add a module declaration scope so that modules from -fmodule-map-file
-  // arguments may shadow modules found implicitly in search paths.
-  CI.getPreprocessor()
-  .getHeaderSearchInfo()
-  .getModuleMap()
-  .finishModuleDeclarationScope();
-
   // If we were asked to load any module files, do so now.
   for (const auto &ModuleFile : CI.getFrontendOpts().ModuleFiles)
 if (!CI.loadModuleFile(ModuleFile))

Added: cfe/trunk/test/Modules/module-imported-by-pch-with-modulemap.m
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/module-imported-by-pch-with-modulemap.m?rev=337447&view=auto
==
--- cfe/trunk/test/Modules/module-imported-by-pch-with-modulemap.m (added)
+++ cfe/trunk/test/Modules/module-imported-by-pch-with-modulemap.m Thu Jul 19 
05:32:06 2018
@@ -0,0 +1,16 @@
+// RUN: rm -rf %t.dst %t.cache
+// RUN: mkdir -p %t.dst/folder-with-modulemap %t.dst/pch-folder
+// RUN: echo '#import "folder-with-modulemap/included.h"' > %t.dst/header.h
+// RUN: echo 'extern int MyModuleVersion;' > 
%t.dst/folder-with-modulemap/MyModule.h
+// RUN: echo '@import MyModule;' > %t.dst/folder-with-modulemap/included.h
+// RUN: echo 'module MyModule { header "MyModule.h" }' > 
%t.dst/folder-with-modulemap/MyModule.modulemap
+
+// RUN: %clang_cc1 -emit-pch -o %t.dst/pch-folder/header.pch 
-fmodule-map-file=%t.dst/folder-with-modulemap/MyModule.modulemap -x 
objective-c-header -fmodules-cache-path=%t.cache -fmodules 
-fimplicit-module-maps %t.dst/header.h
+// RUN: %clang_cc1 -fsyntax-only 
-fmodule-map-file=%t.dst/folder-with-modulemap/MyModule.modulemap 
-fmodules-cache-path=%t.cache -fmodules -fimplicit-module-maps %s -include-pch 
%t.dst/pch-folder/header.pch -verify
+
+// expected-no-diagnostics
+
+void test() {
+  (void)MyModuleVersion; // should be found by implicit import
+}
+


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

[PATCH] D48685: [PCH+Modules] Load -fmodule-map-file content before including PCHs

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL337447: [PCH+Modules] Load -fmodule-map-file content before 
including PCHs (authored by bruno, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D48685?vs=153205&id=156247#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D48685

Files:
  cfe/trunk/lib/Frontend/FrontendAction.cpp
  cfe/trunk/test/Modules/module-imported-by-pch-with-modulemap.m


Index: cfe/trunk/test/Modules/module-imported-by-pch-with-modulemap.m
===
--- cfe/trunk/test/Modules/module-imported-by-pch-with-modulemap.m
+++ cfe/trunk/test/Modules/module-imported-by-pch-with-modulemap.m
@@ -0,0 +1,16 @@
+// RUN: rm -rf %t.dst %t.cache
+// RUN: mkdir -p %t.dst/folder-with-modulemap %t.dst/pch-folder
+// RUN: echo '#import "folder-with-modulemap/included.h"' > %t.dst/header.h
+// RUN: echo 'extern int MyModuleVersion;' > 
%t.dst/folder-with-modulemap/MyModule.h
+// RUN: echo '@import MyModule;' > %t.dst/folder-with-modulemap/included.h
+// RUN: echo 'module MyModule { header "MyModule.h" }' > 
%t.dst/folder-with-modulemap/MyModule.modulemap
+
+// RUN: %clang_cc1 -emit-pch -o %t.dst/pch-folder/header.pch 
-fmodule-map-file=%t.dst/folder-with-modulemap/MyModule.modulemap -x 
objective-c-header -fmodules-cache-path=%t.cache -fmodules 
-fimplicit-module-maps %t.dst/header.h
+// RUN: %clang_cc1 -fsyntax-only 
-fmodule-map-file=%t.dst/folder-with-modulemap/MyModule.modulemap 
-fmodules-cache-path=%t.cache -fmodules -fimplicit-module-maps %s -include-pch 
%t.dst/pch-folder/header.pch -verify
+
+// expected-no-diagnostics
+
+void test() {
+  (void)MyModuleVersion; // should be found by implicit import
+}
+
Index: cfe/trunk/lib/Frontend/FrontendAction.cpp
===
--- cfe/trunk/lib/Frontend/FrontendAction.cpp
+++ cfe/trunk/lib/Frontend/FrontendAction.cpp
@@ -766,6 +766,22 @@
   if (!BeginSourceFileAction(CI))
 goto failure;
 
+  // If we were asked to load any module map files, do so now.
+  for (const auto &Filename : CI.getFrontendOpts().ModuleMapFiles) {
+if (auto *File = CI.getFileManager().getFile(Filename))
+  CI.getPreprocessor().getHeaderSearchInfo().loadModuleMapFile(
+  File, /*IsSystem*/false);
+else
+  CI.getDiagnostics().Report(diag::err_module_map_not_found) << Filename;
+  }
+
+  // Add a module declaration scope so that modules from -fmodule-map-file
+  // arguments may shadow modules found implicitly in search paths.
+  CI.getPreprocessor()
+  .getHeaderSearchInfo()
+  .getModuleMap()
+  .finishModuleDeclarationScope();
+
   // Create the AST context and consumer unless this is a preprocessor only
   // action.
   if (!usesPreprocessorOnly()) {
@@ -855,22 +871,6 @@
"doesn't support modules");
   }
 
-  // If we were asked to load any module map files, do so now.
-  for (const auto &Filename : CI.getFrontendOpts().ModuleMapFiles) {
-if (auto *File = CI.getFileManager().getFile(Filename))
-  CI.getPreprocessor().getHeaderSearchInfo().loadModuleMapFile(
-  File, /*IsSystem*/false);
-else
-  CI.getDiagnostics().Report(diag::err_module_map_not_found) << Filename;
-  }
-
-  // Add a module declaration scope so that modules from -fmodule-map-file
-  // arguments may shadow modules found implicitly in search paths.
-  CI.getPreprocessor()
-  .getHeaderSearchInfo()
-  .getModuleMap()
-  .finishModuleDeclarationScope();
-
   // If we were asked to load any module files, do so now.
   for (const auto &ModuleFile : CI.getFrontendOpts().ModuleFiles)
 if (!CI.loadModuleFile(ModuleFile))


Index: cfe/trunk/test/Modules/module-imported-by-pch-with-modulemap.m
===
--- cfe/trunk/test/Modules/module-imported-by-pch-with-modulemap.m
+++ cfe/trunk/test/Modules/module-imported-by-pch-with-modulemap.m
@@ -0,0 +1,16 @@
+// RUN: rm -rf %t.dst %t.cache
+// RUN: mkdir -p %t.dst/folder-with-modulemap %t.dst/pch-folder
+// RUN: echo '#import "folder-with-modulemap/included.h"' > %t.dst/header.h
+// RUN: echo 'extern int MyModuleVersion;' > %t.dst/folder-with-modulemap/MyModule.h
+// RUN: echo '@import MyModule;' > %t.dst/folder-with-modulemap/included.h
+// RUN: echo 'module MyModule { header "MyModule.h" }' > %t.dst/folder-with-modulemap/MyModule.modulemap
+
+// RUN: %clang_cc1 -emit-pch -o %t.dst/pch-folder/header.pch -fmodule-map-file=%t.dst/folder-with-modulemap/MyModule.modulemap -x objective-c-header -fmodules-cache-path=%t.cache -fmodules -fimplicit-module-maps %t.dst/header.h
+// RUN: %clang_cc1 -fsyntax-only -fmodule-map-file=%t.dst/folder-with-modulemap/MyModule.modulemap -fmodules-cache-path=%t.cache -fmodules -fimplicit-module-maps %s -include-pch %t.dst/pch-folder/header.pch -verify
+
+// expected-no-diagnostics
+
+void test(

[PATCH] D49540: [clangd] FuzzyMatch exposes an API for its word segmentation. NFC

[Eric Liu via Phabricator via cfe-commits]

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

lgtm


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49540



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

[PATCH] D49112: [Sema] Implement -Wmemset-transposed-args

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM aside from a minor nit.




Comment at: clang/lib/Sema/SemaChecking.cpp:8751
+
+  if (auto *BO = dyn_cast(SizeofExpr)) {
+if (BO->getOpcode() != BO_Mul && BO->getOpcode() != BO_Add)

`const auto *`


https://reviews.llvm.org/D49112



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

[PATCH] D49421: [CodeComplete] Fix accessibilty of protected members from base class.

[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:
  rC Clang

https://reviews.llvm.org/D49421



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

r337449 - [PowerPC] Handle __builtin_xxpermdi the same way as GCC does

[Nemanja Ivanovic via cfe-commits]

Author: nemanjai
Date: Thu Jul 19 05:44:15 2018
New Revision: 337449

URL: http://llvm.org/viewvc/llvm-project?rev=337449&view=rev
Log:
[PowerPC] Handle __builtin_xxpermdi the same way as GCC does

The codegen for this builtin was initially implemented to match GCC.
However, due to interest from users GCC changed behaviour to account for the
big endian bias of the instruction and correct it. This patch brings the
handling inline with GCC.

Fixes https://bugs.llvm.org/show_bug.cgi?id=38192

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

Modified:
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
cfe/trunk/test/CodeGen/builtins-ppc-vsx.c

Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=337449&r1=337448&r2=337449&view=diff
==
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Thu Jul 19 05:44:15 2018
@@ -10831,19 +10831,11 @@ Value *CodeGenFunction::EmitPPCBuiltinEx
 Ops[0] = Builder.CreateBitCast(Ops[0], llvm::VectorType::get(Int64Ty, 2));
 Ops[1] = Builder.CreateBitCast(Ops[1], llvm::VectorType::get(Int64Ty, 2));
 
-// Element zero comes from the first input vector and element one comes 
from
-// the second. The element indices within each vector are numbered in big
-// endian order so the shuffle mask must be adjusted for this on little
-// endian platforms (i.e. index is complemented and source vector 
reversed).
-unsigned ElemIdx0;
-unsigned ElemIdx1;
-if (getTarget().isLittleEndian()) {
-  ElemIdx0 = (~Index & 1) + 2;
-  ElemIdx1 = (~Index & 2) >> 1;
-} else { // BigEndian
-  ElemIdx0 = (Index & 2) >> 1;
-  ElemIdx1 = 2 + (Index & 1);
-}
+// Account for endianness by treating this as just a shuffle. So we use the
+// same indices for both LE and BE in order to produce expected results in
+// both cases.
+unsigned ElemIdx0 = (Index & 2) >> 1;;
+unsigned ElemIdx1 = 2 + (Index & 1);;
 
 Constant *ShuffleElts[2] = {ConstantInt::get(Int32Ty, ElemIdx0),
 ConstantInt::get(Int32Ty, ElemIdx1)};

Modified: cfe/trunk/test/CodeGen/builtins-ppc-vsx.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/builtins-ppc-vsx.c?rev=337449&r1=337448&r2=337449&view=diff
==
--- cfe/trunk/test/CodeGen/builtins-ppc-vsx.c (original)
+++ cfe/trunk/test/CodeGen/builtins-ppc-vsx.c Thu Jul 19 05:44:15 2018
@@ -1694,43 +1694,43 @@ vec_xst_be(vd, sll, ad);
 
 res_vd = vec_xxpermdi(vd, vd, 0);
 // CHECK: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
-// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
+// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
 
 res_vf = vec_xxpermdi(vf, vf, 1);
 // CHECK: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
-// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
+// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
 
 res_vsll = vec_xxpermdi(vsll, vsll, 2);
 // CHECK: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
-// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
+// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
 
 res_vull = vec_xxpermdi(vull, vull, 3);
 // CHECK: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
-// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
+// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
 
 res_vsi = vec_xxpermdi(vsi, vsi, 0);
 // CHECK: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
-// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
+// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
 
 res_vui = vec_xxpermdi(vui, vui, 1);
 // CHECK: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
-// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
+// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
 
 res_vss = vec_xxpermdi(vss, vss, 2);
 // CHECK: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
-// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
+// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
 
 res_vus = vec_xxpermdi(vus, vus, 3);
 // CHECK: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
-// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
+// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 

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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno accepted this revision.
bruno added a comment.

Thanks for working on this.

LGTM with improvements in the comments as mentioned by @jkorous


https://reviews.llvm.org/D48786



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

[PATCH] D49424: [PowerPC] Handle __builtin_xxpermdi the same way as GCC does

[Nemanja Ivanovic via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC337449: [PowerPC] Handle __builtin_xxpermdi the same way as 
GCC does (authored by nemanjai, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D49424?vs=155869&id=156251#toc

Repository:
  rC Clang

https://reviews.llvm.org/D49424

Files:
  lib/CodeGen/CGBuiltin.cpp
  test/CodeGen/builtins-ppc-vsx.c


Index: test/CodeGen/builtins-ppc-vsx.c
===
--- test/CodeGen/builtins-ppc-vsx.c
+++ test/CodeGen/builtins-ppc-vsx.c
@@ -1694,43 +1694,43 @@
 
 res_vd = vec_xxpermdi(vd, vd, 0);
 // CHECK: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
-// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
+// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
 
 res_vf = vec_xxpermdi(vf, vf, 1);
 // CHECK: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
-// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
+// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
 
 res_vsll = vec_xxpermdi(vsll, vsll, 2);
 // CHECK: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
-// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
+// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
 
 res_vull = vec_xxpermdi(vull, vull, 3);
 // CHECK: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
-// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
+// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
 
 res_vsi = vec_xxpermdi(vsi, vsi, 0);
 // CHECK: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
-// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
+// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
 
 res_vui = vec_xxpermdi(vui, vui, 1);
 // CHECK: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
-// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
+// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
 
 res_vss = vec_xxpermdi(vss, vss, 2);
 // CHECK: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
-// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
+// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
 
 res_vus = vec_xxpermdi(vus, vus, 3);
 // CHECK: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
-// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
+// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
 
 res_vsc = vec_xxpermdi(vsc, vsc, 0);
 // CHECK: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
-// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
+// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
 
 res_vuc = vec_xxpermdi(vuc, vuc, 1);
 // CHECK: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
-// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
+// CHECK-LE: shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, <2 x 
i32> 
 
 res_vd = vec_xxsldwi(vd, vd, 0);
 // CHECK: shufflevector <4 x i32> %{{[0-9]+}}, <4 x i32> %{{[0-9]+}}, <4 x 
i32> 
@@ -1786,7 +1786,7 @@
 
 // CHECK-LE:  bitcast <4 x i32> %{{[0-9]+}} to <2 x i64>
 // CHECK-LE-NEXT:  bitcast <4 x i32> %{{[0-9]+}} to <2 x i64>
-// CHECK-LE-NEXT:  shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, 
<2 x i32> 
+// CHECK-LE-NEXT:  shufflevector <2 x i64> %{{[0-9]+}}, <2 x i64> %{{[0-9]+}}, 
<2 x i32> 
 // CHECK-LE-NEXT:  bitcast <2 x i64> %{{[0-9]+}} to <4 x i32>
 }
 
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -10831,19 +10831,11 @@
 Ops[0] = Builder.CreateBitCast(Ops[0], llvm::VectorType::get(Int64Ty, 2));
 Ops[1] = Builder.CreateBitCast(Ops[1], llvm::VectorType::get(Int64Ty, 2));
 
-// Element zero comes from the first input vector and element one comes 
from
-// the second. The element indices within each vector are numbered in big
-// endian order so the shuffle mask must be adjusted for this on little
-// endian platforms (i.e. index is complemented and source vector 
reversed).
-unsigned ElemIdx0;
-unsigned ElemIdx1;
-if (getTarget().isLittleEndian()) {
-  ElemIdx0 = (~Index & 1) + 2;
-  ElemIdx1 = (~Index & 2) >> 1;
-} else { // BigEndian
-  ElemIdx0 = (Index & 2) >> 1;
-  ElemIdx1 = 2

r337451 - NFC: Remove extraneous semicolons as pointed out in the differential review

[Nemanja Ivanovic via cfe-commits]

Author: nemanjai
Date: Thu Jul 19 05:49:27 2018
New Revision: 337451

URL: http://llvm.org/viewvc/llvm-project?rev=337451&view=rev
Log:
NFC: Remove extraneous semicolons as pointed out in the differential review

The commit for
https://reviews.llvm.org/D49424
missed the comment about the extraneous semicolons. Remove them.

Modified:
cfe/trunk/lib/CodeGen/CGBuiltin.cpp

Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=337451&r1=337450&r2=337451&view=diff
==
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Thu Jul 19 05:49:27 2018
@@ -10834,8 +10834,8 @@ Value *CodeGenFunction::EmitPPCBuiltinEx
 // Account for endianness by treating this as just a shuffle. So we use the
 // same indices for both LE and BE in order to produce expected results in
 // both cases.
-unsigned ElemIdx0 = (Index & 2) >> 1;;
-unsigned ElemIdx1 = 2 + (Index & 1);;
+unsigned ElemIdx0 = (Index & 2) >> 1;
+unsigned ElemIdx1 = 2 + (Index & 1);
 
 Constant *ShuffleElts[2] = {ConstantInt::get(Int32Ty, ElemIdx0),
 ConstantInt::get(Int32Ty, ElemIdx1)};


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

[PATCH] D49504: Enable C++2a Chrono Literals

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/Lex/LiteralSupport.cpp:815
   .Cases("il", "i", "if", true)
+  .Cases("d", "y", LangOpts.CPlusPlus2a)
   .Default(false);

Is it possible for the library to expose those outside of C++2a mode? We pass 
`true` for the C++14 cases, so I'm wondering about tying it to C++2a explicitly.



Comment at: test/SemaCXX/cxx2a-user-defined-literals.cpp:1
+// RUN: %clang_cc1 -std=c++2a %s -include %s -verify
+

Can you drop the svn properties, please?



Comment at: test/SemaCXX/cxx2a-user-defined-literals.cpp:12
+  }
+  using size_t = decltype(sizeof(0));
+  constexpr chrono::day operator"" d(unsigned long long d) noexcept;

Is this needed?


https://reviews.llvm.org/D49504



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

[PATCH] D45444: [clang-tidy] implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Yes, but that is complicated. Functionality for that will first be
implemented in the libTooling and then utilized here later.

Am 19.07.2018 um 04:16 schrieb Florin Iucha via Phabricator:

> 0x8000- added a comment.
> 
> Any plans for fix-its that will add the suggested 'const' qualifiers?
> 
> Repository:
> 
>   rCTE Clang Tools Extra
> 
> https://reviews.llvm.org/D45444


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D49504: Enable C++2a Chrono Literals

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: lib/Lex/LiteralSupport.cpp:815
   .Cases("il", "i", "if", true)
+  .Cases("d", "y", LangOpts.CPlusPlus2a)
   .Default(false);

aaron.ballman wrote:
> Is it possible for the library to expose those outside of C++2a mode? We pass 
> `true` for the C++14 cases, so I'm wondering about tying it to C++2a 
> explicitly.
In the C++14 cases, we return true because we've checked C++14 mode on line 
805.  I was hoping to do that for consistencies sake. Depending on your 
opinion, bringing 805 down to 812/813/814 (checking inline) might be a good 
idea.

According to the tests, 'sv' (string view) was the only one for C++17, but it 
wasn't really necessary to check here since it is only allowed on strings and 
not on integer literals.



Comment at: test/SemaCXX/cxx2a-user-defined-literals.cpp:1
+// RUN: %clang_cc1 -std=c++2a %s -include %s -verify
+

aaron.ballman wrote:
> Can you drop the svn properties, please?
Yep, will do :)  



Comment at: test/SemaCXX/cxx2a-user-defined-literals.cpp:12
+  }
+  using size_t = decltype(sizeof(0));
+  constexpr chrono::day operator"" d(unsigned long long d) noexcept;

aaron.ballman wrote:
> Is this needed?
It is, in fact, not required.


https://reviews.llvm.org/D49504



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

[PATCH] D49510: [OpenMP][Clang] Usability improvements for OpenMP offloading

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

1. The new tool definitely requires an RFC at first.
2. The patch is too big and should be split into several small patches


Repository:
  rC Clang

https://reviews.llvm.org/D49510



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

[PATCH] D49466: Initial implementation of -fmacro-prefix-mapand -ffile-prefix-map

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: include/clang/Basic/DiagnosticDriverKinds.td:118
   "invalid deployment target for -stdlib=libc++ (requires %0 or later)">;
+def err_drv_invalid_argument_to_fmacro_prefix_map : Error<
+  "invalid argument '%0' to -fmacro-prefix-map">;

Since these are otherwise identical, perhaps a %select{...|...} for the flag 
name?



Comment at: include/clang/Lex/PreprocessorOptions.h:171
+  /// A prefix map for __FILE__ and __BASEFILE__
+  std::map MacroPrefixMap;
+

erichkeane wrote:
> It seems this can be StringRefs as well.
Did you miss this one?  Or is there a good reason these cannot be stringrefs?



Comment at: lib/Driver/ToolChains/Clang.cpp:616
+}
 else
   CmdArgs.push_back(Args.MakeArgString("-fdebug-prefix-map=" + Map));

With the continue above, 'else' is unnecessary/against coding standard.



Comment at: lib/Driver/ToolChains/Clang.cpp:631
+}
+else
+  CmdArgs.push_back(Args.MakeArgString("-fmacro-prefix-map=" + Map));

See above.



Comment at: lib/Lex/PPMacroExpansion.cpp:1457
 
+static std::string remapMacroPath(StringRef Path,
+   const std::mapMacroPrefixMap);

First, comments end in a period.  Second, isn't that what the next line does?



Comment at: lib/Lex/PPMacroExpansion.cpp:1528
 // Escape this filename.  Turn '\' -> '\\' '"' -> '\"'
-SmallString<128> FN;
+std::string FN;
 if (PLoc.isValid()) {

dankm wrote:
> erichkeane wrote:
> > This change shouldn't be necessary, SmallString is still likely the right 
> > answer here.
> I tried that long ago. It didn't work, I don't remember exactly why. But I 
> agree that SmallString should be enough. I'll dig more.
Just noting to handle this before approval.


Repository:
  rC Clang

https://reviews.llvm.org/D49466



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

[PATCH] D49504: Enable C++2a Chrono Literals

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 156252.
erichkeane marked 4 inline comments as done.
erichkeane added a comment.

@aaron.ballman s comments.


https://reviews.llvm.org/D49504

Files:
  lib/Lex/LiteralSupport.cpp
  test/SemaCXX/cxx2a-user-defined-literals.cpp


Index: lib/Lex/LiteralSupport.cpp
===
--- lib/Lex/LiteralSupport.cpp
+++ lib/Lex/LiteralSupport.cpp
@@ -751,7 +751,8 @@
 
   // If we have a hex digit other than 'e' (which denotes a FP exponent) then
   // the code is using an incorrect base.
-  if (isHexDigit(*s) && *s != 'e' && *s != 'E') {
+  if (isHexDigit(*s) && *s != 'e' && *s != 'E' &&
+  !isValidUDSuffix(PP.getLangOpts(), StringRef(s, ThisTokEnd - s))) {
 PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, s-ThisTokBegin),
 diag::err_invalid_digit) << StringRef(s, 1) << (radix == 8 ? 1 : 
0);
 hadError = true;
@@ -804,12 +805,14 @@
   if (!LangOpts.CPlusPlus14)
 return false;
 
-  // In C++1y, "s", "h", "min", "ms", "us", and "ns" are used in the library.
+  // In C++14, "s", "h", "min", "ms", "us", and "ns" are used in the library.
   // Per tweaked N3660, "il", "i", and "if" are also used in the library.
+  // In C++2a "d" and "y" are used in the library.
   return llvm::StringSwitch(Suffix)
   .Cases("h", "min", "s", true)
   .Cases("ms", "us", "ns", true)
   .Cases("il", "i", "if", true)
+  .Cases("d", "y", LangOpts.CPlusPlus2a)
   .Default(false);
 }
 
@@ -922,7 +925,9 @@
 s = SkipBinaryDigits(s);
 if (s == ThisTokEnd) {
   // Done.
-} else if (isHexDigit(*s)) {
+} else if (isHexDigit(*s) &&
+   !isValidUDSuffix(PP.getLangOpts(),
+StringRef(s, ThisTokEnd - s))) {
   PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, s-ThisTokBegin),
   diag::err_invalid_digit) << StringRef(s, 1) << 2;
   hadError = true;
Index: test/SemaCXX/cxx2a-user-defined-literals.cpp
===
--- test/SemaCXX/cxx2a-user-defined-literals.cpp
+++ test/SemaCXX/cxx2a-user-defined-literals.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -std=c++2a %s -include %s -verify
+
+#ifndef INCLUDED
+#define INCLUDED
+
+#pragma clang system_header
+namespace std {
+  namespace chrono {
+struct day{};
+struct year{};
+  }
+  constexpr chrono::day operator"" d(unsigned long long d) noexcept;
+  constexpr chrono::year operator"" y(unsigned long long y) noexcept;
+}
+
+#else
+
+using namespace std;
+chrono::day dec_d = 5d;
+chrono::day oct_d = 05d;
+chrono::day bin_d = 0b011d;
+// expected-error@+3{{no viable conversion from 'int' to 'chrono::day'}}
+// expected-note@9{{candidate constructor (the implicit copy constructor)}}
+// expected-note@9{{candidate constructor (the implicit move constructor)}}
+chrono::day hex_d = 0x44d;
+chrono::year y  = 10y;
+#endif


Index: lib/Lex/LiteralSupport.cpp
===
--- lib/Lex/LiteralSupport.cpp
+++ lib/Lex/LiteralSupport.cpp
@@ -751,7 +751,8 @@
 
   // If we have a hex digit other than 'e' (which denotes a FP exponent) then
   // the code is using an incorrect base.
-  if (isHexDigit(*s) && *s != 'e' && *s != 'E') {
+  if (isHexDigit(*s) && *s != 'e' && *s != 'E' &&
+  !isValidUDSuffix(PP.getLangOpts(), StringRef(s, ThisTokEnd - s))) {
 PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, s-ThisTokBegin),
 diag::err_invalid_digit) << StringRef(s, 1) << (radix == 8 ? 1 : 0);
 hadError = true;
@@ -804,12 +805,14 @@
   if (!LangOpts.CPlusPlus14)
 return false;
 
-  // In C++1y, "s", "h", "min", "ms", "us", and "ns" are used in the library.
+  // In C++14, "s", "h", "min", "ms", "us", and "ns" are used in the library.
   // Per tweaked N3660, "il", "i", and "if" are also used in the library.
+  // In C++2a "d" and "y" are used in the library.
   return llvm::StringSwitch(Suffix)
   .Cases("h", "min", "s", true)
   .Cases("ms", "us", "ns", true)
   .Cases("il", "i", "if", true)
+  .Cases("d", "y", LangOpts.CPlusPlus2a)
   .Default(false);
 }
 
@@ -922,7 +925,9 @@
 s = SkipBinaryDigits(s);
 if (s == ThisTokEnd) {
   // Done.
-} else if (isHexDigit(*s)) {
+} else if (isHexDigit(*s) &&
+   !isValidUDSuffix(PP.getLangOpts(),
+StringRef(s, ThisTokEnd - s))) {
   PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, s-ThisTokBegin),
   diag::err_invalid_digit) << StringRef(s, 1) << 2;
   hadError = true;
Index: test/SemaCXX/cxx2a-user-defined-literals.cpp
===
--- test/SemaCXX/cxx2a-user-defined-literals.cpp
+++ test/SemaCXX/cxx2a-user-defined-literals.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -std=c++2a %s -include %s -verify
+
+#ifndef INCLUDED
+#define INCLUDED
+
+#pragma clang system_header
+namespace std {
+  namespace chrono {
+

[PATCH] D49504: Enable C++2a Chrono Literals

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: test/SemaCXX/cxx2a-user-defined-literals.cpp:23
+// expected-error@+3{{no viable conversion from 'int' to 'chrono::day'}}
+// expected-note@9{{candidate constructor (the implicit copy constructor)}}
+// expected-note@9{{candidate constructor (the implicit move constructor)}}

Of course, the '9' in these two lines is now wrong... I'll fix it in my local 
copy.


https://reviews.llvm.org/D49504



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

[PATCH] D49504: Enable C++2a Chrono Literals

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: test/SemaCXX/cxx2a-user-defined-literals.cpp:23
+// expected-error@+3{{no viable conversion from 'int' to 'chrono::day'}}
+// expected-note@9{{candidate constructor (the implicit copy constructor)}}
+// expected-note@9{{candidate constructor (the implicit move constructor)}}

erichkeane wrote:
> Of course, the '9' in these two lines is now wrong... I'll fix it in my local 
> copy.
Actually not, since it was above the removed line... I need to stop working 
before my first caffeine infusion...


https://reviews.llvm.org/D49504



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

[PATCH] D49543: [clangd] Penalize non-instance members when accessed via class instances.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added a reviewer: sammccall.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, 
ilya-biryukov.

The following are metrics for explicit member access completions. There is no
noticeable impact on other completion types.

Before:
EXPLICIT_MEMBER_ACCESS

  Total measurements: 24382
  All measurements: MRR: 62.27  Top10: 80.21%   Top-100: 94.48%
  Full identifiers: MRR: 98.81  Top10: 99.89%   Top-100: 99.95%
  0-5 filter len:

MRR:  13.25 46.31   62.47   67.77   70.40   81.91
Top-10:  29%74% 84% 91% 91% 97%
Top-100:  67%   99% 99% 99% 99% 100%

After:
EXPLICIT_MEMBER_ACCESS

  Total measurements: 24382
  All measurements: MRR: 63.18  Top10: 80.58%   Top-100: 95.07%
  Full identifiers: MRR: 98.79  Top10: 99.89%   Top-100: 99.95%
  0-5 filter len:

MRR:  13.84 48.39   63.55   68.83   71.28   82.64
Top-10:  30%75% 84% 91% 91% 97%
Top-100:  70%   99% 99% 99% 99% 100%

- Top-N: wanted result is found in the first N completion results.
- MRR: Mean reciprocal rank.

Remark: the change seems to have minor positive impact. Although the improvement
is relatively small, down-ranking non-instance members in instance member access
should reduce noise in the completion results.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49543

Files:
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/Quality.cpp
  clangd/Quality.h
  unittests/clangd/QualityTests.cpp

Index: unittests/clangd/QualityTests.cpp
===
--- unittests/clangd/QualityTests.cpp
+++ unittests/clangd/QualityTests.cpp
@@ -23,6 +23,7 @@
 #include "TestTU.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/Support/Casting.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -227,6 +228,14 @@
   EXPECT_EQ(Scoped.evaluate(), Default.evaluate());
   Scoped.Query = SymbolRelevanceSignals::CodeComplete;
   EXPECT_GT(Scoped.evaluate(), Default.evaluate());
+
+  SymbolRelevanceSignals Instance;
+  Instance.IsInstanceMember = false;
+  EXPECT_EQ(Instance.evaluate(), Default.evaluate());
+  Instance.SemaContext = CodeCompletionContext::CCC_DotMemberAccess;
+  EXPECT_LT(Instance.evaluate(), Default.evaluate());
+  Instance.IsInstanceMember = true;
+  EXPECT_EQ(Instance.evaluate(), Default.evaluate());
 }
 
 TEST(QualityTests, SortText) {
@@ -267,6 +276,35 @@
   EXPECT_EQ(Ctor.Scope, SymbolRelevanceSignals::GlobalScope);
 }
 
+TEST(QualityTests, IsInstanceMember) {
+  auto Header = TestTU::withHeaderCode(R"cpp(
+class Foo {
+public:
+  static void foo() {}
+  void bar() {}
+};
+  )cpp");
+  auto Symbols = Header.headerSymbols();
+
+  SymbolRelevanceSignals Rel;
+  const Symbol &FooSym = findSymbol(Symbols, "Foo::foo");
+  Rel.merge(FooSym);
+  EXPECT_FALSE(Rel.IsInstanceMember);
+  const Symbol &BarSym = findSymbol(Symbols, "Foo::bar");
+  Rel.merge(BarSym);
+  EXPECT_TRUE(Rel.IsInstanceMember);
+
+  auto AST = Header.build();
+  const NamedDecl *Foo = &findDecl(AST, "Foo::foo");
+  const NamedDecl *Bar = &findDecl(AST, "Foo::bar");
+
+  Rel.IsInstanceMember = false;
+  Rel.merge(CodeCompletionResult(Foo, /*Priority=*/0));
+  EXPECT_FALSE(Rel.IsInstanceMember);
+  Rel.merge(CodeCompletionResult(Bar, /*Priority=*/0));
+  EXPECT_TRUE(Rel.IsInstanceMember);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/Quality.h
===
--- clangd/Quality.h
+++ clangd/Quality.h
@@ -26,6 +26,7 @@
 //===-===//
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_QUALITY_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_QUALITY_H
+#include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include 
@@ -98,6 +99,11 @@
 Generic,
   } Query = Generic;
 
+  CodeCompletionContext::Kind SemaContext = CodeCompletionContext::CCC_Other;
+
+  // Whether symbol is an instance member of a class.
+  bool IsInstanceMember = false;
+
   void merge(const CodeCompletionResult &SemaResult);
   void merge(const Symbol &IndexResult);
 
Index: clangd/Quality.cpp
===
--- clangd/Quality.cpp
+++ clangd/Quality.cpp
@@ -11,6 +11,7 @@
 #include "URI.h"
 #include "index/Index.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclVisitor.h"
 #include "clang/Basic/CharInfo.h"
@@ -139,6 +140,25 @@
   llvm_unreachable("Unknown index::SymbolKind");
 }
 
+static bool isInstanceMember(const NamedDecl *ND) {
+  if (!ND)
+return false;
+  if (const auto *CM = dyn_cast(ND))
+return !CM->isStatic();
+  return isa(ND); // Note that static fields are VarDecl.
+}
+

r337453 - [CodeComplete] Fix accessibilty of protected members from base class.

[Eric Liu via cfe-commits]

Author: ioeric
Date: Thu Jul 19 06:32:00 2018
New Revision: 337453

URL: http://llvm.org/viewvc/llvm-project?rev=337453&view=rev
Log:
[CodeComplete] Fix accessibilty of protected members from base class.

Summary:
Currently, protected members from base classes are marked as
inaccessible when completing in derived class. This patch fixes the problem by
setting the naming class correctly when looking up results in base class
according to [11.2.p5].

Reviewers: aaron.ballman, sammccall, rsmith

Reviewed By: aaron.ballman

Subscribers: cfe-commits

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

Modified:
cfe/trunk/lib/Sema/SemaAccess.cpp
cfe/trunk/lib/Sema/SemaCodeComplete.cpp
cfe/trunk/test/Index/complete-access-checks.cpp

Modified: cfe/trunk/lib/Sema/SemaAccess.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaAccess.cpp?rev=337453&r1=337452&r2=337453&view=diff
==
--- cfe/trunk/lib/Sema/SemaAccess.cpp (original)
+++ cfe/trunk/lib/Sema/SemaAccess.cpp Thu Jul 19 06:32:00 2018
@@ -11,6 +11,7 @@
 //
 
//===--===//
 
+#include "clang/Basic/Specifiers.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/CXXInheritance.h"
@@ -1856,29 +1857,31 @@ void Sema::CheckLookupAccess(const Looku
   }
 }
 
-/// Checks access to Decl from the given class. The check will take access
+/// Checks access to Target from the given class. The check will take access
 /// specifiers into account, but no member access expressions and such.
 ///
-/// \param Decl the declaration to check if it can be accessed
+/// \param Target the declaration to check if it can be accessed
 /// \param Ctx the class/context from which to start the search
-/// \return true if the Decl is accessible from the Class, false otherwise.
-bool Sema::IsSimplyAccessible(NamedDecl *Decl, DeclContext *Ctx) {
+/// \return true if the Target is accessible from the Class, false otherwise.
+bool Sema::IsSimplyAccessible(NamedDecl *Target, DeclContext *Ctx) {
   if (CXXRecordDecl *Class = dyn_cast(Ctx)) {
-if (!Decl->isCXXClassMember())
+if (!Target->isCXXClassMember())
   return true;
 
+if (Target->getAccess() == AS_public)
+  return true;
 QualType qType = Class->getTypeForDecl()->getCanonicalTypeInternal();
+// The unprivileged access is AS_none as we don't know how the member was
+// accessed, which is described by the access in DeclAccessPair.
+// `IsAccessible` will examine the actual access of Target (i.e.
+// Decl->getAccess()) when calculating the access.
 AccessTarget Entity(Context, AccessedEntity::Member, Class,
-DeclAccessPair::make(Decl, Decl->getAccess()),
-qType);
-if (Entity.getAccess() == AS_public)
-  return true;
-
+DeclAccessPair::make(Target, AS_none), qType);
 EffectiveContext EC(CurContext);
 return ::IsAccessible(*this, EC, Entity) != ::AR_inaccessible;
   }
-  
-  if (ObjCIvarDecl *Ivar = dyn_cast(Decl)) {
+
+  if (ObjCIvarDecl *Ivar = dyn_cast(Target)) {
 // @public and @package ivars are always accessible.
 if (Ivar->getCanonicalAccessControl() == ObjCIvarDecl::Public ||
 Ivar->getCanonicalAccessControl() == ObjCIvarDecl::Package)

Modified: cfe/trunk/lib/Sema/SemaCodeComplete.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCodeComplete.cpp?rev=337453&r1=337452&r2=337453&view=diff
==
--- cfe/trunk/lib/Sema/SemaCodeComplete.cpp (original)
+++ cfe/trunk/lib/Sema/SemaCodeComplete.cpp Thu Jul 19 06:32:00 2018
@@ -1303,8 +1303,33 @@ namespace {
 void FoundDecl(NamedDecl *ND, NamedDecl *Hiding, DeclContext *Ctx,
bool InBaseClass) override {
   bool Accessible = true;
-  if (Ctx)
-Accessible = Results.getSema().IsSimplyAccessible(ND, Ctx);
+  if (Ctx) {
+DeclContext *AccessingCtx = Ctx;
+// If ND comes from a base class, set the naming class back to the
+// derived class if the search starts from the derived class (i.e.
+// InBaseClass is true).
+//
+// Example:
+//   class B { protected: int X; }
+//   class D : public B { void f(); }
+//   void D::f() { this->^; }
+// The completion after "this->" will have `InBaseClass` set to true 
and
+// `Ctx` set to "B", when looking up in `B`. We need to set the actual
+// accessing context (i.e. naming class) to "D" so that access can be
+// calculated correctly.
+if (InBaseClass && isa(Ctx)) {
+  CXXRecordDecl *RC = nullptr;
+  // Get the enclosing record.
+  for (DeclContext *DC = CurContext; !DC->isFileContext();
+   DC = DC->getParent()) {
+  

[PATCH] D49421: [CodeComplete] Fix accessibilty of protected members from base class.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL337453: [CodeComplete] Fix accessibilty of protected members 
from base class. (authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D49421

Files:
  cfe/trunk/lib/Sema/SemaAccess.cpp
  cfe/trunk/lib/Sema/SemaCodeComplete.cpp
  cfe/trunk/test/Index/complete-access-checks.cpp

Index: cfe/trunk/lib/Sema/SemaAccess.cpp
===
--- cfe/trunk/lib/Sema/SemaAccess.cpp
+++ cfe/trunk/lib/Sema/SemaAccess.cpp
@@ -11,6 +11,7 @@
 //
 //===--===//
 
+#include "clang/Basic/Specifiers.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/CXXInheritance.h"
@@ -1856,29 +1857,31 @@
   }
 }
 
-/// Checks access to Decl from the given class. The check will take access
+/// Checks access to Target from the given class. The check will take access
 /// specifiers into account, but no member access expressions and such.
 ///
-/// \param Decl the declaration to check if it can be accessed
+/// \param Target the declaration to check if it can be accessed
 /// \param Ctx the class/context from which to start the search
-/// \return true if the Decl is accessible from the Class, false otherwise.
-bool Sema::IsSimplyAccessible(NamedDecl *Decl, DeclContext *Ctx) {
+/// \return true if the Target is accessible from the Class, false otherwise.
+bool Sema::IsSimplyAccessible(NamedDecl *Target, DeclContext *Ctx) {
   if (CXXRecordDecl *Class = dyn_cast(Ctx)) {
-if (!Decl->isCXXClassMember())
+if (!Target->isCXXClassMember())
   return true;
 
+if (Target->getAccess() == AS_public)
+  return true;
 QualType qType = Class->getTypeForDecl()->getCanonicalTypeInternal();
+// The unprivileged access is AS_none as we don't know how the member was
+// accessed, which is described by the access in DeclAccessPair.
+// `IsAccessible` will examine the actual access of Target (i.e.
+// Decl->getAccess()) when calculating the access.
 AccessTarget Entity(Context, AccessedEntity::Member, Class,
-DeclAccessPair::make(Decl, Decl->getAccess()),
-qType);
-if (Entity.getAccess() == AS_public)
-  return true;
-
+DeclAccessPair::make(Target, AS_none), qType);
 EffectiveContext EC(CurContext);
 return ::IsAccessible(*this, EC, Entity) != ::AR_inaccessible;
   }
-  
-  if (ObjCIvarDecl *Ivar = dyn_cast(Decl)) {
+
+  if (ObjCIvarDecl *Ivar = dyn_cast(Target)) {
 // @public and @package ivars are always accessible.
 if (Ivar->getCanonicalAccessControl() == ObjCIvarDecl::Public ||
 Ivar->getCanonicalAccessControl() == ObjCIvarDecl::Package)
Index: cfe/trunk/lib/Sema/SemaCodeComplete.cpp
===
--- cfe/trunk/lib/Sema/SemaCodeComplete.cpp
+++ cfe/trunk/lib/Sema/SemaCodeComplete.cpp
@@ -1303,8 +1303,33 @@
 void FoundDecl(NamedDecl *ND, NamedDecl *Hiding, DeclContext *Ctx,
bool InBaseClass) override {
   bool Accessible = true;
-  if (Ctx)
-Accessible = Results.getSema().IsSimplyAccessible(ND, Ctx);
+  if (Ctx) {
+DeclContext *AccessingCtx = Ctx;
+// If ND comes from a base class, set the naming class back to the
+// derived class if the search starts from the derived class (i.e.
+// InBaseClass is true).
+//
+// Example:
+//   class B { protected: int X; }
+//   class D : public B { void f(); }
+//   void D::f() { this->^; }
+// The completion after "this->" will have `InBaseClass` set to true and
+// `Ctx` set to "B", when looking up in `B`. We need to set the actual
+// accessing context (i.e. naming class) to "D" so that access can be
+// calculated correctly.
+if (InBaseClass && isa(Ctx)) {
+  CXXRecordDecl *RC = nullptr;
+  // Get the enclosing record.
+  for (DeclContext *DC = CurContext; !DC->isFileContext();
+   DC = DC->getParent()) {
+if ((RC = dyn_cast(DC)))
+  break;
+  }
+  if (RC)
+AccessingCtx = RC;
+}
+Accessible = Results.getSema().IsSimplyAccessible(ND, AccessingCtx);
+  }
 
   ResultBuilder::Result Result(ND, Results.getBasePriority(ND), nullptr,
false, Accessible, FixIts);
Index: cfe/trunk/test/Index/complete-access-checks.cpp
===
--- cfe/trunk/test/Index/complete-access-checks.cpp
+++ cfe/trunk/test/Index/complete-access-checks.cpp
@@ -36,10 +36,10 @@
 
 // CHECK-SUPER-ACCESS: CXXMethod:{ResultType void}{TypedText doSomething}{LeftParen (}{RightParen )}

[PATCH] D49421: [CodeComplete] Fix accessibilty of protected members from base class.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC337453: [CodeComplete] Fix accessibilty of protected members 
from base class. (authored by ioeric, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D49421?vs=156146&id=156257#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D49421

Files:
  lib/Sema/SemaAccess.cpp
  lib/Sema/SemaCodeComplete.cpp
  test/Index/complete-access-checks.cpp

Index: test/Index/complete-access-checks.cpp
===
--- test/Index/complete-access-checks.cpp
+++ test/Index/complete-access-checks.cpp
@@ -36,10 +36,10 @@
 
 // CHECK-SUPER-ACCESS: CXXMethod:{ResultType void}{TypedText doSomething}{LeftParen (}{RightParen )} (34)
 // CHECK-SUPER-ACCESS: CXXMethod:{ResultType void}{Informative X::}{TypedText func1}{LeftParen (}{RightParen )} (36)
-// CHECK-SUPER-ACCESS: CXXMethod:{ResultType void}{Informative X::}{TypedText func2}{LeftParen (}{RightParen )} (36) (inaccessible)
+// CHECK-SUPER-ACCESS: CXXMethod:{ResultType void}{Informative X::}{TypedText func2}{LeftParen (}{RightParen )} (36)
 // CHECK-SUPER-ACCESS: CXXMethod:{ResultType void}{Informative X::}{TypedText func3}{LeftParen (}{RightParen )} (36) (inaccessible)
 // CHECK-SUPER-ACCESS: FieldDecl:{ResultType int}{Informative X::}{TypedText member1} (37)
-// CHECK-SUPER-ACCESS: FieldDecl:{ResultType int}{Informative X::}{TypedText member2} (37) (inaccessible)
+// CHECK-SUPER-ACCESS: FieldDecl:{ResultType int}{Informative X::}{TypedText member2} (37)
 // CHECK-SUPER-ACCESS: FieldDecl:{ResultType int}{Informative X::}{TypedText member3} (37) (inaccessible)
 // CHECK-SUPER-ACCESS: CXXMethod:{ResultType Y &}{TypedText operator=}{LeftParen (}{Placeholder const Y &}{RightParen )} (79)
 // CHECK-SUPER-ACCESS: CXXMethod:{ResultType X &}{Text X::}{TypedText operator=}{LeftParen (}{Placeholder const X &}{RightParen )} (81)
@@ -87,3 +87,26 @@
 // CHECK-USING-ACCESSIBLE: ClassDecl:{TypedText Q}{Text ::} (75)
 // CHECK-USING-ACCESSIBLE: CXXDestructor:{ResultType void}{Informative P::}{TypedText ~P}{LeftParen (}{RightParen )} (81)
 // CHECK-USING-ACCESSIBLE: CXXDestructor:{ResultType void}{TypedText ~Q}{LeftParen (}{RightParen )} (79)
+
+class B {
+protected:
+  int member;
+};
+
+class C : private B {};
+
+
+class D : public C {
+ public:
+  void f(::B *b);
+};
+
+void D::f(::B *that) {
+  // RUN: c-index-test -code-completion-at=%s:106:9 %s | FileCheck -check-prefix=CHECK-PRIVATE-SUPER-THIS %s
+  this->;
+// CHECK-PRIVATE-SUPER-THIS: FieldDecl:{ResultType int}{Informative B::}{TypedText member} (37) (inaccessible)
+
+  // RUN: c-index-test -code-completion-at=%s:110:9 %s | FileCheck -check-prefix=CHECK-PRIVATE-SUPER-THAT %s
+  that->;
+// CHECK-PRIVATE-SUPER-THAT: FieldDecl:{ResultType int}{TypedText member} (35) (inaccessible)
+}
Index: lib/Sema/SemaAccess.cpp
===
--- lib/Sema/SemaAccess.cpp
+++ lib/Sema/SemaAccess.cpp
@@ -11,6 +11,7 @@
 //
 //===--===//
 
+#include "clang/Basic/Specifiers.h"
 #include "clang/Sema/SemaInternal.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/CXXInheritance.h"
@@ -1856,29 +1857,31 @@
   }
 }
 
-/// Checks access to Decl from the given class. The check will take access
+/// Checks access to Target from the given class. The check will take access
 /// specifiers into account, but no member access expressions and such.
 ///
-/// \param Decl the declaration to check if it can be accessed
+/// \param Target the declaration to check if it can be accessed
 /// \param Ctx the class/context from which to start the search
-/// \return true if the Decl is accessible from the Class, false otherwise.
-bool Sema::IsSimplyAccessible(NamedDecl *Decl, DeclContext *Ctx) {
+/// \return true if the Target is accessible from the Class, false otherwise.
+bool Sema::IsSimplyAccessible(NamedDecl *Target, DeclContext *Ctx) {
   if (CXXRecordDecl *Class = dyn_cast(Ctx)) {
-if (!Decl->isCXXClassMember())
+if (!Target->isCXXClassMember())
   return true;
 
+if (Target->getAccess() == AS_public)
+  return true;
 QualType qType = Class->getTypeForDecl()->getCanonicalTypeInternal();
+// The unprivileged access is AS_none as we don't know how the member was
+// accessed, which is described by the access in DeclAccessPair.
+// `IsAccessible` will examine the actual access of Target (i.e.
+// Decl->getAccess()) when calculating the access.
 AccessTarget Entity(Context, AccessedEntity::Member, Class,
-DeclAccessPair::make(Decl, Decl->getAccess()),
-qType);
-if (Entity.getAccess() == AS_public)
-  return true;
-
+DeclAccessPair::make(Target, AS_none), qType);
 EffectiveContext EC(CurContext);
 return ::IsAccessible(*this, EC, Entity) != ::AR_inaccessib

[PATCH] D49504: Enable C++2a Chrono Literals

[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!




Comment at: lib/Lex/LiteralSupport.cpp:815
   .Cases("il", "i", "if", true)
+  .Cases("d", "y", LangOpts.CPlusPlus2a)
   .Default(false);

erichkeane wrote:
> aaron.ballman wrote:
> > Is it possible for the library to expose those outside of C++2a mode? We 
> > pass `true` for the C++14 cases, so I'm wondering about tying it to C++2a 
> > explicitly.
> In the C++14 cases, we return true because we've checked C++14 mode on line 
> 805.  I was hoping to do that for consistencies sake. Depending on your 
> opinion, bringing 805 down to 812/813/814 (checking inline) might be a good 
> idea.
> 
> According to the tests, 'sv' (string view) was the only one for C++17, but it 
> wasn't really necessary to check here since it is only allowed on strings and 
> not on integer literals.
Ahh, I see what's going on now. Thank you, this looks fine to me.


https://reviews.llvm.org/D49504



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

r337454 - Enable C++2a Chrono Literals

[Erich Keane via cfe-commits]

Author: erichkeane
Date: Thu Jul 19 06:36:57 2018
New Revision: 337454

URL: http://llvm.org/viewvc/llvm-project?rev=337454&view=rev
Log:
Enable C++2a Chrono Literals

C++2a via http://wg21.link/p0355 permits the library
literals of 'd' and 'y'. This patch enables them in the
Lexer so that they can be properly parsed.

Note that 'd' gets confused with the hex character, so
modifications to how octal, binary, and decimal numbers are
parsed were required. Since this is simply making previously
invalid code legal, this should be fine.

Hex still greedily parses the 'd' as a hexit, since it would
a: violate [lex.ext]p1
b: break existing code.

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

Added:
cfe/trunk/test/SemaCXX/cxx2a-user-defined-literals.cpp
Modified:
cfe/trunk/lib/Lex/LiteralSupport.cpp

Modified: cfe/trunk/lib/Lex/LiteralSupport.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/LiteralSupport.cpp?rev=337454&r1=337453&r2=337454&view=diff
==
--- cfe/trunk/lib/Lex/LiteralSupport.cpp (original)
+++ cfe/trunk/lib/Lex/LiteralSupport.cpp Thu Jul 19 06:36:57 2018
@@ -751,7 +751,8 @@ void NumericLiteralParser::ParseDecimalO
 
   // If we have a hex digit other than 'e' (which denotes a FP exponent) then
   // the code is using an incorrect base.
-  if (isHexDigit(*s) && *s != 'e' && *s != 'E') {
+  if (isHexDigit(*s) && *s != 'e' && *s != 'E' &&
+  !isValidUDSuffix(PP.getLangOpts(), StringRef(s, ThisTokEnd - s))) {
 PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, s-ThisTokBegin),
 diag::err_invalid_digit) << StringRef(s, 1) << (radix == 8 ? 1 : 
0);
 hadError = true;
@@ -804,12 +805,14 @@ bool NumericLiteralParser::isValidUDSuff
   if (!LangOpts.CPlusPlus14)
 return false;
 
-  // In C++1y, "s", "h", "min", "ms", "us", and "ns" are used in the library.
+  // In C++14, "s", "h", "min", "ms", "us", and "ns" are used in the library.
   // Per tweaked N3660, "il", "i", and "if" are also used in the library.
+  // In C++2a "d" and "y" are used in the library.
   return llvm::StringSwitch(Suffix)
   .Cases("h", "min", "s", true)
   .Cases("ms", "us", "ns", true)
   .Cases("il", "i", "if", true)
+  .Cases("d", "y", LangOpts.CPlusPlus2a)
   .Default(false);
 }
 
@@ -922,7 +925,9 @@ void NumericLiteralParser::ParseNumberSt
 s = SkipBinaryDigits(s);
 if (s == ThisTokEnd) {
   // Done.
-} else if (isHexDigit(*s)) {
+} else if (isHexDigit(*s) &&
+   !isValidUDSuffix(PP.getLangOpts(),
+StringRef(s, ThisTokEnd - s))) {
   PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, s-ThisTokBegin),
   diag::err_invalid_digit) << StringRef(s, 1) << 2;
   hadError = true;

Added: cfe/trunk/test/SemaCXX/cxx2a-user-defined-literals.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/cxx2a-user-defined-literals.cpp?rev=337454&view=auto
==
--- cfe/trunk/test/SemaCXX/cxx2a-user-defined-literals.cpp (added)
+++ cfe/trunk/test/SemaCXX/cxx2a-user-defined-literals.cpp Thu Jul 19 06:36:57 
2018
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -std=c++2a %s -include %s -verify
+
+#ifndef INCLUDED
+#define INCLUDED
+
+#pragma clang system_header
+namespace std {
+  namespace chrono {
+struct day{};
+struct year{};
+  }
+  constexpr chrono::day operator"" d(unsigned long long d) noexcept;
+  constexpr chrono::year operator"" y(unsigned long long y) noexcept;
+}
+
+#else
+
+using namespace std;
+chrono::day dec_d = 5d;
+chrono::day oct_d = 05d;
+chrono::day bin_d = 0b011d;
+// expected-error@+3{{no viable conversion from 'int' to 'chrono::day'}}
+// expected-note@9{{candidate constructor (the implicit copy constructor)}}
+// expected-note@9{{candidate constructor (the implicit move constructor)}}
+chrono::day hex_d = 0x44d;
+chrono::year y  = 10y;
+#endif


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

[PATCH] D46190: For a used declaration, mark any associated usings as referenced.

[Carlos Alberto Enciso via Phabricator via cfe-commits]

CarlosAlbertoEnciso added a comment.

@probinson: I tried `clang-format-diff` and the main format issues are with the 
test cases.

Do you want the test cases to follow the clang format?


https://reviews.llvm.org/D46190



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

[PATCH] D49504: Enable C++2a Chrono Literals

[Erich Keane via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC337454: Enable C++2a Chrono Literals (authored by 
erichkeane, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D49504

Files:
  lib/Lex/LiteralSupport.cpp
  test/SemaCXX/cxx2a-user-defined-literals.cpp


Index: test/SemaCXX/cxx2a-user-defined-literals.cpp
===
--- test/SemaCXX/cxx2a-user-defined-literals.cpp
+++ test/SemaCXX/cxx2a-user-defined-literals.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -std=c++2a %s -include %s -verify
+
+#ifndef INCLUDED
+#define INCLUDED
+
+#pragma clang system_header
+namespace std {
+  namespace chrono {
+struct day{};
+struct year{};
+  }
+  constexpr chrono::day operator"" d(unsigned long long d) noexcept;
+  constexpr chrono::year operator"" y(unsigned long long y) noexcept;
+}
+
+#else
+
+using namespace std;
+chrono::day dec_d = 5d;
+chrono::day oct_d = 05d;
+chrono::day bin_d = 0b011d;
+// expected-error@+3{{no viable conversion from 'int' to 'chrono::day'}}
+// expected-note@9{{candidate constructor (the implicit copy constructor)}}
+// expected-note@9{{candidate constructor (the implicit move constructor)}}
+chrono::day hex_d = 0x44d;
+chrono::year y  = 10y;
+#endif
Index: lib/Lex/LiteralSupport.cpp
===
--- lib/Lex/LiteralSupport.cpp
+++ lib/Lex/LiteralSupport.cpp
@@ -751,7 +751,8 @@
 
   // If we have a hex digit other than 'e' (which denotes a FP exponent) then
   // the code is using an incorrect base.
-  if (isHexDigit(*s) && *s != 'e' && *s != 'E') {
+  if (isHexDigit(*s) && *s != 'e' && *s != 'E' &&
+  !isValidUDSuffix(PP.getLangOpts(), StringRef(s, ThisTokEnd - s))) {
 PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, s-ThisTokBegin),
 diag::err_invalid_digit) << StringRef(s, 1) << (radix == 8 ? 1 : 
0);
 hadError = true;
@@ -804,12 +805,14 @@
   if (!LangOpts.CPlusPlus14)
 return false;
 
-  // In C++1y, "s", "h", "min", "ms", "us", and "ns" are used in the library.
+  // In C++14, "s", "h", "min", "ms", "us", and "ns" are used in the library.
   // Per tweaked N3660, "il", "i", and "if" are also used in the library.
+  // In C++2a "d" and "y" are used in the library.
   return llvm::StringSwitch(Suffix)
   .Cases("h", "min", "s", true)
   .Cases("ms", "us", "ns", true)
   .Cases("il", "i", "if", true)
+  .Cases("d", "y", LangOpts.CPlusPlus2a)
   .Default(false);
 }
 
@@ -922,7 +925,9 @@
 s = SkipBinaryDigits(s);
 if (s == ThisTokEnd) {
   // Done.
-} else if (isHexDigit(*s)) {
+} else if (isHexDigit(*s) &&
+   !isValidUDSuffix(PP.getLangOpts(),
+StringRef(s, ThisTokEnd - s))) {
   PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, s-ThisTokBegin),
   diag::err_invalid_digit) << StringRef(s, 1) << 2;
   hadError = true;


Index: test/SemaCXX/cxx2a-user-defined-literals.cpp
===
--- test/SemaCXX/cxx2a-user-defined-literals.cpp
+++ test/SemaCXX/cxx2a-user-defined-literals.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -std=c++2a %s -include %s -verify
+
+#ifndef INCLUDED
+#define INCLUDED
+
+#pragma clang system_header
+namespace std {
+  namespace chrono {
+struct day{};
+struct year{};
+  }
+  constexpr chrono::day operator"" d(unsigned long long d) noexcept;
+  constexpr chrono::year operator"" y(unsigned long long y) noexcept;
+}
+
+#else
+
+using namespace std;
+chrono::day dec_d = 5d;
+chrono::day oct_d = 05d;
+chrono::day bin_d = 0b011d;
+// expected-error@+3{{no viable conversion from 'int' to 'chrono::day'}}
+// expected-note@9{{candidate constructor (the implicit copy constructor)}}
+// expected-note@9{{candidate constructor (the implicit move constructor)}}
+chrono::day hex_d = 0x44d;
+chrono::year y  = 10y;
+#endif
Index: lib/Lex/LiteralSupport.cpp
===
--- lib/Lex/LiteralSupport.cpp
+++ lib/Lex/LiteralSupport.cpp
@@ -751,7 +751,8 @@
 
   // If we have a hex digit other than 'e' (which denotes a FP exponent) then
   // the code is using an incorrect base.
-  if (isHexDigit(*s) && *s != 'e' && *s != 'E') {
+  if (isHexDigit(*s) && *s != 'e' && *s != 'E' &&
+  !isValidUDSuffix(PP.getLangOpts(), StringRef(s, ThisTokEnd - s))) {
 PP.Diag(PP.AdvanceToTokenCharacter(TokLoc, s-ThisTokBegin),
 diag::err_invalid_digit) << StringRef(s, 1) << (radix == 8 ? 1 : 0);
 hadError = true;
@@ -804,12 +805,14 @@
   if (!LangOpts.CPlusPlus14)
 return false;
 
-  // In C++1y, "s", "h", "min", "ms", "us", and "ns" are used in the library.
+  // In C++14, "s", "h", "min", "ms", "us", and "ns" are used in the library.
   // Per tweaked N3660, "il", "i", and "if" are also used in the library.
+  // In C++2a "d" and "y" are used in the library.
   return llvm::StringSwit

Re: r336467 - [OPENMP] Fix PR38026: Link -latomic when -fopenmp is used.

[Hal Finkel via cfe-commits]

On 07/16/2018 01:19 PM, Jonas Hahnfeld wrote:
> [ Moving discussion from https://reviews.llvm.org/D49386 to the
> relevant comment on cfe-commits, CC'ing Hal who commented on the
> original issue ]
>
> Is this change really a good idea? It always requires libatomic for
> all OpenMP applications, even if there is no 'omp atomic' directive or
> all of them can be lowered to atomic instructions that don't require a
> runtime library. I'd argue that it's a larger restriction than the
> problem it solves.

Can you please elaborate on why you feel that this is problematic?

> Per https://clang.llvm.org/docs/Toolchain.html#libatomic-gnu the user
> is expected to manually link -latomic whenever Clang can't lower
> atomic instructions - including C11 atomics and C++ atomics. In my
> opinion OpenMP is just another abstraction that doesn't require a
> special treatment.

From my perspective, because we instruct our users that all you need to
do in order to enable OpenMP is pass -fopenmp flags during compiling and
linking. The user should not need to know or care about how atomics are
implemented.

It's not clear to me that our behavior for C++ atomics is good either.
From the documentation, it looks like the rationale is to avoid choosing
between the GNU libatomic implementation and the compiler-rt
implementation? We should probably make a default choice and provide a
flag to override. That would seem more user-friendly to me.

 -Hal

>
> Thoughts?
> Jonas
>
> On 2018-07-06 23:13, Alexey Bataev via cfe-commits wrote:
>> Author: abataev
>> Date: Fri Jul  6 14:13:41 2018
>> New Revision: 336467
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=336467&view=rev
>> Log:
>> [OPENMP] Fix PR38026: Link -latomic when -fopenmp is used.
>>
>> On Linux atomic constructs in OpenMP require libatomic library. Patch
>> links libatomic when -fopenmp is used.
>>
>> Modified:
>>     cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
>>     cfe/trunk/test/OpenMP/linking.c
>>
>> Modified: cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Gnu.cpp?rev=336467&r1=336466&r2=336467&view=diff
>>
>> ==
>>
>> --- cfe/trunk/lib/Driver/ToolChains/Gnu.cpp (original)
>> +++ cfe/trunk/lib/Driver/ToolChains/Gnu.cpp Fri Jul  6 14:13:41 2018
>> @@ -479,6 +479,7 @@ void tools::gnutools::Linker::ConstructJ
>>
>>    bool WantPthread = Args.hasArg(options::OPT_pthread) ||
>>   Args.hasArg(options::OPT_pthreads);
>> +  bool WantAtomic = false;
>>
>>    // FIXME: Only pass GompNeedsRT = true for platforms with
>> libgomp that
>>    // require librt. Most modern Linux platforms do, but some may
>> not.
>> @@ -487,13 +488,16 @@ void tools::gnutools::Linker::ConstructJ
>>     /* GompNeedsRT= */ true))
>>  // OpenMP runtimes implies pthreads when using the GNU
>> toolchain.
>>  // FIXME: Does this really make sense for all GNU toolchains?
>> -    WantPthread = true;
>> +    WantAtomic = WantPthread = true;
>>
>>    AddRunTimeLibs(ToolChain, D, CmdArgs, Args);
>>
>>    if (WantPthread && !isAndroid)
>>  CmdArgs.push_back("-lpthread");
>>
>> +  if (WantAtomic)
>> +    CmdArgs.push_back("-latomic");
>> +
>>    if (Args.hasArg(options::OPT_fsplit_stack))
>>  CmdArgs.push_back("--wrap=pthread_create");
>>
>>
>> Modified: cfe/trunk/test/OpenMP/linking.c
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/linking.c?rev=336467&r1=336466&r2=336467&view=diff
>>
>> ==
>>
>> --- cfe/trunk/test/OpenMP/linking.c (original)
>> +++ cfe/trunk/test/OpenMP/linking.c Fri Jul  6 14:13:41 2018
>> @@ -8,14 +8,14 @@
>>  // RUN:   | FileCheck --check-prefix=CHECK-LD-32 %s
>>  // CHECK-LD-32: "{{.*}}ld{{(.exe)?}}"
>>  // CHECK-LD-32: "-l[[DEFAULT_OPENMP_LIB:[^"]*]]"
>> -// CHECK-LD-32: "-lpthread" "-lc"
>> +// CHECK-LD-32: "-lpthread" "-latomic" "-lc"
>>  //
>>  // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
>>  // RUN: -fopenmp -target x86_64-unknown-linux -rtlib=platform \
>>  // RUN:   | FileCheck --check-prefix=CHECK-LD-64 %s
>>  // CHECK-LD-64: "{{.*}}ld{{(.exe)?}}"
>>  // CHECK-LD-64: "-l[[DEFAULT_OPENMP_LIB:[^"]*]]"
>> -// CHECK-LD-64: "-lpthread" "-lc"
>> +// CHECK-LD-64: "-lpthread" "-latomic" "-lc"
>>  //
>>  // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
>>  // RUN: -fopenmp=libgomp -target i386-unknown-linux
>> -rtlib=platform \
>> @@ -27,7 +27,7 @@
>>  // SIMD-ONLY2-NOT: liomp
>>  // CHECK-GOMP-LD-32: "{{.*}}ld{{(.exe)?}}"
>>  // CHECK-GOMP-LD-32: "-lgomp" "-lrt"
>> -// CHECK-GOMP-LD-32: "-lpthread" "-lc"
>> +// CHECK-GOMP-LD-32: "-lpthread" "-latomic" "-lc"
>>
>>  // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1
>> -fopenmp-simd -target i386-unknown-linux -rtlib=platform | FileCheck
>

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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

Hi Volodymyr, thanks for improving this.

> Need to double check what tests we have when using relative path names at the 
> root level.

Another interesting place to look at is 
`unittests/Basic/VirtualFileSystemTest.cpp`

> I'd like to make the behavior consistent because a file name is a specific 
> case of relative paths. So far there are no assertions and no errors but file 
> lookup doesn't seem to be working.

Makes sense. My general impression is that relative paths don't make sense here 
either, but you can address that in a follow up patch (giving time for any 
potential counterexample on this).




Comment at: clang/lib/Basic/VirtualFileSystem.cpp:1416
+if (NameValueNode)
+  error(NameValueNode, "file is not located in any directory");
+return nullptr;

vsapsai wrote:
> Not happy with the error message but didn't come up with anything better. 
> Suggestions are welcome.
What about the message you used in the comment: `file entry is at the root 
level, outside of any directory` ?


https://reviews.llvm.org/D49518



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

[PATCH] D49502: [CMake] Support statically linking dependencies only to shared or static library

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.



Comment at: libcxx/lib/CMakeLists.txt:269
+AND (TARGET cxxabi_static OR HAVE_LIBCXXABI))
+#if ((TARGET ${LIBCXX_CXX_ABI_LIBRARY}) OR
+#(${LIBCXX_CXX_ABI_LIBRARY} MATCHES "cxxabi(_static|_shared)?" AND 
HAVE_LIBCXXABI))

phosek wrote:
> ldionne wrote:
> > I don't understand why any of this needs to change -- can you please 
> > explain? Also, you probably didn't mean to leave the commented-out lines.
> The reason this change is needed the case when we're linking shared libc++abi 
> into shared libc++ in which case `${LIBCXX_CXX_ABI_LIBRARY}` will be set to 
> `cxxabi_shared` in `HandleLibCXXABI.cmake` but we cannot merge `libc++abi.so` 
> into `libc++.a`, so instead we force the use of `cxxabi_static` here.
> 
> Alternatively, we could modify `HandleLibCXXABI.cmake` to set two 
> dependencies, one for the static case and one for the shared case and use the 
> former one here.
> 
> Removed the commented out lines.
Thanks. There's something I still don't understand. If you are linking the ABI 
library dynamically, why would you want to merge it (well, the static version 
of it) into `libc++.a`? It seems like this is somewhat defeating the purpose of 
dynamically linking against the ABI library, no?



Comment at: libcxxabi/src/CMakeLists.txt:64
   # FIXME: Is it correct to prefer the static version of libunwind?
   if (NOT LIBCXXABI_ENABLE_STATIC_UNWINDER AND (TARGET unwind_shared OR 
HAVE_LIBUNWIND))
 list(APPEND LIBCXXABI_LIBRARIES unwind_shared)

Does this not need to change anymore? I think we'd have to set different flags 
for `cxxabi_shared` and `cxxabi_static`.



Repository:
  rCXX libc++

https://reviews.llvm.org/D49502



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

RE: r336467 - [OPENMP] Fix PR38026: Link -latomic when -fopenmp is used.

[Churbanov, Andrey via cfe-commits]

> Can you please elaborate on why you feel that this is problematic?

This change completely disables OpenMP parallelization on all systems those do 
not have libatomic library accessible.

So many people who earlier had working codes now cannot compiler any OpenMP 
program, even just print "Hello", that sound too huge restriction related to 
the fixed issue - save one compiler option for program with exotic atomic types.

-- Andrey

-Original Message-
From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of Hal 
Finkel via cfe-commits
Sent: Thursday, July 19, 2018 4:44 PM
To: Jonas Hahnfeld ; Alexey Bataev 
Cc: cfe-commits@lists.llvm.org
Subject: Re: r336467 - [OPENMP] Fix PR38026: Link -latomic when -fopenmp is 
used.


On 07/16/2018 01:19 PM, Jonas Hahnfeld wrote:
> [ Moving discussion from https://reviews.llvm.org/D49386 to the 
> relevant comment on cfe-commits, CC'ing Hal who commented on the 
> original issue ]
>
> Is this change really a good idea? It always requires libatomic for 
> all OpenMP applications, even if there is no 'omp atomic' directive or 
> all of them can be lowered to atomic instructions that don't require a 
> runtime library. I'd argue that it's a larger restriction than the 
> problem it solves.

Can you please elaborate on why you feel that this is problematic?

> Per https://clang.llvm.org/docs/Toolchain.html#libatomic-gnu the user 
> is expected to manually link -latomic whenever Clang can't lower 
> atomic instructions - including C11 atomics and C++ atomics. In my 
> opinion OpenMP is just another abstraction that doesn't require a 
> special treatment.

From my perspective, because we instruct our users that all you need to do in 
order to enable OpenMP is pass -fopenmp flags during compiling and linking. The 
user should not need to know or care about how atomics are implemented.

It's not clear to me that our behavior for C++ atomics is good either.
From the documentation, it looks like the rationale is to avoid choosing 
between the GNU libatomic implementation and the compiler-rt implementation? We 
should probably make a default choice and provide a flag to override. That 
would seem more user-friendly to me.

 -Hal

>
> Thoughts?
> Jonas
>
> On 2018-07-06 23:13, Alexey Bataev via cfe-commits wrote:
>> Author: abataev
>> Date: Fri Jul  6 14:13:41 2018
>> New Revision: 336467
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=336467&view=rev
>> Log:
>> [OPENMP] Fix PR38026: Link -latomic when -fopenmp is used.
>>
>> On Linux atomic constructs in OpenMP require libatomic library. Patch 
>> links libatomic when -fopenmp is used.
>>
>> Modified:
>>     cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
>>     cfe/trunk/test/OpenMP/linking.c
>>
>> Modified: cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/G
>> nu.cpp?rev=336467&r1=336466&r2=336467&view=diff
>>
>> =
>> =
>>
>> --- cfe/trunk/lib/Driver/ToolChains/Gnu.cpp (original)
>> +++ cfe/trunk/lib/Driver/ToolChains/Gnu.cpp Fri Jul  6 14:13:41 2018
>> @@ -479,6 +479,7 @@ void tools::gnutools::Linker::ConstructJ
>>
>>    bool WantPthread = Args.hasArg(options::OPT_pthread) ||
>>   Args.hasArg(options::OPT_pthreads);
>> +  bool WantAtomic = false;
>>
>>    // FIXME: Only pass GompNeedsRT = true for platforms with 
>> libgomp that
>>    // require librt. Most modern Linux platforms do, but some may 
>> not.
>> @@ -487,13 +488,16 @@ void tools::gnutools::Linker::ConstructJ
>>     /* GompNeedsRT= */ true))
>>  // OpenMP runtimes implies pthreads when using the GNU 
>> toolchain.
>>  // FIXME: Does this really make sense for all GNU toolchains?
>> -    WantPthread = true;
>> +    WantAtomic = WantPthread = true;
>>
>>    AddRunTimeLibs(ToolChain, D, CmdArgs, Args);
>>
>>    if (WantPthread && !isAndroid)
>>  CmdArgs.push_back("-lpthread");
>>
>> +  if (WantAtomic)
>> +    CmdArgs.push_back("-latomic");
>> +
>>    if (Args.hasArg(options::OPT_fsplit_stack))
>>  CmdArgs.push_back("--wrap=pthread_create");
>>
>>
>> Modified: cfe/trunk/test/OpenMP/linking.c
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/linking.c?r
>> ev=336467&r1=336466&r2=336467&view=diff
>>
>> =
>> =
>>
>> --- cfe/trunk/test/OpenMP/linking.c (original)
>> +++ cfe/trunk/test/OpenMP/linking.c Fri Jul  6 14:13:41 2018
>> @@ -8,14 +8,14 @@
>>  // RUN:   | FileCheck --check-prefix=CHECK-LD-32 %s
>>  // CHECK-LD-32: "{{.*}}ld{{(.exe)?}}"
>>  // CHECK-LD-32: "-l[[DEFAULT_OPENMP_LIB:[^"]*]]"
>> -// CHECK-LD-32: "-lpthread" "-lc"
>> +// CHECK-LD-32: "-lpthread" "-latomic" "-lc"
>>  //
>>  // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
>>  // RUN: -fopenmp -target x86_64-unkno

[PATCH] D48412: [RISCV] Add support for interrupt attribute

[Alex Bradbury via Phabricator via cfe-commits]

asb accepted this revision.
asb added a comment.
Herald added a subscriber: rkruppe.

This looks good to me with two caveats

- the tests don't seem to check that the "machine" is the default mode when the 
"interrupt" attribute has no arguments.
- Although the conversion from RISCVInterruptAttr::user to "user" etc is 
trivial, it would probably be worth testing the emitted attributes are as 
expected?


https://reviews.llvm.org/D48412



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

Re: r336467 - [OPENMP] Fix PR38026: Link -latomic when -fopenmp is used.

[Jonas Hahnfeld via cfe-commits]

On 2018-07-19 15:43, Hal Finkel wrote:

On 07/16/2018 01:19 PM, Jonas Hahnfeld wrote:

[ Moving discussion from https://reviews.llvm.org/D49386 to the
relevant comment on cfe-commits, CC'ing Hal who commented on the
original issue ]

Is this change really a good idea? It always requires libatomic for
all OpenMP applications, even if there is no 'omp atomic' directive or
all of them can be lowered to atomic instructions that don't require a
runtime library. I'd argue that it's a larger restriction than the
problem it solves.


Can you please elaborate on why you feel that this is problematic?


The linked patch deals with the case that there is no libatomic, 
effectively disabling all tests of the OpenMP runtime (even though only 
few of them require atomic instructions). So apparently there are Linux 
systems without libatomic. Taking them any chance to use OpenMP with 
Clang is a large regression IMO and not user-friendly either.



Per https://clang.llvm.org/docs/Toolchain.html#libatomic-gnu the user
is expected to manually link -latomic whenever Clang can't lower
atomic instructions - including C11 atomics and C++ atomics. In my
opinion OpenMP is just another abstraction that doesn't require a
special treatment.


From my perspective, because we instruct our users that all you need to
do in order to enable OpenMP is pass -fopenmp flags during compiling 
and

linking. The user should not need to know or care about how atomics are
implemented.

It's not clear to me that our behavior for C++ atomics is good either.
From the documentation, it looks like the rationale is to avoid 
choosing

between the GNU libatomic implementation and the compiler-rt
implementation? We should probably make a default choice and provide a
flag to override. That would seem more user-friendly to me.


I didn't mean to say it's a good default, but OpenMP is now different 
from C and C++. And as you said, the choice was probably made for a 
reason, so there should be some discussion whether to change it.


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

r337456 - [CodeGen] Disable aggressive structor optimizations at -O0, take 3

[Pavel Labath via cfe-commits]

Author: labath
Date: Thu Jul 19 07:05:22 2018
New Revision: 337456

URL: http://llvm.org/viewvc/llvm-project?rev=337456&view=rev
Log:
[CodeGen] Disable aggressive structor optimizations at -O0, take 3

The previous version of this patch (r332839) was reverted because it was
causing "definition with same mangled name as another definition" errors
in some module builds. This was caused by an unrelated bug in module
importing which it exposed. The importing problem was fixed in r336240,
so this recommits the original patch (r332839).

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

Modified:
cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
cfe/trunk/test/CodeGenCXX/ctor-dtor-alias.cpp
cfe/trunk/test/CodeGenCXX/float16-declarations.cpp

Modified: cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp?rev=337456&r1=337455&r2=337456&view=diff
==
--- cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp (original)
+++ cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp Thu Jul 19 07:05:22 2018
@@ -3737,12 +3737,22 @@ static StructorCodegen getCodegenToUse(C
   }
   llvm::GlobalValue::LinkageTypes Linkage = CGM.getFunctionLinkage(AliasDecl);
 
-  if (llvm::GlobalValue::isDiscardableIfUnused(Linkage))
-return StructorCodegen::RAUW;
+  // All discardable structors can be RAUWed, but we don't want to do that in
+  // unoptimized code, as that makes complete structor symbol disappear
+  // completely, which degrades debugging experience.
+  // Symbols with private linkage can be safely aliased, so we special case 
them
+  // here.
+  if (llvm::GlobalValue::isLocalLinkage(Linkage))
+return CGM.getCodeGenOpts().OptimizationLevel > 0 ? StructorCodegen::RAUW
+  : StructorCodegen::Alias;
 
+  // Linkonce structors cannot be aliased nor placed in a comdat, so these need
+  // to be emitted separately.
   // FIXME: Should we allow available_externally aliases?
-  if (!llvm::GlobalAlias::isValidLinkage(Linkage))
-return StructorCodegen::RAUW;
+  if (llvm::GlobalValue::isDiscardableIfUnused(Linkage) ||
+  !llvm::GlobalAlias::isValidLinkage(Linkage))
+return CGM.getCodeGenOpts().OptimizationLevel > 0 ? StructorCodegen::RAUW
+  : StructorCodegen::Emit;
 
   if (llvm::GlobalValue::isWeakForLinker(Linkage)) {
 // Only ELF and wasm support COMDATs with arbitrary names (C5/D5).

Modified: cfe/trunk/test/CodeGenCXX/ctor-dtor-alias.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/ctor-dtor-alias.cpp?rev=337456&r1=337455&r2=337456&view=diff
==
--- cfe/trunk/test/CodeGenCXX/ctor-dtor-alias.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/ctor-dtor-alias.cpp Thu Jul 19 07:05:22 2018
@@ -1,5 +1,7 @@
-// RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases 
| FileCheck --check-prefix=NOOPT %s
-
+// RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases 
> %t
+// RUN: FileCheck --check-prefix=NOOPT1 --input-file=%t %s
+// RUN: FileCheck --check-prefix=NOOPT2 --input-file=%t %s
+// RUN: FileCheck --check-prefix=NOOPT3 --input-file=%t %s
 // RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases 
-O1 -disable-llvm-passes > %t
 // RUN: FileCheck --check-prefix=CHECK1 --input-file=%t %s
 // RUN: FileCheck --check-prefix=CHECK2 --input-file=%t %s
@@ -21,6 +23,13 @@ namespace test1 {
 // CHECK1: define weak_odr void @_ZN5test16foobarIvED0Ev({{.*}} 
comdat($_ZN5test16foobarIvED5Ev)
 // CHECK1-NOT: comdat
 
+// This should happen regardless of the opt level.
+// NOOPT1: @_ZN5test16foobarIvEC1Ev = weak_odr unnamed_addr alias void {{.*}} 
@_ZN5test16foobarIvEC2Ev
+// NOOPT1: @_ZN5test16foobarIvED1Ev = weak_odr unnamed_addr alias void 
(%"struct.test1::foobar"*), void (%"struct.test1::foobar"*)* 
@_ZN5test16foobarIvED2Ev
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvEC2Ev({{.*}} 
comdat($_ZN5test16foobarIvEC5Ev)
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvED2Ev({{.*}} 
comdat($_ZN5test16foobarIvED5Ev)
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvED0Ev({{.*}} 
comdat($_ZN5test16foobarIvED5Ev)
+
 // COFF doesn't support comdats with arbitrary names (C5/D5).
 // COFF: define weak_odr {{.*}} void @_ZN5test16foobarIvEC2Ev({{.*}} comdat 
align
 // COFF: define weak_odr {{.*}} void @_ZN5test16foobarIvEC1Ev({{.*}} comdat 
align
@@ -37,12 +46,17 @@ template struct foobar;
 }
 
 namespace test2 {
-// test that when the destrucor is linkonce_odr we just replace every use of
+// test that when the destructor is linkonce_odr we just replace every use of
 // C1 with C2.
 
 // CHECK1: define internal void @__cxx_global_var_init()
 // CHECK1: call void @_ZN5test26foobarIvEC2Ev
 // CHECK1: define linkonce_odr void @_ZN5test26foobarIvEC2Ev(

[PATCH] D49509: [libc++] Allow running ABI list tests with different ABI versions

[Louis Dionne via Phabricator via cfe-commits]

ldionne edited subscribers, added: cfe-commits; removed: llvm-commits.
ldionne added a comment.

In https://reviews.llvm.org/D49509#1167500, @smeenai wrote:

> This went out to llvm-commits. You may wanna re-upload with cfe-commits added 
> instead.


Ah! That uncovers a deeper problem -- the instructions when setting up the 
monorepo with Arcanist say to copy `.arcconfig` from `llvm/`. This is wrong, as 
I guess it depends what component you will be submitting to.


Repository:
  rL LLVM

https://reviews.llvm.org/D49509



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

[PATCH] D49509: [libc++] Allow running ABI list tests with different ABI versions

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In https://reviews.llvm.org/D49509#1168084, @ldionne wrote:

> In https://reviews.llvm.org/D49509#1167500, @smeenai wrote:
>
> > This went out to llvm-commits. You may wanna re-upload with cfe-commits 
> > added instead.
>
>
> Ah! That uncovers a deeper problem -- the instructions when setting up the 
> monorepo with Arcanist say to copy `.arcconfig` from `llvm/`. This is wrong, 
> as I guess it depends what component you will be submitting to.


Nope, actually, that's not it. It's this Herald rule kicking in: F6725290: 
Screen Shot 2018-07-19 at 10.24.22.png 


Repository:
  rL LLVM

https://reviews.llvm.org/D49509



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

[PATCH] D49083: [HIP] Register/unregister device fat binary only once

[Yaxun Liu via Phabricator via cfe-commits]

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



Comment at: lib/CodeGen/CGCUDANV.cpp:444
+auto HandleValue =
+CtorBuilder.CreateAlignedLoad(GpuBinaryHandle, CGM.getPointerAlign());
+llvm::Constant *Zero = 
llvm::Constant::getNullValue(HandleValue->getType());

rjmccall wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > yaxunl wrote:
> > > > rjmccall wrote:
> > > > > Do you not need to worry about concurrency here?
> > > > The ctor functions are executed by the dynamic loader before the 
> > > > program gains the control. The dynamic loader cannot excute the ctor 
> > > > functions concurrently since doing that would not gurantee thread 
> > > > safety of the loaded program. Therefore we can assume sequential 
> > > > execution of ctor functions here.
> > > Okay.  That's worth a comment.
> > > 
> > > Is the name here specified by some ABI document, or is it just a 
> > > conventional name that we're picking now?
> > Will add a comment for that.
> > 
> > You mean `__hip_gpubin_handle`? It is an implementation detail. It is not 
> > defined by ABI or other documentation. Since it is only used internally by 
> > ctor functions, it is not a visible elf symbol. Its name is by convention 
> > since the cuda corresponding one was named __cuda_gpubin_handle.
> Well, it *is* ABI, right?  It's necessary for all translation units to agree 
> to use the same symbol here or else the registration will happen multiple 
> times.
Right. I created a pull request for HIP to document this 
https://github.com/ROCm-Developer-Tools/HIP/pull/580/files


https://reviews.llvm.org/D49083



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

[PATCH] D48341: [clang-doc] Refactoring mapper to map by scope

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added inline comments.
Herald added a subscriber: arphaman.



Comment at: clang-tools-extra/test/clang-doc/yaml-record.cpp:44
+// CHECK-0: ---
+// CHECK-0-NEXT: USR: '06B5F6A19BA9F6A832E127C9968282B94619B210'
+// CHECK-0-NEXT: Name:'C'

ioeric wrote:
> > Yes, in most regex worlds, but it doesn't work in FileCheck regex 
> > (complains about unbalanced braces).
> Forgot this was `FileCheck` ;) Have you tried `[0-9A-Z]{{n}}`? If nothing 
> works, I'd probably just  check length of one USR and use `{{.*}}` to match 
> the rest of USRs in all tests.
I can't wholly eliminate USRs from the tests (the bitcode ones use them as 
filenames), and so would the script I'm going to land from D49268 be 
sufficient? That way if the USR spec ever changes, it's fairly trivial to regen 
them all. 


https://reviews.llvm.org/D48341



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

[PATCH] D49546: [clangd] Implement query iterators for Dex symbol index

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev created this revision.
kbobyrev added reviewers: ioeric, sammccall.
kbobyrev added a project: clang-tools-extra.
Herald added subscribers: arphaman, jkorous, MaskRay, ilya-biryukov, mgorny.

This is a proof-of-concept implementation of query iterators. At the moment, it 
is pretty messy and has a lot of FIXMEs but I decided to push it a little 
earlier so that some early high-level feedback could be received.


https://reviews.llvm.org/D49546

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/index/dex/QueryIterator.cpp
  clang-tools-extra/clangd/index/dex/QueryIterator.h
  clang-tools-extra/unittests/clangd/CMakeLists.txt
  clang-tools-extra/unittests/clangd/DexIndexTests.cpp

Index: clang-tools-extra/unittests/clangd/DexIndexTests.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clangd/DexIndexTests.cpp
@@ -0,0 +1,440 @@
+//===-- DexIndexTests.cpp  *- C++ -*---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "index/dex/QueryIterator.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace dex {
+
+using std::make_shared;
+using std::shared_ptr;
+using std::vector;
+
+TEST(DexIndexIterators, DocumentIterator) {
+  const auto Symbols =
+  make_shared(PostingList({4, 7, 8, 20, 42, 100}));
+  DocumentIterator Iterator(Symbols);
+
+  EXPECT_EQ(Iterator.getIndex(), 0U);
+  EXPECT_EQ(Iterator.peek(), 4U);
+  EXPECT_EQ(Iterator.reachedEnd(), false);
+
+  EXPECT_EQ(Iterator.advance(), false);
+  EXPECT_EQ(Iterator.getIndex(), 1U);
+  EXPECT_EQ(Iterator.peek(), 7U);
+  EXPECT_EQ(Iterator.reachedEnd(), false);
+
+  EXPECT_EQ(Iterator.advanceTo(20), false);
+  EXPECT_EQ(Iterator.peek(), 20U);
+  EXPECT_EQ(Iterator.getIndex(), 3U);
+  EXPECT_EQ(Iterator.reachedEnd(), false);
+
+  EXPECT_EQ(Iterator.advanceTo(65), false);
+  EXPECT_EQ(Iterator.peek(), 100U);
+  EXPECT_EQ(Iterator.getIndex(), 5U);
+  EXPECT_EQ(Iterator.reachedEnd(), false);
+
+  EXPECT_EQ(Iterator.advanceTo(420), true);
+  EXPECT_EQ(Iterator.getIndex(), 6U);
+  EXPECT_EQ(Iterator.reachedEnd(), true);
+}
+
+TEST(DexIndexIterators, AndIterator) {
+  const auto EmptyList = make_shared(PostingList());
+  const auto FirstList =
+  make_shared(PostingList({0, 5, 7, 10, 42, 320, 9000}));
+  const auto SecondList =
+  make_shared(PostingList({0, 4, 7, 10, 30, 60, 320, 9000}));
+  const auto ThirdList =
+  make_shared(PostingList({1, 4, 7, 11, 30, 60, 320, 9000}));
+  const auto FourthList =
+  make_shared(PostingList({4, 7, 10, 30, 60, 320, 9000}));
+  const auto FifthList = make_shared(
+  PostingList({1, 4, 7, 10, 30, 60, 320, 9000, 100}));
+
+  vector> Children = {
+  make_shared(EmptyList)};
+  AndIterator And(Children);
+
+  EXPECT_EQ(And.reachedEnd(), true);
+
+  Children = {make_shared(EmptyList),
+  make_shared(FirstList)};
+  And = AndIterator(Children);
+
+  EXPECT_EQ(And.reachedEnd(), true);
+
+  Children = {make_shared(FirstList),
+  make_shared(SecondList)};
+  And = AndIterator(Children);
+
+  EXPECT_EQ(And.reachedEnd(), false);
+  EXPECT_EQ(And.peek(), 0U);
+
+  for (auto Child : And.getChildren())
+EXPECT_EQ(Child->peek(), 0U);
+
+  EXPECT_EQ(And.advance(), false);
+  EXPECT_EQ(And.peek(), 7U);
+  EXPECT_EQ(And.advance(), false);
+  EXPECT_EQ(And.peek(), 10U);
+  EXPECT_EQ(And.advance(), false);
+  EXPECT_EQ(And.peek(), 320U);
+  EXPECT_EQ(And.advance(), false);
+  EXPECT_EQ(And.peek(), 9000U);
+  EXPECT_EQ(And.advance(), true);
+
+  Children = {make_shared(FirstList),
+  make_shared(SecondList)};
+  And = AndIterator(Children);
+
+  EXPECT_EQ(And.advanceTo(0), false);
+  EXPECT_EQ(And.peek(), 0U);
+  EXPECT_EQ(And.advanceTo(5), false);
+  EXPECT_EQ(And.peek(), 7U);
+  EXPECT_EQ(And.advanceTo(10), false);
+  EXPECT_EQ(And.peek(), 10U);
+  EXPECT_EQ(And.advanceTo(42), false);
+  EXPECT_EQ(And.peek(), 320U);
+  EXPECT_EQ(And.advanceTo(8999), false);
+  EXPECT_EQ(And.peek(), 9000U);
+  EXPECT_EQ(And.advanceTo(9001), true);
+
+  Children = {make_shared(FirstList),
+  make_shared(SecondList)};
+  And = AndIterator(Children);
+
+  EXPECT_EQ(And.advanceTo(10), true);
+
+  // More complicated example: nested And iterators.
+  //
+  //+--+
+  //|Top Level Iterator|
+  //++-+
+  // |
+  //   +---+-+
+  //   | | | |
+  //  +v-+ +-v-+ +-v+ +--v--+
+  //  |First List| |Second List| |Third List| |Bottom Level Iterator|
+  //  +--+ +---+ +--+ +-

[PATCH] D46190: For a used declaration, mark any associated usings as referenced.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

@rsmith any further thoughts?  We would really like to get this in before the 
LLVM 7.0 branch, currently scheduled for 1 August.

In https://reviews.llvm.org/D46190#1168027, @CarlosAlbertoEnciso wrote:

> @probinson: I tried `clang-format-diff` and the main format issues are with 
> the test cases.
>
> Do you want the test cases to follow the clang format?


The lib and include files absolutely need to follow clang format.  Tests should 
also, if doing so doesn't disturb their readability.  If clang-format-diff is 
doing things like rearranging comments in the test files, it depends on whether 
it improves or reduces clarity.  Up to you.




Comment at: lib/Sema/SemaDeclCXX.cpp:15515
+
+// Mark the alias declaration and any associated chain as referenced.
+void Sema::MarkNamespaceAliasReferenced(NamedDecl *ND) {

This should use `///` to be  picked up by doxygen.


https://reviews.llvm.org/D46190



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

[PATCH] D48341: [clang-doc] Refactoring mapper to map by scope

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/test/clang-doc/yaml-record.cpp:44
+// CHECK-0: ---
+// CHECK-0-NEXT: USR: '06B5F6A19BA9F6A832E127C9968282B94619B210'
+// CHECK-0-NEXT: Name:'C'

juliehockett wrote:
> ioeric wrote:
> > > Yes, in most regex worlds, but it doesn't work in FileCheck regex 
> > > (complains about unbalanced braces).
> > Forgot this was `FileCheck` ;) Have you tried `[0-9A-Z]{{n}}`? If nothing 
> > works, I'd probably just  check length of one USR and use `{{.*}}` to match 
> > the rest of USRs in all tests.
> I can't wholly eliminate USRs from the tests (the bitcode ones use them as 
> filenames), and so would the script I'm going to land from D49268 be 
> sufficient? That way if the USR spec ever changes, it's fairly trivial to 
> regen them all. 
Sure.

For script-generated tests, please make it's clear to other people, who might 
not be familiar with the tool, to figure out how to regenerate the tests, in 
case they make a change somewhere in clang and break your tests. E.g. a big 
comment on top of file indicating it's generated and pointing to the script. 
The script should also have clear instructions.


https://reviews.llvm.org/D48341



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

[PATCH] D48560: [clangd] JSON <-> XPC conversions

[Jan Korous via Phabricator via cfe-commits]

jkorous marked 12 inline comments as done.
jkorous added inline comments.



Comment at: xpc/XPCJSONConversions.cpp:62
+  if (objType == XPC_TYPE_UINT64)
+return json::Expr(xpc_uint64_get_value(xpcObj));
+  if (objType == XPC_TYPE_STRING)

sammccall wrote:
> hmm, I think this shouldn't compile anymore, rather require you to explicitly 
> do the narrowing conversion to int64 or double.
Explicitly narroving now. Thanks.



Comment at: xpc/XPCJSONConversions.cpp:69
+  xpcObj,
+  ^bool(size_t /* index */, xpc_object_t value) {
+result.emplace_back(xpcToJson(value));

jkorous wrote:
> sammccall wrote:
> > this looks like objC syntax, I'm not familiar with it, but should this file 
> > be `.mm`?
> > 
> > Alternatively, it seems like you could write a C++ loop with 
> > `xpc_array_get_value` (though I don't know if there are performance 
> > concerns).
> > 
> > (and dictionary)
> Oh, sorry. These are actually C blocks - a nonstandard C extension.
> https://en.wikipedia.org/wiki/Blocks_(C_language_extension)
> https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Blocks/Articles/00_Introduction.html
> 
> There was no performance concern on my side - just liked this approach 
> better. Didn't realize how confusing it might be, will rewrite this.
Allright, by trying to get rid of C block in dictionary conversion I remembered 
that there's unfortunately no sane reason how to iterate XPC dictionary without 
using xpc_dictionary_apply() which uses C block for functor parameter.

https://developer.apple.com/documentation/xpc/1505404-xpc_dictionary_apply?language=objc
https://developer.apple.com/documentation/xpc/xpc_dictionary_applier_t?language=objc

Anyway, even had we succeeded in removing C blocks from conversion they still 
would be necessary for dispatch as xpc_connection_set_event_handler() is 
another part of XPC API that uses it.

I guess that there's no point in removing the xpc_array_apply() then. Is that 
ok with you?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48560



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

[PATCH] D49546: [clangd] Implement query iterators for Dex symbol index

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for sending this early!
Rough interface comments - mostly looks good though!




Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:26
+
+using PostingList = std::vector;
+

we should at least use a type alias for a DocID (maybe an opaque one - not sure 
for now)



Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:39
+public:
+  virtual bool reachedEnd() const = 0;
+  // FIXME(kbobyrev): Should advance() and advanceTo() return size_t peek()

or have peek return an InvalidID == size_t{-1}?



Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:43
+  virtual bool advance() = 0;
+  virtual bool advanceTo(size_t Rank) = 0;
+  virtual size_t peek() const = 0;

name Rank is confusing. Type should be DocID (param doesn't need a name in the 
interface)



Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:44
+  virtual bool advanceTo(size_t Rank) = 0;
+  virtual size_t peek() const = 0;
+

could reasonably call this operator*



Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:53
+public:
+  DocumentIterator(std::shared_ptr Documents);
+

is shared_ptr really needed here? Seems like a raw pointer would be fine. I can 
see that when rebuilding the index we'll have some fun lifetime issues that are 
sometimes solved with shared_ptr, but I think that would be at the full-index 
level, not the individual-posting-list level.

(if you want one less indirection, you could have `using PostingListRef = 
ArrayRef`, but it shouldn't matter)



Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:59
+  bool advanceTo(size_t Rank) override;
+  size_t getIndex() const;
+  size_t peek() const override;

what's this for?
I don't think it's worth exposing for testing, should be able to get at the 
needed cases through the public interface



Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:63
+  ///
+  void reset();
+

how will you use this? it's not in the interface, and if you know the 
implementation then creating a new iterator is cheap



Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:76
+public:
+  AndIterator(const std::vector> &Children);
+

again, unless you have a strong reason, don't use shared_ptr, as it makes it 
hard to reason about ownership and lifetimes.

These are a tree, unique_ptr should be fine. That way a top-level owned 
iterator is self-contained.



Comment at: clang-tools-extra/clangd/index/dex/QueryIterator.h:83
+
+  const std::vector> getChildren() const;
+

what's this for?


https://reviews.llvm.org/D49546



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

[PATCH] D48562: [clangd] XPC transport layer

[Jan Korous via Phabricator via cfe-commits]

jkorous marked 7 inline comments as done.
jkorous added inline comments.



Comment at: xpc/test-client/ClangdXPCTestClient.cpp:51
+  dlHandle, "clangd_xpc_get_bundle_identifier");
+  xpc_connection_t conn =
+  xpc_connection_create(clangd_xpc_get_bundle_identifier(), NULL);

arphaman wrote:
> jkorous wrote:
> > arphaman wrote:
> > > We should probably use the main queue here too to ensure that we don't 
> > > get bit by concurrent handler invocations. 
> > As far as I understand it this is taken care of by calling dispatch_main().
> > 
> > Per man dispatch_main:
> > "MAIN QUEUE
> > [...]
> >  Programs must call dispatch_main() at the end of main() in order to 
> > process blocks submitted to the main queue."
> > 
> Not really. The documentation for the parameter states the following: 
> 
> The GCD queue to which the event handler block will be submitted. This 
> parameter may be NULL, in which case the connection's target queue will be 
> libdispatch's default target queue, defined as DISPATCH_TARGET_QUEUE_DEFAULT. 
> The target queue may be changed later with a call to 
> xpc_connection_set_target_queue(). 
> 
> So the handler blocks will be submitted to the concurrent queue 
> (DISPATCH_TARGET_QUEUE_DEFAULT) which means that they might end up running 
> concurrently.
Sorry, my bad.


https://reviews.llvm.org/D48562



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

r337463 - [analyzer] Add support for more basic_string API in

[Reka Kovacs via cfe-commits]

Author: rkovacs
Date: Thu Jul 19 08:10:06 2018
New Revision: 337463

URL: http://llvm.org/viewvc/llvm-project?rev=337463&view=rev
Log:
[analyzer] Add support for more basic_string API in
DanglingInternalBufferChecker.

A pointer referring to the elements of a basic_string may be invalidated
by calling a non-const member function, except operator[], at, front,
back, begin, rbegin, end, and rend. The checker now warns if the pointer
is used after such operations.

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

Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
cfe/trunk/test/Analysis/dangling-internal-buffer.cpp

Modified: 
cfe/trunk/lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp?rev=337463&r1=337462&r2=337463&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp 
(original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp Thu 
Jul 19 08:10:06 2018
@@ -32,8 +32,8 @@ REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap
 // This is a trick to gain access to PtrSet's Factory.
 namespace clang {
 namespace ento {
-template<> struct ProgramStateTrait
-  : public ProgramStatePartialTrait {
+template <>
+struct ProgramStateTrait : public ProgramStatePartialTrait {
   static void *GDMIndex() {
 static int Index = 0;
 return &Index;
@@ -46,7 +46,10 @@ namespace {
 
 class DanglingInternalBufferChecker
 : public Checker {
-  CallDescription CStrFn, DataFn;
+
+  CallDescription AppendFn, AssignFn, ClearFn, CStrFn, DataFn, EraseFn,
+  InsertFn, PopBackFn, PushBackFn, ReplaceFn, ReserveFn, ResizeFn,
+  ShrinkToFitFn, SwapFn;
 
 public:
   class DanglingBufferBRVisitor : public BugReporterVisitor {
@@ -81,7 +84,17 @@ public:
 }
   };
 
-  DanglingInternalBufferChecker() : CStrFn("c_str"), DataFn("data") {}
+  DanglingInternalBufferChecker()
+  : AppendFn("append"), AssignFn("assign"), ClearFn("clear"),
+CStrFn("c_str"), DataFn("data"), EraseFn("erase"), InsertFn("insert"),
+PopBackFn("pop_back"), PushBackFn("push_back"), ReplaceFn("replace"),
+ReserveFn("reserve"), ResizeFn("resize"),
+ShrinkToFitFn("shrink_to_fit"), SwapFn("swap") {}
+
+  /// Check whether the function called on the container object is a
+  /// member function that potentially invalidates pointers referring
+  /// to the objects's internal buffer.
+  bool mayInvalidateBuffer(const CallEvent &Call) const;
 
   /// Record the connection between the symbol returned by c_str() and the
   /// corresponding string object region in the ProgramState. Mark the symbol
@@ -94,6 +107,37 @@ public:
 
 } // end anonymous namespace
 
+// [string.require]
+//
+// "References, pointers, and iterators referring to the elements of a
+// basic_string sequence may be invalidated by the following uses of that
+// basic_string object:
+//
+// -- TODO: As an argument to any standard library function taking a reference
+// to non-const basic_string as an argument. For example, as an argument to
+// non-member functions swap(), operator>>(), and getline(), or as an argument
+// to basic_string::swap().
+//
+// -- Calling non-const member functions, except operator[], at, front, back,
+// begin, rbegin, end, and rend."
+//
+bool DanglingInternalBufferChecker::mayInvalidateBuffer(
+const CallEvent &Call) const {
+  if (const auto *MemOpCall = dyn_cast(&Call)) {
+OverloadedOperatorKind Opc = MemOpCall->getOriginExpr()->getOperator();
+if (Opc == OO_Equal || Opc == OO_PlusEqual)
+  return true;
+return false;
+  }
+  return (isa(Call) || Call.isCalled(AppendFn) ||
+  Call.isCalled(AssignFn) || Call.isCalled(ClearFn) ||
+  Call.isCalled(EraseFn) || Call.isCalled(InsertFn) ||
+  Call.isCalled(PopBackFn) || Call.isCalled(PushBackFn) ||
+  Call.isCalled(ReplaceFn) || Call.isCalled(ReserveFn) ||
+  Call.isCalled(ResizeFn) || Call.isCalled(ShrinkToFitFn) ||
+  Call.isCalled(SwapFn));
+}
+
 void DanglingInternalBufferChecker::checkPostCall(const CallEvent &Call,
   CheckerContext &C) const {
   const auto *ICall = dyn_cast(&Call);
@@ -127,7 +171,7 @@ void DanglingInternalBufferChecker::chec
 return;
   }
 
-  if (isa(ICall)) {
+  if (mayInvalidateBuffer(Call)) {
 if (const PtrSet *PS = State->get(ObjRegion)) {
   // Mark all pointer symbols associated with the deleted object released.
   const Expr *Origin = Call.getOriginExpr();
@@ -161,8 +205,8 @@ void DanglingInternalBufferChecker::chec
   CleanedUpSet = F.remove(CleanedUpSet, Symbol);
   }
   State = CleanedUpSet.isEmpty()
-  ? State->remove(Entry.first)
- 

[PATCH] D49360: [analyzer] Add support for more basic_string API in DanglingInternalBufferChecker

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC337463: [analyzer] Add support for more basic_string API in 
(authored by rkovacs, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D49360

Files:
  lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  test/Analysis/dangling-internal-buffer.cpp

Index: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
+++ lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
@@ -32,8 +32,8 @@
 // This is a trick to gain access to PtrSet's Factory.
 namespace clang {
 namespace ento {
-template<> struct ProgramStateTrait
-  : public ProgramStatePartialTrait {
+template <>
+struct ProgramStateTrait : public ProgramStatePartialTrait {
   static void *GDMIndex() {
 static int Index = 0;
 return &Index;
@@ -46,7 +46,10 @@
 
 class DanglingInternalBufferChecker
 : public Checker {
-  CallDescription CStrFn, DataFn;
+
+  CallDescription AppendFn, AssignFn, ClearFn, CStrFn, DataFn, EraseFn,
+  InsertFn, PopBackFn, PushBackFn, ReplaceFn, ReserveFn, ResizeFn,
+  ShrinkToFitFn, SwapFn;
 
 public:
   class DanglingBufferBRVisitor : public BugReporterVisitor {
@@ -81,7 +84,17 @@
 }
   };
 
-  DanglingInternalBufferChecker() : CStrFn("c_str"), DataFn("data") {}
+  DanglingInternalBufferChecker()
+  : AppendFn("append"), AssignFn("assign"), ClearFn("clear"),
+CStrFn("c_str"), DataFn("data"), EraseFn("erase"), InsertFn("insert"),
+PopBackFn("pop_back"), PushBackFn("push_back"), ReplaceFn("replace"),
+ReserveFn("reserve"), ResizeFn("resize"),
+ShrinkToFitFn("shrink_to_fit"), SwapFn("swap") {}
+
+  /// Check whether the function called on the container object is a
+  /// member function that potentially invalidates pointers referring
+  /// to the objects's internal buffer.
+  bool mayInvalidateBuffer(const CallEvent &Call) const;
 
   /// Record the connection between the symbol returned by c_str() and the
   /// corresponding string object region in the ProgramState. Mark the symbol
@@ -94,6 +107,37 @@
 
 } // end anonymous namespace
 
+// [string.require]
+//
+// "References, pointers, and iterators referring to the elements of a
+// basic_string sequence may be invalidated by the following uses of that
+// basic_string object:
+//
+// -- TODO: As an argument to any standard library function taking a reference
+// to non-const basic_string as an argument. For example, as an argument to
+// non-member functions swap(), operator>>(), and getline(), or as an argument
+// to basic_string::swap().
+//
+// -- Calling non-const member functions, except operator[], at, front, back,
+// begin, rbegin, end, and rend."
+//
+bool DanglingInternalBufferChecker::mayInvalidateBuffer(
+const CallEvent &Call) const {
+  if (const auto *MemOpCall = dyn_cast(&Call)) {
+OverloadedOperatorKind Opc = MemOpCall->getOriginExpr()->getOperator();
+if (Opc == OO_Equal || Opc == OO_PlusEqual)
+  return true;
+return false;
+  }
+  return (isa(Call) || Call.isCalled(AppendFn) ||
+  Call.isCalled(AssignFn) || Call.isCalled(ClearFn) ||
+  Call.isCalled(EraseFn) || Call.isCalled(InsertFn) ||
+  Call.isCalled(PopBackFn) || Call.isCalled(PushBackFn) ||
+  Call.isCalled(ReplaceFn) || Call.isCalled(ReserveFn) ||
+  Call.isCalled(ResizeFn) || Call.isCalled(ShrinkToFitFn) ||
+  Call.isCalled(SwapFn));
+}
+
 void DanglingInternalBufferChecker::checkPostCall(const CallEvent &Call,
   CheckerContext &C) const {
   const auto *ICall = dyn_cast(&Call);
@@ -127,7 +171,7 @@
 return;
   }
 
-  if (isa(ICall)) {
+  if (mayInvalidateBuffer(Call)) {
 if (const PtrSet *PS = State->get(ObjRegion)) {
   // Mark all pointer symbols associated with the deleted object released.
   const Expr *Origin = Call.getOriginExpr();
@@ -161,8 +205,8 @@
   CleanedUpSet = F.remove(CleanedUpSet, Symbol);
   }
   State = CleanedUpSet.isEmpty()
-  ? State->remove(Entry.first)
-  : State->set(Entry.first, CleanedUpSet);
+  ? State->remove(Entry.first)
+  : State->set(Entry.first, CleanedUpSet);
 }
   }
   C.addTransition(State);
@@ -183,7 +227,7 @@
 
   SmallString<256> Buf;
   llvm::raw_svector_ostream OS(Buf);
-  OS << "Pointer to dangling buffer was obtained here";
+  OS << "Dangling inner pointer obtained here";
   PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
  N->getLocationContext());
   return std::make_shared(Pos, OS.str(), true,
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/Mal

[PATCH] D49548: [clangd] XPC WIP

[Jan Korous via Phabricator via cfe-commits]

jkorous created this revision.
jkorous added reviewers: sammccall, arphaman.
Herald added subscribers: cfe-commits, dexonsmith, MaskRay, ioeric, 
ilya-biryukov, mgorny.

Combined, rebased and modified original XPC patches.

- https://reviews.llvm.org/D48559 [clangd] refactoring for XPC transport layer 
[NFCI]
- https://reviews.llvm.org/D48560 [clangd] JSON <-> XPC conversions
- https://reviews.llvm.org/D48562 [clangd] XPC transport layer

- Rebased on TOT.
- Made changes according to review.

Couple of questions are still open in reviews of original patches and more 
importantly the discussion about transport abstraction just started:
https://reviews.llvm.org/D48559#1167340
https://reviews.llvm.org/D49389

I thought it would be useful to have current state of our original approach 
online as it will be foundation for our implementation anyway (implementing 
whatever interface we converge to).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49548

Files:
  CMakeLists.txt
  ClangdLSPServer.cpp
  ClangdLSPServer.h
  DispatcherCommon.cpp
  DispatcherCommon.h
  Features.inc.in
  JSONRPCDispatcher.cpp
  JSONRPCDispatcher.h
  LSPOutput.h
  ProtocolHandlers.cpp
  ProtocolHandlers.h
  clangd/CMakeLists.txt
  clangd/xpc/CMakeLists.txt
  clangd/xpc/ConversionTests.cpp
  clangd/xpc/initialize.test
  lit.cfg
  lit.site.cfg.in
  tool/CMakeLists.txt
  tool/ClangdMain.cpp
  xpc/CMakeLists.txt
  xpc/Conversion.cpp
  xpc/Conversion.h
  xpc/README.txt
  xpc/XPCDispatcher.cpp
  xpc/XPCDispatcher.h
  xpc/cmake/Info.plist.in
  xpc/cmake/XPCServiceInfo.plist.in
  xpc/cmake/modules/CreateClangdXPCFramework.cmake
  xpc/framework/CMakeLists.txt
  xpc/framework/ClangdXPC.cpp
  xpc/test-client/CMakeLists.txt
  xpc/test-client/ClangdXPCTestClient.cpp

Index: clangd/xpc/ConversionTests.cpp
===
--- /dev/null
+++ clangd/xpc/ConversionTests.cpp
@@ -0,0 +1,454 @@
+//===-- ConversionTests.cpp  --*- C++ -*---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "xpc/Conversion.h"
+#include "gtest/gtest.h"
+
+#include 
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using namespace llvm;
+
+TEST(JsonXpcConversionTest, Null) {
+  EXPECT_TRUE(
+xpc_equal(
+  jsonToXpc(json::Value(nullptr)),
+  xpc_null_create()
+)
+  );
+  EXPECT_TRUE(
+json::Value(nullptr) == xpcToJson(xpc_null_create())
+  );
+}
+
+TEST(JsonXpcConversionTest, Bool) {
+  EXPECT_TRUE(
+xpc_equal(
+  jsonToXpc(json::Value(true)),
+  xpc_bool_create(true)
+)
+  );
+  EXPECT_TRUE(
+json::Value(false) == xpcToJson(xpc_bool_create(false))
+  );
+}
+
+TEST(JsonXpcConversionTest, Number) {
+  EXPECT_TRUE(
+xpc_equal(
+  jsonToXpc(json::Value(3.14)),
+  xpc_double_create(3.14)
+)
+  );
+  EXPECT_TRUE(
+json::Value(3.14) == xpcToJson(xpc_double_create(3.14))
+  );
+
+  EXPECT_TRUE(
+xpc_equal(
+  jsonToXpc(json::Value(42)),
+  xpc_double_create(42)
+)
+  );
+  EXPECT_TRUE(
+json::Value(42) == xpcToJson(xpc_double_create(42))
+  );
+  EXPECT_TRUE(
+json::Value(42) == xpcToJson(xpc_int64_create(42))
+  );
+
+  EXPECT_TRUE(
+xpc_equal(
+  jsonToXpc(json::Value(-100)),
+  xpc_double_create(-100)
+)
+  );
+  EXPECT_TRUE(
+json::Value(-100) == xpcToJson(xpc_double_create(-100))
+  );
+
+  int64_t bigPositiveValue = std::numeric_limits::max();
+  ++bigPositiveValue;
+  int64_t bigNegativeValue = std::numeric_limits::min();
+  --bigNegativeValue;
+
+  EXPECT_TRUE(
+xpc_equal(
+  jsonToXpc(json::Value(bigPositiveValue)),
+  xpc_double_create(bigPositiveValue)
+)
+  );
+  EXPECT_TRUE(
+json::Value(bigPositiveValue) == xpcToJson(xpc_double_create(bigPositiveValue))
+  );
+
+  EXPECT_TRUE(
+xpc_equal(
+  jsonToXpc(json::Value(bigNegativeValue)),
+  xpc_double_create(bigNegativeValue)
+)
+  );
+  EXPECT_TRUE(
+json::Value(bigNegativeValue) == xpcToJson(xpc_double_create(bigNegativeValue))
+  );
+}
+
+TEST(JsonXpcConversionTest, String) {
+  EXPECT_TRUE(
+xpc_equal(
+  jsonToXpc(json::Value("foo")),
+  xpc_string_create("foo")
+)
+  );
+  EXPECT_TRUE(
+json::Value("foo") == xpcToJson(xpc_string_create("foo"))
+  );
+
+  EXPECT_TRUE(
+xpc_equal(
+  jsonToXpc(json::Value("")),
+  xpc_string_create("")
+)
+  );
+  EXPECT_TRUE(
+json::Value("") == xpcToJson(xpc_string_create(""))
+  );
+
+  EXPECT_TRUE(
+xpc_equal(
+  jsonToXpc(json::Value("123")),
+  xpc_string_create("123")
+)
+  );
+  EXPECT_TRUE(
+json::Value("123") == xpcToJson(xpc_string_create("123"))
+  );
+
+  EXPECT_TRUE(
+xpc_equal(
+  jsonToXpc(json::Value(" ")),
+  xpc_string_create

[PATCH] D49466: Initial implementation of -fmacro-prefix-mapand -ffile-prefix-map

[Dan McGregor via Phabricator via cfe-commits]

dankm added inline comments.



Comment at: include/clang/Lex/PreprocessorOptions.h:171
+  /// A prefix map for __FILE__ and __BASEFILE__
+  std::map MacroPrefixMap;
+

erichkeane wrote:
> erichkeane wrote:
> > It seems this can be StringRefs as well.
> Did you miss this one?  Or is there a good reason these cannot be stringrefs?
I didn't miss it. StringRefs here don't survive. The function that adds them to 
the map creates temporary strings, that go away once that function ends causing 
StringRefs to dangle. std::string keeps copies.



Comment at: lib/Driver/ToolChains/Clang.cpp:616
+}
 else
   CmdArgs.push_back(Args.MakeArgString("-fdebug-prefix-map=" + Map));

erichkeane wrote:
> With the continue above, 'else' is unnecessary/against coding standard.
Next diff will have that.



Comment at: lib/Lex/PPMacroExpansion.cpp:1457
 
+static std::string remapMacroPath(StringRef Path,
+   const std::map Did clang-format do this formatting?  It looks REALLY weird...
No, that's my text editor. I'll fix it.



Comment at: lib/Lex/PPMacroExpansion.cpp:1532
   FN += PLoc.getFilename();
+  // TODO remap macro prefix here
+  FN = remapMacroPath(FN, PPOpts->MacroPrefixMap);

erichkeane wrote:
> First, comments end in a period.  Second, isn't that what the next line does?
Yes, old comment is old ;)



Comment at: lib/Lex/PPMacroExpansion.cpp:1460
+  for (const auto &Entry : MacroPrefixMap)
+if (Path.startswith(Entry.first))
+  return (Twine(Entry.second) + Path.substr(Entry.first.size())).str();

dankm wrote:
> joerg wrote:
> > This doesn't handle directory vs string prefix prefix correctly, does it? 
> > At the very least, it should have a test case :)
> Good catch. I expect not. But on the other hand, it's exactly what 
> debug-prefix-map does :)
> 
> I'll add test cases in a future review. My first goal was getting something 
> sort-of working.
There should be a test, but apparently the debug prefix map code also does this.

What do you think the correct behaviour should be? a string prefix, or a 
directory prefix?



Comment at: lib/Lex/PPMacroExpansion.cpp:1528
 // Escape this filename.  Turn '\' -> '\\' '"' -> '\"'
-SmallString<128> FN;
+std::string FN;
 if (PLoc.isValid()) {

erichkeane wrote:
> dankm wrote:
> > erichkeane wrote:
> > > This change shouldn't be necessary, SmallString is still likely the right 
> > > answer here.
> > I tried that long ago. It didn't work, I don't remember exactly why. But I 
> > agree that SmallString should be enough. I'll dig more.
> Just noting to handle this before approval.
Yup, with some changes to remapMacroPath SmallString works fine.


Repository:
  rC Clang

https://reviews.llvm.org/D49466



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

[PATCH] D48559: [clangd] refactoring for XPC transport layer [NFCI]

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Sam, just out of curiosity - would it be possible for you to share any relevant 
experience gained by using porting clangd to protobuf-based transport layer?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48559



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

r337466 - [analyzer] Fix disappearing notes in DanglingInternalBufferChecker tests

[Reka Kovacs via cfe-commits]

Author: rkovacs
Date: Thu Jul 19 08:44:46 2018
New Revision: 337466

URL: http://llvm.org/viewvc/llvm-project?rev=337466&view=rev
Log:
[analyzer] Fix disappearing notes in DanglingInternalBufferChecker tests

Correct a mistake of the exact same kind I am writing this checker for.

Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=337466&r1=337465&r2=337466&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Thu Jul 19 08:44:46 
2018
@@ -2901,6 +2901,9 @@ std::shared_ptr Mal
   // Find out if this is an interesting point and what is the kind.
   const char *Msg = nullptr;
   StackHintGeneratorForSymbol *StackHint = nullptr;
+  SmallString<256> Buf;
+  llvm::raw_svector_ostream OS(Buf);
+  
   if (Mode == Normal) {
 if (isAllocated(RS, RSPrev, S)) {
   Msg = "Memory is allocated";
@@ -2917,8 +2920,6 @@ std::shared_ptr Mal
   Msg = "Memory is released";
   break;
 case AF_InternalBuffer: {
-  SmallString<256> Buf;
-  llvm::raw_svector_ostream OS(Buf);
   OS << "Inner pointer invalidated by call to ";
   if (N->getLocation().getKind() == 
ProgramPoint::PostImplicitCallKind) {
 OS << "destructor";


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

[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

[Leonard Chan via Phabricator via cfe-commits]

leonardchan updated this revision to Diff 156280.
leonardchan marked an inline comment as done.

Repository:
  rC Clang

https://reviews.llvm.org/D48661

Files:
  include/clang/AST/ASTContext.h
  include/clang/Basic/FixedPoint.h
  include/clang/Basic/TargetInfo.h
  lib/AST/ASTContext.cpp
  lib/Basic/CMakeLists.txt
  lib/Basic/FixedPoint.cpp
  lib/Sema/SemaExpr.cpp
  test/Frontend/fixed_point_declarations.c
  unittests/Basic/CMakeLists.txt
  unittests/Basic/FixedPointTest.cpp

Index: unittests/Basic/FixedPointTest.cpp
===
--- /dev/null
+++ unittests/Basic/FixedPointTest.cpp
@@ -0,0 +1,683 @@
+//===- unittests/Basic/FixedPointTest.cpp -- fixed point number tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Basic/FixedPoint.h"
+#include "llvm/ADT/APSInt.h"
+#include "gtest/gtest.h"
+
+using clang::APFixedPoint;
+using clang::FixedPointSemantics;
+using llvm::APInt;
+using llvm::APSInt;
+
+namespace {
+
+FixedPointSemantics Saturated(FixedPointSemantics Sema) {
+  Sema.setSaturated(true);
+  return Sema;
+}
+
+FixedPointSemantics getSAccumSema() {
+  return FixedPointSemantics(/*width=*/16, /*scale=*/7, /*isSigned=*/true,
+ /*isSaturated=*/false,
+ /*hasUnsignedPadding=*/false);
+}
+
+FixedPointSemantics getAccumSema() {
+  return FixedPointSemantics(/*width=*/32, /*scale=*/15, /*isSigned=*/true,
+ /*isSaturated=*/false,
+ /*hasUnsignedPadding=*/false);
+}
+
+FixedPointSemantics getLAccumSema() {
+  return FixedPointSemantics(/*width=*/64, /*scale=*/31, /*isSigned=*/true,
+ /*isSaturated=*/false,
+ /*hasUnsignedPadding=*/false);
+}
+
+FixedPointSemantics getSFractSema() {
+  return FixedPointSemantics(/*width=*/8, /*scale=*/7, /*isSigned=*/true,
+ /*isSaturated=*/false,
+ /*hasUnsignedPadding=*/false);
+}
+
+FixedPointSemantics getFractSema() {
+  return FixedPointSemantics(/*width=*/16, /*scale=*/15, /*isSigned=*/true,
+ /*isSaturated=*/false,
+ /*hasUnsignedPadding=*/false);
+}
+
+FixedPointSemantics getLFractSema() {
+  return FixedPointSemantics(/*width=*/32, /*scale=*/31, /*isSigned=*/true,
+ /*isSaturated=*/false,
+ /*hasUnsignedPadding=*/false);
+}
+
+FixedPointSemantics getUSAccumSema() {
+  return FixedPointSemantics(/*width=*/16, /*scale=*/8, /*isSigned=*/false,
+ /*isSaturated=*/false,
+ /*hasUnsignedPadding=*/false);
+}
+
+FixedPointSemantics getUAccumSema() {
+  return FixedPointSemantics(/*width=*/32, /*scale=*/16, /*isSigned=*/false,
+ /*isSaturated=*/false,
+ /*hasUnsignedPadding=*/false);
+}
+
+FixedPointSemantics getULAccumSema() {
+  return FixedPointSemantics(/*width=*/64, /*scale=*/32, /*isSigned=*/false,
+ /*isSaturated=*/false,
+ /*hasUnsignedPadding=*/false);
+}
+
+FixedPointSemantics getUSFractSema() {
+  return FixedPointSemantics(/*width=*/8, /*scale=*/8, /*isSigned=*/false,
+ /*isSaturated=*/false,
+ /*hasUnsignedPadding=*/false);
+}
+
+FixedPointSemantics getUFractSema() {
+  return FixedPointSemantics(/*width=*/16, /*scale=*/16, /*isSigned=*/false,
+ /*isSaturated=*/false,
+ /*hasUnsignedPadding=*/false);
+}
+
+FixedPointSemantics getULFractSema() {
+  return FixedPointSemantics(/*width=*/32, /*scale=*/32, /*isSigned=*/false,
+ /*isSaturated=*/false,
+ /*hasUnsignedPadding=*/false);
+}
+
+FixedPointSemantics getPadUSAccumSema() {
+  return FixedPointSemantics(/*width=*/16, /*scale=*/7, /*isSigned=*/false,
+ /*isSaturated=*/false,
+ /*hasUnsignedPadding=*/true);
+}
+
+FixedPointSemantics getPadUAccumSema() {
+  return FixedPointSemantics(/*width=*/32, /*scale=*/15, /*isSigned=*/false,
+ /*isSaturated=*/false,
+ /*hasUnsignedPadding=*/true);
+}
+
+FixedPointSemantics getPadULAccumSema() {
+  return FixedPointSemantics(/*width=*/64, /*scale=*/31, /*isSigned=*/false,
+ /*isSaturated=*/false,
+ /*hasUnsignedPadding=*/true);
+}
+
+FixedPointSemantics getPadUSFractSema() {
+  return FixedPointSemantics(/*width=*/8, /*scale=*/7, /*isSigned=*/false,
+ 

[PATCH] D49466: Initial implementation of -fmacro-prefix-mapand -ffile-prefix-map

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: include/clang/Lex/PreprocessorOptions.h:171
+  /// A prefix map for __FILE__ and __BASEFILE__
+  std::map MacroPrefixMap;
+

dankm wrote:
> erichkeane wrote:
> > erichkeane wrote:
> > > It seems this can be StringRefs as well.
> > Did you miss this one?  Or is there a good reason these cannot be 
> > stringrefs?
> I didn't miss it. StringRefs here don't survive. The function that adds them 
> to the map creates temporary strings, that go away once that function ends 
> causing StringRefs to dangle. std::string keeps copies.
Oh! I hadn't realized that getAllArgValues gives a vector.  That is 
actually pretty odd for our codebase.  Looking into it, there is no reason that 
function cannot return a vector of StringRef...

Alright, at one point someone should likely fix that, but that person should 
change this type.


Repository:
  rC Clang

https://reviews.llvm.org/D49466



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

[PATCH] D49526: Updated llvm-proto-fuzzer to execute the compiled code

[Matt Morehouse via Phabricator via cfe-commits]

morehouse added a comment.

You can probably get rid of the llvm-objcopy code and make this a lot simpler 
with something like:

1. Call `getSection()` on the Binary object to get the text section.
2. Read the `sh_offset` and `sh_size` of that section.
3. Copy `sh_size` bytes from the start of the binary buffer + `sh_offset` into 
your executable memory.
4. Run it.


Repository:
  rC Clang

https://reviews.llvm.org/D49526



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

[PATCH] D49549: Change 'clang-test' to 'check-clang' on the hacking webpage

[Arnaud Coomans via Phabricator via cfe-commits]

acoomans created this revision.
acoomans added reviewers: chandlerc, cfe-commits.

The command to run tests was previously changed from 'clang-test' to 
'check-clang'; with 'clang-test' remaining available as a legacy alias (see 
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@159483 
91177308-0d34-0410-b5e6-96231b3b80d8).

This commit changes the www documentation accordingly.


Repository:
  rC Clang

https://reviews.llvm.org/D49549

Files:
  www/hacking.html


Index: www/hacking.html
===
--- www/hacking.html
+++ www/hacking.html
@@ -124,7 +124,7 @@
   about what is being run.
 
   If you built LLVM and Clang using CMake, the test suite can be run
-  with make clang-test from the top-level LLVM directory.
+  with make check-clang from the top-level LLVM directory.
 
   The tests primarily consist of a test runner script running the compiler
   under test on individual test files grouped in the directories under the
@@ -191,8 +191,8 @@
   to CMake explicitly.
 
   The cmake build tool is set up to create Visual Studio project files
-  for running the tests, "clang-test" being the root.  Therefore, to
-  run the test from Visual Studio, right-click the clang-test project
+  for running the tests, "check-clang" being the root.  Therefore, to
+  run the test from Visual Studio, right-click the check-clang project
   and select "Build".
 
   


Index: www/hacking.html
===
--- www/hacking.html
+++ www/hacking.html
@@ -124,7 +124,7 @@
   about what is being run.
 
   If you built LLVM and Clang using CMake, the test suite can be run
-  with make clang-test from the top-level LLVM directory.
+  with make check-clang from the top-level LLVM directory.
 
   The tests primarily consist of a test runner script running the compiler
   under test on individual test files grouped in the directories under the
@@ -191,8 +191,8 @@
   to CMake explicitly.
 
   The cmake build tool is set up to create Visual Studio project files
-  for running the tests, "clang-test" being the root.  Therefore, to
-  run the test from Visual Studio, right-click the clang-test project
+  for running the tests, "check-clang" being the root.  Therefore, to
+  run the test from Visual Studio, right-click the check-clang project
   and select "Build".
 
   
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49553: [analyzer] Rename DanglingInternalBufferChecker to InnerPointerChecker

[Reka Kovacs via Phabricator via cfe-commits]

rnkovacs created this revision.
rnkovacs added reviewers: NoQ, dcoughlin, xazax.hun, george.karpenkov.
Herald added subscribers: mikhail.ramalho, a.sidorin, dkrupp, szepet, 
baloghadamsoftware, whisperity, mgorny.

Also, the `AF_InternalBuffer` allocation family is renamed to `AF_InnerBuffer`.
I'm open to other suggestions.


Repository:
  rC Clang

https://reviews.llvm.org/D49553

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/AllocationState.h
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
  lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  test/Analysis/dangling-internal-buffer.cpp
  test/Analysis/inner-pointer.cpp

Index: test/Analysis/inner-pointer.cpp
===
--- test/Analysis/inner-pointer.cpp
+++ test/Analysis/inner-pointer.cpp
@@ -1,4 +1,4 @@
-//RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.DanglingInternalBuffer %s -analyzer-output=text -verify
+//RUN: %clang_analyze_cc1 -analyzer-checker=alpha.cplusplus.InnerPointer %s -analyzer-output=text -verify
 
 namespace std {
 
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -47,7 +47,7 @@
   AF_CXXNewArray,
   AF_IfNameIndex,
   AF_Alloca,
-  AF_InternalBuffer
+  AF_InnerBuffer
 };
 
 class RefState {
@@ -485,7 +485,7 @@
 (!SPrev || !SPrev->isReleased());
   assert(!IsReleased ||
  (Stmt && (isa(Stmt) || isa(Stmt))) ||
- (!Stmt && S->getAllocationFamily() == AF_InternalBuffer));
+ (!Stmt && S->getAllocationFamily() == AF_InnerBuffer));
   return IsReleased;
 }
 
@@ -1473,7 +1473,7 @@
 case AF_CXXNew: os << "'new'"; return;
 case AF_CXXNewArray: os << "'new[]'"; return;
 case AF_IfNameIndex: os << "'if_nameindex()'"; return;
-case AF_InternalBuffer: os << "container-specific allocator"; return;
+case AF_InnerBuffer: os << "container-specific allocator"; return;
 case AF_Alloca:
 case AF_None: llvm_unreachable("not a deallocation expression");
   }
@@ -1486,7 +1486,7 @@
 case AF_CXXNew: os << "'delete'"; return;
 case AF_CXXNewArray: os << "'delete[]'"; return;
 case AF_IfNameIndex: os << "'if_freenameindex()'"; return;
-case AF_InternalBuffer: os << "container-specific deallocator"; return;
+case AF_InnerBuffer: os << "container-specific deallocator"; return;
 case AF_Alloca:
 case AF_None: llvm_unreachable("suspicious argument");
   }
@@ -1662,8 +1662,8 @@
   }
   case AF_CXXNew:
   case AF_CXXNewArray:
-  // FIXME: Add new CheckKind for AF_InternalBuffer.
-  case AF_InternalBuffer: {
+  // FIXME: Add new CheckKind for AF_InnerBuffer.
+  case AF_InnerBuffer: {
 if (IsALeakCheck) {
   if (ChecksEnabled[CK_NewDeleteLeaksChecker])
 return CK_NewDeleteLeaksChecker;
@@ -1995,8 +1995,8 @@
 R->addVisitor(llvm::make_unique(Sym));
 
 const RefState *RS = C.getState()->get(Sym);
-if (RS->getAllocationFamily() == AF_InternalBuffer)
-  R->addVisitor(allocation_state::getDanglingBufferBRVisitor(Sym));
+if (RS->getAllocationFamily() == AF_InnerBuffer)
+  R->addVisitor(allocation_state::getInnerPointerBRVisitor(Sym));
 
 C.emitReport(std::move(R));
   }
@@ -2870,7 +2870,7 @@
   const Stmt *S = PathDiagnosticLocation::getStmt(N);
   // When dealing with containers, we sometimes want to give a note
   // even if the statement is missing.
-  if (!S && (!RS || RS->getAllocationFamily() != AF_InternalBuffer))
+  if (!S && (!RS || RS->getAllocationFamily() != AF_InnerBuffer))
 return nullptr;
 
   const LocationContext *CurrentLC = N->getLocationContext();
@@ -2903,7 +2903,7 @@
   StackHintGeneratorForSymbol *StackHint = nullptr;
   SmallString<256> Buf;
   llvm::raw_svector_ostream OS(Buf);
-  
+
   if (Mode == Normal) {
 if (isAllocated(RS, RSPrev, S)) {
   Msg = "Memory is allocated";
@@ -2919,7 +2919,7 @@
 case AF_IfNameIndex:
   Msg = "Memory is released";
   break;
-case AF_InternalBuffer: {
+case AF_InnerBuffer: {
   OS << "Inner pointer invalidated by call to ";
   if (N->getLocation().getKind() == ProgramPoint::PostImplicitCallKind) {
 OS << "destructor";
@@ -3011,7 +3011,7 @@
   // Generate the extra diagnostic.
   PathDiagnosticLocation Pos;
   if (!S) {
-assert(RS->getAllocationFamily() == AF_InternalBuffer);
+assert(RS->getAllocationFamily() == AF_InnerBuffer);
 auto PostImplCall = N->getLocation().getAs();
 if (!PostImplCall)
   return nullptr;
@@ -3055,7 +3055,7 @@
 
 ProgramStateRef
 markReleased(ProgramStateRef State, SymbolRef Sym, const Expr *Origin) {
-  AllocationFamily Family = AF_Internal

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Dan McGregor via Phabricator via cfe-commits]

dankm updated this revision to Diff 156295.
dankm retitled this revision from "Initial implementation of 
-fmacro-prefix-mapand -ffile-prefix-map" to "Initial implementation of 
-fmacro-prefix-map and -ffile-prefix-map".
dankm edited the summary of this revision.

Repository:
  rC Clang

https://reviews.llvm.org/D49466

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/Driver/Options.td
  include/clang/Lex/PreprocessorOptions.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Lex/PPMacroExpansion.cpp

Index: lib/Lex/PPMacroExpansion.cpp
===
--- lib/Lex/PPMacroExpansion.cpp
+++ lib/Lex/PPMacroExpansion.cpp
@@ -28,6 +28,7 @@
 #include "clang/Lex/MacroInfo.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorLexer.h"
+#include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Lex/PTHLexer.h"
 #include "clang/Lex/Token.h"
 #include "llvm/ADT/ArrayRef.h"
@@ -1453,6 +1454,16 @@
   return TI.getTriple().getEnvironment() == Env.getEnvironment();
 }
 
+static void remapMacroPath(SmallString<128> &Path,
+   const std::map &MacroPrefixMap) {
+  for (const auto &Entry : MacroPrefixMap)
+if (Path.startswith(Entry.first)) {
+  Path = (Twine(Entry.second) + Path.substr(Entry.first.size())).str();
+	  return;
+	}
+}
+
 /// ExpandBuiltinMacro - If an identifier token is read that is to be expanded
 /// as a builtin macro, handle it and return the next token as 'Tok'.
 void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
@@ -1519,6 +1530,7 @@
 SmallString<128> FN;
 if (PLoc.isValid()) {
   FN += PLoc.getFilename();
+  remapMacroPath(FN, PPOpts->MacroPrefixMap);
   Lexer::Stringify(FN);
   OS << '"' << FN << '"';
 }
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2847,6 +2847,9 @@
   for (const auto *A : Args.filtered(OPT_error_on_deserialized_pch_decl))
 Opts.DeserializedPCHDeclsToErrorOn.insert(A->getValue());
 
+  for (const auto &A : Args.getAllArgValues(OPT_fmacro_prefix_map_EQ))
+Opts.MacroPrefixMap.insert(StringRef(A).split('='));
+
   if (const Arg *A = Args.getLastArg(OPT_preamble_bytes_EQ)) {
 StringRef Value(A->getValue());
 size_t Comma = Value.find(',');
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -606,12 +606,28 @@
 
 /// Add a CC1 and CC1AS option to specify the debug file path prefix map.
 static void addDebugPrefixMapArg(const Driver &D, const ArgList &Args, ArgStringList &CmdArgs) {
-  for (const Arg *A : Args.filtered(options::OPT_fdebug_prefix_map_EQ)) {
+  for (const Arg *A : Args.filtered(options::OPT_ffile_prefix_map_EQ,
+options::OPT_fdebug_prefix_map_EQ)) {
 StringRef Map = A->getValue();
-if (Map.find('=') == StringRef::npos)
-  D.Diag(diag::err_drv_invalid_argument_to_fdebug_prefix_map) << Map;
-else
-  CmdArgs.push_back(Args.MakeArgString("-fdebug-prefix-map=" + Map));
+if (Map.find('=') == StringRef::npos) {
+  D.Diag(diag::err_drv_invalid_argument_to_prefix_map) << Map << "debug";
+  continue;
+}
+CmdArgs.push_back(Args.MakeArgString("-fdebug-prefix-map=" + Map));
+A->claim();
+  }
+}
+
+/// Add a CC1 option to specify the macro file path prefix map.
+static void addMacroPrefixMapArg(const Driver &D, const ArgList &Args, ArgStringList &CmdArgs) {
+  for (const Arg *A : Args.filtered(options::OPT_ffile_prefix_map_EQ,
+options::OPT_fmacro_prefix_map_EQ)) {
+StringRef Map = A->getValue();
+if (Map.find('=') == StringRef::npos) {
+  D.Diag(diag::err_drv_invalid_argument_to_prefix_map) << Map << "macro";
+  continue;
+}
+CmdArgs.push_back(Args.MakeArgString("-fmacro-prefix-map=" + Map));
 A->claim();
   }
 }
@@ -1223,6 +1239,8 @@
 // For IAMCU add special include arguments.
 getToolChain().AddIAMCUIncludeArgs(Args, CmdArgs);
   }
+
+  addMacroPrefixMapArg(D, Args, CmdArgs);
 }
 
 // FIXME: Move to target hook.
Index: include/clang/Lex/PreprocessorOptions.h
===
--- include/clang/Lex/PreprocessorOptions.h
+++ include/clang/Lex/PreprocessorOptions.h
@@ -167,6 +167,9 @@
   /// build it again.
   std::shared_ptr FailedModules;
 
+  /// A prefix map for __FILE__ and __BASEFILE__
+  std::map MacroPrefixMap;
+
 public:
   PreprocessorOptions() : PrecompiledPreambleBytes(0, false) {}
 
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1755,10 +1755,16 @@
   Flags<[CC1Option]>, HelpT

r337470 - [Sema] Add a new warning, -Wmemset-transposed-args

[Erik Pilkington via cfe-commits]

Author: epilk
Date: Thu Jul 19 09:46:15 2018
New Revision: 337470

URL: http://llvm.org/viewvc/llvm-project?rev=337470&view=rev
Log:
[Sema] Add a new warning, -Wmemset-transposed-args

This diagnoses calls to memset that have the second and third arguments
transposed, for example:

  memset(buf, sizeof(buf), 0);

This is done by checking if the third argument is a literal 0, or if the second
is a sizeof expression (and the third isn't). The first check is also done for
calls to bzero.

Differential revision: https://reviews.llvm.org/D49112

Added:
cfe/trunk/test/Sema/transpose-memset.c
Modified:
cfe/trunk/include/clang/Basic/DiagnosticGroups.td
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/SemaChecking.cpp

Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=337470&r1=337469&r2=337470&view=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Thu Jul 19 09:46:15 2018
@@ -443,6 +443,13 @@ def : DiagGroup<"synth">;
 def SizeofArrayArgument : DiagGroup<"sizeof-array-argument">;
 def SizeofArrayDecay : DiagGroup<"sizeof-array-decay">;
 def SizeofPointerMemaccess : DiagGroup<"sizeof-pointer-memaccess">;
+def MemsetTransposedArgs : DiagGroup<"memset-transposed-args">;
+def DynamicClassMemaccess : DiagGroup<"dynamic-class-memaccess">;
+def NonTrivialMemaccess : DiagGroup<"nontrivial-memaccess">;
+def SuspiciousBzero : DiagGroup<"suspicious-bzero">;
+def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
+  [SizeofPointerMemaccess, DynamicClassMemaccess,
+   NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>;
 def StaticInInline : DiagGroup<"static-in-inline">;
 def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
 def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;

Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=337470&r1=337469&r2=337470&view=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Jul 19 09:46:15 
2018
@@ -619,14 +619,14 @@ def warn_cstruct_memaccess : Warning<
   "%select{destination for|source of|first operand of|second operand of}0 this 
"
   "%1 call is a pointer to record %2 that is not trivial to "
   "%select{primitive-default-initialize|primitive-copy}3">,
-  InGroup>;
+  InGroup;
 def note_nontrivial_field : Note<
   "field is non-trivial to %select{copy|default-initialize}0">;
 def warn_dyn_class_memaccess : Warning<
   "%select{destination for|source of|first operand of|second operand of}0 this 
"
   "%1 call is a pointer to %select{|class containing a }2dynamic class %3; "
   "vtable pointer will be %select{overwritten|copied|moved|compared}4">,
-  InGroup>;
+  InGroup;
 def note_bad_memaccess_silence : Note<
   "explicitly cast the pointer to silence this warning">;
 def warn_sizeof_pointer_expr_memaccess : Warning<
@@ -655,7 +655,19 @@ def note_memsize_comparison_paren : Note
   "did you mean to compare the result of %0 instead?">;
 def note_memsize_comparison_cast_silence : Note<
   "explicitly cast the argument to size_t to silence this warning">;
-  
+def warn_suspicious_sizeof_memset : Warning<
+  "%select{'size' argument to memset is '0'|"
+  "setting buffer to a 'sizeof' expression}0"
+  "; did you mean to transpose the last two arguments?">,
+  InGroup;
+def note_suspicious_sizeof_memset_silence : Note<
+  "%select{parenthesize the third argument|"
+  "cast the second argument to 'int'}0 to silence">;
+def warn_suspicious_bzero_size : Warning<"'size' argument to bzero is '0'">,
+  InGroup;
+def note_suspicious_bzero_size_silence : Note<
+  "parenthesize the second argument to silence">;
+
 def warn_strncat_large_size : Warning<
   "the value of the size argument in 'strncat' is too large, might lead to a " 
   "buffer overflow">, InGroup;

Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=337470&r1=337469&r2=337470&view=diff
==
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Jul 19 09:46:15 2018
@@ -8629,24 +8629,26 @@ static const CXXRecordDecl *getContained
   return nullptr;
 }
 
+static const UnaryExprOrTypeTraitExpr *getAsSizeOfExpr(const Expr *E) {
+  if (const auto *Unary = dyn_cast(E))
+if (Unary->getKind() == UETT_SizeOf)
+  return Unary;
+  return nullptr;
+}
+
 /// If E is a sizeof expression, returns its argument expression,
 /// otherwise returns NULL.
 static const E

[PATCH] D49112: [Sema] Implement -Wmemset-transposed-args

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC337470: [Sema] Add a new warning, -Wmemset-transposed-args 
(authored by epilk, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D49112?vs=156178&id=156298#toc

Repository:
  rC Clang

https://reviews.llvm.org/D49112

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaChecking.cpp
  test/Sema/transpose-memset.c

Index: test/Sema/transpose-memset.c
===
--- test/Sema/transpose-memset.c
+++ test/Sema/transpose-memset.c
@@ -0,0 +1,60 @@
+// RUN: %clang_cc1   -Wmemset-transposed-args -verify %s
+// RUN: %clang_cc1 -xc++ -Wmemset-transposed-args -verify %s
+
+#define memset(...) __builtin_memset(__VA_ARGS__)
+#define bzero(x,y) __builtin_memset(x, 0, y)
+#define real_bzero(x,y) __builtin_bzero(x,y)
+
+int array[10];
+int *ptr;
+
+int main() {
+  memset(array, sizeof(array), 0); // expected-warning{{'size' argument to memset is '0'; did you mean to transpose the last two arguments?}} expected-note{{parenthesize the third argument to silence}}
+  memset(array, sizeof(array), 0xff); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+  memset(ptr, sizeof(ptr), 0); // expected-warning{{'size' argument to memset is '0'; did you mean to transpose the last two arguments?}} expected-note{{parenthesize the third argument to silence}}
+  memset(ptr, sizeof(*ptr) * 10, 1); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+  memset(ptr, 10 * sizeof(int *), 1); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+  memset(ptr, 10 * sizeof(int *) + 10, 0xff); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+  memset(ptr, sizeof(char) * sizeof(int *), 0xff); // expected-warning{{setting buffer to a 'sizeof' expression; did you mean to transpose the last two arguments?}} expected-note{{cast the second argument to 'int' to silence}}
+  memset(array, sizeof(array), sizeof(array)); // Uh... fine I guess.
+  memset(array, 0, sizeof(array));
+  memset(ptr, 0, sizeof(int *) * 10);
+  memset(array, (int)sizeof(array), (0)); // no warning
+  memset(array, (int)sizeof(array), 32); // no warning
+  memset(array, 32, (0)); // no warning
+
+  bzero(ptr, 0); // expected-warning{{'size' argument to bzero is '0'}} expected-note{{parenthesize the second argument to silence}}
+  real_bzero(ptr, 0); // expected-warning{{'size' argument to bzero is '0'}} expected-note{{parenthesize the second argument to silence}}
+}
+
+void macros() {
+#define ZERO 0
+  int array[10];
+  memset(array, 0xff, ZERO); // no warning
+  // Still emit a diagnostic for memsetting a sizeof expression:
+  memset(array, sizeof(array), ZERO); // expected-warning{{'sizeof'}} expected-note{{cast}}
+  bzero(array, ZERO); // no warning
+  real_bzero(array, ZERO); // no warning
+#define NESTED_DONT_DIAG\
+  memset(array, 0xff, ZERO);\
+  real_bzero(array, ZERO);
+
+  NESTED_DONT_DIAG;
+
+#define NESTED_DO_DIAG  \
+  memset(array, 0xff, 0);   \
+  real_bzero(array, 0)
+
+  NESTED_DO_DIAG; // expected-warning{{'size' argument to memset}} expected-warning{{'size' argument to bzero}} expected-note2{{parenthesize}}
+
+#define FN_MACRO(p) \
+  memset(array, 0xff, p)
+
+  FN_MACRO(ZERO);
+  FN_MACRO(0); // FIXME: should we diagnose this?
+
+  __builtin_memset(array, 0, ZERO); // no warning
+  __builtin_bzero(array, ZERO);
+  __builtin_memset(array, 0, 0); // expected-warning{{'size' argument to memset}} // expected-note{{parenthesize}}
+  __builtin_bzero(array, 0); // expected-warning{{'size' argument to bzero}} // expected-note{{parenthesize}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -8629,24 +8629,26 @@
   return nullptr;
 }
 
+static const UnaryExprOrTypeTraitExpr *getAsSizeOfExpr(const Expr *E) {
+  if (const auto *Unary = dyn_cast(E))
+if (Unary->getKind() == UETT_SizeOf)
+  return Unary;
+  return nullptr;
+}
+
 /// If E is a sizeof expression, returns its argument expression,
 /// otherwise returns NULL.
 static const Expr *getSizeOfExprArg(const Expr *E) {
-  if (const UnaryExprOrTypeTraitExpr *SizeOf =
-  dyn_cast(E))
-if (SizeOf->getKind() == UETT_SizeOf && !SizeOf->isArgumentType())
+  if (con

[PATCH] D49536: [Analyzer] Quick Fix for exponential execution time when simpilifying complex additive expressions

[Artem Dergachev via Phabricator via cfe-commits]

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

I was thinking of two flags, but that'll work too.


https://reviews.llvm.org/D49536



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

r337472 - [analyzer] Memoize complexity of SymExpr

[Mikhail R. Gadelha via cfe-commits]

Author: mramalho
Date: Thu Jul 19 10:03:12 2018
New Revision: 337472

URL: http://llvm.org/viewvc/llvm-project?rev=337472&view=rev
Log:
[analyzer] Memoize complexity of SymExpr

Summary:
This patch introduces a new member to SymExpr, which stores the symbol 
complexity, avoiding recalculating it every time computeComplexity() is called.

Also, increase the complexity of conjured Symbols by one, so it's clear that it 
has a greater complexity than its underlying symbols.

Reviewers: NoQ, george.karpenkov

Reviewed By: NoQ, george.karpenkov

Subscribers: xazax.hun, szepet, a.sidorin

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

Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
cfe/trunk/lib/StaticAnalyzer/Core/SymbolManager.cpp

Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h?rev=337472&r1=337471&r2=337472&view=diff
==
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h 
(original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h Thu Jul 
19 10:03:12 2018
@@ -49,6 +49,8 @@ protected:
 return !T.isNull() && !T->isVoidType();
   }
 
+  mutable unsigned Complexity = 0;
+
 public:
   virtual ~SymExpr() = default;
 
@@ -85,7 +87,7 @@ public:
   symbol_iterator symbol_begin() const { return symbol_iterator(this); }
   static symbol_iterator symbol_end() { return symbol_iterator(); }
 
-  unsigned computeComplexity() const;
+  virtual unsigned computeComplexity() const = 0;
 
   /// Find the region from which this symbol originates.
   ///
@@ -127,6 +129,10 @@ public:
 
   SymbolID getSymbolID() const { return Sym; }
 
+  unsigned computeComplexity() const override {
+return 1;
+  };
+
   // Implement isa support.
   static inline bool classof(const SymExpr *SE) {
 Kind k = SE->getKind();

Modified: 
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h?rev=337472&r1=337471&r2=337472&view=diff
==
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h 
(original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h 
Thu Jul 19 10:03:12 2018
@@ -270,6 +270,12 @@ public:
 // Otherwise, 'To' should also be a valid type.
   }
 
+  unsigned computeComplexity() const override {
+if (Complexity == 0)
+  Complexity = 1 + Operand->computeComplexity();
+return Complexity;
+  }
+
   QualType getType() const override { return ToTy; }
 
   const SymExpr *getOperand() const { return Operand; }
@@ -337,6 +343,12 @@ public:
   const SymExpr *getLHS() const { return LHS; }
   const llvm::APSInt &getRHS() const { return RHS; }
 
+  unsigned computeComplexity() const override {
+if (Complexity == 0)
+  Complexity = 1 + LHS->computeComplexity();
+return Complexity;
+  }
+
   static void Profile(llvm::FoldingSetNodeID& ID, const SymExpr *lhs,
   BinaryOperator::Opcode op, const llvm::APSInt& rhs,
   QualType t) {
@@ -374,6 +386,12 @@ public:
   const SymExpr *getRHS() const { return RHS; }
   const llvm::APSInt &getLHS() const { return LHS; }
 
+  unsigned computeComplexity() const override {
+if (Complexity == 0)
+  Complexity = 1 + RHS->computeComplexity();
+return Complexity;
+  }
+
   static void Profile(llvm::FoldingSetNodeID& ID, const llvm::APSInt& lhs,
   BinaryOperator::Opcode op, const SymExpr *rhs,
   QualType t) {
@@ -412,6 +430,12 @@ public:
 
   void dumpToStream(raw_ostream &os) const override;
 
+  unsigned computeComplexity() const override {
+if (Complexity == 0)
+  Complexity = RHS->computeComplexity() + LHS->computeComplexity();
+return Complexity;
+  }
+
   static void Profile(llvm::FoldingSetNodeID& ID, const SymExpr *lhs,
 BinaryOperator::Opcode op, const SymExpr *rhs, QualType t) 
{
 ID.AddInteger((unsigned) SymSymExprKind);

Modified: cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp?rev=337472&r1=337471&r2=337472&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp Thu Jul 19 10:03:12 
2018
@@ -390,7 +390,7 @@ unsigned AnalyzerOptions::getGraphTrimIn
 
 unsigned AnalyzerOptions::getMaxSymbolComplexit

[PATCH] D49232: [analyzer] Memoize complexity of SymExpr

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC337472: [analyzer] Memoize complexity of SymExpr (authored 
by mramalho, committed by ).
Herald added a subscriber: cfe-commits.

Repository:
  rC Clang

https://reviews.llvm.org/D49232

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h
  include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  lib/StaticAnalyzer/Core/SymbolManager.cpp

Index: lib/StaticAnalyzer/Core/SymbolManager.cpp
===
--- lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -159,13 +159,6 @@
   llvm_unreachable("unhandled expansion case");
 }
 
-unsigned SymExpr::computeComplexity() const {
-  unsigned R = 0;
-  for (symbol_iterator I = symbol_begin(), E = symbol_end(); I != E; ++I)
-R++;
-  return R;
-}
-
 const SymbolRegionValue*
 SymbolManager::getRegionValueSymbol(const TypedValueRegion* R) {
   llvm::FoldingSetNodeID profile;
Index: lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
===
--- lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
+++ lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
@@ -390,7 +390,7 @@
 
 unsigned AnalyzerOptions::getMaxSymbolComplexity() {
   if (!MaxSymbolComplexity.hasValue())
-MaxSymbolComplexity = getOptionAsInteger("max-symbol-complexity", 25);
+MaxSymbolComplexity = getOptionAsInteger("max-symbol-complexity", 35);
   return MaxSymbolComplexity.getValue();
 }
 
Index: include/clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h
@@ -49,6 +49,8 @@
 return !T.isNull() && !T->isVoidType();
   }
 
+  mutable unsigned Complexity = 0;
+
 public:
   virtual ~SymExpr() = default;
 
@@ -85,7 +87,7 @@
   symbol_iterator symbol_begin() const { return symbol_iterator(this); }
   static symbol_iterator symbol_end() { return symbol_iterator(); }
 
-  unsigned computeComplexity() const;
+  virtual unsigned computeComplexity() const = 0;
 
   /// Find the region from which this symbol originates.
   ///
@@ -127,6 +129,10 @@
 
   SymbolID getSymbolID() const { return Sym; }
 
+  unsigned computeComplexity() const override {
+return 1;
+  };
+
   // Implement isa support.
   static inline bool classof(const SymExpr *SE) {
 Kind k = SE->getKind();
Index: include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
@@ -270,6 +270,12 @@
 // Otherwise, 'To' should also be a valid type.
   }
 
+  unsigned computeComplexity() const override {
+if (Complexity == 0)
+  Complexity = 1 + Operand->computeComplexity();
+return Complexity;
+  }
+
   QualType getType() const override { return ToTy; }
 
   const SymExpr *getOperand() const { return Operand; }
@@ -337,6 +343,12 @@
   const SymExpr *getLHS() const { return LHS; }
   const llvm::APSInt &getRHS() const { return RHS; }
 
+  unsigned computeComplexity() const override {
+if (Complexity == 0)
+  Complexity = 1 + LHS->computeComplexity();
+return Complexity;
+  }
+
   static void Profile(llvm::FoldingSetNodeID& ID, const SymExpr *lhs,
   BinaryOperator::Opcode op, const llvm::APSInt& rhs,
   QualType t) {
@@ -374,6 +386,12 @@
   const SymExpr *getRHS() const { return RHS; }
   const llvm::APSInt &getLHS() const { return LHS; }
 
+  unsigned computeComplexity() const override {
+if (Complexity == 0)
+  Complexity = 1 + RHS->computeComplexity();
+return Complexity;
+  }
+
   static void Profile(llvm::FoldingSetNodeID& ID, const llvm::APSInt& lhs,
   BinaryOperator::Opcode op, const SymExpr *rhs,
   QualType t) {
@@ -412,6 +430,12 @@
 
   void dumpToStream(raw_ostream &os) const override;
 
+  unsigned computeComplexity() const override {
+if (Complexity == 0)
+  Complexity = RHS->computeComplexity() + LHS->computeComplexity();
+return Complexity;
+  }
+
   static void Profile(llvm::FoldingSetNodeID& ID, const SymExpr *lhs,
 BinaryOperator::Opcode op, const SymExpr *rhs, QualType t) {
 ID.AddInteger((unsigned) SymSymExprKind);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r337473 - Fix unused variable warning.

[Erich Keane via cfe-commits]

Author: erichkeane
Date: Thu Jul 19 10:19:16 2018
New Revision: 337473

URL: http://llvm.org/viewvc/llvm-project?rev=337473&view=rev
Log:
Fix unused variable warning.

Modified:
cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp

Modified: cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp?rev=337473&r1=337472&r2=337473&view=diff
==
--- cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp Thu Jul 19 10:19:16 2018
@@ -6932,7 +6932,7 @@ private:
 bool IsExpressionFirstInfo = true;
 Address BP = Address::invalid();
 
-if (const auto *ME = dyn_cast(I->getAssociatedExpression())) {
+if (isa(I->getAssociatedExpression())) {
   // The base is the 'this' pointer. The content of the pointer is going
   // to be the base of the field being mapped.
   BP = CGF.LoadCXXThisAddress();


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

[PATCH] D49085: [Sema] Emit a diagnostic for an invalid dependent function template specialization

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington updated this revision to Diff 156317.
erik.pilkington added a comment.

Improve the diagnostics. Thanks!


https://reviews.llvm.org/D49085

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/CXX/temp/temp.decls/temp.friend/p1.cpp


Index: clang/test/CXX/temp/temp.decls/temp.friend/p1.cpp
===
--- clang/test/CXX/temp/temp.decls/temp.friend/p1.cpp
+++ clang/test/CXX/temp/temp.decls/temp.friend/p1.cpp
@@ -359,3 +359,20 @@
   template void f2(X *);
   template void f2(X *); // expected-note{{in instantiation of 
function template specialization 'PR10913::f2' requested here}}
 }
+
+namespace test16 {
+template  struct foo {}; // expected-note{{candidate template 
ignored: template is not a function template}}
+template  class A {
+  friend void foo(); // expected-error{{no candidate function template was 
found for dependent friend function template specialization}}
+};
+}
+
+namespace test17 {
+namespace ns {
+template  void foo() {} // expected-note{{candidate template ignored: 
is not a member of the enclosing namespace}}
+}
+using ns::foo;
+template  struct A {
+  friend void foo() {} // expected-error{{no candidate function template 
was found for dependent friend function template specialization}}
+};
+}
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -8008,17 +8008,34 @@
   // the correct context.
   DeclContext *FDLookupContext = FD->getDeclContext()->getRedeclContext();
   LookupResult::Filter F = Previous.makeFilter();
+  enum DiscardReason { NotAFunctionTemplate, NotAMemberOfEnclosing };
+  SmallVector, 8> DiscardedCandidates;
   while (F.hasNext()) {
 NamedDecl *D = F.next()->getUnderlyingDecl();
-if (!isa(D) ||
-!FDLookupContext->InEnclosingNamespaceSetOf(
-  D->getDeclContext()->getRedeclContext()))
+if (!isa(D)) {
   F.erase();
+  DiscardedCandidates.push_back(std::make_pair(NotAFunctionTemplate, D));
+  continue;
+}
+
+if (!FDLookupContext->InEnclosingNamespaceSetOf(
+D->getDeclContext()->getRedeclContext())) {
+  F.erase();
+  DiscardedCandidates.push_back(std::make_pair(NotAMemberOfEnclosing, D));
+  continue;
+}
   }
   F.done();
 
-  // Should this be diagnosed here?
-  if (Previous.empty()) return true;
+  if (Previous.empty()) {
+Diag(FD->getLocation(),
+ diag::err_dependent_function_template_spec_no_match);
+for (auto &P : DiscardedCandidates)
+  Diag(P.second->getLocation(),
+   diag::note_dependent_function_template_spec_discard_reason)
+  << P.first;
+return true;
+  }
 
   FD->setDependentTemplateSpecialization(Context, Previous.asUnresolvedSet(),
  ExplicitTemplateArgs);
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4103,6 +4103,12 @@
 def err_explicit_specialization_inconsistent_storage_class : Error<
   "explicit specialization has extraneous, inconsistent storage class "
   "'%select{none|extern|static|__private_extern__|auto|register}0'">;
+def err_dependent_function_template_spec_no_match : Error<
+  "no candidate function template was found for dependent"
+  " friend function template specialization">;
+def note_dependent_function_template_spec_discard_reason : Note<
+  "candidate template ignored: %select{template is not a function template"
+  "|is not a member of the enclosing namespace}0">;
 
 // C++ class template specializations and out-of-line definitions
 def err_template_spec_needs_header : Error<


Index: clang/test/CXX/temp/temp.decls/temp.friend/p1.cpp
===
--- clang/test/CXX/temp/temp.decls/temp.friend/p1.cpp
+++ clang/test/CXX/temp/temp.decls/temp.friend/p1.cpp
@@ -359,3 +359,20 @@
   template void f2(X *);
   template void f2(X *); // expected-note{{in instantiation of function template specialization 'PR10913::f2' requested here}}
 }
+
+namespace test16 {
+template  struct foo {}; // expected-note{{candidate template ignored: template is not a function template}}
+template  class A {
+  friend void foo(); // expected-error{{no candidate function template was found for dependent friend function template specialization}}
+};
+}
+
+namespace test17 {
+namespace ns {
+template  void foo() {} // expected-note{{candidate template ignored: is not a member of the enclosing namespace}}
+}
+using ns::foo;
+template  struct A {
+  friend void foo() {} // expected-error{{no candidate function template was found for dependent friend function template specialization}}
+};
+}
Index: clang/lib/Sem

r337474 - [analyzer] Fix memory sanitizer error in MallocChecker.

[Reka Kovacs via cfe-commits]

Author: rkovacs
Date: Thu Jul 19 10:43:09 2018
New Revision: 337474

URL: http://llvm.org/viewvc/llvm-project?rev=337474&view=rev
Log:
[analyzer] Fix memory sanitizer error in MallocChecker.

StringRef's data() returns a string that may be non-null-terminated.
Switch to using StringRefs from const char pointers in visitor notes
to avoid problems.

Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=337474&r1=337473&r2=337474&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Thu Jul 19 10:43:09 
2018
@@ -2899,7 +2899,7 @@ std::shared_ptr Mal
   // (__attribute__((cleanup))).
 
   // Find out if this is an interesting point and what is the kind.
-  const char *Msg = nullptr;
+  StringRef Msg;
   StackHintGeneratorForSymbol *StackHint = nullptr;
   SmallString<256> Buf;
   llvm::raw_svector_ostream OS(Buf);
@@ -2933,7 +2933,7 @@ std::shared_ptr Mal
 }
 OS << "'";
   }
-  Msg = OS.str().data();
+  Msg = OS.str();
   break;
 }
 case AF_None:
@@ -3004,7 +3004,7 @@ std::shared_ptr Mal
 }
   }
 
-  if (!Msg)
+  if (Msg.empty())
 return nullptr;
   assert(StackHint);
 


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

[PATCH] D49509: [libc++] Allow running ABI list tests with different ABI versions

[Eric Fiselier via Phabricator via cfe-commits]

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

LGTM. Though it might be worth noting that we don't really have an "ABI v2" 
yet; we're just staging it ATM.


Repository:
  rL LLVM

https://reviews.llvm.org/D49509



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

[PATCH] D49509: [libc++] Allow running ABI list tests with different ABI versions

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL337477: [libc++] Allow running ABI list tests with different 
ABI versions (authored by ldionne, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D49509?vs=156148&id=156323#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D49509

Files:
  libcxx/trunk/lib/abi/CMakeLists.txt
  libcxx/trunk/lib/abi/x86_64-apple-darwin.abilist
  libcxx/trunk/lib/abi/x86_64-apple-darwin.v1.abilist
  libcxx/trunk/lib/abi/x86_64-apple-darwin.v2.abilist
  libcxx/trunk/lib/abi/x86_64-unknown-linux-gnu.abilist
  libcxx/trunk/lib/abi/x86_64-unknown-linux-gnu.v1.abilist



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

Patch review please: D48862

[Grang, Mandeep Singh via cfe-commits]

Gentle reminder to the reviewers to please review my patch: 
https://reviews.llvm.org/D48862 [Fix lib paths for OpenEmbedded targets].
I wasn't sure who to add as reviewer for OpenEmbedded targets. So 
apologies if you are not the correct person to review this.

Please feel free to add the correct reviewers.

-Mandeep

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

[PATCH] D31696: Automatically add include-what-you-use for when building in tree

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.
Herald added a reviewer: javed.absar.

This can be closed, IWYU no longer officially supports in-tree builds.


https://reviews.llvm.org/D31696



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

[PATCH] D32696: More detailed docs for UsingShadowDecl

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.
Herald added a subscriber: llvm-commits.

Ping!


Repository:
  rL LLVM

https://reviews.llvm.org/D32696



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

[PATCH] D49085: [Sema] Emit a diagnostic for an invalid dependent function template specialization

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:4111
+  "candidate template ignored: %select{template is not a function template"
+  "|is not a member of the enclosing namespace}0">;
 

Your first explanation has a subject, but the second doesn't.  And I think it 
would be nice to suggest adding explicit scope qualification in the second case.

I assume non-templates have previously been filtered out?


https://reviews.llvm.org/D49085



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

[PATCH] D49486: [cfe][CMake] Export the clang resource directory

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

Thank you for doing this, I'm going to guess you have IWYU in mind :-)

So as consumers of this, how do you envision we use the new variable? Something 
like https://stackoverflow.com/a/13429998 to copy the resource dir into our 
build root, and then an install rule to put it where we want it?


https://reviews.llvm.org/D49486



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

[PATCH] D49083: [HIP] Register/unregister device fat binary only once

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/CodeGen/CGCUDANV.cpp:444
+auto HandleValue =
+CtorBuilder.CreateAlignedLoad(GpuBinaryHandle, CGM.getPointerAlign());
+llvm::Constant *Zero = 
llvm::Constant::getNullValue(HandleValue->getType());

yaxunl wrote:
> rjmccall wrote:
> > yaxunl wrote:
> > > rjmccall wrote:
> > > > yaxunl wrote:
> > > > > rjmccall wrote:
> > > > > > Do you not need to worry about concurrency here?
> > > > > The ctor functions are executed by the dynamic loader before the 
> > > > > program gains the control. The dynamic loader cannot excute the ctor 
> > > > > functions concurrently since doing that would not gurantee thread 
> > > > > safety of the loaded program. Therefore we can assume sequential 
> > > > > execution of ctor functions here.
> > > > Okay.  That's worth a comment.
> > > > 
> > > > Is the name here specified by some ABI document, or is it just a 
> > > > conventional name that we're picking now?
> > > Will add a comment for that.
> > > 
> > > You mean `__hip_gpubin_handle`? It is an implementation detail. It is not 
> > > defined by ABI or other documentation. Since it is only used internally 
> > > by ctor functions, it is not a visible elf symbol. Its name is by 
> > > convention since the cuda corresponding one was named 
> > > __cuda_gpubin_handle.
> > Well, it *is* ABI, right?  It's necessary for all translation units to 
> > agree to use the same symbol here or else the registration will happen 
> > multiple times.
> Right. I created a pull request for HIP to document this 
> https://github.com/ROCm-Developer-Tools/HIP/pull/580/files
Okay.  Please leave a comment here explaining that this variable's name, size, 
and initialization pattern are part of the HIP ABI, then.



Comment at: lib/CodeGen/CGCUDANV.cpp:448
+auto HandleValue =
+CtorBuilder.CreateAlignedLoad(GpuBinaryHandle, CGM.getPointerAlign());
+llvm::Constant *Zero = 
llvm::Constant::getNullValue(HandleValue->getType());

Should you just make `GpuBinaryHandle` an `Address` so that you don't have to 
repeat the alignment assumption over and over?

Also, you should set an alignment on the variable itself.



Comment at: lib/CodeGen/CGCUDANV.cpp:452
+CtorBuilder.CreateCondBr(EQZero, IfBlock, ExitBlock);
+CtorBuilder.SetInsertPoint(IfBlock);
+// GpuBinaryHandle = __hipRegisterFatBinary(&FatbinWrapper);

When I'm generating control flow like this, I find it helpful to at least use 
vertical spacing to separate the blocks, and sometimes I even put all the code 
within a block in a brace-statement (`{ ... }`) to more clearly scope the 
block-local values within the block.


https://reviews.llvm.org/D49083



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

[PATCH] D49508: [Sema] Expr::skipRValueSubobjectAdjustments(): record skipped NoOp casts.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Hmm.  I think the approach of flagging ICEs that are semantically part of an 
explicit cast is probably a better representation for tools across the board.

If we *are* going to do it this way, though, I think you should (1) make the 
collection of skipped expressions optional and (2) collect *all* the skipped 
expressions, not just no-op casts.


Repository:
  rC Clang

https://reviews.llvm.org/D49508



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

[PATCH] D49294: Sema: Fix explicit address space cast in C++

[Yaxun Liu via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaOverload.cpp:3150
+  !getLangOpts().OpenCLCPlusPlus)
+return false;
+

rjmccall wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > It's not really OpenCL C++ that's special here, it's the possibility of 
> > > promotions between address spaces.
> > For OpenCL C++, there is language rule about address space promotion.
> > 
> > For other languages, there is no generic rule about adderss space 
> > promotion, since the meaning of address space and their relations are 
> > target dependent. Do we want to introduce a target hook 
> > Target::canPromote(AddressSpace Src, AddressSpace Dest, LanguageOptions 
> > LangOpts) to represent this? Or do we just assume a simple rule, that is, 
> > all address space can be promoted to default address space 0, otherwise it 
> > is not allowed?
> A target-specific hook for handling target-specific address spaces makes 
> sense to me.  I don't think there's a generic rule allowing promotion into 
> the default AS.
I checked the current logic about address space promotions. There are several 
functions Type::Qualifier::isAddressSpaceSupersetOf, 
Type:;Qualifier::compatiblyIncludes, PointerType::isAddressSpaceOverlapping 
which checks address space compatibility. However they are only based on 
language rules and do not depend on targets. Since only OpenCL defined language 
rules regarding address spaces, address space promotion is only allowed for 
OpenCL.

To allow target specific address space promotion, these functions need an extra 
ASTContext parameter to allow them access LanguageOptions and TargetInfo. I am 
not sure if this makes things overly complicated.

I would expect target specific address space casts in C++ to be rare cases 
since it is not part of language standard. In most cases users would just use 
generic pointer. When they do use explicit address space cast, I expect they 
understand what they are doing. Then clang just do not do any target-specific 
promotion and treat it as reinterpret cast. Promotion is only allowed by 
language rules e.g. those defined by OpenCL.



https://reviews.llvm.org/D49294



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

[PATCH] D49508: [Sema] Expr::skipRValueSubobjectAdjustments(): record skipped NoOp casts.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D49508#1168584, @rjmccall wrote:

> Hmm.  I think the approach of flagging ICEs that are semantically part of an 
> explicit cast is probably a better representation for tools across the board.


I could do that, but i couldn't find where it should be done.
Where are these "ICEs that are semantically part of an explicit cast" created? 
Where would we mark them?

> If we *are* going to do it this way, though, I think you should (1) make the 
> collection of skipped expressions optional and (2) collect *all* the skipped 
> expressions, not just no-op casts.

1. I was wondering about that, will do.
2. Well, i could do that, but i would need to filter them afterwards in my 
use-case. So i wonder - what is the motivation for that? Nothing else needs 
that additional knowledge right now.


Repository:
  rC Clang

https://reviews.llvm.org/D49508



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

[PATCH] D49303: [CodeGen][ObjC] Treat non-escaping blocks as global blocks to make copy/dispose a no-op

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 156332.
ahatanak added a comment.

Fix a bug where the capture cleanups weren't pushed when 
BlockDecl::doesNotEscape() returns true.

Test that, when ARC is enabled, captures are retained and copied into the stack 
block object and destroyed when the stack block object goes out of scope.


Repository:
  rC Clang

https://reviews.llvm.org/D49303

Files:
  docs/Block-ABI-Apple.rst
  lib/CodeGen/CGBlocks.cpp
  lib/CodeGen/CGBlocks.h
  test/CodeGenObjC/noescape.m

Index: test/CodeGenObjC/noescape.m
===
--- test/CodeGenObjC/noescape.m
+++ test/CodeGenObjC/noescape.m
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin -fblocks -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -fblocks -emit-llvm -o - %s | FileCheck -check-prefix CHECK -check-prefix CHECK-NOARC %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -fblocks -emit-llvm -fobjc-arc -o - %s | FileCheck -check-prefix CHECK -check-prefix CHECK-ARC %s
 
 typedef void (^BlockTy)(void);
 
@@ -12,6 +13,12 @@
 void noescapeFunc2(__attribute__((noescape)) id);
 void noescapeFunc3(__attribute__((noescape)) union U);
 
+// Block descriptors of non-escaping blocks don't need pointers to copy/dispose
+// helper functions.
+
+// CHECK: %[[STRUCT_BLOCK_DESCRIPTOR:.*]] = type { i64, i64 }
+// CHECK: @[[BLOCK_DESCIPTOR_TMP_2:.*]] = internal constant { i64, i64, i8*, i64 } { i64 0, i64 40, i8* getelementptr inbounds ([6 x i8], [6 x i8]* @{{.*}}, i32 0, i32 0), i64 256 }, align 8
+
 // CHECK-LABEL: define void @test0(
 // CHECK: call void @noescapeFunc0({{.*}}, {{.*}} nocapture {{.*}})
 // CHECK: declare void @noescapeFunc0(i8*, {{.*}} nocapture)
@@ -69,3 +76,41 @@
   ^(int *__attribute__((noescape)) p0){}(p);
   b(p);
 }
+
+// If the block is non-escaping, set the BLOCK_IS_NOESCAPE and BLOCK_IS_GLOBAL
+// bits of field 'flags' and set the 'isa' field to 'NSConcreteGlobalBlock'.
+
+// CHECK: define void @test6(i8* %{{.*}}, i8* %[[B:.*]])
+// CHECK: %{{.*}} = alloca i8*, align 8
+// CHECK: %[[B_ADDR:.*]] = alloca i8*, align 8
+// CHECK: %[[BLOCK:.*]] = alloca <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>, align 8
+// CHECK-NOARC: store i8* %[[B]], i8** %[[B_ADDR]], align 8
+// CHECK-ARC: store i8* null, i8** %[[B_ADDR]], align 8
+// CHECK-ARC: call void @objc_storeStrong(i8** %[[B_ADDR]], i8* %[[B]])
+// CHECK-ARC: %[[V0:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>* %[[BLOCK]], i32 0, i32 5
+// CHECK: %[[BLOCK_ISA:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>* %[[BLOCK]], i32 0, i32 0
+// CHECK: store i8* bitcast (i8** @_NSConcreteGlobalBlock to i8*), i8** %[[BLOCK_ISA]], align 8
+// CHECK: %[[BLOCK_FLAGS:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>* %[[BLOCK]], i32 0, i32 1
+// CHECK: store i32 -796917760, i32* %[[BLOCK_FLAGS]], align 8
+// CHECK: %[[BLOCK_DESCRIPTOR:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>* %[[BLOCK]], i32 0, i32 4
+// CHECK: store %[[STRUCT_BLOCK_DESCRIPTOR]]* bitcast ({ i64, i64, i8*, i64 }* @[[BLOCK_DESCIPTOR_TMP_2]] to %[[STRUCT_BLOCK_DESCRIPTOR]]*), %[[STRUCT_BLOCK_DESCRIPTOR]]** %[[BLOCK_DESCRIPTOR]], align 8
+// CHECK: %[[BLOCK_CAPTURED:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8* }>* %[[BLOCK]], i32 0, i32 5
+// CHECK-NOARC: %[[V1:.*]] = load i8*, i8** %[[B_ADDR]], align 8
+// CHECK-NOARC: store i8* %[[V1]], i8** %[[BLOCK_CAPTURED]], align 8
+// CHECK-ARC: %[[V2:.*]] = load i8*, i8** %[[B_ADDR]], align 8
+// CHECK-ARC: %[[V3:.*]] = call i8* @objc_retain(i8* %[[V2]]) #3
+// CHECK-ARC: store i8* %[[V3]], i8** %[[BLOCK_CAPTURED]], align 8
+// CHECK: call void @noescapeFunc0(
+// CHECK-ARC: call void @objc_storeStrong(i8** %[[V0]], i8* null)
+// CHECK-ARC: call void @objc_storeStrong(i8** %[[B_ADDR]], i8* null)
+
+// Non-escaping blocks don't need copy/dispose helper functions.
+
+// CHECK-NOT: define internal void @__copy_helper_block_
+// CHECK-NOT: define internal void @__destroy_helper_block_
+
+void func(id);
+
+void test6(id a, id b) {
+  noescapeFunc0(a, ^{ func(b); });
+}
Index: lib/CodeGen/CGBlocks.h
===
--- lib/CodeGen/CGBlocks.h
+++ lib/CodeGen/CGBlocks.h
@@ -54,6 +54,7 @@
 };
 
 enum BlockLiteralFlags {
+  BLOCK_IS_NOESCAPE  =  (1 << 23),
   BLOCK_HAS_COPY_DISPOSE =  (1 << 25),
   BLOCK_HAS_CXX_OBJ =   (1 << 26),
   BLOCK_IS_GLOBAL = (1 << 28),
@@ -214,7 +215,8 @@
   ///

[PATCH] D49476: [Index] Set OrigD before D is changed.

[Argyrios Kyrtzidis via Phabricator via cfe-commits]

akyrtzi added a comment.

`CXIndexDataConsumer.cpp` uses `ASTNode.OrigD`, that code is exercised when you 
run `c-index-test -index-file <...>`. I'd recommend to at least check if that 
command would produce a different output for the test case that you detected 
originally.


Repository:
  rC Clang

https://reviews.llvm.org/D49476



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

Re: r336467 - [OPENMP] Fix PR38026: Link -latomic when -fopenmp is used.

[Hal Finkel via cfe-commits]

On 07/19/2018 09:01 AM, Jonas Hahnfeld wrote:
> On 2018-07-19 15:43, Hal Finkel wrote:
>> On 07/16/2018 01:19 PM, Jonas Hahnfeld wrote:
>>> [ Moving discussion from https://reviews.llvm.org/D49386 to the
>>> relevant comment on cfe-commits, CC'ing Hal who commented on the
>>> original issue ]
>>>
>>> Is this change really a good idea? It always requires libatomic for
>>> all OpenMP applications, even if there is no 'omp atomic' directive or
>>> all of them can be lowered to atomic instructions that don't require a
>>> runtime library. I'd argue that it's a larger restriction than the
>>> problem it solves.
>>
>> Can you please elaborate on why you feel that this is problematic?
>
> The linked patch deals with the case that there is no libatomic,
> effectively disabling all tests of the OpenMP runtime (even though
> only few of them require atomic instructions). So apparently there are
> Linux systems without libatomic. Taking them any chance to use OpenMP
> with Clang is a large regression IMO and not user-friendly either.

If there's a significant population of such systems, then this certainly
seems like a problem.

Let's revert this for now while we figure out what to do (which might
just mean updating the documentation to include OpenMP where we talk
about atomics).

>
>>> Per https://clang.llvm.org/docs/Toolchain.html#libatomic-gnu the user
>>> is expected to manually link -latomic whenever Clang can't lower
>>> atomic instructions - including C11 atomics and C++ atomics. In my
>>> opinion OpenMP is just another abstraction that doesn't require a
>>> special treatment.
>>
>> From my perspective, because we instruct our users that all you need to
>> do in order to enable OpenMP is pass -fopenmp flags during compiling and
>> linking. The user should not need to know or care about how atomics are
>> implemented.
>>
>> It's not clear to me that our behavior for C++ atomics is good either.
>> From the documentation, it looks like the rationale is to avoid choosing
>> between the GNU libatomic implementation and the compiler-rt
>> implementation? We should probably make a default choice and provide a
>> flag to override. That would seem more user-friendly to me.
>
> I didn't mean to say it's a good default, but OpenMP is now different
> from C and C++. And as you said, the choice was probably made for a
> reason, so there should be some discussion whether to change it.

Agreed.

 -Hal

>
> Jonas

-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

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

r337480 - fix typo in comment

[Nico Weber via cfe-commits]

Author: nico
Date: Thu Jul 19 11:59:38 2018
New Revision: 337480

URL: http://llvm.org/viewvc/llvm-project?rev=337480&view=rev
Log:
fix typo in comment

Modified:
cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp

Modified: cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp?rev=337480&r1=337479&r2=337480&view=diff
==
--- cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp (original)
+++ cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp Thu Jul 19 11:59:38 2018
@@ -3336,7 +3336,7 @@ CGCXXABI *clang::CodeGen::CreateMicrosof
 //   a reference to the TypeInfo for the type and a reference to the
 //   CompleteHierarchyDescriptor for the type.
 //
-// ClassHieararchyDescriptor: Contains information about a class hierarchy.
+// ClassHierarchyDescriptor: Contains information about a class hierarchy.
 //   Used during dynamic_cast to walk a class hierarchy.  References a base
 //   class array and the size of said array.
 //


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

r337481 - Fix template argument deduction when a parameter pack has a value

[Richard Smith via cfe-commits]

Author: rsmith
Date: Thu Jul 19 12:00:37 2018
New Revision: 337481

URL: http://llvm.org/viewvc/llvm-project?rev=337481&view=rev
Log:
Fix template argument deduction when a parameter pack has a value
provided by an outer template.

We made the incorrect assumption in various places that the only way we
can have any arguments already provided for a pack during template
argument deduction was from a partially-specified pack. That's not true;
we can also have arguments from an enclosing already-instantiated
template, and that can even result in the function template's own pack
parameters having a fixed length and not being packs for the purposes of
template argument deduction.

Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/include/clang/Sema/TemplateDeduction.h
cfe/trunk/lib/Sema/SemaOverload.cpp
cfe/trunk/lib/Sema/SemaTemplate.cpp
cfe/trunk/lib/Sema/SemaTemplateDeduction.cpp
cfe/trunk/lib/Sema/SemaTemplateVariadic.cpp
cfe/trunk/test/SemaTemplate/pack-deduction.cpp

Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=337481&r1=337480&r2=337481&view=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Jul 19 12:00:37 
2018
@@ -3589,6 +3589,9 @@ def note_ovl_candidate_bad_deduction : N
 "candidate template ignored: failed template argument deduction">;
 def note_ovl_candidate_incomplete_deduction : Note<"candidate template 
ignored: "
 "couldn't infer template argument %0">;
+def note_ovl_candidate_incomplete_deduction_pack : Note<"candidate template 
ignored: "
+"deduced too few arguments for expanded pack %0; no argument for %ordinal1 
"
+"expanded parameter in deduced argument pack %2">;
 def note_ovl_candidate_inconsistent_deduction : Note<
 "candidate template ignored: deduced conflicting %select{types|values|"
 "templates}0 for parameter %1%diff{ ($ vs. $)|}2,3">;
@@ -4462,6 +4465,9 @@ def err_pack_expansion_length_conflict :
 def err_pack_expansion_length_conflict_multilevel : Error<
   "pack expansion contains parameter pack %0 that has a different "
   "length (%1 vs. %2) from outer parameter packs">;
+def err_pack_expansion_length_conflict_partial : Error<
+  "pack expansion contains parameter pack %0 that has a different "
+  "length (at least %1 vs. %2) from outer parameter packs">;
 def err_pack_expansion_member_init : Error<
   "pack expansion for initialization of member %0">;
 

Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=337481&r1=337480&r2=337481&view=diff
==
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Thu Jul 19 12:00:37 2018
@@ -6911,6 +6911,9 @@ public:
 /// Template argument deduction did not deduce a value
 /// for every template parameter.
 TDK_Incomplete,
+/// Template argument deduction did not deduce a value for every
+/// expansion of an expanded template parameter pack.
+TDK_IncompletePack,
 /// Template argument deduction produced inconsistent
 /// deduced values for the given template parameter.
 TDK_Inconsistent,

Modified: cfe/trunk/include/clang/Sema/TemplateDeduction.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/TemplateDeduction.h?rev=337481&r1=337480&r2=337481&view=diff
==
--- cfe/trunk/include/clang/Sema/TemplateDeduction.h (original)
+++ cfe/trunk/include/clang/Sema/TemplateDeduction.h Thu Jul 19 12:00:37 2018
@@ -51,6 +51,10 @@ class TemplateDeductionInfo {
   /// The template parameter depth for which we're performing deduction.
   unsigned DeducedDepth;
 
+  /// The number of parameters with explicitly-specified template arguments,
+  /// up to and including the partially-specified pack (if any).
+  unsigned ExplicitArgs = 0;
+
   /// Warnings (and follow-on notes) that were suppressed due to
   /// SFINAE while performing template argument deduction.
   SmallVector SuppressedDiagnostics;
@@ -73,6 +77,11 @@ public:
 return DeducedDepth;
   }
 
+  /// Get the number of explicitly-specified arguments.
+  unsigned getNumExplicitArgs() const {
+return ExplicitArgs;
+  }
+
   /// Take ownership of the deduced template argument list.
   TemplateArgumentList *take() {
 TemplateArgumentList *Result = Deduced;
@@ -100,6 +109,13 @@ public:
 return SuppressedDiagnostics.front();
   }
 
+  /// Provide an initial template argument list that contains the
+  /// explicitly-specified arguments.
+  void setExplicitArgs(TemplateArgumentList *NewDeduce

r337483 - Fix failing testcase to actually be valid.

[Richard Smith via cfe-commits]

Author: rsmith
Date: Thu Jul 19 12:05:13 2018
New Revision: 337483

URL: http://llvm.org/viewvc/llvm-project?rev=337483&view=rev
Log:
Fix failing testcase to actually be valid.

Modified:
cfe/trunk/test/SemaTemplate/pack-deduction.cpp

Modified: cfe/trunk/test/SemaTemplate/pack-deduction.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaTemplate/pack-deduction.cpp?rev=337483&r1=337482&r2=337483&view=diff
==
--- cfe/trunk/test/SemaTemplate/pack-deduction.cpp (original)
+++ cfe/trunk/test/SemaTemplate/pack-deduction.cpp Thu Jul 19 12:05:13 2018
@@ -152,7 +152,7 @@ namespace partial_full_mix {
   pair, tuple> k4 = A().g(pair(), pair(), pair()); 
// expected-error {{no match}}
 
   // FIXME: We should accept this by treating the pack of pairs as having a 
fixed length of 2 here.
-  tuple k5 = A::h(tuple, pair, pair>()); // expected-error {{no 
match}}
+  tuple k5 = A::h(tuple, pair, pair>()); // expected-error {{no 
match}}
 }
 
 namespace substitution_vs_function_deduction {


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

[PATCH] D49508: [Sema] Expr::skipRValueSubobjectAdjustments(): record skipped NoOp casts.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D49508#1168599, @lebedev.ri wrote:

> In https://reviews.llvm.org/D49508#1168584, @rjmccall wrote:
>
> > Hmm.  I think the approach of flagging ICEs that are semantically part of 
> > an explicit cast is probably a better representation for tools across the 
> > board.
>
>
> I could do that, but i couldn't find where it should be done.
>  Where are these "ICEs that are semantically part of an explicit cast" 
> created?


The design of `CastExpr` is that each node does exactly one implicit conversion 
step, but sometimes a conversion involves performing multiple steps which each 
has to get its own node, so all those initial conversions have to be 
represented as ICEs.  Also, while we do make some effort to give the explicit 
cast node a meaningful `CastKind`, in some situations it's just easier to build 
an ICE with the real CK and then just make the explicit cast a `NoOp`.  All 
those ICEs are triggered by the checking of the explicit cast and therefore 
semantically part of that operation even if they're separate nodes for 
technical reasons.

> Where would we mark them?

Well, explicit casts are handled by SemaCast.cpp, and all the checking routines 
are managed by a `CastOperation` helper class.  In fact, it's set up so that 
you could very easily (1) stash the original expression in the `CastOperation` 
construct and then (2) in `complete()`, just walk from the given cast up to the 
original expression and mark every ICE you see as being part of the explicit 
cast.

>> If we *are* going to do it this way, though, I think you should (1) make the 
>> collection of skipped expressions optional and (2) collect *all* the skipped 
>> expressions, not just no-op casts.
> 
> 1. I was wondering about that, will do.
> 2. Well, i could do that, but i would need to filter them afterwards in my 
> use-case.

Well, you're filtering them anyway, right?  You're scanning the list for 
explicit casts.

>   So i wonder - what is the motivation for that?
>   Nothing else needs that additional knowledge right now.

It has a much more obvious specification and it's not hard to imagine clients 
that would want to scan that list for other things.  (In fact, the 
comma-expressions list could pretty easily be combined into that.)


Repository:
  rC Clang

https://reviews.llvm.org/D49508



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

[PATCH] D49303: [CodeGen][ObjC] Treat non-escaping blocks as global blocks to make copy/dispose a no-op

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: docs/Block-ABI-Apple.rst:64
 enum {
+BLOCK_IS_NOESCAPE  =  (1 << 23),
 BLOCK_HAS_COPY_DISPOSE =  (1 << 25),

Something happened to my older comments here, but please document the meaning 
of this flag.  Specifically, it is set on blocks that do have captures (and 
thus are not true global blocks) but which are known not to escape for various 
other reasons.  Please include the fact that it implies `BLOCK_IS_GLOBAL` and 
explain why.



Comment at: lib/CodeGen/CGBlocks.h:283
+  // Indicates whether the block needs a custom copy or dispose function.
+  bool needsCopyDispose() const {
+return NeedsCopyDispose && !Block->doesNotEscape();

Please rename this to `needsCopyDisposeHelpers()` to make it clearer that it's 
specifically about the need for the functions.


Repository:
  rC Clang

https://reviews.llvm.org/D49303



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

[PATCH] D49508: [Sema] Expr::skipRValueSubobjectAdjustments(): record skipped NoOp casts.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D49508#1168620, @rjmccall wrote:

> In https://reviews.llvm.org/D49508#1168599, @lebedev.ri wrote:
>
> > In https://reviews.llvm.org/D49508#1168584, @rjmccall wrote:
> >
> > > Hmm.  I think the approach of flagging ICEs that are semantically part of 
> > > an explicit cast is probably a better representation for tools across the 
> > > board.
> >
> >
> > I could do that, but i couldn't find where it should be done.
> >  Where are these "ICEs that are semantically part of an explicit cast" 
> > created?
>
>
> The design of `CastExpr` is that each node does exactly one implicit 
> conversion step, but sometimes a conversion involves performing multiple 
> steps which each has to get its own node, so all those initial conversions 
> have to be represented as ICEs.  Also, while we do make some effort to give 
> the explicit cast node a meaningful `CastKind`, in some situations it's just 
> easier to build an ICE with the real CK and then just make the explicit cast 
> a `NoOp`.  All those ICEs are triggered by the checking of the explicit cast 
> and therefore semantically part of that operation even if they're separate 
> nodes for technical reasons.
>
> > Where would we mark them?
>
> Well, explicit casts are handled by SemaCast.cpp, and all the checking 
> routines are managed by a `CastOperation` helper class.  In fact, it's set up 
> so that you could very easily (1) stash the original expression in the 
> `CastOperation` construct and then (2) in `complete()`, just walk from the 
> given cast up to the original expression and mark every ICE you see as being 
> part of the explicit cast.


Hmm, thank you for these pointers. I agree that it would be *nicer*. Let's see..

>>> If we *are* going to do it this way, though, I think you should (1) make 
>>> the collection of skipped expressions optional and (2) collect *all* the 
>>> skipped expressions, not just no-op casts.
>> 
>> 1. I was wondering about that, will do.
>> 2. Well, i could do that, but i would need to filter them afterwards in my 
>> use-case.
> 
> Well, you're filtering them anyway, right?  You're scanning the list for 
> explicit casts.

I meant filter the list to only contain skipped casts, before actually scanning 
it for specific conditions.

>>   So i wonder - what is the motivation for that?
>>   Nothing else needs that additional knowledge right now.
> 
> It has a much more obvious specification and it's not hard to imagine clients 
> that would want to scan that list for other things.  (In fact, the 
> comma-expressions list could pretty easily be combined into that.)




Repository:
  rC Clang

https://reviews.llvm.org/D49508



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

[PATCH] D49294: Sema: Fix explicit address space cast in C++

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Sema/SemaOverload.cpp:3150
+  !getLangOpts().OpenCLCPlusPlus)
+return false;
+

yaxunl wrote:
> rjmccall wrote:
> > yaxunl wrote:
> > > rjmccall wrote:
> > > > It's not really OpenCL C++ that's special here, it's the possibility of 
> > > > promotions between address spaces.
> > > For OpenCL C++, there is language rule about address space promotion.
> > > 
> > > For other languages, there is no generic rule about adderss space 
> > > promotion, since the meaning of address space and their relations are 
> > > target dependent. Do we want to introduce a target hook 
> > > Target::canPromote(AddressSpace Src, AddressSpace Dest, LanguageOptions 
> > > LangOpts) to represent this? Or do we just assume a simple rule, that is, 
> > > all address space can be promoted to default address space 0, otherwise 
> > > it is not allowed?
> > A target-specific hook for handling target-specific address spaces makes 
> > sense to me.  I don't think there's a generic rule allowing promotion into 
> > the default AS.
> I checked the current logic about address space promotions. There are several 
> functions Type::Qualifier::isAddressSpaceSupersetOf, 
> Type:;Qualifier::compatiblyIncludes, PointerType::isAddressSpaceOverlapping 
> which checks address space compatibility. However they are only based on 
> language rules and do not depend on targets. Since only OpenCL defined 
> language rules regarding address spaces, address space promotion is only 
> allowed for OpenCL.
> 
> To allow target specific address space promotion, these functions need an 
> extra ASTContext parameter to allow them access LanguageOptions and 
> TargetInfo. I am not sure if this makes things overly complicated.
> 
> I would expect target specific address space casts in C++ to be rare cases 
> since it is not part of language standard. In most cases users would just use 
> generic pointer. When they do use explicit address space cast, I expect they 
> understand what they are doing. Then clang just do not do any target-specific 
> promotion and treat it as reinterpret cast. Promotion is only allowed by 
> language rules e.g. those defined by OpenCL.
> 
Well, address spaces in "pure" C/C++ are specified by ISO/IEC TR 18037, the 
Embedded C specification, which does cover the possibility of target-specific 
overlapping address spaces.  Since we don't support those yet, that's fine, I'm 
not going to ask you to add that infrastructure; but I do think you should at 
least handle the language-specific overlap rules here, and in particular you 
should handle them by calling one of those functions you mentioned, so that if 
someone *does* want to implement target-specific overlapping address spaces, 
it's obvious that this is one of the places that needs to be fixed and tested.


https://reviews.llvm.org/D49294



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

[PATCH] D49085: [Sema] Emit a diagnostic for an invalid dependent function template specialization

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington updated this revision to Diff 156338.
erik.pilkington added a comment.

Improve the diagnostics further.


https://reviews.llvm.org/D49085

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/CXX/temp/temp.decls/temp.friend/p1.cpp


Index: clang/test/CXX/temp/temp.decls/temp.friend/p1.cpp
===
--- clang/test/CXX/temp/temp.decls/temp.friend/p1.cpp
+++ clang/test/CXX/temp/temp.decls/temp.friend/p1.cpp
@@ -359,3 +359,31 @@
   template void f2(X *);
   template void f2(X *); // expected-note{{in instantiation of 
function template specialization 'PR10913::f2' requested here}}
 }
+
+namespace test16 {
+template  struct foo {}; // expected-note{{candidate ignored: not a 
function template}}
+template  class A {
+  friend void foo(); // expected-error{{no candidate function template was 
found for dependent friend function template specialization}}
+};
+}
+
+namespace test17 {
+namespace ns {
+template  void foo() {} // expected-note{{candidate ignored: not a 
member of the enclosing namespace; did you mean to explicitly qualify the 
specialization?}}
+}
+using ns::foo;
+template  struct A {
+  friend void foo() {} // expected-error{{no candidate function template 
was found for dependent friend function template specialization}}
+};
+}
+
+namespace test18 {
+namespace ns1 { template  struct foo {}; } // 
expected-note{{candidate ignored: not a function template}}
+namespace ns2 { void foo() {} } // expected-note{{candidate ignored: not a 
function template}}
+using ns1::foo;
+using ns2::foo;
+
+template  class A {
+  friend void foo() {} // expected-error{{no candidate function template 
was found for dependent friend function template specialization}}
+};
+}
Index: clang/lib/Sema/SemaTemplate.cpp
===
--- clang/lib/Sema/SemaTemplate.cpp
+++ clang/lib/Sema/SemaTemplate.cpp
@@ -8008,17 +8008,34 @@
   // the correct context.
   DeclContext *FDLookupContext = FD->getDeclContext()->getRedeclContext();
   LookupResult::Filter F = Previous.makeFilter();
+  enum DiscardReason { NotAFunctionTemplate, NotAMemberOfEnclosing };
+  SmallVector, 8> DiscardedCandidates;
   while (F.hasNext()) {
 NamedDecl *D = F.next()->getUnderlyingDecl();
-if (!isa(D) ||
-!FDLookupContext->InEnclosingNamespaceSetOf(
-  D->getDeclContext()->getRedeclContext()))
+if (!isa(D)) {
   F.erase();
+  DiscardedCandidates.push_back(std::make_pair(NotAFunctionTemplate, D));
+  continue;
+}
+
+if (!FDLookupContext->InEnclosingNamespaceSetOf(
+D->getDeclContext()->getRedeclContext())) {
+  F.erase();
+  DiscardedCandidates.push_back(std::make_pair(NotAMemberOfEnclosing, D));
+  continue;
+}
   }
   F.done();
 
-  // Should this be diagnosed here?
-  if (Previous.empty()) return true;
+  if (Previous.empty()) {
+Diag(FD->getLocation(),
+ diag::err_dependent_function_template_spec_no_match);
+for (auto &P : DiscardedCandidates)
+  Diag(P.second->getLocation(),
+   diag::note_dependent_function_template_spec_discard_reason)
+  << P.first;
+return true;
+  }
 
   FD->setDependentTemplateSpecialization(Context, Previous.asUnresolvedSet(),
  ExplicitTemplateArgs);
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4103,6 +4103,12 @@
 def err_explicit_specialization_inconsistent_storage_class : Error<
   "explicit specialization has extraneous, inconsistent storage class "
   "'%select{none|extern|static|__private_extern__|auto|register}0'">;
+def err_dependent_function_template_spec_no_match : Error<
+  "no candidate function template was found for dependent"
+  " friend function template specialization">;
+def note_dependent_function_template_spec_discard_reason : Note<
+  "candidate ignored: %select{not a function template"
+  "|not a member of the enclosing namespace; did you mean to explicitly 
qualify the specialization?}0">;
 
 // C++ class template specializations and out-of-line definitions
 def err_template_spec_needs_header : Error<


Index: clang/test/CXX/temp/temp.decls/temp.friend/p1.cpp
===
--- clang/test/CXX/temp/temp.decls/temp.friend/p1.cpp
+++ clang/test/CXX/temp/temp.decls/temp.friend/p1.cpp
@@ -359,3 +359,31 @@
   template void f2(X *);
   template void f2(X *); // expected-note{{in instantiation of function template specialization 'PR10913::f2' requested here}}
 }
+
+namespace test16 {
+template  struct foo {}; // expected-note{{candidate ignored: not a function template}}
+template  class A {
+  friend void foo(); // expected-error