[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Thanks for the patch.

Just a little bit of feedback but overall I'm happy with the approach taken.




Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:99-106
+  CharSourceRange ReplaceRange;
+  if (isa(CastExpr))
+ReplaceRange = CharSourceRange::getCharRange(
+CastExpr->getLParenLoc(),
+CastExpr->getSubExprAsWritten()->getBeginLoc());
+  else if (isa(CastExpr))
+ReplaceRange = CharSourceRange::getCharRange(CastExpr->getBeginLoc(),

The majority of `checkExpr()`'s contents are common to both types, 
`CStyleCastExpr` and `CXXFunctionalCastExpr`.
Only the `ReplaceRange = CharSourceRange::getCharRange...` and the 
`DestTypeString = Lexer::getSourceText...` parts change depending on the Expr 
type.

What about breaking those two assignments out into their own functions, rather 
than templating the entire `checkExpr()` function?

For example, (note: untested code)

```lang=cpp
clang::CharSourceRange GetReplaceRange(const CStyleCastExpr *CastExpr) {
  return CharSourceRange::getCharRange(
  CastExpr->getLParenLoc(), CastExpr->getSubExprAsWritten()->getBeginLoc());
}

clang::CharSourceRange GetReplaceRange(const CXXFunctionalCastExpr *CastExpr) {
  return CharSourceRange::getCharRange(CastExpr->getBeginLoc(),
   CastExpr->getLParenLoc());
}

...

CharSourceRange ReplaceRange =
  isa(CastExpr)
  ? GetReplaceRange(dyn_cast(CastExpr))
  : GetReplaceRange(dyn_cast(CastExpr));
```

Would something like that work?



Comment at: 
clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:151-162
+  if (isa(CastExpr))
+DestTypeString =
+Lexer::getSourceText(CharSourceRange::getTokenRange(
+ CastExpr->getLParenLoc().getLocWithOffset(1),
+ 
CastExpr->getRParenLoc().getLocWithOffset(-1)),
+ SM, getLangOpts());
+  else if (isa(CastExpr))

See comment above.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:139-140
 
+- Updated `google-readability-casting` to diagnose and fix functional casts, 
to achieve feature
+  parity with the corresponding `cpplint.py` check.
 

Single back-ticks are used to do linking. Double back-ticks is probably what 
you're after.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:335
+  const char *str = "foo";
+  auto s = S(str);
+}

Is a test to check `new int(x)` worth including? I see that the cpplint guys 
explicitly filter it out of their results.


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

https://reviews.llvm.org/D114427

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

[PATCH] D113558: [clang-tidy] Fix cppcoreguidelines-virtual-base-class-destructor in macros

[Whisperity via Phabricator via cfe-commits]

whisperity accepted this revision.
whisperity added a comment.

I'm not sure if it is a good idea to merge things on a Friday, but I am 
comfortable with this, let's get the check crash less.


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

https://reviews.llvm.org/D113558

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

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Hi, Could anyone please review this diff? @whisperity, @aaron.ballman


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

https://reviews.llvm.org/D107450

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:100-104
+  if (isa(CastExpr))
+ReplaceRange = CharSourceRange::getCharRange(
+CastExpr->getLParenLoc(),
+CastExpr->getSubExprAsWritten()->getBeginLoc());
+  else if (isa(CastExpr))

Should this be an `if ... else`?


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

https://reviews.llvm.org/D114427

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

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Thank you, this is getting much better!




Comment at: 
clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp:105-108
+  const auto *PositiveCheck = Result.Nodes.getNodeAs("positive");
+  const auto *NegativeCheck = Result.Nodes.getNodeAs("negative");
+  bool Negated = NegativeCheck != nullptr;
+  const auto *Check = Negated ? NegativeCheck : PositiveCheck;

avogelsgesang wrote:
> whisperity wrote:
> > `Comparison` instead of `Check`? These should be matching the 
> > `binaryOperator`, right?
> In most cases, they match the `binaryOperator`. For the first pattern they 
> match the `implicitCastExpr`, though
> 
> Given this is not always a `binaryOperator`, should I still rename it to 
> `Comparison`?
`Comparison` is okay. Just `Check`, `Positive` and `Negative` seem a bit 
ambiguous at first glance.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst:6
+
+Finds usages of `container.count()` and `container.find() == container.end()` 
which should be replaced by a call to the `container.contains()` method 
introduced in C++ 20.
+

avogelsgesang wrote:
> whisperity wrote:
> > whisperity wrote:
> > > Same comment about the backtick count and how you would like the 
> > > rendering to be. Please build the documentation locally and verify 
> > > visually, as both ways most likely compile without warnings or errors.
> > 
> > Please build the documentation locally [...]
> 
> How do I actually do that?
You need to install [[ http://pypi.org/project/Sphinx | `Sphinx`]]. The easiest 
is to install it with //`pip`//, the Python package manager. Then, you tell 
LLVM CMake to create the build targets for the docs, and run it:

```lang=bash
~$ python3 -m venv sphinx-venv
~$ source ~/sphinx-venv/bin/activate
(sphinx-venv) ~$ pip install sphinx
(sphinx-venv) ~$ cd LLVM_BUILD_DIR
(sphinx-venv) LLVM_BUILD_DIR/$ cmake . -DLLVM_BUILD_DOCS=ON 
-DLLVM_ENABLE_SPHINX=ON
# configuration re-run
(sphinx-venv) LLVM_BUILD_DIR/$ cmake --build . -- docs-clang-tools-html
(sphinx-venv) LLVM_BUILD_DIR/$ cd ~
(sphinx-venv) ~$ deactivate
~$ # you are now back in HOME without the virtual env being active
```

The docs will be available starting from 
`${LLVM_BUILD_DIR}/tools/clang/tools/extra/docs/html/clang-tidy/index.html`.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:115
+
+// This is effectively a membership check, as the result is implicitely casted 
to `bool`
+bool returnContains(std::map &M) {

Make sure to format it against 80-line, and also, we conventionally write full 
sentences in comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:129
+
+// Check that we are not confused by aliases
+namespace s2 = std;





Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:174
+
+// The following map has the same interface like `std::map`
+template 





Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:189
+// NO-WARNING.
+// CHECK-FIXES: if (MyMap.count(0))
+return nullptr;

If a fix is not generated, why is this line here?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp:33-35
+
+using namespace std;
+

avogelsgesang wrote:
> whisperity wrote:
> > Tests are to guard our future selves from breaking the system, so perhaps 
> > two tests that involve having `std::` typed out, and also using a different 
> > container that's not `std::whatever` would be useful.
> > 
> > 
> > 
> > Do you think it would be worthwhile to add matching any user-defined object 
> > which has a `count()` and a `contains()` method (with the appropriate 
> > number of parameters) later? So match not only the few standard containers, 
> > but more stuff?
> > 
> > It doesn't have to be done now, but having a test for `MyContainer` not in 
> > `std::` being marked `// NO-WARNING.` or something could indicate that we 
> > purposefully don't want to go down that road just yet.
> > so perhaps two tests that involve having std:: typed out
> 
> rewrote the tests, such that most of them use fully-qualified types. Also 
> added a few test cases involving type defs and namespace aliases (this 
> actually uncovered a mistake in the matcher)
> 
> > Do you think it would be worthwhile to add matching any user-defined object 
> > which has a count() and a contains() method (with the appropriate number of 
> > parameters) later?
> 
> Not sure. At least not for the code base I wrote this check for...
> 
> > having a test for MyContainer not in std:: being marked // NO-WARNING. or 
> > something could indicate that we purposefully don't want to g

[PATCH] D114522: [clangd] Add desugared type to hover

[liu hui via Phabricator via cfe-commits]

lh123 updated this revision to Diff 389931.
lh123 added a comment.
Herald added a reviewer: gkistanova.

Move some `desugared type` to `Definition`.


Repository:
  rZORG LLVM Github Zorg

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

https://reviews.llvm.org/D114522

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Hover.h
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang/include/clang/AST/ASTDiagnostic.h
  clang/lib/AST/ASTDiagnostic.cpp

Index: clang/lib/AST/ASTDiagnostic.cpp
===
--- clang/lib/AST/ASTDiagnostic.cpp
+++ clang/lib/AST/ASTDiagnostic.cpp
@@ -26,7 +26,7 @@
 
 // Returns a desugared version of the QualType, and marks ShouldAKA as true
 // whenever we remove significant sugar from the type.
-static QualType Desugar(ASTContext &Context, QualType QT, bool &ShouldAKA) {
+QualType clang::Desugar(ASTContext &Context, QualType QT, bool &ShouldAKA) {
   QualifierCollector QC;
 
   while (true) {
Index: clang/include/clang/AST/ASTDiagnostic.h
===
--- clang/include/clang/AST/ASTDiagnostic.h
+++ clang/include/clang/AST/ASTDiagnostic.h
@@ -9,6 +9,7 @@
 #ifndef LLVM_CLANG_AST_ASTDIAGNOSTIC_H
 #define LLVM_CLANG_AST_ASTDIAGNOSTIC_H
 
+#include "clang/AST/Type.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticAST.h"
 
@@ -31,6 +32,10 @@
   SmallVectorImpl &Output,
   void *Cookie,
   ArrayRef QualTypeVals);
+
+  /// Returns a desugared version of the QualType, and marks ShouldAKA as true
+  /// whenever we remove significant sugar from the type.
+  QualType Desugar(ASTContext &Context, QualType QT, bool &ShouldAKA);
 }  // end namespace clang
 
 #endif
Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -45,8 +45,8 @@
  HI.Kind = index::SymbolKind::Function;
  HI.Documentation = "Best foo ever.";
  HI.Definition = "void foo()";
- HI.ReturnType = "void";
- HI.Type = "void ()";
+ HI.ReturnType = {"void", llvm::None};
+ HI.Type = {"void ()", llvm::None};
  HI.Parameters.emplace();
}},
   // Inside namespace
@@ -62,8 +62,8 @@
  HI.Kind = index::SymbolKind::Function;
  HI.Documentation = "Best foo ever.";
  HI.Definition = "void foo()";
- HI.ReturnType = "void";
- HI.Type = "void ()";
+ HI.ReturnType = {"void", llvm::None};
+ HI.Type = {"void ()", llvm::None};
  HI.Parameters.emplace();
}},
   // Field
@@ -81,7 +81,7 @@
  HI.Name = "bar";
  HI.Kind = index::SymbolKind::Field;
  HI.Definition = "char bar";
- HI.Type = "char";
+ HI.Type = {"char", llvm::None};
  HI.Offset = 0;
  HI.Size = 1;
  HI.Padding = 7;
@@ -100,7 +100,7 @@
  HI.Name = "bar";
  HI.Kind = index::SymbolKind::Field;
  HI.Definition = "char bar";
- HI.Type = "char";
+ HI.Type = {"char", llvm::None};
  HI.Size = 1;
  HI.Padding = 15;
  HI.AccessSpecifier = "public";
@@ -118,7 +118,7 @@
  HI.Name = "x";
  HI.Kind = index::SymbolKind::Field;
  HI.Definition = "int x : 1";
- HI.Type = "int";
+ HI.Type = {"int", llvm::None};
  HI.AccessSpecifier = "public";
}},
   // Local to class method.
@@ -137,7 +137,7 @@
  HI.Name = "bar";
  HI.Kind = index::SymbolKind::Variable;
  HI.Definition = "int bar";
