[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Please always upload all patches with full context (`=U9`)


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

https://reviews.llvm.org/D55044



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

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:55
+  IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true)),
+  UseLegacyFunction(Options.getLocalOrGlobal("UseLegacyFunction", false)) 
{}
 

I'm not sure it makes sense to look for global `UseLegacyFunction`?
It doesn't sound all that common.



Comment at: docs/clang-tidy/checks/modernize-make-unique.rst:52
+
+.. option:: UseLegacyFunction
+   

Name is too cryptic i'd say.
Maybe just `IgnoreInitListExprs`


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

https://reviews.llvm.org/D55044



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

[PATCH] D57435: [clang-tidy] Add abseil-wrap-unique check

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/abseil/WrapUniqueCheck.cpp:7
+// License. See LICENSE.TXT for details.
+//
+//===--===//

License as above



Comment at: clang-tidy/abseil/WrapUniqueCheck.cpp:77
+  return;
+}
+

we don't put braces on small lines, just


```
if (Cons->isListInitialization())
  return;

```



Comment at: clang-tidy/abseil/WrapUniqueCheck.h:6
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//

this is the wrong license wording now (its changed recently..) check out the 
other files in trunk

but its something like this


```
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//

```





Comment at: docs/clang-tidy/checks/abseil-wrap-unique.rst:10
+.. code-block:: c++
+  class A {
+  public:

there is normally a newline after a `code-block`


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57435



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

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:163
+  // Conservatively disable for list initializations
+  if (UseLegacyFunction && New->getInitializationStyle() == 
CXXNewExpr::ListInit) {
+return;

elide the braces



Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:237
+  // Conservatively disable for list initializations
+  if (UseLegacyFunction && New->getInitializationStyle() == 
CXXNewExpr::ListInit) {
+return;

elide the braces



Comment at: docs/clang-tidy/checks/abseil-make-unique.rst:6
+abseil-make-unique
+
+

the  length needs to match the text above it


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

https://reviews.llvm.org/D55044



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

[clang-tools-extra] r356125 - [clangd] Store explicit template specializations in index for code navigation purposes

[Kadir Cetinkaya via cfe-commits]

Author: kadircet
Date: Thu Mar 14 01:35:17 2019
New Revision: 356125

URL: http://llvm.org/viewvc/llvm-project?rev=356125&view=rev
Log:
[clangd] Store explicit template specializations in index for code navigation 
purposes

Summary:
This introduces ~4k new symbols, and ~10k refs for LLVM. We need that
information for providing better code navigation support:
- When references for a class template is requested, we should return these 
specializations as well.
- When children of a specialization is requested, we should be able to query 
for those symbols(instead of just class template)

Number of symbols: 378574 -> 382784
Number of refs: 5098857 -> 5110689

Reviewers: hokein, gribozavr

Reviewed By: gribozavr

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

Tags: #clang

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

Modified:
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/index/MemIndex.cpp
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
clang-tools-extra/trunk/clangd/index/dex/Dex.cpp
clang-tools-extra/trunk/unittests/clangd/DexTests.cpp
clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp

Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=356125&r1=356124&r2=356125&view=diff
==
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original)
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Thu Mar 14 01:35:17 2019
@@ -1510,6 +1510,13 @@ private:
   }
 };
 
+template  bool isExplicitTemplateSpecialization(const NamedDecl &ND) {
+  if (const auto *TD = dyn_cast(&ND))
+if (TD->getTemplateSpecializationKind() == TSK_ExplicitSpecialization)
+  return true;
+  return false;
+}
+
 } // namespace
 
 clang::CodeCompleteOptions CodeCompleteOptions::getClangCompleteOpts() const {
@@ -1603,6 +1610,13 @@ bool isIndexedForCodeCompletion(const Na
 };
 return false;
   };
+  // We only complete symbol's name, which is the same as the name of the
+  // *primary* template in case of template specializations.
+  if (isExplicitTemplateSpecialization(ND) ||
+  isExplicitTemplateSpecialization(ND) ||
+  isExplicitTemplateSpecialization(ND))
+return false;
+
   if (InTopLevelScope(ND))
 return true;
 

Modified: clang-tools-extra/trunk/clangd/index/MemIndex.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/MemIndex.cpp?rev=356125&r1=356124&r2=356125&view=diff
==
--- clang-tools-extra/trunk/clangd/index/MemIndex.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/MemIndex.cpp Thu Mar 14 01:35:17 2019
@@ -11,6 +11,7 @@
 #include "Logger.h"
 #include "Quality.h"
 #include "Trace.h"
+#include "clang/Index/IndexSymbol.h"
 
 namespace clang {
 namespace clangd {
@@ -37,6 +38,15 @@ bool MemIndex::fuzzyFind(
   for (const auto Pair : Index) {
 const Symbol *Sym = Pair.second;
 
+// FIXME: Enable fuzzy find on template specializations once we start
+// storing template arguments in the name. Currently we only store name for
+// class template, which would cause duplication in the results.
+if (Sym->SymInfo.Properties &
+(static_cast(
+ index::SymbolProperty::TemplateSpecialization) |
+ static_cast(
+ index::SymbolProperty::TemplatePartialSpecialization)))
+  continue;
 // Exact match against all possible scopes.
 if (!Req.AnyScope && !llvm::is_contained(Req.Scopes, Sym->Scope))
   continue;

Modified: clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp?rev=356125&r1=356124&r2=356125&view=diff
==
--- clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp Thu Mar 14 
01:35:17 2019
@@ -221,13 +221,6 @@ RefKind toRefKind(index::SymbolRoleSet R
   return static_cast(static_cast(RefKind::All) & Roles);
 }
 
-template  bool explicitTemplateSpecialization(const NamedDecl &ND) {
-  if (const auto *TD = dyn_cast(&ND))
-if (TD->getTemplateSpecializationKind() == TSK_ExplicitSpecialization)
-  return true;
-  return false;
-}
-
 } // namespace
 
 SymbolCollector::SymbolCollector(Options Opts) : Opts(std::move(Opts)) {}
@@ -279,10 +272,6 @@ bool SymbolCollector::shouldCollectSymbo
 if (!isa(DeclCtx))
   return false;
   }
-  if (explicitTemplateSpecialization(ND) ||
-  explicitTemplateSpecialization(ND) ||
-  explicitTemplateSpecialization(ND))
-return false;
 
   // Avoid indexing inter

[PATCH] D59083: [clangd] Store explicit template specializations in index for code navigation purposes

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL356125: [clangd] Store explicit template specializations in 
index for code navigation… (authored by kadircet, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59083

Files:
  clang-tools-extra/trunk/clangd/CodeComplete.cpp
  clang-tools-extra/trunk/clangd/index/MemIndex.cpp
  clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
  clang-tools-extra/trunk/clangd/index/dex/Dex.cpp
  clang-tools-extra/trunk/unittests/clangd/DexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp

Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -1510,6 +1510,13 @@
   }
 };
 
+template  bool isExplicitTemplateSpecialization(const NamedDecl &ND) {
+  if (const auto *TD = dyn_cast(&ND))
+if (TD->getTemplateSpecializationKind() == TSK_ExplicitSpecialization)
+  return true;
+  return false;
+}
+
 } // namespace
 
 clang::CodeCompleteOptions CodeCompleteOptions::getClangCompleteOpts() const {
@@ -1603,6 +1610,13 @@
 };
 return false;
   };
+  // We only complete symbol's name, which is the same as the name of the
+  // *primary* template in case of template specializations.
+  if (isExplicitTemplateSpecialization(ND) ||
+  isExplicitTemplateSpecialization(ND) ||
+  isExplicitTemplateSpecialization(ND))
+return false;
+
   if (InTopLevelScope(ND))
 return true;
 
Index: clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
@@ -221,13 +221,6 @@
   return static_cast(static_cast(RefKind::All) & Roles);
 }
 
-template  bool explicitTemplateSpecialization(const NamedDecl &ND) {
-  if (const auto *TD = dyn_cast(&ND))
-if (TD->getTemplateSpecializationKind() == TSK_ExplicitSpecialization)
-  return true;
-  return false;
-}
-
 } // namespace
 
 SymbolCollector::SymbolCollector(Options Opts) : Opts(std::move(Opts)) {}
@@ -279,10 +272,6 @@
 if (!isa(DeclCtx))
   return false;
   }
-  if (explicitTemplateSpecialization(ND) ||
-  explicitTemplateSpecialization(ND) ||
-  explicitTemplateSpecialization(ND))
-return false;
 
   // Avoid indexing internal symbols in protobuf generated headers.
   if (isPrivateProtoDecl(ND))
Index: clang-tools-extra/trunk/clangd/index/MemIndex.cpp
===
--- clang-tools-extra/trunk/clangd/index/MemIndex.cpp
+++ clang-tools-extra/trunk/clangd/index/MemIndex.cpp
@@ -11,6 +11,7 @@
 #include "Logger.h"
 #include "Quality.h"
 #include "Trace.h"
