[PATCH] D39903: [libclang] Allow pretty printing declarations

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

In https://reviews.llvm.org/D39903#944182, @cameron314 wrote:

> Locally we've done something similar (adding a 
> `clang_getCursorPrettyPrintedDeclaration` function, though without exposing 
> the `PrintingPolicy`) and overhauled `DeclPrinter` to produce proper pretty 
> names. This is a hack that works well for us, but can never be upstreamed 
> since it breaks too much existing code (and some of the printing decisions 
> are subjective). Your way is better.


You might consider to enhance PrintingPolicy for your use cases?

> I can point out differences in our implementations if you like. The diff is 
> rather long, though.

That would be interesting, yes, but rather later.

First I would like to get a review for this one...

Ping.


Repository:
  rC Clang

https://reviews.llvm.org/D39903



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

[PATCH] D40787: [clang-tidy] Replace the usage of std::uncaught_exception with std::uncaught_exceptions

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang-tidy/modernize/ReplaceUncaughtExceptionCheck.cpp:59
+  if (!BeginLoc.isMacroID()) {
+Diag << FixItHint::CreateInsertion(BeginLoc.getLocWithOffset(TextLength),
+   "s");

aaron.ballman wrote:
> JonasToth wrote:
> > koldaniel wrote:
> > > JonasToth wrote:
> > > > Could the location not simply be `EndLoc`?
> > > As I could see, when EndLoc was used in Diag (diag(..) of 
> > > CreateInsertion(...) methods,  it still pointed to the beginning of the 
> > > token marking the whole call. So if the CreateInsertion function looked 
> > > like this: 
> > > 
> > >   Diag << FixItHint::CreateInsertion(EndLoc.getLocWithOffset(TextLength), 
> > > "s");
> > > 
> > > had the same effect after applying the fix its as
> > > 
> > > Diag << 
> > > FixItHint::CreateInsertion(BeginLoc.getLocWithOffset(TextLength), "s");
> > > 
> > > for calls like 'uncaught_exception()' (without std:: namespace written at 
> > > the beginning, because it increases TextLength, and in case of EndLoc the 
> > > offset will be counted from the beginning of the function name, not the 
> > > namespace identifier).
> > Thats odd. You could do a Replacement with `getSourceRange` probably. 
> > Sounds more inefficient, but might be shorter in Code.
> This fixit can break code if the code disallows narrowing conversions. e.g.,
> ```
> bool b{std::uncaught_exception()};
> ```
> In this case, the fixit will take the deprecated code and make it ill-formed 
> instead. Perhaps a better fix-it would be to transform the code into 
> `(std::uncaught_exceptions() > 0)` to keep the resulting expression type a 
> `bool` and still not impact operator precedence?
Good point, but this kind of fix it is a bit ugly. Maybe skipping the fixit in 
narrowing cases or only generate the more complicated replacement in the 
narrowing case would be nicer. 



Comment at: test/clang-tidy/modernize-replace-uncaught-exception.cpp:16
+template 
+int doSomething(T t) { 
+return t();

It would be great to have a test where the template parameter is a function 
pointer and it is called with `uncaught_exception`. And with a check fixes make 
sure that the definition of the template is untouched.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40787



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

[PATCH] D38425: [clangd] Document highlights for clangd

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Overall looks good, just a few minor comments.
Let me know if need someone to land this patch for you after you fix those.




Comment at: clangd/ClangdUnit.cpp:249
+// Don't keep the same Macro info multiple times.
+// This can happen when nodes in the AST are visited twice.
+std::sort(MacroInfos.begin(), MacroInfos.end());

Mentioning AST in this comment is weird, macros have nothing to do with the 
AST. We should remove/rephrase the comment.
I'm not sure if multiple occurences of `MacroInfo` are even possible here, but 
we could leave the code as is anyway.



Comment at: clangd/Protocol.h:562
+
+///
+/// A document highlight is a range inside a text document which deserves

NIT: remove this empty comment line and all the others.



Comment at: clangd/Protocol.h:581
+const DocumentHighlight ) {
+return std::tie(LHS.range) < std::tie(RHS.range) &&
+   std::tie(LHS.kind) < std::tie(RHS.kind);

This comparison does not provide a total order.
Please use `std::tie(LHS.range, int(LHS.kind)) < std::tie(RHS.range, 
int(RHS.kind))` instead.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38425



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

[PATCH] D40897: [clangd] Introduce a "Symbol" class.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

More comments, but only two major things really:

- I'd like to clearly separate USR from SymbolID (even if you want to keep 
using USRs as their basis for now)
- the file organization (code within files, and names of files) needs some work 
I think

Everything else is details, this looks good




Comment at: clangd/Symbol.h:23
+  // The path of the source file where a symbol occurs.
+  std::string FilePath;
+  // The offset to the first character of the symbol from the beginning of the

hokein wrote:
> malaperle wrote:
> > malaperle wrote:
> > > sammccall wrote:
> > > > ioeric wrote:
> > > > > Is this relative or absolute?
> > > > Having every symbol own a copy of the filepath seems wasteful.
> > > > It seems likely that the index will have per-file information too, so 
> > > > this representation should likely be a key to that. Hash of the 
> > > > filepath might work?
> > > How we model it is that a symbol doesn't have a "location", but its 
> > > occurrence do. One could consider the location of a symbol to be either 
> > > its declaration occurrence (SymbolRole::Declaration) or its definition 
> > > (SymbolRole::Definition).
> > > What we do to get the location path is each occurrence has a pointer (a 
> > > "database" pointer, but it doesn't matter) to a file entry and then we 
> > > get the path from the entry.
> > > 
> > > So conceptually, it works a bit like this (although it fetches 
> > > information on disk).
> > > ```
> > > class IndexOccurrence {
> > > IndexOccurrence *FilePtr;
> > > 
> > > std::string Occurrence::getPath() {
> > >   return FilePtr->getPath();
> > > }
> > > };
> > > ```
> > Oops, wrong type for the field, it should have been:
> > ```
> > class IndexOccurrence {
> > IndexFile *FilePtr;
> > 
> > std::string Occurrence::getPath() {
> >   return FilePtr->getPath();
> > }
> > };
> > ```
> > Is this relative or absolute?
> 
>  Whether the file path is relative or absolute depends on the build system, 
> the file path could be relative (for header files) or absolute (for .cc 
> files).
> 
> > How we model it is that a symbol doesn't have a "location", but its 
> > occurrence do.
> 
> We will also have a SymbolOccurence structure alongside with Symbol (but it 
> is not  in this patch). The "Location" will be a part of SymbolOccurrence.
>  
> Whether the file path is relative or absolute depends on the build system, 
> the file path could be relative (for header files) or absolute (for .cc 
> files).

I'm not convinced this actually works. There's multiple codepaths to the index, 
how can we ensure we don't end up using inconsistent paths?
e.g. we open up a project that includes a system header using a relative path, 
and then open up that system header from file->open (editor knows only the 
absolute path) and do "find references".

I think we need to canonicalize the paths. Absolute is probably easiest.



Comment at: clangd/Symbol.h:37
+// The class presents a C++ symbol, e.g. class, function.
+struct Symbol {
+  // The symbol identifier, using USR.

hokein wrote:
> malaperle wrote:
> > sammccall wrote:
> > > hokein wrote:
> > > > malaperle wrote:
> > > > > I think it would be nice to have methods as an interface to get this 
> > > > > data instead of storing them directly. So that an index-on-disk could 
> > > > > go fetch the data. Especially the occurrences which can take a lot of 
> > > > > memory (I'm working on a branch that does that). But perhaps defining 
> > > > > that interface is not within the scope of this patch and could be 
> > > > > better discussed in D40548 ?
> > > > I agree. We can't load all the symbol occurrences into the memory since 
> > > > they are too large. We need to design interface for the symbol 
> > > > occurrences. 
> > > > 
> > > > We could discuss the interface here, but CodeCompletion is the main 
> > > > thing which this patch focuses on. 
> > > > We can't load all the symbol occurrences into the memory since they are 
> > > > too large
> > > 
> > > I've heard this often, but never backed up by data :-)
> > > 
> > > Naively an array of references for a symbol could be doc ID + offset + 
> > > length, let's say 16 bytes.
> > > 
> > > If a source file consisted entirely of references to 1-character symbols 
> > > separated by punctuation (1 reference per 2 bytes) then the total size of 
> > > these references would be 8x the size of the source file - in practice 
> > > much less. That's not very big.
> > > 
> > > (Maybe there are edge cases with macros/templates, but we can keep them 
> > > under control)
> > I'd have to break down how much memory it used by what, I'll come back to 
> > you on that. Indexing llvm with ClangdIndexDataStorage, which is pretty 
> > packed is about 200MB. That's already a lot considering we want to index 
> > code bases many times bigger. But I'll try to come up with more precise 
> > numbers. I'm open to different 

[PATCH] D40561: [libclang] Fix cursors for functions with trailing return type

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

Ping


https://reviews.llvm.org/D40561



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

[PATCH] D40481: [libclang] Fix cursors for arguments of Subscript and Call operators

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

Ping


https://reviews.llvm.org/D40481



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

[PATCH] D41001: [change-namespace] Fix crash when injected base-class name is used in friend declarations.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
Herald added subscribers: cfe-commits, klimek.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41001

Files:
  change-namespace/ChangeNamespace.cpp
  unittests/change-namespace/ChangeNamespaceTests.cpp


Index: unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- unittests/change-namespace/ChangeNamespaceTests.cpp
+++ unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -2154,6 +2154,60 @@
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
 }
 
+TEST_F(ChangeNamespaceTest, InjectedClassNameInFriendDecl) {
+  OldNamespace = "d";
+  NewNamespace = "e";
+  std::string Code = "namespace a{\n"
+ "template \n"
+ "class Base {\n"
+ " public:\n"
+ "  void f() {\n"
+ "T t;\n"
+ "t.priv();\n"
+ "  }\n"
+ "};\n"
+ "}  // namespace a\n"
+ "namespace d {\n"
+ "class D : public a::Base {\n"
+ " private:\n"
+ "  friend class Base;\n"
+ "  void priv() {}\n"
+ "  Base b;\n"
+ "};\n"
+ "\n"
+ "void f() {\n"
+ "  D d;\n"
+ "  a:: Base b;\n"
+ "  b.f();\n"
+ "}\n"
+ "}  // namespace d\n";
+  std::string Expected = "namespace a{\n"
+ "template \n"
+ "class Base {\n"
+ " public:\n"
+ "  void f() {\n"
+ "T t;\n"
+ "t.priv();\n"
+ "  }\n"
+ "};\n"
+ "}  // namespace a\n"
+ "\n"
+ "namespace e {\n"
+ "class D : public a::Base {\n"
+ " private:\n"
+ "  friend class Base;\n"
+ "  void priv() {}\n"
+ "  a::Base b;\n"
+ "};\n"
+ "\n"
+ "void f() {\n"
+ "  D d;\n"
+ "  a::Base b;\n"
+ "  b.f();\n"
+ "}\n"
+ "}  // namespace e\n";
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
 
 } // anonymous namespace
 } // namespace change_namespace
Index: change-namespace/ChangeNamespace.cpp
===
--- change-namespace/ChangeNamespace.cpp
+++ change-namespace/ChangeNamespace.cpp
@@ -552,6 +552,10 @@
 if (Loc.getTypeLocClass() == TypeLoc::Elaborated) {
   NestedNameSpecifierLoc NestedNameSpecifier =
   Loc.castAs().getQualifierLoc();
+  // This happens for friend declaration of a base class with injected 
class
+  // name.
+  if (!NestedNameSpecifier.getNestedNameSpecifier())
+return;
   const Type *SpecifierType =
   NestedNameSpecifier.getNestedNameSpecifier()->getAsType();
   if (SpecifierType && SpecifierType->isRecordType())


Index: unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- unittests/change-namespace/ChangeNamespaceTests.cpp
+++ unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -2154,6 +2154,60 @@
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
 }
 
+TEST_F(ChangeNamespaceTest, InjectedClassNameInFriendDecl) {
+  OldNamespace = "d";
+  NewNamespace = "e";
+  std::string Code = "namespace a{\n"
+ "template \n"
+ "class Base {\n"
+ " public:\n"
+ "  void f() {\n"
+ "T t;\n"
+ "t.priv();\n"
+ "  }\n"
+ "};\n"
+ "}  // namespace a\n"
+ "namespace d {\n"
+ "class D : public a::Base {\n"
+ " private:\n"
+ "  friend class Base;\n"
+ "  void priv() {}\n"
+ "  Base b;\n"
+ "};\n"
+ "\n"
+ "void f() {\n"
+ "  D d;\n"
+ "  a:: Base b;\n"
+ "  b.f();\n"
+ "}\n"
+ "}  // namespace d\n";
+  std::string Expected = "namespace a{\n"
+ "template \n"
+ "class Base {\n"
+ " public:\n"
+ "  void f() {\n"
+

[PATCH] D40948: Switch Clang's default C++ language target to C++14

[Tim Northover via Phabricator via cfe-commits]

t.p.northover updated this revision to Diff 126129.
t.p.northover added a comment.

Updating with tentative fixes to review comments.


Repository:
  rC Clang

https://reviews.llvm.org/D40948

Files:
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGenCXX/new-overflow.cpp
  clang/test/CodeGenCXX/new.cpp
  clang/test/CodeGenCXX/vtable-available-externally.cpp
  clang/test/Lexer/cxx-features.cpp
  clang/test/Lexer/half-literal.cpp
  clang/test/OpenMP/taskloop_reduction_codegen.cpp
  clang/test/OpenMP/taskloop_simd_reduction_codegen.cpp
  clang/test/Parser/cxx1z-nested-namespace-definition.cpp
  clang/test/SemaCXX/new-array-size-conv.cpp
  clang/test/SemaCXX/new-delete.cpp
  clang/test/SemaCXX/unknown-type-name.cpp
  clang/test/SemaTemplate/class-template-decl.cpp
  clang/test/SemaTemplate/explicit-instantiation.cpp

Index: clang/test/SemaTemplate/explicit-instantiation.cpp
===
--- clang/test/SemaTemplate/explicit-instantiation.cpp
+++ clang/test/SemaTemplate/explicit-instantiation.cpp
@@ -124,10 +124,10 @@
 namespace undefined_static_data_member {
   template struct A {
 static int a; // expected-note {{here}}
-template static int b; // expected-note {{here}} expected-warning {{extension}}
+template static int b; // expected-note {{here}} expected-warning 0+ {{extension}}
   };
   struct B {
-template static int c; // expected-note {{here}} expected-warning {{extension}}
+template static int c; // expected-note {{here}} expected-warning 0+ {{extension}}
   };
 
   template int A::a; // expected-error {{explicit instantiation of undefined static data member 'a' of class template 'undefined_static_data_member::A'}}
@@ -137,14 +137,14 @@
 
   template struct C {
 static int a;
-template static int b; // expected-warning {{extension}}
+template static int b; // expected-warning 0+ {{extension}}
   };
   struct D {
-template static int c; // expected-warning {{extension}}
+template static int c; // expected-warning 0+ {{extension}}
   };
   template int C::a;
-  template template int C::b; // expected-warning {{extension}}
-  template int D::c; // expected-warning {{extension}}
+  template template int C::b; // expected-warning 0+ {{extension}}
+  template int D::c; // expected-warning 0+ {{extension}}
 
   template int C::a;
   template int C::b;
Index: clang/test/SemaTemplate/class-template-decl.cpp
===
--- clang/test/SemaTemplate/class-template-decl.cpp
+++ clang/test/SemaTemplate/class-template-decl.cpp
@@ -57,8 +57,7 @@
   template class X; // expected-error{{expression}}
 }
 
-template class X1 var; // expected-warning{{variable templates are a C++14 extension}} \
-   // expected-error {{variable has incomplete type 'class X1'}} \
+template class X1 var; // expected-error {{variable has incomplete type 'class X1'}} \
// expected-note {{forward declaration of 'X1'}}
 
 namespace M {
Index: clang/test/SemaCXX/unknown-type-name.cpp
===
--- clang/test/SemaCXX/unknown-type-name.cpp
+++ clang/test/SemaCXX/unknown-type-name.cpp
@@ -95,18 +95,23 @@
 template int h(T::type, int); // expected-error{{missing 'typename'}}
 template int h(T::type x, char); // expected-error{{missing 'typename'}}
 
-template int junk1(T::junk); // expected-warning{{variable templates are a C++14 extension}}
+template int junk1(T::junk);
+#if __cplusplus <= 201103L
+// expected-warning@-2 {{variable templates are a C++14 extension}}
+#endif
 template int junk2(T::junk) throw(); // expected-error{{missing 'typename'}}
 template int junk3(T::junk) = delete; // expected-error{{missing 'typename'}}
 #if __cplusplus <= 199711L
 //expected-warning@-2 {{deleted function definitions are a C++11 extension}}
 #endif
 
 template int junk4(T::junk j); // expected-error{{missing 'typename'}}
 
-// FIXME: We can tell this was intended to be a function because it does not
-//have a dependent nested name specifier.
-template int i(T::type, int()); // expected-warning{{variable templates are a C++14 extension}}
+template int i(T::type, int());
+#if __cplusplus <= 201103L
+// expected-warning@-2 {{variable templates are a C++14 extension}}
+#endif
+
 
 // FIXME: We know which type specifier should have been specified here. Provide
 //a fix-it to add 'typename A::type'
Index: clang/test/SemaCXX/new-delete.cpp
===
--- clang/test/SemaCXX/new-delete.cpp
+++ clang/test/SemaCXX/new-delete.cpp
@@ -80,27 +80,43 @@
   (void)new int[1.1];
 #if __cplusplus <= 199711L
   // expected-error@-2 {{array size expression must have integral or enumeration type, not 'double'}}
-#else
+#elif __cplusplus <= 201103L
   // expected-error@-4 {{array size expression must have integral or unscoped 

[PATCH] D40948: Switch Clang's default C++ language target to C++14

[Tim Northover via Phabricator via cfe-commits]

t.p.northover added inline comments.



Comment at: clang/test/CodeGenCXX/new-overflow.cpp:88
   // CHECK:  [[N:%.*]] = sext i16 {{%.*}} to i32
-  // CHECK-NEXT: [[T0:%.*]] = icmp slt i32 [[N]], 0
-  // CHECK-NEXT: [[T1:%.*]] = select i1 [[T0]], i32 -1, i32 [[N]]
-  // CHECK-NEXT: call i8* @_Znaj(i32 [[T1]])
+  // CHECK-NEXT: call i8* @_Znaj(i32 [[N]])
   // CHECK:  getelementptr inbounds {{.*}}, i32 [[N]]

rsmith wrote:
> The changes in this file are a regression; C++14 requires us to check whether 
> the array bound prior to promotion is negative. Can you file a bug on that?
I've filed https://llvm.org/PR35573. Not quite sure what to do about this test 
until it's fixed though. Add a second RUN line to check both variants and then 
XFAIL it?



Comment at: clang/test/CodeGenCXX/vtable-available-externally.cpp:275
 struct C {
   virtual D& operator=(const D&);
 };

rsmith wrote:
> To make this test work in C++11 onwards, you need to add a virtual move 
> assignment operator here:
> 
> ```
> virtual D& operator=(D&&);
> ```
That didn't quite work. The issue appears to be that D has both of those 
implicitly defined in C++14 mode, but only the move assignment operator is used 
below. Speculative VTable emission requires all of them to be used.

So adding a "d = d;" line to the second g function fixes the test. Does that 
sound sane to you, or am I missing the point?



Comment at: clang/test/SemaCXX/unknown-type-name.cpp:98
 
-template int junk1(T::junk); // expected-warning{{variable 
templates are a C++14 extension}}
+template int junk1(T::junk);
+#if __cplusplus <= 201103L

rsmith wrote:
> Huh, we should probably have a `-Wvexing-parse` warning for this. Can you 
> file a bug?
I've filed https://llvm.org/PR35576. You may want to sanity check it though, I 
was pretty light on the detail because I wasn't sure of the exact diagnostic 
being proposed.


Repository:
  rC Clang

https://reviews.llvm.org/D40948



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

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

[Nikolai Kosjar via Phabricator via cfe-commits]

nik created this revision.

When a preamble is created an unsaved file not existing on disk is
already part of PrecompiledPreamble::FilesInPreamble. However, when
checking whether the preamble can be re-used, a failed stat of such an
unsaved file invalidated the preamble, which led to pointless and time
consuming preamble regenerations on subsequent reparses.

Do not require anymore that unsaved files should exist on disk.

This avoids costly preamble invalidations depending on timing issues for
the cases where the file on disk might be removed just to be regenerated
a bit later.

It also allows an IDE to provide in-memory files that might not exist on
disk, e.g. because the build system hasn't generated those yet.


Repository:
  rC Clang

https://reviews.llvm.org/D41005

Files:
  include/clang/Frontend/ASTUnit.h
  include/clang/Frontend/PrecompiledPreamble.h
  lib/Frontend/ASTUnit.cpp
  lib/Frontend/PrecompiledPreamble.cpp
  unittests/Frontend/PCHPreambleTest.cpp

Index: unittests/Frontend/PCHPreambleTest.cpp
===
--- unittests/Frontend/PCHPreambleTest.cpp
+++ unittests/Frontend/PCHPreambleTest.cpp
@@ -124,6 +124,23 @@
   }
 };
 
+TEST_F(PCHPreambleTest, ReparseDoesNotInvalidatePreambleDueToNotExistingUnsavedFile) {
+  std::string Header1 = "//./header1.h";
+  std::string MainName = "//./main.cpp";
+  AddFile(MainName, "#include \"//./header1.h\"\n"
+"int main() { return ZERO; }");
+
+  std::unique_ptr AST(ParseAST(MainName));
+  ASSERT_TRUE(AST.get());
+  auto InitialPreambleCreationTimePoint = AST->getPreambleCreationTimePoint();
+  ASSERT_NE(std::chrono::steady_clock::time_point(), InitialPreambleCreationTimePoint);
+
+  RemapFile(Header1, "#define ZERO 0\n");
+  ASSERT_TRUE(ReparseAST(AST));
+
+  ASSERT_EQ(InitialPreambleCreationTimePoint, AST->getPreambleCreationTimePoint());
+}
+
 TEST_F(PCHPreambleTest, ReparseWithOverriddenFileDoesNotInvalidatePreamble) {
   std::string Header1 = "//./header1.h";
   std::string Header2 = "//./header2.h";
Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -390,10 +390,10 @@
   return PreambleBounds(PreambleBytes.size(), PreambleEndsAtStartOfLine);
 }
 
-bool PrecompiledPreamble::CanReuse(const CompilerInvocation ,
-   const llvm::MemoryBuffer *MainFileBuffer,
-   PreambleBounds Bounds,
-   vfs::FileSystem *VFS) const {
+bool PrecompiledPreamble::CanReuse(
+const CompilerInvocation ,
+const llvm::MemoryBuffer *MainFileBuffer, PreambleBounds Bounds,
+IntrusiveRefCntPtr VFS) const {
 
   assert(
   Bounds.Size <= MainFileBuffer->getBufferSize() &&
@@ -434,19 +434,22 @@
 Status.getSize(), llvm::sys::toTimeT(Status.getLastModificationTime()));
   }
 
+  vfs::OverlayFileSystem OFS(VFS);
+  IntrusiveRefCntPtr IMFS(
+  new vfs::InMemoryFileSystem());
+  OFS.pushOverlay(IMFS);
   for (const auto  : PreprocessorOpts.RemappedFileBuffers) {
+IMFS->addFile(RB.first, 0, llvm::MemoryBuffer::getMemBuffer(""));
 vfs::Status Status;
-if (!moveOnNoError(VFS->status(RB.first), Status))
-  return false;
-
+assert(moveOnNoError(IMFS->status(RB.first), Status));
 OverriddenFiles[Status.getUniqueID()] =
 PreambleFileHash::createForMemoryBuffer(RB.second);
   }
 
   // Check whether anything has changed.
   for (const auto  : FilesInPreamble) {
 vfs::Status Status;
-if (!moveOnNoError(VFS->status(F.first()), Status)) {
+if (!moveOnNoError(OFS.status(F.first()), Status)) {
   // If we can't stat the file, assume that something horrible happened.
   return false;
 }
@@ -495,7 +498,8 @@
 llvm::StringMap FilesInPreamble)
 : Storage(std::move(Storage)), FilesInPreamble(std::move(FilesInPreamble)),
   PreambleBytes(std::move(PreambleBytes)),
-  PreambleEndsAtStartOfLine(PreambleEndsAtStartOfLine) {
+  PreambleEndsAtStartOfLine(PreambleEndsAtStartOfLine),
+  CreationTimePoint(std::chrono::steady_clock::now()){
   assert(this->Storage.getKind() != PCHStorage::Kind::Empty);
 }
 
Index: lib/Frontend/ASTUnit.cpp
===
--- lib/Frontend/ASTUnit.cpp
+++ lib/Frontend/ASTUnit.cpp
@@ -1244,7 +1244,7 @@
 
   if (Preamble) {
 if (Preamble->CanReuse(PreambleInvocationIn, MainFileBuffer.get(), Bounds,
-   VFS.get())) {
+   VFS)) {
   // Okay! We can re-use the precompiled preamble.
 
   // Set the state of the diagnostic object to mimic its state
Index: include/clang/Frontend/PrecompiledPreamble.h
===
--- include/clang/Frontend/PrecompiledPreamble.h
+++ include/clang/Frontend/PrecompiledPreamble.h
@@ 

[PATCH] D40952: [clangd] Convert lit code completion tests to unit-tests. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 126116.
sammccall added a comment.

Add documentation to matcher implementation.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40952

Files:
  test/clangd/completion-items-kinds.test
  test/clangd/completion-priorities.test
  test/clangd/completion-qualifiers.test
  test/clangd/completion-snippet.test
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/Matchers.h

Index: unittests/clangd/Matchers.h
===
--- /dev/null
+++ unittests/clangd/Matchers.h
@@ -0,0 +1,112 @@
+//===-- Matchers.h --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// GMock matchers that aren't specific to particular tests.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_MATCHERS_H
+#define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_MATCHERS_H
+#include "gmock/gmock.h"
+
+namespace clang {
+namespace clangd {
+using ::testing::Matcher;
+
+// EXPECT_IFF expects matcher if condition is true, and Not(matcher) if false.
+// This is hard to write as a function, because matchers may be polymorphic.
+#define EXPECT_IFF(condition, value, matcher)  \
+  do { \
+if (condition) \
+  EXPECT_THAT(value, matcher); \
+else   \
+  EXPECT_THAT(value, ::testing::Not(matcher)); \
+  } while (0)
+
+// HasSubsequence(m1, m2, ...) matches a vector containing elements that match
+// m1, m2 ... in that order.
+//
+// SubsequenceMatcher implements this once the type of vector is known.
+template 
+class SubsequenceMatcher
+: public ::testing::MatcherInterface &> {
+  std::vector Matchers;
+
+public:
+  SubsequenceMatcher(std::vector M) : Matchers(M) {}
+
+  void DescribeTo(std::ostream *OS) const override {
+*OS << "Contains the subsequence [";
+const char *Sep = "";
+for (const auto  : Matchers) {
+  *OS << Sep;
+  M.DescribeTo(OS);
+  Sep = ", ";
+}
+*OS << "]";
+  }
+
+  bool MatchAndExplain(const std::vector ,
+   ::testing::MatchResultListener *L) const override {
+std::vector Matches(Matchers.size());
+size_t I = 0;
+for (size_t J = 0; I < Matchers.size() && J < V.size(); ++J)
+  if (Matchers[I].Matches(V[J]))
+Matches[I++] = J;
+if (I == Matchers.size()) // We exhausted all matchers.
+  return true;
+if (L->IsInterested()) {
+  *L << "\n  Matched:";
+  for (size_t K = 0; K < I; ++K) {
+*L << "\n\t";
+Matchers[K].DescribeTo(L->stream());
+*L << " ==> " << ::testing::PrintToString(V[Matches[K]]);
+  }
+  *L << "\n\t";
+  Matchers[I].DescribeTo(L->stream());
+  *L << " ==> no subsequent match";
+}
+return false;
+  }
+};
+
+// PolySubsequenceMatcher implements a "polymorphic" SubsequenceMatcher.
+// It captures the types of the element matchers, and can be converted to
+// Matcher if each matcher can be converted to Matcher.
+// This allows HasSubsequence() to accept polymorphic matchers like Not().
+template  class PolySubsequenceMatcher {
+  std::tuple Matchers;
+
+public:
+  PolySubsequenceMatcher(M &&... Args)
+  : Matchers(std::make_tuple(std::forward(Args)...)) {}
+
+  template  operator Matcher &>() const {
+return ::testing::MakeMatcher(new SubsequenceMatcher(
+TypedMatchers(llvm::index_sequence_for{})));
+  }
+
+private:
+  template 
+  std::vector TypedMatchers(llvm::index_sequence) const {
+return {std::get(Matchers)...};
+  }
+};
+
+// HasSubsequence(m1, m2, ...) matches a vector containing elements that match
+// m1, m2 ... in that order.
+// The real implementation is in SubsequenceMatcher.
+template 
+PolySubsequenceMatcher HasSubsequence(Args &&... M) {
+  return PolySubsequenceMatcher(std::forward(M)...);
+}
+
+} // namespace clangd
+} // namespace clang
+#endif
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -8,6 +8,7 @@
 //===--===//
 #include "ClangdServer.h"
 #include "Compiler.h"
+#include "Matchers.h"
 #include "Protocol.h"
 #include "TestFS.h"
 #include "gmock/gmock.h"
@@ -18,15 +19,23 @@
 // Let GMock print 

[PATCH] D40548: [clangd] Symbol index interfaces and index-based code completion.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the feedback, Marc!

Yeah, I think the ClangdIndexDataSymbol and ClangdIndexDataOccurrence are 
something similar to what https://reviews.llvm.org/D40897 trying to address, 
maybe put the discussion there? Before that, I think having a sym meeting is a 
good idea to keep us in the same page.

In https://reviews.llvm.org/D40548#949279, @ioeric wrote:

> >   
>
> I think some of the ideas here could be useful. This patch focuses mostly on 
> index interfaces and https://reviews.llvm.org/D40897 emphasizes on the design 
> of symbol structure. The way symbols are stored and used in this patch is 
> likely to change depending on how https://reviews.llvm.org/D40897 goes.
>
> > The "Clangd" prefix adds a bit much of clutter so maybe it should be 
> > removed.  I think the main points are that having generic 
> > foreachSymbols/foreachOccurrence with callbacks is well suited to implement 
> > multiple features with minimal copying.
>
> Although I'm not sure if `foreachSymbols`/... would be feasible for all 
> indexes yet, we do plan to switch to callback-based index interfaces, which 
> Sam also proposed in the review comments.
>
> There have been some offline discussions happening around clangd's indexing, 
> and sorry that we have not been able to keep you up to date. I think it might 
> be more efficient if we could meet via VC/Hangouts and sync on our designs. 
> If you don't mind a meeting, I am happy to arrange it via emails.





Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40548



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

[PATCH] D41001: [change-namespace] Fix crash when injected base-class name is used in friend declarations.

[Haojian Wu via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41001



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

[PATCH] D40952: [clangd] Convert lit code completion tests to unit-tests. NFC

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added a comment.

lg




Comment at: unittests/clangd/Matchers.h:54
+  bool MatchAndExplain(const std::vector ,
+   ::testing::MatchResultListener *L) const {
+std::vector Matches(Matchers.size());

`override`?



Comment at: unittests/clangd/Matchers.h:77
+
+template  class PolySubsequenceMatcher {
+  std::tuple Matchers;

It would be helpful to provide a bit documentation here.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40952



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

[PATCH] D40489: [clangd] Changed tracing interfaces

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/Trace.cpp:138
 return;
-  if (!Args)
-Args = llvm::make_unique();
-  T->event(Ctx, "E",
-   Args ? json::obj{{"args", std::move(*Args)}} : json::obj{});
+  assert(Args && "Args can't be null at this point");
+  T->end_event(Ctx, Name, std::move(*Args));

sammccall wrote:
> why not?
Because `T` must outlive the `Span`, so we can't really have `if (!T)` evaluate 
to different things in constructor and destructor.
Am I missing something?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40489



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

r320135 - In stdbool.h, define bool, false, true only in gnu++98

[Stephan Bergmann via cfe-commits]

Author: sberg
Date: Fri Dec  8 00:28:08 2017
New Revision: 320135

URL: http://llvm.org/viewvc/llvm-project?rev=320135=rev
Log:
In stdbool.h, define bool, false, true only in gnu++98

GCC has meanwhile corrected that with the similar
 "C++11
explicitly forbids macros for bool, true and false."

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

Modified:
cfe/trunk/lib/Headers/stdbool.h
cfe/trunk/test/Headers/stdbool.cpp

Modified: cfe/trunk/lib/Headers/stdbool.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Headers/stdbool.h?rev=320135=320134=320135=diff
==
--- cfe/trunk/lib/Headers/stdbool.h (original)
+++ cfe/trunk/lib/Headers/stdbool.h Fri Dec  8 00:28:08 2017
@@ -32,12 +32,15 @@
 #define true 1
 #define false 0
 #elif defined(__GNUC__) && !defined(__STRICT_ANSI__)
-/* Define _Bool, bool, false, true as a GNU extension. */
+/* Define _Bool as a GNU extension. */
 #define _Bool bool
+#if __cplusplus < 201103L
+/* For C++98, define bool, false, true as a GNU extension. */
 #define bool  bool
 #define false false
 #define true  true
 #endif
+#endif
 
 #define __bool_true_false_are_defined 1
 

Modified: cfe/trunk/test/Headers/stdbool.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Headers/stdbool.cpp?rev=320135=320134=320135=diff
==
--- cfe/trunk/test/Headers/stdbool.cpp (original)
+++ cfe/trunk/test/Headers/stdbool.cpp Fri Dec  8 00:28:08 2017
@@ -1,13 +1,19 @@
-// RUN: %clang_cc1 -E -dM %s | FileCheck --check-prefix=CHECK-GNU-COMPAT %s
+// RUN: %clang_cc1 -std=gnu++98 -E -dM %s | FileCheck 
--check-prefix=CHECK-GNU-COMPAT-98 %s
+// RUN: %clang_cc1 -std=gnu++11 -E -dM %s | FileCheck 
--check-prefix=CHECK-GNU-COMPAT-11 %s
 // RUN: %clang_cc1 -std=c++98 -E -dM %s | FileCheck 
--check-prefix=CHECK-CONFORMING %s
 // RUN: %clang_cc1 -fsyntax-only -std=gnu++98 -verify -Weverything %s
 #include 
 #define zzz
 
-// CHECK-GNU-COMPAT: #define _Bool bool
-// CHECK-GNU-COMPAT: #define bool bool
-// CHECK-GNU-COMPAT: #define false false
-// CHECK-GNU-COMPAT: #define true true
+// CHECK-GNU-COMPAT-98: #define _Bool bool
+// CHECK-GNU-COMPAT-98: #define bool bool
+// CHECK-GNU-COMPAT-98: #define false false
+// CHECK-GNU-COMPAT-98: #define true true
+
+// CHECK-GNU-COMPAT-11: #define _Bool bool
+// CHECK-GNU-COMPAT-11-NOT: #define bool bool
+// CHECK-GNU-COMPAT-11-NOT: #define false false
+// CHECK-GNU-COMPAT-11-NOT: #define true true
 
 // CHECK-CONFORMING-NOT: #define _Bool
 // CHECK-CONFORMING: #define __CHAR_BIT__


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

[PATCH] D40167: In stdbool.h, define bool, false, true only in gnu++98

[Stephan Bergmann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC320135: In stdbool.h, define bool, false, true only in 
gnu++98 (authored by sberg).

Changed prior to commit:
  https://reviews.llvm.org/D40167?vs=123296=126095#toc

Repository:
  rC Clang

https://reviews.llvm.org/D40167

Files:
  lib/Headers/stdbool.h
  test/Headers/stdbool.cpp


Index: test/Headers/stdbool.cpp
===
--- test/Headers/stdbool.cpp
+++ test/Headers/stdbool.cpp
@@ -1,13 +1,19 @@
-// RUN: %clang_cc1 -E -dM %s | FileCheck --check-prefix=CHECK-GNU-COMPAT %s
+// RUN: %clang_cc1 -std=gnu++98 -E -dM %s | FileCheck 
--check-prefix=CHECK-GNU-COMPAT-98 %s
+// RUN: %clang_cc1 -std=gnu++11 -E -dM %s | FileCheck 
--check-prefix=CHECK-GNU-COMPAT-11 %s
 // RUN: %clang_cc1 -std=c++98 -E -dM %s | FileCheck 
--check-prefix=CHECK-CONFORMING %s
 // RUN: %clang_cc1 -fsyntax-only -std=gnu++98 -verify -Weverything %s
 #include 
 #define zzz
 
-// CHECK-GNU-COMPAT: #define _Bool bool
-// CHECK-GNU-COMPAT: #define bool bool
-// CHECK-GNU-COMPAT: #define false false
-// CHECK-GNU-COMPAT: #define true true
+// CHECK-GNU-COMPAT-98: #define _Bool bool
+// CHECK-GNU-COMPAT-98: #define bool bool
+// CHECK-GNU-COMPAT-98: #define false false
+// CHECK-GNU-COMPAT-98: #define true true
+
+// CHECK-GNU-COMPAT-11: #define _Bool bool
+// CHECK-GNU-COMPAT-11-NOT: #define bool bool
+// CHECK-GNU-COMPAT-11-NOT: #define false false
+// CHECK-GNU-COMPAT-11-NOT: #define true true
 
 // CHECK-CONFORMING-NOT: #define _Bool
 // CHECK-CONFORMING: #define __CHAR_BIT__
Index: lib/Headers/stdbool.h
===
--- lib/Headers/stdbool.h
+++ lib/Headers/stdbool.h
@@ -32,12 +32,15 @@
 #define true 1
 #define false 0
 #elif defined(__GNUC__) && !defined(__STRICT_ANSI__)
-/* Define _Bool, bool, false, true as a GNU extension. */
+/* Define _Bool as a GNU extension. */
 #define _Bool bool
+#if __cplusplus < 201103L
+/* For C++98, define bool, false, true as a GNU extension. */
 #define bool  bool
 #define false false
 #define true  true
 #endif
+#endif
 
 #define __bool_true_false_are_defined 1
 


Index: test/Headers/stdbool.cpp
===
--- test/Headers/stdbool.cpp
+++ test/Headers/stdbool.cpp
@@ -1,13 +1,19 @@
-// RUN: %clang_cc1 -E -dM %s | FileCheck --check-prefix=CHECK-GNU-COMPAT %s
+// RUN: %clang_cc1 -std=gnu++98 -E -dM %s | FileCheck --check-prefix=CHECK-GNU-COMPAT-98 %s
+// RUN: %clang_cc1 -std=gnu++11 -E -dM %s | FileCheck --check-prefix=CHECK-GNU-COMPAT-11 %s
 // RUN: %clang_cc1 -std=c++98 -E -dM %s | FileCheck --check-prefix=CHECK-CONFORMING %s
 // RUN: %clang_cc1 -fsyntax-only -std=gnu++98 -verify -Weverything %s
 #include 
 #define zzz
 
-// CHECK-GNU-COMPAT: #define _Bool bool
-// CHECK-GNU-COMPAT: #define bool bool
-// CHECK-GNU-COMPAT: #define false false
-// CHECK-GNU-COMPAT: #define true true
+// CHECK-GNU-COMPAT-98: #define _Bool bool
+// CHECK-GNU-COMPAT-98: #define bool bool
+// CHECK-GNU-COMPAT-98: #define false false
+// CHECK-GNU-COMPAT-98: #define true true
+
+// CHECK-GNU-COMPAT-11: #define _Bool bool
+// CHECK-GNU-COMPAT-11-NOT: #define bool bool
+// CHECK-GNU-COMPAT-11-NOT: #define false false
+// CHECK-GNU-COMPAT-11-NOT: #define true true
 
 // CHECK-CONFORMING-NOT: #define _Bool
 // CHECK-CONFORMING: #define __CHAR_BIT__
Index: lib/Headers/stdbool.h
===
--- lib/Headers/stdbool.h
+++ lib/Headers/stdbool.h
@@ -32,12 +32,15 @@
 #define true 1
 #define false 0
 #elif defined(__GNUC__) && !defined(__STRICT_ANSI__)
-/* Define _Bool, bool, false, true as a GNU extension. */
+/* Define _Bool as a GNU extension. */
 #define _Bool bool
+#if __cplusplus < 201103L
+/* For C++98, define bool, false, true as a GNU extension. */
 #define bool  bool
 #define false false
 #define true  true
 #endif
+#endif
 
 #define __bool_true_false_are_defined 1
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41001: [change-namespace] Fix crash when injected base-class name is used in friend declarations.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL320139: [change-namespace] Fix crash when injected 
base-class name is used in friend… (authored by ioeric).

Repository:
  rL LLVM

https://reviews.llvm.org/D41001

Files:
  clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
  clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp


Index: clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
===
--- clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
+++ clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
@@ -552,6 +552,10 @@
 if (Loc.getTypeLocClass() == TypeLoc::Elaborated) {
   NestedNameSpecifierLoc NestedNameSpecifier =
   Loc.castAs().getQualifierLoc();
+  // This happens for friend declaration of a base class with injected 
class
+  // name.
+  if (!NestedNameSpecifier.getNestedNameSpecifier())
+return;
   const Type *SpecifierType =
   NestedNameSpecifier.getNestedNameSpecifier()->getAsType();
   if (SpecifierType && SpecifierType->isRecordType())
Index: 
clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
+++ clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -2154,6 +2154,60 @@
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
 }
 
+TEST_F(ChangeNamespaceTest, InjectedClassNameInFriendDecl) {
+  OldNamespace = "d";
+  NewNamespace = "e";
+  std::string Code = "namespace a{\n"
+ "template \n"
+ "class Base {\n"
+ " public:\n"
+ "  void f() {\n"
+ "T t;\n"
+ "t.priv();\n"
+ "  }\n"
+ "};\n"
+ "}  // namespace a\n"
+ "namespace d {\n"
+ "class D : public a::Base {\n"
+ " private:\n"
+ "  friend class Base;\n"
+ "  void priv() {}\n"
+ "  Base b;\n"
+ "};\n"
+ "\n"
+ "void f() {\n"
+ "  D d;\n"
+ "  a:: Base b;\n"
+ "  b.f();\n"
+ "}\n"
+ "}  // namespace d\n";
+  std::string Expected = "namespace a{\n"
+ "template \n"
+ "class Base {\n"
+ " public:\n"
+ "  void f() {\n"
+ "T t;\n"
+ "t.priv();\n"
+ "  }\n"
+ "};\n"
+ "}  // namespace a\n"
+ "\n"
+ "namespace e {\n"
+ "class D : public a::Base {\n"
+ " private:\n"
+ "  friend class Base;\n"
+ "  void priv() {}\n"
+ "  a::Base b;\n"
+ "};\n"
+ "\n"
+ "void f() {\n"
+ "  D d;\n"
+ "  a::Base b;\n"
+ "  b.f();\n"
+ "}\n"
+ "}  // namespace e\n";
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
 
 } // anonymous namespace
 } // namespace change_namespace


Index: clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
===
--- clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
+++ clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
@@ -552,6 +552,10 @@
 if (Loc.getTypeLocClass() == TypeLoc::Elaborated) {
   NestedNameSpecifierLoc NestedNameSpecifier =
   Loc.castAs().getQualifierLoc();
+  // This happens for friend declaration of a base class with injected class
+  // name.
+  if (!NestedNameSpecifier.getNestedNameSpecifier())
+return;
   const Type *SpecifierType =
   NestedNameSpecifier.getNestedNameSpecifier()->getAsType();
   if (SpecifierType && SpecifierType->isRecordType())
Index: clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
===
--- clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
+++ clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -2154,6 +2154,60 @@
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
 }
 
+TEST_F(ChangeNamespaceTest, InjectedClassNameInFriendDecl) {
+  

[clang-tools-extra] r320139 - [change-namespace] Fix crash when injected base-class name is used in friend declarations.

[Eric Liu via cfe-commits]

Author: ioeric
Date: Fri Dec  8 02:06:16 2017
New Revision: 320139

URL: http://llvm.org/viewvc/llvm-project?rev=320139=rev
Log:
[change-namespace] Fix crash when injected base-class name is used in friend 
declarations.

Reviewers: hokein

Subscribers: klimek, cfe-commits

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

Modified:
clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp

Modified: clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp?rev=320139=320138=320139=diff
==
--- clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp (original)
+++ clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp Fri Dec  8 
02:06:16 2017
@@ -552,6 +552,10 @@ void ChangeNamespaceTool::run(
 if (Loc.getTypeLocClass() == TypeLoc::Elaborated) {
   NestedNameSpecifierLoc NestedNameSpecifier =
   Loc.castAs().getQualifierLoc();
+  // This happens for friend declaration of a base class with injected 
class
+  // name.
+  if (!NestedNameSpecifier.getNestedNameSpecifier())
+return;
   const Type *SpecifierType =
   NestedNameSpecifier.getNestedNameSpecifier()->getAsType();
   if (SpecifierType && SpecifierType->isRecordType())

Modified: 
clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp?rev=320139=320138=320139=diff
==
--- clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp 
(original)
+++ clang-tools-extra/trunk/unittests/change-namespace/ChangeNamespaceTests.cpp 
Fri Dec  8 02:06:16 2017
@@ -2154,6 +2154,60 @@ TEST_F(ChangeNamespaceTest, DefaultMoveC
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
 }
 
+TEST_F(ChangeNamespaceTest, InjectedClassNameInFriendDecl) {
+  OldNamespace = "d";
+  NewNamespace = "e";
+  std::string Code = "namespace a{\n"
+ "template \n"
+ "class Base {\n"
+ " public:\n"
+ "  void f() {\n"
+ "T t;\n"
+ "t.priv();\n"
+ "  }\n"
+ "};\n"
+ "}  // namespace a\n"
+ "namespace d {\n"
+ "class D : public a::Base {\n"
+ " private:\n"
+ "  friend class Base;\n"
+ "  void priv() {}\n"
+ "  Base b;\n"
+ "};\n"
+ "\n"
+ "void f() {\n"
+ "  D d;\n"
+ "  a:: Base b;\n"
+ "  b.f();\n"
+ "}\n"
+ "}  // namespace d\n";
+  std::string Expected = "namespace a{\n"
+ "template \n"
+ "class Base {\n"
+ " public:\n"
+ "  void f() {\n"
+ "T t;\n"
+ "t.priv();\n"
+ "  }\n"
+ "};\n"
+ "}  // namespace a\n"
+ "\n"
+ "namespace e {\n"
+ "class D : public a::Base {\n"
+ " private:\n"
+ "  friend class Base;\n"
+ "  void priv() {}\n"
+ "  a::Base b;\n"
+ "};\n"
+ "\n"
+ "void f() {\n"
+ "  D d;\n"
+ "  a::Base b;\n"
+ "  b.f();\n"
+ "}\n"
+ "}  // namespace e\n";
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
 
 } // anonymous namespace
 } // namespace change_namespace


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

[PATCH] D40897: [clangd] Introduce a "Symbol" class.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the useful comments!




Comment at: clangd/Symbol.h:1
+//===--- Symbol.h ---*- C++-*-===//
+//

sammccall wrote:
> sammccall wrote:
> > I think that:
> >  - there's other places in clangd that deal with symbols too, this is 
> > specifically for indexing
> >  - the index interface belongs alongside Symbol
> > I'd suggest calling this Index.h
> I don't think having `Symbol`s be completely self-contained objects and 
> passing them around in standard containers like `set`s will prove to be ideal.
> It means they can't share storage for e.g. location filename, that it's hard 
> for us to arena-allocate them, etc.
> 
> I think we could use the concept of a set of symbols which share a lifetime. 
> An initial version might just be
> 
> class SymbolSlab {
> public:
>   using iterator = DenseSet::iterator;
>   iterator begin();
>   iterator end();
> private:
>   DenseSet Symbols;
> }
> 
> But it's easy to add `StringPool` etc there.
> Then this is the natural unit of granularity of large sets of symbols:  a 
> dynamic index that deals with one file at a time would operate on (Filename, 
> SymbolSlab) pairs. SymbolCollector would return a SymbolSlab, etc.
> 
> Then indexes can be built on top of this using non-owning pointers.
+1 It makes sense. 

For initial version, the `Symbol` structure is still owning its fields naively, 
we could improve it (change to pointer or references) in the future.



Comment at: clangd/Symbol.h:23
+  // The path of the source file where a symbol occurs.
+  std::string FilePath;
+  // The offset to the first character of the symbol from the beginning of the

malaperle wrote:
> malaperle wrote:
> > sammccall wrote:
> > > ioeric wrote:
> > > > Is this relative or absolute?
> > > Having every symbol own a copy of the filepath seems wasteful.
> > > It seems likely that the index will have per-file information too, so 
> > > this representation should likely be a key to that. Hash of the filepath 
> > > might work?
> > How we model it is that a symbol doesn't have a "location", but its 
> > occurrence do. One could consider the location of a symbol to be either its 
> > declaration occurrence (SymbolRole::Declaration) or its definition 
> > (SymbolRole::Definition).
> > What we do to get the location path is each occurrence has a pointer (a 
> > "database" pointer, but it doesn't matter) to a file entry and then we get 
> > the path from the entry.
> > 
> > So conceptually, it works a bit like this (although it fetches information 
> > on disk).
> > ```
> > class IndexOccurrence {
> > IndexOccurrence *FilePtr;
> > 
> > std::string Occurrence::getPath() {
> >   return FilePtr->getPath();
> > }
> > };
> > ```
> Oops, wrong type for the field, it should have been:
> ```
> class IndexOccurrence {
> IndexFile *FilePtr;
> 
> std::string Occurrence::getPath() {
>   return FilePtr->getPath();
> }
> };
> ```
> Is this relative or absolute?

 Whether the file path is relative or absolute depends on the build system, the 
file path could be relative (for header files) or absolute (for .cc files).

> How we model it is that a symbol doesn't have a "location", but its 
> occurrence do.

We will also have a SymbolOccurence structure alongside with Symbol (but it is 
not  in this patch). The "Location" will be a part of SymbolOccurrence.
 



Comment at: clangd/Symbol.h:26
+  // source file.
+  unsigned StartOffset;
+  // The offset to the last character of the symbol from the beginning of the

ioeric wrote:
> 0-based or 1-based?
The offset is equivalent to FileOffset in `SourceLocation`. I can't find any 
document about the FileOffset in LLVM, but it is 0-based.



Comment at: clangd/Symbol.h:32
+
+struct CodeCompletionInfo {
+  // FIXME: add fields here.

sammccall wrote:
> Let's not add this until we know what's in it.
> There's gong to be an overlap between information needed for CC and other use 
> cases, so this structure might not help the user navigate.
Removed it.



Comment at: clangd/Symbol.h:37
+// The class presents a C++ symbol, e.g. class, function.
+struct Symbol {
+  // The symbol identifier, using USR.

malaperle wrote:
> sammccall wrote:
> > hokein wrote:
> > > malaperle wrote:
> > > > I think it would be nice to have methods as an interface to get this 
> > > > data instead of storing them directly. So that an index-on-disk could 
> > > > go fetch the data. Especially the occurrences which can take a lot of 
> > > > memory (I'm working on a branch that does that). But perhaps defining 
> > > > that interface is not within the scope of this patch and could be 
> > > > better discussed in D40548 ?
> > > I agree. We can't load all the symbol occurrences into the memory since 
> > > they are too large. We need to design 

[PATCH] D38639: [clangd] #include statements support for Open definition

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdUnit.cpp:38
 
+class DelegatingPPCallbacks : public PPCallbacks {
+

Nebiroth wrote:
> ilya-biryukov wrote:
> > What's the purpose of this class?
> We need to be able to use a wrapper class to be able to make a unique_ptr to 
> be sent to PrecompiledPreamble::Build in order to add the list of 
> preprocessor Callbacks.
Could we implement an instance of `PPCallbacks` that contains  
`CppFilePreambleCallbacks` and forwards to that specific method instead?

The reason is that we're not really delegating other methods in this calls(nor 
should we, the implementation would be too compilcated).
Having a class that contains `CppFilePreambleCallbacks ` and calling 
`Collector.InclusionDirective` seems perfectly fine, though: its purpose is 
clear and the implementation is easy.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38639



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

[PATCH] D40548: [clangd] Symbol index interfaces and index-based code completion.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D40548#949182, @malaperle wrote:

> As a follow-up, here's the interface for querying the index that I am using 
> right now. It's meant to be able to retrieve from any kind of "backend", i.e. 
> in-memory, ClangdIndexDataStore, libIndexStore, etc. I was able to implement 
> "Open Workspace Symbol" (which is close to code completion in concept), Find 
> References and Find Definitions.
>
>   using USR = llvm::SmallString<256>;
>  
>   class ClangdIndexDataOccurrence;
>  
>   class ClangdIndexDataSymbol {
>   public:
> virtual index::SymbolKind getKind() = 0;
> /// For example, for mynamespace::myclass::mymethod, this will be
> /// mymethod.
> virtual std::string getName() = 0;
> /// For example, for mynamespace::myclass::mymethod, this will be
> /// mynamespace::myclass::
> virtual std::string getQualifier() = 0;
> virtual std::string getUsr() = 0;
>  
> virtual void foreachOccurrence(index::SymbolRoleSet Roles, 
> llvm::function_ref Receiver) = 0;
>  
> virtual ~ClangdIndexDataSymbol() = default;
>   };
>  
>   class ClangdIndexDataOccurrence {
>   public:
> enum class OccurrenceType : uint16_t {
>OCCURRENCE,
>DEFINITION_OCCURRENCE
>  };
>  
> virtual OccurrenceType getKind() const = 0;
> virtual std::string getPath() = 0;
> /// Get the start offset of the symbol occurrence. The SourceManager can 
> be
> /// used for implementations that need to convert from a line/column
> /// representation to an offset.
> virtual uint32_t getStartOffset(SourceManager ) = 0;
> /// Get the end offset of the symbol occurrence. The SourceManager can be
> /// used for implementations that need to convert from a line/column
> /// representation to an offset.
> virtual uint32_t getEndOffset(SourceManager ) = 0;
> virtual ~ClangdIndexDataOccurrence() = default;
> //TODO: Add relations
>  
> static bool classof(const ClangdIndexDataOccurrence *O) { return 
> O->getKind() == OccurrenceType::OCCURRENCE; }
>   };
>  
>   /// An occurrence that also has definition with a body that requires 
> additional
>   /// locations to keep track of the beginning and end of the body.
>   class ClangdIndexDataDefinitionOccurrence : public 
> ClangdIndexDataOccurrence {
>   public:
> virtual uint32_t getDefStartOffset(SourceManager ) = 0;
> virtual uint32_t getDefEndOffset(SourceManager ) = 0;
>  
> static bool classof(const ClangdIndexDataOccurrence *O) { return 
> O->getKind() == OccurrenceType::DEFINITION_OCCURRENCE; }
>   };
>  
>   class ClangdIndexDataProvider {
>   public:
>  
> virtual void foreachSymbols(StringRef Pattern, 
> llvm::function_ref Receiver) = 0;
> virtual void foreachSymbols(const USR , 
> llvm::function_ref Receiver) = 0;
>  
> virtual ~ClangdIndexDataProvider() = default;
>   };
>


I think some of the ideas here could be useful. This patch focuses mostly on 
index interfaces and https://reviews.llvm.org/D40897 emphasizes on the design 
of symbol structure. The way symbols are stored and used in this patch is 
likely to change depending on how https://reviews.llvm.org/D40897 goes.

> The "Clangd" prefix adds a bit much of clutter so maybe it should be removed. 
>  I think the main points are that having generic 
> foreachSymbols/foreachOccurrence with callbacks is well suited to implement 
> multiple features with minimal copying.

Although I'm not sure if `foreachSymbols`/... would be feasible for all indexes 
yet, we do plan to switch to callback-based index interfaces, which Sam also 
proposed in the review comments.

There have been some offline discussions happening around clangd's indexing, 
and sorry that we have not been able to keep you up to date. I think it might 
be more efficient if we could meet via VC/Hangouts and sync on our designs. If 
you don't mind a meeting, I am happy to arrange it via emails.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40548



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

[PATCH] D40897: [clangd] Introduce a "Symbol" class.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 126115.
hokein marked 4 inline comments as done.
hokein added a comment.

Address review comments.

- Use SymbolSlab, for allowing future space optimization.
- Fix a Cast issue when building debug unittest.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40897

Files:
  clangd/CMakeLists.txt
  clangd/Symbol.cpp
  clangd/Symbol.h
  unittests/clangd/CMakeLists.txt
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- /dev/null
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -0,0 +1,112 @@
+//===-- SymbolCollectorTests.cpp  ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Symbol.h"
+#include "clang/Index/IndexingAction.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+#include "gmock/gmock.h"
+
+#include 
+#include 
+
+using testing::ElementsAre;
+using testing::Eq;
+using testing::Field;
+
+namespace clang {
+namespace clangd {
+
+namespace {
+class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
+ public:
+  SymbolIndexActionFactory() = default;
+
+  clang::FrontendAction *create() override {
+index::IndexingOptions IndexOpts;
+IndexOpts.SystemSymbolFilter =
+index::IndexingOptions::SystemSymbolFilterKind::All;
+IndexOpts.IndexFunctionLocals = false;
+Collector = std::make_shared();
+FrontendAction *Action =
+index::createIndexingAction(Collector, IndexOpts, nullptr).release();
+return Action;
+  }
+
+  std::shared_ptr Collector;
+};
+
+class SymbolCollectorTest : public ::testing::Test {
+public:
+  bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode) {
+llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+new vfs::InMemoryFileSystem);
+llvm::IntrusiveRefCntPtr Files(
+new FileManager(FileSystemOptions(), InMemoryFileSystem));
+
+const std::string FileName = "symbol.cc";
+const std::string HeaderName = "symbols.h";
+auto Factory = llvm::make_unique();
+
+tooling::ToolInvocation Invocation(
+{"symbol_collector", "-fsyntax-only", "-std=c++11", FileName},
+Factory->create(), Files.get(),
+std::make_shared());
+
+InMemoryFileSystem->addFile(HeaderName, 0,
+llvm::MemoryBuffer::getMemBuffer(HeaderCode));
+
+std::string Content = "#include\"" + std::string(HeaderName) + "\"";
+Content += "\n" + MainCode.str();
+InMemoryFileSystem->addFile(FileName, 0,
+llvm::MemoryBuffer::getMemBuffer(Content));
+Invocation.run();
+Symbols = Factory->Collector->takeSymbols();
+
+EXPECT_EQ(FileName, Factory->Collector->getFilename());
+return true;
+  }
+
+protected:
+  SymbolSlab Symbols;
+};
+
+TEST_F(SymbolCollectorTest, CollectSymbol) {
+  const std::string Header = R"(
+class Foo {
+  void f();
+};
+void f1();
+inline void f2() {}
+  )";
+  const std::string Main = R"(
+namespace {
+void ff() {} // ignore
+}
+void f1() {}
+  )";
+  runSymbolCollector(Header, Main);
+  EXPECT_THAT(Symbols,
+  UnorderedElementsAre(Field(::QualifiedName, Eq("Foo")),
+   Field(::QualifiedName, Eq("Foo::f")),
+   Field(::QualifiedName, Eq("f1")),
+   Field(::QualifiedName, Eq("f2";
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -15,6 +15,7 @@
   JSONExprTests.cpp
   TestFS.cpp
   TraceTests.cpp
+  SymbolCollectorTests.cpp
   )
 
 target_link_libraries(ClangdTests
Index: clangd/Symbol.h
===
--- /dev/null
+++ clangd/Symbol.h
@@ -0,0 +1,133 @@
+//===--- Symbol.h ---*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===-===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SYMBOL_H
+#define 

[PATCH] D40909: [clang-format] Reorganize raw string delimiters

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.



Comment at: include/clang/Format/Format.h:1375
+std::vector EnclosingFunctionNames;
+/// \brief The canonical delimiter for this language.
+std::string CanonicalDelimiter;

djasper wrote:
> krasimir wrote:
> > djasper wrote:
> > > Can you pull apart this patch?
> > > 
> > > In my view, it has three parts that have an ordering, but are actually 
> > > fairly independent:
> > > 
> > > 1. Propagate all configured languages to the formatting library. First 
> > > patch to land, should not affect the visible behavior.
> > > 2. Restructure RawStringFormat to be centered around each language. This 
> > > is a restructuring to make things easier and use #1.
> > > 3. Add a CanonicalDelimiter and make clang-format canonicalize it.
> > > 
> > > I'll focus my comments on what's required for #1 for now as that is 
> > > already complicated (IMO).
> > I believe these should all go together: the reason that we propagate all 
> > configured languages to the formatting library is to be able to use them as 
> > a replacement for the BasedOnStyle in RawStringFormat. To make this 
> > possible, we need to update the internal structure of RawStringFormat 
> > itself to base it around each language. The canonical delimiter part is 
> > just a convenience for this I guess, which could be split.
> > 
> > My biggest concern with (1) is that since it has no visible behavior and no 
> > other uses except for the adaptation of (2), it is not testable by itself 
> > and it's not evident that a patch doing just (1) would handle the things 
> > correctly.
> Ok, if you wish, this is not an unreasonable argument. But let's still do the 
> code review in two steps: Lets for now just get the part of handling multiple 
> languages straight and figure out the rest once we are sure that that part is 
> fine.
> 
> (I do think you can test it, though - but it depends on whether I can 
> convince you to go with the FormatStyleSet approach ;) )
On a philosophical level, something that has no visible behavior, and just 
restructures the code, should be tested by existing tests?

Enclosing function names also seems like an extra feature that could be pulled 
out, btw.



Comment at: lib/Format/Format.cpp:920
+  if (LanguageFound) {
+for (int i = Styles.size() - 1; i >= 0; --i) {
+  if (Styles[i].Language == FormatStyle::LK_None) {

djasper wrote:
> krasimir wrote:
> > djasper wrote:
> > > I think this is getting a bit convoluted and I don't even understand 
> > > whether we are doing what is document (even before this patch).
> > > 
> > > So in lines 892-905, we verify that:
> > > - Only the first Style in the file is allowed be LK_None.
> > > - No language is duplicated.
> > > 
> > > That seems good.
> > > 
> > > According to the documentation: "The first section may have no language 
> > > set, it will set the default style options for all lanugages.". Does the 
> > > latter part actually happen? Seems to me that we are just setting "Style" 
> > > to the style configured for a specific language, completely ignoring 
> > > values that might have been set in the LK_None style. Or is that somehow 
> > > happening when reading the JSON?
> > > 
> > > Independent of that, I think we should use this structure more 
> > > explicitly. I think we should create an additional class (FormatStyles or 
> > > FormatStyleSet or something) that is returned by this function and handed 
> > > to the formatting library. This function then doesn't need to look at the 
> > > language anymore.
> > > 
> > > That class should then have a function getFormatStyle(LanguageKind 
> > > Language); that returns the style for a particular language (doing the 
> > > default logic, etc.). Internally, it can likely just have a map > > Style> and I don't think you need to pre-fill that for all language 
> > > kinds. If a language kind is not in the map, you can just return what's 
> > > stored for LK_None. WDYT?
> > Yes, defaulting to the None for missing language specifications is handled 
> > at line 912:
> > ```
> > if (!LanguageFound && (Styles[i].Language == Language ||
> >Styles[i].Language == FormatStyle::LK_None
> > ```
> > 
> > I was thinking of the FormatStyleSet approach but the problem is that this 
> > has repercussions all over the library. We could indeed update this 
> > specific function that way, but fundamentally the additional language 
> > styles are part of the FormatStyle and need to somehow be recorded inside 
> > there. That's why I went with KISS and directly made this function handle 
> > that case.
> But it's not just defaulting to LK_None what we are saying we are 
> implementing. I think the documentation suggestion that we implement very 
> basic inheritance. E.g. if the style for LK_None set the ColumnLimit to 42, I 
> would expect that the styles for all other languages that don't explicitly 
> set 

[PATCH] D40952: [clangd] Convert lit code completion tests to unit-tests. NFC

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: unittests/clangd/CodeCompleteTests.cpp:289
+};
+void Foo::pub() { this->^ }
+  )cpp");

The `^` symbol conflicts with the corresponding operator.
Even though it's not widely used, I'm wondering whether we should use a 
different marker for completion position.



Comment at: unittests/clangd/CodeCompleteTests.cpp:335
+  )cpp",
+ Opts);
+  EXPECT_THAT(Results.items,

The formatting is a bit weird. Is this a `clang-format` bug?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40952



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

[PATCH] D40712: Add cc1 flag enabling the function stack size section that was added in r319430

[Sean Eveson via Phabricator via cfe-commits]

seaneveson added a comment.

Ping.


https://reviews.llvm.org/D40712



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

[PATCH] D40489: [clangd] Changed tracing interfaces

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clangd/Trace.cpp:138
 return;
-  if (!Args)
-Args = llvm::make_unique();
-  T->event(Ctx, "E",
-   Args ? json::obj{{"args", std::move(*Args)}} : json::obj{});
+  assert(Args && "Args can't be null at this point");
+  T->end_event(Ctx, Name, std::move(*Args));

ilya-biryukov wrote:
> sammccall wrote:
> > why not?
> Because `T` must outlive the `Span`, so we can't really have `if (!T)` 
> evaluate to different things in constructor and destructor.
> Am I missing something?
I was being disingenuous, I agree with you.
But can you change the message to "Changed tracer during a span"?

assert(Args) and "Args can't be null at this point" are basically synonyms :-)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40489



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

[PATCH] D40909: [clang-format] Reorganize raw string delimiters

[Daniel Jasper via Phabricator via cfe-commits]

djasper added inline comments.



Comment at: include/clang/Format/Format.h:1375
+std::vector EnclosingFunctionNames;
+/// \brief The canonical delimiter for this language.
+std::string CanonicalDelimiter;

krasimir wrote:
> djasper wrote:
> > Can you pull apart this patch?
> > 
> > In my view, it has three parts that have an ordering, but are actually 
> > fairly independent:
> > 
> > 1. Propagate all configured languages to the formatting library. First 
> > patch to land, should not affect the visible behavior.
> > 2. Restructure RawStringFormat to be centered around each language. This is 
> > a restructuring to make things easier and use #1.
> > 3. Add a CanonicalDelimiter and make clang-format canonicalize it.
> > 
> > I'll focus my comments on what's required for #1 for now as that is already 
> > complicated (IMO).
> I believe these should all go together: the reason that we propagate all 
> configured languages to the formatting library is to be able to use them as a 
> replacement for the BasedOnStyle in RawStringFormat. To make this possible, 
> we need to update the internal structure of RawStringFormat itself to base it 
> around each language. The canonical delimiter part is just a convenience for 
> this I guess, which could be split.
> 
> My biggest concern with (1) is that since it has no visible behavior and no 
> other uses except for the adaptation of (2), it is not testable by itself and 
> it's not evident that a patch doing just (1) would handle the things 
> correctly.
Ok, if you wish, this is not an unreasonable argument. But let's still do the 
code review in two steps: Lets for now just get the part of handling multiple 
languages straight and figure out the rest once we are sure that that part is 
fine.

(I do think you can test it, though - but it depends on whether I can convince 
you to go with the FormatStyleSet approach ;) )



Comment at: lib/Format/Format.cpp:920
+  if (LanguageFound) {
+for (int i = Styles.size() - 1; i >= 0; --i) {
+  if (Styles[i].Language == FormatStyle::LK_None) {

krasimir wrote:
> djasper wrote:
> > I think this is getting a bit convoluted and I don't even understand 
> > whether we are doing what is document (even before this patch).
> > 
> > So in lines 892-905, we verify that:
> > - Only the first Style in the file is allowed be LK_None.
> > - No language is duplicated.
> > 
> > That seems good.
> > 
> > According to the documentation: "The first section may have no language 
> > set, it will set the default style options for all lanugages.". Does the 
> > latter part actually happen? Seems to me that we are just setting "Style" 
> > to the style configured for a specific language, completely ignoring values 
> > that might have been set in the LK_None style. Or is that somehow happening 
> > when reading the JSON?
> > 
> > Independent of that, I think we should use this structure more explicitly. 
> > I think we should create an additional class (FormatStyles or 
> > FormatStyleSet or something) that is returned by this function and handed 
> > to the formatting library. This function then doesn't need to look at the 
> > language anymore.
> > 
> > That class should then have a function getFormatStyle(LanguageKind 
> > Language); that returns the style for a particular language (doing the 
> > default logic, etc.). Internally, it can likely just have a map 
> > and I don't think you need to pre-fill that for all language kinds. If a 
> > language kind is not in the map, you can just return what's stored for 
> > LK_None. WDYT?
> Yes, defaulting to the None for missing language specifications is handled at 
> line 912:
> ```
> if (!LanguageFound && (Styles[i].Language == Language ||
>Styles[i].Language == FormatStyle::LK_None
> ```
> 
> I was thinking of the FormatStyleSet approach but the problem is that this 
> has repercussions all over the library. We could indeed update this specific 
> function that way, but fundamentally the additional language styles are part 
> of the FormatStyle and need to somehow be recorded inside there. That's why I 
> went with KISS and directly made this function handle that case.
But it's not just defaulting to LK_None what we are saying we are implementing. 
I think the documentation suggestion that we implement very basic inheritance. 
E.g. if the style for LK_None set the ColumnLimit to 42, I would expect that 
the styles for all other languages that don't explicitly set a ColumnLimit 
would also use 42. I don't think this is currently implemented and I don't 
think this patch implements it. But I think we should :).

I agree that the FormatStyleSet approach would have some consequences, but I 
also think that it is much cleaner. Your current solution feels like we us 
working around technical debt and creating more technical debt to do it :(. 
Maybe Manuel has thoughts here?

[PATCH] D39375: [clang] Add PPCallbacks list to preprocessor when building a preacompiled preamble.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: lib/Frontend/PrecompiledPreamble.cpp:242
 std::shared_ptr PCHContainerOps, bool 
StoreInMemory,
-PreambleCallbacks ) {
+PreambleCallbacks , std::unique_ptr PPCallbacks) {
   assert(VFS && "VFS is null");

Nebiroth wrote:
> ilya-biryukov wrote:
> > Could we add a method to `PreambleCallbacks` to create `PPCallbacks` 
> > instead?
> > Otherwise we have both `MacroDefined` in `PreambleCallbacks` and a separate 
> > set of PPCallbacks, so we have two ways of doing the same thing.
> > 
> > ```
> > class PreambleCallbacks {
> > public:
> > // ...
> > 
> >   /// Remove this.
> >   virtual void HandleMacroDefined(...);
> > 
> >   // Add this instead.
> >   virtual std::unique_ptr createPPCallbacks();
> > 
> > }
> > ```
> > 
> > Alternatively, follow the current pattern in `PreambleCallbacks` and add 
> > some extra functions from the `PPCallbacks` interface to it. This would not 
> > require changes to the existing usages of `PrecompiledPreamble` in 
> > `ASTUnit`. Albeit, I'd prefer the first option.
> > ```
> > class PreambleCallbacks {
> > public:
> > // ...
> > 
> >   // Keep this
> >   virtual void HandleMacroDefined(...);
> >   // Add the ones you need, e.g.
> >   virtual void InclusionDirective(...);
> >   virtual void FileChanged(...);
> > };
> > ```
> If we were to do that, one would then be required to define a wrapper class 
> for PPCallbacks and create an instance of it inside createPPCallbacks() for 
> the purpose of creating a unique_ptr? Then that unique_ptr would be sent from 
> within the PreambleCallbacks parameter in the Build function?
We're already passing our own wrapper around `PreambleCallbacks` anyway (see 
`PreambleMacroCallbacks`), we'll pass the `unique_ptr` instead.
But, yes, the clients will have to write their own implementation of 
`PPCallbacks` and pass it as `unique_ptr`. Is there something wrong with that?

Or, have I misunderstood the question entirely?


Repository:
  rC Clang

https://reviews.llvm.org/D39375



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

[PATCH] D40952: [clangd] Convert lit code completion tests to unit-tests. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 126114.
sammccall marked an inline comment as done.
sammccall added a comment.

Reformat around code-complete testcases


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40952

Files:
  test/clangd/completion-items-kinds.test
  test/clangd/completion-priorities.test
  test/clangd/completion-qualifiers.test
  test/clangd/completion-snippet.test
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/Matchers.h

Index: unittests/clangd/Matchers.h
===
--- /dev/null
+++ unittests/clangd/Matchers.h
@@ -0,0 +1,103 @@
+//===-- Matchers.h --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// GMock matchers that aren't specific to particular tests.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_MATCHERS_H
+#define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_MATCHERS_H
+#include "gmock/gmock.h"
+
+namespace clang {
+namespace clangd {
+using ::testing::Matcher;
+
+// EXPECT_IFF expects matcher if condition is true, and Not(matcher) if false.
+// This is hard to write as a function, because matchers may be polymorphic.
+#define EXPECT_IFF(condition, value, matcher)  \
+  do { \
+if (condition) \
+  EXPECT_THAT(value, matcher); \
+else   \
+  EXPECT_THAT(value, ::testing::Not(matcher)); \
+  } while (0)
+
+// HasSubsequence(m1, m2, ...) matches a vector containing elements that match
+// m1, m2 ... in that order.
+template 
+class SubsequenceMatcher
+: public ::testing::MatcherInterface &> {
+  std::vector Matchers;
+
+public:
+  SubsequenceMatcher(std::vector M) : Matchers(M) {}
+
+  void DescribeTo(std::ostream *OS) const {
+*OS << "Contains the subsequence [";
+const char *Sep = "";
+for (const auto  : Matchers) {
+  *OS << Sep;
+  M.DescribeTo(OS);
+  Sep = ", ";
+}
+*OS << "]";
+  }
+
+  bool MatchAndExplain(const std::vector ,
+   ::testing::MatchResultListener *L) const {
+std::vector Matches(Matchers.size());
+size_t I = 0;
+for (size_t J = 0; I < Matchers.size() && J < V.size(); ++J)
+  if (Matchers[I].Matches(V[J]))
+Matches[I++] = J;
+if (I == Matchers.size()) // We exhausted all matchers.
+  return true;
+if (L->IsInterested()) {
+  *L << "\n  Matched:";
+  for (size_t K = 0; K < I; ++K) {
+*L << "\n\t";
+Matchers[K].DescribeTo(L->stream());
+*L << " ==> " << ::testing::PrintToString(V[Matches[K]]);
+  }
+  *L << "\n\t";
+  Matchers[I].DescribeTo(L->stream());
+  *L << " ==> no subsequent match";
+}
+return false;
+  }
+};
+
+template  class PolySubsequenceMatcher {
+  std::tuple Matchers;
+
+public:
+  PolySubsequenceMatcher(M &&... Args)
+  : Matchers(std::make_tuple(std::forward(Args)...)) {}
+
+  template  operator Matcher &>() const {
+return ::testing::MakeMatcher(new SubsequenceMatcher(
+TypedMatchers(llvm::index_sequence_for{})));
+  }
+
+private:
+  template 
+  std::vector TypedMatchers(llvm::index_sequence) const {
+return {std::get(Matchers)...};
+  }
+};
+
+template 
+PolySubsequenceMatcher HasSubsequence(Args &&... M) {
+  return PolySubsequenceMatcher(std::forward(M)...);
+}
+
+} // namespace clangd
+} // namespace clang
+#endif
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -8,6 +8,7 @@
 //===--===//
 #include "ClangdServer.h"
 #include "Compiler.h"
+#include "Matchers.h"
 #include "Protocol.h"
 #include "TestFS.h"
 #include "gmock/gmock.h"
@@ -18,15 +19,23 @@
 // Let GMock print completion items.
 void PrintTo(const CompletionItem , std::ostream *O) {
   llvm::raw_os_ostream OS(*O);
-  OS << toJSON(I);
+  OS << I.label << " - " << toJSON(I);
+}
+void PrintTo(const std::vector , std::ostream *O) {
+  *O << "{\n";
+  for (const auto  : V) {
+*O << "\t";
+PrintTo(I, O);
+*O << "\n";
+  }
+  *O << "}";
 }
 
 namespace {
 using namespace llvm;
 using ::testing::AllOf;
 using ::testing::Contains;
 using ::testing::ElementsAre;
-using ::testing::Matcher;
 using 

[PATCH] D39375: [clang] Add PPCallbacks list to preprocessor when building a preacompiled preamble.

[William Enright via Phabricator via cfe-commits]

Nebiroth added inline comments.



Comment at: lib/Frontend/PrecompiledPreamble.cpp:242
 std::shared_ptr PCHContainerOps, bool 
StoreInMemory,
-PreambleCallbacks ) {
+PreambleCallbacks , std::unique_ptr PPCallbacks) {
   assert(VFS && "VFS is null");

ilya-biryukov wrote:
> Nebiroth wrote:
> > ilya-biryukov wrote:
> > > Could we add a method to `PreambleCallbacks` to create `PPCallbacks` 
> > > instead?
> > > Otherwise we have both `MacroDefined` in `PreambleCallbacks` and a 
> > > separate set of PPCallbacks, so we have two ways of doing the same thing.
> > > 
> > > ```
> > > class PreambleCallbacks {
> > > public:
> > > // ...
> > > 
> > >   /// Remove this.
> > >   virtual void HandleMacroDefined(...);
> > > 
> > >   // Add this instead.
> > >   virtual std::unique_ptr createPPCallbacks();
> > > 
> > > }
> > > ```
> > > 
> > > Alternatively, follow the current pattern in `PreambleCallbacks` and add 
> > > some extra functions from the `PPCallbacks` interface to it. This would 
> > > not require changes to the existing usages of `PrecompiledPreamble` in 
> > > `ASTUnit`. Albeit, I'd prefer the first option.
> > > ```
> > > class PreambleCallbacks {
> > > public:
> > > // ...
> > > 
> > >   // Keep this
> > >   virtual void HandleMacroDefined(...);
> > >   // Add the ones you need, e.g.
> > >   virtual void InclusionDirective(...);
> > >   virtual void FileChanged(...);
> > > };
> > > ```
> > If we were to do that, one would then be required to define a wrapper class 
> > for PPCallbacks and create an instance of it inside createPPCallbacks() for 
> > the purpose of creating a unique_ptr? Then that unique_ptr would be sent 
> > from within the PreambleCallbacks parameter in the Build function?
> We're already passing our own wrapper around `PreambleCallbacks` anyway (see 
> `PreambleMacroCallbacks`), we'll pass the `unique_ptr` instead.
> But, yes, the clients will have to write their own implementation of 
> `PPCallbacks` and pass it as `unique_ptr`. Is there something wrong with that?
> 
> Or, have I misunderstood the question entirely?
No this is fine. I was just making sure I understood correctly.


Repository:
  rC Clang

https://reviews.llvm.org/D39375



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

[PATCH] D14104: clang-format: Add an additional value to AlignAfterOpenBracket: AlwaysBreak.

[Paweł Bylica via Phabricator via cfe-commits]

chfast added inline comments.



Comment at: cfe/trunk/docs/ClangFormatStyleOptions.rst:173
+
+  someLongFunction(argument1,
+  argument2);

I think this example is not achievable. clang-format seems to prefer putting 
all arguments on the next line if they all fit there. So this one will be 
formatted the same as the example for `AlwaysBreak`. 


Repository:
  rL LLVM

https://reviews.llvm.org/D14104



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

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.

This looks useful. Could we avoid creating the `OverlayFileSystem`, though?




Comment at: include/clang/Frontend/PrecompiledPreamble.h:109
 
+  std::chrono::steady_clock::time_point getCreationTimePoint() const {
+return CreationTimePoint;

Having this time stamp in the interface of `PrecompiledPreamble` doesn't really 
makes sense.

I propose we remove this method and test in a different way instead. 

For example, we could add a counter to `ASTUnit` that increases when preamble 
is built and check this counter.
Or we could add a unit-test that uses `PrecompiledPreamble` directly.



Comment at: lib/Frontend/PrecompiledPreamble.cpp:437
 
+  vfs::OverlayFileSystem OFS(VFS);
+  IntrusiveRefCntPtr IMFS(

Can we do the same thing without creating an additional `OverlayFileSystem`?

If we add a separate map to check for those:
```
llvm::StringMap OverriddenFilesWithoutFS.
// Populate the map with paths not existing in VFS.

for (const auto  : FilesInPreamble) {
vfs::Status Status;
if (!moveOnNoError(OFS.status(F.first()), Status)) {
  // If we can't stat the file, check whether it was overriden
  auto It = OverriddenFilesWithoutFS[F.first()];
  if (It == OverriddenFilesWithoutFS.end())
return false;  
  //.
}
//..

}
```



Comment at: lib/Frontend/PrecompiledPreamble.cpp:444
 vfs::Status Status;
-if (!moveOnNoError(VFS->status(RB.first), Status))
-  return false;
-
+assert(moveOnNoError(IMFS->status(RB.first), Status));
 OverriddenFiles[Status.getUniqueID()] =

`assert` macro is a no-op when `NDEBUG` is defined.
One must never put an operation with side-effects inside `assert`.


Repository:
  rC Clang

https://reviews.llvm.org/D41005



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

[PATCH] D40897: [clangd] Introduce a "Symbol" class.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 126156.
hokein marked 6 inline comments as done.
hokein added a comment.

- reorganize files, move to index subdirectory.
- change symbol ID to a hash value, instead of couple with USR.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40897

Files:
  clangd/CMakeLists.txt
  clangd/index/CMakeLists.txt
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolCollector.h
  unittests/clangd/CMakeLists.txt
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- /dev/null
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -0,0 +1,112 @@
+//===-- SymbolCollectorTests.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/SymbolCollector.h"
+#include "clang/Index/IndexingAction.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "gtest/gtest.h"
+#include "gmock/gmock.h"
+
+#include 
+#include 
+
+using testing::UnorderedElementsAre;
+using testing::Eq;
+using testing::Field;
+
+// GMock helpers for matching Symbol.
+MATCHER_P(QName, Name, "") { return arg.QualifiedName == Name; }
+
+namespace clang {
+namespace clangd {
+
+namespace {
+class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
+ public:
+  SymbolIndexActionFactory() = default;
+
+  clang::FrontendAction *create() override {
+index::IndexingOptions IndexOpts;
+IndexOpts.SystemSymbolFilter =
+index::IndexingOptions::SystemSymbolFilterKind::All;
+IndexOpts.IndexFunctionLocals = false;
+Collector = std::make_shared();
+FrontendAction *Action =
+index::createIndexingAction(Collector, IndexOpts, nullptr).release();
+return Action;
+  }
+
+  std::shared_ptr Collector;
+};
+
+class SymbolCollectorTest : public ::testing::Test {
+public:
+  bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode) {
+llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+new vfs::InMemoryFileSystem);
+llvm::IntrusiveRefCntPtr Files(
+new FileManager(FileSystemOptions(), InMemoryFileSystem));
+
+const std::string FileName = "symbol.cc";
+const std::string HeaderName = "symbols.h";
+auto Factory = llvm::make_unique();
+
+tooling::ToolInvocation Invocation(
+{"symbol_collector", "-fsyntax-only", "-std=c++11", FileName},
+Factory->create(), Files.get(),
+std::make_shared());
+
+InMemoryFileSystem->addFile(HeaderName, 0,
+llvm::MemoryBuffer::getMemBuffer(HeaderCode));
+
+std::string Content = "#include\"" + std::string(HeaderName) + "\"";
+Content += "\n" + MainCode.str();
+InMemoryFileSystem->addFile(FileName, 0,
+llvm::MemoryBuffer::getMemBuffer(Content));
+Invocation.run();
+Symbols = Factory->Collector->takeSymbols();
+
+EXPECT_EQ(FileName, Factory->Collector->getFilename());
+return true;
+  }
+
+protected:
+  SymbolSlab Symbols;
+};
+
+TEST_F(SymbolCollectorTest, CollectSymbol) {
+  const std::string Header = R"(
+class Foo {
+  void f();
+};
+void f1();
+inline void f2() {}
+  )";
+  const std::string Main = R"(
+namespace {
+void ff() {} // ignore
+}
+void f1() {}
+  )";
+  runSymbolCollector(Header, Main);
+  EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Foo"), QName("Foo::f"),
+QName("f1"), QName("f2")));
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -15,12 +15,14 @@
   JSONExprTests.cpp
   TestFS.cpp
   TraceTests.cpp
+  SymbolCollectorTests.cpp
   )
 
 target_link_libraries(ClangdTests
   PRIVATE
   clangBasic
   clangDaemon
+  clangdIndex
   clangFormat
   clangFrontend
   clangSema
Index: clangd/index/SymbolCollector.h
===
--- /dev/null
+++ clangd/index/SymbolCollector.h
@@ -0,0 +1,50 @@
+//===--- SymbolCollector.h ---*- C++-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. 

[PATCH] D40897: [clangd] Introduce a "Symbol" class.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Reorganizing the source files made all the comments invalid in the latest 
version :(. Feel free to comment on the old version or the new version.




Comment at: clangd/Symbol.h:23
+  // The path of the source file where a symbol occurs.
+  std::string FilePath;
+  // The offset to the first character of the symbol from the beginning of the

sammccall wrote:
> hokein wrote:
> > malaperle wrote:
> > > malaperle wrote:
> > > > sammccall wrote:
> > > > > ioeric wrote:
> > > > > > Is this relative or absolute?
> > > > > Having every symbol own a copy of the filepath seems wasteful.
> > > > > It seems likely that the index will have per-file information too, so 
> > > > > this representation should likely be a key to that. Hash of the 
> > > > > filepath might work?
> > > > How we model it is that a symbol doesn't have a "location", but its 
> > > > occurrence do. One could consider the location of a symbol to be either 
> > > > its declaration occurrence (SymbolRole::Declaration) or its definition 
> > > > (SymbolRole::Definition).
> > > > What we do to get the location path is each occurrence has a pointer (a 
> > > > "database" pointer, but it doesn't matter) to a file entry and then we 
> > > > get the path from the entry.
> > > > 
> > > > So conceptually, it works a bit like this (although it fetches 
> > > > information on disk).
> > > > ```
> > > > class IndexOccurrence {
> > > > IndexOccurrence *FilePtr;
> > > > 
> > > > std::string Occurrence::getPath() {
> > > >   return FilePtr->getPath();
> > > > }
> > > > };
> > > > ```
> > > Oops, wrong type for the field, it should have been:
> > > ```
> > > class IndexOccurrence {
> > > IndexFile *FilePtr;
> > > 
> > > std::string Occurrence::getPath() {
> > >   return FilePtr->getPath();
> > > }
> > > };
> > > ```
> > > Is this relative or absolute?
> > 
> >  Whether the file path is relative or absolute depends on the build system, 
> > the file path could be relative (for header files) or absolute (for .cc 
> > files).
> > 
> > > How we model it is that a symbol doesn't have a "location", but its 
> > > occurrence do.
> > 
> > We will also have a SymbolOccurence structure alongside with Symbol (but it 
> > is not  in this patch). The "Location" will be a part of SymbolOccurrence.
> >  
> > Whether the file path is relative or absolute depends on the build system, 
> > the file path could be relative (for header files) or absolute (for .cc 
> > files).
> 
> I'm not convinced this actually works. There's multiple codepaths to the 
> index, how can we ensure we don't end up using inconsistent paths?
> e.g. we open up a project that includes a system header using a relative 
> path, and then open up that system header from file->open (editor knows only 
> the absolute path) and do "find references".
> 
> I think we need to canonicalize the paths. Absolute is probably easiest.
Absolute path for .cc file is fine, I was a bit concerned about the path for .h 
file, especially we might use it in `#include`, but we can figure out later.  
Changed to absolute file path.



Comment at: clangd/Symbol.h:51
+  //   * For classes, the canonical location is where they are defined.
+  SymbolLocation CanonicalLocation;
+  // Information for code completion.

sammccall wrote:
> hokein wrote:
> > ioeric wrote:
> > > For functions and classes, should we store both declaration and 
> > > definition locations?
> > I think the symbol occurrences would include both declaration and 
> > definition locations. 
> > 
> > The `CanonicalLocation` is providing a fast/convenient way to find the most 
> > interested location of the symbol (e.g. for code navigation, or include the 
> > missing path for a symbol), without symbol occurrences. 
> I'd be +1 on including both a definition location (if known) and a preferred 
> declaration location, because there's enough use cases that might want 
> definition even if it's not the preferred declaration.
> 
> But i'm fine if we want to omit the separate definition for now. In that 
> case, call this CanonicalDeclaration?
OK, changed it to `CanonicalDeclarationLoc`, and added a FIXME for the 
definition.



Comment at: clangd/Symbol.h:68
+// changed.
+class SymbolCollector : public index::IndexDataConsumer {
+public:

sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > Please pull this into a separate file. Someone providing e.g. symbols 
> > > from a YAML file shouldn't need to pull in AST stuff.
> > > Mabye `IndexFromAST`, which would sort nicely next to `Index`?
> > I can see various meaning of "Index" here:
> > 
> > 1. `Index` in `index::IndexDataConsumer`, which collects and contructs all 
> > symbols by traversing the AST.
> > 2. `Index` term using in clangd, specially for build index on top of these 
> > collected symbols.
> > 
> > I think we should be consistent the index for 2), and `SymbolCollector` is 
> > more 

Re: [PATCH] D40548: [clangd] Symbol index interfaces and index-based code completion.

[Marc-André Laperle via cfe-commits]

A Hangouts meeting sounds good! Yes, let's arrange via emails.


From: Haojian Wu via Phabricator 
Sent: Friday, December 8, 2017 7:14:42 AM
To: ioe...@google.com; Marc-André Laperle; sammcc...@google.com
Cc: hok...@google.com; kli...@google.com; mgo...@gentoo.org; 
ibiryu...@google.com; cfe-commits@lists.llvm.org
Subject: [PATCH] D40548: [clangd] Symbol index interfaces and index-based code 
completion.

hokein added a comment.

Thanks for the feedback, Marc!

Yeah, I think the ClangdIndexDataSymbol and ClangdIndexDataOccurrence are 
something similar to what https://reviews.llvm.org/D40897 trying to address, 
maybe put the discussion there? Before that, I think having a sym meeting is a 
good idea to keep us in the same page.

In https://reviews.llvm.org/D40548#949279, @ioeric wrote:

> >
>
> I think some of the ideas here could be useful. This patch focuses mostly on 
> index interfaces and https://reviews.llvm.org/D40897 emphasizes on the design 
> of symbol structure. The way symbols are stored and used in this patch is 
> likely to change depending on how https://reviews.llvm.org/D40897 goes.
>
> > The "Clangd" prefix adds a bit much of clutter so maybe it should be 
> > removed.  I think the main points are that having generic 
> > foreachSymbols/foreachOccurrence with callbacks is well suited to implement 
> > multiple features with minimal copying.
>
> Although I'm not sure if `foreachSymbols`/... would be feasible for all 
> indexes yet, we do plan to switch to callback-based index interfaces, which 
> Sam also proposed in the review comments.
>
> There have been some offline discussions happening around clangd's indexing, 
> and sorry that we have not been able to keep you up to date. I think it might 
> be more efficient if we could meet via VC/Hangouts and sync on our designs. 
> If you don't mind a meeting, I am happy to arrange it via emails.





Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40548



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

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

It's been a while since I was in this code, but I seem to recall the file 
needed to exist on disk in order to uniquely identify it (via inode). Does this 
break the up-to-date check?


Repository:
  rC Clang

https://reviews.llvm.org/D41005



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

Re: r305110 - [ODRHash] Add support for TemplateArgument types.

[Vassil Vassilev via cfe-commits]

Hi Richard,

  Is there a way to get an ODRHashing which is stable across 
translation units? I'd like to use the TemplateArgument ODRHash to 
lookup template specializations and deserialize them only if they are 
required.


Many thanks!
Vassil
On 6/9/17 11:00 PM, Richard Trieu via cfe-commits wrote:

Author: rtrieu
Date: Fri Jun  9 16:00:10 2017
New Revision: 305110

URL: http://llvm.org/viewvc/llvm-project?rev=305110=rev
Log:
[ODRHash] Add support for TemplateArgument types.

Recommit r304592 that was reverted in r304618.  r305104 should have fixed the
issue.

Modified:
 cfe/trunk/lib/AST/ODRHash.cpp
 cfe/trunk/test/Modules/odr_hash.cpp

Modified: cfe/trunk/lib/AST/ODRHash.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ODRHash.cpp?rev=305110=305109=305110=diff
==
--- cfe/trunk/lib/AST/ODRHash.cpp (original)
+++ cfe/trunk/lib/AST/ODRHash.cpp Fri Jun  9 16:00:10 2017
@@ -140,7 +140,25 @@ void ODRHash::AddTemplateName(TemplateNa
}
  }
  
-void ODRHash::AddTemplateArgument(TemplateArgument TA) {}

+void ODRHash::AddTemplateArgument(TemplateArgument TA) {
+  auto Kind = TA.getKind();
+  ID.AddInteger(Kind);
+
+  switch (Kind) {
+  case TemplateArgument::Null:
+  case TemplateArgument::Declaration:
+  case TemplateArgument::NullPtr:
+  case TemplateArgument::Integral:
+  case TemplateArgument::Template:
+  case TemplateArgument::TemplateExpansion:
+  case TemplateArgument::Expression:
+  case TemplateArgument::Pack:
+break;
+  case TemplateArgument::Type:
+AddQualType(TA.getAsType());
+break;
+  }
+}
  void ODRHash::AddTemplateParameterList(const TemplateParameterList *TPL) {}
  
  void ODRHash::clear() {


Modified: cfe/trunk/test/Modules/odr_hash.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/odr_hash.cpp?rev=305110=305109=305110=diff
==
--- cfe/trunk/test/Modules/odr_hash.cpp (original)
+++ cfe/trunk/test/Modules/odr_hash.cpp Fri Jun  9 16:00:10 2017
@@ -900,6 +900,24 @@ S2 s2;
  #endif
  }
  
+namespace TemplateArgument {

+#if defined(FIRST)
+template struct U1 {};
+struct S1 {
+  U1 u;
+};
+#elif defined(SECOND)
+template struct U1 {};
+struct S1 {
+  U1 u;
+};
+#else
+S1 s1;
+// expected-error@first.h:* {{'TemplateArgument::S1::u' from module 
'FirstModule' is not present in definition of 'TemplateArgument::S1' in module 
'SecondModule'}}
+// expected-note@second.h:* {{declaration of 'u' does not match}}
+#endif
+}
+
  // Interesting cases that should not cause errors.  struct S should not error
  // while struct T should error at the access specifier mismatch at the end.
  namespace AllDecls {


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



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

[PATCH] D40685: [libunwind] Switch to add_llvm_install_targets

[Will Dietz via Phabricator via cfe-commits]

dtzWill added a comment.

Hmm, this change means very recent LLVM is required to build libunwind-- 
previously I was able to use recent libunwind/libcxx/libcxxabi with LLVM 5 and 
LLVM 4.

Other runtime projects (compiler-rt, libcxx, libcxxabi) were modified to 
support the new "strip-and-install" targets such that they didn't depend on 
LLVM cmake bits, could that sort of approach work here?


Repository:
  rL LLVM

https://reviews.llvm.org/D40685



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

[PATCH] D40372: [ExprConstant] Fix assert when initializing constexpr array

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

Ping!


https://reviews.llvm.org/D40372



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

[PATCH] D40952: [clangd] Convert lit code completion tests to unit-tests. NFC

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL320148: [clangd] Convert lit code completion tests to 
unit-tests. NFC (authored by sammccall).

Repository:
  rL LLVM

https://reviews.llvm.org/D40952

Files:
  clang-tools-extra/trunk/test/clangd/completion-items-kinds.test
  clang-tools-extra/trunk/test/clangd/completion-priorities.test
  clang-tools-extra/trunk/test/clangd/completion-qualifiers.test
  clang-tools-extra/trunk/test/clangd/completion-snippet.test
  clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
  clang-tools-extra/trunk/unittests/clangd/Matchers.h

Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -8,6 +8,7 @@
 //===--===//
 #include "ClangdServer.h"
 #include "Compiler.h"
+#include "Matchers.h"
 #include "Protocol.h"
 #include "TestFS.h"
 #include "gmock/gmock.h"
@@ -18,15 +19,23 @@
 // Let GMock print completion items.
 void PrintTo(const CompletionItem , std::ostream *O) {
   llvm::raw_os_ostream OS(*O);
-  OS << toJSON(I);
+  OS << I.label << " - " << toJSON(I);
+}
+void PrintTo(const std::vector , std::ostream *O) {
+  *O << "{\n";
+  for (const auto  : V) {
+*O << "\t";
+PrintTo(I, O);
+*O << "\n";
+  }
+  *O << "}";
 }
 
 namespace {
 using namespace llvm;
 using ::testing::AllOf;
 using ::testing::Contains;
 using ::testing::ElementsAre;
-using ::testing::Matcher;
 using ::testing::Not;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
@@ -57,22 +66,25 @@
 
 // GMock helpers for matching completion items.
 MATCHER_P(Named, Name, "") { return arg.insertText == Name; }
+MATCHER_P(Labeled, Label, "") { return arg.label == Label; }
+MATCHER_P(Kind, K, "") { return arg.kind == K; }
+MATCHER_P(PlainText, Text, "") {
+  return arg.insertTextFormat == clangd::InsertTextFormat::PlainText &&
+ arg.insertText == Text;
+}
+MATCHER_P(Snippet, Text, "") {
+  return arg.insertTextFormat == clangd::InsertTextFormat::Snippet &&
+ arg.insertText == Text;
+}
 // Shorthand for Contains(Named(Name)).
 Matcher &> Has(std::string Name) {
   return Contains(Named(std::move(Name)));
 }
-MATCHER(IsDocumented, "") { return !arg.documentation.empty(); }
-MATCHER(IsSnippet, "") {
-  return arg.kind == clangd::CompletionItemKind::Snippet;
+Matcher &> Has(std::string Name,
+ CompletionItemKind K) {
+  return Contains(AllOf(Named(std::move(Name)), Kind(K)));
 }
-// This is hard to write as a function, because matchers may be polymorphic.
-#define EXPECT_IFF(condition, value, matcher)  \
-  do { \
-if (condition) \
-  EXPECT_THAT(value, matcher); \
-else   \
-  EXPECT_THAT(value, ::testing::Not(matcher)); \
-  } while (0)
+MATCHER(IsDocumented, "") { return !arg.documentation.empty(); }
 
 CompletionList completions(StringRef Text,
clangd::CodeCompleteOptions Opts = {}) {
@@ -133,93 +145,97 @@
 }
 
 void TestAfterDotCompletion(clangd::CodeCompleteOptions Opts) {
-  auto Results = completions(R"cpp(
-#define MACRO X
+  auto Results = completions(
+  R"cpp(
+  #define MACRO X
 
-int global_var;
+  int global_var;
 
-int global_func();
+  int global_func();
 
-struct GlobalClass {};
+  struct GlobalClass {};
 
-struct ClassWithMembers {
-  /// Doc for method.
-  int method();
+  struct ClassWithMembers {
+/// Doc for method.
+int method();
 
-  int field;
-private:
-  int private_field;
-};
+int field;
+  private:
+int private_field;
+  };
 
-int test() {
-  struct LocalClass {};
+  int test() {
+struct LocalClass {};
 
-  /// Doc for local_var.
-  int local_var;
+/// Doc for local_var.
+int local_var;
 
-  ClassWithMembers().^
-}
-)cpp",
- Opts)
- .items;
+ClassWithMembers().^
+  }
+  )cpp",
+  Opts);
 
   // Class members. The only items that must be present in after-dot
   // completion.
-  EXPECT_THAT(Results, AllOf(Has(Opts.EnableSnippets ? "method()" : "method"),
- Has("field")));
-  EXPECT_IFF(Opts.IncludeIneligibleResults, Results, Has("private_field"));
+  EXPECT_THAT(
+  Results.items,
+  AllOf(Has(Opts.EnableSnippets ? "method()" : "method"), Has("field")));
+  EXPECT_IFF(Opts.IncludeIneligibleResults, Results.items,
+ 

[clang-tools-extra] r320148 - [clangd] Convert lit code completion tests to unit-tests. NFC

[Sam McCall via cfe-commits]

Author: sammccall
Date: Fri Dec  8 07:00:59 2017
New Revision: 320148

URL: http://llvm.org/viewvc/llvm-project?rev=320148=rev
Log:
[clangd] Convert lit code completion tests to unit-tests. NFC

Summary: This improves readability of tests and error messages.

Reviewers: ioeric

Subscribers: klimek, ilya-biryukov, cfe-commits

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

Added:
clang-tools-extra/trunk/unittests/clangd/Matchers.h
Removed:
clang-tools-extra/trunk/test/clangd/completion-items-kinds.test
clang-tools-extra/trunk/test/clangd/completion-priorities.test
clang-tools-extra/trunk/test/clangd/completion-qualifiers.test
clang-tools-extra/trunk/test/clangd/completion-snippet.test
Modified:
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp

Removed: clang-tools-extra/trunk/test/clangd/completion-items-kinds.test
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/completion-items-kinds.test?rev=320147=auto
==
--- clang-tools-extra/trunk/test/clangd/completion-items-kinds.test (original)
+++ clang-tools-extra/trunk/test/clangd/completion-items-kinds.test (removed)
@@ -1,42 +0,0 @@
-# RUN: clangd -enable-snippets -run-synchronously < %s | FileCheck %s
-# It is absolutely vital that this file has CRLF line endings.
-#
-Content-Length: 125
-
-{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
-Content-Length: 220
-
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define
 MACRO X\nint variable;\nstruct Struct {};\n int function();\nint X = "}}}
-Content-Length: 148
-
-{"jsonrpc":"2.0","id":1,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":4,"character":7}}}
-# CHECK: {"id":1,"jsonrpc":"2.0","result":{"isIncomplete":false,"items":
-#
-# Function
-# CHECK: 
{"detail":"int","filterText":"function","insertText":"function()","insertTextFormat":1,"kind":3,"label":"function()","sortText":"{{.*}}function"}
-#
-# Variable
-# CHECK: 
{"detail":"int","filterText":"variable","insertText":"variable","insertTextFormat":1,"kind":6,"label":"variable","sortText":"{{.*}}variable"}
-#
-# Keyword
-# CHECK: 
{"filterText":"int","insertText":"int","insertTextFormat":1,"kind":14,"label":"int","sortText":"{{.*}}int"}
-#
-# Struct
-# CHECK: 
{"filterText":"Struct","insertText":"Struct","insertTextFormat":1,"kind":7,"label":"Struct","sortText":"{{.*}}Struct"}
-#
-# Macro
-# CHECK: 
{"filterText":"MACRO","insertText":"MACRO","insertTextFormat":1,"kind":1,"label":"MACRO","sortText":"{{.*}}MACRO"}
-#
-# CHECK-SAME: ]}}
-Content-Length: 146
-
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"nam"}}}
-Content-Length: 148
-
-{"jsonrpc":"2.0","id":2,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":1,"character":3}}}
-# Code pattern (unfortunately there are none in expression context)
-# CHECK-DAG: {"filterText":"namespace","insertText":"namespace 
${1:identifier}{${2:declarations}\n}","insertTextFormat":2,"kind":15,"label":"namespace
 identifier{declarations}","sortText":"{{.*}}namespace"}
-#
-Content-Length: 58
-
-{"jsonrpc":"2.0","id":3,"method":"shutdown","params":null}

Removed: clang-tools-extra/trunk/test/clangd/completion-priorities.test
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/completion-priorities.test?rev=320147=auto
==
--- clang-tools-extra/trunk/test/clangd/completion-priorities.test (original)
+++ clang-tools-extra/trunk/test/clangd/completion-priorities.test (removed)
@@ -1,73 +0,0 @@
-# RUN: clangd -pretty -run-synchronously < %s | FileCheck -strict-whitespace %s
-# It is absolutely vital that this file has CRLF line endings.
-#
-
-Content-Length: 127
-
-{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
-
-Content-Length: 312
-
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"class
 Foo {\npublic:\n  void pub();\n\nprotected:\n  void prot();\n\nprivate:\n  
void priv();\n};\n\nvoid Foo::pub() {\n  this->\n}\n\nvoid test() {\n  Foo f;\n 
 f.\n}"}}}
-
-Content-Length: 151
-
-{"jsonrpc":"2.0","id":2,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":12,"character":8}}}
-#  CHECK:  "id": 2,
-# CHECK-NEXT:  "jsonrpc": "2.0",
-# CHECK-NEXT:  "result": {
-# CHECK-NEXT:"isIncomplete": false,
-# CHECK-NEXT:"items": [
-# CHECK-NEXT:  {
-# CHECK-NEXT:"detail": "void",
-# 

[PATCH] D40952: [clangd] Convert lit code completion tests to unit-tests. NFC

[Sam McCall via Phabricator via cfe-commits]

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



Comment at: unittests/clangd/CodeCompleteTests.cpp:289
+};
+void Foo::pub() { this->^ }
+  )cpp");

ilya-biryukov wrote:
> The `^` symbol conflicts with the corresponding operator.
> Even though it's not widely used, I'm wondering whether we should use a 
> different marker for completion position.
Almost all characters conflict with something in C++ :-(
^ is rarely going to cause problems and is suggestive of an insertion point.

The failure modes here don't seem worth worrying about:
 - we add a test that needs to use `^`. The assert fires, it's easy to tell 
what happened, and we have to add escaping or change the notation. Annoying, 
but easy to detect and pretty unlikely.
 - we add a test that needs to use exactly one `^`, *and* we forget to add the 
cursor marker to the test. The assert doesn't fire, the test fails in some 
random way. This is really annoying, but also vanishingly unlikely.

(I'd feel differently if this wasn't just a local test helper of course)




Comment at: unittests/clangd/CodeCompleteTests.cpp:335
+  )cpp",
+ Opts);
+  EXPECT_THAT(Results.items,

ilya-biryukov wrote:
> The formatting is a bit weird. Is this a `clang-format` bug?
Weird formatting is a clang-format *feature*.
I reformatted all the tests, now they're weird in a different way.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40952



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

[PATCH] D40948: Switch Clang's default C++ language target to C++14

[Filipe Cabecinhas via Phabricator via cfe-commits]

filcab added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1733
   // The PS4 uses C++11 as the default C++ standard.
-  if (T.isPS4())
-LangStd = LangStandard::lang_gnucxx11;
-  else
-LangStd = LangStandard::lang_gnucxx98;
+  LangStd = LangStandard::lang_gnucxx14;
   break;

Why are you changing the PS4 default too?


Repository:
  rC Clang

https://reviews.llvm.org/D40948



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

[PATCH] D40897: [clangd] Introduce a "Symbol" class.

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added a comment.

(From https://reviews.llvm.org/D40548) Here's the interface for querying the 
index that I am using right now. It's meant to be able to retrieve from any 
kind of "backend", i.e. in-memory, ClangdIndexDataStore, libIndexStore, etc. I 
was able to implement "Open Workspace Symbol" (which is close to code 
completion in concept), Find References and Find Definitions.

  using USR = llvm::SmallString<256>;
  
  class ClangdIndexDataOccurrence;
  
  class ClangdIndexDataSymbol {
  public:
virtual index::SymbolKind getKind() = 0;
/// For example, for mynamespace::myclass::mymethod, this will be
/// mymethod.
virtual std::string getName() = 0;
/// For example, for mynamespace::myclass::mymethod, this will be
/// mynamespace::myclass::
virtual std::string getQualifier() = 0;
virtual std::string getUsr() = 0;
  
virtual void foreachOccurrence(index::SymbolRoleSet Roles, 
llvm::function_ref Receiver) = 0;
  
virtual ~ClangdIndexDataSymbol() = default;
  };
  
  class ClangdIndexDataOccurrence {
  public:
enum class OccurrenceType : uint16_t {
   OCCURRENCE,
   DEFINITION_OCCURRENCE
 };
  
virtual OccurrenceType getKind() const = 0;
virtual std::string getPath() = 0;
/// Get the start offset of the symbol occurrence. The SourceManager can be
/// used for implementations that need to convert from a line/column
/// representation to an offset.
virtual uint32_t getStartOffset(SourceManager ) = 0;
/// Get the end offset of the symbol occurrence. The SourceManager can be
/// used for implementations that need to convert from a line/column
/// representation to an offset.
virtual uint32_t getEndOffset(SourceManager ) = 0;
virtual ~ClangdIndexDataOccurrence() = default;
//TODO: Add relations
  
static bool classof(const ClangdIndexDataOccurrence *O) { return 
O->getKind() == OccurrenceType::OCCURRENCE; }
  };
  
  /// An occurrence that also has definition with a body that requires 
additional
  /// locations to keep track of the beginning and end of the body.
  class ClangdIndexDataDefinitionOccurrence : public ClangdIndexDataOccurrence {
  public:
virtual uint32_t getDefStartOffset(SourceManager ) = 0;
virtual uint32_t getDefEndOffset(SourceManager ) = 0;
  
static bool classof(const ClangdIndexDataOccurrence *O) { return 
O->getKind() == OccurrenceType::DEFINITION_OCCURRENCE; }
  };
  
  class ClangdIndexDataProvider {
  public:
  
virtual void foreachSymbols(StringRef Pattern, 
llvm::function_ref Receiver) = 0;
virtual void foreachSymbols(const USR , 
llvm::function_ref Receiver) = 0;
  
virtual ~ClangdIndexDataProvider() = default;
  };

The "Clangd" prefix adds a bit much of clutter so maybe it should be removed. I 
think the main points are that having generic foreachSymbols/foreachOccurrence 
with callbacks is well suited to implement multiple features with minimal 
copying.




Comment at: clangd/Symbol.h:37
+// The class presents a C++ symbol, e.g. class, function.
+struct Symbol {
+  // The symbol identifier, using USR.

sammccall wrote:
> hokein wrote:
> > malaperle wrote:
> > > sammccall wrote:
> > > > hokein wrote:
> > > > > malaperle wrote:
> > > > > > I think it would be nice to have methods as an interface to get 
> > > > > > this data instead of storing them directly. So that an 
> > > > > > index-on-disk could go fetch the data. Especially the occurrences 
> > > > > > which can take a lot of memory (I'm working on a branch that does 
> > > > > > that). But perhaps defining that interface is not within the scope 
> > > > > > of this patch and could be better discussed in D40548 ?
> > > > > I agree. We can't load all the symbol occurrences into the memory 
> > > > > since they are too large. We need to design interface for the symbol 
> > > > > occurrences. 
> > > > > 
> > > > > We could discuss the interface here, but CodeCompletion is the main 
> > > > > thing which this patch focuses on. 
> > > > > We can't load all the symbol occurrences into the memory since they 
> > > > > are too large
> > > > 
> > > > I've heard this often, but never backed up by data :-)
> > > > 
> > > > Naively an array of references for a symbol could be doc ID + offset + 
> > > > length, let's say 16 bytes.
> > > > 
> > > > If a source file consisted entirely of references to 1-character 
> > > > symbols separated by punctuation (1 reference per 2 bytes) then the 
> > > > total size of these references would be 8x the size of the source file 
> > > > - in practice much less. That's not very big.
> > > > 
> > > > (Maybe there are edge cases with macros/templates, but we can keep them 
> > > > under control)
> > > I'd have to break down how much memory it used by what, I'll come back to 
> > > you on that. Indexing llvm with ClangdIndexDataStorage, 

[PATCH] D40948: Switch Clang's default C++ language target to C++14

[Tim Northover via Phabricator via cfe-commits]

t.p.northover added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1733
   // The PS4 uses C++11 as the default C++ standard.
-  if (T.isPS4())
-LangStd = LangStandard::lang_gnucxx11;
-  else
-LangStd = LangStandard::lang_gnucxx98;
+  LangStd = LangStandard::lang_gnucxx14;
   break;

filcab wrote:
> Why are you changing the PS4 default too?
Paul Robinson indicated that it was feasible back in March: 
http://lists.llvm.org/pipermail/cfe-dev/2017-March/052986.html. If that's 
changed I'd be happy to put it back to C++11, but he's one of the main people 
(rightly) bugging me about this so I'd be slightly surprised.

I also notice the comment crept back in. Bother.


Repository:
  rC Clang

https://reviews.llvm.org/D40948



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

[PATCH] D41012: [OpenMP] Further adjustments of nvptx runtime functions

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld created this revision.
Herald added subscribers: cfe-commits, jholewinski.

Pass in default value of 1, similar to previous commit 
https://reviews.llvm.org/rL318836.


Repository:
  rC Clang

https://reviews.llvm.org/D41012

Files:
  lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  test/OpenMP/nvptx_data_sharing.cpp
  test/OpenMP/nvptx_target_teams_codegen.cpp

Index: test/OpenMP/nvptx_target_teams_codegen.cpp
===
--- test/OpenMP/nvptx_target_teams_codegen.cpp
+++ test/OpenMP/nvptx_target_teams_codegen.cpp
@@ -60,7 +60,7 @@
   //
   // CHECK: [[AWAIT_WORK]]
   // CHECK: call void @llvm.nvvm.barrier0()
-  // CHECK: [[KPR:%.+]] = call i1 @__kmpc_kernel_parallel(i8** [[OMP_WORK_FN]], i8*** %shared_args)
+  // CHECK: [[KPR:%.+]] = call i1 @__kmpc_kernel_parallel(i8** [[OMP_WORK_FN]], i8*** %shared_args, i16 1)
   // CHECK: [[KPRB:%.+]] = zext i1 [[KPR]] to i8
   // store i8 [[KPRB]], i8* [[OMP_EXEC_STATUS]], align 1
   // CHECK: [[WORK:%.+]] = load i8*, i8** [[OMP_WORK_FN]],
@@ -148,7 +148,7 @@
   //
   // CHECK: [[AWAIT_WORK]]
   // CHECK: call void @llvm.nvvm.barrier0()
-  // CHECK: [[KPR:%.+]] = call i1 @__kmpc_kernel_parallel(i8** [[OMP_WORK_FN]], i8*** %shared_args)
+  // CHECK: [[KPR:%.+]] = call i1 @__kmpc_kernel_parallel(i8** [[OMP_WORK_FN]], i8*** %shared_args, i16 1)
   // CHECK: [[KPRB:%.+]] = zext i1 [[KPR]] to i8
   // store i8 [[KPRB]], i8* [[OMP_EXEC_STATUS]], align 1
   // CHECK: [[WORK:%.+]] = load i8*, i8** [[OMP_WORK_FN]],
Index: test/OpenMP/nvptx_data_sharing.cpp
===
--- test/OpenMP/nvptx_data_sharing.cpp
+++ test/OpenMP/nvptx_data_sharing.cpp
@@ -24,15 +24,15 @@
 
 // CK1: define internal void @__omp_offloading_{{.*}}test_ds{{.*}}worker(){{.*}}{
 // CK1: [[SHAREDARGS:%.+]] = alloca i8**
-// CK1: call i1 @__kmpc_kernel_parallel(i8** %work_fn, i8*** [[SHAREDARGS]])
+// CK1: call i1 @__kmpc_kernel_parallel(i8** %work_fn, i8*** [[SHAREDARGS]], i16 1)
 // CK1: [[SHARGSTMP:%.+]] = load i8**, i8*** [[SHAREDARGS]]
 // CK1: call void @__omp_outlined___wrapper{{.*}}({{.*}}, i8** [[SHARGSTMP]])
 
 /// = In the kernel function = ///
 
 // CK1: {{.*}}define void @__omp_offloading{{.*}}test_ds{{.*}}()
 // CK1: [[SHAREDARGS1:%.+]] = alloca i8**
-// CK1: call void @__kmpc_kernel_prepare_parallel({{.*}}, i8*** [[SHAREDARGS1]], i32 1)
+// CK1: call void @__kmpc_kernel_prepare_parallel({{.*}}, i8*** [[SHAREDARGS1]], i32 1, i16 1)
 // CK1: [[SHARGSTMP1:%.+]] = load i8**, i8*** [[SHAREDARGS1]]
 // CK1: [[SHARGSTMP2:%.+]] = getelementptr inbounds i8*, i8** [[SHARGSTMP1]]
 // CK1: [[SHAREDVAR:%.+]] = bitcast i32* {{.*}} to i8*
Index: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
===
--- lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
+++ lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
@@ -33,10 +33,11 @@
   /// \brief Call to void __kmpc_spmd_kernel_deinit();
   OMPRTL_NVPTX__kmpc_spmd_kernel_deinit,
   /// \brief Call to void __kmpc_kernel_prepare_parallel(void
-  /// *outlined_function, void ***args, kmp_int32 nArgs);
+  /// *outlined_function, void ***args, kmp_int32 nArgs, int16_t
+  /// IsOMPRuntimeInitialized);
   OMPRTL_NVPTX__kmpc_kernel_prepare_parallel,
   /// \brief Call to bool __kmpc_kernel_parallel(void **outlined_function, void
-  /// ***args);
+  /// ***args, int16_t IsOMPRuntimeInitialized);
   OMPRTL_NVPTX__kmpc_kernel_parallel,
   /// \brief Call to void __kmpc_kernel_end_parallel();
   OMPRTL_NVPTX__kmpc_kernel_end_parallel,
@@ -521,7 +522,9 @@
   // Set up shared arguments
   Address SharedArgs =
   CGF.CreateDefaultAlignTempAlloca(CGF.Int8PtrPtrTy, "shared_args");
-  llvm::Value *Args[] = {WorkFn.getPointer(), SharedArgs.getPointer()};
+  // TODO: Optimize runtime initialization and pass in correct value.
+  llvm::Value *Args[] = {WorkFn.getPointer(), SharedArgs.getPointer(),
+ /*RequiresOMPRuntime=*/Bld.getInt16(1)};
   llvm::Value *Ret = CGF.EmitRuntimeCall(
   createNVPTXRuntimeFunction(OMPRTL_NVPTX__kmpc_kernel_parallel), Args);
   Bld.CreateStore(Bld.CreateZExt(Ret, CGF.Int8Ty), ExecStatus);
@@ -637,18 +640,21 @@
   }
   case OMPRTL_NVPTX__kmpc_kernel_prepare_parallel: {
 /// Build void __kmpc_kernel_prepare_parallel(
-/// void *outlined_function, void ***args, kmp_int32 nArgs);
+/// void *outlined_function, void ***args, kmp_int32 nArgs, int16_t
+/// IsOMPRuntimeInitialized);
 llvm::Type *TypeParams[] = {CGM.Int8PtrTy,
-CGM.Int8PtrPtrTy->getPointerTo(0), CGM.Int32Ty};
+CGM.Int8PtrPtrTy->getPointerTo(0), CGM.Int32Ty,
+CGM.Int16Ty};
 llvm::FunctionType *FnTy =
 llvm::FunctionType::get(CGM.VoidTy, TypeParams, /*isVarArg*/ false);
 RTLFn = CGM.CreateRuntimeFunction(FnTy, "__kmpc_kernel_prepare_parallel");
 break;
   }
   case OMPRTL_NVPTX__kmpc_kernel_parallel: {
-/// 

r320162 - Revert "Unify implementation of our two different flavours of -Wtautological-compare."

[Hans Wennborg via cfe-commits]

Author: hans
Date: Fri Dec  8 08:54:08 2017
New Revision: 320162

URL: http://llvm.org/viewvc/llvm-project?rev=320162=rev
Log:
Revert "Unify implementation of our two different flavours of 
-Wtautological-compare."

> Unify implementation of our two different flavours of -Wtautological-compare.
>
> In so doing, fix a handful of remaining bugs where we would report false
> positives or false negatives if we promote a signed value to an unsigned type
> for the comparison.

This caused a new warning in Chromium:

../../base/trace_event/trace_log.cc:1545:29: error: comparison of constant 64
with expression of type 'unsigned int' is always true
[-Werror,-Wtautological-constant-out-of-range-compare]
  DCHECK(handle.event_index < TraceBufferChunk::kTraceBufferChunkSize);
 ~~ ^ ~~~

The 'unsigned int' is really a 6-bit bitfield, which is why it's always
less than 64.

I thought we didn't use to warn (with out-of-range-compare) when comparing
against the boundaries of a type?

Modified:
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/test/Sema/tautological-constant-compare.c
cfe/trunk/test/Sema/tautological-constant-enum-compare.c
cfe/trunk/test/SemaCXX/compare.cpp

Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=320162=320161=320162=diff
==
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Dec  8 08:54:08 2017
@@ -8662,113 +8662,54 @@ static bool isKnownToHaveUnsignedValue(E
 }
 
 namespace {
-/// The promoted range of values of a type. In general this has the
-/// following structure:
-///
-/// |---| . . . |---|
-/// ^   ^   ^   ^
-///Min   HoleMin  HoleMax  Max
-///
-/// ... where there is only a hole if a signed type is promoted to unsigned
-/// (in which case Min and Max are the smallest and largest representable
-/// values).
-struct PromotedRange {
-  // Min, or HoleMax if there is a hole.
-  llvm::APSInt PromotedMin;
-  // Max, or HoleMin if there is a hole.
-  llvm::APSInt PromotedMax;
-
-  PromotedRange(IntRange R, unsigned BitWidth, bool Unsigned) {
-if (R.Width == 0)
-  PromotedMin = PromotedMax = llvm::APSInt(BitWidth, Unsigned);
-else {
-  PromotedMin = llvm::APSInt::getMinValue(R.Width, R.NonNegative)
-.extOrTrunc(BitWidth);
-  PromotedMin.setIsUnsigned(Unsigned);
-
-  PromotedMax = llvm::APSInt::getMaxValue(R.Width, R.NonNegative)
-.extOrTrunc(BitWidth);
-  PromotedMax.setIsUnsigned(Unsigned);
-}
-  }
 
-  // Determine whether this range is contiguous (has no hole).
-  bool isContiguous() const { return PromotedMin <= PromotedMax; }
+enum class LimitType {
+  Max = 1U << 0U,  // e.g. 32767 for short
+  Min = 1U << 1U,  // e.g. -32768 for short
+  Both = Max | Min // When the value is both the Min and the Max limit at the
+   // same time; e.g. in C++, A::a in enum A { a = 0 };
+};
 
-  // Where a constant value is within the range.
-  enum ComparisonResult {
-LT = 0x1,
-LE = 0x2,
-GT = 0x4,
-GE = 0x8,
-EQ = 0x10,
-NE = 0x20,
-InRangeFlag = 0x40,
-
-Less = LE | LT | NE,
-Min = LE | InRangeFlag,
-InRange = InRangeFlag,
-Max = GE | InRangeFlag,
-Greater = GE | GT | NE,
+} // namespace
 
-OnlyValue = LE | GE | EQ | InRangeFlag,
-InHole = NE
-  };
+/// Checks whether Expr 'Constant' may be the
+/// std::numeric_limits<>::max() or std::numeric_limits<>::min()
+/// of the Expr 'Other'. If true, then returns the limit type (min or max).
+/// The Value is the evaluation of Constant
+static llvm::Optional IsTypeLimit(Sema , Expr *Constant,
+ Expr *Other,
+ const llvm::APSInt ) {
+  if (IsEnumConstOrFromMacro(S, Constant))
+return llvm::Optional();
 
-  ComparisonResult compare(const llvm::APSInt ) const {
-assert(Value.getBitWidth() == PromotedMin.getBitWidth() &&
-   Value.isUnsigned() == PromotedMin.isUnsigned());
-if (!isContiguous()) {
-  assert(Value.isUnsigned() && "discontiguous range for signed compare");
-  if (Value.isMinValue()) return Min;
-  if (Value.isMaxValue()) return Max;
-  if (Value >= PromotedMin) return InRange;
-  if (Value <= PromotedMax) return InRange;
-  return InHole;
-}
+  if (isKnownToHaveUnsignedValue(Other) && Value == 0)
+return LimitType::Min;
 
-switch (llvm::APSInt::compareValues(Value, PromotedMin)) {
-case -1: return Less;
-case 0: return PromotedMin == PromotedMax ? OnlyValue : Min;
-case 1:
-  switch (llvm::APSInt::compareValues(Value, PromotedMax)) {
-  case -1: return InRange;
-  case 0: return Max;
-  case 1: return 

[PATCH] D40685: [libunwind] Switch to add_llvm_install_targets

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

The other runtime projects had the stripped installation targets added manually 
because they officially support being built completely standalone (i.e. without 
any LLVM components), so we can't rely on LLVM's CMake there. That's really 
unfortunate IMO, since it leads to a bunch of duplication, but it is what it is.

libunwind doesn't support building completely standalone AFAIK, so I think 
assuming trunk libunwind will be paired with trunk LLVM is pretty reasonable in 
general. It's easy enough to not break your use case here, however, so I went 
ahead and did so in r320163.


Repository:
  rL LLVM

https://reviews.llvm.org/D40685



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

[PATCH] D40625: Harmonizing attribute GNU/C++ spellings

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: include/clang/Basic/Attr.td:602
 def AnalyzerNoReturn : InheritableAttr {
-  let Spellings = [GNU<"analyzer_noreturn">];
+  let Spellings = [Clang<"analyzer_noreturn">];
   let Documentation = [Undocumented];

aaron.ballman wrote:
> dcoughlin wrote:
> > aaron.ballman wrote:
> > > dcoughlin wrote:
> > > > aaron.ballman wrote:
> > > > > rsmith wrote:
> > > > > > Hmm, should the clang static analyzer reuse the `clang::` 
> > > > > > namespace, or should it get its own?
> > > > > Good question, I don't have strong opinions on the answer here, but 
> > > > > perhaps @dcoughlin does?
> > > > > 
> > > > > If we want to use a separate namespace for the analyzer, would we 
> > > > > want to use that same namespace for any clang-tidy specific 
> > > > > attributes? Or should clang-tidy get its own namespace? (Do we ever 
> > > > > plan to execute clang-tidy through the clang driver? That might 
> > > > > change our answer.)
> > > > How would this look if we added a special namespace for the clang 
> > > > static analyzer? Would this lead to duplication (say, 
> > > > [[clang_analyzer::analyzer_noreturn]]) so that we keep the "analyzer_" 
> > > > prefix for __attribute__((analyzer_noreturn))? Or could we have the 
> > > > "analyzer_" prefix only for GNU-style attributes but not for C++ (for 
> > > > example, [[clang_analyzer::noreturn]])?
> > > > 
> > > > As for clang-tidy, I think it probably makes sense for it to have its 
> > > > own namespace, but we should ask @alexfh.
> > > > How would this look if we added a special namespace for the clang 
> > > > static analyzer? Would this lead to duplication (say, 
> > > > [[clang_analyzer::analyzer_noreturn]]) so that we keep the "analyzer_" 
> > > > prefix for attribute((analyzer_noreturn))? Or could we have the 
> > > > "analyzer_" prefix only for GNU-style attributes but not for C++ (for 
> > > > example, [[clang_analyzer::noreturn]])?
> > > 
> > > We have the ability to do whatever we'd like there. Given that the 
> > > semantics are so similar to `[[noreturn]]`, I think it would be 
> > > reasonable to use `[[clang_analyzer::noreturn]]` and 
> > > `__attribute__((analyzer_noreturn))` if that's the direction you think is 
> > > best.
> > > 
> > > > As for clang-tidy, I think it probably makes sense for it to have its 
> > > > own namespace, but we should ask @alexfh.
> > > 
> > > I'm less enthusiastic about giving clang-tidy a vendor namespace that's 
> > > separate from the static analyzer, should the need arise. My biggest 
> > > concern there is that I would *really* like to see clang-tidy be more 
> > > tightly integrated with the clang driver (so users don't have to manually 
> > > execute a secondary tool). If that were to happen, then the user 
> > > experience would be that there are two vendor namespaces both related to 
> > > analyzer attributes.
> > > 
> > > That said, I would also not be opposed to putting all of these attributes 
> > > under the `clang` vendor namespace and not having a separate vendor for 
> > > the analyzer or clang-tidy.
> > I would be find with keeping all of these things under the `clang` 
> > namespace, too.
> > 
> > That said, I do think there is some value in having a namespace for 
> > analyzer attributes separate from clang proper because the namespace would 
> > make it more clear that the attribute doesn't affect code generation.
> I've changed this one back to the GNU spelling to give us time to decide how 
> we want to handle analyzer attributes.
> 
> I'm not certain "does not affect codegen" is the correct measure to use for 
> this, however. We have other attributes that muddy the water, such as 
> `annotate`, or the format specifier attributes -- these don't (really) impact 
> codegen in any way, but do impact more than just the analyzer. Given the 
> integration of the analyzer with Clang (and the somewhat fluid nature of what 
> is responsible for issuing diagnostics), I think we should proceed with 
> caution on the idea of an analyzer-specific namespace.
> 
> However, do you have a list of attributes you think might qualify as being 
> analyzer-only? I can make sure we leave those spellings alone in this patch.
An argument against clang_tidy and clang_analyzer vendor namespaces is that the 
choice of where to implement a certain check would be connected to adding an 
attribute in one or both of these namespaces, which would complicate things a 
bit. In case both clang-tidy and static analyzer use the same argument, we'd 
need to have duplicate attributes. I definitely don't think we need three 
`noreturn` attributes, for example.


https://reviews.llvm.org/D40625



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

Re: r320162 - Revert "Unify implementation of our two different flavours of -Wtautological-compare."

[Richard Smith via cfe-commits]

On 8 Dec 2017 08:54, "Hans Wennborg via cfe-commits" <
cfe-commits@lists.llvm.org> wrote:

Author: hans
Date: Fri Dec  8 08:54:08 2017
New Revision: 320162

URL: http://llvm.org/viewvc/llvm-project?rev=320162=rev
Log:
Revert "Unify implementation of our two different flavours of
-Wtautological-compare."

> Unify implementation of our two different flavours of
-Wtautological-compare.
>
> In so doing, fix a handful of remaining bugs where we would report false
> positives or false negatives if we promote a signed value to an unsigned
type
> for the comparison.

This caused a new warning in Chromium:

../../base/trace_event/trace_log.cc:1545:29: error: comparison of constant
64
with expression of type 'unsigned int' is always true
[-Werror,-Wtautological-constant-out-of-range-compare]
  DCHECK(handle.event_index < TraceBufferChunk::kTraceBufferChunkSize);
 ~~ ^ ~~~

The 'unsigned int' is really a 6-bit bitfield, which is why it's always
less than 64.

I thought we didn't use to warn (with out-of-range-compare) when comparing
against the boundaries of a type?


This isn't a "boundary of the type" case, though -- 64 is out of range for
a 6 bit bitfield. Your CHECK does nothing. Do you think that it's not
reasonable to warn on this bug, or that it's not a bug?

Modified:
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/test/Sema/tautological-constant-compare.c
cfe/trunk/test/Sema/tautological-constant-enum-compare.c
cfe/trunk/test/SemaCXX/compare.cpp

Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/
SemaChecking.cpp?rev=320162=320161=320162=diff

==
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Dec  8 08:54:08 2017
@@ -8662,113 +8662,54 @@ static bool isKnownToHaveUnsignedValue(E
 }

 namespace {
-/// The promoted range of values of a type. In general this has the
-/// following structure:
-///
-/// |---| . . . |---|
-/// ^   ^   ^   ^
-///Min   HoleMin  HoleMax  Max
-///
-/// ... where there is only a hole if a signed type is promoted to unsigned
-/// (in which case Min and Max are the smallest and largest representable
-/// values).
-struct PromotedRange {
-  // Min, or HoleMax if there is a hole.
-  llvm::APSInt PromotedMin;
-  // Max, or HoleMin if there is a hole.
-  llvm::APSInt PromotedMax;
-
-  PromotedRange(IntRange R, unsigned BitWidth, bool Unsigned) {
-if (R.Width == 0)
-  PromotedMin = PromotedMax = llvm::APSInt(BitWidth, Unsigned);
-else {
-  PromotedMin = llvm::APSInt::getMinValue(R.Width, R.NonNegative)
-.extOrTrunc(BitWidth);
-  PromotedMin.setIsUnsigned(Unsigned);
-
-  PromotedMax = llvm::APSInt::getMaxValue(R.Width, R.NonNegative)
-.extOrTrunc(BitWidth);
-  PromotedMax.setIsUnsigned(Unsigned);
-}
-  }

-  // Determine whether this range is contiguous (has no hole).
-  bool isContiguous() const { return PromotedMin <= PromotedMax; }
+enum class LimitType {
+  Max = 1U << 0U,  // e.g. 32767 for short
+  Min = 1U << 1U,  // e.g. -32768 for short
+  Both = Max | Min // When the value is both the Min and the Max limit at
the
+   // same time; e.g. in C++, A::a in enum A { a = 0 };
+};

-  // Where a constant value is within the range.
-  enum ComparisonResult {
-LT = 0x1,
-LE = 0x2,
-GT = 0x4,
-GE = 0x8,
-EQ = 0x10,
-NE = 0x20,
-InRangeFlag = 0x40,
-
-Less = LE | LT | NE,
-Min = LE | InRangeFlag,
-InRange = InRangeFlag,
-Max = GE | InRangeFlag,
-Greater = GE | GT | NE,
+} // namespace

-OnlyValue = LE | GE | EQ | InRangeFlag,
-InHole = NE
-  };
+/// Checks whether Expr 'Constant' may be the
+/// std::numeric_limits<>::max() or std::numeric_limits<>::min()
+/// of the Expr 'Other'. If true, then returns the limit type (min or max).
+/// The Value is the evaluation of Constant
+static llvm::Optional IsTypeLimit(Sema , Expr *Constant,
+ Expr *Other,
+ const llvm::APSInt ) {
+  if (IsEnumConstOrFromMacro(S, Constant))
+return llvm::Optional();

-  ComparisonResult compare(const llvm::APSInt ) const {
-assert(Value.getBitWidth() == PromotedMin.getBitWidth() &&
-   Value.isUnsigned() == PromotedMin.isUnsigned());
-if (!isContiguous()) {
-  assert(Value.isUnsigned() && "discontiguous range for signed
compare");
-  if (Value.isMinValue()) return Min;
-  if (Value.isMaxValue()) return Max;
-  if (Value >= PromotedMin) return InRange;
-  if (Value <= PromotedMax) return InRange;
-  return InHole;
-}
+  if (isKnownToHaveUnsignedValue(Other) && Value == 0)
+return LimitType::Min;

-switch 

[PATCH] D40948: Switch Clang's default C++ language target to C++14

[Filipe Cabecinhas via Phabricator via cfe-commits]

filcab added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1733
   // The PS4 uses C++11 as the default C++ standard.
-  if (T.isPS4())
-LangStd = LangStandard::lang_gnucxx11;
-  else
-LangStd = LangStandard::lang_gnucxx98;
+  LangStd = LangStandard::lang_gnucxx14;
   break;

t.p.northover wrote:
> filcab wrote:
> > Why are you changing the PS4 default too?
> Paul Robinson indicated that it was feasible back in March: 
> http://lists.llvm.org/pipermail/cfe-dev/2017-March/052986.html. If that's 
> changed I'd be happy to put it back to C++11, but he's one of the main people 
> (rightly) bugging me about this so I'd be slightly surprised.
> 
> I also notice the comment crept back in. Bother.
Sounds good, then. If Paul is happy with that change, I don't see a problem 
there (assuming you do get rid of the comment for good :-) ).

Thank you,

 Filipe


Repository:
  rC Clang

https://reviews.llvm.org/D40948



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

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Anton via Phabricator via cfe-commits]

xgsa added a comment.

In https://reviews.llvm.org/D40671#949687, @alexfh wrote:

> How are unknown check names handled? More specifically: will the `// 
> NOLINT(runtime/explicit)` comment disable all clang-tidy checks or none?


None. If comment is syntactically correct and contains parenthesis, it works 
only for the known checks inside.

Still, I don't think it worth mentioning all the corner cases in documentation 
to keep things simple.


https://reviews.llvm.org/D40671



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

r320168 - [hwasan] typo in docs

[Kostya Serebryany via cfe-commits]

Author: kcc
Date: Fri Dec  8 10:14:03 2017
New Revision: 320168

URL: http://llvm.org/viewvc/llvm-project?rev=320168=rev
Log:
[hwasan] typo in docs

Modified:
cfe/trunk/docs/HardwareAssistedAddressSanitizerDesign.rst

Modified: cfe/trunk/docs/HardwareAssistedAddressSanitizerDesign.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/HardwareAssistedAddressSanitizerDesign.rst?rev=320168=320167=320168=diff
==
--- cfe/trunk/docs/HardwareAssistedAddressSanitizerDesign.rst (original)
+++ cfe/trunk/docs/HardwareAssistedAddressSanitizerDesign.rst Fri Dec  8 
10:14:03 2017
@@ -80,7 +80,7 @@ Errors are generated by `__builtin_trap`
 Attribute
 -
 
-HWASAN uses it's own LLVM IR Attribute `sanitize_hwaddress` and a matching
+HWASAN uses its own LLVM IR Attribute `sanitize_hwaddress` and a matching
 C function attribute. An alternative would be to re-use ASAN's attribute
 `sanitize_address`. The reasons to use a separate attribute are:
 


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

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D40671#945158, @xgsa wrote:

> In https://reviews.llvm.org/D40671#944906, @alexfh wrote:
>
> > BTW, how will this feature interact with cpplint.py's way of handling 
> > specific NOLINT directives that use different lint rule names, which 
> > sometimes refer to the same rule (e.g. `// NOLINT(runtime/explicit)` 
> > suppresses the `runtime/explicit` cpplint rule that enforces the same style 
> > rule as the google-runtime-explicit check)?
>
>
> This feature accepts only full check names.


How are unknown check names handled? More specifically: will the `// 
NOLINT(runtime/explicit)` comment disable all clang-tidy checks or none?


https://reviews.llvm.org/D40671



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

[PATCH] D39363: [clang-tidy] Correctly classify constant arrays and constant strings as constants when checking identifiers naming

[Alexander Kornienko via Phabricator via cfe-commits]

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

Looks good!

Thank you for the fix! Do you need someone to commit it for you? If yes, please 
rebase the fix on top of the current HEAD and let me know.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39363



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

[PATCH] D39363: [clang-tidy] Correctly classify constant arrays and constant strings as constants when checking identifiers naming

[Beren Minor via Phabricator via cfe-commits]

berenm updated this revision to Diff 126177.
berenm added a comment.

Rebase patch on upstream HEAD


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39363

Files:
  clang-tidy/readability/IdentifierNamingCheck.cpp
  test/clang-tidy/readability-identifier-naming.cpp

Index: test/clang-tidy/readability-identifier-naming.cpp
===
--- test/clang-tidy/readability-identifier-naming.cpp
+++ test/clang-tidy/readability-identifier-naming.cpp
@@ -405,6 +405,38 @@
 
   using ::FOO_NS::InlineNamespace::CE_function;
 // CHECK-FIXES: {{^}}  using ::foo_ns::inline_namespace::ce_function;{{$}}
+
+  unsigned MY_LOCAL_array[] = {1,2,3};
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: invalid case style for local variable 'MY_LOCAL_array'
+// CHECK-FIXES: {{^}}  unsigned my_local_array[] = {1,2,3};{{$}}
+
+  unsigned const MyConstLocal_array[] = {1,2,3};
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: invalid case style for local constant 'MyConstLocal_array'
+// CHECK-FIXES: {{^}}  unsigned const kMyConstLocalArray[] = {1,2,3};{{$}}
+
+  static unsigned MY_STATIC_array[] = {1,2,3};
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: invalid case style for static variable 'MY_STATIC_array'
+// CHECK-FIXES: {{^}}  static unsigned s_myStaticArray[] = {1,2,3};{{$}}
+
+  static unsigned const MyConstStatic_array[] = {1,2,3};
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: invalid case style for static constant 'MyConstStatic_array'
+// CHECK-FIXES: {{^}}  static unsigned const MY_CONST_STATIC_ARRAY[] = {1,2,3};{{$}}
+
+  char MY_LOCAL_string[] = "123";
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for local variable 'MY_LOCAL_string'
+// CHECK-FIXES: {{^}}  char my_local_string[] = "123";{{$}}
+
+  char const MyConstLocal_string[] = "123";
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for local constant 'MyConstLocal_string'
+// CHECK-FIXES: {{^}}  char const kMyConstLocalString[] = "123";{{$}}
+
+  static char MY_STATIC_string[] = "123";
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: invalid case style for static variable 'MY_STATIC_string'
+// CHECK-FIXES: {{^}}  static char s_myStaticString[] = "123";{{$}}
+
+  static char const MyConstStatic_string[] = "123";
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: invalid case style for static constant 'MyConstStatic_string'
+// CHECK-FIXES: {{^}}  static char const MY_CONST_STATIC_STRING[] = "123";{{$}}
 }
 
 #define MY_TEST_Macro(X) X()
@@ -418,6 +450,27 @@
 
 template  struct a {
   typename t_t::template b<> c;
+
+  char const MY_ConstMember_string[4] = "123";
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for constant member 'MY_ConstMember_string'
+// CHECK-FIXES: {{^}}  char const my_const_member_string[4] = "123";{{$}}
+
+  static char const MyConstClass_string[];
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: invalid case style for class constant 'MyConstClass_string'
+// CHECK-FIXES: {{^}}  static char const kMyConstClassString[];{{$}}
 };
 
+template
+char const a::MyConstClass_string[] = "123";
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: invalid case style for class constant 'MyConstClass_string'
+// CHECK-FIXES: {{^}}char const a::kMyConstClassString[] = "123";{{$}}
+
 template  class A> struct b { A c; };
+
+unsigned MY_GLOBAL_array[] = {1,2,3};
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: invalid case style for global variable 'MY_GLOBAL_array'
+// CHECK-FIXES: {{^}}unsigned g_my_global_array[] = {1,2,3};{{$}}
+
+unsigned const MyConstGlobal_array[] = {1,2,3};
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for global constant 'MyConstGlobal_array'
+// CHECK-FIXES: {{^}}unsigned const MY_CONST_GLOBAL_ARRAY[] = {1,2,3};{{$}}
Index: clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -449,13 +449,13 @@
   if (const auto *Decl = dyn_cast(D)) {
 QualType Type = Decl->getType();
 
-if (!Type.isNull() && Type.isLocalConstQualified() &&
-NamingStyles[SK_ConstantMember])
-  return SK_ConstantMember;
+if (!Type.isNull() && Type.isConstQualified()) {
+  if (NamingStyles[SK_ConstantMember])
+return SK_ConstantMember;
 
-if (!Type.isNull() && Type.isLocalConstQualified() &&
-NamingStyles[SK_Constant])
-  return SK_Constant;
+  if (NamingStyles[SK_Constant])
+return SK_Constant;
+}
 
 if (Decl->getAccess() == AS_private && NamingStyles[SK_PrivateMember])
   return SK_PrivateMember;
@@ -478,13 +478,13 @@
 if (Decl->isConstexpr() && NamingStyles[SK_ConstexprVariable])
   return SK_ConstexprVariable;
 
-if (!Type.isNull() && Type.isLocalConstQualified() &&
-NamingStyles[SK_ConstantParameter])
-  return SK_ConstantParameter;
+if (!Type.isNull() && Type.isConstQualified()) {

[PATCH] D41012: [OpenMP] Further adjustments of nvptx runtime functions

[Alexey Bataev via Phabricator via cfe-commits]

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

LG


Repository:
  rC Clang

https://reviews.llvm.org/D41012



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

[PATCH] D40968: [OpenMP] Diagnose function name on the link clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: lib/Sema/SemaOpenMP.cpp:12671
+void Sema::checkDeclIsAllowedInOpenMPTarget(Expr *E, Decl *D,
+const DeclarationNameInfo *Id) {
   if (!D || D->isInvalidDecl())

You can get `DeclarationNameInfo` from the `FunctionDecl`:
```
FD->getNameInfo()
```


https://reviews.llvm.org/D40968



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

[libunwind] r320163 - [libunwind] Create install-unwind-stripped target manually

[Shoaib Meenai via cfe-commits]

Author: smeenai
Date: Fri Dec  8 09:15:05 2017
New Revision: 320163

URL: http://llvm.org/viewvc/llvm-project?rev=320163=rev
Log:
[libunwind] Create install-unwind-stripped target manually

This supports using a newer libunwind with an older installation of LLVM
(whose cmake modules wouldn't have add_llvm_install_targets).

Modified:
libunwind/trunk/src/CMakeLists.txt

Modified: libunwind/trunk/src/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/libunwind/trunk/src/CMakeLists.txt?rev=320163=320162=320163=diff
==
--- libunwind/trunk/src/CMakeLists.txt (original)
+++ libunwind/trunk/src/CMakeLists.txt Fri Dec  8 09:15:05 2017
@@ -139,7 +139,15 @@ if (LIBUNWIND_INSTALL_LIBRARY)
 endif()
 
 if (NOT CMAKE_CONFIGURATION_TYPES AND LIBUNWIND_INSTALL_LIBRARY)
-  add_llvm_install_targets(install-unwind
-   DEPENDS unwind
-   COMPONENT unwind)
+  add_custom_target(install-unwind
+DEPENDS unwind
+COMMAND "${CMAKE_COMMAND}"
+-DCMAKE_INSTALL_COMPONENT=unwind
+-P "${LIBUNWIND_BINARY_DIR}/cmake_install.cmake")
+  add_custom_target(install-unwind-stripped
+DEPENDS unwind
+COMMAND "${CMAKE_COMMAND}"
+-DCMAKE_INSTALL_COMPONENT=unwind
+-DCMAKE_INSTALL_DO_STRIP=1
+-P "${LIBUNWIND_BINARY_DIR}/cmake_install.cmake")
 endif()


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

[PATCH] D39363: [clang-tidy] Correctly classify constant arrays and constant strings as constants when checking identifiers naming

[Beren Minor via Phabricator via cfe-commits]

berenm added a comment.

Yes, please could you commit it for me? Thanks!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39363



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

Re: r320124 - Fold together the in-range and out-of-range portions of -Wtautological-compare.

[Hans Wennborg via cfe-commits]

Sorry, it seems it was r320124 that caused this; I shouldn't be
sheriffing after-hours.

I've reverted that one in r320162.

On Thu, Dec 7, 2017 at 9:20 PM, Hans Wennborg  wrote:
> I've reverted in r320133 since it caused new warnings in Chromium that
> I'm not sure were intentional. See comment on the revert.
>
> On Thu, Dec 7, 2017 at 5:00 PM, Richard Smith via cfe-commits
>  wrote:
>> Author: rsmith
>> Date: Thu Dec  7 17:00:27 2017
>> New Revision: 320124
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=320124=rev
>> Log:
>> Fold together the in-range and out-of-range portions of 
>> -Wtautological-compare.
>>
>> Modified:
>> cfe/trunk/lib/Sema/SemaChecking.cpp
>>
>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>> URL: 
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=320124=320123=320124=diff
>> ==
>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Dec  7 17:00:27 2017
>> @@ -8801,12 +8801,7 @@ static bool CheckTautologicalComparison(
>>  Expr *Constant, Expr *Other,
>>  const llvm::APSInt ,
>>  bool RhsConstant) {
>> -  // Disable warning in template instantiations
>> -  // and only analyze <, >, <= and >= operations.
>> -  if (S.inTemplateInstantiation() || !E->isRelationalOp())
>> -return false;
>> -
>> -  if (IsEnumConstOrFromMacro(S, Constant))
>> +  if (S.inTemplateInstantiation())
>>  return false;
>>
>>Expr *OriginalOther = Other;
>> @@ -8833,94 +8828,23 @@ static bool CheckTautologicalComparison(
>>OtherRange.Width =
>>std::min(Bitfield->getBitWidthValue(S.Context), OtherRange.Width);
>>
>> -  // Check whether the constant value can be represented in OtherRange. Bail
>> -  // out if so; this isn't an out-of-range comparison.
>> +  // Determine the promoted range of the other type and see if a comparison 
>> of
>> +  // the constant against that range is tautological.
>>PromotedRange OtherPromotedRange(OtherRange, Value.getBitWidth(),
>> Value.isUnsigned());
>> -
>>auto Cmp = OtherPromotedRange.compare(Value);
>> -  if (Cmp != PromotedRange::Min && Cmp != PromotedRange::Max &&
>> -  Cmp != PromotedRange::OnlyValue)
>> -return false;
>> -
>>auto Result = PromotedRange::constantValue(E->getOpcode(), Cmp, 
>> RhsConstant);
>>if (!Result)
>>  return false;
>>
>> -  // Should be enough for uint128 (39 decimal digits)
>> -  SmallString<64> PrettySourceValue;
>> -  llvm::raw_svector_ostream OS(PrettySourceValue);
>> -  OS << Value;
>> -
>> -  // FIXME: We use a somewhat different formatting for the cases involving
>> -  // boolean values for historical reasons. We should pick a consistent way
>> -  // of presenting these diagnostics.
>> -  if (Other->isKnownToHaveBooleanValue()) {
>> -S.DiagRuntimeBehavior(
>> -  E->getOperatorLoc(), E,
>> -  S.PDiag(diag::warn_tautological_bool_compare)
>> -  << OS.str() << classifyConstantValue(Constant)
>> -  << OtherT << !OtherT->isBooleanType() << *Result
>> -  << E->getLHS()->getSourceRange() << 
>> E->getRHS()->getSourceRange());
>> -return true;
>> -  }
>> -
>> -  unsigned Diag = (isKnownToHaveUnsignedValue(OriginalOther) && Value == 0)
>> -  ? (HasEnumType(OriginalOther)
>> - ? 
>> diag::warn_unsigned_enum_always_true_comparison
>> - : diag::warn_unsigned_always_true_comparison)
>> -  : diag::warn_tautological_constant_compare;
>> -
>> -  S.Diag(E->getOperatorLoc(), Diag)
>> -  << RhsConstant << OtherT << E->getOpcodeStr() << OS.str() << *Result
>> -  << E->getLHS()->getSourceRange() << E->getRHS()->getSourceRange();
>> -
>> -  return true;
>> -}
>> -
>> -static bool DiagnoseOutOfRangeComparison(Sema , BinaryOperator *E,
>> - Expr *Constant, Expr *Other,
>> - const llvm::APSInt ,
>> - bool RhsConstant) {
>> -  // Disable warning in template instantiations.
>> -  if (S.inTemplateInstantiation())
>> -return false;
>> -
>> -  Constant = Constant->IgnoreParenImpCasts();
>> -  Other = Other->IgnoreParenImpCasts();
>> -
>> -  // TODO: Investigate using GetExprRange() to get tighter bounds
>> -  // on the bit ranges.
>> -  QualType OtherT = Other->getType();
>> -  if (const auto *AT = OtherT->getAs())
>> -OtherT = AT->getValueType();
>> -  IntRange OtherRange = IntRange::forValueOfType(S.Context, OtherT);
>> -
>> -  // Whether we're treating Other as being a bool because of the form of
>> -  // expression despite it having another type (typically 'int' in C).
>> -  bool 

[PATCH] D41016: [Sema] Fix crash in unused-lambda-capture warning for VLAs

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons created this revision.
Herald added a subscriber: cfe-commits.

Clang was crashing when diagnosing an unused-lambda-capture for a VLA because
From.getVariable() is null for the capture of a VLA bound.
Warning about the VLA bound capture is not helpful, so only warn for the VLA
itself.

Fixes: PR3


Repository:
  rC Clang

https://reviews.llvm.org/D41016

Files:
  lib/Sema/SemaLambda.cpp
  test/SemaCXX/warn-unused-lambda-capture.cpp


Index: test/SemaCXX/warn-unused-lambda-capture.cpp
===
--- test/SemaCXX/warn-unused-lambda-capture.cpp
+++ test/SemaCXX/warn-unused-lambda-capture.cpp
@@ -191,3 +191,12 @@
 void test_use_template() {
   test_templated(); // expected-note{{in instantiation of function 
template specialization 'test_templated' requested here}}
 }
+
+namespace pr3 {
+int a;
+void b() {
+  int c[a];
+  auto vla_used = [] { return c[0]; };
+  auto vla_unused = [] {}; // expected-warning{{lambda capture 'c' is not 
used}}
+}
+} // namespace pr3
Index: lib/Sema/SemaLambda.cpp
===
--- lib/Sema/SemaLambda.cpp
+++ lib/Sema/SemaLambda.cpp
@@ -1481,6 +1481,9 @@
   if (CaptureHasSideEffects(From))
 return;
 
+  if (From.isVLATypeCapture())
+return;
+
   auto diag = Diag(From.getLocation(), diag::warn_unused_lambda_capture);
   if (From.isThisCapture())
 diag << "'this'";


Index: test/SemaCXX/warn-unused-lambda-capture.cpp
===
--- test/SemaCXX/warn-unused-lambda-capture.cpp
+++ test/SemaCXX/warn-unused-lambda-capture.cpp
@@ -191,3 +191,12 @@
 void test_use_template() {
   test_templated(); // expected-note{{in instantiation of function template specialization 'test_templated' requested here}}
 }
+
+namespace pr3 {
+int a;
+void b() {
+  int c[a];
+  auto vla_used = [] { return c[0]; };
+  auto vla_unused = [] {}; // expected-warning{{lambda capture 'c' is not used}}
+}
+} // namespace pr3
Index: lib/Sema/SemaLambda.cpp
===
--- lib/Sema/SemaLambda.cpp
+++ lib/Sema/SemaLambda.cpp
@@ -1481,6 +1481,9 @@
   if (CaptureHasSideEffects(From))
 return;
 
+  if (From.isVLATypeCapture())
+return;
+
   auto diag = Diag(From.getLocation(), diag::warn_unused_lambda_capture);
   if (From.isThisCapture())
 diag << "'this'";
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r320165 - Fix a comment in the code

[Kamil Rytarowski via cfe-commits]

Author: kamil
Date: Fri Dec  8 09:38:25 2017
New Revision: 320165

URL: http://llvm.org/viewvc/llvm-project?rev=320165=rev
Log:
Fix a comment in the code

The -ldl library is missing on NetBSD too, make the comment more generic.

Modified:
cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp

Modified: cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp?rev=320165=320164=320165=diff
==
--- cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp Fri Dec  8 09:38:25 2017
@@ -544,7 +544,7 @@ void tools::linkSanitizerRuntimeDeps(con
 CmdArgs.push_back("-lrt");
   }
   CmdArgs.push_back("-lm");
-  // There's no libdl on FreeBSD or RTEMS.
+  // There's no libdl on all OSes.
   if (TC.getTriple().getOS() != llvm::Triple::FreeBSD &&
   TC.getTriple().getOS() != llvm::Triple::NetBSD &&
   TC.getTriple().getOS() != llvm::Triple::RTEMS)


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

[PATCH] D40996: Add --no-cuda-version-check in unknown-std.cpp

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

Ideally the tests should be hermetic and should use mock CUDA installation that 
comes with the tests. E.g. `--cuda-path=%S/Inputs/CUDA/usr/local/cuda`


https://reviews.llvm.org/D40996



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

Re: r320162 - Revert "Unify implementation of our two different flavours of -Wtautological-compare."

[Hans Wennborg via cfe-commits]

On Fri, Dec 8, 2017 at 11:10 AM, Hans Wennborg  wrote:
> On Fri, Dec 8, 2017 at 9:30 AM, Richard Smith via cfe-commits
>  wrote:
>> On 8 Dec 2017 08:54, "Hans Wennborg via cfe-commits"
>>  wrote:
>>
>> Author: hans
>> Date: Fri Dec  8 08:54:08 2017
>> New Revision: 320162
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=320162=rev
>> Log:
>> Revert "Unify implementation of our two different flavours of
>> -Wtautological-compare."
>>
>>> Unify implementation of our two different flavours of
>>> -Wtautological-compare.
>>>
>>> In so doing, fix a handful of remaining bugs where we would report false
>>> positives or false negatives if we promote a signed value to an unsigned
>>> type
>>> for the comparison.
>>
>> This caused a new warning in Chromium:
>>
>> ../../base/trace_event/trace_log.cc:1545:29: error: comparison of constant
>> 64
>> with expression of type 'unsigned int' is always true
>> [-Werror,-Wtautological-constant-out-of-range-compare]
>>   DCHECK(handle.event_index < TraceBufferChunk::kTraceBufferChunkSize);
>>  ~~ ^ ~~~
>>
>> The 'unsigned int' is really a 6-bit bitfield, which is why it's always
>> less than 64.
>>
>> I thought we didn't use to warn (with out-of-range-compare) when comparing
>> against the boundaries of a type?
>>
>>
>> This isn't a "boundary of the type" case, though -- 64 is out of range for a
>> 6 bit bitfield. Your CHECK does nothing. Do you think that it's not
>> reasonable to warn on this bug, or that it's not a bug?
>
> I'm not sure it's a bug; and the CHECK will fire in case someone
> widens that bitfield and stores a larger value into it.
>
> I suppose I should rewrite the code as "handle.event_index <=
> TraceBufferChunk::kTraceBufferChunkSize -1"?
>
> The reason I thought this was unintentional is because I thought we
> don't warn in cases like:
>
> void f(int x) {
> if (x <= INT_MAX) {
> foo();
> }
> }
>
> and that's essentially what the code is doing, except it's using <
> instead of <=.

We do warn for

void f(int x) {
if (x < (long long)INT_MAX + 1) {
foo();
}
}

so I suppose your patch is consistent with the existing warning.

Sorry for reverting, go ahead and reland or let me know if you'd like
me to do it.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40968: [OpenMP] Diagnose function name on the link clause

[Kelvin Li via Phabricator via cfe-commits]

kkwli0 added inline comments.



Comment at: lib/Sema/SemaOpenMP.cpp:12671
+void Sema::checkDeclIsAllowedInOpenMPTarget(Expr *E, Decl *D,
+const DeclarationNameInfo *Id) {
   if (!D || D->isInvalidDecl())

ABataev wrote:
> You can get `DeclarationNameInfo` from the `FunctionDecl`:
> ```
> FD->getNameInfo()
> ```
This FD->getNameInfo() will only give the name info from the function 
definition.  What we need here is the name info for 'foo' that appears on the 
pragma in order to give us

```
d2.c:2:33: error: function name is not allowed in 'link' clause
#pragma omp declare target link(foo)
^
```


https://reviews.llvm.org/D40968



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

[PATCH] D41016: [Sema] Fix crash in unused-lambda-capture warning for VLAs

[Dimitry Andric via Phabricator via cfe-commits]

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

For the rest, LGTM.  It fixes the segfault, for both the full original test 
case, and my reduced version.




Comment at: lib/Sema/SemaLambda.cpp:1491
   else
 diag << From.getVariable();
   diag << From.isNonODRUsed();

Just for sanity's sake, I would still put an assert here, that `getVariable()` 
does not return `nullptr`.  That might catch similar issues in the future.  
(And it is better than a segfault... :) )



Repository:
  rC Clang

https://reviews.llvm.org/D41016



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

[PATCH] D40941: [ubsan] Use pass_object_size info in bounds checks (compiler-rt)

[Vedant Kumar via Phabricator via cfe-commits]

vsk closed this revision.
vsk added a comment.

Landed in r320129.


https://reviews.llvm.org/D40941



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

Re: r320162 - Revert "Unify implementation of our two different flavours of -Wtautological-compare."

[Hans Wennborg via cfe-commits]

On Fri, Dec 8, 2017 at 9:30 AM, Richard Smith via cfe-commits
 wrote:
> On 8 Dec 2017 08:54, "Hans Wennborg via cfe-commits"
>  wrote:
>
> Author: hans
> Date: Fri Dec  8 08:54:08 2017
> New Revision: 320162
>
> URL: http://llvm.org/viewvc/llvm-project?rev=320162=rev
> Log:
> Revert "Unify implementation of our two different flavours of
> -Wtautological-compare."
>
>> Unify implementation of our two different flavours of
>> -Wtautological-compare.
>>
>> In so doing, fix a handful of remaining bugs where we would report false
>> positives or false negatives if we promote a signed value to an unsigned
>> type
>> for the comparison.
>
> This caused a new warning in Chromium:
>
> ../../base/trace_event/trace_log.cc:1545:29: error: comparison of constant
> 64
> with expression of type 'unsigned int' is always true
> [-Werror,-Wtautological-constant-out-of-range-compare]
>   DCHECK(handle.event_index < TraceBufferChunk::kTraceBufferChunkSize);
>  ~~ ^ ~~~
>
> The 'unsigned int' is really a 6-bit bitfield, which is why it's always
> less than 64.
>
> I thought we didn't use to warn (with out-of-range-compare) when comparing
> against the boundaries of a type?
>
>
> This isn't a "boundary of the type" case, though -- 64 is out of range for a
> 6 bit bitfield. Your CHECK does nothing. Do you think that it's not
> reasonable to warn on this bug, or that it's not a bug?

I'm not sure it's a bug; and the CHECK will fire in case someone
widens that bitfield and stores a larger value into it.

I suppose I should rewrite the code as "handle.event_index <=
TraceBufferChunk::kTraceBufferChunkSize -1"?

The reason I thought this was unintentional is because I thought we
don't warn in cases like:

void f(int x) {
if (x <= INT_MAX) {
foo();
}
}

and that's essentially what the code is doing, except it's using <
instead of <=.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40940: [ubsan] Use pass_object_size info in bounds checks

[Vedant Kumar via Phabricator via cfe-commits]

vsk closed this revision.
vsk added a comment.

Landed in r320128.


https://reviews.llvm.org/D40940



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

[PATCH] D40685: [libunwind] Switch to add_llvm_install_targets

[Will Dietz via Phabricator via cfe-commits]

dtzWill added a comment.

Okay, that makes sense.  Thanks for accommodating my use-case anyway! :D.


Repository:
  rL LLVM

https://reviews.llvm.org/D40685



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

[PATCH] D40940: [ubsan] Use pass_object_size info in bounds checks

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> It's interesting to me that these array-bound checks don't seem to use 
> @llvm.objectsize in some form already.

That would be a cool experiment.  That said, one of the upsides of the current 
ubsan is that whether it will produce a diagnostic is predictable (as long as 
you don't use uninitialized data); you lose that to some extent with 
llvm.objectsize because it depends on the optimizer.




Comment at: lib/CodeGen/CGExpr.cpp:833
+  // Arrays don't have pass_object_size attributes, but if they have a constant
+  // size modifier it's the array size (C99 6.5.7.2p1).
+  if (auto *DecayedArrayTy = dyn_cast(ParamDecl->getType()))

"int f(int a[10])" might look like an array, but it isn't: it's just a 
different syntax to declare a pointer.  So it's legal to "lie" in the 
signature.  (If you want to actually pass a pointer to an array, you have to 
write "int (*a)[10]".)  And the definition of "static" says "an array with at 
least as many elements as specified by the size expression", which isn't a 
maximum, so that doesn't really help either.

Most people would consider it bad style to put a number into the array bound 
which doesn't reflect reality, but I think we shouldn't try to check it unless 
the user explicitly requests it.


https://reviews.llvm.org/D40940



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

r320185 - [ubsan] array-bounds: Ignore params with constant size

[Vedant Kumar via cfe-commits]

Author: vedantk
Date: Fri Dec  8 11:51:42 2017
New Revision: 320185

URL: http://llvm.org/viewvc/llvm-project?rev=320185=rev
Log:
[ubsan] array-bounds: Ignore params with constant size

This is a follow-up to r320128. Eli pointed out that there is some gray
area in the language standard about whether the constant size is exact,
or a lower bound.

https://reviews.llvm.org/D40940

Modified:
cfe/trunk/lib/CodeGen/CGExpr.cpp
cfe/trunk/test/CodeGen/ubsan-pass-object-size.c

Modified: cfe/trunk/lib/CodeGen/CGExpr.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExpr.cpp?rev=320185=320184=320185=diff
==
--- cfe/trunk/lib/CodeGen/CGExpr.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExpr.cpp Fri Dec  8 11:51:42 2017
@@ -829,14 +829,6 @@ llvm::Value *CodeGenFunction::LoadPassed
   if (!ParamDecl)
 return nullptr;
 
-  // Arrays don't have pass_object_size attributes, but if they have a constant
-  // size modifier it's the array size (C99 6.5.7.2p1).
-  if (auto *DecayedArrayTy = dyn_cast(ParamDecl->getType()))
-if (auto *ArrayTy =
-dyn_cast(DecayedArrayTy->getOriginalType()))
-  return llvm::ConstantInt::get(SizeTy,
-ArrayTy->getSize().getLimitedValue());
-
   auto *POSAttr = ParamDecl->getAttr();
   if (!POSAttr)
 return nullptr;

Modified: cfe/trunk/test/CodeGen/ubsan-pass-object-size.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/ubsan-pass-object-size.c?rev=320185=320184=320185=diff
==
--- cfe/trunk/test/CodeGen/ubsan-pass-object-size.c (original)
+++ cfe/trunk/test/CodeGen/ubsan-pass-object-size.c Fri Dec  8 11:51:42 2017
@@ -55,8 +55,7 @@ int pat(int *const p __attribute__((pass
 
 // CHECK-LABEL: define i32 @cat(
 int cat(int p[static 10], int n) {
-  // CHECK: icmp ult i64 {{.*}}, 10, !nosanitize
-  // CHECK: __ubsan_handle_out_of_bounds
+  // CHECK-NOT: __ubsan_handle_out_of_bounds
   // CHECK: ret i32
   return p[n];
 }


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

[PATCH] D40940: [ubsan] Use pass_object_size info in bounds checks

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

I backed out the part of this patch which deals with array parameters declared 
like p[10] or p[static 10]: r320185.




Comment at: lib/CodeGen/CGExpr.cpp:833
+  // Arrays don't have pass_object_size attributes, but if they have a constant
+  // size modifier it's the array size (C99 6.5.7.2p1).
+  if (auto *DecayedArrayTy = dyn_cast(ParamDecl->getType()))

efriedma wrote:
> "int f(int a[10])" might look like an array, but it isn't: it's just a 
> different syntax to declare a pointer.  So it's legal to "lie" in the 
> signature.  (If you want to actually pass a pointer to an array, you have to 
> write "int (*a)[10]".)  And the definition of "static" says "an array with at 
> least as many elements as specified by the size expression", which isn't a 
> maximum, so that doesn't really help either.
> 
> Most people would consider it bad style to put a number into the array bound 
> which doesn't reflect reality, but I think we shouldn't try to check it 
> unless the user explicitly requests it.
My copy of the C99 draft (n1256) is a little fuzzy on this point [*]. There's 
enough of a gray area here that it seems appropriate to back out this part of 
the patch.

* It states: "In addition to optional type qualifiers and the keyword static, 
the [ and ] may delimit an expression or *. If they delimit an expression 
(which specifies the size of an array) ...". I took the parenthetical 
literally, and didn't know about the 'at least as many' language.


https://reviews.llvm.org/D40940



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

[PATCH] D40712: Add cc1 flag enabling the function stack size section that was added in r319430

[Matthias Braun via Phabricator via cfe-commits]

MatzeB added a comment.

Looks good to me, but I'm not too familiar with the clang driver code. So it 
would be good if you could at least add some more experienced clang developers 
into the reviewer list and wait some more days before committing.

I also wonder whether it would be possible to move the fact that it defaults to 
on for PS4 into the PS4CPU.cpp file somehow (not that things are distributed 
that cleanly right now anyway).


https://reviews.llvm.org/D40712



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

[PATCH] D40948: Switch Clang's default C++ language target to C++14

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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

LGTM with the PS4 comment removed. Thank you!

Please also update the documentation and the release notes.




Comment at: clang/test/CodeGenCXX/vtable-available-externally.cpp:275
 struct C {
   virtual D& operator=(const D&);
 };

t.p.northover wrote:
> rsmith wrote:
> > To make this test work in C++11 onwards, you need to add a virtual move 
> > assignment operator here:
> > 
> > ```
> > virtual D& operator=(D&&);
> > ```
> That didn't quite work. The issue appears to be that D has both of those 
> implicitly defined in C++14 mode, but only the move assignment operator is 
> used below. Speculative VTable emission requires all of them to be used.
> 
> So adding a "d = d;" line to the second g function fixes the test. Does that 
> sound sane to you, or am I missing the point?
This sounds like a good approach, thanks.


Repository:
  rC Clang

https://reviews.llvm.org/D40948



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

Re: r320162 - Revert "Unify implementation of our two different flavours of -Wtautological-compare."

[Bill Seurer via cfe-commits]

It also caused all the sanitizer builds to fail.  For example:

http://lab.llvm.org:8011/builders/sanitizer-ppc64le-linux/builds/3654

/home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm/utils/TableGen/X86RecognizableInstr.cpp:789:21: 
error: comparison of constant -1 with expression of type 
'llvm::X86Disassembler::OpcodeType' is always true 
[-Werror,-Wtautological-constant-out-of-range-compare]

  assert(opcodeType != (OpcodeType)-1 &&
 ~~ ^  ~~
/usr/include/assert.h:86:5: note: expanded from macro 'assert'
  ((expr)   \
^~~~
1 error generated.
utils/TableGen/CMakeFiles/obj.llvm-tblgen.dir/build.make:1022: recipe 
for target 
'utils/TableGen/CMakeFiles/obj.llvm-tblgen.dir/X86RecognizableInstr.cpp.o' 
failed
make[3]: *** 
[utils/TableGen/CMakeFiles/obj.llvm-tblgen.dir/X86RecognizableInstr.cpp.o] 
Error 1




And there are lots more warnings (which are flagged as errors for the 
sanitizer builds) when I run a build by hand.  For example:


[4001/4008] Building CXX object 
tools/clang/tools/libclang/CMakeFiles/libclang.dir/CIndex.cpp.o
In file included from 
/home/seurer/llvm/llvm-test2/tools/clang/tools/libclang/CIndex.cpp:23:
In file included from 
/home/seurer/llvm/llvm-test2/tools/clang/tools/libclang/CursorVisitor.h:16:
In file included from 
/home/seurer/llvm/llvm-test2/tools/clang/include/clang/AST/DeclVisitor.h:19:
In file included from 
/home/seurer/llvm/llvm-test2/tools/clang/include/clang/AST/DeclCXX.h:21:
In file included from 
/home/seurer/llvm/llvm-test2/tools/clang/include/clang/AST/Attr.h:19:
/home/seurer/llvm/llvm-test2/tools/clang/include/clang/AST/Expr.h:2745:17: 
warning: comparison of constant -1 with expression of type 'const 
clang::CastKind' is always true 
[-Wtautological-constant-out-of-range-compare]

assert(kind != CK_Invalid && "creating cast with invalid cast kind");
    ^  ~~
/usr/include/assert.h:89:5: note: expanded from macro 'assert'
  ((expr)   \
^~~~


On 12/08/2017 10:54 AM, Hans Wennborg via cfe-commits wrote:

Author: hans
Date: Fri Dec  8 08:54:08 2017
New Revision: 320162

URL: http://llvm.org/viewvc/llvm-project?rev=320162=rev
Log:
Revert "Unify implementation of our two different flavours of 
-Wtautological-compare."


Unify implementation of our two different flavours of -Wtautological-compare.

In so doing, fix a handful of remaining bugs where we would report false
positives or false negatives if we promote a signed value to an unsigned type
for the comparison.


This caused a new warning in Chromium:

../../base/trace_event/trace_log.cc:1545:29: error: comparison of constant 64
with expression of type 'unsigned int' is always true
[-Werror,-Wtautological-constant-out-of-range-compare]
   DCHECK(handle.event_index < TraceBufferChunk::kTraceBufferChunkSize);
  ~~ ^ ~~~

The 'unsigned int' is really a 6-bit bitfield, which is why it's always
less than 64.

I thought we didn't use to warn (with out-of-range-compare) when comparing
against the boundaries of a type?

Modified:
 cfe/trunk/lib/Sema/SemaChecking.cpp
 cfe/trunk/test/Sema/tautological-constant-compare.c
 cfe/trunk/test/Sema/tautological-constant-enum-compare.c
 cfe/trunk/test/SemaCXX/compare.cpp

Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=320162=320161=320162=diff
==
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Dec  8 08:54:08 2017
@@ -8662,113 +8662,54 @@ static bool isKnownToHaveUnsignedValue(E
  }
  
  namespace {

-/// The promoted range of values of a type. In general this has the
-/// following structure:
-///
-/// |---| . . . |---|
-/// ^   ^   ^   ^
-///Min   HoleMin  HoleMax  Max
-///
-/// ... where there is only a hole if a signed type is promoted to unsigned
-/// (in which case Min and Max are the smallest and largest representable
-/// values).
-struct PromotedRange {
-  // Min, or HoleMax if there is a hole.
-  llvm::APSInt PromotedMin;
-  // Max, or HoleMin if there is a hole.
-  llvm::APSInt PromotedMax;
-
-  PromotedRange(IntRange R, unsigned BitWidth, bool Unsigned) {
-if (R.Width == 0)
-  PromotedMin = PromotedMax = llvm::APSInt(BitWidth, Unsigned);
-else {
-  PromotedMin = llvm::APSInt::getMinValue(R.Width, R.NonNegative)
-.extOrTrunc(BitWidth);
-  PromotedMin.setIsUnsigned(Unsigned);
-
-  PromotedMax = llvm::APSInt::getMaxValue(R.Width, R.NonNegative)
-.extOrTrunc(BitWidth);
-  PromotedMax.setIsUnsigned(Unsigned);
-}
-  }
  
-  // Determine whether this range is contiguous (has 

Re: r320162 - Revert "Unify implementation of our two different flavours of -Wtautological-compare."

[Richard Smith via cfe-commits]

I'm going to reland without the changes to bit-field handling. If we want
those changes (which I think we do, based on the bugs it found in
selfhost), that'll need to be done more carefully, and I don't want to get
the refactoring and bugfixes here tangled up with that.

On 8 December 2017 at 13:27, Bill Seurer via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> It also caused all the sanitizer builds to fail.  For example:
>
> http://lab.llvm.org:8011/builders/sanitizer-ppc64le-linux/builds/3654
>
> /home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/
> llvm/utils/TableGen/X86RecognizableInstr.cpp:789:21: error: comparison of
> constant -1 with expression of type 'llvm::X86Disassembler::OpcodeType'
> is always true [-Werror,-Wtautological-constant-out-of-range-compare]
>   assert(opcodeType != (OpcodeType)-1 &&
>  ~~ ^  ~~
> /usr/include/assert.h:86:5: note: expanded from macro 'assert'
>   ((expr)   \
> ^~~~
> 1 error generated.
> utils/TableGen/CMakeFiles/obj.llvm-tblgen.dir/build.make:1022: recipe for
> target 
> 'utils/TableGen/CMakeFiles/obj.llvm-tblgen.dir/X86RecognizableInstr.cpp.o'
> failed
> make[3]: *** 
> [utils/TableGen/CMakeFiles/obj.llvm-tblgen.dir/X86RecognizableInstr.cpp.o]
> Error 1
>
>
>
> And there are lots more warnings (which are flagged as errors for the
> sanitizer builds) when I run a build by hand.  For example:
>
> [4001/4008] Building CXX object tools/clang/tools/libclang/CMa
> keFiles/libclang.dir/CIndex.cpp.o
> In file included from /home/seurer/llvm/llvm-test2/t
> ools/clang/tools/libclang/CIndex.cpp:23:
> In file included from /home/seurer/llvm/llvm-test2/t
> ools/clang/tools/libclang/CursorVisitor.h:16:
> In file included from /home/seurer/llvm/llvm-test2/t
> ools/clang/include/clang/AST/DeclVisitor.h:19:
> In file included from /home/seurer/llvm/llvm-test2/t
> ools/clang/include/clang/AST/DeclCXX.h:21:
> In file included from /home/seurer/llvm/llvm-test2/t
> ools/clang/include/clang/AST/Attr.h:19:
> /home/seurer/llvm/llvm-test2/tools/clang/include/clang/AST/Expr.h:2745:17:
> warning: comparison of constant -1 with expression of type 'const
> clang::CastKind' is always true [-Wtautological-constant-out-o
> f-range-compare]
> assert(kind != CK_Invalid && "creating cast with invalid cast kind");
> ^  ~~
> /usr/include/assert.h:89:5: note: expanded from macro 'assert'
>   ((expr)   \
> ^~~~
>
>
>
> On 12/08/2017 10:54 AM, Hans Wennborg via cfe-commits wrote:
>
>> Author: hans
>> Date: Fri Dec  8 08:54:08 2017
>> New Revision: 320162
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=320162=rev
>> Log:
>> Revert "Unify implementation of our two different flavours of
>> -Wtautological-compare."
>>
>> Unify implementation of our two different flavours of
>>> -Wtautological-compare.
>>>
>>> In so doing, fix a handful of remaining bugs where we would report false
>>> positives or false negatives if we promote a signed value to an unsigned
>>> type
>>> for the comparison.
>>>
>>
>> This caused a new warning in Chromium:
>>
>> ../../base/trace_event/trace_log.cc:1545:29: error: comparison of
>> constant 64
>> with expression of type 'unsigned int' is always true
>> [-Werror,-Wtautological-constant-out-of-range-compare]
>>DCHECK(handle.event_index < TraceBufferChunk::kTraceBufferChunkSize);
>>   ~~ ^ ~~~
>>
>> The 'unsigned int' is really a 6-bit bitfield, which is why it's always
>> less than 64.
>>
>> I thought we didn't use to warn (with out-of-range-compare) when comparing
>> against the boundaries of a type?
>>
>> Modified:
>>  cfe/trunk/lib/Sema/SemaChecking.cpp
>>  cfe/trunk/test/Sema/tautological-constant-compare.c
>>  cfe/trunk/test/Sema/tautological-constant-enum-compare.c
>>  cfe/trunk/test/SemaCXX/compare.cpp
>>
>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaC
>> hecking.cpp?rev=320162=320161=320162=diff
>> 
>> ==
>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Dec  8 08:54:08 2017
>> @@ -8662,113 +8662,54 @@ static bool isKnownToHaveUnsignedValue(E
>>   }
>> namespace {
>> -/// The promoted range of values of a type. In general this has the
>> -/// following structure:
>> -///
>> -/// |---| . . . |---|
>> -/// ^   ^   ^   ^
>> -///Min   HoleMin  HoleMax  Max
>> -///
>> -/// ... where there is only a hole if a signed type is promoted to
>> unsigned
>> -/// (in which case Min and Max are the smallest and largest representable
>> -/// values).
>> -struct PromotedRange {
>> -  // Min, or HoleMax if there is a hole.
>> -  llvm::APSInt PromotedMin;
>> -  

Re: r320162 - Revert "Unify implementation of our two different flavours of -Wtautological-compare."

[Hans Wennborg via cfe-commits]

For what it's worth, Chromium's bitfield warning is easy to fix. The
"-1 vs enum" comparisons are more plentiful and less straightforward.
I'm guessing it's the same for the self-host.

On Fri, Dec 8, 2017 at 1:33 PM, Richard Smith via cfe-commits
 wrote:
> I'm going to reland without the changes to bit-field handling. If we want
> those changes (which I think we do, based on the bugs it found in selfhost),
> that'll need to be done more carefully, and I don't want to get the
> refactoring and bugfixes here tangled up with that.
>
> On 8 December 2017 at 13:27, Bill Seurer via cfe-commits
>  wrote:
>>
>> It also caused all the sanitizer builds to fail.  For example:
>>
>> http://lab.llvm.org:8011/builders/sanitizer-ppc64le-linux/builds/3654
>>
>>
>> /home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/llvm/utils/TableGen/X86RecognizableInstr.cpp:789:21:
>> error: comparison of constant -1 with expression of type
>> 'llvm::X86Disassembler::OpcodeType' is always true
>> [-Werror,-Wtautological-constant-out-of-range-compare]
>>   assert(opcodeType != (OpcodeType)-1 &&
>>  ~~ ^  ~~
>> /usr/include/assert.h:86:5: note: expanded from macro 'assert'
>>   ((expr)   \
>> ^~~~
>> 1 error generated.
>> utils/TableGen/CMakeFiles/obj.llvm-tblgen.dir/build.make:1022: recipe for
>> target
>> 'utils/TableGen/CMakeFiles/obj.llvm-tblgen.dir/X86RecognizableInstr.cpp.o'
>> failed
>> make[3]: ***
>> [utils/TableGen/CMakeFiles/obj.llvm-tblgen.dir/X86RecognizableInstr.cpp.o]
>> Error 1
>>
>>
>>
>> And there are lots more warnings (which are flagged as errors for the
>> sanitizer builds) when I run a build by hand.  For example:
>>
>> [4001/4008] Building CXX object
>> tools/clang/tools/libclang/CMakeFiles/libclang.dir/CIndex.cpp.o
>> In file included from
>> /home/seurer/llvm/llvm-test2/tools/clang/tools/libclang/CIndex.cpp:23:
>> In file included from
>> /home/seurer/llvm/llvm-test2/tools/clang/tools/libclang/CursorVisitor.h:16:
>> In file included from
>> /home/seurer/llvm/llvm-test2/tools/clang/include/clang/AST/DeclVisitor.h:19:
>> In file included from
>> /home/seurer/llvm/llvm-test2/tools/clang/include/clang/AST/DeclCXX.h:21:
>> In file included from
>> /home/seurer/llvm/llvm-test2/tools/clang/include/clang/AST/Attr.h:19:
>> /home/seurer/llvm/llvm-test2/tools/clang/include/clang/AST/Expr.h:2745:17:
>> warning: comparison of constant -1 with expression of type 'const
>> clang::CastKind' is always true
>> [-Wtautological-constant-out-of-range-compare]
>> assert(kind != CK_Invalid && "creating cast with invalid cast kind");
>> ^  ~~
>> /usr/include/assert.h:89:5: note: expanded from macro 'assert'
>>   ((expr)   \
>> ^~~~
>>
>>
>>
>> On 12/08/2017 10:54 AM, Hans Wennborg via cfe-commits wrote:
>>>
>>> Author: hans
>>> Date: Fri Dec  8 08:54:08 2017
>>> New Revision: 320162
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=320162=rev
>>> Log:
>>> Revert "Unify implementation of our two different flavours of
>>> -Wtautological-compare."
>>>
 Unify implementation of our two different flavours of
 -Wtautological-compare.

 In so doing, fix a handful of remaining bugs where we would report false
 positives or false negatives if we promote a signed value to an unsigned
 type
 for the comparison.
>>>
>>>
>>> This caused a new warning in Chromium:
>>>
>>> ../../base/trace_event/trace_log.cc:1545:29: error: comparison of
>>> constant 64
>>> with expression of type 'unsigned int' is always true
>>> [-Werror,-Wtautological-constant-out-of-range-compare]
>>>DCHECK(handle.event_index < TraceBufferChunk::kTraceBufferChunkSize);
>>>   ~~ ^ ~~~
>>>
>>> The 'unsigned int' is really a 6-bit bitfield, which is why it's always
>>> less than 64.
>>>
>>> I thought we didn't use to warn (with out-of-range-compare) when
>>> comparing
>>> against the boundaries of a type?
>>>
>>> Modified:
>>>  cfe/trunk/lib/Sema/SemaChecking.cpp
>>>  cfe/trunk/test/Sema/tautological-constant-compare.c
>>>  cfe/trunk/test/Sema/tautological-constant-enum-compare.c
>>>  cfe/trunk/test/SemaCXX/compare.cpp
>>>
>>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=320162=320161=320162=diff
>>>
>>> ==
>>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Dec  8 08:54:08 2017
>>> @@ -8662,113 +8662,54 @@ static bool isKnownToHaveUnsignedValue(E
>>>   }
>>> namespace {
>>> -/// The promoted range of values of a type. In general this has the
>>> -/// following structure:
>>> -///
>>> -/// |---| . . . 

[PATCH] D40968: [OpenMP] Diagnose function name on the link clause

[Kelvin Li via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaOpenMP.cpp:12671
+void Sema::checkDeclIsAllowedInOpenMPTarget(Expr *E, Decl *D,
+const DeclarationNameInfo *Id) {
   if (!D || D->isInvalidDecl())

ABataev wrote:
> kkwli0 wrote:
> > ABataev wrote:
> > > You can get `DeclarationNameInfo` from the `FunctionDecl`:
> > > ```
> > > FD->getNameInfo()
> > > ```
> > This FD->getNameInfo() will only give the name info from the function 
> > definition.  What we need here is the name info for 'foo' that appears on 
> > the pragma in order to give us
> > 
> > ```
> > d2.c:2:33: error: function name is not allowed in 'link' clause
> > #pragma omp declare target link(foo)
> > ^
> > ```
> Then just pass `SourceLocation`
Sure.


https://reviews.llvm.org/D40968



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

[libcxx] r320201 - [libc++] Unbreak Apple buildbots

[Shoaib Meenai via cfe-commits]

Author: smeenai
Date: Fri Dec  8 13:50:32 2017
New Revision: 320201

URL: http://llvm.org/viewvc/llvm-project?rev=320201=rev
Log:
[libc++] Unbreak Apple buildbots

These buildbots are using the deprecated target name install-libcxx-headers
instead of the more up to date install-cxx-headers, so I need to add an
install-libcxx-headers-stripped target to satisfy them.

Modified:
libcxx/trunk/include/CMakeLists.txt

Modified: libcxx/trunk/include/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/CMakeLists.txt?rev=320201=320200=320201=diff
==
--- libcxx/trunk/include/CMakeLists.txt (original)
+++ libcxx/trunk/include/CMakeLists.txt Fri Dec  8 13:50:32 2017
@@ -63,6 +63,7 @@ if (LIBCXX_INSTALL_HEADERS)
 
 add_custom_target(libcxx-headers)
 add_custom_target(install-libcxx-headers DEPENDS install-cxx-headers)
+add_custom_target(install-libcxx-headers-stripped DEPENDS 
install-cxx-headers-stripped)
   endif()
 
 endif()


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

[PATCH] D41031: [clangd] (Attempt to) read clang-format file for document formatting

[Raoul Wols via Phabricator via cfe-commits]

rwols created this revision.

Takes into account the clang-format file of the project, if any.
Reverts to LLVM if nothing is found. Replies with an error if any error occured.
For instance, a parse error in the clang-format YAML file.


https://reviews.llvm.org/D41031

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h

Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -286,12 +286,19 @@
   /// given a header file and vice versa.
   llvm::Optional switchSourceHeader(PathRef Path);
 
-  /// Run formatting for \p Rng inside \p File.
-  std::vector formatRange(PathRef File, Range Rng);
-  /// Run formatting for the whole \p File.
-  std::vector formatFile(PathRef File);
-  /// Run formatting after a character was typed at \p Pos in \p File.
-  std::vector formatOnType(PathRef File, Position Pos);
+  /// Run formatting for \p Rng inside \p File with content \p Code.
+  llvm::Expected formatRange(StringRef Code,
+PathRef File, Range Rng);
+
+  /// Run formatting for the whole \p File with content \p Code.
+  llvm::Expected formatFile(StringRef Code,
+   PathRef File);
+
+  /// Run formatting after a character was typed at \p Pos in \p File with
+  /// content \p Code.
+  llvm::Expected
+  formatOnType(StringRef Code, PathRef File, Position Pos);
+
   /// Rename all occurrences of the symbol at the \p Pos in \p File to
   /// \p NewName.
   Expected rename(PathRef File, Position Pos,
@@ -311,6 +318,13 @@
   void onFileEvent(const DidChangeWatchedFilesParams );
 
 private:
+
+  /// FIXME: This stats several files to find a .clang-format file. I/O can be
+  /// slow. Think of a way to cache this.
+  llvm::Expected
+  formatCode(llvm::StringRef Code, PathRef File,
+ ArrayRef Ranges);
+
   std::future
   scheduleReparseAndDiags(PathRef File, VersionedDraft Contents,
   std::shared_ptr Resources,
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -38,16 +38,6 @@
   std::promise 
 };
 
-std::vector formatCode(StringRef Code, StringRef Filename,
- ArrayRef Ranges) {
-  // Call clang-format.
-  // FIXME: Don't ignore style.
-  format::FormatStyle Style = format::getLLVMStyle();
-  auto Result = format::reformat(Style, Code, Ranges, Filename);
-
-  return std::vector(Result.begin(), Result.end());
-}
-
 std::string getStandardResourceDir() {
   static int Dummy; // Just an address in this process.
   return CompilerInvocation::GetResourcesPath("clangd", (void *));
@@ -331,26 +321,23 @@
   return make_tagged(std::move(Result), TaggedFS.Tag);
 }
 
-std::vector ClangdServer::formatRange(PathRef File,
-Range Rng) {
-  std::string Code = getDocument(File);
-
+llvm::Expected
+ClangdServer::formatRange(StringRef Code, PathRef File, Range Rng) {
   size_t Begin = positionToOffset(Code, Rng.start);
   size_t Len = positionToOffset(Code, Rng.end) - Begin;
   return formatCode(Code, File, {tooling::Range(Begin, Len)});
 }
 
-std::vector ClangdServer::formatFile(PathRef File) {
+llvm::Expected ClangdServer::formatFile(StringRef Code,
+   PathRef File) {
   // Format everything.
-  std::string Code = getDocument(File);
   return formatCode(Code, File, {tooling::Range(0, Code.size())});
 }
 
-std::vector ClangdServer::formatOnType(PathRef File,
- Position Pos) {
+llvm::Expected
+ClangdServer::formatOnType(StringRef Code, PathRef File, Position Pos) {
   // Look for the previous opening brace from the character position and
   // format starting from there.
-  std::string Code = getDocument(File);
   size_t CursorPos = positionToOffset(Code, Pos);
   size_t PreviousLBracePos = StringRef(Code).find_last_of('{', CursorPos);
   if (PreviousLBracePos == StringRef::npos)
@@ -509,6 +496,20 @@
   return llvm::None;
 }
 
+llvm::Expected
+ClangdServer::formatCode(llvm::StringRef Code, PathRef File,
+ ArrayRef Ranges) {
+  // Call clang-format.
+  auto TaggedFS = FSProvider.getTaggedFileSystem(File);
+  auto StyleOrError =
+  format::getStyle("file", File, "LLVM", Code, TaggedFS.Value.get());
+  if (!StyleOrError) {
+return StyleOrError.takeError();
+  } else {
+return format::reformat(StyleOrError.get(), Code, Ranges, File);
+  }
+}
+
 std::future ClangdServer::scheduleReparseAndDiags(
 PathRef File, VersionedDraft Contents, std::shared_ptr 

Re: r320162 - Revert "Unify implementation of our two different flavours of -Wtautological-compare."

[Richard Smith via cfe-commits]

For now, I've undone the change that caused us to warn (ever!) on
comparisons between two expressions of enumeration type, and also the
change to use more precise bit-width information for bit-field comparisons,
and re-committed as r320211.

The bugs exposed by self-host have been fixed in r320212 and r320206.

On 8 December 2017 at 14:09, Richard Smith  wrote:

> Selfhost failures are ultimately caused by:
>
> https://github.com/llvm-mirror/llvm/blob/master/utils/
> TableGen/X86RecognizableInstr.cpp#L709
> https://github.com/llvm-mirror/clang/blob/master/include/clang/AST/
> OperationKinds.h#L26
>
> These are genuine bugs: assigning -1 into an enumeration with no negative
> enumerators results in undefined behavior. We should probably have a
> warning for the casts here rather than only detecting the problem under
> -Wtautological-compare, but I would not be surprised to find that Clang
> miscompiles itself with -fstrict-enums due to the above two issues.
>
> On 8 December 2017 at 13:40, Hans Wennborg via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> For what it's worth, Chromium's bitfield warning is easy to fix. The
>> "-1 vs enum" comparisons are more plentiful and less straightforward.
>> I'm guessing it's the same for the self-host.
>>
>> On Fri, Dec 8, 2017 at 1:33 PM, Richard Smith via cfe-commits
>>  wrote:
>> > I'm going to reland without the changes to bit-field handling. If we
>> want
>> > those changes (which I think we do, based on the bugs it found in
>> selfhost),
>> > that'll need to be done more carefully, and I don't want to get the
>> > refactoring and bugfixes here tangled up with that.
>> >
>> > On 8 December 2017 at 13:27, Bill Seurer via cfe-commits
>> >  wrote:
>> >>
>> >> It also caused all the sanitizer builds to fail.  For example:
>> >>
>> >> http://lab.llvm.org:8011/builders/sanitizer-ppc64le-linux/builds/3654
>> >>
>> >>
>> >> /home/buildbots/ppc64le-sanitizer/sanitizer-ppc64le/build/
>> llvm/utils/TableGen/X86RecognizableInstr.cpp:789:21:
>> >> error: comparison of constant -1 with expression of type
>> >> 'llvm::X86Disassembler::OpcodeType' is always true
>> >> [-Werror,-Wtautological-constant-out-of-range-compare]
>> >>   assert(opcodeType != (OpcodeType)-1 &&
>> >>  ~~ ^  ~~
>> >> /usr/include/assert.h:86:5: note: expanded from macro 'assert'
>> >>   ((expr)
>>  \
>> >> ^~~~
>> >> 1 error generated.
>> >> utils/TableGen/CMakeFiles/obj.llvm-tblgen.dir/build.make:1022: recipe
>> for
>> >> target
>> >> 'utils/TableGen/CMakeFiles/obj.llvm-tblgen.dir/X86Recognizab
>> leInstr.cpp.o'
>> >> failed
>> >> make[3]: ***
>> >> [utils/TableGen/CMakeFiles/obj.llvm-tblgen.dir/X86Recognizab
>> leInstr.cpp.o]
>> >> Error 1
>> >>
>> >>
>> >>
>> >> And there are lots more warnings (which are flagged as errors for the
>> >> sanitizer builds) when I run a build by hand.  For example:
>> >>
>> >> [4001/4008] Building CXX object
>> >> tools/clang/tools/libclang/CMakeFiles/libclang.dir/CIndex.cpp.o
>> >> In file included from
>> >> /home/seurer/llvm/llvm-test2/tools/clang/tools/libclang/CIndex.cpp:23:
>> >> In file included from
>> >> /home/seurer/llvm/llvm-test2/tools/clang/tools/libclang/Curs
>> orVisitor.h:16:
>> >> In file included from
>> >> /home/seurer/llvm/llvm-test2/tools/clang/include/clang/AST/D
>> eclVisitor.h:19:
>> >> In file included from
>> >> /home/seurer/llvm/llvm-test2/tools/clang/include/clang/AST/D
>> eclCXX.h:21:
>> >> In file included from
>> >> /home/seurer/llvm/llvm-test2/tools/clang/include/clang/AST/Attr.h:19:
>> >> /home/seurer/llvm/llvm-test2/tools/clang/include/clang/AST/E
>> xpr.h:2745:17:
>> >> warning: comparison of constant -1 with expression of type 'const
>> >> clang::CastKind' is always true
>> >> [-Wtautological-constant-out-of-range-compare]
>> >> assert(kind != CK_Invalid && "creating cast with invalid cast
>> kind");
>> >> ^  ~~
>> >> /usr/include/assert.h:89:5: note: expanded from macro 'assert'
>> >>   ((expr)
>>  \
>> >> ^~~~
>> >>
>> >>
>> >>
>> >> On 12/08/2017 10:54 AM, Hans Wennborg via cfe-commits wrote:
>> >>>
>> >>> Author: hans
>> >>> Date: Fri Dec  8 08:54:08 2017
>> >>> New Revision: 320162
>> >>>
>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=320162=rev
>> >>> Log:
>> >>> Revert "Unify implementation of our two different flavours of
>> >>> -Wtautological-compare."
>> >>>
>>  Unify implementation of our two different flavours of
>>  -Wtautological-compare.
>> 
>>  In so doing, fix a handful of remaining bugs where we would report
>> false
>>  positives or false negatives if we promote a signed value to an
>> unsigned
>>  type
>>  for the comparison.
>> >>>
>> >>>
>> >>> This caused a new warning in Chromium:
>> >>>
>> >>> ../../base/trace_event/trace_log.cc:1545:29: error: comparison of
>> >>> constant 64
>> >>> with expression of type 'unsigned int' is always 

[PATCH] D41042: [analyzer] StackAddrEscape: Delay turning on by default a little bit?

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
Herald added subscribers: cfe-commits, a.sidorin, szepet, xazax.hun.

I'm seeing false positives on the new check added in 
https://reviews.llvm.org/D39438 of the following kind:

  void foo() {
T buf[16];
for ( int n = 0; n < 16; ++n) {
  T *ptr = [n];
  dispatch_async(queue, ^{ use(ptr); });
}
dispatch_barrier_sync(queue, ^{});
  }

In this case, pointer to the local variable `buf` is captured by a block that 
goes into `dispatch_async`, yet it is not a bug because synchronization ensures 
that this block quits before the variable goes out of scope.

I guess it's kinda similar to the semaphore synchronization case. We could 
probably add another suppression for functions that contain any 
`dispatch_barrier_async` calls. The ideal solution is to delay the warning 
until `checkEndFunction` (like the escape-to-global check does) to see if 
things get synced up until then (of course, once we have scopes we can make it 
even more fine-grained).

Alexander, do you think we should keep the checker under a flag (not enabled 
for regular users) until we get a better understanding of what kinds of 
synchronizations can get on the way? This diff splits your new check into 
`alpha.core.StackAddressAsyncEscape`. Or do you already have good quick fixes 
coming?


Repository:
  rC Clang

https://reviews.llvm.org/D41042

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  test/Analysis/stack-capture-leak-arc.mm


Index: test/Analysis/stack-capture-leak-arc.mm
===
--- test/Analysis/stack-capture-leak-arc.mm
+++ test/Analysis/stack-capture-leak-arc.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 
-analyzer-checker=core -fblocks -fobjc-arc -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 
-analyzer-checker=core,alpha.core.StackAddressAsyncEscape -fblocks -fobjc-arc 
-verify %s
 
 typedef struct dispatch_queue_s *dispatch_queue_t;
 typedef void (^dispatch_block_t)(void);
Index: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -37,6 +37,14 @@
   mutable std::unique_ptr BT_capturedstackret;
 
 public:
+  enum CheckKind {
+CK_StackAddrEscapeChecker,
+CK_StackAddrAsyncEscapeChecker,
+CK_NumCheckKinds
+  };
+
+  DefaultBool ChecksEnabled[CK_NumCheckKinds];
+
   void checkPreCall(const CallEvent , CheckerContext ) const;
   void checkPreStmt(const ReturnStmt *RS, CheckerContext ) const;
   void checkEndFunction(CheckerContext ) const;
@@ -225,6 +233,8 @@
 
 void StackAddrEscapeChecker::checkPreCall(const CallEvent ,
   CheckerContext ) const {
+  if (!ChecksEnabled[CK_StackAddrAsyncEscapeChecker])
+return;
   if (!Call.isGlobalCFunction("dispatch_after") &&
   !Call.isGlobalCFunction("dispatch_async"))
 return;
@@ -237,6 +247,8 @@
 
 void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS,
   CheckerContext ) const {
+  if (!ChecksEnabled[CK_StackAddrEscapeChecker])
+return;
 
   const Expr *RetE = RS->getRetValue();
   if (!RetE)
@@ -277,6 +289,9 @@
 }
 
 void StackAddrEscapeChecker::checkEndFunction(CheckerContext ) const {
+  if (!ChecksEnabled[CK_StackAddrEscapeChecker])
+return;
+
   ProgramStateRef State = Ctx.getState();
 
   // Iterate over all bindings to global variables and see if it contains
@@ -346,6 +361,12 @@
   }
 }
 
-void ento::registerStackAddrEscapeChecker(CheckerManager ) {
-  Mgr.registerChecker();
-}
+#define REGISTER_CHECKER(name) \
+  void ento::register##name(CheckerManager ) { \
+StackAddrEscapeChecker *Chk = \
+Mgr.registerChecker(); \
+Chk->ChecksEnabled[StackAddrEscapeChecker::CK_##name] = true; \
+  }
+
+REGISTER_CHECKER(StackAddrEscapeChecker)
+REGISTER_CHECKER(StackAddrAsyncEscapeChecker)
Index: include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -188,6 +188,10 @@
   HelpText<"Check for cases where the dynamic and the static type of an object 
are unrelated.">,
   DescFile<"DynamicTypeChecker.cpp">;
 
+def StackAddrAsyncEscapeChecker : Checker<"StackAddressAsyncEscape">,
+  HelpText<"Check that addresses to stack memory do not escape the function">,
+  DescFile<"StackAddrEscapeChecker.cpp">;
+
 } // end "alpha.core"
 
 let ParentPackage = Nullability in {


Index: test/Analysis/stack-capture-leak-arc.mm
===
--- test/Analysis/stack-capture-leak-arc.mm
+++ test/Analysis/stack-capture-leak-arc.mm
@@ -1,4 +1,4 @@

[PATCH] D41044: Implementation of -fextend-lifetimes and -fextend-this-ptr to aid with debugging of optimized code

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

This might be something we want to turn on automatically for -Og.


https://reviews.llvm.org/D41044



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

[PATCH] D40995: [TextDiagnosticBuffer] Fix diagnostic note emission order.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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

Huh, I'm amazed we've had this bug for so long. Thanks!


https://reviews.llvm.org/D40995



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

[PATCH] D41042: [analyzer] StackAddrEscape: Delay turning on by default a little bit?

[Artem Dergachev via Phabricator via cfe-commits]

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

Update the other run-line.


https://reviews.llvm.org/D41042

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  test/Analysis/stack-capture-leak-arc.mm
  test/Analysis/stack-capture-leak-no-arc.mm


Index: test/Analysis/stack-capture-leak-no-arc.mm
===
--- test/Analysis/stack-capture-leak-no-arc.mm
+++ test/Analysis/stack-capture-leak-no-arc.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 
-analyzer-checker=core -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 
-analyzer-checker=core,alpha.core.StackAddressAsyncEscape -fblocks -verify %s
 
 typedef struct dispatch_queue_s *dispatch_queue_t;
 typedef void (^dispatch_block_t)(void);
Index: test/Analysis/stack-capture-leak-arc.mm
===
--- test/Analysis/stack-capture-leak-arc.mm
+++ test/Analysis/stack-capture-leak-arc.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 
-analyzer-checker=core -fblocks -fobjc-arc -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 
-analyzer-checker=core,alpha.core.StackAddressAsyncEscape -fblocks -fobjc-arc 
-verify %s
 
 typedef struct dispatch_queue_s *dispatch_queue_t;
 typedef void (^dispatch_block_t)(void);
Index: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -37,6 +37,14 @@
   mutable std::unique_ptr BT_capturedstackret;
 
 public:
+  enum CheckKind {
+CK_StackAddrEscapeChecker,
+CK_StackAddrAsyncEscapeChecker,
+CK_NumCheckKinds
+  };
+
+  DefaultBool ChecksEnabled[CK_NumCheckKinds];
+
   void checkPreCall(const CallEvent , CheckerContext ) const;
   void checkPreStmt(const ReturnStmt *RS, CheckerContext ) const;
   void checkEndFunction(CheckerContext ) const;
@@ -225,6 +233,8 @@
 
 void StackAddrEscapeChecker::checkPreCall(const CallEvent ,
   CheckerContext ) const {
+  if (!ChecksEnabled[CK_StackAddrAsyncEscapeChecker])
+return;
   if (!Call.isGlobalCFunction("dispatch_after") &&
   !Call.isGlobalCFunction("dispatch_async"))
 return;
@@ -237,6 +247,8 @@
 
 void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS,
   CheckerContext ) const {
+  if (!ChecksEnabled[CK_StackAddrEscapeChecker])
+return;
 
   const Expr *RetE = RS->getRetValue();
   if (!RetE)
@@ -277,6 +289,9 @@
 }
 
 void StackAddrEscapeChecker::checkEndFunction(CheckerContext ) const {
+  if (!ChecksEnabled[CK_StackAddrEscapeChecker])
+return;
+
   ProgramStateRef State = Ctx.getState();
 
   // Iterate over all bindings to global variables and see if it contains
@@ -346,6 +361,12 @@
   }
 }
 
-void ento::registerStackAddrEscapeChecker(CheckerManager ) {
-  Mgr.registerChecker();
-}
+#define REGISTER_CHECKER(name) \
+  void ento::register##name(CheckerManager ) { \
+StackAddrEscapeChecker *Chk = \
+Mgr.registerChecker(); \
+Chk->ChecksEnabled[StackAddrEscapeChecker::CK_##name] = true; \
+  }
+
+REGISTER_CHECKER(StackAddrEscapeChecker)
+REGISTER_CHECKER(StackAddrAsyncEscapeChecker)
Index: include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -188,6 +188,10 @@
   HelpText<"Check for cases where the dynamic and the static type of an object 
are unrelated.">,
   DescFile<"DynamicTypeChecker.cpp">;
 
+def StackAddrAsyncEscapeChecker : Checker<"StackAddressAsyncEscape">,
+  HelpText<"Check that addresses to stack memory do not escape the function">,
+  DescFile<"StackAddrEscapeChecker.cpp">;
+
 } // end "alpha.core"
 
 let ParentPackage = Nullability in {


Index: test/Analysis/stack-capture-leak-no-arc.mm
===
--- test/Analysis/stack-capture-leak-no-arc.mm
+++ test/Analysis/stack-capture-leak-no-arc.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,alpha.core.StackAddressAsyncEscape -fblocks -verify %s
 
 typedef struct dispatch_queue_s *dispatch_queue_t;
 typedef void (^dispatch_block_t)(void);
Index: test/Analysis/stack-capture-leak-arc.mm
===
--- test/Analysis/stack-capture-leak-arc.mm
+++ test/Analysis/stack-capture-leak-arc.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core 

[PATCH] D41044: Implementation of -fextend-lifetimes and -fextend-this-ptr to aid with debugging of optimized code

[Wolfgang Pieb via Phabricator via cfe-commits]

wolfgangp created this revision.
Herald added a subscriber: JDevlieghere.

The patch implements the clang support for generating artificial uses of local 
variables and parameters to aid with debugging of optimized code. I presented 
this concept in my lightning talk "Debugging of optimized code: Extending the 
lifetime of local variables" at the October 2017 LLVM conference.

Since the concept is not universally supported, the purpose of this review is 
to make the patch available to interested parties and perhaps to restart the 
discussion.

This patch requires the patch posted in https://reviews.llvm.org/D41043 (the 
llvm portion).

The implementation generates calls to the llvm intrinsic fake.use in similar 
fashion as end of lifetime markers.


https://reviews.llvm.org/D41044

Files:
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/CodeGen/CodeGenModule.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/fake-use.cpp

Index: test/CodeGen/fake-use.cpp
===
--- test/CodeGen/fake-use.cpp
+++ test/CodeGen/fake-use.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_cc1 %s -O2 -emit-llvm -fextend-lifetimes -o - | FileCheck %s
+// Check that we generate a fake_use call with the 'this' pointer as argument.
+// The call should appear after the call to bar().
+
+extern void bar();
+
+class v
+{
+public:
+int x;
+int y;
+int z;
+int w;
+
+v(int a, int b, int c, int d) : x(a), y(b), z(c), w(d) {}
+};
+
+class w
+{
+public:
+v test(int, int, int, int, int, int, int, int, int, int);
+w(int in): a(in), b(1234) {}
+
+private:
+int a;
+int b;
+};
+
+v w::test(int q, int w, int e, int r, int t, int y, int u, int i, int o, int p)
+{
+// CHECK: define{{.*}}test
+int res = q*w + e - r*t + y*u*i*o*p;
+int res2 = (w + e + r + t + y + o)*(p*q);
+int res3 = res + res2;
+int res4 = q*e + t*y*i + p*e*w * 6 * 4 * 3;
+
+v V(res, res2, res3, res4);
+
+bar();
+// CHECK: call{{.*}}bar
+// CHECK: call void (...) @llvm.fake.use(%class.w* %this)
+return V;
+// CHECK: ret
+}
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -999,6 +999,11 @@
   Opts.CudaGpuBinaryFileNames =
   Args.getAllArgValues(OPT_fcuda_include_gpubinary);
 
+  Opts.ExtendThisPtr =
+  Opts.OptimizationLevel > 0 && Args.hasArg(OPT_fextend_this_ptr);
+  Opts.ExtendLifetimes =
+  Opts.OptimizationLevel > 0 && Args.hasArg(OPT_fextend_lifetimes);
+
   Opts.Backchain = Args.hasArg(OPT_mbackchain);
 
   Opts.EmitCheckPathComponentsToStrip = getLastArgIntValue(
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -4434,6 +4434,11 @@
   if (Args.hasArg(options::OPT_fretain_comments_from_system_headers))
 CmdArgs.push_back("-fretain-comments-from-system-headers");
 
+  if (Args.hasArg(options::OPT_fextend_this_ptr))
+CmdArgs.push_back("-fextend-this-ptr");
+  if (Args.hasArg(options::OPT_fextend_lifetimes))
+CmdArgs.push_back("-fextend-lifetimes");
+
   // Forward -fcomment-block-commands to -cc1.
   Args.AddAllArgs(CmdArgs, options::OPT_fcomment_block_commands);
   // Forward -fparse-all-comments to -cc1.
Index: lib/CodeGen/CodeGenModule.h
===
--- lib/CodeGen/CodeGenModule.h
+++ lib/CodeGen/CodeGenModule.h
@@ -484,6 +484,9 @@
   /// void @llvm.lifetime.end(i64 %size, i8* nocapture )
   llvm::Constant *LifetimeEndFn = nullptr;
 
+  /// void @llvm.fake.use(i8* nocapture )
+  llvm::Constant *FakeUseFn = nullptr;
+
   GlobalDecl initializedGlobalDecl;
 
   std::unique_ptr SanitizerMD;
@@ -970,6 +973,7 @@
 
   llvm::Constant *getLLVMLifetimeStartFn();
   llvm::Constant *getLLVMLifetimeEndFn();
+  llvm::Constant *getLLVMFakeUseFn();
 
   // Make sure that this type is translated.
   void UpdateCompletedType(const TagDecl *TD);
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -421,6 +421,20 @@
 }
   };
 
+  // We are using objects of this 'cleanup' class to emit fake.use calls
+  // for -fextend-lifetimes. They are placed at the end of a variable's
+  // scope analogous to lifetime markers.
+  class FakeUse final : public EHScopeStack::Cleanup {
+Address Addr;
+
+  public:
+FakeUse(Address addr) : Addr(addr) {}
+
+void Emit(CodeGenFunction , Flags flags) override {
+  CGF.EmitFakeUse(Addr);
+}
+  };
+
   /// Header for data within LifetimeExtendedCleanupStack.
   struct LifetimeExtendedCleanupHeader {
 /// The size of the following 

r320239 - Revert r320230 to fix buildbots.

[Richard Trieu via cfe-commits]

Author: rtrieu
Date: Fri Dec  8 19:02:21 2017
New Revision: 320239

URL: http://llvm.org/viewvc/llvm-project?rev=320239=rev
Log:
Revert r320230 to fix buildbots.

Modified:
cfe/trunk/include/clang/AST/Decl.h
cfe/trunk/include/clang/AST/ODRHash.h
cfe/trunk/include/clang/Basic/DiagnosticSerializationKinds.td
cfe/trunk/include/clang/Serialization/ASTReader.h
cfe/trunk/lib/AST/Decl.cpp
cfe/trunk/lib/AST/ODRHash.cpp
cfe/trunk/lib/Serialization/ASTReader.cpp
cfe/trunk/lib/Serialization/ASTReaderDecl.cpp
cfe/trunk/lib/Serialization/ASTWriterDecl.cpp
cfe/trunk/test/Modules/odr_hash.cpp

Modified: cfe/trunk/include/clang/AST/Decl.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=320239=320238=320239=diff
==
--- cfe/trunk/include/clang/AST/Decl.h (original)
+++ cfe/trunk/include/clang/AST/Decl.h Fri Dec  8 19:02:21 2017
@@ -1760,11 +1760,6 @@ protected:
   unsigned IsCopyDeductionCandidate : 1;
 
 private:
-
-  /// Store the ODRHash after first calculation.
-  unsigned HasODRHash : 1;
-  unsigned ODRHash;
-
   /// \brief End part of this FunctionDecl's source range.
   ///
   /// We could compute the full range in getSourceRange(). However, when we're
@@ -1847,9 +1842,8 @@ protected:
 IsExplicitlyDefaulted(false), HasImplicitReturnZero(false),
 IsLateTemplateParsed(false), IsConstexpr(isConstexprSpecified),
 InstantiationIsPending(false), UsesSEHTry(false), 
HasSkippedBody(false),
-WillHaveBody(false), IsCopyDeductionCandidate(false), 
HasODRHash(false),
-ODRHash(0), EndRangeLoc(NameInfo.getEndLoc()),
-DNLoc(NameInfo.getInfo()) {}
+WillHaveBody(false), IsCopyDeductionCandidate(false),
+EndRangeLoc(NameInfo.getEndLoc()), DNLoc(NameInfo.getInfo()) {}
 
   using redeclarable_base = Redeclarable;
 
@@ -2449,10 +2443,6 @@ public:
   /// returns 0.
   unsigned getMemoryFunctionKind() const;
 
-  /// \brief Returns ODRHash of the function.  This value is calculated and
-  /// stored on first call, then the stored value returned on the other calls.
-  unsigned getODRHash();
-
   // Implement isa/cast/dyncast/etc.
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool classofKind(Kind K) {

Modified: cfe/trunk/include/clang/AST/ODRHash.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ODRHash.h?rev=320239=320238=320239=diff
==
--- cfe/trunk/include/clang/AST/ODRHash.h (original)
+++ cfe/trunk/include/clang/AST/ODRHash.h Fri Dec  8 19:02:21 2017
@@ -53,10 +53,6 @@ public:
   // more information than the AddDecl class.
   void AddCXXRecordDecl(const CXXRecordDecl *Record);
 
-  // Use this for ODR checking functions between modules.  This method compares
-  // more information than the AddDecl class.
-  void AddFunctionDecl(const FunctionDecl *Function);
-
   // Process SubDecls of the main Decl.  This method calls the DeclVisitor
   // while AddDecl does not.
   void AddSubDecl(const Decl *D);

Modified: cfe/trunk/include/clang/Basic/DiagnosticSerializationKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSerializationKinds.td?rev=320239=320238=320239=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticSerializationKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSerializationKinds.td Fri Dec  8 
19:02:21 2017
@@ -270,29 +270,6 @@ def note_module_odr_violation_mismatch_d
   "friend function %2|"
   "}1">;
 
-def err_module_odr_violation_function : Error<
-  "%q0 has different definitions in different modules; "
-  "%select{definition in module '%2'|defined here}1 "
-  "first difference is "
-  "%select{"
-  "return type is %4|"
-  "%ordinal4 parameter with name %5|"
-  "%ordinal4 parameter with type %5%select{| decayed from %7}6|"
-  "%ordinal4 parameter with%select{out|}5 a default argument|"
-  "%ordinal4 parameter with a default argument|"
-  "function body"
-  "}3">;
-
-def note_module_odr_violation_function : Note<"but in '%0' found "
-  "%select{"
-  "different return type %2|"
-  "%ordinal2 parameter with name %3|"
-  "%ordinal2 parameter with type %3%select{| decayed from %5}4|"
-  "%ordinal2 parameter with%select{out|}3 a default argument|"
-  "%ordinal2 parameter with a different default argument|"
-  "a different body"
-  "}1">;
-
 def err_module_odr_violation_mismatch_decl_unknown : Error<
   "%q0 %select{with definition in module '%2'|defined here}1 has different "
   "definitions in different modules; first difference is this "

Modified: cfe/trunk/include/clang/Serialization/ASTReader.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTReader.h?rev=320239=320238=320239=diff

[PATCH] D40995: [TextDiagnosticBuffer] Fix diagnostic note emission order.

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Hi Richard. Thanks for accepting. I don't have commit privileges. Would you 
please commit for me?


https://reviews.llvm.org/D40995



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

[PATCH] D41042: [analyzer] StackAddrEscape: Delay turning on by default a little bit?

[Alexander Shaposhnikov via Phabricator via cfe-commits]

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

i see, to be honest, this is kind of unfortunate, if i am not mistaken, i've 
seen these false-positives, but not too many, most reports were real bugs. But 
if it's annoying, than i think it's fine to make it "alpha" until i figure out 
a good way to workaround this issue.


https://reviews.llvm.org/D41042



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

[PATCH] D41036: IRGen: When performing CFI checks, load vtable pointer from vbase when necessary.

[Peter Collingbourne via Phabricator via cfe-commits]

pcc created this revision.

Under the Microsoft ABI, it is possible for an object not to have
a virtual table pointer of its own if all of its virtual functions
were introduced by virtual bases. In that case, we need to load the
vtable pointer from one of the virtual bases and perform the type
check using its type.


https://reviews.llvm.org/D41036

Files:
  clang/lib/CodeGen/CGCXXABI.h
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGExprCXX.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/test/CodeGenCXX/cfi-ms-vbase-derived-cast.cpp
  clang/test/CodeGenCXX/cfi-ms-vbase-nvcall.cpp

Index: clang/test/CodeGenCXX/cfi-ms-vbase-nvcall.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/cfi-ms-vbase-nvcall.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -flto -flto-unit -emit-llvm -o - -triple=x86_64-pc-win32 %s -fsanitize=cfi-nvcall -fsanitize-trap=cfi-nvcall | FileCheck %s
+
+struct foo {
+  virtual ~foo() {}
+  virtual void f() = 0;
+};
+
+template 
+struct bar : virtual public foo {
+  void f() {}
+};
+
+struct baz : public bar {
+  virtual ~baz() {}
+  void g() {}
+};
+
+void f(baz *z) {
+  // CHECK: define{{.*}}@"\01?f@@YAXPEAUbaz@@@Z"
+  // Load z, vbtable, vbase offset and vtable.
+  // CHECK: load
+  // CHECK: load
+  // CHECK: load
+  // CHECK: load
+  // CHECK: @llvm.type.test{{.*}}!"?AUfoo@@"
+  z->g();
+}
Index: clang/test/CodeGenCXX/cfi-ms-vbase-derived-cast.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/cfi-ms-vbase-derived-cast.cpp
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -flto -flto-unit -emit-llvm -o - -triple=x86_64-pc-win32 %s -fsanitize=cfi-derived-cast -fsanitize-trap=cfi-derived-cast | FileCheck %s
+
+struct foo {
+  virtual ~foo() {}
+  virtual void f() = 0;
+};
+
+template 
+struct bar : virtual public foo {
+  void f() {
+// CHECK: define{{.*}}@"\01?f@?$bar@UbazUEAAXXZ"
+// Load "this", vbtable, vbase offset and vtable.
+// CHECK: load
+// CHECK: load
+// CHECK: load
+// CHECK: load
+// CHECK: @llvm.type.test{{.*}}!"?AUfoo@@"
+static_cast(*this);
+  }
+};
+
+struct baz : public bar {
+  virtual ~baz() {}
+};
+
+int main() {
+  baz *z = new baz;
+  z->f();
+}
Index: clang/lib/CodeGen/MicrosoftCXXABI.cpp
===
--- clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -578,7 +578,7 @@
 return GetVBaseOffsetFromVBPtr(CGF, Base, VBPOffset, VBTOffset, VBPtr);
   }
 
-  std::pair
+  std::tuple
   performBaseAdjustment(CodeGenFunction , Address Value,
 QualType SrcRecordTy);
 
@@ -745,6 +745,10 @@
 
   llvm::GlobalVariable *getThrowInfo(QualType T) override;
 
+  std::pair
+  LoadVTablePtr(CodeGenFunction , Address This,
+const CXXRecordDecl *RD) override;
+
 private:
   typedef std::pair VFTableIdTy;
   typedef llvm::DenseMap VTablesMapTy;
@@ -926,7 +930,7 @@
 /// We need to perform a generic polymorphic operation (like a typeid
 /// or a cast), which requires an object with a vfptr.  Adjust the
 /// address to point to an object with a vfptr.
-std::pair
+std::tuple
 MicrosoftCXXABI::performBaseAdjustment(CodeGenFunction , Address Value,
QualType SrcRecordTy) {
   Value = CGF.Builder.CreateBitCast(Value, CGF.Int8PtrTy);
@@ -937,7 +941,8 @@
   // covers non-virtual base subobjects: a class with its own virtual
   // functions would be a candidate to be a primary base.
   if (Context.getASTRecordLayout(SrcDecl).hasExtendableVFPtr())
-return std::make_pair(Value, llvm::ConstantInt::get(CGF.Int32Ty, 0));
+return std::make_tuple(Value, llvm::ConstantInt::get(CGF.Int32Ty, 0),
+   SrcDecl);
 
   // Okay, one of the vbases must have a vfptr, or else this isn't
   // actually a polymorphic class.
@@ -956,7 +961,7 @@
   llvm::Value *Ptr = CGF.Builder.CreateInBoundsGEP(Value.getPointer(), Offset);
   CharUnits VBaseAlign =
 CGF.CGM.getVBaseAlignment(Value.getAlignment(), SrcDecl, PolymorphicBase);
-  return std::make_pair(Address(Ptr, VBaseAlign), Offset);
+  return std::make_tuple(Address(Ptr, VBaseAlign), Offset, PolymorphicBase);
 }
 
 bool MicrosoftCXXABI::shouldTypeidBeNullChecked(bool IsDeref,
@@ -987,7 +992,7 @@
  QualType SrcRecordTy,
  Address ThisPtr,
  llvm::Type *StdTypeInfoPtrTy) {
-  std::tie(ThisPtr, std::ignore) =
+  std::tie(ThisPtr, std::ignore, std::ignore) =
   performBaseAdjustment(CGF, ThisPtr, SrcRecordTy);
   auto Typeid = emitRTtypeidCall(CGF, 

[PATCH] D40968: [OpenMP] Diagnose function name on the link clause

[Kelvin Li via Phabricator via cfe-commits]

kkwli0 updated this revision to Diff 126217.
kkwli0 marked an inline comment as done.

https://reviews.llvm.org/D40968

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Sema/SemaOpenMP.cpp
  test/OpenMP/declare_target_ast_print.cpp
  test/OpenMP/declare_target_messages.cpp

Index: test/OpenMP/declare_target_messages.cpp
===
--- test/OpenMP/declare_target_messages.cpp
+++ test/OpenMP/declare_target_messages.cpp
@@ -19,6 +19,10 @@
 
 void c(); // expected-warning {{declaration is not declared in any declare target region}}
 
+void func() {} // expected-note {{'func' defined here}}
+
+#pragma omp declare target link(func) // expected-error {{function name is not allowed in 'link' clause}}
+
 extern int b;
 
 struct NonT {
Index: test/OpenMP/declare_target_ast_print.cpp
===
--- test/OpenMP/declare_target_ast_print.cpp
+++ test/OpenMP/declare_target_ast_print.cpp
@@ -108,9 +108,7 @@
 // CHECK: #pragma omp end declare target
 
 int c1, c2, c3;
-void f3() {
-}
-#pragma omp declare target link(c1) link(c2), link(c3, f3)
+#pragma omp declare target link(c1) link(c2), link(c3)
 // CHECK: #pragma omp declare target link
 // CHECK: int c1;
 // CHECK: #pragma omp end declare target
@@ -120,9 +118,6 @@
 // CHECK: #pragma omp declare target link
 // CHECK: int c3;
 // CHECK: #pragma omp end declare target
-// CHECK: #pragma omp declare target link
-// CHECK: void f3()
-// CHECK: #pragma omp end declare target
 
 struct SSSt {
 #pragma omp declare target
Index: lib/Sema/SemaOpenMP.cpp
===
--- lib/Sema/SemaOpenMP.cpp
+++ lib/Sema/SemaOpenMP.cpp
@@ -12600,7 +12600,7 @@
   ND->addAttr(A);
   if (ASTMutationListener *ML = Context.getASTMutationListener())
 ML->DeclarationMarkedOpenMPDeclareTarget(ND, A);
-  checkDeclIsAllowedInOpenMPTarget(nullptr, ND);
+  checkDeclIsAllowedInOpenMPTarget(nullptr, ND, Id.getLoc());
 } else if (ND->getAttr()->getMapType() != MT) {
   Diag(Id.getLoc(), diag::err_omp_declare_target_to_and_link)
   << Id.getName();
@@ -12689,7 +12689,8 @@
   return true;
 }
 
-void Sema::checkDeclIsAllowedInOpenMPTarget(Expr *E, Decl *D) {
+void Sema::checkDeclIsAllowedInOpenMPTarget(Expr *E, Decl *D,
+SourceLocation IdLoc) {
   if (!D || D->isInvalidDecl())
 return;
   SourceRange SR = E ? E->getSourceRange() : D->getSourceRange();
@@ -12718,6 +12719,16 @@
   return;
 }
   }
+  if (FunctionDecl *FD = dyn_cast(D)) {
+if (FD->hasAttr() &&
+(FD->getAttr()->getMapType() ==
+ OMPDeclareTargetDeclAttr::MT_Link)) {
+  assert(IdLoc.isValid() && "Source location is expected");
+  Diag(IdLoc, diag::err_omp_function_in_link_clause);
+  Diag(FD->getLocation(), diag::note_defined_here) << FD;
+  return;
+}
+  }
   if (!E) {
 // Checking declaration inside declare target region.
 if (!D->hasAttr() &&
Index: include/clang/Sema/Sema.h
===
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -8656,7 +8656,8 @@
 OMPDeclareTargetDeclAttr::MapTypeTy MT,
 NamedDeclSetType );
   /// Check declaration inside target region.
-  void checkDeclIsAllowedInOpenMPTarget(Expr *E, Decl *D);
+  void checkDeclIsAllowedInOpenMPTarget(Expr *E, Decl *D,
+SourceLocation IdLoc = SourceLocation());
   /// Return true inside OpenMP declare target region.
   bool isInOpenMPDeclareTargetContext() const {
 return IsInOpenMPDeclareTargetContext;
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -8709,6 +8709,8 @@
 def warn_omp_not_in_target_context : Warning<
   "declaration is not declared in any declare target region">,
   InGroup;
+def err_omp_function_in_link_clause : Error<
+  "function name is not allowed in 'link' clause">;
 def err_omp_aligned_expected_array_or_ptr : Error<
   "argument of aligned clause should be array"
   "%select{ or pointer|, pointer, reference to array or reference to pointer}1"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r320212 - Remove creation of out-of-bounds value of enumeration type (resulting in UB).

[Richard Smith via cfe-commits]

Author: rsmith
Date: Fri Dec  8 15:29:59 2017
New Revision: 320212

URL: http://llvm.org/viewvc/llvm-project?rev=320212=rev
Log:
Remove creation of out-of-bounds value of enumeration type (resulting in UB).

Also remove unnecessary initialization of out-parameters with this value, so
that MSan is able to catch errors appropriately.

Modified:
cfe/trunk/include/clang/AST/Expr.h
cfe/trunk/include/clang/AST/OperationKinds.h
cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h
cfe/trunk/lib/Sema/Sema.cpp
cfe/trunk/lib/Sema/SemaExpr.cpp
cfe/trunk/lib/Sema/SemaExprCXX.cpp

Modified: cfe/trunk/include/clang/AST/Expr.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Expr.h?rev=320212=320211=320212=diff
==
--- cfe/trunk/include/clang/AST/Expr.h (original)
+++ cfe/trunk/include/clang/AST/Expr.h Fri Dec  8 15:29:59 2017
@@ -2742,7 +2742,6 @@ protected:
ty->containsUnexpandedParameterPack()) ||
   (op && op->containsUnexpandedParameterPack(,
 Op(op) {
-assert(kind != CK_Invalid && "creating cast with invalid cast kind");
 CastExprBits.Kind = kind;
 setBasePathSize(BasePathSize);
 assert(CastConsistency());

Modified: cfe/trunk/include/clang/AST/OperationKinds.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/OperationKinds.h?rev=320212=320211=320212=diff
==
--- cfe/trunk/include/clang/AST/OperationKinds.h (original)
+++ cfe/trunk/include/clang/AST/OperationKinds.h Fri Dec  8 15:29:59 2017
@@ -23,8 +23,6 @@ enum CastKind {
 #include "clang/AST/OperationKinds.def"
 };
 
-static const CastKind CK_Invalid = static_cast(-1);
-
 enum BinaryOperatorKind {
 #define BINARY_OPERATION(Name, Spelling) BO_##Name,
 #include "clang/AST/OperationKinds.def"

Modified: cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h?rev=320212=320211=320212=diff
==
--- cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h (original)
+++ cfe/trunk/lib/ASTMatchers/Dynamic/Marshallers.h Fri Dec  8 15:29:59 2017
@@ -122,21 +122,20 @@ template <> struct ArgTypeTraits struct ArgTypeTraits {
 private:
-  static attr::Kind getAttrKind(llvm::StringRef AttrKind) {
-return llvm::StringSwitch(AttrKind)
+  static Optional getAttrKind(llvm::StringRef AttrKind) {
+return llvm::StringSwitch(AttrKind)
 #define ATTR(X) .Case("attr::" #X, attr:: X)
 #include "clang/Basic/AttrList.inc"
-.Default(attr::Kind(-1));
+.Default(llvm::None);
   }
 
 public:
   static bool is(const VariantValue ) {
-return Value.isString() &&
-getAttrKind(Value.getString()) != attr::Kind(-1);
+return Value.isString() && getAttrKind(Value.getString());
   }
 
   static attr::Kind get(const VariantValue ) {
-return getAttrKind(Value.getString());
+return *getAttrKind(Value.getString());
   }
 
   static ArgKind getKind() {
@@ -146,21 +145,20 @@ public:
 
 template <> struct ArgTypeTraits {
 private:
-  static CastKind getCastKind(llvm::StringRef AttrKind) {
-return llvm::StringSwitch(AttrKind)
+  static Optional getCastKind(llvm::StringRef AttrKind) {
+return llvm::StringSwitch(AttrKind)
 #define CAST_OPERATION(Name) .Case( #Name, CK_##Name)
 #include "clang/AST/OperationKinds.def"
-.Default(CK_Invalid);
+.Default(llvm::None);
   }
 
 public:
   static bool is(const VariantValue ) {
-return Value.isString() &&  
-getCastKind(Value.getString()) != CK_Invalid;
+return Value.isString() && getCastKind(Value.getString());
   }
 
   static CastKind get(const VariantValue ) {
-return getCastKind(Value.getString());
+return *getCastKind(Value.getString());
   }
 
   static ArgKind getKind() {

Modified: cfe/trunk/lib/Sema/Sema.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/Sema.cpp?rev=320212=320211=320212=diff
==
--- cfe/trunk/lib/Sema/Sema.cpp (original)
+++ cfe/trunk/lib/Sema/Sema.cpp Fri Dec  8 15:29:59 2017
@@ -529,7 +529,7 @@ CastKind Sema::ScalarTypeToBooleanCastKi
   case Type::STK_IntegralComplex: return CK_IntegralComplexToBoolean;
   case Type::STK_FloatingComplex: return CK_FloatingComplexToBoolean;
   }
-  return CK_Invalid;
+  llvm_unreachable("unknown scalar type kind");
 }
 
 /// \brief Used to prune the decls of Sema's UnusedFileScopedDecls vector.

Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=320212=320211=320212=diff
==
--- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
+++ cfe/trunk/lib/Sema/SemaExpr.cpp Fri Dec  

[PATCH] D40897: [clangd] Introduce a "Symbol" class.

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle added inline comments.



Comment at: clangd/Symbol.h:37
+// The class presents a C++ symbol, e.g. class, function.
+struct Symbol {
+  // The symbol identifier, using USR.

malaperle wrote:
> sammccall wrote:
> > hokein wrote:
> > > malaperle wrote:
> > > > sammccall wrote:
> > > > > hokein wrote:
> > > > > > malaperle wrote:
> > > > > > > I think it would be nice to have methods as an interface to get 
> > > > > > > this data instead of storing them directly. So that an 
> > > > > > > index-on-disk could go fetch the data. Especially the occurrences 
> > > > > > > which can take a lot of memory (I'm working on a branch that does 
> > > > > > > that). But perhaps defining that interface is not within the 
> > > > > > > scope of this patch and could be better discussed in D40548 ?
> > > > > > I agree. We can't load all the symbol occurrences into the memory 
> > > > > > since they are too large. We need to design interface for the 
> > > > > > symbol occurrences. 
> > > > > > 
> > > > > > We could discuss the interface here, but CodeCompletion is the main 
> > > > > > thing which this patch focuses on. 
> > > > > > We can't load all the symbol occurrences into the memory since they 
> > > > > > are too large
> > > > > 
> > > > > I've heard this often, but never backed up by data :-)
> > > > > 
> > > > > Naively an array of references for a symbol could be doc ID + offset 
> > > > > + length, let's say 16 bytes.
> > > > > 
> > > > > If a source file consisted entirely of references to 1-character 
> > > > > symbols separated by punctuation (1 reference per 2 bytes) then the 
> > > > > total size of these references would be 8x the size of the source 
> > > > > file - in practice much less. That's not very big.
> > > > > 
> > > > > (Maybe there are edge cases with macros/templates, but we can keep 
> > > > > them under control)
> > > > I'd have to break down how much memory it used by what, I'll come back 
> > > > to you on that. Indexing llvm with ClangdIndexDataStorage, which is 
> > > > pretty packed is about 200MB. That's already a lot considering we want 
> > > > to index code bases many times bigger. But I'll try to come up with 
> > > > more precise numbers. I'm open to different strategies.
> > > > 
> > > I can see two points here:
> > > 
> > > 1. For all symbol occurrences of a TU, it is not quite large, and we can 
> > > keep them in memory.
> > > 2. For all symbol occurrences of a whole project, it's not a good idea to 
> > > load all of them into memory (For LLVM project, the size of YAML dataset 
> > > is ~1.2G).  
> > (This is still a sidebar - not asking for any changes)
> > 
> > The YAML dataset is not a good proxy for how big the data is (at least 
> > without an effort to estimate constant factor).
> > And "it's not a good idea" isn't an assertion that can hold without 
> > reasons, assumptions, and data.
> > If the size turns out to be, say, 120MB for LLVM, and we want to scale to 
> > 10x that, and we're spending 500MB/file for ASTs, then it might well be a 
> > good trade.
> > The YAML dataset is not a good proxy for how big the data is (at least 
> > without an effort to estimate constant factor).
> 
> Indeed. I'll try to come up with more realistic numbers. There are other 
> things not accounted for in the 16 bytes mentioned above, like storing roles 
> and relations.
> 
> > 500MB/file for ASTs
> 
> What do you mean? 500MB worth of occurrences per file? Or Preambles perhaps?
> What do you mean? 500MB worth of occurrences per file? Or Preambles perhaps?

Oh I see, the AST must be in memory for fast reparse. I just tried opening 3 
files at the same time I it was already around 500MB. Hmm, that's a bit 
alarming.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40897



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

[PATCH] D40998: [driver][darwin] Take the OS version specified in "-target" as the target OS instead of inferring it from SDK / environment

[Alex Lorenz via Phabricator via cfe-commits]

arphaman updated this revision to Diff 126243.
arphaman added a comment.

I rewrote the patch on top of https://reviews.llvm.org/D41035 as suggested by 
Duncan.

I also changed some of the semantics:

- If `-target` is used with `Darwin` OS, then the OS will be determined using 
the old semantics.
- If `-target` specifies a concrete OS (even without version), that OS is 
**always** used, no matter what other options/environment variables are given. 
The driver will warn about superfluous `-m-version-min` and environment 
variables in this case.


Repository:
  rC Clang

https://reviews.llvm.org/D40998

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  lib/Driver/ToolChains/Darwin.cpp
  test/Driver/darwin-version.c
  test/Driver/objc-weak.m
  test/Driver/pic.c
  test/Driver/unavailable_aligned_allocation.cpp

Index: test/Driver/unavailable_aligned_allocation.cpp
===
--- test/Driver/unavailable_aligned_allocation.cpp
+++ test/Driver/unavailable_aligned_allocation.cpp
@@ -10,15 +10,15 @@
 // RUN: %clang -target thumbv7-apple-watchos3 -c -### %s 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=UNAVAILABLE
 //
-// RUN: %clang -target x86_64-apple-macosx10.13 -mios-simulator-version-min=10 \
+// RUN: %clang -target x86_64-apple-darwin -mios-simulator-version-min=10 \
 // RUN:  -c -### %s 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=UNAVAILABLE
 //
-// RUN: %clang -target x86_64-apple-macosx10.13 -mtvos-simulator-version-min=10 \
+// RUN: %clang -target x86_64-apple-darwin -mtvos-simulator-version-min=10 \
 // RUN: -c -### %s 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=UNAVAILABLE
 //
-// RUN: %clang -target x86_64-apple-macosx10.13 -mwatchos-simulator-version-min=3 \
+// RUN: %clang -target x86_64-apple-darwin -mwatchos-simulator-version-min=3 \
 // RUN: -c -### %s 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=UNAVAILABLE
 //
@@ -39,15 +39,15 @@
 // RUN: %clang -target x86_64-unknown-linux-gnu -c -### %s 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=AVAILABLE
 //
-// RUN: %clang -target x86_64-apple-macosx10.12 -mios-simulator-version-min=11 \
+// RUN: %clang -target x86_64-apple-darwin -mios-simulator-version-min=11 \
 // RUN:  -c -### %s 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=AVAILABLE
 //
-// RUN: %clang -target x86_64-apple-macosx10.12 -mtvos-simulator-version-min=11 \
+// RUN: %clang -target x86_64-apple-darwin -mtvos-simulator-version-min=11 \
 // RUN: -c -### %s 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=AVAILABLE
 //
-// RUN: %clang -target x86_64-apple-macosx10.12 -mwatchos-simulator-version-min=4 \
+// RUN: %clang -target x86_64-apple-darwin -mwatchos-simulator-version-min=4 \
 // RUN: -c -### %s 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=AVAILABLE
 //
Index: test/Driver/pic.c
===
--- test/Driver/pic.c
+++ test/Driver/pic.c
@@ -221,19 +221,19 @@
 //
 // Checks for ARM+Apple+IOS including -fapple-kext, -mkernel, and iphoneos
 // version boundaries.
-// RUN: %clang -c %s -target armv7-apple-ios -fapple-kext -miphoneos-version-min=6.0.0 -### 2>&1 \
+// RUN: %clang -c %s -target armv7-apple-ios6 -fapple-kext -### 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-PIC2
-// RUN: %clang -c %s -target armv7-apple-ios -mkernel -miphoneos-version-min=6.0.0 -### 2>&1 \
+// RUN: %clang -c %s -target armv7-apple-ios6 -mkernel -### 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-PIC2
-// RUN: %clang -c %s -target arm64-apple-ios -mkernel -miphoneos-version-min=7.0.0 -### 2>&1 \
+// RUN: %clang -c %s -target arm64-apple-ios7 -mkernel -### 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-PIC2
-// RUN: %clang -x assembler -c %s -target arm64-apple-ios -mkernel -miphoneos-version-min=7.0.0 -no-integrated-as -### 2>&1 \
+// RUN: %clang -x assembler -c %s -target arm64-apple-ios7 -mkernel -no-integrated-as -### 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-NO-STATIC
-// RUN: %clang -c %s -target armv7k-apple-watchos -fapple-kext -mwatchos-version-min=1.0.0 -### 2>&1 \
+// RUN: %clang -c %s -target armv7k-apple-watchos1 -fapple-kext -### 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-PIC2
-// RUN: %clang -c %s -target armv7-apple-ios -fapple-kext -miphoneos-version-min=5.0.0 -### 2>&1 \
+// RUN: %clang -c %s -target armv7-apple-ios5 -fapple-kext -### 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-NO-PIC
-// RUN: %clang -c %s -target armv7-apple-ios -fapple-kext -miphoneos-version-min=6.0.0 -static -### 2>&1 \
+// RUN: %clang -c %s -target armv7-apple-ios6 -fapple-kext -static -### 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-NO-PIC
 // RUN: %clang -c %s -target armv7-apple-unknown-macho -static -### 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-NO-PIC
Index: test/Driver/objc-weak.m
===
--- test/Driver/objc-weak.m
+++ test/Driver/objc-weak.m
@@ -1,27 +1,27 

[PATCH] D41035: [driver][darwin] Refactor the target selection code, NFC

[Alex Lorenz via Phabricator via cfe-commits]

arphaman created this revision.

This patch refactors the target selection in Darwin's driver.
Firstly, the simulator variant of Darwin's platforms is removed in favor of a 
new environment field.
Secondly, the code that selects the platform and the version is split into 4 
different functions instead of being all in one function.
This is an NFC patch, although it slightly improves the "invalid version 
number" diagnostic by displaying the environment variable instead of 
-m-version-min if the OS version was derived from the environment.

This patch is a preparation for making `-target` the canonical way of 
specifying the platform. This will be done in a follow-up patch 
https://reviews.llvm.org/D40998.


Repository:
  rC Clang

https://reviews.llvm.org/D41035

Files:
  lib/Driver/ToolChains/Darwin.cpp
  lib/Driver/ToolChains/Darwin.h
  test/Driver/darwin-version.c

Index: test/Driver/darwin-version.c
===
--- test/Driver/darwin-version.c
+++ test/Driver/darwin-version.c
@@ -139,3 +139,8 @@
 // RUN: %clang -target x86_64-apple-watchos4.0 -c %s -### 2>&1 | \
 // RUN: FileCheck --check-prefix=CHECK-VERSION-WATCHOS-TARGET %s
 // CHECK-VERSION-WATCHOS-TARGET: "x86_64-apple-watchos4.0.0-simulator"
+
+// RUN: env MACOSX_DEPLOYMENT_TARGET=1000.1000 \
+// RUN:   %clang -c %s -### 2>&1 | \
+// RUN:   FileCheck --check-prefix=CHECK-VERSION-INVALID-ENV %s
+// CHECK-VERSION-INVALID-ENV: invalid version number in 'MACOSX_DEPLOYMENT_TARGET=1000.1000'
Index: lib/Driver/ToolChains/Darwin.h
===
--- lib/Driver/ToolChains/Darwin.h
+++ lib/Driver/ToolChains/Darwin.h
@@ -268,14 +268,17 @@
   enum DarwinPlatformKind {
 MacOS,
 IPhoneOS,
-IPhoneOSSimulator,
 TvOS,
-TvOSSimulator,
 WatchOS,
-WatchOSSimulator
+LastDarwinPlatform = WatchOS
+  };
+  enum DarwinEnvironmentKind {
+NativeEnvironment,
+Simulator,
   };
 
   mutable DarwinPlatformKind TargetPlatform;
+  mutable DarwinEnvironmentKind TargetEnvironment;
 
   /// The OS version we are targeting.
   mutable VersionTuple TargetVersion;
@@ -317,32 +320,34 @@
 
   // FIXME: Eliminate these ...Target functions and derive separate tool chains
   // for these targets and put version in constructor.
-  void setTarget(DarwinPlatformKind Platform, unsigned Major, unsigned Minor,
- unsigned Micro) const {
+  void setTarget(DarwinPlatformKind Platform, DarwinEnvironmentKind Environment,
+ unsigned Major, unsigned Minor, unsigned Micro) const {
 // FIXME: For now, allow reinitialization as long as values don't
 // change. This will go away when we move away from argument translation.
 if (TargetInitialized && TargetPlatform == Platform &&
+TargetEnvironment == Environment &&
 TargetVersion == VersionTuple(Major, Minor, Micro))
   return;
 
 assert(!TargetInitialized && "Target already initialized!");
 TargetInitialized = true;
 TargetPlatform = Platform;
+TargetEnvironment = Environment;
 TargetVersion = VersionTuple(Major, Minor, Micro);
-if (Platform == IPhoneOSSimulator || Platform == TvOSSimulator ||
-Platform == WatchOSSimulator)
+if (Environment == Simulator)
   const_cast(this)->setTripleEnvironment(llvm::Triple::Simulator);
   }
 
   bool isTargetIPhoneOS() const {
 assert(TargetInitialized && "Target not initialized!");
-return TargetPlatform == IPhoneOS || TargetPlatform == TvOS;
+return (TargetPlatform == IPhoneOS || TargetPlatform == TvOS) &&
+   TargetEnvironment == NativeEnvironment;
   }
 
   bool isTargetIOSSimulator() const {
 assert(TargetInitialized && "Target not initialized!");
-return TargetPlatform == IPhoneOSSimulator ||
-   TargetPlatform == TvOSSimulator;
+return (TargetPlatform == IPhoneOS || TargetPlatform == TvOS) &&
+   TargetEnvironment == Simulator;
   }
 
   bool isTargetIOSBased() const {
@@ -352,32 +357,32 @@
 
   bool isTargetTvOS() const {
 assert(TargetInitialized && "Target not initialized!");
-return TargetPlatform == TvOS;
+return TargetPlatform == TvOS && TargetEnvironment == NativeEnvironment;
   }
 
   bool isTargetTvOSSimulator() const {
 assert(TargetInitialized && "Target not initialized!");
-return TargetPlatform == TvOSSimulator;
+return TargetPlatform == TvOS && TargetEnvironment == Simulator;
   }
 
   bool isTargetTvOSBased() const {
 assert(TargetInitialized && "Target not initialized!");
-return TargetPlatform == TvOS || TargetPlatform == TvOSSimulator;
+return TargetPlatform == TvOS;
   }
 
   bool isTargetWatchOS() const {
 assert(TargetInitialized && "Target not initialized!");
-return TargetPlatform == WatchOS;
+return TargetPlatform == WatchOS && TargetEnvironment == NativeEnvironment;
   }
 
   bool isTargetWatchOSSimulator() const {
 assert(TargetInitialized && 

[PATCH] D41035: [driver][darwin] Refactor the target selection code, NFC

[Alex Lorenz via Phabricator via cfe-commits]

arphaman updated this revision to Diff 126222.
arphaman marked an inline comment as done.
arphaman added a comment.

Add static_assert


Repository:
  rC Clang

https://reviews.llvm.org/D41035

Files:
  lib/Driver/ToolChains/Darwin.cpp
  lib/Driver/ToolChains/Darwin.h
  test/Driver/darwin-version.c

Index: test/Driver/darwin-version.c
===
--- test/Driver/darwin-version.c
+++ test/Driver/darwin-version.c
@@ -139,3 +139,8 @@
 // RUN: %clang -target x86_64-apple-watchos4.0 -c %s -### 2>&1 | \
 // RUN: FileCheck --check-prefix=CHECK-VERSION-WATCHOS-TARGET %s
 // CHECK-VERSION-WATCHOS-TARGET: "x86_64-apple-watchos4.0.0-simulator"
+
+// RUN: env MACOSX_DEPLOYMENT_TARGET=1000.1000 \
+// RUN:   %clang -c %s -### 2>&1 | \
+// RUN:   FileCheck --check-prefix=CHECK-VERSION-INVALID-ENV %s
+// CHECK-VERSION-INVALID-ENV: invalid version number in 'MACOSX_DEPLOYMENT_TARGET=1000.1000'
Index: lib/Driver/ToolChains/Darwin.h
===
--- lib/Driver/ToolChains/Darwin.h
+++ lib/Driver/ToolChains/Darwin.h
@@ -268,14 +268,17 @@
   enum DarwinPlatformKind {
 MacOS,
 IPhoneOS,
-IPhoneOSSimulator,
 TvOS,
-TvOSSimulator,
 WatchOS,
-WatchOSSimulator
+LastDarwinPlatform = WatchOS
+  };
+  enum DarwinEnvironmentKind {
+NativeEnvironment,
+Simulator,
   };
 
   mutable DarwinPlatformKind TargetPlatform;
+  mutable DarwinEnvironmentKind TargetEnvironment;
 
   /// The OS version we are targeting.
   mutable VersionTuple TargetVersion;
@@ -317,32 +320,34 @@
 
   // FIXME: Eliminate these ...Target functions and derive separate tool chains
   // for these targets and put version in constructor.
-  void setTarget(DarwinPlatformKind Platform, unsigned Major, unsigned Minor,
- unsigned Micro) const {
+  void setTarget(DarwinPlatformKind Platform, DarwinEnvironmentKind Environment,
+ unsigned Major, unsigned Minor, unsigned Micro) const {
 // FIXME: For now, allow reinitialization as long as values don't
 // change. This will go away when we move away from argument translation.
 if (TargetInitialized && TargetPlatform == Platform &&
+TargetEnvironment == Environment &&
 TargetVersion == VersionTuple(Major, Minor, Micro))
   return;
 
 assert(!TargetInitialized && "Target already initialized!");
 TargetInitialized = true;
 TargetPlatform = Platform;
+TargetEnvironment = Environment;
 TargetVersion = VersionTuple(Major, Minor, Micro);
-if (Platform == IPhoneOSSimulator || Platform == TvOSSimulator ||
-Platform == WatchOSSimulator)
+if (Environment == Simulator)
   const_cast(this)->setTripleEnvironment(llvm::Triple::Simulator);
   }
 
   bool isTargetIPhoneOS() const {
 assert(TargetInitialized && "Target not initialized!");
-return TargetPlatform == IPhoneOS || TargetPlatform == TvOS;
+return (TargetPlatform == IPhoneOS || TargetPlatform == TvOS) &&
+   TargetEnvironment == NativeEnvironment;
   }
 
   bool isTargetIOSSimulator() const {
 assert(TargetInitialized && "Target not initialized!");
-return TargetPlatform == IPhoneOSSimulator ||
-   TargetPlatform == TvOSSimulator;
+return (TargetPlatform == IPhoneOS || TargetPlatform == TvOS) &&
+   TargetEnvironment == Simulator;
   }
 
   bool isTargetIOSBased() const {
@@ -352,32 +357,32 @@
 
   bool isTargetTvOS() const {
 assert(TargetInitialized && "Target not initialized!");
-return TargetPlatform == TvOS;
+return TargetPlatform == TvOS && TargetEnvironment == NativeEnvironment;
   }
 
   bool isTargetTvOSSimulator() const {
 assert(TargetInitialized && "Target not initialized!");
-return TargetPlatform == TvOSSimulator;
+return TargetPlatform == TvOS && TargetEnvironment == Simulator;
   }
 
   bool isTargetTvOSBased() const {
 assert(TargetInitialized && "Target not initialized!");
-return TargetPlatform == TvOS || TargetPlatform == TvOSSimulator;
+return TargetPlatform == TvOS;
   }
 
   bool isTargetWatchOS() const {
 assert(TargetInitialized && "Target not initialized!");
-return TargetPlatform == WatchOS;
+return TargetPlatform == WatchOS && TargetEnvironment == NativeEnvironment;
   }
 
   bool isTargetWatchOSSimulator() const {
 assert(TargetInitialized && "Target not initialized!");
-return TargetPlatform == WatchOSSimulator;
+return TargetPlatform == WatchOS && TargetEnvironment == Simulator;
   }
 
   bool isTargetWatchOSBased() const {
 assert(TargetInitialized && "Target not initialized!");
-return TargetPlatform == WatchOS || TargetPlatform == WatchOSSimulator;
+return TargetPlatform == WatchOS;
   }
 
   bool isTargetMacOS() const {
@@ -413,10 +418,11 @@
  Action::OffloadKind DeviceOffloadKind) const override;
 
   StringRef getPlatformFamily() const;
-  static 

r320233 - Fix fsanitize-blacklist test on Windows.

[Evgeniy Stepanov via cfe-commits]

Author: eugenis
Date: Fri Dec  8 18:15:42 2017
New Revision: 320233

URL: http://llvm.org/viewvc/llvm-project?rev=320233=rev
Log:
Fix fsanitize-blacklist test on Windows.

Broken in r320232.

Modified:
cfe/trunk/test/Driver/fsanitize-blacklist.c

Modified: cfe/trunk/test/Driver/fsanitize-blacklist.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/fsanitize-blacklist.c?rev=320233=320232=320233=diff
==
--- cfe/trunk/test/Driver/fsanitize-blacklist.c (original)
+++ cfe/trunk/test/Driver/fsanitize-blacklist.c Fri Dec  8 18:15:42 2017
@@ -22,9 +22,9 @@
 
 // Check that the default blacklist is not added as an extra dependency.
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=address 
-resource-dir=%S/Inputs/resource_dir %s -### 2>&1 | FileCheck %s 
--check-prefix=CHECK-DEFAULT-BLACKLIST-ASAN --implicit-check-not=fdepfile-entry
-// CHECK-DEFAULT-BLACKLIST-ASAN: -fsanitize-blacklist={{.*}}/asan_blacklist.txt
+// CHECK-DEFAULT-BLACKLIST-ASAN: 
-fsanitize-blacklist={{.*[^w]}}asan_blacklist.txt
 // RUN: %clang -target aarch64-linux-gnu -fsanitize=hwaddress 
-resource-dir=%S/Inputs/resource_dir %s -### 2>&1 | FileCheck %s 
--check-prefix=CHECK-DEFAULT-BLACKLIST-HWASAN 
--implicit-check-not=fdepfile-entry
-// CHECK-DEFAULT-BLACKLIST-HWASAN: 
-fsanitize-blacklist={{.*}}/hwasan_blacklist.txt
+// CHECK-DEFAULT-BLACKLIST-HWASAN: 
-fsanitize-blacklist={{.*}}hwasan_blacklist.txt
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=integer 
-resource-dir=%S/Inputs/resource_dir %s -### 2>&1 | FileCheck %s 
--check-prefix=CHECK-DEFAULT-UBSAN-BLACKLIST --implicit-check-not=fdepfile-entry
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=nullability 
-resource-dir=%S/Inputs/resource_dir %s -### 2>&1 | FileCheck %s 
--check-prefix=CHECK-DEFAULT-UBSAN-BLACKLIST --implicit-check-not=fdepfile-entry


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