[PATCH] D53329: Generate DIFile with main program if source is not available

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

F7545597: clang-crash.tar 

@dblaikie I got a standalone reproducible test now.

  -bash-4.4$ pwd
  /home/yhs/work/bcc/clang-crash
  -bash-4.4$ ls
  clang-test.cpp  compile.sh  README  ttest.c  ttest.h
  -bash-4.4$

README has details how to reproduce the issue. Basically, step 1, build a llvm 
with x86/bpf support; step 2, run "compile.sh"; step 3, run the binary 
"clang-test" produced in step 2 and you will get IR with missing file content 
and segfault.

I did not investigate further, but my above patch does not really fix the 
issue. So the right fix should be a different one. There could be multiple 
issues with clang remapped files.

It will be great if you or others can help resolve these issues!

2. $ ./compile.sh
3. $ ./clang-test
  
  The test will print IR before JITing for BPF backend.
  The IR has something like:
 ...
  !8 = !DIFile(filename: "/virtual/ttest.h", directory: "/tmp", checksumkind: 
CSK_MD5, checksum: "6a615c0cc48c46cdbe7d9917c635d292", source: "\0A#define 
BPF_PERF_OUTPUT2(_name) \5C\0Astruct _name##_table_t { \5C\0A  int key; \5C\0A  
unsigned leaf; \5C\0A  int (*perf_submit) (void *, void *, unsigned); \5C\0A  
int (*perf_submit_skb) (void *, unsigned, void *, unsigned); \5C\0A  unsigned 
max_entries; \5C\0A}; \5C\0A__attribute__((section(\22maps/perf_output\22))) 
\5C\0Astruct _name##_table_t _name = { .max_entries = 0 }\0A\0Aint g;\0A")
  !9 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
  !10 = !DIFile(filename: "/virtual/ttest.c", directory: "/tmp")
  !11 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: 
"probe_table_t", file: !10, line: 3, size: 256, elements: !12)
 ...
  Segmentation fault (core dumped)

Note that the "!10" DIFile does not have source code.


Repository:
  rC Clang

https://reviews.llvm.org/D53329



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

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: docs/LanguageExtensions.rst:1096
+  equivalent to copying the underlying bytes and then dropping the source 
object
+  on the floor.
 * ``__is_destructible`` (MSVC 2013)

Quuxplusone wrote:
> rjmccall wrote:
> > Quuxplusone wrote:
> > > rjmccall wrote:
> > > > Quuxplusone wrote:
> > > > > rjmccall wrote:
> > > > > > Quuxplusone wrote:
> > > > > > > @rjmccall wrote:
> > > > > > > > trivial_abi permits annotated types to be passed and returned 
> > > > > > > > in registers, which is ABI-breaking. Skimming the blog post, it 
> > > > > > > > looks like trivially_relocatable does not permit this — it 
> > > > > > > > merely signifies that destruction is a no-op after a move 
> > > > > > > > construction or assignment.
> > > > > > > 
> > > > > > > Not necessarily a "no-op"; my canonical example is a 
> > > > > > > CopyOnlyCXX03SharedPtr which increments a refcount on 
> > > > > > > construction and decrements on destruction. But move-construction 
> > > > > > > plus destruction should "balance out" and result in no observable 
> > > > > > > side effects.
> > > > > > > 
> > > > > > > > This is usefully different in the design space, since it means 
> > > > > > > > you can safely add the attribute retroactively to e.g. 
> > > > > > > > std::unique_ptr, and other templates can then detect that 
> > > > > > > > std::unique_ptr is trivially-relocatable and optimize 
> > > > > > > > themselves to use memcpy or realloc or whatever it is that they 
> > > > > > > > want to do. So in that sense trivial_abi is a *stronger* 
> > > > > > > > attribute, not a *weaker* one: the property it determines ought 
> > > > > > > > to imply trivially_relocatable.
> > > > > > > 
> > > > > > > `trivial_abi` is an "orthogonal" attribute: you can have 
> > > > > > > `trivial_abi` types with non-trivial constructors and 
> > > > > > > destructors, which can have observable side effects. For example,
> > > > > > > ```
> > > > > > > struct [[clang::trivial_abi]] DestructionAnnouncer {
> > > > > > > ~DestructionAnnouncer() { puts("hello!"); }
> > > > > > > };
> > > > > > > ```
> > > > > > > is `trivial_abi` (because of the annotation) yet not trivially 
> > > > > > > relocatable, because its "move plus destroy" operation has 
> > > > > > > observable side effects.
> > > > > > > 
> > > > > > > > The only interesting question in the language design that I 
> > > > > > > > know of is what happens if you put the attribute on a template 
> > > > > > > > that's instantiated to contain a sub-object that is definitely 
> > > > > > > > not trivially relocatable / trivial-ABI. For trivial_abi, we 
> > > > > > > > decided that the attribute is simply ignored — it implicitly 
> > > > > > > > only applies to specializations where the attribute would be 
> > > > > > > > legal. I haven't dug into the design enough to know what 
> > > > > > > > trivially_relocatable decides in this situation, but the three 
> > > > > > > > basic options are:
> > > > > > > >
> > > > > > > > - the attribute always has effect and allows trivial relocation 
> > > > > > > > regardless of the subobject types; this is obviously unsafe, so 
> > > > > > > > it limits the safe applicability of the attribute to templates
> > > > > > > > - the attribute is ignored, like trivial_abi is
> > > > > > > > - the attribute is ill-formed, and you'll need to add a 
> > > > > > > > [[trivially_relocatable(bool)]] version to support templates
> > > > > > > 
> > > > > > > What happens is basically the first thing you said, except that I 
> > > > > > > disagree that it's "obviously unsafe." Right now, conditionally 
> > > > > > > trivial relocation is possible via template metaprogramming; see 
> > > > > > > the libcxx patch at e.g.
> > > > > > > https://github.com/Quuxplusone/libcxx/commit/6524822c009e#diff-38adc80cec663f2f29c22e9ffc0de912
> > > > > > > Since the attribute is an opt-in mechanism, it makes perfect 
> > > > > > > sense to me that if you put it on a class (or class template), 
> > > > > > > then it applies to the class, without any further sanity-checking 
> > > > > > > by the compiler. The compiler has no reason to second-guess the 
> > > > > > > programmer here.
> > > > > > > 
> > > > > > > However, there's one more interesting case. Suppose the 
> > > > > > > programmer puts the attribute on a class that isn't relocatable 
> > > > > > > at all! (For example, the union case @erichkeane mentioned, or a 
> > > > > > > class type with a deleted destructor.) In that case, this patch 
> > > > > > > *does* give an error... *unless* the class was produced by 
> > > > > > > instantiating a template, in which case we *don't* give an error, 
> > > > > > > because it's not the template-writer's fault.
> > > > > > > https://p1144.godbolt.org/z/wSZPba
> > > > > > > trivial_abi is an "orthogonal" attribute: you can have 
> > > > > > > trivial_abi types with non-trivial constructors and destructors, 
> > > > > > > wh