- HI.Type = "int";
+ HI.Type = {"int", llvm::None};
}},
   // Anon namespace and local scope.
   {R"cpp(
@@ -153,7 +153,7 @@
  HI.Name = "bar";
  HI.Kind = index::SymbolKind::Field;
  HI.Definition = "char bar";
- HI.Type = "char";
+ HI.Type = {"char", llvm::None};
  HI.Offset = 0;
  HI.Size = 1;
  HI.AccessSpecifier = "public";
@@ -179,7 +179,7 @@
  HI.Name = "foo";
  HI.Kind = index::SymbolKind::Variable;
  HI.Definition = "Foo foo = Foo(5)";
- HI.Type = "Foo";
+ HI.Type = {"Foo", llvm::None};
}},
   // Implicit template instantiation
   {R"cpp(
@@ -211,12 +211,19 @@
   bool Q = false, class... Ts>
 class Foo {})cpp";
  HI.TemplateParameters = {
- {std::string("template  class"),
-  std::string("C"), llvm::None},
- {std::string("typename"), llvm::None, std::string("char")},
- {std::string("int"), llvm::None, std::string("0")},
- {std::string("bool"), std::string("Q"), std::string("false")},
- {std::string("class..."), std::string("Ts"), llvm::None},
+ {{{std::s

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

This generally looks fine for me, but I'd rather have other people who are more 
knowledgeable about the standard's intricacies looked at it too.

AFAIK Aaron is busy this week and the next (?) due to C++ Committee meetings, 
or something like that. And I bet after such meetings everyone would take a few 
more days off from looking at C++ stuff.


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

https://reviews.llvm.org/D107450

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

[PATCH] D107450: [clang-tidy] Fix wrong FixIt in performance-move-const-arg

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

In D107450#3155019 , @whisperity 
wrote:

> This generally looks fine for me, but I'd rather have other people who are 
> more knowledgeable about the standard's intricacies looked at it too.
>
> AFAIK Aaron is busy this week and the next (?) due to C++ Committee meetings, 
> or something like that. And I bet after such meetings everyone would take a 
> few more days off from looking at C++ stuff.

Ok, that sounds great, thanks a lot!


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

https://reviews.llvm.org/D107450

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

[PATCH] D114522: [clangd] Add desugared type to hover

[liu hui via Phabricator via cfe-commits]

lh123 added inline comments.



Comment at: clang-tools-extra/clangd/Hover.cpp:691
   } else {
 HI.Definition = printType(QT, PP);
 

sammccall wrote:
> This seems like a pretty important case to handle.
> A wrinkle is that this gets treated as C++ code (e.g. client-side syntax 
> highlighted).
> 
> So we might need something like
> 
> ```
> int64_t
> // aka
> long long
> ```
> 
> or
> ```
> int64_t
> // aka: long long
> ```
It seems we can't do this, vscode always display
```
int64_t
// aka: long long
```
as
```
int64_t
// aka: long long
```

So I change it to `int64_t // aka: long long`


Repository:
  rZORG LLVM Github Zorg

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

https://reviews.llvm.org/D114522

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

[PATCH] D114320: [clang-format] Extend AllowShortBlocksOnASingleLine for else blocks

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D114320#3154544 , @jessesna wrote:

> Thanks a lot for your time and help @MyDeveloperDay

No problem, come play some more! We need people to help and review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114320

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

[PATCH] D114292: [clang-tidy] Fix `altera-struct-pack-align` check for empty structs

[Whisperity via Phabricator via cfe-commits]

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

I believe this is worth a note in the `ReleaseNotes.rst` file. People who might 
have disabled the check specifically for the reason outlined in the PR would 
get to know it's safe to reenable it.




Comment at: clang-tools-extra/clang-tidy/altera/StructPackAlignCheck.cpp:77
+  CharUnits::fromQuantity(std::max(
+  ceil((float)TotalBitSize / CharSize), 1));
   CharUnits MaxAlign = CharUnits::fromQuantity(

If we are changing this, can we make this more C++-y?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114292

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

[PATCH] D113518: [clang][Sema] Create delegating constructors even in templates

[Whisperity via Phabricator via cfe-commits]

whisperity edited reviewers, added: Quuxplusone; removed: carlosgalvezp, 
whisperity.
whisperity added subscribers: carlosgalvezp, whisperity.
whisperity added a comment.
This revision now requires review to proceed.

(@carlosgalvezp Removing you from reviewers so the patch shows up as not yet 
reviewed for others! The significant part is the Sema which should be reviewed.)


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

https://reviews.llvm.org/D113518

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

[PATCH] D113518: [clang][Sema] Create delegating constructors even in templates

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Thanks for the finding and sorry for delaying the review, I didn't know that :( 
Do you know if there's an option for signaling "LGTM but I want someone else to 
review?". Otherwise I can just leave a comment and don't accept the revision.


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

https://reviews.llvm.org/D113518

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

[PATCH] D113518: [clang][Sema] Create delegating constructors even in templates

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D113518#3155040 , @carlosgalvezp 
wrote:

> Thanks for the finding and sorry for delaying the review, I didn't know that 
> :( Do you know if there's an option for signaling "LGTM but I want someone 
> else to review?". Otherwise I can just leave a comment and don't accept the 
> revision.

There is an action "resign from review", but as I have taken you off explicitly 
and by force, no need for that. 🙂 I just wrote the comment FYI so that you know 
I'm not just interfering destructively! (And due to no accepting reviewers 
remaining acting as such, the patch automatically went back to //"now require 
review to proceed"//.)


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

https://reviews.llvm.org/D113518

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

[clang] bad8977 - [clang] Change ordering of PreableCallbacks to make sure PP can be referenced in them

[Kirill Bobyrev via cfe-commits]

Author: Kirill Bobyrev
Date: 2021-11-26T10:10:52+01:00
New Revision: bad8977786382739256f4bd966fb4cdcfd50be2a

URL: 
https://github.com/llvm/llvm-project/commit/bad8977786382739256f4bd966fb4cdcfd50be2a
DIFF: 
https://github.com/llvm/llvm-project/commit/bad8977786382739256f4bd966fb4cdcfd50be2a.diff

LOG: [clang] Change ordering of PreableCallbacks to make sure PP can be 
referenced in them

Currently, BeforeExecute is called before BeginSourceFile which does not allow
using PP in the callbacks. Change the ordering to ensure it is possible.
This is a prerequisite for D114370.

Originated from a discussion with @kadircet.

Reviewed By: sammccall

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

Added: 


Modified: 
clang/include/clang/Frontend/PrecompiledPreamble.h
clang/lib/Frontend/PrecompiledPreamble.cpp

Removed: 




diff  --git a/clang/include/clang/Frontend/PrecompiledPreamble.h 
b/clang/include/clang/Frontend/PrecompiledPreamble.h
index bb7fd97fe5df4..dacbffef0b121 100644
--- a/clang/include/clang/Frontend/PrecompiledPreamble.h
+++ b/clang/include/clang/Frontend/PrecompiledPreamble.h
@@ -274,7 +274,7 @@ class PreambleCallbacks {
 public:
   virtual ~PreambleCallbacks() = default;
 
-  /// Called before FrontendAction::BeginSourceFile.
+  /// Called before FrontendAction::Execute.
   /// Can be used to store references to various CompilerInstance fields
   /// (e.g. SourceManager) that may be interesting to the consumers of other
   /// callbacks.
@@ -291,7 +291,7 @@ class PreambleCallbacks {
   /// used instead, but having only this method allows a simpler API.
   virtual void HandleTopLevelDecl(DeclGroupRef DG);
   /// Creates wrapper class for PPCallbacks so we can also process information
-  /// about includes that are inside of a preamble
+  /// about includes that are inside of a preamble. Called after BeforeExecute.
   virtual std::unique_ptr createPPCallbacks();
   /// The returned CommentHandler will be added to the preprocessor if not 
null.
   virtual CommentHandler *getCommentHandler();

diff  --git a/clang/lib/Frontend/PrecompiledPreamble.cpp 
b/clang/lib/Frontend/PrecompiledPreamble.cpp
index af82ab3f5558b..8aa80a4c96fb4 100644
--- a/clang/lib/Frontend/PrecompiledPreamble.cpp
+++ b/clang/lib/Frontend/PrecompiledPreamble.cpp
@@ -412,10 +412,13 @@ llvm::ErrorOr 
PrecompiledPreamble::Build(
   std::unique_ptr Act;
   Act.reset(new PrecompilePreambleAction(
   StoreInMemory ? &Storage.asMemory().Data : nullptr, Callbacks));
-  Callbacks.BeforeExecute(*Clang);
   if (!Act->BeginSourceFile(*Clang.get(), Clang->getFrontendOpts().Inputs[0]))
 return BuildPreambleError::BeginSourceFileFailed;
 
+  // Performed after BeginSourceFile to ensure Clang->Preprocessor can be
+  // referenced in the callback.
+  Callbacks.BeforeExecute(*Clang);
+
   std::unique_ptr DelegatedPPCallbacks =
   Callbacks.createPPCallbacks();
   if (DelegatedPPCallbacks)



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

[PATCH] D114525: [clang] Change ordering of PreableCallbacks to make sure PP can be referenced in them

[Kirill Bobyrev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbad897778638: [clang] Change ordering of PreableCallbacks to 
make sure PP can be referenced… (authored by kbobyrev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114525

Files:
  clang/include/clang/Frontend/PrecompiledPreamble.h
  clang/lib/Frontend/PrecompiledPreamble.cpp


Index: clang/lib/Frontend/PrecompiledPreamble.cpp
===
--- clang/lib/Frontend/PrecompiledPreamble.cpp
+++ clang/lib/Frontend/PrecompiledPreamble.cpp
@@ -412,10 +412,13 @@
   std::unique_ptr Act;
   Act.reset(new PrecompilePreambleAction(
   StoreInMemory ? &Storage.asMemory().Data : nullptr, Callbacks));
-  Callbacks.BeforeExecute(*Clang);
   if (!Act->BeginSourceFile(*Clang.get(), Clang->getFrontendOpts().Inputs[0]))
 return BuildPreambleError::BeginSourceFileFailed;
 
+  // Performed after BeginSourceFile to ensure Clang->Preprocessor can be
+  // referenced in the callback.
+  Callbacks.BeforeExecute(*Clang);
+
   std::unique_ptr DelegatedPPCallbacks =
   Callbacks.createPPCallbacks();
   if (DelegatedPPCallbacks)
Index: clang/include/clang/Frontend/PrecompiledPreamble.h
===
--- clang/include/clang/Frontend/PrecompiledPreamble.h
+++ clang/include/clang/Frontend/PrecompiledPreamble.h
@@ -274,7 +274,7 @@
 public:
   virtual ~PreambleCallbacks() = default;
 
-  /// Called before FrontendAction::BeginSourceFile.
+  /// Called before FrontendAction::Execute.
   /// Can be used to store references to various CompilerInstance fields
   /// (e.g. SourceManager) that may be interesting to the consumers of other
   /// callbacks.
@@ -291,7 +291,7 @@
   /// used instead, but having only this method allows a simpler API.
   virtual void HandleTopLevelDecl(DeclGroupRef DG);
   /// Creates wrapper class for PPCallbacks so we can also process information
-  /// about includes that are inside of a preamble
+  /// about includes that are inside of a preamble. Called after BeforeExecute.
   virtual std::unique_ptr createPPCallbacks();
   /// The returned CommentHandler will be added to the preprocessor if not 
null.
   virtual CommentHandler *getCommentHandler();


Index: clang/lib/Frontend/PrecompiledPreamble.cpp
===
--- clang/lib/Frontend/PrecompiledPreamble.cpp
+++ clang/lib/Frontend/PrecompiledPreamble.cpp
@@ -412,10 +412,13 @@
   std::unique_ptr Act;
   Act.reset(new PrecompilePreambleAction(
   StoreInMemory ? &Storage.asMemory().Data : nullptr, Callbacks));
-  Callbacks.BeforeExecute(*Clang);
   if (!Act->BeginSourceFile(*Clang.get(), Clang->getFrontendOpts().Inputs[0]))
 return BuildPreambleError::BeginSourceFileFailed;
 
+  // Performed after BeginSourceFile to ensure Clang->Preprocessor can be
+  // referenced in the callback.
+  Callbacks.BeforeExecute(*Clang);
+
   std::unique_ptr DelegatedPPCallbacks =
   Callbacks.createPPCallbacks();
   if (DelegatedPPCallbacks)
Index: clang/include/clang/Frontend/PrecompiledPreamble.h
===
--- clang/include/clang/Frontend/PrecompiledPreamble.h
+++ clang/include/clang/Frontend/PrecompiledPreamble.h
@@ -274,7 +274,7 @@
 public:
   virtual ~PreambleCallbacks() = default;
 
-  /// Called before FrontendAction::BeginSourceFile.
+  /// Called before FrontendAction::Execute.
   /// Can be used to store references to various CompilerInstance fields
   /// (e.g. SourceManager) that may be interesting to the consumers of other
   /// callbacks.
@@ -291,7 +291,7 @@
   /// used instead, but having only this method allows a simpler API.
   virtual void HandleTopLevelDecl(DeclGroupRef DG);
   /// Creates wrapper class for PPCallbacks so we can also process information
-  /// about includes that are inside of a preamble
+  /// about includes that are inside of a preamble. Called after BeforeExecute.
   virtual std::unique_ptr createPPCallbacks();
   /// The returned CommentHandler will be added to the preprocessor if not null.
   virtual CommentHandler *getCommentHandler();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114609: [clang] Fix crash on broken parameter declarators

[Haojian Wu via Phabricator via cfe-commits]

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

Thanks!




Comment at: clang/test/Parser/cxx-decltype-parameter.cpp:1
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+

I'd place this test in `Parser/cxx-keyword-identifiers.cpp` since the crash is 
in the relevant code path.



Comment at: clang/test/Parser/cxx-decltype-parameter.cpp:5
+struct Foo {
+  Bar bar(*decltype(Bar{}) aux); // expected-error {{C++ requires a type 
specifier for all declarations}}. \
+ // expected-error {{expected ')'}} 
expected-note {{to match this '('}}

this can be simplified further:  `void bar(*decltype(1) aux);`




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114609

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

[PATCH] D114522: [clangd] Add desugared type to hover

[liu hui via Phabricator via cfe-commits]

lh123 added inline comments.



Comment at: clang-tools-extra/clangd/Hover.cpp:166
+if (ShouldAKA)
+  AKA = Result + DesugaredTy.getAsString(PP);
+  }

It seems we lost `namespace qualifiers`.
It always display `std::string` as `basic_string`


Repository:
  rZORG LLVM Github Zorg

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

https://reviews.llvm.org/D114522

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

[PATCH] D103317: [Analyzer][Core] Make SValBuilder to better simplify svals with 3 symbols in the tree

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 389935.
martong marked an inline comment as done.
martong added a comment.

- Add new test case (for no-crash)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103317

Files:
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/test/Analysis/svalbuilder-simplify-compound-svals.cpp
  clang/test/Analysis/taint-tester.c

Index: clang/test/Analysis/taint-tester.c
===
--- clang/test/Analysis/taint-tester.c
+++ clang/test/Analysis/taint-tester.c
@@ -59,7 +59,7 @@
   int tty = xy.y; // expected-warning + {{tainted}}
 }
 
-void BitwiseOp(int in, char inn) {
+void BitwiseOp(int in, char inn, int zz) {
   // Taint on bitwise operations, integer to integer cast.
   int m;
   int x = 0;
@@ -67,11 +67,12 @@
   int y = (in << (x << in)) * 5;// expected-warning + {{tainted}}
   // The next line tests integer to integer cast.
   int z = y & inn; // expected-warning + {{tainted}}
-  if (y == 5) // expected-warning + {{tainted}}
+  if (y == zz) { // expected-warning + {{tainted}}
 m = z | z;// expected-warning + {{tainted}}
+  }
   else
 m = inn;
-  int mm = m; // expected-warning + {{tainted}}
+  int mm = m; // expected-warning 1 {{tainted}}
 }
 
 // Test getenv.
Index: clang/test/Analysis/svalbuilder-simplify-compound-svals.cpp
===
--- /dev/null
+++ clang/test/Analysis/svalbuilder-simplify-compound-svals.cpp
@@ -0,0 +1,77 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -verify
+
+// Here we test whether the SValBuilder is capable to simplify existing
+// compound SVals (where there are at leaset 3 symbols in the tree) based on
+// newly added constraints.
+
+void clang_analyzer_eval(bool);
+void clang_analyzer_warnIfReached();
+
+void test_left_tree_constrained(int x, int y, int z) {
+  if (x + y + z != 0)
+return;
+  if (x + y != 0)
+return;
+  clang_analyzer_eval(x + y + z == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(x + y == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(z == 0); // expected-warning{{TRUE}}
+  x = y = z = 1;
+  return;
+}
+
+void test_right_tree_constrained(int x, int y, int z) {
+  if (x + y * z != 0)
+return;
+  if (y * z != 0)
+return;
+  clang_analyzer_eval(x + y * z == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(y * z == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
+  return;
+}
+
+void test_left_tree_constrained_minus(int x, int y, int z) {
+  if (x - y - z != 0)
+return;
+  if (x - y != 0)
+return;
+  clang_analyzer_eval(x - y - z == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(x - y == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(z == 0); // expected-warning{{TRUE}}
+  x = y = z = 1;
+  return;
+}
+
+void test_SymInt_constrained(int x, int y, int z) {
+  if (x * y * z != 4)
+return;
+  if (z != 2)
+return;
+  if (x * y == 3) {
+clang_analyzer_warnIfReached(); // no-warning
+return;
+  }
+  (void)(x * y * z);
+}
+
+void test_SValBuilder_simplifies_IntSym(int x, int y, int z) {
+  // Most IntSym BinOps are transformed to SymInt in SimpleSValBuilder.
+  // Division is one exception.
+  x = 77 / (y + z);
+  if (y + z != 1)
+return;
+  clang_analyzer_eval(x == 77); // expected-warning{{TRUE}}
+  (void)(x * y * z);
+}
+
+void recurring_symbol(int b, int a) {
+  if (b * b != b) {
+if ((b * b) * b * b != (b * b) * b)
+  if (b * b == 1)
+; // no-crash (assertion should not be violated)
+  }
+}
Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -1102,7 +1102,6 @@
   if (SymbolRef Sym = V.getAsSymbol())
 return state->getConstraintManager().getSymVal(state, Sym);
 
-  // FIXME: Add support for SymExprs.
   return nullptr;
 }
 
@@ -1134,6 +1133,24 @@
   return cache(Sym, SVB.makeSymbolVal(Sym));
 }
 
+// Return the known const value for the Sym if available, or return Undef
+// otherwise.
+SVal getConst(SymbolRef Sym) {
+  const llvm::APSInt *Const =
+  State->getConstraintManager().getSymVal(State, Sym);
+  if (Const)
+return Loc::isLocType(Sym->getType()) ? (SVal)SVB.makeIntLocVal(*Const)
+  : (SVal)SVB.makeIntVal(*Const);
+  return UndefinedVal();
+}
+
+SVal getConstOrVisit(SymbolRef Sym) {
+  const SVal Ret = getConst(Sym);
+  if (Ret.isUndef())
+return Visit(Sym);
+  return Ret;
+}
+
   

[PATCH] D103317: [Analyzer][Core] Make SValBuilder to better simplify svals with 3 symbols in the tree

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 389936.
martong added a comment.

- Remove superfluous param (int a) from the new test case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103317

Files:
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/test/Analysis/svalbuilder-simplify-compound-svals.cpp
  clang/test/Analysis/taint-tester.c

Index: clang/test/Analysis/taint-tester.c
===
--- clang/test/Analysis/taint-tester.c
+++ clang/test/Analysis/taint-tester.c
@@ -59,7 +59,7 @@
   int tty = xy.y; // expected-warning + {{tainted}}
 }
 
-void BitwiseOp(int in, char inn) {
+void BitwiseOp(int in, char inn, int zz) {
   // Taint on bitwise operations, integer to integer cast.
   int m;
   int x = 0;
@@ -67,11 +67,12 @@
   int y = (in << (x << in)) * 5;// expected-warning + {{tainted}}
   // The next line tests integer to integer cast.
   int z = y & inn; // expected-warning + {{tainted}}
-  if (y == 5) // expected-warning + {{tainted}}
+  if (y == zz) { // expected-warning + {{tainted}}
 m = z | z;// expected-warning + {{tainted}}
+  }
   else
 m = inn;
-  int mm = m; // expected-warning + {{tainted}}
+  int mm = m; // expected-warning 1 {{tainted}}
 }
 
 // Test getenv.
Index: clang/test/Analysis/svalbuilder-simplify-compound-svals.cpp
===
--- /dev/null
+++ clang/test/Analysis/svalbuilder-simplify-compound-svals.cpp
@@ -0,0 +1,77 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -verify
+
+// Here we test whether the SValBuilder is capable to simplify existing
+// compound SVals (where there are at leaset 3 symbols in the tree) based on
+// newly added constraints.
+
+void clang_analyzer_eval(bool);
+void clang_analyzer_warnIfReached();
+
+void test_left_tree_constrained(int x, int y, int z) {
+  if (x + y + z != 0)
+return;
+  if (x + y != 0)
+return;
+  clang_analyzer_eval(x + y + z == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(x + y == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(z == 0); // expected-warning{{TRUE}}
+  x = y = z = 1;
+  return;
+}
+
+void test_right_tree_constrained(int x, int y, int z) {
+  if (x + y * z != 0)
+return;
+  if (y * z != 0)
+return;
+  clang_analyzer_eval(x + y * z == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(y * z == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
+  return;
+}
+
+void test_left_tree_constrained_minus(int x, int y, int z) {
+  if (x - y - z != 0)
+return;
+  if (x - y != 0)
+return;
+  clang_analyzer_eval(x - y - z == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(x - y == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(z == 0); // expected-warning{{TRUE}}
+  x = y = z = 1;
+  return;
+}
+
+void test_SymInt_constrained(int x, int y, int z) {
+  if (x * y * z != 4)
+return;
+  if (z != 2)
+return;
+  if (x * y == 3) {
+clang_analyzer_warnIfReached(); // no-warning
+return;
+  }
+  (void)(x * y * z);
+}
+
+void test_SValBuilder_simplifies_IntSym(int x, int y, int z) {
+  // Most IntSym BinOps are transformed to SymInt in SimpleSValBuilder.
+  // Division is one exception.
+  x = 77 / (y + z);
+  if (y + z != 1)
+return;
+  clang_analyzer_eval(x == 77); // expected-warning{{TRUE}}
+  (void)(x * y * z);
+}
+
+void recurring_symbol(int b) {
+  if (b * b != b) {
+if ((b * b) * b * b != (b * b) * b)
+  if (b * b == 1)
+; // no-crash (assertion should not be violated)
+  }
+}
Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -1102,7 +1102,6 @@
   if (SymbolRef Sym = V.getAsSymbol())
 return state->getConstraintManager().getSymVal(state, Sym);
 
-  // FIXME: Add support for SymExprs.
   return nullptr;
 }
 
@@ -1134,6 +1133,24 @@
   return cache(Sym, SVB.makeSymbolVal(Sym));
 }
 
+// Return the known const value for the Sym if available, or return Undef
+// otherwise.
+SVal getConst(SymbolRef Sym) {
+  const llvm::APSInt *Const =
+  State->getConstraintManager().getSymVal(State, Sym);
+  if (Const)
+return Loc::isLocType(Sym->getType()) ? (SVal)SVB.makeIntLocVal(*Const)
+  : (SVal)SVB.makeIntVal(*Const);
+  return UndefinedVal();
+}
+
+SVal getConstOrVisit(SymbolRef Sym) {
+  const SVal Ret = getConst(Sym);
+  if (Ret.isUndef())
+return Visit(Sym);
+  return Ret;
+}
+
   public:
 Simplifier(Pr

[PATCH] D114326: Update the list of CUDA versions up to 11.5

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp resigned from this revision.
carlosgalvezp added a comment.
This revision now requires review to proceed.

I just learned that by approving this patch I make it not visible for other 
reviewers that they should still review, so I'll remove my vote. LGTM though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114326

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

[PATCH] D103317: [Analyzer][Core] Make SValBuilder to better simplify svals with 3 symbols in the tree

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

I've added a new test case that triggers an assertion. That problem is getting 
fixed in a new parent patch, which I am about to add to the stack very soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103317

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

[PATCH] D114522: [clangd] Add desugared type to hover

[liu hui via Phabricator via cfe-commits]

lh123 added inline comments.



Comment at: clang-tools-extra/clangd/Hover.cpp:166
+if (ShouldAKA)
+  AKA = Result + DesugaredTy.getAsString(PP);
+  }

lh123 wrote:
> It seems we lost `namespace qualifiers`.
> It always display `std::string` as `basic_string`
Should we set `FullyQualifiedName` printing policy  for `DesugaredTy`.


Repository:
  rZORG LLVM Github Zorg

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

https://reviews.llvm.org/D114522

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

[PATCH] D72326: [clang-format] Rebased on master: Add option to specify explicit config file

[Thibault North via Phabricator via cfe-commits]

tnorth added a comment.

Hi there,
Yes, sorry for the long silence, I ended up not having time to come back to 
this anymore for personal reasons. I also lack familiarity with the codebase 
and tests, and I couldn't invest more time into it; I had the impression that 
this last failing test would require major changes.
I'm happy if this ends up being merged. Thanks for your time and guidance again.


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

https://reviews.llvm.org/D72326

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

[PATCH] D114522: [clangd] Add desugared type to hover

[liu hui via Phabricator via cfe-commits]

lh123 added inline comments.



Comment at: clang-tools-extra/clangd/Hover.cpp:166
+if (ShouldAKA)
+  AKA = Result + DesugaredTy.getAsString(PP);
+  }

lh123 wrote:
> lh123 wrote:
> > It seems we lost `namespace qualifiers`.
> > It always display `std::string` as `basic_string`
> Should we set `FullyQualifiedName` printing policy  for `DesugaredTy`.
Sometimes we need to distinguish certain types with the same name based on the 
`fully qualified name`.
eg.
```
namespace custom {
template  struct vector {};
} // namespace custom

void code() {
  custom::vector a;
  std::vector b;
}
```
Currently, both `a` and `b` show `vector`.


Repository:
  rZORG LLVM Github Zorg

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

https://reviews.llvm.org/D114522

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

[PATCH] D113804: [clang-tidy] Fix behavior of `modernize-use-using` with nested structs/unions

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

`hasParent()` is the //direct// upwards traversal, right? I remember some 
discussion about matchers like that being potentially costly. But I can't find 
any description of this, so I guess it's fine, rewriting the matcher to have 
the opposite logic 
(`decl(hasDescendant(...the-real-matcher-here...)).bind("blah")`) would just 
make everything ugly for no tangible benefit.




Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:29
+  Finder->addMatcher(typedefDecl(unless(isInstantiated()),
+ hasParent(decl().bind("parent-decl")))
+ .bind("typedef"),

(Nit: Once we have multiple uses of an identifier, it's better to hoist it to a 
definition and reuse it that way, to prevent future typos introducing arcane 
issues. Generally, a `static constexpr llvm::StringLiteral` in the TU's global 
scope, is sufficient for that.)



Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:50-52
+  if (!ParentDecl) {
+return;
+  }

(Style: Elid braces.)



Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:132-137
 bool Invalid;
-Type = std::string(
-Lexer::getSourceText(CharSourceRange::getTokenRange(LastTagDeclRange),
- *Result.SourceManager, getLangOpts(), &Invalid));
+Type = std::string(Lexer::getSourceText(
+CharSourceRange::getTokenRange(LastTagDeclRange->second),
+*Result.SourceManager, getLangOpts(), &Invalid));
 if (Invalid)
   return;

(Nit: if the source ranges are invalid, a default constructed `StringRef` is 
returned, which is empty and converts to a default-constructed `std::string`.)


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

https://reviews.llvm.org/D113804

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

[PATCH] D114370: [clangd] IncludeCleaner: Attribute symbols from non self-contained headers to their parents

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 389946.
kbobyrev marked 6 inline comments as done.
kbobyrev added a comment.

Address review comments. Next step: split this patch into two, as suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114370

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp

Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -82,8 +82,7 @@
 return {};
   }
   IncludeStructure Includes;
-  Clang->getPreprocessor().addPPCallbacks(
-  Includes.collect(Clang->getSourceManager()));
+  Clang->getPreprocessor().addPPCallbacks(Includes.collect(*Clang));
   if (llvm::Error Err = Action.Execute()) {
 ADD_FAILURE() << "failed to execute action: " << std::move(Err);
 return {};
Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -281,7 +281,9 @@
   auto &SM = AST.getSourceManager();
   auto &Includes = AST.getIncludeStructure();
 
-  auto ReferencedFiles = findReferencedFiles(findReferencedLocations(AST), SM);
+  auto ReferencedFiles =
+  findReferencedFiles(findReferencedLocations(AST),
+  AST.getIncludeStructure(), AST.getSourceManager());
   llvm::StringSet<> ReferencedFileNames;
   for (FileID FID : ReferencedFiles)
 ReferencedFileNames.insert(
@@ -303,6 +305,52 @@
   EXPECT_THAT(getUnused(AST, ReferencedHeaders), ::testing::IsEmpty());
 }
 
+TEST(IncludeCleaner, NonSelfContainedHeaders) {
+  TestTU TU;
+  TU.Code = R"cpp(
+#include "bar.h"
+#include "foo.h"
+
+int LocalFoo = foo::Variable,
+LocalBar = bar::Variable;
+)cpp";
+  TU.AdditionalFiles["bar.h"] = R"cpp(
+#pragma once
+namespace bar {
+#include "indirection.h"
+}
+)cpp";
+  TU.AdditionalFiles["foo.h"] = R"cpp(
+#ifndef FOO_H
+#define FOO_H
+namespace foo {
+#include "unguarded.h"
+}
+#endif // FOO_H
+)cpp";
+  TU.AdditionalFiles["indirection.h"] = R"cpp(
+#include "unguarded.h"
+)cpp";
+  TU.AdditionalFiles["unguarded.h"] = R"cpp(
+constexpr int Variable = 42;
+)cpp";
+
+  ParsedAST AST = TU.build();
+
+  auto ReferencedFiles =
+  findReferencedFiles(findReferencedLocations(AST),
+  AST.getIncludeStructure(), AST.getSourceManager());
+  llvm::StringSet<> ReferencedFileNames;
+  auto &SM = AST.getSourceManager();
+  for (FileID FID : ReferencedFiles)
+ReferencedFileNames.insert(
+SM.getPresumedLoc(SM.getLocForStartOfFile(FID)).getFilename());
+  // Note that we have uplifted the referenced files from non self-contained
+  // headers to header-guarded ones.
+  EXPECT_THAT(ReferencedFileNames.keys(),
+  UnorderedElementsAre(testPath("bar.h"), testPath("foo.h")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -80,8 +80,7 @@
 EXPECT_TRUE(
 Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
 IncludeStructure Includes;
-Clang->getPreprocessor().addPPCallbacks(
-Includes.collect(Clang->getSourceManager()));
+Clang->getPreprocessor().addPPCallbacks(Includes.collect(*Clang));
 EXPECT_FALSE(Action.Execute());
 Action.EndSourceFile();
 return Includes;
@@ -363,6 +362,39 @@
   EXPECT_THAT(collectIncludes().MainFileIncludes,
   Contains(AllOf(IncludeLine(2), Written("";
 }
+
+TEST_F(HeadersTest, SelfContainedHeaders) {
+  // Including through non-builtin file has no effects.
+  FS.Files[MainFile] = R"cpp(
+#include "includeguarded.h"
+#include "nonguarded.h"
+#include "pp_depend.h"
+#include "pragmaguarded.h"
+)cpp";
+  FS.Files["pragmaguarded.h"] = R"cpp(
+#pragma once
+)cpp";
+  FS.Files["includeguarded.h"] = R"cpp(
+#ifndef INCLUDE_GUARDED_H
+#define INCLUDE_GUARDED_H
+void foo();
+#endif // INCLUDE

[PATCH] D114370: [clangd] IncludeCleaner: Attribute symbols from non self-contained headers to their parents

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 389947.
kbobyrev added a comment.

Leave the feature changes (IncludeCleaner bits) out of this patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114370

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp

Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -82,8 +82,7 @@
 return {};
   }
   IncludeStructure Includes;
-  Clang->getPreprocessor().addPPCallbacks(
-  Includes.collect(Clang->getSourceManager()));
+  Clang->getPreprocessor().addPPCallbacks(Includes.collect(*Clang));
   if (llvm::Error Err = Action.Execute()) {
 ADD_FAILURE() << "failed to execute action: " << std::move(Err);
 return {};
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -80,8 +80,7 @@
 EXPECT_TRUE(
 Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
 IncludeStructure Includes;
-Clang->getPreprocessor().addPPCallbacks(
-Includes.collect(Clang->getSourceManager()));
+Clang->getPreprocessor().addPPCallbacks(Includes.collect(*Clang));
 EXPECT_FALSE(Action.Execute());
 Action.EndSourceFile();
 return Includes;
@@ -363,6 +362,39 @@
   EXPECT_THAT(collectIncludes().MainFileIncludes,
   Contains(AllOf(IncludeLine(2), Written("";
 }
+
+TEST_F(HeadersTest, SelfContainedHeaders) {
+  // Including through non-builtin file has no effects.
+  FS.Files[MainFile] = R"cpp(
+#include "includeguarded.h"
+#include "nonguarded.h"
+#include "pp_depend.h"
+#include "pragmaguarded.h"
+)cpp";
+  FS.Files["pragmaguarded.h"] = R"cpp(
+#pragma once
+)cpp";
+  FS.Files["includeguarded.h"] = R"cpp(
+#ifndef INCLUDE_GUARDED_H
+#define INCLUDE_GUARDED_H
+void foo();
+#endif // INCLUDE_GUARDED_H
+)cpp";
+  FS.Files["nonguarded.h"] = R"cpp(
+)cpp";
+  FS.Files["pp_depend.h"] = R"cpp(
+  #ifndef REQUIRED_PP_DIRECTIVE
+  # error You have to have PP directive set to include this one!
+  #endif
+)cpp";
+
+  auto Includes = collectIncludes();
+  EXPECT_TRUE(Includes.isSelfContained(getID("pragmaguarded.h", Includes)));
+  EXPECT_TRUE(Includes.isSelfContained(getID("includeguarded.h", Includes)));
+  EXPECT_FALSE(Includes.isSelfContained(getID("nonguarded.h", Includes)));
+  EXPECT_FALSE(Includes.isSelfContained(getID("pp_depend.h", Includes)));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -266,7 +266,8 @@
 return toURI(Canonical);
   }
 }
-if (!isSelfContainedHeader(FID, FE)) {
+if (!isSelfContainedHeader(FE, FID, PP->getSourceManager(),
+   PP->getHeaderSearchInfo())) {
   // A .inc or .def file is often included into a real header to define
   // symbols (e.g. LLVM tablegen files).
   if (Filename.endswith(".inc") || Filename.endswith(".def"))
@@ -278,54 +279,6 @@
 // Standard case: just insert the file itself.
 return toURI(FE);
   }
-
-  bool isSelfContainedHeader(FileID FID, const FileEntry *FE) {
-// FIXME: Should files that have been #import'd be considered
-// self-contained? That's really a property of the includer,
-// not of the file.
-if (!PP->getHeaderSearchInfo().isFileMultipleIncludeGuarded(FE) &&
-!PP->getHeaderSearchInfo().hasFileBeenImported(FE))
-  return false;
-// This pattern indicates that a header can't be used without
-// particular preprocessor state, usually set up by another header.
-if (isDontIncludeMeHeader(SM.getBufferData(FID)))
-  return false;
-return true;
-  }
-
-  // Is Line an #if or #ifdef directive?
-  static bool isIf(llvm::StringRef Line) {
-Line = Line.ltrim();
-if (!Line.consume_front("#"))
-  return false;
-Line = Line.ltrim();
-return Line.startswith("if");
-  }
-
-  // Is Line an #error directive mentioning includes?
-  static bool isErrorAboutInclude(llvm::StringRef Line) {
-Line = Line.ltrim();
-if (!Line.consume_front("#"))
-

[PATCH] D114370: [clangd] Record information about non self-contained headers in IncludeStructure

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 389950.
kbobyrev added a comment.

[clangd] IncludeCleaner: Attribute symbols from non self-contained headers to 
their parents

When a symbol comes from the non self-contained header, we recursively uplift
the file we consider used to the first includer that has a header guard. We
need to do this while we still have FileIDs because every time a non
self-contained header is included, it gets a new FileID but is later
deduplicated by HeaderID and it's not possible to understand where it was
included from.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114370

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp

Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -82,8 +82,7 @@
 return {};
   }
   IncludeStructure Includes;
-  Clang->getPreprocessor().addPPCallbacks(
-  Includes.collect(Clang->getSourceManager()));
+  Clang->getPreprocessor().addPPCallbacks(Includes.collect(*Clang));
   if (llvm::Error Err = Action.Execute()) {
 ADD_FAILURE() << "failed to execute action: " << std::move(Err);
 return {};
Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -281,7 +281,8 @@
   auto &SM = AST.getSourceManager();
   auto &Includes = AST.getIncludeStructure();
 
-  auto ReferencedFiles = findReferencedFiles(findReferencedLocations(AST), SM);
+  auto ReferencedFiles =
+  findReferencedFiles(findReferencedLocations(AST), Includes, SM);
   llvm::StringSet<> ReferencedFileNames;
   for (FileID FID : ReferencedFiles)
 ReferencedFileNames.insert(
@@ -303,6 +304,52 @@
   EXPECT_THAT(getUnused(AST, ReferencedHeaders), ::testing::IsEmpty());
 }
 
+TEST(IncludeCleaner, NonSelfContainedHeaders) {
+  TestTU TU;
+  TU.Code = R"cpp(
+#include "bar.h"
+#include "foo.h"
+
+int LocalFoo = foo::Variable,
+LocalBar = bar::Variable;
+)cpp";
+  TU.AdditionalFiles["bar.h"] = R"cpp(
+#pragma once
+namespace bar {
+#include "indirection.h"
+}
+)cpp";
+  TU.AdditionalFiles["foo.h"] = R"cpp(
+#ifndef FOO_H
+#define FOO_H
+namespace foo {
+#include "unguarded.h"
+}
+#endif // FOO_H
+)cpp";
+  TU.AdditionalFiles["indirection.h"] = R"cpp(
+#include "unguarded.h"
+)cpp";
+  TU.AdditionalFiles["unguarded.h"] = R"cpp(
+constexpr int Variable = 42;
+)cpp";
+
+  ParsedAST AST = TU.build();
+
+  auto ReferencedFiles =
+  findReferencedFiles(findReferencedLocations(AST),
+  AST.getIncludeStructure(), AST.getSourceManager());
+  llvm::StringSet<> ReferencedFileNames;
+  auto &SM = AST.getSourceManager();
+  for (FileID FID : ReferencedFiles)
+ReferencedFileNames.insert(
+SM.getPresumedLoc(SM.getLocForStartOfFile(FID)).getFilename());
+  // Note that we have uplifted the referenced files from non self-contained
+  // headers to header-guarded ones.
+  EXPECT_THAT(ReferencedFileNames.keys(),
+  UnorderedElementsAre(testPath("bar.h"), testPath("foo.h")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -80,8 +80,7 @@
 EXPECT_TRUE(
 Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
 IncludeStructure Includes;
-Clang->getPreprocessor().addPPCallbacks(
-Includes.collect(Clang->getSourceManager()));
+Clang->getPreprocessor().addPPCallbacks(Includes.collect(*Clang));
 EXPECT_FALSE(Action.Execute());
 Action.EndSourceFile();
 return Includes;
@@ -363,6 +362,39 @@
   EXPECT_THAT(collectIncludes().MainFileIncludes,
   Contains(AllOf(IncludeLine(2), Written("";
 }
+
+TEST_F(HeadersTest, SelfContainedHeaders) {
+  // Including through non-builtin file has no effects.
+  FS.Files[MainFile] = R"cpp(
+#include "

[PATCH] D114579: [clang-tidy] Exempt _MSVC_EXECUTION_CHARACTER_SET from cppcoreguidelines-macro-usage

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:40
 const MacroDirective *MD) override {
 if (SM.isWrittenInBuiltinFile(MD->getLocation()) ||
 MD->getMacroInfo()->isUsedForHeaderGuard() ||

Can that macro be defined elsewhere so this check fires instead of having to 
special-case the macro here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114579

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

[clang] d026f2f - [clang] Fix crash on broken parameter declarators

[Kadir Cetinkaya via cfe-commits]

Author: Kadir Cetinkaya
Date: 2021-11-26T10:56:54+01:00
New Revision: d026f2f7c688b326eae429286a06bf4080c8b527

URL: 
https://github.com/llvm/llvm-project/commit/d026f2f7c688b326eae429286a06bf4080c8b527
DIFF: 
https://github.com/llvm/llvm-project/commit/d026f2f7c688b326eae429286a06bf4080c8b527.diff

LOG: [clang] Fix crash on broken parameter declarators

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

Added: 


Modified: 
clang/lib/Parse/ParseDecl.cpp
clang/test/Parser/cxx-keyword-identifiers.cpp

Removed: 




diff  --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index a0871062395e..1bdeccc4cbf5 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -6978,13 +6978,13 @@ void Parser::ParseParameterDeclarationClause(
   //
   // We care about case 1) where the declarator type should be known, and
   // the identifier should be null.
-  if (!ParmDeclarator.isInvalidType() && !ParmDeclarator.hasName()) {
-if (Tok.getIdentifierInfo() &&
-Tok.getIdentifierInfo()->isKeyword(getLangOpts())) {
-  Diag(Tok, diag::err_keyword_as_parameter) << PP.getSpelling(Tok);
-  // Consume the keyword.
-  ConsumeToken();
-}
+  if (!ParmDeclarator.isInvalidType() && !ParmDeclarator.hasName() &&
+  Tok.isNot(tok::raw_identifier) && !Tok.isAnnotation() &&
+  Tok.getIdentifierInfo() &&
+  Tok.getIdentifierInfo()->isKeyword(getLangOpts())) {
+Diag(Tok, diag::err_keyword_as_parameter) << PP.getSpelling(Tok);
+// Consume the keyword.
+ConsumeToken();
   }
   // Inform the actions module about the parameter declarator, so it gets
   // added to the current scope.

diff  --git a/clang/test/Parser/cxx-keyword-identifiers.cpp 
b/clang/test/Parser/cxx-keyword-identifiers.cpp
index 4a60ce9233f5..2c6f18ec83e0 100644
--- a/clang/test/Parser/cxx-keyword-identifiers.cpp
+++ b/clang/test/Parser/cxx-keyword-identifiers.cpp
@@ -25,3 +25,7 @@ void test() {
 int case; // expected-error {{expected member name or ';'}}
   };
 }
+struct Foo {
+  void bar(*decltype(1) aux); // expected-error {{C++ requires a type 
specifier for all declarations}}. \
+ // expected-error {{expected ')'}} 
expected-note {{to match this '('}}
+};



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

[PATCH] D114609: [clang] Fix crash on broken parameter declarators

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
kadircet marked 2 inline comments as done.
Closed by commit rGd026f2f7c688: [clang] Fix crash on broken parameter 
declarators (authored by kadircet).

Changed prior to commit:
  https://reviews.llvm.org/D114609?vs=389851&id=389951#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114609

Files:
  clang/lib/Parse/ParseDecl.cpp
  clang/test/Parser/cxx-keyword-identifiers.cpp


Index: clang/test/Parser/cxx-keyword-identifiers.cpp
===
--- clang/test/Parser/cxx-keyword-identifiers.cpp
+++ clang/test/Parser/cxx-keyword-identifiers.cpp
@@ -25,3 +25,7 @@
 int case; // expected-error {{expected member name or ';'}}
   };
 }
+struct Foo {
+  void bar(*decltype(1) aux); // expected-error {{C++ requires a type 
specifier for all declarations}}. \
+ // expected-error {{expected ')'}} 
expected-note {{to match this '('}}
+};
Index: clang/lib/Parse/ParseDecl.cpp
===
--- clang/lib/Parse/ParseDecl.cpp
+++ clang/lib/Parse/ParseDecl.cpp
@@ -6978,13 +6978,13 @@
   //
   // We care about case 1) where the declarator type should be known, and
   // the identifier should be null.
-  if (!ParmDeclarator.isInvalidType() && !ParmDeclarator.hasName()) {
-if (Tok.getIdentifierInfo() &&
-Tok.getIdentifierInfo()->isKeyword(getLangOpts())) {
-  Diag(Tok, diag::err_keyword_as_parameter) << PP.getSpelling(Tok);
-  // Consume the keyword.
-  ConsumeToken();
-}
+  if (!ParmDeclarator.isInvalidType() && !ParmDeclarator.hasName() &&
+  Tok.isNot(tok::raw_identifier) && !Tok.isAnnotation() &&
+  Tok.getIdentifierInfo() &&
+  Tok.getIdentifierInfo()->isKeyword(getLangOpts())) {
+Diag(Tok, diag::err_keyword_as_parameter) << PP.getSpelling(Tok);
+// Consume the keyword.
+ConsumeToken();
   }
   // Inform the actions module about the parameter declarator, so it gets
   // added to the current scope.


Index: clang/test/Parser/cxx-keyword-identifiers.cpp
===
--- clang/test/Parser/cxx-keyword-identifiers.cpp
+++ clang/test/Parser/cxx-keyword-identifiers.cpp
@@ -25,3 +25,7 @@
 int case; // expected-error {{expected member name or ';'}}
   };
 }
+struct Foo {
+  void bar(*decltype(1) aux); // expected-error {{C++ requires a type specifier for all declarations}}. \
+ // expected-error {{expected ')'}} expected-note {{to match this '('}}
+};
Index: clang/lib/Parse/ParseDecl.cpp
===
--- clang/lib/Parse/ParseDecl.cpp
+++ clang/lib/Parse/ParseDecl.cpp
@@ -6978,13 +6978,13 @@
   //
   // We care about case 1) where the declarator type should be known, and
   // the identifier should be null.
-  if (!ParmDeclarator.isInvalidType() && !ParmDeclarator.hasName()) {
-if (Tok.getIdentifierInfo() &&
-Tok.getIdentifierInfo()->isKeyword(getLangOpts())) {
-  Diag(Tok, diag::err_keyword_as_parameter) << PP.getSpelling(Tok);
-  // Consume the keyword.
-  ConsumeToken();
-}
+  if (!ParmDeclarator.isInvalidType() && !ParmDeclarator.hasName() &&
+  Tok.isNot(tok::raw_identifier) && !Tok.isAnnotation() &&
+  Tok.getIdentifierInfo() &&
+  Tok.getIdentifierInfo()->isKeyword(getLangOpts())) {
+Diag(Tok, diag::err_keyword_as_parameter) << PP.getSpelling(Tok);
+// Consume the keyword.
+ConsumeToken();
   }
   // Inform the actions module about the parameter declarator, so it gets
   // added to the current scope.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114370: [clangd] Record information about non self-contained headers in IncludeStructure

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 389952.
kbobyrev added a comment.

Change base to D114370 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114370

Files:
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -281,7 +281,8 @@
   auto &SM = AST.getSourceManager();
   auto &Includes = AST.getIncludeStructure();
 
-  auto ReferencedFiles = findReferencedFiles(findReferencedLocations(AST), SM);
+  auto ReferencedFiles =
+  findReferencedFiles(findReferencedLocations(AST), Includes, SM);
   llvm::StringSet<> ReferencedFileNames;
   for (FileID FID : ReferencedFiles)
 ReferencedFileNames.insert(
@@ -303,6 +304,52 @@
   EXPECT_THAT(getUnused(AST, ReferencedHeaders), ::testing::IsEmpty());
 }
 
+TEST(IncludeCleaner, NonSelfContainedHeaders) {
+  TestTU TU;
+  TU.Code = R"cpp(
+#include "bar.h"
+#include "foo.h"
+
+int LocalFoo = foo::Variable,
+LocalBar = bar::Variable;
+)cpp";
+  TU.AdditionalFiles["bar.h"] = R"cpp(
+#pragma once
+namespace bar {
+#include "indirection.h"
+}
+)cpp";
+  TU.AdditionalFiles["foo.h"] = R"cpp(
+#ifndef FOO_H
+#define FOO_H
+namespace foo {
+#include "unguarded.h"
+}
+#endif // FOO_H
+)cpp";
+  TU.AdditionalFiles["indirection.h"] = R"cpp(
+#include "unguarded.h"
+)cpp";
+  TU.AdditionalFiles["unguarded.h"] = R"cpp(
+constexpr int Variable = 42;
+)cpp";
+
+  ParsedAST AST = TU.build();
+
+  auto ReferencedFiles =
+  findReferencedFiles(findReferencedLocations(AST),
+  AST.getIncludeStructure(), AST.getSourceManager());
+  llvm::StringSet<> ReferencedFileNames;
+  auto &SM = AST.getSourceManager();
+  for (FileID FID : ReferencedFiles)
+ReferencedFileNames.insert(
+SM.getPresumedLoc(SM.getLocForStartOfFile(FID)).getFilename());
+  // Note that we have uplifted the referenced files from non self-contained
+  // headers to header-guarded ones.
+  EXPECT_THAT(ReferencedFileNames.keys(),
+  UnorderedElementsAre(testPath("bar.h"), testPath("foo.h")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/IncludeCleaner.h
===
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -52,6 +52,7 @@
 /// The output only includes things SourceManager sees as files (not macro IDs).
 /// This can include ,  etc that are not true files.
 llvm::DenseSet findReferencedFiles(const ReferencedLocations &Locs,
+   const IncludeStructure &Includes,
const SourceManager &SM);
 
 /// Maps FileIDs to the internal IncludeStructure representation (HeaderIDs).
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -234,20 +234,35 @@
 
 llvm::DenseSet
 findReferencedFiles(const llvm::DenseSet &Locs,
-const SourceManager &SM) {
+const IncludeStructure &Includes, const SourceManager &SM) {
   std::vector Sorted{Locs.begin(), Locs.end()};
   llvm::sort(Sorted); // Group by FileID.
-  ReferencedFiles Result(SM);
+  ReferencedFiles Files(SM);
   for (auto It = Sorted.begin(); It < Sorted.end();) {
 FileID FID = SM.getFileID(*It);
-Result.add(FID, *It);
+Files.add(FID, *It);
 // Cheaply skip over all the other locations from the same FileID.
 // This avoids lots of redundant Loc->File lookups for the same file.
 do
   ++It;
 while (It != Sorted.end() && SM.isInFileID(*It, FID));
   }
-  return std::move(Result.Files);
+  // If a header is not self-contained, we consider its symbols a logical part
+  // of the including file. Therefore, mark the parents of all used
+  // non-self-contained FileIDs as used. Perform this on FileIDs rather than
+  // HeaderIDs, as each inclusion of a non-self-contained file is distinct.
+  llvm::DenseSet Result;
+  for (FileID ID : Files.Files) {
+// Unroll the chain of non self-contained headers to get to the first one
+// with the header guard.
+for (const FileEntry *FE = SM.getFileEntryForID(ID);
+ ID != SM.getMainFileID() && FE &&
+ !Includes.isSelfContained(*Includes.getID(FE));
+ ID = SM.getFileID(SM.getIncludeLoc(ID)), FE = SM.getFileEntryForID(ID))
+  ;
+Result

[PATCH] D112881: [clang-tidy] Allow disabling integer to floating-point narrowing conversions for cppcoreguidelines-narrowing-conversions

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-narrowingintegertofloatingpoint-option.cpp:12
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:14: warning: narrowing conversion 
from 'unsigned long long' to 'double' [cppcoreguidelines-narrowing-conversions]
+  // DISABLED: No warning for integer to floating-point narrowing conversions 
when WarnOnIntegerToFloatingPointNarrowingConversion = false.
+}

Build is failing because you don't have a CHECK-MESSAGES-DISABLED line anywhere 
in the file.
You could change `// DISABLED:` to `// CHECK-MESSAGES-DISABLED-NOT:` and check 
for the absence of the check warning.


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

https://reviews.llvm.org/D112881

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

[PATCH] D114370: [clangd] Record information about non self-contained headers in IncludeStructure

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 389953.
kbobyrev added a comment.

Get the changes back to this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114370

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp

Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -82,8 +82,7 @@
 return {};
   }
   IncludeStructure Includes;
-  Clang->getPreprocessor().addPPCallbacks(
-  Includes.collect(Clang->getSourceManager()));
+  Clang->getPreprocessor().addPPCallbacks(Includes.collect(*Clang));
   if (llvm::Error Err = Action.Execute()) {
 ADD_FAILURE() << "failed to execute action: " << std::move(Err);
 return {};
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -80,8 +80,7 @@
 EXPECT_TRUE(
 Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
 IncludeStructure Includes;
-Clang->getPreprocessor().addPPCallbacks(
-Includes.collect(Clang->getSourceManager()));
+Clang->getPreprocessor().addPPCallbacks(Includes.collect(*Clang));
 EXPECT_FALSE(Action.Execute());
 Action.EndSourceFile();
 return Includes;
@@ -363,6 +362,39 @@
   EXPECT_THAT(collectIncludes().MainFileIncludes,
   Contains(AllOf(IncludeLine(2), Written("";
 }
+
+TEST_F(HeadersTest, SelfContainedHeaders) {
+  // Including through non-builtin file has no effects.
+  FS.Files[MainFile] = R"cpp(
+#include "includeguarded.h"
+#include "nonguarded.h"
+#include "pp_depend.h"
+#include "pragmaguarded.h"
+)cpp";
+  FS.Files["pragmaguarded.h"] = R"cpp(
+#pragma once
+)cpp";
+  FS.Files["includeguarded.h"] = R"cpp(
+#ifndef INCLUDE_GUARDED_H
+#define INCLUDE_GUARDED_H
+void foo();
+#endif // INCLUDE_GUARDED_H
+)cpp";
+  FS.Files["nonguarded.h"] = R"cpp(
+)cpp";
+  FS.Files["pp_depend.h"] = R"cpp(
+  #ifndef REQUIRED_PP_DIRECTIVE
+  # error You have to have PP directive set to include this one!
+  #endif
+)cpp";
+
+  auto Includes = collectIncludes();
+  EXPECT_TRUE(Includes.isSelfContained(getID("pragmaguarded.h", Includes)));
+  EXPECT_TRUE(Includes.isSelfContained(getID("includeguarded.h", Includes)));
+  EXPECT_FALSE(Includes.isSelfContained(getID("nonguarded.h", Includes)));
+  EXPECT_FALSE(Includes.isSelfContained(getID("pp_depend.h", Includes)));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -266,7 +266,8 @@
 return toURI(Canonical);
   }
 }
-if (!isSelfContainedHeader(FID, FE)) {
+if (!isSelfContainedHeader(FE, FID, PP->getSourceManager(),
+   PP->getHeaderSearchInfo())) {
   // A .inc or .def file is often included into a real header to define
   // symbols (e.g. LLVM tablegen files).
   if (Filename.endswith(".inc") || Filename.endswith(".def"))
@@ -278,54 +279,6 @@
 // Standard case: just insert the file itself.
 return toURI(FE);
   }
-
-  bool isSelfContainedHeader(FileID FID, const FileEntry *FE) {
-// FIXME: Should files that have been #import'd be considered
-// self-contained? That's really a property of the includer,
-// not of the file.
-if (!PP->getHeaderSearchInfo().isFileMultipleIncludeGuarded(FE) &&
-!PP->getHeaderSearchInfo().hasFileBeenImported(FE))
-  return false;
-// This pattern indicates that a header can't be used without
-// particular preprocessor state, usually set up by another header.
-if (isDontIncludeMeHeader(SM.getBufferData(FID)))
-  return false;
-return true;
-  }
-
-  // Is Line an #if or #ifdef directive?
-  static bool isIf(llvm::StringRef Line) {
-Line = Line.ltrim();
-if (!Line.consume_front("#"))
-  return false;
-Line = Line.ltrim();
-return Line.startswith("if");
-  }
-
-  // Is Line an #error directive mentioning includes?
-  static bool isErrorAboutInclude(llvm::StringRef Line) {
-Line = Line.ltrim();
-if (!Line.consume_front("#"))
-  return false;
-Line = Li

[PATCH] D112881: [clang-tidy] Allow disabling integer to floating-point narrowing conversions for cppcoreguidelines-narrowing-conversions

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-narrowingintegertofloatingpoint-option.cpp:12
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:14: warning: narrowing conversion 
from 'unsigned long long' to 'double' [cppcoreguidelines-narrowing-conversions]
+  // DISABLED: No warning for integer to floating-point narrowing conversions 
when WarnOnIntegerToFloatingPointNarrowingConversion = false.
+}

salman-javed-nz wrote:
> Build is failing because you don't have a CHECK-MESSAGES-DISABLED line 
> anywhere in the file.
> You could change `// DISABLED:` to `// CHECK-MESSAGES-DISABLED-NOT:` and 
> check for the absence of the check warning.



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

https://reviews.llvm.org/D112881

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

[PATCH] D114619: [Analyzer][solver] Do not remove the simplified symbol from the eq class

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added a reviewer: steakhal.
Herald added subscribers: manas, ASDenysPetrov, gamesh411, dkrupp, donat.nagy, 
Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, 
xazax.hun.
Herald added a reviewer: Szelethus.
martong requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Currently, during symbol simplification we remove the original member symbol
from the equivalence class (`ClassMembers` trait). However, we keep the
reverse link (`ClassMap` trait), in order to be able the query the
related constraints even for the old member. This asymmetry can lead to
a problem when we merge equivalence classes:

  ClassA: [a, b]   // ClassMembers trait,
  a->a, b->a   // ClassMap trait, a is the representative symbol

Now lets delete `a`:

  ClassA: [b]
  a->a, b->a

Let's merge the trivial class `c` into ClassA:

  ClassA: [c, b]
  c->c, b->c, a->a

Now after the merge operation, `c` and `a` are actually in different
equivalence classes, which is inconsistent.

One solution to this problem is to simply avoid removing the original
member and this is what this patch does.

Other options I have considered:

1. Always merge the trivial class into the non-trivial class. This might work 
most of the time, however, will fail if we have to merge two non-trivial 
classes (in that case we no longer can track equivalences precisely).
2. In `removeMember`, update the reverse link as well. This would cease the 
inconsistency, but we'd loose precision since we could not query the 
constraints for the removed member.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114619

Files:
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/test/Analysis/expr-inspection-printState-eq-classes.c
  clang/test/Analysis/symbol-simplification-disequality-info.cpp
  clang/test/Analysis/symbol-simplification-fixpoint-one-iteration.cpp
  clang/test/Analysis/symbol-simplification-fixpoint-two-iterations.cpp

Index: clang/test/Analysis/symbol-simplification-fixpoint-two-iterations.cpp
===
--- clang/test/Analysis/symbol-simplification-fixpoint-two-iterations.cpp
+++ clang/test/Analysis/symbol-simplification-fixpoint-two-iterations.cpp
@@ -27,17 +27,20 @@
   if (b != 0)
 return;
   clang_analyzer_printState();
-  // CHECK:   "constraints": [
-  // CHECK-NEXT:{ "symbol": "(reg_$0) != (reg_$3)", "range": "{ [0, 0] }" },
-  // CHECK-NEXT:{ "symbol": "reg_$1", "range": "{ [0, 0] }" },
-  // CHECK-NEXT:{ "symbol": "reg_$2", "range": "{ [0, 0] }" }
-  // CHECK-NEXT:  ],
-  // CHECK-NEXT:  "equivalence_classes": [
-  // CHECK-NEXT:[ "(reg_$0) != (reg_$3)" ],
-  // CHECK-NEXT:[ "reg_$0", "reg_$3" ],
-  // CHECK-NEXT:[ "reg_$2" ]
-  // CHECK-NEXT:  ],
-  // CHECK-NEXT:  "disequality_info": null,
+  // CHECK:  "constraints": [
+  // CHECK-NEXT:   { "symbol": "(((reg_$0) + (reg_$1)) + (reg_$2)) != (reg_$3)", "range": "{ [0, 0] }" },
+  // CHECK-NEXT:   { "symbol": "((reg_$0) + (reg_$2)) != (reg_$3)", "range": "{ [0, 0] }" },
+  // CHECK-NEXT:   { "symbol": "(reg_$0) != (reg_$3)", "range": "{ [0, 0] }" },
+  // CHECK-NEXT:   { "symbol": "(reg_$2) + (reg_$1)", "range": "{ [0, 0] }" },
+  // CHECK-NEXT:   { "symbol": "reg_$1", "range": "{ [0, 0] }" },
+  // CHECK-NEXT:   { "symbol": "reg_$2", "range": "{ [0, 0] }" }
+  // CHECK-NEXT: ],
+  // CHECK-NEXT: "equivalence_classes": [
+  // CHECK-NEXT:   [ "(((reg_$0) + (reg_$1)) + (reg_$2)) != (reg_$3)", "((reg_$0) + (reg_$2)) != (reg_$3)", "(reg_$0) != (reg_$3)" ],
+  // CHECK-NEXT:   [ "((reg_$0) + (reg_$1)) + (reg_$2)", "(reg_$0) + (reg_$2)", "reg_$0", "reg_$3" ],
+  // CHECK-NEXT:   [ "(reg_$2) + (reg_$1)", "reg_$2" ]
+  // CHECK-NEXT: ],
+  // CHECK-NEXT: "disequality_info": null,
 
   // Keep the symbols and the constraints! alive.
   (void)(a * b * c * d);
Index: clang/test/Analysis/symbol-simplification-fixpoint-one-iteration.cpp
===
--- clang/test/Analysis/symbol-simplification-fixpoint-one-iteration.cpp
+++ clang/test/Analysis/symbol-simplification-fixpoint-one-iteration.cpp
@@ -24,14 +24,15 @@
   if (b != 0)
 return;
   clang_analyzer_printState();
-  // CHECK:"constraints": [
-  // CHECK-NEXT: { "symbol": "(reg_$0) != (reg_$2)", "range": "{ [0, 0] }" },
-  // CHECK-NEXT: { "symbol": "reg_$1", "range": "{ [0, 0] }" }
-  // CHECK-NEXT:   ],
-  // CHECK-NEXT:   "equivalence_classes": [
-  // CHECK-NEXT: [ "(reg_$0) != (reg_$2)" ],
-  // CHECK-NEXT: [ "reg_$0", "reg_$2" ]
-  // CHECK-NEXT:   ],
+  // CHECK:  "constraints": [
+  // CHECK-NEXT:   { "symbol": "((reg_$0) + (reg_$1)) != (reg_$2)", "range": "{ [0, 0] }" },
+  // CHECK-NEXT:   { "symbol": "(reg_$0) != (reg_$2)", "range": "{ [0, 0] }" },
+  // CHECK-NEXT:   { "symbol": "reg_$1", "range": "{ [0, 0] }" }
+  // CHECK-NE

[PATCH] D103317: [Analyzer][Core] Make SValBuilder to better simplify svals with 3 symbols in the tree

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> Please repeat the measurement for `openssl`. There must have been some 
> interference in the memory consumption. Aside from that the results look 
> great.

I did. Average memory usage is unaffected.

F20722167: svalbuilder_improvements_openssl.png 


F20722178: stats.html 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103317

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

[PATCH] D112881: [clang-tidy] Allow disabling integer to floating-point narrowing conversions for cppcoreguidelines-narrowing-conversions

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added a comment.

LGTM once tests are passing. Maybe wait a bit for a comment from @aaron.ballman 
.


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

https://reviews.llvm.org/D112881

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

[PATCH] D109856: [libunwind][ARM] Handle end of stack during unwind

[Daniel Kiss via Phabricator via cfe-commits]

danielkiss added a comment.

In D109856#3154763 , @mstorsjo wrote:

> Looks reasonable I think. Is this a deficiency in the EHABI implementation 
> only, i.e. this aspect works as it should in the regular dwarf implementation?

Yes, it works on dwarf already ( also the test pass on X86/Aarch64 ).


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

https://reviews.llvm.org/D109856

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

[PATCH] D114621: [clangd] Show parameters for construct.

[liu hui via Phabricator via cfe-commits]

lh123 created this revision.
lh123 added reviewers: sammccall, kadircet.
lh123 added a project: clang-tools-extra.
Herald added subscribers: usaxena95, arphaman.
lh123 requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.

Show parameters for construct.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114621

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -2683,6 +2683,32 @@
   },
   {
   [](HoverInfo &HI) {
+HI.Definition = "cls(int a, int b = 5)";
+HI.AccessSpecifier = "public";
+HI.Kind = index::SymbolKind::Constructor;
+HI.NamespaceScope = "";
+HI.LocalScope = "cls";
+HI.Name = "cls";
+HI.Parameters.emplace();
+HI.Parameters->emplace_back();
+HI.Parameters->back().Type = "int";
+HI.Parameters->back().Name = "a";
+HI.Parameters->emplace_back();
+HI.Parameters->back().Type = "int";
+HI.Parameters->back().Name = "b";
+HI.Parameters->back().Default = "5";
+  },
+  R"(constructor cls
+
+Parameters:
+- int a
+- int b = 5
+
+// In cls
+public: cls(int a, int b = 5))",
+  },
+  {
+  [](HoverInfo &HI) {
 HI.Kind = index::SymbolKind::Union;
 HI.AccessSpecifier = "private";
 HI.Name = "foo";
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -1058,20 +1058,21 @@
 // - `bool param1`
 // - `int param2 = 5`
 Output.addParagraph().appendText("→ ").appendCode(*ReturnType);
-if (Parameters && !Parameters->empty()) {
-  Output.addParagraph().appendText("Parameters: ");
-  markup::BulletList &L = Output.addBulletList();
-  for (const auto &Param : *Parameters) {
-std::string Buffer;
-llvm::raw_string_ostream OS(Buffer);
-OS << Param;
-L.addItem().addParagraph().appendCode(std::move(OS.str()));
-  }
-}
   } else if (Type) {
 Output.addParagraph().appendText("Type: ").appendCode(*Type);
   }
 
+  if (Parameters && !Parameters->empty()) {
+Output.addParagraph().appendText("Parameters: ");
+markup::BulletList &L = Output.addBulletList();
+for (const auto &Param : *Parameters) {
+  std::string Buffer;
+  llvm::raw_string_ostream OS(Buffer);
+  OS << Param;
+  L.addItem().addParagraph().appendCode(std::move(OS.str()));
+}
+  }
+
   if (Value) {
 markup::Paragraph &P = Output.addParagraph();
 P.appendText("Value = ");


Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -2683,6 +2683,32 @@
   },
   {
   [](HoverInfo &HI) {
+HI.Definition = "cls(int a, int b = 5)";
+HI.AccessSpecifier = "public";
+HI.Kind = index::SymbolKind::Constructor;
+HI.NamespaceScope = "";
+HI.LocalScope = "cls";
+HI.Name = "cls";
+HI.Parameters.emplace();
+HI.Parameters->emplace_back();
+HI.Parameters->back().Type = "int";
+HI.Parameters->back().Name = "a";
+HI.Parameters->emplace_back();
+HI.Parameters->back().Type = "int";
+HI.Parameters->back().Name = "b";
+HI.Parameters->back().Default = "5";
+  },
+  R"(constructor cls
+
+Parameters:
+- int a
+- int b = 5
+
+// In cls
+public: cls(int a, int b = 5))",
+  },
+  {
+  [](HoverInfo &HI) {
 HI.Kind = index::SymbolKind::Union;
 HI.AccessSpecifier = "private";
 HI.Name = "foo";
Index: clang-tools-extra/clangd/Hover.cpp
===
--- clang-tools-extra/clangd/Hover.cpp
+++ clang-tools-extra/clangd/Hover.cpp
@@ -1058,20 +1058,21 @@
 // - `bool param1`
 // - `int param2 = 5`
 Output.addParagraph().appendText("→ ").appendCode(*ReturnType);
-if (Parameters && !Parameters->empty()) {
-  Output.addParagraph().appendText("Parameters: ");
-  markup::BulletList &L = Output.addBulletList();
-  for (const auto &Param : *Parameters) {
-std::string Buffer;
-llvm::raw_string_ostream OS(Buffer);
-OS << Param;
-L.addItem().addParagraph().appendCode(std::move(OS.str()));
-  }
-}
   } else if (Type) {
 Output.addParagraph().appendT

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Balázs Benics via Phabricator via cfe-commits]

steakhal created this revision.
steakhal added reviewers: aaron.ballman, alexfh, hokein, courbet, NoQ, 
xazax.hun, martong, whisperity, davrec.
Herald added subscribers: carlosgalvezp, manas, ASDenysPetrov, dkrupp, 
donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, 
baloghadamsoftware, dylanmckay.
Herald added a reviewer: Szelethus.
steakhal requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Consider this example:

  #include 
  using std::is_same;
  bool top() {
return is_same::value || is_same::value;
  }

We can see that the `value` static members are different, but in fact,
they refer to the same VarDecl entity. It is because both `is_same`
class instances inherit from the common `false_type` class, which
actually owns the `value` member.

The `alpha.core.IdenticalExpr` Static Analyzer checker actually warns
for this, since it only checked if the referred `Decls` are the same.
Interestingly, the clang-tidy `misc-redundant-expression` also reports
this is for the exact same reason, so I'm fixing both of them at the same
time.

This patch fixes this by checking if the `Decls` are the same and if they
are it will try to acquire the nested namespace specifier as a type and
compare them as well. If they are inequal, that means that the
//spelling// of the nested namespace specifiers actually differed, this
it's likely to be a false-positive.

I'd like to note that the checker/check was actually right about that
the expressions were semantically equal in that given context, however,
we still don't want these reports in general.

We could introduce checker options to finetune this behavior if needed.

PS: According to the code coverage of the test, all touched parts are
completely covered.

Thanks David Rector (@davrec) for the idea on the cfe-dev forum:
https://lists.llvm.org/pipermail/cfe-dev/2021-November/069389.html


https://reviews.llvm.org/D114622

Files:
  clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
  clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
  clang/test/Analysis/identical-expressions.cpp

Index: clang/test/Analysis/identical-expressions.cpp
===
--- clang/test/Analysis/identical-expressions.cpp
+++ clang/test/Analysis/identical-expressions.cpp
@@ -1562,3 +1562,50 @@
   ;
   }
 }
+
+template  struct boolean_value {
+  static constexpr bool value = V;
+};
+template  struct my_trait : boolean_value {};
+
+bool respect_nested_name_specifiers(bool sink) {
+  sink |= my_trait::value || my_trait::value; // no-warning
+
+  sink |= my_trait::value || my_trait::value;
+  // expected-warning@-1 {{identical expressions on both sides of logical operator}}
+
+  using my_char = char;
+  sink |= my_trait::value || my_trait::value;
+  // expected-warning@-1 {{identical expressions on both sides of logical operator}}
+
+  return sink;
+}
+
+static void static_function() {}
+namespace my {
+int fn(int = 1);
+}
+void respect_namespaces(bool coin) {
+  namespace other = my;
+  using namespace other;
+
+  coin ? my::fn(1) : my::fn(2);// no-warning
+  coin ? my::fn(1) : other::fn(2); // no-warning
+
+  // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+  coin ? my::fn(1) : my::fn(1);
+  coin ? my::fn(1) : other::fn(1);
+  coin ? my::fn(1) : fn(1);
+
+  coin ? my::fn(1) : fn(); // FIXME: We should have a warning for this: the default parameter is also 1.
+
+  my::fn(1) & my::fn(1);// no-warning
+  my::fn(1) & other::fn(1); // no-warning
+
+  // expected-warning@+2 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+2 {{identical expressions on both sides of ':' in conditional expression}}
+  coin ? ::static_function() : ::static_function();
+  coin ? ::static_function() : static_function();
+}
Index: clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
@@ -295,6 +295,31 @@
   return true;
 }
 
+static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
+   const NestedNameSpecifier *Right) {
+  if (const auto *L = Left->getAsType())
+if (const auto *R = Right->getAsType())
+  return L->getCanonicalTypeUnqualified() ==
+ R->getCanonicalTypeUnqualified();
+
+  // FIXME: We should probably check the other kinds of nested name specifiers.
+  return true;
+}
+
+static bool areEquivalentDeclRefs(const DeclRefE

[PATCH] D114522: [clangd] Add desugared type to hover

[liu hui via Phabricator via cfe-commits]

lh123 updated this revision to Diff 389968.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114522

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Hover.h
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang/include/clang/AST/ASTDiagnostic.h
  clang/lib/AST/ASTDiagnostic.cpp

Index: clang/lib/AST/ASTDiagnostic.cpp
===
--- clang/lib/AST/ASTDiagnostic.cpp
+++ clang/lib/AST/ASTDiagnostic.cpp
@@ -26,7 +26,7 @@
 
 // Returns a desugared version of the QualType, and marks ShouldAKA as true
 // whenever we remove significant sugar from the type.
-static QualType Desugar(ASTContext &Context, QualType QT, bool &ShouldAKA) {
+QualType clang::Desugar(ASTContext &Context, QualType QT, bool &ShouldAKA) {
   QualifierCollector QC;
 
   while (true) {
Index: clang/include/clang/AST/ASTDiagnostic.h
===
--- clang/include/clang/AST/ASTDiagnostic.h
+++ clang/include/clang/AST/ASTDiagnostic.h
@@ -9,6 +9,7 @@
 #ifndef LLVM_CLANG_AST_ASTDIAGNOSTIC_H
 #define LLVM_CLANG_AST_ASTDIAGNOSTIC_H
 
+#include "clang/AST/Type.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticAST.h"
 
@@ -31,6 +32,10 @@
   SmallVectorImpl &Output,
   void *Cookie,
   ArrayRef QualTypeVals);
+
+  /// Returns a desugared version of the QualType, and marks ShouldAKA as true
+  /// whenever we remove significant sugar from the type.
+  QualType Desugar(ASTContext &Context, QualType QT, bool &ShouldAKA);
 }  // end namespace clang
 
 #endif
Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -45,8 +45,8 @@
  HI.Kind = index::SymbolKind::Function;
  HI.Documentation = "Best foo ever.";
  HI.Definition = "void foo()";
- HI.ReturnType = "void";
- HI.Type = "void ()";
+ HI.ReturnType = {"void", llvm::None};
+ HI.Type = {"void ()", llvm::None};
  HI.Parameters.emplace();
}},
   // Inside namespace
@@ -62,8 +62,8 @@
  HI.Kind = index::SymbolKind::Function;
  HI.Documentation = "Best foo ever.";
  HI.Definition = "void foo()";
- HI.ReturnType = "void";
- HI.Type = "void ()";
+ HI.ReturnType = {"void", llvm::None};
+ HI.Type = {"void ()", llvm::None};
  HI.Parameters.emplace();
}},
   // Field
@@ -81,7 +81,7 @@
  HI.Name = "bar";
  HI.Kind = index::SymbolKind::Field;
  HI.Definition = "char bar";
- HI.Type = "char";
+ HI.Type = {"char", llvm::None};
  HI.Offset = 0;
  HI.Size = 1;
  HI.Padding = 7;
@@ -100,7 +100,7 @@
  HI.Name = "bar";
  HI.Kind = index::SymbolKind::Field;
  HI.Definition = "char bar";
- HI.Type = "char";
+ HI.Type = {"char", llvm::None};
  HI.Size = 1;
  HI.Padding = 15;
  HI.AccessSpecifier = "public";
@@ -118,7 +118,7 @@
  HI.Name = "x";
  HI.Kind = index::SymbolKind::Field;
  HI.Definition = "int x : 1";
- HI.Type = "int";
+ HI.Type = {"int", llvm::None};
  HI.AccessSpecifier = "public";
}},
   // Local to class method.
@@ -137,7 +137,7 @@
  HI.Name = "bar";
  HI.Kind = index::SymbolKind::Variable;
  HI.Definition = "int bar";
- HI.Type = "int";
+ HI.Type = {"int", llvm::None};
}},
   // Anon namespace and local scope.
   {R"cpp(
@@ -153,7 +153,7 @@
  HI.Name = "bar";
  HI.Kind = index::SymbolKind::Field;
  HI.Definition = "char bar";
- HI.Type = "char";
+ HI.Type = {"char", llvm::None};
  HI.Offset = 0;
  HI.Size = 1;
  HI.AccessSpecifier = "public";
@@ -179,7 +179,7 @@
  HI.Name = "foo";
  HI.Kind = index::SymbolKind::Variable;
  HI.Definition = "Foo foo = Foo(5)";
- HI.Type = "Foo";
+ HI.Type = {"Foo", llvm::None};
}},
   // Implicit template instantiation
   {R"cpp(
@@ -211,12 +211,19 @@
   bool Q = false, class... Ts>
 class Foo {})cpp";
  HI.TemplateParameters = {
- {std::string("template  class"),
-  std::string("C"), llvm::None},
- {std::string("typename"), llvm::None, std::string("char")},
- {std::string("int"), llvm::None, std::string("0")},
- {std::string("bool"), std::string("Q"), std::string("false")},
- {std::string("class..."), std::string("Ts"), llvm::None},
+ {{{std::string("template  class"), llvm::None}},
+  std::string("C"),
+  llvm::None},
+   

[PATCH] D114623: [clangd] IncludeCleaner: Attribute symbols from non self-contained headers to their parents

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev created this revision.
kbobyrev added a reviewer: sammccall.
Herald added subscribers: usaxena95, kadircet, arphaman.
kbobyrev requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

When a symbol comes from the non self-contained header, we recursively uplift
the file we consider used to the first includer that has a header guard. We
need to do this while we still have FileIDs because every time a non
self-contained header is included, it gets a new FileID but is later
deduplicated by HeaderID and it's not possible to understand where it was
included from.

Based on D114370 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114623

Files:
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -281,7 +281,8 @@
   auto &SM = AST.getSourceManager();
   auto &Includes = AST.getIncludeStructure();
 
-  auto ReferencedFiles = findReferencedFiles(findReferencedLocations(AST), SM);
+  auto ReferencedFiles =
+  findReferencedFiles(findReferencedLocations(AST), Includes, SM);
   llvm::StringSet<> ReferencedFileNames;
   for (FileID FID : ReferencedFiles)
 ReferencedFileNames.insert(
@@ -303,6 +304,52 @@
   EXPECT_THAT(getUnused(AST, ReferencedHeaders), ::testing::IsEmpty());
 }
 
+TEST(IncludeCleaner, NonSelfContainedHeaders) {
+  TestTU TU;
+  TU.Code = R"cpp(
+#include "bar.h"
+#include "foo.h"
+
+int LocalFoo = foo::Variable,
+LocalBar = bar::Variable;
+)cpp";
+  TU.AdditionalFiles["bar.h"] = R"cpp(
+#pragma once
+namespace bar {
+#include "indirection.h"
+}
+)cpp";
+  TU.AdditionalFiles["foo.h"] = R"cpp(
+#ifndef FOO_H
+#define FOO_H
+namespace foo {
+#include "unguarded.h"
+}
+#endif // FOO_H
+)cpp";
+  TU.AdditionalFiles["indirection.h"] = R"cpp(
+#include "unguarded.h"
+)cpp";
+  TU.AdditionalFiles["unguarded.h"] = R"cpp(
+constexpr int Variable = 42;
+)cpp";
+
+  ParsedAST AST = TU.build();
+
+  auto ReferencedFiles =
+  findReferencedFiles(findReferencedLocations(AST),
+  AST.getIncludeStructure(), AST.getSourceManager());
+  llvm::StringSet<> ReferencedFileNames;
+  auto &SM = AST.getSourceManager();
+  for (FileID FID : ReferencedFiles)
+ReferencedFileNames.insert(
+SM.getPresumedLoc(SM.getLocForStartOfFile(FID)).getFilename());
+  // Note that we have uplifted the referenced files from non self-contained
+  // headers to header-guarded ones.
+  EXPECT_THAT(ReferencedFileNames.keys(),
+  UnorderedElementsAre(testPath("bar.h"), testPath("foo.h")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/IncludeCleaner.h
===
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -52,6 +52,7 @@
 /// The output only includes things SourceManager sees as files (not macro IDs).
 /// This can include ,  etc that are not true files.
 llvm::DenseSet findReferencedFiles(const ReferencedLocations &Locs,
+   const IncludeStructure &Includes,
const SourceManager &SM);
 
 /// Maps FileIDs to the internal IncludeStructure representation (HeaderIDs).
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -234,20 +234,35 @@
 
 llvm::DenseSet
 findReferencedFiles(const llvm::DenseSet &Locs,
-const SourceManager &SM) {
+const IncludeStructure &Includes, const SourceManager &SM) {
   std::vector Sorted{Locs.begin(), Locs.end()};
   llvm::sort(Sorted); // Group by FileID.
-  ReferencedFiles Result(SM);
+  ReferencedFiles Files(SM);
   for (auto It = Sorted.begin(); It < Sorted.end();) {
 FileID FID = SM.getFileID(*It);
-Result.add(FID, *It);
+Files.add(FID, *It);
 // Cheaply skip over all the other locations from the same FileID.
 // This avoids lots of redundant Loc->File lookups for the same file.
 do
   ++It;
 while (It != Sorted.end() && SM.isInFileID(*It, FID));
   }
-  return std::move(Result.Files);
+  // If a header is not self-contained, we consider its symbols a logical part
+  // of the including file. Therefore, mark the parents of all used
+  // non-self-contained FileIDs as used. Perform this on FileIDs rather th

[PATCH] D103317: [Analyzer][Core] Make SValBuilder to better simplify svals with 3 symbols in the tree

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D103317#3155191 , @martong wrote:

>> Please repeat the measurement for `openssl`. There must have been some 
>> interference in the memory consumption. Aside from that the results look 
>> great.
>
> I did. Average memory usage is unaffected.
>
> F20722167: svalbuilder_improvements_openssl.png 
> 
>
> F20722178: stats.html 

It's good enough for me, thanks.




Comment at: clang/test/Analysis/svalbuilder-simplify-compound-svals.cpp:75
+  if (b * b == 1)
+; // no-crash (assertion should not be violated)
+  }

I guess, this statement should not be reachable.
Please demonstrate it by using the `clang_analyzer_WarnIfReached()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103317

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

[clang] d8a3538 - [clang][deps] NFC: Remove else after early return

[Jan Svoboda via cfe-commits]

Author: Jan Svoboda
Date: 2021-11-26T12:18:37+01:00
New Revision: d8a35387881bf408c19deeba7b5e3648dbbf467c

URL: 
https://github.com/llvm/llvm-project/commit/d8a35387881bf408c19deeba7b5e3648dbbf467c
DIFF: 
https://github.com/llvm/llvm-project/commit/d8a35387881bf408c19deeba7b5e3648dbbf467c.diff

LOG: [clang][deps] NFC: Remove else after early return

Added: 


Modified: 
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

Removed: 




diff  --git 
a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp 
b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
index 40e8bd2b87760..5f3e6480eba48 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -191,8 +191,7 @@ 
DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
   //   files before building them, and then looks for them again. If we
   //   cache the stat failure, it won't see them the second time.
   return MaybeStatus.getError();
-else
-  CacheEntry = CachedFileSystemEntry(MaybeStatus.getError());
+CacheEntry = CachedFileSystemEntry(MaybeStatus.getError());
   } else if (MaybeStatus->isDirectory())
 CacheEntry = CachedFileSystemEntry::createDirectoryEntry(
 std::move(*MaybeStatus));



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

[clang] 12eafd9 - [clang][deps] NFC: Clean up wording (ignored vs minimized)

[Jan Svoboda via cfe-commits]

Author: Jan Svoboda
Date: 2021-11-26T12:18:37+01:00
New Revision: 12eafd944e0ffccae93402ddbe2855beb7a939ff

URL: 
https://github.com/llvm/llvm-project/commit/12eafd944e0ffccae93402ddbe2855beb7a939ff
DIFF: 
https://github.com/llvm/llvm-project/commit/12eafd944e0ffccae93402ddbe2855beb7a939ff.diff

LOG: [clang][deps] NFC: Clean up wording (ignored vs minimized)

The filesystem used during dependency scanning does two things: it caches file 
entries and minimizes source file contents. We use the term "ignored file" in a 
couple of places, but it's not clear what exactly that means. This commit 
clears up the semantics, explicitly spelling out this relates to minimization.

Added: 


Modified: 

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
clang/unittests/Tooling/DependencyScannerTest.cpp

Removed: 




diff  --git 
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h 
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
index c52da3305f7c1..b3a869af61078 100644
--- 
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ 
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -195,11 +195,14 @@ class DependencyScanningWorkerFilesystem : public 
llvm::vfs::ProxyFileSystem {
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override;
 
-  void clearIgnoredFiles() { IgnoredFiles.clear(); }
-  void ignoreFile(StringRef Filename);
+  /// Disable minimization of the given file.
+  void disableMinimization(StringRef Filename);
+  /// Enable minimization of all files.
+  void enableMinimizationOfAllFiles() { NotToBeMinimized.clear(); }
 
 private:
-  bool shouldIgnoreFile(StringRef Filename);
+  /// Check whether the file should be minimized.
+  bool shouldMinimize(StringRef Filename);
 
   llvm::ErrorOr
   getOrCreateFileSystemEntry(const StringRef Filename);
@@ -214,7 +217,7 @@ class DependencyScanningWorkerFilesystem : public 
llvm::vfs::ProxyFileSystem {
   /// currently active preprocessor.
   ExcludedPreprocessorDirectiveSkipMapping *PPSkipMappings;
   /// The set of files that should not be minimized.
-  llvm::StringSet<> IgnoredFiles;
+  llvm::StringSet<> NotToBeMinimized;
 };
 
 } // end namespace dependencies

diff  --git 
a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp 
b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
index 5f3e6480eba48..8973f2658ed69 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -129,7 +129,7 @@ DependencyScanningFilesystemSharedCache::get(StringRef Key, 
bool Minimized) {
 ///
 /// This is kinda hacky, it would be better if we knew what kind of file Clang
 /// was expecting instead.
-static bool shouldMinimize(StringRef Filename) {
+static bool shouldMinimizeBasedOnExtension(StringRef Filename) {
   StringRef Ext = llvm::sys::path::extension(Filename);
   if (Ext.empty())
 return true; // C++ standard library
@@ -147,26 +147,30 @@ static bool shouldCacheStatFailures(StringRef Filename) {
   StringRef Ext = llvm::sys::path::extension(Filename);
   if (Ext.empty())
 return false; // This may be the module cache directory.
-  return shouldMinimize(Filename); // Only cache stat failures on source files.
+  // Only cache stat failures on source files.
+  return shouldMinimizeBasedOnExtension(Filename);
 }
 
-void DependencyScanningWorkerFilesystem::ignoreFile(StringRef RawFilename) {
+void DependencyScanningWorkerFilesystem::disableMinimization(
+StringRef RawFilename) {
   llvm::SmallString<256> Filename;
   llvm::sys::path::native(RawFilename, Filename);
-  IgnoredFiles.insert(Filename);
+  NotToBeMinimized.insert(Filename);
 }
 
-bool DependencyScanningWorkerFilesystem::shouldIgnoreFile(
-StringRef RawFilename) {
+bool DependencyScanningWorkerFilesystem::shouldMinimize(StringRef RawFilename) 
{
+  if (!shouldMinimizeBasedOnExtension(RawFilename))
+return false;
+
   llvm::SmallString<256> Filename;
   llvm::sys::path::native(RawFilename, Filename);
-  return IgnoredFiles.contains(Filename);
+  return !NotToBeMinimized.contains(Filename);
 }
 
 llvm::ErrorOr
 DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
 const StringRef Filename) {
-  bool ShouldMinimize = !shouldIgnoreFile(Filename) && 
shouldMinimize(Filename);
+  bool ShouldMinimize = shouldMinimize(Filename);
 
   if (const auto *Entry = Cache.getCachedEntry(Filename, ShouldMinimize))
 return Entry;

diff  --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp 
b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
i

[PATCH] D114619: [Analyzer][solver] Do not remove the simplified symbol from the eq class

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I see. Simplification is always good. Let's measure and compare the runtime 
characteristics before moving forward.




Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2226
+  //
+  // Empirical measurements show that if we relax assumption G then the
+  // runtime does not grow noticeably. This is most probably because the

Emphasize what you mean by //relaxation//. You meant probably something like 
//not replacing the complex symbol, just adding the simplified version to the 
class//.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2227
+  // Empirical measurements show that if we relax assumption G then the
+  // runtime does not grow noticeably. This is most probably because the
+  // cost of removing the simplified member is much higher than the cost of

nor memory consumption



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2228-2229
+  // runtime does not grow noticeably. This is most probably because the
+  // cost of removing the simplified member is much higher than the cost of
+  // simplifying the symbol.
   State = reAssume(State, ClassConstraint, SimplifiedMemberVal);

Could you please elaborate on this in the review? I don't get the reasoning. I 
might miss something.



Comment at: clang/test/Analysis/symbol-simplification-disequality-info.cpp:15-26
+  // CHECK:  "disequality_info": [
+  // CHECK-NEXT:   {
+  // CHECK-NEXT: "class": [ "((reg_$0) + (reg_$1)) + 
(reg_$2)" ],
+  // CHECK-NEXT: "disequal_to": [
+  // CHECK-NEXT:   [ "reg_$3" ]]
+  // CHECK-NEXT:   },
+  // CHECK-NEXT:   {

Please try to omit unnecessary changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114619

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

[PATCH] D114370: [clangd] Record information about non self-contained headers in IncludeStructure

[Sam McCall via Phabricator via cfe-commits]

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

Thanks!




Comment at: clang-tools-extra/clangd/Headers.h:167
 
+  void recordNonSelfContained(HeaderID ID) { NonSelfContained.insert(ID); }
+

could consider `friend class RecordHeaders`, either is ugly, up to you


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114370

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

[PATCH] D109856: [libunwind][ARM] Handle end of stack during unwind

[Martin Storsjö via Phabricator via cfe-commits]

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

In D109856#3155235 , @danielkiss 
wrote:

> In D109856#3154763 , @mstorsjo 
> wrote:
>
>> Looks reasonable I think. Is this a deficiency in the EHABI implementation 
>> only, i.e. this aspect works as it should in the regular dwarf 
>> implementation?
>
> Yes, it works on dwarf already ( also the test pass on X86/Aarch64 ).

Ok, thanks for the clarification!


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

https://reviews.llvm.org/D109856

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

[PATCH] D114623: [clangd] IncludeCleaner: Attribute symbols from non self-contained headers to their parents

[Sam McCall via Phabricator via cfe-commits]

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

LG with a couple of readability things




Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:259
+for (const FileEntry *FE = SM.getFileEntryForID(ID);
+ ID != SM.getMainFileID() && FE &&
+ !Includes.isSelfContained(*Includes.getID(FE));

there's some subtle logic buried in here here:
 - the FE may be null, and if it is then we consider it the responsible header 
(rather than its include parent or possibly include child)
 - the HeaderID is never null

I'd prefer to see these expressed these more explicitly, and it's worth 
thinking if they're the right decisions or just provide the tersest loop :-)

Really I think pulling out a function: `headerResponsible(FileID, ...)` might 
make it possible to make this code clearer without cluttering the outer 
algorithm



Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:331
+  TU.AdditionalFiles["indirection.h"] = R"cpp(
+#include "unguarded.h"
+)cpp";

This testcase is trying to cover too many bases, I think. It's complex and also 
doesn't manage to test everything.

The point of having "unguarded.h" reachable via two paths yielding different 
symbols seems to be that one of them can be used and the other unused, and we 
can only tell the difference by operating on FileIDs.
But because both variables are referenced here, we can't tell the difference 
and would get the same result by operating on HeaderIDs.

I think it'd be best to split it:
 - `DistinctUnguardedInclusions` which is but with `indirection.h` removed, 
only one style of header guard, and bar::Variable unreferenced
 - `NonSelfContainedHeaders` which just has main -> foo -> indirection -> 
unguarded, with no namespace tricks. `Variable` is referenced.

If you want to test the multiple styles of include guards that's OK, but 
probably belongs rather on the test for IncludeStructure::isSelfContained


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114623

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

[PATCH] D114533: LLVM IR should allow bitcast between address spaces with the same size.

[krishna chaitanya sankisa via Phabricator via cfe-commits]

skc7 updated this revision to Diff 389983.
skc7 added a comment.

Updated diff with changes suggested by jrtc27


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

https://reviews.llvm.org/D114533

Files:
  clang/lib/CodeGen/CGAtomic.cpp
  llvm/docs/LangRef.rst
  llvm/include/llvm/Analysis/TargetFolder.h
  llvm/include/llvm/IR/ConstantFolder.h
  llvm/include/llvm/IR/Constants.h
  llvm/include/llvm/IR/IRBuilder.h
  llvm/include/llvm/IR/IRBuilderFolder.h
  llvm/include/llvm/IR/InstrTypes.h
  llvm/include/llvm/IR/Instructions.h
  llvm/include/llvm/IR/NoFolder.h
  llvm/lib/Analysis/ConstantFolding.cpp
  llvm/lib/Analysis/LoopUnrollAnalyzer.cpp
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/IR/AutoUpgrade.cpp
  llvm/lib/IR/Constants.cpp
  llvm/lib/IR/Instructions.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/lib/Transforms/Coroutines/Coroutines.cpp
  llvm/lib/Transforms/Utils/VNCoercion.cpp
  llvm/test/Transforms/GVN/gvn-eliminate-inttoptr-ptrtoint-for-load.ll
  llvm/test/Transforms/GVN/gvn-eliminate-inttoptr-ptrtoint-for-vector-load.ll
  
llvm/test/Transforms/GVN/gvn-eliminate-inttoptr-ptrtoint-for-vector-ptr-load.ll
  llvm/test/Verifier/bitcast-vector-pointer-as-neg.ll
  llvm/test/Verifier/bitcast-vector-pointer-different-addrspace-illegal.ll
  llvm/test/Verifier/bitcast-vector-pointer-neg.ll
  llvm/test/Verifier/bitcast-vector-pointer-pos.ll
  llvm/test/Verifier/bitcast-vector-pointer-same-addrspace.ll
  llvm/unittests/IR/InstructionsTest.cpp

Index: llvm/unittests/IR/InstructionsTest.cpp
===
--- llvm/unittests/IR/InstructionsTest.cpp
+++ llvm/unittests/IR/InstructionsTest.cpp
@@ -189,6 +189,10 @@
 TEST(InstructionsTest, CastInst) {
   LLVMContext C;
 
+  DataLayout DL("e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-"
+"p6:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-"
+"v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-ni:7");
+
   Type *Int8Ty = Type::getInt8Ty(C);
   Type *Int16Ty = Type::getInt16Ty(C);
   Type *Int32Ty = Type::getInt32Ty(C);
@@ -217,7 +221,11 @@
   Type *Int32PtrAS1Ty = PointerType::get(Int32Ty, 1);
   Type *Int64PtrAS1Ty = PointerType::get(Int64Ty, 1);
 
+  Type *Int32PtrAS2Ty = PointerType::get(Int32Ty, 2);
+  Type *Int32PtrAS3Ty = PointerType::get(Int32Ty, 3);
+
   Type *V2Int32PtrAS1Ty = FixedVectorType::get(Int32PtrAS1Ty, 2);
+  Type *V2Int32PtrAS2Ty = FixedVectorType::get(Int32PtrAS2Ty, 2);
   Type *V2Int64PtrAS1Ty = FixedVectorType::get(Int64PtrAS1Ty, 2);
   Type *V4Int32PtrAS1Ty = FixedVectorType::get(Int32PtrAS1Ty, 4);
   Type *VScaleV4Int32PtrAS1Ty = ScalableVectorType::get(Int32PtrAS1Ty, 4);
@@ -238,50 +246,52 @@
   EXPECT_EQ(CastInst::Trunc, CastInst::getCastOpcode(c64, true, V8x8Ty, true));
   EXPECT_EQ(CastInst::SExt, CastInst::getCastOpcode(c8, true, V8x64Ty, true));
 
-  EXPECT_FALSE(CastInst::isBitCastable(V8x8Ty, X86MMXTy));
-  EXPECT_FALSE(CastInst::isBitCastable(X86MMXTy, V8x8Ty));
-  EXPECT_FALSE(CastInst::isBitCastable(Int64Ty, X86MMXTy));
-  EXPECT_FALSE(CastInst::isBitCastable(V8x64Ty, V8x8Ty));
-  EXPECT_FALSE(CastInst::isBitCastable(V8x8Ty, V8x64Ty));
-
-  // Check address space casts are rejected since we don't know the sizes here
-  EXPECT_FALSE(CastInst::isBitCastable(Int32PtrTy, Int32PtrAS1Ty));
-  EXPECT_FALSE(CastInst::isBitCastable(Int32PtrAS1Ty, Int32PtrTy));
-  EXPECT_FALSE(CastInst::isBitCastable(V2Int32PtrTy, V2Int32PtrAS1Ty));
-  EXPECT_FALSE(CastInst::isBitCastable(V2Int32PtrAS1Ty, V2Int32PtrTy));
-  EXPECT_TRUE(CastInst::isBitCastable(V2Int32PtrAS1Ty, V2Int64PtrAS1Ty));
+  EXPECT_FALSE(CastInst::isBitCastable(V8x8Ty, X86MMXTy, DL));
+  EXPECT_FALSE(CastInst::isBitCastable(X86MMXTy, V8x8Ty, DL));
+  EXPECT_FALSE(CastInst::isBitCastable(Int64Ty, X86MMXTy, DL));
+  EXPECT_FALSE(CastInst::isBitCastable(V8x64Ty, V8x8Ty, DL));
+  EXPECT_FALSE(CastInst::isBitCastable(V8x8Ty, V8x64Ty, DL));
+
+  // Check validity of casts here
+  EXPECT_TRUE(CastInst::isBitCastable(Int32PtrAS2Ty, Int32PtrAS3Ty, DL));
+  EXPECT_FALSE(CastInst::isBitCastable(Int32PtrAS1Ty, Int32PtrAS2Ty, DL));
+  EXPECT_TRUE(CastInst::isBitCastable(Int32PtrTy, Int32PtrAS1Ty, DL));
+  EXPECT_TRUE(CastInst::isBitCastable(Int32PtrAS1Ty, Int32PtrTy, DL));
+  EXPECT_TRUE(CastInst::isBitCastable(V2Int32PtrTy, V2Int32PtrAS1Ty, DL));
+  EXPECT_TRUE(CastInst::isBitCastable(V2Int32PtrAS1Ty, V2Int32PtrTy, DL));
+  EXPECT_TRUE(CastInst::isBitCastable(V2Int32PtrAS1Ty, V2Int64PtrAS1Ty, DL));
+  EXPECT_FALSE(CastInst::isBitCastable(V2Int32PtrAS1Ty, V2Int32PtrAS2Ty, DL));
   EXPECT_EQ(CastInst::AddrSpaceCast, CastInst::getCastOpcode(v2ptr32, true,
  V2Int32PtrAS1Ty,
  true));
 
   // Test mismatched number of elements for pointers
-  EXPECT_FALSE(CastInst::isBitCastable(V2Int32PtrAS1Ty, V4Int64PtrAS1Ty));
-  EXPECT_FALSE(CastInst::isBitCastable(V4Int64PtrAS1Ty, V2

[PATCH] D114072: [clangd] Record IWYU pragma keep in the IncludeStructure

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

It's annoying that we see comments and inclusion directives out-of-order, we 
can try fixing it on the parser side (I think it is incidental that these are 
issued in that order currently, they are eagerly trying to generate a 
fix/diagnostic for tokens after a pp-directive. hence we can first issue the PP 
callback, and then diagnose for rest of the line instead).
But I don't think it matters in the long run especially when we want to handle 
different types of pragmas in the future (which is the plan IIRC), they might 
not even necessarily be in the main file, let alone being seen with right 
sequencing.

So I'd suggest just building a side table information about these interesting 
comments/pragmas we see as we go, and then merge them with rest of the 
information we've built inside `RecordHeaders::EndOfMainFile`.
That'll enable us to separate concerns here. Instead of piggy backing 
`CommentHandler` on `RecordHeaders`, we can have a separate comment handler 
exposed via `IncludeStructure::commentHandler()` (similar to `collect`) and let 
it update this side table of pragma/comment information.

WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114072

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

[PATCH] D114072: [clangd] Record IWYU pragma keep in the IncludeStructure

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Sorry for forgetting about this one. Hopefully I can still help now by 
disagreeing with Kadir and creating an awkward stalemate instead.

I agree that the ownership situation is ugly, but I think a single object here 
is still simpler. Partly because the ordering isn't some weird coincidence (see 
below)




Comment at: clang-tools-extra/clangd/Headers.cpp:103
 
+  // Because PP wants to own the PPCallbacks, CommentHandler will be added 
first
+  // and will also be called before handling the include directive. Example:

I don't think this has anything to do with ownership or registration order, 
rather events get recorded in order that constructs *finish* rather than start.

The PP doesn't call InclusionDirective until it sees the end of the line, and 
it has to skip over the comment to get there.

(I like the example and the sequencing, I'd just replace the first paragraph 
with "Given:" or so)



Comment at: clang-tools-extra/clangd/Headers.h:130
+  // IWYU pragmas but it needs to be explicitly added to as a preprocessor
+  // comment handler. PP wants to own the PPCallbacks, so the typical way to
+  // do both is:

Alternatively we could just give up on having a pure-ish API and say `void 
collect(Preprocessor&)`.
Again there's not really any other sensible way to use it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114072

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

[PATCH] D114370: [clangd] Record information about non self-contained headers in IncludeStructure

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: clang-tools-extra/clangd/Headers.h:167
 
+  void recordNonSelfContained(HeaderID ID) { NonSelfContained.insert(ID); }
+

sammccall wrote:
> could consider `friend class RecordHeaders`, either is ugly, up to you
That was the original problem I had the public field in the previous iteration: 
`RecordHeaders` is defined in anonymous namespace, so IIUC I can not friend it 
from here unless I make it a subclass or somehow visible from here, otherwise 
it wouldn't work, would it? Is there any clean way to do the `friend` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114370

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

[PATCH] D109856: [libunwind][ARM] Handle end of stack during unwind

[Daniel Kiss via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG632acec73776: [libunwind][ARM] Handle end of stack during 
unwind (authored by danielkiss).
Herald added projects: libc++abi, libunwind.
Herald added 1 blocking reviewer(s): libc++abi.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109856

Files:
  libcxxabi/src/cxa_personality.cpp
  libcxxabi/test/forced_unwind3.pass.cpp
  libunwind/src/Unwind-EHABI.cpp

Index: libunwind/src/Unwind-EHABI.cpp
===
--- libunwind/src/Unwind-EHABI.cpp
+++ libunwind/src/Unwind-EHABI.cpp
@@ -187,9 +187,14 @@
   if (result != _URC_CONTINUE_UNWIND)
 return result;
 
-  if (__unw_step(reinterpret_cast(context)) != UNW_STEP_SUCCESS)
+  switch (__unw_step(reinterpret_cast(context))) {
+  case UNW_STEP_SUCCESS:
+return _URC_CONTINUE_UNWIND;
+  case UNW_STEP_END:
+return _URC_END_OF_STACK;
+  default:
 return _URC_FAILURE;
-  return _URC_CONTINUE_UNWIND;
+  }
 }
 
 // Generates mask discriminator for _Unwind_VRS_Pop, e.g. for _UVRSC_CORE /
@@ -678,12 +683,13 @@
 unwind_phase2_forced(unw_context_t *uc, unw_cursor_t *cursor,
  _Unwind_Exception *exception_object, _Unwind_Stop_Fn stop,
  void *stop_parameter) {
+  bool endOfStack = false;
   // See comment at the start of unwind_phase1 regarding VRS integrity.
   __unw_init_local(cursor, uc);
   _LIBUNWIND_TRACE_UNWINDING("unwind_phase2_force(ex_ojb=%p)",
  static_cast(exception_object));
   // Walk each frame until we reach where search phase said to stop
-  while (true) {
+  while (!endOfStack) {
 // Update info about this frame.
 unw_proc_info_t frameInfo;
 if (__unw_get_proc_info(cursor, &frameInfo) != UNW_ESUCCESS) {
@@ -756,6 +762,14 @@
 // We may get control back if landing pad calls _Unwind_Resume().
 __unw_resume(cursor);
 break;
+  case _URC_END_OF_STACK:
+_LIBUNWIND_TRACE_UNWINDING("unwind_phase2_forced(ex_ojb=%p): "
+   "personality returned "
+   "_URC_END_OF_STACK",
+   (void *)exception_object);
+// Personalty routine did the step and it can't step forward.
+endOfStack = true;
+break;
   default:
 // Personality routine returned an unknown result code.
 _LIBUNWIND_TRACE_UNWINDING("unwind_phase2_forced(ex_ojb=%p): "
@@ -1133,9 +1147,14 @@
 __gnu_unwind_frame(_Unwind_Exception *exception_object,
struct _Unwind_Context *context) {
   unw_cursor_t *cursor = (unw_cursor_t *)context;
-  if (__unw_step(cursor) != UNW_STEP_SUCCESS)
+  switch (__unw_step(cursor)) {
+  case UNW_STEP_SUCCESS:
+return _URC_OK;
+  case UNW_STEP_END:
+return _URC_END_OF_STACK;
+  default:
 return _URC_FAILURE;
-  return _URC_OK;
+  }
 }
 
 #endif  // defined(_LIBUNWIND_ARM_EHABI)
Index: libcxxabi/test/forced_unwind3.pass.cpp
===
--- /dev/null
+++ libcxxabi/test/forced_unwind3.pass.cpp
@@ -0,0 +1,79 @@
+//===--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+// Let's run ForcedUnwind until it reaches end of the stack, this test simulates
+// what pthread_cancel does.
+
+// UNSUPPORTED: c++03
+// UNSUPPORTED: libcxxabi-no-threads
+// UNSUPPORTED: no-exceptions
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include <__cxxabi_config.h>
+
+// TODO: dump version back to 14 once clang is updated on the CI.
+#if defined(_LIBCXXABI_ARM_EHABI) && defined(__clang__) && __clang_major__ < 15
+// _Unwind_ForcedUnwind is not available or broken before version 14.
+int main(int, char**) { return 0; }
+
+#else
+static bool destructorCalled = false;
+
+struct myClass {
+  myClass() {}
+  ~myClass() {
+assert(destructorCalled == false);
+destructorCalled = true;
+  };
+};
+
+template 
+struct Stop;
+
+template 
+struct Stop {
+  // The third argument of _Unwind_Stop_Fn is uint64_t in Itanium C++ ABI/LLVM
+  // libunwind while _Unwind_Exception_Class in libgcc.
+  typedef typename std::tuple_element<2, std::tuple>::type type;
+
+  static _Unwind_Reason_Code stop(int, _Unwind_Action actions, type, struct _Unwind_Exception*, struct _Unwind_Context*,
+  void*) {
+if (actions & _UA_END_OF_STACK) {
+  assert(destructorCalled == true);
+  exit(0);
+}
+return _URC_NO_REASON;
+  }
+};
+
+static void forced_unwind() {
+  _Unw

[libunwind] 632acec - [libunwind][ARM] Handle end of stack during unwind

[Daniel Kiss via cfe-commits]

Author: Daniel Kiss
Date: 2021-11-26T13:26:49+01:00
New Revision: 632acec73776c4d6f7073d6de04ed6b8bfd36e6d

URL: 
https://github.com/llvm/llvm-project/commit/632acec73776c4d6f7073d6de04ed6b8bfd36e6d
DIFF: 
https://github.com/llvm/llvm-project/commit/632acec73776c4d6f7073d6de04ed6b8bfd36e6d.diff

LOG: [libunwind][ARM] Handle end of stack during unwind

When unwind step reaches the end of the stack that means the force unwind 
should notify the stop function.
This is not an error, it could mean just the thread is cleaned up completely.

Reviewed By: #libunwind, mstorsjo

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

Added: 
libcxxabi/test/forced_unwind3.pass.cpp

Modified: 
libcxxabi/src/cxa_personality.cpp
libunwind/src/Unwind-EHABI.cpp

Removed: 




diff  --git a/libcxxabi/src/cxa_personality.cpp 
b/libcxxabi/src/cxa_personality.cpp
index ccad45979db2a..f6e135f137c09 100644
--- a/libcxxabi/src/cxa_personality.cpp
+++ b/libcxxabi/src/cxa_personality.cpp
@@ -1004,9 +1004,14 @@ extern "C" _Unwind_Reason_Code 
__gnu_unwind_frame(_Unwind_Exception*,
 static _Unwind_Reason_Code continue_unwind(_Unwind_Exception* unwind_exception,
_Unwind_Context* context)
 {
-if (__gnu_unwind_frame(unwind_exception, context) != _URC_OK)
-return _URC_FAILURE;
+  switch (__gnu_unwind_frame(unwind_exception, context)) {
+  case _URC_OK:
 return _URC_CONTINUE_UNWIND;
+  case _URC_END_OF_STACK:
+return _URC_END_OF_STACK;
+  default:
+return _URC_FAILURE;
+  }
 }
 
 // ARM register names

diff  --git a/libcxxabi/test/forced_unwind3.pass.cpp 
b/libcxxabi/test/forced_unwind3.pass.cpp
new file mode 100644
index 0..3bcc7978aeab8
--- /dev/null
+++ b/libcxxabi/test/forced_unwind3.pass.cpp
@@ -0,0 +1,79 @@
+//===--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+// Let's run ForcedUnwind until it reaches end of the stack, this test 
simulates
+// what pthread_cancel does.
+
+// UNSUPPORTED: c++03
+// UNSUPPORTED: libcxxabi-no-threads
+// UNSUPPORTED: no-exceptions
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include <__cxxabi_config.h>
+
+// TODO: dump version back to 14 once clang is updated on the CI.
+#if defined(_LIBCXXABI_ARM_EHABI) && defined(__clang__) && __clang_major__ < 15
+// _Unwind_ForcedUnwind is not available or broken before version 14.
+int main(int, char**) { return 0; }
+
+#else
+static bool destructorCalled = false;
+
+struct myClass {
+  myClass() {}
+  ~myClass() {
+assert(destructorCalled == false);
+destructorCalled = true;
+  };
+};
+
+template 
+struct Stop;
+
+template 
+struct Stop {
+  // The third argument of _Unwind_Stop_Fn is uint64_t in Itanium C++ ABI/LLVM
+  // libunwind while _Unwind_Exception_Class in libgcc.
+  typedef typename std::tuple_element<2, std::tuple>::type type;
+
+  static _Unwind_Reason_Code stop(int, _Unwind_Action actions, type, struct 
_Unwind_Exception*, struct _Unwind_Context*,
+  void*) {
+if (actions & _UA_END_OF_STACK) {
+  assert(destructorCalled == true);
+  exit(0);
+}
+return _URC_NO_REASON;
+  }
+};
+
+static void forced_unwind() {
+  _Unwind_Exception* exc = new _Unwind_Exception;
+  memset(&exc->exception_class, 0, sizeof(exc->exception_class));
+  exc->exception_cleanup = 0;
+  _Unwind_ForcedUnwind(exc, Stop<_Unwind_Stop_Fn>::stop, 0);
+  abort();
+}
+
+__attribute__((__noinline__)) static void test() {
+  myClass c{};
+  forced_unwind();
+  abort();
+}
+
+int main(int, char**) {
+  std::thread t{test};
+  t.join();
+  return -1;
+}
+#endif

diff  --git a/libunwind/src/Unwind-EHABI.cpp b/libunwind/src/Unwind-EHABI.cpp
index d3577c9f7cf8d..5959d2a25fead 100644
--- a/libunwind/src/Unwind-EHABI.cpp
+++ b/libunwind/src/Unwind-EHABI.cpp
@@ -187,9 +187,14 @@ static _Unwind_Reason_Code unwindOneFrame(_Unwind_State 
state,
   if (result != _URC_CONTINUE_UNWIND)
 return result;
 
-  if (__unw_step(reinterpret_cast(context)) != 
UNW_STEP_SUCCESS)
+  switch (__unw_step(reinterpret_cast(context))) {
+  case UNW_STEP_SUCCESS:
+return _URC_CONTINUE_UNWIND;
+  case UNW_STEP_END:
+return _URC_END_OF_STACK;
+  default:
 return _URC_FAILURE;
-  return _URC_CONTINUE_UNWIND;
+  }
 }
 
 // Generates mask discriminator for _Unwind_VRS_Pop, e.g. for _UVRSC_CORE /
@@ -678,12 +683,13 @@ static _Unwind_Reason_Code
 unwind_phase2_forced(unw_context_t *uc, unw_cursor_t *cursor,
  _Unwind_Exception *exception_object, _Unwind_Stop_Fn stop,
  void *stop_p

[PATCH] D114602: [clang-tidy] Improve documentation of bugprone-unhandled-exception-at-new [NFC]

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst:8-10
 Calls to ``new`` can throw exceptions of type ``std::bad_alloc`` that should
 be handled by the code. Alternatively, the nonthrowing form of ``new`` can be
 used. The check verifies that the exception is handled in the function

`by the code` is either superfluous or misleading. Did you mean that the 
exception should be caught and handled in the same scope where the allocation 
took place?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst:16-17
+
+The exception handler is checked for types ``std::bad_alloc``,
+``std::exception``, and catch-all handler.
 The check assumes that any user-defined ``operator new`` is either





Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst:19-20
 The check assumes that any user-defined ``operator new`` is either
 ``noexcept`` or may throw an exception of type ``std::bad_alloc`` (or derived
 from it). Other exception types or exceptions occurring in the object's
 constructor are not taken into account.

What does

> exception types or exceptions

mean?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst:26
+  int *f() noexcept {
+int *p = new int[1000]; // warning: exception handler is missing
+// ...

Is that the warning message the check prints?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst:35
+
+  int *f1() { // no 'noexcept'
+int *p = new int[1000]; // no warning: exception can be handled outside




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114602

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

[PATCH] D114632: [Docs] Removed /Zd flag still mentioned in documentation

[BHUMITRAM KUMAR via Phabricator via cfe-commits]

BHUMITRAM created this revision.
BHUMITRAM added reviewers: aaron.ballman, xgupta.
BHUMITRAM requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

https://reviews.llvm.org/D93458 removed the /Zd flag as MSVC doesn't support 
that syntax. Instead users should be using -gline-tables-only.
The /Zd flag is still mentioned at 
https://clang.llvm.org/docs/UsersManual.html#clang-cl :   /Zd   
  Emit debug line number tables only.

Fix PR52571


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114632

Files:
  clang/docs/UsersManual.rst


Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -3716,7 +3716,6 @@
   /Zc:trigraphs   Enable trigraphs
   /Zc:twoPhase-   Disable two-phase name lookup in templates
   /Zc:twoPhaseEnable two-phase name lookup in templates
-  /Zd Emit debug line number tables only
   /Zi Alias for /Z7. Does not produce PDBs.
   /Zl Don't mention any default libraries in the 
object file
   /Zp Set the default maximum struct packing alignment 
to 1


Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -3716,7 +3716,6 @@
   /Zc:trigraphs   Enable trigraphs
   /Zc:twoPhase-   Disable two-phase name lookup in templates
   /Zc:twoPhaseEnable two-phase name lookup in templates
-  /Zd Emit debug line number tables only
   /Zi Alias for /Z7. Does not produce PDBs.
   /Zl Don't mention any default libraries in the object file
   /Zp Set the default maximum struct packing alignment to 1
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

I haven't looked at the patch in detail yet, but I know I've run into the issue 
this aims to fix when developing a Tidy check!

There is a `// NOLINTNEXTLINE(misc-redundant-expression)` directive in the 
source file 
`clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp` 
right before a `static_assert` that checks some `type_traits`-y stuff. Should 
be at line 510 -- according to my local checkout. Could you please remove that 
line and have that change be part of this patch too? I did a grep on the 
project and that's the only "user-side" mention to the misc-redundant-... 
check. Running Tidy on Tidy itself afterwards would turn out to be a nice 
real-world test that the fix indeed works. 🙂


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

https://reviews.llvm.org/D114622

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

[PATCH] D114632: [Docs] Removed /Zd flag still mentioned in documentation

[Shivam Gupta via Phabricator via cfe-commits]

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

LGTM, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114632

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

[clang] a3b099b - [Docs] Removed /Zd flag still mentioned in documentation

[Shivam Gupta via cfe-commits]

Author: Bhumitram Kumar
Date: 2021-11-26T18:08:06+05:30
New Revision: a3b099b68c0c156aa8ed9ec81c5dfdf150c6329c

URL: 
https://github.com/llvm/llvm-project/commit/a3b099b68c0c156aa8ed9ec81c5dfdf150c6329c
DIFF: 
https://github.com/llvm/llvm-project/commit/a3b099b68c0c156aa8ed9ec81c5dfdf150c6329c.diff

LOG: [Docs] Removed /Zd flag still mentioned in documentation

https://reviews.llvm.org/D93458 removed the /Zd flag as MSVC doesn't support 
that syntax. Instead users should be using -gline-tables-only.
The /Zd flag is still mentioned at 
https://clang.llvm.org/docs/UsersManual.html#clang-cl :   /Zd   
  Emit debug line number tables only.

Fix PR52571

Reviewed By: xgupta

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

Added: 


Modified: 
clang/docs/UsersManual.rst

Removed: 




diff  --git a/clang/docs/UsersManual.rst b/clang/docs/UsersManual.rst
index f534b21333583..ce66ef58fca62 100644
--- a/clang/docs/UsersManual.rst
+++ b/clang/docs/UsersManual.rst
@@ -3716,7 +3716,6 @@ Execute ``clang-cl /?`` to see a list of supported 
options:
   /Zc:trigraphs   Enable trigraphs
   /Zc:twoPhase-   Disable two-phase name lookup in templates
   /Zc:twoPhaseEnable two-phase name lookup in templates
-  /Zd Emit debug line number tables only
   /Zi Alias for /Z7. Does not produce PDBs.
   /Zl Don't mention any default libraries in the 
object file
   /Zp Set the default maximum struct packing alignment 
to 1



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

[PATCH] D114632: [Docs] Removed /Zd flag still mentioned in documentation

[Shivam Gupta via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa3b099b68c0c: [Docs] Removed /Zd flag still mentioned in 
documentation (authored by BHUMITRAM, committed by xgupta).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114632

Files:
  clang/docs/UsersManual.rst


Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -3716,7 +3716,6 @@
   /Zc:trigraphs   Enable trigraphs
   /Zc:twoPhase-   Disable two-phase name lookup in templates
   /Zc:twoPhaseEnable two-phase name lookup in templates
-  /Zd Emit debug line number tables only
   /Zi Alias for /Z7. Does not produce PDBs.
   /Zl Don't mention any default libraries in the 
object file
   /Zp Set the default maximum struct packing alignment 
to 1


Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -3716,7 +3716,6 @@
   /Zc:trigraphs   Enable trigraphs
   /Zc:twoPhase-   Disable two-phase name lookup in templates
   /Zc:twoPhaseEnable two-phase name lookup in templates
-  /Zd Emit debug line number tables only
   /Zi Alias for /Z7. Does not produce PDBs.
   /Zl Don't mention any default libraries in the object file
   /Zp Set the default maximum struct packing alignment to 1
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D114072: [clangd] Record IWYU pragma keep in the IncludeStructure

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Didn't really address this part

> But I don't think it matters in the long run especially when we want to 
> handle different types of pragmas in the future
> So I'd suggest just building a side table information

That's a more general approach for sure. But it does seem there's some 
complexity: what is the data structure and how do we merge it back into the 
graph. (e.g. are we going to record line numbers of each #include in the graph 
for this merging?)

So let's think about the concrete cases:

- export
- begin_exports...end_exports
- private maybe?

The TL;DR is that each of these either stand alone, or we see the directive 
before the inclusion it describes in a way that's easy to keep track of 
incrementally. So I'm not really seeing much implementation difficulty to 
justify trying to move the directives and inclusion processing apart.

Export seems basically similar. We're going to see the pragma immediately 
before the directive.
The same stateful solution seems just as clear, I don't see any problem with it 
not being in the main file.
(There's the "same line" issue of having to look up line tables for everything, 
maybe we can dodge it by comparing SourceLocations in raw form instead, or use 
the statefulness to avoid checks in most cases)

Begin/end is a bit different. The stateful solution looks like: keep a 
`Set` where exports are turned on, mutate it when you see a directive, 
and check it when you seen an inclusion.
The side-table solution is a bit more complicated: a map of fileIDs to a list 
of ranges or something. Manageable for sure.

Private I think we have to treat as a standalone directive that mutates the 
current file, and the reference to another file is treated as a string. It's 
not a file the PP has necessarily seen. So the result is a map 
and it seems adequate for this to be written straight into IncludeStructure.




Comment at: clang-tools-extra/clangd/Headers.cpp:119
+  bool HandleComment(Preprocessor &PP, SourceRange Range) override {
+llvm::StringRef Text =
+Lexer::getSourceText(CharSourceRange::getCharRange(Range),

This code will run for every comment in the preamble, so we should probably be 
careful about its performance.

I'd guess the cheapest thing is:
 - check that Range.begin() is not a macro
 - SM.getCharacterData(Range.begin()) yields a `char*` that's guaranteed 
null-terminated
 - try to consume the `IWYU pragma:` prefix

This doesn't deal with makeFileCharRange, touch End at all, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114072

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

[PATCH] D114370: [clangd] Record information about non self-contained headers in IncludeStructure

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/Headers.h:167
 
+  void recordNonSelfContained(HeaderID ID) { NonSelfContained.insert(ID); }
+

kbobyrev wrote:
> sammccall wrote:
> > could consider `friend class RecordHeaders`, either is ugly, up to you
> That was the original problem I had the public field in the previous 
> iteration: `RecordHeaders` is defined in anonymous namespace, so IIUC I can 
> not friend it from here unless I make it a subclass or somehow visible from 
> here, otherwise it wouldn't work, would it? Is there any clean way to do the 
> `friend` here?
No, you need to move it out of the anonymous namespace. Which is part of the 
ugliness, though not harmful in practice.

(If you're worried about namespace pollution you can make it a nested class of 
IncludeStructure. You have to forward declare it in the header. In that case 
you don't have to friend it)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114370

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

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:54-55
const NestedNameSpecifier *Right) {
-  llvm::FoldingSetNodeID LeftID, RightID;
-  Left->Profile(LeftID);
-  Right->Profile(RightID);
-  return LeftID == RightID;
+  if (const auto *L = Left->getAsType())
+if (const auto *R = Right->getAsType())
+  return L->getCanonicalTypeUnqualified() ==

(Wrt. to the comment about not typing stuff out directly, I guess `L` and `R` 
here will be `X` and `X` and thus not the same.)



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:54-57
+  if (const auto *L = Left->getAsType())
+if (const auto *R = Right->getAsType())
+  return L->getCanonicalTypeUnqualified() ==
+ R->getCanonicalTypeUnqualified();

whisperity wrote:
> (Wrt. to the comment about not typing stuff out directly, I guess `L` and `R` 
> here will be `X` and `X` and thus not the same.)
(Style nit to lessen the indent depth, until C++17...)



Comment at: 
clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:132-134
+const DeclRefExpr *DeclRef1 = cast(Left);
+const DeclRefExpr *DeclRef2 = cast(Right);
+return areEquivalentDeclRefs(DeclRef1, DeclRef2);

(Style nit. Or `LeftDR`, `RightDR`.)



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp:822
+
+  using my_char = char;
+  sink |= my_trait::value || my_trait::value;

Are there any more ways we could trick this check to cause false positives? 
Something through which it doesn't look the same but still refers to the same 
thing?

At the top of my head I'm thinking about something along the lines of this:

```lang=cpp
template 
struct X {
  template 
  static constexpr bool same = std::is_same_v;
};

X ch() { return X{}; }
X i() { return X{}; }

void test() {
  coin ? ch().same : i().same;
}
```

So the "type name" where we want to look up isn't typed out directly.
Or would these still be caught (and thus fixed) by looking at a 
`NestedNameSpecifier`?


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

https://reviews.llvm.org/D114622

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

[clang] 97e504c - [clang][deps] NFC: Extract function

[Jan Svoboda via cfe-commits]

Author: Jan Svoboda
Date: 2021-11-26T14:01:24+01:00
New Revision: 97e504cff956b7120da9bd932644b00a853ee68a

URL: 
https://github.com/llvm/llvm-project/commit/97e504cff956b7120da9bd932644b00a853ee68a
DIFF: 
https://github.com/llvm/llvm-project/commit/97e504cff956b7120da9bd932644b00a853ee68a.diff

LOG: [clang][deps] NFC: Extract function

This commits extracts a couple of nested conditions into a separate function 
with early returns, making the control flow easier to understand.

Added: 


Modified: 

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

Removed: 




diff  --git 
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h 
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
index b3a869af6107..af02fa2e7e87 100644
--- 
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ 
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -207,6 +207,11 @@ class DependencyScanningWorkerFilesystem : public 
llvm::vfs::ProxyFileSystem {
   llvm::ErrorOr
   getOrCreateFileSystemEntry(const StringRef Filename);
 
+  /// Create a cached file system entry based on the initial status result.
+  CachedFileSystemEntry
+  createFileSystemEntry(llvm::ErrorOr &&MaybeStatus,
+StringRef Filename, bool ShouldMinimize);
+
   /// The global cache shared between worker threads.
   DependencyScanningFilesystemSharedCache &SharedCache;
   /// The local cache is used by the worker thread to cache file system queries

diff  --git 
a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp 
b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
index 8973f2658ed6..f7c711690d7e 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -167,6 +167,19 @@ bool 
DependencyScanningWorkerFilesystem::shouldMinimize(StringRef RawFilename) {
   return !NotToBeMinimized.contains(Filename);
 }
 
+CachedFileSystemEntry 
DependencyScanningWorkerFilesystem::createFileSystemEntry(
+llvm::ErrorOr &&MaybeStatus, StringRef Filename,
+bool ShouldMinimize) {
+  if (!MaybeStatus)
+return CachedFileSystemEntry(MaybeStatus.getError());
+
+  if (MaybeStatus->isDirectory())
+return 
CachedFileSystemEntry::createDirectoryEntry(std::move(*MaybeStatus));
+
+  return CachedFileSystemEntry::createFileEntry(Filename, getUnderlyingFS(),
+ShouldMinimize);
+}
+
 llvm::ErrorOr
 DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
 const StringRef Filename) {
@@ -186,22 +199,15 @@ 
DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
 CachedFileSystemEntry &CacheEntry = SharedCacheEntry.Value;
 
 if (!CacheEntry.isValid()) {
-  llvm::vfs::FileSystem &FS = getUnderlyingFS();
-  auto MaybeStatus = FS.status(Filename);
-  if (!MaybeStatus) {
-if (!shouldCacheStatFailures(Filename))
-  // HACK: We need to always restat non source files if the stat fails.
-  //   This is because Clang first looks up the module cache and module
-  //   files before building them, and then looks for them again. If we
-  //   cache the stat failure, it won't see them the second time.
-  return MaybeStatus.getError();
-CacheEntry = CachedFileSystemEntry(MaybeStatus.getError());
-  } else if (MaybeStatus->isDirectory())
-CacheEntry = CachedFileSystemEntry::createDirectoryEntry(
-std::move(*MaybeStatus));
-  else
-CacheEntry = CachedFileSystemEntry::createFileEntry(Filename, FS,
-ShouldMinimize);
+  auto MaybeStatus = getUnderlyingFS().status(Filename);
+  if (!MaybeStatus && !shouldCacheStatFailures(Filename))
+// HACK: We need to always restat non source files if the stat fails.
+//   This is because Clang first looks up the module cache and module
+//   files before building them, and then looks for them again. If we
+//   cache the stat failure, it won't see them the second time.
+return MaybeStatus.getError();
+  CacheEntry = createFileSystemEntry(std::move(MaybeStatus), Filename,
+ ShouldMinimize);
 }
 
 Result = &CacheEntry;



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

[PATCH] D114197: [clang-tidy] Fix false positives involving type aliases in `misc-unconventional-assign-operator` check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-unconventional-assign-operator.cpp:151
+  using Alias3 = TemplateTypeAlias;
+  Alias3 &operator=(int) { return *this; }
+};

This is a no-warn due to the parameter being a completely unrelated type, 
right? Might worth a comment. I don't see at first glance why a warning should 
not happen here.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-unconventional-assign-operator.cpp:152
+  Alias3 &operator=(int) { return *this; }
+};

What about `Alias3& operator =(const Alias1&) { return *this; 
}`? That should trigger a warning as it is an unrelated type, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114197

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

[PATCH] D112421: [clang][ARM] PACBTI-M frontend support

[Ties Stuij via Phabricator via cfe-commits]

stuij updated this revision to Diff 390021.
stuij added a comment.

addressed review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112421

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Basic/Targets/AArch64.cpp
  clang/lib/Basic/Targets/ARM.cpp
  clang/lib/Basic/Targets/ARM.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/arm-branch-protection-attr-1.c
  clang/test/CodeGen/arm-branch-protection-attr-2.c
  clang/test/CodeGen/arm_neon_intrinsics.c
  clang/test/Driver/aarch64-security-options.c
  clang/test/Driver/arm-security-options.c
  clang/test/Frontend/arm-invalid-branch-protection.c
  clang/test/Sema/aarch64-branch-protection-attr-err.c
  clang/test/Sema/arm-branch-protection-attr-err.c
  clang/test/Sema/branch-protection-attr-err.c
  llvm/include/llvm/Support/AArch64TargetParser.h
  llvm/include/llvm/Support/TargetParser.h
  llvm/lib/Support/AArch64TargetParser.cpp
  llvm/lib/Support/TargetParser.cpp

Index: llvm/lib/Support/TargetParser.cpp
===
--- llvm/lib/Support/TargetParser.cpp
+++ llvm/lib/Support/TargetParser.cpp
@@ -333,3 +333,51 @@
 
 } // namespace RISCV
 } // namespace llvm
+
+// Parse a branch protection specification, which has the form
+//   standard | none | [bti,pac-ret[+b-key,+leaf]*]
+// Returns true on success, with individual elements of the specification
+// returned in `PBP`. Returns false in error, with `Err` containing
+// an erroneous part of the spec.
+bool ARM::parseBranchProtection(StringRef Spec, ParsedBranchProtection &PBP,
+StringRef &Err) {
+  PBP = {"none", "a_key", false};
+  if (Spec == "none")
+return true; // defaults are ok
+
+  if (Spec == "standard") {
+PBP.Scope = "non-leaf";
+PBP.BranchTargetEnforcement = true;
+return true;
+  }
+
+  SmallVector Opts;
+  Spec.split(Opts, "+");
+  for (int I = 0, E = Opts.size(); I != E; ++I) {
+StringRef Opt = Opts[I].trim();
+if (Opt == "bti") {
+  PBP.BranchTargetEnforcement = true;
+  continue;
+}
+if (Opt == "pac-ret") {
+  PBP.Scope = "non-leaf";
+  for (; I + 1 != E; ++I) {
+StringRef PACOpt = Opts[I + 1].trim();
+if (PACOpt == "leaf")
+  PBP.Scope = "all";
+else if (PACOpt == "b-key")
+  PBP.Key = "b_key";
+else
+  break;
+  }
+  continue;
+}
+if (Opt == "")
+  Err = "";
+else
+  Err = Opt;
+return false;
+  }
+
+  return true;
+}
Index: llvm/lib/Support/AArch64TargetParser.cpp
===
--- llvm/lib/Support/AArch64TargetParser.cpp
+++ llvm/lib/Support/AArch64TargetParser.cpp
@@ -240,52 +240,4 @@
   return C.ArchID;
   }
   return ArchKind::INVALID;
-}
-
-// Parse a branch protection specification, which has the form
-//   standard | none | [bti,pac-ret[+b-key,+leaf]*]
-// Returns true on success, with individual elements of the specification
-// returned in `PBP`. Returns false in error, with `Err` containing
-// an erroneous part of the spec.
-bool AArch64::parseBranchProtection(StringRef Spec, ParsedBranchProtection &PBP,
-StringRef &Err) {
-  PBP = {"none", "a_key", false};
-  if (Spec == "none")
-return true; // defaults are ok
-
-  if (Spec == "standard") {
-PBP.Scope = "non-leaf";
-PBP.BranchTargetEnforcement = true;
-return true;
-  }
-
-  SmallVector Opts;
-  Spec.split(Opts, "+");
-  for (int I = 0, E = Opts.size(); I != E; ++I) {
-StringRef Opt = Opts[I].trim();
-if (Opt == "bti") {
-  PBP.BranchTargetEnforcement = true;
-  continue;
-}
-if (Opt == "pac-ret") {
-  PBP.Scope = "non-leaf";
-  for (; I + 1 != E; ++I) {
-StringRef PACOpt = Opts[I + 1].trim();
-if (PACOpt == "leaf")
-  PBP.Scope = "all";
-else if (PACOpt == "b-key")
-  PBP.Key = "b_key";
-else
-  break;
-  }
-  continue;
-}
-if (Opt == "")
-  Err = "";
-else
-  Err = Opt;
-return false;
-  }
-
-  return true;
-}
+}
\ No newline at end of file
Index: llvm/include/llvm/Support/TargetParser.h
===
--- llvm/include/llvm/Support/TargetParser.h
+++ llvm/include/llvm/Support/TargetParser.h
@@ -177,6 +177,18 @@
 
 } // namespace RISCV
 