+#include "clang/Index/IndexSymbol.h"
 
 namespace clang {
 namespace clangd {
@@ -37,6 +38,15 @@
   for (const auto Pair : Index) {
 const Symbol *Sym = Pair.second;
 
+// FIXME: Enable fuzzy find on template specializations once we start
+// storing template arguments in the name. Currently we only store name for
+// class template, which would cause duplication in the results.
+if (Sym->SymInfo.Properties &
+(static_cast(
+ index::SymbolProperty::TemplateSpecialization) |
+ static_cast(
+ index::SymbolProperty::TemplatePartialSpecialization)))
+  continue;
 // Exact match against all possible scopes.
 if (!Req.AnyScope && !llvm::is_contained(Req.Scopes, Sym->Scope))
   continue;
Index: clang-tools-extra/trunk/clangd/index/dex/Dex.cpp
===
--- clang-tools-extra/trunk/clangd/index/dex/Dex.cpp
+++ clang-tools-extra/trunk/clangd/index/dex/Dex.cpp
@@ -86,6 +86,15 @@
   llvm::DenseMap> TempInvertedIndex;
   for (DocID SymbolRank = 0; SymbolRank < Symbols.size(); ++SymbolRank) {
 const auto *Sym = Symbols[SymbolRank];
+// FIXME: Enable fuzzy find on template specializations once we start
+// storing template arguments in the name. Currently we only store name for
+// class template, which would cause duplication in the results.
+if (Sym->SymInfo.Properties &
+(static_cast(
+ index::SymbolProperty::TemplateSpecialization) |
+ static_cast(
+ index::SymbolProperty::TemplatePartialSpecialization)))
+  continue;
 for (const auto &Token : generateSearchTokens(*Sym))
   TempInvertedIndex[Token].push_back(SymbolRank);
   }
Index: clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
===
--- clan

[PATCH] D57893: [analyzer] Fix function macro crash

[Tibor Brunner via Phabricator via cfe-commits]

bruntib updated this revision to Diff 190575.
bruntib added a comment.

I've uploaded another version of the last fix. The previous one contained an 
UB, although it worked practically.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57893

Files:
  lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
  test/Analysis/plist-macros-with-expansion.cpp

Index: test/Analysis/plist-macros-with-expansion.cpp
===
--- test/Analysis/plist-macros-with-expansion.cpp
+++ test/Analysis/plist-macros-with-expansion.cpp
@@ -451,3 +451,21 @@
 1 / value; // expected-warning{{Division by zero}}
// expected-warning@-1{{expression result unused}}
 }
+
+#define FOO(x) int foo() { return x; }
+#define APPLY_ZERO1(function) function(0)
+
+APPLY_ZERO1(FOO)
+void useZeroApplier1() { (void)(1 / foo()); } // expected-warning{{Division by zero}}
+
+// CHECK: nameAPPLY_ZERO1
+// CHECK-NEXT: expansionint foo() { return x; }(0)
+
+#define BAR(x) int bar() { return x; }
+#define APPLY_ZERO2 BAR(0)
+
+APPLY_ZERO2
+void useZeroApplier2() { (void)(1 / bar()); } // expected-warning{{Division by zero}}
+
+// CHECK: nameAPPLY_ZERO2
+// CHECK-NEXT: expansionint bar() { return 0; }
Index: test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
===
--- test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
+++ test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
@@ -5577,6 +5577,484 @@

   
   
+  
+   path
+   
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line459
+   col33
+   file0
+  
+  
+   line459
+   col33
+   file0
+  
+ 
+end
+ 
+  
+   line459
+   col37
+   file0
+  
+  
+   line459
+   col39
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line459
+  col37
+  file0
+ 
+ ranges
+ 
+   
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col41
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Calling 'foo'
+ message
+ Calling 'foo'
+
+
+ kindevent
+ location
+ 
+  line458
+  col1
+  file0
+ 
+ depth1
+ extended_message
+ Entered call from 'useZeroApplier1'
+ message
+ Entered call from 'useZeroApplier1'
+
+
+ kindevent
+ location
+ 
+  line458
+  col1
+  file0
+ 
+ ranges
+ 
+   
+
+ line458
+ col1
+ file0
+
+
+ line458
+ col16
+ file0
+
+   
+ 
+ depth1
+ extended_message
+ Returning zero
+ message
+ Returning zero
+
+
+ kindevent
+ location
+ 
+  line459
+  col37
+  file0
+ 
+ ranges
+ 
+   
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col41
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Returning from 'foo'
+ message
+ Returning from 'foo'
+
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line459
+   col37
+   file0
+  
+  
+   line459
+   col39
+   file0
+  
+ 
+end
+ 
+  
+   line459
+   col35
+   file0
+  
+  
+   line459
+   col35
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line459
+  col35
+  file0
+ 
+ ranges
+ 
+   
+
+ line459
+ col33
+ file0
+
+
+ line459
+ col41
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Division by zero
+ message
+ Division by zero
+
+   
+   macro_expansions
+   
+
+ location
+ 
+  line458
+  col1
+  file0
+ 
+ nameAPPLY_ZERO1
+ expansionint foo() { return x; }(0)
+
+   
+   descriptionDivision by zero
+   categoryLogic error
+   typeDivision by zero
+   check_namecore.DivideZero
+   
+   issue_hash_content_of_line_in_context7ff82561a6c752746649d05220deeb40
+  issue_context_kindfunction
+  issue_contextuseZeroApplier1
+  issue_hash_function_offset0
+  location
+  
+   line459
+   col35
+   file0
+  
+  ExecutedLines
+  
+   0
+   
+458
+459
+   
+  

[PATCH] D59283: Fixed global constant/variable naming check on C++ class for ObjC++ files.

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.



Comment at: test/clang-tidy/google-objc-global-variable-declaration.mm:11
+};
\ No newline at end of file


nit: I think we should have a newline at end of file.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59283



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

[PATCH] D59264: [Driver] Support compiler-rt crtbegin.o/crtend.o for Linux

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

> A nonzero __dso_handle has to match the value passed to __cxa_atexit but a 
> zero __dso_handle matches every function registered. So it matters that DSO 
> fini calls use &__dso_handle to match their registrations for the dlclose case

Yes, but this won't matter if we change `void *__dso_handle = 0;` to `void 
*__dso_handle = &__dso_handle;`.

  static void __do_global_dtors_aux() { // static void __do_fini() in D28791
if (__cxa_finalize)
  __cxa_finalize(__dso_handle); // what happens if we change `void 
*__dso_handle = 0;` to `void *__dso_handle = &__dso_handle;`.
...
  }

exit calls `__run_exit_handlers` which goes through `__exit_funcs`.
`__cxa_atexit(_dl_fini,0,__dso_handle)` runs before other atexit registered 
functions after main() is executed.[1]

`__do_global_dtors_aux` is called by `_dl_fini`. When it gets called and it 
calls `__cxa_finalize(0)`, other atexit registered functions have run, thus 
__cxa_finalize(0) will do nothing.

The differentiation of `crtbegin.o crtbeginS.o` is unnecessary. It adds 
complexity for little size benefit (crtbegin.o is a bit smaller than 
crtbeginS.o).
While we are adding support for crtbegin.o, it'd be really good to get this 
fixed.

[1]: If a shared object has a constructor that calls `__cxa_atexit`, 
`__cxa_atexit(_dl_fini,...)` will not be the first.

  If you try really hard you can break that in glibc (but not in FreeBSD libc), 
but I'll call that an unsupported land as functions in the main executable 
(`register_in_exe`) shouldn't be called before it is initialized.
  `__attribute__((constructor)) void init_in_dso() { register_in_exe(); 
atexit(fini_in_dso); }`


Repository:
  rC Clang

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

https://reviews.llvm.org/D59264



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

[PATCH] D59350: [clangd] Build Dex index after loading all shards in BackgroundIndex.

[Haojian Wu via Phabricator via cfe-commits]

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

Currently after loadding all shards, we use MemIndex which has poor query
performance, we should use Dex.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D59350

Files:
  clangd/index/Background.cpp


Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -374,7 +374,9 @@
 // extra index build.
 reset(
 IndexedSymbols.buildIndex(IndexType::Heavy, DuplicateHandling::Merge));
-log("BackgroundIndex: rebuilt symbol index.");
+log("BackgroundIndex: rebuilt symbol index with estimated memory {0} "
+"bytes.",
+estimateMemoryUsage());
   }
 }
 
@@ -603,8 +605,10 @@
 }
   }
   vlog("Loaded all shards");
-  reset(IndexedSymbols.buildIndex(IndexType::Light, DuplicateHandling::Merge));
-
+  reset(IndexedSymbols.buildIndex(IndexType::Heavy, DuplicateHandling::Merge));
+  vlog("BackgroundIndex: built symbol index with estimated memory {0} "
+"bytes.",
+estimateMemoryUsage());
   return NeedsReIndexing;
 }
 


Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -374,7 +374,9 @@
 // extra index build.
 reset(
 IndexedSymbols.buildIndex(IndexType::Heavy, DuplicateHandling::Merge));
-log("BackgroundIndex: rebuilt symbol index.");
+log("BackgroundIndex: rebuilt symbol index with estimated memory {0} "
+"bytes.",
+estimateMemoryUsage());
   }
 }
 
@@ -603,8 +605,10 @@
 }
   }
   vlog("Loaded all shards");
-  reset(IndexedSymbols.buildIndex(IndexType::Light, DuplicateHandling::Merge));
-
+  reset(IndexedSymbols.buildIndex(IndexType::Heavy, DuplicateHandling::Merge));
+  vlog("BackgroundIndex: built symbol index with estimated memory {0} "
+"bytes.",
+estimateMemoryUsage());
   return NeedsReIndexing;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

Why did the numeric constant break this? Which path would the function run 
through?


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

https://reviews.llvm.org/D59309



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

[PATCH] D59350: [clangd] Build Dex index after loading all shards in BackgroundIndex.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added inline comments.
This revision is now accepted and ready to land.



Comment at: clangd/index/Background.cpp:609
+  reset(IndexedSymbols.buildIndex(IndexType::Heavy, DuplicateHandling::Merge));
+  vlog("BackgroundIndex: built symbol index with estimated memory {0} "
+"bytes.",

could you run clang-format


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59350



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

[clang-tools-extra] r356126 - [clangd] Build Dex index after loading all shards in BackgroundIndex.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Thu Mar 14 02:57:10 2019
New Revision: 356126

URL: http://llvm.org/viewvc/llvm-project?rev=356126&view=rev
Log:
[clangd] Build Dex index after loading all shards in BackgroundIndex.

Summary:
Currently after loadding all shards, we use MemIndex which has poor query
performance, we should use Dex.

Reviewers: kadircet

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

Tags: #clang

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

Modified:
clang-tools-extra/trunk/clangd/index/Background.cpp

Modified: clang-tools-extra/trunk/clangd/index/Background.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.cpp?rev=356126&r1=356125&r2=356126&view=diff
==
--- clang-tools-extra/trunk/clangd/index/Background.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/Background.cpp Thu Mar 14 02:57:10 2019
@@ -374,7 +374,9 @@ void BackgroundIndex::buildIndex() {
 // extra index build.
 reset(
 IndexedSymbols.buildIndex(IndexType::Heavy, DuplicateHandling::Merge));
-log("BackgroundIndex: rebuilt symbol index.");
+log("BackgroundIndex: rebuilt symbol index with estimated memory {0} "
+"bytes.",
+estimateMemoryUsage());
   }
 }
 
@@ -603,8 +605,10 @@ BackgroundIndex::loadShards(std::vector<
 }
   }
   vlog("Loaded all shards");
-  reset(IndexedSymbols.buildIndex(IndexType::Light, DuplicateHandling::Merge));
-
+  reset(IndexedSymbols.buildIndex(IndexType::Heavy, DuplicateHandling::Merge));
+  vlog("BackgroundIndex: built symbol index with estimated memory {0} "
+   "bytes.",
+   estimateMemoryUsage());
   return NeedsReIndexing;
 }
 


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

[PATCH] D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D59309#1428799 , @klimek wrote:

> Why did the numeric constant break this? Which path would the function run 
> through?


as its looking though the parameter list is doesn't recognize a numeric 
constant as being a valid thing it would see in an argument list and so falls 
through the bottom to return false, it will only be where there is a 
numeric_constant as the first argument, if it has a second argument and that 
argument is just a simple type it would still fail, but switch them around and 
it won't

As I look more at this, even this would fail to break

  int TestFn(A=1)
  {
return a;
  }




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

https://reviews.llvm.org/D59309



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

[PATCH] D59350: [clangd] Build Dex index after loading all shards in BackgroundIndex.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 190579.
hokein marked an inline comment as done.
hokein added a comment.

Format the code.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59350

Files:
  clangd/index/Background.cpp


Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -374,7 +374,9 @@
 // extra index build.
 reset(
 IndexedSymbols.buildIndex(IndexType::Heavy, DuplicateHandling::Merge));
-log("BackgroundIndex: rebuilt symbol index.");
+log("BackgroundIndex: rebuilt symbol index with estimated memory {0} "
+"bytes.",
+estimateMemoryUsage());
   }
 }
 
@@ -603,8 +605,10 @@
 }
   }
   vlog("Loaded all shards");
-  reset(IndexedSymbols.buildIndex(IndexType::Light, DuplicateHandling::Merge));
-
+  reset(IndexedSymbols.buildIndex(IndexType::Heavy, DuplicateHandling::Merge));
+  vlog("BackgroundIndex: built symbol index with estimated memory {0} "
+   "bytes.",
+   estimateMemoryUsage());
   return NeedsReIndexing;
 }
 


Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -374,7 +374,9 @@
 // extra index build.
 reset(
 IndexedSymbols.buildIndex(IndexType::Heavy, DuplicateHandling::Merge));
-log("BackgroundIndex: rebuilt symbol index.");
+log("BackgroundIndex: rebuilt symbol index with estimated memory {0} "
+"bytes.",
+estimateMemoryUsage());
   }
 }
 
@@ -603,8 +605,10 @@
 }
   }
   vlog("Loaded all shards");
-  reset(IndexedSymbols.buildIndex(IndexType::Light, DuplicateHandling::Merge));
-
+  reset(IndexedSymbols.buildIndex(IndexType::Heavy, DuplicateHandling::Merge));
+  vlog("BackgroundIndex: built symbol index with estimated memory {0} "
+   "bytes.",
+   estimateMemoryUsage());
   return NeedsReIndexing;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59350: [clangd] Build Dex index after loading all shards in BackgroundIndex.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL356126: [clangd] Build Dex index after loading all shards in 
BackgroundIndex. (authored by hokein, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59350

Files:
  clang-tools-extra/trunk/clangd/index/Background.cpp


Index: clang-tools-extra/trunk/clangd/index/Background.cpp
===
--- clang-tools-extra/trunk/clangd/index/Background.cpp
+++ clang-tools-extra/trunk/clangd/index/Background.cpp
@@ -374,7 +374,9 @@
 // extra index build.
 reset(
 IndexedSymbols.buildIndex(IndexType::Heavy, DuplicateHandling::Merge));
-log("BackgroundIndex: rebuilt symbol index.");
+log("BackgroundIndex: rebuilt symbol index with estimated memory {0} "
+"bytes.",
+estimateMemoryUsage());
   }
 }
 
@@ -603,8 +605,10 @@
 }
   }
   vlog("Loaded all shards");
-  reset(IndexedSymbols.buildIndex(IndexType::Light, DuplicateHandling::Merge));
-
+  reset(IndexedSymbols.buildIndex(IndexType::Heavy, DuplicateHandling::Merge));
+  vlog("BackgroundIndex: built symbol index with estimated memory {0} "
+   "bytes.",
+   estimateMemoryUsage());
   return NeedsReIndexing;
 }
 


Index: clang-tools-extra/trunk/clangd/index/Background.cpp
===
--- clang-tools-extra/trunk/clangd/index/Background.cpp
+++ clang-tools-extra/trunk/clangd/index/Background.cpp
@@ -374,7 +374,9 @@
 // extra index build.
 reset(
 IndexedSymbols.buildIndex(IndexType::Heavy, DuplicateHandling::Merge));
-log("BackgroundIndex: rebuilt symbol index.");
+log("BackgroundIndex: rebuilt symbol index with estimated memory {0} "
+"bytes.",
+estimateMemoryUsage());
   }
 }
 
@@ -603,8 +605,10 @@
 }
   }
   vlog("Loaded all shards");
-  reset(IndexedSymbols.buildIndex(IndexType::Light, DuplicateHandling::Merge));
-
+  reset(IndexedSymbols.buildIndex(IndexType::Heavy, DuplicateHandling::Merge));
+  vlog("BackgroundIndex: built symbol index with estimated memory {0} "
+   "bytes.",
+   estimateMemoryUsage());
   return NeedsReIndexing;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57765: [ARM] Add Cortex-M35P Support

[Luke Cheeseman via Phabricator via cfe-commits]

LukeCheeseman added a comment.

ping


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

https://reviews.llvm.org/D57765



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

[clang-tools-extra] r356127 - [clangd] Fix an out-of-date FIXME, NFC.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Thu Mar 14 03:01:07 2019
New Revision: 356127

URL: http://llvm.org/viewvc/llvm-project?rev=356127&view=rev
Log:
[clangd] Fix an out-of-date FIXME, NFC.

Modified:
clang-tools-extra/trunk/clangd/index/Index.h

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=356127&r1=356126&r2=356127&view=diff
==
--- clang-tools-extra/trunk/clangd/index/Index.h (original)
+++ clang-tools-extra/trunk/clangd/index/Index.h Thu Mar 14 03:01:07 2019
@@ -103,9 +103,6 @@ public:
 llvm::function_ref Callback) const = 0;
 
   /// Returns estimated size of index (in bytes).
-  // FIXME(kbobyrev): Currently, this only returns the size of index itself
-  // excluding the size of actual symbol slab index refers to. We should 
include
-  // both.
   virtual size_t estimateMemoryUsage() const = 0;
 };
 


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

[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

[Edward Jones via Phabricator via cfe-commits]

edward-jones added a comment.

In D58896#1419964 , @aaron.ballman 
wrote:

> Do you have some evidence that the current behavior is causing a lot of false 
> positives in the wild? For ASCII character literals, I can sort of guess at 
> why people might want to do this, but for things like wide character 
> literals, or character literals relying on the current code page, etc, I'm 
> less convinced.


I don't know about the false positive rate,  just the one report on twitter 
which triggered me to submit this change. As for wide character literals I was 
under the impression that they would be promoted to integers and wouldn't have 
triggered the -Wchar-subscript anyway?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58896



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

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/include-mapping/gen_std.py:46
+//
+// Generated from cppreference offline HTML book v%s.
+//===--===//

I'd suggest dropping `v` prefix as it might be confusing (e.g. one would search 
for the exact same version number on cppreference archive). Maybe add a short 
comment explaining that this is the modification date of the doc?



Comment at: clangd/include-mapping/gen_std.py:114
+  # so we use the modified time of the symbol_index.html as the version.
+  cppreference_version = datetime.datetime.fromtimestamp(
+os.stat(index_page_path).st_mtime).strftime('%Y-%m-%d')

s/version/date/, as this not really a version.



Comment at: clangd/include-mapping/gen_std.py:129
+headers = ParseSymbolPage(f.read())
+  if len(headers) == 0:
+sys.stderr.write("No header found for symbol %s at %s\n" % 
(symbol_name,

`if not headers:`



Comment at: clangd/index/CanonicalIncludes.cpp:123
 
   static const std::vector>
   SystemHeaderMap = {

ioeric wrote:
> hokein wrote:
> > sammccall wrote:
> > > hokein wrote:
> > > > ioeric wrote:
> > > > > Can we remove the suffix header mapping now? Is it for the 
> > > > > `std::chrono::` symbols? What are the reasons not to include them in 
> > > > > this patch? 
> > > > Ideally, we could remove it once we get a perfect symbol mapping but 
> > > > I'd prefer to keep it (it serves as a fallback when we don't find 
> > > > symbols in the symbol mapping), so that we could add new symbol mapping 
> > > > increasingly without changing the current behavior.
> > > > 
> > > > Removing it should be done carefully without introducing regression, 
> > > > and is outside the scope of this patch.
> > > We do need fallback heuristics. We should conisder cases:
> > >  - for known `std::` symbols mapped to one system header, we don't need a 
> > > fallback
> > >  - for known `std::` mapped to multiple system headers (`std::move`), we 
> > > need some fallback. There are probably few enough of these that it 
> > > doesn't justify listing *all* system header paths.
> > >  - for known `std::` symbols with a primary template in one file and 
> > > specializations/overloads in another (swap?) always inserting the primary 
> > > seems fine 
> > >  - For "random" unknown symbols from system header `foo/bits/_bar.h`, we 
> > > can not insert, correct to ``, or use the full path name. I don't 
> > > have a strong preference here, maybe we should do what's easiest.
> > >  - for c standard library (`::printf` --> `` etc) we probably 
> > > want mappings just like in this patch, I think cppreference has them.
> > >  - other "standardish" headers (POSIX etc) - hmm, unclear.
> > > 
> > > Thinking about all these cases, I'm thinking the nicest solution would be 
> > > having the symbol -> header mapping, having a small (symbol,header) -> 
> > > header mapping **only** for ambiguous cases, and not inserting "system" 
> > > headers that aren't in the main mapping at all. Then we can improve 
> > > coverage of posix, windows etc headers over time.
> > > Question is, how can we recognize "system" headers?
> > I think we were talking about C++ std symbols.
> > 
> > > for known std:: symbols mapped to one system header, we don't need a 
> > > fallback
> > > for known std:: mapped to multiple system headers (std::move), we need 
> > > some fallback. There are probably few enough of these that it doesn't 
> > > justify listing *all* system header paths.
> > > for known std:: symbols with a primary template in one file and 
> > > specializations/overloads in another (swap?) always inserting the primary 
> > > seems fine
> > 
> > As discussed in this patch, we have other ways to disambiguate these 
> > symbols (rather than relying on the header mapping), so it is possible to 
> > remove STL headers mapping, but we'll lose correct headers for STL 
> > implement-related symbols (e.g. `__hash_code`), ideally we should drop 
> > these symbols in the indexer...
> > 
> > > for c standard library (::printf -->  etc) we probably want 
> > > mappings just like in this patch, I think cppreference has them.
> > 
> > Yes, cppreference has them.
> > 
> > > other "standardish" headers (POSIX etc) - hmm, unclear.
> > 
> > I think we should still keep the header mapping. Not sure whether there are 
> > some sources like cppreference that list all symbols.
> > 
> > 
> So, do we have a plan on how to handle ambiguous symbols properly? If so, 
> could you please summarize this in the FIXME comment so that we don't lose 
> track of it?
> 
> > I think we should still keep the header mapping. Not sure whether there are 
> > some sources like cppreference that list all symbols.
> What's the reasoning?
> 
> I am +1 on Sam's idea about skipping include insertion for std:: symbo

[PATCH] D33029: [clang-format] add option for dangling parenthesis

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D33029#1423949 , @MyDeveloperDay 
wrote:

> In D33029#1423947 , @lebedev.ri 
> wrote:
>
> > In D33029#1423944 , 
> > @MyDeveloperDay wrote:
> >
> > > Adding the unit tests lets us see how this option will work in various 
> > > cases, it will let us understand that its not breaking anything else.
> > >
> > > I personally don't like to see revisions like this sit for 2 years with 
> > > nothing happening, I don't see anything wrong with this that would 
> > > prevent it going in so I don't understand whats blocking it?,
> > >
> > > if you had some tests and a release note I'd give it a LGTM (but as I've 
> > > said before I'm not the code owner, but someone wanting to address 
> > > defects and add capabilities. but I think we need to be able to move 
> > > forward, people will object soon enough if they don't like it.)
> > >
> > > Generally I don't understand why clang-format is so reluctant to change 
> > > anything, As such we don't have many people involved and getting anything 
> > > done (even defects) is extremely hard.
> > >
> > > It looks like you met the criteria, and reviewers have been given ample 
> > > opportunity to make an objection. the number of subscribers and like 
> > > tokens would suggest this is wanted,
> > >
> > > Please also add a line the in the release notes to say what you are 
> > > adding. In the absence of any other constructive input all we can do is 
> > > follow the guidance on doing a review, for what its worth I notice in the 
> > > rest of LLVM there seems to be a much larger amount of commits that go in 
> > > even without a review, I'm not sure what makes this area so strict, so 
> > > reluctant to change especailly when revisions do seem to be reviewed.
> >
> >
> > I don't have any stake here, but i just want to point out that no tool 
> > (including clang-format)
> >  will ever support all the things all the people out there will want it to 
> > support. But every
> >  new knob is not just a single knob, it needs to work well with all the 
> > other existing knobs
> >  (and all of the combination of knob params), and every new knob after that.
> >
> > It's a snowball effect. Things can (and likely will, unless there is at 
> > least a *very* strict testing/quality policy
> >  (which is 
> > https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options
> >  about))
> >  get out of hand real quickly..
> >
> > Just 2c.
>
>
> Correct we should always be adding tests and show we don't break existing 
> tests... We need to apply the "Beyonce Rule".. "if you liked it you should 
> have put a test on it."
>
> We shouldn't just give up making improvements or fixing defects because 
> something is hard or complex.


The problem is that clang-format is already at a complexity level where most 
patches for new things we see are overly complex and will make adding new 
things *even harder*, reducing the ability to move at all.
As I've written before, I agree that we're in a bad state, and I'm truly sorry 
for it - we need to get active contributors into a state where they can make 
changes aligned with the architectural spirit of clang-format, which make 
clang-format at least not more complex :)

For this specific patch, yes, please change it as djasper suggested to be 
configurable in the BracketAlignmentStyle.


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

https://reviews.llvm.org/D33029



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

[PATCH] D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D59309#1428807 , @MyDeveloperDay 
wrote:

> In D59309#1428799 , @klimek wrote:
>
> > Why did the numeric constant break this? Which path would the function run 
> > through?
>
>
> as its looking though the parameter list is doesn't recognize a numeric 
> constant as being a valid thing it would see in an argument list and so falls 
> through the bottom to return false, it will only be where there is a 
> numeric_constant as the first argument, if it has a second argument and that 
> argument is just a simple type it would still fail, but switch them around 
> and it won't
>
> As I look more at this, even this would (and still would) fail to break
>
>   int TestFn(A=1)
>   {
> return a;
>   }
>
>
> originally I didn't have the counting of the <> but one of the unit tests 
> failed where we had
>
>   verifyFormat("LngType\n"
>"LngVariable(1);");
>   
>   
>
> I guess they wanted to do something like this when the type and name is very 
> long
>
> int
>  variable(1);
>
> but I think this gets thought of as being a potential FunctionDecl
>
> so I held off just adding tok::numeric_constant to the isOneOf(), and went 
> with the use case
>
> I think perhaps I could solve the default argument by adding a
>
> startsSequence(tok::equal, tok::numeric_constant)


How does this even work for

  void Test(A a)
  {
  }

This code seems to deeply assume that the { is the last thing in the line for a 
definition. One question is whether we can just mark this as a function 
definition when we see the { in the UnwrappedLineParser...


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

https://reviews.llvm.org/D59309



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

[PATCH] D57765: [ARM] Add Cortex-M35P Support

[Oliver Stannard via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D57765



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D55170#1423941 , @MyDeveloperDay 
wrote:

> In D55170#1423798 , @reuk wrote:
>
> > Thanks for the review, and for approving this PR. It's very much 
> > appreciated!
> >
> > I don't have merge rights - could someone commit this for me please?
>
>
> I'd actively encourage you to go get commit access, I think its much better 
> when the commit comes from the author.
>
> There just isn't enough people actively involved in clang-format (and other 
> tools) for us to get reviews even looked at (even for defects), we need more 
> people involved fixing defects and doing reviews, getting commit access shows 
> an intent to fix anything in that comes from your own review which is one of 
> the things the code owners push as being an objection to adding more code.


I'm sorry for not having a positive answer here, but I'm not in favor of this 
change. The style rule looks like it introduces arbitrary complexity at a point 
where I don't understand at all  how it matters. We cannot possibly support all 
style guides that come up with random variance. I do not think this option 
pulls its weight.


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

https://reviews.llvm.org/D55170



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

[PATCH] D59353: [WebAssembly] Use rethrow intrinsic in the rethrow block

[Heejin Ahn via Phabricator via cfe-commits]

aheejin created this revision.
aheejin added a reviewer: dschuff.
Herald added subscribers: cfe-commits, sunfish, jgravelle-google, sbc100.
Herald added a project: clang.

Because in wasm we merge all catch clauses into one big catchpad, in
case none of the types in catch handlers matches after we test against
each of them, we should unwind to the next EH enclosing scope. For this,
we should NOT use a call to `__cxa_rethrow` but rather a call to our own
rethrow intrinsic, because what we're trying to do here is just to
transfer the control flow into the next enclosing EH pad (or the
caller). Calls to `__cxa_rethrow` should only be used after a call to
`__cxa_begin_catch`.


Repository:
  rC Clang

https://reviews.llvm.org/D59353

Files:
  include/clang/Basic/BuiltinsWebAssembly.def
  lib/CodeGen/CGBuiltin.cpp
  lib/CodeGen/CGException.cpp
  test/CodeGen/builtins-wasm.c
  test/CodeGenCXX/wasm-eh.cpp

Index: test/CodeGenCXX/wasm-eh.cpp
===
--- test/CodeGenCXX/wasm-eh.cpp
+++ test/CodeGenCXX/wasm-eh.cpp
@@ -62,7 +62,7 @@
 // CHECK-NEXT:   br label %[[TRY_CONT_BB]]
 
 // CHECK: [[RETHROW_BB]]:
-// CHECK-NEXT:   call void @__cxa_rethrow() {{.*}} [ "funclet"(token %[[CATCHPAD]]) ]
+// CHECK-NEXT:   call void @llvm.wasm.rethrow.in.catch() {{.*}} [ "funclet"(token %[[CATCHPAD]]) ]
 // CHECK-NEXT:   unreachable
 
 // Single catch-all
@@ -232,7 +232,7 @@
 // CHECK:   catchret from %[[CATCHPAD]] to label %{{.*}}
 
 // CHECK: [[RETHROW_BB]]:
-// CHECK-NEXT:   invoke void @__cxa_rethrow() {{.*}} [ "funclet"(token %[[CATCHPAD]]) ]
+// CHECK-NEXT:   invoke void @llvm.wasm.rethrow.in.catch() {{.*}} [ "funclet"(token %[[CATCHPAD]]) ]
 // CHECK-NEXT:  to label %[[UNREACHABLE_BB:.*]] unwind label %[[EHCLEANUP_BB1:.*]]
 
 // CHECK: [[EHCLEANUP_BB2]]:
@@ -296,7 +296,7 @@
 
 // CHECK:   catchret from %[[CATCHPAD0]] to label
 
-// CHECK:   invoke void @__cxa_rethrow() {{.*}} [ "funclet"(token %[[CATCHPAD0]]) ]
+// CHECK:   invoke void @llvm.wasm.rethrow.in.catch() {{.*}} [ "funclet"(token %[[CATCHPAD0]]) ]
 
 // CHECK:   %[[CLEANUPPAD1:.*]] = cleanuppad within %[[CATCHPAD0]] []
 // CHECK:   cleanupret from %[[CLEANUPPAD1]] unwind label
@@ -368,11 +368,11 @@
 
 // CHECK:   catchret from %[[CATCHPAD1]] to label
 
-// CHECK:   invoke void @__cxa_rethrow() {{.*}} [ "funclet"(token %[[CATCHPAD1]]) ]
+// CHECK:   invoke void @llvm.wasm.rethrow.in.catch() {{.*}} [ "funclet"(token %[[CATCHPAD1]]) ]
 
 // CHECK:   catchret from %[[CATCHPAD0]] to label
 
-// CHECK:   call void @__cxa_rethrow() {{.*}} [ "funclet"(token %[[CATCHPAD0]]) ]
+// CHECK:   call void @llvm.wasm.rethrow.in.catch() {{.*}} [ "funclet"(token %[[CATCHPAD0]]) ]
 // CHECK:   unreachable
 
 // CHECK:   %[[CLEANUPPAD0:.*]] = cleanuppad within %[[CATCHPAD1]] []
Index: test/CodeGen/builtins-wasm.c
===
--- test/CodeGen/builtins-wasm.c
+++ test/CodeGen/builtins-wasm.c
@@ -44,10 +44,10 @@
   // WEBASSEMBLY64: call void @llvm.wasm.throw(i32 %{{.*}}, i8* %{{.*}})
 }
 
-void rethrow(void) {
-  return __builtin_wasm_rethrow();
-  // WEBASSEMBLY32: call void @llvm.wasm.rethrow()
-  // WEBASSEMBLY64: call void @llvm.wasm.rethrow()
+void rethrow_in_catch(void) {
+  return __builtin_wasm_rethrow_in_catch();
+  // WEBASSEMBLY32: call void @llvm.wasm.rethrow.in.catch()
+  // WEBASSEMBLY64: call void @llvm.wasm.rethrow.in.catch()
 }
 
 int atomic_wait_i32(int *addr, int expected, long long timeout) {
Index: lib/CodeGen/CGException.cpp
===
--- lib/CodeGen/CGException.cpp
+++ lib/CodeGen/CGException.cpp
@@ -1259,7 +1259,9 @@
 }
 assert(RethrowBlock != WasmCatchStartBlock && RethrowBlock->empty());
 Builder.SetInsertPoint(RethrowBlock);
-CGM.getCXXABI().emitRethrow(*this, /*isNoReturn=*/true);
+llvm::Function *RethrowInCatchFn =
+CGM.getIntrinsic(llvm::Intrinsic::wasm_rethrow_in_catch);
+EmitNoreturnRuntimeCallOrInvoke(RethrowInCatchFn, {});
   }
 
   EmitBlock(ContBB);
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -13513,8 +13513,8 @@
 Function *Callee = CGM.getIntrinsic(Intrinsic::wasm_throw);
 return Builder.CreateCall(Callee, {Tag, Obj});
   }
-  case WebAssembly::BI__builtin_wasm_rethrow: {
-Function *Callee = CGM.getIntrinsic(Intrinsic::wasm_rethrow);
+  case WebAssembly::BI__builtin_wasm_rethrow_in_catch: {
+Function *Callee = CGM.getIntrinsic(Intrinsic::wasm_rethrow_in_catch);
 return Builder.CreateCall(Callee);
   }
   case WebAssembly::BI__builtin_wasm_atomic_wait_i32: {
Index: include/clang/Basic/BuiltinsWebAssembly.def
===
--- include/clang/Basic/BuiltinsWebAssembly.def
+++ include/clang/Basic/BuiltinsWebAssembly.def
@@ -37,7 +37,7 @@
 
 // Exception han

[PATCH] D52150: [clang-format] BeforeHash added to IndentPPDirectives

[Manuel Klimek via Phabricator via cfe-commits]

klimek accepted this revision.
klimek added a comment.

Given this doesn't add an extra option (but only an extra enum) and is fairly 
straight forward, LG.


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

https://reviews.llvm.org/D52150



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

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

[Haojian Wu via Phabricator via cfe-commits]

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

Address comments


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58345

Files:
  clangd/StdSymbolMap.inc
  clangd/include-mapping/gen_std.py
  clangd/include-mapping/test.py
  clangd/index/CanonicalIncludes.cpp

Index: clangd/index/CanonicalIncludes.cpp
===
--- clangd/index/CanonicalIncludes.cpp
+++ clangd/index/CanonicalIncludes.cpp
@@ -107,57 +107,29 @@
 
 void addSystemHeadersMapping(CanonicalIncludes *Includes) {
   static const std::vector> SymbolMap = {
-  {"std::addressof", ""},
-  // Map symbols in  to their preferred includes.
   {"std::basic_filebuf", ""},
-  {"std::basic_fstream", ""},
-  {"std::basic_ifstream", ""},
-  {"std::basic_ofstream", ""},
   {"std::filebuf", ""},
-  {"std::fstream", ""},
-  {"std::ifstream", ""},
-  {"std::ofstream", ""},
   {"std::wfilebuf", ""},
-  {"std::wfstream", ""},
-  {"std::wifstream", ""},
-  {"std::wofstream", ""},
-  {"std::basic_ios", ""},
-  {"std::ios", ""},
-  {"std::wios", ""},
-  {"std::basic_iostream", ""},
-  {"std::iostream", ""},
-  {"std::wiostream", ""},
   {"std::basic_istream", ""},
   {"std::istream", ""},
   {"std::wistream", ""},
-  {"std::istreambuf_iterator", ""},
-  {"std::ostreambuf_iterator", ""},
   {"std::basic_ostream", ""},
   {"std::ostream", ""},
   {"std::wostream", ""},
-  {"std::basic_istringstream", ""},
-  {"std::basic_ostringstream", ""},
-  {"std::basic_stringbuf", ""},
-  {"std::basic_stringstream", ""},
-  {"std::istringstream", ""},
-  {"std::ostringstream", ""},
-  {"std::string", ""},
-  {"std::stringbuf", ""},
-  {"std::stringstream", ""},
-  {"std::wistringstream", ""},
-  {"std::wostringstream", ""},
-  {"std::wstringbuf", ""},
-  {"std::wstringstream", ""},
-  {"std::basic_streambuf", ""},
-  {"std::streambuf", ""},
-  {"std::wstreambuf", ""},
   {"std::uint_least16_t", ""}, //  redeclares these
   {"std::uint_least32_t", ""},
-  {"std::declval", ""},
+#define SYMBOL(Name, NameSpace, Header) { #NameSpace#Name, #Header },
+  #include "StdSymbolMap.inc"
+#undef SYMBOL
   };
   for (const auto &Pair : SymbolMap)
 Includes->addSymbolMapping(Pair.first, Pair.second);
 
+  // FIXME: remove the std header mapping once we support ambiguous symbols, now
+  // it serves as a fallback to disambiguate:
+  //   - symbols with mulitiple headers (e.g. std::move)
+  //   - symbols with a primary template in one header and a specialization in
+  // another (std::abs)
   static const std::vector>
   SystemHeaderMap = {
   {"include/__stddef_max_align_t.h", ""},
Index: clangd/include-mapping/test.py
===
--- /dev/null
+++ clangd/include-mapping/test.py
@@ -0,0 +1,101 @@
+#!/usr/bin/env python
+#===- test.py -  -*- python -*--===#
+#
+# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+# See https://llvm.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+#
+#======#
+
+from gen_std import ParseSymbolPage, ParseIndexPage
+
+import unittest
+
+class TestStdGen(unittest.TestCase):
+
+  def testParseIndexPage(self):
+html = """
+ abs() (int) 
+ abs<>() (std::complex) 
+ acos() 
+ acosh() (since C++11) 
+ as_bytes<>() (since C++20) 
+ """
+
+actual = ParseIndexPage(html)
+expected = [
+  ("abs", "abs.html"),
+  ("abs", "complex/abs.html"),
+  ("acos", "acos.html"),
+  ("acosh", "acosh.html"),
+  ("as_bytes", "as_bytes.html"),
+]
+self.assertEqual(len(actual), len(expected))
+for i in range(0, len(actual)):
+  self.assertEqual(expected[i][0], actual[i][0])
+  self.assertTrue(actual[i][1].endswith(expected[i][1]))
+
+
+  def testParseSymbolPage_SingleHeader(self):
+# Defined in header 
+html = """
+ 
+  
+   Defined in header 
+   
+  
+  
+  
+
+"""
+self.assertEqual(ParseSymbolPage(html), [''])
+
+
+  def testParseSymbolPage_MulHeaders(self):
+#  Defined in header 
+#  Defined in header 
+#  Defined in header 
+html = """
+
+  
+ Defined in header 
+ 
+ 
+
+  
+  
+ Defined in header 
+ 
+
+
+  
+  
+ Defined in header 
+ 
+
+
+  
+
+"""
+self.assertEqual(ParseSymbolPage(html),
+['', '', ''])
+
+
+  def testParseSymbolPage_MulHeadersInSameDiv(self):
+# Multile  blocks in a Div.
+# Defined in header 
+# Defined in header 
+html

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clangd/index/CanonicalIncludes.cpp:123
 
   static const std::vector>
   SystemHeaderMap = {

ioeric wrote:
> ioeric wrote:
> > hokein wrote:
> > > sammccall wrote:
> > > > hokein wrote:
> > > > > ioeric wrote:
> > > > > > Can we remove the suffix header mapping now? Is it for the 
> > > > > > `std::chrono::` symbols? What are the reasons not to include them 
> > > > > > in this patch? 
> > > > > Ideally, we could remove it once we get a perfect symbol mapping but 
> > > > > I'd prefer to keep it (it serves as a fallback when we don't find 
> > > > > symbols in the symbol mapping), so that we could add new symbol 
> > > > > mapping increasingly without changing the current behavior.
> > > > > 
> > > > > Removing it should be done carefully without introducing regression, 
> > > > > and is outside the scope of this patch.
> > > > We do need fallback heuristics. We should conisder cases:
> > > >  - for known `std::` symbols mapped to one system header, we don't need 
> > > > a fallback
> > > >  - for known `std::` mapped to multiple system headers (`std::move`), 
> > > > we need some fallback. There are probably few enough of these that it 
> > > > doesn't justify listing *all* system header paths.
> > > >  - for known `std::` symbols with a primary template in one file and 
> > > > specializations/overloads in another (swap?) always inserting the 
> > > > primary seems fine 
> > > >  - For "random" unknown symbols from system header `foo/bits/_bar.h`, 
> > > > we can not insert, correct to ``, or use the full path name. I 
> > > > don't have a strong preference here, maybe we should do what's easiest.
> > > >  - for c standard library (`::printf` --> `` etc) we probably 
> > > > want mappings just like in this patch, I think cppreference has them.
> > > >  - other "standardish" headers (POSIX etc) - hmm, unclear.
> > > > 
> > > > Thinking about all these cases, I'm thinking the nicest solution would 
> > > > be having the symbol -> header mapping, having a small (symbol,header) 
> > > > -> header mapping **only** for ambiguous cases, and not inserting 
> > > > "system" headers that aren't in the main mapping at all. Then we can 
> > > > improve coverage of posix, windows etc headers over time.
> > > > Question is, how can we recognize "system" headers?
> > > I think we were talking about C++ std symbols.
> > > 
> > > > for known std:: symbols mapped to one system header, we don't need a 
> > > > fallback
> > > > for known std:: mapped to multiple system headers (std::move), we need 
> > > > some fallback. There are probably few enough of these that it doesn't 
> > > > justify listing *all* system header paths.
> > > > for known std:: symbols with a primary template in one file and 
> > > > specializations/overloads in another (swap?) always inserting the 
> > > > primary seems fine
> > > 
> > > As discussed in this patch, we have other ways to disambiguate these 
> > > symbols (rather than relying on the header mapping), so it is possible to 
> > > remove STL headers mapping, but we'll lose correct headers for STL 
> > > implement-related symbols (e.g. `__hash_code`), ideally we should drop 
> > > these symbols in the indexer...
> > > 
> > > > for c standard library (::printf -->  etc) we probably want 
> > > > mappings just like in this patch, I think cppreference has them.
> > > 
> > > Yes, cppreference has them.
> > > 
> > > > other "standardish" headers (POSIX etc) - hmm, unclear.
> > > 
> > > I think we should still keep the header mapping. Not sure whether there 
> > > are some sources like cppreference that list all symbols.
> > > 
> > > 
> > So, do we have a plan on how to handle ambiguous symbols properly? If so, 
> > could you please summarize this in the FIXME comment so that we don't lose 
> > track of it?
> > 
> > > I think we should still keep the header mapping. Not sure whether there 
> > > are some sources like cppreference that list all symbols.
> > What's the reasoning?
> > 
> > I am +1 on Sam's idea about skipping include insertion for std:: symbols 
> > that are not on cppreference. It's fine to keep the suffix header mapping 
> > in the short term while we are working out ambiguous symbols. But I don't 
> > think we would want to maintain both mappings in the long term. 
> > 
> > Could you maybe add comment for the header mapping indicating that they are 
> > now the fallback mapping for std:: symbols?
> This is not done?
Oops, I missed it. Added a FIXME.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58345



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

[PATCH] D59354: [clangd] Print arguments in template specializations

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59354

Files:
  clang-tools-extra/clangd/AST.cpp
  clang/lib/AST/TypePrinter.cpp


Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1632,6 +1632,18 @@
   return A.getArgument();
 }
 
+static void printArgument(const TemplateArgument &A,
+const PrintingPolicy &PP,
+llvm::raw_ostream &OS) {
+  A.print(PP, OS);
+}
+
+static void printArgument(const TemplateArgumentLoc &A,
+const PrintingPolicy &PP,
+llvm::raw_ostream &OS) {
+  A.getTypeSourceInfo()->getType().print(OS, PP);
+}
+
 template
 static void printTo(raw_ostream &OS, ArrayRef Args,
 const PrintingPolicy &Policy, bool SkipBrackets) {
@@ -1653,7 +1665,7 @@
 } else {
   if (!FirstArg)
 OS << Comma;
-  Argument.print(Policy, ArgOS);
+  printArgument(Arg, Policy, ArgOS);
 }
 StringRef ArgString = ArgOS.str();
 
Index: clang-tools-extra/clangd/AST.cpp
===
--- clang-tools-extra/clangd/AST.cpp
+++ clang-tools-extra/clangd/AST.cpp
@@ -50,6 +50,17 @@
   return SM.getSpellingLoc(D->getLocation());
 }
 
+static llvm::Optional>
+getTemplateSpecializationArgLocs(const NamedDecl &ND) {
+  if (auto *Func = llvm::dyn_cast(&ND))
+return Func->getTemplateSpecializationArgsAsWritten()->arguments();
+  if (auto *Cls = llvm::dyn_cast(&ND))
+return Cls->getTemplateArgsAsWritten()->arguments();
+  if (auto *Var = llvm::dyn_cast(&ND))
+return Var->getTemplateArgsInfo().arguments();
+  return llvm::None;
+}
+
 std::string printQualifiedName(const NamedDecl &ND) {
   std::string QName;
   llvm::raw_string_ostream OS(QName);
@@ -60,6 +71,10 @@
   // namespaces to query: the preamble doesn't have a dedicated list.
   Policy.SuppressUnwrittenScope = true;
   ND.printQualifiedName(OS, Policy);
+  if (auto Args = getTemplateSpecializationArgLocs(ND))
+printTemplateArgumentList(OS, *Args, Policy);
+  else if (auto *Cls = llvm::dyn_cast(&ND))
+printTemplateArgumentList(OS, Cls->getTemplateArgs().asArray(), Policy);
   OS.flush();
   assert(!StringRef(QName).startswith("::"));
   return QName;


Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1632,6 +1632,18 @@
   return A.getArgument();
 }
 
+static void printArgument(const TemplateArgument &A,
+const PrintingPolicy &PP,
+llvm::raw_ostream &OS) {
+  A.print(PP, OS);
+}
+
+static void printArgument(const TemplateArgumentLoc &A,
+const PrintingPolicy &PP,
+llvm::raw_ostream &OS) {
+  A.getTypeSourceInfo()->getType().print(OS, PP);
+}
+
 template
 static void printTo(raw_ostream &OS, ArrayRef Args,
 const PrintingPolicy &Policy, bool SkipBrackets) {
@@ -1653,7 +1665,7 @@
 } else {
   if (!FirstArg)
 OS << Comma;
-  Argument.print(Policy, ArgOS);
+  printArgument(Arg, Policy, ArgOS);
 }
 StringRef ArgString = ArgOS.str();
 
Index: clang-tools-extra/clangd/AST.cpp
===
--- clang-tools-extra/clangd/AST.cpp
+++ clang-tools-extra/clangd/AST.cpp
@@ -50,6 +50,17 @@
   return SM.getSpellingLoc(D->getLocation());
 }
 
+static llvm::Optional>
+getTemplateSpecializationArgLocs(const NamedDecl &ND) {
+  if (auto *Func = llvm::dyn_cast(&ND))
+return Func->getTemplateSpecializationArgsAsWritten()->arguments();
+  if (auto *Cls = llvm::dyn_cast(&ND))
+return Cls->getTemplateArgsAsWritten()->arguments();
+  if (auto *Var = llvm::dyn_cast(&ND))
+return Var->getTemplateArgsInfo().arguments();
+  return llvm::None;
+}
+
 std::string printQualifiedName(const NamedDecl &ND) {
   std::string QName;
   llvm::raw_string_ostream OS(QName);
@@ -60,6 +71,10 @@
   // namespaces to query: the preamble doesn't have a dedicated list.
   Policy.SuppressUnwrittenScope = true;
   ND.printQualifiedName(OS, Policy);
+  if (auto Args = getTemplateSpecializationArgLocs(ND))
+printTemplateArgumentList(OS, *Args, Policy);
+  else if (auto *Cls = llvm::dyn_cast(&ND))
+printTemplateArgumentList(OS, Cls->getTemplateArgs().asArray(), Policy);
   OS.flush();
   assert(!StringRef(QName).startswith("::"));
   return QName;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-

[PATCH] D59354: [clangd] Print arguments in template specializations

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/AST.cpp:57
+return Func->getTemplateSpecializationArgsAsWritten()->arguments();
+  if (auto *Cls = llvm::dyn_cast(&ND))
+return Cls->getTemplateArgsAsWritten()->arguments();

For `ClassTemplateSpecializationDecl`, could we try extracting the arguments 
from the result of `getTypeAsWritten`?



Comment at: clang/lib/AST/TypePrinter.cpp:1636
+static void printArgument(const TemplateArgument &A,
+const PrintingPolicy &PP,
+llvm::raw_ostream &OS) {

NIT: clang-format the diff



Comment at: clang/lib/AST/TypePrinter.cpp:1644
+llvm::raw_ostream &OS) {
+  A.getTypeSourceInfo()->getType().print(OS, PP);
+}

Maybe print the result of `getTypeLoc()` here, if it's available?
Would produce results closer to the ones written in the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59354



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

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added inline comments.
This revision is now accepted and ready to land.



Comment at: clangd/index/CanonicalIncludes.cpp:111
-  {"std::addressof", ""},
-  // Map symbols in  to their preferred includes.
   {"std::basic_filebuf", ""},

nit: as these symbols are not covered by the new mapping, I think this comment 
is still meaningful.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58345



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

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

[Shiva Chen via Phabricator via cfe-commits]

shiva0217 updated this revision to Diff 190595.
shiva0217 added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Passing small data limitation by module flag.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57497

Files:
  docs/ClangCommandLineReference.rst
  include/clang/Basic/CodeGenOptions.def
  include/clang/Driver/CC1Options.td
  include/clang/Driver/Options.td
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/CodeGenModule.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/RISCVToolchain.cpp
  lib/Driver/ToolChains/RISCVToolchain.h
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/riscv-sdata-module-flag.c

Index: test/CodeGen/riscv-sdata-module-flag.c
===
--- /dev/null
+++ test/CodeGen/riscv-sdata-module-flag.c
@@ -0,0 +1,31 @@
+// RUN: %clang -target riscv32-unknown-elf %s -S -emit-llvm -o - \
+// RUN:   | FileCheck %s -check-prefix=RV32-DEFAULT
+// RUN: %clang -target riscv32-unknown-elf %s -S -emit-llvm -G4 -o - \
+// RUN:   | FileCheck %s -check-prefix=RV32-G4
+// RUN: %clang -target riscv32-unknown-elf %s -S -emit-llvm -msmall-data-limit=0 -o - \
+// RUN:   | FileCheck %s -check-prefix=RV32-S0
+// RUN: %clang -target riscv32-unknown-elf %s -S -emit-llvm -fpic -o - \
+// RUN:   | FileCheck %s -check-prefix=RV32-PIC
+
+// RUN: %clang -target riscv64-unknown-elf %s -S -emit-llvm -o - \
+// RUN:   | FileCheck %s -check-prefix=RV64-DEFAULT
+// RUN: %clang -target riscv64-unknown-elf %s -S -emit-llvm -G4 -o - \
+// RUN:   | FileCheck %s -check-prefix=RV64-G4
+// RUN: %clang -target riscv64-unknown-elf %s -S -emit-llvm -msmall-data-limit=0 -o - \
+// RUN:   | FileCheck %s -check-prefix=RV64-S0
+// RUN: %clang -target riscv64-unknown-elf %s -S -emit-llvm -fpic -o - \
+// RUN:   | FileCheck %s -check-prefix=RV64-PIC
+// RUN: %clang -target riscv64-unknown-elf %s -S -emit-llvm -mcmodel=large -o - \
+// RUN:   | FileCheck %s -check-prefix=RV64-LARGE
+
+
+// RV32-DEFAULT: !{i32 1, !"SmallDataLimit", i32 8}
+// RV32-G4:  !{i32 1, !"SmallDataLimit", i32 4}
+// RV32-S0:  !{i32 1, !"SmallDataLimit", i32 0}
+// RV32-PIC: !{i32 1, !"SmallDataLimit", i32 0}
+
+// RV64-DEFAULT: !{i32 1, !"SmallDataLimit", i32 8}
+// RV64-G4:  !{i32 1, !"SmallDataLimit", i32 4}
+// RV64-S0:  !{i32 1, !"SmallDataLimit", i32 0}
+// RV64-PIC: !{i32 1, !"SmallDataLimit", i32 0}
+// RV64-LARGE:   !{i32 1, !"SmallDataLimit", i32 0}
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -849,6 +849,7 @@
 
   Opts.NoZeroInitializedInBSS = Args.hasArg(OPT_mno_zero_initialized_in_bss);
   Opts.NumRegisterParameters = getLastArgIntValue(Args, OPT_mregparm, 0, Diags);
+  Opts.SmallDataLimit = getLastArgIntValue(Args, OPT_msmall_data_limit, 0, Diags);
   Opts.NoExecStack = Args.hasArg(OPT_mno_exec_stack);
   Opts.FatalWarnings = Args.hasArg(OPT_massembler_fatal_warnings);
   Opts.EnableSegmentedStacks = Args.hasArg(OPT_split_stacks);
Index: lib/Driver/ToolChains/RISCVToolchain.h
===
--- lib/Driver/ToolChains/RISCVToolchain.h
+++ lib/Driver/ToolChains/RISCVToolchain.h
@@ -33,6 +33,7 @@
llvm::opt::ArgStringList &CC1Args) const override;
 
 protected:
+  bool HasNativeLLVMSupport() const override { return true; }
   Tool *buildLinker() const override;
 
 private:
Index: lib/Driver/ToolChains/RISCVToolchain.cpp
===
--- lib/Driver/ToolChains/RISCVToolchain.cpp
+++ lib/Driver/ToolChains/RISCVToolchain.cpp
@@ -114,6 +114,13 @@
   {options::OPT_T_Group, options::OPT_e, options::OPT_s,
options::OPT_t, options::OPT_Z_Flag, options::OPT_r});
 
+  if (D.isUsingLTO()) {
+assert(!Inputs.empty() && "Must have at least one input.");
+AddGoldPlugin(ToolChain, Args, CmdArgs, Output, Inputs[0],
+  D.getLTOMode() == LTOK_Thin);
+
+  }
+
   AddLinkerInputs(ToolChain, Inputs, Args, CmdArgs, JA);
 
   // TODO: add C++ includes and libs if compiling C++.
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -1799,6 +1799,27 @@
 
   CmdArgs.push_back("-target-abi");
   CmdArgs.push_back(ABIName);
+
+  // Forward the -msmall-data-limit= option.
+  if (Arg *A = Args.getLastArg(options::OPT_G)) {
+CmdArgs.push_back("-msmall-data-limit");
+CmdArgs.push_back(A->getValue());
+  // Not support linker relaxation for PIC.
+  } else if (Args.getLastArg(options::OPT_shared, options::OPT_fpic,
+ options::OPT_fPIC)) {
+CmdArgs.push_back("-msmall-data-limit");
+CmdArgs.

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 190598.
hokein marked an inline comment as done.
hokein added a comment.

One more comment.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58345

Files:
  clangd/StdSymbolMap.inc
  clangd/include-mapping/gen_std.py
  clangd/include-mapping/test.py
  clangd/index/CanonicalIncludes.cpp

Index: clangd/index/CanonicalIncludes.cpp
===
--- clangd/index/CanonicalIncludes.cpp
+++ clangd/index/CanonicalIncludes.cpp
@@ -107,57 +107,30 @@
 
 void addSystemHeadersMapping(CanonicalIncludes *Includes) {
   static const std::vector> SymbolMap = {
-  {"std::addressof", ""},
   // Map symbols in  to their preferred includes.
   {"std::basic_filebuf", ""},
-  {"std::basic_fstream", ""},
-  {"std::basic_ifstream", ""},
-  {"std::basic_ofstream", ""},
   {"std::filebuf", ""},
-  {"std::fstream", ""},
-  {"std::ifstream", ""},
-  {"std::ofstream", ""},
   {"std::wfilebuf", ""},
-  {"std::wfstream", ""},
-  {"std::wifstream", ""},
-  {"std::wofstream", ""},
-  {"std::basic_ios", ""},
-  {"std::ios", ""},
-  {"std::wios", ""},
-  {"std::basic_iostream", ""},
-  {"std::iostream", ""},
-  {"std::wiostream", ""},
   {"std::basic_istream", ""},
   {"std::istream", ""},
   {"std::wistream", ""},
-  {"std::istreambuf_iterator", ""},
-  {"std::ostreambuf_iterator", ""},
   {"std::basic_ostream", ""},
   {"std::ostream", ""},
   {"std::wostream", ""},
-  {"std::basic_istringstream", ""},
-  {"std::basic_ostringstream", ""},
-  {"std::basic_stringbuf", ""},
-  {"std::basic_stringstream", ""},
-  {"std::istringstream", ""},
-  {"std::ostringstream", ""},
-  {"std::string", ""},
-  {"std::stringbuf", ""},
-  {"std::stringstream", ""},
-  {"std::wistringstream", ""},
-  {"std::wostringstream", ""},
-  {"std::wstringbuf", ""},
-  {"std::wstringstream", ""},
-  {"std::basic_streambuf", ""},
-  {"std::streambuf", ""},
-  {"std::wstreambuf", ""},
   {"std::uint_least16_t", ""}, //  redeclares these
   {"std::uint_least32_t", ""},
-  {"std::declval", ""},
+#define SYMBOL(Name, NameSpace, Header) { #NameSpace#Name, #Header },
+  #include "StdSymbolMap.inc"
+#undef SYMBOL
   };
   for (const auto &Pair : SymbolMap)
 Includes->addSymbolMapping(Pair.first, Pair.second);
 
+  // FIXME: remove the std header mapping once we support ambiguous symbols, now
+  // it serves as a fallback to disambiguate:
+  //   - symbols with mulitiple headers (e.g. std::move)
+  //   - symbols with a primary template in one header and a specialization in
+  // another (std::abs)
   static const std::vector>
   SystemHeaderMap = {
   {"include/__stddef_max_align_t.h", ""},
Index: clangd/include-mapping/test.py
===
--- /dev/null
+++ clangd/include-mapping/test.py
@@ -0,0 +1,101 @@
+#!/usr/bin/env python
+#===- test.py -  -*- python -*--===#
+#
+# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+# See https://llvm.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+#
+#======#
+
+from gen_std import ParseSymbolPage, ParseIndexPage
+
+import unittest
+
+class TestStdGen(unittest.TestCase):
+
+  def testParseIndexPage(self):
+html = """
+ abs() (int) 
+ abs<>() (std::complex) 
+ acos() 
+ acosh() (since C++11) 
+ as_bytes<>() (since C++20) 
+ """
+
+actual = ParseIndexPage(html)
+expected = [
+  ("abs", "abs.html"),
+  ("abs", "complex/abs.html"),
+  ("acos", "acos.html"),
+  ("acosh", "acosh.html"),
+  ("as_bytes", "as_bytes.html"),
+]
+self.assertEqual(len(actual), len(expected))
+for i in range(0, len(actual)):
+  self.assertEqual(expected[i][0], actual[i][0])
+  self.assertTrue(actual[i][1].endswith(expected[i][1]))
+
+
+  def testParseSymbolPage_SingleHeader(self):
+# Defined in header 
+html = """
+ 
+  
+   Defined in header 
+   
+  
+  
+  
+
+"""
+self.assertEqual(ParseSymbolPage(html), [''])
+
+
+  def testParseSymbolPage_MulHeaders(self):
+#  Defined in header 
+#  Defined in header 
+#  Defined in header 
+html = """
+
+  
+ Defined in header 
+ 
+ 
+
+  
+  
+ Defined in header 
+ 
+
+
+  
+  
+ Defined in header 
+ 
+
+
+  
+
+"""
+self.assertEqual(ParseSymbolPage(html),
+['', '', ''])
+
+
+  def testParseSymbolPage_MulHeadersInSameDiv(self):
+# Multile  blocks in a Div.
+# Defined in header 
+# Defined in header 
+htm

[clang-tools-extra] r356134 - [clangd] Using symbol name to map includes for STL symbols.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Thu Mar 14 04:25:26 2019
New Revision: 356134

URL: http://llvm.org/viewvc/llvm-project?rev=356134&view=rev
Log:
[clangd] Using symbol name to map includes for STL symbols.

Summary:
Using suffix path mapping relies on the STL implementations, and it is
not portable. This patch is using symbol name mapping, which should
work with different STL implementations, fix clangd/clangd#9.

To generate the symbol mapping, we parse the cppreference symbol index
page to build a lookup table.

The mapping is not completed, a few TODOs:
  - support symbols from different headers (e.g. std::move)
  - support STL macros
  - support symbols from std's sub-namespaces (e.g. chrono)

Reviewers: ioeric, jfb, serge-sans-paille

Reviewed By: ioeric

Subscribers: sammccall, klimek, ilya-biryukov, ioeric, MaskRay, jkorous, 
mgrang, arphaman, kadircet, jfb, jdoerfert, cfe-commits

Tags: #clang-tools-extra, #clang

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

Added:
clang-tools-extra/trunk/clangd/StdSymbolMap.inc
clang-tools-extra/trunk/clangd/include-mapping/
clang-tools-extra/trunk/clangd/include-mapping/gen_std.py   (with props)
clang-tools-extra/trunk/clangd/include-mapping/test.py   (with props)
Modified:
clang-tools-extra/trunk/clangd/index/CanonicalIncludes.cpp

Added: clang-tools-extra/trunk/clangd/StdSymbolMap.inc
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/StdSymbolMap.inc?rev=356134&view=auto
==
--- clang-tools-extra/trunk/clangd/StdSymbolMap.inc (added)
+++ clang-tools-extra/trunk/clangd/StdSymbolMap.inc Thu Mar 14 04:25:26 2019
@@ -0,0 +1,1226 @@
+//===-- gen_std.py generated file ---*- C++ 
-*-===//
+//
+// Used to build a lookup table (qualified names => include headers) for C++
+// Standard Library symbols.
+//
+// Automatically generated file, DO NOT EDIT!
+//
+// Generated from cppreference offline HTML book (modified on 2018-10-28).
+//===--===//
+
+SYMBOL(Assignable, std::, )
+SYMBOL(Boolean, std::, )
+SYMBOL(Common, std::, )
+SYMBOL(CommonReference, std::, )
+SYMBOL(Constructible, std::, )
+SYMBOL(ConvertibleTo, std::, )
+SYMBOL(CopyConstructible, std::, )
+SYMBOL(Copyable, std::, )
+SYMBOL(DefaultConstructible, std::, )
+SYMBOL(DerivedFrom, std::, )
+SYMBOL(Destructible, std::, )
+SYMBOL(EqualityComparable, std::, )
+SYMBOL(EqualityComparableWith, std::, )
+SYMBOL(FILE, std::, )
+SYMBOL(Integral, std::, )
+SYMBOL(Invocable, std::, )
+SYMBOL(Movable, std::, )
+SYMBOL(MoveConstructible, std::, )
+SYMBOL(Predicate, std::, )
+SYMBOL(Regular, std::, )
+SYMBOL(RegularInvocable, std::, )
+SYMBOL(Relation, std::, )
+SYMBOL(Same, std::, )
+SYMBOL(Semiregular, std::, )
+SYMBOL(SignedIntegral, std::, )
+SYMBOL(StrictTotallyOrdered, std::, )
+SYMBOL(StrictTotallyOrderedWith, std::, )
+SYMBOL(StrictWeakOrder, std::, )
+SYMBOL(Swappable, std::, )
+SYMBOL(SwappableWith, std::, )
+SYMBOL(UniformRandomBitGenerator, std::, )
+SYMBOL(UnsignedIntegral, std::, )
+SYMBOL(_Exit, std::, )
+SYMBOL(accumulate, std::, )
+SYMBOL(add_const, std::, )
+SYMBOL(add_const_t, std::, )
+SYMBOL(add_cv, std::, )
+SYMBOL(add_cv_t, std::, )
+SYMBOL(add_lvalue_reference, std::, )
+SYMBOL(add_lvalue_reference_t, std::, )
+SYMBOL(add_pointer, std::, )
+SYMBOL(add_pointer_t, std::, )
+SYMBOL(add_rvalue_reference, std::, )
+SYMBOL(add_rvalue_reference_t, std::, )
+SYMBOL(add_volatile, std::, )
+SYMBOL(add_volatile_t, std::, )
+SYMBOL(addressof, std::, )
+SYMBOL(adjacent_difference, std::, )
+SYMBOL(adjacent_find, std::, )
+SYMBOL(adopt_lock, std::, )
+SYMBOL(adopt_lock_t, std::, )
+SYMBOL(advance, std::, )
+SYMBOL(align, std::, )
+SYMBOL(align_val_t, std::, )
+SYMBOL(aligned_alloc, std::, )
+SYMBOL(aligned_storage, std::, )
+SYMBOL(aligned_storage_t, std::, )
+SYMBOL(aligned_union, std::, )
+SYMBOL(aligned_union_t, std::, )
+SYMBOL(alignment_of, std::, )
+SYMBOL(alignment_of_v, std::, )
+SYMBOL(all_of, std::, )
+SYMBOL(allocate_shared, std::, )
+SYMBOL(allocator, std::, )
+SYMBOL(allocator_arg, std::, )
+SYMBOL(allocator_arg_t, std::, )
+SYMBOL(allocator_traits, std::, )
+SYMBOL(any, std::, )
+SYMBOL(any_of, std::, )
+SYMBOL(apply, std::, )
+SYMBOL(arg, std::, )
+SYMBOL(array, std::, )
+SYMBOL(as_const, std::, )
+SYMBOL(asctime, std::, )
+SYMBOL(async, std::, )
+SYMBOL(at_quick_exit, std::, )
+SYMBOL(atexit, std::, )
+SYMBOL(atof, std::, )
+SYMBOL(atoi, std::, )
+SYMBOL(atol, std::, )
+SYMBOL(atoll, std::, )
+SYMBOL(atomic_compare_exchange_strong, std::, )
+SYMBOL(atomic_compare_exchange_strong_explicit, std::, )
+SYMBOL(atomic_compare_exchange_weak, std::, )
+SYMBOL(atomic_compare_exchange_weak_explicit, std::, )
+SYMBOL(atomic_exchange, std::, )
+SYMBOL(atomic_exchange_explicit, std::, )
+SYMBOL(atomic_fetch_add, std::, )
+SYMBOL(atomic_fetch_add_explicit, std::, )
+SYMBOL(atomic_fetch_and, std::, )
+SYM

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE356134: [clangd] Using symbol name to map includes for STL 
symbols. (authored by hokein, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D58345?vs=190598&id=190600#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58345

Files:
  clangd/StdSymbolMap.inc
  clangd/include-mapping/gen_std.py
  clangd/include-mapping/test.py
  clangd/index/CanonicalIncludes.cpp

Index: clangd/include-mapping/gen_std.py
===
--- clangd/include-mapping/gen_std.py
+++ clangd/include-mapping/gen_std.py
@@ -0,0 +1,149 @@
+#!/usr/bin/env python
+#===- gen_std.py -  --*- python -*--===#
+#
+# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+# See https://llvm.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+#
+#======#
+
+"""gen_std.py is a tool to generate a lookup table (from qualified names to
+include headers) for C++ Standard Library symbols by parsing archieved HTML
+files from cppreference.
+
+Caveats and FIXMEs:
+  - only symbols directly in "std" namespace are added, we should also add std's
+subnamespace symbols (e.g. chrono).
+  - symbols with multiple variants or defined in multiple headers aren't added,
+e.g. std::move, std::swap
+
+Usage:
+  1. Install BeautifulSoup dependency, see instruction:
+   https://www.crummy.com/software/BeautifulSoup/bs4/doc/#installing-beautiful-soup
+  2. Download cppreference offline HTML files (e.g. html_book_20181028.zip) at
+   https://en.cppreference.com/w/Cppreference:Archives
+  3. Unzip the zip file from step 2 to directory , you should
+ get a "reference" directory in 
+  4. Run the command:
+   gen_std.py -cppreference  > StdSymbolMap.inc
+"""
+
+from bs4 import BeautifulSoup
+
+import argparse
+import datetime
+import os
+import sys
+
+STDGEN_CODE_PREFIX = """\
+//===-- gen_std.py generated file ---*- C++ -*-===//
+//
+// Used to build a lookup table (qualified names => include headers) for C++
+// Standard Library symbols.
+//
+// Automatically generated file, DO NOT EDIT!
+//
+// Generated from cppreference offline HTML book (modified on %s).
+//===--===//
+"""
+
+def ParseSymbolPage(symbol_page_html):
+  """Parse symbol page and retrieve the include header defined in this page.
+  The symbol page provides header for the symbol, specifically in
+  "Defined in header " section. An example:
+
+  
+ Defined in header  
+  
+
+  Returns a list of headers.
+  """
+  headers = []
+
+  soup = BeautifulSoup(symbol_page_html, "html.parser")
+  #  "Defined in header " are defined in  or
+  #  .
+  for header_tr in soup.select('tr.t-dcl-header,tr.t-dsc-header'):
+if "Defined in header " in header_tr.text:
+  # The interesting header content (e.g. ) is wrapped in .
+  for header_code in header_tr.find_all("code"):
+headers.append(header_code.text)
+  return headers
+
+
+def ParseIndexPage(index_page_html):
+  """Parse index page.
+  The index page lists all std symbols and hrefs to their detailed pages
+  (which contain the defined header). An example:
+
+  abs() (int) 
+  acos() 
+
+  Returns a list of tuple (symbol_name, relative_path_to_symbol_page).
+  """
+  symbols = []
+  soup = BeautifulSoup(index_page_html, "html.parser")
+  for symbol_href in soup.select("a[title]"):
+symbol_tt = symbol_href.find("tt")
+if symbol_tt:
+  symbols.append((symbol_tt.text.rstrip("<>()"), # strip any trailing <>()
+  symbol_href["href"]))
+  return symbols
+
+
+def ParseArg():
+  parser = argparse.ArgumentParser(description='Generate StdGen file')
+  parser.add_argument('-cppreference', metavar='PATH',
+  default='',
+  help='path to the cppreference offline HTML directory',
+  required=True
+  )
+  return parser.parse_args()
+
+
+def main():
+  args = ParseArg()
+  cpp_reference_root = args.cppreference
+  cpp_symbol_root = os.path.join(cpp_reference_root, "en", "cpp")
+  index_page_path = os.path.join(cpp_symbol_root, "symbol_index.html")
+  if not os.path.exists(index_page_path):
+exit("Path %s doesn't exist!" % index_page_path)
+
+  # We don't have version information from the unzipped offline HTML files.
+  # so we use the modified time of the symbol_index.html as the version.
+  cppreference_modified_date = datetime.datetime.fromtimestamp(
+os.stat(index_page_path).st_mtime).strftime('%Y-%m-%d')
+
+  # Workflow steps:
+  #   1. Parse index page which lists all symbols to get symbol

[PATCH] D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

That works because the argument list is just tok::identifier and TT_StartOfName

so if we see StartOfName we are going to return true

identifier will simply be skipped round the loop

``
int
Test(A a)
{}

  

AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=int L=3 PPK=2 FakeLParens= 
FakeRParens=0 II=0x11be8c48b20 Text='int'
 M=1 C=1 T=FunctionDeclarationName S=1 B=0 BK=0 P=220 Name=identifier L=88 
PPK=2 FakeLParens= FakeRParens=0 II=0x11be8c42d00 Text='Test'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=l_paren L=89 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=140 Name=identifier L=90 PPK=2 FakeLParens= 
FakeRParens=0 II=0x11be8c42c70 Text='A'
 M=0 C=1 T=StartOfName S=1 B=0 BK=0 P=240 Name=identifier L=92 PPK=2 
FakeLParens= FakeRParens=0 II=0x11be8c42ca0 Text='a'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=43 Name=r_paren L=93 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text=')'

  Where you are correct in its handling is this, because it doesn't see the 
TT_StartOfName it doesn't return true and so falls out the bottom of the 
function with false
  and hence doesn't break the return type
  
  int Test(A) {}

AnnotatedTokens(L=0):
 M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=int L=3 PPK=2 FakeLParens= 
FakeRParens=0 II=0x245bb8e7140 Text='int'
 M=0 C=1 T=StartOfName S=1 B=0 BK=0 P=220 Name=identifier L=8 PPK=2 
FakeLParens= FakeRParens=0 II=0x245bb8ebdc0 Text='Test'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=l_paren L=9 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 B=0 BK=0 P=140 Name=identifier L=10 PPK=2 FakeLParens= 
FakeRParens=0 II=0x245bb8ebd30 Text='A'
 M=0 C=0 T=Unknown S=0 B=0 BK=0 P=43 Name=r_paren L=11 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text=')'

  


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

https://reviews.llvm.org/D59309



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

[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D28462#1406716 , @jacob-keller 
wrote:

> In regards to whether clang-format should support this feature, I think it's 
> pretty clear that a large number of users want it to. The tool already 
> supports formatting various other definitions in a similar manner, including 
> variables and struct members. So I don't see how a similar features which 
> aligns consecutive macros is something which doesn't make sense.
>
> Additionally, because the formatting will *undo* such formatting if done 
> manually, it means that the existing source code formatting this way cannot 
> be handled via clang-format. In my case, it essentially means that I cannot 
> use clang-format to enforce the style guidelines, as the macros definitions 
> are aligned.
>
> Additionally, aligning a series of macros makes sense because it helps 
> improve readability, and is thus something that I'd rather not lose if I were 
> to switch to using clang-format without the feature.







Comment at: lib/Format/WhitespaceManager.cpp:436
+static void
+AlignTokenSequenceAll(unsigned Start, unsigned End, unsigned Column,
+  F &&Matches,

I don't think the 'All' postfix in the name is helpful. What are you trying to 
say with that name?



Comment at: lib/Format/WhitespaceManager.cpp:437
+AlignTokenSequenceAll(unsigned Start, unsigned End, unsigned Column,
+  F &&Matches,
+  SmallVector &Changes) {

Why an rvalue ref? Also, this is used only once, so why make this a template?



Comment at: lib/Format/WhitespaceManager.cpp:442
+
+  for (unsigned i = Start; i != End; ++i) {
+if (Changes[i].NewlinesBefore > 0) {

llvm style: use an upper case I for index vars.



Comment at: lib/Format/WhitespaceManager.cpp:473
+
+  // Line number of the start and the end of the current token sequence.
+  unsigned StartOfSequence = 0;

These are indices into Matches, not line numbers, right?



Comment at: lib/Format/WhitespaceManager.cpp:497
+
+  unsigned i = 0;
+  for (unsigned e = Changes.size(); i != e; ++i) {

llvm style: use upper-case I and E.



Comment at: lib/Format/WhitespaceManager.cpp:500
+if (Changes[i].NewlinesBefore != 0) {
+  EndOfSequence = i;
+  // If there is a blank line, or if the last line didn't contain any

Why set EndOfSequence outside the if below?



Comment at: lib/Format/WhitespaceManager.cpp:512
+
+// If there is more than one matching token per line, end the sequence.
+if (FoundMatchOnLine)

What's the reason for this?


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

https://reviews.llvm.org/D28462



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

[PATCH] D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D59309#1428969 , @MyDeveloperDay 
wrote:

> That works because the argument list is just tok::identifier and 
> TT_StartOfName


Should we fix isStartOfName then?

In A or A<8> A should be TT_StartOfName, too, right?


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

https://reviews.llvm.org/D59309



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

[PATCH] D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

of course, this isn't legal

  int TestFn(A=1)
  {
return a;
  }

and, would see the TT_StartOfName for (a) and would break

  int
  TestFn(A a = 1)
  {
return a;
  }


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

https://reviews.llvm.org/D59309



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

[PATCH] D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D59309#1428970 , @klimek wrote:

> In D59309#1428969 , @MyDeveloperDay 
> wrote:
>
> > That works because the argument list is just tok::identifier and 
> > TT_StartOfName
>
>
> Should we fix isStartOfName then?
>
> In A or A<8> A should be TT_StartOfName, too, right?


Maybe that is a better way, let me look..

I've realized that defining a function Test which returns a class, and has an 
unnamed class parameter in its definition

  B Test(A);

is indistinguishable from a variable declaration Test of type B which passes a 
variable A into its constructor, without knowing that A is a class and not a 
variable, there isn't enough information to say this is a function or not

  B Test(A);

Which is why we should always name our parameters in function declarations ;-)


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

https://reviews.llvm.org/D59309



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

[PATCH] D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D59309#1428970 , @klimek wrote:

> In D59309#1428969 , @MyDeveloperDay 
> wrote:
>
> > That works because the argument list is just tok::identifier and 
> > TT_StartOfName
>
>
> Should we fix isStartOfName then?
>
> In A or A<8> A should be TT_StartOfName, too, right?


I'm not sure that would work, the <8> is part of the type not the 
TT_StartOfName, or did I miss something

  M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=int L=3 PPK=2 FakeLParens= 
FakeRParens=0 II=0x263a5ddd090 Text='int'
  M=1 C=1 T=FunctionDeclarationName S=1 B=0 BK=0 P=220 Name=identifier L=88 
PPK=2 FakeLParens= FakeRParens=0 II=0x263a5dda5a0 Text='Test'
  M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=l_paren L=89 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text='('
  M=0 C=1 T=Unknown S=0 B=0 BK=0 P=140 Name=identifier L=90 PPK=2 FakeLParens= 
FakeRParens=0 II=0x263a5dda510 Text='A'
  M=0 C=1 T=StartOfName S=1 B=0 BK=0 P=240 Name=identifier L=92 PPK=2 
FakeLParens= FakeRParens=0 II=0x263a5dda540 Text='a'
  M=0 C=0 T=Unknown S=0 B=0 BK=0 P=43 Name=r_paren L=93 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text=')



  M=0 C=0 T=Unknown S=1 B=0 BK=0 P=0 Name=int L=3 PPK=2 FakeLParens= 
FakeRParens=0 II=0x263a5ddd090 Text='int'
  M=1 C=1 T=FunctionDeclarationName S=1 B=0 BK=0 P=220 Name=identifier L=88 
PPK=2 FakeLParens= FakeRParens=0 II=0x263a5dda4e0 Text='TestFn'
  M=0 C=0 T=Unknown S=0 B=0 BK=0 P=23 Name=l_paren L=89 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text='('
  M=0 C=1 T=Unknown S=0 B=0 BK=0 P=140 Name=identifier L=90 PPK=2 FakeLParens= 
FakeRParens=0 II=0x263a5dda510 Text='A'
  M=0 C=0 T=TemplateOpener S=0 B=0 BK=0 P=50 Name=less L=91 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text='<'
  M=0 C=1 T=Unknown S=0 B=0 BK=0 P=380 Name=bool L=95 PPK=2 FakeLParens= 
FakeRParens=0 II=0x263a5ddd670 Text='bool'
  M=0 C=0 T=TemplateCloser S=0 B=0 BK=0 P=290 Name=greater L=96 PPK=2 
FakeLParens= FakeRParens=0 II=0x0 Text='>'
  M=0 C=1 T=StartOfName S=1 B=0 BK=0 P=240 Name=identifier L=98 PPK=2 
FakeLParens= FakeRParens=0 II=0x263a5dda540 Text='a'
  M=0 C=0 T=Unknown S=0 B=0 BK=0 P=43 Name=r_paren L=99 PPK=2 FakeLParens= 
FakeRParens=0 II=0x0 Text=')'


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

https://reviews.llvm.org/D59309



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

[PATCH] D57860: [analyzer] Validate checker option names and values

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

xazax.hun added a comment.

Did we test all the codepaths including the package level configs? If not, 
please add some package level config option related tests.
Otherwise the patch looks good to me after all the comments are resolved.


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

https://reviews.llvm.org/D57860



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

[PATCH] D59300: [clangd] Tune the fuzzy-matching algorithm

[Ilya Biryukov via Phabricator via cfe-commits]

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

- Adjust the tweaks, bring back removed heuristics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59300

Files:
  clang-tools-extra/clangd/FuzzyMatch.cpp
  clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp


Index: clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
===
--- clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
+++ clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
@@ -9,6 +9,7 @@
 #include "FuzzyMatch.h"
 
 #include "llvm/ADT/StringExtras.h"
+#include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -270,12 +271,18 @@
 // Verify some bounds so we know scores fall in the right range.
 // Testing exact scores is fragile, so we prefer Ranking tests.
 TEST(FuzzyMatch, Scoring) {
-  EXPECT_THAT("abs", matches("[a]w[B]xYz[S]", 0.f));
+  EXPECT_THAT("abs", matches("[a]w[B]xYz[S]", 7.f / 12.f));
   EXPECT_THAT("abs", matches("[abs]l", 1.f));
   EXPECT_THAT("abs", matches("[abs]", 2.f));
   EXPECT_THAT("Abs", matches("[abs]", 2.f));
 }
 
+TEST(FuzzyMatch, InitialismAndSegment) {
+  // We want these scores to be roughly the same.
+  EXPECT_THAT("up", matches("[u]nique_[p]tr", 3.f / 4.f));
+  EXPECT_THAT("up", matches("[up]per_bound", 1.f));
+}
+
 // Returns pretty-printed segmentation of Text.
 // e.g. std::basic_string --> +--  + +-
 std::string segment(llvm::StringRef Text) {
Index: clang-tools-extra/clangd/FuzzyMatch.cpp
===
--- clang-tools-extra/clangd/FuzzyMatch.cpp
+++ clang-tools-extra/clangd/FuzzyMatch.cpp
@@ -71,7 +71,7 @@
 // Score field is 15 bits wide, min value is -2^14, we use half of that.
 static constexpr int AwfulScore = -(1 << 13);
 static bool isAwful(int S) { return S < AwfulScore / 2; }
-static constexpr int PerfectBonus = 3; // Perfect per-pattern-char score.
+static constexpr int PerfectBonus = 4; // Perfect per-pattern-char score.
 
 FuzzyMatcher::FuzzyMatcher(llvm::StringRef Pattern)
 : PatN(std::min(MaxPat, Pattern.size())),
@@ -267,24 +267,26 @@
 }
 
 int FuzzyMatcher::skipPenalty(int W, Action Last) const {
-  int S = 0;
+  if (W == 0) // Skipping the first character.
+return 3;
   if (WordRole[W] == Head) // Skipping a segment.
-S += 1;
-  if (Last == Match) // Non-consecutive match.
-S += 2;  // We'd rather skip a segment than split our match.
-  return S;
+return 1; // We want to keep it lower than the bonus for a consecutive
+  // match.
+  return 0;
 }
 
 int FuzzyMatcher::matchBonus(int P, int W, Action Last) const {
   assert(LowPat[P] == LowWord[W]);
   int S = 1;
-  // Bonus: pattern so far is a (case-insensitive) prefix of the word.
-  if (P == W) // We can't skip pattern characters, so we must have matched all.
-++S;
+  bool IsPatSingleCase =
+  (PatTypeSet == 1 << Lower) || (PatTypeSet == 1 << Upper);
   // Bonus: case matches, or a Head in the pattern aligns with one in the word.
-  if ((Pat[P] == Word[W] && ((PatTypeSet & 1 << Upper) || P == W)) ||
-  (PatRole[P] == Head && WordRole[W] == Head))
+  if (Pat[P] == Word[W] ||
+  (WordRole[W] == Head && (IsPatSingleCase || PatRole[P] == Head)))
 ++S;
+  // Bonus: a consecutive match.
+  if (W == 0 || Last == Match)
+S += 2;
   // Penalty: matching inside a segment (and previous char wasn't matched).
   if (WordRole[W] == Tail && P && Last == Miss)
 S -= 3;


Index: clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
===
--- clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
+++ clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp
@@ -9,6 +9,7 @@
 #include "FuzzyMatch.h"
 
 #include "llvm/ADT/StringExtras.h"
+#include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -270,12 +271,18 @@
 // Verify some bounds so we know scores fall in the right range.
 // Testing exact scores is fragile, so we prefer Ranking tests.
 TEST(FuzzyMatch, Scoring) {
-  EXPECT_THAT("abs", matches("[a]w[B]xYz[S]", 0.f));
+  EXPECT_THAT("abs", matches("[a]w[B]xYz[S]", 7.f / 12.f));
   EXPECT_THAT("abs", matches("[abs]l", 1.f));
   EXPECT_THAT("abs", matches("[abs]", 2.f));
   EXPECT_THAT("Abs", matches("[abs]", 2.f));
 }
 
+TEST(FuzzyMatch, InitialismAndSegment) {
+  // We want these scores to be roughly the same.
+  EXPECT_THAT("up", matches("[u]nique_[p]tr", 3.f / 4.f));
+  EXPECT_THAT("up", matches("[up]per_bound", 1.f));
+}
+
 // Returns pretty-printed segmentation of Text.
 // e.g. std::basic_string --> +--  + +-
 std::string segment(llvm::StringRef Text) {
Index: clang-tools-extra/clangd/FuzzyMatch.cpp
===
--- clang-tools-extra/clangd/FuzzyMatch.

[PATCH] D59300: [clangd] Tune the fuzzy-matching algorithm

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Here are the stats for the latest version. We now get a significant bump for 
initialisms and a much lower hit in non-initialism cases.

  
==
  OVERALL (excl. CROSS_NAMESPACE and 
INITIALISMS)
  
==
Total measurements: 108483 (+0)
Average latency (ms): 223.570343018 (-5)
All measurements:
MRR: 69.72 (-0.31)  Top-1: 60.32% (-0.40%)  Top-5: 81.56% (-0.13%)  
Top-100: 96.12% (-0.03%)
Full identifiers:
MRR: 97.72 (-0.48)  Top-1: 96.69% (-0.88%)  Top-5: 98.97% (-0.04%)  
Top-100: 99.15% (+0.00%)
Filter length 0-5:
MRR:  29.15 (-0.00) 62.03 (+0.42)   71.61 (-0.50)   
74.13 (-0.43)   76.52 (-0.60)   80.52 (-0.74)
Top-1:17.52% (+0.00%)   49.21% (+0.50%) 60.51% 
(-0.46%) 63.35% (-0.48%) 66.59% (-0.74%) 71.83% (-0.93%)
Top-5:42.53% (-0.01%)   78.96% (+0.40%) 86.18% 
(-0.29%) 87.76% (-0.31%) 88.92% (-0.38%) 91.36% (-0.39%)
Top-100:  84.57% (-0.01%)   96.74% (+0.15%) 98.15% 
(-0.03%) 98.31% (-0.13%) 98.43% (-0.16%) 98.60% (-0.08%)
  
==
  INITIALISMS
  
==
Total measurements: 16489 (+0)
Average latency (ms): 207.854690552 (-5)
All measurements:
MRR: 82.31 (+3.81)  Top-1: 74.67% (+4.40%)  Top-5: 91.76% (+2.93%)  
Top-100: 98.41% (+0.28%)
Initialism length 2-4:
MRR:  80.43 (+2.49) 85.02 (+3.48)   86.64 (+13.05)
Top-1:71.77% (+3.06%)   78.81% (+3.58%) 81.47% 
(+15.15%)
Top-5:91.22% (+1.60%)   92.54% (+3.27%) 92.94% 
(+10.37%)
Top-100:  98.34% (+0.19%)   98.57% (+0.26%) 98.40% 
(+0.86%)
  
==
  DEFAULT
  
==
Total measurements: 51805 (+0)
Average latency (ms): 256.839630127 (-3)
All measurements:
MRR: 61.93 (-0.44)  Top-1: 51.91% (-0.52%)  Top-5: 74.55% (-0.21%)  
Top-100: 92.72% (-0.07%)
Full identifiers:
MRR: 96.38 (-0.15)  Top-1: 95.18% (-0.20%)  Top-5: 97.96% (-0.05%)  
Top-100: 98.30% (+0.00%)
Filter length 0-5:
MRR:  16.76 (-0.01) 48.89 (+0.32)   63.24 (-0.85)   
67.15 (-0.58)   71.30 (-0.78)   73.47 (-1.25)
Top-1:9.05% (+0.00%)34.65% (+0.26%) 50.29% 
(-0.82%) 54.37% (-0.67%) 59.95% (-0.89%) 63.15% (-1.56%)
Top-5:23.55% (-0.01%)   68.04% (+0.51%) 80.76% 
(-0.33%) 83.50% (-0.46%) 85.67% (-0.61%) 86.59% (-0.67%)
Top-100:  70.69% (-0.01%)   93.65% (+0.31%) 96.64% 
(-0.06%) 96.96% (-0.27%) 97.20% (-0.34%) 97.35% (-0.17%)
  
==
  EXPLICIT_MEMBER_ACCESS
  
==
Total measurements: 30162 (+0)
Average latency (ms): 113.366485596 (-9)
All measurements:
MRR: 68.79 (-0.18)  Top-1: 59.23% (-0.28%)  Top-5: 80.68% (-0.05%)  
Top-100: 98.64% (+0.00%)
Full identifiers:
MRR: 98.20 (-1.43)  Top-1: 96.72% (-2.71%)  Top-5: 99.82% (-0.07%)  
Top-100: 99.89% (+0.00%)
Filter length 0-5:
MRR:  27.83 (-0.00) 62.41 (+0.79)   69.19 (-0.27)   
71.31 (-0.13)   72.93 (-0.07)   82.67 (-0.11)
Top-1:16.48% (+0.00%)   49.23% (+1.06%) 57.90% 
(-0.09%) 60.58% (+0.00%) 62.56% (-0.02%) 74.12% (-0.13%)
Top-5:39.63% (+0.00%)   80.07% (+0.55%) 83.96% 
(-0.47%) 85.22% (-0.19%) 85.93% (-0.12%) 93.20% (-0.08%)
Top-100:  94.21% (+0.00%)   99.21% (+0.02%) 99.24% 
(+0.00%) 99.26% (+0.00%) 99.28% (+0.00%) 99.71% (+0.00%)
  
==
  WANT_LOCAL
  
=

[PATCH] D59347: [DebugInfo] Combine Trivial and NonTrivial flags

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

I'd preserve the trivial test cases and show that the NonTrivial flag is *not* 
present.  I can suggest ways to achieve this if you like (note there is no 
CHECK-DAG-NOT combined directive).




Comment at: test/CodeGenCXX/debug-info-composite-triviality.cpp:88
-
-// CHECK-DAG: !DICompositeType({{.*}}, name: "NonTrivialE",{{.*}}flags: 
{{.*}}DIFlagNonTrivial
-struct NonTrivialE : Trivial, NonTrivial {

This one used to be flagged nontrivial, and now it's not flagged?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59347



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

[PATCH] D59360: [clang-tidy] Fix more false positives for bugprone-string-integer-assignment

[Clement Courbet via Phabricator via cfe-commits]

courbet created this revision.
courbet added reviewers: JonasToth, alexfh, xazax.hun.
Herald added subscribers: jdoerfert, rnkovacs.
Herald added a project: clang.

And add various tests gleaned for our codebase.

See PR27723.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59360

Files:
  clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp
  clang-tools-extra/test/clang-tidy/bugprone-string-integer-assignment.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-string-integer-assignment.cpp
===
--- clang-tools-extra/test/clang-tidy/bugprone-string-integer-assignment.cpp
+++ clang-tools-extra/test/clang-tidy/bugprone-string-integer-assignment.cpp
@@ -7,6 +7,8 @@
   basic_string& operator=(basic_string);
   basic_string& operator+=(T);
   basic_string& operator+=(basic_string);
+  const T &operator[](int i) const;
+  T &operator[](int i);
 };
 
 typedef basic_string string;
@@ -21,10 +23,13 @@
 
 typedef int MyArcaneChar;
 
+constexpr char kCharConstant = 'a';
+
 int main() {
   std::string s;
   std::wstring ws;
   int x = 5;
+  const char c = 'c';
 
   s = 6;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: an integer is interpreted as a character code when assigning {{.*}} [bugprone-string-integer-assignment]
@@ -58,12 +63,36 @@
 
   s += toupper(x);
   s += tolower(x);
-  s += std::tolower(x);
+  s += (std::tolower(x));
+
+  s += c & s[1];
+  s += c ^ s[1];
+  s += c | s[1];
+
+  s[x] += 1;
+  s += s[x];
+  as += as[x];
 
   // Likely character expressions.
   s += x & 0xff;
   s += 0xff & x;
+  s += x % 26;
+  s += 26 % x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: an integer is interpreted as a chara
+  // CHECK-FIXES: {{^}}  s += std::to_string(26 % x);{{$}}
+  s += c | 0x80;
+  s += c | 0x8000;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: an integer is interpreted as a chara
+  // CHECK-FIXES: {{^}}  s += std::to_string(c | 0x8000);{{$}}
+  as += c | 0x8000;
 
   s += 'a' + (x % 26);
+  s += kCharConstant + (x % 26);
+  s += 'a' + (s[x] & 0xf);
   s += (x % 10) + 'b';
+
+  s += x > 255 ? c : x;
+  s += x > 255 ? 12 : x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: an integer is interpreted as a chara
+  // CHECK-FIXES: {{^}}  s += std::to_string(x > 255 ? 12 : x);{{$}}
 }
Index: clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp
@@ -45,38 +45,98 @@
   this);
 }
 
-static bool isLikelyCharExpression(const Expr *Argument,
-   const ASTContext &Ctx) {
-  const auto *BinOp = dyn_cast(Argument);
-  if (!BinOp)
+class CharExpressionDetector {
+public:
+  CharExpressionDetector(const QualType CharType, const ASTContext &Ctx)
+  : CharType(CharType), Ctx(Ctx) {}
+
+  bool isLikelyCharExpression(const Expr *E) const {
+if (isCharTyped(E))
+  return true;
+
+if (const auto *BinOp = dyn_cast(E)) {
+  const auto *LHS = BinOp->getLHS()->IgnoreParenImpCasts();
+  const auto *RHS = BinOp->getRHS()->IgnoreParenImpCasts();
+  // Handle both directions, e.g. `'a' + (i % 26)` and `(i % 26) + 'a'`.
+  if (BinOp->isAdditiveOp() || BinOp->isBitwiseOp())
+return handleBinaryOp(BinOp->getOpcode(), LHS, RHS) ||
+   handleBinaryOp(BinOp->getOpcode(), RHS, LHS);
+  // Except in the case of '%'.
+  if (BinOp->getOpcode() == BO_Rem)
+return handleBinaryOp(BinOp->getOpcode(), LHS, RHS);
+  return false;
+}
+
+// ternary where at least one branch is a likely char expression, e.g.
+//i < 265 ? i : ' '
+if (const auto *CondOp = dyn_cast(E))
+  return isLikelyCharExpression(
+ CondOp->getFalseExpr()->IgnoreParenImpCasts()) ||
+ isLikelyCharExpression(
+ CondOp->getTrueExpr()->IgnoreParenImpCasts());
 return false;
-  const auto *LHS = BinOp->getLHS()->IgnoreParenImpCasts();
-  const auto *RHS = BinOp->getRHS()->IgnoreParenImpCasts();
-  //  & , mask is a compile time constant.
-  Expr::EvalResult RHSVal;
-  if (BinOp->getOpcode() == BO_And &&
-  (RHS->EvaluateAsInt(RHSVal, Ctx, Expr::SE_AllowSideEffects) ||
-   LHS->EvaluateAsInt(RHSVal, Ctx, Expr::SE_AllowSideEffects)))
-return true;
-  //  + ( % ), where  is a char literal.
-  const auto IsCharPlusModExpr = [](const Expr *L, const Expr *R) {
-const auto *ROp = dyn_cast(R);
-return ROp && ROp->getOpcode() == BO_Rem && isa(L);
-  };
-  if (BinOp->getOpcode() == BO_Add) {
-if (IsCharPlusModExpr(LHS, RHS) || IsCharPlusModExpr(RHS, LHS))
+  }
+
+private:
+  bool handleBinaryOp(clang::BinaryOperatorKind Opcode, const Expr *const LHS,
+  const Expr *const RHS) const {
+//(c++ integer promotion rules make this an
+// 

[PATCH] D57464: Generalize method overloading on addr spaces to C++

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In D57464#1425904 , @Anastasia wrote:

> I think not. :( But I am wondering if we could proceed for now in some 
> general direction and then make any improvements later. Probably the biggest 
> part of this patch is not going to change. Would this make sense?


Well, I'm still not convinced that it's the right way to do it...  And it feels 
a bit off to fix things later if we pretty much know what the correct way is 
now. It feels a bit unreasonable to ask for a larger redesign since this has 
been laying for a while at this point, but I think that if we're going to do it 
we should do it right from the start.

I think that the way to approach it is either to

- Pass a pointer to the `LateParsedAttrList` down through the 
`Parse*Declarator` functions and `ParseTypeQualifierListOpt` and use it 
appropriately in `ParseFunctionDeclarator`. That's fairly invasive in all of 
the callers of the declarator parsing functions, though.
- Store a pointer to the `LateParsedAttrList` in `Declarator` and use it the 
same way as above. This avoids messing with most of the existing functions, but 
makes Declarator a bit larger.

I think I would lean towards the latter since it means less fudging around with 
a whole bunch of unrelated methods. Do @rjmccall or @rsmith have any further 
opinions on this?


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

https://reviews.llvm.org/D57464



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

[PATCH] D59327: [Sema] Fix a use-after-free of a _Nonnull ParsedAttr

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rC Clang

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

https://reviews.llvm.org/D59327



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

[PATCH] D59361: [CUDA][Windows] Partial fix for bug 38811 (Step 1 of 3)

[Evgeny Mankov via Phabricator via cfe-commits]

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

Partial fix for the clang Bug 38811 
 "Clang fails to compile with 
CUDA-9.x on Windows".

Adding  `defined(_WIN64)` check along with existing `#if defined(__LP64__)` 
eliminates the below clang (64-bit) compilation error on Windows.

  
C:/GIT/LLVM/trunk/llvm-64-release-vs2017/dist/lib/clang/9.0.0\include\__clang_cuda_device_functions.h(1609,45):
 error GEF7559A7: no matching function for call to 'roundf'
   __DEVICE__ long lroundf(float __a) { return roundf(__a); }

[How to repro]

  clang++.exe -x cuda "c:\ProgramData\NVIDIA Corporation\CUDA 
Samples\v9.0\0_Simple\simplePrintf\simplePrintf.cu" -I"c:\ProgramData\NVIDIA 
Corporation\CUDA Samples\v8.0\common\inc" --cuda-gpu-arch=sm_50 
--cuda-path="C:\Program Files\NVIDIA GPU Computing Toolkit\CUDA\v9.0" 
-L"c:\Program Files\NVIDIA GPU Computing Toolkit\CUDA\v9.0\lib\x64" 
-lcudart.lib  -v




Repository:
  rC Clang

https://reviews.llvm.org/D59361

Files:
  clang/lib/Headers/__clang_cuda_device_functions.h


Index: clang/lib/Headers/__clang_cuda_device_functions.h
===
--- clang/lib/Headers/__clang_cuda_device_functions.h
+++ clang/lib/Headers/__clang_cuda_device_functions.h
@@ -1563,7 +1563,7 @@
 __DEVICE__ float j1f(float __a) { return __nv_j1f(__a); }
 __DEVICE__ double jn(int __n, double __a) { return __nv_jn(__n, __a); }
 __DEVICE__ float jnf(int __n, float __a) { return __nv_jnf(__n, __a); }
-#if defined(__LP64__)
+#if defined(__LP64__) || defined(_WIN64)
 __DEVICE__ long labs(long __a) { return llabs(__a); };
 #else
 __DEVICE__ long labs(long __a) { return __nv_abs(__a); };
@@ -1597,7 +1597,7 @@
 __DEVICE__ float logf(float __a) {
   return __FAST_OR_SLOW(__nv_fast_logf, __nv_logf)(__a);
 }
-#if defined(__LP64__)
+#if defined(__LP64__) || defined(_WIN64)
 __DEVICE__ long lrint(double __a) { return llrint(__a); }
 __DEVICE__ long lrintf(float __a) { return __float2ll_rn(__a); }
 __DEVICE__ long lround(double __a) { return llround(__a); }


Index: clang/lib/Headers/__clang_cuda_device_functions.h
===
--- clang/lib/Headers/__clang_cuda_device_functions.h
+++ clang/lib/Headers/__clang_cuda_device_functions.h
@@ -1563,7 +1563,7 @@
 __DEVICE__ float j1f(float __a) { return __nv_j1f(__a); }
 __DEVICE__ double jn(int __n, double __a) { return __nv_jn(__n, __a); }
 __DEVICE__ float jnf(int __n, float __a) { return __nv_jnf(__n, __a); }
-#if defined(__LP64__)
+#if defined(__LP64__) || defined(_WIN64)
 __DEVICE__ long labs(long __a) { return llabs(__a); };
 #else
 __DEVICE__ long labs(long __a) { return __nv_abs(__a); };
@@ -1597,7 +1597,7 @@
 __DEVICE__ float logf(float __a) {
   return __FAST_OR_SLOW(__nv_fast_logf, __nv_logf)(__a);
 }
-#if defined(__LP64__)
+#if defined(__LP64__) || defined(_WIN64)
 __DEVICE__ long lrint(double __a) { return llrint(__a); }
 __DEVICE__ long lrintf(float __a) { return __float2ll_rn(__a); }
 __DEVICE__ long lround(double __a) { return llround(__a); }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59360: [clang-tidy] Fix more false positives for bugprone-string-integer-assignment

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp:50
+public:
+  CharExpressionDetector(const QualType CharType, const ASTContext &Ctx)
+  : CharType(CharType), Ctx(Ctx) {}

No need for the top-level const in the first parameter.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp:70
+
+// ternary where at least one branch is a likely char expression, e.g.
+//i < 265 ? i : ' '

nit: Please capitalize the first word.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp:115
+Expr::EvalResult EvalResult;
+if (!E->EvaluateAsInt(EvalResult, Ctx, Expr::SE_AllowSideEffects))
+  return false;

I believe you should also check (or assert) that `E` is not 
instantiation-dependent before running the evaluator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59360



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

[PATCH] D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

Yea, the
a b(c)
problem is the olden C++ problem clang-format will never be able to solve, thus 
all these crazy heuristics.

So, inching closer to understanding this - I totally missed the isLiteral() 
that short-circuits out before. I'm more happy with your current approach, 
except I'd turn it around:
I'd only short-circuit out of the loop returning false in line 2064 if 
isLiteral() && (TemplateOpenerLevel > 0).
Alternatively, we could try to skip things within template <> - don't we have 
the closing > for an opening < in MatchingParen?


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

https://reviews.llvm.org/D59309



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

[PATCH] D59360: [clang-tidy] Fix more false positives for bugprone-string-integer-assignment

[Clement Courbet via Phabricator via cfe-commits]

courbet marked 3 inline comments as done.
courbet added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp:115
+Expr::EvalResult EvalResult;
+if (!E->EvaluateAsInt(EvalResult, Ctx, Expr::SE_AllowSideEffects))
+  return false;

alexfh wrote:
> I believe you should also check (or assert) that `E` is not 
> instantiation-dependent before running the evaluator.
Interesting. AFAICT if I don't check that , I end up warning in the cases when 
the instantiation does result in a too large constant, which is what we want 
(the user should add a cast if they want to silence this). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59360



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

[PATCH] D59360: [clang-tidy] Fix more false positives for bugprone-string-integer-assignment

[Clement Courbet via Phabricator via cfe-commits]

courbet updated this revision to Diff 190610.
courbet added a comment.

Address review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59360

Files:
  clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp
  clang-tools-extra/test/clang-tidy/bugprone-string-integer-assignment.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-string-integer-assignment.cpp
===
--- clang-tools-extra/test/clang-tidy/bugprone-string-integer-assignment.cpp
+++ clang-tools-extra/test/clang-tidy/bugprone-string-integer-assignment.cpp
@@ -7,6 +7,8 @@
   basic_string& operator=(basic_string);
   basic_string& operator+=(T);
   basic_string& operator+=(basic_string);
+  const T &operator[](int i) const;
+  T &operator[](int i);
 };
 
 typedef basic_string string;