[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D52674#1297893, @smeenai wrote:

> In https://reviews.llvm.org/D52674#1297879, @rjmccall wrote:
>
> > I'm not worried about the mangler being re-used for multiple declarations, 
> > I'm worried about a global flag changing how we mangle all components of a 
> > type when we only mean to change it at the top level.
>
>
> Hmm, but don't we want it to affect all components? For example, consider 
> something like:
>
>   @interface I
>   @end
>  
>   template  class C {};
>  
>   void f();
>   void g() {
> try {
>   f();
> } catch (C *) {
> }
>   }
>
>
> I would say that we want the RTTI for `C *` to have the discriminator for 
> `I`.


Why?  IIUC, you're adding the discriminator to distinguish between two 
different RTTI objects.  It's not like there are two different types.  You 
should mangle `C` normally here.


Repository:
  rC Clang

https://reviews.llvm.org/D52674



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

[clang-tools-extra] r346835 - [clang-tidy] Avoid C arrays check

[Roman Lebedev via cfe-commits]

Author: lebedevri
Date: Wed Nov 14 01:01:08 2018
New Revision: 346835

URL: http://llvm.org/viewvc/llvm-project?rev=346835&view=rev
Log:
[clang-tidy] Avoid C arrays check

Summary:
[[ https://bugs.llvm.org/show_bug.cgi?id=39224 | PR39224 ]]
As discussed, we can't always do the transform automatically due to that 
array-to-pointer decay of C array.
In order to detect whether we can do said transform, we'd need to be able to 
see all usages of said array,
which is, i would say, rather impossible if e.g. it is in the header.
Thus right now no fixit exists.

Exceptions: `extern "C"` code.

References:
* [[ 
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es27-use-stdarray-or-stack_array-for-arrays-on-the-stack
 | CPPCG ES.27: Use std::array or stack_array for arrays on the stack ]]
* [[ 
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#slcon1-prefer-using-stl-array-or-vector-instead-of-a-c-array
 | CPPCG SL.con.1: Prefer using STL array or vector instead of a C array ]]
* HICPP `4.1.1 Ensure that a function argument does not undergo an 
array-to-pointer conversion`
* MISRA `5-2-12 An identifier with array type passed as a function argument 
shall not decay to a pointer`

Reviewers: aaron.ballman, JonasToth, alexfh, hokein, xazax.hun

Reviewed By: JonasToth

Subscribers: Eugene.Zelenko, mgorny, rnkovacs, cfe-commits

Tags: #clang-tools-extra

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

Added:
clang-tools-extra/trunk/clang-tidy/modernize/AvoidCArraysCheck.cpp
clang-tools-extra/trunk/clang-tidy/modernize/AvoidCArraysCheck.h

clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-avoid-c-arrays.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/hicpp-avoid-c-arrays.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-avoid-c-arrays.rst
clang-tools-extra/trunk/test/clang-tidy/modernize-avoid-c-arrays.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CMakeLists.txt

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tools-extra/trunk/clang-tidy/hicpp/HICPPTidyModule.cpp
clang-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/modernize/ModernizeTidyModule.cpp
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

Modified: clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CMakeLists.txt?rev=346835&r1=346834&r2=346835&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CMakeLists.txt 
(original)
+++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CMakeLists.txt Wed Nov 
14 01:01:08 2018
@@ -28,6 +28,7 @@ add_clang_library(clangTidyCppCoreGuidel
   clangLex
   clangTidy
   clangTidyMiscModule
+  clangTidyModernizeModule
   clangTidyReadabilityModule
   clangTidyUtils
   clangTooling

Modified: 
clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp?rev=346835&r1=346834&r2=346835&view=diff
==
--- 
clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
 (original)
+++ 
clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
 Wed Nov 14 01:01:08 2018
@@ -12,6 +12,7 @@
 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/NonPrivateMemberVariablesInClassesCheck.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
+#include "../modernize/AvoidCArraysCheck.h"
 #include "../readability/MagicNumbersCheck.h"
 #include "AvoidGotoCheck.h"
 #include "InterfacesGlobalInitCheck.h"
@@ -40,6 +41,8 @@ namespace cppcoreguidelines {
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+CheckFactories.registerCheck(
+"cppcoreguidelines-avoid-c-arrays");
 CheckFactories.registerCheck(
 "cppcoreguidelines-avoid-goto");
 CheckFactories.registerCheck(

Modified: clang-tools-extra/trunk/clang-tidy/hicpp/HICPPTidyModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/hicpp/HICPPTidyModule.cpp?rev=346835&r1=346834&r2=346835&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/hicpp/HICPPTidyModule.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/hicpp/HICPPTidyModule.cpp Wed Nov 14 
01:01:08 2018
@@ -22,6 +22,7 @@
 #include "../misc/NewDeleteOverloadsCheck.h"
 #include "../misc/StaticAssertCheck.h"
 #include "../bugprone/UndelegatedConstructorCheck.h"
+#include "..

[PATCH] D53771: [clang-tidy] Avoid C arrays check

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346835: [clang-tidy] Avoid C arrays check (authored by 
lebedevri, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53771?vs=173514&id=173996#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D53771

Files:
  clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CMakeLists.txt
  
clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/hicpp/HICPPTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/modernize/AvoidCArraysCheck.cpp
  clang-tools-extra/trunk/clang-tidy/modernize/AvoidCArraysCheck.h
  clang-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-avoid-c-arrays.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/hicpp-avoid-c-arrays.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-avoid-c-arrays.rst
  clang-tools-extra/trunk/test/clang-tidy/modernize-avoid-c-arrays.cpp

Index: clang-tools-extra/trunk/test/clang-tidy/modernize-avoid-c-arrays.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/modernize-avoid-c-arrays.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-avoid-c-arrays.cpp
@@ -0,0 +1,88 @@
+// RUN: %check_clang_tidy %s modernize-avoid-c-arrays %t
+
+int a[] = {1, 2};
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
+
+int b[1];
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
+
+void foo() {
+  int c[b[0]];
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C VLA arrays, use std::vector<> instead
+
+  using d = decltype(c);
+  d e;
+  // Semi-FIXME: we do not diagnose these last two lines separately,
+  // because we point at typeLoc.getBeginLoc(), which is the decl before that
+  // (int c[b[0]];), which is already diagnosed.
+}
+
+template 
+class array {
+  T d[Size];
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+
+  int e[1];
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+};
+
+array d;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not declare C-style arrays, use std::array<> instead
+
+using k = int[4];
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not declare C-style arrays, use std::array<> instead
+
+array dk;
+
+template 
+class unique_ptr {
+  T *d;
+
+  int e[1];
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+};
+
+unique_ptr d2;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not declare C-style arrays, use std::array<> instead
+
+using k2 = int[];
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not declare C-style arrays, use std::array<> instead
+
+unique_ptr dk2;
+
+// Some header
+extern "C" {
+
+int f[] = {1, 2};
+
+int j[1];
+
+inline void bar() {
+  {
+int j[j[0]];
+  }
+}
+
+extern "C++" {
+int f3[] = {1, 2};
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
+
+int j3[1];
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
+
+struct Foo {
+  int f3[3] = {1, 2};
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+
+  int j3[1];
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+};
+}
+
+struct Bar {
+
+  int f[3] = {1, 2};
+
+  int j[1];
+};
+}
Index: clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
@@ -12,6 +12,7 @@
 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/NonPrivateMemberVariablesInClassesCheck.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
+#include "../modernize/AvoidCArraysCheck.h"
 #include "../readability/MagicNumbersCheck.h"
 #include "AvoidGotoCheck.h"
 #include "InterfacesGlobalInitCheck.h"
@@ -40,6 +41,8 @@
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+CheckFactories.registerCheck(
+"cppcoreguidelines-avoid-c-arrays");
 CheckFactories.registerCheck(
 "cppcoreguidelines-avoid-goto");
 CheckFactories.registerCheck(
Index: clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CMakeLists.txt
=

[clang-tools-extra] r346836 - [clangd] Improve code completion for ObjC methods

[Sam McCall via cfe-commits]

Author: sammccall
Date: Wed Nov 14 01:05:19 2018
New Revision: 346836

URL: http://llvm.org/viewvc/llvm-project?rev=346836&view=rev
Log:
[clangd] Improve code completion for ObjC methods

Summary:
Previously code completion did not work well for Objective-C methods
which contained multiple arguments as clangd did not expect to see
multiple typed-text chunks when handling code completion.

Note that even with this change, we do not consider selector fragments
from previous arguments to be part of the signature (although we
could in the future).

Reviewers: sammccall

Reviewed By: sammccall

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

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

Modified:
clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
clang-tools-extra/trunk/unittests/clangd/CodeCompletionStringsTests.cpp

Modified: clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp?rev=346836&r1=346835&r2=346836&view=diff
==
--- clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp (original)
+++ clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp Wed Nov 14 
01:05:19 2018
@@ -76,6 +76,7 @@ std::string getDeclComment(const ASTCont
 void getSignature(const CodeCompletionString &CCS, std::string *Signature,
   std::string *Snippet, std::string *RequiredQualifiers) {
   unsigned ArgCount = 0;
+  bool HadObjCArguments = false;
   for (const auto &Chunk : CCS) {
 // Informative qualifier chunks only clutter completion results, skip
 // them.
@@ -85,13 +86,36 @@ void getSignature(const CodeCompletionSt
 switch (Chunk.Kind) {
 case CodeCompletionString::CK_TypedText:
   // The typed-text chunk is the actual name. We don't record this chunk.
-  // In general our string looks like .
-  // So once we see the name, any text we recorded so far should be
-  // reclassified as qualifiers.
-  if (RequiredQualifiers)
-*RequiredQualifiers = std::move(*Signature);
-  Signature->clear();
-  Snippet->clear();
+  // C++:
+  //   In general our string looks like .
+  //   So once we see the name, any text we recorded so far should be
+  //   reclassified as qualifiers.
+  //
+  // Objective-C:
+  //   Objective-C methods may have multiple typed-text chunks, so we must
+  //   treat them carefully. For Objective-C methods, all typed-text chunks
+  //   will end in ':' (unless there are no arguments, in which case we
+  //   can safely treat them as C++).
+  if (!StringRef(Chunk.Text).endswith(":")) {  // Treat as C++.
+if (RequiredQualifiers)
+  *RequiredQualifiers = std::move(*Signature);
+Signature->clear();
+Snippet->clear();
+  } else {  // Objective-C method with args.
+// If this is the first TypedText to the Objective-C method, discard 
any
+// text that we've previously seen (such as previous parameter 
selector,
+// which will be marked as Informative text).
+//
+// TODO: Make previous parameters part of the signature for Objective-C
+// methods.
+if (!HadObjCArguments) {
+  HadObjCArguments = true;
+  Signature->clear();
+} else {  // Subsequent argument, considered part of snippet/signature.
+  *Signature += Chunk.Text;
+  *Snippet += Chunk.Text;
+}
+  }
   break;
 case CodeCompletionString::CK_Text:
   *Signature += Chunk.Text;

Modified: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp?rev=346836&r1=346835&r2=346836&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp Wed Nov 14 
01:05:19 2018
@@ -67,6 +67,7 @@ MATCHER(InsertInclude, "") {
 }
 MATCHER_P(SnippetSuffix, Text, "") { return arg.SnippetSuffix == Text; }
 MATCHER_P(Origin, OriginSet, "") { return arg.Origin == OriginSet; }
+MATCHER_P(Signature, S, "") { return arg.Signature == S; }
 
 // Shorthand for Contains(Named(Name)).
 Matcher &> Has(std::string Name) {
@@ -105,7 +106,8 @@ CodeCompleteResult completions(ClangdSer
 
 CodeCompleteResult completions(ClangdServer &Server, StringRef Text,
std::vector IndexSymbols = {},
-   clangd::CodeCompleteOptions Opts = {}) {
+   clangd::CodeCompleteOptions Opts = {},
+   PathRef FilePath = "foo.cpp") {
   std::unique_ptr OverrideIndex;

[PATCH] D53934: [clangd] Improve code completion for ObjC methods

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE346836: [clangd] Improve code completion for ObjC methods 
(authored by sammccall, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D53934?vs=173737&id=173997#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53934

Files:
  clangd/CodeCompletionStrings.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/CodeCompletionStringsTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -67,6 +67,7 @@
 }
 MATCHER_P(SnippetSuffix, Text, "") { return arg.SnippetSuffix == Text; }
 MATCHER_P(Origin, OriginSet, "") { return arg.Origin == OriginSet; }
+MATCHER_P(Signature, S, "") { return arg.Signature == S; }
 
 // Shorthand for Contains(Named(Name)).
 Matcher &> Has(std::string Name) {
@@ -105,15 +106,16 @@
 
 CodeCompleteResult completions(ClangdServer &Server, StringRef Text,
std::vector IndexSymbols = {},
-   clangd::CodeCompleteOptions Opts = {}) {
+   clangd::CodeCompleteOptions Opts = {},
+   PathRef FilePath = "foo.cpp") {
   std::unique_ptr OverrideIndex;
   if (!IndexSymbols.empty()) {
 assert(!Opts.Index && "both Index and IndexSymbols given!");
 OverrideIndex = memIndex(std::move(IndexSymbols));
 Opts.Index = OverrideIndex.get();
   }
 
-  auto File = testPath("foo.cpp");
+  auto File = testPath(FilePath);
   Annotations Test(Text);
   runAddDocument(Server, File, Test.code());
   auto CompletionList =
@@ -125,12 +127,14 @@
 // If IndexSymbols is non-empty, an index will be built and passed to opts.
 CodeCompleteResult completions(StringRef Text,
std::vector IndexSymbols = {},
-   clangd::CodeCompleteOptions Opts = {}) {
+   clangd::CodeCompleteOptions Opts = {},
+   PathRef FilePath = "foo.cpp") {
   MockFSProvider FS;
   MockCompilationDatabase CDB;
   IgnoreDiagnostics DiagConsumer;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
-  return completions(Server, Text, std::move(IndexSymbols), std::move(Opts));
+  return completions(Server, Text, std::move(IndexSymbols), std::move(Opts),
+ FilePath);
 }
 
 std::string replace(StringRef Haystack, StringRef Needle, StringRef Repl) {
@@ -2204,6 +2208,84 @@
  {cls("naber"), cls("nx::naber")}, Opts);
   EXPECT_THAT(Results.Completions, UnorderedElementsAre());
 }
+
+TEST(CompletionTest, ObjectiveCMethodNoArguments) {
+  auto Results = completions(R"objc(
+  @interface Foo
+  @property(nonatomic, setter=setXToIgnoreComplete:) int value;
+  @end
+  Foo *foo = [Foo new]; int y = [foo v^]
+)objc",
+/*IndexSymbols=*/{},
+/*Opts=*/{},
+"Foo.m");
+
+  auto C = Results.Completions;
+  EXPECT_THAT(C, ElementsAre(Named("value")));
+  EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method)));
+  EXPECT_THAT(C, ElementsAre(ReturnType("int")));
+  EXPECT_THAT(C, ElementsAre(Signature("")));
+  EXPECT_THAT(C, ElementsAre(SnippetSuffix("")));
+}
+
+TEST(CompletionTest, ObjectiveCMethodOneArgument) {
+  auto Results = completions(R"objc(
+  @interface Foo
+  - (int)valueForCharacter:(char)c;
+  @end
+  Foo *foo = [Foo new]; int y = [foo v^]
+)objc",
+/*IndexSymbols=*/{},
+/*Opts=*/{},
+"Foo.m");
+
+  auto C = Results.Completions;
+  EXPECT_THAT(C, ElementsAre(Named("valueForCharacter:")));
+  EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method)));
+  EXPECT_THAT(C, ElementsAre(ReturnType("int")));
+  EXPECT_THAT(C, ElementsAre(Signature("(char)")));
+  EXPECT_THAT(C, ElementsAre(SnippetSuffix("${1:(char)}")));
+}
+
+TEST(CompletionTest, ObjectiveCMethodTwoArgumentsFromBeginning) {
+  auto Results = completions(R"objc(
+  @interface Foo
+  + (id)fooWithValue:(int)value fooey:(unsigned int)fooey;
+  @end
+  id val = [Foo foo^]
+)objc",
+/*IndexSymbols=*/{},
+/*Opts=*/{},
+"Foo.m");
+
+  auto C = Results.Completions;
+  EXPECT_THAT(C, ElementsAre(Named("fooWithValue:")));
+  EXPECT_THAT(C, ElementsAre(Kind(CompletionItemKind::Method)));
+  EXPECT_THAT(C, ElementsAre(ReturnType("id")));
+  EXPECT_THAT(C, ElementsAre(Signature("(int) fooey:(unsigned int)")));
+  EXPECT_THAT(C,
+  ElementsAre(SnippetSuffix("${1:(int)} fooey:${2:(unsigned int)}")));
+}
+
+TEST(CompletionTest, ObjectiveCMethodTwoArgumentsFromMiddle) {
+  auto Results = completions(R"objc(
+  @interface Foo
+  + (id)fooWithValue:(int)value fooey:(unsigned int)fooey;
+  @end
+  id val = [Foo fooWithValue:10 f^]
+)objc",
+/*IndexSymbols=*/{},
+/*Opts=*/{},
+"Foo.m");
+

r346837 - [Clang] - Add '-gsplit-dwarf[=split, =single]' version for '-gsplit-dwarf' option.

[George Rimar via cfe-commits]

Author: grimar
Date: Wed Nov 14 01:22:16 2018
New Revision: 346837

URL: http://llvm.org/viewvc/llvm-project?rev=346837&view=rev
Log:
[Clang] - Add '-gsplit-dwarf[=split,=single]' version for '-gsplit-dwarf' 
option.

The DWARF5 specification says(Appendix F.1):

"The sections that do not require relocation, however, can be
written to the relocatable object (.o) file but ignored by the
linker or they can be written to a separate DWARF object (.dwo)
file that need not be accessed by the linker."

The first part describes a single file split DWARF feature and there
is no way to trigger this behavior atm. 
Fortunately, no many changes are required to keep *.dwo sections
in a .o, the patch does that.

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

Added:
cfe/trunk/test/CodeGen/split-debug-single-file.c
Modified:
cfe/trunk/include/clang/Driver/CC1Options.td
cfe/trunk/include/clang/Driver/Options.td
cfe/trunk/include/clang/Frontend/CodeGenOptions.def
cfe/trunk/include/clang/Frontend/CodeGenOptions.h
cfe/trunk/lib/CodeGen/BackendUtil.cpp
cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
cfe/trunk/lib/Driver/ToolChains/Clang.cpp
cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp
cfe/trunk/lib/Driver/ToolChains/CommonArgs.h
cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
cfe/trunk/lib/Driver/ToolChains/MinGW.cpp
cfe/trunk/lib/Frontend/CompilerInvocation.cpp
cfe/trunk/test/Driver/split-debug.c
cfe/trunk/test/Driver/split-debug.s

Modified: cfe/trunk/include/clang/Driver/CC1Options.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/CC1Options.td?rev=346837&r1=346836&r2=346837&view=diff
==
--- cfe/trunk/include/clang/Driver/CC1Options.td (original)
+++ cfe/trunk/include/clang/Driver/CC1Options.td Wed Nov 14 01:22:16 2018
@@ -649,7 +649,9 @@ def fblocks_runtime_optional : Flag<["-"
 def fexternc_nounwind : Flag<["-"], "fexternc-nounwind">,
   HelpText<"Assume all functions with C linkage do not unwind">;
 def enable_split_dwarf : Flag<["-"], "enable-split-dwarf">,
-  HelpText<"Use split dwarf/Fission">;
+  HelpText<"Use DWARF fission in 'split' mode">;
+def enable_split_dwarf_EQ : Joined<["-"], "enable-split-dwarf=">,
+  HelpText<"Set DWARF fission mode to either 'split' or 'single'">, 
Values<"split,single">;
 def fno_wchar : Flag<["-"], "fno-wchar">,
   HelpText<"Disable C++ builtin type wchar_t">;
 def fconstant_string_class : Separate<["-"], "fconstant-string-class">,

Modified: cfe/trunk/include/clang/Driver/Options.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=346837&r1=346836&r2=346837&view=diff
==
--- cfe/trunk/include/clang/Driver/Options.td (original)
+++ cfe/trunk/include/clang/Driver/Options.td Wed Nov 14 01:22:16 2018
@@ -1840,6 +1840,9 @@ def gno_strict_dwarf : Flag<["-"], "gno-
 def gcolumn_info : Flag<["-"], "gcolumn-info">, Group, 
Flags<[CoreOption]>;
 def gno_column_info : Flag<["-"], "gno-column-info">, Group, 
Flags<[CoreOption]>;
 def gsplit_dwarf : Flag<["-"], "gsplit-dwarf">, Group;
+def gsplit_dwarf_EQ : Joined<["-"], "gsplit-dwarf=">, Group,
+  HelpText<"Set DWARF fission mode to either 'split' or 'single'">,
+  Values<"split,single">;
 def ggnu_pubnames : Flag<["-"], "ggnu-pubnames">, Group, 
Flags<[CC1Option]>;
 def gno_gnu_pubnames : Flag<["-"], "gno-gnu-pubnames">, Group, 
Flags<[CC1Option]>;
 def gpubnames : Flag<["-"], "gpubnames">, Group, 
Flags<[CC1Option]>;

Modified: cfe/trunk/include/clang/Frontend/CodeGenOptions.def
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/CodeGenOptions.def?rev=346837&r1=346836&r2=346837&view=diff
==
--- cfe/trunk/include/clang/Frontend/CodeGenOptions.def (original)
+++ cfe/trunk/include/clang/Frontend/CodeGenOptions.def Wed Nov 14 01:22:16 2018
@@ -252,7 +252,9 @@ CODEGENOPT(DebugTypeExtRefs, 1, 0) ///<
 CODEGENOPT(DebugExplicitImport, 1, 0)  ///< Whether or not debug info should
///< contain explicit imports for
///< anonymous namespaces
-CODEGENOPT(EnableSplitDwarf, 1, 0) ///< Whether to enable split DWARF
+
+ENUM_CODEGENOPT(SplitDwarfMode, DwarfFissionKind, 2, NoFission) ///< DWARF 
fission mode to use.
+
 CODEGENOPT(SplitDwarfInlining, 1, 1) ///< Whether to include inlining info in 
the
  ///< skeleton CU to allow for 
symbolication
  ///< of inline stack frames without .dwo 
files.

Modified: cfe/trunk/include/clang/Frontend/CodeGenOptions.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/CodeGenOptions.h?rev=346837&r1=346836&r2=346837&view=diff

[PATCH] D52296: [Clang] - Add '-gsplit-dwarf[=split, =single]' version for '-gsplit-dwarf' option.

[George Rimar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC346837: [Clang] - Add 
'-gsplit-dwarf[=split,=single]' version for 
'-gsplit-dwarf'… (authored by grimar, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D52296?vs=173643&id=173998#toc

Repository:
  rC Clang

https://reviews.llvm.org/D52296

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  include/clang/Frontend/CodeGenOptions.h
  lib/CodeGen/BackendUtil.cpp
  lib/CodeGen/CGDebugInfo.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/CommonArgs.cpp
  lib/Driver/ToolChains/CommonArgs.h
  lib/Driver/ToolChains/Gnu.cpp
  lib/Driver/ToolChains/MinGW.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/split-debug-single-file.c
  test/Driver/split-debug.c
  test/Driver/split-debug.s

Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -596,9 +596,25 @@
   Opts.MacroDebugInfo = Args.hasArg(OPT_debug_info_macro);
   Opts.WholeProgramVTables = Args.hasArg(OPT_fwhole_program_vtables);
   Opts.LTOVisibilityPublicStd = Args.hasArg(OPT_flto_visibility_public_std);
-  Opts.EnableSplitDwarf = Args.hasArg(OPT_enable_split_dwarf);
   Opts.SplitDwarfFile = Args.getLastArgValue(OPT_split_dwarf_file);
   Opts.SplitDwarfInlining = !Args.hasArg(OPT_fno_split_dwarf_inlining);
+
+  if (Arg *A =
+  Args.getLastArg(OPT_enable_split_dwarf, OPT_enable_split_dwarf_EQ)) {
+if (A->getOption().matches(options::OPT_enable_split_dwarf)) {
+  Opts.setSplitDwarfMode(CodeGenOptions::SplitFileFission);
+} else {
+  StringRef Name = A->getValue();
+  if (Name == "single")
+Opts.setSplitDwarfMode(CodeGenOptions::SingleFileFission);
+  else if (Name == "split")
+Opts.setSplitDwarfMode(CodeGenOptions::SplitFileFission);
+  else
+Diags.Report(diag::err_drv_invalid_value)
+<< A->getAsString(Args) << Name;
+}
+  }
+
   Opts.DebugTypeExtRefs = Args.hasArg(OPT_dwarf_ext_refs);
   Opts.DebugExplicitImport = Args.hasArg(OPT_dwarf_explicit_import);
   Opts.DebugFwdTemplateParams = Args.hasArg(OPT_debug_forward_template_params);
Index: lib/CodeGen/BackendUtil.cpp
===
--- lib/CodeGen/BackendUtil.cpp
+++ lib/CodeGen/BackendUtil.cpp
@@ -468,7 +468,7 @@
   Options.EmitStackSizeSection = CodeGenOpts.StackSizeSection;
   Options.EmitAddrsig = CodeGenOpts.Addrsig;
 
-  if (CodeGenOpts.EnableSplitDwarf)
+  if (CodeGenOpts.getSplitDwarfMode() != CodeGenOptions::NoFission)
 Options.MCOptions.SplitDwarfFile = CodeGenOpts.SplitDwarfFile;
   Options.MCOptions.MCRelaxAll = CodeGenOpts.RelaxAll;
   Options.MCOptions.MCSaveTempLabels = CodeGenOpts.SaveTempLabels;
@@ -832,7 +832,8 @@
 break;
 
   default:
-if (!CodeGenOpts.SplitDwarfFile.empty()) {
+if (!CodeGenOpts.SplitDwarfFile.empty() &&
+(CodeGenOpts.getSplitDwarfMode() == CodeGenOptions::SplitFileFission)) {
   DwoOS = openOutputFile(CodeGenOpts.SplitDwarfFile);
   if (!DwoOS)
 return;
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -581,8 +581,11 @@
   CGOpts.EmitVersionIdentMetadata ? Producer : "",
   LO.Optimize || CGOpts.PrepareForLTO || CGOpts.PrepareForThinLTO,
   CGOpts.DwarfDebugFlags, RuntimeVers,
-  CGOpts.EnableSplitDwarf ? "" : CGOpts.SplitDwarfFile, EmissionKind,
-  0 /* DWOid */, CGOpts.SplitDwarfInlining, CGOpts.DebugInfoForProfiling,
+  (CGOpts.getSplitDwarfMode() != CodeGenOptions::NoFission)
+  ? ""
+  : CGOpts.SplitDwarfFile,
+  EmissionKind, 0 /* DWOid */, CGOpts.SplitDwarfInlining,
+  CGOpts.DebugInfoForProfiling,
   CGM.getTarget().getTriple().isNVPTX()
   ? llvm::DICompileUnit::DebugNameTableKind::None
   : static_cast(
Index: lib/Driver/ToolChains/CommonArgs.cpp
===
--- lib/Driver/ToolChains/CommonArgs.cpp
+++ lib/Driver/ToolChains/CommonArgs.cpp
@@ -808,7 +808,12 @@
   return false;
 }
 
-const char *tools::SplitDebugName(const ArgList &Args, const InputInfo &Input) {
+const char *tools::SplitDebugName(const ArgList &Args, const InputInfo &Input,
+  const InputInfo &Output) {
+  if (Arg *A = Args.getLastArg(options::OPT_gsplit_dwarf_EQ))
+if (StringRef(A->getValue()) == "single")
+  return Args.MakeArgString(Output.getFilename());
+
   Arg *FinalOutput = Args.getLastArg(options::OPT_o);
   if (FinalOutput && Args.hasArg(options::OPT_c)) {
 SmallString<128> T(FinalOutput->getValue());
Index: lib/Driver/ToolChains/MinGW.cpp

[PATCH] D53809: Fix invalid address space generation for clk_event_t

[Alexey Sotkin via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC346838: [OpenCL] Fix invalid address space generation for 
clk_event_t (authored by AlexeySotkin, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D53809

Files:
  lib/CodeGen/CGBuiltin.cpp
  test/CodeGenOpenCL/cl20-device-side-enqueue.cl


Index: test/CodeGenOpenCL/cl20-device-side-enqueue.cl
===
--- test/CodeGenOpenCL/cl20-device-side-enqueue.cl
+++ test/CodeGenOpenCL/cl20-device-side-enqueue.cl
@@ -106,6 +106,13 @@
a[i] = b[i];
  });
 
+  // COMMON-LABEL: call i32 @__enqueue_kernel_basic_events
+  // COMMON-SAME: (%opencl.queue_t{{.*}}* {{%[0-9]+}}, i32 {{%[0-9]+}}, 
%struct.ndrange_t* {{.*}}, i32 1, %opencl.clk_event_t{{.*}}* addrspace(4)* 
{{%[0-9]+}}, %opencl.clk_event_t{{.*}}* addrspace(4)* null,
+  enqueue_kernel(default_queue, flags, ndrange, 1, &event_wait_list, 0,
+ ^(void) {
+   return;
+ });
+
   // Emits global block literal [[BLG1]] and block kernel [[INVGK1]].
   // COMMON: [[DEF_Q:%[0-9]+]] = load %opencl.queue_t{{.*}}*, 
%opencl.queue_t{{.*}}** %default_queue
   // COMMON: [[FLAGS:%[0-9]+]] = load i32, i32* %flags
@@ -390,7 +397,7 @@
 // COMMON: define internal spir_kernel void [[INVGK5]](i8 addrspace(4)*{{.*}}, 
i8 addrspace(3)*{{.*}})
 // COMMON: define internal spir_kernel void [[INVGK6]](i8 addrspace(4)*, i8 
addrspace(3)*, i8 addrspace(3)*, i8 addrspace(3)*) #{{[0-9]+}} {
 // COMMON: entry:
-// COMMON:  call void @__device_side_enqueue_block_invoke_8(i8 addrspace(4)* 
%0, i8 addrspace(3)* %1, i8 addrspace(3)* %2, i8 addrspace(3)* %3)
+// COMMON:  call void @__device_side_enqueue_block_invoke_9(i8 addrspace(4)* 
%0, i8 addrspace(3)* %1, i8 addrspace(3)* %2, i8 addrspace(3)* %3)
 // COMMON:  ret void
 // COMMON: }
 // COMMON: define internal spir_kernel void [[INVGK7]](i8 addrspace(4)*{{.*}}, 
i8 addrspace(3)*{{.*}})
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -3610,7 +3610,9 @@
   llvm::Value *ClkEvent = EmitScalarExpr(E->getArg(5));
   // Convert to generic address space.
   EventList = Builder.CreatePointerCast(EventList, EventPtrTy);
-  ClkEvent = Builder.CreatePointerCast(ClkEvent, EventPtrTy);
+  ClkEvent = ClkEvent->getType()->isIntegerTy()
+   ? Builder.CreateBitOrPointerCast(ClkEvent, EventPtrTy)
+   : Builder.CreatePointerCast(ClkEvent, EventPtrTy);
   auto Info =
   CGM.getOpenCLRuntime().emitOpenCLEnqueuedBlock(*this, E->getArg(6));
   llvm::Value *Kernel =


Index: test/CodeGenOpenCL/cl20-device-side-enqueue.cl
===
--- test/CodeGenOpenCL/cl20-device-side-enqueue.cl
+++ test/CodeGenOpenCL/cl20-device-side-enqueue.cl
@@ -106,6 +106,13 @@
a[i] = b[i];
  });
 
+  // COMMON-LABEL: call i32 @__enqueue_kernel_basic_events
+  // COMMON-SAME: (%opencl.queue_t{{.*}}* {{%[0-9]+}}, i32 {{%[0-9]+}}, %struct.ndrange_t* {{.*}}, i32 1, %opencl.clk_event_t{{.*}}* addrspace(4)* {{%[0-9]+}}, %opencl.clk_event_t{{.*}}* addrspace(4)* null,
+  enqueue_kernel(default_queue, flags, ndrange, 1, &event_wait_list, 0,
+ ^(void) {
+   return;
+ });
+
   // Emits global block literal [[BLG1]] and block kernel [[INVGK1]].
   // COMMON: [[DEF_Q:%[0-9]+]] = load %opencl.queue_t{{.*}}*, %opencl.queue_t{{.*}}** %default_queue
   // COMMON: [[FLAGS:%[0-9]+]] = load i32, i32* %flags
@@ -390,7 +397,7 @@
 // COMMON: define internal spir_kernel void [[INVGK5]](i8 addrspace(4)*{{.*}}, i8 addrspace(3)*{{.*}})
 // COMMON: define internal spir_kernel void [[INVGK6]](i8 addrspace(4)*, i8 addrspace(3)*, i8 addrspace(3)*, i8 addrspace(3)*) #{{[0-9]+}} {
 // COMMON: entry:
-// COMMON:  call void @__device_side_enqueue_block_invoke_8(i8 addrspace(4)* %0, i8 addrspace(3)* %1, i8 addrspace(3)* %2, i8 addrspace(3)* %3)
+// COMMON:  call void @__device_side_enqueue_block_invoke_9(i8 addrspace(4)* %0, i8 addrspace(3)* %1, i8 addrspace(3)* %2, i8 addrspace(3)* %3)
 // COMMON:  ret void
 // COMMON: }
 // COMMON: define internal spir_kernel void [[INVGK7]](i8 addrspace(4)*{{.*}}, i8 addrspace(3)*{{.*}})
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -3610,7 +3610,9 @@
   llvm::Value *ClkEvent = EmitScalarExpr(E->getArg(5));
   // Convert to generic address space.
   EventList = Builder.CreatePointerCast(EventList, EventPtrTy);
-  ClkEvent = Builder.CreatePointerCast(ClkEvent, EventPtrTy);
+  ClkEvent = ClkEvent->getType()->isIntegerTy()
+   ? Builder.CreateBitOrPointerCast(ClkEvent, EventPtrTy)
+

[PATCH] D54309: [AST] Allow limiting the scope of common AST traversals (getParents, RAV).

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 174000.
sammccall marked 2 inline comments as done.
sammccall added a comment.

Address review comments.
Remove special case for TUDecl since no-parents is now handled.


Repository:
  rC Clang

https://reviews.llvm.org/D54309

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/RecursiveASTVisitor.h
  lib/AST/ASTContext.cpp
  lib/ASTMatchers/ASTMatchFinder.cpp
  unittests/AST/ASTContextParentMapTest.cpp
  unittests/Tooling/CMakeLists.txt
  unittests/Tooling/RecursiveASTVisitorTests/TraversalScope.cpp

Index: unittests/Tooling/RecursiveASTVisitorTests/TraversalScope.cpp
===
--- /dev/null
+++ unittests/Tooling/RecursiveASTVisitorTests/TraversalScope.cpp
@@ -0,0 +1,51 @@
+//===- unittest/Tooling/RecursiveASTVisitorTests/TraversalScope.cpp ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "TestVisitor.h"
+
+using namespace clang;
+
+namespace {
+
+class Visitor : public ExpectedLocationVisitor {
+public:
+  Visitor(ASTContext *Context) { this->Context = Context; }
+
+  bool VisitNamedDecl(NamedDecl *D) {
+if (!D->isImplicit())
+  Match(D->getName(), D->getLocation());
+return true;
+  }
+};
+
+TEST(RecursiveASTVisitor, RespectsTraversalScope) {
+  auto AST = tooling::buildASTFromCode(
+  R"cpp(
+struct foo {
+  struct bar {
+struct baz {};
+  };
+};
+  )cpp",
+  "foo.cpp", std::make_shared());
+  auto &Ctx = AST->getASTContext();
+  auto &TU = *Ctx.getTranslationUnitDecl();
+  auto &Foo = *TU.lookup(&Ctx.Idents.get("foo")).front();
+  auto &Bar = *cast(Foo).lookup(&Ctx.Idents.get("bar")).front();
+
+  Ctx.setTraversalScope({&Bar});
+
+  Visitor V(&Ctx);
+  V.DisallowMatch("foo", 2, 8);
+  V.ExpectMatch("bar", 3, 10);
+  V.ExpectMatch("baz", 4, 12);
+  V.TraverseAST(Ctx);
+}
+
+} // end anonymous namespace
Index: unittests/Tooling/CMakeLists.txt
===
--- unittests/Tooling/CMakeLists.txt
+++ unittests/Tooling/CMakeLists.txt
@@ -40,6 +40,7 @@
   RecursiveASTVisitorTests/NestedNameSpecifiers.cpp
   RecursiveASTVisitorTests/ParenExpr.cpp
   RecursiveASTVisitorTests/TemplateArgumentLocTraverser.cpp
+  RecursiveASTVisitorTests/TraversalScope.cpp
   RecursiveASTVisitorTestDeclVisitor.cpp
   RecursiveASTVisitorTestPostOrderVisitor.cpp
   RecursiveASTVisitorTestTypeLocVisitor.cpp
Index: unittests/AST/ASTContextParentMapTest.cpp
===
--- unittests/AST/ASTContextParentMapTest.cpp
+++ unittests/AST/ASTContextParentMapTest.cpp
@@ -17,6 +17,9 @@
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Tooling.h"
 #include "gtest/gtest.h"
+#include "gmock/gmock.h"
+
+using testing::ElementsAre;
 
 namespace clang {
 namespace ast_matchers {
@@ -78,5 +81,30 @@
   hasAncestor(cxxRecordDecl(unless(isTemplateInstantiation(;
 }
 
+TEST(GetParents, RespectsTraversalScope) {
+  auto AST =
+  tooling::buildASTFromCode("struct foo { int bar; };", "foo.cpp",
+std::make_shared());
+  auto &Ctx = AST->getASTContext();
+  auto &TU = *Ctx.getTranslationUnitDecl();
+  auto &Foo = *TU.lookup(&Ctx.Idents.get("foo")).front();
+  auto &Bar = *cast(Foo).lookup(&Ctx.Idents.get("bar")).front();
+
+  using ast_type_traits::DynTypedNode;
+  // Initially, scope is the whole TU.
+  EXPECT_THAT(Ctx.getParents(Bar), ElementsAre(DynTypedNode::create(Foo)));
+  EXPECT_THAT(Ctx.getParents(Foo), ElementsAre(DynTypedNode::create(TU)));
+
+  // Restrict the scope, now some parents are gone.
+  Ctx.setTraversalScope({&Foo});
+  EXPECT_THAT(Ctx.getParents(Bar), ElementsAre(DynTypedNode::create(Foo)));
+  EXPECT_THAT(Ctx.getParents(Foo), ElementsAre());
+
+  // Reset the scope, we get back the original results.
+  Ctx.setTraversalScope({&TU});
+  EXPECT_THAT(Ctx.getParents(Bar), ElementsAre(DynTypedNode::create(Foo)));
+  EXPECT_THAT(Ctx.getParents(Foo), ElementsAre(DynTypedNode::create(TU)));
+}
+
 } // end namespace ast_matchers
 } // end namespace clang
Index: lib/ASTMatchers/ASTMatchFinder.cpp
===
--- lib/ASTMatchers/ASTMatchFinder.cpp
+++ lib/ASTMatchers/ASTMatchFinder.cpp
@@ -635,10 +635,6 @@
   bool memoizedMatchesAncestorOfRecursively(
   const ast_type_traits::DynTypedNode &Node, const DynTypedMatcher &Matcher,
   BoundNodesTreeBuilder *Builder, AncestorMatchMode MatchMode) {
-if (Node.get() ==
-ActiveASTContext->getTranslationUnitDecl())
-  return false;
-
 // For AST-nodes that don't have an identity, we can't memoize.
 if (!Builder->isComparable())
   return matchesAncestorOfRecursively(Nod

[PATCH] D54309: [AST] Allow limiting the scope of common AST traversals (getParents, RAV).

[Eric Liu via Phabricator via cfe-commits]

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

lgtm




Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:674
+  // Nodes may have no parents if:
+  //  a) the node is the TranslationUnitDecl
+  //  a) there is a bug in the AST, and the node is not reachable

nit: a,b,c



Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:680
+  assert(Node.get() ||
+ !llvm::any_of(ActiveASTContext->getTraversalScope(),
+   [](Decl *D) {

`none_of`?


Repository:
  rC Clang

https://reviews.llvm.org/D54309



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

r346847 - [AST] Allow limiting the scope of common AST traversals (getParents, RAV).

[Sam McCall via cfe-commits]

Author: sammccall
Date: Wed Nov 14 02:33:30 2018
New Revision: 346847

URL: http://llvm.org/viewvc/llvm-project?rev=346847&view=rev
Log:
[AST] Allow limiting the scope of common AST traversals (getParents, RAV).

Summary:
The goal is to allow analyses such as clang-tidy checks to run on a
subset of the AST, e.g. "only on main-file decls" for interactive tools.

Today, these become "problematically global" by running RecursiveASTVisitors
rooted at the TUDecl, or by navigating up via ASTContext::getParent().

The scope is restricted using a set of top-level-decls that RecursiveASTVisitors
should be rooted at. This also applies to the visitor that populates the
parent map, and so the top-level-decls are considered to have no parents.

This patch makes the traversal scope a mutable property of ASTContext.
The more obvious way to do this is to pass the top-level decls to
relevant functions directly, but this has some problems:
 - it's error-prone: accidentally mixing restricted and unrestricted
   scopes is a performance trap. Interleaving multiple analyses is
   common (many clang-tidy checks run matchers or RAVs from matcher callbacks)
 - it doesn't map well to the actual use cases, where we really do want
   *all* traversals to be restricted.
 - it involves a lot of plumbing in parts of the code that don't care
   about traversals.
This approach was tried out in D54259 and D54261, I wanted to like it
but it feels pretty awful in practice.

Caveats: to get scope-limiting behavior of RecursiveASTVisitors, callers
have to call the new TraverseAST(Ctx) function instead of TraverseDecl(TU).
I think this is an improvement to the API regardless.

Reviewers: klimek, ioeric

Subscribers: mgorny, cfe-commits

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

Added:
cfe/trunk/unittests/Tooling/RecursiveASTVisitorTests/TraversalScope.cpp
Modified:
cfe/trunk/include/clang/AST/ASTContext.h
cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
cfe/trunk/lib/AST/ASTContext.cpp
cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp
cfe/trunk/unittests/AST/ASTContextParentMapTest.cpp
cfe/trunk/unittests/Tooling/CMakeLists.txt

Modified: cfe/trunk/include/clang/AST/ASTContext.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTContext.h?rev=346847&r1=346846&r2=346847&view=diff
==
--- cfe/trunk/include/clang/AST/ASTContext.h (original)
+++ cfe/trunk/include/clang/AST/ASTContext.h Wed Nov 14 02:33:30 2018
@@ -569,26 +569,6 @@ public:
   IntrusiveRefCntPtr ExternalSource;
   ASTMutationListener *Listener = nullptr;
 
-  /// Contains parents of a node.
-  using ParentVector = llvm::SmallVector;
-
-  /// Maps from a node to its parents. This is used for nodes that have
-  /// pointer identity only, which are more common and we can save space by
-  /// only storing a unique pointer to them.
-  using ParentMapPointers =
-  llvm::DenseMap>;
-
-  /// Parent map for nodes without pointer identity. We store a full
-  /// DynTypedNode for all keys.
-  using ParentMapOtherNodes =
-  llvm::DenseMap>;
-
   /// Container for either a single DynTypedNode or for an ArrayRef to
   /// DynTypedNode. For use with ParentMap.
   class DynTypedNodeList {
@@ -630,7 +610,17 @@ public:
 }
   };
 
-  /// Returns the parents of the given node.
+  // A traversal scope limits the parts of the AST visible to certain analyses.
+  // RecursiveASTVisitor::TraverseAST will only visit reachable nodes, and
+  // getParents() will only observe reachable parent edges.
+  //
+  // The scope is defined by a set of "top-level" declarations.
+  // Initially, it is the entire TU: {getTranslationUnitDecl()}.
+  // Changing the scope clears the parent cache, which is expensive to rebuild.
+  std::vector getTraversalScope() const { return TraversalScope; }
+  void setTraversalScope(const std::vector &);
+
+  /// Returns the parents of the given node (within the traversal scope).
   ///
   /// Note that this will lazily compute the parents of all nodes
   /// and store them for later retrieval. Thus, the first call is O(n)
@@ -2924,13 +2914,13 @@ private:
   // but we include it here so that ASTContext can quickly deallocate them.
   llvm::PointerIntPair LastSDM;
 
-  std::unique_ptr PointerParents;
-  std::unique_ptr OtherParents;
+  std::vector TraversalScope;
+  class ParentMap;
+  std::unique_ptr Parents;
 
   std::unique_ptr VTContext;
 
   void ReleaseDeclContextMaps();
-  void ReleaseParentMapEntries();
 
 public:
   enum PragmaSectionFlag : unsigned {

Modified: cfe/trunk/include/clang/AST/RecursiveASTVisitor.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/RecursiveASTVisitor.h?rev=346847&r1=346846&r2=346847&view=diff
==
--- cfe/trunk/include/clang/AST/RecursiveASTVisitor.h (original)
+++ cfe/trunk/include/clang/AST/RecursiveASTVisitor.h We

[PATCH] D54309: [AST] Allow limiting the scope of common AST traversals (getParents, RAV).

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
sammccall marked 2 inline comments as done.
Closed by commit rC346847: [AST] Allow limiting the scope of common AST 
traversals (getParents, RAV). (authored by sammccall, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D54309?vs=174000&id=174004#toc

Repository:
  rC Clang

https://reviews.llvm.org/D54309

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/RecursiveASTVisitor.h
  lib/AST/ASTContext.cpp
  lib/ASTMatchers/ASTMatchFinder.cpp
  unittests/AST/ASTContextParentMapTest.cpp
  unittests/Tooling/CMakeLists.txt
  unittests/Tooling/RecursiveASTVisitorTests/TraversalScope.cpp

Index: lib/ASTMatchers/ASTMatchFinder.cpp
===
--- lib/ASTMatchers/ASTMatchFinder.cpp
+++ lib/ASTMatchers/ASTMatchFinder.cpp
@@ -635,10 +635,6 @@
   bool memoizedMatchesAncestorOfRecursively(
   const ast_type_traits::DynTypedNode &Node, const DynTypedMatcher &Matcher,
   BoundNodesTreeBuilder *Builder, AncestorMatchMode MatchMode) {
-if (Node.get() ==
-ActiveASTContext->getTranslationUnitDecl())
-  return false;
-
 // For AST-nodes that don't have an identity, we can't memoize.
 if (!Builder->isComparable())
   return matchesAncestorOfRecursively(Node, Matcher, Builder, MatchMode);
@@ -673,7 +669,22 @@
 BoundNodesTreeBuilder *Builder,
 AncestorMatchMode MatchMode) {
 const auto &Parents = ActiveASTContext->getParents(Node);
-assert(!Parents.empty() && "Found node that is not in the parent map.");
+if (Parents.empty()) {
+  // Nodes may have no parents if:
+  //  a) the node is the TranslationUnitDecl
+  //  b) we have a limited traversal scope that excludes the parent edges
+  //  c) there is a bug in the AST, and the node is not reachable
+  // Usually the traversal scope is the whole AST, which precludes b.
+  // Bugs are common enough that it's worthwhile asserting when we can.
+  assert(Node.get() ||
+ /* Traversal scope is limited if none of the bounds are the TU */
+ llvm::none_of(ActiveASTContext->getTraversalScope(),
+   [](Decl *D) {
+ return D->getKind() == Decl::TranslationUnit;
+   }) &&
+ "Found node that is not in the complete parent map!");
+  return false;
+}
 if (Parents.size() == 1) {
   // Only one parent - do recursive memoization.
   const ast_type_traits::DynTypedNode Parent = Parents[0];
@@ -1019,7 +1030,7 @@
   internal::MatchASTVisitor Visitor(&Matchers, Options);
   Visitor.set_active_ast_context(&Context);
   Visitor.onStartOfTranslationUnit();
-  Visitor.TraverseDecl(Context.getTranslationUnitDecl());
+  Visitor.TraverseAST(Context);
   Visitor.onEndOfTranslationUnit();
 }
 
Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -796,11 +796,10 @@
   CommentCommandTraits(BumpAlloc, LOpts.CommentOpts),
   CompCategories(this_()), LastSDM(nullptr, 0) {
   TUDecl = TranslationUnitDecl::Create(*this);
+  TraversalScope = {TUDecl};
 }
 
 ASTContext::~ASTContext() {
-  ReleaseParentMapEntries();
-
   // Release the DenseMaps associated with DeclContext objects.
   // FIXME: Is this the ideal solution?
   ReleaseDeclContextMaps();
@@ -838,22 +837,80 @@
 Value.second->~PerModuleInitializers();
 }
 
-void ASTContext::ReleaseParentMapEntries() {
-  if (!PointerParents) return;
-  for (const auto &Entry : *PointerParents) {
-if (Entry.second.is()) {
-  delete Entry.second.get();
-} else if (Entry.second.is()) {
-  delete Entry.second.get();
-}
-  }
-  for (const auto &Entry : *OtherParents) {
-if (Entry.second.is()) {
-  delete Entry.second.get();
-} else if (Entry.second.is()) {
-  delete Entry.second.get();
-}
+class ASTContext::ParentMap {
+  /// Contains parents of a node.
+  using ParentVector = llvm::SmallVector;
+
+  /// Maps from a node to its parents. This is used for nodes that have
+  /// pointer identity only, which are more common and we can save space by
+  /// only storing a unique pointer to them.
+  using ParentMapPointers = llvm::DenseMap<
+  const void *,
+  llvm::PointerUnion4>;
+
+  /// Parent map for nodes without pointer identity. We store a full
+  /// DynTypedNode for all keys.
+  using ParentMapOtherNodes = llvm::DenseMap<
+  ast_type_traits::DynTypedNode,
+  llvm::PointerUnion4>;
+
+  ParentMapPointers PointerParents;
+  ParentMapOtherNodes OtherParents;
+  class ASTVisitor;
+
+  static ast_type_traits::DynTypedNode
+  getSingleDynTypedNodeFromParentMap(ParentMapPointers::mapped_type U) {
+if (const auto *D = U.dyn_cast())
+  return ast_type_traits::DynTypedNode::create(*

[PATCH] D54475: [clangd] Allow observation of changes to global CDBs.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/Function.h:90
+class Event {
+public:
+  // A Listener is the callback through which events are delivered.

I assume the `Event` is supposed to be used only with non-reference and 
non-const qualified types.
Maybe add a static assert for that? Something like:
`static_assert(std::is_same_v, T>)` 



Comment at: clangd/Function.h:135
+for (const auto &L : Listeners)
+  L.first(V);
+  }

As discussed offline, running the callbacks under the lock might cause 
deadlocks.
We probably won't hit this case in near future, but it would be nice to find a 
way to diagnose this situation. Not sure what we can do non-intrusively, though.



Comment at: clangd/GlobalCompilationDatabase.h:72
   tooling::CompilationDatabase *getCDBForFile(PathRef File) const;
-  tooling::CompilationDatabase *getCDBInDirLocked(PathRef File) const;
+  std::pair
+  getCDBInDirLocked(PathRef File) const;

Maybe add a `/*Cached*/`  comment here too?



Comment at: unittests/clangd/GlobalCompilationDatabaseTests.cpp:98
+Changes.push_back(ChangedFiles);
+  });
+}

Maybe add an actual test?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54475



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

[PATCH] D54519: [clangd] Fix no results returned for global symbols in dexp

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ioeric.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ilya-biryukov.

For symbols in global namespace (without any scope), we need to
add global scope "" to the fuzzy request.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54519

Files:
  clangd/index/dex/dexp/Dexp.cpp


Index: clangd/index/dex/dexp/Dexp.cpp
===
--- clangd/index/dex/dexp/Dexp.cpp
+++ clangd/index/dex/dexp/Dexp.cpp
@@ -58,6 +58,10 @@
   auto Names = splitQualifiedName(QualifiedName);
   if (IsGlobalScope || !Names.first.empty())
 Request.Scopes = {Names.first};
+  else
+// QualifiedName refers to a symbol in global scope (e.g. "GlobalSymbol"),
+// add the global scope to the request.
+Request.Scopes = {""};
 
   Request.Query = Names.second;
   std::vector SymIDs;


Index: clangd/index/dex/dexp/Dexp.cpp
===
--- clangd/index/dex/dexp/Dexp.cpp
+++ clangd/index/dex/dexp/Dexp.cpp
@@ -58,6 +58,10 @@
   auto Names = splitQualifiedName(QualifiedName);
   if (IsGlobalScope || !Names.first.empty())
 Request.Scopes = {Names.first};
+  else
+// QualifiedName refers to a symbol in global scope (e.g. "GlobalSymbol"),
+// add the global scope to the request.
+Request.Scopes = {""};
 
   Request.Query = Names.second;
   std::vector SymIDs;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54269: Introduce shard storage to auto-index.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, mostly interface tweaks now.




Comment at: clangd/index/Background.cpp:187
+  BackgroundIndexStorage *IndexStorage = getIndexStorage(Directory);
+  if (!IndexStorage)
+elog("No index storage for: {0}", Directory);

I think it would be simpler to just disallow the storage factory from returning 
null, and instead have it return a no-op implementation that logs.

Again, this moves the complexity behind an abstraction, where the rest of the 
code is simpler.



Comment at: clangd/index/Background.h:36
+  // retrieved later on with the same identifier.
+  virtual llvm::Error storeShard(llvm::StringRef ShardIdentifier,
+ IndexFileOut Shard) const = 0;

I like that the implementations treat the shard scheme as opaque, but it'd be 
good for humans to know that this is going to be a filename in practice.

I'd suggest either changing the param name or adding to the interface comment: 
"Shards of the index are stored and retrieved independently, keyed by shard 
identifier - in practice this is a source file name"



Comment at: clangd/index/Background.h:39
+
+  virtual llvm::Expected
+  loadShard(llvm::StringRef ShardIdentifier) const = 0;

docs



Comment at: clangd/index/Background.h:39
+
+  virtual llvm::Expected
+  loadShard(llvm::StringRef ShardIdentifier) const = 0;

sammccall wrote:
> docs
Hmm, we're going to attempt to load the shard corresponding to every file that 
we have in the CDB, even if the index isn't built yet. So "file doesn't exist" 
is an expected case (where we don't want to log etc), vs e.g. "file was 
corrupted" is unexpected and should definitely be logged. How do we distinguish 
these with this API?

One defensible option would be to have implementations handle errors: return 
Optional, and have the disk version log errors appropriately and 
return None.

Another would be Expected> or so, which is a little messy.



Comment at: clangd/index/Background.h:43
+  static std::unique_ptr
+  createDiskStorage(llvm::StringRef CDBDirectory);
+};

docs



Comment at: clangd/index/Background.h:48
 // all commands in a compilation database. Indexing happens in the background.
+// Takes a factory function to create IndexStorage units for each compilation
+// database. Those databases are identified by directory they are found.

nit: I'd move the description of IndexStorageFactory semantics to above the 
Using declaration - generally splitting docs to the most specific decl should 
make them easier to match up to the code.



Comment at: clangd/index/Background.h:49
+// Takes a factory function to create IndexStorage units for each compilation
+// database. Those databases are identified by directory they are found.
 // FIXME: it should also persist its state on disk for fast start.

by the directory they are found in?



Comment at: clangd/index/Background.h:103
+  // Maps CDB Directory to index storage.
+  llvm::StringMap> IndexStorageMap;
+  IndexStorageFactory IndexStorageCreator;

I do think having the factory own and cache the storages would be simpler:
- it moves more work out of this complicated class with a lot of 
responsibilities, and puts it somewhere simpler
- it would mean we can just use a single storage object in tests that don't use 
multiple CDBs.



Comment at: clangd/index/Background.h:103
+  // Maps CDB Directory to index storage.
+  llvm::StringMap> IndexStorageMap;
+  IndexStorageFactory IndexStorageCreator;

sammccall wrote:
> I do think having the factory own and cache the storages would be simpler:
> - it moves more work out of this complicated class with a lot of 
> responsibilities, and puts it somewhere simpler
> - it would mean we can just use a single storage object in tests that don't 
> use multiple CDBs.
don't accesses to this map need to be locked?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54269



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

[PATCH] D54473: [sanitizers] Initial implementation for -fsanitize=init-locals

[Alexander Potapenko via Phabricator via cfe-commits]

glider added a comment.

In https://reviews.llvm.org/D54473#1297460, @kcc wrote:

> This new flag inhibits the warnings from -Wuninitialized, right? 
>  While this is fine for experimenting (and I want to have this in ToT to 
> enable wide experimentation)
>  we should clearly state (in the comments) that the final intent is to make 
> the feature work together with -Wuninitialized.


No, as far as I can see, the warnings from -Wuninitialized are still present 
(see the test).

> Another thing that we will need (and not necessary in the first change) is to 
> have fine grained controls over what we zero-initialize. 
>  We may want to make separate decisions for pointer/non-pointer scalars, 
> PODs, arrays of {scalars, pointers, PODs}, etc.

Ok, noted.


Repository:
  rC Clang

https://reviews.llvm.org/D54473



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

[clang-tools-extra] r346852 - [clangd] Replace StringRef in SymbolLocation with a char pointer.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Wed Nov 14 03:55:45 2018
New Revision: 346852

URL: http://llvm.org/viewvc/llvm-project?rev=346852&view=rev
Log:
[clangd] Replace StringRef in SymbolLocation with a char pointer.

Summary:
This would save us 8 bytes per ref, and buy us ~40MB in total
for llvm index (from ~300MB to ~260 MB).

The char pointer must be null-terminated, and llvm::StringSaver
guarantees it.

Reviewers: sammccall

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

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

Modified:
clang-tools-extra/trunk/clangd/index/Index.cpp
clang-tools-extra/trunk/clangd/index/Index.h
clang-tools-extra/trunk/clangd/index/Merge.cpp
clang-tools-extra/trunk/clangd/index/Serialization.cpp
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
clang-tools-extra/trunk/clangd/index/YAMLSerialization.cpp
clang-tools-extra/trunk/clangd/index/dex/Dex.cpp
clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
clang-tools-extra/trunk/unittests/clangd/DexTests.cpp
clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/SerializationTests.cpp
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp

Modified: clang-tools-extra/trunk/clangd/index/Index.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Index.cpp?rev=346852&r1=346851&r2=346852&view=diff
==
--- clang-tools-extra/trunk/clangd/index/Index.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/Index.cpp Wed Nov 14 03:55:45 2018
@@ -165,7 +165,8 @@ raw_ostream &operator<<(raw_ostream &OS,
 void RefSlab::Builder::insert(const SymbolID &ID, const Ref &S) {
   auto &M = Refs[ID];
   M.push_back(S);
-  M.back().Location.FileURI = UniqueStrings.save(M.back().Location.FileURI);
+  M.back().Location.FileURI =
+  UniqueStrings.save(M.back().Location.FileURI).data();
 }
 
 RefSlab RefSlab::Builder::build() && {

Modified: clang-tools-extra/trunk/clangd/index/Index.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Index.h?rev=346852&r1=346851&r2=346852&view=diff
==
--- clang-tools-extra/trunk/clangd/index/Index.h (original)
+++ clang-tools-extra/trunk/clangd/index/Index.h Wed Nov 14 03:55:45 2018
@@ -56,14 +56,19 @@ struct SymbolLocation {
 uint32_t Column : 12; // 0-based
   };
 
-  // The URI of the source file where a symbol occurs.
-  llvm::StringRef FileURI;
-
   /// The symbol range, using half-open range [Start, End).
   Position Start;
   Position End;
 
-  explicit operator bool() const { return !FileURI.empty(); }
+  explicit operator bool() const { return !StringRef(FileURI).empty(); }
+
+  // The URI of the source file where a symbol occurs.
+  // The string must be null-terminated.
+  //
+  // We avoid using llvm::StringRef here to save memory.
+  // WARNING: unless you know what you are doing, it is recommended to use it
+  // via llvm::StringRef.
+  const char *FileURI = "";
 };
 inline bool operator==(const SymbolLocation::Position &L,
const SymbolLocation::Position &R) {
@@ -76,12 +81,16 @@ inline bool operator<(const SymbolLocati
  std::make_tuple(R.line(), R.column());
 }
 inline bool operator==(const SymbolLocation &L, const SymbolLocation &R) {
-  return std::tie(L.FileURI, L.Start, L.End) ==
- std::tie(R.FileURI, R.Start, R.End);
+  assert(L.FileURI && R.FileURI);
+  return !std::strcmp(L.FileURI, R.FileURI) &&
+ std::tie(L.Start, L.End) == std::tie(R.Start, R.End);
 }
 inline bool operator<(const SymbolLocation &L, const SymbolLocation &R) {
-  return std::tie(L.FileURI, L.Start, L.End) <
- std::tie(R.FileURI, R.Start, R.End);
+  assert(L.FileURI && R.FileURI);
+  int Cmp = std::strcmp(L.FileURI, R.FileURI);
+  if (Cmp != 0)
+return Cmp < 0;
+  return std::tie(L.Start, L.End) < std::tie(R.Start, R.End);
 }
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, const SymbolLocation &);
 
@@ -288,12 +297,19 @@ raw_ostream &operator<<(raw_ostream &, S
 template  void visitStrings(Symbol &S, const Callback &CB) {
   CB(S.Name);
   CB(S.Scope);
-  CB(S.CanonicalDeclaration.FileURI);
-  CB(S.Definition.FileURI);
   CB(S.Signature);
   CB(S.CompletionSnippetSuffix);
   CB(S.Documentation);
   CB(S.ReturnType);
+  auto RawCharPointerCB = [&CB](const char *&P) {
+llvm::StringRef S(P);
+CB(S);
+assert(!S.data()[S.size()] && "Visited StringRef must be null-terminated");
+P = S.data();
+  };
+  RawCharPointerCB(S.CanonicalDeclaration.FileURI);
+  RawCharPointerCB(S.Definition.FileURI);
+
   for (auto &Include : S.IncludeHeaders)
 CB(Include.IncludeHeader);
 

[PATCH] D53427: [clangd] Replace StringRef in SymbolLocation with a char pointer.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346852: [clangd] Replace StringRef in SymbolLocation with a 
char pointer. (authored by hokein, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D53427

Files:
  clang-tools-extra/trunk/clangd/index/Index.cpp
  clang-tools-extra/trunk/clangd/index/Index.h
  clang-tools-extra/trunk/clangd/index/Merge.cpp
  clang-tools-extra/trunk/clangd/index/Serialization.cpp
  clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
  clang-tools-extra/trunk/clangd/index/YAMLSerialization.cpp
  clang-tools-extra/trunk/clangd/index/dex/Dex.cpp
  clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
  clang-tools-extra/trunk/unittests/clangd/DexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/SerializationTests.cpp
  clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp

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
@@ -583,7 +583,7 @@
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
   auto BarURI = URI::createFile(BarHeader).toString();
   Symbol Sym = cls("ns::X");
-  Sym.CanonicalDeclaration.FileURI = BarURI;
+  Sym.CanonicalDeclaration.FileURI = BarURI.c_str();
   Sym.IncludeHeaders.emplace_back(BarURI, 1);
   // Shoten include path based on search dirctory and insert.
   auto Results = completions(Server,
@@ -614,8 +614,8 @@
   Symbol SymY = cls("ns::Y");
   std::string BarHeader = testPath("bar.h");
   auto BarURI = URI::createFile(BarHeader).toString();
-  SymX.CanonicalDeclaration.FileURI = BarURI;
-  SymY.CanonicalDeclaration.FileURI = BarURI;
+  SymX.CanonicalDeclaration.FileURI = BarURI.c_str();
+  SymY.CanonicalDeclaration.FileURI = BarURI.c_str();
   SymX.IncludeHeaders.emplace_back("", 1);
   SymY.IncludeHeaders.emplace_back("", 1);
   // Shoten include path based on search dirctory and insert.
@@ -1252,7 +1252,7 @@
 
   // Differences in header-to-insert suppress bundling.
   std::string DeclFile = URI::createFile(testPath("foo")).toString();
-  NoArgsGFunc.CanonicalDeclaration.FileURI = DeclFile;
+  NoArgsGFunc.CanonicalDeclaration.FileURI = DeclFile.c_str();
   NoArgsGFunc.IncludeHeaders.emplace_back("", 1);
   EXPECT_THAT(
   completions(Context + "int y = GFunc^", {NoArgsGFunc}, Opts).Completions,
@@ -1959,7 +1959,7 @@
 TEST(CompletionTest, InsertTheMostPopularHeader) {
   std::string DeclFile = URI::createFile(testPath("foo")).toString();
   Symbol sym = func("Func");
-  sym.CanonicalDeclaration.FileURI = DeclFile;
+  sym.CanonicalDeclaration.FileURI = DeclFile.c_str();
   sym.IncludeHeaders.emplace_back("\"foo.h\"", 2);
   sym.IncludeHeaders.emplace_back("\"bar.h\"", 1000);
 
@@ -1981,7 +1981,7 @@
 
   std::string DeclFile = URI::createFile(testPath("foo")).toString();
   Symbol sym = func("Func");
-  sym.CanonicalDeclaration.FileURI = DeclFile;
+  sym.CanonicalDeclaration.FileURI = DeclFile.c_str();
   sym.IncludeHeaders.emplace_back("\"foo.h\"", 2);
   sym.IncludeHeaders.emplace_back("\"bar.h\"", 1000);
 
Index: clang-tools-extra/trunk/unittests/clangd/SerializationTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/SerializationTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/SerializationTests.cpp
@@ -111,7 +111,7 @@
   EXPECT_EQ(Sym1.Signature, "");
   EXPECT_EQ(Sym1.Documentation, "Foo doc");
   EXPECT_EQ(Sym1.ReturnType, "int");
-  EXPECT_EQ(Sym1.CanonicalDeclaration.FileURI, "file:///path/foo.h");
+  EXPECT_EQ(StringRef(Sym1.CanonicalDeclaration.FileURI), "file:///path/foo.h");
   EXPECT_EQ(Sym1.Origin, SymbolOrigin::Static);
   EXPECT_TRUE(Sym1.Flags & Symbol::IndexedForCodeCompletion);
   EXPECT_FALSE(Sym1.Flags & Symbol::Deprecated);
@@ -122,7 +122,8 @@
   EXPECT_THAT(Sym2, QName("clang::Foo2"));
   EXPECT_EQ(Sym2.Signature, "-sig");
   EXPECT_EQ(Sym2.ReturnType, "");
-  EXPECT_EQ(Sym2.CanonicalDeclaration.FileURI, "file:///path/bar.h");
+  EXPECT_EQ(llvm::StringRef(Sym2.CanonicalDeclaration.FileURI),
+"file:///path/bar.h");
   EXPECT_FALSE(Sym2.Flags & Symbol::IndexedForCodeCompletion);
   EXPECT_TRUE(Sym2.Flags & Symbol::Deprecated);
 
@@ -134,7 +135,7 @@
testing::SizeIs(1;
   auto Ref1 = ParsedYAML->Refs->begin()->second.front();
   EXPECT_EQ(Ref1.Kind, RefKind::Reference);
-  EXPECT_EQ(Ref1.Location.FileURI, "file:///path/foo.cc");
+  EXPECT_EQ(StringRef(Ref1.Location.FileURI), "file:///path/foo.cc");
 }
 
 std::vector YAMLFromSymbols(const SymbolSlab &Slab) {
Index: clang

r346838 - [OpenCL] Fix invalid address space generation for clk_event_t

[via cfe-commits]

Author: AlexeySotkin
Date: Wed Nov 14 01:40:05 2018
New Revision: 346838

URL: http://llvm.org/viewvc/llvm-project?rev=346838&view=rev
Log:
[OpenCL] Fix invalid address space generation for clk_event_t

Summary:
Addrspace(32) was generated when putting 0 in clk_event_t * event_ret
parameter for enqueue_kernel function.

Patch by Viktoria Maksimova

Reviewers: Anastasia, yaxunl, AlexeySotkin

Reviewed By:  Anastasia, AlexeySotkin

Subscribers: cfe-commits

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

Modified:
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
cfe/trunk/test/CodeGenOpenCL/cl20-device-side-enqueue.cl

Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=346838&r1=346837&r2=346838&view=diff
==
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Wed Nov 14 01:40:05 2018
@@ -3610,7 +3610,9 @@ RValue CodeGenFunction::EmitBuiltinExpr(
   llvm::Value *ClkEvent = EmitScalarExpr(E->getArg(5));
   // Convert to generic address space.
   EventList = Builder.CreatePointerCast(EventList, EventPtrTy);
-  ClkEvent = Builder.CreatePointerCast(ClkEvent, EventPtrTy);
+  ClkEvent = ClkEvent->getType()->isIntegerTy()
+   ? Builder.CreateBitOrPointerCast(ClkEvent, EventPtrTy)
+   : Builder.CreatePointerCast(ClkEvent, EventPtrTy);
   auto Info =
   CGM.getOpenCLRuntime().emitOpenCLEnqueuedBlock(*this, E->getArg(6));
   llvm::Value *Kernel =

Modified: cfe/trunk/test/CodeGenOpenCL/cl20-device-side-enqueue.cl
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenOpenCL/cl20-device-side-enqueue.cl?rev=346838&r1=346837&r2=346838&view=diff
==
--- cfe/trunk/test/CodeGenOpenCL/cl20-device-side-enqueue.cl (original)
+++ cfe/trunk/test/CodeGenOpenCL/cl20-device-side-enqueue.cl Wed Nov 14 
01:40:05 2018
@@ -106,6 +106,13 @@ kernel void device_side_enqueue(global i
a[i] = b[i];
  });
 
+  // COMMON-LABEL: call i32 @__enqueue_kernel_basic_events
+  // COMMON-SAME: (%opencl.queue_t{{.*}}* {{%[0-9]+}}, i32 {{%[0-9]+}}, 
%struct.ndrange_t* {{.*}}, i32 1, %opencl.clk_event_t{{.*}}* addrspace(4)* 
{{%[0-9]+}}, %opencl.clk_event_t{{.*}}* addrspace(4)* null,
+  enqueue_kernel(default_queue, flags, ndrange, 1, &event_wait_list, 0,
+ ^(void) {
+   return;
+ });
+
   // Emits global block literal [[BLG1]] and block kernel [[INVGK1]].
   // COMMON: [[DEF_Q:%[0-9]+]] = load %opencl.queue_t{{.*}}*, 
%opencl.queue_t{{.*}}** %default_queue
   // COMMON: [[FLAGS:%[0-9]+]] = load i32, i32* %flags
@@ -390,7 +397,7 @@ kernel void device_side_enqueue(global i
 // COMMON: define internal spir_kernel void [[INVGK5]](i8 addrspace(4)*{{.*}}, 
i8 addrspace(3)*{{.*}})
 // COMMON: define internal spir_kernel void [[INVGK6]](i8 addrspace(4)*, i8 
addrspace(3)*, i8 addrspace(3)*, i8 addrspace(3)*) #{{[0-9]+}} {
 // COMMON: entry:
-// COMMON:  call void @__device_side_enqueue_block_invoke_8(i8 addrspace(4)* 
%0, i8 addrspace(3)* %1, i8 addrspace(3)* %2, i8 addrspace(3)* %3)
+// COMMON:  call void @__device_side_enqueue_block_invoke_9(i8 addrspace(4)* 
%0, i8 addrspace(3)* %1, i8 addrspace(3)* %2, i8 addrspace(3)* %3)
 // COMMON:  ret void
 // COMMON: }
 // COMMON: define internal spir_kernel void [[INVGK7]](i8 addrspace(4)*{{.*}}, 
i8 addrspace(3)*{{.*}})


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

[PATCH] D54475: [clangd] Allow observation of changes to global CDBs.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 174014.
sammccall marked 3 inline comments as done.
sammccall added a comment.

Address comments.
Add missing OverlayCDB->Base watching (oops!)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54475

Files:
  clangd/Function.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  unittests/clangd/GlobalCompilationDatabaseTests.cpp

Index: unittests/clangd/GlobalCompilationDatabaseTests.cpp
===
--- unittests/clangd/GlobalCompilationDatabaseTests.cpp
+++ unittests/clangd/GlobalCompilationDatabaseTests.cpp
@@ -88,6 +88,23 @@
   ElementsAre("clang", testPath("foo.cc"), "-DA=6"));
 }
 
+TEST_F(OverlayCDBTest, Watch) {
+  OverlayCDB Inner(nullptr);
+  OverlayCDB Outer(&Inner);
+
+  std::vector> Changes;
+  auto Sub = Outer.watch([&](const std::vector &ChangedFiles) {
+Changes.push_back(ChangedFiles);
+  });
+
+  Inner.setCompileCommand("A.cpp", tooling::CompileCommand());
+  Outer.setCompileCommand("B.cpp", tooling::CompileCommand());
+  Inner.setCompileCommand("A.cpp", llvm::None);
+  Outer.setCompileCommand("C.cpp", llvm::None);
+  EXPECT_THAT(Changes, ElementsAre(ElementsAre("A.cpp"), ElementsAre("B.cpp"),
+   ElementsAre("A.cpp"), ElementsAre("C.cpp")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -11,6 +11,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_GLOBALCOMPILATIONDATABASE_H
 
 #include "Path.h"
+#include "Function.h"
 #include "llvm/ADT/StringMap.h"
 #include 
 #include 
@@ -41,8 +42,15 @@
   /// Clangd should treat the results as unreliable.
   virtual tooling::CompileCommand getFallbackCommand(PathRef File) const;
 
-  /// FIXME(ibiryukov): add facilities to track changes to compilation flags of
-  /// existing targets.
+  using CommandChanged = Event>;
+  /// The callback is notified when files may have new compile commands.
+  /// The argument is a list of full file paths.
+  CommandChanged::Subscription watch(CommandChanged::Listener L) const {
+return OnCommandChanged.observe(std::move(L));
+  }
+
+protected:
+  mutable CommandChanged OnCommandChanged;
 };
 
 /// Gets compile args from tooling::CompilationDatabases built for parent
@@ -61,7 +69,8 @@
 
 private:
   tooling::CompilationDatabase *getCDBForFile(PathRef File) const;
-  tooling::CompilationDatabase *getCDBInDirLocked(PathRef File) const;
+  std::pair
+  getCDBInDirLocked(PathRef File) const;
 
   mutable std::mutex Mutex;
   /// Caches compilation databases loaded from directories(keys are
@@ -81,8 +90,7 @@
   // Base may be null, in which case no entries are inherited.
   // FallbackFlags are added to the fallback compile command.
   OverlayCDB(const GlobalCompilationDatabase *Base,
- std::vector FallbackFlags = {})
-  : Base(Base), FallbackFlags(std::move(FallbackFlags)) {}
+ std::vector FallbackFlags = {});
 
   llvm::Optional
   getCompileCommand(PathRef File) const override;
@@ -98,6 +106,7 @@
   llvm::StringMap Commands; /* GUARDED_BY(Mut) */
   const GlobalCompilationDatabase *Base;
   std::vector FallbackFlags;
+  CommandChanged::Subscription BaseChanged;
 };
 
 } // namespace clangd
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -49,17 +49,17 @@
   return None;
 }
 
-tooling::CompilationDatabase *
+std::pair
 DirectoryBasedGlobalCompilationDatabase::getCDBInDirLocked(PathRef Dir) const {
   // FIXME(ibiryukov): Invalidate cached compilation databases on changes
   auto CachedIt = CompilationDatabases.find(Dir);
   if (CachedIt != CompilationDatabases.end())
-return CachedIt->second.get();
+return {CachedIt->second.get(), true};
   std::string Error = "";
   auto CDB = tooling::CompilationDatabase::loadFromDirectory(Dir, Error);
   auto Result = CDB.get();
   CompilationDatabases.insert(std::make_pair(Dir, std::move(CDB)));
-  return Result;
+  return {Result, false};
 }
 
 tooling::CompilationDatabase *
@@ -69,14 +69,29 @@
   path::is_absolute(File, path::Style::windows)) &&
  "path must be absolute");
 
+  tooling::CompilationDatabase * CDB = nullptr;
+  bool Cached;
   std::lock_guard Lock(Mutex);
-  if (CompileCommandsDir)
-return getCDBInDirLocked(*CompileCommandsDir);
-  for (auto Path = path::parent_path(File); !Path.empty();
-   Path = path::parent_path(Path))
-if (auto CDB = getCDBInDirLocked(Path))
-  return CDB;
-  return nullptr;
+  if (CompileCommandsDir) {
+std::tie(CDB, Cached) = getCDBInDirLocked(*CompileCommandsDir);
+  } else {
+for (auto Path = path::parent_path(File); !CDB && !Path.empt

[PATCH] D54450: Get the correct range of tokens for preprocessor conditions

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos added a comment.

As far as I see GCC warns on these 3 things.




Comment at: lib/Lex/PPDirectives.cpp:553
 } else {
   const SourceLocation CondBegin = CurPPLexer->getSourceLocation();
   // Restore the value of LexingRawMode so that identifiers are

Is `CondBegin` still needed after your changes?



Comment at: lib/Lex/PPDirectives.cpp:563
   if (Callbacks) {
 const SourceLocation CondEnd = CurPPLexer->getSourceLocation();
+Callbacks->Elif(

Is `CondEnd` still needed after your changes?




Comment at: lib/Lex/PPExpressions.cpp:852
 DisableMacroExpansion = DisableMacroExpansionAtStartOfDirective;
 return {false, DT.IncludedUndefinedIds};
   }

The new `ExprRange` member is not initialized here, it seems.



https://reviews.llvm.org/D54450



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

[PATCH] D54269: Introduce shard storage to auto-index.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clangd/index/Background.h:39
+
+  virtual llvm::Expected
+  loadShard(llvm::StringRef ShardIdentifier) const = 0;

sammccall wrote:
> sammccall wrote:
> > docs
> Hmm, we're going to attempt to load the shard corresponding to every file 
> that we have in the CDB, even if the index isn't built yet. So "file doesn't 
> exist" is an expected case (where we don't want to log etc), vs e.g. "file 
> was corrupted" is unexpected and should definitely be logged. How do we 
> distinguish these with this API?
> 
> One defensible option would be to have implementations handle errors: return 
> Optional, and have the disk version log errors appropriately and 
> return None.
> 
> Another would be Expected> or so, which is a little 
> messy.
used the former proposition, but with std::unique_ptr instead of llvm::Optional 
since IndexFileIn is move-only.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54269



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

[PATCH] D54269: Introduce shard storage to auto-index.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 174017.
kadircet marked 10 inline comments as done.
kadircet added a comment.

- Address comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54269

Files:
  clangd/index/Background.cpp
  clangd/index/Background.h
  unittests/clangd/BackgroundIndexTests.cpp

Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -76,5 +76,73 @@
FileURI("unittest:///root/B.cc")}));
 }
 
+TEST(BackgroundIndexTest, ShardStorageWriteTest) {
+  class MemoryShardStorage : public BackgroundIndexStorage {
+mutable std::mutex StorageMu;
+llvm::StringMap &Storage;
+size_t &CacheHits;
+
+  public:
+MemoryShardStorage(llvm::StringMap &Storage, size_t &CacheHits)
+: Storage(Storage), CacheHits(CacheHits) {}
+llvm::Error storeShard(llvm::StringRef ShardIdentifier,
+   IndexFileOut Shard) const override {
+  std::lock_guard Lock(StorageMu);
+  std::string &str = Storage[ShardIdentifier];
+  llvm::raw_string_ostream OS(str);
+  OS << Shard;
+  OS.flush();
+  return llvm::Error::success();
+}
+std::unique_ptr
+loadShard(llvm::StringRef ShardIdentifier) const override {
+  std::lock_guard Lock(StorageMu);
+  if (Storage.find(ShardIdentifier) == Storage.end()) {
+elog("Shard {0}: not found.", ShardIdentifier);
+return nullptr;
+  }
+  auto IndexFile = readIndexFile(Storage[ShardIdentifier]);
+  if (!IndexFile) {
+elog("Error while reading {0}: {1}.", ShardIdentifier,
+ IndexFile.takeError());
+return nullptr;
+  }
+  CacheHits++;
+  return llvm::make_unique(std::move(*IndexFile));
+}
+  };
+  MockFSProvider FS;
+  FS.Files[testPath("root/A.h")] = R"cpp(
+  void common();
+  void f_b();
+  class A_CC {};
+  )cpp";
+  FS.Files[testPath("root/A.cc")] =
+  "#include \"A.h\"\nvoid g() { (void)common; }";
+
+  llvm::StringMap Storage;
+  size_t CacheHits = 0;
+  BackgroundIndex::IndexStorageFactory MemoryStorageFactory =
+  [&Storage, &CacheHits](llvm::StringRef) {
+static MemoryShardStorage MSS(Storage, CacheHits);
+return &MSS;
+  };
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
+  // Check nothing is loaded from Storage, but A.cc and A.h has been stored.
+  {
+BackgroundIndex Idx(Context::empty(), "", FS, /*URISchemes=*/{"unittest"},
+MemoryStorageFactory);
+Idx.enqueue(testPath("root"), Cmd);
+Idx.blockUntilIdleForTest();
+  }
+  EXPECT_EQ(Storage.size(), 2U);
+  EXPECT_NE(Storage.find(testPath("root/A.h")), Storage.end());
+  EXPECT_NE(Storage.find(testPath("root/A.cc")), Storage.end());
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/index/Background.h
===
--- clangd/index/Background.h
+++ clangd/index/Background.h
@@ -14,6 +14,7 @@
 #include "FSProvider.h"
 #include "index/FileIndex.h"
 #include "index/Index.h"
+#include "index/Serialization.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/SHA1.h"
@@ -27,15 +28,41 @@
 namespace clang {
 namespace clangd {
 
+// Handles storage and retrieval of index shards. Both store and load
+// operations can be called from multiple-threads concurrently.
+class BackgroundIndexStorage {
+public:
+  // Shards of the index are stored and retrieved independently, keyed by shard
+  // identifier - in practice this is a source file name
+  virtual llvm::Error storeShard(llvm::StringRef ShardIdentifier,
+ IndexFileOut Shard) const = 0;
+
+  // Tries to load shard with given identifier, returns llvm::None if shard
+  // couldn't be loaded.
+  virtual std::unique_ptr
+  loadShard(llvm::StringRef ShardIdentifier) const = 0;
+
+  // Creates an Index Storage that saves shards into disk. Index storage uses
+  // CDBDirectory + ".clangd-index/" as the folder to save shards.
+  static BackgroundIndexStorage *
+  createDiskStorage(llvm::StringRef CDBDirectory);
+};
+
 // Builds an in-memory index by by running the static indexer action over
 // all commands in a compilation database. Indexing happens in the background.
 // FIXME: it should also persist its state on disk for fast start.
 // FIXME: it should watch for changes to files on disk.
 class BackgroundIndex : public SwapIndex {
 public:
+  // A factory function used to create IndexStorage units for each compilation
+  // database. Those databases are identified by the directory they are found
+  // in. The function must be thread-safe.
+  using IndexStorageFactory =
+  std::function;
   //

[PATCH] D54204: [clangd] Initial clang-tidy diagnostics support.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Looks mostly good, just a few nits.

This patch contains two parts (clang-tidy and clangd), I think we could split 
into two,  but I'm not insisting, up to you.




Comment at: clang-tidy/modernize/LoopConvertUtils.h:59
 
   /// \brief Run the analysis on the TranslationUnitDecl.
   ///

The comment is out-of-date.



Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:518
 void SimplifyBooleanExprCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(translationUnitDecl().bind("top"), this);
+  Finder->addMatcher(matchOnce(&MatchedOnce), this);
 

maybe add a comment  describing we are trying to find the top level decl?



Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.cpp:566
 replaceCompoundReturnWithCondition(Result, Compound, true);
-  else if (const auto TU = Result.Nodes.getNodeAs("top"))
-Visitor(this, Result).TraverseDecl(const_cast(TU));
+  else // MatchOnce matcher
+Visitor(this, Result).TraverseAST(*Result.Context);

add an `assert (MatchOnce)`?



Comment at: clang-tidy/readability/SimplifyBooleanExprCheck.h:82
 
+  bool MatchedOnce = false;
   const bool ChainedConditionalReturn;

The name seems a bit unclear for readers without any context, maybe 
`FoundTopDecl`?



Comment at: clangd/ClangdUnit.cpp:158
+  // Clang-tidy has some limitiations to ensure reasonable performance:
+  //  - checks don't see all preprocessor events in the preamble
+  //  - matchers run only over the main-file top-level decls (and can't see

just want to make sure -- do we receive all preprocessor events in the main 
file when we use preamble? We have a few checks that generate `#include` 
insertion as part of FixIt.

`TestTU` doesn't use preamble, it would be nice to have a test running on a 
main file with a real preamble, but this can be a follow-up I think.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54204



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

[PATCH] D54269: Introduce shard storage to auto-index.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 174019.
kadircet added a comment.

- Get rid off getIndexStorage and use IndexStorageCreator directly.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54269

Files:
  clangd/index/Background.cpp
  clangd/index/Background.h
  unittests/clangd/BackgroundIndexTests.cpp

Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -76,5 +76,73 @@
FileURI("unittest:///root/B.cc")}));
 }
 
+TEST(BackgroundIndexTest, ShardStorageWriteTest) {
+  class MemoryShardStorage : public BackgroundIndexStorage {
+mutable std::mutex StorageMu;
+llvm::StringMap &Storage;
+size_t &CacheHits;
+
+  public:
+MemoryShardStorage(llvm::StringMap &Storage, size_t &CacheHits)
+: Storage(Storage), CacheHits(CacheHits) {}
+llvm::Error storeShard(llvm::StringRef ShardIdentifier,
+   IndexFileOut Shard) const override {
+  std::lock_guard Lock(StorageMu);
+  std::string &str = Storage[ShardIdentifier];
+  llvm::raw_string_ostream OS(str);
+  OS << Shard;
+  OS.flush();
+  return llvm::Error::success();
+}
+std::unique_ptr
+loadShard(llvm::StringRef ShardIdentifier) const override {
+  std::lock_guard Lock(StorageMu);
+  if (Storage.find(ShardIdentifier) == Storage.end()) {
+elog("Shard {0}: not found.", ShardIdentifier);
+return nullptr;
+  }
+  auto IndexFile = readIndexFile(Storage[ShardIdentifier]);
+  if (!IndexFile) {
+elog("Error while reading {0}: {1}.", ShardIdentifier,
+ IndexFile.takeError());
+return nullptr;
+  }
+  CacheHits++;
+  return llvm::make_unique(std::move(*IndexFile));
+}
+  };
+  MockFSProvider FS;
+  FS.Files[testPath("root/A.h")] = R"cpp(
+  void common();
+  void f_b();
+  class A_CC {};
+  )cpp";
+  FS.Files[testPath("root/A.cc")] =
+  "#include \"A.h\"\nvoid g() { (void)common; }";
+
+  llvm::StringMap Storage;
+  size_t CacheHits = 0;
+  BackgroundIndex::IndexStorageFactory MemoryStorageFactory =
+  [&Storage, &CacheHits](llvm::StringRef) {
+static MemoryShardStorage MSS(Storage, CacheHits);
+return &MSS;
+  };
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
+  // Check nothing is loaded from Storage, but A.cc and A.h has been stored.
+  {
+BackgroundIndex Idx(Context::empty(), "", FS, /*URISchemes=*/{"unittest"},
+MemoryStorageFactory);
+Idx.enqueue(testPath("root"), Cmd);
+Idx.blockUntilIdleForTest();
+  }
+  EXPECT_EQ(Storage.size(), 2U);
+  EXPECT_NE(Storage.find(testPath("root/A.h")), Storage.end());
+  EXPECT_NE(Storage.find(testPath("root/A.cc")), Storage.end());
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/index/Background.h
===
--- clangd/index/Background.h
+++ clangd/index/Background.h
@@ -14,6 +14,7 @@
 #include "FSProvider.h"
 #include "index/FileIndex.h"
 #include "index/Index.h"
+#include "index/Serialization.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/SHA1.h"
@@ -27,15 +28,41 @@
 namespace clang {
 namespace clangd {
 
+// Handles storage and retrieval of index shards. Both store and load
+// operations can be called from multiple-threads concurrently.
+class BackgroundIndexStorage {
+public:
+  // Shards of the index are stored and retrieved independently, keyed by shard
+  // identifier - in practice this is a source file name
+  virtual llvm::Error storeShard(llvm::StringRef ShardIdentifier,
+ IndexFileOut Shard) const = 0;
+
+  // Tries to load shard with given identifier, returns llvm::None if shard
+  // couldn't be loaded.
+  virtual std::unique_ptr
+  loadShard(llvm::StringRef ShardIdentifier) const = 0;
+
+  // Creates an Index Storage that saves shards into disk. Index storage uses
+  // CDBDirectory + ".clangd-index/" as the folder to save shards.
+  static BackgroundIndexStorage *
+  createDiskStorage(llvm::StringRef CDBDirectory);
+};
+
 // Builds an in-memory index by by running the static indexer action over
 // all commands in a compilation database. Indexing happens in the background.
 // FIXME: it should also persist its state on disk for fast start.
 // FIXME: it should watch for changes to files on disk.
 class BackgroundIndex : public SwapIndex {
 public:
+  // A factory function used to create IndexStorage units for each compilation
+  // database. Those databases are identified by the directory they are found
+  // in. The function must be thread-safe.
+  using IndexStorageFactory =
+  std::function;
 

[PATCH] D54453: Remove myself as owner of clang-query.

[Aaron Ballman via Phabricator via cfe-commits]

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

I'll go ahead and add myself as code owner. Peter can remove himself at his 
leisure.


Repository:
  rL LLVM

https://reviews.llvm.org/D54453



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

[clang-tools-extra] r346856 - Adding myself as the code owner for clang-query as discussed in https://reviews.llvm.org/D54453.

[Aaron Ballman via cfe-commits]

Author: aaronballman
Date: Wed Nov 14 05:03:50 2018
New Revision: 346856

URL: http://llvm.org/viewvc/llvm-project?rev=346856&view=rev
Log:
Adding myself as the code owner for clang-query as discussed in 
https://reviews.llvm.org/D54453.

Modified:
clang-tools-extra/trunk/CODE_OWNERS.TXT

Modified: clang-tools-extra/trunk/CODE_OWNERS.TXT
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/CODE_OWNERS.TXT?rev=346856&r1=346855&r2=346856&view=diff
==
--- clang-tools-extra/trunk/CODE_OWNERS.TXT (original)
+++ clang-tools-extra/trunk/CODE_OWNERS.TXT Wed Nov 14 05:03:50 2018
@@ -8,6 +8,10 @@ beautification by scripts.  The fields a
 (W), PGP key ID and fingerprint (P), description (D), and snail-mail address
 (S).
 
+N: Aaron Ballman
+E: aa...@aaronballman.com
+D: clang-query
+
 N: Peter Collingbourne
 E: pe...@pcc.me.uk
 D: clang-query


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

[PATCH] D54473: [sanitizers] Initial implementation for -fsanitize=init-locals

[Alexander Potapenko via Phabricator via cfe-commits]

glider added inline comments.



Comment at: include/clang/Basic/Sanitizers.def:163
+// Initialize local variables.
+SANITIZER("init-locals", InitLocals)
+

lebedev.ri wrote:
> Unless i'm mistaken, I suspect you may see some surprising behavior here, 
> unfortunately.
> [[ https://bugs.llvm.org/show_bug.cgi?id=39425 | Bug 39425 - SanitizerOrdinal 
> is out of bits ]]
Um, this is unfortunate.
We don't strictly need this feature to live under -fsanitize= flag, although 
having the corresponding attribute could've been handy.


Repository:
  rC Clang

https://reviews.llvm.org/D54473



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

[PATCH] D54450: Get the correct range of tokens for preprocessor conditions

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos added a comment.

Oh, and running the `check-clang-tools` target, this currently results in:

  Failing Tests (3):
  Clang Tools :: modularize/ProblemsInconsistent.modularize
  Clang Tools :: pp-trace/pp-trace-conditional.cpp
  Clang Tools :: pp-trace/pp-trace-macro.cpp

Perhaps the pp-trace one just has to be updated for the new behavior. :-)


https://reviews.llvm.org/D54450



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

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:79-81
+  // TODO: Provide an automatic fix if the number is exactly representable in 
the destination type.
+  f += 2.0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 
constant 'double' to 'float' [cppcoreguidelines-narrowing-conversions]

This is not a narrowing conversion -- there should be no diagnostic here. See 
[dcl.init.list]p7.



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:99
+void narrow_integer_to_floating() {
+  {
+long long ll; // 64 bits

Missing tests involving literals, which are not narrowing conversions. e.g., 
`float f = 1ULL;` should not diagnose.



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:174
+  c = uc;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 
'unsigned char' to 'char' is implementation-defined 
[cppcoreguidelines-narrowing-conversions]
+  c = us;

This is only true on architectures where `char` is defined to be `signed char` 
rather than `unsigned char`.



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:188
+  i = l;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'long' 
to 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  i = ll;

This is only narrowing if `sizeof(int) < sizeof(long)` which is not the case on 
all architectures.



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:211
+  ll = ul;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 
'unsigned long' to 'long long' is implementation-defined 
[cppcoreguidelines-narrowing-conversions]
+  ll = ull;

Wha? How does one narrow from an unsigned 32-bit number to a signed 64-bit 
number?



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:228
+void narrow_constant_to_signed_integer_is_not_ok() {
+  char c1 = -128;
+  char c2 = 127;

This may be narrowing on an implementation that defines `char` to be `unsigned`.



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:232
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: narrowing conversion from 
constant value -129 (0xFF7F) of type 'int' to 'char' is 
implementation-defined [cppcoreguidelines-narrowing-conversions]
+  char c4 = 128;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: narrowing conversion from 
constant value 128 (0x0080) of type 'int' to 'char' is 
implementation-defined [cppcoreguidelines-narrowing-conversions]

This may be fine on implementations that define `char` to be `unsigned`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488



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

[PATCH] D53427: [clangd] Replace StringRef in SymbolLocation with a char pointer.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tools-extra/trunk/clangd/index/Index.h:71
+  // via llvm::StringRef.
+  const char *FileURI = "";
 };

If the users of the SymbolLocation have a way to get a common "context", you 
could keep a list of filenames in this context and just store an index into 
this list (which could be even shorter). If the structure is used where no 
context is available, you could still keep a list of StringRefs somewhere 
(probably close to where the string data pointed by the StringRefs lives) and 
keep a `StringRef *` here. At a cost of one indirection you'd avoid calling 
strlen.

Not sure what trade-off is better for this particular use case though.


Repository:
  rL LLVM

https://reviews.llvm.org/D53427



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

[PATCH] D54404: Exclude matchers which can have multiple results

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:606
+  // be used with particular arguments.
+  static std::vector ExcludedMatchers{
+  "allOf",

sbenza wrote:
> I don't think we are allowed to have non-trivial static storage duration 
> objects like this.
> You have to use llvm::ManagedStatic. Or just make it a raw array.
IIRC, that's only needed for global statics, not local statics. Local statics 
are less terrifying than their global brethren. 


Repository:
  rC Clang

https://reviews.llvm.org/D54404



___
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 added a comment.

In https://reviews.llvm.org/D41005#1295632, @ilya-biryukov wrote:

> > Before this change, this was not a problem because OverriddenFiles were 
> > keyed on Status.getUniqueID(). Starting with this change, the key is the 
> > file path.
>
> I suggest keeping two maps for overridden files: one for existing files (key 
> is UniqueID), another one for the ones that don't exist (key is the file 
> path).


Done.

>> Is there a nice way to map different file paths of the same file to the same 
>> id without touching the real file system? Would it be sufficient to 
>> normalize the file paths? If yes, is there a utility function for this 
>> (can't find it right now).
> 
> I don't think there is one, the unique ids are closely tied to the 
> filesystem. IIUC the unique ids are the same whenever two different paths are 
> symlinks (or hardlinks?) pointing to the same physical file and there's no 
> way to tell if they're the same without resolving the symlink.

OK, so if the unsaved file is not being backed up by a real file on disk and 
symlinks are used, we can't do much about it.
I've normalized the file paths to handle at least that case where they might 
differ.


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] D54523: [Preamble] Reuse preamble even if an unsaved file does not exist

[Nikolai Kosjar via Phabricator via cfe-commits]

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

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/D54523

Files:
  include/clang/Frontend/ASTUnit.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
@@ -53,7 +53,10 @@
   FileSystemOptions FSOpts;
 
 public:
-  void SetUp() override {
+  void SetUp() override { ResetVFS(); }
+  void TearDown() override {}
+
+  void ResetVFS() {
 VFS = new ReadCountingInMemoryFileSystem();
 // We need the working directory to be set to something absolute,
 // otherwise it ends up being inadvertently set to the current
@@ -64,9 +67,6 @@
 VFS->setCurrentWorkingDirectory("//./");
   }
 
-  void TearDown() override {
-  }
-
   void AddFile(const std::string &Filename, const std::string &Contents) {
 ::time_t now;
 ::time(&now);
@@ -124,6 +124,72 @@
   }
 };
 
+TEST_F(PCHPreambleTest, ReparseReusesPreambleWithUnsavedFileNotExistingOnDisk) {
+  std::string Header1 = "//./header1.h";
+  std::string MainName = "//./main.cpp";
+  AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+  RemapFile(Header1, "#define ZERO 0\n");
+
+  // Parse with header file provided as unsaved file, which does not exist on
+  // disk.
+  std::unique_ptr AST(ParseAST(MainName));
+  ASSERT_TRUE(AST.get());
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  // Reparse and check that the preamble was reused.
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_EQ(AST->getPreambleCounter(), 1U);
+}
+
+TEST_F(PCHPreambleTest, ReparseReusesPreambleAfterUnsavedFileWasCreatedOnDisk) {
+  std::string Header1 = "//./header1.h";
+  std::string MainName = "//./main.cpp";
+  AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+  RemapFile(Header1, "#define ZERO 0\n");
+
+  // Parse with header file provided as unsaved file, which does not exist on
+  // disk.
+  std::unique_ptr AST(ParseAST(MainName));
+  ASSERT_TRUE(AST.get());
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  // Create the unsaved file also on disk and check that preamble was reused.
+  AddFile(Header1, "#define ZERO 0\n");
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_EQ(AST->getPreambleCounter(), 1U);
+}
+
+TEST_F(PCHPreambleTest,
+   ReparseReusesPreambleAfterUnsavedFileWasRemovedFromDisk) {
+  std::string Header1 = "//./foo/../header1.h";
+  std::string MainName = "//./main.cpp";
+  std::string MainFileContent = R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp";
+  AddFile(MainName, MainFileContent);
+  AddFile(Header1, "#define ZERO 0\n");
+  RemapFile(Header1, "#define ZERO 0\n");
+
+  // Parse with header file provided as unsaved file, which exists on disk.
+  std::unique_ptr AST(ParseAST(MainName));
+  ASSERT_TRUE(AST.get());
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+  ASSERT_EQ(AST->getPreambleCounter(), 1U);
+
+  // Remove the unsaved file from disk and check that the preamble was reused.
+  ResetVFS();
+  AddFile(MainName, MainFileContent);
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_EQ(AST->getPreambleCounter(), 1U);
+}
+
 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
@@ -28,6 +28,7 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/MutexGuard.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include 
@@ -221,6 +222,14 @@
   return true;
 }
 
+template 
+llvm::SmallString<256> PathNormalized(IteratorType Start, IteratorType End) {
+  llvm::SmallString<256> Path{Start, End};
+  llvm::sys::path::remove_dots(Path, /*remove_dot_dots=*/true);
+  Path = llvm::sys::path::convert_to_slash(Path);
+  return Path;
+}
+
 } // na

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

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



Comment at: clang-tidy/readability/ReadabilityTidyModule.cpp:84
+CheckFactories.registerCheck(
+"readability-redundant-preprocessor");
 CheckFactories.registerCheck(

aaron.ballman wrote:
> Please keep this list sorted alphabetically.
Done.



Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:56-59
+StringRef SourceText =
+Lexer::getSourceText(CharSourceRange::getTokenRange(ConditionRange),
+ PP.getSourceManager(), PP.getLangOpts());
+std::string Condition = getCondition(SourceText);

aaron.ballman wrote:
> Szelethus wrote:
> > vmiklos wrote:
> > > Szelethus wrote:
> > > > I'm a little confused. To me, it seems like you acquired the condition 
> > > > already -- doesn't `ConditionRange` actually cover the, well, condition 
> > > > range? This is how I imagined it:
> > > > 
> > > > ```
> > > > #ifdef CUTE_PANDA_CUBS
> > > >^~~
> > > >ConditionRange
> > > > ```
> > > > Why is there a need for `getCondition`? Is there any? If there is 
> > > > (maybe the acquired text contains other things), can you document it? I 
> > > > haven't played with `PPCallbacks` much, so I'm fine with being in the 
> > > > wrong.
> > > ConditionRange covers more than what you expect:
> > > 
> > > ```
> > > #if FOO == 4
> > >^
> > > void f();
> > > ~
> > > #endif
> > > ~~
> > > ```
> > > 
> > > to find out if the condition of the `#if` is the same as a previous one, 
> > > I want to extract just `FOO == 4` from that, then deal with that part 
> > > similar to `#ifdef` and `#ifndef`, which are easier as you have a single 
> > > Token for the condition. But you're right, I should add a comment 
> > > explaining this.
> > Oh my god. There is no tool or a convenient method for this??? I had the 
> > displeasure of working with the preprocessor in the last couple months, and 
> > I get shocked by things like this almost every day.
> > 
> > Yea, unfortunately you will have to write excessive amount of comments to 
> > counterweights the shortcomings of `Preprocessor` :/
> This is working around a bug, and I think it would be better to fix that bug 
> instead of jump through these hoops here.
> 
> `Preprocessor::EvaluateDirectiveExpression()` needs to squirrel away the 
> condition range in the `DirectiveEvalResult` it returns. I'll take a stab at 
> it and report back.
Thanks! I've now rebased this on top of D54450, to be able to drop the ugly 
`getCondition()` function.


https://reviews.llvm.org/D54349



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Miklos Vajna via Phabricator via cfe-commits]

vmiklos updated this revision to Diff 174021.
vmiklos marked 2 inline comments as done.

https://reviews.llvm.org/D54349

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.cpp
  clang-tidy/readability/RedundantPreprocessorCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-redundant-preprocessor.rst
  test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
  test/clang-tidy/readability-redundant-preprocessor.cpp
  test/clang-tidy/readability-redundant-preprocessor.h

Index: test/clang-tidy/readability-redundant-preprocessor.h
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.h
@@ -0,0 +1,5 @@
+#ifndef FOO
+#ifndef FOO // this would warn, but not in a header
+void f();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.cpp
@@ -0,0 +1,76 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -I %S
+
+// Positive testing.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifndef was here
+void f();
+#endif
+#endif
+
+// Positive testing of inverted condition.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifndef was here
+void f2();
+#endif
+#endif
+
+// Negative testing.
+#include "readability-redundant-preprocessor.h"
+
+#ifndef BAR
+void g();
+#endif
+
+#ifndef FOO
+#ifndef BAR
+void h();
+#endif
+#endif
+
+#ifndef FOO
+#ifdef BAR
+void i();
+#endif
+#endif
+
+// Positive #if testing.
+#define FOO 4
+
+#if FOO == 4
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant if; consider removing it [readability-redundant-preprocessor]
+#if FOO == 4
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous if was here
+void j();
+#endif
+#endif
+
+#if FOO == \
+4
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant if; consider removing it [readability-redundant-preprocessor]
+#if FOO == \
+4
+// CHECK-NOTES: [[@LINE-5]]:2: note: previous if was here
+void j();
+#endif
+#endif
+
+// Negative #if testing.
+#define BAR 4
+
+#if FOO == 4
+#if BAR == 4
+void k();
+#endif
+#endif
+
+#if FOO == \
+4
+#if BAR == \
+5
+void k();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
@@ -0,0 +1,36 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -DFOO
+
+// Positive testing.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifdef was here
+void f();
+#endif
+#endif
+
+// Positive testing of inverted condition.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifdef was here
+void f2();
+#endif
+#endif
+
+// Negative testing.
+#ifdef BAR
+void g();
+#endif
+
+#ifdef FOO
+#ifdef BAR
+void h();
+#endif
+#endif
+
+#ifdef FOO
+#ifndef BAR
+void i();
+#endif
+#endif
Index: docs/clang-tidy/checks/readability-redundant-preprocessor.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-redundant-preprocessor.rst
@@ -0,0 +1,61 @@
+.. title:: clang-tidy - readability-redundant-preprocessor
+
+readability-redundant-preprocessor
+==
+
+Finds potentially redundant preprocessor directives. At the moment the
+following cases are detected:
+
+* `#ifdef` .. `#endif` pairs which are nested inside an outer pair with the
+  same condition. For example:
+
+.. code-block:: c++
+
+  #ifdef FOO
+  #ifdef FOO // inner ifdef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* Same for `#ifndef` .. `#endif` pairs. For example:
+
+.. code-block:: c++
+
+  #ifndef FOO
+  #ifndef FOO // inner ifndef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* `#ifndef` inside an `#ifdef` with the same condition:
+
+.. code-block:: c++
+
+  #ifdef FOO
+  #ifndef FOO // inner ifndef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* `#ifdef` inside an `#ifndef` with the same condition:
+
+.. code-block:: c++
+
+  #ifndef FOO
+  #ifdef FOO // inner ifdef is considered redundant
+  void f();
+  #endif
+  #endif
+
+* `#if` .. `

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

[Nikolai Kosjar via Phabricator via cfe-commits]

nik updated this revision to Diff 174022.
nik added a comment.

Addressed comments.


Repository:
  rC Clang

https://reviews.llvm.org/D41005

Files:
  include/clang/Frontend/ASTUnit.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
@@ -53,7 +53,10 @@
   FileSystemOptions FSOpts;
 
 public:
-  void SetUp() override {
+  void SetUp() override { ResetVFS(); }
+  void TearDown() override {}
+
+  void ResetVFS() {
 VFS = new ReadCountingInMemoryFileSystem();
 // We need the working directory to be set to something absolute,
 // otherwise it ends up being inadvertently set to the current
@@ -64,9 +67,6 @@
 VFS->setCurrentWorkingDirectory("//./");
   }
 
-  void TearDown() override {
-  }
-
   void AddFile(const std::string &Filename, const std::string &Contents) {
 ::time_t now;
 ::time(&now);
@@ -124,6 +124,72 @@
   }
 };
 
+TEST_F(PCHPreambleTest, ReparseReusesPreambleWithUnsavedFileNotExistingOnDisk) {
+  std::string Header1 = "//./header1.h";
+  std::string MainName = "//./main.cpp";
+  AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+  RemapFile(Header1, "#define ZERO 0\n");
+
+  // Parse with header file provided as unsaved file, which does not exist on
+  // disk.
+  std::unique_ptr AST(ParseAST(MainName));
+  ASSERT_TRUE(AST.get());
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  // Reparse and check that the preamble was reused.
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_EQ(AST->getPreambleCounter(), 1U);
+}
+
+TEST_F(PCHPreambleTest, ReparseReusesPreambleAfterUnsavedFileWasCreatedOnDisk) {
+  std::string Header1 = "//./header1.h";
+  std::string MainName = "//./main.cpp";
+  AddFile(MainName, R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp");
+  RemapFile(Header1, "#define ZERO 0\n");
+
+  // Parse with header file provided as unsaved file, which does not exist on
+  // disk.
+  std::unique_ptr AST(ParseAST(MainName));
+  ASSERT_TRUE(AST.get());
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  // Create the unsaved file also on disk and check that preamble was reused.
+  AddFile(Header1, "#define ZERO 0\n");
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_EQ(AST->getPreambleCounter(), 1U);
+}
+
+TEST_F(PCHPreambleTest,
+   ReparseReusesPreambleAfterUnsavedFileWasRemovedFromDisk) {
+  std::string Header1 = "//./foo/../header1.h";
+  std::string MainName = "//./main.cpp";
+  std::string MainFileContent = R"cpp(
+#include "//./header1.h"
+int main() { return ZERO; }
+)cpp";
+  AddFile(MainName, MainFileContent);
+  AddFile(Header1, "#define ZERO 0\n");
+  RemapFile(Header1, "#define ZERO 0\n");
+
+  // Parse with header file provided as unsaved file, which exists on disk.
+  std::unique_ptr AST(ParseAST(MainName));
+  ASSERT_TRUE(AST.get());
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+  ASSERT_EQ(AST->getPreambleCounter(), 1U);
+
+  // Remove the unsaved file from disk and check that the preamble was reused.
+  ResetVFS();
+  AddFile(MainName, MainFileContent);
+  ASSERT_TRUE(ReparseAST(AST));
+  ASSERT_EQ(AST->getPreambleCounter(), 1U);
+}
+
 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
@@ -28,6 +28,7 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/MutexGuard.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include 
@@ -221,6 +222,14 @@
   return true;
 }
 
+template 
+llvm::SmallString<256> PathNormalized(IteratorType Start, IteratorType End) {
+  llvm::SmallString<256> Path{Start, End};
+  llvm::sys::path::remove_dots(Path, /*remove_dot_dots=*/true);
+  Path = llvm::sys::path::convert_to_slash(Path);
+  return Path;
+}
+
 } // namespace
 
 PreambleBounds clang::ComputePreambleBounds(const LangOptions &LangOpts,
@@ -367,13 +376,15 @@
 const FileEntry *File = Clang->getFileManager().getFile(Filename);
 if (!File || File == SourceMgr.getFileEntryForID(SourceMgr.getMainFileID()))
   continue;
+auto FilePath =
+PathNormalized(File->getName().begin(), File->getName().end());
 if (time_t ModTime = File->getModificationTime()) {
-  FilesInPreamble[File->getName()] =
+  FilesInPreamble[FilePath] =
   PrecompiledPreamble::PreambleFileHash::createForFile(File->getSize(),
ModTime);
 } else {
   

[PATCH] D51575: [clang-tidy/checks] Implement a clang-tidy check to verify Google Objective-C function naming conventions 📜

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D51575#1297664, @stephanemoore wrote:

> Thanks for the review everyone!
>
> Let me know if there are any further changes that you want me to make or any 
> further action required on my part to land this 😁


It's good to go in -- do you have commit access, or would you like someone to 
land this for you?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51575



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

[PATCH] D54524: [AST] Pack UnaryOperator

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno created this revision.
riccibruno added a reviewer: dblaikie.
riccibruno added a project: clang.
Herald added a subscriber: cfe-commits.

Use the newly available space in the bit-fields of `Stmt`
to store some data from `UnaryOperator`.
This saves 8 bytes per `UnaryOperator`.


Repository:
  rC Clang

https://reviews.llvm.org/D54524

Files:
  include/clang/AST/Expr.h
  include/clang/AST/Stmt.h

Index: include/clang/AST/Stmt.h
===
--- include/clang/AST/Stmt.h
+++ include/clang/AST/Stmt.h
@@ -374,6 +374,17 @@
 unsigned Kind : 3;
   };
 
+  class UnaryOperatorBitfields {
+friend class UnaryOperator;
+
+unsigned : NumExprBits;
+
+unsigned Opc : 5;
+unsigned CanOverflow : 1;
+
+SourceLocation Loc;
+  };
+
   class UnaryExprOrTypeTraitExprBitfields {
 friend class UnaryExprOrTypeTraitExpr;
 
@@ -513,6 +524,7 @@
 DeclRefExprBitfields DeclRefExprBits;
 FloatingLiteralBitfields FloatingLiteralBits;
 CharacterLiteralBitfields CharacterLiteralBits;
+UnaryOperatorBitfields UnaryOperatorBits;
 UnaryExprOrTypeTraitExprBitfields UnaryExprOrTypeTraitExprBits;
 CallExprBitfields CallExprBits;
 CastExprBitfields CastExprBits;
Index: include/clang/AST/Expr.h
===
--- include/clang/AST/Expr.h
+++ include/clang/AST/Expr.h
@@ -1869,47 +1869,50 @@
 ///   later returns zero in the type of the operand.
 ///
 class UnaryOperator : public Expr {
+  Stmt *Val;
+
 public:
   typedef UnaryOperatorKind Opcode;
 
-private:
-  unsigned Opc : 5;
-  unsigned CanOverflow : 1;
-  SourceLocation Loc;
-  Stmt *Val;
-public:
   UnaryOperator(Expr *input, Opcode opc, QualType type, ExprValueKind VK,
 ExprObjectKind OK, SourceLocation l, bool CanOverflow)
   : Expr(UnaryOperatorClass, type, VK, OK,
  input->isTypeDependent() || type->isDependentType(),
  input->isValueDependent(),
  (input->isInstantiationDependent() ||
   type->isInstantiationDependentType()),
  input->containsUnexpandedParameterPack()),
-Opc(opc), CanOverflow(CanOverflow), Loc(l), Val(input) {}
+Val(input) {
+UnaryOperatorBits.Opc = opc;
+UnaryOperatorBits.CanOverflow = CanOverflow;
+UnaryOperatorBits.Loc = l;
+  }
 
   /// Build an empty unary operator.
-  explicit UnaryOperator(EmptyShell Empty)
-: Expr(UnaryOperatorClass, Empty), Opc(UO_AddrOf) { }
+  explicit UnaryOperator(EmptyShell Empty) : Expr(UnaryOperatorClass, Empty) {
+UnaryOperatorBits.Opc = UO_AddrOf;
+  }
 
-  Opcode getOpcode() const { return static_cast(Opc); }
-  void setOpcode(Opcode O) { Opc = O; }
+  Opcode getOpcode() const {
+return static_cast(UnaryOperatorBits.Opc);
+  }
+  void setOpcode(Opcode Opc) { UnaryOperatorBits.Opc = Opc; }
 
   Expr *getSubExpr() const { return cast(Val); }
   void setSubExpr(Expr *E) { Val = E; }
 
   /// getOperatorLoc - Return the location of the operator.
-  SourceLocation getOperatorLoc() const { return Loc; }
-  void setOperatorLoc(SourceLocation L) { Loc = L; }
+  SourceLocation getOperatorLoc() const { return UnaryOperatorBits.Loc; }
+  void setOperatorLoc(SourceLocation L) { UnaryOperatorBits.Loc = L; }
 
   /// Returns true if the unary operator can cause an overflow. For instance,
   ///   signed int i = INT_MAX; i++;
   ///   signed char c = CHAR_MAX; c++;
   /// Due to integer promotions, c++ is promoted to an int before the postfix
   /// increment, and the result is an int that cannot overflow. However, i++
   /// can overflow.
-  bool canOverflow() const { return CanOverflow; }
-  void setCanOverflow(bool C) { CanOverflow = C; }
+  bool canOverflow() const { return UnaryOperatorBits.CanOverflow; }
+  void setCanOverflow(bool C) { UnaryOperatorBits.CanOverflow = C; }
 
   /// isPostfix - Return true if this is a postfix operation, like x++.
   static bool isPostfix(Opcode Op) {
@@ -1961,12 +1964,12 @@
   static OverloadedOperatorKind getOverloadedOperator(Opcode Opc);
 
   SourceLocation getBeginLoc() const LLVM_READONLY {
-return isPostfix() ? Val->getBeginLoc() : Loc;
+return isPostfix() ? Val->getBeginLoc() : getOperatorLoc();
   }
   SourceLocation getEndLoc() const LLVM_READONLY {
-return isPostfix() ? Loc : Val->getEndLoc();
+return isPostfix() ? getOperatorLoc() : Val->getEndLoc();
   }
-  SourceLocation getExprLoc() const LLVM_READONLY { return Loc; }
+  SourceLocation getExprLoc() const { return getOperatorLoc(); }
 
   static bool classof(const Stmt *T) {
 return T->getStmtClass() == UnaryOperatorClass;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54161: [AST] Pack DeclRefExpr, UnaryOperator, MemberExpr and BinaryOperator

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno abandoned this revision.
riccibruno added a comment.

Broken into pieces and sent individually for review.


Repository:
  rC Clang

https://reviews.llvm.org/D54161



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

[PATCH] D53427: [clangd] Replace StringRef in SymbolLocation with a char pointer.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/trunk/clangd/index/Index.h:71
+  // via llvm::StringRef.
+  const char *FileURI = "";
 };

alexfh wrote:
> If the users of the SymbolLocation have a way to get a common "context", you 
> could keep a list of filenames in this context and just store an index into 
> this list (which could be even shorter). If the structure is used where no 
> context is available, you could still keep a list of StringRefs somewhere 
> (probably close to where the string data pointed by the StringRefs lives) and 
> keep a `StringRef *` here. At a cost of one indirection you'd avoid calling 
> strlen.
> 
> Not sure what trade-off is better for this particular use case though.
`SymbolLocation` is a public interface, and the underlying data is stored in 
arena (which is not visible to clients), so clients don't know any `Context` 
except `SymbolLocation`.

 Using `StringRef *` is an interesting idea -- keeping a list of `StringRefs` 
needs extra space, the number should be relatively small (only the number of 
different files), so we will spend  extra ` * sizeof(StringRef)` 
bytes (for llvm, ~128 KB memory) , compared with the raw pointer solution.


Repository:
  rL LLVM

https://reviews.llvm.org/D53427



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

[PATCH] D54525: [AST] Pack MemberExpr

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno created this revision.
riccibruno added a reviewer: dblaikie.
riccibruno added a project: clang.
Herald added a subscriber: cfe-commits.

Use the newly available space in the bit-fields of `Stmt`
to store some data from `MemberExpr`. This saves
one pointer per `MemberExpr`.


Repository:
  rC Clang

https://reviews.llvm.org/D54525

Files:
  include/clang/AST/Expr.h
  include/clang/AST/Stmt.h
  lib/AST/Expr.cpp
  lib/Serialization/ASTWriterStmt.cpp

Index: lib/Serialization/ASTWriterStmt.cpp
===
--- lib/Serialization/ASTWriterStmt.cpp
+++ lib/Serialization/ASTWriterStmt.cpp
@@ -655,8 +655,8 @@
   if (E->hasQualifier())
 Record.AddNestedNameSpecifierLoc(E->getQualifierLoc());
 
-  Record.push_back(E->HasTemplateKWAndArgsInfo);
-  if (E->HasTemplateKWAndArgsInfo) {
+  Record.push_back(E->hasTemplateKWAndArgsInfo());
+  if (E->hasTemplateKWAndArgsInfo()) {
 Record.AddSourceLocation(E->getTemplateKeywordLoc());
 unsigned NumTemplateArgs = E->getNumTemplateArgs();
 Record.push_back(NumTemplateArgs);
Index: lib/AST/Expr.cpp
===
--- lib/AST/Expr.cpp
+++ lib/AST/Expr.cpp
@@ -1528,15 +1528,16 @@
  QualifierLoc.getNestedNameSpecifier()->isInstantiationDependent())
   E->setInstantiationDependent(true);
 
-E->HasQualifierOrFoundDecl = true;
+E->MemberExprBits.HasQualifierOrFoundDecl = true;
 
 MemberExprNameQualifier *NQ =
 E->getTrailingObjects();
 NQ->QualifierLoc = QualifierLoc;
 NQ->FoundDecl = founddecl;
   }
 
-  E->HasTemplateKWAndArgsInfo = (targs || TemplateKWLoc.isValid());
+  E->MemberExprBits.HasTemplateKWAndArgsInfo =
+  (targs || TemplateKWLoc.isValid());
 
   if (targs) {
 bool Dependent = false;
Index: include/clang/AST/Stmt.h
===
--- include/clang/AST/Stmt.h
+++ include/clang/AST/Stmt.h
@@ -402,6 +402,35 @@
 unsigned NumPreArgs : 1;
   };
 
+  class MemberExprBitfields {
+friend class MemberExpr;
+
+unsigned : NumExprBits;
+
+/// IsArrow - True if this is "X->F", false if this is "X.F".
+unsigned IsArrow : 1;
+
+/// True if this member expression used a nested-name-specifier to
+/// refer to the member, e.g., "x->Base::f", or found its member via
+/// a using declaration.  When true, a MemberExprNameQualifier
+/// structure is allocated immediately after the MemberExpr.
+unsigned HasQualifierOrFoundDecl : 1;
+
+/// True if this member expression specified a template keyword
+/// and/or a template argument list explicitly, e.g., x->f,
+/// x->template f, x->template f.
+/// When true, an ASTTemplateKWAndArgsInfo structure and its
+/// TemplateArguments (if any) are present.
+unsigned HasTemplateKWAndArgsInfo : 1;
+
+/// True if this member expression refers to a method that
+/// was resolved from an overloaded set having size greater than 1.
+unsigned HadMultipleCandidates : 1;
+
+/// This is the location of the -> or . in the expression.
+SourceLocation OperatorLoc;
+  };
+
   class CastExprBitfields {
 friend class CastExpr;
 friend class ImplicitCastExpr;
@@ -527,6 +556,7 @@
 UnaryOperatorBitfields UnaryOperatorBits;
 UnaryExprOrTypeTraitExprBitfields UnaryExprOrTypeTraitExprBits;
 CallExprBitfields CallExprBits;
+MemberExprBitfields MemberExprBits;
 CastExprBitfields CastExprBits;
 InitListExprBitfields InitListExprBits;
 PseudoObjectExprBitfields PseudoObjectExprBits;
Index: include/clang/AST/Expr.h
===
--- include/clang/AST/Expr.h
+++ include/clang/AST/Expr.h
@@ -2559,6 +2559,10 @@
   private llvm::TrailingObjects {
+  friend class ASTReader;
+  friend class ASTStmtWriter;
+  friend TrailingObjects;
+
   /// Base - the expression for the base pointer or structure references.  In
   /// X.F, this is "X".
   Stmt *Base;
@@ -2574,35 +2578,20 @@
   /// MemberLoc - This is the location of the member name.
   SourceLocation MemberLoc;
 
-  /// This is the location of the -> or . in the expression.
-  SourceLocation OperatorLoc;
-
-  /// IsArrow - True if this is "X->F", false if this is "X.F".
-  bool IsArrow : 1;
-
-  /// True if this member expression used a nested-name-specifier to
-  /// refer to the member, e.g., "x->Base::f", or found its member via a using
-  /// declaration.  When true, a MemberExprNameQualifier
-  /// structure is allocated immediately after the MemberExpr.
-  bool HasQualifierOrFoundDecl : 1;
-
-  /// True if this member expression specified a template keyword
-  /// and/or a template argument list explicitly, e.g., x->f,
-  /// x->template f, x->template f.
-  /// When true, an ASTTemplateKWAndArgsInfo structure and its
-  /// TemplateArguments (if any) are present.
-  bool HasTemplateKWAndArgsInfo : 1;
-
-  /// True if this

[PATCH] D53427: [clangd] Replace StringRef in SymbolLocation with a char pointer.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tools-extra/trunk/clangd/index/Index.h:71
+  // via llvm::StringRef.
+  const char *FileURI = "";
 };

hokein wrote:
> alexfh wrote:
> > If the users of the SymbolLocation have a way to get a common "context", 
> > you could keep a list of filenames in this context and just store an index 
> > into this list (which could be even shorter). If the structure is used 
> > where no context is available, you could still keep a list of StringRefs 
> > somewhere (probably close to where the string data pointed by the 
> > StringRefs lives) and keep a `StringRef *` here. At a cost of one 
> > indirection you'd avoid calling strlen.
> > 
> > Not sure what trade-off is better for this particular use case though.
> `SymbolLocation` is a public interface, and the underlying data is stored in 
> arena (which is not visible to clients), so clients don't know any `Context` 
> except `SymbolLocation`.
> 
>  Using `StringRef *` is an interesting idea -- keeping a list of `StringRefs` 
> needs extra space, the number should be relatively small (only the number of 
> different files), so we will spend  extra ` * 
> sizeof(StringRef)` bytes (for llvm, ~128 KB memory) , compared with the raw 
> pointer solution.
Another option mentioned by Ilya offline is similar to MS's BSTR: keep the 
length along with the string data in the arena. Something like:

```
class InlineString {
  size_t Length;
  char Data[0];  // Open-ended array with a null-terminated string in it.
 public:
   operator StringRef() const { return StringRef(&Data[0], Length); }
   ...
};
```

And then keep `const InlineString *` here.


Repository:
  rL LLVM

https://reviews.llvm.org/D53427



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

[PATCH] D52835: [Diagnostics] Check integer to floating point number implicit conversions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/Sema/SemaChecking.cpp:113-115
+static void PrettyPrintFloat(const llvm::APFloat &floatValue,
+ const llvm::fltSemantics &floatSem,
+ SmallVectorImpl &prettyFloatValue) {

Parameter names should be in PascalCase, not camelCase.



Comment at: lib/Sema/SemaChecking.cpp:10920-10921
+if (E->EvaluateAsInt(IntValue, S.Context, Expr::SE_AllowSideEffects)) {
+  if (S.SourceMgr.isInSystemMacro(CC))
+return;
+  const llvm::fltSemantics &FloatSemantics =

It seems wrong to early return here -- that means none of the later checks are 
run on system macros, but we've also not diagnosed anything as being wrong with 
the user's code yet.



Comment at: lib/Sema/SemaChecking.cpp:10938
+<< E->getSourceRange() << clang::SourceRange(CC));
+return;
+  }

This doesn't seem like something we need to early return for?


https://reviews.llvm.org/D52835



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

r346864 - [AST][NFC] Order the bit-field classes of Stmt like in StmtNodes.td

[Bruno Ricci via cfe-commits]

Author: brunoricci
Date: Wed Nov 14 06:19:34 2018
New Revision: 346864

URL: http://llvm.org/viewvc/llvm-project?rev=346864&view=rev
Log:
[AST][NFC] Order the bit-field classes of Stmt like in StmtNodes.td

Reorder the bit-field classes and the members of the anonymous union
so that they both match the order in StmtNodes.td.

There is already a fair amount of them, and this is not going to
improve. Therefore lets try to keep some order here.

Strictly NFC.


Modified:
cfe/trunk/include/clang/AST/Stmt.h

Modified: cfe/trunk/include/clang/AST/Stmt.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Stmt.h?rev=346864&r1=346863&r2=346864&view=diff
==
--- cfe/trunk/include/clang/AST/Stmt.h (original)
+++ cfe/trunk/include/clang/AST/Stmt.h Wed Nov 14 06:19:34 2018
@@ -332,12 +332,20 @@ protected:
 SourceLocation Loc;
   };
 
-  class CharacterLiteralBitfields {
-friend class CharacterLiteral;
+  class DeclRefExprBitfields {
+friend class ASTStmtReader; // deserialization
+friend class DeclRefExpr;
 
 unsigned : NumExprBits;
 
-unsigned Kind : 3;
+unsigned HasQualifier : 1;
+unsigned HasTemplateKWAndArgsInfo : 1;
+unsigned HasFoundDecl : 1;
+unsigned HadMultipleCandidates : 1;
+unsigned RefersToEnclosingVariableOrCapture : 1;
+
+/// The location of the declaration name itself.
+SourceLocation Loc;
   };
 
   enum APFloatSemantics {
@@ -358,6 +366,14 @@ protected:
 unsigned IsExact : 1;
   };
 
+  class CharacterLiteralBitfields {
+friend class CharacterLiteral;
+
+unsigned : NumExprBits;
+
+unsigned Kind : 3;
+  };
+
   class UnaryExprOrTypeTraitExprBitfields {
 friend class UnaryExprOrTypeTraitExpr;
 
@@ -367,20 +383,12 @@ protected:
 unsigned IsType : 1; // true if operand is a type, false if an expression.
   };
 
-  class DeclRefExprBitfields {
-friend class ASTStmtReader; // deserialization
-friend class DeclRefExpr;
+  class CallExprBitfields {
+friend class CallExpr;
 
 unsigned : NumExprBits;
 
-unsigned HasQualifier : 1;
-unsigned HasTemplateKWAndArgsInfo : 1;
-unsigned HasFoundDecl : 1;
-unsigned HadMultipleCandidates : 1;
-unsigned RefersToEnclosingVariableOrCapture : 1;
-
-/// The location of the declaration name itself.
-SourceLocation Loc;
+unsigned NumPreArgs : 1;
   };
 
   class CastExprBitfields {
@@ -394,24 +402,14 @@ protected:
 unsigned BasePathIsEmpty : 1;
   };
 
-  class CallExprBitfields {
-friend class CallExpr;
-
-unsigned : NumExprBits;
-
-unsigned NumPreArgs : 1;
-  };
-
-  class ExprWithCleanupsBitfields {
-friend class ASTStmtReader; // deserialization
-friend class ExprWithCleanups;
+  class InitListExprBitfields {
+friend class InitListExpr;
 
 unsigned : NumExprBits;
 
-// When false, it must not have side effects.
-unsigned CleanupsHaveSideEffects : 1;
-
-unsigned NumObjects : 32 - 1 - NumExprBits;
+/// Whether this initializer list originally had a GNU array-range
+/// designator in it. This is a temporary marker used by CodeGen.
+unsigned HadArrayRangeDesignator : 1;
   };
 
   class PseudoObjectExprBitfields {
@@ -426,33 +424,7 @@ protected:
 unsigned ResultIndex : 32 - 8 - NumExprBits;
   };
 
-  class OpaqueValueExprBitfields {
-friend class OpaqueValueExpr;
-
-unsigned : NumExprBits;
-
-/// The OVE is a unique semantic reference to its source expressio if this
-/// bit is set to true.
-unsigned IsUnique : 1;
-  };
-
-  class ObjCIndirectCopyRestoreExprBitfields {
-friend class ObjCIndirectCopyRestoreExpr;
-
-unsigned : NumExprBits;
-
-unsigned ShouldCopy : 1;
-  };
-
-  class InitListExprBitfields {
-friend class InitListExpr;
-
-unsigned : NumExprBits;
-
-/// Whether this initializer list originally had a GNU array-range
-/// designator in it. This is a temporary marker used by CodeGen.
-unsigned HadArrayRangeDesignator : 1;
-  };
+  //===--- C++ Expression bitfields classes ---===//
 
   class TypeTraitExprBitfields {
 friend class ASTStmtReader;
@@ -472,6 +444,20 @@ protected:
 unsigned NumArgs : 32 - 8 - 1 - NumExprBits;
   };
 
+  class ExprWithCleanupsBitfields {
+friend class ASTStmtReader; // deserialization
+friend class ExprWithCleanups;
+
+unsigned : NumExprBits;
+
+// When false, it must not have side effects.
+unsigned CleanupsHaveSideEffects : 1;
+
+unsigned NumObjects : 32 - 1 - NumExprBits;
+  };
+
+  //===--- C++ Coroutines TS bitfields classes ---===//
+
   class CoawaitExprBitfields {
 friend class CoawaitExpr;
 
@@ -480,7 +466,30 @@ protected:
 unsigned IsImplicit : 1;
   };
 
+  //===--- Obj-C Expression bitfields classes ---===//
+
+  class ObjCIndirectCopyRestoreExprBitfields {
+friend class ObjCIndirectCopyRestoreExpr;
+
+unsigned : NumExprBits;
+
+unsigned ShouldCopy 

[PATCH] D54526: [AST] Pack BinaryOperator

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno created this revision.
riccibruno added a reviewer: dblaikie.
riccibruno added a project: clang.
Herald added a subscriber: cfe-commits.

Use the newly available space in the bit-fields of `Stmt`.
This saves 8 bytes per `BinaryOperator`.


Repository:
  rC Clang

https://reviews.llvm.org/D54526

Files:
  include/clang/AST/Expr.h
  include/clang/AST/Stmt.h

Index: include/clang/AST/Stmt.h
===
--- include/clang/AST/Stmt.h
+++ include/clang/AST/Stmt.h
@@ -442,6 +442,17 @@
 unsigned BasePathIsEmpty : 1;
   };
 
+  class BinaryOperatorBitfields {
+friend class BinaryOperator;
+
+unsigned : NumExprBits;
+
+unsigned Opc : 6;
+unsigned FPFeatures : 3;
+
+SourceLocation OpLoc;
+  };
+
   class InitListExprBitfields {
 friend class InitListExpr;
 
@@ -558,6 +569,7 @@
 CallExprBitfields CallExprBits;
 MemberExprBitfields MemberExprBits;
 CastExprBitfields CastExprBits;
+BinaryOperatorBitfields BinaryOperatorBits;
 InitListExprBitfields InitListExprBits;
 PseudoObjectExprBitfields PseudoObjectExprBits;
 
Index: include/clang/AST/Expr.h
===
--- include/clang/AST/Expr.h
+++ include/clang/AST/Expr.h
@@ -3187,20 +3187,11 @@
 /// "+" resolves to an overloaded operator, CXXOperatorCallExpr will
 /// be used to express the computation.
 class BinaryOperator : public Expr {
-public:
-  typedef BinaryOperatorKind Opcode;
-
-private:
-  unsigned Opc : 6;
-
-  // This is only meaningful for operations on floating point types and 0
-  // otherwise.
-  unsigned FPFeatures : 3;
-  SourceLocation OpLoc;
-
   enum { LHS, RHS, END_EXPR };
-  Stmt* SubExprs[END_EXPR];
+  Stmt *SubExprs[END_EXPR];
+
 public:
+  typedef BinaryOperatorKind Opcode;
 
   BinaryOperator(Expr *lhs, Expr *rhs, Opcode opc, QualType ResTy,
  ExprValueKind VK, ExprObjectKind OK,
@@ -3211,24 +3202,29 @@
(lhs->isInstantiationDependent() ||
 rhs->isInstantiationDependent()),
(lhs->containsUnexpandedParameterPack() ||
-rhs->containsUnexpandedParameterPack())),
-  Opc(opc), FPFeatures(FPFeatures.getInt()), OpLoc(opLoc) {
+rhs->containsUnexpandedParameterPack())) {
+BinaryOperatorBits.Opc = opc;
+BinaryOperatorBits.FPFeatures = FPFeatures.getInt();
+BinaryOperatorBits.OpLoc = opLoc;
 SubExprs[LHS] = lhs;
 SubExprs[RHS] = rhs;
 assert(!isCompoundAssignmentOp() &&
"Use CompoundAssignOperator for compound assignments");
   }
 
   /// Construct an empty binary operator.
-  explicit BinaryOperator(EmptyShell Empty)
-: Expr(BinaryOperatorClass, Empty), Opc(BO_Comma) { }
+  explicit BinaryOperator(EmptyShell Empty) : Expr(BinaryOperatorClass, Empty) {
+BinaryOperatorBits.Opc = BO_Comma;
+  }
 
-  SourceLocation getExprLoc() const LLVM_READONLY { return OpLoc; }
-  SourceLocation getOperatorLoc() const { return OpLoc; }
-  void setOperatorLoc(SourceLocation L) { OpLoc = L; }
+  SourceLocation getExprLoc() const { return getOperatorLoc(); }
+  SourceLocation getOperatorLoc() const { return BinaryOperatorBits.OpLoc; }
+  void setOperatorLoc(SourceLocation L) { BinaryOperatorBits.OpLoc = L; }
 
-  Opcode getOpcode() const { return static_cast(Opc); }
-  void setOpcode(Opcode O) { Opc = O; }
+  Opcode getOpcode() const {
+return static_cast(BinaryOperatorBits.Opc);
+  }
+  void setOpcode(Opcode Opc) { BinaryOperatorBits.Opc = Opc; }
 
   Expr *getLHS() const { return cast(SubExprs[LHS]); }
   void setLHS(Expr *E) { SubExprs[LHS] = E; }
@@ -3257,7 +3253,11 @@
   static OverloadedOperatorKind getOverloadedOperator(Opcode Opc);
 
   /// predicates to categorize the respective opcodes.
-  bool isPtrMemOp() const { return Opc == BO_PtrMemD || Opc == BO_PtrMemI; }
+  static bool isPtrMemOp(Opcode Opc) {
+return Opc == BO_PtrMemD || Opc == BO_PtrMemI;
+  }
+  bool isPtrMemOp() const { return isPtrMemOp(getOpcode()); }
+
   static bool isMultiplicativeOp(Opcode Opc) {
 return Opc >= BO_Mul && Opc <= BO_Rem;
   }
@@ -3356,21 +3356,23 @@
 
   // Set the FP contractability status of this operator. Only meaningful for
   // operations on floating point types.
-  void setFPFeatures(FPOptions F) { FPFeatures = F.getInt(); }
+  void setFPFeatures(FPOptions F) {
+BinaryOperatorBits.FPFeatures = F.getInt();
+  }
 
-  FPOptions getFPFeatures() const { return FPOptions(FPFeatures); }
+  FPOptions getFPFeatures() const {
+return FPOptions(BinaryOperatorBits.FPFeatures);
+  }
 
   // Get the FP contractability status of this operator. Only meaningful for
   // operations on floating point types.
   bool isFPContractableWithinStatement() const {
-return FPOptions(FPFeatures).allowFPContractWithinStatement();
+return getFPFeatures().allowFPContractWithinStatement();
   }
 
   // Get the FENV_ACCESS status of this operator. Only meaningful for
   // operations on floa

r346865 - [Diagnostics] Check integer to floating point number implicit conversions

[David Bolvansky via cfe-commits]

Author: xbolva00
Date: Wed Nov 14 06:24:33 2018
New Revision: 346865

URL: http://llvm.org/viewvc/llvm-project?rev=346865&view=rev
Log:
[Diagnostics] Check integer to floating point number implicit conversions

Summary:
GCC already catches these situations so we should handle it too.

GCC warns in C++ mode only (does anybody know why?). I think it is useful in C 
mode too.

Reviewers: rsmith, erichkeane, aaron.ballman, efriedma, xbolva00

Reviewed By: xbolva00

Subscribers: efriedma, craig.topper, scanon, cfe-commits

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

Added:
cfe/trunk/test/Sema/impcast-integer-float.c
Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/test/Sema/ext_vector_casts.c

Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=346865&r1=346864&r2=346865&view=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Wed Nov 14 06:24:33 
2018
@@ -3229,6 +3229,10 @@ def warn_impcast_float_integer : Warning
   "implicit conversion turns floating-point number into integer: %0 to %1">,
   InGroup, DefaultIgnore;
 
+def warn_impcast_precision_float_to_integer : Warning<
+  "implicit conversion from %0 to %1 changes value from %2 to %3">,
+  InGroup>;
+
 def warn_impcast_float_to_integer : Warning<
   "implicit conversion from %0 to %1 changes value from %2 to %3">,
   InGroup, DefaultIgnore;

Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=346865&r1=346864&r2=346865&view=diff
==
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Wed Nov 14 06:24:33 2018
@@ -105,6 +105,19 @@ SourceLocation Sema::getLocationOfString
Context.getTargetInfo());
 }
 
+// FIXME: Force the precision of the source value down so we don't print
+// digits which are usually useless (we don't really care here if we
+// truncate a digit by accident in edge cases).  Ideally, APFloat::toString
+// would automatically print the shortest representation, but it's a bit
+// tricky to implement.
+static void PrettyPrintFloat(const llvm::APFloat &floatValue,
+ const llvm::fltSemantics &floatSem,
+ SmallVectorImpl &prettyFloatValue) {
+  unsigned precision = llvm::APFloat::semanticsPrecision(floatSem);
+  precision = llvm::divideCeil(precision * 59, 196);
+  floatValue.toString(prettyFloatValue, precision);
+}
+
 /// Checks that a call expression's argument count is the desired number.
 /// This is useful when doing custom type-checking.  Returns true on error.
 static bool checkArgCount(Sema &S, CallExpr *call, unsigned desiredArgCount) {
@@ -10473,15 +10486,8 @@ static void DiagnoseFloatingImpCast(Sema
 DiagID = diag::warn_impcast_float_to_integer;
   }
 
-  // FIXME: Force the precision of the source value down so we don't print
-  // digits which are usually useless (we don't really care here if we
-  // truncate a digit by accident in edge cases).  Ideally, APFloat::toString
-  // would automatically print the shortest representation, but it's a bit
-  // tricky to implement.
   SmallString<16> PrettySourceValue;
-  unsigned precision = llvm::APFloat::semanticsPrecision(Value.getSemantics());
-  precision = (precision * 59 + 195) / 196;
-  Value.toString(PrettySourceValue, precision);
+  PrettyPrintFloat(Value, Value.getSemantics(), PrettySourceValue);
 
   SmallString<16> PrettyTargetValue;
   if (IsBool)
@@ -10914,6 +10920,32 @@ CheckImplicitConversion(Sema &S, Expr *E
 return;
   }
 
+  if (Source->isIntegerType() && TargetBT && TargetBT->isFloatingType()) {
+llvm::APSInt IntValue;
+if (E->EvaluateAsInt(IntValue, S.Context, Expr::SE_AllowSideEffects)) {
+  if (S.SourceMgr.isInSystemMacro(CC))
+return;
+  const llvm::fltSemantics &FloatSemantics =
+  S.Context.getFloatTypeSemantics(QualType(TargetBT, 0));
+  llvm::APFloat FloatValue(FloatSemantics);
+  if (FloatValue.convertFromAPInt(IntValue, Source->isSignedIntegerType(),
+  llvm::APFloat::rmNearestTiesToEven) !=
+  llvm::APFloat::opOK) {
+SmallString<16> PrettyTargetValue;
+SmallString<16> PrettySourceValue;
+PrettyPrintFloat(FloatValue, FloatSemantics, PrettyTargetValue);
+IntValue.toString(PrettySourceValue);
+
+S.DiagRuntimeBehavior(
+E->getExprLoc(), E,
+S.PDiag(diag::warn_impcast_precision_float_to_integer)
+<< E->getType() << T << PrettySourceValue << PrettyTargetValue
+  

[PATCH] D52835: [Diagnostics] Check integer to floating point number implicit conversions

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rC346865: [Diagnostics] Check integer to floating point number 
implicit conversions (authored by xbolva00, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D52835?vs=172509&id=174030#toc

Repository:
  rC Clang

https://reviews.llvm.org/D52835

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaChecking.cpp
  test/Sema/ext_vector_casts.c
  test/Sema/impcast-integer-float.c

Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -3229,6 +3229,10 @@
   "implicit conversion turns floating-point number into integer: %0 to %1">,
   InGroup, DefaultIgnore;
 
+def warn_impcast_precision_float_to_integer : Warning<
+  "implicit conversion from %0 to %1 changes value from %2 to %3">,
+  InGroup>;
+
 def warn_impcast_float_to_integer : Warning<
   "implicit conversion from %0 to %1 changes value from %2 to %3">,
   InGroup, DefaultIgnore;
Index: test/Sema/ext_vector_casts.c
===
--- test/Sema/ext_vector_casts.c
+++ test/Sema/ext_vector_casts.c
@@ -118,7 +118,7 @@
   vf = l + vf;
   vf = 2.0 + vf;
   vf = d + vf; // expected-warning {{implicit conversion loses floating-point precision}}
-  vf = vf + 0x;
+  vf = vf + 0x; // expected-warning {{implicit conversion from 'unsigned int' to 'float2' (vector of 2 'float' values) changes value from 4294967295 to 4.2949673E+9}}
   vf = vf + 2.1; // expected-warning {{implicit conversion loses floating-point precision}}
   
   vd = l + vd;
Index: test/Sema/impcast-integer-float.c
===
--- test/Sema/impcast-integer-float.c
+++ test/Sema/impcast-integer-float.c
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 %s -verify -Wfloat-precision -fsyntax-only
+
+#define shift_plus_one(x) ((1ULL << x) + 1)
+
+void test(void) {
+float a1 = (1ULL << 31) + 1; // expected-warning {{implicit conversion from 'unsigned long long' to 'float' changes value from 2147483649 to 2.1474836E+9}}
+float a2 = 1ULL << 31;
+float a3 = shift_plus_one(31); // expected-warning {{implicit conversion from 'unsigned long long' to 'float' changes value from 2147483649 to 2.1474836E+9}}
+float a4 = (1ULL << 31) - 1; // expected-warning {{implicit conversion from 'unsigned long long' to 'float' changes value from 2147483647 to 2.1474836E+9}}
+
+double b1 = (1ULL << 63) + 1; // expected-warning {{implicit conversion from 'unsigned long long' to 'double' changes value from 9223372036854775809 to 9.223372036854775E+18}}
+double b2 = 1ULL << 63;
+double b3 = shift_plus_one(63); // expected-warning {{implicit conversion from 'unsigned long long' to 'double' changes value from 9223372036854775809 to 9.223372036854775E+18}}
+double b4 = (1ULL << 63) - 1; // expected-warning {{implicit conversion from 'unsigned long long' to 'double' changes value from 9223372036854775807 to 9.223372036854775E+18}} 
+
+long double c1 = ((__int128)1 << 127) + 1; // expected-warning {{implicit conversion from '__int128' to 'long double' changes value from -170141183460469231731687303715884105727 to -1.7014118346046923173E+38}}
+long double c2 = (__int128)1 << 127;
+
+_Float16 d1 = (1ULL << 15) + 1; // expected-warning {{implicit conversion from 'unsigned long long' to '_Float16' changes value from 32769 to 3.277E+4}}
+_Float16 d2 = 1ULL << 15;
+_Float16 d3 = shift_plus_one(15); // expected-warning {{implicit conversion from 'unsigned long long' to '_Float16' changes value from 32769 to 3.277E+4}}
+_Float16 d4 = (1ULL << 15) - 1; // expected-warning {{implicit conversion from 'unsigned long long' to '_Float16' changes value from 32767 to 3.277E+4}}
+
+float e = (__uint128_t)-1; // expected-warning {{implicit conversion from '__uint128_t' (aka 'unsigned __int128') to 'float' changes value from 340282366920938463463374607431768211455 to +Inf}}
+}
+// RUN: %clang_cc1 %s -verify -Wfloat-precision -fsyntax-only
+
+#define shift_plus_one(x) ((1ULL << x) + 1)
+
+void test(void) {
+float a1 = (1ULL << 31) + 1; // expected-warning {{implicit conversion from 'unsigned long long' to 'float' changes value from 2147483649 to 2.1474836E+9}}
+float a2 = 1ULL << 31;
+float a3 = shift_plus_one(31); // expected-warning {{implicit conversion from 'unsigned long long' to 'float' changes value from 2147483649 to 2.1474836E+9}}
+float a4 = (1ULL << 31) - 1; // expected-warning {{implicit conversion from 'unsigned long long' to 'float' changes value from 2147483647 to 2.1474836E+9}}
+
+double b1 = (1ULL << 63) + 1; // expected-warning {{implicit conversion from 'unsigned long long' to 'double' changes value from 9223372036854775809 to 9.223372036854775E+18}}
+double b2 

r346866 - Reverted D52835 to fix review comments

[David Bolvansky via cfe-commits]

Author: xbolva00
Date: Wed Nov 14 06:27:51 2018
New Revision: 346866

URL: http://llvm.org/viewvc/llvm-project?rev=346866&view=rev
Log:
Reverted D52835 to fix review comments

Removed:
cfe/trunk/test/Sema/impcast-integer-float.c
Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/test/Sema/ext_vector_casts.c

Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=346866&r1=346865&r2=346866&view=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Wed Nov 14 06:27:51 
2018
@@ -3229,10 +3229,6 @@ def warn_impcast_float_integer : Warning
   "implicit conversion turns floating-point number into integer: %0 to %1">,
   InGroup, DefaultIgnore;
 
-def warn_impcast_precision_float_to_integer : Warning<
-  "implicit conversion from %0 to %1 changes value from %2 to %3">,
-  InGroup>;
-
 def warn_impcast_float_to_integer : Warning<
   "implicit conversion from %0 to %1 changes value from %2 to %3">,
   InGroup, DefaultIgnore;

Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=346866&r1=346865&r2=346866&view=diff
==
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Wed Nov 14 06:27:51 2018
@@ -105,19 +105,6 @@ SourceLocation Sema::getLocationOfString
Context.getTargetInfo());
 }
 
-// FIXME: Force the precision of the source value down so we don't print
-// digits which are usually useless (we don't really care here if we
-// truncate a digit by accident in edge cases).  Ideally, APFloat::toString
-// would automatically print the shortest representation, but it's a bit
-// tricky to implement.
-static void PrettyPrintFloat(const llvm::APFloat &floatValue,
- const llvm::fltSemantics &floatSem,
- SmallVectorImpl &prettyFloatValue) {
-  unsigned precision = llvm::APFloat::semanticsPrecision(floatSem);
-  precision = llvm::divideCeil(precision * 59, 196);
-  floatValue.toString(prettyFloatValue, precision);
-}
-
 /// Checks that a call expression's argument count is the desired number.
 /// This is useful when doing custom type-checking.  Returns true on error.
 static bool checkArgCount(Sema &S, CallExpr *call, unsigned desiredArgCount) {
@@ -10486,8 +10473,15 @@ static void DiagnoseFloatingImpCast(Sema
 DiagID = diag::warn_impcast_float_to_integer;
   }
 
+  // FIXME: Force the precision of the source value down so we don't print
+  // digits which are usually useless (we don't really care here if we
+  // truncate a digit by accident in edge cases).  Ideally, APFloat::toString
+  // would automatically print the shortest representation, but it's a bit
+  // tricky to implement.
   SmallString<16> PrettySourceValue;
-  PrettyPrintFloat(Value, Value.getSemantics(), PrettySourceValue);
+  unsigned precision = llvm::APFloat::semanticsPrecision(Value.getSemantics());
+  precision = (precision * 59 + 195) / 196;
+  Value.toString(PrettySourceValue, precision);
 
   SmallString<16> PrettyTargetValue;
   if (IsBool)
@@ -10920,32 +10914,6 @@ CheckImplicitConversion(Sema &S, Expr *E
 return;
   }
 
-  if (Source->isIntegerType() && TargetBT && TargetBT->isFloatingType()) {
-llvm::APSInt IntValue;
-if (E->EvaluateAsInt(IntValue, S.Context, Expr::SE_AllowSideEffects)) {
-  if (S.SourceMgr.isInSystemMacro(CC))
-return;
-  const llvm::fltSemantics &FloatSemantics =
-  S.Context.getFloatTypeSemantics(QualType(TargetBT, 0));
-  llvm::APFloat FloatValue(FloatSemantics);
-  if (FloatValue.convertFromAPInt(IntValue, Source->isSignedIntegerType(),
-  llvm::APFloat::rmNearestTiesToEven) !=
-  llvm::APFloat::opOK) {
-SmallString<16> PrettyTargetValue;
-SmallString<16> PrettySourceValue;
-PrettyPrintFloat(FloatValue, FloatSemantics, PrettyTargetValue);
-IntValue.toString(PrettySourceValue);
-
-S.DiagRuntimeBehavior(
-E->getExprLoc(), E,
-S.PDiag(diag::warn_impcast_precision_float_to_integer)
-<< E->getType() << T << PrettySourceValue << PrettyTargetValue
-<< E->getSourceRange() << clang::SourceRange(CC));
-return;
-  }
-}
-  }
-
   DiagnoseNullConversion(S, E, T, CC);
 
   S.DiscardMisalignedMemberAddress(Target, E);

Modified: cfe/trunk/test/Sema/ext_vector_casts.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/ext_vector_casts.c?rev=346866&r1=346865&r2=346866&view=diff
===

[PATCH] D52835: [Diagnostics] Check integer to floating point number implicit conversions

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

xbolva00 added a comment.

Ok, I will address your comments soon.


Repository:
  rC Clang

https://reviews.llvm.org/D52835



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

[PATCH] D54391: Fix compatibility with z3-4.8.1

[Mikhail Ramalho via Phabricator via cfe-commits]

mikhail.ramalho added a comment.

Since we're supporting version 4.8.1 now, the cmake file should be changed to 
"minimum" instead of "exact".


Repository:
  rL LLVM

https://reviews.llvm.org/D54391



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

[PATCH] D54269: Introduce shard storage to auto-index.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

just some nits. Main thing is the LoggingStorage - again I think this may have 
been a misunderstanding.




Comment at: clangd/index/Background.cpp:76
+else
+  elog("Error while reading shard {0}: {1}", ShardIdentifier,
+   I.takeError());

ADD_FAILURE() I think



Comment at: clangd/index/Background.cpp:94
+  // directory for all shard files.
+  DiskBackedIndexStorage(llvm::StringRef Directory) {
+llvm::SmallString<128> CDBDirectory(Directory);

nit: move constructor to the top?



Comment at: clangd/index/Background.cpp:98
+DiskShardRoot = CDBDirectory.str();
+if (!llvm::sys::fs::exists(DiskShardRoot)) {
+  std::error_code OK;

create_directory returns success if the directory exists, so no need for this 
check



Comment at: clangd/index/Background.cpp:108
+
+class LoggingIndexStorage : public BackgroundIndexStorage {
+public:

Hmm, I'm not really sure what this is for - does someone constructing 
BackgroundIndex ever not want to define storage *and* not want to control 
logging?



Comment at: clangd/index/Background.cpp:344
+  if (auto Error = IndexStorage->storeShard(Path, Shard))
+elog("Failed to store shard for {0}: {1}", Path, std::move(Error));
+}

this doesn't give enough context - the logger is global to clangd.

maybe "Failed to write background-index shard for file {0}: {1}"



Comment at: clangd/index/Background.h:39
+
+  virtual llvm::Expected
+  loadShard(llvm::StringRef ShardIdentifier) const = 0;

kadircet wrote:
> sammccall wrote:
> > sammccall wrote:
> > > docs
> > Hmm, we're going to attempt to load the shard corresponding to every file 
> > that we have in the CDB, even if the index isn't built yet. So "file 
> > doesn't exist" is an expected case (where we don't want to log etc), vs 
> > e.g. "file was corrupted" is unexpected and should definitely be logged. 
> > How do we distinguish these with this API?
> > 
> > One defensible option would be to have implementations handle errors: 
> > return Optional, and have the disk version log errors 
> > appropriately and return None.
> > 
> > Another would be Expected> or so, which is a little 
> > messy.
> used the former proposition, but with std::unique_ptr instead of 
> llvm::Optional since IndexFileIn is move-only.
This is fine, though Optional works fine with move-only types.

(your comment still says None)



Comment at: clangd/index/Background.h:48
+  static BackgroundIndexStorage *
+  createDiskStorage(llvm::StringRef CDBDirectory);
+};

we may also want to provide the standard factory - can go in a later patch 
though.



Comment at: unittests/clangd/BackgroundIndexTests.cpp:127
+  [&Storage, &CacheHits](llvm::StringRef) {
+static MemoryShardStorage MSS(Storage, CacheHits);
+return &MSS;

nit: seems a little less unusual to avoid the static:
```
MemoryShardStorage Storage;
auto MemoryStorageFactory = [&](llvm::StringRef){ return &Storage; }
...
EXPECT_THAT(Storage.contains("root/A.h"));
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54269



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

[PATCH] D53427: [clangd] Replace StringRef in SymbolLocation with a char pointer.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/trunk/clangd/index/Index.h:71
+  // via llvm::StringRef.
+  const char *FileURI = "";
 };

alexfh wrote:
> hokein wrote:
> > alexfh wrote:
> > > If the users of the SymbolLocation have a way to get a common "context", 
> > > you could keep a list of filenames in this context and just store an 
> > > index into this list (which could be even shorter). If the structure is 
> > > used where no context is available, you could still keep a list of 
> > > StringRefs somewhere (probably close to where the string data pointed by 
> > > the StringRefs lives) and keep a `StringRef *` here. At a cost of one 
> > > indirection you'd avoid calling strlen.
> > > 
> > > Not sure what trade-off is better for this particular use case though.
> > `SymbolLocation` is a public interface, and the underlying data is stored 
> > in arena (which is not visible to clients), so clients don't know any 
> > `Context` except `SymbolLocation`.
> > 
> >  Using `StringRef *` is an interesting idea -- keeping a list of 
> > `StringRefs` needs extra space, the number should be relatively small (only 
> > the number of different files), so we will spend  extra ` * 
> > sizeof(StringRef)` bytes (for llvm, ~128 KB memory) , compared with the raw 
> > pointer solution.
> Another option mentioned by Ilya offline is similar to MS's BSTR: keep the 
> length along with the string data in the arena. Something like:
> 
> ```
> class InlineString {
>   size_t Length;
>   char Data[0];  // Open-ended array with a null-terminated string in it.
>  public:
>operator StringRef() const { return StringRef(&Data[0], Length); }
>...
> };
> ```
> 
> And then keep `const InlineString *` here.
Yeah, operator `StringRef()` doesn't really work well unfortunately (I tried a 
`StringRefz` which just wrapped a char pointer, in a bid to avoid accidentally 
using pointer semantics).

Conversion rules are too complicated and existing APIs are already maximally 
constrained, so you can't satisfy all of {functions that take StringRef only, 
functions that take Twine only, functions that overload for 
StringRef/char*/std::string, functions that *also* overload for Twine, ...}


Repository:
  rL LLVM

https://reviews.llvm.org/D53427



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

[PATCH] D52835: [Diagnostics] Check integer to floating point number implicit conversions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D52835#1298428, @xbolva00 wrote:

> Reverted.
>  Ok, I will address your comments soon.


Thanks! I'm sorry for the delayed reviews -- there were wg21 meetings last 
week, which several of the reviewers were attending.


Repository:
  rC Clang

https://reviews.llvm.org/D52835



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

[PATCH] D54166: [AST] Store the string data in StringLiteral in a trailing array of chars

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno updated this revision to Diff 174032.
riccibruno added a comment.

Factored out some NFSs which I will submit separately.


Repository:
  rC Clang

https://reviews.llvm.org/D54166

Files:
  include/clang/AST/Expr.h
  include/clang/AST/Stmt.h
  lib/AST/Expr.cpp
  lib/Serialization/ASTReaderStmt.cpp
  lib/Serialization/ASTWriterStmt.cpp

Index: lib/Serialization/ASTWriterStmt.cpp
===
--- lib/Serialization/ASTWriterStmt.cpp
+++ lib/Serialization/ASTWriterStmt.cpp
@@ -518,17 +518,23 @@
 
 void ASTStmtWriter::VisitStringLiteral(StringLiteral *E) {
   VisitExpr(E);
-  Record.push_back(E->getByteLength());
+
+  // Store the various bits of data of StringLiteral.
   Record.push_back(E->getNumConcatenated());
+  Record.push_back(E->getLength());
+  Record.push_back(E->getCharByteWidth());
   Record.push_back(E->getKind());
   Record.push_back(E->isPascal());
-  // FIXME: String data should be stored as a blob at the end of the
-  // StringLiteral. However, we can't do so now because we have no
-  // provision for coping with abbreviations when we're jumping around
-  // the AST file during deserialization.
-  Record.append(E->getBytes().begin(), E->getBytes().end());
+
+  // Store the trailing array of SourceLocation.
   for (unsigned I = 0, N = E->getNumConcatenated(); I != N; ++I)
 Record.AddSourceLocation(E->getStrTokenLoc(I));
+
+  // Store the trailing array of char holding the string data.
+  StringRef StrData = E->getBytes();
+  for (unsigned I = 0, N = E->getByteLength(); I != N; ++I)
+Record.push_back(StrData[I]);
+
   Code = serialization::EXPR_STRING_LITERAL;
 }
 
Index: lib/Serialization/ASTReaderStmt.cpp
===
--- lib/Serialization/ASTReaderStmt.cpp
+++ lib/Serialization/ASTReaderStmt.cpp
@@ -595,22 +595,35 @@
 
 void ASTStmtReader::VisitStringLiteral(StringLiteral *E) {
   VisitExpr(E);
-  unsigned Len = Record.readInt();
-  assert(Record.peekInt() == E->getNumConcatenated() &&
- "Wrong number of concatenated tokens!");
-  Record.skipInts(1);
-  auto kind = static_cast(Record.readInt());
-  bool isPascal = Record.readInt();
 
-  // Read string data
-  auto B = &Record.peekInt();
-  SmallString<16> Str(B, B + Len);
-  E->setString(Record.getContext(), Str, kind, isPascal);
-  Record.skipInts(Len);
-
-  // Read source locations
-  for (unsigned I = 0, N = E->getNumConcatenated(); I != N; ++I)
+  // NumConcatenated, Length and CharByteWidth are set by the empty
+  // ctor since they are needed to allocate storage for the trailing objects.
+  unsigned NumConcatenated = Record.readInt();
+  unsigned Length = Record.readInt();
+  unsigned CharByteWidth = Record.readInt();
+  assert((NumConcatenated == E->getNumConcatenated()) &&
+ "Wrong number of concatenated tokens!");
+  assert((Length == E->getLength()) && "Wrong Length!");
+  assert((CharByteWidth == E->getCharByteWidth()) && "Wrong character width!");
+  E->StringLiteralBits.Kind = Record.readInt();
+  E->StringLiteralBits.IsPascal = Record.readInt();
+
+  // The character width is originally computed via mapCharByteWidth.
+  // Check that the deserialized character width is consistant with the result
+  // of calling mapCharByteWidth.
+  assert((CharByteWidth ==
+  StringLiteral::mapCharByteWidth(Record.getContext().getTargetInfo(),
+  E->getKind())) &&
+ "Wrong character width!");
+
+  // Deserialize the trailing array of SourceLocation.
+  for (unsigned I = 0; I < NumConcatenated; ++I)
 E->setStrTokenLoc(I, ReadSourceLocation());
+
+  // Deserialize the trailing array of char holding the string data.
+  char *StrData = E->getStrDataAsChar();
+  for (unsigned I = 0; I < Length * CharByteWidth; ++I)
+StrData[I] = Record.readInt();
 }
 
 void ASTStmtReader::VisitCharacterLiteral(CharacterLiteral *E) {
@@ -2423,8 +2436,11 @@
   break;
 
 case EXPR_STRING_LITERAL:
-  S = StringLiteral::CreateEmpty(Context,
- Record[ASTStmtReader::NumExprFields + 1]);
+  S = StringLiteral::CreateEmpty(
+  Context,
+  /* NumConcatenated=*/Record[ASTStmtReader::NumExprFields + 0],
+  /* Length=*/Record[ASTStmtReader::NumExprFields + 1],
+  /* CharByteWidth=*/Record[ASTStmtReader::NumExprFields + 2]);
   break;
 
 case EXPR_CHARACTER_LITERAL:
Index: lib/AST/Expr.cpp
===
--- lib/AST/Expr.cpp
+++ lib/AST/Expr.cpp
@@ -912,42 +912,80 @@
   return CharByteWidth;
 }
 
-StringLiteral *StringLiteral::Create(const ASTContext &C, StringRef Str,
- StringKind Kind, bool Pascal, QualType Ty,
- const SourceLocation *Loc,
- unsigned NumStrs) {
-  assert(C.getAsConstantArrayType(Ty) &&
+StringLiteral::StringLi

[PATCH] D53764: [OpenCL] Enable address spaces for references in C++

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia updated this revision to Diff 174033.
Anastasia added a comment.

Fixed check for AS mismatch of pointer type and added missing test case


https://reviews.llvm.org/D53764

Files:
  include/clang/Sema/Sema.h
  lib/AST/Expr.cpp
  lib/CodeGen/CGExpr.cpp
  lib/Sema/DeclSpec.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaInit.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenOpenCLCXX/address-space-deduction.cl

Index: test/CodeGenOpenCLCXX/address-space-deduction.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/address-space-deduction.cl
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm -o - | FileCheck %s -check-prefixes=COMMON,PTR
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -O0 -emit-llvm -o - -DREF | FileCheck %s -check-prefixes=COMMON,REF
+
+#ifdef REF
+#define PTR &
+#define ADR(x) x
+#else
+#define PTR *
+#define ADR(x) &x
+#endif
+
+//COMMON: @glob = addrspace(1) global i32
+int glob;
+//PTR: @glob_p = addrspace(1) global i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @glob to i32 addrspace(4)*)
+//REF: @glob_p = addrspace(1) global i32 addrspace(4)* null
+int PTR glob_p = ADR(glob);
+
+//COMMON: @_ZZ3fooi{{P|R}}U3AS4iE6loc_st = internal addrspace(1) global i32
+//PTR: @_ZZ3fooiPU3AS4iE8loc_st_p = internal addrspace(1) global i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @_ZZ3fooiPU3AS4iE6loc_st to i32 addrspace(4)*)
+//REF: @_ZZ3fooiRU3AS4iE8loc_st_p = internal addrspace(1) global i32 addrspace(4)* null
+//COMMON: @loc_ext_p = external addrspace(1) {{global|constant}} i32 addrspace(4)*
+//COMMON: @loc_ext = external addrspace(1) global i32
+
+//REF: store i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @glob to i32 addrspace(4)*), i32 addrspace(4)* addrspace(1)* @glob_p
+
+//COMMON: define spir_func i32 @_Z3fooi{{P|R}}U3AS4i(i32 %par, i32 addrspace(4)*{{.*}} %par_p)
+int foo(int par, int PTR par_p){
+  //COMMON: %loc = alloca i32
+  int loc;
+  //COMMON: %loc_p = alloca i32 addrspace(4)*
+  //COMMON: %loc_p_const = alloca i32*
+  //COMMON: [[GAS:%[0-9]+]] = addrspacecast i32* %loc to i32 addrspace(4)*
+  //COMMON: store i32 addrspace(4)* [[GAS]], i32 addrspace(4)** %loc_p
+  int PTR loc_p = ADR(loc);
+  //COMMON: store i32* %loc, i32** %loc_p_const
+  const __private int PTR loc_p_const = ADR(loc);
+
+  // CHECK directives for the following code are located above.
+  static int loc_st;
+  //REF: store i32 addrspace(4)* addrspacecast (i32 addrspace(1)* @_ZZ3fooiRU3AS4iE6loc_st to i32 addrspace(4)*), i32 addrspace(4)* addrspace(1)* @_ZZ3fooiRU3AS4iE8loc_st_p
+  static int PTR loc_st_p = ADR(loc_st);
+  extern int loc_ext;
+  extern int PTR loc_ext_p;
+  (void)loc_ext_p;
+  return loc_ext;
+}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -7177,7 +7177,8 @@
   bool IsPointee =
   ChunkIndex > 0 &&
   (D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Pointer ||
-   D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::BlockPointer);
+   D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::BlockPointer ||
+   D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Reference);
   bool IsFuncReturnType =
   ChunkIndex > 0 &&
   D.getTypeObject(ChunkIndex - 1).Kind == DeclaratorChunk::Function;
Index: lib/Sema/SemaInit.cpp
===
--- lib/Sema/SemaInit.cpp
+++ lib/Sema/SemaInit.cpp
@@ -7209,12 +7209,20 @@
   return CreateMaterializeTemporaryExpr(E->getType(), E, false);
 }
 
-ExprResult
-InitializationSequence::Perform(Sema &S,
-const InitializedEntity &Entity,
-const InitializationKind &Kind,
-MultiExprArg Args,
-QualType *ResultType) {
+ExprResult Sema::PerformQualificationConversion(Expr *E, QualType Ty,
+ExprValueKind VK,
+CheckedConversionKind CCK) {
+  CastKind CK = (Ty.getAddressSpace() != E->getType().getAddressSpace())
+? CK_AddressSpaceConversion
+: CK_NoOp;
+  return ImpCastExprToType(E, Ty, CK, VK, /*BasePath=*/nullptr, CCK);
+}
+
+ExprResult InitializationSequence::Perform(Sema &S,
+   const InitializedEntity &Entity,
+   const InitializationKind &Kind,
+   MultiExprArg Args,
+   QualType *ResultType) {
   if (Failed()) {
 Diagnose(S, Entity, Kind, Args);
 return ExprError();
@@ -7603,12 +7611,11 @@
 case SK_QualificationConversionRValue: {
   // Perform a qualification conversion; these can never 

[PATCH] D54166: [AST] Store the string data in StringLiteral in a trailing array of chars

[Bruno Ricci via Phabricator via cfe-commits]

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



Comment at: include/clang/AST/Expr.h:1615
+  }
+
+  /// Build a string literal.

Note that the trailing array of chars is aligned to 4 bytes
since it is after the array of `SourceLocation`.
Therefore I believe that the `uint16_t *` and `uint32_t *`
point to properly aligned memory. However I can add an
assertion here if needed.


Repository:
  rC Clang

https://reviews.llvm.org/D54166



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

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

> The only thing I didn't include in this patch were the suggested changes to 
> FixedPointSemantics which would indicate if the semantics were original from 
> an integer type. I'm not sure how necessary this is because AFAIK the only 
> time we would need to care if the semantics came from an int is during 
> conversion from a fixed point type to an int, which should only occur during 
> casting and not binary operations. In that sense, I don't think this info 
> needs to be something bound to the semantics, but more to the conversion 
> operation. I also don't think that would be relevant for this patch since all 
> integers get converted to fixed point types anyway and not the other way 
> around.



> For the integer conversion though, I was going to add that in a separate 
> fixed-point-to-int and int-to-fixed-point patch.

Okay, that's fine! You're right in that the semantics commoning will never 
produce an integer semantic like that.




Comment at: clang/lib/Basic/FixedPoint.cpp:129
+  std::max(NonFractBits, OtherNonFractBits) + CommonScale;
+
+  bool ResultIsSigned = isSigned() || Other.isSigned();

Does this properly compensate for types of equal width but different signedness?

If you have a signed 8-bit 7-scale fixed point number, and operate with an 
unsigned 8-bit integer, you'll get CommonWidth=15 and CommonScale=7, leaving 8 
bits of integral. But the MSB in that cannot both be sign bit and unsigned MSB 
at the same time. I think you need an extra bit.



Comment at: clang/lib/Basic/FixedPoint.cpp:135
+ResultHasUnsignedPadding =
+hasUnsignedPadding() || Other.hasUnsignedPadding();
+

If one has padding but the other doesn't, then the padding must be significant, 
so the full precision semantic cannot have padding.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3385
+  if (IsCommonSaturated)
+CommonFixedSema.setSaturated(false);
+

Can EmitFixedPointConversion not determine this on its own?



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3387
+
+  // Convert and align the operands.
+  Value *LHSAligned = EmitFixedPointConversion(

'align' usually means something else. Maybe 'Convert the operands to the full 
precision type' and 'FullLHS'?



Comment at: clang/lib/Sema/SemaExpr.cpp:1306
+OtherTy = ResultTy;
+  }
+

Does this handle compound assignment? The other functions handle this specially.



Comment at: clang/lib/Sema/SemaExpr.cpp:1403
+  else if (RHSType->isFixedPointType())
+return handleFixedPointConversion(*this, RHS, LHS, RHSType, LHSType);
+

Can this commutation be folded into the function to align it with the rest?


Repository:
  rC Clang

https://reviews.llvm.org/D53738



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

[PATCH] D53764: [OpenCL] Enable address spaces for references in C++

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: lib/Sema/SemaExprCXX.cpp:4289
+ /*BasePath=*/nullptr, CCK)
+   .get();
 

rjmccall wrote:
> Anastasia wrote:
> > rjmccall wrote:
> > > Okay.  But if `ToType` *isn't* a reference type, this will never be an 
> > > address-space conversion.  I feel like this code could be written more 
> > > clearly to express what it's trying to do.
> > I hope it makes more sense now. Btw, it also applies to pointer type.
> The logic is wrong for pointer types; if you're converting pointers, you need 
> to be checking the address space of the pointee type of the from type.
> 
> It sounds like this is totally inadequately tested; please flesh out the test 
> with all of these cases.  While you're at it, please ensure that there are 
> tests verifying that we don't allowing address-space changes in nested 
> positions.
Thanks for spotting this bug! The generated IR for the test was still correct 
because AS of `FromType` happened to correctly mismatch AS of pointee of 
`ToType`.

I failed to construct the test case where it would miss classifying 
`addrspacecast` due to OpenCL or C++ sema rules but I managed to add a case in 
which `addrspacecast` was incorrectly added for pointers where it wasn't needed 
(see line 36 of the test). I think this code is covered now.

As for the address space position in pointers, the following test checks the 
address spaces of pointers in `addrspacecast`. For the other program paths we 
also have a test with similar checks in 
`test/CodeGenOpenCL/address-spaces-conversions.cl` that we now run for C++ mode 
too.

BTW, while trying to construct a test case for the bug, I have discovered that 
multiple pointer indirection casting isn't working correctly. I.e. for the 
following program:
  kernel void foo(){
 __private int** loc;
 int** loc_p = loc;
 **loc_p = 1;
  }
We generate:
  bitcast i32* addrspace(4)* %0 to i32 addrspace(4)* addrspace(4)*
in OpenCL C and then perform `store` over pointer in AS 4 (generic). We have 
now lost the information that the original pointer was in `private` AS and that 
the adjustment of AS segment has to be performed before accessing memory 
pointed by the pointer. Based on the current specification of `addrspacecast` 
in https://llvm.org/docs/LangRef.html#addrspacecast-to-instruction I am not 
very clear whether it can be used for this case without any modifications or 
clarifications and also what would happen if there are multiple AS mismatches. 
I am going to look at this issue separately in more details. In OpenCL C++ an 
ICE is triggered for this though. Let me know if you have any thoughts on this.


https://reviews.llvm.org/D53764



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

[PATCH] D54326: [AST] Pack CXXThrowExpr, CXXDefaultArgExpr and CXXDefaultInitExpr

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno abandoned this revision.
riccibruno added a comment.

Broken down and sent for review individually.


Repository:
  rC Clang

https://reviews.llvm.org/D54326



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

[PATCH] D54325: [AST] Pack CXXBoolLiteralExpr, CXXNullPtrLiteralExpr and CXXThisExpr

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno abandoned this revision.
riccibruno added a comment.

Broken down and sent for review individually.


Repository:
  rC Clang

https://reviews.llvm.org/D54325



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

[PATCH] D54246: [clang-tidy] Add the abseil-duration-factory-scale check

[Hyrum Wright via Phabricator via cfe-commits]

hwright updated this revision to Diff 174039.
hwright marked 11 inline comments as done.
hwright added a comment.

Combined multiplication and division logic, and also now handles scaling of 
multiple steps (e.g., Seconds * 3600).


https://reviews.llvm.org/D54246

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/DurationFactoryScaleCheck.cpp
  clang-tidy/abseil/DurationFactoryScaleCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-duration-factory-scale.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-duration-factory-scale.cpp

Index: test/clang-tidy/abseil-duration-factory-scale.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-factory-scale.cpp
@@ -0,0 +1,130 @@
+// RUN: %check_clang_tidy %s abseil-duration-factory-scale %t
+
+// Mimic the implementation of absl::Duration
+namespace absl {
+
+class Duration {};
+
+Duration Nanoseconds(long long);
+Duration Microseconds(long long);
+Duration Milliseconds(long long);
+Duration Seconds(long long);
+Duration Minutes(long long);
+Duration Hours(long long);
+
+#define GENERATE_DURATION_FACTORY_OVERLOADS(NAME) \
+  Duration NAME(float n); \
+  Duration NAME(double n);\
+  template\
+  Duration NAME(T n);
+
+GENERATE_DURATION_FACTORY_OVERLOADS(Nanoseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Microseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Milliseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Seconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Minutes);
+GENERATE_DURATION_FACTORY_OVERLOADS(Hours);
+#undef GENERATE_DURATION_FACTORY_OVERLOADS
+
+}  // namespace absl
+
+void ScaleTest() {
+  absl::Duration d;
+
+  // Zeroes
+  d = absl::Hours(0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+  // CHECK-FIXES: absl::ZeroDuration();
+  d = absl::Minutes(0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+  // CHECK-FIXES: absl::ZeroDuration();
+  d = absl::Seconds(0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+  // CHECK-FIXES: absl::ZeroDuration();
+  d = absl::Milliseconds(0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+  // CHECK-FIXES: absl::ZeroDuration();
+  d = absl::Microseconds(0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+  // CHECK-FIXES: absl::ZeroDuration();
+  d = absl::Nanoseconds(0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+  // CHECK-FIXES: absl::ZeroDuration();
+  d = absl::Seconds(0.0);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+  // CHECK-FIXES: absl::ZeroDuration();
+  d = absl::Seconds(0x0.01p-126f);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+  // CHECK-FIXES: absl::ZeroDuration();
+
+  // Fold seconds into minutes
+  d = absl::Seconds(30 * 60);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+  // CHECK-FIXES: absl::Minutes(30);
+  d = absl::Seconds(60 * 30);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+  // CHECK-FIXES: absl::Minutes(30);
+
+  // Try a few more exotic multiplications
+  d = absl::Seconds(60 * 30 * 60);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+  // CHECK-FIXES: absl::Minutes(60 * 30);
+  d = absl::Seconds(1e-3 * 30);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+  // CHECK-FIXES: absl::Milliseconds(30);
+  d = absl::Milliseconds(30 * 1000);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+  // CHECK-FIXES: absl::Seconds(30);
+  d = absl::Milliseconds(30 * 0.001);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+  // CHECK-FIXES: absl::Microseconds(30);
+
+  // Multiple steps
+  d = absl::Seconds(5 * 3600);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+  // CHECK-FIXES: absl::Hours(5);
+  d = absl::Microseconds(5 * 1e6);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abs

[PATCH] D54172: [AST] Pack ArraySubscriptExpr and CallExpr

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno abandoned this revision.
riccibruno added a comment.

Sent for review individually.


Repository:
  rC Clang

https://reviews.llvm.org/D54172



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

[PATCH] D54246: [clang-tidy] Add the abseil-duration-factory-scale check

[Hyrum Wright via Phabricator via cfe-commits]

hwright added inline comments.



Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:57-58
+// One and only one of `IntLit` and `FloatLit` should be provided.
+static double GetValue(const IntegerLiteral *IntLit,
+   const FloatingLiteral *FloatLit) {
+  if (IntLit) {

aaron.ballman wrote:
> hwright wrote:
> > aaron.ballman wrote:
> > > I really don't like this interface where you pass two arguments, only one 
> > > of which is ever valid. That is pretty confusing. Given that the result 
> > > of this function is only ever passed to `GetNewMultScale()`, and that 
> > > function only does integral checks, I'd prefer logic more like:
> > > 
> > > * If the literal is integral, get its value and call `GetNewMultScale()`.
> > > * If the literal is float, convert it to an integral and call 
> > > `GetNewMultScale()` only if the conversion is exact (this can be done via 
> > > `APFloat::convertToInteger()`).
> > > * `GetNewMultScale()` can now accept an integer value and removes the 
> > > questions about inexact equality tests from the function.
> > > 
> > > With that logic, I don't see a need for `GetValue()` at all, but if a 
> > > helper function is useful, I'd probably guess this is a better signature: 
> > > `int64_t getIntegralValue(const Expr *Literal, bool &ResultIsExact);`
> > >  Given that the result of this function is only ever passed to 
> > > `GetNewMultScale()`, and that function only does integral checks, I'd 
> > > prefer logic more like:
> > 
> > That's actually not true: `GetNewMultScale()` does checks against values 
> > like `1e-3` which aren't integers.  Does this change your suggestion?
> Hmm, yeah, I suppose it has to! :-D
I've reworked the bulk of this logic. It still uses doubles, but that doesn't 
trouble our test cases.  Please let me know if there's more to do here.



Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:63
+  assert(FloatLit != nullptr && "Neither IntLit nor FloatLit set");
+  return FloatLit->getValueAsApproximateDouble();
+}

aaron.ballman wrote:
> hwright wrote:
> > aaron.ballman wrote:
> > > I believe the approximate results here can lead to bugs where the 
> > > floating-point literal is subnormal -- it may return 0.0 for literals 
> > > that are not zero.
> > Do you have an example which I could put in a test?
> `0x0.01p-126f` should get you a new, exciting way to spell `0`.
I've added this as a test, and it resolves as normal.  Was your comment 
intended to indicate that it //should//, or that doing so would be a bug?



Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:81-84
+if (Multiplier == 60.0)
+  return DurationScale::Minutes;
+if (Multiplier == 1e-3)
+  return DurationScale::Milliseconds;

aaron.ballman wrote:
> hwright wrote:
> > aaron.ballman wrote:
> > > What about scaling with a multiplier of 3600 to go from seconds to hours, 
> > > and other plausible conversions?
> > That's a good point, and part of a broader design discussion: should we 
> > support all multipliers?  (e.g., what about multiplying microseconds by 
> > `1.0/864.0`?)
> > 
> > If we do think it's worth handling all of these cases, we probably want a 
> > different construct than the equivalent of a lookup table to do this 
> > computation.
> > That's a good point, and part of a broader design discussion: should we 
> > support all multipliers?
> 
> That's kind of what I'm leaning towards. It's certainly more explainable to 
> users that all the various scaling operations just work, rather than some 
> particular set.
> 
> However, maybe you know more about the user base than I do and there's a 
> sound reason to not handle all cases?
> 
> > If we do think it's worth handling all of these cases, we probably want a 
> > different construct than the equivalent of a lookup table to do this 
> > computation.
> 
> There's a part of me that wonders if we can use `std::ratio` to describe the 
> scaling operations, but I freely admit I've not thought about implementation 
> strategies all that hard.
`std::ratio` didn't look like it made much sense here (though I don't have much 
first-hand experience using it), but we do now handle multiple scaling steps.


https://reviews.llvm.org/D54246



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

[PATCH] D53770: Support g++ headers in include/g++

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

rsmith added a comment.

In https://reviews.llvm.org/D53770#1297646, @greened wrote:

> Oops, I didn't realize this hadn't been formally accepted yet.  Still 
> learning the Phab process.  Let me know if you want it reverted for a formal 
> accept.


No, this is fine.


Repository:
  rL LLVM

https://reviews.llvm.org/D53770



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

[PATCH] D54450: Get the correct range of tokens for preprocessor conditions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 174041.
aaron.ballman marked 3 inline comments as done.
aaron.ballman added a comment.

Updating based on review feedback.


https://reviews.llvm.org/D54450

Files:
  include/clang/Lex/Preprocessor.h
  lib/Lex/PPDirectives.cpp
  lib/Lex/PPExpressions.cpp
  unittests/Lex/PPCallbacksTest.cpp

Index: unittests/Lex/PPCallbacksTest.cpp
===
--- unittests/Lex/PPCallbacksTest.cpp
+++ unittests/Lex/PPCallbacksTest.cpp
@@ -65,6 +65,29 @@
   SrcMgr::CharacteristicKind FileType;
 };
 
+class CondDirectiveCallbacks : public PPCallbacks {
+public:
+  struct Result {
+SourceRange ConditionRange;
+ConditionValueKind ConditionValue;
+
+Result(SourceRange R, ConditionValueKind K)
+: ConditionRange(R), ConditionValue(K) {}
+  };
+
+  std::vector Results;
+
+  void If(SourceLocation Loc, SourceRange ConditionRange,
+  ConditionValueKind ConditionValue) override {
+Results.emplace_back(ConditionRange, ConditionValue);
+  }
+
+  void Elif(SourceLocation Loc, SourceRange ConditionRange,
+ConditionValueKind ConditionValue, SourceLocation IfLoc) override {
+Results.emplace_back(ConditionRange, ConditionValue);
+  }
+};
+
 // Stub to collect data from PragmaOpenCLExtension callbacks.
 class PragmaOpenCLExtensionCallbacks : public PPCallbacks {
 public:
@@ -137,6 +160,15 @@
 return StringRef(B, E - B);
   }
 
+  StringRef GetSourceStringToEnd(CharSourceRange Range) {
+const char *B = SourceMgr.getCharacterData(Range.getBegin());
+const char *E = SourceMgr.getCharacterData(Range.getEnd());
+
+return StringRef(
+B,
+E - B + Lexer::MeasureTokenLength(Range.getEnd(), SourceMgr, LangOpts));
+  }
+
   // Run lexer over SourceText and collect FilenameRange from
   // the InclusionDirective callback.
   CharSourceRange InclusionDirectiveFilenameRange(const char *SourceText,
@@ -199,6 +231,36 @@
 return Callbacks;
   }
 
+  std::vector
+  DirectiveExprRange(StringRef SourceText) {
+TrivialModuleLoader ModLoader;
+MemoryBufferCache PCMCache;
+std::unique_ptr Buf =
+llvm::MemoryBuffer::getMemBuffer(SourceText);
+SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(Buf)));
+HeaderSearch HeaderInfo(std::make_shared(), SourceMgr,
+Diags, LangOpts, Target.get());
+Preprocessor PP(std::make_shared(), Diags, LangOpts,
+SourceMgr, PCMCache, HeaderInfo, ModLoader,
+/*IILookup =*/nullptr,
+/*OwnsHeaderSearch =*/false);
+PP.Initialize(*Target);
+auto *Callbacks = new CondDirectiveCallbacks;
+PP.addPPCallbacks(std::unique_ptr(Callbacks));
+
+// Lex source text.
+PP.EnterMainSourceFile();
+
+while (true) {
+  Token Tok;
+  PP.Lex(Tok);
+  if (Tok.is(tok::eof))
+break;
+}
+
+return Callbacks->Results;
+  }
+
   PragmaOpenCLExtensionCallbacks::CallbackParameters
   PragmaOpenCLExtensionCall(const char *SourceText) {
 LangOptions OpenCLLangOpts;
@@ -368,4 +430,42 @@
   ASSERT_EQ(ExpectedState, Parameters.State);
 }
 
-} // anonoymous namespace
+TEST_F(PPCallbacksTest, DirectiveExprRanges) {
+  const auto &Results1 = DirectiveExprRange("#if FLUZZY_FLOOF\n#endif\n");
+  EXPECT_EQ(Results1.size(), 1);
+  EXPECT_EQ(
+  GetSourceStringToEnd(CharSourceRange(Results1[0].ConditionRange, false)),
+  "FLUZZY_FLOOF");
+
+  const auto &Results2 = DirectiveExprRange("#if 1 + 4 < 7\n#endif\n");
+  EXPECT_EQ(Results2.size(), 1);
+  EXPECT_EQ(
+  GetSourceStringToEnd(CharSourceRange(Results2[0].ConditionRange, false)),
+  "1 + 4 < 7");
+
+  const auto &Results3 = DirectiveExprRange("#if 1 + \\\n  2\n#endif\n");
+  EXPECT_EQ(Results3.size(), 1);
+  EXPECT_EQ(
+  GetSourceStringToEnd(CharSourceRange(Results3[0].ConditionRange, false)),
+  "1 + \\\n  2");
+
+  const auto &Results4 = DirectiveExprRange("#if 0\n#elif FLOOFY\n#endif\n");
+  EXPECT_EQ(Results4.size(), 2);
+  EXPECT_EQ(
+  GetSourceStringToEnd(CharSourceRange(Results4[0].ConditionRange, false)),
+  "0");
+  EXPECT_EQ(
+  GetSourceStringToEnd(CharSourceRange(Results4[1].ConditionRange, false)),
+  "FLOOFY");
+
+  const auto &Results5 = DirectiveExprRange("#if 1\n#elif FLOOFY\n#endif\n");
+  EXPECT_EQ(Results5.size(), 2);
+  EXPECT_EQ(
+  GetSourceStringToEnd(CharSourceRange(Results5[0].ConditionRange, false)),
+  "1");
+  EXPECT_EQ(
+  GetSourceStringToEnd(CharSourceRange(Results5[1].ConditionRange, false)),
+  "FLOOFY");
+}
+
+} // namespace
Index: lib/Lex/PPExpressions.cpp
===
--- lib/Lex/PPExpressions.cpp
+++ lib/Lex/PPExpressions.cpp
@@ -152,8 +152,8 @@
   return true;
 }
 // Consume the ).
-Result.setEnd(PeekTok.getLocation());
 PP.LexNonComment(PeekTok);
+Result.setEnd(PeekTok.getLocation());
   } else {

[PATCH] D54450: Get the correct range of tokens for preprocessor conditions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D54450#1298319, @vmiklos wrote:

> Oh, and running the `check-clang-tools` target, this currently results in:
>
>   Failing Tests (3):
>   Clang Tools :: modularize/ProblemsInconsistent.modularize
>   Clang Tools :: pp-trace/pp-trace-conditional.cpp
>   Clang Tools :: pp-trace/pp-trace-macro.cpp
>
>
> Perhaps the pp-trace one just has to be updated for the new behavior. :-)


Good catch -- I didn't run the clang-tools-extra tests. The pp-trace tests 
needed simple updating, but the modularize test has changed behavior and I'm 
not entirely certain of why. I've attached a file for the clang-tools-extra 
test changes: F7547470: preproc2.patch  I am 
entirely unfamiliar with the "modularize" tool and there was a surprising 
change in behavior there, but I think the behavior is still okay (or, at least, 
no worse than the previous behavior which was already suspicious in that test 
case).




Comment at: lib/Lex/PPDirectives.cpp:553
 } else {
   const SourceLocation CondBegin = CurPPLexer->getSourceLocation();
   // Restore the value of LexingRawMode so that identifiers are

vmiklos wrote:
> Is `CondBegin` still needed after your changes?
Nope, good catch!



Comment at: lib/Lex/PPExpressions.cpp:852
 DisableMacroExpansion = DisableMacroExpansionAtStartOfDirective;
 return {false, DT.IncludedUndefinedIds};
   }

vmiklos wrote:
> The new `ExprRange` member is not initialized here, it seems.
> 
`ExprRange` is default initialized in that case, so the default constructor is 
called as expected.


https://reviews.llvm.org/D54450



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

[PATCH] D52835: [Diagnostics] Check integer to floating point number implicit conversions

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Are you sure `check-clang` passes with this patch?
It looks like some bots weren't happy before the revert.
(Also, is this warning enabled when building llvm? If yes, you might want to 
double-check stage2 build.)


Repository:
  rC Clang

https://reviews.llvm.org/D52835



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

[PATCH] D54529: [clangd] Add USR to textDocument/definition response

[Jan Korous via Phabricator via cfe-commits]

jkorous created this revision.
jkorous added reviewers: sammccall, ilya-biryukov, arphaman, benlangmuir.
jkorous added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, kadircet, dexonsmith, MaskRay, ioeric.

We need a way for given code position to get the best definition/declaration 
location in given translation unit and it's USR.

Since the first element in list returned by textDocument/definition is meeting 
the def/decl location requirement we'd just need to add the USR. Other option 
would be to split this to a separate method. I thought this patch to be a 
better fit from protocol perspective but I am happy to go the other way if 
that's preferred.

I'd like to ask if someone more familiar with gtest matchers could take a look 
at changes I made in unit-tests. I am not sure it's exactly the idiomatic way. 
I was trying to use Property() matchers first but gave up since I wasn't able 
to debug it (got lost in template error messages).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54529

Files:
  ClangdLSPServer.cpp
  ClangdLSPServer.h
  ClangdServer.cpp
  ClangdServer.h
  Protocol.cpp
  Protocol.h
  XRefs.cpp
  XRefs.h
  clangd/ClangdTests.cpp
  clangd/SyncAPI.cpp
  clangd/SyncAPI.h
  clangd/XRefsTests.cpp
  clangd/textdocument-didchange-fail.test
  clangd/xrefs.test

Index: clangd/XRefsTests.cpp
===
--- clangd/XRefsTests.cpp
+++ clangd/XRefsTests.cpp
@@ -94,6 +94,11 @@
 }
 
 MATCHER_P(RangeIs, R, "") { return arg.range == R; }
+MATCHER_P(RangeInLocationIs, R, "") { return arg.DefOrDeclLocation.range == R; }
+MATCHER_P(RangeInLocationIsRH, R, "") {
+  return arg == R.DefOrDeclLocation.range;
+}
+MATCHER_P(LocationIs, R, "") { return arg.DefOrDeclLocation == R; }
 
 TEST(GoToDefinition, WithIndex) {
   Annotations SymbolHeader(R"cpp(
@@ -124,35 +129,39 @@
   ^f1();
 }
   )cpp");
-  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
-  testing::ElementsAreArray(
-  {RangeIs(SymbolCpp.range("f1")), RangeIs(Test.range())}));
+  EXPECT_THAT(
+  runFindDefinitionsWithIndex(Test),
+  testing::ElementsAreArray({RangeInLocationIs(SymbolCpp.range("f1")),
+ RangeInLocationIs(Test.range())}));
 
   Test = Annotations(R"cpp(// definition in AST.
 void [[f1]]() {}
 int main() {
   ^f1();
 }
   )cpp");
-  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
-  testing::ElementsAreArray(
-  {RangeIs(Test.range()), RangeIs(SymbolHeader.range("f1"))}));
+  EXPECT_THAT(
+  runFindDefinitionsWithIndex(Test),
+  testing::ElementsAreArray({RangeInLocationIs(Test.range()),
+ RangeInLocationIs(SymbolHeader.range("f1"))}));
 
   Test = Annotations(R"cpp(// forward declaration in AST.
 class [[Foo]];
 F^oo* create();
   )cpp");
-  EXPECT_THAT(runFindDefinitionsWithIndex(Test),
-  testing::ElementsAreArray(
-  {RangeIs(SymbolHeader.range("foo")), RangeIs(Test.range())}));
+  EXPECT_THAT(
+  runFindDefinitionsWithIndex(Test),
+  testing::ElementsAreArray({RangeInLocationIs(SymbolHeader.range("foo")),
+ RangeInLocationIs(Test.range())}));
 
   Test = Annotations(R"cpp(// defintion in AST.
 class [[Forward]] {};
 F^orward create();
   )cpp");
   EXPECT_THAT(runFindDefinitionsWithIndex(Test),
   testing::ElementsAreArray({
-  RangeIs(Test.range()), RangeIs(SymbolHeader.range("forward")),
+  RangeInLocationIs(Test.range()),
+  RangeInLocationIs(SymbolHeader.range("forward")),
   }));
 }
 
@@ -302,12 +311,13 @@
   for (const char *Test : Tests) {
 Annotations T(Test);
 auto AST = TestTU::withCode(T.code()).build();
-std::vector> ExpectedLocations;
-for (const auto &R : T.ranges())
-  ExpectedLocations.push_back(RangeIs(R));
-EXPECT_THAT(findDefinitions(AST, T.point()),
-ElementsAreArray(ExpectedLocations))
-<< Test;
+
+auto definitions = findDefinitions(AST, T.point());
+std::vector> Assertions;
+for (const auto &D : definitions)
+  Assertions.push_back(RangeInLocationIsRH(D));
+
+EXPECT_THAT(T.ranges(), ElementsAreArray(Assertions)) << Test;
   }
 }
 
@@ -334,22 +344,27 @@
   )cpp");
   auto AST = TestTU::withCode(T.code()).build();
   EXPECT_THAT(findDefinitions(AST, T.point("1")),
-  ElementsAre(RangeIs(T.range("str")), RangeIs(T.range("foo4";
+  ElementsAre(RangeInLocationIs(T.range("str")),
+  RangeInLocationIs(T.range("foo4";
   EXPECT_THAT(findDefinitions(AST, T.point("2")),
-  ElementsAre(RangeIs(T.range("str";
+  ElementsAre(RangeInLocationIs(T.range("str";
   EXPECT_THAT(findDefinitions(AST, T.point("3")),
- 

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added a comment.

Thx for the review. I have two suggestions in the comments let me know what you 
think.




Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:79-81
+  // TODO: Provide an automatic fix if the number is exactly representable in 
the destination type.
+  f += 2.0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 
constant 'double' to 'float' [cppcoreguidelines-narrowing-conversions]

aaron.ballman wrote:
> This is not a narrowing conversion -- there should be no diagnostic here. See 
> [dcl.init.list]p7.
Thx for pointing this out.
I would like to keep the diagnostic (without mentioning the narrowing) because 
there is no reason to write `2.0` instead of `2.0f`. WDYT?



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:99
+void narrow_integer_to_floating() {
+  {
+long long ll; // 64 bits

aaron.ballman wrote:
> Missing tests involving literals, which are not narrowing conversions. e.g., 
> `float f = 1ULL;` should not diagnose.
This is handled on l.262



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:174
+  c = uc;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 
'unsigned char' to 'char' is implementation-defined 
[cppcoreguidelines-narrowing-conversions]
+  c = us;

aaron.ballman wrote:
> This is only true on architectures where `char` is defined to be `signed 
> char` rather than `unsigned char`.
This is taken care of. The test fails when adding `-funsigned-char`.
The current test is executed with `-target x86_64-unknown-linux` which should 
imply `-fsigned-char`. Anyways, I'll add `-fsigned-char` to make sure this is 
obvious.



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:188
+  i = l;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'long' 
to 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  i = ll;

aaron.ballman wrote:
> This is only narrowing if `sizeof(int) < sizeof(long)` which is not the case 
> on all architectures.
Yes this is taken care of in the code, the size of the type must be smaller.
It works here because the test runs with `-target x86_64-unknown-linux`



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:211
+  ll = ul;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 
'unsigned long' to 'long long' is implementation-defined 
[cppcoreguidelines-narrowing-conversions]
+  ll = ull;

aaron.ballman wrote:
> Wha? How does one narrow from an unsigned 32-bit number to a signed 
> 64-bit number?
`unsigned long` is unsigned 64-bit so it does not fit in signed 64-bit.
https://godbolt.org/z/VnmG0s



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:228
+void narrow_constant_to_signed_integer_is_not_ok() {
+  char c1 = -128;
+  char c2 = 127;

aaron.ballman wrote:
> This may be narrowing on an implementation that defines `char` to be 
> `unsigned`.
Same as previous comments. Would it make sense to create a different test with 
`-funsigned-char` to check these behaviors?



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:232
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: narrowing conversion from 
constant value -129 (0xFF7F) of type 'int' to 'char' is 
implementation-defined [cppcoreguidelines-narrowing-conversions]
+  char c4 = 128;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: narrowing conversion from 
constant value 128 (0x0080) of type 'int' to 'char' is 
implementation-defined [cppcoreguidelines-narrowing-conversions]

aaron.ballman wrote:
> This may be fine on implementations that define `char` to be `unsigned`.
ditto


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488



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

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet updated this revision to Diff 174044.
gchatelet marked 7 inline comments as done.
gchatelet added a comment.

- State char signess clearly


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488

Files:
  clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
  clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
  test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp

Index: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
===
--- test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
+++ test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t -config="{CheckOptions: [{key: "cppcoreguidelines-narrowing-conversions.WarnOnFloatingPointNarrowingConversion", value: 1}]}" -- -target x86_64-unknown-linux -fsigned-char
 
 float ceil(float);
 namespace std {
@@ -9,47 +9,290 @@
 namespace floats {
 
 struct ConvertsToFloat {
-  operator float() const { return 0.5; }
+  operator float() const { return 0.5f; }
 };
 
-float operator "" _Pa(unsigned long long);
+float operator"" _float(unsigned long long);
 
-void not_ok(double d) {
+void narrow_double_to_int_not_ok(double d) {
   int i = 0;
   i = d;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
   i = 0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i = static_cast(d);
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i = ConvertsToFloat();
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
-  i = 15_Pa;
+  i = 15_float;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
 }
 
-void not_ok_binary_ops(double d) {
+void narrow_double_to_int_not_ok_binary_ops(double d) {
   int i = 0;
   i += 0.5;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
   i += 0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i += d;
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
   // We warn on the following even though it's not dangerous because there is no
   // reason to use a double literal here.
-  // TODO(courbet): Provide an automatic fix.
+  // TODO: Provide an automatic fix if the number is exactly representable in the destination type.
   i += 2.0;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
   i += 2.0f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
 
   i *= 0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i /= 0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i += (double)0.5f;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+}
+
+double operator"" _double(unsigned long long);
+
+float narr

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

[Aaron Ballman via Phabricator via cfe-commits]

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

In https://reviews.llvm.org/D54258#1297687, @richardmembarth wrote:

> > In https://reviews.llvm.org/D54258#1297191, @Anastasia wrote:
> > 
> > I agree with the change itself... but it's quite annoying that such things 
> > can't be tested. :(
>
> Yes, that's a pity :(


Yeah, it seems like trying to change this in the frontend would also be a fair 
amount of work.

> Is there anything missing so that this can be merged?

No, I think this truly is untestable without major work. Given how trivial this 
change is, I'm okay with it.


Repository:
  rC Clang

https://reviews.llvm.org/D54258



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

[PATCH] D53329: Generate DIFile with main program if source is not available

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

The above segment looks like

  #0  0x00a1 in ?? ()
  #1  0x01490d63 in clang::SourceManager::~SourceManager() ()
  #2  0x005ad0ee in clang::CompilerInstance::~CompilerInstance() ()
  #3  0x0056ad1b in main ()

In my example, I did not implement the customized SourceManager while the real 
bcc implements it
https://github.com/iovisor/bcc/blob/master/src/cc/bpf_module.cc#L73

I will try to implement this later. With the attached tar file, you should be 
still observe the missing
file contents. And my patch does fix this particular issue.


Repository:
  rC Clang

https://reviews.llvm.org/D53329



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

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:79-81
+  // TODO: Provide an automatic fix if the number is exactly representable in 
the destination type.
+  f += 2.0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 
constant 'double' to 'float' [cppcoreguidelines-narrowing-conversions]

gchatelet wrote:
> aaron.ballman wrote:
> > This is not a narrowing conversion -- there should be no diagnostic here. 
> > See [dcl.init.list]p7.
> Thx for pointing this out.
> I would like to keep the diagnostic (without mentioning the narrowing) 
> because there is no reason to write `2.0` instead of `2.0f`. WDYT?
I see no reason to warn on a literal value that isn't changed as a result of 
the notional narrowing. The standard clearly defines the behavior, so I think 
that would be needlessly chatty.



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:99
+void narrow_integer_to_floating() {
+  {
+long long ll; // 64 bits

gchatelet wrote:
> aaron.ballman wrote:
> > Missing tests involving literals, which are not narrowing conversions. 
> > e.g., `float f = 1ULL;` should not diagnose.
> This is handled on l.262
Ah, I missed that, thank you!



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:174
+  c = uc;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 
'unsigned char' to 'char' is implementation-defined 
[cppcoreguidelines-narrowing-conversions]
+  c = us;

gchatelet wrote:
> aaron.ballman wrote:
> > This is only true on architectures where `char` is defined to be `signed 
> > char` rather than `unsigned char`.
> This is taken care of. The test fails when adding `-funsigned-char`.
> The current test is executed with `-target x86_64-unknown-linux` which should 
> imply `-fsigned-char`. Anyways, I'll add `-fsigned-char` to make sure this is 
> obvious.
Ah, thank you! Can you also pass `-funsigned-char` to test the behavior is as 
expected?



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:188
+  i = l;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'long' 
to 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+  i = ll;

gchatelet wrote:
> aaron.ballman wrote:
> > This is only narrowing if `sizeof(int) < sizeof(long)` which is not the 
> > case on all architectures.
> Yes this is taken care of in the code, the size of the type must be smaller.
> It works here because the test runs with `-target x86_64-unknown-linux`
Please add a test for another target where this is not the case.



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:211
+  ll = ul;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 
'unsigned long' to 'long long' is implementation-defined 
[cppcoreguidelines-narrowing-conversions]
+  ll = ull;

gchatelet wrote:
> aaron.ballman wrote:
> > Wha? How does one narrow from an unsigned 32-bit number to a signed 
> > 64-bit number?
> `unsigned long` is unsigned 64-bit so it does not fit in signed 64-bit.
> https://godbolt.org/z/VnmG0s
unsigned long is not 64-bit on all targets, and that should be tested. 
https://godbolt.org/z/VQx-Yy


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488



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

[PATCH] D54529: [clangd] Add USR to textDocument/definition response

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Hi Jan,

Clangd uses SymbolIDs rather than USRs, to identify symbols.

However these are used only internally, and for extension point APIs 
(SymbolIndex), and not actually exposed over the wire.
Can you explain more about the need to identify the symbol in go-to-definition?

Conceptually the idea of providing at least an opaque identifier when naming 
symbols makes sense - the cursor position can often be ambiguous.
However LSP decided not to go this route, and we risk diverging (e.g. should 
"hover" be able to accept such an identifier too? etc).

Giving particular semantics to the identifiers seems much more risky.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54529



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

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/Basic/Attr.td:2096
+def TriviallyRelocatable : InheritableAttr {
+  let Spellings = [CXX11<"", "trivially_relocatable", 200809>,
+   CXX11<"clang", "trivially_relocatable">];

erichkeane wrote:
> Quuxplusone wrote:
> > erichkeane wrote:
> > > This spelling is almost definitely not acceptable until it is in the 
> > > standard.  Also, why specify that it was added in 2008?
> > Agreed, it should be `[[clang::trivially_relocatable]]` for Clang's 
> > purposes. This spelling was because this patch came from the Godbolt 
> > Compiler Explorer patch where I wanted the shorter/future spelling for 
> > public relations reasons. :)
> > IIUC, the appropriate fix here is to change these two lines from
> > ```
> > let Spellings = [CXX11<"", "trivially_relocatable", "200809">,
> >   CXX11<"clang", "trivially_relocatable">];
> > ```
> > to
> > ```
> > let Spellings = [Clang<"trivially_relocatable">,
> > ```
> > I believe the "200809" was because I wanted it to be available in C++11 
> > mode on Godbolt.
> Yep, thats the suggested fix.
> to
> `let Spellings = [Clang<"trivially_relocatable">,`

Close, but since this attribute has no meaning in C mode, it should be `let 
Spellings = [Clang<"trivially_relocatable", 0>,`



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8207
 
+def err_attribute_missing_on_first_decl : Error<
+  "type %0 declared %1 after its first declaration">;

erichkeane wrote:
> Quuxplusone wrote:
> > erichkeane wrote:
> > > I'm shocked that there isn't a different diagnostic to do this same 
> > > thing.  @aaron.ballman likely knows better...  I haven't seen the usage 
> > > yet, but I presume you don't necessarily want a behavior that doesn't 
> > > allow forward declarations.
> > I would be very happy to see this diagnostic get into trunk separately and 
> > earlier than D50119. There are some other diagnostics that could be merged 
> > with this one, e.g. `[[noreturn]]` needs a version of this diagnostic, and 
> > I believe `[[clang::trivial_abi]]` should have it added.
> > 
> > I don't know how to link to comments on Phabricator, but Ctrl+F downward 
> > for this example:
> > ```
> > struct S { S(S&&); ~S(); };
> > std::vector vec;
> > struct [[trivially_relocatable]] S;  // ha ha, now you have to re-do all of 
> > vector's codegen!
> > ```
> > This is why it is important to diagnose and disallow "backward 
> > declarations." I don't particularly care about "forward declarations" 
> > (either to allow or disallow them). The attribute would end up getting used 
> > only on library types where IMHO nobody should ever be forward-declaring 
> > them anyway. E.g. it is not a good idea for a regular C++ programmer to 
> > forward-declare `unique_ptr`. But if there's a way to allow 
> > forward-declarations (when the type remains incomplete) while disallowing 
> > backward-declarations (adding the attribute to an already-complete type), 
> > then I will be happy to do it.
> Sure, I get that... I'm just surprised/amazed it doesn't already exist.  
> Hopefully Aaron takes a look.
Yeah, I'm also surprised this doesn't already exist, but... it doesn't seem to. 
We have `err_noreturn_missing_on_first_decl`, 
`err_carries_dependency_missing_on_first_decl`, and 
`err_internal_linkage_redeclaration` to all do the same thing.


Repository:
  rC Clang

https://reviews.llvm.org/D50119



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

[PATCH] D54529: [clangd] Add USR to textDocument/definition response

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

One more thing - shall I create new client capability flag for this?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54529



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

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: docs/LanguageExtensions.rst:1096
+  equivalent to copying the underlying bytes and then dropping the source 
object
+  on the floor.
 * ``__is_destructible`` (MSVC 2013)

rjmccall wrote:
> Quuxplusone wrote:
> > rjmccall wrote:
> > > Quuxplusone wrote:
> > > > rjmccall wrote:
> > > > > Quuxplusone wrote:
> > > > > > rjmccall wrote:
> > > > > > > Quuxplusone wrote:
> > > > > > > > @rjmccall wrote:
> > > > > > > > > trivial_abi permits annotated types to be passed and returned 
> > > > > > > > > in registers, which is ABI-breaking. Skimming the blog post, 
> > > > > > > > > it looks like trivially_relocatable does not permit this — it 
> > > > > > > > > merely signifies that destruction is a no-op after a move 
> > > > > > > > > construction or assignment.
> > > > > > > > 
> > > > > > > > Not necessarily a "no-op"; my canonical example is a 
> > > > > > > > CopyOnlyCXX03SharedPtr which increments a refcount on 
> > > > > > > > construction and decrements on destruction. But 
> > > > > > > > move-construction plus destruction should "balance out" and 
> > > > > > > > result in no observable side effects.
> > > > > > > > 
> > > > > > > > > This is usefully different in the design space, since it 
> > > > > > > > > means you can safely add the attribute retroactively to e.g. 
> > > > > > > > > std::unique_ptr, and other templates can then detect that 
> > > > > > > > > std::unique_ptr is trivially-relocatable and optimize 
> > > > > > > > > themselves to use memcpy or realloc or whatever it is that 
> > > > > > > > > they want to do. So in that sense trivial_abi is a *stronger* 
> > > > > > > > > attribute, not a *weaker* one: the property it determines 
> > > > > > > > > ought to imply trivially_relocatable.
> > > > > > > > 
> > > > > > > > `trivial_abi` is an "orthogonal" attribute: you can have 
> > > > > > > > `trivial_abi` types with non-trivial constructors and 
> > > > > > > > destructors, which can have observable side effects. For 
> > > > > > > > example,
> > > > > > > > ```
> > > > > > > > struct [[clang::trivial_abi]] DestructionAnnouncer {
> > > > > > > > ~DestructionAnnouncer() { puts("hello!"); }
> > > > > > > > };
> > > > > > > > ```
> > > > > > > > is `trivial_abi` (because of the annotation) yet not trivially 
> > > > > > > > relocatable, because its "move plus destroy" operation has 
> > > > > > > > observable side effects.
> > > > > > > > 
> > > > > > > > > The only interesting question in the language design that I 
> > > > > > > > > know of is what happens if you put the attribute on a 
> > > > > > > > > template that's instantiated to contain a sub-object that is 
> > > > > > > > > definitely not trivially relocatable / trivial-ABI. For 
> > > > > > > > > trivial_abi, we decided that the attribute is simply ignored 
> > > > > > > > > — it implicitly only applies to specializations where the 
> > > > > > > > > attribute would be legal. I haven't dug into the design 
> > > > > > > > > enough to know what trivially_relocatable decides in this 
> > > > > > > > > situation, but the three basic options are:
> > > > > > > > >
> > > > > > > > > - the attribute always has effect and allows trivial 
> > > > > > > > > relocation regardless of the subobject types; this is 
> > > > > > > > > obviously unsafe, so it limits the safe applicability of the 
> > > > > > > > > attribute to templates
> > > > > > > > > - the attribute is ignored, like trivial_abi is
> > > > > > > > > - the attribute is ill-formed, and you'll need to add a 
> > > > > > > > > [[trivially_relocatable(bool)]] version to support templates
> > > > > > > > 
> > > > > > > > What happens is basically the first thing you said, except that 
> > > > > > > > I disagree that it's "obviously unsafe." Right now, 
> > > > > > > > conditionally trivial relocation is possible via template 
> > > > > > > > metaprogramming; see the libcxx patch at e.g.
> > > > > > > > https://github.com/Quuxplusone/libcxx/commit/6524822c009e#diff-38adc80cec663f2f29c22e9ffc0de912
> > > > > > > > Since the attribute is an opt-in mechanism, it makes perfect 
> > > > > > > > sense to me that if you put it on a class (or class template), 
> > > > > > > > then it applies to the class, without any further 
> > > > > > > > sanity-checking by the compiler. The compiler has no reason to 
> > > > > > > > second-guess the programmer here.
> > > > > > > > 
> > > > > > > > However, there's one more interesting case. Suppose the 
> > > > > > > > programmer puts the attribute on a class that isn't relocatable 
> > > > > > > > at all! (For example, the union case @erichkeane mentioned, or 
> > > > > > > > a class type with a deleted destructor.) In that case, this 
> > > > > > > > patch *does* give an error... *unless* the class was produced 
> > > > > > > > by instantiating a template, in which case we *don't* give an 
> > > > > > > > error, because it's not the template-writer's fault.
> > > > 

[PATCH] D54269: Introduce shard storage to auto-index.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 174052.
kadircet added a comment.
Herald added a subscriber: mgorny.

- Address comments.
- Move storage related things to new files.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54269

Files:
  clangd/CMakeLists.txt
  clangd/index/Background.cpp
  clangd/index/Background.h
  clangd/index/BackgroundIndexStorage.cpp
  clangd/index/BackgroundIndexStorage.h
  unittests/clangd/BackgroundIndexTests.cpp

Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -43,7 +43,9 @@
   void f_b() {
 (void)common;
   })cpp";
-  BackgroundIndex Idx(Context::empty(), "", FS, /*URISchemes=*/{"unittest"});
+  BackgroundIndex Idx(
+  Context::empty(), "", FS, /*URISchemes=*/{"unittest"},
+  llvm::make_unique(nullptr));
 
   tooling::CompileCommand Cmd;
   Cmd.Filename = testPath("root/A.cc");
@@ -76,5 +78,75 @@
FileURI("unittest:///root/B.cc")}));
 }
 
+TEST(BackgroundIndexTest, ShardStorageWriteTest) {
+  class MemoryShardStorage : public BackgroundIndexStorage {
+mutable std::mutex StorageMu;
+llvm::StringMap &Storage;
+size_t &CacheHits;
+
+  public:
+MemoryShardStorage(llvm::StringMap &Storage, size_t &CacheHits)
+: Storage(Storage), CacheHits(CacheHits) {}
+llvm::Error storeShard(llvm::StringRef ShardIdentifier,
+   IndexFileOut Shard) const override {
+  std::lock_guard Lock(StorageMu);
+  std::string &str = Storage[ShardIdentifier];
+  llvm::raw_string_ostream OS(str);
+  OS << Shard;
+  OS.flush();
+  return llvm::Error::success();
+}
+std::unique_ptr
+loadShard(llvm::StringRef ShardIdentifier) const override {
+  std::lock_guard Lock(StorageMu);
+  if (Storage.find(ShardIdentifier) == Storage.end()) {
+ADD_FAILURE() << "Shard " << ShardIdentifier << ": not found.";
+return nullptr;
+  }
+  auto IndexFile = readIndexFile(Storage[ShardIdentifier]);
+  if (!IndexFile) {
+ADD_FAILURE() << "Error while reading " << ShardIdentifier << ':'
+  << IndexFile.takeError();
+return nullptr;
+  }
+  CacheHits++;
+  return llvm::make_unique(std::move(*IndexFile));
+}
+  };
+  MockFSProvider FS;
+  FS.Files[testPath("root/A.h")] = R"cpp(
+  void common();
+  void f_b();
+  class A_CC {};
+  )cpp";
+  FS.Files[testPath("root/A.cc")] =
+  "#include \"A.h\"\nvoid g() { (void)common; }";
+
+  llvm::StringMap Storage;
+  size_t CacheHits = 0;
+  BackgroundIndexStorageFactory::IndexStorageCreator MemoryStorageCreator =
+  [&](llvm::StringRef) {
+return llvm::make_unique(Storage, CacheHits);
+  };
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
+  std::unique_ptr BISF =
+  llvm::make_unique(
+  std::move(MemoryStorageCreator));
+  // Check nothing is loaded from Storage, but A.cc and A.h has been stored.
+  {
+BackgroundIndex Idx(Context::empty(), "", FS, /*URISchemes=*/{"unittest"},
+std::move(BISF));
+Idx.enqueue(testPath("root"), Cmd);
+Idx.blockUntilIdleForTest();
+  }
+  EXPECT_EQ(Storage.size(), 2U);
+  EXPECT_NE(Storage.find(testPath("root/A.h")), Storage.end());
+  EXPECT_NE(Storage.find(testPath("root/A.cc")), Storage.end());
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/index/BackgroundIndexStorage.h
===
--- /dev/null
+++ clangd/index/BackgroundIndexStorage.h
@@ -0,0 +1,63 @@
+//===--- Background.h - Build an index in a background thread *- 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_INDEX_BACKGROUND_INDEX_STORAGE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_BACKGROUND_INDEX_STORAGE_H
+
+#include "index/Serialization.h"
+#include "llvm/ADT/FunctionExtras.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+
+// Handles storage and retrieval of index shards. Both store and load
+// operations can be called from multiple-threads concurrently.
+class BackgroundIndexStorage {
+public:
+  // Shards of the index are stored and retrieved independently, keyed by shard
+  // identifier - in practice this is a source file name
+  virtual llvm::Error storeShard(llvm::StringRef ShardIdentifier,
+ IndexFileOut Shard) const = 0;
+
+  // Tries to load shard with given

[PATCH] D54269: Introduce shard storage to auto-index.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 9 inline comments as done.
kadircet added a comment.

I fear we still need to expose createDiskStorage, because someone still needs 
to tell factory to which creator function to use.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54269



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

[clang-tools-extra] r346872 - [clangd] Delete unused includes.

[Kadir Cetinkaya via cfe-commits]

Author: kadircet
Date: Wed Nov 14 09:07:39 2018
New Revision: 346872

URL: http://llvm.org/viewvc/llvm-project?rev=346872&view=rev
Log:
[clangd] Delete unused includes.

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

Modified: clang-tools-extra/trunk/unittests/clangd/TestFS.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TestFS.cpp?rev=346872&r1=346871&r2=346872&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/TestFS.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/TestFS.cpp Wed Nov 14 09:07:39 2018
@@ -8,11 +8,9 @@
 
//===--===//
 #include "TestFS.h"
 #include "URI.h"
-#include "clang/AST/DeclCXX.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
-#include "gtest/gtest.h"
 
 namespace clang {
 namespace clangd {


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

[PATCH] D54535: cmake: z3: Remove EXACT from 4.7.1 after being compatible with 4.8.1

[Jan Kratochvil via Phabricator via cfe-commits]

jankratochvil created this revision.
jankratochvil added a reviewer: mikhail.ramalho.
jankratochvil added a project: clang.
Herald added subscribers: cfe-commits, mgorny.

D54391 comment by @mikhail.ramalho  
says:

> Since we're supporting version 4.8.1 now, the cmake file should be changed to 
> "minimum" instead of "exact".


Repository:
  rC Clang

https://reviews.llvm.org/D54535

Files:
  CMakeLists.txt


Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -410,7 +410,7 @@
 
 set(CLANG_ANALYZER_Z3_INSTALL_DIR "" CACHE STRING "Install directory of the Z3 
solver.")
 
-find_package(Z3 4.7.1 EXACT)
+find_package(Z3 4.7.1)
 
 if (CLANG_ANALYZER_Z3_INSTALL_DIR)
   if (NOT Z3_FOUND)


Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -410,7 +410,7 @@
 
 set(CLANG_ANALYZER_Z3_INSTALL_DIR "" CACHE STRING "Install directory of the Z3 solver.")
 
-find_package(Z3 4.7.1 EXACT)
+find_package(Z3 4.7.1)
 
 if (CLANG_ANALYZER_Z3_INSTALL_DIR)
   if (NOT Z3_FOUND)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54529: [clangd] Add USR to textDocument/definition response

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Hi Sam!

I am aware of clangd using SymbolID. We basically need USR for integration with 
our external indexing service. We don't plan to systematically use it in the 
protocol and I am not aware of any other requirement for using USR in LSP - 
will double check that to be sure.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54529



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

[PATCH] D54437: [analyzer][NFC] Merge ClangCheckerRegistry to CheckerRegistry

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

Szelethus added a comment.
Herald added subscribers: gamesh411, baloghadamsoftware.

Unfortunately, I found //yet another// corner case I didn't cover: if the macro 
arguments themselves are macros. I already fixed it, but it raises the question 
that what other things I may have missed? I genuinely dislike this project, and 
I fear that I can't test this enough to be be 100% it does it's job perfectly. 
Heck, I'm a little unsure whether it wouldn't ever cause a crash. This is 
unnerving to put it lightly, but doing changes within `Preprocessor` is //most 
definitely// out of my reach.

Maybe it'd be worth to caution the users of this feature that it's 
experimental, but then again, it will always be, as long as there's no 
out-of-the-box solution from `Preprocessor`.


Repository:
  rC Clang

https://reviews.llvm.org/D54437



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

[PATCH] D52795: [analyzer][PlistMacroExpansion] Part 3.: Macro arguments are expanded

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

Szelethus added a comment.
Herald added a subscriber: gamesh411.

Unfortunately, I found //yet another// corner case I didn't cover: if the macro 
arguments themselves are macros. I already fixed it, but it raises the question 
that what other things I may have missed? I genuinely dislike this project, and 
I fear that I can't test this enough to be be 100% it does it's job perfectly. 
Heck, I'm a little unsure whether it wouldn't ever cause a crash. This is 
unnerving to put it lightly, but doing changes within `Preprocessor` is //most 
definitely// out of my reach.

Maybe it'd be worth to caution the users of this feature that it's 
experimental, but then again, it will always be, as long as there's no 
out-of-the-box solution from `Preprocessor`.


https://reviews.llvm.org/D52795



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

[PATCH] D54529: [clangd] Add USR to textDocument/definition response

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

We are also discussing creating separate method as we'll likely want to add 
other data to the response in the future. Would you prefer USR not to be in the 
standard part of LSP but only in our extension?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54529



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

[PATCH] D53329: Generate DIFile with main program if source is not available

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

If I am using regular files instead of remapped files, I did not see the 
segfault. So is it possible that this is a clang issue?


Repository:
  rC Clang

https://reviews.llvm.org/D53329



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

[PATCH] D53764: [OpenCL] Enable address spaces for references in C++

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Sema/SemaExprCXX.cpp:4289
+ /*BasePath=*/nullptr, CCK)
+   .get();
 

Anastasia wrote:
> rjmccall wrote:
> > Anastasia wrote:
> > > rjmccall wrote:
> > > > Okay.  But if `ToType` *isn't* a reference type, this will never be an 
> > > > address-space conversion.  I feel like this code could be written more 
> > > > clearly to express what it's trying to do.
> > > I hope it makes more sense now. Btw, it also applies to pointer type.
> > The logic is wrong for pointer types; if you're converting pointers, you 
> > need to be checking the address space of the pointee type of the from type.
> > 
> > It sounds like this is totally inadequately tested; please flesh out the 
> > test with all of these cases.  While you're at it, please ensure that there 
> > are tests verifying that we don't allowing address-space changes in nested 
> > positions.
> Thanks for spotting this bug! The generated IR for the test was still correct 
> because AS of `FromType` happened to correctly mismatch AS of pointee of 
> `ToType`.
> 
> I failed to construct the test case where it would miss classifying 
> `addrspacecast` due to OpenCL or C++ sema rules but I managed to add a case 
> in which `addrspacecast` was incorrectly added for pointers where it wasn't 
> needed (see line 36 of the test). I think this code is covered now.
> 
> As for the address space position in pointers, the following test checks the 
> address spaces of pointers in `addrspacecast`. For the other program paths we 
> also have a test with similar checks in 
> `test/CodeGenOpenCL/address-spaces-conversions.cl` that we now run for C++ 
> mode too.
> 
> BTW, while trying to construct a test case for the bug, I have discovered 
> that multiple pointer indirection casting isn't working correctly. I.e. for 
> the following program:
>   kernel void foo(){
>  __private int** loc;
>  int** loc_p = loc;
>  **loc_p = 1;
>   }
> We generate:
>   bitcast i32* addrspace(4)* %0 to i32 addrspace(4)* addrspace(4)*
> in OpenCL C and then perform `store` over pointer in AS 4 (generic). We have 
> now lost the information that the original pointer was in `private` AS and 
> that the adjustment of AS segment has to be performed before accessing memory 
> pointed by the pointer. Based on the current specification of `addrspacecast` 
> in https://llvm.org/docs/LangRef.html#addrspacecast-to-instruction I am not 
> very clear whether it can be used for this case without any modifications or 
> clarifications and also what would happen if there are multiple AS 
> mismatches. I am going to look at this issue separately in more details. In 
> OpenCL C++ an ICE is triggered for this though. Let me know if you have any 
> thoughts on this.
Thanks, the check looks good now.

> BTW, while trying to construct a test case for the bug, I have discovered 
> that multiple pointer indirection casting isn't working correctly.

This needs to be an error in Sema.  The only qualification conversions that 
should be allowed in general on nested pointers (i.e. on `T` in `T**` or `T*&`) 
are the basic C qualifiers: `const`, `volatile`, and `restrict`; any other 
qualification change there is unsound.


https://reviews.llvm.org/D53764



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

[PATCH] D54195: Fix linker option for -fprofile-arcs -ftest-coverage

[Marco Castelluccio via Phabricator via cfe-commits]

marco-c accepted this revision.
marco-c added a comment.
This revision is now accepted and ready to land.

Thanks!




Comment at: test/Driver/clang_f_opts.c:89
+// RUN: %clang -### -fprofile-arcs -ftest-coverage %s 2>&1 | FileCheck 
-check-prefix=CHECK-u %s
+// CHECK-u-NOT: "-u{{.*}}"
+

Could you add a test for --coverage too?


Repository:
  rC Clang

https://reviews.llvm.org/D54195



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

[PATCH] D54536: [AST] Fix typo in MicrosoftMangle

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai created this revision.
smeenai added reviewers: majnemer, rnk.

I noticed that we were spelling it as Artifical instead of Artificial
when working on the mangling for Obj-C RTTI. I'm putting this up for
review instead of committing it directly because I'm not 100% sure what
the policy here would be; it does clutter up the blame, but at the same
time it's fixing a gotcha for people writing new code.


Repository:
  rC Clang

https://reviews.llvm.org/D54536

Files:
  lib/AST/MicrosoftMangle.cpp

Index: lib/AST/MicrosoftMangle.cpp
===
--- lib/AST/MicrosoftMangle.cpp
+++ lib/AST/MicrosoftMangle.cpp
@@ -309,7 +309,7 @@
 const MethodVFTableLocation &ML);
   void mangleNumber(int64_t Number);
   void mangleTagTypeKind(TagTypeKind TK);
-  void mangleArtificalTagType(TagTypeKind TK, StringRef UnqualifiedName,
+  void mangleArtificialTagType(TagTypeKind TK, StringRef UnqualifiedName,
   ArrayRef NestedNames = None);
   void mangleType(QualType T, SourceRange Range,
   QualifierMangleMode QMM = QMM_Mangle);
@@ -1070,7 +1070,7 @@
   if (PointersAre64Bit)
 Out << 'E';
   Out << 'A';
-  mangleArtificalTagType(TTK_Struct,
+  mangleArtificialTagType(TTK_Struct,
  Discriminate("__block_literal", Discriminator,
   ParameterDiscriminator));
   Out << "@Z";
@@ -1365,7 +1365,7 @@
 // It's a global variable.
 Out << '3';
 // It's a struct called __s_GUID.
-mangleArtificalTagType(TTK_Struct, "__s_GUID");
+mangleArtificialTagType(TTK_Struct, "__s_GUID");
 // It's const.
 Out << 'B';
 return;
@@ -1521,9 +1521,9 @@
 
   Stream << "?$";
   Extra.mangleSourceName("Protocol");
-  Extra.mangleArtificalTagType(TTK_Struct, PD->getName());
+  Extra.mangleArtificialTagType(TTK_Struct, PD->getName());
 
-  mangleArtificalTagType(TTK_Struct, TemplateMangling, {"__ObjC"});
+  mangleArtificialTagType(TTK_Struct, TemplateMangling, {"__ObjC"});
 }
 
 void MicrosoftCXXNameMangler::mangleObjCLifetime(const QualType Type,
@@ -1552,7 +1552,7 @@
   Extra.manglePointerExtQualifiers(Quals, Type);
   Extra.mangleType(Type, Range);
 
-  mangleArtificalTagType(TTK_Struct, TemplateMangling, {"__ObjC"});
+  mangleArtificialTagType(TTK_Struct, TemplateMangling, {"__ObjC"});
 }
 
 void MicrosoftCXXNameMangler::mangleObjCKindOfType(const ObjCObjectType *T,
@@ -1569,7 +1569,7 @@
->getAs(),
Quals, Range);
 
-  mangleArtificalTagType(TTK_Struct, TemplateMangling, {"__ObjC"});
+  mangleArtificialTagType(TTK_Struct, TemplateMangling, {"__ObjC"});
 }
 
 void MicrosoftCXXNameMangler::mangleQualifiers(Qualifiers Quals,
@@ -1765,7 +1765,7 @@
   ArgBackRefMap::iterator Found = TypeBackReferences.find(TypePtr);
 
   if (Found == TypeBackReferences.end()) {
-mangleArtificalTagType(TTK_Enum, "__pass_object_size" + llvm::utostr(Type),
+mangleArtificialTagType(TTK_Enum, "__pass_object_size" + llvm::utostr(Type),
{"__clang"});
 
 if (TypeBackReferences.size() < 10) {
@@ -1951,13 +1951,13 @@
 llvm_unreachable("placeholder types shouldn't get to name mangling");
 
   case BuiltinType::ObjCId:
-mangleArtificalTagType(TTK_Struct, "objc_object");
+mangleArtificialTagType(TTK_Struct, "objc_object");
 break;
   case BuiltinType::ObjCClass:
-mangleArtificalTagType(TTK_Struct, "objc_class");
+mangleArtificialTagType(TTK_Struct, "objc_class");
 break;
   case BuiltinType::ObjCSel:
-mangleArtificalTagType(TTK_Struct, "objc_selector");
+mangleArtificialTagType(TTK_Struct, "objc_selector");
 break;
 
 #define IMAGE_TYPE(ImgType, Id, SingletonId, Access, Suffix) \
@@ -1967,40 +1967,40 @@
 #include "clang/Basic/OpenCLImageTypes.def"
   case BuiltinType::OCLSampler:
 Out << "PA";
-mangleArtificalTagType(TTK_Struct, "ocl_sampler");
+mangleArtificialTagType(TTK_Struct, "ocl_sampler");
 break;
   case BuiltinType::OCLEvent:
 Out << "PA";
-mangleArtificalTagType(TTK_Struct, "ocl_event");
+mangleArtificialTagType(TTK_Struct, "ocl_event");
 break;
   case BuiltinType::OCLClkEvent:
 Out << "PA";
-mangleArtificalTagType(TTK_Struct, "ocl_clkevent");
+mangleArtificialTagType(TTK_Struct, "ocl_clkevent");
 break;
   case BuiltinType::OCLQueue:
 Out << "PA";
-mangleArtificalTagType(TTK_Struct, "ocl_queue");
+mangleArtificialTagType(TTK_Struct, "ocl_queue");
 break;
   case BuiltinType::OCLReserveID:
 Out << "PA";
-mangleArtificalTagType(TTK_Struct, "ocl_reserveid");
+mangleArtificialTagType(TTK_Struct, "ocl_reserveid");
 break;
 #define EXT_OPAQUE_TYPE(ExtType, Id, Ext) \
   case BuiltinType::Id: \
-mangleArtificalTagType(TTK_Struct, "ocl_" #ExtType); \
+mangleArtificialTagType(TTK_Struct, "ocl_" #ExtType); \
 break;
 #in

[PATCH] D54529: [clangd] Add USR to textDocument/definition response

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Correction - I asked offline about our intended use of USR in LSP and it seems 
we might want to receive it as part of other responses too. Nothing specific 
for now but it's probable that it won't be just this singular case.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54529



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

[PATCH] D53764: [OpenCL] Enable address spaces for references in C++

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: lib/Sema/SemaExprCXX.cpp:4289
+ /*BasePath=*/nullptr, CCK)
+   .get();
 

rjmccall wrote:
> Anastasia wrote:
> > rjmccall wrote:
> > > Anastasia wrote:
> > > > rjmccall wrote:
> > > > > Okay.  But if `ToType` *isn't* a reference type, this will never be 
> > > > > an address-space conversion.  I feel like this code could be written 
> > > > > more clearly to express what it's trying to do.
> > > > I hope it makes more sense now. Btw, it also applies to pointer type.
> > > The logic is wrong for pointer types; if you're converting pointers, you 
> > > need to be checking the address space of the pointee type of the from 
> > > type.
> > > 
> > > It sounds like this is totally inadequately tested; please flesh out the 
> > > test with all of these cases.  While you're at it, please ensure that 
> > > there are tests verifying that we don't allowing address-space changes in 
> > > nested positions.
> > Thanks for spotting this bug! The generated IR for the test was still 
> > correct because AS of `FromType` happened to correctly mismatch AS of 
> > pointee of `ToType`.
> > 
> > I failed to construct the test case where it would miss classifying 
> > `addrspacecast` due to OpenCL or C++ sema rules but I managed to add a case 
> > in which `addrspacecast` was incorrectly added for pointers where it wasn't 
> > needed (see line 36 of the test). I think this code is covered now.
> > 
> > As for the address space position in pointers, the following test checks 
> > the address spaces of pointers in `addrspacecast`. For the other program 
> > paths we also have a test with similar checks in 
> > `test/CodeGenOpenCL/address-spaces-conversions.cl` that we now run for C++ 
> > mode too.
> > 
> > BTW, while trying to construct a test case for the bug, I have discovered 
> > that multiple pointer indirection casting isn't working correctly. I.e. for 
> > the following program:
> >   kernel void foo(){
> >  __private int** loc;
> >  int** loc_p = loc;
> >  **loc_p = 1;
> >   }
> > We generate:
> >   bitcast i32* addrspace(4)* %0 to i32 addrspace(4)* addrspace(4)*
> > in OpenCL C and then perform `store` over pointer in AS 4 (generic). We 
> > have now lost the information that the original pointer was in `private` AS 
> > and that the adjustment of AS segment has to be performed before accessing 
> > memory pointed by the pointer. Based on the current specification of 
> > `addrspacecast` in 
> > https://llvm.org/docs/LangRef.html#addrspacecast-to-instruction I am not 
> > very clear whether it can be used for this case without any modifications 
> > or clarifications and also what would happen if there are multiple AS 
> > mismatches. I am going to look at this issue separately in more details. In 
> > OpenCL C++ an ICE is triggered for this though. Let me know if you have any 
> > thoughts on this.
> Thanks, the check looks good now.
> 
> > BTW, while trying to construct a test case for the bug, I have discovered 
> > that multiple pointer indirection casting isn't working correctly.
> 
> This needs to be an error in Sema.  The only qualification conversions that 
> should be allowed in general on nested pointers (i.e. on `T` in `T**` or 
> `T*&`) are the basic C qualifiers: `const`, `volatile`, and `restrict`; any 
> other qualification change there is unsound.
I see. I guess it's because C++ rules don't cover address spaces.

It feels like it would be a regression for OpenCL C++ vs OpenCL C to reject 
nested pointers with address spaces because it was allowed before. :(

However, the generation for OpenCL C and C are incorrect currently. I will try 
to sort that all out as a separate patch though, if it makes sense? 


https://reviews.llvm.org/D53764



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

[PATCH] D53900: [CodeComplete] Penalize inherited ObjC properties for auto-completion

[David Goldman via Phabricator via cfe-commits]

dgoldman added a comment.

@sammccall to land this


Repository:
  rC Clang

https://reviews.llvm.org/D53900



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

[PATCH] D54529: [clangd] Add USR to textDocument/definition response

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

@sammccall RE using USRs: we want to integrate clangd into a larger source 
tooling system that is already using USRs extensively to identify symbols. The 
most obvious case is that we have an index outside of clangd that uses USRs 
from clang-the-compiler, so exposing USRs in clangd lets us integrate 
information from both places.  The most critical piece of this is being able to 
ask "what symbol is at this location" and correlate that with index queries. 
Beyond this one case, my experience is that any time a language service API is 
providing information about a symbol or list of symbols it may be useful to add 
the USR (e.g. hover, definition or workspace symbols).

You mentioned a concern that this would make us diverge from LSP - could you 
expand on that?  I think for any existing requests the USR is purely additive.  
If the concern is about affecting the return values of existing queries, would 
it help to put this behind a configuration option?

There is also an extension that might be interesting to look at here: 
https://github.com/sourcegraph/language-server-protocol/blob/master/extension-symbol-descriptor.md
  In this case the symbol descriptor is opaque, but in practice our use case 
would require knowing how to dig out a USR string.

For the specific use case of getting cursor info, as @jkorous mentioned we 
might want to move this out of "definition".  Our experience with sourcekitd 
for Swift is that a cursor info request is a useful place to put an assortment 
of information that may grow over time - e.g. name, type name, USR, type USR, 
symbol kind, local declaration/definition location, etc.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54529



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

[PATCH] D51866: [analyzer][UninitializedObjectChecker] New flag to ignore guarded uninitialized fields

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

Szelethus added a comment.
Herald added subscribers: gamesh411, baloghadamsoftware.

@george.karpenkov Matching macros is a very non-trivial job, how would you feel 
if we shipped this patch as-is, and maybe leave a TODO about adding macro 
`assert`s down the line?


https://reviews.llvm.org/D51866



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

[PATCH] D54246: [clang-tidy] Add the abseil-duration-factory-scale check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:63
+  assert(FloatLit != nullptr && "Neither IntLit nor FloatLit set");
+  return FloatLit->getValueAsApproximateDouble();
+}

hwright wrote:
> aaron.ballman wrote:
> > hwright wrote:
> > > aaron.ballman wrote:
> > > > I believe the approximate results here can lead to bugs where the 
> > > > floating-point literal is subnormal -- it may return 0.0 for literals 
> > > > that are not zero.
> > > Do you have an example which I could put in a test?
> > `0x0.01p-126f` should get you a new, exciting way to spell `0`.
> I've added this as a test, and it resolves as normal.  Was your comment 
> intended to indicate that it //should//, or that doing so would be a bug?
Hmm, in hindsight, I think this code is okay as-is. The situation I was worried 
about is where compile-time evaluation gives you one value (say, 0) while 
runtime evaluation gives you another (nonzero) because of the floating-point 
format for the target architecture. However, 1) I think APFloat does a good job 
of avoiding that, so we may be fine by default, and 2) I am questioning whether 
abseil users would be writing those constants anyway.



Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:73
+  case DurationScale::Hours:
+if (Multiplier <= 1.0 / 60.0)
+  return std::make_tuple(DurationScale::Minutes, Multiplier * 60.0);

This doesn't seem quite right. `1.0/6000.0` is less than `1.0/60.0` but isn't a 
`Minutes` scaling.


https://reviews.llvm.org/D54246



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

[PATCH] D54529: [clangd] Add USR to textDocument/definition response

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

If we're extending the API, we should be careful to do it in an orthogonal and 
reasonable general way. Apple may not need USR in other places now, but there 
are users other than Apple and times other than today :-)

One of the reasons this looks odd is the method is "find definitions" rather 
than "get symbol info". So e.g. it's odd to return the symbol USR here but not 
in "find references", and doing a lookup in the index is unnecessary if what 
you want want is the USR. So I do think a new "get symbol info" method is 
better, if that's what you want.

This is doable as an LSP extension, though it does give me pause to reconsider 
the design:

  - the C++ API is for this much easier to design: get a SymbolID & Decl* while 
the AST is alive, with the option of looking up in the index. With LSP, it's 
not obvious what to include/exclude.
  - an LSP call can't return any persistent handle to the symbol (i.e Decl*) as 
the AST has no lifetime guarantees. So any follow-up operation on the symbol 
needs to repeat work and handle errors (symbol gone?). This puts further 
pressure on orthogonality.
- it's early days, are there more? The more custom actions there are that don't 
fit LSP, the less coherent ClangdLSPServer becomes (it's meant to model LSP 
quite closely, where ClangdServer matches "reality")

So are we still sure "mostly unmodified clangd over a different transport with 
a few extensions" is a better fit for the xcode XPC service than "AST 
management from ClangdServer, some features from ClangdServer, some custom 
features built on TUScheduler, exposed over an XPC protocol inspired by LSP"?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54529



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

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:37
+CheckMacroRedundancy(Loc, Condition, IfStack,
+ "nested redundant if; consider removing it",
+ "previous if was here", true);

I think these diagnostics should be hoisted as private constant members of the 
class. Something like:
`nested redundant %select{#if|#ifdef|#ifndef}0; consider removing it"` and 
`previous %select{#if|#ifdef|#ifndef}0 here`



Comment at: test/clang-tidy/readability-redundant-preprocessor.cpp:45
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant if; consider removing 
it [readability-redundant-preprocessor]
+#if FOO == 4
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous if was here

Can you add a test like `#if FOO == 3 + 1` as well?


https://reviews.llvm.org/D54349



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