+namespace ARM {
+struct ParsedBranchProtection {
+  StringRef Scope;
+  StringRef Key;
+  bool BranchTargetEnforcement;
+};
+
+bool parseBranchProtection(StringRef Spec, ParsedBranchProtection &PBP,
+   StringRef &Err);
+
+} // namespace ARM

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D114622#3155605 , @whisperity 
wrote:

> I haven't looked at the patch in detail yet, but I know I've run into the 
> issue this aims to fix when developing a Tidy check!
>
> There is a `// NOLINTNEXTLINE(misc-redundant-expression)` directive in the 
> source file 
> `clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp` 
> right before a `static_assert` that checks some `type_traits`-y stuff. Should 
> be at line **510** -- according to my local checkout. Could you please remove 
> that line and have that change be part of this patch too? I did a grep on the 
> project and that's the only "user-side" mention to the misc-redundant-... 
> check. Running Tidy on Tidy itself afterwards would turn out to be a nice 
> real-world test that the fix indeed works. 🙂

Yup, it works. I removed your `NOLINT` comment.


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

https://reviews.llvm.org/D114622

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

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Balázs Benics via Phabricator via cfe-commits]

steakhal updated this revision to Diff 390023.

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

https://reviews.llvm.org/D114622

Files:
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
  clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
  clang/test/Analysis/identical-expressions.cpp