@@ -21,10 +23,13 @@
 
 typedef int MyArcaneChar;
 
+constexpr char kCharConstant = 'a';
+
 int main() {
   std::string s;
   std::wstring ws;
   int x = 5;
+  const char c = 'c';
 
   s = 6;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: an integer is interpreted as a character code when assigning {{.*}} [bugprone-string-integer-assignment]
@@ -58,12 +63,36 @@
 
   s += toupper(x);
   s += tolower(x);
-  s += std::tolower(x);
+  s += (std::tolower(x));
+
+  s += c & s[1];
+  s += c ^ s[1];
+  s += c | s[1];
+
+  s[x] += 1;
+  s += s[x];
+  as += as[x];
 
   // Likely character expressions.
   s += x & 0xff;
   s += 0xff & x;
+  s += x % 26;
+  s += 26 % x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: an integer is interpreted as a chara
+  // CHECK-FIXES: {{^}}  s += std::to_string(26 % x);{{$}}
+  s += c | 0x80;
+  s += c | 0x8000;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: an integer is interpreted as a chara
+  // CHECK-FIXES: {{^}}  s += std::to_string(c | 0x8000);{{$}}
+  as += c | 0x8000;
 
   s += 'a' + (x % 26);
+  s += kCharConstant + (x % 26);
+  s += 'a' + (s[x] & 0xf);
   s += (x % 10) + 'b';
+
+  s += x > 255 ? c : x;
+  s += x > 255 ? 12 : x;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: an integer is interpreted as a chara
+  // CHECK-FIXES: {{^}}  s += std::to_string(x > 255 ? 12 : x);{{$}}
 }
