[PATCH] D49074: [Analyzer] [WIP] Basic support for multiplication and division in the constraint manager

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

baloghadamsoftware added a comment.

In https://reviews.llvm.org/D49074#1156110, @george.karpenkov wrote:

> @baloghadamsoftware @dkrupp @xazax.hun Interesting. What do you think about 
> instead using Z3 cross-check functionality recently added, to solve this and 
> all other similar problems instead?


Using Z3 generally is 20 to 30 times slower according to our measurements. This 
means that a 8 hours analysis (one night, this is quite common) would take 7-10 
days. Our customers would never accept that. As far as I know the purpose of 
the cross-check functionality is to execute Z3 only for the cases which are too 
complex for the range-based constraint manager. If we regard simple 
multiplications and divisions as too complex cases we are afraid that it still 
takes too long. That is why we try to reduce the set of too complex cases by 
improving the range-based constraint manager.

As we discussed with @dcoughlin at the Euro LLVM 2018 the community should 
consider a new constraint manager, which is in between the range-based one and 
the Z3 both feature and performance wise. However, this is something long term, 
until then we should improve the existing one  as far as reasonable. I am sure 
that multiplicative operations can be solved similarly to the additive ones.




Comment at: test/Analysis/constraint_manager_scale.c:78
+  assert(x * 2 < 8);
+  clang_analyzer_eval(x < 4); // expected-warning{{TRUE}}
+  clang_analyzer_eval(x < 2); // expected-warning{{UNKNOWN}}

NoQ wrote:
> If `int` is 32-bit and `x` equal to 2³¹ - 1, we have `x * 2` equal to `-2` 
> which is less than 8, while `x` is greater than 4.
Thank you for pointing me out this! I uploaded this patch as WIP since I was 
sure I missed something because of modular arithmetic. I think this can be 
solved, even if not so easily. Maybe we should restrict the first version to 
division only?


https://reviews.llvm.org/D49074



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

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D48903#1157600, @simark wrote:

> Thanks.  I have seen no failures in `check-clang` and `check-clang-tools`, so 
> I will push it.


LG! We can always revert the change is anything breaks...


Repository:
  rC Clang

https://reviews.llvm.org/D48903



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

[PATCH] D49074: [Analyzer] [WIP] Basic support for multiplication and division in the constraint manager

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

baloghadamsoftware added a comment.

In https://reviews.llvm.org/D49074#1156609, @george.karpenkov wrote:

> The overall point is that writing this kind of code is *extremely* 
> error-prone.


Every modifications on the core is extremely error-prone. That is why we must 
cross check it and only apply it if it is 100% mathematically correct. This 
patch is only WIP for now. Freezing the core in its current state helps us to 
not introduce new errors but also prevents improving the current functionality 
which is not yet sufficient for many of our customers. What I did is not 
hacking, multiplicative operations can be implemented a similar way to additive 
ones, they are just a bit more complex.

> We are actually considering going in a different direction and doing a 
> rollback for the previous rearrangement patches due to some issues.

What issues could it cause since it is guarded by an option? As for the 
rearrangement of the additive operations it was @NoQ's idea but I agree there. 
Actually I feel this rearrangement patch a bit of hacking, but I also agree 
with @NoQ that it may be useful for other checkers as well. That was the reason 
to put it into `SValBuilder` instead of the checker itself. As you may be aware 
of it I originally did the rearrangement inside the checker.

> Could you see whether Z3 visitor would meet your needs?

We will look at it, but please read my previous answer.


https://reviews.llvm.org/D49074



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

[PATCH] D33537: [clang-tidy] Exception Escape Checker

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

baloghadamsoftware added a comment.

Any news regarding this?


https://reviews.llvm.org/D33537



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