Index: clang/test/Analysis/identical-expressions.cpp
===
--- clang/test/Analysis/identical-expressions.cpp
+++ clang/test/Analysis/identical-expressions.cpp
@@ -1562,3 +1562,50 @@
   ;
   }
 }
+
+template  struct boolean_value {
+  static constexpr bool value = V;
+};
+template  struct my_trait : boolean_value {};
+
+bool respect_nested_name_specifiers(bool sink) {
+  sink |= my_trait::value || my_trait::value; // no-warning
+
+  sink |= my_trait::value || my_trait::value;
+  // expected-warning@-1 {{identical expressions on both sides of logical operator}}
+
+  using my_char = char;
+  sink |= my_trait::value || my_trait::value;
+  // expected-warning@-1 {{identical expressions on both sides of logical operator}}
+
+  return sink;
+}
+
+static void static_function() {}
+namespace my {
+int fn(int = 1);
+}
+void respect_namespaces(bool coin) {
+  namespace other = my;
+  using namespace other;
+
+  coin ? my::fn(1) : my::fn(2);// no-warning
+  coin ? my::fn(1) : other::fn(2); // no-warning
+
+  // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+  coin ? my::fn(1) : my::fn(1);
+  coin ? my::fn(1) : other::fn(1);
+  coin ? my::fn(1) : fn(1);
+
+  coin ? my::fn(1) : fn(); // FIXME: We should have a warning for this: the default parameter is also 1.
+
+  my::fn(1) & my::fn(1);// no-warning
+  my::fn(1) & other::fn(1); // no-warning
+
+  // expected-warning@+2 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+2 {{identical expressions on both sides of ':' in conditional expression}}
+  coin ? ::static_function() : ::static_function();
+  coin ? ::static_function() : static_function();
+}
Index: clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
@@ -295,6 +295,31 @@
   return true;
 }
 