Index: clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp
@@ -45,38 +45,98 @@
   this);
 }
 
-static bool isLikelyCharExpression(const Expr *Argument,
-   const ASTContext &Ctx) {
-  const auto *BinOp = dyn_cast(Argument);
-  if (!BinOp)
+class CharExpressionDetector {
+public:
+  CharExpressionDetector(QualType CharType, const ASTContext &Ctx)
+  : CharType(CharType), Ctx(Ctx) {}
+
+  bool isLikelyCharExpression(const Expr *E) const {
+if (isCharTyped(E))
+  return true;
+
+if (const auto *BinOp = dyn_cast(E)) {
+  const auto *LHS = BinOp->getLHS()->IgnoreParenImpCasts();
+  const auto *RHS = BinOp->getRHS()->IgnoreParenImpCasts();
+  // Handle both directions, e.g. `'a' + (i % 26)` and `(i % 26) + 'a'`.
+  if (BinOp->isAdditiveOp() || BinOp->isBitwiseOp())
+return handleBinaryOp(BinOp->getOpcode(), LHS, RHS) ||
+   handleBinaryOp(BinOp->getOpcode(), RHS, LHS);
+  // Except in the case of '%'.
+  if (BinOp->getOpcode() == BO_Rem)
+return handleBinaryOp(BinOp->getOpcode(), LHS, RHS);
+  return false;
+}
+
+// Ternary where at least one branch is a likely char expression, e.g.
+//i < 265 ? i : ' '
+if (const auto *CondOp = dyn_cast(E))
+  return isLikelyCharExpression(
+ CondOp->getFalseExpr()->IgnoreParenImpCasts()) ||
+ isLikelyCharExpression(
+ CondOp->getTrueExpr()->IgnoreParenImpCasts());
 return false;
-  const auto *LHS = BinOp->getLHS()->IgnoreParenImpCasts();
-  const auto *RHS = BinOp->getRHS()->IgnoreParenImpCasts();
-  //  & , mask is a compile time constant.
-  Expr::EvalResult RHSVal;
-  if (BinOp->getOpcode() == BO_And &&
-  (RHS->EvaluateAsInt(RHSVal, Ctx, Expr::SE_AllowSideEffects) ||
-   LHS->EvaluateAsInt(RHSVal, Ctx, Expr::SE_AllowSideEffects)))
-return true;
-  //  + ( % ), where  is a char literal.
-  const auto IsCharPlusModExpr = [](const Expr *L, const Expr *R) {
-const auto *ROp = dyn_cast(R);
-return ROp && ROp->getOpcode() == BO_Rem && isa(L);
-  };
-  if (BinOp->getOpcode() == BO_Add) {
-if (IsCharPlusModExpr(LHS, RHS) || IsCharPlusModExpr(RHS, LHS))
+  }
+
+private:
+  bool handleBinaryOp(clang::BinaryOperatorKind Opcode, const Expr *const LHS,
+  const Expr *const RHS) const {
+//(c++ integer promotion rules make this an
+// int), e.g.
+//'a' + c
+if (isCharTyped(LHS) && isCharTyped

r356142 - [analyzer] Fix function macro crash

[Kristof Umann via cfe-commits]

Author: szelethus
Date: Thu Mar 14 06:38:16 2019
New Revision: 356142

URL: http://llvm.org/viewvc/llvm-project?rev=356142&view=rev
Log:
[analyzer] Fix function macro crash

Re-commit D57893.

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

Modified:
cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
cfe/trunk/test/Analysis/plist-macros-with-expansion.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp?rev=356142&r1=356141&r2=356142&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp Thu Mar 14 06:38:16 
2019
@@ -900,7 +900,8 @@ static std::string getMacroNameAndPrintE
   // If this is a function-like macro, skip its arguments, as
   // getExpandedMacro() already printed them. If this is the case, let's
   // first jump to the '(' token.
-  if (MI->getNumParams() != 0)
+  auto N = std::next(It);
+  if (N != E && N->is(tok::l_paren))
 It = getMatchingRParen(++It, E);
   continue;
 }
@@ -928,7 +929,16 @@ static std::string getMacroNameAndPrintE
 
 getMacroNameAndPrintExpansion(Printer, ArgIt->getLocation(), PP,
   Info.Args, AlreadyProcessedTokens);
-if (MI->getNumParams() != 0)
+// Peek the next token if it is a tok::l_paren. This way we can decide
+// if this is the application or just a reference to a function maxro
+// symbol:
+//
+// #define apply(f) ...
+// #define func(x) ...
+// apply(func)
+// apply(func(42))
+auto N = std::next(ArgIt);
+if (N != ArgEnd && N->is(tok::l_paren))
   ArgIt = getMatchingRParen(++ArgIt, ArgEnd);
   }
   continue;
@@ -990,8 +1000,16 @@ static MacroNameAndArgs getMacroNameAndA
 return { MacroName, MI, {} };
 
   RawLexer.LexFromRawLexer(TheTok);
-  assert(TheTok.is(tok::l_paren) &&
- "The token after the macro's identifier token should be '('!");
+  // When this is a token which expands to another macro function then its
+  // parentheses are not at its expansion locaiton. For example:
+  //
+  // #define foo(x) int bar() { return x; }
+  // #define apply_zero(f) f(0)
+  // apply_zero(foo)
+  //   ^
+  //   This is not a tok::l_paren, but foo is a function.
+  if (TheTok.isNot(tok::l_paren))
+return { MacroName, MI, {} };
 
   MacroArgMap Args;
 

Modified: 
cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist?rev=356142&r1=356141&r2=356142&view=diff
==
--- 
cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
 (original)
+++ 
cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
 Thu Mar 14 06:38:16 2019
@@ -5577,6 +5577,484 @@

   
   
+  
+   path
+   
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line459
+   col33
+   file0
+  
+  
+   line459
+   col33
+   file0
+  
+ 
+end
+ 
+  
+   line459
+   col37
+   file0
+  
+  
+   line459
+   col39
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line459
+  col37
+  file0
+ 
+ ranges
+ 
+   
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col41
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Calling 'foo'
+ message
+ Calling 'foo'
+
+
+ kindevent
+ location
+ 
+  line458
+  col1
+  file0
+ 
+ depth1
+ extended_message
+ Entered call from 'useZeroApplier1'
+ message
+ Entered call from 'useZeroApplier1'
+
+
+ kindevent
+ location
+ 
+  line458
+  col1
+  file0
+ 
+ ranges
+ 
+   
+
+ line458
+ col1
+ file0
+
+
+ line458
+ col16
+ file0
+
+   
+ 
+ depth1
+ extended_message
+ Returning zero
+ message
+ Returning zero
+
+
+ kindevent
+ location
+ 
+  line459
+  col37
+  file0
+ 
+ ranges
+ 
+   
+
+ line459
+ col37
+ file0
+
+
+

[clang-tools-extra] r356141 - [clang-tidy] Add additional patterns to the abseil-duration-unnecessary-conversion check.

[Hyrum Wright via cfe-commits]

Author: hwright
Date: Thu Mar 14 06:38:16 2019
New Revision: 356141

URL: http://llvm.org/viewvc/llvm-project?rev=356141&view=rev
Log:
[clang-tidy] Add additional patterns to the 
abseil-duration-unnecessary-conversion check.

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

Modified:

clang-tools-extra/trunk/clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp

clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst
clang-tools-extra/trunk/test/clang-tidy/Inputs/absl/time/time.h

clang-tools-extra/trunk/test/clang-tidy/abseil-duration-unnecessary-conversion.cpp

Modified: 
clang-tools-extra/trunk/clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp?rev=356141&r1=356140&r2=356141&view=diff
==
--- 
clang-tools-extra/trunk/clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp
 (original)
+++ 
clang-tools-extra/trunk/clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp
 Thu Mar 14 06:38:16 2019
@@ -28,12 +28,35 @@ void DurationUnnecessaryConversionCheck:
 std::string IntegerConversion =
 (llvm::Twine("::absl::ToInt64") + Scale).str();
 
+// Matcher which matches the current scale's factory with a `1` argument,
+// e.g. `absl::Seconds(1)`.
+auto factory_matcher = cxxConstructExpr(hasArgument(
+0,
+callExpr(callee(functionDecl(hasName(DurationFactory))),
+ hasArgument(0, 
ignoringImpCasts(integerLiteral(equals(1)));
+
+// Matcher which matches either inverse function and binds its argument,
+// e.g. `absl::ToDoubleSeconds(dur)`.
+auto inverse_function_matcher = callExpr(
+callee(functionDecl(hasAnyName(FloatConversion, IntegerConversion))),
+hasArgument(0, expr().bind("arg")));
+
+// Matcher which matches a duration divided by the factory_matcher above,
+// e.g. `dur / absl::Seconds(1)`.
+auto division_operator_matcher = cxxOperatorCallExpr(
+hasOverloadedOperatorName("/"), hasArgument(0, expr().bind("arg")),
+hasArgument(1, factory_matcher));
+
+// Matcher which matches a duration argument to `FDivDuration`,
+// e.g. `absl::FDivDuration(dur, absl::Seconds(1))`
+auto fdiv_matcher = callExpr(
+callee(functionDecl(hasName("::absl::FDivDuration"))),
+hasArgument(0, expr().bind("arg")), hasArgument(1, factory_matcher));
+
 Finder->addMatcher(
-callExpr(
-callee(functionDecl(hasName(DurationFactory))),
-hasArgument(0, callExpr(callee(functionDecl(hasAnyName(
-FloatConversion, IntegerConversion))),
-hasArgument(0, expr().bind("arg")
+callExpr(callee(functionDecl(hasName(DurationFactory))),
+ hasArgument(0, anyOf(inverse_function_matcher,
+  division_operator_matcher, 
fdiv_matcher)))
 .bind("call"),
 this);
   }
@@ -47,7 +70,8 @@ void DurationUnnecessaryConversionCheck:
   if (isInMacro(Result, OuterCall))
 return;
 
-  diag(OuterCall->getBeginLoc(), "remove unnecessary absl::Duration 
conversions")
+  diag(OuterCall->getBeginLoc(),
+   "remove unnecessary absl::Duration conversions")
   << FixItHint::CreateReplacement(
  OuterCall->getSourceRange(),
  tooling::fixit::getText(*Arg, *Result.Context));

Modified: 
clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst?rev=356141&r1=356140&r2=356141&view=diff
==
--- 
clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst
 (original)
+++ 
clang-tools-extra/trunk/docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst
 Thu Mar 14 06:38:16 2019
@@ -6,7 +6,7 @@ abseil-duration-unnecessary-conversion
 Finds and fixes cases where ``absl::Duration`` values are being converted to
 numeric types and back again.
 
-Examples:
+Floating-point examples:
 
 .. code-block:: c++
 
@@ -17,6 +17,15 @@ Examples:
   // Suggestion - Remove unnecessary conversions
   absl::Duration d2 = d1;
 
+  // Original - Division to convert to double and back again
+  absl::Duration d2 = absl::Seconds(absl::FDivDuration(d1, absl::Seconds(1)));
+
+  // Suggestion - Remove division and conversion
+  absl::Duration d2 = d1;
+
+Integer examples:
+
+.. code-block:: c++
 
   // Original - Conversion to integer and back again
   absl::Duration d1;
@@ -25,6 +34,12 @@ Examples:
   // Suggestion - Remove unnecessary conversions
   absl::Duration d2 = d1;
 
+  // Original - Integer division follow

[PATCH] D59183: [clang-tidy] Expand cases covered by the abseil-duration-unnecessary-conversion check

[Hyrum Wright via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE356141: [clang-tidy] Add additional patterns to the 
abseil-duration-unnecessary… (authored by hwright, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59183?vs=190456&id=190613#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59183

Files:
  clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp
  docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst
  test/clang-tidy/Inputs/absl/time/time.h
  test/clang-tidy/abseil-duration-unnecessary-conversion.cpp

Index: test/clang-tidy/Inputs/absl/time/time.h
===
--- test/clang-tidy/Inputs/absl/time/time.h
+++ test/clang-tidy/Inputs/absl/time/time.h
@@ -21,6 +21,7 @@
 template  Duration operator*(Duration lhs, T rhs);
 template  Duration operator*(T lhs, Duration rhs);
 template  Duration operator/(Duration lhs, T rhs);
+int64_t operator/(Duration lhs, Duration rhs);
 
 class Time{};
 
@@ -86,4 +87,6 @@
 inline Time operator-(Time lhs, Duration rhs);
 inline Duration operator-(Time lhs, Time rhs);
 
+double FDivDuration(Duration num, Duration den);
+
 }  // namespace absl
Index: test/clang-tidy/abseil-duration-unnecessary-conversion.cpp
===
--- test/clang-tidy/abseil-duration-unnecessary-conversion.cpp
+++ test/clang-tidy/abseil-duration-unnecessary-conversion.cpp
@@ -8,42 +8,80 @@
   // Floating point
   d2 = absl::Hours(absl::ToDoubleHours(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Minutes(absl::ToDoubleMinutes(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Seconds(absl::ToDoubleSeconds(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Milliseconds(absl::ToDoubleMilliseconds(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Microseconds(absl::ToDoubleMicroseconds(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Nanoseconds(absl::ToDoubleNanoseconds(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
 
   // Integer point
   d2 = absl::Hours(absl::ToInt64Hours(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Minutes(absl::ToInt64Minutes(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Seconds(absl::ToInt64Seconds(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Milliseconds(absl::ToInt64Milliseconds(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Microseconds(absl::ToInt64Microseconds(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
   d2 = absl::Nanoseconds(absl::ToInt64Nanoseconds(d1));
   // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
-  // CHECK-FIXES: d1
+  // CHECK-FIXES: d2 = d1
+
+  d2 = absl::Hours(d1 / absl::Hours(1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1
+  d2 = absl::Minutes(d1 / absl::Minutes(1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration conversions [abseil-duration-unnecessary-conversion]
+  // CHECK-FIXES: d2 = d1
+  d2 = absl::Seconds(d1 / absl::Seconds(1));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: remove unnecessary absl::Duration convers

[PATCH] D57893: [analyzer] Fix function macro crash

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rL356142: [analyzer] Fix function macro crash (authored by 
Szelethus, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57893?vs=190575&id=190614#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57893

Files:
  cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  
cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
  cfe/trunk/test/Analysis/plist-macros-with-expansion.cpp

Index: cfe/trunk/test/Analysis/plist-macros-with-expansion.cpp
===
--- cfe/trunk/test/Analysis/plist-macros-with-expansion.cpp
+++ cfe/trunk/test/Analysis/plist-macros-with-expansion.cpp
@@ -451,3 +451,21 @@
 1 / value; // expected-warning{{Division by zero}}
// expected-warning@-1{{expression result unused}}
 }
+
+#define FOO(x) int foo() { return x; }
+#define APPLY_ZERO1(function) function(0)
+
+APPLY_ZERO1(FOO)
+void useZeroApplier1() { (void)(1 / foo()); } // expected-warning{{Division by zero}}
+
+// CHECK: nameAPPLY_ZERO1
+// CHECK-NEXT: expansionint foo() { return x; }(0)
+
+#define BAR(x) int bar() { return x; }
+#define APPLY_ZERO2 BAR(0)
+
+APPLY_ZERO2
+void useZeroApplier2() { (void)(1 / bar()); } // expected-warning{{Division by zero}}
+
+// CHECK: nameAPPLY_ZERO2
+// CHECK-NEXT: expansionint bar() { return 0; }
Index: cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
===
--- cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
+++ cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
@@ -5577,6 +5577,484 @@

   
   
+  
+   path
+   
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line459
+   col33
+   file0
+  
+  
+   line459
+   col33
+   file0
+  
+ 
+end
+ 
+  
+   line459
+   col37
+   file0
+  
+  
+   line459
+   col39
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line459
+  col37
+  file0
+ 
+ ranges
+ 
+   
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col41
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Calling 'foo'
+ message
+ Calling 'foo'
+
+
+ kindevent
+ location
+ 
+  line458
+  col1
+  file0
+ 
+ depth1
+ extended_message
+ Entered call from 'useZeroApplier1'
+ message
+ Entered call from 'useZeroApplier1'
+
+
+ kindevent
+ location
+ 
+  line458
+  col1
+  file0
+ 
+ ranges
+ 
+   
+
+ line458
+ col1
+ file0
+
+
+ line458
+ col16
+ file0
+
+   
+ 
+ depth1
+ extended_message
+ Returning zero
+ message
+ Returning zero
+
+
+ kindevent
+ location
+ 
+  line459
+  col37
+  file0
+ 
+ ranges
+ 
+   
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col41
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Returning from 'foo'
+ message
+ Returning from 'foo'
+
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line459
+   col37
+   file0
+  
+  
+   line459
+   col39
+   file0
+  
+ 
+end
+ 
+  
+   line459
+   col35
+   file0
+  
+  
+   line459
+   col35
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line459
+  col35
+  file0
+ 
+ ranges
+ 
+   
+
+ line459
+ col33
+ file0
+
+
+ line459
+ col41
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Division by zero
+ message
+ Division by zero
+
+   
+   macro_expansions
+   
+
+ location
+ 
+  line458
+  col1
+  file0
+ 
+ nameAPPLY_ZERO1
+ expansionint foo() { return x; }(0)
+
+   
+   descriptionDivision by zero
+   categoryLogic error
+   typeDivision by zero
+   check_namecore.DivideZero
+   
+   issue_hash_content_of_line_in_context7

r356148 - Objective-C++11: Support static_assert() in @interface/@implementation ivar lists and method declarations

[Nico Weber via cfe-commits]

Author: nico
Date: Thu Mar 14 07:18:56 2019
New Revision: 356148

URL: http://llvm.org/viewvc/llvm-project?rev=356148&view=rev
Log:
Objective-C++11: Support static_assert() in @interface/@implementation ivar 
lists and method declarations

This adds support for static_assert() (and _Static_assert()) in
@interface/@implementation ivar lists and in @interface method declarations.

It was already supported in @implementation blocks outside of the ivar lists.

The assert AST nodes are added at file scope, matching where other
(non-Objective-C) declarations at @interface / @implementation level go (cf
`allTUVariables`).

Also add a `__has_feature(objc_c_static_assert)` that's true in C11 (and
`__has_extension(objc_c_static_assert)` that's always true) and
`__has_feature(objc_cxx_static_assert)` that's true in C++11 modea fter this
patch, so it's possible to check if this is supported.

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

Added:
cfe/trunk/test/Parser/objc-static-assert.m
cfe/trunk/test/Parser/objc-static-assert.mm
Modified:
cfe/trunk/include/clang/Basic/Features.def
cfe/trunk/lib/Parse/ParseDecl.cpp
cfe/trunk/lib/Parse/ParseObjc.cpp
cfe/trunk/lib/Sema/SemaExpr.cpp

Modified: cfe/trunk/include/clang/Basic/Features.def
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Features.def?rev=356148&r1=356147&r2=356148&view=diff
==
--- cfe/trunk/include/clang/Basic/Features.def (original)
+++ cfe/trunk/include/clang/Basic/Features.def Thu Mar 14 07:18:56 2019
@@ -116,6 +116,9 @@ FEATURE(objc_bridge_id_on_typedefs, true
 FEATURE(objc_generics, LangOpts.ObjC)
 FEATURE(objc_generics_variance, LangOpts.ObjC)
 FEATURE(objc_class_property, LangOpts.ObjC)
+FEATURE(objc_c_static_assert, LangOpts.C11)
+FEATURE(objc_cxx_static_assert, LangOpts.CPlusPlus11)
+EXTENSION(objc_c_static_assert, true)
 // C11 features
 FEATURE(c_alignas, LangOpts.C11)
 FEATURE(c_alignof, LangOpts.C11)

Modified: cfe/trunk/lib/Parse/ParseDecl.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseDecl.cpp?rev=356148&r1=356147&r2=356148&view=diff
==
--- cfe/trunk/lib/Parse/ParseDecl.cpp (original)
+++ cfe/trunk/lib/Parse/ParseDecl.cpp Thu Mar 14 07:18:56 2019
@@ -3889,6 +3889,9 @@ void Parser::ParseDeclarationSpecifiers(
 /// ParseStructDeclaration - Parse a struct declaration without the terminating
 /// semicolon.
 ///
+/// Note that a struct declaration refers to a declaration in a struct,
+/// not to the declaration of a struct.
+///
 ///   struct-declaration:
 /// [C2x]   attributes-specifier-seq[opt]
 ///   specifier-qualifier-list struct-declarator-list

Modified: cfe/trunk/lib/Parse/ParseObjc.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseObjc.cpp?rev=356148&r1=356147&r2=356148&view=diff
==
--- cfe/trunk/lib/Parse/ParseObjc.cpp (original)
+++ cfe/trunk/lib/Parse/ParseObjc.cpp Thu Mar 14 07:18:56 2019
@@ -623,6 +623,8 @@ void Parser::ParseObjCInterfaceDeclList(
 }
 // Ignore excess semicolons.
 if (Tok.is(tok::semi)) {
+  // FIXME: This should use ConsumeExtraSemi() for extraneous semicolons,
+  // to make -Wextra-semi diagnose them.
   ConsumeToken();
   continue;
 }
@@ -646,7 +648,19 @@ void Parser::ParseObjCInterfaceDeclList(
   // erroneous r_brace would cause an infinite loop if not handled here.
   if (Tok.is(tok::r_brace))
 break;
+
   ParsedAttributesWithRange attrs(AttrFactory);
+
+  // Since we call ParseDeclarationOrFunctionDefinition() instead of
+  // ParseExternalDeclaration() below (so that this doesn't parse nested
+  // @interfaces), this needs to duplicate some code from the latter.
+  if (Tok.isOneOf(tok::kw_static_assert, tok::kw__Static_assert)) {
+SourceLocation DeclEnd;
+allTUVariables.push_back(
+ParseDeclaration(DeclaratorContext::FileContext, DeclEnd, attrs));
+continue;
+  }
+
   allTUVariables.push_back(ParseDeclarationOrFunctionDefinition(attrs));
   continue;
 }
@@ -1875,6 +1889,7 @@ void Parser::HelperActionsForIvarDeclara
 /// ';'
 /// objc-instance-variable-decl-list objc-visibility-spec
 /// objc-instance-variable-decl-list objc-instance-variable-decl ';'
+/// objc-instance-variable-decl-list static_assert-declaration
 /// objc-instance-variable-decl-list ';'
 ///
 ///   objc-visibility-spec:
@@ -1945,6 +1960,15 @@ void Parser::ParseObjCClassInstanceVaria
   return cutOffParsing();
 }
 
+// This needs to duplicate a small amount of code from
+// ParseStructUnionBody() for things that should work in both
+// C struct and in Objective-C class instance variables.
+if (Tok.isOneOf(tok::kw_static_assert, tok::kw__Static_a

[PATCH] D59354: [clangd] Print arguments in template specializations

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 190617.
kadircet marked 3 inline comments as done.
kadircet added a comment.
Herald added a subscriber: jdoerfert.

- Address comments
- Add template specializations to fuzzyFind results


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59354

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/index/MemIndex.cpp
  clang-tools-extra/clangd/index/dex/Dex.cpp
  clang-tools-extra/unittests/clangd/DexTests.cpp
  clang-tools-extra/unittests/clangd/IndexTests.cpp
  clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
  clang/lib/AST/TypePrinter.cpp

Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1632,6 +1632,33 @@
   return A.getArgument();
 }
 
+static void printArgument(const TemplateArgument &A, const PrintingPolicy &PP,
+  llvm::raw_ostream &OS) {
+  A.print(PP, OS);
+}
+
+static void printArgument(const TemplateArgumentLoc &A,
+  const PrintingPolicy &PP, llvm::raw_ostream &OS) {
+  llvm::errs() << A.getArgument().getKind() << '\n';
+  switch (A.getArgument().getKind()) {
+  case TemplateArgument::ArgKind::Null:
+assert(false && "TemplateArgumentKind can not be null!");
+break;
+  case TemplateArgument::ArgKind::Type:
+A.getLocInfo().getAsTypeSourceInfo()->getType().print(OS, PP);
+break;
+  case TemplateArgument::ArgKind::Declaration:
+  case TemplateArgument::ArgKind::NullPtr:
+  case TemplateArgument::ArgKind::Integral:
+  case TemplateArgument::ArgKind::Template:
+  case TemplateArgument::ArgKind::TemplateExpansion:
+  case TemplateArgument::ArgKind::Expression:
+  case TemplateArgument::ArgKind::Pack:
+A.getArgument().print(PP, OS);
+break;
+  }
+}
+
 template
 static void printTo(raw_ostream &OS, ArrayRef Args,
 const PrintingPolicy &Policy, bool SkipBrackets) {
@@ -1653,7 +1680,7 @@
 } else {
   if (!FirstArg)
 OS << Comma;
-  Argument.print(Policy, ArgOS);
+  printArgument(Arg, Policy, ArgOS);
 }
 StringRef ArgString = ArgOS.str();
 
Index: clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
===
--- clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
+++ clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
@@ -394,23 +394,36 @@
   Annotations Header(R"(
 // Primary template and explicit specialization are indexed, instantiation
 // is not.
-template  struct [[Tmpl]] {T $xdecl[[x]] = 0;};
-template <> struct $specdecl[[Tmpl]] {};
-template  struct $partspecdecl[[Tmpl]] {};
-extern template struct Tmpl;
-template struct Tmpl;
+template  class $barclasstemp[[Bar]] {};
+template  class Z, int Q>
+struct [[Tmpl]] { T $xdecl[[x]] = 0; };
+template <> struct $specdecl[[Tmpl]] {};
+template  struct $partspecdecl[[Tmpl]] {};
+extern template struct Tmpl;
+template struct Tmpl;
+template  class $fooclasstemp[[Foo]] {};
+template<> class $templtempl[[Foo]], int, double> {};
   )");
   runSymbolCollector(Header.code(), /*Main=*/"");
-  EXPECT_THAT(Symbols,
-  UnorderedElementsAre(
-  AllOf(QName("Tmpl"), DeclRange(Header.range()),
-ForCodeCompletion(true)),
-  AllOf(QName("Tmpl"), DeclRange(Header.range("specdecl")),
-ForCodeCompletion(false)),
-  AllOf(QName("Tmpl"), DeclRange(Header.range("partspecdecl")),
-ForCodeCompletion(false)),
-  AllOf(QName("Tmpl::x"), DeclRange(Header.range("xdecl")),
-ForCodeCompletion(false;
+  EXPECT_THAT(
+  Symbols,
+  UnorderedElementsAre(
+  AllOf(QName("Tmpl"), DeclRange(Header.range()),
+ForCodeCompletion(true)),
+  AllOf(QName("Bar"), DeclRange(Header.range("barclasstemp")),
+ForCodeCompletion(true)),
+  AllOf(QName("Foo"), DeclRange(Header.range("fooclasstemp")),
+ForCodeCompletion(true)),
+  AllOf(QName("Tmpl"),
+DeclRange(Header.range("specdecl")), ForCodeCompletion(false)),
+  AllOf(QName("Tmpl"),
+DeclRange(Header.range("partspecdecl")),
+ForCodeCompletion(false)),
+  AllOf(QName("Foo, int, double>"),
+DeclRange(Header.range("templtempl")),
+ForCodeCompletion(false)),
+  AllOf(QName("Tmpl::x"), DeclRange(Header.range("xdecl")),
+ForCodeCompletion(false;
 }
 
 TEST_F(SymbolCollectorTest, ObjCSymbols) {
Index: clang-tools-extra/unittests/clangd/IndexTests.cpp
===
--- clang-tools-extra/un

[PATCH] D59364: [clangd] Add std subnamespace symbols to the symbol map.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: ioeric.
Herald added subscribers: jdoerfert, kadircet, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a reviewer: serge-sans-paille.
Herald added a project: clang.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D59364

Files:
  clangd/StdSymbolMap.inc
  clangd/include-mapping/gen_std.py

Index: clangd/include-mapping/gen_std.py
===
--- clangd/include-mapping/gen_std.py
+++ clangd/include-mapping/gen_std.py
@@ -90,59 +90,101 @@
   symbol_href["href"]))
   return symbols
 
+class Symbol:
 
-def ParseArg():
-  parser = argparse.ArgumentParser(description='Generate StdGen file')
-  parser.add_argument('-cppreference', metavar='PATH',
-  default='',
-  help='path to the cppreference offline HTML directory',
-  required=True
-  )
-  return parser.parse_args()
+  def __init__(self, name, namespace, headers):
+# unqualifed symbol name, e.g. "move"
+self.name = name
+# namespace of the symbol (with trailing "::"), e.g. "std::"
+self.namespace = namespace
+# a list of corresponding headers
+self.headers = headers
 
 
-def main():
-  args = ParseArg()
-  cpp_reference_root = args.cppreference
-  cpp_symbol_root = os.path.join(cpp_reference_root, "en", "cpp")
-  index_page_path = os.path.join(cpp_symbol_root, "symbol_index.html")
-  if not os.path.exists(index_page_path):
-exit("Path %s doesn't exist!" % index_page_path)
+def GetSymbols(root_dir, index_page_name, namespace):
+  """Get all symbols listed in the index page. All symbols should be in the
+  given namespace.
 
-  # We don't have version information from the unzipped offline HTML files.
-  # so we use the modified time of the symbol_index.html as the version.
-  cppreference_modified_date = datetime.datetime.fromtimestamp(
-os.stat(index_page_path).st_mtime).strftime('%Y-%m-%d')
+  Returns a list of Symbols.
+  """
 
   # Workflow steps:
   #   1. Parse index page which lists all symbols to get symbol
   #  name (unqualified name) and its href link to the symbol page which
   #  contains the defined header.
   #   2. Parse the symbol page to get the defined header.
-
-  # A map from symbol name to a set of headers.
-  symbols = {}
+  index_page_path = os.path.join(root_dir, index_page_name)
+  symbols = []
   with open(index_page_path, "r") as f:
+# A map from symbol name to a set of headers.
+symbol_headers = {}
 for symbol_name, symbol_page_path in ParseIndexPage(f.read()):
-  with open(os.path.join(cpp_symbol_root, symbol_page_path), "r") as f:
+  with open(os.path.join(root_dir, symbol_page_path), "r") as f:
 headers = ParseSymbolPage(f.read())
   if not headers:
 sys.stderr.write("No header found for symbol %s at %s\n" % (symbol_name,
   symbol_page_path))
 continue
 
-  if symbol_name not in symbols:
-symbols[symbol_name] = set()
-  symbols[symbol_name].update(headers)
+  if symbol_name not in symbol_headers:
+symbol_headers[symbol_name] = set()
+  symbol_headers[symbol_name].update(headers)
 
-# Emit results to stdout.
-print STDGEN_CODE_PREFIX % cppreference_modified_date
-for name, headers in sorted(symbols.items(), key=lambda t : t[0]):
-  if len(headers) > 1:
-# FIXME: support symbols with multiple headers (e.g. std::move).
-continue
-  # SYMBOL(unqualified_name, namespace, header)
-  print "SYMBOL(%s, %s, %s)" % (name, "std::", list(headers)[0])
+for name, headers in sorted(symbol_headers.items(), key=lambda t : t[0]):
+  symbols.append(Symbol(name, namespace, list(headers)))
+  return symbols
+
+
+def ParseArg():
+  parser = argparse.ArgumentParser(description='Generate StdGen file')
+  parser.add_argument('-cppreference', metavar='PATH',
+  default='',
+  help='path to the cppreference offline HTML directory',
+  required=True
+  )
+  return parser.parse_args()
+
+
+def main():
+  args = ParseArg()
+  cpp_root = os.path.join(args.cppreference, "en", "cpp")
+  symbol_index_root = os.path.join(cpp_root, "symbol_index")
+  if not os.path.exists(symbol_index_root):
+exit("Path %s doesn't exist!" % symbol_index_root)
+
+  parse_pages =  [
+(cpp_root, "symbol_index.html", "std::"),
+# std sub-namespace symbols have separated pages.
+# We don't index std literal operators (e.g.
+# std::literals::chrono_literals::operator""d), these symbols can't be
+# accessed by std::.
+# FIXME: index std::placeholders symbols, placeholders.html page is
+# different (which contains one entry for _1, _2, ..., _N), we need special
+# handling.
+(symbol_index_root, "chrono.html", "std::chrono::"),
+(symbol_index_root, "filesystem.htm

[PATCH] D59223: Objective-C++11: Support static_assert() in @interface/@implementation ivar lists and method declarations

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC356148: Objective-C++11: Support static_assert() in 
@interface/@implementation ivar… (authored by nico, committed by ).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D59223?vs=190534&id=190619#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D59223

Files:
  include/clang/Basic/Features.def
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseObjc.cpp
  lib/Sema/SemaExpr.cpp
  test/Parser/objc-static-assert.m
  test/Parser/objc-static-assert.mm

Index: include/clang/Basic/Features.def
===
--- include/clang/Basic/Features.def
+++ include/clang/Basic/Features.def
@@ -116,6 +116,9 @@
 FEATURE(objc_generics, LangOpts.ObjC)
 FEATURE(objc_generics_variance, LangOpts.ObjC)
 FEATURE(objc_class_property, LangOpts.ObjC)
+FEATURE(objc_c_static_assert, LangOpts.C11)
+FEATURE(objc_cxx_static_assert, LangOpts.CPlusPlus11)
+EXTENSION(objc_c_static_assert, true)
 // C11 features
 FEATURE(c_alignas, LangOpts.C11)
 FEATURE(c_alignof, LangOpts.C11)
Index: test/Parser/objc-static-assert.m
===
--- test/Parser/objc-static-assert.m
+++ test/Parser/objc-static-assert.m
@@ -0,0 +1,54 @@
+// RUN: %clang_cc1 -fobjc-runtime=macosx-fragile -fsyntax-only -verify -Wno-objc-root-class %s
+// RUN: %clang_cc1 -std=c89 -fobjc-runtime=macosx-fragile -fsyntax-only -verify -Wno-objc-root-class %s
+
+
+#if __STDC_VERSION__ >= 201112L
+
+#if !__has_feature(objc_c_static_assert)
+#error failed
+#endif
+
+#if !__has_extension(objc_c_static_assert)
+#error failed
+#endif
+
+@interface A {
+  int a;
+  _Static_assert(1, "");
+  _Static_assert(0, ""); // expected-error {{static_assert failed}}
+
+  _Static_assert(a, ""); // expected-error {{use of undeclared identifier 'a'}}
+  _Static_assert(sizeof(a), ""); // expected-error {{use of undeclared identifier 'a'}}
+}
+
+_Static_assert(1, "");
+
+@end
+
+struct S {
+  @defs(A);
+};
+
+#else
+
+// _Static_assert is available before C11 as an extension, but -pedantic
+// warns on it.
+#if __has_feature(objc_c_static_assert)
+#error failed
+#endif
+
+#if !__has_extension(objc_c_static_assert)
+#error failed
+#endif
+
+@interface A {
+  int a;
+  _Static_assert(1, "");
+  _Static_assert(0, ""); // expected-error {{static_assert failed}}
+}
+
+_Static_assert(1, "");
+
+@end
+
+#endif
Index: test/Parser/objc-static-assert.mm
===
--- test/Parser/objc-static-assert.mm
+++ test/Parser/objc-static-assert.mm
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
+// RUN: %clang_cc1 -std=c++98 -fsyntax-only -verify -Wno-objc-root-class %s
+
+#if !__has_feature(objc_c_static_assert)
+#error failed
+#endif
+
+#if __cplusplus >= 201103L
+
+#if !__has_feature(objc_cxx_static_assert)
+#error failed
+#endif
+
+// C++11
+
+@interface A {
+  int a;
+  static_assert(1, "");
+  _Static_assert(1, "");
+
+  static_assert(0, ""); // expected-error {{static_assert failed}}
+  _Static_assert(0, ""); // expected-error {{static_assert failed}}
+
+  static_assert(a, ""); // expected-error {{static_assert expression is not an integral constant expression}}
+  static_assert(sizeof(a) == 4, "");
+  static_assert(sizeof(a) == 3, ""); // expected-error {{static_assert failed}}
+}
+
+static_assert(1, "");
+_Static_assert(1, "");
+
+- (void)f;
+@end
+
+@implementation A {
+  int b;
+  static_assert(1, "");
+  _Static_assert(1, "");
+  static_assert(sizeof(b) == 4, "");
+  static_assert(sizeof(b) == 3, ""); // expected-error {{static_assert failed}}
+}
+
+static_assert(1, "");
+
+- (void)f {
+  static_assert(1, "");
+}
+@end
+
+@interface B
+@end
+
+@interface B () {
+  int b;
+  static_assert(sizeof(b) == 4, "");
+  static_assert(sizeof(b) == 3, ""); // expected-error {{static_assert failed}}
+}
+@end
+
+#else
+
+#if __has_feature(objc_cxx_static_assert)
+#error failed
+#endif
+
+// C++98
+@interface A {
+  int a;
+  static_assert(1, ""); // expected-error {{type name requires a specifier or qualifier}} expected-error{{expected parameter declarator}} expected-error {{expected ')'}} expected-note {{to match this '('}}
+  _Static_assert(1, "");
+  _Static_assert(0, ""); // expected-error {{static_assert failed}}
+}
+@end
+#endif
Index: lib/Parse/ParseObjc.cpp
===
--- lib/Parse/ParseObjc.cpp
+++ lib/Parse/ParseObjc.cpp
@@ -623,6 +623,8 @@
 }
 // Ignore excess semicolons.
 if (Tok.is(tok::semi)) {
+  // FIXME: This should use ConsumeExtraSemi() for extraneous semicolons,
+  // to make -Wextra-semi diagnose them.
   ConsumeToken();
   continue;
 }
@@ -646,7 +648,19 @@
   // erroneous r_brace would cause an infinite loop if not handled here.

r356151 - [ASTImporter] Fix import of NestedNameSpecifierLoc.

[Balazs Keri via cfe-commits]

Author: balazske
Date: Thu Mar 14 07:20:23 2019
New Revision: 356151

URL: http://llvm.org/viewvc/llvm-project?rev=356151&view=rev
Log:
[ASTImporter] Fix import of NestedNameSpecifierLoc.

Summary:
Import type location in case of TypeSpec and TypeSpecWithTemplate.
Without this fix the imported NespedNameSpecifierLoc will have an
invalid begin location.

Reviewers: a.sidorin, shafik, a_sidorin, martong

Reviewed By: a_sidorin

Subscribers: rnkovacs, jdoerfert, dkrupp, martong, Szelethus, gamesh411, 
cfe-commits

Tags: #clang

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

Modified:
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/unittests/AST/ASTImporterTest.cpp

Modified: cfe/trunk/lib/AST/ASTImporter.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTImporter.cpp?rev=356151&r1=356150&r2=356151&view=diff
==
--- cfe/trunk/lib/AST/ASTImporter.cpp (original)
+++ cfe/trunk/lib/AST/ASTImporter.cpp Thu Mar 14 07:20:23 2019
@@ -8092,8 +8092,11 @@ ASTImporter::Import_New(NestedNameSpecif
 
 case NestedNameSpecifier::TypeSpec:
 case NestedNameSpecifier::TypeSpecWithTemplate: {
+  SourceLocation ToTLoc;
+  if (Error Err = importInto(ToTLoc, NNS.getTypeLoc().getBeginLoc()))
+return std::move(Err);
   TypeSourceInfo *TSI = getToContext().getTrivialTypeSourceInfo(
-QualType(Spec->getAsType(), 0));
+QualType(Spec->getAsType(), 0), ToTLoc);
   Builder.Extend(getToContext(), ToLocalBeginLoc, TSI->getTypeLoc(),
  ToLocalEndLoc);
   break;

Modified: cfe/trunk/unittests/AST/ASTImporterTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/ASTImporterTest.cpp?rev=356151&r1=356150&r2=356151&view=diff
==
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp (original)
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp Thu Mar 14 07:20:23 2019
@@ -1232,6 +1232,26 @@ TEST_P(ImportExpr, DependentSizedArrayTy
   has(fieldDecl(hasType(dependentSizedArrayType(;
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportBeginLocOfDeclRefExpr) {
+  Decl *FromTU = getTuDecl(
+  "class A { public: static int X; }; void f() { (void)A::X; }", Lang_CXX);
+  auto From = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("f")));
+  ASSERT_TRUE(From);
+  ASSERT_TRUE(
+  cast(cast(From->getBody())->body_front())
+  ->getSubExpr()
+  ->getBeginLoc()
+  .isValid());
+  FunctionDecl *To = Import(From, Lang_CXX);
+  ASSERT_TRUE(To);
+  ASSERT_TRUE(
+  cast(cast(To->getBody())->body_front())
+  ->getSubExpr()
+  ->getBeginLoc()
+  .isValid());
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase,
ImportOfTemplatedDeclOfClassTemplateDecl) {
   Decl *FromTU = getTuDecl("template struct S{};", Lang_CXX);


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

r356152 - Remove unused variable to silence compiler warning [NFC]

[Mikael Holmen via cfe-commits]

Author: uabelho
Date: Thu Mar 14 07:20:50 2019
New Revision: 356152

URL: http://llvm.org/viewvc/llvm-project?rev=356152&view=rev
Log:
Remove unused variable to silence compiler warning [NFC]

The only use of MI was removed in r356142.

Modified:
cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp?rev=356152&r1=356151&r2=356152&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp Thu Mar 14 07:20:50 
2019
@@ -892,8 +892,7 @@ static std::string getMacroNameAndPrintE
 
 // If this token is a macro that should be expanded inside the current
 // macro.
-if (const MacroInfo *MI =
- getMacroInfoForLocation(PP, SM, II, T.getLocation())) 
{
+if (getMacroInfoForLocation(PP, SM, II, T.getLocation())) {
   getMacroNameAndPrintExpansion(Printer, T.getLocation(), PP, Info.Args,
 AlreadyProcessedTokens);
 


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

[PATCH] D55358: [ASTImporter] Fix import of NestedNameSpecifierLoc.

[Balázs Kéri via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL356151: [ASTImporter] Fix import of NestedNameSpecifierLoc. 
(authored by balazske, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55358

Files:
  cfe/trunk/lib/AST/ASTImporter.cpp
  cfe/trunk/unittests/AST/ASTImporterTest.cpp


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -8092,8 +8092,11 @@
 
 case NestedNameSpecifier::TypeSpec:
 case NestedNameSpecifier::TypeSpecWithTemplate: {
+  SourceLocation ToTLoc;
+  if (Error Err = importInto(ToTLoc, NNS.getTypeLoc().getBeginLoc()))
+return std::move(Err);
   TypeSourceInfo *TSI = getToContext().getTrivialTypeSourceInfo(
-QualType(Spec->getAsType(), 0));
+QualType(Spec->getAsType(), 0), ToTLoc);
   Builder.Extend(getToContext(), ToLocalBeginLoc, TSI->getTypeLoc(),
  ToLocalEndLoc);
   break;
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -1232,6 +1232,26 @@
   has(fieldDecl(hasType(dependentSizedArrayType(;
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportBeginLocOfDeclRefExpr) {
+  Decl *FromTU = getTuDecl(
+  "class A { public: static int X; }; void f() { (void)A::X; }", Lang_CXX);
+  auto From = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("f")));
+  ASSERT_TRUE(From);
+  ASSERT_TRUE(
+  cast(cast(From->getBody())->body_front())
+  ->getSubExpr()
+  ->getBeginLoc()
+  .isValid());
+  FunctionDecl *To = Import(From, Lang_CXX);
+  ASSERT_TRUE(To);
+  ASSERT_TRUE(
+  cast(cast(To->getBody())->body_front())
+  ->getSubExpr()
+  ->getBeginLoc()
+  .isValid());
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase,
ImportOfTemplatedDeclOfClassTemplateDecl) {
   Decl *FromTU = getTuDecl("template struct S{};", Lang_CXX);


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -8092,8 +8092,11 @@
 
 case NestedNameSpecifier::TypeSpec:
 case NestedNameSpecifier::TypeSpecWithTemplate: {
+  SourceLocation ToTLoc;
+  if (Error Err = importInto(ToTLoc, NNS.getTypeLoc().getBeginLoc()))
+return std::move(Err);
   TypeSourceInfo *TSI = getToContext().getTrivialTypeSourceInfo(
-QualType(Spec->getAsType(), 0));
+QualType(Spec->getAsType(), 0), ToTLoc);
   Builder.Extend(getToContext(), ToLocalBeginLoc, TSI->getTypeLoc(),
  ToLocalEndLoc);
   break;
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -1232,6 +1232,26 @@
   has(fieldDecl(hasType(dependentSizedArrayType(;
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportBeginLocOfDeclRefExpr) {
+  Decl *FromTU = getTuDecl(
+  "class A { public: static int X; }; void f() { (void)A::X; }", Lang_CXX);
+  auto From = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("f")));
+  ASSERT_TRUE(From);
+  ASSERT_TRUE(
+  cast(cast(From->getBody())->body_front())
+  ->getSubExpr()
+  ->getBeginLoc()
+  .isValid());
+  FunctionDecl *To = Import(From, Lang_CXX);
+  ASSERT_TRUE(To);
+  ASSERT_TRUE(
+  cast(cast(To->getBody())->body_front())
+  ->getSubExpr()
+  ->getBeginLoc()
+  .isValid());
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase,
ImportOfTemplatedDeclOfClassTemplateDecl) {
   Decl *FromTU = getTuDecl("template struct S{};", Lang_CXX);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59223: Objective-C++11: Support static_assert() in @interface/@implementation ivar lists and method declarations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/test/Parser/objc-static-assert.m:29
+struct S {
+  @defs(A);
+};

Given that static assertions are member declarations of a struct, should this 
"replay" the static asserts from the interface (including failing the 
assertions)?



Comment at: clang/test/Parser/objc-static-assert.mm:4
+
+#if !__has_feature(objc_c_static_assert)
+#error failed

Why do we expect `objc_c_static_assert` to be a supported feature in ObjC++? I 
would have expected this as an extension, but not a feature.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59223



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

[PATCH] D59354: [clangd] Print arguments in template specializations

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang/lib/AST/TypePrinter.cpp:1644
+llvm::raw_ostream &OS) {
+  A.getTypeSourceInfo()->getType().print(OS, PP);
+}

ilya-biryukov wrote:
> Maybe print the result of `getTypeLoc()` here, if it's available?
> Would produce results closer to the ones written in the code.
unfortunately it is not available.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59354



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

[PATCH] D57855: [analyzer] Reimplement checker options

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

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

> Let's put at least a FIXME here that the documentation for this option was 
> missing.

I'd prefer to just simply fix this. @NoQ, could you help us fill in the gaps? 
We need a desctiption for

- `nullability:NoDiagnoseCallsToSystemHeaders`
- `osx.cocoa.RetainCount:CheckOSObject`
- `osx.cocoa.RetainCount:leak-diagnostics-reference-allocation`
- `osx.cocoa.RetainCount:TrackNSCFStartParam`




Comment at: lib/Frontend/CompilerInvocation.cpp:425
   bool HasFailed = getStringOption(Config, Name, std::to_string(DefaultVal))
- .getAsInteger(10, OptionField);
+ .getAsInteger(0, OptionField);
   if (Diags && HasFailed)

whisperity wrote:
> What is this trying to do?
Oh right, this is why (mind you, we had a checker option that was hexadecimal, 
but apparently never used, as it caused an assertation failure):

```
template
std::enable_if::is_signed, bool>::type
llvm::StringRef::getAsInteger(unsigned Radix, T &Result) const
```
Parse the current string as an integer of the specified radix.

If Radix is specified as zero, this does radix autosensing using extended C 
rules: 0 is octal, 0x is hex, 0b is binary.

If the string is invalid or if only a subset of the string is valid, this 
returns true to signify the error. The string is considered erroneous if empty 
or if it overflows T.


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

https://reviews.llvm.org/D57855



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

[PATCH] D59354: [clangd] Print arguments in template specializations

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

- Get rid of debug printing
- Update tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59354

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/index/MemIndex.cpp
  clang-tools-extra/clangd/index/dex/Dex.cpp
  clang-tools-extra/unittests/clangd/DexTests.cpp
  clang-tools-extra/unittests/clangd/IndexTests.cpp
  clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
  clang/lib/AST/TypePrinter.cpp

Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1632,6 +1632,32 @@
   return A.getArgument();
 }
 
+static void printArgument(const TemplateArgument &A, const PrintingPolicy &PP,
+  llvm::raw_ostream &OS) {
+  A.print(PP, OS);
+}
+
+static void printArgument(const TemplateArgumentLoc &A,
+  const PrintingPolicy &PP, llvm::raw_ostream &OS) {
+  switch (A.getArgument().getKind()) {
+  case TemplateArgument::ArgKind::Null:
+assert(false && "TemplateArgumentKind can not be null!");
+break;
+  case TemplateArgument::ArgKind::Type:
+A.getTypeSourceInfo()->getType().print(OS, PP);
+break;
+  case TemplateArgument::ArgKind::Declaration:
+  case TemplateArgument::ArgKind::NullPtr:
+  case TemplateArgument::ArgKind::Integral:
+  case TemplateArgument::ArgKind::Template:
+  case TemplateArgument::ArgKind::TemplateExpansion:
+  case TemplateArgument::ArgKind::Expression:
+  case TemplateArgument::ArgKind::Pack:
+A.getArgument().print(PP, OS);
+break;
+  }
+}
+
 template
 static void printTo(raw_ostream &OS, ArrayRef Args,
 const PrintingPolicy &Policy, bool SkipBrackets) {
@@ -1653,7 +1679,7 @@
 } else {
   if (!FirstArg)
 OS << Comma;
-  Argument.print(Policy, ArgOS);
+  printArgument(Arg, Policy, ArgOS);
 }
 StringRef ArgString = ArgOS.str();
 
Index: clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
===
--- clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
+++ clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
@@ -394,23 +394,36 @@
   Annotations Header(R"(
 // Primary template and explicit specialization are indexed, instantiation
 // is not.
-template  struct [[Tmpl]] {T $xdecl[[x]] = 0;};
-template <> struct $specdecl[[Tmpl]] {};
-template  struct $partspecdecl[[Tmpl]] {};
-extern template struct Tmpl;
-template struct Tmpl;
+template  class $barclasstemp[[Bar]] {};
+template  class Z, int Q>
+struct [[Tmpl]] { T $xdecl[[x]] = 0; };
+template <> struct $specdecl[[Tmpl]] {};
+template  struct $partspecdecl[[Tmpl]] {};
+extern template struct Tmpl;
+template struct Tmpl;
+template  class $fooclasstemp[[Foo]] {};
+template<> class $templtempl[[Foo]], int, double> {};
   )");
   runSymbolCollector(Header.code(), /*Main=*/"");
-  EXPECT_THAT(Symbols,
-  UnorderedElementsAre(
-  AllOf(QName("Tmpl"), DeclRange(Header.range()),
-ForCodeCompletion(true)),
-  AllOf(QName("Tmpl"), DeclRange(Header.range("specdecl")),
-ForCodeCompletion(false)),
-  AllOf(QName("Tmpl"), DeclRange(Header.range("partspecdecl")),
-ForCodeCompletion(false)),
-  AllOf(QName("Tmpl::x"), DeclRange(Header.range("xdecl")),
-ForCodeCompletion(false;
+  EXPECT_THAT(
+  Symbols,
+  UnorderedElementsAre(
+  AllOf(QName("Tmpl"), DeclRange(Header.range()),
+ForCodeCompletion(true)),
+  AllOf(QName("Bar"), DeclRange(Header.range("barclasstemp")),
+ForCodeCompletion(true)),
+  AllOf(QName("Foo"), DeclRange(Header.range("fooclasstemp")),
+ForCodeCompletion(true)),
+  AllOf(QName("Tmpl"),
+DeclRange(Header.range("specdecl")), ForCodeCompletion(false)),
+  AllOf(QName("Tmpl"),
+DeclRange(Header.range("partspecdecl")),
+ForCodeCompletion(false)),
+  AllOf(QName("Foo, int, double>"),
+DeclRange(Header.range("templtempl")),
+ForCodeCompletion(false)),
+  AllOf(QName("Tmpl::x"), DeclRange(Header.range("xdecl")),
+ForCodeCompletion(false;
 }
 
 TEST_F(SymbolCollectorTest, ObjCSymbols) {
Index: clang-tools-extra/unittests/clangd/IndexTests.cpp
===
--- clang-tools-extra/unittests/clangd/IndexTests.cpp
+++ clang-tools-extra/unittests/clangd/IndexTests.cpp
@@ -15,6 +15,7 @@
 #include "index/Merge.h"
 #include "index/Symbol.h"
 #include "clang/Index/

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

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

Szelethus updated this revision to Diff 190625.
Szelethus edited the summary of this revision.
Szelethus added a comment.
Herald added subscribers: Charusso, jdoerfert.

Also, don't forget package options :^)


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

https://reviews.llvm.org/D57858

Files:
  include/clang/Driver/CC1Options.td
  include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  include/clang/StaticAnalyzer/Frontend/FrontendActions.h
  lib/Frontend/CompilerInvocation.cpp
  lib/FrontendTool/ExecuteCompilerInvocation.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/analyzer-checker-option-help.c

Index: test/Analysis/analyzer-checker-option-help.c
===
--- /dev/null
+++ test/Analysis/analyzer-checker-option-help.c
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -analyzer-checker-option-help 2>&1 | FileCheck %s
+
+// CHECK: OVERVIEW: Clang Static Analyzer Checker and Package Option List
+//
+// CHECK: USAGE: clang -cc1 [CLANG_OPTIONS] -analyzer-config 
+//
+// CHECK:clang -cc1 [CLANG_OPTIONS] -analyzer-config OPTION1=VALUE,
+// CHECK-SAME: -analyzer-config OPTION2=VALUE, ...
+//
+// CHECK:clang [CLANG_OPTIONS] -Xclang -analyzer-config
+// CHECK-SAME: -Xclang
+//
+// CHECK:clang [CLANG_OPTIONS] -Xclang -analyzer-config
+// CHECK-SAME: -Xclang OPTION1=VALUE, -Xclang -analyzer-config
+// CHECK-SAME: -Xclang OPTION2=VALUE, ...
+//
+// CHECK: OPTIONS:
+//
+// CHECK:   alpha.clone.CloneChecker:MinimumCloneComplexity  
+// CHECK-SAME:   (int) Ensures that every clone has at least
+// CHECK:the given complexity. Complexity is here
+// CHECK:defined as the total amount of children
+// CHECK:of a statement. This constraint assumes
+// CHECK:the first statement in the group is representative
+// CHECK:for all other statements in the group in
+// CHECK:terms of complexity. (default: 50)
Index: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -515,3 +515,75 @@
   for (const auto *i : enabledCheckers)
 Out << i->FullName << '\n';
 }
+
+// Some global constants to help with printing.
+constexpr size_t MinLineWidth = 90;
+constexpr size_t PadForOpt = 2;
+constexpr size_t OptionWidth = 50;
+constexpr size_t PadForDesc = PadForOpt + OptionWidth;
+static_assert(MinLineWidth > PadForDesc, "MinLineWidth must be greater!");
+
+static void printCmdLineOption(llvm::formatted_raw_ostream &FOut,
+   StringRef CheckerOrPackageFullName,
+   const CheckerRegistry::CmdLineOption &Option) {
+  FOut.PadToColumn(PadForOpt) << CheckerOrPackageFullName << ':'
+  << Option.OptionName;
+
+  // If the buffer's length is greater then PadForDesc, print a newline.
+  if (FOut.getColumn() > PadForDesc)
+FOut << '\n';
+
+  FOut.PadToColumn(PadForDesc) << "(" << Option.OptionType << ") ";
+
+  for (char C : Option.Description) {
+if (FOut.getColumn() > MinLineWidth && C == ' ') {
+  FOut << '\n';
+  FOut.PadToColumn(PadForDesc);
+  continue;
+}
+FOut << C;
+  }
+
+  if (!Option.Description.empty())
+FOut << ' ';
+  if (FOut.getColumn() > MinLineWidth) {
+FOut << '\n';
+FOut.PadToColumn(PadForDesc);
+  }
+  FOut << "(default: "
+   << (Option.DefaultValStr.empty() ? "\"\"" : Option.DefaultValStr)
+   << ")\n\n";
+}
+
+void CheckerRegistry::printCheckerOptionList(raw_ostream &Out) const {
+  Out << "OVERVIEW: Clang Static Analyzer Checker and Package Option List\n\n";
+  Out << "USAGE: clang -cc1 [CLANG_OPTIONS] -analyzer-config "
+"\n\n";
+  Out << "   clang -cc1 [CLANG_OPTIONS] -analyzer-config OPTION1=VALUE, "
+  "-analyzer-config OPTION2=VALUE, ...\n\n";
+  Out << "   clang [CLANG_OPTIONS] -Xclang -analyzer-config -Xclang"
+"\n\n";
+  Out << "   clang [CLANG_OPTIONS] -Xclang -analyzer-config -Xclang "
+  "OPTION1=VALUE, -Xclang -analyzer-config -Xclang "
+  "OPTION2=VALUE, ...\n\n";
+  Out << "OPTIONS:\n\n";
+
+  llvm::formatted_raw_ostream FOut(Out);
+
+  std::multimap OptionMap;
+
+  for (const CheckerInfo &Checker : Checkers) {
+for (const CmdLineOption &Option : Checker.CmdLineOptions) {
+  OptionMap.insert({Checker.FullName, Option});
+}
+  }
+
+  for (const PackageInfo &Package : Packages) {
+for (const CmdLineOption &Option : Package.CmdLineOptions) {
+ 

r356154 - Fix test after r356148

[Nico Weber via cfe-commits]

Author: nico
Date: Thu Mar 14 07:40:48 2019
New Revision: 356154

URL: http://llvm.org/viewvc/llvm-project?rev=356154&view=rev
Log:
Fix test after r356148

Modified:
cfe/trunk/test/Parser/objc-static-assert.mm

Modified: cfe/trunk/test/Parser/objc-static-assert.mm
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/objc-static-assert.mm?rev=356154&r1=356153&r2=356154&view=diff
==
--- cfe/trunk/test/Parser/objc-static-assert.mm (original)
+++ cfe/trunk/test/Parser/objc-static-assert.mm Thu Mar 14 07:40:48 2019
@@ -1,7 +1,10 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
 // RUN: %clang_cc1 -std=c++98 -fsyntax-only -verify -Wno-objc-root-class %s
 
-#if !__has_feature(objc_c_static_assert)
+#if __has_feature(objc_c_static_assert)
+#error failed
+#endif
+#if !__has_extension(objc_c_static_assert)
 #error failed
 #endif
 


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

[PATCH] D59223: Objective-C++11: Support static_assert() in @interface/@implementation ivar lists and method declarations

[Nico Weber via Phabricator via cfe-commits]

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



Comment at: clang/test/Parser/objc-static-assert.m:29
+struct S {
+  @defs(A);
+};

aaron.ballman wrote:
> Given that static assertions are member declarations of a struct, should this 
> "replay" the static asserts from the interface (including failing the 
> assertions)?
This only works in the old fragile abi and is unsupported in "modern" (8 year 
old) objc, and it was used very rarely even back then. I don't think we need to 
do anything about this other than not crashing.



Comment at: clang/test/Parser/objc-static-assert.mm:4
+
+#if !__has_feature(objc_c_static_assert)
+#error failed

aaron.ballman wrote:
> Why do we expect `objc_c_static_assert` to be a supported feature in ObjC++? 
> I would have expected this as an extension, but not a feature.
Thanks for catching this, looks I forgot to re-run the .mm test after changing 
Features.def :-( Fixed now in r356154.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59223



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

[PATCH] D57922: [analyzer] Insert checker options into AnalyzerOption::ConfigTable

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

Szelethus updated this revision to Diff 190628.

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

https://reviews.llvm.org/D57922

Files:
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/analyzer-config.c

Index: test/Analysis/analyzer-config.c
===
--- test/Analysis/analyzer-config.c
+++ test/Analysis/analyzer-config.c
@@ -3,6 +3,16 @@
 
 // CHECK: [config]
 // CHECK-NEXT: aggressive-binary-operation-simplification = false
+// CHECK-NEXT: alpha.clone.CloneChecker:IgnoredFilesPattern = ""
+// CHECK-NEXT: alpha.clone.CloneChecker:MinimumCloneComplexity = 50
+// CHECK-NEXT: alpha.clone.CloneChecker:ReportNormalClones = true
+// CHECK-NEXT: alpha.cplusplus.UninitializedObject:CheckPointeeInitialization = false
+// CHECK-NEXT: alpha.cplusplus.UninitializedObject:IgnoreGuardedFields = false
+// CHECK-NEXT: alpha.cplusplus.UninitializedObject:IgnoreRecordsWithField = ""
+// CHECK-NEXT: alpha.cplusplus.UninitializedObject:NotesAsWarnings = false
+// CHECK-NEXT: alpha.cplusplus.UninitializedObject:Pedantic = false
+// CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtExec = 0x04
+// CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtRead = 0x01
 // CHECK-NEXT: avoid-suppressing-null-argument-paths = false
 // CHECK-NEXT: c++-allocator-inlining = true
 // CHECK-NEXT: c++-container-inlining = false
@@ -18,9 +28,26 @@
 // CHECK-NEXT: cfg-rich-constructors = true
 // CHECK-NEXT: cfg-scopes = false
 // CHECK-NEXT: cfg-temporary-dtors = true
+// CHECK-NEXT: cplusplus.Move:WarnOn = KnownsAndLocals
 // CHECK-NEXT: crosscheck-with-z3 = false
 // CHECK-NEXT: ctu-dir = ""
 // CHECK-NEXT: ctu-index-name = externalDefMap.txt
+// CHECK-NEXT: debug.AnalysisOrder:* = false
+// CHECK-NEXT: debug.AnalysisOrder:Bind = false
+// CHECK-NEXT: debug.AnalysisOrder:EndFunction = false
+// CHECK-NEXT: debug.AnalysisOrder:LiveSymbols = false
+// CHECK-NEXT: debug.AnalysisOrder:NewAllocator = false
+// CHECK-NEXT: debug.AnalysisOrder:PostCall = false
+// CHECK-NEXT: debug.AnalysisOrder:PostStmtArraySubscriptExpr = false
+// CHECK-NEXT: debug.AnalysisOrder:PostStmtCXXNewExpr = false
+// CHECK-NEXT: debug.AnalysisOrder:PostStmtCastExpr = false
+// CHECK-NEXT: debug.AnalysisOrder:PostStmtOffsetOfExpr = false
+// CHECK-NEXT: debug.AnalysisOrder:PreCall = false
+// CHECK-NEXT: debug.AnalysisOrder:PreStmtArraySubscriptExpr = false
+// CHECK-NEXT: debug.AnalysisOrder:PreStmtCXXNewExpr = false
+// CHECK-NEXT: debug.AnalysisOrder:PreStmtCastExpr = false
+// CHECK-NEXT: debug.AnalysisOrder:PreStmtOffsetOfExpr = false
+// CHECK-NEXT: debug.AnalysisOrder:RegionChanges = false
 // CHECK-NEXT: display-ctu-progress = false
 // CHECK-NEXT: eagerly-assume = true
 // CHECK-NEXT: elide-constructors = true
@@ -40,7 +67,15 @@
 // CHECK-NEXT: mode = deep
 // CHECK-NEXT: model-path = ""
 // CHECK-NEXT: notes-as-events = false
+// CHECK-NEXT: nullability:NoDiagnoseCallsToSystemHeaders = false
 // CHECK-NEXT: objc-inlining = true
+// CHECK-NEXT: optin.cplusplus.VirtualCall:PureOnly = false
+// CHECK-NEXT: optin.osx.cocoa.localizability.NonLocalizedStringChecker:AggressiveReport = false
+// CHECK-NEXT: optin.performance.Padding:AllowedPad = 24
+// CHECK-NEXT: osx.NumberObjectConversion:Pedantic = false
+// CHECK-NEXT: osx.cocoa.RetainCount:CheckOSObject = true
+// CHECK-NEXT: osx.cocoa.RetainCount:TrackNSCFStartParam = false
+// CHECK-NEXT: osx.cocoa.RetainCount:leak-diagnostics-reference-allocation = false
 // CHECK-NEXT: prune-paths = true
 // CHECK-NEXT: region-store-small-struct-limit = 2
 // CHECK-NEXT: report-in-main-source-file = false
@@ -49,7 +84,8 @@
 // CHECK-NEXT: suppress-c++-stdlib = true
 // CHECK-NEXT: suppress-inlined-defensive-checks = true
 // CHECK-NEXT: suppress-null-return-paths = true
+// CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 49
+// CHECK-NEXT: num-entries = 85
Index: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -316,18 +316,20 @@
 }
 
 void CheckerRegistry::addCheckerOption(StringRef OptionType,
-   StringRef CheckerFullName,
+   StringRef FullName,
StringRef OptionName,
StringRef DefaultValStr,
StringRef Description) {
 
-  auto CheckerIt = binaryFind(Checkers, CheckerFullName);
+  auto CheckerIt = binaryFind(Checkers, FullName);
   assert(CheckerIt != Checkers.end() &&
  "Failed to find the checker while attempting to add a command 

[PATCH] D57860: [analyzer] Validate checker option names and values

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

Szelethus updated this revision to Diff 190629.
Szelethus marked 6 inline comments as done.
Szelethus added a comment.

Fixes according to inline comments.


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

https://reviews.llvm.org/D57860

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/invalid-checker-option.c

Index: test/Analysis/invalid-checker-option.c
===
--- test/Analysis/invalid-checker-option.c
+++ test/Analysis/invalid-checker-option.c
@@ -14,6 +14,53 @@
 // CHECK-NON-EXISTENT-CHECKER: (frontend): no analyzer checkers or packages
 // CHECK-NON-EXISTENT-CHECKER-SAME: are associated with 'RetainOneTwoThree'
 
+
+// Every other error should be avoidable in compatiblity mode.
+
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config debug.AnalysisOrder:Everything=false \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-NON-EXISTENT-CHECKER-OPTION
+
+// CHECK-NON-EXISTENT-CHECKER-OPTION: (frontend): checker 'debug.AnalysisOrder'
+// CHECK-NON-EXISTENT-CHECKER-OPTION-SAME: has no option called 'Everything'
+
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config debug.AnalysisOrder:Everything=false
+
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config debug.AnalysisOrder:*=nothankyou \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-INVALID-BOOL-VALUE
+
+// CHECK-INVALID-BOOL-VALUE: (frontend): invalid input for checker option
+// CHECK-INVALID-BOOL-VALUE-SAME: 'debug.AnalysisOrder:*', that expects a
+// CHECK-INVALID-BOOL-VALUE-SAME: boolean value
+
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config debug.AnalysisOrder:*=nothankyou
+
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config optin.performance.Padding:AllowedPad=surpriseme \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-INVALID-INT-VALUE
+
+// CHECK-INVALID-INT-VALUE: (frontend): invalid input for checker option
+// CHECK-INVALID-INT-VALUE-SAME: 'optin.performance.Padding:AllowedPad', that
+// CHECK-INVALID-INT-VALUE-SAME: expects an integer value
+
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config optin.performance.Padding:AllowedPad=surpriseme
+
 // expected-no-diagnostics
 
 int main() {}
Index: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -315,6 +315,50 @@
   CheckerIt->Dependencies.emplace_back(&*DependencyIt);
 }
 
+/// Insert the checker/package option to AnalyzerOptions' config table, and
+/// validate it, if the user supplied it on the command line.
+static void insertAndValidate(StringRef OptionType, StringRef FullName,
+  StringRef OptionName, StringRef DefaultValue,
+  AnalyzerOptions &AnOpts,
+  DiagnosticsEngine &Diags) {
+
+  std::string FullOption = (FullName + ":" + OptionName).str();
+
+  auto It = AnOpts.Config.insert({FullOption, DefaultValue});
+
+  // Insertation was successful -- CmdLineOption's constructor will validate
+  // whether values received from plugins or TableGen files are correct.
+  if (It.second)
+return;
+
+  // Insertion failed, the user supplied this package/checker option on the
+  // command line. If the supplied value is invalid, we'll emit an error.
+
+  StringRef SuppliedValue = It.first->getValue();
+
+  if (OptionType == "bool") {
+if (SuppliedValue != "true" && SuppliedValue != "false") {
+  if (AnOpts.ShouldEmitErrorsOnInvalidConfigValue) {
+Diags.Report(diag::err_analyzer_checker_option_invalid_input)
+<< FullOption << "a boolean value";
+  }
+}
+return;
+  }
+
+  if (OptionType == "int") {
+int Tmp;
+bool HasFailed = SuppliedValue.getAsInteger(0, Tmp);
+if (HasFailed) {
+  if (AnOpts.ShouldEmitErrorsOnInvalidConfigValue) {
+Diags.Report(diag::err_analyzer_checker_option_invalid_input)
+<< FullOption << "an integer value";
+  }
+}
+return;
+  }
+}
+
 void CheckerRegistry::addCheckerOption(StringRef OptionType,
StringRef FullName,
StringRef OptionName,
@@ -329,7 +373,8 @@
   CheckerIt->CmdLineOptions.emplace_back(
   OptionType, OptionName, DefaultValStr, Descrip

[PATCH] D57860: [analyzer] Validate checker option names and values

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

Szelethus updated this revision to Diff 190630.
Szelethus added a comment.

Add a testcase for packages.


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

https://reviews.llvm.org/D57860

Files:
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h
  lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
  test/Analysis/invalid-checker-option.c

Index: test/Analysis/invalid-checker-option.c
===
--- test/Analysis/invalid-checker-option.c
+++ test/Analysis/invalid-checker-option.c
@@ -14,6 +14,63 @@
 // CHECK-NON-EXISTENT-CHECKER: (frontend): no analyzer checkers or packages
 // CHECK-NON-EXISTENT-CHECKER-SAME: are associated with 'RetainOneTwoThree'
 
+
+// Every other error should be avoidable in compatiblity mode.
+
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config debug.AnalysisOrder:Everything=false \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-NON-EXISTENT-CHECKER-OPTION
+
+// CHECK-NON-EXISTENT-CHECKER-OPTION: (frontend): checker 'debug.AnalysisOrder'
+// CHECK-NON-EXISTENT-CHECKER-OPTION-SAME: has no option called 'Everything'
+
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config debug.AnalysisOrder:Everything=false
+
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config debug.AnalysisOrder:*=nothankyou \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-INVALID-BOOL-VALUE
+
+// CHECK-INVALID-BOOL-VALUE: (frontend): invalid input for checker option
+// CHECK-INVALID-BOOL-VALUE-SAME: 'debug.AnalysisOrder:*', that expects a
+// CHECK-INVALID-BOOL-VALUE-SAME: boolean value
+
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config debug.AnalysisOrder:*=nothankyou
+
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config optin.performance.Padding:AllowedPad=surpriseme \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-INVALID-INT-VALUE
+
+// CHECK-INVALID-INT-VALUE: (frontend): invalid input for checker option
+// CHECK-INVALID-INT-VALUE-SAME: 'optin.performance.Padding:AllowedPad', that
+// CHECK-INVALID-INT-VALUE-SAME: expects an integer value
+
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config optin.performance.Padding:AllowedPad=surpriseme
+
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config nullability:NoDiagnoseCallsToSystemHeaders=sure \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-PACKAGE-VALUE
+
+// CHECK-PACKAGE-VALUE: (frontend): invalid input for checker option
+// CHECK-PACKAGE-VALUE-SAME: 'nullability:NoDiagnoseCallsToSystemHeaders', that
+// CHECK-PACKAGE-VALUE-SAME: expects a boolean value
+
 // expected-no-diagnostics
 
 int main() {}
Index: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -315,6 +315,50 @@
   CheckerIt->Dependencies.emplace_back(&*DependencyIt);
 }
 
+/// Insert the checker/package option to AnalyzerOptions' config table, and
+/// validate it, if the user supplied it on the command line.
+static void insertAndValidate(StringRef OptionType, StringRef FullName,
+  StringRef OptionName, StringRef DefaultValue,
+  AnalyzerOptions &AnOpts,
+  DiagnosticsEngine &Diags) {
+
+  std::string FullOption = (FullName + ":" + OptionName).str();
+
+  auto It = AnOpts.Config.insert({FullOption, DefaultValue});
+
+  // Insertation was successful -- CmdLineOption's constructor will validate
+  // whether values received from plugins or TableGen files are correct.
+  if (It.second)
+return;
+
+  // Insertion failed, the user supplied this package/checker option on the
+  // command line. If the supplied value is invalid, we'll emit an error.
+
+  StringRef SuppliedValue = It.first->getValue();
+
+  if (OptionType == "bool") {
+if (SuppliedValue != "true" && SuppliedValue != "false") {
+  if (AnOpts.ShouldEmitErrorsOnInvalidConfigValue) {
+Diags.Report(diag::err_analyzer_checker_option_invalid_input)
+<< FullOption << "a boolean value";
+  }
+}
+return;
+  }
+
+  if (OptionType == "int") {
+int Tmp;
+bool HasFailed = SuppliedValue.getAsInteger(0, Tmp);
+if (HasFailed) {
+  if (AnOpts.ShouldEmitErrorsOnInvalidConfigValue) {
+Diags.Report(diag::err_analyzer_checker_option_invalid_input)
+

[PATCH] D59223: Objective-C++11: Support static_assert() in @interface/@implementation ivar lists and method declarations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/test/Parser/objc-static-assert.m:29
+struct S {
+  @defs(A);
+};

thakis wrote:
> aaron.ballman wrote:
> > Given that static assertions are member declarations of a struct, should 
> > this "replay" the static asserts from the interface (including failing the 
> > assertions)?
> This only works in the old fragile abi and is unsupported in "modern" (8 year 
> old) objc, and it was used very rarely even back then. I don't think we need 
> to do anything about this other than not crashing.
Fine by me!



Comment at: clang/test/Parser/objc-static-assert.mm:4
+
+#if !__has_feature(objc_c_static_assert)
+#error failed

thakis wrote:
> aaron.ballman wrote:
> > Why do we expect `objc_c_static_assert` to be a supported feature in 
> > ObjC++? I would have expected this as an extension, but not a feature.
> Thanks for catching this, looks I forgot to re-run the .mm test after 
> changing Features.def :-( Fixed now in r356154.
Great, thank you!

Other than those points, this LGTM as well. :-)


Repository:
  rC Clang

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

https://reviews.llvm.org/D59223



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

r356156 - [CodeGen][ObjC] Remove the leading 'l' from symbols for protocol

[Akira Hatanaka via cfe-commits]

Author: ahatanak
Date: Thu Mar 14 08:17:37 2019
New Revision: 356156

URL: http://llvm.org/viewvc/llvm-project?rev=356156&view=rev
Log:
[CodeGen][ObjC] Remove the leading 'l' from symbols for protocol
metadata and protocol list

The leading 'l' tells ld64 to remove the symbol name, which can make
debugging difficult.

rdar://problem/47256637

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

Modified:
cfe/trunk/lib/CodeGen/CGObjCMac.cpp
cfe/trunk/test/CodeGenObjC/forward-protocol-metadata-symbols.m
cfe/trunk/test/CodeGenObjC/hidden-visibility.m
cfe/trunk/test/CodeGenObjC/metadata-class-properties.m
cfe/trunk/test/CodeGenObjC/metadata-symbols-64.m
cfe/trunk/test/CodeGenObjC/protocol-comdat.m
cfe/trunk/test/CodeGenObjC/protocol-in-extended-class.m
cfe/trunk/test/CodeGenObjC/protocols.m
cfe/trunk/test/CodeGenObjC/sections.m
cfe/trunk/test/CodeGenObjC/undefined-protocol2.m

Modified: cfe/trunk/lib/CodeGen/CGObjCMac.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGObjCMac.cpp?rev=356156&r1=356155&r2=356156&view=diff
==
--- cfe/trunk/lib/CodeGen/CGObjCMac.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGObjCMac.cpp Thu Mar 14 08:17:37 2019
@@ -6809,7 +6809,7 @@ llvm::Constant *CGObjCNonFragileABIMac::
 // reference or not. At module finalization we add the empty
 // contents for protocols which were referenced but never defined.
 llvm::SmallString<64> Protocol;
-llvm::raw_svector_ostream(Protocol) << "\01l_OBJC_PROTOCOL_$_"
+llvm::raw_svector_ostream(Protocol) << "_OBJC_PROTOCOL_$_"
 << PD->getObjCRuntimeNameAsString();
 
 Entry = new llvm::GlobalVariable(CGM.getModule(), 
ObjCTypes.ProtocolnfABITy,
@@ -6901,7 +6901,7 @@ llvm::Constant *CGObjCNonFragileABIMac::
   } else {
 llvm::SmallString<64> symbolName;
 llvm::raw_svector_ostream(symbolName)
-  << "\01l_OBJC_PROTOCOL_$_" << PD->getObjCRuntimeNameAsString();
+  << "_OBJC_PROTOCOL_$_" << PD->getObjCRuntimeNameAsString();
 
 Entry = values.finishAndCreateGlobal(symbolName, CGM.getPointerAlign(),
  /*constant*/ false,
@@ -6917,7 +6917,7 @@ llvm::Constant *CGObjCNonFragileABIMac::
   // Use this protocol meta-data to build protocol list table in section
   // __DATA, __objc_protolist
   llvm::SmallString<64> ProtocolRef;
-  llvm::raw_svector_ostream(ProtocolRef) << "\01l_OBJC_LABEL_PROTOCOL_$_"
+  llvm::raw_svector_ostream(ProtocolRef) << "_OBJC_LABEL_PROTOCOL_$_"
  << PD->getObjCRuntimeNameAsString();
 
   llvm::GlobalVariable *PTGV =

Modified: cfe/trunk/test/CodeGenObjC/forward-protocol-metadata-symbols.m
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenObjC/forward-protocol-metadata-symbols.m?rev=356156&r1=356155&r2=356156&view=diff
==
--- cfe/trunk/test/CodeGenObjC/forward-protocol-metadata-symbols.m (original)
+++ cfe/trunk/test/CodeGenObjC/forward-protocol-metadata-symbols.m Thu Mar 14 
08:17:37 2019
@@ -18,14 +18,14 @@ int main() {
   return 0;
 }
 
-// CHECK: @"\01l_OBJC_PROTOCOL_$_P0" = weak hidden global
-// CHECK: @"\01l_OBJC_LABEL_PROTOCOL_$_P0" = weak hidden global
+// CHECK: @"_OBJC_PROTOCOL_$_P0" = weak hidden global
+// CHECK: @"_OBJC_LABEL_PROTOCOL_$_P0" = weak hidden global
 // CHECK: @"\01l_OBJC_CLASS_PROTOCOLS_$_A" = private global
 // CHECK: @"\01l_OBJC_PROTOCOL_REFERENCE_$_P0" = weak hidden global
 
 // CHECK: llvm.used = appending global [3 x i8*]
-// CHECK-SAME: "\01l_OBJC_PROTOCOL_$_P0"
-// CHECK-SAME: "\01l_OBJC_LABEL_PROTOCOL_$_P0"
+// CHECK-SAME: "_OBJC_PROTOCOL_$_P0"
+// CHECK-SAME: "_OBJC_LABEL_PROTOCOL_$_P0"
 // CHECK-SAME: "\01l_OBJC_PROTOCOL_REFERENCE_$_P0"
 
 // CHECK: llvm.compiler.used = appending global [7 x i8*]

Modified: cfe/trunk/test/CodeGenObjC/hidden-visibility.m
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenObjC/hidden-visibility.m?rev=356156&r1=356155&r2=356156&view=diff
==
--- cfe/trunk/test/CodeGenObjC/hidden-visibility.m (original)
+++ cfe/trunk/test/CodeGenObjC/hidden-visibility.m Thu Mar 14 08:17:37 2019
@@ -2,7 +2,7 @@
 // CHECK: @"OBJC_IVAR_$_I.P" = hidden
 // CHECK: @"OBJC_CLASS_$_I" = hidden
 // CHECK: @"OBJC_METACLASS_$_I" = hidden
-// CHECK: @"\01l_OBJC_PROTOCOL_$_Prot0" = weak hidden
+// CHECK: @"_OBJC_PROTOCOL_$_Prot0" = weak hidden
 
 @interface I {
   int P;

Modified: cfe/trunk/test/CodeGenObjC/metadata-class-properties.m
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenObjC/metadata-class-properties.m?rev=356156&r1=356155&r2=356156&view=diff
==
--- cfe/trunk/test/CodeGenObjC/metadata-class-properties.m (original)
+++ cfe/trunk/test/Co

[PATCH] D59234: [CodeGen][ObjC] Remove the leading 'l' from symbols for protocol metadata and protocol list

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

Okay, I'll remove the leading 'l' from other symbols and make them internal in 
a separate patch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59234



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

[PATCH] D59234: [CodeGen][ObjC] Remove the leading 'l' from symbols for protocol metadata and protocol list

[Akira Hatanaka via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL356156: [CodeGen][ObjC] Remove the leading 'l' 
from symbols for protocol (authored by ahatanak, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D59234?vs=190165&id=190634#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59234

Files:
  cfe/trunk/lib/CodeGen/CGObjCMac.cpp
  cfe/trunk/test/CodeGenObjC/forward-protocol-metadata-symbols.m
  cfe/trunk/test/CodeGenObjC/hidden-visibility.m
  cfe/trunk/test/CodeGenObjC/metadata-class-properties.m
  cfe/trunk/test/CodeGenObjC/metadata-symbols-64.m
  cfe/trunk/test/CodeGenObjC/protocol-comdat.m
  cfe/trunk/test/CodeGenObjC/protocol-in-extended-class.m
  cfe/trunk/test/CodeGenObjC/protocols.m
  cfe/trunk/test/CodeGenObjC/sections.m
  cfe/trunk/test/CodeGenObjC/undefined-protocol2.m

Index: cfe/trunk/lib/CodeGen/CGObjCMac.cpp
===
--- cfe/trunk/lib/CodeGen/CGObjCMac.cpp
+++ cfe/trunk/lib/CodeGen/CGObjCMac.cpp
@@ -6809,7 +6809,7 @@
 // reference or not. At module finalization we add the empty
 // contents for protocols which were referenced but never defined.
 llvm::SmallString<64> Protocol;
-llvm::raw_svector_ostream(Protocol) << "\01l_OBJC_PROTOCOL_$_"
+llvm::raw_svector_ostream(Protocol) << "_OBJC_PROTOCOL_$_"
 << PD->getObjCRuntimeNameAsString();
 
 Entry = new llvm::GlobalVariable(CGM.getModule(), ObjCTypes.ProtocolnfABITy,
@@ -6901,7 +6901,7 @@
   } else {
 llvm::SmallString<64> symbolName;
 llvm::raw_svector_ostream(symbolName)
-  << "\01l_OBJC_PROTOCOL_$_" << PD->getObjCRuntimeNameAsString();
+  << "_OBJC_PROTOCOL_$_" << PD->getObjCRuntimeNameAsString();
 
 Entry = values.finishAndCreateGlobal(symbolName, CGM.getPointerAlign(),
  /*constant*/ false,
@@ -6917,7 +6917,7 @@
   // Use this protocol meta-data to build protocol list table in section
   // __DATA, __objc_protolist
   llvm::SmallString<64> ProtocolRef;
-  llvm::raw_svector_ostream(ProtocolRef) << "\01l_OBJC_LABEL_PROTOCOL_$_"
+  llvm::raw_svector_ostream(ProtocolRef) << "_OBJC_LABEL_PROTOCOL_$_"
  << PD->getObjCRuntimeNameAsString();
 
   llvm::GlobalVariable *PTGV =
Index: cfe/trunk/test/CodeGenObjC/undefined-protocol2.m
===
--- cfe/trunk/test/CodeGenObjC/undefined-protocol2.m
+++ cfe/trunk/test/CodeGenObjC/undefined-protocol2.m
@@ -3,7 +3,7 @@
 // Test that we produce a declaration for the protocol. It must be matched
 // by a definition in another TU, so external is the correct linkage
 // (not extern_weak).
-// CHECK: @"\01l_OBJC_PROTOCOL_$_p1" = external global
+// CHECK: @"_OBJC_PROTOCOL_$_p1" = external global
 
 @interface NSObject
 @end
Index: cfe/trunk/test/CodeGenObjC/metadata-symbols-64.m
===
--- cfe/trunk/test/CodeGenObjC/metadata-symbols-64.m
+++ cfe/trunk/test/CodeGenObjC/metadata-symbols-64.m
@@ -11,8 +11,8 @@
 // CHECK: @"\01l_OBJC_$_CLASS_METHODS_A" = private global {{.*}} section "__DATA, __objc_const", align 8
 // CHECK: @"\01l_OBJC_$_PROTOCOL_INSTANCE_METHODS_P" = private global {{.*}} section "__DATA, __objc_const", align 8
 // CHECK: @"\01l_OBJC_$_PROTOCOL_CLASS_METHODS_P" = private global {{.*}} section "__DATA, __objc_const", align 8
-// CHECK: @"\01l_OBJC_PROTOCOL_$_P" = weak hidden global {{.*}}, align 8
-// CHECK: @"\01l_OBJC_LABEL_PROTOCOL_$_P" = weak hidden global {{.*}} section "__DATA,__objc_protolist,coalesced,no_dead_strip", align 8
+// CHECK: @"_OBJC_PROTOCOL_$_P" = weak hidden global {{.*}}, align 8
+// CHECK: @"_OBJC_LABEL_PROTOCOL_$_P" = weak hidden global {{.*}} section "__DATA,__objc_protolist,coalesced,no_dead_strip", align 8
 // CHECK: @"\01l_OBJC_CLASS_PROTOCOLS_$_A" = private global {{.*}} section "__DATA, __objc_const", align 8
 // CHECK: @"\01l_OBJC_METACLASS_RO_$_A" = private global {{.*}} section "__DATA, __objc_const", align 8
 // CHECK: @"\01l_OBJC_$_INSTANCE_METHODS_A" = private global {{.*}} section "__DATA, __objc_const", align 8
Index: cfe/trunk/test/CodeGenObjC/protocol-comdat.m
===
--- cfe/trunk/test/CodeGenObjC/protocol-comdat.m
+++ cfe/trunk/test/CodeGenObjC/protocol-comdat.m
@@ -18,11 +18,11 @@
   return @protocol(Q) == @protocol(R);
 }
 
-// CHECK: $"\01l_OBJC_PROTOCOL_$_P" = comdat any
-// CHECK: $"\01l_OBJC_LABEL_PROTOCOL_$_P" = comdat any
+// CHECK: $"_OBJC_PROTOCOL_$_P" = comdat any
+// CHECK: $"_OBJC_LABEL_PROTOCOL_$_P" = comdat any
 // CHECK: $"\01l_OBJC_PROTOCOL_REFERENCE_$_Q" = comdat any
 // CHECK: $"\01l_OBJC_PROTOCOL_REFERENCE_$_R" = comdat any
 
-// CHECK: @"\01l_OBJC_

[PATCH] D55269: [CUDA] Fix nvidia-cuda-toolkit detection on Ubuntu

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

jdenny added a comment.
Herald added a project: LLVM.

In D55269#1320382 , @tra wrote:

> In D55269#1320207 , @Hahnfeld wrote:
>
> > In D55269#1319109 , @jdenny wrote:
> >
> > > [...]
> > >
> > > In D55269#1318901 , @tra wrote:
> > >
> > > > --cuda-path=/usr was never supposed to work -- /usr is *not* the root 
> > > > of the CUDA SDK.
> > >
> > >
> > > /usr/lib/cuda/bin/nvcc doesn't exist, so that's probably why 
> > > FindCUDA.cmake finds /usr/bin/nvcc (also installed by 
> > > nvidia-cuda-toolkit).  Is it fair then to say that /usr/lib/cuda isn't 
> > > the root either?
> > >
> > > [...]
> > >
> > > It seems that nvidia-cuda-toolkit still isn't installing a complete CUDA 
> > > install in one location.  Even if it installed it all to /usr/lib/cuda, 
> > > FindCUDA.cmake would probably still see /usr/bin/nvcc and assume /usr is 
> > > the CUDA install root.
> >
> >
> > I think this needs to be fixed then: The shim package should provide links 
> > to all necessary things and CMake must be prepared to deal with it. IMO we 
> > shouldn't workaround broken build system detection in the compiler.
>
>
> That's exactly what I proposed to Debian folks 
> https://bugs.llvm.org/show_bug.cgi?id=26966#c6 and I was under impression 
> that that's what they did. It appears that they only created an empty 
> directory structure with version.txt in it. At least that's what I see when I 
> unpack nvidia-cuda-toolkit_9.1.85-3ubuntu1_amd64.deb. Perhaps they do 
> something extra in the install script, but I didn't find anything obvious in 
> the deb itself.
>
> > 
> > 
> > In D55269#1319116 , @jdenny wrote:
> > 
> >> By the way, nvidia-cuda-toolkit does install the following, but there's no 
> >> nvvm directory as clang currently expects:
> > 
> > 
> > Then again the distro's packaging is broken and needs to be adjusted.
>
> Perhaps we can build a shim package ourselves and distribute it along with 
> the clang. This would decouple usability of clang from the Ubuntu/Debian 
> release process and would make it possible to make clang work with CUDA on 
> older debian/Ubuntu versions.
>
> >> Let's start with fixing OpenMP's cmake files. Once it no longer insists on 
> >> specifying --cuda-path=/usr, and isUbuntu is in place, what is the 
> >> remaining failure that you see?
> > 
> > I disagree here: It's not OpenMP but CMake that's detecting the wrong path 
> > here. This is the place to fix it once and for all (and possibly get the 
> > shim package right in that process).
>
> It's somewhat orthogonal, IMO. I agree that it's not OpenMP's problem. Cmake 
> will fix it at some point in future, but, presumably, we want OpenMP 
> buildable *now*. Adding a temporary workaround for something that Cmake can't 
> handle now would make building OpenMP with clang somewhat easier which was 
> the end goal of this patch. In the end it's up to OpenMP maintainers what 
> they would want to do.


For openmp, I worked around the aforementioned cmake limitation in D55588 
.

I reported the aforementioned problem with the Ubuntu package 
nvidia-cuda-toolkit in December at:

https://bugs.launchpad.net/ubuntu/+source/nvidia-cuda-toolkit/+bug/1808999

Today, I received a response (included at the above link) that this is not a 
valid bug and that clang++ works fine with nvidia-cuda-toolkit.  I suppose the 
latter half is true, but the point is that we're having to work around the 
strange Debian/Ubuntu packaging in both Clang and our cmake files.  Do people 
here think it's worth pursuing this point further with Debian/Ubuntu package 
maintainers?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55269



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

[PATCH] D59319: [OpenMP][Offloading][1/3] A generic and simple target region interface

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: openmp/libomptarget/deviceRTLs/common/target_region.h:27
+
+/// The target region _kernel_ interface for GPUs
+///

jdoerfert wrote:
> ABataev wrote:
> > All exported functions are declared in the `interface.h` file. I don't 
> > think we need an extra interface file here
> `interface.h`, or to be more precise for people that do not know, 
> `deviceRTLs/nvptx/src/interface.h`, is nvptx specific. This file, 
> `deviceRTLs/common/target_region.h`, is by design target agnostic and not 
> placed _under_ the nvptx subfolder. If you are willing to move `interface.h` 
> into a common space and remove the nvptx specific functions we can merge the 
> two. Otherwise, I have strong reservations agains that and good reason not to 
> do it.
I see that currently it is written in Cuda. It means, it targets NVidia GPUs, 
at least at the moment. I'm fine to put this header file into the common 
directory, if you're sure that this is really target agnostic. But maybe just 
for a start we should put it to NVPTX directory? Later, when you or somebody 
else will add support for other GPUs and he/she will find out that these 
functions are really target agnostic, they can be moved into the common 
directory?



Comment at: openmp/libomptarget/deviceRTLs/common/target_region.h:100
+///
+EXTERN int8_t __kmpc_target_region_kernel_init(bool UseSPMDMode,
+   bool RequiresOMPRuntime,

jdoerfert wrote:
> ABataev wrote:
> > Better to use `ident_loc` for passing info about execution mode and 
> > full/lightweight runtime.
> Could you please explain why you think that? Adding indirection through a 
> structure does not really seem beneficial to me.
Almost all function from libomp rely on `ident_loc`. The functions, which were 
added for NVPTX without this parameter had a lot of problems later and most of 
them were replaced with the functions with this parameter type. Plus, this 
parameter is used for OMPD/OMPT and it may be important for future OMPD/OMPT 
support.



Comment at: openmp/libomptarget/deviceRTLs/common/target_region.h:124
+/// unpacking code.
+typedef void (*ParallelWorkFnTy)(char * /* SharedValues */,
+ char * /* PrivateValues */);

We used `void *` for buffers usually, I think it is better to use `void *` here 
too instead of `char *`.



Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/omp_data.cu:70
+
+__device__ __shared__ target_region_shared_buffer _target_region_shared_memory;
+

It would be good to store it the global memory rather than in the shared to 
save th shared memory. Also, we already are using several shared memory buffers 
for different purposes, it would be good to merge them somehow to reduce 
pressure on shared memory.



Comment at: openmp/libomptarget/deviceRTLs/nvptx/src/target_region.cu:64
+
+/// Filter threads into masters and workers. If \p UseStateMachine is true,
+/// required workers will enter a state machine through and be trapped there.

What is the criteria for `UseStateMachine`? Under what conditions it can be set 
to `true` and `false`? Also, what if have several parallel regions in non-SPMD 
kernel and `UseStateMachine` is `true`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59319



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

[PATCH] D59367: [Sema] Adjust address space of operands in remaining builtin operators

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added reviewers: rjmccall, bevinh.
Herald added subscribers: jdoerfert, ebevhan.

Fix address space of reference operand in the remaining builtin compound 
assignment operators.


https://reviews.llvm.org/D59367

Files:
  lib/Sema/SemaOverload.cpp
  test/CodeGenOpenCLCXX/addrspace-operators.cl


Index: test/CodeGenOpenCLCXX/addrspace-operators.cl
===
--- test/CodeGenOpenCLCXX/addrspace-operators.cl
+++ test/CodeGenOpenCLCXX/addrspace-operators.cl
@@ -14,6 +14,8 @@
 };
 
 __global E globE;
+volatile __global int globVI;
+__global int globI;
 //CHECK-LABEL: define spir_func void @_Z3barv()
 void bar() {
   C c;
@@ -25,13 +27,18 @@
   c.OrAssign(a);
 
   E e;
-  // CHECK: store i32 1, i32* %e
+  //CHECK: store i32 1, i32* %e
   e = b;
-  // CHECK: store i32 0, i32 addrspace(1)* @globE
+  //CHECK: store i32 0, i32 addrspace(1)* @globE
   globE = a;
-  // FIXME: Sema fails here because it thinks the types are incompatible.
-  //e = b;
-  //globE = a;
+  //CHECK: store i32 %or, i32 addrspace(1)* @globI
+  globI |= b;
+  //CHECK: store i32 %add, i32 addrspace(1)* @globI
+  globI += a;
+  //CHECK: store volatile i32 %and, i32 addrspace(1)* @globVI
+  globVI &= b;
+  //CHECK: store volatile i32 %sub, i32 addrspace(1)* @globVI
+  globVI -= a;
 }
 
 //CHECK: define linkonce_odr void @_ZNU3AS41C6AssignE1E(%class.C addrspace(4)* 
%this, i32 %e)
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -8513,17 +8513,16 @@
Right < LastPromotedArithmeticType; ++Right) {
 QualType ParamTypes[2];
 ParamTypes[1] = ArithmeticTypes[Right];
-
+auto LeftBaseTy = AdjustAddressSpaceForBuiltinOperandType(
+S, ArithmeticTypes[Left], Args[0]);
 // Add this built-in operator as a candidate (VQ is empty).
-ParamTypes[0] =
-  S.Context.getLValueReferenceType(ArithmeticTypes[Left]);
+ParamTypes[0] = S.Context.getLValueReferenceType(LeftBaseTy);
 S.AddBuiltinCandidate(ParamTypes, Args, CandidateSet,
   /*IsAssigmentOperator=*/isEqualOp);
 
 // Add this built-in operator as a candidate (VQ is 'volatile').
 if (VisibleTypeConversionsQuals.hasVolatile()) {
-  ParamTypes[0] =
-S.Context.getVolatileType(ArithmeticTypes[Left]);
+  ParamTypes[0] = S.Context.getVolatileType(LeftBaseTy);
   ParamTypes[0] = S.Context.getLValueReferenceType(ParamTypes[0]);
   S.AddBuiltinCandidate(ParamTypes, Args, CandidateSet,
 /*IsAssigmentOperator=*/isEqualOp);
@@ -8579,15 +8578,14 @@
Right < LastPromotedIntegralType; ++Right) {
 QualType ParamTypes[2];
 ParamTypes[1] = ArithmeticTypes[Right];
-
+auto LeftBaseTy = AdjustAddressSpaceForBuiltinOperandType(
+S, ArithmeticTypes[Left], Args[0]);
 // Add this built-in operator as a candidate (VQ is empty).
-ParamTypes[0] = S.Context.getLValueReferenceType(
-AdjustAddressSpaceForBuiltinOperandType(S, ArithmeticTypes[Left],
-Args[0]));
+ParamTypes[0] = S.Context.getLValueReferenceType(LeftBaseTy);
 S.AddBuiltinCandidate(ParamTypes, Args, CandidateSet);
 if (VisibleTypeConversionsQuals.hasVolatile()) {
   // Add this built-in operator as a candidate (VQ is 'volatile').
-  ParamTypes[0] = ArithmeticTypes[Left];
+  ParamTypes[0] = LeftBaseTy;
   ParamTypes[0] = S.Context.getVolatileType(ParamTypes[0]);
   ParamTypes[0] = S.Context.getLValueReferenceType(ParamTypes[0]);
   S.AddBuiltinCandidate(ParamTypes, Args, CandidateSet);


Index: test/CodeGenOpenCLCXX/addrspace-operators.cl
===
--- test/CodeGenOpenCLCXX/addrspace-operators.cl
+++ test/CodeGenOpenCLCXX/addrspace-operators.cl
@@ -14,6 +14,8 @@
 };
 
 __global E globE;
+volatile __global int globVI;
+__global int globI;
 //CHECK-LABEL: define spir_func void @_Z3barv()
 void bar() {
   C c;
@@ -25,13 +27,18 @@
   c.OrAssign(a);
 
   E e;
-  // CHECK: store i32 1, i32* %e
+  //CHECK: store i32 1, i32* %e
   e = b;
-  // CHECK: store i32 0, i32 addrspace(1)* @globE
+  //CHECK: store i32 0, i32 addrspace(1)* @globE
   globE = a;
-  // FIXME: Sema fails here because it thinks the types are incompatible.
-  //e = b;
-  //globE = a;
+  //CHECK: store i32 %or, i32 addrspace(1)* @globI
+  globI |= b;
+  //CHECK: store i32 %add, i32 addrspace(1)* @globI
+  globI += a;
+  //CHECK: store volatile i32 %and, i32 addrspace(1)* @globVI
+  globVI &= b;
+  //CHECK: store volatile i32 %sub, i32 addrspace(1)* @globVI
+  globVI -= a;
 }
 
 //CHECK: define linkonce_odr void @_ZNU3AS41C6AssignE1E(%class.C addrspace(4)* %

[PATCH] D59354: [clangd] Print arguments in template specializations

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Mostly nits, but could you elaborate why can't we print the type-loc when 
printing the template argument (see the corresponding comment)?




Comment at: clang-tools-extra/clangd/AST.cpp:66
+return Var->getTemplateArgsInfo().arguments();
+  return llvm::None;
+}

NIT: add a note that we return null for `ClassTemplateSpecializationDecl ` 
because we can't construct an `ArrayRef` for it



Comment at: clang-tools-extra/clangd/AST.cpp:84
+if (auto STL = TL.getAs()) {
+  std::vector ArgLocs;
+  for (unsigned I = 0; I < STL.getNumArgs(); I++)

NIT: use `SmallVector<8>` or some other small-enough number to avoid most 
allocs.



Comment at: clang-tools-extra/unittests/clangd/IndexTests.cpp:18
 #include "clang/Index/IndexSymbol.h"
+#include "gmock/gmock-generated-matchers.h"
 #include "gmock/gmock.h"

NIT: maybe remove it? clangd keeps adding those, but I don't think we actually 
want it: `gmock.h` should be enough



Comment at: clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp:417
+ForCodeCompletion(true)),
+  AllOf(QName("Tmpl"),
+DeclRange(Header.range("specdecl")), ForCodeCompletion(false)),

Does it mean typing `bool` could potentially match `vector` in 
`workspaceSymbols` now?
If that's the case we might start producing some noise.



Comment at: clang/lib/AST/TypePrinter.cpp:1644
+llvm::raw_ostream &OS) {
+  A.getTypeSourceInfo()->getType().print(OS, PP);
+}

kadircet wrote:
> ilya-biryukov wrote:
> > Maybe print the result of `getTypeLoc()` here, if it's available?
> > Would produce results closer to the ones written in the code.
> unfortunately it is not available.
you mean the function to print a type loc or the type loc itself?



Comment at: clang/lib/AST/TypePrinter.cpp:1646
+break;
+  case TemplateArgument::ArgKind::Type:
+A.getTypeSourceInfo()->getType().print(OS, PP);

Maybe simplify the switch to:
```
if (A.getKind() == TemplateArgument::ArgKind::Type) {
A.getTypeSourceInfo()->getType().print(OS, PP);
return;
}
A.getArgument().print(PP, OS);
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59354



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

[PATCH] D59354: [clangd] Print arguments in template specializations

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 190642.
kadircet marked 2 inline comments as done.
kadircet added a comment.

- Address some of the comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59354

Files:
  clang-tools-extra/clangd/AST.cpp
  clang-tools-extra/clangd/index/MemIndex.cpp
  clang-tools-extra/clangd/index/dex/Dex.cpp
  clang-tools-extra/unittests/clangd/DexTests.cpp
  clang-tools-extra/unittests/clangd/IndexTests.cpp
  clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
  clang/lib/AST/TypePrinter.cpp

Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1632,6 +1632,32 @@
   return A.getArgument();
 }
 
+static void printArgument(const TemplateArgument &A, const PrintingPolicy &PP,
+  llvm::raw_ostream &OS) {
+  A.print(PP, OS);
+}
+
+static void printArgument(const TemplateArgumentLoc &A,
+  const PrintingPolicy &PP, llvm::raw_ostream &OS) {
+  switch (A.getArgument().getKind()) {
+  case TemplateArgument::ArgKind::Null:
+assert(false && "TemplateArgumentKind can not be null!");
+break;
+  case TemplateArgument::ArgKind::Type:
+A.getTypeSourceInfo()->getType().print(OS, PP);
+break;
+  case TemplateArgument::ArgKind::Declaration:
+  case TemplateArgument::ArgKind::NullPtr:
+  case TemplateArgument::ArgKind::Integral:
+  case TemplateArgument::ArgKind::Template:
+  case TemplateArgument::ArgKind::TemplateExpansion:
+  case TemplateArgument::ArgKind::Expression:
+  case TemplateArgument::ArgKind::Pack:
+A.getArgument().print(PP, OS);
+break;
+  }
+}
+
 template
 static void printTo(raw_ostream &OS, ArrayRef Args,
 const PrintingPolicy &Policy, bool SkipBrackets) {
@@ -1653,7 +1679,7 @@
 } else {
   if (!FirstArg)
 OS << Comma;
-  Argument.print(Policy, ArgOS);
+  printArgument(Arg, Policy, ArgOS);
 }
 StringRef ArgString = ArgOS.str();
 
Index: clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
===
--- clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
+++ clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
@@ -394,23 +394,36 @@
   Annotations Header(R"(
 // Primary template and explicit specialization are indexed, instantiation
 // is not.
-template  struct [[Tmpl]] {T $xdecl[[x]] = 0;};
-template <> struct $specdecl[[Tmpl]] {};
-template  struct $partspecdecl[[Tmpl]] {};
-extern template struct Tmpl;
-template struct Tmpl;
+template  class $barclasstemp[[Bar]] {};
+template  class Z, int Q>
+struct [[Tmpl]] { T $xdecl[[x]] = 0; };
+template <> struct $specdecl[[Tmpl]] {};
+template  struct $partspecdecl[[Tmpl]] {};
+extern template struct Tmpl;
+template struct Tmpl;
+template  class $fooclasstemp[[Foo]] {};
+template<> class $templtempl[[Foo]], int, double> {};
   )");
   runSymbolCollector(Header.code(), /*Main=*/"");
-  EXPECT_THAT(Symbols,
-  UnorderedElementsAre(
-  AllOf(QName("Tmpl"), DeclRange(Header.range()),
-ForCodeCompletion(true)),
-  AllOf(QName("Tmpl"), DeclRange(Header.range("specdecl")),
-ForCodeCompletion(false)),
-  AllOf(QName("Tmpl"), DeclRange(Header.range("partspecdecl")),
-ForCodeCompletion(false)),
-  AllOf(QName("Tmpl::x"), DeclRange(Header.range("xdecl")),
-ForCodeCompletion(false;
+  EXPECT_THAT(
+  Symbols,
+  UnorderedElementsAre(
+  AllOf(QName("Tmpl"), DeclRange(Header.range()),
+ForCodeCompletion(true)),
+  AllOf(QName("Bar"), DeclRange(Header.range("barclasstemp")),
+ForCodeCompletion(true)),
+  AllOf(QName("Foo"), DeclRange(Header.range("fooclasstemp")),
+ForCodeCompletion(true)),
+  AllOf(QName("Tmpl"),
+DeclRange(Header.range("specdecl")), ForCodeCompletion(false)),
+  AllOf(QName("Tmpl"),
+DeclRange(Header.range("partspecdecl")),
+ForCodeCompletion(false)),
+  AllOf(QName("Foo, int, double>"),
+DeclRange(Header.range("templtempl")),
+ForCodeCompletion(false)),
+  AllOf(QName("Tmpl::x"), DeclRange(Header.range("xdecl")),
+ForCodeCompletion(false;
 }
 
 TEST_F(SymbolCollectorTest, ObjCSymbols) {
Index: clang-tools-extra/unittests/clangd/IndexTests.cpp
===
--- clang-tools-extra/unittests/clangd/IndexTests.cpp
+++ clang-tools-extra/unittests/clangd/IndexTests.cpp
@@ -187,35 +187,30 @@
   SymbolSlab::Builder B;
 
   Symbol S =

[PATCH] D59354: [clangd] Print arguments in template specializations

[Kadir Cetinkaya via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clangd/AST.cpp:84
+if (auto STL = TL.getAs()) {
+  std::vector ArgLocs;
+  for (unsigned I = 0; I < STL.getNumArgs(); I++)

ilya-biryukov wrote:
> NIT: use `SmallVector<8>` or some other small-enough number to avoid most 
> allocs.
calling reserve beforehand



Comment at: clang-tools-extra/unittests/clangd/IndexTests.cpp:18
 #include "clang/Index/IndexSymbol.h"
+#include "gmock/gmock-generated-matchers.h"
 #include "gmock/gmock.h"

ilya-biryukov wrote:
> NIT: maybe remove it? clangd keeps adding those, but I don't think we 
> actually want it: `gmock.h` should be enough
Should we add IWYU pragmas to those files? 
https://github.com/google/googlemock/blob/master/googlemock/include/gmock/gmock-generated-matchers.h



Comment at: clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp:417
+ForCodeCompletion(true)),
+  AllOf(QName("Tmpl"),
+DeclRange(Header.range("specdecl")), ForCodeCompletion(false)),

ilya-biryukov wrote:
> Does it mean typing `bool` could potentially match `vector` in 
> `workspaceSymbols` now?
> If that's the case we might start producing some noise.
unfortunately yes it does, what do you suggest? it seems like we can perform a 
"isprefix" check for symbols with template specs kind?



Comment at: clang/lib/AST/TypePrinter.cpp:1644
+llvm::raw_ostream &OS) {
+  A.getTypeSourceInfo()->getType().print(OS, PP);
+}

ilya-biryukov wrote:
> kadircet wrote:
> > ilya-biryukov wrote:
> > > Maybe print the result of `getTypeLoc()` here, if it's available?
> > > Would produce results closer to the ones written in the code.
> > unfortunately it is not available.
> you mean the function to print a type loc or the type loc itself?
function to print a typeloc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59354



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

[PATCH] D58777: [analyzer] Fix an assertation failurure for invalid sourcelocation, add a new debug checker

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

Szelethus updated this revision to Diff 190643.
Szelethus marked 7 inline comments as done.
Szelethus added a comment.

Fixes according to inline comments.


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

https://reviews.llvm.org/D58777

Files:
  docs/analyzer/developer-docs/DebugChecks.rst
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
  lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  test/Analysis/diagnostics/invalid-srcloc-fix.cpp
  test/Analysis/plist-html-macros.c

Index: test/Analysis/plist-html-macros.c
===
--- test/Analysis/plist-html-macros.c
+++ test/Analysis/plist-html-macros.c
@@ -3,7 +3,10 @@
 
 // RUN: rm -rf %t.dir
 // RUN: mkdir -p %t.dir
-// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-output=plist-html -o %t.dir/index.plist %s
+//
+// RUN: %clang_analyze_cc1 -o %t.dir/index.plist %s \
+// RUN:   -analyzer-checker=core -analyzer-output=plist-html
+//
 // RUN: ls %t.dir | grep '\.html' | count 1
 // RUN: grep '\.html' %t.dir/index.plist | count 1
 
Index: test/Analysis/diagnostics/invalid-srcloc-fix.cpp
===
--- /dev/null
+++ test/Analysis/diagnostics/invalid-srcloc-fix.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-output=plist -o %t.plist \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ReportStmts
+
+struct h {
+  operator int();
+};
+
+int k() {
+  return h(); // expected-warning 3 {{Statement}}
+}
Index: lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -571,6 +571,8 @@
 } while (!L.isValid());
   }
 
+  // FIXME: Ironically, this assert actually fails in some cases.
+  //assert(L.isValid());
   return L;
 }
 
@@ -671,7 +673,15 @@
 PathDiagnosticLocation
 PathDiagnosticLocation::createMemberLoc(const MemberExpr *ME,
 const SourceManager &SM) {
-  return PathDiagnosticLocation(ME->getMemberLoc(), SM, SingleLocK);
+
+  assert(ME->getMemberLoc().isValid() || ME->getBeginLoc().isValid());
+
+  // In some cases, getMemberLoc isn't valid -- in this case we'll return with
+  // some other related valid SourceLocation.
+  if (ME->getMemberLoc().isValid())
+return PathDiagnosticLocation(ME->getMemberLoc(), SM, SingleLocK);
+
+  return PathDiagnosticLocation(ME->getBeginLoc(), SM, SingleLocK);
 }
 
 PathDiagnosticLocation
Index: lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
===
--- lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
+++ lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
@@ -266,3 +266,34 @@
 bool ento::shouldRegisterExplodedGraphViewer(const LangOptions &LO) {
   return true;
 }
+
+//===--===//
+// Emits a report for every Stmt that the analyzer visits.
+//===--===//
+
+namespace {
+
+class ReportStmts : public Checker> {
+  BuiltinBug BT_stmtLoc{this, "Statement"};
+
+public:
+  void checkPreStmt(const Stmt *S, CheckerContext &C) const {
+ExplodedNode *Node = C.generateNonFatalErrorNode();
+if (!Node)
+  return;
+
+auto Report = llvm::make_unique(BT_stmtLoc, "Statement", Node);
+
+C.emitReport(std::move(Report));
+  }
+};
+
+} // end of anonymous namespace
+
+void ento::registerReportStmts(CheckerManager &mgr) {
+  mgr.registerChecker();
+}
+
+bool ento::shouldRegisterReportStmts(const LangOptions &LO) {
+  return true;
+}
Index: include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1019,6 +1019,10 @@
   HelpText<"View Exploded Graphs using GraphViz">,
   Documentation;
 
+def ReportStmts : Checker<"ReportStmts">,
+  HelpText<"Emits a warning for every statement.">,
+  Documentation;
+
 } // end "debug"
 
 
Index: docs/analyzer/developer-docs/DebugChecks.rst
===
--- docs/analyzer/developer-docs/DebugChecks.rst
+++ docs/analyzer/developer-docs/DebugChecks.rst
@@ -285,3 +285,10 @@
 statistics within the analyzer engine. Note the Stats checker (which produces at
 least one bug report per function) may actually change the values reported by
 -analyzer-stats.
+
+Output testing checkers
+===
+
+- debug.ReportStmts reports a warning at **every** statement, making it a very
+  useful tool for testing thoroughly bug report construction and output
+  emission.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http

[PATCH] D58777: [analyzer] Fix an assertation failurure for invalid sourcelocation, add a new debug checker

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

Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/DebugCheckers.cpp:271
+//===--===//
+// Emits a report for each and every Stmt.
+//===--===//

NoQ wrote:
> Technically, we don't invoke `checkPreStmt` for every `Stmt` (eg., `IfStmt` 
> is omitted in favor of `checkBranchCondition`, `IntegerLiteral` is outright 
> skipped because `ExprEngine` doesn't even evaluate anything at this point). 
> Moreover, for some `Stmt`s we more-or-less-intentionally only invoke 
> `checkPreStmt` but not `checkPostStmt`.
> 
> Also would you eventually want to expand this checker with non-`Stmt` 
> callbacks?
>Also would you eventually want to expand this checker with non-Stmt callbacks?
No :)



Comment at: lib/StaticAnalyzer/Core/PathDiagnostic.cpp:679
+
+  if (ME->getMemberLoc().isValid())
+return PathDiagnosticLocation(ME->getMemberLoc(), SM, SingleLocK);

NoQ wrote:
> I think it's worth commenting why exactly this may fail. It's fairly hard to 
> guess that we're talking about member operators.
I mean, I have absolutely no idea how this happened :') The best I can do is to 
document that it does happen from time to time and why this is the solution to 
that.



Comment at: test/Analysis/invalid-pos-fix.cpp:6
+
+struct h {
+  operator int();

NoQ wrote:
> Random ideas on organizing tests so that to avoid adding thousands of 
> one-test files:
> * Give these objects fancier names and/or add a `no-crash` so that it was 
> clear that this test tests a crash for reporting a bug over a `MemberExpr` 
> that corresponds to an `operator()`
> * Or maybe wrap this into a namespace, so that it was easier to expand this 
> file.
> * We traditionally put such tests into `test/Analysis/diagnostics`, dunno why 
> though.
> etc.
> 
> I also recall @george.karpenkov arguing for making test directory structure 
> mirror source directory structure, but that's definitely not a blocker for 
> this patch :]
>I also recall @george.karpenkov arguing for making test directory structure 
>mirror source directory structure, but that's definitely not a blocker for 
>this patch :]

I always wondered why we are all bunched up in `test/Analysis`.


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

https://reviews.llvm.org/D58777



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

[PATCH] D59307: Patch llvm bug 41033 concerning atomicity of statement expressions

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

In D59307#1428145 , @jfb wrote:

> In D59307#1428101 , @mibintc wrote:
>
> > In D59307#1427644 , @jfb wrote:
> >
> > > I think you also want to test C++ `std::atomic` ...
> >
> >
> > The bug described in https://bugs.llvm.org/show_bug.cgi?id=41033 doesn't 
> > occur using C++ atomic, I rewrote the test case this way, and it compiles 
> > without error.  The "load" operation returns int since all the atomic 
> > operations occur under the covers using builtins.   Does this convey the 
> > test you have in mind?
> >
> > #include 
> >  int fum(int y) {
> >
> >   std::atomic x(1);
> >   y = ({x.load();});
> >
> > }
>
>
> What I had in mind with `atomic` isn't relevant, because it would try to call 
> `atomic(const atomic&) = delete;`. Your test case here isn't something I'm 
> worried about.
>
> `volatile` is still and issue.
>
> I'm still not sure this is something we want.


Now I think the problem needs to be fixed at the atomic load, not in this 
place.  Thanks for pointing out the odd use of tmp. I'm going to abandon this 
patch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59307



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

[PATCH] D59307: Patch llvm bug 41033 concerning atomicity of statement expressions

[Melanie Blower via Phabricator via cfe-commits]

mibintc added a comment.

In D59307#1427853 , @erichkeane wrote:

> This is my concern here: 
>  https://godbolt.org/z/icS4fa
>  The patch will change template instantiation.
>
> GCC doesn't seem to allow using _Atomic in C++ mode, which is perhaps a 
> necessary part of this solution?  I see we already consider overload sets 
> with int and _Atomic(int) to be ambiguous (in addition to not considering 
> conversion operators of _Atomic(int) at all??), so it is limited to type 
> deduction. I could see this causing problems, since specializations of 
> templates are allowed on int and _Atomic(int) despite not being used in 
> overload sets; https://godbolt.org/z/oMAvpz


Recognizing _Atomic in c++ mode is an extension right?  Probably it needs more 
thought, for example the test case you provided.  Thank you


Repository:
  rC Clang

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

https://reviews.llvm.org/D59307



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

[PATCH] D59307: Patch llvm bug 41033 concerning atomicity of statement expressions

[Melanie Blower via Phabricator via cfe-commits]

mibintc abandoned this revision.
mibintc added a comment.

The bug should be fixed at the atomic-load (drop _Atomic qualifier) then this 
patch won't be needed in StmtExpr


Repository:
  rC Clang

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

https://reviews.llvm.org/D59307



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

r356161 - [analyzer] Fix an assertation failure for invalid sourcelocation, add a new debug checker

[Kristof Umann via cfe-commits]

Author: szelethus
Date: Thu Mar 14 09:10:29 2019
New Revision: 356161

URL: http://llvm.org/viewvc/llvm-project?rev=356161&view=rev
Log:
[analyzer] Fix an assertation failure for invalid sourcelocation, add a new 
debug checker

For a rather short code snippet, if debug.ReportStmts (added in this patch) was
enabled, a bug reporter visitor crashed:

struct h {
  operator int();
};

int k() {
  return h();
}

Ultimately, this originated from PathDiagnosticLocation::createMemberLoc, as it
didn't handle the case where it's MemberExpr typed parameter returned and
invalid SourceLocation for MemberExpr::getMemberLoc. The solution was to find
any related valid SourceLocaion, and Stmt::getBeginLoc happens to be just that.

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

Added:
cfe/trunk/test/Analysis/diagnostics/invalid-srcloc-fix.cpp
Modified:
cfe/trunk/docs/analyzer/developer-docs/DebugChecks.rst
cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
cfe/trunk/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
cfe/trunk/test/Analysis/plist-html-macros.c

Modified: cfe/trunk/docs/analyzer/developer-docs/DebugChecks.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/analyzer/developer-docs/DebugChecks.rst?rev=356161&r1=356160&r2=356161&view=diff
==
--- cfe/trunk/docs/analyzer/developer-docs/DebugChecks.rst (original)
+++ cfe/trunk/docs/analyzer/developer-docs/DebugChecks.rst Thu Mar 14 09:10:29 
2019
@@ -285,3 +285,10 @@ There is also an additional -analyzer-st
 statistics within the analyzer engine. Note the Stats checker (which produces 
at
 least one bug report per function) may actually change the values reported by
 -analyzer-stats.
+
+Output testing checkers
+===
+
+- debug.ReportStmts reports a warning at **every** statement, making it a very
+  useful tool for testing thoroughly bug report construction and output
+  emission.

Modified: cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td?rev=356161&r1=356160&r2=356161&view=diff
==
--- cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td Thu Mar 14 
09:10:29 2019
@@ -1019,6 +1019,10 @@ def ExplodedGraphViewer : Checker<"ViewE
   HelpText<"View Exploded Graphs using GraphViz">,
   Documentation;
 
+def ReportStmts : Checker<"ReportStmts">,
+  HelpText<"Emits a warning for every statement.">,
+  Documentation;
+
 } // end "debug"
 
 

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp?rev=356161&r1=356160&r2=356161&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp Thu Mar 14 09:10:29 
2019
@@ -266,3 +266,34 @@ void ento::registerExplodedGraphViewer(C
 bool ento::shouldRegisterExplodedGraphViewer(const LangOptions &LO) {
   return true;
 }
+
+//===--===//
+// Emits a report for every Stmt that the analyzer visits.
+//===--===//
+
+namespace {
+
+class ReportStmts : public Checker> {
+  BuiltinBug BT_stmtLoc{this, "Statement"};
+
+public:
+  void checkPreStmt(const Stmt *S, CheckerContext &C) const {
+ExplodedNode *Node = C.generateNonFatalErrorNode();
+if (!Node)
+  return;
+
+auto Report = llvm::make_unique(BT_stmtLoc, "Statement", Node);
+
+C.emitReport(std::move(Report));
+  }
+};
+
+} // end of anonymous namespace
+
+void ento::registerReportStmts(CheckerManager &mgr) {
+  mgr.registerChecker();
+}
+
+bool ento::shouldRegisterReportStmts(const LangOptions &LO) {
+  return true;
+}

Modified: cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp?rev=356161&r1=356160&r2=356161&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp Thu Mar 14 09:10:29 
2019
@@ -571,6 +571,8 @@ static SourceLocation getValidSourceLoca
 } while (!L.isValid());
   }
 
+  // FIXME: Ironically, this assert actually fails in some cases.
+  //assert(L.isValid());
   return L;
 }
 
@@ -671,7 +673,15 @@ PathDiagnosticLocation::createConditiona
 PathDiagnosticLocation
 PathDiagnosticLocation::createMemberLoc(const MemberExpr *ME,

[PATCH] D58777: [analyzer] Fix an assertation failurure for invalid sourcelocation, add a new debug checker

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rC356161: [analyzer] Fix an assertation failure for invalid 
sourcelocation, add a new… (authored by Szelethus, committed by ).

Repository:
  rC Clang

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

https://reviews.llvm.org/D58777

Files:
  docs/analyzer/developer-docs/DebugChecks.rst
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
  lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  test/Analysis/diagnostics/invalid-srcloc-fix.cpp
  test/Analysis/plist-html-macros.c

Index: test/Analysis/plist-html-macros.c
===
--- test/Analysis/plist-html-macros.c
+++ test/Analysis/plist-html-macros.c
@@ -3,7 +3,10 @@
 
 // RUN: rm -rf %t.dir
 // RUN: mkdir -p %t.dir
-// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-output=plist-html -o %t.dir/index.plist %s
+//
+// RUN: %clang_analyze_cc1 -o %t.dir/index.plist %s \
+// RUN:   -analyzer-checker=core -analyzer-output=plist-html
+//
 // RUN: ls %t.dir | grep '\.html' | count 1
 // RUN: grep '\.html' %t.dir/index.plist | count 1
 
Index: test/Analysis/diagnostics/invalid-srcloc-fix.cpp
===
--- test/Analysis/diagnostics/invalid-srcloc-fix.cpp
+++ test/Analysis/diagnostics/invalid-srcloc-fix.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-output=plist -o %t.plist \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ReportStmts
+
+struct h {
+  operator int();
+};
+
+int k() {
+  return h(); // expected-warning 3 {{Statement}}
+}
Index: lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -571,6 +571,8 @@
 } while (!L.isValid());
   }
 
+  // FIXME: Ironically, this assert actually fails in some cases.
+  //assert(L.isValid());
   return L;
 }
 
@@ -671,7 +673,15 @@
 PathDiagnosticLocation
 PathDiagnosticLocation::createMemberLoc(const MemberExpr *ME,
 const SourceManager &SM) {
-  return PathDiagnosticLocation(ME->getMemberLoc(), SM, SingleLocK);
+
+  assert(ME->getMemberLoc().isValid() || ME->getBeginLoc().isValid());
+
+  // In some cases, getMemberLoc isn't valid -- in this case we'll return with
+  // some other related valid SourceLocation.
+  if (ME->getMemberLoc().isValid())
+return PathDiagnosticLocation(ME->getMemberLoc(), SM, SingleLocK);
+
+  return PathDiagnosticLocation(ME->getBeginLoc(), SM, SingleLocK);
 }
 
 PathDiagnosticLocation
Index: lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
===
--- lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
+++ lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
@@ -266,3 +266,34 @@
 bool ento::shouldRegisterExplodedGraphViewer(const LangOptions &LO) {
   return true;
 }
+
+//===--===//
+// Emits a report for every Stmt that the analyzer visits.
+//===--===//
+
+namespace {
+
+class ReportStmts : public Checker> {
+  BuiltinBug BT_stmtLoc{this, "Statement"};
+
+public:
+  void checkPreStmt(const Stmt *S, CheckerContext &C) const {
+ExplodedNode *Node = C.generateNonFatalErrorNode();
+if (!Node)
+  return;
+
+auto Report = llvm::make_unique(BT_stmtLoc, "Statement", Node);
+
+C.emitReport(std::move(Report));
+  }
+};
+
+} // end of anonymous namespace
+
+void ento::registerReportStmts(CheckerManager &mgr) {
+  mgr.registerChecker();
+}
+
+bool ento::shouldRegisterReportStmts(const LangOptions &LO) {
+  return true;
+}
Index: docs/analyzer/developer-docs/DebugChecks.rst
===
--- docs/analyzer/developer-docs/DebugChecks.rst
+++ docs/analyzer/developer-docs/DebugChecks.rst
@@ -285,3 +285,10 @@
 statistics within the analyzer engine. Note the Stats checker (which produces at
 least one bug report per function) may actually change the values reported by
 -analyzer-stats.
+
+Output testing checkers
+===
+
+- debug.ReportStmts reports a warning at **every** statement, making it a very
+  useful tool for testing thoroughly bug report construction and output
+  emission.
Index: include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1019,6 +1019,10 @@
   HelpText<"View Exploded Graphs using GraphViz">,
   Documentation;
 
+def ReportStmts : Checker<"ReportStmts">,
+  HelpText<"Emits a warning for every statement.">,
+  Documentation;
+
 

[PATCH] D59300: [clangd] Tune the fuzzy-matching algorithm

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:74
 static bool isAwful(int S) { return S < AwfulScore / 2; }
-static constexpr int PerfectBonus = 3; // Perfect per-pattern-char score.
+static constexpr int PerfectBonus = 4; // Perfect per-pattern-char score.
 

could you explain this change?



Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:270
 int FuzzyMatcher::skipPenalty(int W, Action Last) const {
-  int S = 0;
+  if (W == 0) // Skipping the first character.
+return 3;

If the word is "_something", do we still want to penalize skipping first char?



Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:275
+  // match.
+  return 0;
 }

iiuc, there is no penalty by default because consecutive matches will gain 
bonus. This is not trivial here. Maybe add a comment? Or consider apply penalty 
for non-consecutive matches and no bonus for consecutive ones.



Comment at: clang-tools-extra/clangd/FuzzyMatch.cpp:285
+  if (Pat[P] == Word[W] ||
+  (WordRole[W] == Head && (IsPatSingleCase || PatRole[P] == Head)))
 ++S;

could you explain the intention of this change? Is it relevant to other changes 
in the patch? The original looks reasonable to me. 



Comment at: clang-tools-extra/unittests/clangd/FuzzyMatchTests.cpp:248
   EXPECT_THAT("foo", ranks("[foo]", "[Foo]"));
   EXPECT_THAT("onMes",
-  ranks("[onMes]sage", "[onmes]sage", "[on]This[M]ega[Es]capes"));

could you add a case for "onmes" pattern?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59300



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

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

IIRC, last time I looked only 4 statement/expression classes currently have 
some abbreviation defined. It would probably be useful to have someone go 
systematically through the list of classes and fix this.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214



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

[PATCH] D59196: [NFC][clang][PCH] ASTStmtReader::VisitStmt(): fixup faulty assert.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

I am not an expert in the serialization code (just did some modifications), but 
this seems reasonable to me.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59196



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

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D59214#1429384 , @riccibruno wrote:

> IIRC, last time I looked only 4 statement/expression classes currently have 
> some abbreviation defined.


Yep, that is what i'm seeing in this diff.

In D59214#1429384 , @riccibruno wrote:

> It would probably be useful to have someone go systematically through the 
> list of classes and fix this.


Yeah, i suspect that might be **really** beneficial.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214



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

[PATCH] D59254: [RFC] Implementation of Clang randstruct

[JF Bastien via Phabricator via cfe-commits]

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

I find it easier to understand the code by looking at the tests. When you add 
tests, please make sure you test for:

- Alignment attribute
- Packed attribute
- No unique address attribute
- Bit-fields
- Zero-width bit-field
- Zero or unsized array at end of struct
- C++ inheritance with vtables
- C++ virtual inheritance
- Anonymous unions (and the anonymous struct extension)
- Types with common initial sequence 

I assume you only change field location, not their constructor's call order? 
i.e. `class S { std::string a, b; };` always constructs `a` before `b`, but 
might reorder them.

There are cases where a developer could observe the difference in field order. 
For example, `bit_cast` or `memcpy` of a type which got reordered. What are the 
gotchas? Can you diagnose them?




Comment at: clang/include/clang/AST/RandstructSeed.h:1
+#ifndef RANDSTRUCTSEED_H
+#define RANDSTRUCTSEED_H

This file needs the standard license comment.



Comment at: clang/include/clang/AST/RandstructSeed.h:6
+extern std::string RandstructSeed;
+extern bool RandstructAutoSelect;
+}

Returning a string is weird, and so it `extern` stuff. Is randomness a property 
of something in particular? Seems like `Module` or something similar might be 
the right place.



Comment at: clang/include/clang/AST/RecordFieldReorganizer.h:54
+  std::seed_seq Seq;
+  std::default_random_engine rng;
+};

vlad.tsyrklevich wrote:
> timpugh wrote:
> > connorkuehl wrote:
> > > pcc wrote:
> > > > I don't think we can use `default_random_engine` for this because the 
> > > > behaviour would need to be consistent between C++ standard library 
> > > > implementations, and the behaviour of `default_random_engine` is 
> > > > implementation defined. Similarly, I don't think that we can use 
> > > > `std::shuffle` (see note in 
> > > > https://en.cppreference.com/w/cpp/algorithm/random_shuffle ).
> > > Sure thing, we'll begin investigating alternatives.
> > @pcc  if we were to swap `default_random_engine` for a pre-defined random 
> > generator such as `mt19937_64` would this suffice? It is included in the 
> > random number library.
> > 
> > https://en.cppreference.com/w/cpp/numeric/random
> In that case the random numbers would be deterministic; however, std::shuffle 
> would still vary by implementation (as mentioned in the note Peter linked 
> to.) A quick search didn't reveal a deterministic shuffle in the LLVM code so 
> you may have to implement one.
`Module::createRNG` was created to support randomization of things. You should 
look at the discussion that led to its addition: in parts it tries to offer 
stable output when it can, and the idea was that you could inject a seed if you 
wanted to.

Unfortunately the other randomization patches weren't committed, but they're 
also on Phabricator.



Comment at: clang/include/clang/Basic/AttrDocs.td:4161
+char *c;
+  }__attribute__((randomize_layout)) __attribute__((no_randomize_layout));
+}];

I'm not sure you need to document what happens when both are present. If people 
do it the message they get should be sufficient.



Comment at: clang/lib/AST/RecordFieldReorganizer.cpp:95
+// FIXME: Is there a way to detect this? (i.e. on 32bit system vs 64?)
+const size_t CACHE_LINE = 64;
+

https://en.cppreference.com/w/cpp/thread/hardware_destructive_interference_size 
will provide this. I have an RFC on how to implement it, haven't gotten to it. 
Just leave a FIXME. 64 is the right value, we all know ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59254



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

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In D59214#1429400 , @lebedev.ri wrote:

> In D59214#1429384 , @riccibruno 
> wrote:
>
> > IIRC, last time I looked only 4 statement/expression classes currently have 
> > some abbreviation defined.
>
>
> Yep, that is what i'm seeing in this diff.
>
> In D59214#1429384 , @riccibruno 
> wrote:
>
> > It would probably be useful to have someone go systematically through the 
> > list of classes and fix this.
>
>
> Yeah, i suspect that might be **really** beneficial.


Especially given that more people are going to use modules now that they are in 
the process of being standardized.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214



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

[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

[Nick Renieris via Phabricator via cfe-commits]

VelocityRa marked 11 inline comments as done.
VelocityRa added a comment.

Waiting for further feedback before pushing an update.




Comment at: lib/Format/WhitespaceManager.cpp:436
+static void
+AlignTokenSequenceAll(unsigned Start, unsigned End, unsigned Column,
+  F &&Matches,

klimek wrote:
> I don't think the 'All' postfix in the name is helpful. What are you trying 
> to say with that name?
I'm not particularly fond of `All` either, suggestions welcome.

As the comment above explains, `All` refers to the fact that it operates on all 
tokens, instead of being limited to certain cases like `AlignTokenSequence` is. 
Maybe I should name //this// one `AlignTokenSequence` and the other one 
`AlignTokenSequenceOuterScope`, or something.



Comment at: lib/Format/WhitespaceManager.cpp:437
+AlignTokenSequenceAll(unsigned Start, unsigned End, unsigned Column,
+  F &&Matches,
+  SmallVector &Changes) {

klimek wrote:
> Why an rvalue ref? Also, this is used only once, so why make this a template?
It's an rvalue ref, because that's also the case for `AlignTokenSequence` 
(wasn't sure why, so I left it as is).
It's used only once, but the function is more generic that way, no? That's the 
point of its generic name.
Tell me if I should change it.



Comment at: lib/Format/WhitespaceManager.cpp:442
+
+  for (unsigned i = Start; i != End; ++i) {
+if (Changes[i].NewlinesBefore > 0) {

klimek wrote:
> llvm style: use an upper case I for index vars.
Ok, I assume your style changed because this is copied from 
`AlignTokenSequence`.



Comment at: lib/Format/WhitespaceManager.cpp:473
+
+  // Line number of the start and the end of the current token sequence.
+  unsigned StartOfSequence = 0;

klimek wrote:
> These are indices into Matches, not line numbers, right?
Correct. My bad.



Comment at: lib/Format/WhitespaceManager.cpp:500
+if (Changes[i].NewlinesBefore != 0) {
+  EndOfSequence = i;
+  // If there is a blank line, or if the last line didn't contain any

klimek wrote:
> Why set EndOfSequence outside the if below?
It's from `AlignTokens`. I think it's because due to some of the loop logic, it 
ends up not checking up to the point of the the last token.
Without setting this and calling `AlignCurrentSequence()` once more at the end, 
the last line of a macro group does not get properly aligned, the tests fail.



Comment at: lib/Format/WhitespaceManager.cpp:512
+
+// If there is more than one matching token per line, end the sequence.
+if (FoundMatchOnLine)

klimek wrote:
> What's the reason for this?
I don't remember, but it was unnecessary apparently. The tests pass without 
this check.


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

https://reviews.llvm.org/D28462



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

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D59214#1429422 , @riccibruno wrote:

> In D59214#1429400 , @lebedev.ri 
> wrote:
>
> > In D59214#1429384 , @riccibruno 
> > wrote:
> >
> > > IIRC, last time I looked only 4 statement/expression classes currently 
> > > have some abbreviation defined.
> >
> >
> > Yep, that is what i'm seeing in this diff.
> >
> > In D59214#1429384 , @riccibruno 
> > wrote:
> >
> > > It would probably be useful to have someone go systematically through the 
> > > list of classes and fix this.
> >
> >
> > Yeah, i suspect that might be **really** beneficial.
>
>
> Especially given that more people are going to use modules now that they are 
> in the process of being standardized.


Ah, good point, filed https://bugs.llvm.org/show_bug.cgi?id=41072


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214



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

[PATCH] D59354: [clangd] Print arguments in template specializations

[Kadir Cetinkaya via Phabricator via cfe-commits]

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



Comment at: clang/lib/AST/TypePrinter.cpp:1646
+break;
+  case TemplateArgument::ArgKind::Type:
+A.getTypeSourceInfo()->getType().print(OS, PP);

ilya-biryukov wrote:
> Maybe simplify the switch to:
> ```
> if (A.getKind() == TemplateArgument::ArgKind::Type) {
> A.getTypeSourceInfo()->getType().print(OS, PP);
> return;
> }
> A.getArgument().print(PP, OS);
> ```
> 
It was rather to catch any changes in the ArgKind at compile-time, still can do 
if you think this should not cause any problems


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59354



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