[PATCH] D49158: [clang-tidy] Fixing segfault when there's no IdentifierInfo

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.cpp:44
   bool &isInterface) const {
-  StringRef Name = Node->getIdentifier()->getName();
-  llvm::StringMapConstIterator Pair = InterfaceMap.find(Name);
-  if (Pair == InterfaceMap.end())
-return false;
-  isInterface = Pair->second;
-  return true;
+  if (!Node) return false;
+  if (const auto *Id = Node->getIdentifier()) {

Please move then `return false` onto the next line.


https://reviews.llvm.org/D49158



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

[PATCH] D49158: [clang-tidy] Fixing segfault when there's no IdentifierInfo

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Is there a way to add a test, that would trigger the old segfault and show that 
it does not happen anymore with this fix?


https://reviews.llvm.org/D49158



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

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

[Carlos Alberto Enciso via Phabricator via cfe-commits]

CarlosAlbertoEnciso added a comment.

@rsmith Is there anything I can add to this patch?

The review

https://reviews.llvm.org/D44826

is already approved and it is dependent on this patch being reviewed.

Thanks very much


https://reviews.llvm.org/D46190



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

[PATCH] D49074: [Analyzer] [WIP] Basic support for multiplication and division in the constraint manager

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

baloghadamsoftware added a comment.

In https://reviews.llvm.org/D49074#1156110, @george.karpenkov wrote:

> @baloghadamsoftware @dkrupp @xazax.hun Interesting. What do you think about 
> instead using Z3 cross-check functionality recently added, to solve this and 
> all other similar problems instead?


As far as I know the purpose of the cross-check functionality is to execute Z3 
only on actual bug report paths. This may remove false-positives (like the 
example above), but we surely cannot find new errors where multiplicative 
operations are used.

As we discussed with @dcoughlin at the Euro LLVM 2018 the community should 
consider a new constraint manager, which is in between the range-based one and 
the Z3 both feature and performance wise. However, this is something long term, 
until then we should improve the existing one  as far as reasonable. I am sure 
that multiplicative operations can be solved similarly to the additive ones.


https://reviews.llvm.org/D49074



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

r336776 - [AST] Structural equivalence of methods

[Balazs Keri via cfe-commits]

Author: balazske
Date: Wed Jul 11 02:37:24 2018
New Revision: 336776

URL: http://llvm.org/viewvc/llvm-project?rev=336776&view=rev
Log:
[AST] Structural equivalence of methods

Summary:
Added structural equivalence check for C++ methods.
Improved structural equivalence tests.
Added related ASTImporter tests.

Reviewers: a.sidorin, szepet, xazax.hun, martong, a_sidorin

Reviewed By: martong, a_sidorin

Subscribers: a_sidorin, rnkovacs, cfe-commits

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

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

Modified: cfe/trunk/lib/AST/ASTImporter.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTImporter.cpp?rev=336776&r1=336775&r2=336776&view=diff
==
--- cfe/trunk/lib/AST/ASTImporter.cpp (original)
+++ cfe/trunk/lib/AST/ASTImporter.cpp Wed Jul 11 02:37:24 2018
@@ -230,6 +230,7 @@ namespace clang {
 bool IsStructuralMatch(EnumConstantDecl *FromEC, EnumConstantDecl *ToEC);
 bool IsStructuralMatch(FunctionTemplateDecl *From,
FunctionTemplateDecl *To);
+bool IsStructuralMatch(FunctionDecl *From, FunctionDecl *To);
 bool IsStructuralMatch(ClassTemplateDecl *From, ClassTemplateDecl *To);
 bool IsStructuralMatch(VarTemplateDecl *From, VarTemplateDecl *To);
 Decl *VisitDecl(Decl *D);
@@ -1525,6 +1526,13 @@ bool ASTNodeImporter::IsStructuralMatch(
   return Ctx.IsStructurallyEquivalent(From, To);
 }
 
+bool ASTNodeImporter::IsStructuralMatch(FunctionDecl *From, FunctionDecl *To) {
+  StructuralEquivalenceContext Ctx(
+  Importer.getFromContext(), Importer.getToContext(),
+  Importer.getNonEquivalentDecls(), false, false);
+  return Ctx.IsStructurallyEquivalent(From, To);
+}
+
 bool ASTNodeImporter::IsStructuralMatch(EnumConstantDecl *FromEC,
 EnumConstantDecl *ToEC) {
   const llvm::APSInt &FromVal = FromEC->getInitVal();
@@ -2433,13 +2441,15 @@ Decl *ASTNodeImporter::VisitFunctionDecl
   if (auto *FoundFunction = dyn_cast(FoundDecl)) {
 if (FoundFunction->hasExternalFormalLinkage() &&
 D->hasExternalFormalLinkage()) {
-  if (Importer.IsStructurallyEquivalent(D->getType(), 
-FoundFunction->getType())) {
-  if (D->doesThisDeclarationHaveABody() &&
-  FoundFunction->hasBody())
-return Importer.Imported(D, FoundFunction);
-  FoundByLookup = FoundFunction;
-  break;
+  if (IsStructuralMatch(D, FoundFunction)) {
+const FunctionDecl *Definition = nullptr;
+if (D->doesThisDeclarationHaveABody() &&
+FoundFunction->hasBody(Definition)) {
+  return Importer.Imported(
+  D, const_cast(Definition));
+}
+FoundByLookup = FoundFunction;
+break;
   }
 
   // FIXME: Check for overloading more carefully, e.g., by boosting

Modified: cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp?rev=336776&r1=336775&r2=336776&view=diff
==
--- cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp (original)
+++ cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp Wed Jul 11 02:37:24 2018
@@ -250,6 +250,9 @@ static bool IsStructurallyEquivalent(Str
   if (T1.isNull() || T2.isNull())
 return T1.isNull() && T2.isNull();
 
+  QualType OrigT1 = T1;
+  QualType OrigT2 = T2;
+
   if (!Context.StrictTypeSpelling) {
 // We aren't being strict about token-to-token equivalence of types,
 // so map down to the canonical type.
@@ -422,6 +425,7 @@ static bool IsStructurallyEquivalent(Str
   case Type::FunctionProto: {
 const auto *Proto1 = cast(T1);
 const auto *Proto2 = cast(T2);
+
 if (Proto1->getNumParams() != Proto2->getNumParams())
   return false;
 for (unsigned I = 0, N = Proto1->getNumParams(); I != N; ++I) {
@@ -431,23 +435,33 @@ static bool IsStructurallyEquivalent(Str
 }
 if (Proto1->isVariadic() != Proto2->isVariadic())
   return false;
-if (Proto1->getExceptionSpecType() != Proto2->getExceptionSpecType())
+
+if (Proto1->getTypeQuals() != Proto2->getTypeQuals())
+  return false;
+
+// Check exceptions, this information is lost in canonical type.
+const auto *OrigProto1 =
+cast(OrigT1.getDesugaredType(Context.FromCtx));
+const auto *OrigProto2 =
+cast(OrigT2.getDesugaredType(Context.ToCtx));
+auto Spec1 = OrigProto1->getExceptionSpecType();
+auto Spec2 = OrigProto2->getExceptionSpecType();
+
+if (Spec1 != Spec2)
   return false;
-if (Pr

[PATCH] D48628: [AST] Structural equivalence of methods

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

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL336776: [AST] Structural equivalence of methods (authored by 
balazske, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D48628?vs=154175&id=154951#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D48628

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

Index: cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
===
--- cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
+++ cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
@@ -250,6 +250,9 @@
   if (T1.isNull() || T2.isNull())
 return T1.isNull() && T2.isNull();
 
+  QualType OrigT1 = T1;
+  QualType OrigT2 = T2;
+
   if (!Context.StrictTypeSpelling) {
 // We aren't being strict about token-to-token equivalence of types,
 // so map down to the canonical type.
@@ -422,6 +425,7 @@
   case Type::FunctionProto: {
 const auto *Proto1 = cast(T1);
 const auto *Proto2 = cast(T2);
+
 if (Proto1->getNumParams() != Proto2->getNumParams())
   return false;
 for (unsigned I = 0, N = Proto1->getNumParams(); I != N; ++I) {
@@ -431,23 +435,33 @@
 }
 if (Proto1->isVariadic() != Proto2->isVariadic())
   return false;
-if (Proto1->getExceptionSpecType() != Proto2->getExceptionSpecType())
+
+if (Proto1->getTypeQuals() != Proto2->getTypeQuals())
+  return false;
+
+// Check exceptions, this information is lost in canonical type.
+const auto *OrigProto1 =
+cast(OrigT1.getDesugaredType(Context.FromCtx));
+const auto *OrigProto2 =
+cast(OrigT2.getDesugaredType(Context.ToCtx));
+auto Spec1 = OrigProto1->getExceptionSpecType();
+auto Spec2 = OrigProto2->getExceptionSpecType();
+
+if (Spec1 != Spec2)
   return false;
-if (Proto1->getExceptionSpecType() == EST_Dynamic) {
-  if (Proto1->getNumExceptions() != Proto2->getNumExceptions())
+if (Spec1 == EST_Dynamic) {
+  if (OrigProto1->getNumExceptions() != OrigProto2->getNumExceptions())
 return false;
-  for (unsigned I = 0, N = Proto1->getNumExceptions(); I != N; ++I) {
-if (!IsStructurallyEquivalent(Context, Proto1->getExceptionType(I),
-  Proto2->getExceptionType(I)))
+  for (unsigned I = 0, N = OrigProto1->getNumExceptions(); I != N; ++I) {
+if (!IsStructurallyEquivalent(Context, OrigProto1->getExceptionType(I),
+  OrigProto2->getExceptionType(I)))
   return false;
   }
-} else if (isComputedNoexcept(Proto1->getExceptionSpecType())) {
-  if (!IsStructurallyEquivalent(Context, Proto1->getNoexceptExpr(),
-Proto2->getNoexceptExpr()))
+} else if (isComputedNoexcept(Spec1)) {
+  if (!IsStructurallyEquivalent(Context, OrigProto1->getNoexceptExpr(),
+OrigProto2->getNoexceptExpr()))
 return false;
 }
-if (Proto1->getTypeQuals() != Proto2->getTypeQuals())
-  return false;
 
 // Fall through to check the bits common with FunctionNoProtoType.
 LLVM_FALLTHROUGH;
@@ -830,6 +844,56 @@
   return true;
 }
 
+/// Determine structural equivalence of two methodss.
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
+ CXXMethodDecl *Method1,
+ CXXMethodDecl *Method2) {
+  bool PropertiesEqual =
+  Method1->getDeclKind() == Method2->getDeclKind() &&
+  Method1->getRefQualifier() == Method2->getRefQualifier() &&
+  Method1->getAccess() == Method2->getAccess() &&
+  Method1->getOverloadedOperator() == Method2->getOverloadedOperator() &&
+  Method1->isStatic() == Method2->isStatic() &&
+  Method1->isConst() == Method2->isConst() &&
+  Method1->isVolatile() == Method2->isVolatile() &&
+  Method1->isVirtual() == Method2->isVirtual() &&
+  Method1->isPure() == Method2->isPure() &&
+  Method1->isDefaulted() == Method2->isDefaulted() &&
+  Method1->isDeleted() == Method2->isDeleted();
+  if (!PropertiesEqual)
+return false;
+  // FIXME: Check for 'final'.
+
+  if (auto *Constructor1 = dyn_cast(Method1)) {
+auto *Constructor2 = cast(Method2);
+if (Constructor1->isExplicit() != Constructor2->isExplicit())
+  return false;
+  }
+
+  if (auto *Conversion1 = dyn_cast(Method1)) {
+auto *Conversion2 = cast(Method2);
+if (Conversion1->isExplicit() != Conversion2->isExplicit())
+  return false;
+if (!IsStructurallyEquivalent(Context, Conversion1->getConversionType(),
+  Conversion2->getCon

[PATCH] D47946: [ASTImporter] Fix infinite recursion on function import with struct definition in parameters

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin accepted this revision.
a.sidorin added a comment.

Hello Zoltán,

Sorry for the delay. I think the patch is fine, just some minor nits inline.




Comment at: unittests/AST/ASTImporterTest.cpp:234
+assert(ToAST);
+createVirtualFileIfNeeded(ToAST.get(), It->FileName, It->Code);
+return &*It;

Can we move the side effect into import()?



Comment at: unittests/AST/ASTImporterTest.cpp:1049
+  ImportType(FromVar->getType().getCanonicalType(), FromVar, Lang_C);
+  ASSERT_FALSE(ToType.isNull());
+}

EXPECT_FALSE?


https://reviews.llvm.org/D47946



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

[PATCH] D49012: [clangd] Uprank delcarations when "using q::name" is present in the main file

[Kirill Bobyrev via Phabricator via cfe-commits]

omtcyfz updated this revision to Diff 154956.
omtcyfz marked 3 inline comments as done.
omtcyfz added a comment.

Address few comments, add test for macro definition, slightly reformat 
`QualityTests.cpp`


https://reviews.llvm.org/D49012

Files:
  clang-tools-extra/clangd/Quality.cpp
  clang-tools-extra/unittests/clangd/QualityTests.cpp
  clang/include/clang/Sema/CodeCompleteConsumer.h
  clang/lib/Sema/SemaCodeComplete.cpp

Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -859,12 +859,12 @@
   }
 
   // Look through using declarations.
-  if (const UsingShadowDecl *Using =
-  dyn_cast(R.Declaration)) {
-MaybeAddResult(Result(Using->getTargetDecl(),
-  getBasePriority(Using->getTargetDecl()),
-  R.Qualifier),
-   CurContext);
+  if (const UsingShadowDecl *Using = dyn_cast(R.Declaration)) {
+CodeCompletionResult Result(Using->getTargetDecl(),
+getBasePriority(Using->getTargetDecl()),
+R.Qualifier);
+Result.ShadowDecl = Using;
+MaybeAddResult(Result, CurContext);
 return;
   }
 
@@ -977,10 +977,11 @@
 
   // Look through using declarations.
   if (const UsingShadowDecl *Using = dyn_cast(R.Declaration)) {
-AddResult(Result(Using->getTargetDecl(),
- getBasePriority(Using->getTargetDecl()),
- R.Qualifier),
-  CurContext, Hiding);
+CodeCompletionResult Result(Using->getTargetDecl(),
+getBasePriority(Using->getTargetDecl()),
+R.Qualifier);
+Result.ShadowDecl = Using;
+AddResult(Result, CurContext, Hiding);
 return;
   }
 
@@ -1004,10 +1005,10 @@
   if (AsNestedNameSpecifier) {
 R.StartsNestedNameSpecifier = true;
 R.Priority = CCP_NestedNameSpecifier;
-  }
-  else if (Filter == &ResultBuilder::IsMember && !R.Qualifier && InBaseClass &&
-   isa(R.Declaration->getDeclContext()
-  ->getRedeclContext()))
+  } else if (Filter == &ResultBuilder::IsMember && !R.Qualifier &&
+ InBaseClass &&
+ isa(
+ R.Declaration->getDeclContext()->getRedeclContext()))
 R.QualifierIsInformative = true;
 
   // If this result is supposed to have an informative qualifier, add one.
Index: clang/include/clang/Sema/CodeCompleteConsumer.h
===
--- clang/include/clang/Sema/CodeCompleteConsumer.h
+++ clang/include/clang/Sema/CodeCompleteConsumer.h
@@ -45,8 +45,9 @@
 class NamedDecl;
 class NestedNameSpecifier;
 class Preprocessor;
-class Sema;
 class RawComment;
+class Sema;
+class UsingShadowDecl;
 
 /// Default priority values for code-completion results based
 /// on their kind.
@@ -836,6 +837,12 @@
   /// informative rather than required.
   NestedNameSpecifier *Qualifier = nullptr;
 
+  /// If this Decl was unshadowed by using declaration, this can store a
+  /// pointer to the UsingShadowDecl which was used in the unshadowing process.
+  /// This information can be used to uprank CodeCompletionResults / which have
+  /// corresponding `using decl::qualified::name;` nearby.
+  const UsingShadowDecl *ShadowDecl = nullptr;
+
   /// Build a result that refers to a declaration.
   CodeCompletionResult(const NamedDecl *Declaration, unsigned Priority,
NestedNameSpecifier *Qualifier = nullptr,
@@ -847,7 +854,7 @@
 QualifierIsInformative(QualifierIsInformative),
 StartsNestedNameSpecifier(false), AllParametersAreInformative(false),
 DeclaringEntity(false), Qualifier(Qualifier) {
-//FIXME: Add assert to check FixIts range requirements.
+// FIXME: Add assert to check FixIts range requirements.
 computeCursorKindAndAvailability(Accessible);
   }
 
Index: clang-tools-extra/unittests/clangd/QualityTests.cpp
===
--- clang-tools-extra/unittests/clangd/QualityTests.cpp
+++ clang-tools-extra/unittests/clangd/QualityTests.cpp
@@ -77,18 +77,31 @@
 TEST(QualityTests, SymbolRelevanceSignalExtraction) {
   TestTU Test;
   Test.HeaderCode = R"cpp(
-int header();
-int header_main();
-)cpp";
+  int header();
+  int header_main();
+
+  namespace hdr { class Bar {}; } // namespace hdr
+
+  #define DEFINE_FLAG(X) \
+  namespace flags { \
+  int FLAGS_##X; \
+  } \
+
+  DEFINE_FLAG(FOO)
+  )cpp";
   Test.Code = R"cpp(
-int ::header_main() {}
-int main();
+  using hdr::Bar;
 
-[[deprecated]]
-int deprecated() { return 0; }
+  using flags::FLAGS_FOO;
+
+  int ::header_main() {}
+  int main();
+
+  [[deprecated]]
+  int deprecated() { return 0; }
 
-namespace { struct X { void y() { int z; } }; }
-struct S{}
+  namesp

[PATCH] D49008: [clangd] Upgrade logging facilities with levels and formatv.

[Sam McCall via Phabricator via cfe-commits]

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

Add workaround for formatv+error issues.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49008

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdServer.cpp
  clangd/ClangdUnit.cpp
  clangd/CodeComplete.cpp
  clangd/Compiler.cpp
  clangd/Diagnostics.cpp
  clangd/FileDistance.cpp
  clangd/FindSymbols.cpp
  clangd/GlobalCompilationDatabase.cpp
  clangd/JSONRPCDispatcher.cpp
  clangd/JSONRPCDispatcher.h
  clangd/Logger.cpp
  clangd/Logger.h
  clangd/Protocol.cpp
  clangd/ProtocolHandlers.cpp
  clangd/SourceCode.cpp
  clangd/TUScheduler.cpp
  clangd/XRefs.cpp
  clangd/index/SymbolCollector.cpp
  clangd/tool/ClangdMain.cpp

Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -98,6 +98,14 @@
 PrettyPrint("pretty", llvm::cl::desc("Pretty-print JSON output"),
 llvm::cl::init(false));
 
+static llvm::cl::opt LogLevel(
+"log", llvm::cl::desc("Verbosity of log messages written to stderr"),
+llvm::cl::values(clEnumValN(Logger::Error, "error", "Error messages only"),
+ clEnumValN(Logger::Info, "info",
+"High level execution tracing"),
+ clEnumValN(Logger::Debug, "verbose", "Low level details")),
+llvm::cl::init(Logger::Info));
+
 static llvm::cl::opt Test(
 "lit-test",
 llvm::cl::desc(
@@ -223,7 +231,7 @@
   if (Tracer)
 TracingSession.emplace(*Tracer);
 
-  JSONOutput Out(llvm::outs(), llvm::errs(),
+  JSONOutput Out(llvm::outs(), llvm::errs(), LogLevel,
  InputMirrorStream ? InputMirrorStream.getPointer() : nullptr,
  PrettyPrint);
 
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -52,7 +52,7 @@
   if (std::error_code EC =
   SM.getFileManager().getVirtualFileSystem()->makeAbsolute(
   AbsolutePath))
-log("Warning: could not make absolute file: " + EC.message());
+log("Warning: could not make absolute file: {0}", EC.message());
   if (llvm::sys::path::is_absolute(AbsolutePath)) {
 // Handle the symbolic link path case where the current working directory
 // (getCurrentWorkingDirectory) is a symlink./ We always want to the real
@@ -86,8 +86,7 @@
   return U->toString();
 ErrMsg += llvm::toString(U.takeError()) + "\n";
   }
-  log(llvm::Twine("Failed to create an URI for file ") + AbsolutePath + ": " +
-  ErrMsg);
+  log("Failed to create an URI for file {0}: {1}", AbsolutePath, ErrMsg);
   return llvm::None;
 }
 
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -46,12 +46,12 @@
 return llvm::None;
   auto Uri = URI::parse(Loc.FileURI);
   if (!Uri) {
-log("Could not parse URI: " + Loc.FileURI);
+log("Could not parse URI: {0}", Loc.FileURI);
 return llvm::None;
   }
   auto Path = URI::resolve(*Uri, HintPath);
   if (!Path) {
-log("Could not resolve URI: " + Loc.FileURI);
+log("Could not resolve URI: {0}", Loc.FileURI);
 return llvm::None;
   }
   Location LSPLoc;
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -329,14 +329,14 @@
 // Remove the old AST if it's still in cache.
 IdleASTs.take(this);
 
-log("Updating file " + FileName + " with command [" +
-Inputs.CompileCommand.Directory + "] " +
+log("Updating file {0} with command [{1}] {2}", FileName,
+Inputs.CompileCommand.Directory,
 llvm::join(Inputs.CompileCommand.CommandLine, " "));
 // Rebuild the preamble and the AST.
 std::unique_ptr Invocation =
 buildCompilerInvocation(Inputs);
 if (!Invocation) {
-  log("Could not build CompilerInvocation for file " + FileName);
+  elog("Could not build CompilerInvocation for file {0}", FileName);
   // Make sure anyone waiting for the preamble gets notified it could not
   // be built.
   PreambleWasBuilt.notify();
@@ -628,8 +628,8 @@
 void TUScheduler::remove(PathRef File) {
   bool Removed = Files.erase(File);
   if (!Removed)
-log("Trying to remove file from TUScheduler that is not tracked. File:" +
-File);
+elog("Trying to remove file from TUScheduler that is not tracked: {0}",
+ File);
 }
 
 void TUScheduler::runWithAST(
Index: clangd/SourceCode.cpp
===
--- clangd/SourceCode.cpp
+++ clangd/SourceCode.cpp
@@ -192,7 +192,7 @@
 FilePath = F->getName();
   if (!llvm::sys::path::is_absolute(FilePath)) {
 if (!SourceMgr.getFileManager().makeAbsolutePath(FilePath)) {
-  log("Could not turn relativ

[clang-tools-extra] r336785 - [clangd] Upgrade logging facilities with levels and formatv.

[Sam McCall via cfe-commits]

Author: sammccall
Date: Wed Jul 11 03:35:11 2018
New Revision: 336785

URL: http://llvm.org/viewvc/llvm-project?rev=336785&view=rev
Log:
[clangd] Upgrade logging facilities with levels and formatv.

Summary:
log() is split into four functions:
 - elog()/log()/vlog() have different severity levels, allowing filtering
 - dlog() is a lazy macro which uses LLVM_DEBUG - it logs to the logger, but
   conditionally based on -debug-only flag and is omitted in release builds

All logging functions use formatv-style format strings now, e.g:
  log("Could not resolve URI {0}: {1}", URI, Result.takeError());

Existing log sites have been split between elog/log/vlog by best guess.

This includes a workaround for passing Error to formatv that can be
simplified when D49170 or similar lands.

Subscribers: ilya-biryukov, javed.absar, ioeric, MaskRay, jkorous, cfe-commits

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

Modified:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdUnit.cpp
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/Compiler.cpp
clang-tools-extra/trunk/clangd/Diagnostics.cpp
clang-tools-extra/trunk/clangd/FileDistance.cpp
clang-tools-extra/trunk/clangd/FindSymbols.cpp
clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp
clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h
clang-tools-extra/trunk/clangd/Logger.cpp
clang-tools-extra/trunk/clangd/Logger.h
clang-tools-extra/trunk/clangd/Protocol.cpp
clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
clang-tools-extra/trunk/clangd/SourceCode.cpp
clang-tools-extra/trunk/clangd/TUScheduler.cpp
clang-tools-extra/trunk/clangd/XRefs.cpp
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp

Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=336785&r1=336784&r2=336785&view=diff
==
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Wed Jul 11 03:35:11 2018
@@ -160,7 +160,7 @@ void ClangdLSPServer::onDocumentDidChang
 DraftMgr.removeDraft(File);
 Server.removeDocument(File);
 CDB.invalidate(File);
-log(llvm::toString(Contents.takeError()));
+elog("Failed to update {0}: {1}", File, Contents.takeError());
 return;
   }
 

Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=336785&r1=336784&r2=336785&view=diff
==
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Wed Jul 11 03:35:11 2018
@@ -118,12 +118,12 @@ void ClangdServer::setRootPath(PathRef R
   auto FS = FSProvider.getFileSystem();
   auto Status = FS->status(RootPath);
   if (!Status)
-log("Failed to get status for RootPath " + RootPath + ": " +
-Status.getError().message());
+elog("Failed to get status for RootPath {0}: {1}", RootPath,
+ Status.getError().message());
   else if (Status->isDirectory())
 this->RootPath = RootPath;
   else
-log("The provided RootPath " + RootPath + " is not a directory.");
+elog("The provided RootPath {0} is not a directory.", RootPath);
 }
 
 void ClangdServer::addDocument(PathRef File, StringRef Contents,

Modified: clang-tools-extra/trunk/clangd/ClangdUnit.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdUnit.cpp?rev=336785&r1=336784&r2=336785&view=diff
==
--- clang-tools-extra/trunk/clangd/ClangdUnit.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdUnit.cpp Wed Jul 11 03:35:11 2018
@@ -147,7 +147,7 @@ ParsedAST::Build(std::unique_ptr();
   const FrontendInputFile &MainInput = Clang->getFrontendOpts().Inputs[0];
   if (!Action->BeginSourceFile(*Clang, MainInput)) {
-log("BeginSourceFile() failed when building AST for " +
+log("BeginSourceFile() failed when building AST for {0}",
 MainInput.getFile());
 return llvm::None;
   }
@@ -159,7 +159,7 @@ ParsedAST::Build(std::unique_ptrgetSourceManager(), &Includes));
 
   if (!Action->Execute())
-log("Execute() failed when building AST for " + MainInput.getFile());
+log("Execute() failed when building AST for {0}", MainInput.getFile());
 
   // UnitDiagsConsumer is local, we can not store it in CompilerInstance that
   // has a longer lifetime.
@@ -307,11 +307,11 @@ std::shared_ptr clan
   compileCommandsAreEqual(Inputs.CompileCommand,

[PATCH] D49008: [clangd] Upgrade logging facilities with levels and formatv.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL336785: [clangd] Upgrade logging facilities with levels and 
formatv. (authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D49008?vs=154959&id=154960#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D49008

Files:
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/ClangdServer.cpp
  clang-tools-extra/trunk/clangd/ClangdUnit.cpp
  clang-tools-extra/trunk/clangd/CodeComplete.cpp
  clang-tools-extra/trunk/clangd/Compiler.cpp
  clang-tools-extra/trunk/clangd/Diagnostics.cpp
  clang-tools-extra/trunk/clangd/FileDistance.cpp
  clang-tools-extra/trunk/clangd/FindSymbols.cpp
  clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp
  clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
  clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h
  clang-tools-extra/trunk/clangd/Logger.cpp
  clang-tools-extra/trunk/clangd/Logger.h
  clang-tools-extra/trunk/clangd/Protocol.cpp
  clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
  clang-tools-extra/trunk/clangd/SourceCode.cpp
  clang-tools-extra/trunk/clangd/TUScheduler.cpp
  clang-tools-extra/trunk/clangd/XRefs.cpp
  clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
  clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp

Index: clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
===
--- clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
+++ clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
@@ -33,7 +33,7 @@
   if (fromJSON(RawParams, P)) {
 (Callbacks->*Handler)(P);
   } else {
-log("Failed to decode " + Method + " request.");
+elog("Failed to decode {0} request.", Method);
   }
 });
   }
Index: clang-tools-extra/trunk/clangd/ClangdServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp
@@ -118,12 +118,12 @@
   auto FS = FSProvider.getFileSystem();
   auto Status = FS->status(RootPath);
   if (!Status)
-log("Failed to get status for RootPath " + RootPath + ": " +
-Status.getError().message());
+elog("Failed to get status for RootPath {0}: {1}", RootPath,
+ Status.getError().message());
   else if (Status->isDirectory())
 this->RootPath = RootPath;
   else
-log("The provided RootPath " + RootPath + " is not a directory.");
+elog("The provided RootPath {0} is not a directory.", RootPath);
 }
 
 void ClangdServer::addDocument(PathRef File, StringRef Contents,
Index: clang-tools-extra/trunk/clangd/Logger.cpp
===
--- clang-tools-extra/trunk/clangd/Logger.cpp
+++ clang-tools-extra/trunk/clangd/Logger.cpp
@@ -25,15 +25,24 @@
 
 LoggingSession::~LoggingSession() { L = nullptr; }
 
-void log(const llvm::Twine &Message) {
+void detail::log(Logger::Level Level,
+ const llvm::formatv_object_base &Message) {
   if (L)
-L->log(Message);
+L->log(Level, Message);
   else {
 static std::mutex Mu;
 std::lock_guard Guard(Mu);
 llvm::errs() << Message << "\n";
   }
 }
 
+const char *detail::debugType(const char *Filename) {
+  if (const char *Slash = strrchr(Filename, '/'))
+return Slash + 1;
+  if (const char *Backslash = strrchr(Filename, '\\'))
+return Backslash + 1;
+  return Filename;
+}
+
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/trunk/clangd/Diagnostics.cpp
===
--- clang-tools-extra/trunk/clangd/Diagnostics.cpp
+++ clang-tools-extra/trunk/clangd/Diagnostics.cpp
@@ -375,7 +375,7 @@
   if (mentionsMainFile(*LastDiag))
 Output.push_back(std::move(*LastDiag));
   else
-log(Twine("Dropped diagnostic outside main file:") + LastDiag->File + ":" +
+log("Dropped diagnostic outside main file: {0}: {1}", LastDiag->File,
 LastDiag->Message);
   LastDiag.reset();
 }
Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -308,11 +308,10 @@
 if (Include->second)
   Completion.HeaderInsertion = Includes.insert(Include->first);
   } else
-log(llvm::formatv(
-"Failed to generate include insertion edits for adding header "
+log("Failed to generate include insertion edits for adding header "
 "(FileURI='{0}', IncludeHeader='{1}') into {2}",
 C.IndexResult->CanonicalDeclaration.FileURI,
-C.IndexResult->Detail->IncludeHeader, FileName));
+C.IndexResult->Detail->IncludeHeader, FileName);
 }
   }
 
@@ -593,11 +592,10 @@
 if (NumResults

[PATCH] D46940: [ASTImporter] make sure that ACtx::getParents still works

[Rafael Stahl via Phabricator via cfe-commits]

r.stahl added a comment.

@rsmith Yes, this should generally better be handled outside of the ASTContext. 
That would be out of scope of this patch. Is it fine to proceed with this one 
for now?

For my usage scenario it actually is convenient this way. In my checker I use 
getParents, but if I would replace this with some tooling functions I wouldn't 
know when the maps become invalid since the invalidation happens on a lower 
level within the analyzer. I'd have to search the parents on every callback 
invocation which would be a massive performance issue.

Once an external parent utility exists, we would still need some invalidation 
notification. If anyone uses the analyzer and parent maps he'd want to know 
when they become invalid for performance reasons. Ideally only when necessary 
like the invalidateParents calls in this patch.


https://reviews.llvm.org/D46940



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

[PATCH] D49012: [clangd] Uprank delcarations when "using q::name" is present in the main file

[Kirill Bobyrev via Phabricator via cfe-commits]

omtcyfz updated this revision to Diff 154969.
omtcyfz added a comment.

Slightly refactor code by making lambda only used once anonymous.


https://reviews.llvm.org/D49012

Files:
  clang-tools-extra/clangd/Quality.cpp
  clang-tools-extra/unittests/clangd/QualityTests.cpp
  clang/include/clang/Sema/CodeCompleteConsumer.h
  clang/lib/Sema/SemaCodeComplete.cpp

Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -859,12 +859,12 @@
   }
 
   // Look through using declarations.
-  if (const UsingShadowDecl *Using =
-  dyn_cast(R.Declaration)) {
-MaybeAddResult(Result(Using->getTargetDecl(),
-  getBasePriority(Using->getTargetDecl()),
-  R.Qualifier),
-   CurContext);
+  if (const UsingShadowDecl *Using = dyn_cast(R.Declaration)) {
+CodeCompletionResult Result(Using->getTargetDecl(),
+getBasePriority(Using->getTargetDecl()),
+R.Qualifier);
+Result.ShadowDecl = Using;
+MaybeAddResult(Result, CurContext);
 return;
   }
 
@@ -977,10 +977,11 @@
 
   // Look through using declarations.
   if (const UsingShadowDecl *Using = dyn_cast(R.Declaration)) {
-AddResult(Result(Using->getTargetDecl(),
- getBasePriority(Using->getTargetDecl()),
- R.Qualifier),
-  CurContext, Hiding);
+CodeCompletionResult Result(Using->getTargetDecl(),
+getBasePriority(Using->getTargetDecl()),
+R.Qualifier);
+Result.ShadowDecl = Using;
+AddResult(Result, CurContext, Hiding);
 return;
   }
 
@@ -1004,10 +1005,10 @@
   if (AsNestedNameSpecifier) {
 R.StartsNestedNameSpecifier = true;
 R.Priority = CCP_NestedNameSpecifier;
-  }
-  else if (Filter == &ResultBuilder::IsMember && !R.Qualifier && InBaseClass &&
-   isa(R.Declaration->getDeclContext()
-  ->getRedeclContext()))
+  } else if (Filter == &ResultBuilder::IsMember && !R.Qualifier &&
+ InBaseClass &&
+ isa(
+ R.Declaration->getDeclContext()->getRedeclContext()))
 R.QualifierIsInformative = true;
 
   // If this result is supposed to have an informative qualifier, add one.
Index: clang/include/clang/Sema/CodeCompleteConsumer.h
===
--- clang/include/clang/Sema/CodeCompleteConsumer.h
+++ clang/include/clang/Sema/CodeCompleteConsumer.h
@@ -45,8 +45,9 @@
 class NamedDecl;
 class NestedNameSpecifier;
 class Preprocessor;
-class Sema;
 class RawComment;
+class Sema;
+class UsingShadowDecl;
 
 /// Default priority values for code-completion results based
 /// on their kind.
@@ -836,6 +837,12 @@
   /// informative rather than required.
   NestedNameSpecifier *Qualifier = nullptr;
 
+  /// If this Decl was unshadowed by using declaration, this can store a
+  /// pointer to the UsingShadowDecl which was used in the unshadowing process.
+  /// This information can be used to uprank CodeCompletionResults / which have
+  /// corresponding `using decl::qualified::name;` nearby.
+  const UsingShadowDecl *ShadowDecl = nullptr;
+
   /// Build a result that refers to a declaration.
   CodeCompletionResult(const NamedDecl *Declaration, unsigned Priority,
NestedNameSpecifier *Qualifier = nullptr,
@@ -847,7 +854,7 @@
 QualifierIsInformative(QualifierIsInformative),
 StartsNestedNameSpecifier(false), AllParametersAreInformative(false),
 DeclaringEntity(false), Qualifier(Qualifier) {
-//FIXME: Add assert to check FixIts range requirements.
+// FIXME: Add assert to check FixIts range requirements.
 computeCursorKindAndAvailability(Accessible);
   }
 
Index: clang-tools-extra/unittests/clangd/QualityTests.cpp
===
--- clang-tools-extra/unittests/clangd/QualityTests.cpp
+++ clang-tools-extra/unittests/clangd/QualityTests.cpp
@@ -77,18 +77,31 @@
 TEST(QualityTests, SymbolRelevanceSignalExtraction) {
   TestTU Test;
   Test.HeaderCode = R"cpp(
-int header();
-int header_main();
-)cpp";
+  int header();
+  int header_main();
+
+  namespace hdr { class Bar {}; } // namespace hdr
+
+  #define DEFINE_FLAG(X) \
+  namespace flags { \
+  int FLAGS_##X; \
+  } \
+
+  DEFINE_FLAG(FOO)
+  )cpp";
   Test.Code = R"cpp(
-int ::header_main() {}
-int main();
+  using hdr::Bar;
 
-[[deprecated]]
-int deprecated() { return 0; }
+  using flags::FLAGS_FOO;
+
+  int ::header_main() {}
+  int main();
 
-namespace { struct X { void y() { int z; } }; }
-struct S{}
+  [[deprecated]]
+  int deprecated() { return 0; }
+
+  namespace { struct X { void y() { int z; } }; }
+  struct S{}
   )cpp";
 

[PATCH] D49175: [clangd] Ignore sema code complete callback with recovery context.

[Eric Liu via Phabricator via cfe-commits]

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

Sema code complete in the recovery mode is generally useless. For many
cases, sema first completes in recovery context and then recovers to more useful
context, in which it's favorable to ignore results from recovery (as results are
often bad e.g. all builtin symbols and top-level symbols). There is also case
where only sema would fail to recover e.g. completions in excluded #if block.
Sema would try to give results, but the results are often useless (see the 
updated
excluded #if block test).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49175

Files:
  clangd/CodeComplete.cpp
  unittests/clangd/CodeCompleteTests.cpp


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -787,20 +787,24 @@
   EXPECT_THAT(Results.Completions, Contains(Named("X")));
 }
 
-TEST(CompletionTest, CompleteInExcludedPPBranch) {
+TEST(CompletionTest, IgnoreCompleteInExcludedPPBranchWithRecoveryContext) {
   auto Results = completions(R"cpp(
 int bar(int param_in_bar) {
 }
 
 int foo(int param_in_foo) {
 #if 0
+  // In recorvery mode, "param_in_foo" will also be suggested among many other
+  // unrelated symbols; however, this is really a special case where this 
works.
+  // If the #if block is outside of the function, "param_in_foo" is still
+  // suggested, but "bar" and "foo" are missing. So the recovery mode doesn't
+  // really provide useful results in excluded branches.
   par^
 #endif
 }
 )cpp");
 
-  EXPECT_THAT(Results.Completions, Contains(Labeled("param_in_foo")));
-  EXPECT_THAT(Results.Completions, Not(Contains(Labeled("param_in_bar";
+  EXPECT_TRUE(Results.Completions.empty());
 }
 
 SignatureHelp signatures(StringRef Text) {
@@ -1293,6 +1297,18 @@
   EXPECT_EQ(R.detail, "[2 overloads]\n\"foo.h\"");
 }
 
+TEST(CompletionTest, IgnoreRecoveryResults) {
+  auto Results = completions(
+  R"cpp(
+  namespace ns { int NotRecovered() { return 0; } }
+  void f() {
+// Sema enters recovery mode first and then normal mode.
+if (auto x = ns::NotRecover^)
+  }
+  )cpp");
+  EXPECT_THAT(Results.Completions, 
UnorderedElementsAre(Named("NotRecovered")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -587,6 +587,18 @@
   void ProcessCodeCompleteResults(class Sema &S, CodeCompletionContext Context,
   CodeCompletionResult *InResults,
   unsigned NumResults) override final {
+// Results from recovery mode are generally useless, and the callback after
+// recovery (if any) is usually more interesting. To make sure we handle 
the
+// future callback from sema, we just ignore all callbacks in recovery 
mode,
+// as taking only results from recovery mode results in poor completion
+// results.
+// FIXME: in case there is no future sema completion callback after the
+// recovery mode, we migth still want to provide some results.
+if (Context.getKind() == CodeCompletionContext::CCC_Recovery) {
+  log("Code complete: Ignoring sema code complete callback with Recovery "
+  "context.");
+  return;
+}
 // If a callback is called without any sema result and the context does not
 // support index-based completion, we simply skip it to give way to
 // potential future callbacks with results.


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -787,20 +787,24 @@
   EXPECT_THAT(Results.Completions, Contains(Named("X")));
 }
 
-TEST(CompletionTest, CompleteInExcludedPPBranch) {
+TEST(CompletionTest, IgnoreCompleteInExcludedPPBranchWithRecoveryContext) {
   auto Results = completions(R"cpp(
 int bar(int param_in_bar) {
 }
 
 int foo(int param_in_foo) {
 #if 0
+  // In recorvery mode, "param_in_foo" will also be suggested among many other
+  // unrelated symbols; however, this is really a special case where this works.
+  // If the #if block is outside of the function, "param_in_foo" is still
+  // suggested, but "bar" and "foo" are missing. So the recovery mode doesn't
+  // really provide useful results in excluded branches.
   par^
 #endif
 }
 )cpp");
 
-  EXPECT_THAT(Results.Completions, Contains(Labeled("param_in_foo")));
-  EXPECT_THAT(Results.Completions, Not(Contains(Labeled("param_in_bar";
+  EXPECT_TRUE(Results.Completions.empty());
 }
 
 SignatureHelp signatures(StringRef Text) {
@@ -1293,6 +1

[PATCH] D49175: [clangd] Ignore sema code complete callback with recovery context.

[Sam McCall via Phabricator via cfe-commits]

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

I like this idea, of course hard to know how it will affect all practical cases.

Is it easy to get have internal stats on how many wins/losses this has?




Comment at: clangd/CodeComplete.cpp:596
+// FIXME: in case there is no future sema completion callback after the
+// recovery mode, we migth still want to provide some results.
+if (Context.getKind() == CodeCompletionContext::CCC_Recovery) {

might



Comment at: clangd/CodeComplete.cpp:596
+// FIXME: in case there is no future sema completion callback after the
+// recovery mode, we migth still want to provide some results.
+if (Context.getKind() == CodeCompletionContext::CCC_Recovery) {

sammccall wrote:
> might
This might also be a case where we can trial identifier-based completion. 
(Eventually I think we'd want it in other contexts too)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49175



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

r336798 - [mips] Add '-mvirt', '-mno-virt', '-mginv', '-mno-ginv' options

[Vladimir Stefanovic via cfe-commits]

Author: vstefanovic
Date: Wed Jul 11 05:45:25 2018
New Revision: 336798

URL: http://llvm.org/viewvc/llvm-project?rev=336798&view=rev
Log:
[mips] Add '-mvirt', '-mno-virt', '-mginv', '-mno-ginv' options

'-mvirt'/'-mno-virt' enables/disables Virtualization ASE.
'-mginv'/'-mno-ginv' enables/disables GINV (Global Invalidate) ASE.

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

Modified:
cfe/trunk/include/clang/Driver/Options.td
cfe/trunk/lib/Driver/ToolChains/Arch/Mips.cpp
cfe/trunk/test/Driver/mips-features.c

Modified: cfe/trunk/include/clang/Driver/Options.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=336798&r1=336797&r2=336798&view=diff
==
--- cfe/trunk/include/clang/Driver/Options.td (original)
+++ cfe/trunk/include/clang/Driver/Options.td Wed Jul 11 05:45:25 2018
@@ -2216,6 +2216,10 @@ def mno_abicalls : Flag<["-"], "mno-abic
   HelpText<"Disable SVR4-style position-independent code (Mips only)">;
 def mno_crc : Flag<["-"], "mno-crc">, Group,
   HelpText<"Disallow use of CRC instructions (Mips only)">;
+def mvirt : Flag<["-"], "mvirt">, Group;
+def mno_virt : Flag<["-"], "mno-virt">, Group;
+def mginv : Flag<["-"], "mginv">, Group;
+def mno_ginv : Flag<["-"], "mno-ginv">, Group;
 def mips1 : Flag<["-"], "mips1">,
   Alias, AliasArgs<["mips1"]>, Group,
   HelpText<"Equivalent to -march=mips1">, Flags<[HelpHidden]>;

Modified: cfe/trunk/lib/Driver/ToolChains/Arch/Mips.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Arch/Mips.cpp?rev=336798&r1=336797&r2=336798&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Arch/Mips.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Arch/Mips.cpp Wed Jul 11 05:45:25 2018
@@ -352,6 +352,10 @@ void mips::getMIPSTargetFeatures(const D
   AddTargetFeature(Args, Features, options::OPT_mmt, options::OPT_mno_mt, 
"mt");
   AddTargetFeature(Args, Features, options::OPT_mcrc, options::OPT_mno_crc,
"crc");
+  AddTargetFeature(Args, Features, options::OPT_mvirt, options::OPT_mno_virt,
+   "virt");
+  AddTargetFeature(Args, Features, options::OPT_mginv, options::OPT_mno_ginv,
+   "ginv");
 
   if (Arg *A = Args.getLastArg(options::OPT_mindirect_jump_EQ)) {
 StringRef Val = StringRef(A->getValue());

Modified: cfe/trunk/test/Driver/mips-features.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/mips-features.c?rev=336798&r1=336797&r2=336798&view=diff
==
--- cfe/trunk/test/Driver/mips-features.c (original)
+++ cfe/trunk/test/Driver/mips-features.c Wed Jul 11 05:45:25 2018
@@ -420,3 +420,27 @@
 // RUN: -mcrc -mno-crc 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-CRC %s
 // CHECK-NO-CRC: "-target-feature" "-crc"
+//
+// -mvirt
+// RUN: %clang -target mips-unknown-linux-gnu -### -c %s \
+// RUN: -mno-virt -mvirt 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-VIRT %s
+// CHECK-VIRT: "-target-feature" "+virt"
+//
+// -mno-virt
+// RUN: %clang -target mips-unknown-linux-gnu -### -c %s \
+// RUN: -mvirt -mno-virt 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-VIRT %s
+// CHECK-NO-VIRT: "-target-feature" "-virt"
+//
+// -mginv
+// RUN: %clang -target mips-unknown-linux-gnu -### -c %s \
+// RUN: -mno-ginv -mginv 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-GINV %s
+// CHECK-GINV: "-target-feature" "+ginv"
+//
+// -mno-ginv
+// RUN: %clang -target mips-unknown-linux-gnu -### -c %s \
+// RUN: -mginv -mno-ginv 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-GINV %s
+// CHECK-NO-GINV: "-target-feature" "-ginv"


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

[PATCH] D48982: [mips] Add '-mvirt', '-mno-virt', '-mginv', '-mno-ginv' options

[Vladimir Stefanovic via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC336798: [mips] Add '-mvirt', 
'-mno-virt', '-mginv', '-mno-ginv' options 
(authored by vstefanovic, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D48982

Files:
  include/clang/Driver/Options.td
  lib/Driver/ToolChains/Arch/Mips.cpp
  test/Driver/mips-features.c


Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -2216,6 +2216,10 @@
   HelpText<"Disable SVR4-style position-independent code (Mips only)">;
 def mno_crc : Flag<["-"], "mno-crc">, Group,
   HelpText<"Disallow use of CRC instructions (Mips only)">;
+def mvirt : Flag<["-"], "mvirt">, Group;
+def mno_virt : Flag<["-"], "mno-virt">, Group;
+def mginv : Flag<["-"], "mginv">, Group;
+def mno_ginv : Flag<["-"], "mno-ginv">, Group;
 def mips1 : Flag<["-"], "mips1">,
   Alias, AliasArgs<["mips1"]>, Group,
   HelpText<"Equivalent to -march=mips1">, Flags<[HelpHidden]>;
Index: test/Driver/mips-features.c
===
--- test/Driver/mips-features.c
+++ test/Driver/mips-features.c
@@ -420,3 +420,27 @@
 // RUN: -mcrc -mno-crc 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-CRC %s
 // CHECK-NO-CRC: "-target-feature" "-crc"
+//
+// -mvirt
+// RUN: %clang -target mips-unknown-linux-gnu -### -c %s \
+// RUN: -mno-virt -mvirt 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-VIRT %s
+// CHECK-VIRT: "-target-feature" "+virt"
+//
+// -mno-virt
+// RUN: %clang -target mips-unknown-linux-gnu -### -c %s \
+// RUN: -mvirt -mno-virt 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-VIRT %s
+// CHECK-NO-VIRT: "-target-feature" "-virt"
+//
+// -mginv
+// RUN: %clang -target mips-unknown-linux-gnu -### -c %s \
+// RUN: -mno-ginv -mginv 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-GINV %s
+// CHECK-GINV: "-target-feature" "+ginv"
+//
+// -mno-ginv
+// RUN: %clang -target mips-unknown-linux-gnu -### -c %s \
+// RUN: -mginv -mno-ginv 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-GINV %s
+// CHECK-NO-GINV: "-target-feature" "-ginv"
Index: lib/Driver/ToolChains/Arch/Mips.cpp
===
--- lib/Driver/ToolChains/Arch/Mips.cpp
+++ lib/Driver/ToolChains/Arch/Mips.cpp
@@ -352,6 +352,10 @@
   AddTargetFeature(Args, Features, options::OPT_mmt, options::OPT_mno_mt, 
"mt");
   AddTargetFeature(Args, Features, options::OPT_mcrc, options::OPT_mno_crc,
"crc");
+  AddTargetFeature(Args, Features, options::OPT_mvirt, options::OPT_mno_virt,
+   "virt");
+  AddTargetFeature(Args, Features, options::OPT_mginv, options::OPT_mno_ginv,
+   "ginv");
 
   if (Arg *A = Args.getLastArg(options::OPT_mindirect_jump_EQ)) {
 StringRef Val = StringRef(A->getValue());


Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -2216,6 +2216,10 @@
   HelpText<"Disable SVR4-style position-independent code (Mips only)">;
 def mno_crc : Flag<["-"], "mno-crc">, Group,
   HelpText<"Disallow use of CRC instructions (Mips only)">;
+def mvirt : Flag<["-"], "mvirt">, Group;
+def mno_virt : Flag<["-"], "mno-virt">, Group;
+def mginv : Flag<["-"], "mginv">, Group;
+def mno_ginv : Flag<["-"], "mno-ginv">, Group;
 def mips1 : Flag<["-"], "mips1">,
   Alias, AliasArgs<["mips1"]>, Group,
   HelpText<"Equivalent to -march=mips1">, Flags<[HelpHidden]>;
Index: test/Driver/mips-features.c
===
--- test/Driver/mips-features.c
+++ test/Driver/mips-features.c
@@ -420,3 +420,27 @@
 // RUN: -mcrc -mno-crc 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-CRC %s
 // CHECK-NO-CRC: "-target-feature" "-crc"
+//
+// -mvirt
+// RUN: %clang -target mips-unknown-linux-gnu -### -c %s \
+// RUN: -mno-virt -mvirt 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-VIRT %s
+// CHECK-VIRT: "-target-feature" "+virt"
+//
+// -mno-virt
+// RUN: %clang -target mips-unknown-linux-gnu -### -c %s \
+// RUN: -mvirt -mno-virt 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-VIRT %s
+// CHECK-NO-VIRT: "-target-feature" "-virt"
+//
+// -mginv
+// RUN: %clang -target mips-unknown-linux-gnu -### -c %s \
+// RUN: -mno-ginv -mginv 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-GINV %s
+// CHECK-GINV: "-target-feature" "+ginv"
+//
+// -mno-ginv
+// RUN: %clang -target mips-unknown-linux-gnu -### -c %s \
+// RUN: -mginv -mno-ginv 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-NO-GINV %s
+// CHECK-NO-GINV: "-target-feature" "-ginv"
Index: lib/Driver/ToolChains/Arch/Mips.cpp
===
--- lib/Driver/ToolChains/Arch/Mips.cpp
+++ lib/Driv

[PATCH] D49175: [clangd] Ignore sema code complete callback with recovery context.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 154979.
ioeric marked 2 inline comments as done.
ioeric added a comment.

- Addressed review comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49175

Files:
  clangd/CodeComplete.cpp
  unittests/clangd/CodeCompleteTests.cpp


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -787,20 +787,24 @@
   EXPECT_THAT(Results.Completions, Contains(Named("X")));
 }
 
-TEST(CompletionTest, CompleteInExcludedPPBranch) {
+TEST(CompletionTest, IgnoreCompleteInExcludedPPBranchWithRecoveryContext) {
   auto Results = completions(R"cpp(
 int bar(int param_in_bar) {
 }
 
 int foo(int param_in_foo) {
 #if 0
+  // In recorvery mode, "param_in_foo" will also be suggested among many other
+  // unrelated symbols; however, this is really a special case where this 
works.
+  // If the #if block is outside of the function, "param_in_foo" is still
+  // suggested, but "bar" and "foo" are missing. So the recovery mode doesn't
+  // really provide useful results in excluded branches.
   par^
 #endif
 }
 )cpp");
 
-  EXPECT_THAT(Results.Completions, Contains(Labeled("param_in_foo")));
-  EXPECT_THAT(Results.Completions, Not(Contains(Labeled("param_in_bar";
+  EXPECT_TRUE(Results.Completions.empty());
 }
 
 SignatureHelp signatures(StringRef Text) {
@@ -1293,6 +1297,18 @@
   EXPECT_EQ(R.detail, "[2 overloads]\n\"foo.h\"");
 }
 
+TEST(CompletionTest, IgnoreRecoveryResults) {
+  auto Results = completions(
+  R"cpp(
+  namespace ns { int NotRecovered() { return 0; } }
+  void f() {
+// Sema enters recovery mode first and then normal mode.
+if (auto x = ns::NotRecover^)
+  }
+  )cpp");
+  EXPECT_THAT(Results.Completions, 
UnorderedElementsAre(Named("NotRecovered")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -587,6 +587,19 @@
   void ProcessCodeCompleteResults(class Sema &S, CodeCompletionContext Context,
   CodeCompletionResult *InResults,
   unsigned NumResults) override final {
+// Results from recovery mode are generally useless, and the callback after
+// recovery (if any) is usually more interesting. To make sure we handle 
the
+// future callback from sema, we just ignore all callbacks in recovery 
mode,
+// as taking only results from recovery mode results in poor completion
+// results.
+// FIXME: in case there is no future sema completion callback after the
+// recovery mode, we might still want to provide some results (e.g. trivial
+// identifier-based completion).
+if (Context.getKind() == CodeCompletionContext::CCC_Recovery) {
+  log("Code complete: Ignoring sema code complete callback with Recovery "
+  "context.");
+  return;
+}
 // If a callback is called without any sema result and the context does not
 // support index-based completion, we simply skip it to give way to
 // potential future callbacks with results.


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -787,20 +787,24 @@
   EXPECT_THAT(Results.Completions, Contains(Named("X")));
 }
 
-TEST(CompletionTest, CompleteInExcludedPPBranch) {
+TEST(CompletionTest, IgnoreCompleteInExcludedPPBranchWithRecoveryContext) {
   auto Results = completions(R"cpp(
 int bar(int param_in_bar) {
 }
 
 int foo(int param_in_foo) {
 #if 0
+  // In recorvery mode, "param_in_foo" will also be suggested among many other
+  // unrelated symbols; however, this is really a special case where this works.
+  // If the #if block is outside of the function, "param_in_foo" is still
+  // suggested, but "bar" and "foo" are missing. So the recovery mode doesn't
+  // really provide useful results in excluded branches.
   par^
 #endif
 }
 )cpp");
 
-  EXPECT_THAT(Results.Completions, Contains(Labeled("param_in_foo")));
-  EXPECT_THAT(Results.Completions, Not(Contains(Labeled("param_in_bar";
+  EXPECT_TRUE(Results.Completions.empty());
 }
 
 SignatureHelp signatures(StringRef Text) {
@@ -1293,6 +1297,18 @@
   EXPECT_EQ(R.detail, "[2 overloads]\n\"foo.h\"");
 }
 
+TEST(CompletionTest, IgnoreRecoveryResults) {
+  auto Results = completions(
+  R"cpp(
+  namespace ns { int NotRecovered() { return 0; } }
+  void f() {
+// Sema enters recovery mode first and then normal mode.
+if (auto x = ns::NotRecover^)
+  }
+  )cpp");
+  EXPECT_THAT(Results.Completions, UnorderedElementsAre(Named(

[PATCH] D49175: [clangd] Ignore sema code complete callback with recovery context.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.

Thanks! That's a simple fix that makes things better overall! 
LGTM with a NIT about the comment in the test.




Comment at: unittests/clangd/CodeCompleteTests.cpp:797
 #if 0
+  // In recorvery mode, "param_in_foo" will also be suggested among many other
+  // unrelated symbols; however, this is really a special case where this 
works.

NIT.

I would argue the results were actually useful here and we introduced a 
regression.
This patches fixes the more important cases, e.g. `if (int x = id^)`, so it's 
actually improving things overall. However, I'm not sure about the comment that 
says clangd is doing the right thing here (it actually gives a worse UX). WDYT? 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49175



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

[PATCH] D49175: [clangd] Ignore sema code complete callback with recovery context.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D49175#1158562, @sammccall wrote:

> I like this idea, of course hard to know how it will affect all practical 
> cases.
>
> Is it easy to get have internal stats on how many wins/losses this has?


This would fix bad completion results when sema could recover to the normal 
mode (e.g. completion in macros like EXPECT_THAT, expressions in if 
statements). From the code completion metrics I collected from existing code, 
the number of completions where no result was found dropped by 30%. The loss 
seems to be we will get no result in recovery mode rather than bad results 
(e.g. in excluded preprocessor branches).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49175



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

[clang-tools-extra] r336801 - [clangd] Ignore sema code complete callback with recovery context.

[Eric Liu via cfe-commits]

Author: ioeric
Date: Wed Jul 11 06:15:31 2018
New Revision: 336801

URL: http://llvm.org/viewvc/llvm-project?rev=336801&view=rev
Log:
[clangd] Ignore sema code complete callback with recovery context.

Summary:
Sema code complete in the recovery mode is generally useless. For many
cases, sema first completes in recovery context and then recovers to more useful
context, in which it's favorable to ignore results from recovery (as results are
often bad e.g. all builtin symbols and top-level symbols). There is also case
where only sema would fail to recover e.g. completions in excluded #if block.
Sema would try to give results, but the results are often useless (see the 
updated
excluded #if block test).

Reviewers: sammccall, ilya-biryukov

Subscribers: MaskRay, jkorous, cfe-commits

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

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

Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=336801&r1=336800&r2=336801&view=diff
==
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original)
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Wed Jul 11 06:15:31 2018
@@ -586,6 +586,19 @@ struct CompletionRecorder : public CodeC
   void ProcessCodeCompleteResults(class Sema &S, CodeCompletionContext Context,
   CodeCompletionResult *InResults,
   unsigned NumResults) override final {
+// Results from recovery mode are generally useless, and the callback after
+// recovery (if any) is usually more interesting. To make sure we handle 
the
+// future callback from sema, we just ignore all callbacks in recovery 
mode,
+// as taking only results from recovery mode results in poor completion
+// results.
+// FIXME: in case there is no future sema completion callback after the
+// recovery mode, we might still want to provide some results (e.g. trivial
+// identifier-based completion).
+if (Context.getKind() == CodeCompletionContext::CCC_Recovery) {
+  log("Code complete: Ignoring sema code complete callback with Recovery "
+  "context.");
+  return;
+}
 // If a callback is called without any sema result and the context does not
 // support index-based completion, we simply skip it to give way to
 // potential future callbacks with results.

Modified: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp?rev=336801&r1=336800&r2=336801&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp Wed Jul 11 
06:15:31 2018
@@ -787,20 +787,24 @@ int f(int input_num) {
   EXPECT_THAT(Results.Completions, Contains(Named("X")));
 }
 
-TEST(CompletionTest, CompleteInExcludedPPBranch) {
+TEST(CompletionTest, IgnoreCompleteInExcludedPPBranchWithRecoveryContext) {
   auto Results = completions(R"cpp(
 int bar(int param_in_bar) {
 }
 
 int foo(int param_in_foo) {
 #if 0
+  // In recorvery mode, "param_in_foo" will also be suggested among many other
+  // unrelated symbols; however, this is really a special case where this 
works.
+  // If the #if block is outside of the function, "param_in_foo" is still
+  // suggested, but "bar" and "foo" are missing. So the recovery mode doesn't
+  // really provide useful results in excluded branches.
   par^
 #endif
 }
 )cpp");
 
-  EXPECT_THAT(Results.Completions, Contains(Labeled("param_in_foo")));
-  EXPECT_THAT(Results.Completions, Not(Contains(Labeled("param_in_bar";
+  EXPECT_TRUE(Results.Completions.empty());
 }
 
 SignatureHelp signatures(StringRef Text) {
@@ -1293,6 +1297,18 @@ TEST(CompletionTest, Render) {
   EXPECT_EQ(R.detail, "[2 overloads]\n\"foo.h\"");
 }
 
+TEST(CompletionTest, IgnoreRecoveryResults) {
+  auto Results = completions(
+  R"cpp(
+  namespace ns { int NotRecovered() { return 0; } }
+  void f() {
+// Sema enters recovery mode first and then normal mode.
+if (auto x = ns::NotRecover^)
+  }
+  )cpp");
+  EXPECT_THAT(Results.Completions, 
UnorderedElementsAre(Named("NotRecovered")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang


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

[PATCH] D49175: [clangd] Ignore sema code complete callback with recovery context.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL336801: [clangd] Ignore sema code complete callback with 
recovery context. (authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D49175

Files:
  clang-tools-extra/trunk/clangd/CodeComplete.cpp
  clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp


Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -586,6 +586,19 @@
   void ProcessCodeCompleteResults(class Sema &S, CodeCompletionContext Context,
   CodeCompletionResult *InResults,
   unsigned NumResults) override final {
+// Results from recovery mode are generally useless, and the callback after
+// recovery (if any) is usually more interesting. To make sure we handle 
the
+// future callback from sema, we just ignore all callbacks in recovery 
mode,
+// as taking only results from recovery mode results in poor completion
+// results.
+// FIXME: in case there is no future sema completion callback after the
+// recovery mode, we might still want to provide some results (e.g. trivial
+// identifier-based completion).
+if (Context.getKind() == CodeCompletionContext::CCC_Recovery) {
+  log("Code complete: Ignoring sema code complete callback with Recovery "
+  "context.");
+  return;
+}
 // If a callback is called without any sema result and the context does not
 // support index-based completion, we simply skip it to give way to
 // potential future callbacks with results.
Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -787,20 +787,24 @@
   EXPECT_THAT(Results.Completions, Contains(Named("X")));
 }
 
-TEST(CompletionTest, CompleteInExcludedPPBranch) {
+TEST(CompletionTest, IgnoreCompleteInExcludedPPBranchWithRecoveryContext) {
   auto Results = completions(R"cpp(
 int bar(int param_in_bar) {
 }
 
 int foo(int param_in_foo) {
 #if 0
+  // In recorvery mode, "param_in_foo" will also be suggested among many other
+  // unrelated symbols; however, this is really a special case where this 
works.
+  // If the #if block is outside of the function, "param_in_foo" is still
+  // suggested, but "bar" and "foo" are missing. So the recovery mode doesn't
+  // really provide useful results in excluded branches.
   par^
 #endif
 }
 )cpp");
 
-  EXPECT_THAT(Results.Completions, Contains(Labeled("param_in_foo")));
-  EXPECT_THAT(Results.Completions, Not(Contains(Labeled("param_in_bar";
+  EXPECT_TRUE(Results.Completions.empty());
 }
 
 SignatureHelp signatures(StringRef Text) {
@@ -1293,6 +1297,18 @@
   EXPECT_EQ(R.detail, "[2 overloads]\n\"foo.h\"");
 }
 
+TEST(CompletionTest, IgnoreRecoveryResults) {
+  auto Results = completions(
+  R"cpp(
+  namespace ns { int NotRecovered() { return 0; } }
+  void f() {
+// Sema enters recovery mode first and then normal mode.
+if (auto x = ns::NotRecover^)
+  }
+  )cpp");
+  EXPECT_THAT(Results.Completions, 
UnorderedElementsAre(Named("NotRecovered")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang


Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -586,6 +586,19 @@
   void ProcessCodeCompleteResults(class Sema &S, CodeCompletionContext Context,
   CodeCompletionResult *InResults,
   unsigned NumResults) override final {
+// Results from recovery mode are generally useless, and the callback after
+// recovery (if any) is usually more interesting. To make sure we handle the
+// future callback from sema, we just ignore all callbacks in recovery mode,
+// as taking only results from recovery mode results in poor completion
+// results.
+// FIXME: in case there is no future sema completion callback after the
+// recovery mode, we might still want to provide some results (e.g. trivial
+// identifier-based completion).
+if (Context.getKind() == CodeCompletionContext::CCC_Recovery) {
+  log("Code complete: Ignoring sema code complete callback with Recovery "
+  "context.");
+  return;
+}
 // If a callback is called without any sema result and the context does not
 // support index-based completion, we simply skip it to give way to
 //

Re: [PATCH] D49175: [clangd] Ignore sema code complete callback with recovery context.

[Sam McCall via cfe-commits]

On Wed, Jul 11, 2018, 15:18 Eric Liu via Phabricator <
revi...@reviews.llvm.org> wrote:

> ioeric added a comment.
>
> In https://reviews.llvm.org/D49175#1158562, @sammccall wrote:
>
> > I like this idea, of course hard to know how it will affect all
> practical cases.
> >
> > Is it easy to get have internal stats on how many wins/losses this has?
>
>
> This would fix bad completion results when sema could recover to the
> normal mode (e.g. completion in macros like EXPECT_THAT, expressions in if
> statements). From the code completion metrics I collected from existing
> code, the number of completions where no result was found dropped by 30%.
> The loss seems to be we will get no result in recovery mode rather than bad
> results (e.g. in excluded preprocessor branches).
>
Thanks! I have no doubt this is positive overall, so just thinking out loud
here.

For changes like this, we can show that quantitavely by running
before/after completions at the same sites, and reporting e.g.
Percent of all sites that are better vs worse vs neutral (in terms of rank
of desired result)
Change in MRR: mean/std deviation

For our internal quality evaluation tools, we could think about how to make
it easy to get these numbers for a candidate change.
Having the raw results as before/after pairs is great for investigating
wins/losses too.



>
> Repository:
>   rCTE Clang Tools Extra
>
> https://reviews.llvm.org/D49175
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47946: [ASTImporter] Fix infinite recursion on function import with struct definition in parameters

[Zoltán Gera via Phabricator via cfe-commits]

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



Comment at: unittests/AST/ASTImporterTest.cpp:234
+assert(ToAST);
+createVirtualFileIfNeeded(ToAST.get(), It->FileName, It->Code);
+return &*It;

a.sidorin wrote:
> Can we move the side effect into import()?
Sure, thank you.


https://reviews.llvm.org/D47946



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

[PATCH] D47946: [ASTImporter] Fix infinite recursion on function import with struct definition in parameters

[Zoltán Gera via Phabricator via cfe-commits]

gerazo updated this revision to Diff 154991.
gerazo marked an inline comment as done.
gerazo added a comment.

Minor fixes for Aleksei's comments.


https://reviews.llvm.org/D47946

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

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -284,19 +284,44 @@
   // Buffer for the To context, must live in the test scope.
   std::string ToCode;
 
+  // Represents a "From" translation unit and holds an importer object which we
+  // use to import from this translation unit.
   struct TU {
 // Buffer for the context, must live in the test scope.
 std::string Code;
 std::string FileName;
 std::unique_ptr Unit;
 TranslationUnitDecl *TUDecl = nullptr;
+std::unique_ptr Importer;
+
 TU(StringRef Code, StringRef FileName, ArgVector Args)
 : Code(Code), FileName(FileName),
   Unit(tooling::buildASTFromCodeWithArgs(this->Code, Args,
  this->FileName)),
   TUDecl(Unit->getASTContext().getTranslationUnitDecl()) {
   Unit->enableSourceFileDiagnostics();
 }
+
+void lazyInitImporter(ASTUnit *ToAST) {
+  assert(ToAST);
+  if (!Importer) {
+Importer.reset(new ASTImporter(
+ToAST->getASTContext(), ToAST->getFileManager(),
+Unit->getASTContext(), Unit->getFileManager(), false));
+  }
+  assert(&ToAST->getASTContext() == &Importer->getToContext());
+  createVirtualFileIfNeeded(ToAST, FileName, Code);
+}
+
+Decl *import(ASTUnit *ToAST, Decl *FromDecl) {
+  lazyInitImporter(ToAST);
+  return Importer->Import(FromDecl);
+ }
+
+QualType import(ASTUnit *ToAST, QualType FromType) {
+  lazyInitImporter(ToAST);
+  return Importer->Import(FromType);
+}
   };
 
   // We may have several From contexts and related translation units. In each
@@ -308,6 +333,26 @@
   // vector is expanding, with the list we won't have these issues.
   std::list FromTUs;
 
+  void lazyInitToAST(Language ToLang) {
+if (ToAST)
+  return;
+ArgVector ToArgs = getArgVectorForLanguage(ToLang);
+// Build the AST from an empty file.
+ToAST = tooling::buildASTFromCodeWithArgs(/*Code=*/"", ToArgs, "empty.cc");
+ToAST->enableSourceFileDiagnostics();
+  }
+
+  TU *findFromTU(Decl *From) {
+// Create a virtual file in the To Ctx which corresponds to the file from
+// which we want to import the `From` Decl. Without this source locations
+// will be invalid in the ToCtx.
+auto It = std::find_if(FromTUs.begin(), FromTUs.end(), [From](const TU &E) {
+  return E.TUDecl == From->getTranslationUnitDecl();
+});
+assert(It != FromTUs.end());
+return &*It;
+  }
+
 public:
   // We may have several From context but only one To context.
   std::unique_ptr ToAST;
@@ -329,14 +374,10 @@
 ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
 ToAST->enableSourceFileDiagnostics();
 
-ASTContext &FromCtx = FromTU.Unit->getASTContext(),
-   &ToCtx = ToAST->getASTContext();
+ASTContext &FromCtx = FromTU.Unit->getASTContext();
 
 createVirtualFileIfNeeded(ToAST.get(), InputFileName, FromTU.Code);
 
-ASTImporter Importer(ToCtx, ToAST->getFileManager(), FromCtx,
- FromTU.Unit->getFileManager(), false);
-
 IdentifierInfo *ImportedII = &FromCtx.Idents.get(Identifier);
 assert(ImportedII && "Declaration with the given identifier "
  "should be specified in test!");
@@ -347,7 +388,7 @@
 
 assert(FoundDecls.size() == 1);
 
-Decl *Imported = Importer.Import(FoundDecls.front());
+Decl *Imported = FromTU.import(ToAST.get(), FoundDecls.front());
 assert(Imported);
 return std::make_tuple(*FoundDecls.begin(), Imported);
   }
@@ -384,30 +425,17 @@
   // May be called several times in a given test.
   // The different instances of the param From may have different ASTContext.
   Decl *Import(Decl *From, Language ToLang) {
-if (!ToAST) {
-  ArgVector ToArgs = getArgVectorForLanguage(ToLang);
-  // Build the AST from an empty file.
-  ToAST =
-  tooling::buildASTFromCodeWithArgs(/*Code=*/"", ToArgs, "empty.cc");
-  ToAST->enableSourceFileDiagnostics();
-}
-
-// Create a virtual file in the To Ctx which corresponds to the file from
-// which we want to import the `From` Decl. Without this source locations
-// will be invalid in the ToCtx.
-auto It = std::find_if(FromTUs.begin(), FromTUs.end(), [From](const TU &E) {
-  return E.TUDecl == From->getTranslationUnitDecl();
-});
-assert(It != FromTUs.end());
-createVirtualFileIfNeeded(ToAST.get(), It->FileName, It->Code);
-
-ASTContext &FromCtx = From->getASTContext(),
-   &ToCtx = ToAST->getASTContext();
-   

r336807 - [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via cfe-commits]

Author: simark
Date: Wed Jul 11 07:08:17 2018
New Revision: 336807

URL: http://llvm.org/viewvc/llvm-project?rev=336807&view=rev
Log:
[VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the 
requested name

Summary:
InMemoryFileSystem::status behaves differently than
RealFileSystem::status.  The Name contained in the Status returned by
RealFileSystem::status will be the path as requested by the caller,
whereas InMemoryFileSystem::status returns the normalized path.

For example, when requested the status for "../src/first.h",
RealFileSystem returns a Status with "../src/first.h" as the Name.
InMemoryFileSystem returns "/absolute/path/to/src/first.h".

The reason for this change is that I want to make a unit test in the
clangd testsuite (where we use an InMemoryFileSystem) to reproduce a
bug I get with the clangd program (where a RealFileSystem is used).
This difference in behavior "hides" the bug in the unit test version.

In general, I guess it's good if InMemoryFileSystem works as much as
possible like RealFileSystem.

Doing so made the FileEntry::RealPathName value (assigned in
FileManager::getFile) wrong when using the InMemoryFileSystem.  That's
because it assumes that vfs::File::getName will always return the real
path.  I changed to to use FileSystem::getRealPath instead.

Subscribers: ilya-biryukov, ioeric, cfe-commits

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

Modified:
cfe/trunk/lib/Basic/FileManager.cpp
cfe/trunk/lib/Basic/VirtualFileSystem.cpp
cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp
cfe/trunk/unittests/Driver/ToolChainTest.cpp

Modified: cfe/trunk/lib/Basic/FileManager.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=336807&r1=336806&r2=336807&view=diff
==
--- cfe/trunk/lib/Basic/FileManager.cpp (original)
+++ cfe/trunk/lib/Basic/FileManager.cpp Wed Jul 11 07:08:17 2018
@@ -315,9 +315,11 @@ const FileEntry *FileManager::getFile(St
   UFE.InPCH = Data.InPCH;
   UFE.File = std::move(F);
   UFE.IsValid = true;
-  if (UFE.File)
-if (auto RealPathName = UFE.File->getName())
-  UFE.RealPathName = *RealPathName;
+
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+UFE.RealPathName = RealPathName.str();
+
   return &UFE;
 }
 

Modified: cfe/trunk/lib/Basic/VirtualFileSystem.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/VirtualFileSystem.cpp?rev=336807&r1=336806&r2=336807&view=diff
==
--- cfe/trunk/lib/Basic/VirtualFileSystem.cpp (original)
+++ cfe/trunk/lib/Basic/VirtualFileSystem.cpp Wed Jul 11 07:08:17 2018
@@ -471,15 +471,33 @@ enum InMemoryNodeKind { IME_File, IME_Di
 /// The in memory file system is a tree of Nodes. Every node can either be a
 /// file or a directory.
 class InMemoryNode {
-  Status Stat;
   InMemoryNodeKind Kind;
+  Status Stat;
+
+protected:
+  /// Return Stat.  This should only be used for internal/debugging use.  When
+  /// clients wants the Status of this node, they should use
+  /// \p getStatus(StringRef).
+  const Status& getStatus() const {
+return Stat;
+  }
 
 public:
   InMemoryNode(Status Stat, InMemoryNodeKind Kind)
-  : Stat(std::move(Stat)), Kind(Kind) {}
+  : Kind(Kind), Stat(std::move(Stat)) {}
   virtual ~InMemoryNode() = default;
 
-  const Status &getStatus() const { return Stat; }
+  /// Return the \p Status for this node. \p RequestedName should be the name
+  /// through which the caller referred to this node. It will override
+  /// \p Status::Name in the return value, to mimic the behavior of \p 
RealFile.
+  Status getStatus(StringRef RequestedName) const {
+return Status::copyWithNewName(Stat, RequestedName);
+  }
+
+  /// Get the filename of this node (the name without the directory part).
+  StringRef getFileName() const {
+return llvm::sys::path::filename(Stat.getName());
+  }
   InMemoryNodeKind getKind() const { return Kind; }
   virtual std::string toString(unsigned Indent) const = 0;
 };
@@ -504,14 +522,22 @@ public:
   }
 };
 
-/// Adapt a InMemoryFile for VFS' File interface.
+/// Adapt a InMemoryFile for VFS' File interface.  The goal is to make
+/// \p InMemoryFileAdaptor mimic as much as possible the behavior of
+/// \p RealFile.
 class InMemoryFileAdaptor : public File {
   InMemoryFile &Node;
 
+  /// The name to use when returning a Status for this file.
+  std::string RequestedName;
+
 public:
-  explicit InMemoryFileAdaptor(InMemoryFile &Node) : Node(Node) {}
+  explicit InMemoryFileAdaptor(InMemoryFile &Node, std::string RequestedName)
+  : Node(Node), RequestedName(std::move(RequestedName)) {}
 
-  llvm::ErrorOr status() override { return Node.getStatus(); }
+  llvm::ErrorOr status() override {
+return Node.getStatus(RequestedName);
+  }
 
   llvm::ErrorOr>
   getBuffer(const Twine &Name, int

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC336807: [VirtualFileSystem] InMemoryFileSystem::status: 
Return a Status with the… (authored by simark, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D48903?vs=154835&id=154995#toc

Repository:
  rC Clang

https://reviews.llvm.org/D48903

Files:
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -315,9 +315,11 @@
   UFE.InPCH = Data.InPCH;
   UFE.File = std::move(F);
   UFE.IsValid = true;
-  if (UFE.File)
-if (auto RealPathName = UFE.File->getName())
-  UFE.RealPathName = *RealPathName;
+
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+UFE.RealPathName = RealPathName.str();
+
   return &UFE;
 }
 
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -471,15 +471,33 @@
 /// The in memory file system is a tree of Nodes. Every node can either be a
 /// file or a directory.
 class InMemoryNode {
-  Status Stat;
   InMemoryNodeKind Kind;
+  Status Stat;
+
+protected:
+  /// Return Stat.  This should only be used for internal/debugging use.  When
+  /// clients wants the Status of this node, they should use
+  /// \p getStatus(StringRef).
+  const Status& getStatus() const {
+return Stat;
+  }
 
 public:
   InMemoryNode(Status Stat, InMemoryNodeKind Kind)
-  : Stat(std::move(Stat)), Kind(Kind) {}
+  : Kind(Kind), Stat(std::move(Stat)) {}
   virtual ~InMemoryNode() = default;
 
-  const Status &getStatus() const { return Stat; }
+  /// Return the \p Status for this node. \p RequestedName should be the name
+  /// through which the caller referred to this node. It will override
+  /// \p Status::Name in the return value, to mimic the behavior of \p RealFile.
+  Status getStatus(StringRef RequestedName) const {
+return Status::copyWithNewName(Stat, RequestedName);
+  }
+
+  /// Get the filename of this node (the name without the directory part).
+  StringRef getFileName() const {
+return llvm::sys::path::filename(Stat.getName());
+  }
   InMemoryNodeKind getKind() const { return Kind; }
   virtual std::string toString(unsigned Indent) const = 0;
 };
@@ -504,14 +522,22 @@
   }
 };
 
-/// Adapt a InMemoryFile for VFS' File interface.
+/// Adapt a InMemoryFile for VFS' File interface.  The goal is to make
+/// \p InMemoryFileAdaptor mimic as much as possible the behavior of
+/// \p RealFile.
 class InMemoryFileAdaptor : public File {
   InMemoryFile &Node;
 
+  /// The name to use when returning a Status for this file.
+  std::string RequestedName;
+
 public:
-  explicit InMemoryFileAdaptor(InMemoryFile &Node) : Node(Node) {}
+  explicit InMemoryFileAdaptor(InMemoryFile &Node, std::string RequestedName)
+  : Node(Node), RequestedName(std::move(RequestedName)) {}
 
-  llvm::ErrorOr status() override { return Node.getStatus(); }
+  llvm::ErrorOr status() override {
+return Node.getStatus(RequestedName);
+  }
 
   llvm::ErrorOr>
   getBuffer(const Twine &Name, int64_t FileSize, bool RequiresNullTerminator,
@@ -711,7 +737,7 @@
 llvm::ErrorOr InMemoryFileSystem::status(const Twine &Path) {
   auto Node = lookupInMemoryNode(*this, Root.get(), Path);
   if (Node)
-return (*Node)->getStatus();
+return (*Node)->getStatus(Path.str());
   return Node.getError();
 }
 
@@ -724,7 +750,8 @@
   // When we have a file provide a heap-allocated wrapper for the memory buffer
   // to match the ownership semantics for File.
   if (auto *F = dyn_cast(*Node))
-return std::unique_ptr(new detail::InMemoryFileAdaptor(*F));
+return std::unique_ptr(
+new detail::InMemoryFileAdaptor(*F, Path.str()));
 
   // FIXME: errc::not_a_file?
   return make_error_code(llvm::errc::invalid_argument);
@@ -736,21 +763,33 @@
 class InMemoryDirIterator : public clang::vfs::detail::DirIterImpl {
   detail::InMemoryDirectory::const_iterator I;
   detail::InMemoryDirectory::const_iterator E;
+  std::string RequestedDirName;
+
+  void setCurrentEntry() {
+if (I != E) {
+  SmallString<256> Path(RequestedDirName);
+  llvm::sys::path::append(Path, I->second->getFileName());
+  CurrentEntry = I->second->getStatus(Path.str());
+} else {
+  // When we're at the end, make CurrentEntry invalid and DirIterImpl will
+  // do the rest.
+  CurrentEntry = Status();
+}
+  }
 
 public:
   InMemoryDirIterator() = default;
 
-  explicit InMemoryDirIterator(detail::InMemoryDirectory &Dir)
-  : I(Dir.begin()), E(Dir.end()) {
-if (I != E)
-  CurrentEntry = I->second->getStatus();
+  explicit InMemoryDirIterator(detail::InMemoryDirectory &Dir

Re: [PATCH] D49175: [clangd] Ignore sema code complete callback with recovery context.

[Eric Liu via cfe-commits]

Sounds good. Thanks for the suggestion!

On Wed, Jul 11, 2018 at 3:45 PM Sam McCall  wrote:

>
>
> On Wed, Jul 11, 2018, 15:18 Eric Liu via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> ioeric added a comment.
>>
>> In https://reviews.llvm.org/D49175#1158562, @sammccall wrote:
>>
>> > I like this idea, of course hard to know how it will affect all
>> practical cases.
>> >
>> > Is it easy to get have internal stats on how many wins/losses this has?
>>
>>
>> This would fix bad completion results when sema could recover to the
>> normal mode (e.g. completion in macros like EXPECT_THAT, expressions in if
>> statements). From the code completion metrics I collected from existing
>> code, the number of completions where no result was found dropped by 30%.
>> The loss seems to be we will get no result in recovery mode rather than bad
>> results (e.g. in excluded preprocessor branches).
>>
> Thanks! I have no doubt this is positive overall, so just thinking out
> loud here.
>
> For changes like this, we can show that quantitavely by running
> before/after completions at the same sites, and reporting e.g.
> Percent of all sites that are better vs worse vs neutral (in terms of rank
> of desired result)
> Change in MRR: mean/std deviation
>
> For our internal quality evaluation tools, we could think about how to
> make it easy to get these numbers for a candidate change.
> Having the raw results as before/after pairs is great for investigating
> wins/losses too.
>
>
>
>>
>> Repository:
>>   rCTE Clang Tools Extra
>>
>> https://reviews.llvm.org/D49175
>>
>>
>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49120: [libc++] P0898R3 2 of 12: Implement header

[Casey Carter via Phabricator via cfe-commits]

CaseyCarter added inline comments.



Comment at: include/concepts:175
+template 
+_LIBCPP_CONCEPT_DECL Same = __same_impl<_Tp, _Up> && __same_impl<_Up, _Tp>;
+

Quuxplusone wrote:
> CaseyCarter wrote:
> > Quuxplusone wrote:
> > > Peanut gallery asks: From lines 166-171 it looks awfully like 
> > > `__same_impl<_Tp, _Up>` is true if and only if `__same_impl<_Up, _Tp>` is 
> > > true. So why bother instantiating both templates?
> > This is the library implementation of the ["`Same` subsumes `Same > T>`" requirement](http://eel.is/c++draft/concept.same#1).
> Ah, so this and tangentially line 280 are both about fiddling with 
> "subsumption." Admittedly I don't know how the compiler side is implemented, 
> but it seems too bad that the library has to do double the work (even at this 
> leafy of a level) just to deal with subsumption, which AIUI is required in 
> order to properly order competing template specializations (a situation that 
> is(?) unlikely to come up in the average end-user's code). :/
> Anyway, carry on.
Ideally, `Same` would be implemented with an intrinsic that tells the compiler 
to ignore the parameter mappings (http://eel.is/c++draft/temp.constr.atomic#2) 
when it checks the atomic constraints defined in this concept for equivalence. 
Or better yet, we could expose that feature in the language - say with a 
context-sensitive keyword `concept symmetric Same` - that could be applied to 
any two-type concept. There are quite a few symmetric concepts that could 
benefit similarly - `Common`, `CommonReference`, all of the `FooWith` concepts 
- for which we don't require subsumption because evaluating them is 
significantly more expensive than `Same`.



Comment at: include/concepts:152
+#if !defined(_LIBCPP_HAS_NO_CONCEPTS) && _LIBCPP_STD_VER > 17
+#define __cpp_lib_concepts 201806L
+

I've just realized I failed to update `` with the feature test macro 
value. Does libc++ have a plan yet for how to handle the feature test macros? 
Should I `#define` it in `` and include that here?

I suspect we should rename `__config` to `` and define all feature 
test macros therein.


https://reviews.llvm.org/D49120



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

r336810 - [clangd] Uprank delcarations when "using q::name" is present in the main file

[Kirill Bobyrev via cfe-commits]

Author: omtcyfz
Date: Wed Jul 11 07:49:49 2018
New Revision: 336810

URL: http://llvm.org/viewvc/llvm-project?rev=336810&view=rev
Log:
[clangd] Uprank delcarations when "using q::name" is present in the main file

Having `using qualified::name;` for some symbol is an important signal
for clangd code completion as the user is more likely to use such
symbol.  This patch helps to uprank the relevant symbols by saving
UsingShadowDecl in the new field of CodeCompletionResult and checking
whether the corresponding UsingShadowDecl is located in the main file
later in ClangD code completion routine. While the relative importance
of such signal is a subject to change in the future, this patch simply
bumps DeclProximity score to the value of 1.0 which should be enough for
now.

The patch was tested using

`$ ninja check-clang check-clang-tools`

No unexpected failures were noticed after running the relevant testsets.

Reviewers: sammccall, ioeric

Subscribers: MaskRay, jkorous, cfe-commits

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

Modified:
cfe/trunk/include/clang/Sema/CodeCompleteConsumer.h
cfe/trunk/lib/Sema/SemaCodeComplete.cpp

Modified: cfe/trunk/include/clang/Sema/CodeCompleteConsumer.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/CodeCompleteConsumer.h?rev=336810&r1=336809&r2=336810&view=diff
==
--- cfe/trunk/include/clang/Sema/CodeCompleteConsumer.h (original)
+++ cfe/trunk/include/clang/Sema/CodeCompleteConsumer.h Wed Jul 11 07:49:49 2018
@@ -45,8 +45,9 @@ class LangOptions;
 class NamedDecl;
 class NestedNameSpecifier;
 class Preprocessor;
-class Sema;
 class RawComment;
+class Sema;
+class UsingShadowDecl;
 
 /// Default priority values for code-completion results based
 /// on their kind.
@@ -836,6 +837,12 @@ public:
   /// informative rather than required.
   NestedNameSpecifier *Qualifier = nullptr;
 
+  /// If this Decl was unshadowed by using declaration, this can store a
+  /// pointer to the UsingShadowDecl which was used in the unshadowing process.
+  /// This information can be used to uprank CodeCompletionResults / which have
+  /// corresponding `using decl::qualified::name;` nearby.
+  const UsingShadowDecl *ShadowDecl = nullptr;
+
   /// Build a result that refers to a declaration.
   CodeCompletionResult(const NamedDecl *Declaration, unsigned Priority,
NestedNameSpecifier *Qualifier = nullptr,
@@ -847,7 +854,7 @@ public:
 QualifierIsInformative(QualifierIsInformative),
 StartsNestedNameSpecifier(false), AllParametersAreInformative(false),
 DeclaringEntity(false), Qualifier(Qualifier) {
-//FIXME: Add assert to check FixIts range requirements.
+// FIXME: Add assert to check FixIts range requirements.
 computeCursorKindAndAvailability(Accessible);
   }
 

Modified: cfe/trunk/lib/Sema/SemaCodeComplete.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCodeComplete.cpp?rev=336810&r1=336809&r2=336810&view=diff
==
--- cfe/trunk/lib/Sema/SemaCodeComplete.cpp (original)
+++ cfe/trunk/lib/Sema/SemaCodeComplete.cpp Wed Jul 11 07:49:49 2018
@@ -859,12 +859,12 @@ void ResultBuilder::MaybeAddResult(Resul
   }
 
   // Look through using declarations.
-  if (const UsingShadowDecl *Using =
-  dyn_cast(R.Declaration)) {
-MaybeAddResult(Result(Using->getTargetDecl(),
-  getBasePriority(Using->getTargetDecl()),
-  R.Qualifier),
-   CurContext);
+  if (const UsingShadowDecl *Using = dyn_cast(R.Declaration)) 
{
+CodeCompletionResult Result(Using->getTargetDecl(),
+getBasePriority(Using->getTargetDecl()),
+R.Qualifier);
+Result.ShadowDecl = Using;
+MaybeAddResult(Result, CurContext);
 return;
   }
 
@@ -977,10 +977,11 @@ void ResultBuilder::AddResult(Result R,
 
   // Look through using declarations.
   if (const UsingShadowDecl *Using = dyn_cast(R.Declaration)) 
{
-AddResult(Result(Using->getTargetDecl(),
- getBasePriority(Using->getTargetDecl()),
- R.Qualifier),
-  CurContext, Hiding);
+CodeCompletionResult Result(Using->getTargetDecl(),
+getBasePriority(Using->getTargetDecl()),
+R.Qualifier);
+Result.ShadowDecl = Using;
+AddResult(Result, CurContext, Hiding);
 return;
   }
 
@@ -1004,10 +1005,10 @@ void ResultBuilder::AddResult(Result R,
   if (AsNestedNameSpecifier) {
 R.StartsNestedNameSpecifier = true;
 R.Priority = CCP_NestedNameSpecifier;
-  }
-  else if (Filter == &ResultBuilder::IsMember && !R.Qualifier && InBaseClass &&
-   isa(R.Declaration->getDeclContext()
-  ->getRedeclContext

[clang-tools-extra] r336810 - [clangd] Uprank delcarations when "using q::name" is present in the main file

[Kirill Bobyrev via cfe-commits]

Author: omtcyfz
Date: Wed Jul 11 07:49:49 2018
New Revision: 336810

URL: http://llvm.org/viewvc/llvm-project?rev=336810&view=rev
Log:
[clangd] Uprank delcarations when "using q::name" is present in the main file

Having `using qualified::name;` for some symbol is an important signal
for clangd code completion as the user is more likely to use such
symbol.  This patch helps to uprank the relevant symbols by saving
UsingShadowDecl in the new field of CodeCompletionResult and checking
whether the corresponding UsingShadowDecl is located in the main file
later in ClangD code completion routine. While the relative importance
of such signal is a subject to change in the future, this patch simply
bumps DeclProximity score to the value of 1.0 which should be enough for
now.

The patch was tested using

`$ ninja check-clang check-clang-tools`

No unexpected failures were noticed after running the relevant testsets.

Reviewers: sammccall, ioeric

Subscribers: MaskRay, jkorous, cfe-commits

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

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

Modified: clang-tools-extra/trunk/clangd/Quality.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Quality.cpp?rev=336810&r1=336809&r2=336810&view=diff
==
--- clang-tools-extra/trunk/clangd/Quality.cpp (original)
+++ clang-tools-extra/trunk/clangd/Quality.cpp Wed Jul 11 07:49:49 2018
@@ -41,6 +41,17 @@ static bool hasDeclInMainFile(const Decl
   return false;
 }
 
+static bool hasUsingDeclInMainFile(const CodeCompletionResult &R) {
+  const auto &Context = R.Declaration->getASTContext();
+  const auto &SourceMgr = Context.getSourceManager();
+  if (R.ShadowDecl) {
+const auto Loc = SourceMgr.getExpansionLoc(R.ShadowDecl->getLocation());
+if (SourceMgr.isWrittenInMainFile(Loc))
+  return true;
+  }
+  return false;
+}
+
 static SymbolQualitySignals::SymbolCategory categorize(const NamedDecl &ND) {
   class Switch
   : public ConstDeclVisitor {
@@ -231,8 +242,10 @@ void SymbolRelevanceSignals::merge(const
 // We boost things that have decls in the main file. We give a fixed score
 // for all other declarations in sema as they are already included in the
 // translation unit.
-float DeclProximity =
-hasDeclInMainFile(*SemaCCResult.Declaration) ? 1.0 : 0.6;
+float DeclProximity = (hasDeclInMainFile(*SemaCCResult.Declaration) ||
+   hasUsingDeclInMainFile(SemaCCResult))
+  ? 1.0
+  : 0.6;
 SemaProximityScore = std::max(DeclProximity, SemaProximityScore);
   }
 

Modified: clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp?rev=336810&r1=336809&r2=336810&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp Wed Jul 11 
07:49:49 2018
@@ -77,18 +77,31 @@ TEST(QualityTests, SymbolQualitySignalEx
 TEST(QualityTests, SymbolRelevanceSignalExtraction) {
   TestTU Test;
   Test.HeaderCode = R"cpp(
-int header();
-int header_main();
-)cpp";
+  int header();
+  int header_main();
+
+  namespace hdr { class Bar {}; } // namespace hdr
+
+  #define DEFINE_FLAG(X) \
+  namespace flags { \
+  int FLAGS_##X; \
+  } \
+
+  DEFINE_FLAG(FOO)
+  )cpp";
   Test.Code = R"cpp(
-int ::header_main() {}
-int main();
+  using hdr::Bar;
+
+  using flags::FLAGS_FOO;
+
+  int ::header_main() {}
+  int main();
 
-[[deprecated]]
-int deprecated() { return 0; }
+  [[deprecated]]
+  int deprecated() { return 0; }
 
-namespace { struct X { void y() { int z; } }; }
-struct S{}
+  namespace { struct X { void y() { int z; } }; }
+  struct S{}
   )cpp";
   auto AST = Test.build();
 
@@ -111,6 +124,32 @@ TEST(QualityTests, SymbolRelevanceSignal
   EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f)
   << "Current file and header";
 
+  auto constructShadowDeclCompletionResult = [&](const std::string DeclName) {
+auto *Shadow =
+*dyn_cast(
+ &findAnyDecl(AST,
+  [&](const NamedDecl &ND) {
+if (const UsingDecl *Using =
+dyn_cast(&ND))
+  if (Using->shadow_size() &&
+  Using->getQualifiedNameAsString() == 
DeclName)
+return true;
+return false;
+  }))
+ ->shadow_begin();
+CodeCompletionResult Result(Shadow->getTargetDecl(), 42);
+Result.ShadowDecl = Shadow;
+return Result;
+  };
+
+  Relevance 

[PATCH] D49188: [OpenMP] Initialize data sharing for SPMD case

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea created this revision.
gtbercea added reviewers: ABataev, carlo.bertolli, caomhin.
Herald added subscribers: cfe-commits, guansong, jholewinski.

In the SPMD case, we need to initialize the data sharing and globalization 
infrastructure. This covers the case when an SPMD region calls a function in a 
different compilation unit.


Repository:
  rC Clang

https://reviews.llvm.org/D49188

Files:
  lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp


Index: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
===
--- lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
+++ lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
@@ -81,6 +81,8 @@
   OMPRTL_NVPTX__kmpc_end_reduce_nowait,
   /// Call to void __kmpc_data_sharing_init_stack();
   OMPRTL_NVPTX__kmpc_data_sharing_init_stack,
+  /// Call to void __kmpc_data_sharing_init_stack_spmd();
+  OMPRTL_NVPTX__kmpc_data_sharing_init_stack_spmd,
   /// Call to void* __kmpc_data_sharing_push_stack(size_t size,
   /// int16_t UseSharedMemory);
   OMPRTL_NVPTX__kmpc_data_sharing_push_stack,
@@ -1025,6 +1027,12 @@
  /*RequiresDataSharing=*/Bld.getInt16(1)};
   CGF.EmitRuntimeCall(
   createNVPTXRuntimeFunction(OMPRTL_NVPTX__kmpc_spmd_kernel_init), Args);
+
+  // For data sharing, we need to initialize the stack.
+  CGF.EmitRuntimeCall(
+  createNVPTXRuntimeFunction(
+  OMPRTL_NVPTX__kmpc_data_sharing_init_stack_spmd));
+
   CGF.EmitBranch(ExecuteBB);
 
   CGF.EmitBlock(ExecuteBB);
@@ -1107,11 +1115,6 @@
   // Wait for parallel work
   syncCTAThreads(CGF);
 
-  // For data sharing, we need to initialize the stack for workers.
-  CGF.EmitRuntimeCall(
-  createNVPTXRuntimeFunction(
-  OMPRTL_NVPTX__kmpc_data_sharing_init_stack));
-
   Address WorkFn =
   CGF.CreateDefaultAlignTempAlloca(CGF.Int8PtrTy, /*Name=*/"work_fn");
   Address ExecStatus =
@@ -1417,6 +1420,13 @@
 RTLFn = CGM.CreateRuntimeFunction(FnTy, "__kmpc_data_sharing_init_stack");
 break;
   }
+  case OMPRTL_NVPTX__kmpc_data_sharing_init_stack_spmd: {
+/// Build void __kmpc_data_sharing_init_stack_spmd();
+auto *FnTy =
+llvm::FunctionType::get(CGM.VoidTy, llvm::None, /*isVarArg*/ false);
+RTLFn = CGM.CreateRuntimeFunction(FnTy, 
"__kmpc_data_sharing_init_stack_spmd");
+break;
+  }
   case OMPRTL_NVPTX__kmpc_data_sharing_push_stack: {
 // Build void *__kmpc_data_sharing_push_stack(size_t size,
 // int16_t UseSharedMemory);


Index: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
===
--- lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
+++ lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
@@ -81,6 +81,8 @@
   OMPRTL_NVPTX__kmpc_end_reduce_nowait,
   /// Call to void __kmpc_data_sharing_init_stack();
   OMPRTL_NVPTX__kmpc_data_sharing_init_stack,
+  /// Call to void __kmpc_data_sharing_init_stack_spmd();
+  OMPRTL_NVPTX__kmpc_data_sharing_init_stack_spmd,
   /// Call to void* __kmpc_data_sharing_push_stack(size_t size,
   /// int16_t UseSharedMemory);
   OMPRTL_NVPTX__kmpc_data_sharing_push_stack,
@@ -1025,6 +1027,12 @@
  /*RequiresDataSharing=*/Bld.getInt16(1)};
   CGF.EmitRuntimeCall(
   createNVPTXRuntimeFunction(OMPRTL_NVPTX__kmpc_spmd_kernel_init), Args);
+
+  // For data sharing, we need to initialize the stack.
+  CGF.EmitRuntimeCall(
+  createNVPTXRuntimeFunction(
+  OMPRTL_NVPTX__kmpc_data_sharing_init_stack_spmd));
+
   CGF.EmitBranch(ExecuteBB);
 
   CGF.EmitBlock(ExecuteBB);
@@ -1107,11 +1115,6 @@
   // Wait for parallel work
   syncCTAThreads(CGF);
 
-  // For data sharing, we need to initialize the stack for workers.
-  CGF.EmitRuntimeCall(
-  createNVPTXRuntimeFunction(
-  OMPRTL_NVPTX__kmpc_data_sharing_init_stack));
-
   Address WorkFn =
   CGF.CreateDefaultAlignTempAlloca(CGF.Int8PtrTy, /*Name=*/"work_fn");
   Address ExecStatus =
@@ -1417,6 +1420,13 @@
 RTLFn = CGM.CreateRuntimeFunction(FnTy, "__kmpc_data_sharing_init_stack");
 break;
   }
+  case OMPRTL_NVPTX__kmpc_data_sharing_init_stack_spmd: {
+/// Build void __kmpc_data_sharing_init_stack_spmd();
+auto *FnTy =
+llvm::FunctionType::get(CGM.VoidTy, llvm::None, /*isVarArg*/ false);
+RTLFn = CGM.CreateRuntimeFunction(FnTy, "__kmpc_data_sharing_init_stack_spmd");
+break;
+  }
   case OMPRTL_NVPTX__kmpc_data_sharing_push_stack: {
 // Build void *__kmpc_data_sharing_push_stack(size_t size,
 // int16_t UseSharedMemory);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49012: [clangd] Uprank delcarations when "using q::name" is present in the main file

[Kirill Bobyrev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE336810: [clangd] Uprank delcarations when "using 
q::name" is present in the main file (authored by omtcyfz, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D49012?vs=154969&id=155001#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49012

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

Index: unittests/clangd/QualityTests.cpp
===
--- unittests/clangd/QualityTests.cpp
+++ unittests/clangd/QualityTests.cpp
@@ -77,18 +77,31 @@
 TEST(QualityTests, SymbolRelevanceSignalExtraction) {
   TestTU Test;
   Test.HeaderCode = R"cpp(
-int header();
-int header_main();
-)cpp";
+  int header();
+  int header_main();
+
+  namespace hdr { class Bar {}; } // namespace hdr
+
+  #define DEFINE_FLAG(X) \
+  namespace flags { \
+  int FLAGS_##X; \
+  } \
+
+  DEFINE_FLAG(FOO)
+  )cpp";
   Test.Code = R"cpp(
-int ::header_main() {}
-int main();
+  using hdr::Bar;
 
-[[deprecated]]
-int deprecated() { return 0; }
+  using flags::FLAGS_FOO;
+
+  int ::header_main() {}
+  int main();
 
-namespace { struct X { void y() { int z; } }; }
-struct S{}
+  [[deprecated]]
+  int deprecated() { return 0; }
+
+  namespace { struct X { void y() { int z; } }; }
+  struct S{}
   )cpp";
   auto AST = Test.build();
 
@@ -111,6 +124,32 @@
   EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f)
   << "Current file and header";
 
+  auto constructShadowDeclCompletionResult = [&](const std::string DeclName) {
+auto *Shadow =
+*dyn_cast(
+ &findAnyDecl(AST,
+  [&](const NamedDecl &ND) {
+if (const UsingDecl *Using =
+dyn_cast(&ND))
+  if (Using->shadow_size() &&
+  Using->getQualifiedNameAsString() == DeclName)
+return true;
+return false;
+  }))
+ ->shadow_begin();
+CodeCompletionResult Result(Shadow->getTargetDecl(), 42);
+Result.ShadowDecl = Shadow;
+return Result;
+  };
+
+  Relevance = {};
+  Relevance.merge(constructShadowDeclCompletionResult("Bar"));
+  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f)
+  << "Using declaration in main file";
+  Relevance.merge(constructShadowDeclCompletionResult("FLAGS_FOO"));
+  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f)
+  << "Using declaration in main file";
+
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findAnyDecl(AST, "X"), 42));
   EXPECT_EQ(Relevance.Scope, SymbolRelevanceSignals::FileScope);
@@ -191,7 +230,8 @@
 }
 
 TEST(QualityTests, SortText) {
-  EXPECT_LT(sortText(std::numeric_limits::infinity()), sortText(1000.2f));
+  EXPECT_LT(sortText(std::numeric_limits::infinity()),
+sortText(1000.2f));
   EXPECT_LT(sortText(1000.2f), sortText(1));
   EXPECT_LT(sortText(1), sortText(0.3f));
   EXPECT_LT(sortText(0.3f), sortText(0));
Index: clangd/Quality.cpp
===
--- clangd/Quality.cpp
+++ clangd/Quality.cpp
@@ -41,6 +41,17 @@
   return false;
 }
 
+static bool hasUsingDeclInMainFile(const CodeCompletionResult &R) {
+  const auto &Context = R.Declaration->getASTContext();
+  const auto &SourceMgr = Context.getSourceManager();
+  if (R.ShadowDecl) {
+const auto Loc = SourceMgr.getExpansionLoc(R.ShadowDecl->getLocation());
+if (SourceMgr.isWrittenInMainFile(Loc))
+  return true;
+  }
+  return false;
+}
+
 static SymbolQualitySignals::SymbolCategory categorize(const NamedDecl &ND) {
   class Switch
   : public ConstDeclVisitor {
@@ -231,8 +242,10 @@
 // We boost things that have decls in the main file. We give a fixed score
 // for all other declarations in sema as they are already included in the
 // translation unit.
-float DeclProximity =
-hasDeclInMainFile(*SemaCCResult.Declaration) ? 1.0 : 0.6;
+float DeclProximity = (hasDeclInMainFile(*SemaCCResult.Declaration) ||
+   hasUsingDeclInMainFile(SemaCCResult))
+  ? 1.0
+  : 0.6;
 SemaProximityScore = std::max(DeclProximity, SemaProximityScore);
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49188: [OpenMP] Initialize data sharing stack for SPMD case

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Tests?


Repository:
  rC Clang

https://reviews.llvm.org/D49188



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

[PATCH] D49188: [OpenMP] Initialize data sharing stack for SPMD case

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

What about the check for the new function call?


Repository:
  rC Clang

https://reviews.llvm.org/D49188



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

[PATCH] D49188: [OpenMP] Initialize data sharing stack for SPMD case

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea updated this revision to Diff 155002.
gtbercea added a comment.

Fix test.


Repository:
  rC Clang

https://reviews.llvm.org/D49188

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


Index: test/OpenMP/nvptx_data_sharing.cpp
===
--- test/OpenMP/nvptx_data_sharing.cpp
+++ test/OpenMP/nvptx_data_sharing.cpp
@@ -30,7 +30,6 @@
 /// = In the worker function = ///
 // CK1: {{.*}}define internal void 
@__omp_offloading{{.*}}test_ds{{.*}}_worker()
 // CK1: call void @llvm.nvvm.barrier0()
-// CK1: call void @__kmpc_data_sharing_init_stack
 
 /// = In the kernel function = ///
 
Index: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
===
--- lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
+++ lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
@@ -81,6 +81,8 @@
   OMPRTL_NVPTX__kmpc_end_reduce_nowait,
   /// Call to void __kmpc_data_sharing_init_stack();
   OMPRTL_NVPTX__kmpc_data_sharing_init_stack,
+  /// Call to void __kmpc_data_sharing_init_stack_spmd();
+  OMPRTL_NVPTX__kmpc_data_sharing_init_stack_spmd,
   /// Call to void* __kmpc_data_sharing_push_stack(size_t size,
   /// int16_t UseSharedMemory);
   OMPRTL_NVPTX__kmpc_data_sharing_push_stack,
@@ -1025,6 +1027,12 @@
  /*RequiresDataSharing=*/Bld.getInt16(1)};
   CGF.EmitRuntimeCall(
   createNVPTXRuntimeFunction(OMPRTL_NVPTX__kmpc_spmd_kernel_init), Args);
+
+  // For data sharing, we need to initialize the stack.
+  CGF.EmitRuntimeCall(
+  createNVPTXRuntimeFunction(
+  OMPRTL_NVPTX__kmpc_data_sharing_init_stack_spmd));
+
   CGF.EmitBranch(ExecuteBB);
 
   CGF.EmitBlock(ExecuteBB);
@@ -1107,11 +1115,6 @@
   // Wait for parallel work
   syncCTAThreads(CGF);
 
-  // For data sharing, we need to initialize the stack for workers.
-  CGF.EmitRuntimeCall(
-  createNVPTXRuntimeFunction(
-  OMPRTL_NVPTX__kmpc_data_sharing_init_stack));
-
   Address WorkFn =
   CGF.CreateDefaultAlignTempAlloca(CGF.Int8PtrTy, /*Name=*/"work_fn");
   Address ExecStatus =
@@ -1417,6 +1420,13 @@
 RTLFn = CGM.CreateRuntimeFunction(FnTy, "__kmpc_data_sharing_init_stack");
 break;
   }
+  case OMPRTL_NVPTX__kmpc_data_sharing_init_stack_spmd: {
+/// Build void __kmpc_data_sharing_init_stack_spmd();
+auto *FnTy =
+llvm::FunctionType::get(CGM.VoidTy, llvm::None, /*isVarArg*/ false);
+RTLFn = CGM.CreateRuntimeFunction(FnTy, 
"__kmpc_data_sharing_init_stack_spmd");
+break;
+  }
   case OMPRTL_NVPTX__kmpc_data_sharing_push_stack: {
 // Build void *__kmpc_data_sharing_push_stack(size_t size,
 // int16_t UseSharedMemory);


Index: test/OpenMP/nvptx_data_sharing.cpp
===
--- test/OpenMP/nvptx_data_sharing.cpp
+++ test/OpenMP/nvptx_data_sharing.cpp
@@ -30,7 +30,6 @@
 /// = In the worker function = ///
 // CK1: {{.*}}define internal void @__omp_offloading{{.*}}test_ds{{.*}}_worker()
 // CK1: call void @llvm.nvvm.barrier0()
-// CK1: call void @__kmpc_data_sharing_init_stack
 
 /// = In the kernel function = ///
 
Index: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
===
--- lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
+++ lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
@@ -81,6 +81,8 @@
   OMPRTL_NVPTX__kmpc_end_reduce_nowait,
   /// Call to void __kmpc_data_sharing_init_stack();
   OMPRTL_NVPTX__kmpc_data_sharing_init_stack,
+  /// Call to void __kmpc_data_sharing_init_stack_spmd();
+  OMPRTL_NVPTX__kmpc_data_sharing_init_stack_spmd,
   /// Call to void* __kmpc_data_sharing_push_stack(size_t size,
   /// int16_t UseSharedMemory);
   OMPRTL_NVPTX__kmpc_data_sharing_push_stack,
@@ -1025,6 +1027,12 @@
  /*RequiresDataSharing=*/Bld.getInt16(1)};
   CGF.EmitRuntimeCall(
   createNVPTXRuntimeFunction(OMPRTL_NVPTX__kmpc_spmd_kernel_init), Args);
+
+  // For data sharing, we need to initialize the stack.
+  CGF.EmitRuntimeCall(
+  createNVPTXRuntimeFunction(
+  OMPRTL_NVPTX__kmpc_data_sharing_init_stack_spmd));
+
   CGF.EmitBranch(ExecuteBB);
 
   CGF.EmitBlock(ExecuteBB);
@@ -1107,11 +1115,6 @@
   // Wait for parallel work
   syncCTAThreads(CGF);
 
-  // For data sharing, we need to initialize the stack for workers.
-  CGF.EmitRuntimeCall(
-  createNVPTXRuntimeFunction(
-  OMPRTL_NVPTX__kmpc_data_sharing_init_stack));
-
   Address WorkFn =
   CGF.CreateDefaultAlignTempAlloca(CGF.Int8PtrTy, /*Name=*/"work_fn");
   Address ExecStatus =
@@ -1417,6 +1420,13 @@
 RTLFn = CGM.CreateRuntimeFunction(FnTy, "__kmpc_data_sharing_init_stack");
 break;
   }
+  case OMPRTL_NVPTX__kmpc_data_sharing_init_stack_spmd: {
+/// Build void __kmpc_data_sharing_init_stack_spmd();
+auto *FnTy =
+llvm::FunctionType::get(CGM.VoidTy, llvm::None, /*isVarArg*/ false);
+

[PATCH] D49190: [clang-tidy/ObjC] Add SQL to list of acronyms

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton created this revision.
benhamilton added reviewers: Wizard, hokein.
Herald added a subscriber: cfe-commits.

SQL is a common acronym.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49190

Files:
  clang-tidy/objc/PropertyDeclarationCheck.cpp


Index: clang-tidy/objc/PropertyDeclarationCheck.cpp
===
--- clang-tidy/objc/PropertyDeclarationCheck.cpp
+++ clang-tidy/objc/PropertyDeclarationCheck.cpp
@@ -98,6 +98,7 @@
 "SC",
 "SDK",
 "SHA",
+"SQL",
 "SSO",
 "TCP",
 "TIFF",


Index: clang-tidy/objc/PropertyDeclarationCheck.cpp
===
--- clang-tidy/objc/PropertyDeclarationCheck.cpp
+++ clang-tidy/objc/PropertyDeclarationCheck.cpp
@@ -98,6 +98,7 @@
 "SC",
 "SDK",
 "SHA",
+"SQL",
 "SSO",
 "TCP",
 "TIFF",
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49188: [OpenMP] Initialize data sharing stack for SPMD case

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld added inline comments.



Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:84-85
   OMPRTL_NVPTX__kmpc_data_sharing_init_stack,
+  /// Call to void __kmpc_data_sharing_init_stack_spmd();
+  OMPRTL_NVPTX__kmpc_data_sharing_init_stack_spmd,
   /// Call to void* __kmpc_data_sharing_push_stack(size_t size,

In case I'm not missing something obvious this function doesn't exist (yet) in 
libomptarget-nvptx?



Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:1110-1114
-  // For data sharing, we need to initialize the stack for workers.
-  CGF.EmitRuntimeCall(
-  createNVPTXRuntimeFunction(
-  OMPRTL_NVPTX__kmpc_data_sharing_init_stack));
-

Why is this call being removed? There's no mention in the summary AFAICS


Repository:
  rC Clang

https://reviews.llvm.org/D49188



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

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

[Nico Weber via Phabricator via cfe-commits]

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

lgtm assuming you ran this on some large code base to make sure it doesn't have 
false positives.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:659
+def warn_suspicious_sizeof_memset : Warning<
+  "%select{'size' argument to memset is '0'|setting buffer to a 'sizeof' 
expression}0; did you mean to transpose the last two arguments?">,
+  InGroup;

nit: stay below 80 columns


https://reviews.llvm.org/D49112



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

r336817 - [AST] Fix for structural equivalence tests in rL336776.

[Balazs Keri via cfe-commits]

Author: balazske
Date: Wed Jul 11 08:26:26 2018
New Revision: 336817

URL: http://llvm.org/viewvc/llvm-project?rev=336817&view=rev
Log:
[AST] Fix for structural equivalence tests in rL336776.

Modified:
cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp

Modified: cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp?rev=336817&r1=336816&r2=336817&view=diff
==
--- cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp (original)
+++ cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp Wed Jul 11 08:26:26 
2018
@@ -404,14 +404,14 @@ TEST_F(StructuralEquivalenceCXXMethodTes
 TEST_F(StructuralEquivalenceCXXMethodTest, Constructor) {
   auto t = makeDecls(
   "void foo();", "struct foo { foo(); };", Lang_CXX,
-  functionDecl(), cxxConstructorDecl());
+  functionDecl(hasName("foo")), cxxConstructorDecl(hasName("foo")));
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
 TEST_F(StructuralEquivalenceCXXMethodTest, ConstructorParam) {
   auto t = makeDecls("struct X { X(); };",
  "struct X { X(int); };", Lang_CXX,
- cxxConstructorDecl());
+ cxxConstructorDecl(hasName("X")));
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
@@ -419,7 +419,7 @@ TEST_F(StructuralEquivalenceCXXMethodTes
   auto t = makeDecls("struct X { X(int); };",
  "struct X { explicit X(int); };",
  Lang_CXX11,
- cxxConstructorDecl());
+ cxxConstructorDecl(hasName("X")));
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
@@ -427,7 +427,7 @@ TEST_F(StructuralEquivalenceCXXMethodTes
   auto t = makeDecls("struct X { X(); };",
  "struct X { X() = default; };",
  Lang_CXX11,
- cxxConstructorDecl());
+ cxxConstructorDecl(hasName("X")));
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
@@ -474,7 +474,8 @@ TEST_F(StructuralEquivalenceRecordTest,
   "struct A{ };",
   "struct B{ };",
   Lang_CXX,
-  cxxRecordDecl());
+  cxxRecordDecl(hasName("A")),
+  cxxRecordDecl(hasName("B")));
   EXPECT_FALSE(testStructuralMatch(t));
 }
 


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

[PATCH] D47894: [clang]: Add support for "-fno-delete-null-pointer-checks"

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added a comment.

@efriedma @jyknight Does the change match your expectations where warnings are 
still generated but codeGen does not emit nonnull attribute?




Comment at: test/Sema/nonnull.c:2
 // RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fno-delete-null-pointer-checks -verify %s
 // rdar://9584012

All warnings are still issued if nullptr is passed to function nonnull 
attribute.


Repository:
  rC Clang

https://reviews.llvm.org/D47894



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

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

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Ping. Added reviewers suggestion.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48559



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

[PATCH] D36892: [clang-tidy] check_clang_tidy.py: support CHECK-NOTES prefix

[Alexander Kornienko via Phabricator via cfe-commits]

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

As per the previous comment: I have no concerns as long as the documentation is 
updated and at least one existing test is changed to use this feature (see the 
list in the previous comment).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D36892



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

[PATCH] D48395: Added PublicOnly flag

[Julie Hockett via Phabricator via cfe-commits]

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

Remember to mark comments as done when they are. Otherwise, LGTM unless @ioeric 
has any concerns.


https://reviews.llvm.org/D48395



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

[PATCH] D33537: [clang-tidy] Exception Escape Checker

[Alexander Kornienko via Phabricator via cfe-commits]

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

Looks good with one comment.




Comment at: test/clang-tidy/bugprone-exception-escape.cpp:178
+void indirect_implicit() noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in 
function 'indirect_implicit' which should not throw exceptions
+  implicit_int_thrower();

baloghadamsoftware wrote:
> dberris wrote:
> > Can we make the warning more accurate here? Something like:
> > 
> > ```
> > warning: call to 'implicit_int_thrower' may throw an exception and 
> > propagate through noexcept function 'indirect_implicit'
> > ```
> > 
> > It would be helpful to diagnose the point at which the exception may be 
> > thrown from within the function (if it's an operator, a function call, 
> > etc.) that doesn't have exceptions handled. If you can highlight not just 
> > the line number but the actual expression in which the uncaught exception 
> > may propagate, it would make this warning much better.
> > 
> > If you think it's worth it (or if it's feasible), having a FixIt hint to 
> > wrap a block of statements where exceptions may propagate in a `try { ... } 
> > catch (...) { ... }` block would turn this warning from a good warning, to 
> > a great warning -- potentially something that could be automatically 
> > applied by a tool as well.
> I think this is a good future improvement. However, I tried to make the first 
> version as small as possible, and then enhance it incrementally. Even the 
> current code is too big for a Clang-Tidy check.
Could you add a relevant FIXME comment next to the diag() call in the check?


https://reviews.llvm.org/D33537



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

[PATCH] D42682: [clang-tidy] Add io-functions-misused checker

[Alexander Kornienko via Phabricator via cfe-commits]

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

(removing from my dashboard)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42682



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

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

[Simon Marchi via Phabricator via cfe-commits]

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

I noticed that when getVirtualFile is called for a file, subsequent
calls to getFile will return a FileEntry without the RealPathName
computed:

  const FileEntry *VFE = Mgr->getVirtualFile("/foo/../bar", ...);
  const FileEntry *FE = Mgr->getFile("/foo/../bar");
  
  FE->tryGetRealPathName() returns an empty string.

This is because getVirtualFile creates a FileEntry without computing the
RealPathName field and stores it in SeenFileEntries.  getFile then
simply retrieves this object and returns it, when asked for the same
file.

I think it's reasonable to try to compute RealPathName in getVirtualFile
too.

There might be a similar issue with the File field.  If you call
getVirtualFile, then call getFile with open=true, you may get a
FileEntry with the File field NULL, which is not what you requested.  I
have not addressed this issue in this patch though.


Repository:
  rC Clang

https://reviews.llvm.org/D49197

Files:
  lib/Basic/FileManager.cpp
  unittests/Basic/FileManagerTest.cpp


Index: unittests/Basic/FileManagerTest.cpp
===
--- unittests/Basic/FileManagerTest.cpp
+++ unittests/Basic/FileManagerTest.cpp
@@ -322,4 +322,40 @@
   EXPECT_EQ(Path, ExpectedResult);
 }
 
+/// When calling getVirtualFile then getFile for the same file, the real path
+/// should be computed (in other words, getVirtualFile should compute the real
+/// path).
+TEST_F(FileManagerTest, getVirtualFileThenGetFileRealPathName) {
+  SmallString<64> RealPath;
+  SmallString<64> NonRealPath;
+  SmallString<64> WorkingDirectory;
+
+#ifdef _WIN32
+  RealPath = "C:\\a\\b";
+  NonRealPath = "C:\\a\\..\\a\\b";
+  WorkingDirectory = "C:\\";
+#else
+  RealPath = "/a/b";
+  NonRealPath = "/a/../a/b";
+  WorkingDirectory = "/";
+#endif
+
+  auto FS =
+  IntrusiveRefCntPtr(new vfs::InMemoryFileSystem);
+  ASSERT_TRUE(!FS->setCurrentWorkingDirectory(WorkingDirectory));
+
+  FS->addFile(RealPath, 0, llvm::MemoryBuffer::getMemBuffer(""));
+
+  FileSystemOptions Opts;
+  FileManager Manager(Opts, FS);
+
+  const FileEntry *VFE = Manager.getVirtualFile(NonRealPath, 0, 0);
+  ASSERT_TRUE(VFE != nullptr);
+  EXPECT_EQ(VFE->tryGetRealPathName(), RealPath);
+
+  const FileEntry *FE = Manager.getFile(NonRealPath);
+  ASSERT_TRUE(FE != nullptr);
+  EXPECT_EQ(FE->tryGetRealPathName(), RealPath);
+}
+
 } // anonymous namespace
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -390,6 +390,11 @@
   UFE->UID = NextFileUID++;
   UFE->IsValid = true;
   UFE->File.reset();
+
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+UFE->RealPathName = RealPathName.str();
+
   return UFE;
 }
 


Index: unittests/Basic/FileManagerTest.cpp
===
--- unittests/Basic/FileManagerTest.cpp
+++ unittests/Basic/FileManagerTest.cpp
@@ -322,4 +322,40 @@
   EXPECT_EQ(Path, ExpectedResult);
 }
 
+/// When calling getVirtualFile then getFile for the same file, the real path
+/// should be computed (in other words, getVirtualFile should compute the real
+/// path).
+TEST_F(FileManagerTest, getVirtualFileThenGetFileRealPathName) {
+  SmallString<64> RealPath;
+  SmallString<64> NonRealPath;
+  SmallString<64> WorkingDirectory;
+
+#ifdef _WIN32
+  RealPath = "C:\\a\\b";
+  NonRealPath = "C:\\a\\..\\a\\b";
+  WorkingDirectory = "C:\\";
+#else
+  RealPath = "/a/b";
+  NonRealPath = "/a/../a/b";
+  WorkingDirectory = "/";
+#endif
+
+  auto FS =
+  IntrusiveRefCntPtr(new vfs::InMemoryFileSystem);
+  ASSERT_TRUE(!FS->setCurrentWorkingDirectory(WorkingDirectory));
+
+  FS->addFile(RealPath, 0, llvm::MemoryBuffer::getMemBuffer(""));
+
+  FileSystemOptions Opts;
+  FileManager Manager(Opts, FS);
+
+  const FileEntry *VFE = Manager.getVirtualFile(NonRealPath, 0, 0);
+  ASSERT_TRUE(VFE != nullptr);
+  EXPECT_EQ(VFE->tryGetRealPathName(), RealPath);
+
+  const FileEntry *FE = Manager.getFile(NonRealPath);
+  ASSERT_TRUE(FE != nullptr);
+  EXPECT_EQ(FE->tryGetRealPathName(), RealPath);
+}
+
 } // anonymous namespace
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -390,6 +390,11 @@
   UFE->UID = NextFileUID++;
   UFE->IsValid = true;
   UFE->File.reset();
+
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+UFE->RealPathName = RealPathName.str();
+
   return UFE;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 155027.
simark added a comment.

Update commit message.


Repository:
  rC Clang

https://reviews.llvm.org/D49197

Files:
  lib/Basic/FileManager.cpp
  unittests/Basic/FileManagerTest.cpp


Index: unittests/Basic/FileManagerTest.cpp
===
--- unittests/Basic/FileManagerTest.cpp
+++ unittests/Basic/FileManagerTest.cpp
@@ -322,4 +322,40 @@
   EXPECT_EQ(Path, ExpectedResult);
 }
 
+/// When calling getVirtualFile then getFile for the same file, the real path
+/// should be computed (in other words, getVirtualFile should compute the real
+/// path).
+TEST_F(FileManagerTest, getVirtualFileThenGetFileRealPathName) {
+  SmallString<64> RealPath;
+  SmallString<64> NonRealPath;
+  SmallString<64> WorkingDirectory;
+
+#ifdef _WIN32
+  RealPath = "C:\\a\\b";
+  NonRealPath = "C:\\a\\..\\a\\b";
+  WorkingDirectory = "C:\\";
+#else
+  RealPath = "/a/b";
+  NonRealPath = "/a/../a/b";
+  WorkingDirectory = "/";
+#endif
+
+  auto FS =
+  IntrusiveRefCntPtr(new vfs::InMemoryFileSystem);
+  ASSERT_TRUE(!FS->setCurrentWorkingDirectory(WorkingDirectory));
+
+  FS->addFile(RealPath, 0, llvm::MemoryBuffer::getMemBuffer(""));
+
+  FileSystemOptions Opts;
+  FileManager Manager(Opts, FS);
+
+  const FileEntry *VFE = Manager.getVirtualFile(NonRealPath, 0, 0);
+  ASSERT_TRUE(VFE != nullptr);
+  EXPECT_EQ(VFE->tryGetRealPathName(), RealPath);
+
+  const FileEntry *FE = Manager.getFile(NonRealPath);
+  ASSERT_TRUE(FE != nullptr);
+  EXPECT_EQ(FE->tryGetRealPathName(), RealPath);
+}
+
 } // anonymous namespace
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -390,6 +390,11 @@
   UFE->UID = NextFileUID++;
   UFE->IsValid = true;
   UFE->File.reset();
+
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+UFE->RealPathName = RealPathName.str();
+
   return UFE;
 }
 


Index: unittests/Basic/FileManagerTest.cpp
===
--- unittests/Basic/FileManagerTest.cpp
+++ unittests/Basic/FileManagerTest.cpp
@@ -322,4 +322,40 @@
   EXPECT_EQ(Path, ExpectedResult);
 }
 
+/// When calling getVirtualFile then getFile for the same file, the real path
+/// should be computed (in other words, getVirtualFile should compute the real
+/// path).
+TEST_F(FileManagerTest, getVirtualFileThenGetFileRealPathName) {
+  SmallString<64> RealPath;
+  SmallString<64> NonRealPath;
+  SmallString<64> WorkingDirectory;
+
+#ifdef _WIN32
+  RealPath = "C:\\a\\b";
+  NonRealPath = "C:\\a\\..\\a\\b";
+  WorkingDirectory = "C:\\";
+#else
+  RealPath = "/a/b";
+  NonRealPath = "/a/../a/b";
+  WorkingDirectory = "/";
+#endif
+
+  auto FS =
+  IntrusiveRefCntPtr(new vfs::InMemoryFileSystem);
+  ASSERT_TRUE(!FS->setCurrentWorkingDirectory(WorkingDirectory));
+
+  FS->addFile(RealPath, 0, llvm::MemoryBuffer::getMemBuffer(""));
+
+  FileSystemOptions Opts;
+  FileManager Manager(Opts, FS);
+
+  const FileEntry *VFE = Manager.getVirtualFile(NonRealPath, 0, 0);
+  ASSERT_TRUE(VFE != nullptr);
+  EXPECT_EQ(VFE->tryGetRealPathName(), RealPath);
+
+  const FileEntry *FE = Manager.getFile(NonRealPath);
+  ASSERT_TRUE(FE != nullptr);
+  EXPECT_EQ(FE->tryGetRealPathName(), RealPath);
+}
+
 } // anonymous namespace
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -390,6 +390,11 @@
   UFE->UID = NextFileUID++;
   UFE->IsValid = true;
   UFE->File.reset();
+
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+UFE->RealPathName = RealPathName.str();
+
   return UFE;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

I'm not sure who should review this patch, please add anybody you think is 
qualified/responsible.


Repository:
  rC Clang

https://reviews.llvm.org/D49197



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

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

Background:  I'm trying to fix the cases why we receive a FileEntry without a 
real path computed in clangd, so we can avoid having to compute it ourselves.


Repository:
  rC Clang

https://reviews.llvm.org/D49197



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

[PATCH] D49190: [clang-tidy/ObjC] Add SQL to list of acronyms

[Haojian Wu via Phabricator via cfe-commits]

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

LGTM


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49190



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

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D48903#1155403, @ilya-biryukov wrote:

> In https://reviews.llvm.org/D48903#1154846, @simark wrote:
>
> > With the `InMemoryFileSystem`, this now returns a non-real path.  The 
> > result is that we fill `RealPathName` with that non-real path.  I see two 
> > options here:
> >
> > 1. Either the FileManager is wrong to assume that `File::getName` returns a 
> > real path, and should call `FS->getRealPath` to do the job.
> > 2. If the contract is that the ` File::getName` interface should return a 
> > real path, then we should fix the `File::getName` implementation to do that 
> > somehow.
> >
> >   I would opt for 1.  This way, we could compute the `RealPathName` field 
> > even if we don't open the file (unlike currently).
>
>
> I'd also say `FileManager` should use `FileSystem::getRealPath`. The code 
> that does it differently was there before `FileSystem::getRealPath` was added.
>  And `RealFile` should probably not do any magic in `getName`, we could add a 
> separate method for (`getRealName`?) if that's absolutely needed.
>
> Refactorings in that area would probably break stuff and won't be trivial and 
> I don't think this change should be blocked by those. So I'd be happy if this 
> landed right away with a FIXME in `FileManager` mentioning that 
> `InMemoryFileSystem` might give surprising results there.
>  @ioeric added `FileSystem::getRealPath`, he may more ideas on how we should 
> proceed.


Sorry for being late in the discussion. I'm not sure if `getRealPath` is the 
right thing to do in `FileManager` as it also supports virtual files added via 
`FileManager::getVirtualFile`, and they don't necessary have a real path. This 
would also break users of `ClangTool::mapVirtualFile` when the mapped files are 
relative. As a matter of fact, I'm already seeing related breakages in our 
downstream branch :(

Would you mind reverting this patch for now so that we can come up with a 
solution to address those use cases?

Sorry again about missing the discussion earlier!


Repository:
  rC Clang

https://reviews.llvm.org/D48903



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

Re: r336776 - [AST] Structural equivalence of methods

[Galina Kistanova via cfe-commits]

Hello Balazs,

This commit broke at least one of our builders:
http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/10897

. . .
Failing Tests (1):
Clang-Unit :: AST/./ASTTests.exe/StructuralEquivalenceRecordTest.Name

Please have a look?
It is not good idea to keep the bot red for too long. This hides new
problem which later hard to track down.

Thanks

Galina


On Wed, Jul 11, 2018 at 2:37 AM, Balazs Keri via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: balazske
> Date: Wed Jul 11 02:37:24 2018
> New Revision: 336776
>
> URL: http://llvm.org/viewvc/llvm-project?rev=336776&view=rev
> Log:
> [AST] Structural equivalence of methods
>
> Summary:
> Added structural equivalence check for C++ methods.
> Improved structural equivalence tests.
> Added related ASTImporter tests.
>
> Reviewers: a.sidorin, szepet, xazax.hun, martong, a_sidorin
>
> Reviewed By: martong, a_sidorin
>
> Subscribers: a_sidorin, rnkovacs, cfe-commits
>
> Differential Revision: https://reviews.llvm.org/D48628
>
> Modified:
> cfe/trunk/lib/AST/ASTImporter.cpp
> cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
> cfe/trunk/unittests/AST/ASTImporterTest.cpp
> cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp
>
> Modified: cfe/trunk/lib/AST/ASTImporter.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/
> ASTImporter.cpp?rev=336776&r1=336775&r2=336776&view=diff
> 
> ==
> --- cfe/trunk/lib/AST/ASTImporter.cpp (original)
> +++ cfe/trunk/lib/AST/ASTImporter.cpp Wed Jul 11 02:37:24 2018
> @@ -230,6 +230,7 @@ namespace clang {
>  bool IsStructuralMatch(EnumConstantDecl *FromEC, EnumConstantDecl
> *ToEC);
>  bool IsStructuralMatch(FunctionTemplateDecl *From,
> FunctionTemplateDecl *To);
> +bool IsStructuralMatch(FunctionDecl *From, FunctionDecl *To);
>  bool IsStructuralMatch(ClassTemplateDecl *From, ClassTemplateDecl
> *To);
>  bool IsStructuralMatch(VarTemplateDecl *From, VarTemplateDecl *To);
>  Decl *VisitDecl(Decl *D);
> @@ -1525,6 +1526,13 @@ bool ASTNodeImporter::IsStructuralMatch(
>return Ctx.IsStructurallyEquivalent(From, To);
>  }
>
> +bool ASTNodeImporter::IsStructuralMatch(FunctionDecl *From, FunctionDecl
> *To) {
> +  StructuralEquivalenceContext Ctx(
> +  Importer.getFromContext(), Importer.getToContext(),
> +  Importer.getNonEquivalentDecls(), false, false);
> +  return Ctx.IsStructurallyEquivalent(From, To);
> +}
> +
>  bool ASTNodeImporter::IsStructuralMatch(EnumConstantDecl *FromEC,
>  EnumConstantDecl *ToEC) {
>const llvm::APSInt &FromVal = FromEC->getInitVal();
> @@ -2433,13 +2441,15 @@ Decl *ASTNodeImporter::VisitFunctionDecl
>if (auto *FoundFunction = dyn_cast(FoundDecl)) {
>  if (FoundFunction->hasExternalFormalLinkage() &&
>  D->hasExternalFormalLinkage()) {
> -  if (Importer.IsStructurallyEquivalent(D->getType(),
> -
> FoundFunction->getType())) {
> -  if (D->doesThisDeclarationHaveABody() &&
> -  FoundFunction->hasBody())
> -return Importer.Imported(D, FoundFunction);
> -  FoundByLookup = FoundFunction;
> -  break;
> +  if (IsStructuralMatch(D, FoundFunction)) {
> +const FunctionDecl *Definition = nullptr;
> +if (D->doesThisDeclarationHaveABody() &&
> +FoundFunction->hasBody(Definition)) {
> +  return Importer.Imported(
> +  D, const_cast(Definition));
> +}
> +FoundByLookup = FoundFunction;
> +break;
>}
>
>// FIXME: Check for overloading more carefully, e.g., by
> boosting
>
> Modified: cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/
> ASTStructuralEquivalence.cpp?rev=336776&r1=336775&r2=336776&view=diff
> 
> ==
> --- cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp (original)
> +++ cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp Wed Jul 11 02:37:24
> 2018
> @@ -250,6 +250,9 @@ static bool IsStructurallyEquivalent(Str
>if (T1.isNull() || T2.isNull())
>  return T1.isNull() && T2.isNull();
>
> +  QualType OrigT1 = T1;
> +  QualType OrigT2 = T2;
> +
>if (!Context.StrictTypeSpelling) {
>  // We aren't being strict about token-to-token equivalence of types,
>  // so map down to the canonical type.
> @@ -422,6 +425,7 @@ static bool IsStructurallyEquivalent(Str
>case Type::FunctionProto: {
>  const auto *Proto1 = cast(T1);
>  const auto *Proto2 = cast(T2);
> +
>  if (Proto1->getNumParams() != Proto2->getNumParams())
>return false;
>  for (unsigned I = 0, N = Proto1->getNumParams(); I != N; ++I) {
> @@ -431,23 +435,33 @@ static bool IsStructurallyEquivalent(Str

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D48903#1159023, @ioeric wrote:

> In https://reviews.llvm.org/D48903#1155403, @ilya-biryukov wrote:
>
> > In https://reviews.llvm.org/D48903#1154846, @simark wrote:
> >
> > > With the `InMemoryFileSystem`, this now returns a non-real path.  The 
> > > result is that we fill `RealPathName` with that non-real path.  I see two 
> > > options here:
> > >
> > > 1. Either the FileManager is wrong to assume that `File::getName` returns 
> > > a real path, and should call `FS->getRealPath` to do the job.
> > > 2. If the contract is that the ` File::getName` interface should return a 
> > > real path, then we should fix the `File::getName` implementation to do 
> > > that somehow.
> > >
> > >   I would opt for 1.  This way, we could compute the `RealPathName` field 
> > > even if we don't open the file (unlike currently).
> >
> >
> > I'd also say `FileManager` should use `FileSystem::getRealPath`. The code 
> > that does it differently was there before `FileSystem::getRealPath` was 
> > added.
> >  And `RealFile` should probably not do any magic in `getName`, we could add 
> > a separate method for (`getRealName`?) if that's absolutely needed.
> >
> > Refactorings in that area would probably break stuff and won't be trivial 
> > and I don't think this change should be blocked by those. So I'd be happy 
> > if this landed right away with a FIXME in `FileManager` mentioning that 
> > `InMemoryFileSystem` might give surprising results there.
> >  @ioeric added `FileSystem::getRealPath`, he may more ideas on how we 
> > should proceed.
>
>
> Sorry for being late in the discussion. I'm not sure if `getRealPath` is the 
> right thing to do in `FileManager` as it also supports virtual files added 
> via `FileManager::getVirtualFile`, and they don't necessary have a real path. 
> This would also break users of `ClangTool::mapVirtualFile` when the mapped 
> files are relative. As a matter of fact, I'm already seeing related breakages 
> in our downstream branch :(
>
> Would you mind reverting this patch for now so that we can come up with a 
> solution to address those use cases?
>
> Sorry again about missing the discussion earlier!


Sorry, having taken a closer look, it seems that only 
`ClangTool::mapVirtualFile` would be affected. I'll try to see if there is an 
easy fix.


Repository:
  rC Clang

https://reviews.llvm.org/D48903



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

Re: r336807 - [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Galina Kistanova via cfe-commits]

Hello Simon,

This commit added broken tests to one of our builders:
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/18386/steps/test/logs/stdio
. . .
Failing Tests (4):
Clang :: PCH/case-insensitive-include.c
Clang-Unit ::
Basic/./BasicTests.exe/InMemoryFileSystemTest.DirectoryIteration
Clang-Unit :: Basic/./BasicTests.exe/InMemoryFileSystemTest.StatusName
LLVM :: MC/ELF/debug-prefix-map.s

Please have a look?
The builder was already red and did not send notifications.

Thanks

Galina

On Wed, Jul 11, 2018 at 7:08 AM, Simon Marchi via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: simark
> Date: Wed Jul 11 07:08:17 2018
> New Revision: 336807
>
> URL: http://llvm.org/viewvc/llvm-project?rev=336807&view=rev
> Log:
> [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the
> requested name
>
> Summary:
> InMemoryFileSystem::status behaves differently than
> RealFileSystem::status.  The Name contained in the Status returned by
> RealFileSystem::status will be the path as requested by the caller,
> whereas InMemoryFileSystem::status returns the normalized path.
>
> For example, when requested the status for "../src/first.h",
> RealFileSystem returns a Status with "../src/first.h" as the Name.
> InMemoryFileSystem returns "/absolute/path/to/src/first.h".
>
> The reason for this change is that I want to make a unit test in the
> clangd testsuite (where we use an InMemoryFileSystem) to reproduce a
> bug I get with the clangd program (where a RealFileSystem is used).
> This difference in behavior "hides" the bug in the unit test version.
>
> In general, I guess it's good if InMemoryFileSystem works as much as
> possible like RealFileSystem.
>
> Doing so made the FileEntry::RealPathName value (assigned in
> FileManager::getFile) wrong when using the InMemoryFileSystem.  That's
> because it assumes that vfs::File::getName will always return the real
> path.  I changed to to use FileSystem::getRealPath instead.
>
> Subscribers: ilya-biryukov, ioeric, cfe-commits
>
> Differential Revision: https://reviews.llvm.org/D48903
>
> Modified:
> cfe/trunk/lib/Basic/FileManager.cpp
> cfe/trunk/lib/Basic/VirtualFileSystem.cpp
> cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp
> cfe/trunk/unittests/Driver/ToolChainTest.cpp
>
> Modified: cfe/trunk/lib/Basic/FileManager.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/
> FileManager.cpp?rev=336807&r1=336806&r2=336807&view=diff
> 
> ==
> --- cfe/trunk/lib/Basic/FileManager.cpp (original)
> +++ cfe/trunk/lib/Basic/FileManager.cpp Wed Jul 11 07:08:17 2018
> @@ -315,9 +315,11 @@ const FileEntry *FileManager::getFile(St
>UFE.InPCH = Data.InPCH;
>UFE.File = std::move(F);
>UFE.IsValid = true;
> -  if (UFE.File)
> -if (auto RealPathName = UFE.File->getName())
> -  UFE.RealPathName = *RealPathName;
> +
> +  SmallString<128> RealPathName;
> +  if (!FS->getRealPath(InterndFileName, RealPathName))
> +UFE.RealPathName = RealPathName.str();
> +
>return &UFE;
>  }
>
>
> Modified: cfe/trunk/lib/Basic/VirtualFileSystem.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/
> VirtualFileSystem.cpp?rev=336807&r1=336806&r2=336807&view=diff
> 
> ==
> --- cfe/trunk/lib/Basic/VirtualFileSystem.cpp (original)
> +++ cfe/trunk/lib/Basic/VirtualFileSystem.cpp Wed Jul 11 07:08:17 2018
> @@ -471,15 +471,33 @@ enum InMemoryNodeKind { IME_File, IME_Di
>  /// The in memory file system is a tree of Nodes. Every node can either
> be a
>  /// file or a directory.
>  class InMemoryNode {
> -  Status Stat;
>InMemoryNodeKind Kind;
> +  Status Stat;
> +
> +protected:
> +  /// Return Stat.  This should only be used for internal/debugging use.
> When
> +  /// clients wants the Status of this node, they should use
> +  /// \p getStatus(StringRef).
> +  const Status& getStatus() const {
> +return Stat;
> +  }
>
>  public:
>InMemoryNode(Status Stat, InMemoryNodeKind Kind)
> -  : Stat(std::move(Stat)), Kind(Kind) {}
> +  : Kind(Kind), Stat(std::move(Stat)) {}
>virtual ~InMemoryNode() = default;
>
> -  const Status &getStatus() const { return Stat; }
> +  /// Return the \p Status for this node. \p RequestedName should be the
> name
> +  /// through which the caller referred to this node. It will override
> +  /// \p Status::Name in the return value, to mimic the behavior of \p
> RealFile.
> +  Status getStatus(StringRef RequestedName) const {
> +return Status::copyWithNewName(Stat, RequestedName);
> +  }
> +
> +  /// Get the filename of this node (the name without the directory part).
> +  StringRef getFileName() const {
> +return llvm::sys::path::filename(Stat.getName());
> +  }
>InMemoryNodeKind getKind() const { return Kind; }
>virtual std::string toString(unsigned Indent)

[PATCH] D49188: [OpenMP] Initialize data sharing stack for SPMD case

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea updated this revision to Diff 155037.
gtbercea added a comment.

Add test for spmd stack init function.


Repository:
  rC Clang

https://reviews.llvm.org/D49188

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


Index: test/OpenMP/nvptx_data_sharing_spmd.cpp
===
--- /dev/null
+++ test/OpenMP/nvptx_data_sharing_spmd.cpp
@@ -0,0 +1,24 @@
+// Test device global memory data sharing initialization codegen for spmd.
+///==///
+
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple powerpc64le-unknown-unknown 
-fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple nvptx64-unknown-unknown 
-fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device 
-fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck %s --check-prefix CK1
+
+// expected-no-diagnostics
+
+#ifndef HEADER
+#define HEADER
+
+void test_ds(){
+  #pragma omp target teams distribute parallel for
+  for (int i = 0; i < 100; i++ ) {
+int a = 100;
+  }
+}
+
+// CK1: {{.*}}define weak void @__omp_offloading{{.*}}test_ds{{.*}}()
+// CK1: call void @__kmpc_spmd_kernel_init
+// CK1-NEXT: call void @__kmpc_data_sharing_init_stack_spmd
+
+#endif
+
Index: test/OpenMP/nvptx_data_sharing.cpp
===
--- test/OpenMP/nvptx_data_sharing.cpp
+++ test/OpenMP/nvptx_data_sharing.cpp
@@ -30,7 +30,6 @@
 /// = In the worker function = ///
 // CK1: {{.*}}define internal void 
@__omp_offloading{{.*}}test_ds{{.*}}_worker()
 // CK1: call void @llvm.nvvm.barrier0()
-// CK1: call void @__kmpc_data_sharing_init_stack
 
 /// = In the kernel function = ///
 
Index: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
===
--- lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
+++ lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
@@ -81,6 +81,8 @@
   OMPRTL_NVPTX__kmpc_end_reduce_nowait,
   /// Call to void __kmpc_data_sharing_init_stack();
   OMPRTL_NVPTX__kmpc_data_sharing_init_stack,
+  /// Call to void __kmpc_data_sharing_init_stack_spmd();
+  OMPRTL_NVPTX__kmpc_data_sharing_init_stack_spmd,
   /// Call to void* __kmpc_data_sharing_push_stack(size_t size,
   /// int16_t UseSharedMemory);
   OMPRTL_NVPTX__kmpc_data_sharing_push_stack,
@@ -1025,6 +1027,12 @@
  /*RequiresDataSharing=*/Bld.getInt16(1)};
   CGF.EmitRuntimeCall(
   createNVPTXRuntimeFunction(OMPRTL_NVPTX__kmpc_spmd_kernel_init), Args);
+
+  // For data sharing, we need to initialize the stack.
+  CGF.EmitRuntimeCall(
+  createNVPTXRuntimeFunction(
+  OMPRTL_NVPTX__kmpc_data_sharing_init_stack_spmd));
+
   CGF.EmitBranch(ExecuteBB);
 
   CGF.EmitBlock(ExecuteBB);
@@ -1107,11 +1115,6 @@
   // Wait for parallel work
   syncCTAThreads(CGF);
 
-  // For data sharing, we need to initialize the stack for workers.
-  CGF.EmitRuntimeCall(
-  createNVPTXRuntimeFunction(
-  OMPRTL_NVPTX__kmpc_data_sharing_init_stack));
-
   Address WorkFn =
   CGF.CreateDefaultAlignTempAlloca(CGF.Int8PtrTy, /*Name=*/"work_fn");
   Address ExecStatus =
@@ -1417,6 +1420,13 @@
 RTLFn = CGM.CreateRuntimeFunction(FnTy, "__kmpc_data_sharing_init_stack");
 break;
   }
+  case OMPRTL_NVPTX__kmpc_data_sharing_init_stack_spmd: {
+/// Build void __kmpc_data_sharing_init_stack_spmd();
+auto *FnTy =
+llvm::FunctionType::get(CGM.VoidTy, llvm::None, /*isVarArg*/ false);
+RTLFn = CGM.CreateRuntimeFunction(FnTy, 
"__kmpc_data_sharing_init_stack_spmd");
+break;
+  }
   case OMPRTL_NVPTX__kmpc_data_sharing_push_stack: {
 // Build void *__kmpc_data_sharing_push_stack(size_t size,
 // int16_t UseSharedMemory);


Index: test/OpenMP/nvptx_data_sharing_spmd.cpp
===
--- /dev/null
+++ test/OpenMP/nvptx_data_sharing_spmd.cpp
@@ -0,0 +1,24 @@
+// Test device global memory data sharing initialization codegen for spmd.
+///==///
+
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple nvptx64-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck %s --check-prefix CK1
+
+// expected-no-diagnostics
+
+#ifndef HEADER
+#define HEADER
+
+void test_ds(){
+  #pragma omp target teams distribute parallel for
+  for (int i = 0; i < 100; i++ ) {
+int a = 100;
+  }
+}
+
+// CK1: {{.*}}define weak void @__omp_offloading{{.*}}test_ds{{.*}}()
+// CK1: call void @__kmpc_spmd_kernel_init
+// CK1-NEXT: call void @_

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D48903#1159023, @ioeric wrote:

> Would you mind reverting this patch for now so that we can come up with a 
> solution to address those use cases?
>
> Sorry again about missing the discussion earlier!


Of course, feel free to revert if needed (I'm not sure how to do that).  Are 
you able to come up with a test case that covers the use case you mention?


Repository:
  rC Clang

https://reviews.llvm.org/D48903



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

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D48903#1159046, @simark wrote:

> In https://reviews.llvm.org/D48903#1159023, @ioeric wrote:
>
> > Would you mind reverting this patch for now so that we can come up with a 
> > solution to address those use cases?
> >
> > Sorry again about missing the discussion earlier!
>
>
> Of course, feel free to revert if needed (I'm not sure how to do that).  Are 
> you able to come up with a test case that covers the use case you mention?


Thanks! I'll try to come up with a smaller test case asap.

The virtual file support in `FileManager` is totally confusing and full of 
traps (when used with VFS). I really feel we should replace it with 
InMemoryFileSystem at some point :(


Repository:
  rC Clang

https://reviews.llvm.org/D48903



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

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: lib/Basic/FileManager.cpp:395
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+UFE->RealPathName = RealPathName.str();

It seems that at this point, we have failed to find `FileName` in vfs (with 
`getStatValue` above), so `getRealPath` would also fail? 

In general, the virtual file here can be an actual *virtual* file that doesn't 
exist anywhere, and I think there are users who use this to map virtual file 
(possibly with relative paths) into file manager (while they should really use 
overlay vfs!).


Repository:
  rC Clang

https://reviews.llvm.org/D49197



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

[PATCH] D47946: [ASTImporter] Fix infinite recursion on function import with struct definition in parameters

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

@gerazo, Do you have commit rights, or should I help with the commit?


https://reviews.llvm.org/D47946



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

[PATCH] D49188: [OpenMP] Initialize data sharing stack for SPMD case

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

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



Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:84-85
   OMPRTL_NVPTX__kmpc_data_sharing_init_stack,
+  /// Call to void __kmpc_data_sharing_init_stack_spmd();
+  OMPRTL_NVPTX__kmpc_data_sharing_init_stack_spmd,
   /// Call to void* __kmpc_data_sharing_push_stack(size_t size,

Hahnfeld wrote:
> In case I'm not missing something obvious this function doesn't exist (yet) 
> in libomptarget-nvptx?
Correct. There's a libomptarget patch coming up as well.



Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:1110-1114
-  // For data sharing, we need to initialize the stack for workers.
-  CGF.EmitRuntimeCall(
-  createNVPTXRuntimeFunction(
-  OMPRTL_NVPTX__kmpc_data_sharing_init_stack));
-

Hahnfeld wrote:
> Why is this call being removed? There's no mention in the summary AFAICS
Not needed on workers after libomptarget patch. I could perhaps put this into a 
separate patch along with the test fix.


Repository:
  rC Clang

https://reviews.llvm.org/D49188



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

[PATCH] D47632: [ASTImporter] Refactor Decl creation

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Hi Aleksei, could you pleas take an other quick look, there were only minor 
changes since your last comments. Thanks!


Repository:
  rC Clang

https://reviews.llvm.org/D47632



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

[PATCH] D44493: [libunwind] Add a cmake option for disabling the use of the dynamic linker

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo abandoned this revision.
mstorsjo added a comment.
Herald added a subscriber: chrib.

I choose not to commit this one after all, I managed to work around the issue 
with Windows Store apps in a much simpler way by providing a Windows Store 
compatible version of EnumProcessModules, see 
https://sourceforge.net/p/mingw-w64/mingw-w64/ci/0e1f41358845706a1adafcf4f9d27db0319dd3d9/.


Repository:
  rUNW libunwind

https://reviews.llvm.org/D44493



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

[PATCH] D44494: [libunwind] Support __register_frame with a full .eh_frame section

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo abandoned this revision.
mstorsjo added a comment.
Herald added a subscriber: chrib.

FWIW, I'm not going to put any more effort into this one at the moment, I 
managed to fix my use case in a much simpler way by providing a Windows Store 
compatible version of EnumProcessModules, see 
https://sourceforge.net/p/mingw-w64/mingw-w64/ci/0e1f41358845706a1adafcf4f9d27db0319dd3d9/.


https://reviews.llvm.org/D44494



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

r336831 - Revert "[VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name"

[Eric Liu via cfe-commits]

Author: ioeric
Date: Wed Jul 11 11:43:07 2018
New Revision: 336831

URL: http://llvm.org/viewvc/llvm-project?rev=336831&view=rev
Log:
Revert "[VirtualFileSystem] InMemoryFileSystem::status: Return a Status with 
the requested name"

This reverts commit r336807. This breaks users of
ClangTool::mapVirtualFile. Will try to investigate a fix. See also the
discussion on https://reviews.llvm.org/D48903

Modified:
cfe/trunk/lib/Basic/FileManager.cpp
cfe/trunk/lib/Basic/VirtualFileSystem.cpp
cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp
cfe/trunk/unittests/Driver/ToolChainTest.cpp

Modified: cfe/trunk/lib/Basic/FileManager.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=336831&r1=336830&r2=336831&view=diff
==
--- cfe/trunk/lib/Basic/FileManager.cpp (original)
+++ cfe/trunk/lib/Basic/FileManager.cpp Wed Jul 11 11:43:07 2018
@@ -315,11 +315,9 @@ const FileEntry *FileManager::getFile(St
   UFE.InPCH = Data.InPCH;
   UFE.File = std::move(F);
   UFE.IsValid = true;
-
-  SmallString<128> RealPathName;
-  if (!FS->getRealPath(InterndFileName, RealPathName))
-UFE.RealPathName = RealPathName.str();
-
+  if (UFE.File)
+if (auto RealPathName = UFE.File->getName())
+  UFE.RealPathName = *RealPathName;
   return &UFE;
 }
 

Modified: cfe/trunk/lib/Basic/VirtualFileSystem.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/VirtualFileSystem.cpp?rev=336831&r1=336830&r2=336831&view=diff
==
--- cfe/trunk/lib/Basic/VirtualFileSystem.cpp (original)
+++ cfe/trunk/lib/Basic/VirtualFileSystem.cpp Wed Jul 11 11:43:07 2018
@@ -471,33 +471,15 @@ enum InMemoryNodeKind { IME_File, IME_Di
 /// The in memory file system is a tree of Nodes. Every node can either be a
 /// file or a directory.
 class InMemoryNode {
-  InMemoryNodeKind Kind;
   Status Stat;
-
-protected:
-  /// Return Stat.  This should only be used for internal/debugging use.  When
-  /// clients wants the Status of this node, they should use
-  /// \p getStatus(StringRef).
-  const Status& getStatus() const {
-return Stat;
-  }
+  InMemoryNodeKind Kind;
 
 public:
   InMemoryNode(Status Stat, InMemoryNodeKind Kind)
-  : Kind(Kind), Stat(std::move(Stat)) {}
+  : Stat(std::move(Stat)), Kind(Kind) {}
   virtual ~InMemoryNode() = default;
 
-  /// Return the \p Status for this node. \p RequestedName should be the name
-  /// through which the caller referred to this node. It will override
-  /// \p Status::Name in the return value, to mimic the behavior of \p 
RealFile.
-  Status getStatus(StringRef RequestedName) const {
-return Status::copyWithNewName(Stat, RequestedName);
-  }
-
-  /// Get the filename of this node (the name without the directory part).
-  StringRef getFileName() const {
-return llvm::sys::path::filename(Stat.getName());
-  }
+  const Status &getStatus() const { return Stat; }
   InMemoryNodeKind getKind() const { return Kind; }
   virtual std::string toString(unsigned Indent) const = 0;
 };
@@ -522,22 +504,14 @@ public:
   }
 };
 
-/// Adapt a InMemoryFile for VFS' File interface.  The goal is to make
-/// \p InMemoryFileAdaptor mimic as much as possible the behavior of
-/// \p RealFile.
+/// Adapt a InMemoryFile for VFS' File interface.
 class InMemoryFileAdaptor : public File {
   InMemoryFile &Node;
 
-  /// The name to use when returning a Status for this file.
-  std::string RequestedName;
-
 public:
-  explicit InMemoryFileAdaptor(InMemoryFile &Node, std::string RequestedName)
-  : Node(Node), RequestedName(std::move(RequestedName)) {}
+  explicit InMemoryFileAdaptor(InMemoryFile &Node) : Node(Node) {}
 
-  llvm::ErrorOr status() override {
-return Node.getStatus(RequestedName);
-  }
+  llvm::ErrorOr status() override { return Node.getStatus(); }
 
   llvm::ErrorOr>
   getBuffer(const Twine &Name, int64_t FileSize, bool RequiresNullTerminator,
@@ -737,7 +711,7 @@ lookupInMemoryNode(const InMemoryFileSys
 llvm::ErrorOr InMemoryFileSystem::status(const Twine &Path) {
   auto Node = lookupInMemoryNode(*this, Root.get(), Path);
   if (Node)
-return (*Node)->getStatus(Path.str());
+return (*Node)->getStatus();
   return Node.getError();
 }
 
@@ -750,8 +724,7 @@ InMemoryFileSystem::openFileForRead(cons
   // When we have a file provide a heap-allocated wrapper for the memory buffer
   // to match the ownership semantics for File.
   if (auto *F = dyn_cast(*Node))
-return std::unique_ptr(
-new detail::InMemoryFileAdaptor(*F, Path.str()));
+return std::unique_ptr(new detail::InMemoryFileAdaptor(*F));
 
   // FIXME: errc::not_a_file?
   return make_error_code(llvm::errc::invalid_argument);
@@ -763,33 +736,21 @@ namespace {
 class InMemoryDirIterator : public clang::vfs::detail::DirIterImpl {
   detail::InMemoryDirectory::const_iterator I;
   detail::InMemoryDirectory::const_iter

[PATCH] D49199: [analyzer][UninitializedObjectChecker] Pointer/reference objects are dereferenced according to dynamic type

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

Szelethus created this revision.
Szelethus added reviewers: NoQ, george.karpenkov, xazax.hun, rnkovacs.
Herald added subscribers: cfe-commits, mikhail.ramalho, a.sidorin, szepet, 
whisperity.

This diff fixes a long debated issues with pointers/references not being 
dereferenced according to their dynamic type. This also fixed an issue where 
regions that were pointed to by void pointers were not analyzed.

Note how I also added a test case about inheritance, clearly showing that it 
doesn't find an uninitialized field that it should. Because base class handling 
is still being discussed, I'm planning to fix that issue in a different patch. 
The reason why it's still in this patch is that it might be closely tied to 
these changes too.

Thumbs up to @NoQ for setting me on the right track! 



Repository:
  rC Clang

https://reviews.llvm.org/D49199

Files:
  lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
  test/Analysis/cxx-uninitialized-object-inheritance.cpp
  test/Analysis/cxx-uninitialized-object-ptr-ref.cpp

Index: test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
===
--- test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
+++ test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
@@ -191,7 +191,7 @@
 
 struct CyclicPointerTest {
   int *ptr;
-  CyclicPointerTest() : ptr(reinterpret_cast(&ptr)) {}
+  CyclicPointerTest() : ptr(reinterpret_cast(&ptr)) {}
 };
 
 void fCyclicPointerTest() {
@@ -280,13 +280,62 @@
   void *vptr; // no-crash
 
   CyclicVoidPointerTest() : vptr(&vptr) {}
-
 };
 
 void fCyclicVoidPointerTest() {
   CyclicVoidPointerTest();
 }
 
+struct IntDynTypedVoidPointerTest1 {
+  void *vptr; // expected-note{{uninitialized pointee 'this->vptr'}}
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  IntDynTypedVoidPointerTest1(void *vptr) : vptr(vptr) {} // expected-warning{{1 uninitialized field}}
+};
+
+void fIntDynTypedVoidPointerTest1() {
+  int a;
+  IntDynTypedVoidPointerTest1 tmp(&a);
+}
+
+struct RecordDynTypedVoidPointerTest {
+  struct RecordType {
+int x; // expected-note{{uninitialized field 'this->vptr->x'}}
+int y; // expected-note{{uninitialized field 'this->vptr->y'}}
+  };
+
+  void *vptr;
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  RecordDynTypedVoidPointerTest(void *vptr) : vptr(vptr) {} // expected-warning{{2 uninitialized fields}}
+};
+
+void fRecordDynTypedVoidPointerTest() {
+  RecordDynTypedVoidPointerTest::RecordType a;
+  RecordDynTypedVoidPointerTest tmp(&a);
+}
+
+struct NestedNonVoidDynTypedVoidPointerTest {
+  struct RecordType {
+int x;  // expected-note{{uninitialized field 'this->vptr->x'}}
+int y;  // expected-note{{uninitialized field 'this->vptr->y'}}
+void *vptr; // expected-note{{uninitialized pointee 'this->vptr->vptr'}}
+  };
+
+  void *vptr;
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  NestedNonVoidDynTypedVoidPointerTest(void *vptr, void *c) : vptr(vptr) {
+static_cast(vptr)->vptr = c; // expected-warning{{3 uninitialized fields}}
+  }
+};
+
+void fNestedNonVoidDynTypedVoidPointerTest() {
+  NestedNonVoidDynTypedVoidPointerTest::RecordType a;
+  char c;
+  NestedNonVoidDynTypedVoidPointerTest tmp(&a, &c);
+}
+
 //===--===//
 // Multipointer tests.
 //===--===//
Index: test/Analysis/cxx-uninitialized-object-inheritance.cpp
===
--- test/Analysis/cxx-uninitialized-object-inheritance.cpp
+++ test/Analysis/cxx-uninitialized-object-inheritance.cpp
@@ -773,3 +773,26 @@
 void fVirtualDiamondInheritanceTest3() {
   VirtualDiamondInheritanceTest3();
 }
+
+//===--===//
+// Dynamic type test.
+//===--===//
+
+struct DynTBase {};
+struct DynTDerived : DynTBase {
+  // TODO: we'd expect the note: {{uninitialized field 'this->x'}}
+  int x; // no-note
+};
+
+struct DynamicTypeTest {
+  DynTBase *bptr;
+  int i = 0;
+
+  // TODO: we'd expect the warning: {{1 uninitialized field}}
+  DynamicTypeTest(DynTBase *bptr) : bptr(bptr) {} // no-warning
+};
+
+void f() {
+  DynTDerived d;
+  DynamicTypeTest t(&d);
+};
Index: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
@@ -30,7 +30,7 @@
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
-#include 
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h"
 
 using namespace clang;
 using

Re: r336776 - [AST] Structural equivalence of methods

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

Hi,
I am aware of the problem, there should be already a commit to (hopefully)
correct the failure.

Balázs Kéri

Galina Kistanova  ezt írta (időpont: 2018. júl. 11.,
Sze, 20:01):

> Hello Balazs,
>
> This commit broke at least one of our builders:
>
> http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/10897
>
> . . .
> Failing Tests (1):
> Clang-Unit :: AST/./ASTTests.exe/StructuralEquivalenceRecordTest.Name
>
> Please have a look?
> It is not good idea to keep the bot red for too long. This hides new
> problem which later hard to track down.
>
> Thanks
>
> Galina
>
>
> On Wed, Jul 11, 2018 at 2:37 AM, Balazs Keri via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: balazske
>> Date: Wed Jul 11 02:37:24 2018
>> New Revision: 336776
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=336776&view=rev
>> Log:
>> [AST] Structural equivalence of methods
>>
>> Summary:
>> Added structural equivalence check for C++ methods.
>> Improved structural equivalence tests.
>> Added related ASTImporter tests.
>>
>> Reviewers: a.sidorin, szepet, xazax.hun, martong, a_sidorin
>>
>> Reviewed By: martong, a_sidorin
>>
>> Subscribers: a_sidorin, rnkovacs, cfe-commits
>>
>> Differential Revision: https://reviews.llvm.org/D48628
>>
>> Modified:
>> cfe/trunk/lib/AST/ASTImporter.cpp
>> cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
>> cfe/trunk/unittests/AST/ASTImporterTest.cpp
>> cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp
>>
>> Modified: cfe/trunk/lib/AST/ASTImporter.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTImporter.cpp?rev=336776&r1=336775&r2=336776&view=diff
>>
>> ==
>> --- cfe/trunk/lib/AST/ASTImporter.cpp (original)
>> +++ cfe/trunk/lib/AST/ASTImporter.cpp Wed Jul 11 02:37:24 2018
>> @@ -230,6 +230,7 @@ namespace clang {
>>  bool IsStructuralMatch(EnumConstantDecl *FromEC, EnumConstantDecl
>> *ToEC);
>>  bool IsStructuralMatch(FunctionTemplateDecl *From,
>> FunctionTemplateDecl *To);
>> +bool IsStructuralMatch(FunctionDecl *From, FunctionDecl *To);
>>  bool IsStructuralMatch(ClassTemplateDecl *From, ClassTemplateDecl
>> *To);
>>  bool IsStructuralMatch(VarTemplateDecl *From, VarTemplateDecl *To);
>>  Decl *VisitDecl(Decl *D);
>> @@ -1525,6 +1526,13 @@ bool ASTNodeImporter::IsStructuralMatch(
>>return Ctx.IsStructurallyEquivalent(From, To);
>>  }
>>
>> +bool ASTNodeImporter::IsStructuralMatch(FunctionDecl *From, FunctionDecl
>> *To) {
>> +  StructuralEquivalenceContext Ctx(
>> +  Importer.getFromContext(), Importer.getToContext(),
>> +  Importer.getNonEquivalentDecls(), false, false);
>> +  return Ctx.IsStructurallyEquivalent(From, To);
>> +}
>> +
>>  bool ASTNodeImporter::IsStructuralMatch(EnumConstantDecl *FromEC,
>>  EnumConstantDecl *ToEC) {
>>const llvm::APSInt &FromVal = FromEC->getInitVal();
>> @@ -2433,13 +2441,15 @@ Decl *ASTNodeImporter::VisitFunctionDecl
>>if (auto *FoundFunction = dyn_cast(FoundDecl)) {
>>  if (FoundFunction->hasExternalFormalLinkage() &&
>>  D->hasExternalFormalLinkage()) {
>> -  if (Importer.IsStructurallyEquivalent(D->getType(),
>> -
>> FoundFunction->getType())) {
>> -  if (D->doesThisDeclarationHaveABody() &&
>> -  FoundFunction->hasBody())
>> -return Importer.Imported(D, FoundFunction);
>> -  FoundByLookup = FoundFunction;
>> -  break;
>> +  if (IsStructuralMatch(D, FoundFunction)) {
>> +const FunctionDecl *Definition = nullptr;
>> +if (D->doesThisDeclarationHaveABody() &&
>> +FoundFunction->hasBody(Definition)) {
>> +  return Importer.Imported(
>> +  D, const_cast(Definition));
>> +}
>> +FoundByLookup = FoundFunction;
>> +break;
>>}
>>
>>// FIXME: Check for overloading more carefully, e.g., by
>> boosting
>>
>> Modified: cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp?rev=336776&r1=336775&r2=336776&view=diff
>>
>> ==
>> --- cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp (original)
>> +++ cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp Wed Jul 11 02:37:24
>> 2018
>> @@ -250,6 +250,9 @@ static bool IsStructurallyEquivalent(Str
>>if (T1.isNull() || T2.isNull())
>>  return T1.isNull() && T2.isNull();
>>
>> +  QualType OrigT1 = T1;
>> +  QualType OrigT2 = T2;
>> +
>>if (!Context.StrictTypeSpelling) {
>>  // We aren't being strict about token-to-token equivalence of types,
>>  // so map down to the canonical type.
>> @@ -422,6 +425,7 @@ static bool IsStructurallyEquivalent(Str
>>case Type:

[PATCH] D49074: [Analyzer] [WIP] Basic support for multiplication and division in the constraint manager

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

> What issues could it cause since it is guarded by an option?

I've meant the rearrangement. Actually, I would like to apologize for that 
point, I thought it was causing some issues, but it wasn't, we've just checked 
it yesterday.

> As far as I know the purpose of the cross-check functionality is to execute 
> Z3 only on actual bug report paths.

Yes.

> This may remove false-positives (like the example above), but we surely 
> cannot find new errors where multiplicative operations are used.

Do you have examples of those? In my understanding, (almost) all of the 
imprecisions in the solver lead purely to false positives.
The only example where you lose bugs is when you stop exploring due to reaching 
a (false) fatal error.


https://reviews.llvm.org/D49074



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

Re: r336807 - [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Eric Liu via cfe-commits]

fyi, I've reverted this commit with r336831 due to some other issues.

On Wed, Jul 11, 2018 at 8:23 PM Galina Kistanova via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Hello Simon,
>
> This commit added broken tests to one of our builders:
>
> http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/18386/steps/test/logs/stdio
> . . .
> Failing Tests (4):
> Clang :: PCH/case-insensitive-include.c
> Clang-Unit ::
> Basic/./BasicTests.exe/InMemoryFileSystemTest.DirectoryIteration
> Clang-Unit :: Basic/./BasicTests.exe/InMemoryFileSystemTest.StatusName
> LLVM :: MC/ELF/debug-prefix-map.s
>
> Please have a look?
> The builder was already red and did not send notifications.
>
> Thanks
>
>
> Galina
>
> On Wed, Jul 11, 2018 at 7:08 AM, Simon Marchi via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: simark
>> Date: Wed Jul 11 07:08:17 2018
>> New Revision: 336807
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=336807&view=rev
>> Log:
>> [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the
>> requested name
>>
>> Summary:
>> InMemoryFileSystem::status behaves differently than
>> RealFileSystem::status.  The Name contained in the Status returned by
>> RealFileSystem::status will be the path as requested by the caller,
>> whereas InMemoryFileSystem::status returns the normalized path.
>>
>> For example, when requested the status for "../src/first.h",
>> RealFileSystem returns a Status with "../src/first.h" as the Name.
>> InMemoryFileSystem returns "/absolute/path/to/src/first.h".
>>
>> The reason for this change is that I want to make a unit test in the
>> clangd testsuite (where we use an InMemoryFileSystem) to reproduce a
>> bug I get with the clangd program (where a RealFileSystem is used).
>> This difference in behavior "hides" the bug in the unit test version.
>>
>> In general, I guess it's good if InMemoryFileSystem works as much as
>> possible like RealFileSystem.
>>
>> Doing so made the FileEntry::RealPathName value (assigned in
>> FileManager::getFile) wrong when using the InMemoryFileSystem.  That's
>> because it assumes that vfs::File::getName will always return the real
>> path.  I changed to to use FileSystem::getRealPath instead.
>>
>> Subscribers: ilya-biryukov, ioeric, cfe-commits
>>
>> Differential Revision: https://reviews.llvm.org/D48903
>>
>> Modified:
>> cfe/trunk/lib/Basic/FileManager.cpp
>> cfe/trunk/lib/Basic/VirtualFileSystem.cpp
>> cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp
>> cfe/trunk/unittests/Driver/ToolChainTest.cpp
>>
>> Modified: cfe/trunk/lib/Basic/FileManager.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=336807&r1=336806&r2=336807&view=diff
>>
>> ==
>> --- cfe/trunk/lib/Basic/FileManager.cpp (original)
>> +++ cfe/trunk/lib/Basic/FileManager.cpp Wed Jul 11 07:08:17 2018
>> @@ -315,9 +315,11 @@ const FileEntry *FileManager::getFile(St
>>UFE.InPCH = Data.InPCH;
>>UFE.File = std::move(F);
>>UFE.IsValid = true;
>> -  if (UFE.File)
>> -if (auto RealPathName = UFE.File->getName())
>> -  UFE.RealPathName = *RealPathName;
>> +
>> +  SmallString<128> RealPathName;
>> +  if (!FS->getRealPath(InterndFileName, RealPathName))
>> +UFE.RealPathName = RealPathName.str();
>> +
>>return &UFE;
>>  }
>>
>>
>> Modified: cfe/trunk/lib/Basic/VirtualFileSystem.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/VirtualFileSystem.cpp?rev=336807&r1=336806&r2=336807&view=diff
>>
>> ==
>> --- cfe/trunk/lib/Basic/VirtualFileSystem.cpp (original)
>> +++ cfe/trunk/lib/Basic/VirtualFileSystem.cpp Wed Jul 11 07:08:17 2018
>> @@ -471,15 +471,33 @@ enum InMemoryNodeKind { IME_File, IME_Di
>>  /// The in memory file system is a tree of Nodes. Every node can either
>> be a
>>  /// file or a directory.
>>  class InMemoryNode {
>> -  Status Stat;
>>InMemoryNodeKind Kind;
>> +  Status Stat;
>> +
>> +protected:
>> +  /// Return Stat.  This should only be used for internal/debugging
>> use.  When
>> +  /// clients wants the Status of this node, they should use
>> +  /// \p getStatus(StringRef).
>> +  const Status& getStatus() const {
>> +return Stat;
>> +  }
>>
>>  public:
>>InMemoryNode(Status Stat, InMemoryNodeKind Kind)
>> -  : Stat(std::move(Stat)), Kind(Kind) {}
>> +  : Kind(Kind), Stat(std::move(Stat)) {}
>>virtual ~InMemoryNode() = default;
>>
>> -  const Status &getStatus() const { return Stat; }
>> +  /// Return the \p Status for this node. \p RequestedName should be the
>> name
>> +  /// through which the caller referred to this node. It will override
>> +  /// \p Status::Name in the return value, to mimic the behavior of \p
>> RealFile.
>> +  Status getStatus(StringRef RequestedName) const {
>> +return Status::copyWithNewName(

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

Thanks for working on this! Please upload the patch with the full context (git 
diff -U9). It helps the reviewers :)

In https://reviews.llvm.org/D48845#1158103, @alexshap wrote:

> I'm kind of interested in this fixit, but one thought which i have - probably 
> it should be more conservative (i.e. fix captures by reference, integral 
> types, etc) (since the code might rely on side-effects of 
> copy-ctors/move-ctors or extend the lifetime of an object), but fixing only 
> simple cases still seems to be useful imo. CC: @aaron.ballman , @arphaman, 
> @ahatanak


Are you talking about a more conservative warning or a more conservative fixit? 
If it doesn't make sense for us to have a fixit for a particular capture, does 
it make sense for us to have a warning for that capture in the first place?

It would be helpful to add some tests with macros to ensure that the logic for 
how the removal range is computed can handle macros. (E.g. macro that expands 
to a full/partial capture, lambda in a macro).


Repository:
  rC Clang

https://reviews.llvm.org/D48845



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

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: lib/Basic/FileManager.cpp:395
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+UFE->RealPathName = RealPathName.str();

ioeric wrote:
> It seems that at this point, we have failed to find `FileName` in vfs (with 
> `getStatValue` above), so `getRealPath` would also fail? 
> 
> In general, the virtual file here can be an actual *virtual* file that 
> doesn't exist anywhere, and I think there are users who use this to map 
> virtual file (possibly with relative paths) into file manager (while they 
> should really use overlay vfs!).
> It seems that at this point, we have failed to find FileName in vfs (with 
> getStatValue above), so getRealPath would also fail?

From what I understood, this code will be executed if the file actually exists 
but it's the first time we access it (we won't take the return at line 373).  
If we take the return at line 373, then some previous invocation of getFile or 
getVirtualFile has created the file, and that invocation would have been 
responsible for computing the real path.

> In general, the virtual file here can be an actual *virtual* file that 
> doesn't exist anywhere, and I think there are users who use this to map 
> virtual file (possibly with relative paths) into file manager (while they 
> should really use overlay vfs!).

My thinking was that the worst that could happen is that `getRealPath` fails in 
that case.


Repository:
  rC Clang

https://reviews.llvm.org/D49197



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

r336835 - [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker.

[Reka Kovacs via cfe-commits]

Author: rkovacs
Date: Wed Jul 11 12:08:02 2018
New Revision: 336835

URL: http://llvm.org/viewvc/llvm-project?rev=336835&view=rev
Log:
[analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker.

Previously, the checker only tracked one raw pointer symbol for each
container object. But member functions returning a pointer to the
object's inner buffer may be called on the object several times. These
pointer symbols are now collected in a set inside the program state map
and thus all of them is checked for use-after-free problems.

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

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

Modified: 
cfe/trunk/lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp?rev=336835&r1=336834&r2=336835&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp 
(original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp Wed 
Jul 11 12:08:02 2018
@@ -24,10 +24,23 @@
 using namespace clang;
 using namespace ento;
 
-// FIXME: member functions that return a pointer to the container's internal
-// buffer may be called on the object many times, so the object's memory
-// region should have a list of pointer symbols associated with it.
-REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, SymbolRef)
+using PtrSet = llvm::ImmutableSet;
+
+// Associate container objects with a set of raw pointer symbols.
+REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, PtrSet)
+
+// This is a trick to gain access to PtrSet's Factory.
+namespace clang {
+namespace ento {
+template<> struct ProgramStateTrait
+  : public ProgramStatePartialTrait {
+  static void *GDMIndex() {
+static int Index = 0;
+return &Index;
+  }
+};
+} // end namespace ento
+} // end namespace clang
 
 namespace {
 
@@ -61,7 +74,7 @@ public:
 bool isSymbolTracked(ProgramStateRef State, SymbolRef Sym) {
   RawPtrMapTy Map = State->get();
   for (const auto Entry : Map) {
-if (Entry.second == Sym)
+if (Entry.second.contains(Sym))
   return true;
   }
   return false;
@@ -88,11 +101,11 @@ void DanglingInternalBufferChecker::chec
 return;
 
   SVal Obj = ICall->getCXXThisVal();
-  const auto *TypedR = dyn_cast_or_null(Obj.getAsRegion());
-  if (!TypedR)
+  const auto *ObjRegion = 
dyn_cast_or_null(Obj.getAsRegion());
+  if (!ObjRegion)
 return;
 
-  auto *TypeDecl = TypedR->getValueType()->getAsCXXRecordDecl();
+  auto *TypeDecl = ObjRegion->getValueType()->getAsCXXRecordDecl();
   if (TypeDecl->getName() != "basic_string")
 return;
 
@@ -100,20 +113,30 @@ void DanglingInternalBufferChecker::chec
 
   if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
 SVal RawPtr = Call.getReturnValue();
-if (!RawPtr.isUnknown()) {
-  State = State->set(TypedR, RawPtr.getAsSymbol());
+if (SymbolRef Sym = RawPtr.getAsSymbol(/*IncludeBaseRegions=*/true)) {
+  // Start tracking this raw pointer by adding it to the set of symbols
+  // associated with this container object in the program state map.
+  PtrSet::Factory &F = State->getStateManager().get_context();
+  const PtrSet *SetPtr = State->get(ObjRegion);
+  PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet();
+  assert(C.wasInlined || !Set.contains(Sym));
+  Set = F.add(Set, Sym);
+  State = State->set(ObjRegion, Set);
   C.addTransition(State);
 }
 return;
   }
 
   if (isa(ICall)) {
-if (State->contains(TypedR)) {
-  const SymbolRef *StrBufferPtr = State->get(TypedR);
-  // FIXME: What if Origin is null?
+if (const PtrSet *PS = State->get(ObjRegion)) {
+  // Mark all pointer symbols associated with the deleted object released.
   const Expr *Origin = Call.getOriginExpr();
-  State = allocation_state::markReleased(State, *StrBufferPtr, Origin);
-  State = State->remove(TypedR);
+  for (const auto Symbol : *PS) {
+// NOTE: `Origin` may be null, and will be stored so in the symbol's
+// `RefState` in MallocChecker's `RegionState` program state map.
+State = allocation_state::markReleased(State, Symbol, Origin);
+  }
+  State = State->remove(ObjRegion);
   C.addTransition(State);
   return;
 }
@@ -123,15 +146,24 @@ void DanglingInternalBufferChecker::chec
 void DanglingInternalBufferChecker::checkDeadSymbols(SymbolReaper &SymReaper,
  CheckerContext &C) const {
   ProgramStateRef State = C.getState();
+  PtrSet::Factory &F = State->getStateManager().get_context();
   RawPtrMapTy RPM = State->get();
   for (const auto Entry : RPM) {
-if (!SymReaper.isLive(Entr

[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC336835: [analyzer] Track multiple raw pointer symbols in 
DanglingInternalBufferChecker. (authored by rkovacs, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D49057

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

Index: test/Analysis/dangling-internal-buffer.cpp
===
--- test/Analysis/dangling-internal-buffer.cpp
+++ test/Analysis/dangling-internal-buffer.cpp
@@ -108,6 +108,30 @@
   // expected-note@-1 {{Use of memory after it is freed}}
 }
 
+void multiple_symbols(bool cond) {
+  const char *c1, *d1;
+  {
+std::string s1;
+c1 = s1.c_str(); // expected-note {{Pointer to dangling buffer was obtained here}}
+d1 = s1.data(); // expected-note {{Pointer to dangling buffer was obtained here}}
+const char *local = s1.c_str();
+consume(local); // no-warning
+  } // expected-note {{Internal buffer is released because the object was destroyed}}
+  // expected-note@-1 {{Internal buffer is released because the object was destroyed}}
+  std::string s2;
+  const char *c2 = s2.c_str();
+  if (cond) {
+// expected-note@-1 {{Assuming 'cond' is not equal to 0}}
+// expected-note@-2 {{Taking true branch}}
+// expected-note@-3 {{Assuming 'cond' is 0}}
+// expected-note@-4 {{Taking false branch}}
+consume(c1); // expected-warning {{Use of memory after it is freed}}
+// expected-note@-1 {{Use of memory after it is freed}}
+  } else {
+consume(d1); // expected-warning {{Use of memory after it is freed}}
+  } // expected-note@-1 {{Use of memory after it is freed}}
+}
+
 void deref_after_scope_cstr_ok() {
   const char *c;
   std::string s;
Index: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
+++ lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
@@ -24,10 +24,23 @@
 using namespace clang;
 using namespace ento;
 
-// FIXME: member functions that return a pointer to the container's internal
-// buffer may be called on the object many times, so the object's memory
-// region should have a list of pointer symbols associated with it.
-REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, SymbolRef)
+using PtrSet = llvm::ImmutableSet;
+
+// Associate container objects with a set of raw pointer symbols.
+REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, PtrSet)
+
+// This is a trick to gain access to PtrSet's Factory.
+namespace clang {
+namespace ento {
+template<> struct ProgramStateTrait
+  : public ProgramStatePartialTrait {
+  static void *GDMIndex() {
+static int Index = 0;
+return &Index;
+  }
+};
+} // end namespace ento
+} // end namespace clang
 
 namespace {
 
@@ -61,7 +74,7 @@
 bool isSymbolTracked(ProgramStateRef State, SymbolRef Sym) {
   RawPtrMapTy Map = State->get();
   for (const auto Entry : Map) {
-if (Entry.second == Sym)
+if (Entry.second.contains(Sym))
   return true;
   }
   return false;
@@ -88,32 +101,42 @@
 return;
 
   SVal Obj = ICall->getCXXThisVal();
-  const auto *TypedR = dyn_cast_or_null(Obj.getAsRegion());
-  if (!TypedR)
+  const auto *ObjRegion = dyn_cast_or_null(Obj.getAsRegion());
+  if (!ObjRegion)
 return;
 
-  auto *TypeDecl = TypedR->getValueType()->getAsCXXRecordDecl();
+  auto *TypeDecl = ObjRegion->getValueType()->getAsCXXRecordDecl();
   if (TypeDecl->getName() != "basic_string")
 return;
 
   ProgramStateRef State = C.getState();
 
   if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
 SVal RawPtr = Call.getReturnValue();
-if (!RawPtr.isUnknown()) {
-  State = State->set(TypedR, RawPtr.getAsSymbol());
+if (SymbolRef Sym = RawPtr.getAsSymbol(/*IncludeBaseRegions=*/true)) {
+  // Start tracking this raw pointer by adding it to the set of symbols
+  // associated with this container object in the program state map.
+  PtrSet::Factory &F = State->getStateManager().get_context();
+  const PtrSet *SetPtr = State->get(ObjRegion);
+  PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet();
+  assert(C.wasInlined || !Set.contains(Sym));
+  Set = F.add(Set, Sym);
+  State = State->set(ObjRegion, Set);
   C.addTransition(State);
 }
 return;
   }
 
   if (isa(ICall)) {
-if (State->contains(TypedR)) {
-  const SymbolRef *StrBufferPtr = State->get(TypedR);
-  // FIXME: What if Origin is null?
+if (const PtrSet *PS = State->get(ObjRegion)) {
+  // Mark all pointer symbols associated with the deleted object released.
   const Expr *Origin = Call.getOriginExpr();
-  State = allocation_state::markReleased(State, *StrBufferPtr, Origin);
-  State = State->remove(TypedR);
+  for (const auto Sym

r336836 - [NFC] Replace usage of QualType.getTypePtr()-> with operator->

[Erich Keane via cfe-commits]

Author: erichkeane
Date: Wed Jul 11 12:09:21 2018
New Revision: 336836

URL: http://llvm.org/viewvc/llvm-project?rev=336836&view=rev
Log:
[NFC] Replace usage of QualType.getTypePtr()-> with operator->

Modified:
cfe/trunk/lib/Sema/SemaDecl.cpp

Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=336836&r1=336835&r2=336836&view=diff
==
--- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDecl.cpp Wed Jul 11 12:09:21 2018
@@ -8253,7 +8253,7 @@ Sema::ActOnFunctionDeclarator(Scope *S,
   bool &AddToScope) {
   QualType R = TInfo->getType();
 
-  assert(R.getTypePtr()->isFunctionType());
+  assert(R->isFunctionType());
 
   // TODO: consider using NameInfo for diagnostic.
   DeclarationNameInfo NameInfo = GetNameForDeclarator(D);


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

[PATCH] D49074: [Analyzer] [WIP] Basic support for multiplication and division in the constraint manager

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

xazax.hun added a comment.

In https://reviews.llvm.org/D49074#1159095, @george.karpenkov wrote:

> > This may remove false-positives (like the example above), but we surely 
> > cannot find new errors where multiplicative operations are used.
>
> Do you have examples of those? In my understanding, (almost) all of the 
> imprecisions in the solver lead purely to false positives.
>  The only example where you lose bugs is when you stop exploring due to 
> reaching a (false) fatal error.


This is not an example from the iterator checker (hopefully Adam will provide 
us with one example from there) but think of division by zero. The division by 
zero check will only warn if a value is constrained to be zero. The imprecision 
in the built in solver might result in failure to constrain a value to zero 
while the Z3 might be able to do that.


https://reviews.llvm.org/D49074



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

[PATCH] D49199: [analyzer][UninitializedObjectChecker] Pointer/reference objects are dereferenced according to dynamic type

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

Szelethus added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:591-609
+// TODO: This function constructs an incorrect string if a void pointer is a
+// part of the chain:
+//
+//   struct B { int x; }
+//
+//   struct A {
+// void *vptr;

I already have a fix for this, but I want to be extra sure, so I'm running the 
checker on LLVM to see whether anything breaks.

This is also important for `nonloc::LocAsInteger`, which I'm going add fix in 
yet another patch.


Repository:
  rC Clang

https://reviews.llvm.org/D49199



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

[PATCH] D48845: [Sema] Add fixit for unused lambda captures

[Alexander Shaposhnikov via Phabricator via cfe-commits]

alexshap added a comment.

> Are you talking about a more conservative warning or a more conservative 
> fixit? If it doesn't make sense for us to have a fixit for a particular 
> capture, does it make sense for us to have a warning for that >capture in the 
> first place?

to be honest i'm more concerned with the fixit (so basically to avoid breaking 
the code - especially if these modifications are applied at scale, the code 
might be get broken silently and will be hard to find later, so I'd start with 
handling only simple cases where it's a strictly positive change)) )

> It would be helpful to add some tests with macros to ensure that the logic 
> for how the removal range is computed can handle macros. (E.g. macro that 
> expands to a full/partial capture, lambda in a macro).

+1


Repository:
  rC Clang

https://reviews.llvm.org/D48845



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

r336840 - [NFC] typo

[JF Bastien via cfe-commits]

Author: jfb
Date: Wed Jul 11 12:51:40 2018
New Revision: 336840

URL: http://llvm.org/viewvc/llvm-project?rev=336840&view=rev
Log:
[NFC] typo

Modified:
cfe/trunk/lib/CodeGen/ConstantEmitter.h

Modified: cfe/trunk/lib/CodeGen/ConstantEmitter.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ConstantEmitter.h?rev=336840&r1=336839&r2=336840&view=diff
==
--- cfe/trunk/lib/CodeGen/ConstantEmitter.h (original)
+++ cfe/trunk/lib/CodeGen/ConstantEmitter.h Wed Jul 11 12:51:40 2018
@@ -50,7 +50,7 @@ public:
 : CGM(CGM), CGF(CGF) {}
 
   /// Initialize this emission in the context of the given function.
-  /// Use this if the expression might contain contextaul references like
+  /// Use this if the expression might contain contextual references like
   /// block addresses or PredefinedExprs.
   ConstantEmitter(CodeGenFunction &CGF)
 : CGM(CGF.CGM), CGF(&CGF) {}


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

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

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.



Comment at: lib/CodeGen/CGExprScalar.cpp:1694
 // handle things like function to ptr-to-function decay etc.
 Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) {
   Expr *E = CE->getSubExpr();

lebedev.ri wrote:
> vsk wrote:
> > I think maintaining a stack of visited cast exprs in the emitter be 
> > cheaper/simpler than using ASTContext::getParents. You could push CE here 
> > and use a RAII helper to pop it. The 'isCastPartOfExplicitCast' check then 
> > simplifies to a quick stack traversal.
> Hmm, two things come to mind:
> 1. This pessimizes the (most popular) case when the sanitizer is disabled.
> 2. `ASTContext::getParents()` may return more than one parent. I'm not sure 
> if that matters here?
> I'll take a look..
As for (1), it's not necessary to push/pop the stack when this sanitizer is 
disabled. And for (2), IIUC the only interesting case is "explicit-cast 
+", and none of the implicit casts here have more than one 
parent.


Repository:
  rC Clang

https://reviews.llvm.org/D48958



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

r336842 - Fix setting of empty implicit-section-name attribute

[Petr Pavlu via cfe-commits]

Author: petr.pavlu
Date: Wed Jul 11 13:17:54 2018
New Revision: 336842

URL: http://llvm.org/viewvc/llvm-project?rev=336842&view=rev
Log:
Fix setting of empty implicit-section-name attribute

Code in `CodeGenModule::SetFunctionAttributes()` could set an empty
attribute `implicit-section-name` on a function that is affected by
`#pragma clang text="section"`. This is incorrect because the attribute
should contain a valid section name. If the function additionally also
used `__attribute__((section("section")))` then this could result in
emitting the function in a section with an empty name.

The patch fixes the issue by removing the problematic code that sets
empty `implicit-section-name` from
`CodeGenModule::SetFunctionAttributes()` because it is sufficient to set
this attribute only from a similar code in `setNonAliasAttributes()`
when the function is emitted.

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

Added:
cfe/trunk/test/CodeGen/clang-sections-attribute.c
Modified:
cfe/trunk/lib/CodeGen/CodeGenModule.cpp

Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=336842&r1=336841&r2=336842&view=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Wed Jul 11 13:17:54 2018
@@ -1485,10 +1485,6 @@ void CodeGenModule::SetFunctionAttribute
   setLinkageForGV(F, FD);
   setGVProperties(F, FD);
 
-  if (FD->getAttr()) {
-F->addFnAttr("implicit-section-name");
-  }
-
   if (const SectionAttr *SA = FD->getAttr())
 F->setSection(SA->getName());
 

Added: cfe/trunk/test/CodeGen/clang-sections-attribute.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/clang-sections-attribute.c?rev=336842&view=auto
==
--- cfe/trunk/test/CodeGen/clang-sections-attribute.c (added)
+++ cfe/trunk/test/CodeGen/clang-sections-attribute.c Wed Jul 11 13:17:54 2018
@@ -0,0 +1,76 @@
+// RUN: %clang_cc1 -emit-llvm -triple arm-none-eabi -o - %s | FileCheck %s
+
+// Test interaction between __attribute__((section())) and '#pragma clang
+// section' directives. The attribute should always have higher priority than
+// the pragma.
+
+// Text tests.
+#pragma clang section text=".ext_fun_pragma"
+void ext_fun(void) __attribute__((section(".ext_fun_attr")));
+void ext_fun(void) {}
+#pragma clang section text=""
+
+void ext_fun2(void) __attribute__((section(".ext_fun2_attr")));
+#pragma clang section text=".ext_fun2_pragma"
+void ext_fun2(void) {}
+#pragma clang section text=""
+
+#pragma clang section text=".int_fun_pragma"
+static void int_fun(void) __attribute__((section(".int_fun_attr"), used));
+static void int_fun(void) {}
+#pragma clang section text=""
+
+static void int_fun2(void) __attribute__((section(".int_fun2_attr"), used));
+#pragma clang section text=".int_fun2_pragma"
+static void int_fun2(void) {}
+#pragma clang section text=""
+
+// Rodata tests.
+#pragma clang section rodata=".ext_const_pragma"
+__attribute__((section(".ext_const_attr")))
+const int ext_const = 1;
+#pragma clang section rodata=""
+
+#pragma clang section rodata=".int_const_pragma"
+__attribute__((section(".int_const_attr"), used))
+static const int int_const = 1;
+#pragma clang section rodata=""
+
+// Data tests.
+#pragma clang section data=".ext_var_pragma"
+__attribute__((section(".ext_var_attr")))
+int ext_var = 1;
+#pragma clang section data=""
+
+#pragma clang section data=".int_var_pragma"
+__attribute__((section(".int_var_attr"), used))
+static int int_var = 1;
+#pragma clang section data=""
+
+// Bss tests.
+#pragma clang section bss=".ext_zvar_pragma"
+__attribute__((section(".ext_zvar_attr")))
+int ext_zvar;
+#pragma clang section bss=""
+
+#pragma clang section bss=".int_zvar_pragma"
+__attribute__((section(".int_zvar_attr"), used))
+static int int_zvar;
+#pragma clang section bss=""
+
+// CHECK: @ext_const = constant i32 1, section ".ext_const_attr", align 4{{$}}
+// CHECK: @int_const = internal constant i32 1, section ".int_const_attr", 
align 4{{$}}
+// CHECK: @ext_var = global i32 1, section ".ext_var_attr", align 4{{$}}
+// CHECK: @int_var = internal global i32 1, section ".int_var_attr", align 
4{{$}}
+// CHECK: @ext_zvar = global i32 0, section ".ext_zvar_attr", align 4{{$}}
+// CHECK: @int_zvar = internal global i32 0, section ".int_zvar_attr", align 
4{{$}}
+// CHECK: define void @ext_fun() #0 section ".ext_fun_attr"
+// CHECK: define void @ext_fun2() #0 section ".ext_fun2_attr"
+// CHECK: define internal void @int_fun() #0 section ".int_fun_attr"
+// CHECK: define internal void @int_fun2() #0 section ".int_fun2_attr"
+//
+// Function attributes should not include implicit-section-name.
+// CHECK-NOT: attributes #0 = {{.*}}implicit-section-name
+//
+// No other attribute group should be present in the file.
+// CHECK-NOT: 

[PATCH] D48916: Fix setting of empty implicit-section-name attribute for functions affected by '#pragma clang section'

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC336842: Fix setting of empty implicit-section-name attribute 
(authored by petr.pavlu, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D48916

Files:
  lib/CodeGen/CodeGenModule.cpp
  test/CodeGen/clang-sections-attribute.c


Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -1485,10 +1485,6 @@
   setLinkageForGV(F, FD);
   setGVProperties(F, FD);
 
-  if (FD->getAttr()) {
-F->addFnAttr("implicit-section-name");
-  }
-
   if (const SectionAttr *SA = FD->getAttr())
 F->setSection(SA->getName());
 
Index: test/CodeGen/clang-sections-attribute.c
===
--- test/CodeGen/clang-sections-attribute.c
+++ test/CodeGen/clang-sections-attribute.c
@@ -0,0 +1,76 @@
+// RUN: %clang_cc1 -emit-llvm -triple arm-none-eabi -o - %s | FileCheck %s
+
+// Test interaction between __attribute__((section())) and '#pragma clang
+// section' directives. The attribute should always have higher priority than
+// the pragma.
+
+// Text tests.
+#pragma clang section text=".ext_fun_pragma"
+void ext_fun(void) __attribute__((section(".ext_fun_attr")));
+void ext_fun(void) {}
+#pragma clang section text=""
+
+void ext_fun2(void) __attribute__((section(".ext_fun2_attr")));
+#pragma clang section text=".ext_fun2_pragma"
+void ext_fun2(void) {}
+#pragma clang section text=""
+
+#pragma clang section text=".int_fun_pragma"
+static void int_fun(void) __attribute__((section(".int_fun_attr"), used));
+static void int_fun(void) {}
+#pragma clang section text=""
+
+static void int_fun2(void) __attribute__((section(".int_fun2_attr"), used));
+#pragma clang section text=".int_fun2_pragma"
+static void int_fun2(void) {}
+#pragma clang section text=""
+
+// Rodata tests.
+#pragma clang section rodata=".ext_const_pragma"
+__attribute__((section(".ext_const_attr")))
+const int ext_const = 1;
+#pragma clang section rodata=""
+
+#pragma clang section rodata=".int_const_pragma"
+__attribute__((section(".int_const_attr"), used))
+static const int int_const = 1;
+#pragma clang section rodata=""
+
+// Data tests.
+#pragma clang section data=".ext_var_pragma"
+__attribute__((section(".ext_var_attr")))
+int ext_var = 1;
+#pragma clang section data=""
+
+#pragma clang section data=".int_var_pragma"
+__attribute__((section(".int_var_attr"), used))
+static int int_var = 1;
+#pragma clang section data=""
+
+// Bss tests.
+#pragma clang section bss=".ext_zvar_pragma"
+__attribute__((section(".ext_zvar_attr")))
+int ext_zvar;
+#pragma clang section bss=""
+
+#pragma clang section bss=".int_zvar_pragma"
+__attribute__((section(".int_zvar_attr"), used))
+static int int_zvar;
+#pragma clang section bss=""
+
+// CHECK: @ext_const = constant i32 1, section ".ext_const_attr", align 4{{$}}
+// CHECK: @int_const = internal constant i32 1, section ".int_const_attr", 
align 4{{$}}
+// CHECK: @ext_var = global i32 1, section ".ext_var_attr", align 4{{$}}
+// CHECK: @int_var = internal global i32 1, section ".int_var_attr", align 
4{{$}}
+// CHECK: @ext_zvar = global i32 0, section ".ext_zvar_attr", align 4{{$}}
+// CHECK: @int_zvar = internal global i32 0, section ".int_zvar_attr", align 
4{{$}}
+// CHECK: define void @ext_fun() #0 section ".ext_fun_attr"
+// CHECK: define void @ext_fun2() #0 section ".ext_fun2_attr"
+// CHECK: define internal void @int_fun() #0 section ".int_fun_attr"
+// CHECK: define internal void @int_fun2() #0 section ".int_fun2_attr"
+//
+// Function attributes should not include implicit-section-name.
+// CHECK-NOT: attributes #0 = {{.*}}implicit-section-name
+//
+// No other attribute group should be present in the file.
+// CHECK-NOT: attributes #1


Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -1485,10 +1485,6 @@
   setLinkageForGV(F, FD);
   setGVProperties(F, FD);
 
-  if (FD->getAttr()) {
-F->addFnAttr("implicit-section-name");
-  }
-
   if (const SectionAttr *SA = FD->getAttr())
 F->setSection(SA->getName());
 
Index: test/CodeGen/clang-sections-attribute.c
===
--- test/CodeGen/clang-sections-attribute.c
+++ test/CodeGen/clang-sections-attribute.c
@@ -0,0 +1,76 @@
+// RUN: %clang_cc1 -emit-llvm -triple arm-none-eabi -o - %s | FileCheck %s
+
+// Test interaction between __attribute__((section())) and '#pragma clang
+// section' directives. The attribute should always have higher priority than
+// the pragma.
+
+// Text tests.
+#pragma clang section text=".ext_fun_pragma"
+void ext_fun(void) __attribute__((section(".ext_fun_attr")));
+void ext_fun(void) {}
+#pragma clang section text=""
+
+void ext_fun2(void) __attribute__((section(".ext_fun2_attr")))

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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

C++ Core Guidelines contains ES.45: Avoid "magic constants"; use symbolic 
constants 
,
 so I think check should be moved into cppcoreguidelines module.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114



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

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 155057.
simark added a comment.

This is an update of my work in progress.  I haven't done any sharing with
SymbolCollector, as it's not clear to me how to proceed with that.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687

Files:
  clangd/SourceCode.cpp
  unittests/clangd/TestFS.cpp
  unittests/clangd/TestFS.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -312,27 +312,65 @@
 }
 
 TEST(GoToDefinition, RelPathsInCompileCommand) {
+  // The source is in "/clangd-test/src".
+  // We build in "/clangd-test/build".
+
   Annotations SourceAnnotations(R"cpp(
+#include "header_in_preamble.h"
 int [[foo]];
-int baz = f^oo;
+#include "header_not_in_preamble.h"
+int baz = f$p1^oo + bar_pre$p2^amble + bar_not_pre$p3^amble;
+)cpp");
+
+  Annotations HeaderInPreambleAnnotations(R"cpp(
+int [[bar_preamble]];
+)cpp");
+
+  Annotations HeaderNotInPreambleAnnotations(R"cpp(
+int [[bar_not_preamble]];
 )cpp");
 
+  // Make the compilation paths appear as ../src/foo.cpp in the compile
+  // commands.
+  SmallString<32> RelPathPrefix("..");
+  llvm::sys::path::append(RelPathPrefix, "src");
+  std::string BuildDir = testPath("build");
+  MockCompilationDatabase CDB(BuildDir, RelPathPrefix);
+
   IgnoreDiagnostics DiagConsumer;
-  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
   MockFSProvider FS;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
 
-  auto FooCpp = testPath("foo.cpp");
+  // Fill the filesystem.
+  auto FooCpp = testPath("src/foo.cpp");
   FS.Files[FooCpp] = "";
+  auto HeaderInPreambleH = testPath("src/header_in_preamble.h");
+  FS.Files[HeaderInPreambleH] = HeaderInPreambleAnnotations.code();
+  auto HeaderNotInPreambleH = testPath("src/header_not_in_preamble.h");
+  FS.Files[HeaderNotInPreambleH] = HeaderNotInPreambleAnnotations.code();
 
-  Server.addDocument(FooCpp, SourceAnnotations.code());
   runAddDocument(Server, FooCpp, SourceAnnotations.code());
+
+  // Go to a definition in main source file.
   auto Locations =
-  runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+  runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p1"));
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-
   EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
SourceAnnotations.range()}));
+
+  // Go to a definition in header_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderInPreambleH},
+   HeaderInPreambleAnnotations.range()}));
+
+  // Go to a definition in header_not_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderNotInPreambleH},
+   HeaderNotInPreambleAnnotations.range()}));
 }
 
 TEST(Hover, All) {
Index: unittests/clangd/TestFS.h
===
--- unittests/clangd/TestFS.h
+++ unittests/clangd/TestFS.h
@@ -39,15 +39,22 @@
 // A Compilation database that returns a fixed set of compile flags.
 class MockCompilationDatabase : public GlobalCompilationDatabase {
 public:
-  /// When \p UseRelPaths is true, uses relative paths in compile commands.
-  /// When \p UseRelPaths is false, uses absoulte paths.
-  MockCompilationDatabase(bool UseRelPaths = false);
+  /// If \p Directory is not null, use that as the Directory field of the
+  /// CompileCommand.
+  ///
+  /// If \p RelPathPrefix is not null, use that as a prefix in front of the
+  /// source file name, instead of using an absolute path.
+  MockCompilationDatabase(StringRef Directory = StringRef(),
+  StringRef RelPathPrefix = StringRef());
 
   llvm::Optional
   getCompileCommand(PathRef File) const override;
 
   std::vector ExtraClangFlags;
-  const bool UseRelPaths;
+
+private:
+  StringRef Directory;
+  StringRef RelPathPrefix;
 };
 
 // Returns an absolute (fake) test directory for this OS.
Index: unittests/clangd/TestFS.cpp
===
--- unittests/clangd/TestFS.cpp
+++ unittests/clangd/TestFS.cpp
@@ -30,22 +30,36 @@
   return MemFS;
 }
 
-MockCompilationDatabase::MockCompilationDatabase(bool UseRelPaths)
-: ExtraClangFlags({"-ffreestanding"}), UseRelPaths(UseRelPaths) {
+MockCompilationDatabase::MockCompilationDatabase(StringRef Directory,
+ StringRef RelPa

[clang-tools-extra] r336849 - [Documentation] Link format and order of Clang-tidy changes in Release Notes

[Eugene Zelenko via cfe-commits]

Author: eugenezelenko
Date: Wed Jul 11 13:41:16 2018
New Revision: 336849

URL: http://llvm.org/viewvc/llvm-project?rev=336849&view=rev
Log:
[Documentation] Link format and order of Clang-tidy changes in Release Notes

Modified:
clang-tools-extra/trunk/docs/ReleaseNotes.rst

Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=336849&r1=336848&r2=336849&view=diff
==
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Wed Jul 11 13:41:16 2018
@@ -95,6 +95,16 @@ Improvements to clang-tidy
 
   Warns on unused function return values.
 
+- New :doc:`cert-msc32-c
+  ` check
+
+  Detects inappropriate seeding of ``srand()`` function.
+
+- New :doc:`cert-msc51-cpp
+  ` check
+
+  Detects inappropriate seeding of C++ random generators and C ``srand()`` 
function.
+  
 - New :doc:`cppcoreguidelines-avoid-goto
   ` check.
 
@@ -253,17 +263,6 @@ Improvements to clang-tidy
 
 - The 'google-runtime-member-string-references' check was removed.
 
-- New `cert-msc51-cpp
-  
`_
 check
-
-  Detects inappropriate seeding of C++ random generators and C ``srand()`` 
function.
-  
-- New `cert-msc32-c
-  
`_
 check
-
-  Detects inappropriate seeding of ``srand()`` function.
-
-
 Improvements to include-fixer
 -
 


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

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

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

Quuxplusone added inline comments.



Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:11
+
+   double circleArea = 3.1415926535 * radius * radius;
+

0x8000- wrote:
> Eugene.Zelenko wrote:
> > JonasToth wrote:
> > > This example is good, but right now the code only checks for integer 
> > > literals. Maybe an integer example would be better?
> > Please use .. code-block:: c++. Same for good example.
> @JonasToth I raked my brain but I just can't come up with a short and 
> effective example. I haven't given up yet.
A common "magic number" in real code would be something like
```
int get_answer() {
if (cond) return x;
return -1;  // FILENOTFOUND
}
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49114



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

[clang-tools-extra] r336850 - [Documentation] Fix incorrect documentation references, new checks order in Release Notes

[Eugene Zelenko via cfe-commits]

Author: eugenezelenko
Date: Wed Jul 11 13:56:26 2018
New Revision: 336850

URL: http://llvm.org/viewvc/llvm-project?rev=336850&view=rev
Log:
[Documentation] Fix incorrect documentation references, new checks order in 
Release Notes

Modified:
clang-tools-extra/trunk/docs/ReleaseNotes.rst

Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=336850&r1=336849&r2=336850&view=diff
==
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Wed Jul 11 13:56:26 2018
@@ -67,6 +67,12 @@ Improvements to clang-tidy
 
 - New module ``zircon`` for checks related to Fuchsia's Zircon kernel.
 
+- New :doc:`abseil-string-find-startswith
+  ` check.
+
+  Checks whether a ``std::string::find()`` result is compared with 0, and
+  suggests replacing with ``absl::StartsWith()``.
+
 - New :doc:`android-comparison-in-temp-failure-retry
   ` check.
 
@@ -96,12 +102,12 @@ Improvements to clang-tidy
   Warns on unused function return values.
 
 - New :doc:`cert-msc32-c
-  ` check
+  ` check
 
   Detects inappropriate seeding of ``srand()`` function.
 
 - New :doc:`cert-msc51-cpp
-  ` check
+  ` check
 
   Detects inappropriate seeding of C++ random generators and C ``srand()`` 
function.
   
@@ -122,12 +128,6 @@ Improvements to clang-tidy
 
   Warns if a class inherits from multiple classes that are not pure virtual.
 
-- New :doc:`abseil-string-find-startswith
-  ` check.
-
-  Checks whether a ``std::string::find()`` result is compared with 0, and
-  suggests replacing with ``absl::StartsWith()``.
-
 - New `fuchsia-restrict-system-includes
   
`_
 check
 


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

[PATCH] D46845: [libcxx][c++17] P0083R5: Splicing Maps and Sets

[Louis Dionne via Phabricator via cfe-commits]

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

Generally looks pretty good, but I have a couple questions/suggestions.




Comment at: libcxx/include/__hash_table:2165
+#if _LIBCPP_STD_VER > 14
+template 
+template 

When a function is declared with a visibility macro 
(`_LIBCPP_INLINE_VISIBILITY` in this case), I think it is customary in libc++ 
to repeat the visibility macro on the definition as well. If that's correct, 
apply here and to other similar functions below, and also in `__tree`.



Comment at: libcxx/include/__hash_table:2212
+allocator_type __alloc(__node_alloc());
+return _NodeHandle(remove(__p).release(), __alloc);
+}

Just a quick Q: am I mistaken or `remove()` is not part of the `unordered_map` 
API? We're not naming it `__remove()` just because we already have other 
functions called `remove()`, so any macro redefining `remove` would already 
break things -- is that the idea?



Comment at: libcxx/include/__node_handle:33
+template 
+struct __generic_container_node_destructor;
+

Clever way of reducing header dependencies. I like it.



Comment at: libcxx/include/__node_handle:36
+template 
+class __node_handle_base
+{

I'd like to suggest a different approach for implementing the node handles for 
sets and maps. I believe this would simplify things slightly:

```
template  class 
_MapOrSetSpecifics>
class __basic_node_handle 
: public _MapOrSetSpecifics<_NodeType, __basic_node_handle<_NodeType, 
_Alloc, _MapOrSetSpecifics>>
{
// same as right now
};
```

Then, you can get the two different node handles by simply providing the 
methods in the base class:

```
template 
struct __map_node_handle_specifics {
using key_type = typename _NodeType::__node_value_type::key_type;
using mapped_type = typename _NodeType::__node_value_type::mapped_type;

_LIBCPP_INLINE_VISIBILITY
key_type& key() const
{
return static_cast<_Derived 
const*>(this)->__ptr_->__value_.__ref().first;
}

_LIBCPP_INLINE_VISIBILITY
mapped_type& mapped() const
{
return static_cast<_Derived 
const*>(this)->__ptr_->__value_.__ref().second;
}
};

template 
struct __set_node_handle_specifics {
using value_type = typename _NodeType::__node_value_type;

_LIBCPP_INLINE_VISIBILITY
value_type& value() const
{
return static_cast<_Derived const*>(this)->__ptr_->__value_;
}
};

template 
using __map_node_handle = __basic_node_handle<_NodeType, _Alloc, 
__map_node_handle_specifics>;

template 
using __set_node_handle = __basic_node_handle<_NodeType, _Alloc, 
__set_node_handle_specifics>;
```

This is a classic application of the CRTP. I believe it would reduce the amount 
of boilerplate currently required for special member functions. It's possible 
that you thought of this and realized it didn't work for a reason I'm missing 
right now, too, but the idea seems to work on the surface: 
https://wandbox.org/permlink/nMaKEg43PVJpA0ld.

You don't have to do this, but please consider it if you think it would 
simplify the code.




Comment at: libcxx/include/map:915
+
+__base& __get_tree() { return __tree_; }
+

I believe this function should be marked with `_LIBCPP_INLINE_VISIBILITY` -- 
even though in practice the compiler will probably always inline it.

Also, you don't seem to be using that function at all in the diff -- is that 
right?

So please either mark with `_LIBCPP_INLINE_VISIBILITY` and use it, or remove it 
altogether. This comment applies to all 8 containers to which similar methods 
were added.



Comment at: libcxx/include/set:399
 
+template 
+class multiset;

Is this forward declaration necessary?



Comment at: libcxx/include/unordered_set:569
+"node_type with incompatible allocator passed to 
unordered_set::insert()");
+return insert(_VSTD::move(__nh)).position;
+}

Why don't you use
```
return __table.template __node_handle_insert_unique(__hint, 
_VSTD::move(__nh));
```

instead? It seems like this way, if 
`__node_handle_insert_unique(const_iterator, node_type)` eventually starts 
taking advantage of the iterator hint, you'd get the improvement here for free.



Comment at: libcxx/include/unordered_set:1069
+_LIBCPP_INLINE_VISIBILITY
+node_type extract(const_iterator __position)
+{

For the other containers, the two `insert` methods come before the two 
`extract` methods. This is very nitpicky, but you should consider keeping the 
same order here too for consistency.



Comment at: 
libcxx/test/std/containers/unord/unord.map/unord.map.modifiers/insert_extract.pass.cpp:14
+
+// class unordered_map
+

Please be more specific about whi

r336852 - Fix determination of whether one set of cvr-qualifiers is compatible

[Richard Smith via cfe-commits]

Author: rsmith
Date: Wed Jul 11 14:07:04 2018
New Revision: 336852

URL: http://llvm.org/viewvc/llvm-project?rev=336852&view=rev
Log:
Fix determination of whether one set of cvr-qualifiers is compatible
with another in template argument deduction.

We happened to typically get away with getting this wrong, because the
cases where we'd produce a bogus deduction were caught by the final
"deduced A is compatible with A" check.

Added:
cfe/trunk/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.type/p1.cpp
Modified:
cfe/trunk/lib/Sema/SemaTemplateDeduction.cpp

Modified: cfe/trunk/lib/Sema/SemaTemplateDeduction.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaTemplateDeduction.cpp?rev=336852&r1=336851&r2=336852&view=diff
==
--- cfe/trunk/lib/Sema/SemaTemplateDeduction.cpp (original)
+++ cfe/trunk/lib/Sema/SemaTemplateDeduction.cpp Wed Jul 11 14:07:04 2018
@@ -1019,8 +1019,10 @@ DeduceTemplateArguments(Sema &S,
   return Sema::TDK_Success;
 }
 
-/// Determine whether the parameter has qualifiers that are either
-/// inconsistent with or a superset of the argument's qualifiers.
+/// Determine whether the parameter has qualifiers that the argument
+/// lacks. Put another way, determine whether there is no way to add
+/// a deduced set of qualifiers to the ParamType that would result in
+/// its qualifiers matching those of the ArgType.
 static bool hasInconsistentOrSupersetQualifiersOf(QualType ParamType,
   QualType ArgType) {
   Qualifiers ParamQs = ParamType.getQualifiers();
@@ -1044,10 +1046,8 @@ static bool hasInconsistentOrSupersetQua
   ParamQs.hasObjCLifetime())
 return true;
 
-  // CVR qualifier superset.
-  return (ParamQs.getCVRQualifiers() != ArgQs.getCVRQualifiers()) &&
-  ((ParamQs.getCVRQualifiers() | ArgQs.getCVRQualifiers())
-== ParamQs.getCVRQualifiers());
+  // CVR qualifiers inconsistent or a superset.
+  return (ParamQs.getCVRQualifiers() & ~ArgQs.getCVRQualifiers()) != 0;
 }
 
 /// Compare types for equality with respect to possibly compatible

Added: cfe/trunk/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.type/p1.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.type/p1.cpp?rev=336852&view=auto
==
--- cfe/trunk/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.type/p1.cpp 
(added)
+++ cfe/trunk/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.type/p1.cpp 
Wed Jul 11 14:07:04 2018
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -verify %s
+
+// an attempt is made to find template argument values that will make P, after
+// substitution of the deduced values, compatible with A
+
+namespace cv_mismatch {
+  template struct X {};
+  template void f(X); // expected-note {{cannot deduce a 
type for 'T' that would make 'const T' equal 'volatile int'}}
+  void g() { f(X()); } // expected-error {{no matching}}
+}


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

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: lib/CodeGen/CGExprScalar.cpp:1694
 // handle things like function to ptr-to-function decay etc.
 Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) {
   Expr *E = CE->getSubExpr();

vsk wrote:
> lebedev.ri wrote:
> > vsk wrote:
> > > I think maintaining a stack of visited cast exprs in the emitter be 
> > > cheaper/simpler than using ASTContext::getParents. You could push CE here 
> > > and use a RAII helper to pop it. The 'isCastPartOfExplicitCast' check 
> > > then simplifies to a quick stack traversal.
> > Hmm, two things come to mind:
> > 1. This pessimizes the (most popular) case when the sanitizer is disabled.
> > 2. `ASTContext::getParents()` may return more than one parent. I'm not sure 
> > if that matters here?
> > I'll take a look..
> As for (1), it's not necessary to push/pop the stack when this sanitizer is 
> disabled. And for (2), IIUC the only interesting case is "explicit-cast 
> +", and none of the implicit casts here have more than one 
> parent.
> I think maintaining a stack of visited cast exprs in the emitter be 
> cheaper/simpler than using ASTContext::getParents. 

{F6660377}

So yeah, this could work.
Except sadly it breaks in subtle cases like https://godbolt.org/g/5V2czU
I have added those tests beforehand.

Is `ASTContext::getParents()` really horribly slow so we want to 
duplicate/maintain/track
the current AST stack ourselves?
If so, we will need to maintain the *entire* stack, not just `CastExpr''s...


Repository:
  rC Clang

https://reviews.llvm.org/D48958



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

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

[Roman Lebedev via Phabricator via cfe-commits]

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

Add some more tricky tests where maintaining just the `CastExpr` part of AST 
stack would break them.


Repository:
  rC Clang

https://reviews.llvm.org/D48958

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

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

[PATCH] D49074: [Analyzer] [WIP] Basic support for multiplication and division in the constraint manager

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

> The imprecision in the built in solver might result in failure to constrain a 
> value to zero while the Z3 might be able to do that.

Ah right, that's a good point, we only throw a null-pointer deref/division by 
zero when the pointer/value is definitely zero.

Do you have *motivating* examples though, like an actual code samples where you 
got a bug and the analyzer does not warn?
In my experience, we almost never warn about division by zero anyway,
and the null pointer dereference check is too noisy if anything.


https://reviews.llvm.org/D49074



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

[libcxx] r336855 - Fix a test #ifdef that was reversed. NFC to the library.

[Marshall Clow via cfe-commits]

Author: marshall
Date: Wed Jul 11 14:20:42 2018
New Revision: 336855

URL: http://llvm.org/viewvc/llvm-project?rev=336855&view=rev
Log:
Fix a test #ifdef that was reversed. NFC to the library.


Modified:
libcxx/trunk/test/std/strings/string.view/string.view.cons/assign.pass.cpp

Modified: 
libcxx/trunk/test/std/strings/string.view/string.view.cons/assign.pass.cpp
URL: 
http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/std/strings/string.view/string.view.cons/assign.pass.cpp?rev=336855&r1=336854&r2=336855&view=diff
==
--- libcxx/trunk/test/std/strings/string.view/string.view.cons/assign.pass.cpp 
(original)
+++ libcxx/trunk/test/std/strings/string.view/string.view.cons/assign.pass.cpp 
Wed Jul 11 14:20:42 2018
@@ -41,7 +41,7 @@ int main () {
 
 #if TEST_STD_VER > 11
 static_assert( test({ "abc", 3}), "");
-#ifdef _LIBCPP_HAS_NO_UNICODE_CHARS
+#ifndef _LIBCPP_HAS_NO_UNICODE_CHARS
 static_assert( test ({u"abc", 3}), "");
 static_assert( test ({U"abc", 3}), "");
 #endif


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

[libcxx] r336856 - Same reversed ifdef happened twice. Test fix only, NFC to the library.

[Marshall Clow via cfe-commits]

Author: marshall
Date: Wed Jul 11 14:22:13 2018
New Revision: 336856

URL: http://llvm.org/viewvc/llvm-project?rev=336856&view=rev
Log:
Same reversed ifdef happened twice. Test fix only, NFC to the library.


Modified:
libcxx/trunk/test/std/strings/string.view/string.view.cons/assign.pass.cpp

Modified: 
libcxx/trunk/test/std/strings/string.view/string.view.cons/assign.pass.cpp
URL: 
http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/std/strings/string.view/string.view.cons/assign.pass.cpp?rev=336856&r1=336855&r2=336856&view=diff
==
--- libcxx/trunk/test/std/strings/string.view/string.view.cons/assign.pass.cpp 
(original)
+++ libcxx/trunk/test/std/strings/string.view/string.view.cons/assign.pass.cpp 
Wed Jul 11 14:22:13 2018
@@ -33,7 +33,7 @@ bool test (T sv0)
 int main () {
 
 assert( test( "1234"));
-#ifdef _LIBCPP_HAS_NO_UNICODE_CHARS
+#ifndef _LIBCPP_HAS_NO_UNICODE_CHARS
 assert( test (u"1234"));
 assert( test (U"1234"));
 #endif


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

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

ping


Repository:
  rC Clang

https://reviews.llvm.org/D45898



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

[PATCH] D46845: [libcxx][c++17] P0083R5: Splicing Maps and Sets

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

I forgot to mention it in my initial review, but it also seems like you forgot 
to update all the synopsis in the comments at the beginning of headers.


https://reviews.llvm.org/D46845



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

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

After looking at a few more use cases of the in-memory file system, I think a 
problem we need to address is the consistency of file paths you get from clang 
when using in-mem vfs.  The clang-move tests that you have mitigated in 
https://reviews.llvm.org/D48951 could be an example.

For example, suppose you have header search directories `-I/path/to/include` 
and `-I.` in your compile command. When preprocessor searches for an #include 
"x.h", it will try to stat "/path/to/include/x.h" and "./x.h" and take the 
first one that exists. This can introduce indeterminism for the path (./x.h or 
/abs/x.h) you later get for the header file, e.g. when you try to look up file 
name by `FileID` through `SourceManager`. The path you get for a `FileEntry` or 
`FileID`  would depend on how clang looks up a file and how a file is first 
opened into `SourceManager`/`FileManager`.  It seems that the current behavior 
of clangd + in-memory file system would give you paths that are relative to the 
working directory for both cases. I'm not sure if that's the best behavior, but 
I think the consistency has its value. For example, in unit tests where 
in-memory file systems are heavily used, it's important to have a way to 
compare the reported file path (e.g. file path corresponding to a source 
location) with the expected paths. We could choose to always return real path, 
which could be potentially expensive, or we could require users to always 
compare real paths (it's unclear how well this would work though e.g. ClangTool 
doesn't expose its vfs), but they need to be enforced by the API. Otherwise, I 
worry it would cause more confusions for folks who use clang with in-memory 
file system in the future.


Repository:
  rC Clang

https://reviews.llvm.org/D48903



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