+static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
+   const NestedNameSpecifier *Right) {
+  if (const auto *L = Left->getAsType())
+if (const auto *R = Right->getAsType())
+  return L->getCanonicalTypeUnqualified() ==
+ R->getCanonicalTypeUnqualified();
+
+  // FIXME: We should probably check the other kinds of nested name specifiers.
+  return true;
+}
+
+static bool areEquivalentDeclRefs(const DeclRefExpr *Left,
+  const DeclRefExpr *Right) {
+  if (Left->getDecl()->getCanonicalDecl() !=
+  Right->getDecl()->getCanonicalDecl()) {
+return false;
+  }
+
+  // The decls were already matched, so just return true at any later point.
+  if (!Left->hasQualifier() || !Right->hasQualifier())
+return true;
+  return areEquivalentNameSpecifier(Left->getQualifier(),
+Right->getQualifier());
+}
+
 /// Determines whether two statement trees are identical regarding
 /// operators and symbols.
 ///
@@ -461,7 +486,7 @@
   case Stmt::DeclRefExprClass: {
 const DeclRefExpr *DeclRef1 = cast(Stmt1);
 const DeclRefExpr *DeclRef2 = cast(Stmt2);
-return DeclRef1->getDecl() == DeclRef2->getDecl();
+return areEquivalentDeclRefs(DeclRef1, DeclRef2);
   }
   case Stmt::IntegerLiteralClass: {
 const IntegerLiteral *IntLit1 = cast(Stmt1);
Index: clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
@@ -807,3 +807,52 @@
 };
 
 } // namespace no_crash
+
+template  struct boolean_value {
+  static constexpr bool value = V;
+};
+template  struct my_trait : boolean_value {};
+
+bool respect_nested_name_specifiers(bool sink) {
+  sink |= my_trait::value || my_trait::value; // no-warning
+
+  sink |= my_trait::value || my_trait::value;
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: both sides of operator are equivalent
+
+  using my_char = char;
+  sink |= my_trait::value || my_trait::value;
+  // CHECK-MESSAGES: :[[@LIN

[PATCH] D114370: [clangd] Record information about non self-contained headers in IncludeStructure

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 390024.
kbobyrev marked 2 inline comments as done.
kbobyrev added a comment.

Get rid of the setter in IncludeStructure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114370

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp

Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -82,8 +82,7 @@
 return {};
   }
   IncludeStructure Includes;
-  Clang->getPreprocessor().addPPCallbacks(
-  Includes.collect(Clang->getSourceManager()));
+  Clang->getPreprocessor().addPPCallbacks(Includes.collect(*Clang));
   if (llvm::Error Err = Action.Execute()) {
 ADD_FAILURE() << "failed to execute action: " << std::move(Err);
 return {};
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -80,8 +80,7 @@
 EXPECT_TRUE(
 Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
 IncludeStructure Includes;
-Clang->getPreprocessor().addPPCallbacks(
-Includes.collect(Clang->getSourceManager()));
+Clang->getPreprocessor().addPPCallbacks(Includes.collect(*Clang));
 EXPECT_FALSE(Action.Execute());
 Action.EndSourceFile();
 return Includes;
@@ -363,6 +362,39 @@
   EXPECT_THAT(collectIncludes().MainFileIncludes,
   Contains(AllOf(IncludeLine(2), Written("";
 }
+
+TEST_F(HeadersTest, SelfContainedHeaders) {
+  // Including through non-builtin file has no effects.
+  FS.Files[MainFile] = R"cpp(
+#include "includeguarded.h"
+#include "nonguarded.h"
+#include "pp_depend.h"
+#include "pragmaguarded.h"
+)cpp";
+  FS.Files["pragmaguarded.h"] = R"cpp(
+#pragma once
+)cpp";
+  FS.Files["includeguarded.h"] = R"cpp(
+#ifndef INCLUDE_GUARDED_H
+#define INCLUDE_GUARDED_H
+void foo();
+#endif // INCLUDE_GUARDED_H
+)cpp";
+  FS.Files["nonguarded.h"] = R"cpp(
+)cpp";
+  FS.Files["pp_depend.h"] = R"cpp(
+  #ifndef REQUIRED_PP_DIRECTIVE
+  # error You have to have PP directive set to include this one!
+  #endif
+)cpp";
+
+  auto Includes = collectIncludes();
+  EXPECT_TRUE(Includes.isSelfContained(getID("pragmaguarded.h", Includes)));
+  EXPECT_TRUE(Includes.isSelfContained(getID("includeguarded.h", Includes)));
+  EXPECT_FALSE(Includes.isSelfContained(getID("nonguarded.h", Includes)));
+  EXPECT_FALSE(Includes.isSelfContained(getID("pp_depend.h", Includes)));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -266,7 +266,8 @@
 return toURI(Canonical);
   }
 }
-if (!isSelfContainedHeader(FID, FE)) {
+if (!isSelfContainedHeader(FE, FID, PP->getSourceManager(),
+   PP->getHeaderSearchInfo())) {
   // A .inc or .def file is often included into a real header to define
   // symbols (e.g. LLVM tablegen files).
   if (Filename.endswith(".inc") || Filename.endswith(".def"))
@@ -278,54 +279,6 @@
 // Standard case: just insert the file itself.
 return toURI(FE);
   }
-
-  bool isSelfContainedHeader(FileID FID, const FileEntry *FE) {
-// FIXME: Should files that have been #import'd be considered
-// self-contained? That's really a property of the includer,
-// not of the file.
-if (!PP->getHeaderSearchInfo().isFileMultipleIncludeGuarded(FE) &&
-!PP->getHeaderSearchInfo().hasFileBeenImported(FE))
-  return false;
-// This pattern indicates that a header can't be used without
-// particular preprocessor state, usually set up by another header.
-if (isDontIncludeMeHeader(SM.getBufferData(FID)))
-  return false;
-return true;
-  }
-
-  // Is Line an #if or #ifdef directive?
-  static bool isIf(llvm::StringRef Line) {
-Line = Line.ltrim();
-if (!Line.consume_front("#"))
-  return false;
-Line = Line.ltrim();
-return Line.startswith("if");
-  }
-
-  // Is Line an #error directive mentioning includes?
-  static bool isErrorAboutInclude(llvm::StringRef Line) {
-Line = Line.ltrim();
-if (!Line.consu

[PATCH] D114370: [clangd] Record information about non self-contained headers in IncludeStructure

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: clang-tools-extra/clangd/Headers.h:167
 
+  void recordNonSelfContained(HeaderID ID) { NonSelfContained.insert(ID); }
+

sammccall wrote:
> kbobyrev wrote:
> > sammccall wrote:
> > > could consider `friend class RecordHeaders`, either is ugly, up to you
> > That was the original problem I had the public field in the previous 
> > iteration: `RecordHeaders` is defined in anonymous namespace, so IIUC I can 
> > not friend it from here unless I make it a subclass or somehow visible from 
> > here, otherwise it wouldn't work, would it? Is there any clean way to do 
> > the `friend` here?
> No, you need to move it out of the anonymous namespace. Which is part of the 
> ugliness, though not harmful in practice.
> 
> (If you're worried about namespace pollution you can make it a nested class 
> of IncludeStructure. You have to forward declare it in the header. In that 
> case you don't have to friend it)
I see, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114370

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

[clang-tools-extra] cd0ca5a - [clangd] Record information about non self-contained headers in IncludeStructure

[Kirill Bobyrev via cfe-commits]

Author: Kirill Bobyrev
Date: 2021-11-26T14:12:54+01:00
New Revision: cd0ca5a0eaa1b75b445e82753ea093bbb8e7e85c

URL: 
https://github.com/llvm/llvm-project/commit/cd0ca5a0eaa1b75b445e82753ea093bbb8e7e85c
DIFF: 
https://github.com/llvm/llvm-project/commit/cd0ca5a0eaa1b75b445e82753ea093bbb8e7e85c.diff

LOG: [clangd] Record information about non self-contained headers in 
IncludeStructure

This will be useful for IncludeCleaner.

Reviewed By: sammccall

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

Added: 


Modified: 
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/Headers.cpp
clang-tools-extra/clangd/Headers.h
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/SourceCode.cpp
clang-tools-extra/clangd/SourceCode.h
clang-tools-extra/clangd/index/SymbolCollector.cpp
clang-tools-extra/clangd/unittests/HeadersTests.cpp
clang-tools-extra/clangd/unittests/PreambleTests.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/CodeComplete.cpp 
b/clang-tools-extra/clangd/CodeComplete.cpp
index ac6c3b077d47..63111c264c73 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/clang-tools-extra/clangd/CodeComplete.cpp
@@ -1272,8 +1272,7 @@ bool 
semaCodeComplete(std::unique_ptr Consumer,
   // Force them to be deserialized so SemaCodeComplete sees them.
   loadMainFilePreambleMacros(Clang->getPreprocessor(), Input.Preamble);
   if (Includes)
-Clang->getPreprocessor().addPPCallbacks(
-Includes->collect(Clang->getSourceManager()));
+Clang->getPreprocessor().addPPCallbacks(Includes->collect(*Clang));
   if (llvm::Error Err = Action.Execute()) {
 log("Execute() failed when running codeComplete for {0}: {1}",
 Input.FileName, toString(std::move(Err)));

diff  --git a/clang-tools-extra/clangd/Headers.cpp 
b/clang-tools-extra/clangd/Headers.cpp
index 9f5ab244aa63..dc113068ef3a 100644
--- a/clang-tools-extra/clangd/Headers.cpp
+++ b/clang-tools-extra/clangd/Headers.cpp
@@ -22,12 +22,12 @@
 
 namespace clang {
 namespace clangd {
-namespace {
 
-class RecordHeaders : public PPCallbacks {
+class IncludeStructure::RecordHeaders : public PPCallbacks {
 public:
-  RecordHeaders(const SourceManager &SM, IncludeStructure *Out)
-  : SM(SM), Out(Out) {}
+  RecordHeaders(const SourceManager &SM, HeaderSearch &HeaderInfo,
+IncludeStructure *Out)
+  : SM(SM), HeaderInfo(HeaderInfo), Out(Out) {}
 
   // Record existing #includes - both written and resolved paths. Only 
#includes
   // in the main file are collected.
@@ -85,10 +85,17 @@ class RecordHeaders : public PPCallbacks {
 InBuiltinFile = true;
   }
   break;
-case PPCallbacks::ExitFile:
+case PPCallbacks::ExitFile: {
   if (PrevFID == BuiltinFile)
 InBuiltinFile = false;
+  // At file exit time HeaderSearchInfo is valid and can be used to
+  // determine whether the file was a self-contained header or not.
+  if (const FileEntry *FE = SM.getFileEntryForID(PrevFID))
+if (!isSelfContainedHeader(FE, PrevFID, SM, HeaderInfo))
+  Out->NonSelfContained.insert(
+  *Out->getID(SM.getFileEntryForID(PrevFID)));
   break;
+}
 case PPCallbacks::RenameFile:
 case PPCallbacks::SystemHeaderPragma:
   break;
@@ -97,6 +104,7 @@ class RecordHeaders : public PPCallbacks {
 
 private:
   const SourceManager &SM;
+  HeaderSearch &HeaderInfo;
   // Set after entering the  file.
   FileID BuiltinFile;
   // Indicates whether  file is part of include stack.
@@ -105,8 +113,6 @@ class RecordHeaders : public PPCallbacks {
   IncludeStructure *Out;
 };
 
-} // namespace
-
 bool isLiteralInclude(llvm::StringRef Include) {
   return Include.startswith("<") || Include.startswith("\"");
 }
@@ -152,9 +158,11 @@ llvm::SmallVector 
getRankedIncludes(const Symbol &Sym) {
 }
 
 std::unique_ptr
-IncludeStructure::collect(const SourceManager &SM) {
+IncludeStructure::collect(const CompilerInstance &CI) {
+  auto &SM = CI.getSourceManager();
   MainFileEntry = SM.getFileEntryForID(SM.getMainFileID());
-  return std::make_unique(SM, this);
+  return std::make_unique(
+  SM, CI.getPreprocessor().getHeaderSearchInfo(), this);
 }
 
 llvm::Optional

diff  --git a/clang-tools-extra/clangd/Headers.h 
b/clang-tools-extra/clangd/Headers.h
index 7b42598b819c..b6c67478568f 100644
--- a/clang-tools-extra/clangd/Headers.h
+++ b/clang-tools-extra/clangd/Headers.h
@@ -17,10 +17,12 @@
 #include "clang/Basic/FileEntry.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Format/Format.h"
+#include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Tooling/Inclusions/HeaderIncludes.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Stri

[PATCH] D114370: [clangd] Record information about non self-contained headers in IncludeStructure

[Kirill Bobyrev via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGcd0ca5a0eaa1: [clangd] Record information about non 
self-contained headers in IncludeStructure (authored by kbobyrev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114370

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Headers.cpp
  clang-tools-extra/clangd/Headers.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/SourceCode.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/HeadersTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp

Index: clang-tools-extra/clangd/unittests/PreambleTests.cpp
===
--- clang-tools-extra/clangd/unittests/PreambleTests.cpp
+++ clang-tools-extra/clangd/unittests/PreambleTests.cpp
@@ -82,8 +82,7 @@
 return {};
   }
   IncludeStructure Includes;
-  Clang->getPreprocessor().addPPCallbacks(
-  Includes.collect(Clang->getSourceManager()));
+  Clang->getPreprocessor().addPPCallbacks(Includes.collect(*Clang));
   if (llvm::Error Err = Action.Execute()) {
 ADD_FAILURE() << "failed to execute action: " << std::move(Err);
 return {};
Index: clang-tools-extra/clangd/unittests/HeadersTests.cpp
===
--- clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -80,8 +80,7 @@
 EXPECT_TRUE(
 Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
 IncludeStructure Includes;
-Clang->getPreprocessor().addPPCallbacks(
-Includes.collect(Clang->getSourceManager()));
+Clang->getPreprocessor().addPPCallbacks(Includes.collect(*Clang));
 EXPECT_FALSE(Action.Execute());
 Action.EndSourceFile();
 return Includes;
@@ -363,6 +362,39 @@
   EXPECT_THAT(collectIncludes().MainFileIncludes,
   Contains(AllOf(IncludeLine(2), Written("";
 }
+
+TEST_F(HeadersTest, SelfContainedHeaders) {
+  // Including through non-builtin file has no effects.
+  FS.Files[MainFile] = R"cpp(
+#include "includeguarded.h"
+#include "nonguarded.h"
+#include "pp_depend.h"
+#include "pragmaguarded.h"
+)cpp";
+  FS.Files["pragmaguarded.h"] = R"cpp(
+#pragma once
+)cpp";
+  FS.Files["includeguarded.h"] = R"cpp(
+#ifndef INCLUDE_GUARDED_H
+#define INCLUDE_GUARDED_H
+void foo();
+#endif // INCLUDE_GUARDED_H
+)cpp";
+  FS.Files["nonguarded.h"] = R"cpp(
+)cpp";
+  FS.Files["pp_depend.h"] = R"cpp(
+  #ifndef REQUIRED_PP_DIRECTIVE
+  # error You have to have PP directive set to include this one!
+  #endif
+)cpp";
+
+  auto Includes = collectIncludes();
+  EXPECT_TRUE(Includes.isSelfContained(getID("pragmaguarded.h", Includes)));
+  EXPECT_TRUE(Includes.isSelfContained(getID("includeguarded.h", Includes)));
+  EXPECT_FALSE(Includes.isSelfContained(getID("nonguarded.h", Includes)));
+  EXPECT_FALSE(Includes.isSelfContained(getID("pp_depend.h", Includes)));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -266,7 +266,8 @@
 return toURI(Canonical);
   }
 }
-if (!isSelfContainedHeader(FID, FE)) {
+if (!isSelfContainedHeader(FE, FID, PP->getSourceManager(),
+   PP->getHeaderSearchInfo())) {
   // A .inc or .def file is often included into a real header to define
   // symbols (e.g. LLVM tablegen files).
   if (Filename.endswith(".inc") || Filename.endswith(".def"))
@@ -278,54 +279,6 @@
 // Standard case: just insert the file itself.
 return toURI(FE);
   }
-
-  bool isSelfContainedHeader(FileID FID, const FileEntry *FE) {
-// FIXME: Should files that have been #import'd be considered
-// self-contained? That's really a property of the includer,
-// not of the file.
-if (!PP->getHeaderSearchInfo().isFileMultipleIncludeGuarded(FE) &&
-!PP->getHeaderSearchInfo().hasFileBeenImported(FE))
-  return false;
-// This pattern indicates that a header can't be used without
-// particular preprocessor state, usually set up by another header.
-if (isDontIncludeMeHeader(SM.getBufferData(FID)))
-  return false;
-return true;
-  }
-
-  // Is Line an #if or #ifdef directive?
-  static bool isIf(llvm::StringRef Line) {
-Line = Line.ltrim();
-if (!Line.consume_front("#"))
-  return false;
-Line = Line.ltrim();
-return Line.startswith("if");
-  }
-
-  // Is Line an #error directive mentioning include

[PATCH] D114234: [clang][dataflow] Add base types for building dataflow analyses

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev updated this revision to Diff 390029.
sgatev marked 3 inline comments as done.
sgatev added a comment.

Replace "Dynamic" with "TypeErased" in the names of types and their members.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114234

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
  clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
  clang/include/clang/Analysis/FlowSensitive/DataflowLattice.h
  clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
  clang/lib/Analysis/CMakeLists.txt
  clang/lib/Analysis/FlowSensitive/CMakeLists.txt
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===
--- /dev/null
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -0,0 +1,35 @@
+//===- TypeErasedDataflowAnalysis.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines type-erased base types and functions for building dataflow
+//  analyses that run over Control-Flow Graphs (CFGs).
+//
+//===--===//
+
+#include 
+
+#include "clang/Analysis/CFG.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h"
+#include "llvm/ADT/Optional.h"
+
+using namespace clang;
+using namespace dataflow;
+
+std::vector>
+runTypeErasedDataflowAnalysis(const CFG &Cfg,
+  TypeErasedDataflowAnalysis &Analysis,
+  const Environment &InitEnv) {
+  // FIXME: Consider enforcing that `Cfg` meets the requirements that
+  // are specified in the header. This could be done by remembering
+  // what options were used to build `Cfg` and asserting on them here.
+
+  // FIXME: Implement work list-based algorithm to compute the fixed
+  // point of `Analysis::transform` for every basic block in `Cfg`.
+  return {};
+}
Index: clang/lib/Analysis/FlowSensitive/CMakeLists.txt
===
--- /dev/null
+++ clang/lib/Analysis/FlowSensitive/CMakeLists.txt
@@ -0,0 +1,7 @@
+add_clang_library(clangAnalysisFlowSensitive
+  TypeErasedDataflowAnalysis.cpp
+
+  LINK_LIBS
+  clangAnalysis
+  clangAST
+  )
Index: clang/lib/Analysis/CMakeLists.txt
===
--- clang/lib/Analysis/CMakeLists.txt
+++ clang/lib/Analysis/CMakeLists.txt
@@ -44,3 +44,4 @@
   )
 
 add_subdirectory(plugins)
+add_subdirectory(FlowSensitive)
Index: clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
===
--- /dev/null
+++ clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
@@ -0,0 +1,93 @@
+//===- TypeErasedDataflowAnalysis.h -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines type-erased base types and functions for building dataflow
+//  analyses that run over Control-Flow Graphs (CFGs).
+//
+//===--===//
+
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_TYPEERASEDDATAFLOWANALYSIS_H
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_TYPEERASEDDATAFLOWANALYSIS_H
+
+#include 
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Analysis/CFG.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "llvm/ADT/Any.h"
+#include "llvm/ADT/Optional.h"
+
+namespace clang {
+namespace dataflow {
+
+/// Type-erased lattice element container.
+///
+/// Requirements:
+///
+///  The type of the object stored in the container must be a bounded
+///  join-semilattice.
+struct TypeErasedLattice {
+  llvm::Any Value;
+};
+
+/// Type-erased base class for dataflow analyses built on a single lattice type.
+class TypeErasedDataflowAnalysis {
+public:
+  /// Returns the `ASTContext` that is used by the analysis.
+  virtual ASTContext &getASTContext() = 0;
+
+  /// Returns a type-erased lattice element that models the initial state of a
+  /// basic block.
+  virtual TypeErasedLattice typeErasedInitialElement() = 0;
+
+  /

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Balázs Benics via Phabricator via cfe-commits]

steakhal updated this revision to Diff 390032.
steakhal marked 3 inline comments as done.
steakhal added a comment.

Fixing nits: renaming variables, etc.


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

https://reviews.llvm.org/D114622

Files:
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
  clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
  clang/test/Analysis/identical-expressions.cpp

Index: clang/test/Analysis/identical-expressions.cpp
===
--- clang/test/Analysis/identical-expressions.cpp
+++ clang/test/Analysis/identical-expressions.cpp
@@ -1562,3 +1562,50 @@
   ;
   }
 }
+
+template  struct boolean_value {
+  static constexpr bool value = V;
+};
+template  struct my_trait : boolean_value {};
+
+bool respect_nested_name_specifiers(bool sink) {
+  sink |= my_trait::value || my_trait::value; // no-warning
+
+  sink |= my_trait::value || my_trait::value;
+  // expected-warning@-1 {{identical expressions on both sides of logical operator}}
+
+  using my_char = char;
+  sink |= my_trait::value || my_trait::value;
+  // expected-warning@-1 {{identical expressions on both sides of logical operator}}
+
+  return sink;
+}
+
+static void static_function() {}
+namespace my {
+int fn(int = 1);
+}
+void respect_namespaces(bool coin) {
+  namespace other = my;
+  using namespace other;
+
+  coin ? my::fn(1) : my::fn(2);// no-warning
+  coin ? my::fn(1) : other::fn(2); // no-warning
+
+  // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+3 {{identical expressions on both sides of ':' in conditional expression}}
+  coin ? my::fn(1) : my::fn(1);
+  coin ? my::fn(1) : other::fn(1);
+  coin ? my::fn(1) : fn(1);
+
+  coin ? my::fn(1) : fn(); // FIXME: We should have a warning for this: the default parameter is also 1.
+
+  my::fn(1) & my::fn(1);// no-warning
+  my::fn(1) & other::fn(1); // no-warning
+
+  // expected-warning@+2 {{identical expressions on both sides of ':' in conditional expression}}
+  // expected-warning@+2 {{identical expressions on both sides of ':' in conditional expression}}
+  coin ? ::static_function() : ::static_function();
+  coin ? ::static_function() : static_function();
+}
Index: clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp
@@ -295,6 +295,32 @@
   return true;
 }
 
+static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
+   const NestedNameSpecifier *Right) {
+  const Type *LTy = Left->getAsType();
+  const Type *RTy = Right->getAsType();
+  if (LTy && RTy)
+return LTy->getCanonicalTypeUnqualified() ==
+   RTy->getCanonicalTypeUnqualified();
+
+  // FIXME: We should probably check the other kinds of nested name specifiers.
+  return true;
+}
+
+static bool areEquivalentDeclRefs(const DeclRefExpr *Left,
+  const DeclRefExpr *Right) {
+  if (Left->getDecl()->getCanonicalDecl() !=
+  Right->getDecl()->getCanonicalDecl()) {
+return false;
+  }
+
+  // The decls were already matched, so just return true at any later point.
+  if (!Left->hasQualifier() || !Right->hasQualifier())
+return true;
+  return areEquivalentNameSpecifier(Left->getQualifier(),
+Right->getQualifier());
+}
+
 /// Determines whether two statement trees are identical regarding
 /// operators and symbols.
 ///
@@ -458,11 +484,9 @@
 const CharacterLiteral *CharLit2 = cast(Stmt2);
 return CharLit1->getValue() == CharLit2->getValue();
   }
-  case Stmt::DeclRefExprClass: {
-const DeclRefExpr *DeclRef1 = cast(Stmt1);
-const DeclRefExpr *DeclRef2 = cast(Stmt2);
-return DeclRef1->getDecl() == DeclRef2->getDecl();
-  }
+  case Stmt::DeclRefExprClass:
+return areEquivalentDeclRefs(cast(Stmt1),
+ cast(Stmt2));
   case Stmt::IntegerLiteralClass: {
 const IntegerLiteral *IntLit1 = cast(Stmt1);
 const IntegerLiteral *IntLit2 = cast(Stmt2);
Index: clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
@@ -807,3 +807,52 @@
 };
 
 } // namespace no_crash
+
+template  struct boolean_value {
+  static constexpr bool value = V;
+};
+template  struct my_trait : boolean_

[PATCH] D114622: [clang-tidy][analyzer] Fix false-positive in IdenticalExprChecker and misc-redundant-expression

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:54-55
const NestedNameSpecifier *Right) {
-  llvm::FoldingSetNodeID LeftID, RightID;
-  Left->Profile(LeftID);
-  Right->Profile(RightID);
-  return LeftID == RightID;
+  if (const auto *L = Left->getAsType())
+if (const auto *R = Right->getAsType())
+  return L->getCanonicalTypeUnqualified() ==

whisperity wrote:
> whisperity wrote:
> > (Wrt. to the comment about not typing stuff out directly, I guess `L` and 
> > `R` here will be `X` and `X` and thus not the same.)
> (Style nit to lessen the indent depth, until C++17...)
> (Wrt. to the comment about not typing stuff out directly, I guess L and R 
> here will be X and X and thus not the same.)
I don't quite get what do you mean by this.



Comment at: 
clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp:132-134
+const DeclRefExpr *DeclRef1 = cast(Left);
+const DeclRefExpr *DeclRef2 = cast(Right);
+return areEquivalentDeclRefs(DeclRef1, DeclRef2);

whisperity wrote:
> (Style nit. Or `LeftDR`, `RightDR`.)
`DeclRef1` -> `LeftDR`



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp:822
+
+  using my_char = char;
+  sink |= my_trait::value || my_trait::value;

whisperity wrote:
> Are there any more ways we could trick this check to cause false positives? 
> Something through which it doesn't look the same but still refers to the same 
> thing?
> 
> At the top of my head I'm thinking about something along the lines of this:
> 
> ```lang=cpp
> template 
> struct X {
>   template 
>   static constexpr bool same = std::is_same_v;
> };
> 
> X ch() { return X{}; }
> X i() { return X{}; }
> 
> void test() {
>   coin ? ch().same : i().same;
> }
> ```
> 
> So the "type name" where we want to look up isn't typed out directly.
> Or would these still be caught (and thus fixed) by looking at a 
> `NestedNameSpecifier`?
For the example:
```lang=C++
template  struct integral_constant {
static constexpr T value = V;
typedef T value_type;
typedef integral_constant type;
};
typedef integral_constant true_type;
typedef integral_constant false_type;
template  struct is_same : false_type {};
template  struct is_same : true_type {};

template 
struct X {
  template 
  static constexpr bool same = is_same::value;
};

X ch();
X i();

void test(bool coin) {
  coin ? ch().same : i().same;
}
```
We would have no warnings for this code, nor we had previously:
https://godbolt.org/z/9xo4adfbP

Should I demonstrate this in the test file?
I wouldn't pollute it TBH, since it requires some template machinery which is 
quite verbose.
However, if you insist I will add it.


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

https://reviews.llvm.org/D114622

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

[PATCH] D114072: [clangd] Record IWYU pragma keep in the IncludeStructure

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

> Sorry for forgetting about this one. Hopefully I can still help now by 
> disagreeing with Kadir and creating an awkward stalemate instead.

Haha, always welcome!

> Partly because the ordering isn't some weird coincidence (see below)

Right, I suppose it makes sense when you look at the "construct being 
finalized" perspective.

> That's a more general approach for sure. But it does seem there's some 
> complexity: what is the data structure and how do we merge it back into the 
> graph. (e.g. are we going to record line numbers of each #include in the 
> graph for this merging?)

Definitely, but we are not going to get rid of that extra complexity no matter 
which solution we go with. We still need to store some extra state about these 
comments, the only difference is when we process them & whether we store just 
the latest or the whole set.
Surely processing them in a different callback then `InclusionDirective` would 
add extra complexity, but I think it might also be simpler in general since we 
don't need to think about all the sequencing of these operations.
But after looking at the particular cases, I think I agree with your suggestion 
overall. Relying on the current sequencing to keep the state incremental rather 
than whole history sounds like the better trade off here. (details below).

> So let's think about the concrete cases:
>
> - export
> - begin_exports...end_exports
> - private maybe?
>
> The TL;DR is that each of these either stand alone, or we see the directive 
> before the inclusion it describes in a way that's easy to keep track of 
> incrementally. So I'm not really seeing much implementation difficulty to 
> justify trying to move the directives and inclusion processing apart.

Yeah these are the cases I had in mind, but didn't really think about them 
thoroughly.
I guess in all of the export cases we just need to operate with the information 
we've seen before a particular inclusion directive, and the file including it. 
So there doesn't seem to be any need for postponing the processing. (Plus as 
you pointed out, keeping just the current state is definitely more simpler than 
keeping a set of those).

> Private I think we have to treat as a standalone directive that mutates the 
> current file, and the reference to another file is treated as a string. It's 
> not a file the PP has necessarily seen. So the result is a map 
> and it seems adequate for this to be written straight into IncludeStructure.

Yup, I guess this one looks pretty much the same for both approaches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114072

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

[PATCH] D114234: [clang][dataflow] Add base types for building dataflow analyses

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev updated this revision to Diff 390034.
sgatev added a comment.

Minor tweaks to documentation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114234

Files:
  clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
  clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
  clang/include/clang/Analysis/FlowSensitive/DataflowLattice.h
  clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
  clang/lib/Analysis/CMakeLists.txt
  clang/lib/Analysis/FlowSensitive/CMakeLists.txt
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===
--- /dev/null
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -0,0 +1,35 @@
+//===- TypeErasedDataflowAnalysis.cpp -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines type-erased base types and functions for building dataflow
+//  analyses that run over Control-Flow Graphs (CFGs).
+//
+//===--===//
+
+#include 
+
+#include "clang/Analysis/CFG.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h"
+#include "llvm/ADT/Optional.h"
+
+using namespace clang;
+using namespace dataflow;
+
+std::vector>
+runTypeErasedDataflowAnalysis(const CFG &Cfg,
+  TypeErasedDataflowAnalysis &Analysis,
+  const Environment &InitEnv) {
+  // FIXME: Consider enforcing that `Cfg` meets the requirements that
+  // are specified in the header. This could be done by remembering
+  // what options were used to build `Cfg` and asserting on them here.
+
+  // FIXME: Implement work list-based algorithm to compute the fixed
+  // point of `Analysis::transform` for every basic block in `Cfg`.
+  return {};
+}
Index: clang/lib/Analysis/FlowSensitive/CMakeLists.txt
===
--- /dev/null
+++ clang/lib/Analysis/FlowSensitive/CMakeLists.txt
@@ -0,0 +1,7 @@
+add_clang_library(clangAnalysisFlowSensitive
+  TypeErasedDataflowAnalysis.cpp
+
+  LINK_LIBS
+  clangAnalysis
+  clangAST
+  )
Index: clang/lib/Analysis/CMakeLists.txt
===
--- clang/lib/Analysis/CMakeLists.txt
+++ clang/lib/Analysis/CMakeLists.txt
@@ -44,3 +44,4 @@
   )
 
 add_subdirectory(plugins)
+add_subdirectory(FlowSensitive)
Index: clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
===
--- /dev/null
+++ clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
@@ -0,0 +1,93 @@
+//===- TypeErasedDataflowAnalysis.h -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//  This file defines type-erased base types and functions for building dataflow
+//  analyses that run over Control-Flow Graphs (CFGs).
+//
+//===--===//
+
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_TYPEERASEDDATAFLOWANALYSIS_H
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_TYPEERASEDDATAFLOWANALYSIS_H
+
+#include 
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Analysis/CFG.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "llvm/ADT/Any.h"
+#include "llvm/ADT/Optional.h"
+
+namespace clang {
+namespace dataflow {
+
+/// Type-erased lattice element container.
+///
+/// Requirements:
+///
+///  The type of the object stored in the container must be a bounded
+///  join-semilattice.
+struct TypeErasedLattice {
+  llvm::Any Value;
+};
+
+/// Type-erased base class for dataflow analyses built on a single lattice type.
+class TypeErasedDataflowAnalysis {
+public:
+  /// Returns the `ASTContext` that is used by the analysis.
+  virtual ASTContext &getASTContext() = 0;
+
+  /// Returns a type-erased lattice element that models the initial state of a
+  /// basic block.
+  virtual TypeErasedLattice typeErasedInitialElement() = 0;
+
+  /// Joins two type-erased lattice elements by computing their least upper
+  /// bound. 

[PATCH] D114234: [clang][dataflow] Add base types for building dataflow analyses

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added a comment.

Thanks Gábor and Dmitri!




Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:48
+/// Type-erased base class for dataflow analyses built on a single lattice 
type.
+class DataflowAnalysisDynamic {
+public:

xazax.hun wrote:
> sgatev wrote:
> > xazax.hun wrote:
> > > Does the `Dynamic` in the suffix refer to the fact this is using type 
> > > erasure as opposed to templates? 
> > > 
> > > I have multiple questions at this point:
> > > * Why do we prefer type erasure over generic programming?
> > > * Do you plan to have non-dynamic counterparts?
> > > 
> > > Nit: having Dynamic both in the class name and in the method names sounds 
> > > overly verbose to me.
> > > Nit: please add a comment what dynamic refers to in the name,
> > Right. The "Dynamic" suffix emphasizes the type-erased nature of the class 
> > and its members and is used to differentiate them from their typed 
> > counterparts in `DataflowAnalysis`. I added this to the documentation of 
> > the type. I also split the typed and type-erased interfaces in separate 
> > files. Users of the framework shouldn't need to interact with types and 
> > functions from `DataflowAnalysisDynamic.h`.
> > 
> > The names are verbose, but should be used in relatively few internal places 
> > in the framework. Currently, we have one call site for each of the methods 
> > of `DataflowAnalysisDynamic`. Nevertheless, if you have ideas for better 
> > names I'd be happy to change them.
> > 
> > The reason we went with a type-erased layer is to avoid pulling a lot of 
> > code in the templates. Over time the implementation got cleaner and as I'm 
> > preparing these patches I see some opportunities to simplify it further. 
> > However, it's still non-trivial amount of code. I think we should revisit 
> > this decision at some point and consider having entirely template-based 
> > implementation of the algorithm. I personally don't see clear benefits of 
> > one approach over the other at this point. If you have strong preference 
> > for using templates, we can consider going down that route now.
> Thanks for the explanation! I don't have a strong preference, we could stick 
> with the type-erased version unless we see some reason to change in the 
> future. However, I don't see much value in the "Dynamic" suffix for the 
> method names. What do you think about simply dropping them?
Well, the only reason I see to have the suffix is to differentiate the 
type-erased methods from the methods of the concrete dataflow analysis class 
that will derive from `DataflowAnalysis`. For example, we could have

```
class ConstantPropagationAnalysis : public DataflowAnalysis<
  ConstantPropagationAnalysis, ConstantPropagationLattice> {
public:
  ConstantPropagationLattice initialElement();

  ConstantPropagationLattice transfer(
const Stmt *, const ConstantPropagationLattice &, Environment &);
};
``` 

In this setup `ConstantPropagationAnalysis` has two methods that return the 
initial element and two methods that perform the transfer function - one pair 
in the definition and one pair inherited from `TypeErasedDataflowAnalysis`. For 
the initial element we need to use different names as the two methods differ 
only in their return types. For the transfer function we could use the same 
name and rely on overload resolution, however it seems that's not recommended - 
https://clang.llvm.org/docs/DiagnosticsReference.html#woverloaded-virtual. The 
names of the remaining methods (`joinTypeErased` and `isEqualTypeErased`) have 
the suffix for consistency.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:94
+// Model of the program at a given program point.
+template  struct DataflowAnalysisState {
+  // Model of a program property.

xazax.hun wrote:
> If I understand this correctly, this could derive from 
> `DataflowAnalysisStateDynamic`, it could just provide a getter function that 
> casts the type erased lattice element to `LatticeT`, returning a reference to 
> the contents of the `any` object. As a result, you would no longer need to do 
> move/copy in `runDataflowAnalysis`. On the other hand, the user would need to 
> call a getter to get out the lattice element. I guess we expect lattice 
> elements to be relatively cheap to move. Feel free to leave this unchanged, 
> it is more of an observation.
Acknowledged. I'll keep this option in mind. For now I suggest we keep the 
typed and type-erased structs separate if possible and if the cost isn't too 
high.



Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:102
+
+/// Performs dataflow analysis and returns a mapping from basic block IDs to
+/// dataflow analysis states that model the respective basic blocks.

xazax.hun wrote:
> While it is probably obvious to most of us, I wonder if it is obvious to all 
> future readers that the b

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon created this revision.
RKSimon added reviewers: rnk, aaron.ballman, zturner, jhenderson, Meinersbur, 
jansvoboda11, gbedwell.
Herald added subscribers: dexonsmith, mstorsjo, mgorny.
RKSimon requested review of this revision.
Herald added projects: clang, LLVM.
Herald added a subscriber: cfe-commits.

As raised here: 
https://lists.llvm.org/pipermail/llvm-dev/2021-November/153881.html

Now that VS2022 is on general release, LLVM is expected to build on VS2017, 
VS2019 and VS2022, which is proving hazardous to maintain due to changes in 
behaviour including preprocessor and constexpr changes. Plus of the few 
developers that work with VS, many have already moved to VS2019/22.

This patch proposes to raise the minimum supported version to VS2019 (16.x) - 
I've made the hard limit 16.0 or later, with the soft limit 16.11 (VS2019 
latest) - older versions os VS2019 are "allowed" (at your own risk) via the 
LLVM_FORCE_USE_OLD_TOOLCHAIN cmake flag.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114639

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/UsersManual.rst
  llvm/cmake/modules/CheckCompilerVersion.cmake
  llvm/docs/CMake.rst
  llvm/docs/GettingStarted.rst
  llvm/docs/GettingStartedVS.rst
  llvm/docs/ReleaseNotes.rst
  llvm/include/llvm/Support/Compiler.h

Index: llvm/include/llvm/Support/Compiler.h
===
--- llvm/include/llvm/Support/Compiler.h
+++ llvm/include/llvm/Support/Compiler.h
@@ -80,12 +80,21 @@
 /// * 1916: VS2017, version 15.9
 /// * 1920: VS2019, version 16.0
 /// * 1921: VS2019, version 16.1
+/// * 1922: VS2019, version 16.2
+/// * 1923: VS2019, version 16.3
+/// * 1924: VS2019, version 16.4
+/// * 1925: VS2019, version 16.5
+/// * 1926: VS2019, version 16.6
+/// * 1927: VS2019, version 16.7
+/// * 1928: VS2019, version 16.8 + 16.9
+/// * 1929: VS2019, version 16.10 + 16.11
+/// * 1930: VS2022, version 17.0
 #ifdef _MSC_VER
 #define LLVM_MSC_PREREQ(version) (_MSC_VER >= (version))
 
-// We require at least MSVC 2017.
-#if !LLVM_MSC_PREREQ(1910)
-#error LLVM requires at least MSVC 2017.
+// We require at least VS 2019.
+#if !LLVM_MSC_PREREQ(1920)
+#error LLVM requires at least VS 2019.
 #endif
 
 #else
@@ -97,12 +106,7 @@
 /// Sadly, this is separate from just rvalue reference support because GCC
 /// and MSVC implemented this later than everything else. This appears to be
 /// corrected in MSVC 2019 but not MSVC 2017.
-#if __has_feature(cxx_rvalue_references) || defined(__GNUC__) ||   \
-LLVM_MSC_PREREQ(1920)
 #define LLVM_HAS_RVALUE_REFERENCE_THIS 1
-#else
-#define LLVM_HAS_RVALUE_REFERENCE_THIS 0
-#endif
 
 /// Expands to '&' if ref-qualifiers for *this are supported.
 ///
Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -65,7 +65,7 @@
 Changes to building LLVM
 
 
-* ...
+* Building LLVM with Visual Studio now requires version 2019 or later.
 
 Changes to TableGen
 ---
Index: llvm/docs/GettingStartedVS.rst
===
--- llvm/docs/GettingStartedVS.rst
+++ llvm/docs/GettingStartedVS.rst
@@ -36,7 +36,7 @@
 
 Hardware
 
-Any system that can adequately run Visual Studio 2017 is fine. The LLVM
+Any system that can adequately run Visual Studio 2019 is fine. The LLVM
 source tree including the git index consumes approximately 3GB.
 Object files, libraries and executables consume approximately 5GB in
 Release mode and much more in Debug mode. SSD drive and >16GB RAM are
@@ -45,7 +45,7 @@
 
 Software
 
-You will need `Visual Studio `_ 2017 or
+You will need `Visual Studio `_ 2019 or
 higher, with the latest Update installed. Visual Studio Community Edition
 suffices.
 
Index: llvm/docs/GettingStarted.rst
===
--- llvm/docs/GettingStarted.rst
+++ llvm/docs/GettingStarted.rst
@@ -238,7 +238,7 @@
 * Clang 3.5
 * Apple Clang 6.0
 * GCC 5.1
-* Visual Studio 2017
+* Visual Studio 2019
 
 Anything older than these toolchains *may* work, but will require forcing the
 build system with a special option and is not really a supported host platform.
@@ -273,7 +273,7 @@
 This section mostly applies to Linux and older BSDs. On macOS, you should
 have a sufficiently modern Xcode, or you will likely need to upgrade until you
 do. Windows does not have a "system compiler", so you must install either Visual
-Studio 2017 or a recent version of mingw64. FreeBSD 10.0 and newer have a modern
+Studio 2019 or a recent version of mingw64. FreeBSD 10.0 and newer have a modern
 Clang as the system compiler.
 
 However, some Linux distributions and some other or older BSDs sometimes have
Index: llvm/docs/CMake.rst
=

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.



Comment at: clang/docs/UsersManual.rst:3546
 
-cmake -G"Visual Studio 15 2017" -T LLVM ..
+cmake -G"Visual Studio 16 2019" -T LLVM ..
 

Maybe make this VS2022 instead, to help it last longer?



Comment at: llvm/docs/GettingStarted.rst:276
 do. Windows does not have a "system compiler", so you must install either 
Visual
-Studio 2017 or a recent version of mingw64. FreeBSD 10.0 and newer have a 
modern
+Studio 2019 or a recent version of mingw64. FreeBSD 10.0 and newer have a 
modern
 Clang as the system compiler.

Perhaps add "or Visual Studio 2022" or similar?



Comment at: llvm/include/llvm/Support/Compiler.h:106-108
 /// Sadly, this is separate from just rvalue reference support because GCC
 /// and MSVC implemented this later than everything else. This appears to be
 /// corrected in MSVC 2019 but not MSVC 2017.

Does this comment need changing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114639

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

[PATCH] D113558: [clang-tidy] Fix cppcoreguidelines-virtual-base-class-destructor in macros

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D113558#3154967 , @whisperity 
wrote:

> I'm not sure if it is a good idea to merge things on a Friday, but I am 
> comfortable with this, let's get the check crash less.

I've run this on a few projects and it did not introduce new crashes. I plan to 
commit this on Monday.


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

https://reviews.llvm.org/D113558

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

[PATCH] D114522: [clangd] Add desugared type to hover

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This looks pretty good in terms of behavior, some impl comments but we should 
land this.

My main remaining concern is just that this will trigger too often when there's 
no confusion over types, and clutter the display.
Some common cases I'm worried about:

- std::string -> basic_string. Maybe we should special-case this to hide 
the aka?
- `int64_t` vs `long long` et al. I have no idea what to do about this.

@kadircet hopefully has thoughts on what hover should look like




Comment at: clang-tools-extra/clangd/Hover.cpp:142
 
-std::string printType(QualType QT, const PrintingPolicy &PP) {
+HoverInfo::PrintedType printType(QualType QT, ASTContext &Context,
+ const PrintingPolicy &PP) {

nit: ASTCtx or Ctx to distinguish from clangd::Context



Comment at: clang-tools-extra/clangd/Hover.cpp:149
 QT = QT->getAs()->getUnderlyingType();
   std::string Result;
   llvm::raw_string_ostream OS(Result);

nit: declare a `HoverInfo::PrintedType Result` and write directly into it 
instead of these partial variables?



Comment at: clang-tools-extra/clangd/Hover.cpp:160
   OS.flush();
-  QT.print(OS, PP);
-  return Result;
+  std::string Type = Result + QT.getAsString(PP);
+  llvm::Optional AKA;

prefer printing into the same string rather than copying into a new one



Comment at: clang-tools-extra/clangd/Hover.cpp:166
+if (ShouldAKA)
+  AKA = Result + DesugaredTy.getAsString(PP);
+  }

The e.g. "class " prefix isn't needed in the a.k.a, so no need to copy strings 
for that.

(and in fact I'm not sure it's possible to hit an AKA when this string is 
nonempty)



Comment at: clang-tools-extra/clangd/Hover.cpp:189
 
-std::string printType(const TemplateTemplateParmDecl *TTP,
-  const PrintingPolicy &PP) {
-  std::string Res;
-  llvm::raw_string_ostream OS(Res);
-  OS << "template <";
-  llvm::StringRef Sep = "";
+HoverInfo::PrintedType printType(const TemplateTemplateParmDecl *TTP,
+ const PrintingPolicy &PP) {

How strongly do you feel about handling this case?

This code is much more surprising than the previous version and maybe we can 
find a simpler way to express it, but all of this only matters if we have a 
template-template-paramater with a non-type-template-parameter with nontrivial 
sugar which obscures meaning. 

That seems pretty unlikely to ever come up, and if it does I'm not sure 
repeating the whole TTP is a great experience.

I'd suggest just dropping the possibility of an AKA type here until someone 
runs into a case where it's a real issue...



Comment at: clang-tools-extra/clangd/Hover.cpp:689
+  auto PType = printType(PrettyThisType, ASTCtx, PP);
+  HI.Definition = llvm::formatv("{0}", PType.Type);
+  if (PType.AKA)

HI.Definition = std::move(PType.Type)



Comment at: clang-tools-extra/clangd/Hover.cpp:689
+  auto PType = printType(PrettyThisType, ASTCtx, PP);
+  HI.Definition = llvm::formatv("{0}", PType.Type);
+  if (PType.AKA)

sammccall wrote:
> HI.Definition = std::move(PType.Type)
this formatting of the definition appears twice, pull out a function? `string 
typeAsDefinition(PrintedType)` or so



Comment at: clang-tools-extra/clangd/Hover.cpp:1093
 // - `int param2 = 5`
-Output.addParagraph().appendText("→ ").appendCode(*ReturnType);
+std::string Buffer;
+llvm::raw_string_ostream OS(Buffer);

appendCode(llvm::to_string(ReturnType))

(from ScopedPrinter.h)



Comment at: clang-tools-extra/clangd/Hover.cpp:1271
 OS << " = " << *P.Default;
+  if (P.Type && P.Type->AKA)
+OS << llvm::formatv(" (aka {0})", *P.Type->AKA);

Hmm, this seems to render as:

```std::string Foo = "" (aka basic_string)```

it's not *completely* clear what the aka refers to.
I don't have a better solution, though.
@kadircet any opinions here?



Comment at: clang-tools-extra/clangd/Hover.cpp:166
+if (ShouldAKA)
+  AKA = Result + DesugaredTy.getAsString(PP);
+  }

lh123 wrote:
> lh123 wrote:
> > lh123 wrote:
> > > It seems we lost `namespace qualifiers`.
> > > It always display `std::string` as `basic_string`
> > Should we set `FullyQualifiedName` printing policy  for `DesugaredTy`.
> Sometimes we need to distinguish certain types with the same name based on 
> the `fully qualified name`.
> eg.
> ```
> namespace custom {
> template  struct vector {};
> } // namespace custom
> 
> void code() {
>   custom::vector a;
>   std::vector b;
> }
> ```
> Currently, both `a` and `b` show `vector`.
I'd lean against setting FullyQualifiedName for the AKA type. Clang doesn't for 
diagnostics (I see `'std::string' aka 'basic_string'`).
It is more specific and adds v

[PATCH] D114256: [clang-tidy] Fix crashing altera-struct-pack-align on invalid RecordDecls

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I plan to commit this stack on Monday.


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

https://reviews.llvm.org/D114256

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

[PATCH] D114602: [clang-tidy][docs][NFC] Improve documentation of bugprone-unhandled-exception-at-new

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

balazske updated this revision to Diff 390048.
balazske added a comment.

Apply of review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114602

Files:
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst


Index: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst
===
--- 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst
+++ 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst
@@ -5,21 +5,51 @@
 
 Finds calls to ``new`` with missing exception handler for ``std::bad_alloc``.
 
+Calls to ``new`` may throw exceptions of type ``std::bad_alloc`` that should
+be handled. Alternatively, the nonthrowing form of ``new`` can be
+used. The check verifies that the exception is handled in the function
+that calls ``new``.
+
+If a nonthrowing version is used or the exception is allowed to propagate out
+of the function no warning is generated.
+
+The exception handler is checked if it catches the ``std::bad_alloc``, or
+``std::exception`` exception types, or all exceptions (catch-all).
+The check assumes that any user-defined ``operator new`` is either
+``noexcept`` or may throw an exception of type ``std::bad_alloc`` (or one
+derived from it). Other exception class types are not taken into account.
+
 .. code-block:: c++
 
   int *f() noexcept {
-int *p = new int[1000];
+int *p = new int[1000]; // warning: missing exception handler for 
allocation failure at 'new'
+// ...
+return p;
+  }
+
+.. code-block:: c++
+
+  int *f1() { // not 'noexcept'
+int *p = new int[1000]; // no warning: exception can be handled outside
+// of this function
+// ...
+return p;
+  }
+
+  int *f2() noexcept {
+try {
+  int *p = new int[1000]; // no warning: exception is handled
+  // ...
+  return p;
+} catch (std::bad_alloc &) {
+  // ...
+}
+// ...
+  }
+
+  int *f3() noexcept {
+int *p = new (std::nothrow) int[1000]; // no warning: "nothrow" is used
 // ...
 return p;
   }
 
-Calls to ``new`` can throw exceptions of type ``std::bad_alloc`` that should
-be handled by the code. Alternatively, the nonthrowing form of ``new`` can be
-used. The check verifies that the exception is handled in the function
-that calls ``new``, unless a nonthrowing version is used or the exception
-is allowed to propagate out of the function (exception handler is checked for
-types ``std::bad_alloc``, ``std::exception``, and catch-all handler).
-The check assumes that any user-defined ``operator new`` is either
-``noexcept`` or may throw an exception of type ``std::bad_alloc`` (or derived
-from it). Other exception types or exceptions occurring in the object's
-constructor are not taken into account.


Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst
@@ -5,21 +5,51 @@
 
 Finds calls to ``new`` with missing exception handler for ``std::bad_alloc``.
 
+Calls to ``new`` may throw exceptions of type ``std::bad_alloc`` that should
+be handled. Alternatively, the nonthrowing form of ``new`` can be
+used. The check verifies that the exception is handled in the function
+that calls ``new``.
+
+If a nonthrowing version is used or the exception is allowed to propagate out
+of the function no warning is generated.
+
+The exception handler is checked if it catches the ``std::bad_alloc``, or
+``std::exception`` exception types, or all exceptions (catch-all).
+The check assumes that any user-defined ``operator new`` is either
+``noexcept`` or may throw an exception of type ``std::bad_alloc`` (or one
+derived from it). Other exception class types are not taken into account.
+
 .. code-block:: c++
 
   int *f() noexcept {
-int *p = new int[1000];
+int *p = new int[1000]; // warning: missing exception handler for allocation failure at 'new'
+// ...
+return p;
+  }
+
+.. code-block:: c++
+
+  int *f1() { // not 'noexcept'
+int *p = new int[1000]; // no warning: exception can be handled outside
+// of this function
+// ...
+return p;
+  }
+
+  int *f2() noexcept {
+try {
+  int *p = new int[1000]; // no warning: exception is handled
+  // ...
+  return p;
+} catch (std::bad_alloc &) {
+  // ...
+}
+// ...
+  }
+
+  int *f3() noexcept {
+int *p = new (std::nothrow) int[1000]; // no warning: "nothrow" is used
 // ...
 return p;
   }
 
-Calls to ``new`` can throw exceptions of type ``std::bad_alloc`` that should
-be handled by the code. Al

[PATCH] D114602: [clang-tidy][docs][NFC] Improve documentation of bugprone-unhandled-exception-at-new

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

balazske marked 5 inline comments as done.
balazske added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst:19-20
 The check assumes that any user-defined ``operator new`` is either
 ``noexcept`` or may throw an exception of type ``std::bad_alloc`` (or derived
 from it). Other exception types or exceptions occurring in the object's
 constructor are not taken into account.

whisperity wrote:
> What does
> 
> > exception types or exceptions
> 
> mean?
Now it is probably better. "Other exception" is any thrown exception of other 
class than the mentioned ones.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst:26
+  int *f() noexcept {
+int *p = new int[1000]; // warning: exception handler is missing
+// ...

whisperity wrote:
> Is that the warning message the check prints?
Warning text is changed to the exact checker's message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114602

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

[PATCH] D114623: [clangd] IncludeCleaner: Attribute symbols from non self-contained headers to their parents

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 390051.
kbobyrev marked 2 inline comments as done.
kbobyrev added a comment.

Address review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114623

Files:
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -281,7 +281,8 @@
   auto &SM = AST.getSourceManager();
   auto &Includes = AST.getIncludeStructure();
 
-  auto ReferencedFiles = findReferencedFiles(findReferencedLocations(AST), SM);
+  auto ReferencedFiles =
+  findReferencedFiles(findReferencedLocations(AST), Includes, SM);
   llvm::StringSet<> ReferencedFileNames;
   for (FileID FID : ReferencedFiles)
 ReferencedFileNames.insert(
@@ -303,6 +304,80 @@
   EXPECT_THAT(getUnused(AST, ReferencedHeaders), ::testing::IsEmpty());
 }
 
+TEST(IncludeCleaner, DistinctUnguardedInclusions) {
+  TestTU TU;
+  TU.Code = R"cpp(
+#include "bar.h"
+#include "foo.h"
+
+int LocalFoo = foo::Variable;
+)cpp";
+  TU.AdditionalFiles["foo.h"] = R"cpp(
+#pragma once
+namespace foo {
+#include "unguarded.h"
+}
+)cpp";
+  TU.AdditionalFiles["bar.h"] = R"cpp(
+#pragma once
+namespace bar {
+#include "unguarded.h"
+}
+)cpp";
+  TU.AdditionalFiles["unguarded.h"] = R"cpp(
+constexpr int Variable = 42;
+)cpp";
+
+  ParsedAST AST = TU.build();
+
+  auto ReferencedFiles =
+  findReferencedFiles(findReferencedLocations(AST),
+  AST.getIncludeStructure(), AST.getSourceManager());
+  llvm::StringSet<> ReferencedFileNames;
+  auto &SM = AST.getSourceManager();
+  for (FileID FID : ReferencedFiles)
+ReferencedFileNames.insert(
+SM.getPresumedLoc(SM.getLocForStartOfFile(FID)).getFilename());
+  // Note that we have uplifted the referenced files from non self-contained
+  // headers to header-guarded ones.
+  EXPECT_THAT(ReferencedFileNames.keys(),
+  UnorderedElementsAre(testPath("foo.h")));
+}
+
+TEST(IncludeCleaner, NonSelfContainedHeaders) {
+  TestTU TU;
+  TU.Code = R"cpp(
+#include "foo.h"
+
+int LocalFoo = Variable;
+)cpp";
+  TU.AdditionalFiles["foo.h"] = R"cpp(
+#pragma once
+#include "indirection.h"
+)cpp";
+  TU.AdditionalFiles["indirection.h"] = R"cpp(
+#include "unguarded.h"
+)cpp";
+  TU.AdditionalFiles["unguarded.h"] = R"cpp(
+constexpr int Variable = 42;
+)cpp";
+
+  ParsedAST AST = TU.build();
+
+  auto ReferencedFiles =
+  findReferencedFiles(findReferencedLocations(AST),
+  AST.getIncludeStructure(), AST.getSourceManager());
+  llvm::StringSet<> ReferencedFileNames;
+  auto &SM = AST.getSourceManager();
+  for (FileID FID : ReferencedFiles)
+ReferencedFileNames.insert(
+SM.getPresumedLoc(SM.getLocForStartOfFile(FID)).getFilename());
+  // Note that we have uplifted the referenced files from non self-contained
+  // headers to header-guarded ones.
+  EXPECT_THAT(ReferencedFileNames.keys(),
+  UnorderedElementsAre(testPath("foo.h")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/IncludeCleaner.h
===
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -52,6 +52,7 @@
 /// The output only includes things SourceManager sees as files (not macro IDs).
 /// This can include ,  etc that are not true files.
 llvm::DenseSet findReferencedFiles(const ReferencedLocations &Locs,
+   const IncludeStructure &Includes,
const SourceManager &SM);
 
 /// Maps FileIDs to the internal IncludeStructure representation (HeaderIDs).
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -8,6 +8,7 @@
 
 #include "IncludeCleaner.h"
 #include "Config.h"
+#include "Headers.h"
 #include "ParsedAST.h"
 #include "Protocol.h"
 #include "SourceCode.h"
@@ -16,6 +17,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/Syntax/Tokens.h"
@@ -221,6 +223,27 @@
   return true;
 }
 
+FileID headerResponsible(FileID ID, const SourceManager &SM,
+ const IncludeStructure &Includes) {
+  //

[PATCH] D114623: [clangd] IncludeCleaner: Attribute symbols from non self-contained headers to their parents

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 390052.
kbobyrev added a comment.

Add some docs for the helper function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114623

Files:
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -281,7 +281,8 @@
   auto &SM = AST.getSourceManager();
   auto &Includes = AST.getIncludeStructure();
 
-  auto ReferencedFiles = findReferencedFiles(findReferencedLocations(AST), SM);
+  auto ReferencedFiles =
+  findReferencedFiles(findReferencedLocations(AST), Includes, SM);
   llvm::StringSet<> ReferencedFileNames;
   for (FileID FID : ReferencedFiles)
 ReferencedFileNames.insert(
@@ -303,6 +304,80 @@
   EXPECT_THAT(getUnused(AST, ReferencedHeaders), ::testing::IsEmpty());
 }
 
+TEST(IncludeCleaner, DistinctUnguardedInclusions) {
+  TestTU TU;
+  TU.Code = R"cpp(
+#include "bar.h"
+#include "foo.h"
+
+int LocalFoo = foo::Variable;
+)cpp";
+  TU.AdditionalFiles["foo.h"] = R"cpp(
+#pragma once
+namespace foo {
+#include "unguarded.h"
+}
+)cpp";
+  TU.AdditionalFiles["bar.h"] = R"cpp(
+#pragma once
+namespace bar {
+#include "unguarded.h"
+}
+)cpp";
+  TU.AdditionalFiles["unguarded.h"] = R"cpp(
+constexpr int Variable = 42;
+)cpp";
+
+  ParsedAST AST = TU.build();
+
+  auto ReferencedFiles =
+  findReferencedFiles(findReferencedLocations(AST),
+  AST.getIncludeStructure(), AST.getSourceManager());
+  llvm::StringSet<> ReferencedFileNames;
+  auto &SM = AST.getSourceManager();
+  for (FileID FID : ReferencedFiles)
+ReferencedFileNames.insert(
+SM.getPresumedLoc(SM.getLocForStartOfFile(FID)).getFilename());
+  // Note that we have uplifted the referenced files from non self-contained
+  // headers to header-guarded ones.
+  EXPECT_THAT(ReferencedFileNames.keys(),
+  UnorderedElementsAre(testPath("foo.h")));
+}
+
+TEST(IncludeCleaner, NonSelfContainedHeaders) {
+  TestTU TU;
+  TU.Code = R"cpp(
+#include "foo.h"
+
+int LocalFoo = Variable;
+)cpp";
+  TU.AdditionalFiles["foo.h"] = R"cpp(
+#pragma once
+#include "indirection.h"
+)cpp";
+  TU.AdditionalFiles["indirection.h"] = R"cpp(
+#include "unguarded.h"
+)cpp";
+  TU.AdditionalFiles["unguarded.h"] = R"cpp(
+constexpr int Variable = 42;
+)cpp";
+
+  ParsedAST AST = TU.build();
+
+  auto ReferencedFiles =
+  findReferencedFiles(findReferencedLocations(AST),
+  AST.getIncludeStructure(), AST.getSourceManager());
+  llvm::StringSet<> ReferencedFileNames;
+  auto &SM = AST.getSourceManager();
+  for (FileID FID : ReferencedFiles)
+ReferencedFileNames.insert(
+SM.getPresumedLoc(SM.getLocForStartOfFile(FID)).getFilename());
+  // Note that we have uplifted the referenced files from non self-contained
+  // headers to header-guarded ones.
+  EXPECT_THAT(ReferencedFileNames.keys(),
+  UnorderedElementsAre(testPath("foo.h")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/IncludeCleaner.h
===
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -52,6 +52,7 @@
 /// The output only includes things SourceManager sees as files (not macro IDs).
 /// This can include ,  etc that are not true files.
 llvm::DenseSet findReferencedFiles(const ReferencedLocations &Locs,
+   const IncludeStructure &Includes,
const SourceManager &SM);
 
 /// Maps FileIDs to the internal IncludeStructure representation (HeaderIDs).
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -8,6 +8,7 @@
 
 #include "IncludeCleaner.h"
 #include "Config.h"
+#include "Headers.h"
 #include "ParsedAST.h"
 #include "Protocol.h"
 #include "SourceCode.h"
@@ -16,6 +17,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/Syntax/Tokens.h"
@@ -221,6 +223,31 @@
   return true;
 }
 
+// In case symbols are coming from non self-contained header, we need to find
+// its first includer that is self-contained. This is the header users can
+//

[clang-tools-extra] 34cc210 - [clangd] IncludeCleaner: Attribute symbols from non self-contained headers to their parents

[Kirill Bobyrev via cfe-commits]

Author: Kirill Bobyrev
Date: 2021-11-26T16:20:48+01:00
New Revision: 34cc210aa8af2fd33598e5559d0f5b51f9423dd6

URL: 
https://github.com/llvm/llvm-project/commit/34cc210aa8af2fd33598e5559d0f5b51f9423dd6
DIFF: 
https://github.com/llvm/llvm-project/commit/34cc210aa8af2fd33598e5559d0f5b51f9423dd6.diff

LOG: [clangd] IncludeCleaner: Attribute symbols from non self-contained headers 
to their parents

When a symbol comes from the non self-contained header, we recursively uplift
the file we consider used to the first includer that has a header guard. We
need to do this while we still have FileIDs because every time a non
self-contained header is included, it gets a new FileID but is later
deduplicated by HeaderID and it's not possible to understand where it was
included from.

Based on D114370.

Reviewed By: sammccall

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

Added: 


Modified: 
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/IncludeCleaner.h
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/IncludeCleaner.cpp 
b/clang-tools-extra/clangd/IncludeCleaner.cpp
index a0b9e303e3bfe..33964d9f83ce0 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -8,6 +8,7 @@
 
 #include "IncludeCleaner.h"
 #include "Config.h"
+#include "Headers.h"
 #include "ParsedAST.h"
 #include "Protocol.h"
 #include "SourceCode.h"
@@ -16,6 +17,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/Syntax/Tokens.h"
@@ -221,6 +223,31 @@ bool mayConsiderUnused(const Inclusion &Inc, ParsedAST 
&AST) {
   return true;
 }
 
+// In case symbols are coming from non self-contained header, we need to find
+// its first includer that is self-contained. This is the header users can
+// include, so it will be responsible for bringing the symbols from given
+// header into the scope.
+FileID headerResponsible(FileID ID, const SourceManager &SM,
+ const IncludeStructure &Includes) {
+  // Unroll the chain of non self-contained headers until we find the one that
+  // can be included.
+  for (const FileEntry *FE = SM.getFileEntryForID(ID); ID != 
SM.getMainFileID();
+   FE = SM.getFileEntryForID(ID)) {
+// If FE is nullptr, we consider it to be the responsible header.
+if (!FE)
+  break;
+auto HID = Includes.getID(FE);
+assert(HID && "We're iterating over headers already existing in "
+  "IncludeStructure");
+if (Includes.isSelfContained(*HID))
+  break;
+// The header is not self-contained: put the responsibility for its symbols
+// on its includer.
+ID = SM.getFileID(SM.getIncludeLoc(ID));
+  }
+  return ID;
+}
+
 } // namespace
 
 ReferencedLocations findReferencedLocations(ParsedAST &AST) {
@@ -234,20 +261,27 @@ ReferencedLocations findReferencedLocations(ParsedAST 
&AST) {
 
 llvm::DenseSet
 findReferencedFiles(const llvm::DenseSet &Locs,
-const SourceManager &SM) {
+const IncludeStructure &Includes, const SourceManager &SM) 
{
   std::vector Sorted{Locs.begin(), Locs.end()};
   llvm::sort(Sorted); // Group by FileID.
-  ReferencedFiles Result(SM);
+  ReferencedFiles Files(SM);
   for (auto It = Sorted.begin(); It < Sorted.end();) {
 FileID FID = SM.getFileID(*It);
-Result.add(FID, *It);
+Files.add(FID, *It);
 // Cheaply skip over all the other locations from the same FileID.
 // This avoids lots of redundant Loc->File lookups for the same file.
 do
   ++It;
 while (It != Sorted.end() && SM.isInFileID(*It, FID));
   }
-  return std::move(Result.Files);
+  // If a header is not self-contained, we consider its symbols a logical part
+  // of the including file. Therefore, mark the parents of all used
+  // non-self-contained FileIDs as used. Perform this on FileIDs rather than
+  // HeaderIDs, as each inclusion of a non-self-contained file is distinct.
+  llvm::DenseSet Result;
+  for (FileID ID : Files.Files)
+Result.insert(headerResponsible(ID, SM, Includes));
+  return Result;
 }
 
 std::vector
@@ -304,12 +338,8 @@ std::vector 
computeUnusedIncludes(ParsedAST &AST) {
   const auto &SM = AST.getSourceManager();
 
   auto Refs = findReferencedLocations(AST);
-  // FIXME(kirillbobyrev): Attribute the symbols from non self-contained
-  // headers to their parents while the information about respective
-  // SourceLocations and FileIDs is not lost. It is not possible to do that
-  // later when non-guarded headers included multiple times will get same
-  // HeaderID.
-  auto ReferencedFileIDs = findReferencedFiles(Refs, SM);
+  auto ReferencedFile

[PATCH] D114623: [clangd] IncludeCleaner: Attribute symbols from non self-contained headers to their parents

[Kirill Bobyrev via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG34cc210aa8af: [clangd] IncludeCleaner: Attribute symbols 
from non self-contained headers to… (authored by kbobyrev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114623

Files:
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -281,7 +281,8 @@
   auto &SM = AST.getSourceManager();
   auto &Includes = AST.getIncludeStructure();
 
-  auto ReferencedFiles = findReferencedFiles(findReferencedLocations(AST), SM);
+  auto ReferencedFiles =
+  findReferencedFiles(findReferencedLocations(AST), Includes, SM);
   llvm::StringSet<> ReferencedFileNames;
   for (FileID FID : ReferencedFiles)
 ReferencedFileNames.insert(
@@ -303,6 +304,80 @@
   EXPECT_THAT(getUnused(AST, ReferencedHeaders), ::testing::IsEmpty());
 }
 
+TEST(IncludeCleaner, DistinctUnguardedInclusions) {
+  TestTU TU;
+  TU.Code = R"cpp(
+#include "bar.h"
+#include "foo.h"
+
+int LocalFoo = foo::Variable;
+)cpp";
+  TU.AdditionalFiles["foo.h"] = R"cpp(
+#pragma once
+namespace foo {
+#include "unguarded.h"
+}
+)cpp";
+  TU.AdditionalFiles["bar.h"] = R"cpp(
+#pragma once
+namespace bar {
+#include "unguarded.h"
+}
+)cpp";
+  TU.AdditionalFiles["unguarded.h"] = R"cpp(
+constexpr int Variable = 42;
+)cpp";
+
+  ParsedAST AST = TU.build();
+
+  auto ReferencedFiles =
+  findReferencedFiles(findReferencedLocations(AST),
+  AST.getIncludeStructure(), AST.getSourceManager());
+  llvm::StringSet<> ReferencedFileNames;
+  auto &SM = AST.getSourceManager();
+  for (FileID FID : ReferencedFiles)
+ReferencedFileNames.insert(
+SM.getPresumedLoc(SM.getLocForStartOfFile(FID)).getFilename());
+  // Note that we have uplifted the referenced files from non self-contained
+  // headers to header-guarded ones.
+  EXPECT_THAT(ReferencedFileNames.keys(),
+  UnorderedElementsAre(testPath("foo.h")));
+}
+
+TEST(IncludeCleaner, NonSelfContainedHeaders) {
+  TestTU TU;
+  TU.Code = R"cpp(
+#include "foo.h"
+
+int LocalFoo = Variable;
+)cpp";
+  TU.AdditionalFiles["foo.h"] = R"cpp(
+#pragma once
+#include "indirection.h"
+)cpp";
+  TU.AdditionalFiles["indirection.h"] = R"cpp(
+#include "unguarded.h"
+)cpp";
+  TU.AdditionalFiles["unguarded.h"] = R"cpp(
+constexpr int Variable = 42;
+)cpp";
+
+  ParsedAST AST = TU.build();
+
+  auto ReferencedFiles =
+  findReferencedFiles(findReferencedLocations(AST),
+  AST.getIncludeStructure(), AST.getSourceManager());
+  llvm::StringSet<> ReferencedFileNames;
+  auto &SM = AST.getSourceManager();
+  for (FileID FID : ReferencedFiles)
+ReferencedFileNames.insert(
+SM.getPresumedLoc(SM.getLocForStartOfFile(FID)).getFilename());
+  // Note that we have uplifted the referenced files from non self-contained
+  // headers to header-guarded ones.
+  EXPECT_THAT(ReferencedFileNames.keys(),
+  UnorderedElementsAre(testPath("foo.h")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/IncludeCleaner.h
===
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -52,6 +52,7 @@
 /// The output only includes things SourceManager sees as files (not macro IDs).
 /// This can include ,  etc that are not true files.
 llvm::DenseSet findReferencedFiles(const ReferencedLocations &Locs,
+   const IncludeStructure &Includes,
const SourceManager &SM);
 
 /// Maps FileIDs to the internal IncludeStructure representation (HeaderIDs).
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -8,6 +8,7 @@
 
 #include "IncludeCleaner.h"
 #include "Config.h"
+#include "Headers.h"
 #include "ParsedAST.h"
 #include "Protocol.h"
 #include "SourceCode.h"
@@ -16,6 +17,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/Syntax/Tokens.h"
@@ -221,6 +223,31 @@
   return true;
 }
 
+// I

[PATCH] D88299: [clang-format] Add MacroUnexpander.

[Manuel Klimek via Phabricator via cfe-commits]

klimek updated this revision to Diff 390057.
klimek marked 49 inline comments as done.
klimek added a comment.

Work in review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88299

Files:
  clang/lib/Format/CMakeLists.txt
  clang/lib/Format/FormatToken.h
  clang/lib/Format/MacroCallReconstructor.cpp
  clang/lib/Format/Macros.h
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/CMakeLists.txt
  clang/unittests/Format/MacroCallReconstructorTest.cpp

Index: clang/unittests/Format/MacroCallReconstructorTest.cpp
===
--- /dev/null
+++ clang/unittests/Format/MacroCallReconstructorTest.cpp
@@ -0,0 +1,688 @@
+#include "../../lib/Format/Macros.h"
+#include "../../lib/Format/UnwrappedLineParser.h"
+#include "TestLexer.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+
+namespace clang {
+namespace format {
+namespace {
+
+using UnexpandedMap =
+llvm::DenseMap>;
+
+// Keeps track of a sequence of macro expansions.
+//
+// The expanded tokens are accessible via getTokens(), while a map of macro call
+// identifier token to unexpanded token stream is accessible via
+// getUnexpanded().
+class Expansion {
+public:
+  Expansion(TestLexer &Lex, MacroExpander &Macros) : Lex(Lex), Macros(Macros) {}
+
+  // Appends the token stream obtained from expanding the macro Name given
+  // the provided arguments, to be later retrieved with getTokens().
+  // Returns the list of tokens making up the unexpanded macro call.
+  TokenList
+  expand(llvm::StringRef Name,
+ const SmallVector, 1> &Args) {
+auto *ID = Lex.id(Name);
+auto UnexpandedLine = std::make_unique();
+UnexpandedLine->Tokens.push_back(ID);
+if (!Args.empty()) {
+  UnexpandedLine->Tokens.push_back(Lex.id("("));
+  for (auto I = Args.begin(), E = Args.end(); I != E; ++I) {
+if (I != Args.begin())
+  UnexpandedLine->Tokens.push_back(Lex.id(","));
+UnexpandedLine->Tokens.insert(UnexpandedLine->Tokens.end(), I->begin(),
+  I->end());
+  }
+  UnexpandedLine->Tokens.push_back(Lex.id(")"));
+}
+Unexpanded[ID] = std::move(UnexpandedLine);
+
+auto Expanded = uneof(Macros.expand(ID, Args));
+Tokens.append(Expanded.begin(), Expanded.end());
+
+TokenList UnexpandedTokens;
+for (const UnwrappedLineNode &Node : Unexpanded[ID]->Tokens) {
+  UnexpandedTokens.push_back(Node.Tok);
+}
+return UnexpandedTokens;
+  }
+
+  TokenList expand(llvm::StringRef Name,
+   const std::vector &Args = {}) {
+return expand(Name, lexArgs(Args));
+  }
+
+  const UnexpandedMap &getUnexpanded() const { return Unexpanded; }
+
+  const TokenList &getTokens() const { return Tokens; }
+
+private:
+  llvm::SmallVector
+  lexArgs(const std::vector &Args) {
+llvm::SmallVector Result;
+for (const auto &Arg : Args) {
+  Result.push_back(uneof(Lex.lex(Arg)));
+}
+return Result;
+  }
+  llvm::DenseMap> Unexpanded;
+  llvm::SmallVector Tokens;
+  TestLexer &Lex;
+  MacroExpander &Macros;
+};
+
+struct Chunk {
+  Chunk(llvm::ArrayRef Tokens)
+  : Tokens(Tokens.begin(), Tokens.end()) {}
+  Chunk(llvm::ArrayRef Children)
+  : Children(Children.begin(), Children.end()) {}
+  llvm::SmallVector Tokens;
+  llvm::SmallVector Children;
+};
+
+bool tokenMatches(const FormatToken *Left, const FormatToken *Right) {
+  if (Left->getType() == Right->getType() &&
+  Left->TokenText == Right->TokenText)
+return true;
+  llvm::dbgs() << Left->TokenText << " != " << Right->TokenText << "\n";
+  return false;
+}
+
+// Allows to produce chunks of a token list by typing the code of equal tokens.
+//
+// Created from a list of tokens, users call "consume" to get the next chunk
+// of tokens, checking that they match the written code.
+struct Matcher {
+  Matcher(const TokenList &Tokens, TestLexer &Lex)
+  : Tokens(Tokens), It(this->Tokens.begin()), Lex(Lex) {}
+
+  Chunk consume(StringRef Tokens) {
+TokenList Result;
+for (const FormatToken *Token : uneof(Lex.lex(Tokens))) {
+  assert(tokenMatches(*It, Token));
+  Result.push_back(*It);
+  ++It;
+}
+return Chunk(Result);
+  }
+
+  TokenList Tokens;
+  TokenList::iterator It;
+  TestLexer &Lex;
+};
+
+UnexpandedMap mergeUnexpanded(const UnexpandedMap &M1,
+  const UnexpandedMap &M2) {
+  UnexpandedMap Result;
+  for (const auto &KV : M1) {
+Result[KV.first] = std::make_unique(*KV.second);
+  }
+  for (const auto &KV : M2) {
+Result[KV.first] = std::make_unique(*KV.second);
+  }
+  return Result;
+}
+
+class MacroCallReconstructorTest : public ::testing::Test {
+public:
+  MacroCallReconstructorTest() : Lex(Allocator, Buffer

[PATCH] D114553: [HIP] Add atomic load, atomic store and atomic cmpxchng_weak builtin support in HIP-clang

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.



Comment at: clang/test/CodeGenCUDA/atomic-ops.cu:14
+// CHECK: load atomic i32, i32* {{%[0-9]+}} syncscope("singlethread-one-as") 
monotonic, align 4
+// CHECK: store i32 {{%[0-9]+}}, i32* %{{.*}}, align 4
+// CHECK: cmpxchg weak i32* {{%[0-9]+}}, i32 {{%[0-9]+}}, i32 {{%[0-9]+}} 
syncscope("singlethread-one-as") monotonic monotonic, align 4

should have atomic, syncscope and monotonic. same as below



Comment at: clang/test/CodeGenCUDA/atomic-ops.cu:31
+  __hip_atomic_store(ptr, val, __ATOMIC_RELAXED, 
__HIP_MEMORY_SCOPE_SINGLETHREAD);
+  flag = __hip_atomic_compare_exchange_weak(ptr, ptr2, val, desired, 
__ATOMIC_RELAXED, __HIP_MEMORY_SCOPE_SINGLETHREAD);
   return flag ? val : desired;

the signature of `__hip_atomic_compare_exchange_weak` is the same as 
`__hip_atomic_compare_exchange_strong` and should be called with the same 
arguments. It should end up with one atomic cmpxchg instruction in IR. same as 
below



Comment at: clang/test/SemaCUDA/atomic-ops.cu:10
+  val = __hip_atomic_load(pi32, 0, 0); // expected-error {{synchronization 
scope argument to atomic operation is invalid}}
+  val = __hip_atomic_load(pi32, 0, __HIP_MEMORY_SCOPE_SINGLETHREAD);
+  val = __hip_atomic_load(pi32, 0, __HIP_MEMORY_SCOPE_WAVEFRONT);

should use predefined macros for memory order, e.g. __ATOMIC_RELAXED. same as 
below



Comment at: clang/test/SemaCUDA/atomic-ops.cu:67
+  flag = __hip_atomic_compare_exchange_weak(ptr, 0, 0, 0, 0, 
__HIP_MEMORY_SCOPE_SYSTEM); // expected-warning {{null passed to a callee that 
requires a non-null argument}}
+  flag = __hip_atomic_compare_exchange_weak(ptr, ptr2, val, desired, 0, 
__HIP_MEMORY_SCOPE_SYSTEM);
+  flag = __hip_atomic_compare_exchange_weak(ptr, ptr2, val, desired, 1, 
__HIP_MEMORY_SCOPE_SINGLETHREAD);

signature is wrong. see above comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114553

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

[PATCH] D112420: [clang][ARM] PACBTI-M assembly support

[Alexandros Lamprineas via Phabricator via cfe-commits]

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

Looks like you've addressed Oliver's comments. I don't have any new suggestions 
from my end. Just make sure you've removed the test xfail before merging.




Comment at: clang/test/Driver/darwin-ld-lto.c:2
 // REQUIRES: system-darwin
-
+// XFAIL: *
 // Check that ld gets "-lto_library".

Seems accidental.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112420

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

[PATCH] D114621: [clangd] Show parameters for construct.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/Hover.cpp:1065
 
+  if (Parameters && !Parameters->empty()) {
+Output.addParagraph().appendText("Parameters: ");

it's a subtle invariant that we only have parameters for functions (which has a 
return type) or constructor/destructor/conversion-operators (which doesn't have 
either a type or a return type).

I think we should assert on `Type` not being present here, as otherwise we 
would probably duplicate parameters in both places. can you also add a 
condition around `!Type` and have a comment saying ` // Don't print parameters 
after Type, as they're likely to be mentioned there.`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114621

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

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added inline comments.



Comment at: llvm/include/llvm/Support/Compiler.h:106-108
 /// Sadly, this is separate from just rvalue reference support because GCC
 /// and MSVC implemented this later than everything else. This appears to be
 /// corrected in MSVC 2019 but not MSVC 2017.

jhenderson wrote:
> Does this comment need changing?
An even bigger question is - can we get rid of the 
LLVM_HAS_RVALUE_REFERENCE_THIS define entirely now? Either as part of this 
patch or as a followup


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114639

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

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon updated this revision to Diff 390082.
RKSimon added a comment.

address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114639

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/UsersManual.rst
  llvm/cmake/modules/CheckCompilerVersion.cmake
  llvm/docs/CMake.rst
  llvm/docs/GettingStarted.rst
  llvm/docs/GettingStartedVS.rst
  llvm/docs/ReleaseNotes.rst
  llvm/include/llvm/Support/Compiler.h

Index: llvm/include/llvm/Support/Compiler.h
===
--- llvm/include/llvm/Support/Compiler.h
+++ llvm/include/llvm/Support/Compiler.h
@@ -80,12 +80,21 @@
 /// * 1916: VS2017, version 15.9
 /// * 1920: VS2019, version 16.0
 /// * 1921: VS2019, version 16.1
+/// * 1922: VS2019, version 16.2
+/// * 1923: VS2019, version 16.3
+/// * 1924: VS2019, version 16.4
+/// * 1925: VS2019, version 16.5
+/// * 1926: VS2019, version 16.6
+/// * 1927: VS2019, version 16.7
+/// * 1928: VS2019, version 16.8 + 16.9
+/// * 1929: VS2019, version 16.10 + 16.11
+/// * 1930: VS2022, version 17.0
 #ifdef _MSC_VER
 #define LLVM_MSC_PREREQ(version) (_MSC_VER >= (version))
 
-// We require at least MSVC 2017.
-#if !LLVM_MSC_PREREQ(1910)
-#error LLVM requires at least MSVC 2017.
+// We require at least VS 2019.
+#if !LLVM_MSC_PREREQ(1920)
+#error LLVM requires at least VS 2019.
 #endif
 
 #else
@@ -97,12 +106,7 @@
 /// Sadly, this is separate from just rvalue reference support because GCC
 /// and MSVC implemented this later than everything else. This appears to be
 /// corrected in MSVC 2019 but not MSVC 2017.
-#if __has_feature(cxx_rvalue_references) || defined(__GNUC__) ||   \
-LLVM_MSC_PREREQ(1920)
 #define LLVM_HAS_RVALUE_REFERENCE_THIS 1
-#else
-#define LLVM_HAS_RVALUE_REFERENCE_THIS 0
-#endif
 
 /// Expands to '&' if ref-qualifiers for *this are supported.
 ///
Index: llvm/docs/ReleaseNotes.rst
===
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -65,7 +65,7 @@
 Changes to building LLVM
 
 
-* ...
+* Building LLVM with Visual Studio now requires version 2019 or later.
 
 Changes to TableGen
 ---
Index: llvm/docs/GettingStartedVS.rst
===
--- llvm/docs/GettingStartedVS.rst
+++ llvm/docs/GettingStartedVS.rst
@@ -36,7 +36,7 @@
 
 Hardware
 
-Any system that can adequately run Visual Studio 2017 is fine. The LLVM
+Any system that can adequately run Visual Studio 2019 is fine. The LLVM
 source tree including the git index consumes approximately 3GB.
 Object files, libraries and executables consume approximately 5GB in
 Release mode and much more in Debug mode. SSD drive and >16GB RAM are
@@ -45,8 +45,8 @@
 
 Software
 
-You will need `Visual Studio `_ 2017 or
-higher, with the latest Update installed. Visual Studio Community Edition
+You will need `Visual Studio `_ 2019 or
+later, with the latest Update installed. Visual Studio Community Edition
 suffices.
 
 You will also need the `CMake `_ build system since it
Index: llvm/docs/GettingStarted.rst
===
--- llvm/docs/GettingStarted.rst
+++ llvm/docs/GettingStarted.rst
@@ -238,7 +238,7 @@
 * Clang 3.5
 * Apple Clang 6.0
 * GCC 5.1
-* Visual Studio 2017
+* Visual Studio 2019
 
 Anything older than these toolchains *may* work, but will require forcing the
 build system with a special option and is not really a supported host platform.
@@ -273,8 +273,8 @@
 This section mostly applies to Linux and older BSDs. On macOS, you should
 have a sufficiently modern Xcode, or you will likely need to upgrade until you
 do. Windows does not have a "system compiler", so you must install either Visual
-Studio 2017 or a recent version of mingw64. FreeBSD 10.0 and newer have a modern
-Clang as the system compiler.
+Studio 2019 (or later), or a recent version of mingw64. FreeBSD 10.0 and newer
+have a modern Clang as the system compiler.
 
 However, some Linux distributions and some other or older BSDs sometimes have
 extremely old versions of GCC. These steps attempt to help you upgrade you
Index: llvm/docs/CMake.rst
===
--- llvm/docs/CMake.rst
+++ llvm/docs/CMake.rst
@@ -447,9 +447,7 @@
   creation of certain convenience build system targets, such as the various
   ``install-*`` and ``check-*`` targets, since IDEs don't always deal well with
   a large number of targets. This is usually autodetected, but it can be
-  configured manually to explicitly control the generation of those targets. One
-  scenario where a manual override may be desirable is when using Visual Studio
-  2017's CMake integration, which wou

[PATCH] D114522: [clangd] Add desugared type to hover

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

> My main remaining concern is just that this will trigger too often when 
> there's no confusion over types, and clutter the display.
> Some common cases I'm worried about:
>
> - std::string -> basic_string. Maybe we should special-case this to 
> hide the aka?
> - int64_t vs long long et al. I have no idea what to do about this.

As discussed offline I share these concerns, and it's not the first time we're 
facing them. I am afraid that this might be disturbing for a good fraction of 
users if we don't polish it around common types.
A couple of suggestions I have (they don't necessarily need to land as part of 
this patch, but I think we should land some damage control mechanism before 
next release):

- Make `AKA-printing` completely optional, to give users a way out (while 
probably starting with off by default and experimenting for a while to see the 
most annoying cases and polishing them as needed)
- Have a list of user-configurable FQNs that AKA printing would be disabled for 
(and provide a good set of defaults inside clangd for STL)




Comment at: clang-tools-extra/clangd/Hover.cpp:1271
 OS << " = " << *P.Default;
+  if (P.Type && P.Type->AKA)
+OS << llvm::formatv(" (aka {0})", *P.Type->AKA);

sammccall wrote:
> Hmm, this seems to render as:
> 
> ```std::string Foo = "" (aka basic_string)```
> 
> it's not *completely* clear what the aka refers to.
> I don't have a better solution, though.
> @kadircet any opinions here?
i think it's attached to the type of the parameter, not it's name nor the 
initializer-expr. so I think we should move this closer to the type (i.e. 
`std::string (aka basic_sting) Foo = ""`, basically 
`llvm::toString(P.Type)` ?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114522

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

[PATCH] D112646: [clang-tidy] Add `readability-container-contains` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 390085.
avogelsgesang marked 7 inline comments as done.
avogelsgesang added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112646

Files:
  clang-tools-extra/clang-tidy/readability/CMakeLists.txt
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
  clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/readability-container-contains.rst
  clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-container-contains.cpp
@@ -0,0 +1,196 @@
+// RUN: %check_clang_tidy -std=c++20 %s readability-container-contains %t
+
+// Some *very* simplified versions of `map` etc.
+namespace std {
+
+template 
+struct map {
+  unsigned count(const Key &K) const;
+  bool contains(const Key &K) const;
+  void *find(const Key &K);
+  void *end();
+};
+
+template 
+struct set {
+  unsigned count(const Key &K) const;
+  bool contains(const Key &K) const;
+};
+
+template 
+struct unordered_set {
+  unsigned count(const Key &K) const;
+  bool contains(const Key &K) const;
+};
+
+template 
+struct multimap {
+  unsigned count(const Key &K) const;
+  bool contains(const Key &K) const;
+};
+
+} // namespace std
+
+// Check that we detect various common ways to check for membership
+int testDifferentCheckTypes(std::map &MyMap) {
+  if (MyMap.count(0))
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use 'contains' to check for membership [readability-container-contains]
+// CHECK-FIXES: if (MyMap.contains(0))
+return 1;
+  bool C1 = MyMap.count(1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = MyMap.contains(1);
+  auto C2 = static_cast(MyMap.count(1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C2 = static_cast(MyMap.contains(1));
+  auto C3 = MyMap.count(2) != 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C3 = MyMap.contains(2);
+  auto C4 = MyMap.count(3) > 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C4 = MyMap.contains(3);
+  auto C5 = MyMap.count(4) >= 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C5 = MyMap.contains(4);
+  auto C6 = MyMap.find(5) != MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C6 = MyMap.contains(5);
+  return C1 + C2 + C3 + C4 + C5 + C6;
+}
+
+// Check that we detect various common ways to check for non-membership
+int testNegativeChecks(std::map &MyMap) {
+  bool C1 = !MyMap.count(-1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = !MyMap.contains(-1);
+  auto C2 = MyMap.count(-2) == 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C2 = !MyMap.contains(-2);
+  auto C3 = MyMap.count(-3) <= 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C3 = !MyMap.contains(-3);
+  auto C4 = MyMap.count(-4) < 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C4 = !MyMap.contains(-4);
+  auto C5 = MyMap.find(-5) == MyMap.end();
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: auto C5 = !MyMap.contains(-5);
+  return C1 + C2 + C3 + C4 + C5;
+}
+
+// Check for various types
+int testDifferentTypes(std::map &M, std::unordered_set &US, std::set &S, std::multimap &MM) {
+  bool C1 = M.count(1001);
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: use 'contains' to check for membership [readability-container-contains]
+  // CHECK-FIXES: bool C1 = M.contains(1001);
+  bool C2 = US.count(1002);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: use 'contains' to check for membership [readability-container-contain