[PATCH] D54077: [clangd] Implemented DraftFileSystem

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I don't think either point 1 or 2 above have been addressed, and so I'm not 
comfortable moving forward with this change.

> I think it would be reasonable to say that a large portion of C++ users are 
> used to the behavior that this patch introduces.

I agree, the new behavior is reasonable. The old behavior is also reasonable. 
This is the basis for point 1 above.

> Current behavior may confuse new users, since, other IDEs mostly (all?) shows 
> diagnostics for edited files instead of saved one. And it's unexpected that 
> headers have 2 states - visible and saved (which cannot be viewed in IDE at 
> all).

Maybe part of the issue is culture clash between "IDEs" and "editors" - the 
distinction is somewhat artificial, but I think the current behavior in editors 
is somewhat traditional, and certainly the two-states is what editor users 
expect (the saved state is what you get when you run the compiler).

> Looks like performance will be same like usage of 'auto save after delay' 
> feature in editor, if we make debounce delay configurable, what do you think?

I think there will be substantial performance loss *and* additional complexity 
from this mode once we start updating diagnostics on file change. (Point 2 
above).
Additionally, if this needs to be configurable, that adds further complexity.

In https://reviews.llvm.org/D54077#1287301, @simark wrote:

> Of course, that is dependent of having an efficient cancelling mechanism.  
> Even with some debouncing, an update to a header file can trigger the 
> re-processing of many TUs, so if I do another edit to the same header shortly 
> after, all the queued jobs from the first edit should be dropped.  But I 
> think we already have that, don't we?


Not for preambles (as we don't yet even update diagnostics on file events), and 
that case is significantly more complicated than main-files:

- there are multiple TUs to consider as you say
- preambles can be 100x more expensive than mainfiles
- this affects background files, so is "usually" unimportant work
- this happens while latency-sensitive operations like code-complete happen in 
the header file itself


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54077



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

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

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In https://reviews.llvm.org/D53738#1288333, @rjmccall wrote:

> In https://reviews.llvm.org/D53738#1287123, @ebevhan wrote:
>
> > In https://reviews.llvm.org/D53738#1284213, @rjmccall wrote:
> >
> > > Not out of line with other features that significantly break with what's 
> > > expressible.  But the easy alternative to storing the intermediate "type" 
> > > in the AST is to just provide a global function that can compute it on 
> > > demand.
> >
> >
> > Yeah, it might just be simpler to go the route of not storing the 
> > computation semantic in the AST, at least for now. That's fairly similar to 
> > the solution in the patch, so I feel a bit silly for just going around in a 
> > circle.
> >
> > To make that work I think the patch needs some changes, though. There 
> > should be a function in FixedPointSemantics to find the common 
> > full-precision semantic between two semantics, and the 
> > `getFixedPointSemantics` in ASTContext should be amended to take integer 
> > types (or another method should be provided for this, but that one already 
> > exists). I think that the `FixedPointConversions` method should also be 
> > embedded into the rest of `UsualArithmeticConversions` as there shouldn't 
> > be any need to have it separate. You still want to do the rvalue conversion 
> > and other promotions, and the rules for fixed-point still fit into the 
> > arithmetic conversions, even in the spec.
> >
> > `EmitFixedPointConversion` should be changed to take FixedPointSemantics 
> > rather than QualType. This has a couple of downsides:
> >
> > - It can no longer handle floating point conversions. They would have to be 
> > handled in EmitScalarConversion.
> > - Conversion from integer is fine, but conversion to integer cannot be 
> > generalized away with the fixed-point semantics as they are currently, as 
> > that kind of conversion must round towards zero. This requires a rounding 
> > step for negative numbers before downscaling, which the current code does 
> > not do. Is there a better way of generalizing this?
>
>
> `FixedPointSemantics` is free to do whatever is convenient for the 
> representation; if it helps to make it able to explicitly represent an 
> integral type — and then just assert that it's never in that form when it's 
> used in certain places, I think that works.  Although you might consider 
> making a `FixedPointOrIntegralSemantics` class and then making 
> `FixedPointSemantics` a subclass which adds nothing to the representation but 
> semantically asserts that it's representing a fixed-point type.


It might just be simpler and a bit more general to add a 
`DownscalingRoundsTowardZero` field to `FixedPointSemantics`, which specifies 
that the source value should be rounded toward zero before downscaling. Then 
conversion routines could handle the integer case explicitly. I'm not sure if 
the field would be useful for anything else, though; it has a pretty specific 
meaning.

I think it's a bit odd to have `FixedPointSemantics` as a specialization of 
something else, since technically integers are a specialization of fixed-point 
values and not the other way around.


Repository:
  rC Clang

https://reviews.llvm.org/D53738



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

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:20
+
+static const char LoopName[] = "forLoopName";
+static const char loopVarName[] = "loopVar";

JonasToth wrote:
> ztamas wrote:
> > JonasToth wrote:
> > > Please move these variable in the matcher function and make them 
> > > `StringRef` instead of `const char[]`.
> > These variables are used not only inside the matcher function but also in 
> > the check() function.
> > 
> I didn't see that, but they should still be `StringRef` as type.
One more nit i forgot: these variables should be `static` for linkage and be 
CamelCase to match the naming conventions.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53974



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

r346216 - T was unused on assertion disabled builds.

[Kadir Cetinkaya via cfe-commits]

Author: kadircet
Date: Tue Nov  6 00:59:25 2018
New Revision: 346216

URL: http://llvm.org/viewvc/llvm-project?rev=346216&view=rev
Log:
T was unused on assertion disabled builds.

Modified:
cfe/trunk/lib/CodeGen/CGBuiltin.cpp

Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=346216&r1=346215&r2=346216&view=diff
==
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Tue Nov  6 00:59:25 2018
@@ -249,12 +249,13 @@ static Value *MakeAtomicCmpXchgValue(Cod
 static
 Value *EmitAtomicCmpXchgForMSIntrin(CodeGenFunction &CGF, const CallExpr *E,
 AtomicOrdering SuccessOrdering = AtomicOrdering::SequentiallyConsistent) {
-  auto T = E->getType();
   assert(E->getArg(0)->getType()->isPointerType());
-  assert(CGF.getContext().hasSameUnqualifiedType(T,
-  E->getArg(0)->getType()->getPointeeType()));
-  assert(CGF.getContext().hasSameUnqualifiedType(T, E->getArg(1)->getType()));
-  assert(CGF.getContext().hasSameUnqualifiedType(T, E->getArg(2)->getType()));
+  assert(CGF.getContext().hasSameUnqualifiedType(
+  E->getType(), E->getArg(0)->getType()->getPointeeType()));
+  assert(CGF.getContext().hasSameUnqualifiedType(E->getType(),
+ E->getArg(1)->getType()));
+  assert(CGF.getContext().hasSameUnqualifiedType(E->getType(),
+ E->getArg(2)->getType()));
 
   auto *Destination = CGF.EmitScalarExpr(E->getArg(0));
   auto *Comparand = CGF.EmitScalarExpr(E->getArg(2));


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

[PATCH] D54077: [clangd] Implemented DraftFileSystem

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Someone mentioned to me that the interaction-between-features argument wasn't 
clear here:

- we **don't** currently update diagnostics for `A.cc` when `A.h` is edited
- we should, this seems more obvious & important than what we do with drafts
- this interacts badly with using draft state, as this patch proposes - there 
are too many edits


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54077



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

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

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

Szelethus added inline comments.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:20
+
+static const char LoopName[] = "forLoopName";
+static const char loopVarName[] = "loopVar";

JonasToth wrote:
> JonasToth wrote:
> > ztamas wrote:
> > > JonasToth wrote:
> > > > Please move these variable in the matcher function and make them 
> > > > `StringRef` instead of `const char[]`.
> > > These variables are used not only inside the matcher function but also in 
> > > the check() function.
> > > 
> > I didn't see that, but they should still be `StringRef` as type.
> One more nit i forgot: these variables should be `static` for linkage and be 
> CamelCase to match the naming conventions.
Hmmm, `StringRef` actually has a `constexpr` constructor, we could make these 
`constexpr` too I guess.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53974



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

[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:20
+
+static const char LoopName[] = "forLoopName";
+static const char loopVarName[] = "loopVar";

Szelethus wrote:
> JonasToth wrote:
> > JonasToth wrote:
> > > ztamas wrote:
> > > > JonasToth wrote:
> > > > > Please move these variable in the matcher function and make them 
> > > > > `StringRef` instead of `const char[]`.
> > > > These variables are used not only inside the matcher function but also 
> > > > in the check() function.
> > > > 
> > > I didn't see that, but they should still be `StringRef` as type.
> > One more nit i forgot: these variables should be `static` for linkage and 
> > be CamelCase to match the naming conventions.
> Hmmm, `StringRef` actually has a `constexpr` constructor, we could make these 
> `constexpr` too I guess.
You want `StringLiteral`:
```
static constexpr llvm::StringLiteral loopVarCastName = 
llvm::StringLiteral("loopVarCast");
static constexpr llvm::StringLiteral loopCondName = 
llvm::StringLiteral("loopCond");
static constexpr llvm::StringLiteral loopIncrementName = 
llvm::StringLiteral("loopIncrement");
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53974



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

[PATCH] D54061: [clang-tidy] run() doesn't update the SourceManager.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In https://reviews.llvm.org/D54061#1286956, @JonasToth wrote:

> > Theoretically, we could replace `ClangTidyCheck::check` with 
> > `ClangTidyCheck::run`, but I'm not sure it is worth, 
> > `ClangTidyCheck::check` is a public API, and is widely-used (for all 
> > clang-tidy checks), replacing it requires large changes (although it is 
> > one-line change), it might break downstream clang-tidy checks.
>
> We can add a deprecation warning and remove the `check` method in the
>  next version?


As Haojian says, this is a lot of churn.
I don't think the benefit of pushing people to migrate, even with a grace 
period, is clear.

Additionally it's *possible* that future refactorings may mean we want to run 
additional "framework" logic when the MatchFinder calls us, and so requiring 
`ClangTidy::check` and `MatchCallback::run` to be the same function seems a 
little risky.

I'll add a comment explaining why the two functions exist.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54061



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

[PATCH] D54061: [clang-tidy] run() doesn't update the SourceManager.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In https://reviews.llvm.org/D54061#1288395, @sammccall wrote:

> In https://reviews.llvm.org/D54061#1286956, @JonasToth wrote:
>
> > > Theoretically, we could replace `ClangTidyCheck::check` with 
> > > `ClangTidyCheck::run`, but I'm not sure it is worth, 
> > > `ClangTidyCheck::check` is a public API, and is widely-used (for all 
> > > clang-tidy checks), replacing it requires large changes (although it is 
> > > one-line change), it might break downstream clang-tidy checks.
> >
> > We can add a deprecation warning and remove the `check` method in the
> >  next version?
>
>
> As Haojian says, this is a lot of churn.
>  I don't think the benefit of pushing people to migrate, even with a grace 
> period, is clear.
>
> Additionally it's *possible* that future refactorings may mean we want to run 
> additional "framework" logic when the MatchFinder calls us, and so requiring 
> `ClangTidy::check` and `MatchCallback::run` to be the same function seems a 
> little risky.
>
> I'll add a comment explaining why the two functions exist.


Alright.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54061



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

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

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

xazax.hun added a comment.

I would love to see a test with deeper macro in macro expansion and larger 
number of arguments, with some of the arguments unused. Some minor nits inline, 
otherwise looks good.




Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:831
   const MacroInfo *MI = Info.MI;
+  MacroArgMap &Args = Info.Args;
+

IS there any value of having `Args` here instead of `Info.Args`? I would just 
remove the `Args reference here.`



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:857
  getMacroInfoForLocation(PP, SM, II, T.getLocation())) 
{
-  getMacroNameAndPrintExpansion(Printer, T.getLocation(), PP);
+  getMacroNameAndPrintExpansion(Printer, T.getLocation(), PP, PrevArgs);
 

Maybe instead of mutating PrevArgs above, we could keep `PrevArgs` argument to 
point to the previous arguments and pass the address of `Info.Args` here? Do we 
need the null pointer semantics? Expanding macros from an empty map should be 
noop. Maybe we could eliminate some branches this way.



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:982
+  int ParenthesesDepth = 1;
+  while (ParenthesesDepth != 0) {
 ++It;

Is the misspelling already committed? If not, better fix it in the other 
revision to keep this smaller. Otherwise, it is fine.



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:1018
+auto It = CurrExpArgTokens.begin();
+while (It != CurrExpArgTokens.end()) {
+  if (It->isNot(tok::identifier)) {

Maybe a for loop mor natural here?


https://reviews.llvm.org/D52795



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

[clang-tools-extra] r346219 - [clang-tidy] run() doesn't update the SourceManager.

[Sam McCall via cfe-commits]

Author: sammccall
Date: Tue Nov  6 01:28:23 2018
New Revision: 346219

URL: http://llvm.org/viewvc/llvm-project?rev=346219&view=rev
Log:
[clang-tidy] run() doesn't update the SourceManager.

Summary:
By now the context's SourceManager is now initialized everywhere that
ClangTidyCheck::registerMatcher() is called, so the call from run() seems
entirely redundant, and indeed all the tests pass.

This solves a problem with embedding clang-tidy: if using a DiagnosticsEngine
which already has file state, re-setting its SourceManager (to the same value)
causes an assertion.
(There are other ways to solve this problem, but this is the simplest).

Reviewers: hokein, alexfh

Subscribers: xazax.hun, cfe-commits

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

Modified:
clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp

Modified: clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp?rev=346219&r1=346218&r2=346219&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp Tue Nov  6 01:28:23 2018
@@ -441,7 +441,9 @@ DiagnosticBuilder ClangTidyCheck::diag(S
 }
 
 void ClangTidyCheck::run(const ast_matchers::MatchFinder::MatchResult &Result) 
{
-  Context->setSourceManager(Result.SourceManager);
+  // For historical reasons, checks don't implement the MatchFinder run()
+  // callback directly. We keep the run()/check() distinction to avoid 
interface
+  // churn, and to allow us to add cross-cutting logic in the future.
   check(Result);
 }
 


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

[PATCH] D54061: [clang-tidy] run() doesn't update the SourceManager.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE346219: [clang-tidy] run() doesn't update the 
SourceManager. (authored by sammccall, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D54061?vs=172471&id=172719#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54061

Files:
  clang-tidy/ClangTidy.cpp


Index: clang-tidy/ClangTidy.cpp
===
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -441,7 +441,9 @@
 }
 
 void ClangTidyCheck::run(const ast_matchers::MatchFinder::MatchResult &Result) 
{
-  Context->setSourceManager(Result.SourceManager);
+  // For historical reasons, checks don't implement the MatchFinder run()
+  // callback directly. We keep the run()/check() distinction to avoid 
interface
+  // churn, and to allow us to add cross-cutting logic in the future.
   check(Result);
 }
 


Index: clang-tidy/ClangTidy.cpp
===
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -441,7 +441,9 @@
 }
 
 void ClangTidyCheck::run(const ast_matchers::MatchFinder::MatchResult &Result) {
-  Context->setSourceManager(Result.SourceManager);
+  // For historical reasons, checks don't implement the MatchFinder run()
+  // callback directly. We keep the run()/check() distinction to avoid interface
+  // churn, and to allow us to add cross-cutting logic in the future.
   check(Result);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54077: [clangd] Implemented DraftFileSystem

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D54077#1288387, @sammccall wrote:

> Someone mentioned to me that the interaction-between-features argument wasn't 
> clear here:
>
> - we **don't** currently update diagnostics for `A.cc` when `A.h` is edited
> - we should, this seems more obvious & important than what we do with drafts
> - this interacts badly with using draft state, as this patch proposes - there 
> are too many edits


FWIW, one of my "pain points" when using vim+clangd is:

- Edit `A.h` in a buffer (and forget to save)
- Switch to `A.cc` in another buffer
- Realize that I forgot to save `A.h`
- Go back to save `A.h`
- Jump back to `A.cc` file.

I would be very happy if `A.cc` can see the unsaved `A.h` when I am editing 
`A.cc`. I think the update should be relatively less frequent with this 
approach, as people don't usually update multiple files at the same time.  Not 
sure if I want all files that depend on `A.h` to be updated when I edit `A.h` 
though; it seems much more complicated and less important (to me).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54077



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

[PATCH] D54077: [clangd] Implemented DraftFileSystem

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In https://reviews.llvm.org/D54077#1288404, @ioeric wrote:

> In https://reviews.llvm.org/D54077#1288387, @sammccall wrote:
>
> > Someone mentioned to me that the interaction-between-features argument 
> > wasn't clear here:
> >
> > - we **don't** currently update diagnostics for `A.cc` when `A.h` is edited
> > - we should, this seems more obvious & important than what we do with drafts
> > - this interacts badly with using draft state, as this patch proposes - 
> > there are too many edits
>
>
> FWIW, one of my "pain points" when using vim+clangd is:
>
> - Edit `A.h` in a buffer (and forget to save)
> - Switch to `A.cc` in another buffer
> - Realize that I forgot to save `A.h`
> - Go back to save `A.h`
> - Jump back to `A.cc` file.
>
>   I would be very happy if `A.cc` can see the unsaved `A.h` when I am editing 
> `A.cc`. I think the update should be relatively less frequent with this 
> approach, as people don't usually update multiple files at the same time.  
> Not sure if I want all files that depend on `A.h` to be updated when I edit 
> `A.h` though; it seems much more complicated and less important (to me).


I can see this is a problem when you forget to save. On the other hand, one 
case I encounter frequently enough is:
edit A.h, save
go to B.cc (that includes it), see errors that it causes (here and in other 
files)
go to A.h, fix error, save
go to B.cc (or a different file that includes A.h) and wanting to see the error 
gone


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54077



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

[PATCH] D54077: [clangd] Implemented DraftFileSystem

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

In https://reviews.llvm.org/D54077#1288404, @ioeric wrote:

> I would be very happy if `A.cc` can see the unsaved `A.h` when I am editing 
> `A.cc`.


We have that in Qt Creator (with libclang) and that's quite handy as it can 
save you some build cycles.

> Not sure if I want all files that depend on `A.h` to be updated when I edit 
> `A.h` though; it seems much more complicated and less important (to me).

If it helps (and the LSP allows it): In case A.h is edited, we flag it dirty. 
If the user makes some file depending on it visible (or the current file), we 
trigger a reparse for that. Not sure whether the LSP has a notion of current or 
visible files...


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54077



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

[PATCH] D54077: [clangd] Implemented DraftFileSystem

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

In https://reviews.llvm.org/D54077#1288413, @nik wrote:

> If it helps (and the LSP allows it): In case A.h is edited, we flag it dirty. 
> If the user makes some file depending on it visible (or the current file), we 
> trigger a reparse for that. Not sure whether the LSP has a notion of current 
> or visible files...


Huch..., I meant that we flag the files dirty that depend on A.h.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54077



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

[PATCH] D52967: Extend shelf-life by 70 years

[Bernhard M. Wiedemann via Phabricator via cfe-commits]

bmwiedemann added a comment.

I tested now, that it still works on i586 (in addition to x86_64)


Repository:
  rC Clang

https://reviews.llvm.org/D52967



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

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Jonas Toth via Phabricator via cfe-commits]

JonasToth created this revision.
JonasToth added reviewers: alexfh, aaron.ballman, hokein, sammccall, lebedev.ri.
Herald added subscribers: cfe-commits, xazax.hun, mgorny.

`run-clang-tidy.py` is the parallel executor for `clang-tidy`. Due to the
common header-inclusion problem in C++/C diagnostics that are usually emitted
in class declarations are emitted every time their corresponding header is
included.

This results in a *VERY* high amount of spam and renders the output basically
useles for bigger projects.
With this patch `run-clang-tidy.py` gets another option that enables
deduplication of all emitted diagnostics. This is achieved with parsing the
diagnostic output from each `clang-tidy` invocation, identifying warnings and
error and parsing until the next occurence of an error or warning. The collected
diagnostic is hashed and stored in a set. Every new diagnostic will only be
emitted if its hash is not in the set already.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54141

Files:
  clang-tidy/tool/run-clang-tidy.py
  clang-tidy/tool/run_clang_tidy.py
  clang-tidy/tool/test_input/out_csa_cmake.log
  clang-tidy/tool/test_input/out_performance_cmake.log
  clang-tidy/tool/test_log_parser.py
  docs/ReleaseNotes.rst

Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -182,6 +182,9 @@
   ` check does not warn
   about calls inside macros anymore by default.
 
+- `run-clang-tidy.py` support deduplication of `clang-tidy` diagnostics
+  to reduce the amount of output with the optional `-deduplicate` flag.
+
 Improvements to include-fixer
 -
 
Index: clang-tidy/tool/test_log_parser.py
===
--- /dev/null
+++ clang-tidy/tool/test_log_parser.py
@@ -0,0 +1,236 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+
+import unittest
+from run_clang_tidy import Diagnostic, Deduplication
+from run_clang_tidy import ParseClangTidyDiagnostics, _is_valid_diag_match
+
+
+class TestDiagnostics(unittest.TestCase):
+"""Test fingerprinting diagnostic messages"""
+
+def test_construction(self):
+d = Diagnostic("/home/user/project/my_file.h", 24, 4,
+   "warning: Do not do this thing [warning-category]")
+self.assertIsNotNone(d)
+self.assertEqual(str(d),
+ "/home/user/project/my_file.h:24:4: warning: Do not do this thing [warning-category]")
+
+d.add_additional_line("  MyCodePiece();")
+d.add_additional_line("  ^")
+
+self.assertEqual(str(d),
+ "/home/user/project/my_file.h:24:4: warning: Do not do this thing [warning-category]"
+ "\n  MyCodePiece();"
+ "\n  ^")
+
+
+class TestDeduplication(unittest.TestCase):
+"""Test the `DiagEssence` based deduplication of diagnostic messages."""
+
+def test_construction(self):
+self.assertIsNotNone(Deduplication())
+
+def test_dedup(self):
+dedup = Deduplication()
+d = Diagnostic("/home/user/project/my_file.h", 24, 4,
+   "warning: Do not do this thing [warning-category]")
+self.assertTrue(dedup.insert_and_query(d))
+self.assertFalse(dedup.insert_and_query(d))
+
+d2 = Diagnostic("/home/user/project/my_file.h", 24, 4,
+"warning: Do not do this thing [warning-category]")
+d2.add_additional_line("  MyCodePiece();")
+d2.add_additional_line("  ^")
+self.assertTrue(dedup.insert_and_query(d2))
+self.assertFalse(dedup.insert_and_query(d2))
+
+d3 = Diagnostic("/home/user/project/my_file.h", 24, 4,
+"warning: Do not do this thing [warning-category]")
+self.assertFalse(dedup.insert_and_query(d3))
+
+class TestLinewiseParsing(unittest.TestCase):
+def test_construction(self):
+self.assertIsNotNone(ParseClangTidyDiagnostics())
+
+def test_valid_diags_regex(self):
+pp = ParseClangTidyDiagnostics()
+
+warning = "/home/user/project/my_file.h:123:1: warning: don't do it [no]"
+m = pp._diag_re.match(warning)
+self.assertTrue(m)
+self.assertTrue(_is_valid_diag_match(m.groups()))
+
+error = "/home/user/project/my_file.h:1:110: error: wrong! [not-ok]"
+m = pp._diag_re.match(error)
+self.assertTrue(m)
+self.assertTrue(_is_valid_diag_match(m.groups()))
+
+hybrid = "/home/user/project/boo.cpp:30:42: error: wrong! [not-ok,bad]"
+m = pp._diag_re.match(hybrid)
+self.assertTrue(m)
+self.assertTrue(_is_valid_diag_match(m.groups()))
+
+note = "/home/user/project/my_file.h:1:110: note: alksdj"
+m = pp._diag_re.match(note)
+self.assertFalse(m)
+
+garbage = "not a path:not_a_number:110: gibberish"
+   

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 172726.
JonasToth added a comment.

- spurious change in my git


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54141

Files:
  clang-tidy/tool/run-clang-tidy.py
  clang-tidy/tool/run_clang_tidy.py
  clang-tidy/tool/test_input/out_csa_cmake.log
  clang-tidy/tool/test_input/out_performance_cmake.log
  clang-tidy/tool/test_log_parser.py
  docs/ReleaseNotes.rst

Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -182,6 +182,9 @@
   ` check does not warn
   about calls inside macros anymore by default.
 
+- `run-clang-tidy.py` support deduplication of `clang-tidy` diagnostics
+  to reduce the amount of output with the optional `-deduplicate` flag.
+
 Improvements to include-fixer
 -
 
Index: clang-tidy/tool/test_log_parser.py
===
--- /dev/null
+++ clang-tidy/tool/test_log_parser.py
@@ -0,0 +1,236 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+
+import unittest
+from run_clang_tidy import Diagnostic, Deduplication
+from run_clang_tidy import ParseClangTidyDiagnostics, _is_valid_diag_match
+
+
+class TestDiagnostics(unittest.TestCase):
+"""Test fingerprinting diagnostic messages"""
+
+def test_construction(self):
+d = Diagnostic("/home/user/project/my_file.h", 24, 4,
+   "warning: Do not do this thing [warning-category]")
+self.assertIsNotNone(d)
+self.assertEqual(str(d),
+ "/home/user/project/my_file.h:24:4: warning: Do not do this thing [warning-category]")
+
+d.add_additional_line("  MyCodePiece();")
+d.add_additional_line("  ^")
+
+self.assertEqual(str(d),
+ "/home/user/project/my_file.h:24:4: warning: Do not do this thing [warning-category]"
+ "\n  MyCodePiece();"
+ "\n  ^")
+
+
+class TestDeduplication(unittest.TestCase):
+"""Test the `DiagEssence` based deduplication of diagnostic messages."""
+
+def test_construction(self):
+self.assertIsNotNone(Deduplication())
+
+def test_dedup(self):
+dedup = Deduplication()
+d = Diagnostic("/home/user/project/my_file.h", 24, 4,
+   "warning: Do not do this thing [warning-category]")
+self.assertTrue(dedup.insert_and_query(d))
+self.assertFalse(dedup.insert_and_query(d))
+
+d2 = Diagnostic("/home/user/project/my_file.h", 24, 4,
+"warning: Do not do this thing [warning-category]")
+d2.add_additional_line("  MyCodePiece();")
+d2.add_additional_line("  ^")
+self.assertTrue(dedup.insert_and_query(d2))
+self.assertFalse(dedup.insert_and_query(d2))
+
+d3 = Diagnostic("/home/user/project/my_file.h", 24, 4,
+"warning: Do not do this thing [warning-category]")
+self.assertFalse(dedup.insert_and_query(d3))
+
+class TestLinewiseParsing(unittest.TestCase):
+def test_construction(self):
+self.assertIsNotNone(ParseClangTidyDiagnostics())
+
+def test_valid_diags_regex(self):
+pp = ParseClangTidyDiagnostics()
+
+warning = "/home/user/project/my_file.h:123:1: warning: don't do it [no]"
+m = pp._diag_re.match(warning)
+self.assertTrue(m)
+self.assertTrue(_is_valid_diag_match(m.groups()))
+
+error = "/home/user/project/my_file.h:1:110: error: wrong! [not-ok]"
+m = pp._diag_re.match(error)
+self.assertTrue(m)
+self.assertTrue(_is_valid_diag_match(m.groups()))
+
+hybrid = "/home/user/project/boo.cpp:30:42: error: wrong! [not-ok,bad]"
+m = pp._diag_re.match(hybrid)
+self.assertTrue(m)
+self.assertTrue(_is_valid_diag_match(m.groups()))
+
+note = "/home/user/project/my_file.h:1:110: note: alksdj"
+m = pp._diag_re.match(note)
+self.assertFalse(m)
+
+garbage = "not a path:not_a_number:110: gibberish"
+m = pp._diag_re.match(garbage)
+self.assertFalse(m)
+
+def test_single_diagnostics(self):
+pp = ParseClangTidyDiagnostics()
+example_warning = [
+"/project/git/Source/kwsys/Terminal.c:53:21: warning: use of a signed integer operand with a binary bitwise operator [hicpp-signed-bitwise]",
+]
+pp._parse_lines(example_warning)
+self.assertEqual(
+str(pp.get_diags()[0]),
+"/project/git/Source/kwsys/Terminal.c:53:21: warning: use of a signed integer operand with a binary bitwise operator [hicpp-signed-bitwise]"
+)
+
+def test_no_diag(self):
+pp = ParseClangTidyDiagnostics()
+garbage_lines = \
+"""
+hicpp-no-array-decay
+hicpp-no-assembler
+hicpp-no-malloc
+hicpp-noexcept-move
+hicpp-signed-bitwis

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/tool/run_clang_tidy.py:1
+run-clang-tidy.py

This simlink is required for my unittests, I don't know how to add the added 
tests in the `lit` test-suite so there is no change there yet. A bit of 
guidance there would be nice :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54141



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

[PATCH] D54077: [clangd] Implemented DraftFileSystem

[Lutsenko Danil via Phabricator via cfe-commits]

LutsenkoDanil added a comment.

@sammccall Thank you for detailed explanation!

In https://reviews.llvm.org/D54077#1288387, @sammccall wrote:

> Someone mentioned to me that the interaction-between-features argument wasn't 
> clear here:
>
> - we **don't** currently update diagnostics for `A.cc` when `A.h` is edited
> - we should, this seems more obvious & important than what we do with drafts
> - this interacts badly with using draft state, as this patch proposes - there 
> are too many edits


I already made a patch which introduces such behavior (not uploaded here yet), 
and looks like it works well with draft fs: diagnostics updates for depended 
files in 'real-time' on typing for opened files and no seen performance 
glitches, in multi-threaded mode at least.
I suggest continue discussion when/if dependencies tracking will be implemented 
and real performance reduce introduced by this patch can be checked with real 
code.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54077



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

[PATCH] D53433: [clangd] auto-index stores symbols per-file instead of per-TU.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 172732.
ioeric added a comment.

- Rebase


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53433

Files:
  clangd/index/Background.cpp
  clangd/index/Background.h
  clangd/index/FileIndex.cpp
  clangd/index/FileIndex.h
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolCollector.h
  clangd/index/dex/Dex.h
  unittests/clangd/BackgroundIndexTests.cpp
  unittests/clangd/DexTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/SyncAPI.cpp
  unittests/clangd/SyncAPI.h

Index: unittests/clangd/SyncAPI.h
===
--- unittests/clangd/SyncAPI.h
+++ unittests/clangd/SyncAPI.h
@@ -52,6 +52,7 @@
 
 SymbolSlab runFuzzyFind(const SymbolIndex &Index, StringRef Query);
 SymbolSlab runFuzzyFind(const SymbolIndex &Index, const FuzzyFindRequest &Req);
+RefSlab getRefs(const SymbolIndex &Index, SymbolID ID);
 
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -8,6 +8,7 @@
 //===--===//
 
 #include "SyncAPI.h"
+#include "index/Index.h"
 
 using namespace llvm;
 namespace clang {
@@ -138,5 +139,14 @@
   return std::move(Builder).build();
 }
 
+RefSlab getRefs(const SymbolIndex &Index, SymbolID ID) {
+  RefsRequest Req;
+  Req.IDs = {ID};
+  RefSlab::Builder Slab;
+  Index.refs(Req, [&](const Ref &S) { Slab.insert(ID, S); });
+  return std::move(Slab).build();
+}
+
+
 } // namespace clangd
 } // namespace clang
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -14,6 +14,7 @@
 #include "TestFS.h"
 #include "TestTU.h"
 #include "index/FileIndex.h"
+#include "index/Index.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
@@ -39,6 +40,7 @@
 }
 MATCHER_P(FileURI, F, "") { return arg.Location.FileURI == F; }
 MATCHER_P(DeclURI, U, "") { return arg.CanonicalDeclaration.FileURI == U; }
+MATCHER_P(DefURI, U, "") { return arg.Definition.FileURI == U; }
 MATCHER_P(QName, N, "") { return (arg.Scope + arg.Name).str() == N; }
 
 using namespace llvm;
@@ -73,14 +75,6 @@
   return llvm::make_unique(std::move(Slab).build());
 }
 
-RefSlab getRefs(const SymbolIndex &I, SymbolID ID) {
-  RefsRequest Req;
-  Req.IDs = {ID};
-  RefSlab::Builder Slab;
-  I.refs(Req, [&](const Ref &S) { Slab.insert(ID, S); });
-  return std::move(Slab).build();
-}
-
 TEST(FileSymbolsTest, UpdateAndGet) {
   FileSymbols FS;
   EXPECT_THAT(runFuzzyFind(*FS.buildIndex(IndexType::Light), ""), IsEmpty());
@@ -102,6 +96,27 @@
  QName("4"), QName("5")));
 }
 
+TEST(FileSymbolsTest, MergeOverlap) {
+  FileSymbols FS;
+  auto OneSymboSlab = [](Symbol Sym) {
+SymbolSlab::Builder S;
+S.insert(Sym);
+return make_unique(std::move(S).build());
+  };
+  auto X1 = symbol("x");
+  X1.CanonicalDeclaration.FileURI = "file:///x1";
+  auto X2 = symbol("x");
+  X2.Definition.FileURI = "file:///x2";
+
+  FS.update("f1", OneSymboSlab(X1), nullptr);
+  FS.update("f2", OneSymboSlab(X2), nullptr);
+  for (auto Type : {IndexType::Light, IndexType::Heavy})
+EXPECT_THAT(
+runFuzzyFind(*FS.buildIndex(Type, DuplicateHandling::Merge), "x"),
+UnorderedElementsAre(
+AllOf(QName("x"), DeclURI("file:///x1"), DefURI("file:///x2";
+}
+
 TEST(FileSymbolsTest, SnapshotAliveAfterRemove) {
   FileSymbols FS;
 
Index: unittests/clangd/DexTests.cpp
===
--- unittests/clangd/DexTests.cpp
+++ unittests/clangd/DexTests.cpp
@@ -491,15 +491,6 @@
"other::A"));
 }
 
-TEST(DexTest, DexDeduplicate) {
-  std::vector Symbols = {symbol("1"), symbol("2"), symbol("3"),
- symbol("2") /* duplicate */};
-  FuzzyFindRequest Req;
-  Req.Query = "2";
-  Dex I(Symbols, RefSlab(), URISchemes);
-  EXPECT_THAT(match(I, Req), ElementsAre("2"));
-}
-
 TEST(DexTest, DexLimitedNumMatches) {
   auto I = Dex::build(generateNumSymbols(0, 100), RefSlab(), URISchemes);
   FuzzyFindRequest Req;
Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -4,33 +4,76 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
+using testing::_;
+using testing::AllOf;
+using testing::Not;
 using testing::UnorderedElementsAre;
 
 namespace clang {
 namespace clangd {
 
 MATCHER_P(Named, N, "") { return arg.Name == N; }
+MATCHER(Declared, "") { return !arg.CanonicalDeclaration.FileURI.empty(); }
+MATCHER(D

[clang-tools-extra] r346221 - [clangd] auto-index stores symbols per-file instead of per-TU.

[Eric Liu via cfe-commits]

Author: ioeric
Date: Tue Nov  6 02:55:21 2018
New Revision: 346221

URL: http://llvm.org/viewvc/llvm-project?rev=346221&view=rev
Log:
[clangd] auto-index stores symbols per-file instead of per-TU.

Summary:
This allows us to deduplicate header symbols across TUs. File digests
are collects when collecting symbols/refs. And the index store deduplicates
file symbols based on the file digest.

Reviewers: sammccall, hokein

Reviewed By: sammccall

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

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

Modified:
clang-tools-extra/trunk/clangd/index/Background.cpp
clang-tools-extra/trunk/clangd/index/Background.h
clang-tools-extra/trunk/clangd/index/FileIndex.cpp
clang-tools-extra/trunk/clangd/index/FileIndex.h
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
clang-tools-extra/trunk/clangd/index/SymbolCollector.h
clang-tools-extra/trunk/clangd/index/dex/Dex.h
clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/DexTests.cpp
clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/SyncAPI.cpp
clang-tools-extra/trunk/unittests/clangd/SyncAPI.h

Modified: clang-tools-extra/trunk/clangd/index/Background.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.cpp?rev=346221&r1=346220&r2=346221&view=diff
==
--- clang-tools-extra/trunk/clangd/index/Background.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/Background.cpp Tue Nov  6 02:55:21 2018
@@ -13,11 +13,19 @@
 #include "Logger.h"
 #include "Threading.h"
 #include "Trace.h"
+#include "URI.h"
 #include "index/IndexAction.h"
 #include "index/MemIndex.h"
 #include "index/Serialization.h"
+#include "index/SymbolCollector.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/SHA1.h"
 #include 
+#include 
 
 using namespace llvm;
 namespace clang {
@@ -125,6 +133,142 @@ void BackgroundIndex::enqueueLocked(tool
   std::move(Cmd)));
 }
 
+static BackgroundIndex::FileDigest digest(StringRef Content) {
+  return SHA1::hash({(const uint8_t *)Content.data(), Content.size()});
+}
+
+static Optional digestFile(const SourceManager 
&SM,
+FileID FID) {
+  bool Invalid = false;
+  StringRef Content = SM.getBufferData(FID, &Invalid);
+  if (Invalid)
+return None;
+  return digest(Content);
+}
+
+// Resolves URI to file paths with cache.
+class URIToFileCache {
+public:
+  URIToFileCache(llvm::StringRef HintPath) : HintPath(HintPath) {}
+
+  llvm::StringRef resolve(llvm::StringRef FileURI) {
+auto I = URIToPathCache.try_emplace(FileURI);
+if (I.second) {
+  auto U = URI::parse(FileURI);
+  if (!U) {
+elog("Failed to parse URI {0}: {1}", FileURI, U.takeError());
+assert(false && "Failed to parse URI");
+return "";
+  }
+  auto Path = URI::resolve(*U, HintPath);
+  if (!Path) {
+elog("Failed to resolve URI {0}: {1}", FileURI, Path.takeError());
+assert(false && "Failed to resolve URI");
+return "";
+  }
+  I.first->second = *Path;
+}
+return I.first->second;
+  }
+
+private:
+  std::string HintPath;
+  llvm::StringMap URIToPathCache;
+};
+
+/// Given index results from a TU, only update files in \p FilesToUpdate.
+void BackgroundIndex::update(StringRef MainFile, SymbolSlab Symbols,
+ RefSlab Refs,
+ const StringMap &FilesToUpdate) {
+  // Partition symbols/references into files.
+  struct File {
+DenseSet Symbols;
+DenseSet Refs;
+  };
+  StringMap Files;
+  URIToFileCache URICache(MainFile);
+  for (const auto &Sym : Symbols) {
+if (Sym.CanonicalDeclaration) {
+  auto DeclPath = URICache.resolve(Sym.CanonicalDeclaration.FileURI);
+  if (FilesToUpdate.count(DeclPath) != 0)
+Files[DeclPath].Symbols.insert(&Sym);
+}
+// For symbols with different declaration and definition locations, we 
store
+// the full symbol in both the header file and the implementation file, so
+// that merging can tell the preferred symbols (from canonical headers) 
from
+// other symbols (e.g. forward declarations).
+if (Sym.Definition &&
+Sym.Definition.FileURI != Sym.CanonicalDeclaration.FileURI) {
+  auto DefPath = URICache.resolve(Sym.Definition.FileURI);
+  if (FilesToUpdate.count(DefPath) != 0)
+Files[DefPath].Symbols.insert(&Sym);
+}
+  }
+  DenseMap RefToIDs;
+  for (const auto &SymRefs : Refs) {
+for (const auto &R : SymRefs.second) {
+  auto Path = URICache.resolve(R.Location.FileURI);
+  if (FilesToUpdat

[PATCH] D53433: [clangd] auto-index stores symbols per-file instead of per-TU.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346221: [clangd] auto-index stores symbols per-file instead 
of per-TU. (authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D53433

Files:
  clang-tools-extra/trunk/clangd/index/Background.cpp
  clang-tools-extra/trunk/clangd/index/Background.h
  clang-tools-extra/trunk/clangd/index/FileIndex.cpp
  clang-tools-extra/trunk/clangd/index/FileIndex.h
  clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
  clang-tools-extra/trunk/clangd/index/SymbolCollector.h
  clang-tools-extra/trunk/clangd/index/dex/Dex.h
  clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/DexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/SyncAPI.cpp
  clang-tools-extra/trunk/unittests/clangd/SyncAPI.h

Index: clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
@@ -14,6 +14,7 @@
 #include "TestFS.h"
 #include "TestTU.h"
 #include "index/FileIndex.h"
+#include "index/Index.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
@@ -39,6 +40,7 @@
 }
 MATCHER_P(FileURI, F, "") { return arg.Location.FileURI == F; }
 MATCHER_P(DeclURI, U, "") { return arg.CanonicalDeclaration.FileURI == U; }
+MATCHER_P(DefURI, U, "") { return arg.Definition.FileURI == U; }
 MATCHER_P(QName, N, "") { return (arg.Scope + arg.Name).str() == N; }
 
 using namespace llvm;
@@ -73,14 +75,6 @@
   return llvm::make_unique(std::move(Slab).build());
 }
 
-RefSlab getRefs(const SymbolIndex &I, SymbolID ID) {
-  RefsRequest Req;
-  Req.IDs = {ID};
-  RefSlab::Builder Slab;
-  I.refs(Req, [&](const Ref &S) { Slab.insert(ID, S); });
-  return std::move(Slab).build();
-}
-
 TEST(FileSymbolsTest, UpdateAndGet) {
   FileSymbols FS;
   EXPECT_THAT(runFuzzyFind(*FS.buildIndex(IndexType::Light), ""), IsEmpty());
@@ -102,6 +96,27 @@
  QName("4"), QName("5")));
 }
 
+TEST(FileSymbolsTest, MergeOverlap) {
+  FileSymbols FS;
+  auto OneSymboSlab = [](Symbol Sym) {
+SymbolSlab::Builder S;
+S.insert(Sym);
+return make_unique(std::move(S).build());
+  };
+  auto X1 = symbol("x");
+  X1.CanonicalDeclaration.FileURI = "file:///x1";
+  auto X2 = symbol("x");
+  X2.Definition.FileURI = "file:///x2";
+
+  FS.update("f1", OneSymboSlab(X1), nullptr);
+  FS.update("f2", OneSymboSlab(X2), nullptr);
+  for (auto Type : {IndexType::Light, IndexType::Heavy})
+EXPECT_THAT(
+runFuzzyFind(*FS.buildIndex(Type, DuplicateHandling::Merge), "x"),
+UnorderedElementsAre(
+AllOf(QName("x"), DeclURI("file:///x1"), DefURI("file:///x2";
+}
+
 TEST(FileSymbolsTest, SnapshotAliveAfterRemove) {
   FileSymbols FS;
 
Index: clang-tools-extra/trunk/unittests/clangd/SyncAPI.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/SyncAPI.cpp
+++ clang-tools-extra/trunk/unittests/clangd/SyncAPI.cpp
@@ -8,6 +8,7 @@
 //===--===//
 
 #include "SyncAPI.h"
+#include "index/Index.h"
 
 using namespace llvm;
 namespace clang {
@@ -138,5 +139,14 @@
   return std::move(Builder).build();
 }
 
+RefSlab getRefs(const SymbolIndex &Index, SymbolID ID) {
+  RefsRequest Req;
+  Req.IDs = {ID};
+  RefSlab::Builder Slab;
+  Index.refs(Req, [&](const Ref &S) { Slab.insert(ID, S); });
+  return std::move(Slab).build();
+}
+
+
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
@@ -4,33 +4,76 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
+using testing::_;
+using testing::AllOf;
+using testing::Not;
 using testing::UnorderedElementsAre;
 
 namespace clang {
 namespace clangd {
 
 MATCHER_P(Named, N, "") { return arg.Name == N; }
+MATCHER(Declared, "") { return !arg.CanonicalDeclaration.FileURI.empty(); }
+MATCHER(Defined, "") { return !arg.Definition.FileURI.empty(); }
+
+MATCHER_P(FileURI, F, "") { return arg.Location.FileURI == F; }
+testing::Matcher
+RefsAre(std::vector> Matchers) {
+  return ElementsAre(testing::Pair(_, UnorderedElementsAreArray(Matchers)));
+}
 
 TEST(BackgroundIndexTest, IndexTwoFiles) {
   MockFSProvider FS;
   // a.h yields different symbols when included by A.cc vs B.cc.
-  // Currently we store symbols for each TU, so we get both.
-  FS.Files[testPath("roo

[PATCH] D53933: [clangd] Get rid of QueryScopes.empty() == AnyScope special case.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 172735.
ioeric added a comment.

- rebase


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53933

Files:
  clangd/FindSymbols.cpp
  clangd/index/Index.h
  clangd/index/MemIndex.cpp
  clangd/index/dex/Dex.cpp
  unittests/clangd/DexTests.cpp
  unittests/clangd/IndexTests.cpp
  unittests/clangd/SyncAPI.cpp

Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -130,6 +130,7 @@
 SymbolSlab runFuzzyFind(const SymbolIndex &Index, StringRef Query) {
   FuzzyFindRequest Req;
   Req.Query = Query;
+  Req.AnyScope = true;
   return runFuzzyFind(Index, Req);
 }
 
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -90,14 +90,16 @@
  symbol("2") /* duplicate */};
   FuzzyFindRequest Req;
   Req.Query = "2";
+  Req.AnyScope = true;
   MemIndex I(Symbols, RefSlab());
   EXPECT_THAT(match(I, Req), ElementsAre("2"));
 }
 
 TEST(MemIndexTest, MemIndexLimitedNumMatches) {
   auto I = MemIndex::build(generateNumSymbols(0, 100), RefSlab());
   FuzzyFindRequest Req;
   Req.Query = "5";
+  Req.AnyScope = true;
   Req.Limit = 3;
   bool Incomplete;
   auto Matches = match(*I, Req, &Incomplete);
@@ -112,6 +114,7 @@
   RefSlab());
   FuzzyFindRequest Req;
   Req.Query = "lol";
+  Req.AnyScope = true;
   Req.Limit = 2;
   EXPECT_THAT(match(*I, Req),
   UnorderedElementsAre("LaughingOutLoud", "LittleOldLady"));
@@ -122,6 +125,7 @@
   MemIndex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
+  Req.AnyScope = true;
   EXPECT_THAT(match(*I, Req), UnorderedElementsAre("a::y1", "b::y2", "y3"));
 }
 
Index: unittests/clangd/DexTests.cpp
===
--- unittests/clangd/DexTests.cpp
+++ unittests/clangd/DexTests.cpp
@@ -485,6 +485,7 @@
   UnorderedElementsAre("other::A", "other::ABC"));
   Req.Query = "";
   Req.Scopes = {};
+  Req.AnyScope = true;
   EXPECT_THAT(match(*Index, Req),
   UnorderedElementsAre("ns::ABC", "ns::BCD", "::ABC",
"ns::nested::ABC", "other::ABC",
@@ -495,6 +496,7 @@
   auto I = Dex::build(generateNumSymbols(0, 100), RefSlab(), URISchemes);
   FuzzyFindRequest Req;
   Req.Query = "5";
+  Req.AnyScope = true;
   Req.Limit = 3;
   bool Incomplete;
   auto Matches = match(*I, Req, &Incomplete);
@@ -509,6 +511,7 @@
   RefSlab(), URISchemes);
   FuzzyFindRequest Req;
   Req.Query = "lol";
+  Req.AnyScope = true;
   Req.Limit = 2;
   EXPECT_THAT(match(*I, Req),
   UnorderedElementsAre("LaughingOutLoud", "LittleOldLady"));
@@ -518,6 +521,7 @@
   auto I =
   Dex::build(generateSymbols({"OneTwoThreeFour"}), RefSlab(), URISchemes);
   FuzzyFindRequest Req;
+  Req.AnyScope = true;
   bool Incomplete;
 
   EXPECT_THAT(match(*I, Req, &Incomplete), ElementsAre("OneTwoThreeFour"));
@@ -540,6 +544,7 @@
   auto I = Dex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab(),
   URISchemes);
   FuzzyFindRequest Req;
+  Req.AnyScope = true;
   Req.Query = "y";
   EXPECT_THAT(match(*I, Req), UnorderedElementsAre("a::y1", "b::y2", "y3"));
 }
@@ -584,9 +589,9 @@
   auto I =
   Dex::build(generateSymbols({"a::y1", "a::b::y2", "c::y3"}), RefSlab(), URISchemes);
   FuzzyFindRequest Req;
+  Req.AnyScope = true;
   Req.Query = "y";
   Req.Scopes = {"a::"};
-  Req.AnyScope = true;
   EXPECT_THAT(match(*I, Req),
   UnorderedElementsAre("a::y1", "a::b::y2", "c::y3"));
 }
@@ -626,6 +631,7 @@
   std::vector Symbols{CodeCompletionSymbol, NonCodeCompletionSymbol};
   Dex I(Symbols, RefSlab(), URISchemes);
   FuzzyFindRequest Req;
+  Req.AnyScope = true;
   Req.RestrictForCodeCompletion = false;
   EXPECT_THAT(match(I, Req), ElementsAre("Completion", "NoCompletion"));
   Req.RestrictForCodeCompletion = true;
@@ -642,6 +648,7 @@
   Dex I(Symbols, RefSlab(), URISchemes);
 
   FuzzyFindRequest Req;
+  Req.AnyScope = true;
   Req.Query = "abc";
   // The best candidate can change depending on the proximity paths.
   Req.Limit = 1;
Index: clangd/index/dex/Dex.cpp
===
--- clangd/index/dex/Dex.cpp
+++ clangd/index/dex/Dex.cpp
@@ -178,7 +178,7 @@
   std::vector> ScopeIterators;
   for (const auto &Scope : Req.Scopes)
 ScopeIterators.push_back(iterator(Token(Token::Kind::Scope, Scope)));
-  if (Req.AnyScope || /*legacy*/ Req.Scopes.empty())
+  if (Req.AnyScope)
 ScopeIterators.push_back(
 Corpus.boost(Corpus.all(), ScopeIterators.empty() ? 1.0 : 0.2));
   Criteria.push_back(Corpus.unionOf(move(ScopeIterators)));
Index: clangd/index/MemIndex.cpp

[clang-tools-extra] r346223 - [clangd] Get rid of QueryScopes.empty() == AnyScope special case.

[Eric Liu via cfe-commits]

Author: ioeric
Date: Tue Nov  6 03:08:17 2018
New Revision: 346223

URL: http://llvm.org/viewvc/llvm-project?rev=346223&view=rev
Log:
[clangd] Get rid of QueryScopes.empty() == AnyScope special case.

Reviewers: sammccall

Reviewed By: sammccall

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

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

Modified:
clang-tools-extra/trunk/clangd/FindSymbols.cpp
clang-tools-extra/trunk/clangd/index/Index.h
clang-tools-extra/trunk/clangd/index/MemIndex.cpp
clang-tools-extra/trunk/clangd/index/dex/Dex.cpp
clang-tools-extra/trunk/unittests/clangd/DexTests.cpp
clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/SyncAPI.cpp

Modified: clang-tools-extra/trunk/clangd/FindSymbols.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/FindSymbols.cpp?rev=346223&r1=346222&r2=346223&view=diff
==
--- clang-tools-extra/trunk/clangd/FindSymbols.cpp (original)
+++ clang-tools-extra/trunk/clangd/FindSymbols.cpp Tue Nov  6 03:08:17 2018
@@ -116,6 +116,8 @@ getWorkspaceSymbols(StringRef Query, int
   // not).
   if (IsGlobalQuery || !Names.first.empty())
 Req.Scopes = {Names.first};
+  else
+Req.AnyScope = true;
   if (Limit)
 Req.Limit = Limit;
   TopN Top(

Modified: clang-tools-extra/trunk/clangd/index/Index.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Index.h?rev=346223&r1=346222&r2=346223&view=diff
==
--- clang-tools-extra/trunk/clangd/index/Index.h (original)
+++ clang-tools-extra/trunk/clangd/index/Index.h Tue Nov  6 03:08:17 2018
@@ -460,9 +460,6 @@ struct FuzzyFindRequest {
   /// namespace xyz::abc.
   ///
   /// The global scope is "", a top level scope is "foo::", etc.
-  /// FIXME: drop the special case for empty list, which is the same as
-  /// `AnyScope = true`.
-  /// FIXME: support scope proximity.
   std::vector Scopes;
   /// If set to true, allow symbols from any scope. Scopes explicitly listed
   /// above will be ranked higher.

Modified: clang-tools-extra/trunk/clangd/index/MemIndex.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/MemIndex.cpp?rev=346223&r1=346222&r2=346223&view=diff
==
--- clang-tools-extra/trunk/clangd/index/MemIndex.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/MemIndex.cpp Tue Nov  6 03:08:17 2018
@@ -39,8 +39,7 @@ bool MemIndex::fuzzyFind(const FuzzyFind
 const Symbol *Sym = Pair.second;
 
 // Exact match against all possible scopes.
-if (!Req.AnyScope && !Req.Scopes.empty() &&
-!is_contained(Req.Scopes, Sym->Scope))
+if (!Req.AnyScope && !is_contained(Req.Scopes, Sym->Scope))
   continue;
 if (Req.RestrictForCodeCompletion &&
 !(Sym->Flags & Symbol::IndexedForCodeCompletion))

Modified: clang-tools-extra/trunk/clangd/index/dex/Dex.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/dex/Dex.cpp?rev=346223&r1=346222&r2=346223&view=diff
==
--- clang-tools-extra/trunk/clangd/index/dex/Dex.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/dex/Dex.cpp Tue Nov  6 03:08:17 2018
@@ -178,7 +178,7 @@ bool Dex::fuzzyFind(const FuzzyFindReque
   std::vector> ScopeIterators;
   for (const auto &Scope : Req.Scopes)
 ScopeIterators.push_back(iterator(Token(Token::Kind::Scope, Scope)));
-  if (Req.AnyScope || /*legacy*/ Req.Scopes.empty())
+  if (Req.AnyScope)
 ScopeIterators.push_back(
 Corpus.boost(Corpus.all(), ScopeIterators.empty() ? 1.0 : 0.2));
   Criteria.push_back(Corpus.unionOf(move(ScopeIterators)));

Modified: clang-tools-extra/trunk/unittests/clangd/DexTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/DexTests.cpp?rev=346223&r1=346222&r2=346223&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/DexTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/DexTests.cpp Tue Nov  6 03:08:17 
2018
@@ -485,6 +485,7 @@ TEST(Dex, FuzzyFind) {
   UnorderedElementsAre("other::A", "other::ABC"));
   Req.Query = "";
   Req.Scopes = {};
+  Req.AnyScope = true;
   EXPECT_THAT(match(*Index, Req),
   UnorderedElementsAre("ns::ABC", "ns::BCD", "::ABC",
"ns::nested::ABC", "other::ABC",
@@ -495,6 +496,7 @@ TEST(DexTest, DexLimitedNumMatches) {
   auto I = Dex::build(generateNumSymbols(0, 100), RefSlab(), URISchemes);
   FuzzyFindRequest Req;
   Req.Query = "5";
+  Req.AnyScope = true;
   Req.Limit = 3;
   bool Incomplete;
   auto Matches = match(*I, Req, &Incomplet

[PATCH] D53933: [clangd] Get rid of QueryScopes.empty() == AnyScope special case.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE346223: [clangd] Get rid of QueryScopes.empty() == 
AnyScope special case. (authored by ioeric, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D53933?vs=172735&id=172737#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53933

Files:
  clangd/FindSymbols.cpp
  clangd/index/Index.h
  clangd/index/MemIndex.cpp
  clangd/index/dex/Dex.cpp
  unittests/clangd/DexTests.cpp
  unittests/clangd/IndexTests.cpp
  unittests/clangd/SyncAPI.cpp

Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -130,6 +130,7 @@
 SymbolSlab runFuzzyFind(const SymbolIndex &Index, StringRef Query) {
   FuzzyFindRequest Req;
   Req.Query = Query;
+  Req.AnyScope = true;
   return runFuzzyFind(Index, Req);
 }
 
Index: unittests/clangd/DexTests.cpp
===
--- unittests/clangd/DexTests.cpp
+++ unittests/clangd/DexTests.cpp
@@ -485,6 +485,7 @@
   UnorderedElementsAre("other::A", "other::ABC"));
   Req.Query = "";
   Req.Scopes = {};
+  Req.AnyScope = true;
   EXPECT_THAT(match(*Index, Req),
   UnorderedElementsAre("ns::ABC", "ns::BCD", "::ABC",
"ns::nested::ABC", "other::ABC",
@@ -495,6 +496,7 @@
   auto I = Dex::build(generateNumSymbols(0, 100), RefSlab(), URISchemes);
   FuzzyFindRequest Req;
   Req.Query = "5";
+  Req.AnyScope = true;
   Req.Limit = 3;
   bool Incomplete;
   auto Matches = match(*I, Req, &Incomplete);
@@ -509,6 +511,7 @@
   RefSlab(), URISchemes);
   FuzzyFindRequest Req;
   Req.Query = "lol";
+  Req.AnyScope = true;
   Req.Limit = 2;
   EXPECT_THAT(match(*I, Req),
   UnorderedElementsAre("LaughingOutLoud", "LittleOldLady"));
@@ -518,6 +521,7 @@
   auto I =
   Dex::build(generateSymbols({"OneTwoThreeFour"}), RefSlab(), URISchemes);
   FuzzyFindRequest Req;
+  Req.AnyScope = true;
   bool Incomplete;
 
   EXPECT_THAT(match(*I, Req, &Incomplete), ElementsAre("OneTwoThreeFour"));
@@ -540,6 +544,7 @@
   auto I = Dex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab(),
   URISchemes);
   FuzzyFindRequest Req;
+  Req.AnyScope = true;
   Req.Query = "y";
   EXPECT_THAT(match(*I, Req), UnorderedElementsAre("a::y1", "b::y2", "y3"));
 }
@@ -584,9 +589,9 @@
   auto I =
   Dex::build(generateSymbols({"a::y1", "a::b::y2", "c::y3"}), RefSlab(), URISchemes);
   FuzzyFindRequest Req;
+  Req.AnyScope = true;
   Req.Query = "y";
   Req.Scopes = {"a::"};
-  Req.AnyScope = true;
   EXPECT_THAT(match(*I, Req),
   UnorderedElementsAre("a::y1", "a::b::y2", "c::y3"));
 }
@@ -626,6 +631,7 @@
   std::vector Symbols{CodeCompletionSymbol, NonCodeCompletionSymbol};
   Dex I(Symbols, RefSlab(), URISchemes);
   FuzzyFindRequest Req;
+  Req.AnyScope = true;
   Req.RestrictForCodeCompletion = false;
   EXPECT_THAT(match(I, Req), ElementsAre("Completion", "NoCompletion"));
   Req.RestrictForCodeCompletion = true;
@@ -642,6 +648,7 @@
   Dex I(Symbols, RefSlab(), URISchemes);
 
   FuzzyFindRequest Req;
+  Req.AnyScope = true;
   Req.Query = "abc";
   // The best candidate can change depending on the proximity paths.
   Req.Limit = 1;
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -90,14 +90,16 @@
  symbol("2") /* duplicate */};
   FuzzyFindRequest Req;
   Req.Query = "2";
+  Req.AnyScope = true;
   MemIndex I(Symbols, RefSlab());
   EXPECT_THAT(match(I, Req), ElementsAre("2"));
 }
 
 TEST(MemIndexTest, MemIndexLimitedNumMatches) {
   auto I = MemIndex::build(generateNumSymbols(0, 100), RefSlab());
   FuzzyFindRequest Req;
   Req.Query = "5";
+  Req.AnyScope = true;
   Req.Limit = 3;
   bool Incomplete;
   auto Matches = match(*I, Req, &Incomplete);
@@ -112,6 +114,7 @@
   RefSlab());
   FuzzyFindRequest Req;
   Req.Query = "lol";
+  Req.AnyScope = true;
   Req.Limit = 2;
   EXPECT_THAT(match(*I, Req),
   UnorderedElementsAre("LaughingOutLoud", "LittleOldLady"));
@@ -122,6 +125,7 @@
   MemIndex::build(generateSymbols({"a::y1", "b::y2", "y3"}), RefSlab());
   FuzzyFindRequest Req;
   Req.Query = "y";
+  Req.AnyScope = true;
   EXPECT_THAT(match(*I, Req), UnorderedElementsAre("a::y1", "b::y2", "y3"));
 }
 
Index: clangd/FindSymbols.cpp
===
--- clangd/FindSymbols.cpp
+++ clangd/FindSymbols.cpp
@@ -116,6 +116,8 @@
   // not).
   if (IsGlobalQuery || !Names.first.empty())
 Req.Scopes = {Names.first};
+  else
+Req.AnyScope = true;
   if (Limit)
 Req.Limit = Limit;
   TopN Top(
Index: clangd/index/Index.h
===
--- 

[PATCH] D54106: [clangd] Limit the index-returned results in dexp.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 172736.
hokein added a comment.

Update based on the offline discussion.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54106

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


Index: clangd/index/dex/dexp/Dexp.cpp
===
--- clangd/index/dex/dexp/Dexp.cpp
+++ clangd/index/dex/dexp/Dexp.cpp
@@ -50,8 +50,10 @@
 }
 
 std::vector getSymbolIDsFromIndex(StringRef QualifiedName,
-const SymbolIndex *Index) {
+const SymbolIndex *Index,
+unsigned Limit) {
   FuzzyFindRequest Request;
+  Request.Limit = Limit;
   // Remove leading "::" qualifier as FuzzyFind doesn't need leading "::"
   // qualifier for global scope.
   bool IsGlobalScope = QualifiedName.consume_front("::");
@@ -158,6 +160,12 @@
   cl::opt Name{
   "name", cl::desc("Qualified name to look up."),
   };
+  cl::opt Limit{
+  "limit",
+  cl::init(10),
+  cl::desc("Max results to display. This flag is only meaningful when 
-name"
+   " is set."),
+  };
 
   void run() override {
 if (ID.getNumOccurrences() == 0 && Name.getNumOccurrences() == 0) {
@@ -173,7 +181,7 @@
   }
   IDs.push_back(*SID);
 } else {
-  IDs = getSymbolIDsFromIndex(Name, Index);
+  IDs = getSymbolIDsFromIndex(Name, Index, Limit);
 }
 
 LookupRequest Request;
@@ -216,7 +224,13 @@
   }
   IDs.push_back(*SID);
 } else {
-  IDs = getSymbolIDsFromIndex(Name, Index);
+  IDs = getSymbolIDsFromIndex(Name, Index, /*Limit=*/1);
+  if (IDs.size() != 1) {
+outs() << formatv("The name {0} is ambiguous, found {1} different "
+  "symbols. Please use id flag to disambiguate.\n",
+  Name, IDs.size());
+return;
+  }
 }
 RefsRequest RefRequest;
 RefRequest.IDs.insert(IDs.begin(), IDs.end());


Index: clangd/index/dex/dexp/Dexp.cpp
===
--- clangd/index/dex/dexp/Dexp.cpp
+++ clangd/index/dex/dexp/Dexp.cpp
@@ -50,8 +50,10 @@
 }
 
 std::vector getSymbolIDsFromIndex(StringRef QualifiedName,
-const SymbolIndex *Index) {
+const SymbolIndex *Index,
+unsigned Limit) {
   FuzzyFindRequest Request;
+  Request.Limit = Limit;
   // Remove leading "::" qualifier as FuzzyFind doesn't need leading "::"
   // qualifier for global scope.
   bool IsGlobalScope = QualifiedName.consume_front("::");
@@ -158,6 +160,12 @@
   cl::opt Name{
   "name", cl::desc("Qualified name to look up."),
   };
+  cl::opt Limit{
+  "limit",
+  cl::init(10),
+  cl::desc("Max results to display. This flag is only meaningful when -name"
+   " is set."),
+  };
 
   void run() override {
 if (ID.getNumOccurrences() == 0 && Name.getNumOccurrences() == 0) {
@@ -173,7 +181,7 @@
   }
   IDs.push_back(*SID);
 } else {
-  IDs = getSymbolIDsFromIndex(Name, Index);
+  IDs = getSymbolIDsFromIndex(Name, Index, Limit);
 }
 
 LookupRequest Request;
@@ -216,7 +224,13 @@
   }
   IDs.push_back(*SID);
 } else {
-  IDs = getSymbolIDsFromIndex(Name, Index);
+  IDs = getSymbolIDsFromIndex(Name, Index, /*Limit=*/1);
+  if (IDs.size() != 1) {
+outs() << formatv("The name {0} is ambiguous, found {1} different "
+  "symbols. Please use id flag to disambiguate.\n",
+  Name, IDs.size());
+return;
+  }
 }
 RefsRequest RefRequest;
 RefRequest.IDs.insert(IDs.begin(), IDs.end());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54105: [clangd] Deduplicate query scopes.

[Ilya Biryukov via Phabricator via cfe-commits]

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

LGTM


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54105



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

[PATCH] D54111: [clang-format] Do not treat the asm clobber [ as ObjCExpr

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir updated this revision to Diff 172738.
krasimir added a comment.

- Address review comments


Repository:
  rC Clang

https://reviews.llvm.org/D54111

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -12753,6 +12753,21 @@
   guessLanguage("foo.h", "int(^foo[(kNumEntries + 10)])(char, float);"));
 }
 
+TEST_F(FormatTest, GuessedLanguageWithInlineAsmClobbers) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h",
+   "void f() {\n"
+   "  asm (\"mov %[e], %[d]\"\n"
+   " : [d] \"=rm\" (d)\n"
+   "   [e] \"rm\" (*e));\n"
+   "}"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "void f() {\n"
+   "  asm volatile (\"mov %[e], %[d]\"\n"
+   " : [d] \"=rm\" (d)\n"
+   "   [e] \"rm\" (*e));\n"
+   "}"));
+}
+
 TEST_F(FormatTest, GuessLanguageWithChildLines) {
   EXPECT_EQ(FormatStyle::LK_Cpp,
 guessLanguage("foo.h", "#define FOO ({ std::string s; })"));
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -403,8 +403,9 @@
 Contexts.back().CanBeExpression && Left->isNot(TT_LambdaLSquare) &&
 !CurrentToken->isOneOf(tok::l_brace, tok::r_square) &&
 (!Parent ||
- Parent->isOneOf(tok::colon, tok::l_square, tok::l_paren,
- tok::kw_return, tok::kw_throw) ||
+ (Parent->is(tok::colon) && Parent->isNot(TT_InlineASMColon)) ||
+ Parent->isOneOf(tok::l_square, tok::l_paren, tok::kw_return,
+ tok::kw_throw) ||
  Parent->isUnaryOperator() ||
  // FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen.
  Parent->isOneOf(TT_ObjCForIn, TT_CastRParen) ||


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -12753,6 +12753,21 @@
   guessLanguage("foo.h", "int(^foo[(kNumEntries + 10)])(char, float);"));
 }
 
+TEST_F(FormatTest, GuessedLanguageWithInlineAsmClobbers) {
+  EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h",
+   "void f() {\n"
+   "  asm (\"mov %[e], %[d]\"\n"
+   " : [d] \"=rm\" (d)\n"
+   "   [e] \"rm\" (*e));\n"
+   "}"));
+  EXPECT_EQ(FormatStyle::LK_Cpp,
+guessLanguage("foo.h", "void f() {\n"
+   "  asm volatile (\"mov %[e], %[d]\"\n"
+   " : [d] \"=rm\" (d)\n"
+   "   [e] \"rm\" (*e));\n"
+   "}"));
+}
+
 TEST_F(FormatTest, GuessLanguageWithChildLines) {
   EXPECT_EQ(FormatStyle::LK_Cpp,
 guessLanguage("foo.h", "#define FOO ({ std::string s; })"));
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -403,8 +403,9 @@
 Contexts.back().CanBeExpression && Left->isNot(TT_LambdaLSquare) &&
 !CurrentToken->isOneOf(tok::l_brace, tok::r_square) &&
 (!Parent ||
- Parent->isOneOf(tok::colon, tok::l_square, tok::l_paren,
- tok::kw_return, tok::kw_throw) ||
+ (Parent->is(tok::colon) && Parent->isNot(TT_InlineASMColon)) ||
+ Parent->isOneOf(tok::l_square, tok::l_paren, tok::kw_return,
+ tok::kw_throw) ||
  Parent->isUnaryOperator() ||
  // FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen.
  Parent->isOneOf(TT_ObjCForIn, TT_CastRParen) ||
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54106: [clangd] Limit the index-returned results in dexp.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/index/dex/dexp/Dexp.cpp:166
+  cl::init(10),
+  cl::desc("Max results to display. This flag is only meaningful when 
-name"
+   " is set."),

Maybe `The max number of symbols with the same name but different IDs to 
display. Only applies when -name is set.`? I think it's worth calling out that 
there can be different IDs for the same name.



Comment at: clangd/index/dex/dexp/Dexp.cpp:227
 } else {
-  IDs = getSymbolIDsFromIndex(Name, Index);
+  IDs = getSymbolIDsFromIndex(Name, Index, /*Limit=*/1);
+  if (IDs.size() != 1) {

I think you would want to set limit to >1. Currently, the error condition will 
never be triggered?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54106



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

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

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:1018
+auto It = CurrExpArgTokens.begin();
+while (It != CurrExpArgTokens.end()) {
+  if (It->isNot(tok::identifier)) {

xazax.hun wrote:
> Maybe a for loop mor natural here?
I asked this one already in the earlier (non-split) diff and the reason for the 
`while` is that there are a lot of conditional moves, advancements and even an 
`erase` call in the loop body.


https://reviews.llvm.org/D52795



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

[clang-tools-extra] r346224 - [clangd] Deduplicate query scopes.

[Eric Liu via cfe-commits]

Author: ioeric
Date: Tue Nov  6 03:17:40 2018
New Revision: 346224

URL: http://llvm.org/viewvc/llvm-project?rev=346224&view=rev
Log:
[clangd] Deduplicate query scopes.

Summary:
For example, when anonymous namespace is present, duplicated namespaces might be
generated for the enclosing namespace.

Reviewers: ilya-biryukov

Subscribers: MaskRay, jkorous, arphaman, kadircet, cfe-commits

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

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

Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=346224&r1=346223&r2=346224&view=diff
==
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original)
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Tue Nov  6 03:17:40 2018
@@ -541,16 +541,14 @@ struct SpecifiedScope {
   // Set if the qualifier is not fully resolved by Sema.
   Optional UnresolvedQualifier;
 
-  // Construct scopes being queried in indexes.
+  // Construct scopes being queried in indexes. The results are deduplicated.
   // This method format the scopes to match the index request representation.
   std::vector scopesForIndexQuery() {
-std::vector Results;
-for (StringRef AS : AccessibleScopes) {
-  Results.push_back(AS);
-  if (UnresolvedQualifier)
-Results.back() += *UnresolvedQualifier;
-}
-return Results;
+std::set Results;
+for (StringRef AS : AccessibleScopes)
+  Results.insert(
+  ((UnresolvedQualifier ? *UnresolvedQualifier : "") + AS).str());
+return {Results.begin(), Results.end()};
   }
 };
 

Modified: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp?rev=346224&r1=346223&r2=346224&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp Tue Nov  6 
03:17:40 2018
@@ -1142,6 +1142,23 @@ TEST(CompletionTest, GlobalQualifiedQuer
   UnorderedElementsAre("";
 }
 
+TEST(CompletionTest, NoDuplicatedQueryScopes) {
+  auto Requests = captureIndexRequests(R"cpp(
+  namespace {}
+
+  namespace na {
+  namespace {}
+  namespace nb {
+  ^
+  } // namespace nb
+  } // namespace na
+  )cpp");
+
+  EXPECT_THAT(Requests,
+  ElementsAre(Field(&FuzzyFindRequest::Scopes,
+UnorderedElementsAre("na::", "na::nb::", 
"";
+}
+
 TEST(CompletionTest, NoIndexCompletionsInsideClasses) {
   auto Completions = completions(
   R"cpp(


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

[PATCH] D54077: [clangd] Implemented DraftFileSystem

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D54077#1287289, @klimek wrote:

> don't most IDEs show whether a file is saved or just modified?


They do, but whenever you run the build from them, they will save all modified 
files before actually running it.

In https://reviews.llvm.org/D54077#1288455, @LutsenkoDanil wrote:

> I already made a patch which introduces such behavior (not uploaded here 
> yet), and looks like it works well with draft fs: diagnostics updates for 
> depended files in 'real-time' on typing for opened files and no seen 
> performance glitches, in multi-threaded mode at least.
>  I suggest continue discussion when/if dependencies tracking will be 
> implemented and real performance reduce introduced by this patch can be 
> checked with real code.


Exciting! Please send it out! I'm also starting to look into this, but my work 
is mostly focused on watching files in the filesystem.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54077



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

[PATCH] D54105: [clangd] Deduplicate query scopes.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346224: [clangd] Deduplicate query scopes. (authored by 
ioeric, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D54105

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


Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -1142,6 +1142,23 @@
   UnorderedElementsAre("";
 }
 
+TEST(CompletionTest, NoDuplicatedQueryScopes) {
+  auto Requests = captureIndexRequests(R"cpp(
+  namespace {}
+
+  namespace na {
+  namespace {}
+  namespace nb {
+  ^
+  } // namespace nb
+  } // namespace na
+  )cpp");
+
+  EXPECT_THAT(Requests,
+  ElementsAre(Field(&FuzzyFindRequest::Scopes,
+UnorderedElementsAre("na::", "na::nb::", 
"";
+}
+
 TEST(CompletionTest, NoIndexCompletionsInsideClasses) {
   auto Completions = completions(
   R"cpp(
Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -541,16 +541,14 @@
   // Set if the qualifier is not fully resolved by Sema.
   Optional UnresolvedQualifier;
 
-  // Construct scopes being queried in indexes.
+  // Construct scopes being queried in indexes. The results are deduplicated.
   // This method format the scopes to match the index request representation.
   std::vector scopesForIndexQuery() {
-std::vector Results;
-for (StringRef AS : AccessibleScopes) {
-  Results.push_back(AS);
-  if (UnresolvedQualifier)
-Results.back() += *UnresolvedQualifier;
-}
-return Results;
+std::set Results;
+for (StringRef AS : AccessibleScopes)
+  Results.insert(
+  ((UnresolvedQualifier ? *UnresolvedQualifier : "") + AS).str());
+return {Results.begin(), Results.end()};
   }
 };
 


Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -1142,6 +1142,23 @@
   UnorderedElementsAre("";
 }
 
+TEST(CompletionTest, NoDuplicatedQueryScopes) {
+  auto Requests = captureIndexRequests(R"cpp(
+  namespace {}
+
+  namespace na {
+  namespace {}
+  namespace nb {
+  ^
+  } // namespace nb
+  } // namespace na
+  )cpp");
+
+  EXPECT_THAT(Requests,
+  ElementsAre(Field(&FuzzyFindRequest::Scopes,
+UnorderedElementsAre("na::", "na::nb::", "";
+}
+
 TEST(CompletionTest, NoIndexCompletionsInsideClasses) {
   auto Completions = completions(
   R"cpp(
Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -541,16 +541,14 @@
   // Set if the qualifier is not fully resolved by Sema.
   Optional UnresolvedQualifier;
 
-  // Construct scopes being queried in indexes.
+  // Construct scopes being queried in indexes. The results are deduplicated.
   // This method format the scopes to match the index request representation.
   std::vector scopesForIndexQuery() {
-std::vector Results;
-for (StringRef AS : AccessibleScopes) {
-  Results.push_back(AS);
-  if (UnresolvedQualifier)
-Results.back() += *UnresolvedQualifier;
-}
-return Results;
+std::set Results;
+for (StringRef AS : AccessibleScopes)
+  Results.insert(
+  ((UnresolvedQualifier ? *UnresolvedQualifier : "") + AS).str());
+return {Results.begin(), Results.end()};
   }
 };
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53334: [Frontend/Modules] Show diagnostics on prebuilt module configuration mismatch too

[Whisperity via Phabricator via cfe-commits]

whisperity planned changes to this revision.
whisperity added a comment.

In https://reviews.llvm.org/D53334#1288057, @dblaikie wrote:

> In https://reviews.llvm.org/D53334#1273877, @whisperity wrote:
>
> > @dblaikie I have created a test, but unfortunately `%clang_cpp` in LIT 
> > invokes `clang --driver-mode=cpp` which is not the same as if `clang++` is 
> > called. I'm trying to construct the following command-line
> >
> > `clang++ -fmodules-ts -fprebuilt-module-path=%t/mods --precompile -c 
> > file.cppm -o file.pcm`
> >
> > However, using `%clang_cc1` I can't get it to accept `--precompile` as a 
> > valid argument, and using `%clang_cpp` I get an "unused argument" warning 
> > for `--precompile` and the output file is just a preprocessed output (like 
> > `-E`), which will, of course, cause errors, but not the errors I am wanting 
> > to test about.
>
>
> Hey, sorry for the delay - you can use "clang -### " (or 
> "clang++ -### " to get clang to print out the underlying 
> -cc1 command line that is used.
>
> If you're changing the driver then we'd write a driver test (that tests that, 
> given "clang -foo -###" it produces some "clang -cc1 -bar" command line to 
> run the frontend.
>
> But since you're changing the driver, it's not interesting to (re) test how 
> --precompile is lowered from the driver to cc1. Instead we test the frontend 
> (cc1) directly.


Understood, thank you for the help, I'll try looking into this. It was just at 
first a strange behaviour to see that conventionally an external call command 
messes up and behaves different.
Until then I'll re-mark this as a WIP because the tests are half-baked anyways.


Repository:
  rC Clang

https://reviews.llvm.org/D53334



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

[PATCH] D52984: [analyzer] Checker reviewer's checklist

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

Szelethus added a comment.

We probably should also add an entry about some code conventions we use here, 
for example, the use of `auto` was debated recently when used with 
`SVal::getAs`, maybe something like this:

- As an LLVM subproject, the code in the Static Analyzer should too follow the 
https://llvm.org/docs/CodingStandards.html, but we do have some further 
restrictions/additions to that:
  - Prefer the usage of `auto` when using `SVal::getAs`, and `const auto *` 
when using `MemRegion::getAs`. The coding standard states that "//Use auto if 
and only if it makes the code more readable or easier to maintain.//", and even 
though `SVal::getAs` returns with `llvm::Optional`, which is not 
completely trivial, `SVal` is one of the most heavily used types in the Static 
Analyzer, and this convention makes the code significantly more readable.
  - Use `llvm::SmallString` and `llvm::raw_svector_ostream` to construct report 
messages. The coding standard explicitly **doesn't** forbid the use of the 
`` library, but we do.
  - Do not forward declare or define static functions inside an anonymous 
namespace in checker files.
  - Document checkers on top of the file, rather then with comments before the 
checker class.
  - Checker files usually contain the checker class, the registration 
functions, and of course the actual implementation (usually several more 
functions and data structures) of the checker logic. To make the code more 
readable, keep the checker class on top of the file, forward declare all data 
structures and implementation functions below that, and put the registration 
function on the very bottom. The actual logic should be in between. For example:

  //===- MyChecker.cpp -*- C++ 
-*-==//
  //
  // The LLVM Compiler Infrastructure
  //
  // This file is distributed under the University of Illinois Open Source
  // License. See LICENSE.TXT for details.
  //
  
//===--===//
  //
  // MyChecker is a tool to show an example how a checker file should be
  // organized.
  //
  
//===--===//
  
  #include "ClangSACheckers.h"
  #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
  #include "clang/StaticAnalyzer/Core/Checker.h"
  #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
  
  namespace {
  
  // Any documentation you'd like to put here should instead be put on top of 
the
  // file, where the checker is described.
  class MyChecker : public Checker {
  public:
MyChecker() = default;
void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const;
  };
  
  /// Helper class that is only here for this example.
  class Helper {
  public:
/// Helps.
void help();
  };
  
  } // end of anonymous namespace
  
  /// Determines whether the problem is solvable.
  static bool isProblemSolvable();
  
  
//===--===//
  // Methods for MyChecker.
  
//===--===//
  
  void MyChecker::checkEndFunction(const ReturnStmt *RS,
   CheckerContext &C) const {
// TODO: Implement the logic.
  }
  
  
//===--===//
  // Methods for Helper.
  
//===--===//
  
  void Helper::help() {
// TODO: Actually help.
  }
  
  
//===--===//
  // Utility functions.
  
//===--===//
  
  static bool isProblemSolvable() {
return false;
  }
  
  void ento::registerMyChecker(CheckerManager &Mgr) {
auto Chk = Mgr.registerChecker();
  }


https://reviews.llvm.org/D52984



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

[PATCH] D52984: [analyzer] Checker reviewer's checklist

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

Szelethus added a comment.

Also, context is missing :)


https://reviews.llvm.org/D52984



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

Re: r346167 - [Driver] Reland again again: Default Android toolchains to libc++.

[Hans Wennborg via cfe-commits]

Just fyi: this broke Chrome's build of compiler-rt using tot Clang
against the sysroot generated by NDK r16's (I think)
make_standalong_toolchain.py, see https://crbug.com/902270

It looks like we'll just hack around it though.

On Mon, Nov 5, 2018 at 9:57 PM, Dan Albert via cfe-commits
 wrote:
> Author: danalbert
> Date: Mon Nov  5 12:57:46 2018
> New Revision: 346167
>
> URL: http://llvm.org/viewvc/llvm-project?rev=346167&view=rev
> Log:
> [Driver] Reland again again: Default Android toolchains to libc++.
>
> Landed more fixes to the compiler-rt Android tests.
>
> Original review was https://reviews.llvm.org/D53109.
>
> Added:
> 
> cfe/trunk/test/Driver/Inputs/basic_android_ndk_tree/sysroot/usr/include/c++/v1/.keep
> Modified:
> cfe/trunk/lib/Driver/ToolChains/Linux.cpp
> cfe/trunk/lib/Driver/ToolChains/Linux.h
> cfe/trunk/test/Driver/android-ndk-standalone.cpp
>
> Modified: cfe/trunk/lib/Driver/ToolChains/Linux.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Linux.cpp?rev=346167&r1=346166&r2=346167&view=diff
> ==
> --- cfe/trunk/lib/Driver/ToolChains/Linux.cpp (original)
> +++ cfe/trunk/lib/Driver/ToolChains/Linux.cpp Mon Nov  5 12:57:46 2018
> @@ -443,6 +443,12 @@ Linux::Linux(const Driver &D, const llvm
>addPathIfExists(D, SysRoot + "/usr/lib", Paths);
>  }
>
> +ToolChain::CXXStdlibType Linux::GetDefaultCXXStdlibType() const {
> +  if (getTriple().isAndroid())
> +return ToolChain::CST_Libcxx;
> +  return ToolChain::CST_Libstdcxx;
> +}
> +
>  bool Linux::HasNativeLLVMSupport() const { return true; }
>
>  Tool *Linux::buildLinker() const { return new 
> tools::gnutools::Linker(*this); }
>
> Modified: cfe/trunk/lib/Driver/ToolChains/Linux.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Linux.h?rev=346167&r1=346166&r2=346167&view=diff
> ==
> --- cfe/trunk/lib/Driver/ToolChains/Linux.h (original)
> +++ cfe/trunk/lib/Driver/ToolChains/Linux.h Mon Nov  5 12:57:46 2018
> @@ -37,6 +37,7 @@ public:
>llvm::opt::ArgStringList &CC1Args) const override;
>void AddIAMCUIncludeArgs(const llvm::opt::ArgList &DriverArgs,
> llvm::opt::ArgStringList &CC1Args) const override;
> +  CXXStdlibType GetDefaultCXXStdlibType() const override;
>bool isPIEDefault() const override;
>bool IsMathErrnoDefault() const override;
>SanitizerMask getSupportedSanitizers() const override;
>
> Added: 
> cfe/trunk/test/Driver/Inputs/basic_android_ndk_tree/sysroot/usr/include/c++/v1/.keep
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/Inputs/basic_android_ndk_tree/sysroot/usr/include/c%2B%2B/v1/.keep?rev=346167&view=auto
> ==
> (empty)
>
> Modified: cfe/trunk/test/Driver/android-ndk-standalone.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/android-ndk-standalone.cpp?rev=346167&r1=346166&r2=346167&view=diff
> ==
> --- cfe/trunk/test/Driver/android-ndk-standalone.cpp (original)
> +++ cfe/trunk/test/Driver/android-ndk-standalone.cpp Mon Nov  5 12:57:46 2018
> @@ -2,21 +2,13 @@
>  // toolchain.
>  //
>  // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
> -// RUN: -target arm-linux-androideabi21 -stdlib=libstdc++ \
> +// RUN: -target arm-linux-androideabi21 \
>  // RUN: -B%S/Inputs/basic_android_ndk_tree \
>  // RUN: --sysroot=%S/Inputs/basic_android_ndk_tree/sysroot \
>  // RUN:   | FileCheck  %s
>  // CHECK: {{.*}}clang{{.*}}" "-cc1"
>  // CHECK: "-resource-dir" "[[RESOURCE_DIR:[^"]+]]"
> -// CHECK: "-internal-isystem" "{{.*}}/include/c++/4.9"
> -// CHECK-NOT: "-internal-isystem" 
> "{{.*}}/include/c++/4.9/arm-linux-androideabi/armv7-a/thumb"
> -// CHECK-NOT: "-internal-isystem" 
> "{{.*}}/include/c++/4.9/arm-linux-androideabi/armv7-a"
> -// CHECK-NOT: "-internal-isystem" 
> "{{.*}}/include/c++/4.9/arm-linux-androideabi/thumb"
> -// CHECK: "-internal-isystem" "{{.*}}/include/c++/4.9/arm-linux-androideabi"
> -// CHECK-NOT: "-internal-isystem" 
> "{{.*}}/include/c++/4.9/arm-linux-androideabi/armv7-a/thumb"
> -// CHECK-NOT: "-internal-isystem" 
> "{{.*}}/include/c++/4.9/arm-linux-androideabi/armv7-a"
> -// CHECK-NOT: "-internal-isystem" 
> "{{.*}}/include/c++/4.9/arm-linux-androideabi/thumb"
> -// CHECK: "-internal-isystem" "{{.*}}/include/c++/4.9/backward"
> +// CHECK: "-internal-isystem" "{{.*}}/include/c++/v1"
>  // CHECK: "-internal-isystem" "{{.*}}/sysroot/usr/local/include"
>  // CHECK: "-internal-isystem" "[[RESOURCE_DIR]]{{(/|)}}include"
>  // CHECK: "-internal-externc-isystem" 
> "{{.*}}/sysroot/usr/include/arm-linux-androideabi"
> @@ -49,21 +41,47 @@
>  // CHECK-14: "-L{{.*}}/sysroot/usr/lib/arm-linux-and

[PATCH] D51729: [Tooling] JSONCompilationDatabasePlugin infers compile commands for missing files

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn added a comment.

In https://reviews.llvm.org/D51729#1288360, @sammccall wrote:

> > I'm not entirely sure what to do here. The old behavior works great in 
> > cases where a complete database is available (produced by CMake). The new 
> > behavior might work better for clangd (?), but it breaks a use case (see 
> > above).
>
> For clangd, but also clang-tidy and clang-query when the user *does* want to 
> use it on files not represented in the CDB. (e.g. stale or headers)
>  There's indeed a tension here, because the CDB discovery needs to have a 
> default configuration.


Support for querying header files seems valuable and a good argument to keep 
this change.

> That said, in this case the behavior looks appropriate to me: you've 
> explicitly specified files on the command line, ignoring them and returning 
> with status 0 seems surprising.

The test explicitly lists them, but in practice the `find` or `grep -rl` 
provides the file names which might contain uninteresting matches that are not 
present in the CDB. As a compromise, it could return with status 1 but proceed 
querying files that were successfully parsed.

> For the case of "search over all TUs in CDB", the CDB does offer the ability 
> to list TUs and iterate over compile commands, and ClangTool lets you run in 
> this mode. We've discussed in the past adding a filename filter to 
> `AllTUsExecutor`, which would be useful for this purpose and others. @ioeric

`AllTUsToolExecutor` looks useful to enable concurrency, but a filename filter 
would not help in my case. The reason I do a "cheap" grep first before using 
clang-query is because building the AST is slow and consumes a lot of memory 
(after ~1k .c files, 12G was in use).

Querying the CDB and filtering out entries is currently possible, but quite 
verbose:

  grep -le 'search term' $(jq -r '.[].file' compile_commands.json) | xargs 
clang-query -c 'm ...'

Thank you for the feedback, my concerns with this CDB patch has been resolved.


Repository:
  rC Clang

https://reviews.llvm.org/D51729



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

[PATCH] D53457: clang-cl: Add "/clang:" pass-through arg support.

[Hans Wennborg via Phabricator via cfe-commits]

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

Okay, looks good to me (with a small nit).




Comment at: docs/UsersManual.rst:2852
   /Brepro Emit an object file which can be reproduced over 
time
+  /clang:Pass  to the clang driver
   /C  Don't discard comments when preprocessing

ultra nit: no space between the colon and 



Comment at: test/Driver/cl-options.c:619
+
+// RUN: %clang_cl -O2 -### -- %s 2>&1 | FileCheck -check-prefix=NOCLANG %s
+// NOCLANG: "--dependent-lib=libcmt"

I'm not sure this test adds much value..


https://reviews.llvm.org/D53457



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

Re: r345591 - [CodeGen] Disable the machine verifier on a ThinLTO test

[Francis Visoiu Mistrih via cfe-commits]

Thanks for the suggestion, I think it’s reasonable since it’s all due to:

* ICALL_BRANCH_FUNNEL: Bad machine code: Explicit definition marked as use 
https://bugs.llvm.org/show_bug.cgi?id=39436 
.

I’ll look into it.

> On 5 Nov 2018, at 19:13, David Blaikie  wrote:
> 
> If ThinLTO doesn't pass the machine verifier - should it maybe be turned off 
> at the thinlto level in general, rather than for this specific test?
> 
> On Tue, Oct 30, 2018 at 5:20 AM Francis Visoiu Mistrih via cfe-commits 
> mailto:cfe-commits@lists.llvm.org>> wrote:
> Author: thegameg
> Date: Tue Oct 30 05:18:33 2018
> New Revision: 345591
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=345591&view=rev 
> 
> Log:
> [CodeGen] Disable the machine verifier on a ThinLTO test
> 
> This allows us to turn the machine verifier on by default on X86.
> 
> Modified:
> cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll
> 
> Modified: cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll?rev=345591&r1=345590&r2=345591&view=diff
>  
> 
> ==
> --- cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll (original)
> +++ cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll Tue Oct 30 
> 05:18:33 2018
> @@ -6,7 +6,9 @@
> 
>  ; RUN: opt -thinlto-bc -o %t.o %s
> 
> +; FIXME: Fix machine verifier issues and remove -verify-machineinstrs=0. 
> PR39436.
>  ; RUN: llvm-lto2 run -thinlto-distributed-indexes %t.o \
> +; RUN:   -verify-machineinstrs=0 \
>  ; RUN:   -o %t2.index \
>  ; RUN:   -r=%t.o,test,px \
>  ; RUN:   -r=%t.o,_ZN1A1nEi,p \
> 
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org 
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits 
> 

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

[PATCH] D54109: [clang-query] continue querying even if files are skipped

[Peter Wu via Phabricator via cfe-commits]

Lekensteyn updated this revision to Diff 172747.
Lekensteyn retitled this revision from "[clang-query] continue even if files 
are skipped" to "[clang-query] continue querying even if files are skipped".
Lekensteyn added a comment.

Changes:

- Return 1 (instead of 0) if none of the files could be parsed (and add a test 
for it)
- Propagate any error code (like 2 in case of some missing files) from 
`Tool.buildASTs` instead of returning 0.
- Change test to accomodate the change in behavior/output due to 
https://reviews.llvm.org/D51729 (Thanks to Sam McCall for feedback.)


https://reviews.llvm.org/D54109

Files:
  clang-query/tool/ClangQuery.cpp
  test/clang-query/Inputs/database_template.json
  test/clang-query/database-missing-entry.c


Index: test/clang-query/database-missing-entry.c
===
--- /dev/null
+++ test/clang-query/database-missing-entry.c
@@ -0,0 +1,16 @@
+// RUN: rm -rf %t && mkdir -p %t/src %t/build
+// RUN: sed 's|test_dir|%t|g' %S/Inputs/database_template.json > 
%t/build/compile_commands.json
+// RUN: echo 'int A = AVAL;' > %t/src/a.c
+// RUN: echo 'deliberate parsing error' > %t/src/b.c
+// RUN: not clang-query -p=%t/build -c "m integerLiteral()" %t/src/a.c 
%t/src/b.c %t/src/missing.c 2>&1 | FileCheck %s
+
+// Test that neither parse errors nor missing database entries prevent further 
processing.
+// CHECK: deliberate parsing error
+// CHECK: Error while processing {{.*[/\\]}}missing.c.
+// CHECK-NOT-EXIST: Error while processing {{.*[/\\]}}missing.c.
+// CHECK-NOT-EXIST: unable to handle compilation
+// CHECK: a.c:1:9: note: "root" binds here
+
+// Test that an empty AST due to lack of any source files is bad.
+// RUN: not clang-query -p=%t/build -c "m integerLiteral()" %t/src/missing.c 
2>&1 | FileCheck --check-prefix=CHECK-NONE %s
+// CHECK-NONE: Error while processing {{.*[/\\]}}missing.c.
Index: test/clang-query/Inputs/database_template.json
===
--- /dev/null
+++ test/clang-query/Inputs/database_template.json
@@ -0,0 +1,12 @@
+[
+{
+  "directory": "test_dir/build",
+  "command": "clang -DAVAL=8 -o a.o test_dir/src/a.c",
+  "file": "test_dir/src/a.c"
+},
+{
+  "directory": "test_dir/build",
+  "command": "clang -o b.o test_dir/src/b.c",
+  "file": "test_dir/src/b.c"
+}
+]
Index: clang-query/tool/ClangQuery.cpp
===
--- clang-query/tool/ClangQuery.cpp
+++ clang-query/tool/ClangQuery.cpp
@@ -100,8 +100,9 @@
   ClangTool Tool(OptionsParser.getCompilations(),
  OptionsParser.getSourcePathList());
   std::vector> ASTs;
-  if (Tool.buildASTs(ASTs) != 0)
-return 1;
+  int RunResult = Tool.buildASTs(ASTs);
+  if (ASTs.empty())
+  return 1;
 
   QuerySession QS(ASTs);
 
@@ -134,5 +135,5 @@
 }
   }
 
-  return 0;
+  return RunResult;
 }


Index: test/clang-query/database-missing-entry.c
===
--- /dev/null
+++ test/clang-query/database-missing-entry.c
@@ -0,0 +1,16 @@
+// RUN: rm -rf %t && mkdir -p %t/src %t/build
+// RUN: sed 's|test_dir|%t|g' %S/Inputs/database_template.json > %t/build/compile_commands.json
+// RUN: echo 'int A = AVAL;' > %t/src/a.c
+// RUN: echo 'deliberate parsing error' > %t/src/b.c
+// RUN: not clang-query -p=%t/build -c "m integerLiteral()" %t/src/a.c %t/src/b.c %t/src/missing.c 2>&1 | FileCheck %s
+
+// Test that neither parse errors nor missing database entries prevent further processing.
+// CHECK: deliberate parsing error
+// CHECK: Error while processing {{.*[/\\]}}missing.c.
+// CHECK-NOT-EXIST: Error while processing {{.*[/\\]}}missing.c.
+// CHECK-NOT-EXIST: unable to handle compilation
+// CHECK: a.c:1:9: note: "root" binds here
+
+// Test that an empty AST due to lack of any source files is bad.
+// RUN: not clang-query -p=%t/build -c "m integerLiteral()" %t/src/missing.c 2>&1 | FileCheck --check-prefix=CHECK-NONE %s
+// CHECK-NONE: Error while processing {{.*[/\\]}}missing.c.
Index: test/clang-query/Inputs/database_template.json
===
--- /dev/null
+++ test/clang-query/Inputs/database_template.json
@@ -0,0 +1,12 @@
+[
+{
+  "directory": "test_dir/build",
+  "command": "clang -DAVAL=8 -o a.o test_dir/src/a.c",
+  "file": "test_dir/src/a.c"
+},
+{
+  "directory": "test_dir/build",
+  "command": "clang -o b.o test_dir/src/b.c",
+  "file": "test_dir/src/b.c"
+}
+]
Index: clang-query/tool/ClangQuery.cpp
===
--- clang-query/tool/ClangQuery.cpp
+++ clang-query/tool/ClangQuery.cpp
@@ -100,8 +100,9 @@
   ClangTool Tool(OptionsParser.getCompilations(),
  OptionsParser.getSourcePathList());
   std::vector> ASTs;
-  if (Tool.buildASTs(ASTs) != 0)
-return 1;
+  int RunResult = Tool.buildASTs(ASTs);
+  if (ASTs.empty())
+  return 1;
 
   QuerySessio

[PATCH] D54149: [Analyzer] [WIP] Standard C++ library functions checker for the std::find() family

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

baloghadamsoftware created this revision.
baloghadamsoftware added a reviewer: NoQ.
baloghadamsoftware added a project: clang.
Herald added subscribers: donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, 
rnkovacs, szepet, whisperity.
Herald added a reviewer: george.karpenkov.

Repository:
  rC Clang

https://reviews.llvm.org/D54149

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  test/Analysis/Inputs/system-header-simulator-cxx.h
  test/Analysis/diagnostics/explicit-suppression.cpp
  test/Analysis/iterator-range.cpp
  test/Analysis/std-c-library-functions.cpp
  test/Analysis/std-cxx-library-functions.cpp

Index: test/Analysis/std-cxx-library-functions.cpp
===
--- /dev/null
+++ test/Analysis/std-cxx-library-functions.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=apiModeling.StdCXXLibraryFunctions,debug.ExprInspection -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void clang_analyzer_eval(bool);
+
+void test_find(std::vector V, int n) {
+  const std::vector::iterator b = V.begin(), e = V.end();
+  clang_analyzer_eval(std::find(b, e, n) == e); // expected-warning{{TRUE}}
+  // expected-warning@-1{{FALSE}}
+}
Index: test/Analysis/std-c-library-functions.cpp
===
--- test/Analysis/std-c-library-functions.cpp
+++ test/Analysis/std-c-library-functions.cpp
@@ -12,3 +12,20 @@
 void test() {
   clang_analyzer_eval(isalpha('A')); // no-crash // expected-warning{{UNKNOWN}}
 }
+
+namespace std {
+int isalnum(int);
+}
+
+namespace {
+// Non std!!!
+
+int isalnum(int) {
+  return 0;
+}
+}
+
+void test_non_std() {
+  clang_analyzer_eval(std::isalnum('A')); // expected-warning{{TRUE}}
+  clang_analyzer_eval(isalnum('A')); // expected-warning{{FALSE}}
+}
Index: test/Analysis/iterator-range.cpp
===
--- test/Analysis/iterator-range.cpp
+++ test/Analysis/iterator-range.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -verify
-// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,apiModeling.StdCXXLibraryFunctions,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,apiModeling.StdCXXLibraryFunctions,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
 
 #include "Inputs/system-header-simulator-cxx.h"
 
@@ -97,6 +97,108 @@
 *i2; // expected-warning{{Iterator accessed outside of its range}}
 }
 
+void good_find(std::vector &V, int e) {
+  auto first = std::find(V.begin(), V.end(), e);
+  if (V.end() != first)
+*first; // no-warning
+}
+
+void bad_find(std::vector &V, int e) {
+  auto first = std::find(V.begin(), V.end(), e);
+  *first; // expected-warning{{Iterator accessed outside of its range}}
+}
+
+void good_find_end(std::vector &V, std::vector &seq) {
+  auto last = std::find_end(V.begin(), V.end(), seq.begin(), seq.end());
+  if (V.end() != last)
+*last; // no-warning
+}
+
+void bad_find_end(std::vector &V, std::vector &seq) {
+  auto last = std::find_end(V.begin(), V.end(), seq.begin(), seq.end());
+  *last; // expected-warning{{Iterator accessed outside of its range}}
+}
+
+void good_find_first_of(std::vector &V, std::vector &seq) {
+  auto first =
+  std::find_first_of(V.begin(), V.end(), seq.begin(), seq.end());
+  if (V.end() != first)
+*first; // no-warning
+}
+
+void bad_find_first_of(std::vector &V, std::vector &seq) {
+  auto first = std::find_end(V.begin(), V.end(), seq.begin(), seq.end());
+  *first; // expected-warning{{Iterator accessed outside of its range}}
+}
+
+bool odd(int i) { return i % 2; }
+
+void good_find_if(std::vector &V) {
+  auto first = std::find_if(V.begin(), V.end(), odd);
+  if (V.end() != first)
+*first; // no-warning
+}
+
+void bad_find_if(std::vector &V, int e) {
+  auto first = std::find_if(V.begin(), V.end(), odd);
+  *first; // expected-warning{{Iterator accessed outside of its range}}
+}
+
+void good_find_if_not(std::vector &V) {
+  auto first = std::find_if_not(V.begin(), V.end(), odd);
+  if (V.end() != first)
+*first; // no-warning
+}
+
+void bad_find_if_not(std::vector &V, int e) {
+  auto first = std::find_if_not(V.begin(), V.end(), odd);
+  *first

[PATCH] D54148: [NFC][Clang][Aarch64] Add missing test file

[Diogo N. Sampaio via Phabricator via cfe-commits]

dnsampaio created this revision.
dnsampaio added a reviewer: olista01.
Herald added subscribers: cfe-commits, kristof.beyls, javed.absar.

The commit https://reviews.llvm.org/rL345273 by @LukeCheeseman has a missing 
test file, see https://reviews.llvm.org/D51429.
This patch adds the missing test file.


Repository:
  rC Clang

https://reviews.llvm.org/D54148

Files:
  test/Driver/aarch64-security-options.c


Index: test/Driver/aarch64-security-options.c
===
--- /dev/null
+++ test/Driver/aarch64-security-options.c
@@ -0,0 +1,54 @@
+// Check the -msign-return-address= option, which has a required argument to
+// select scope.
+// RUN: %clang -target aarch64--none-eabi -c %s -### 
-msign-return-address=none 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RA-OFF  --check-prefix=KEY-A 
--check-prefix=BTE-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### 
-msign-return-address=non-leaf 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RA-NON-LEAF --check-prefix=KEY-A 
--check-prefix=BTE-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -msign-return-address=all 
 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RA-ALL  --check-prefix=KEY-A 
--check-prefix=BTE-OFF
+
+// Check that the -msign-return-address= option can also accept the signing key
+// to use.
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### 
-msign-return-address=non-leaf   2>&1 | \
+// RUN: FileCheck %s --check-prefix=RA-NON-LEAF --check-prefix=KEY-B 
--check-prefix=BTE-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -msign-return-address=all 
   2>&1 | \
+// RUN: FileCheck %s --check-prefix=RA-ALL  --check-prefix=KEY-B 
--check-prefix=BTE-OFF
+
+// -mbranch-protection with standard
+// RUN: %clang -target aarch64--none-eabi -c %s -### 
-mbranch-protection=standard2>&1 | \
+// RUN: FileCheck %s --check-prefix=RA-NON-LEAF --check-prefix=KEY-A 
--check-prefix=BTE-ON
+
+// If the -msign-return-address and -mbranch-protection are both used, the
+// right-most one controls return address signing.
+// RUN: %clang -target aarch64--none-eabi -c %s -### 
-msign-return-address=non-leaf -mbranch-protection=none 2>&1 | \
+// RUN: FileCheck %s --check-prefix=CONFLICT
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### 
-mbranch-protection=pac-ret -msign-return-address=none 2>&1 | \
+// RUN: FileCheck %s --check-prefix=CONFLICT
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -msign-return-address=foo 
2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-RA-PROTECTION
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -mbranch-protection=bar   
  2>&1 | \
+// RUN: FileCheck %s --check-prefix=BAD-BP-PROTECTION
+
+// RA-OFF: "-msign-return-address=none"
+// RA-NON-LEAF: "-msign-return-address=non-leaf"
+// RA-ALL: "-msign-return-address=all"
+
+// KEY-A: "-msign-return-address-key=a_key"
+
+// BTE-OFF-NOT: "-mbranch-target-enforce"
+// BTE-ON: "-mbranch-target-enforce"
+
+// CONFLICT: "-msign-return-address=none"
+
+// BAD-RA-PROTECTION: invalid branch protection option 'foo' in 
'-msign-return-address={{.*}}'
+// BAD-BP-PROTECTION: invalid branch protection option 'bar' in 
'-mbranch-protection={{.*}}'
+
+// BAD-B-KEY-COMBINATION: invalid branch protection option 'b-key' in 
'-mbranch-protection={{.*}}'
+// BAD-LEAF-COMBINATION: invalid branch protection option 'leaf' in 
'-mbranch-protection={{.*}}'


Index: test/Driver/aarch64-security-options.c
===
--- /dev/null
+++ test/Driver/aarch64-security-options.c
@@ -0,0 +1,54 @@
+// Check the -msign-return-address= option, which has a required argument to
+// select scope.
+// RUN: %clang -target aarch64--none-eabi -c %s -### -msign-return-address=none 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RA-OFF  --check-prefix=KEY-A --check-prefix=BTE-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -msign-return-address=non-leaf 2>&1 | \
+// RUN: FileCheck %s --check-prefix=RA-NON-LEAF --check-prefix=KEY-A --check-prefix=BTE-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -msign-return-address=all  2>&1 | \
+// RUN: FileCheck %s --check-prefix=RA-ALL  --check-prefix=KEY-A --check-prefix=BTE-OFF
+
+// Check that the -msign-return-address= option can also accept the signing key
+// to use.
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -msign-return-address=non-leaf   2>&1 | \
+// RUN: FileCheck %s --check-prefix=RA-NON-LEAF --check-prefix=KEY-B --check-prefix=BTE-OFF
+
+// RUN: %clang -target aarch64--none-eabi -c %s -### -msign-return-address=all2>&1 | \
+// RUN: FileCheck %s --check-prefix=RA-ALL  --check-prefix=KEY-B --che

[PATCH] D52034: [Clang] Add options -Xclang -coverage-filter and -Xclang -coverage-exclude to filter the files to instrument with gcov

[calixte via Phabricator via cfe-commits]

calixte updated this revision to Diff 172752.
calixte added a comment.

Change options names to -fprofile-exclude-files & -fprofile-filter-files and 
add some doc in UserManual.


Repository:
  rC Clang

https://reviews.llvm.org/D52034

Files:
  docs/UsersManual.rst
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.h
  lib/CodeGen/BackendUtil.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/Inputs/code-coverage-filter1.h
  test/CodeGen/Inputs/code-coverage-filter2.h
  test/CodeGen/code-coverage-filter.c

Index: test/CodeGen/code-coverage-filter.c
===
--- /dev/null
+++ test/CodeGen/code-coverage-filter.c
@@ -0,0 +1,84 @@
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes %s -o - \
+// RUN:| FileCheck -check-prefix=ALL %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -fprofile-exclude-files=".*\.h$" %s -o - \
+// RUN:| FileCheck -check-prefix=NO-HEADER %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -fprofile-filter-files=".*\.c$" %s -o - \
+// RUN:| FileCheck -check-prefix=NO-HEADER %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -fprofile-filter-files=".*\.c$:.*1\.h$" %s -o - \
+// RUN:| FileCheck -check-prefix=NO-HEADER2 %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -fprofile-exclude-files=".*2\.h$:.*1\.h$" %s -o - \
+// RUN:| FileCheck -check-prefix=JUST-C %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -fprofile-exclude-files=".*code\-coverage\-filter\.c$" %s -o - \
+// RUN:| FileCheck -check-prefix=HEADER %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -fprofile-filter-files=".*\.c$" -fprofile-exclude-files=".*\.c$" %s -o - \
+// RUN:| FileCheck -check-prefix=NONE %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -fprofile-filter-files=".*\.c$" -fprofile-exclude-files=".*\.h$" %s -o - \
+// RUN:| FileCheck -check-prefix=JUST-C %s
+
+#include "Inputs/code-coverage-filter1.h"
+#include "Inputs/code-coverage-filter2.h"
+
+void test() {
+  test1();
+  test2();
+}
+
+// ALL: define void @test1() #0 {{.*}}
+// ALL: {{.*}}__llvm_gcov_ctr{{.*}}
+// ALL: ret void
+// ALL: define void @test2() #0 {{.*}}
+// ALL: {{.*}}__llvm_gcov_ctr{{.*}}
+// ALL: ret void
+// ALL: define void @test() #0 {{.*}}
+// ALL: {{.*}}__llvm_gcov_ctr{{.*}}
+// ALL: ret void
+
+// NO-HEADER: define void @test1() #0 {{.*}}
+// NO-HEADER-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// NO-HEADER: ret void
+// NO-HEADER: define void @test2() #0 {{.*}}
+// NO-HEADER-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// NO-HEADER: ret void
+// NO-HEADER: define void @test() #0 {{.*}}
+// NO-HEADER: {{.*}}__llvm_gcov_ctr{{.*}}
+// NO-HEADER: ret void
+
+// NO-HEADER2: define void @test1() #0 {{.*}}
+// NO-HEADER2: {{.*}}__llvm_gcov_ctr{{.*}}
+// NO-HEADER2: ret void
+// NO-HEADER2: define void @test2() #0 {{.*}}
+// NO-HEADER2-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// NO-HEADER2: ret void
+// NO-HEADER2: define void @test() #0 {{.*}}
+// NO-HEADER2: {{.*}}__llvm_gcov_ctr{{.*}}
+// NO-HEADER2: ret void
+
+// JUST-C: define void @test1() #0 {{.*}}
+// JUST-C-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// JUST-C: ret void
+// JUST-C: define void @test2() #0 {{.*}}
+// JUST-C-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// JUST-C: ret void
+// JUST-C: define void @test() #0 {{.*}}
+// JUST-C: {{.*}}__llvm_gcov_ctr{{.*}}
+// JUST-C: ret void
+
+// HEADER: define void @test1() #0 {{.*}}
+// HEADER: {{.*}}__llvm_gcov_ctr{{.*}}
+// HEADER: ret void
+// HEADER: define void @test2() #0 {{.*}}
+// HEADER: {{.*}}__llvm_gcov_ctr{{.*}}
+// HEADER: ret void
+// HEADER: define void @test() #0 {{.*}}
+// HEADER-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// HEADER: ret void
+
+// NONE: define void @test1() #0 {{.*}}
+// NONE-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// NONE: ret void
+// NONE: define void @test2() #0 {{.*}}
+// NONE-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// NONE: ret void
+// NONE: define void @test() #0 {{.*}}
+// NONE-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// NONE: ret void
Index: test/CodeGen/Inputs/code-coverage-filter2.h
===
--- /dev/null
+++ test/CodeGen/Inputs/code-coverage-filter2.h
@@ -0,0 +1 @@
+void test2() {}
Index: test/CodeGen/Inputs/code-coverage-filter1.h
===
--- /dev/null
+++ test/CodeGen/Inputs/code-coverage-filter1.h
@@ -0,0 +1 @@
+void test1() {}
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -811,6 +811,10 @@
 Opts.CoverageExtraChecksum = Args.hasArg(OPT_coverage_cfg_checksum);
 Opts.CoverageNoFunctionNamesInData =
 Args.hasArg(OPT_coverag

[PATCH] D53701: [Analyzer] Record and process comparison of symbols instead of iterator positions in interator checkers

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

baloghadamsoftware added a comment.

In https://reviews.llvm.org/D53701#1288258, @NoQ wrote:

> Mmm, is it possible to detect adapters and inline them as an exception from 
> the rule? You can foresee a pretty complicated system of rules and exceptions 
> if we go down this path, but i believe that it is still much easier and more 
> reliable than the system that tries to synchronize two different models of 
> iterators, so i really encourage you to at least give it a try somehow.


I am almost sure it is implossible. It is not only about iterator-adapters in 
the classical sense but also any iterator that internally deals with other 
iterators.

For example I created an iterator in the past that iterates over the elements 
of a list of lists. This iterator contained two iterators, one for the outer 
and one for the inner list. In this case determining whether two iterators are 
the same is non-trivial: for all in-range iterators both the outer and the 
inner iterators must be the same. However if the outer iterator is the past-end 
iterator of the outer list then the inner iterator is irrelevant. Thus the 
comparison operator of such iterator must check first whether any of the outer 
iterators is the past-end iterator and only compare the inner iterator none of 
them is. If both outer iterators are the past-end iterator then they are equal 
regardless of the inner iterators.

Another example could be an iterator that somehow merges two lists internally 
using two different iterators. In this case only one of the inner iterators is 
relevant when comparing two merging iterators.

So by dropping the inlining we lose some intrinsic knowledge of the analyzed 
code which leads the checker to wrong assumptions.

>> I  wonder whether the current "mixed" approach introduces additional paths 
>> because we do not do explicit state-splits and function 
>> `processComparison()` removes contradicting branches.
> 
> I believe that the effect of eager state splits is going to be roughly 
> similar to the actual -analyzer-eagerly-assume:
> 
> 1. Split paths earlier than it is absolutely necessary, which may slow down 
> the analysis and duplicate some work, but most of the time it'll be just a 
> few nodes before the actual check in the code would have caused a split 
> anyway.
> 2. Simplify constraint solving on each of the new paths, which will indeed 
> help with refuting some invalid paths, especially those in which new 
> constraints over the symbols are introduced after the check, but that's due 
> to pecularities of our constraint solver, i.e. rather accidentally than 
> intentionally.

When I first started working on Clang Static Analyzer Anna told be the 
`-analyzer-eagerly-assume` should be the default. In the iterator checkers the 
refutation happens intentionally in the functions `relateSymbols()` and 
`processComparison()`.


Repository:
  rC Clang

https://reviews.llvm.org/D53701



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

[PATCH] D54152: [OpenCL] Fix diagnostic message about overload candidates

[Alexey Sachkov via Phabricator via cfe-commits]

AlexeySachkov created this revision.
AlexeySachkov added reviewers: asavonic, Anastasia.
Herald added a subscriber: yaxunl.

I wonder if there are some extension which need to be disabled to get
overloadable candidate available.


Repository:
  rC Clang

https://reviews.llvm.org/D54152

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  test/SemaOpenCL/extension-begin.cl


Index: test/SemaOpenCL/extension-begin.cl
===
--- test/SemaOpenCL/extension-begin.cl
+++ test/SemaOpenCL/extension-begin.cl
@@ -40,7 +40,7 @@
   PointerOfA test_A_pointer; // expected-error {{use of type 'PointerOfA' (aka 
'const struct A *') requires my_ext extension to be enabled}}
   f(); // expected-error {{use of declaration 'f' requires my_ext extension to 
be enabled}}
   g(0); // expected-error {{no matching function for call to 'g'}}
-// expected-note@extension-begin.h:18 {{candidate unavailable as it 
requires OpenCL extension 'my_ext' to be disabled}}
+// expected-note@extension-begin.h:18 {{candidate unavailable as it 
requires OpenCL extension 'my_ext' to be enabled}}
 // expected-note@extension-begin.h:23 {{candidate function not viable: 
requires 0 arguments, but 1 was provided}}
 }
 
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -3681,7 +3681,7 @@
 def note_ovl_candidate_disabled_by_function_cond_attr : Note<
 "candidate disabled: %0">;
 def note_ovl_candidate_disabled_by_extension : Note<
-"candidate unavailable as it requires OpenCL extension '%0' to be 
disabled">;
+"candidate unavailable as it requires OpenCL extension '%0' to be 
enabled">;
 def err_addrof_function_disabled_by_enable_if_attr : Error<
 "cannot take address of function %0 because it has one or more "
 "non-tautological enable_if conditions">;


Index: test/SemaOpenCL/extension-begin.cl
===
--- test/SemaOpenCL/extension-begin.cl
+++ test/SemaOpenCL/extension-begin.cl
@@ -40,7 +40,7 @@
   PointerOfA test_A_pointer; // expected-error {{use of type 'PointerOfA' (aka 'const struct A *') requires my_ext extension to be enabled}}
   f(); // expected-error {{use of declaration 'f' requires my_ext extension to be enabled}}
   g(0); // expected-error {{no matching function for call to 'g'}}
-// expected-note@extension-begin.h:18 {{candidate unavailable as it requires OpenCL extension 'my_ext' to be disabled}}
+// expected-note@extension-begin.h:18 {{candidate unavailable as it requires OpenCL extension 'my_ext' to be enabled}}
 // expected-note@extension-begin.h:23 {{candidate function not viable: requires 0 arguments, but 1 was provided}}
 }
 
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -3681,7 +3681,7 @@
 def note_ovl_candidate_disabled_by_function_cond_attr : Note<
 "candidate disabled: %0">;
 def note_ovl_candidate_disabled_by_extension : Note<
-"candidate unavailable as it requires OpenCL extension '%0' to be disabled">;
+"candidate unavailable as it requires OpenCL extension '%0' to be enabled">;
 def err_addrof_function_disabled_by_enable_if_attr : Error<
 "cannot take address of function %0 because it has one or more "
 "non-tautological enable_if conditions">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52034: [Clang] Add options -fprofile-filter-files and -fprofile-exclude-files to filter the files to instrument with gcov

[calixte via Phabricator via cfe-commits]

calixte updated this revision to Diff 172761.
calixte added a comment.

Fix plural form of regex and just use ';' as separator.


Repository:
  rC Clang

https://reviews.llvm.org/D52034

Files:
  docs/UsersManual.rst
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.h
  lib/CodeGen/BackendUtil.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/Inputs/code-coverage-filter1.h
  test/CodeGen/Inputs/code-coverage-filter2.h
  test/CodeGen/code-coverage-filter.c

Index: test/CodeGen/code-coverage-filter.c
===
--- /dev/null
+++ test/CodeGen/code-coverage-filter.c
@@ -0,0 +1,84 @@
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes %s -o - \
+// RUN:| FileCheck -check-prefix=ALL %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -fprofile-exclude-files=".*\.h$" %s -o - \
+// RUN:| FileCheck -check-prefix=NO-HEADER %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -fprofile-filter-files=".*\.c$" %s -o - \
+// RUN:| FileCheck -check-prefix=NO-HEADER %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -fprofile-filter-files=".*\.c$;.*1\.h$" %s -o - \
+// RUN:| FileCheck -check-prefix=NO-HEADER2 %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -fprofile-exclude-files=".*2\.h$;.*1\.h$" %s -o - \
+// RUN:| FileCheck -check-prefix=JUST-C %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -fprofile-exclude-files=".*code\-coverage\-filter\.c$" %s -o - \
+// RUN:| FileCheck -check-prefix=HEADER %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -fprofile-filter-files=".*\.c$" -fprofile-exclude-files=".*\.c$" %s -o - \
+// RUN:| FileCheck -check-prefix=NONE %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -fprofile-filter-files=".*\.c$" -fprofile-exclude-files=".*\.h$" %s -o - \
+// RUN:| FileCheck -check-prefix=JUST-C %s
+
+#include "Inputs/code-coverage-filter1.h"
+#include "Inputs/code-coverage-filter2.h"
+
+void test() {
+  test1();
+  test2();
+}
+
+// ALL: define void @test1() #0 {{.*}}
+// ALL: {{.*}}__llvm_gcov_ctr{{.*}}
+// ALL: ret void
+// ALL: define void @test2() #0 {{.*}}
+// ALL: {{.*}}__llvm_gcov_ctr{{.*}}
+// ALL: ret void
+// ALL: define void @test() #0 {{.*}}
+// ALL: {{.*}}__llvm_gcov_ctr{{.*}}
+// ALL: ret void
+
+// NO-HEADER: define void @test1() #0 {{.*}}
+// NO-HEADER-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// NO-HEADER: ret void
+// NO-HEADER: define void @test2() #0 {{.*}}
+// NO-HEADER-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// NO-HEADER: ret void
+// NO-HEADER: define void @test() #0 {{.*}}
+// NO-HEADER: {{.*}}__llvm_gcov_ctr{{.*}}
+// NO-HEADER: ret void
+
+// NO-HEADER2: define void @test1() #0 {{.*}}
+// NO-HEADER2: {{.*}}__llvm_gcov_ctr{{.*}}
+// NO-HEADER2: ret void
+// NO-HEADER2: define void @test2() #0 {{.*}}
+// NO-HEADER2-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// NO-HEADER2: ret void
+// NO-HEADER2: define void @test() #0 {{.*}}
+// NO-HEADER2: {{.*}}__llvm_gcov_ctr{{.*}}
+// NO-HEADER2: ret void
+
+// JUST-C: define void @test1() #0 {{.*}}
+// JUST-C-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// JUST-C: ret void
+// JUST-C: define void @test2() #0 {{.*}}
+// JUST-C-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// JUST-C: ret void
+// JUST-C: define void @test() #0 {{.*}}
+// JUST-C: {{.*}}__llvm_gcov_ctr{{.*}}
+// JUST-C: ret void
+
+// HEADER: define void @test1() #0 {{.*}}
+// HEADER: {{.*}}__llvm_gcov_ctr{{.*}}
+// HEADER: ret void
+// HEADER: define void @test2() #0 {{.*}}
+// HEADER: {{.*}}__llvm_gcov_ctr{{.*}}
+// HEADER: ret void
+// HEADER: define void @test() #0 {{.*}}
+// HEADER-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// HEADER: ret void
+
+// NONE: define void @test1() #0 {{.*}}
+// NONE-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// NONE: ret void
+// NONE: define void @test2() #0 {{.*}}
+// NONE-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// NONE: ret void
+// NONE: define void @test() #0 {{.*}}
+// NONE-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// NONE: ret void
Index: test/CodeGen/Inputs/code-coverage-filter2.h
===
--- /dev/null
+++ test/CodeGen/Inputs/code-coverage-filter2.h
@@ -0,0 +1 @@
+void test2() {}
Index: test/CodeGen/Inputs/code-coverage-filter1.h
===
--- /dev/null
+++ test/CodeGen/Inputs/code-coverage-filter1.h
@@ -0,0 +1 @@
+void test1() {}
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -811,6 +811,10 @@
 Opts.CoverageExtraChecksum = Args.hasArg(OPT_coverage_cfg_checksum);
 Opts.CoverageNoFunctionNamesInData =
 Args.hasArg(OPT_coverage_no_function_names_in_data);
+Opts.ProfileFil

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

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

Coming back to this one, I see a failing test: 
PCHPreambleTest.ReparseWithOverriddenFileDoesNotInvalidatePreamble

Note that PCHPreambleTest.ReparseWithOverriddenFileDoesNotInvalidatePreamble 
references the header paths in different ways ("//./header1.h" vs 
"//./foo/../header1.h"). Before this change, this was not a problem because 
OverriddenFiles were keyed on Status.getUniqueID(). Starting with this change, 
the key is the file path.

Is there a nice way to map different file paths of the same file to the same id 
without touching the real file system? Would it be sufficient to normalize the 
file paths? If yes, is there a utility function for this (can't find it right 
now).




Comment at: include/clang/Frontend/ASTUnit.h:193
   /// some number of calls.
-  unsigned PreambleRebuildCounter;
+  unsigned PreambleRebuildCountdownCounter;
+

ilya-biryukov wrote:
> NIT: Maybe shorten to `PreambleRebuildCountdown`?
> It's not a great name, but a bit shorter than now and seems to convey the 
> same meaning.
Will do.



Comment at: unittests/Frontend/PCHPreambleTest.cpp:130
+  std::string MainName = "//./main.cpp";
+  AddFile(MainName, "#include \"//./header1.h\"\n"
+"int main() { return ZERO; }");

ilya-biryukov wrote:
> NIT: Maybe use raw string literals? Escpated string are hard to read.
> E.g., 
> 
> ```R"cpp(
> #include "//./header1.h"
> int main() { return ZERO; }
> )cpp"
> ```
Will do.



Comment at: unittests/Frontend/PCHPreambleTest.cpp:133
+
+  std::unique_ptr AST(ParseAST(MainName));
+  ASSERT_TRUE(AST.get());

ilya-biryukov wrote:
> Are we testing the right thing here?
> 
> We're testing that preamble does not get rebuild when some header that was 
> previously unresolved when seen inside `#include` directive is now added to 
> the filesystem. That is actually a bug, we should rebuild the preamble in 
> that case.
> 
> We should probably call `RemapFile` before calling `ParseAST` instead and 
> make sure it's properly resolved.
> ```
>   AddFile(MainName, ...);
>   // We don't call AddFile for the header at this point, so that it does not 
> exist on the filesystem.
>   RemapFile(Header1, "#define ZERO 0\n");
> 
>   std::unique_ptr AST(ParseAST(MainName));
>   // Also assert that there were no compiler errors? (I.e. file was resolved 
> properly)
>   //  
> 
>   // Now the file is on the filesystem, call reparse and check that we reused 
> the preamble.
>   AddFile(Header1, "#define ZERO 0\n");
>   ASSERT_TRUE(ReparseAST(AST));
>   ASSERT_EQ(AST->getPreambleCounter(), 1U);
> ```
> Are we testing the right thing here?
Huch, indeed, the test is wrong. 

I'll incorporate your suggested test (real file is added after providing 
unsaved file) and add also another one: real file is removed after providing an 
unsaved file.

> That is actually a bug, we should rebuild the preamble in that case.
Agree, the preamble should be rebuild in such a case, but that's something for 
a separate change.



Repository:
  rC Clang

https://reviews.llvm.org/D41005



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

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the patch and nice improvements.

Some initial thoughts:

- The output of clang-tidy diagnostic is YAML, and YAML is not an 
space-efficient format (just for human readability). If you want to save space 
further, we might consider using some compressed formats, e.g. llvm::bitcode. 
Given the reduced YAML result (5.4MB) is promising, this might not matter.
- clang-tidy itself doesn't do deduplication, and `run-clang-tidy.py` seems an 
old way of running clang-tidy in parallel. The python script seems become more 
complicated now.  We have `AllTUsToolExecutor` right now, which supports 
running clang tools on a compilation database in parallel, so another option 
would be to use `AllTUsToolExecutor` in clang-tidy, and we can do deduplication 
inside clang-tidy binary (in reduce phase), which should be faster than the 
python script (spawn new clang-tidy processes and do round-trip of all the data 
through YAML-on-disk).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54141



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

[PATCH] D52998: [benchmark] Disable exceptions in Microsoft STL

[Elizabeth Andrews via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346237: [benchmark] Disable exceptions in Microsoft STL 
(authored by eandrews, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D52998?vs=172171&id=172769#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D52998

Files:
  llvm/trunk/utils/benchmark/CMakeLists.txt
  llvm/trunk/utils/benchmark/README.LLVM


Index: llvm/trunk/utils/benchmark/CMakeLists.txt
===
--- llvm/trunk/utils/benchmark/CMakeLists.txt
+++ llvm/trunk/utils/benchmark/CMakeLists.txt
@@ -99,6 +99,7 @@
   if (NOT BENCHMARK_ENABLE_EXCEPTIONS)
 add_cxx_compiler_flag(-EHs-)
 add_cxx_compiler_flag(-EHa-)
+add_definitions(-D_HAS_EXCEPTIONS=0)
   endif()
   # Link time optimisation
   if (BENCHMARK_ENABLE_LTO)
Index: llvm/trunk/utils/benchmark/README.LLVM
===
--- llvm/trunk/utils/benchmark/README.LLVM
+++ llvm/trunk/utils/benchmark/README.LLVM
@@ -19,3 +19,5 @@
   is applied to fix cross compilation with MinGW headers
 * 
https://github.com/google/benchmark/commit/439d6b1c2a6da5cb6adc4c4dfc555af235722396
   is applied to fix building with MinGW headers for ARM
+* 
https://github.com/google/benchmark/commit/a9b31c51b1ee7ec7b31438c647123c2cbac5d956
+  is applied to disable exceptions in Microsoft STL when exceptions are 
disabled


Index: llvm/trunk/utils/benchmark/CMakeLists.txt
===
--- llvm/trunk/utils/benchmark/CMakeLists.txt
+++ llvm/trunk/utils/benchmark/CMakeLists.txt
@@ -99,6 +99,7 @@
   if (NOT BENCHMARK_ENABLE_EXCEPTIONS)
 add_cxx_compiler_flag(-EHs-)
 add_cxx_compiler_flag(-EHa-)
+add_definitions(-D_HAS_EXCEPTIONS=0)
   endif()
   # Link time optimisation
   if (BENCHMARK_ENABLE_LTO)
Index: llvm/trunk/utils/benchmark/README.LLVM
===
--- llvm/trunk/utils/benchmark/README.LLVM
+++ llvm/trunk/utils/benchmark/README.LLVM
@@ -19,3 +19,5 @@
   is applied to fix cross compilation with MinGW headers
 * https://github.com/google/benchmark/commit/439d6b1c2a6da5cb6adc4c4dfc555af235722396
   is applied to fix building with MinGW headers for ARM
+* https://github.com/google/benchmark/commit/a9b31c51b1ee7ec7b31438c647123c2cbac5d956
+  is applied to disable exceptions in Microsoft STL when exceptions are disabled
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54156: [CodeComplete] Do not complete self-initializations

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.
ilya-biryukov added a reviewer: sammccall.
Herald added a subscriber: arphaman.

Removes references to initialized variable from the following completions:

  int x = ^;

Handles only the trivial cases where the variable name is completed
immediately at the start of initializer or assignment, more complicated
cases aren't covered, e.g. these completions still contain 'x':

  // More complicated expressions.
  int x = foo(^);
  int x = 10 + ^;
  // Other kinds of initialization.
  int x{^};
  int x(^);
  // Constructor initializers.
  struct Foo {
Foo() : x(^) {}
int x;
  };

We should address those in the future, but they are outside of the scope of
this initial change.


Repository:
  rC Clang

https://reviews.llvm.org/D54156

Files:
  lib/Sema/SemaCodeComplete.cpp
  test/CodeCompletion/ordinary-name-cxx11.cpp
  test/CodeCompletion/ordinary-name.cpp
  test/CodeCompletion/self-inits.cpp
  test/Index/complete-type-factors.m


Index: test/Index/complete-type-factors.m
===
--- test/Index/complete-type-factors.m
+++ test/Index/complete-type-factors.m
@@ -39,16 +39,14 @@
 // CHECK-CC1: FunctionDecl:{ResultType enum Priority}{TypedText 
func2}{LeftParen (}{Placeholder int}{RightParen )} (25)
 // CHECK-CC1: EnumConstantDecl:{ResultType enum Color}{TypedText Green} (32)
 // CHECK-CC1: EnumConstantDecl:{ResultType enum Priority}{TypedText High} (32)
-// CHECK-CC1: VarDecl:{ResultType int}{TypedText i} (8)
 // CHECK-CC1: ParmDecl:{ResultType int}{TypedText integer} (8)
 // CHECK-CC1: EnumConstantDecl:{ResultType enum Priority}{TypedText Low} (32)
 // CHECK-CC1: ParmDecl:{ResultType enum Priority}{TypedText priority} (17)
 // CHECK-CC1: EnumConstantDecl:{ResultType enum Color}{TypedText Red} (32)
 // CHECK-CC1: NotImplemented:{ResultType size_t}{TypedText sizeof}{LeftParen 
(}{Placeholder expression-or-type}{RightParen )} (40)
 // CHECK-CC1: FunctionDecl:{ResultType enum Priority}{TypedText 
test1}{LeftParen (}{Placeholder enum Priority priority}{Comma , }{Placeholder 
enum Color color}{Comma , }{Placeholder int integer}{RightParen )} (25)
 // RUN: c-index-test -code-completion-at=%s:17:18 -Xclang 
-code-completion-patterns %s | FileCheck -check-prefix=CHECK-CC2 %s
 // CHECK-CC2: EnumConstantDecl:{ResultType enum Color}{TypedText Blue} (16)
-// CHECK-CC2: VarDecl:{ResultType enum Color}{TypedText c} (8)
 // CHECK-CC2: ParmDecl:{ResultType enum Color}{TypedText color} (8)
 // CHECK-CC2: FunctionDecl:{ResultType int}{TypedText func1}{LeftParen 
(}{Placeholder enum Color}{RightParen )} (25)
 // CHECK-CC2: FunctionDecl:{ResultType enum Priority}{TypedText 
func2}{LeftParen (}{Placeholder int}{RightParen )} (50)
Index: test/CodeCompletion/self-inits.cpp
===
--- /dev/null
+++ test/CodeCompletion/self-inits.cpp
@@ -0,0 +1,3 @@
+int foo = 10;
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:1:11 %s -o - | 
FileCheck --check-prefix=CC1 %s
+// CC1-NOT: foo
Index: test/CodeCompletion/ordinary-name.cpp
===
--- test/CodeCompletion/ordinary-name.cpp
+++ test/CodeCompletion/ordinary-name.cpp
@@ -171,7 +171,6 @@
   // CHECK-CC4-NEXT: COMPLETION: volatile
   // CHECK-CC4-NEXT: COMPLETION: wchar_t
   // CHECK-CC4-NEXT: COMPLETION: X : X
-  // CHECK-CC4-NEXT: COMPLETION: y : [#int#]y
   // CHECK-CC4-NEXT: COMPLETION: z : [#void#]z(<#int#>)
 
   // RUN: %clang_cc1 -fsyntax-only -fno-rtti -code-completion-patterns 
-code-completion-at=%s:6:14 -std=gnu++98 %s -o - | FileCheck 
-check-prefix=CHECK-NO-RTTI %s
Index: test/CodeCompletion/ordinary-name-cxx11.cpp
===
--- test/CodeCompletion/ordinary-name-cxx11.cpp
+++ test/CodeCompletion/ordinary-name-cxx11.cpp
@@ -197,7 +197,6 @@
   // CHECK-CC4-NEXT: COMPLETION: volatile
   // CHECK-CC4-NEXT: COMPLETION: wchar_t
   // CHECK-CC4-NEXT: COMPLETION: X : X
-  // CHECK-CC4-NEXT: COMPLETION: y : [#int#]y
   // CHECK-CC4-NEXT: COMPLETION: z : [#void#]z(<#int#>)
 
   // RUN: %clang_cc1 -fsyntax-only -fno-rtti -code-completion-patterns 
-code-completion-at=%s:6:14 -std=gnu++11 %s -o - | FileCheck 
-check-prefix=CHECK-NO-RTTI %s
Index: lib/Sema/SemaCodeComplete.cpp
===
--- lib/Sema/SemaCodeComplete.cpp
+++ lib/Sema/SemaCodeComplete.cpp
@@ -4747,7 +4747,12 @@
 return;
   }
 
-  CodeCompleteExpression(S, VD->getType());
+  CodeCompleteExpressionData Data;
+  Data.PreferredType = VD->getType();
+  // Ignore VD to avoid completing the variable itself, e.g. in 'int foo = ^'.
+  Data.IgnoreDecls.push_back(VD);
+
+  CodeCompleteExpression(S, std::move(Data));
 }
 
 void Sema::CodeCompleteReturn(Scope *S) {


Index: test/Index/complete-type-factors.m
===
--- test/Index/complete-

[PATCH] D54156: [CodeComplete] Do not complete self-initializations

[Ilya Biryukov via Phabricator via cfe-commits]

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

- Remove std::move, the target is const ref, so it does nothing (thanks, 
clang-tidy!)


Repository:
  rC Clang

https://reviews.llvm.org/D54156

Files:
  lib/Sema/SemaCodeComplete.cpp
  test/CodeCompletion/ordinary-name-cxx11.cpp
  test/CodeCompletion/ordinary-name.cpp
  test/CodeCompletion/self-inits.cpp
  test/Index/complete-type-factors.m


Index: test/Index/complete-type-factors.m
===
--- test/Index/complete-type-factors.m
+++ test/Index/complete-type-factors.m
@@ -39,16 +39,14 @@
 // CHECK-CC1: FunctionDecl:{ResultType enum Priority}{TypedText 
func2}{LeftParen (}{Placeholder int}{RightParen )} (25)
 // CHECK-CC1: EnumConstantDecl:{ResultType enum Color}{TypedText Green} (32)
 // CHECK-CC1: EnumConstantDecl:{ResultType enum Priority}{TypedText High} (32)
-// CHECK-CC1: VarDecl:{ResultType int}{TypedText i} (8)
 // CHECK-CC1: ParmDecl:{ResultType int}{TypedText integer} (8)
 // CHECK-CC1: EnumConstantDecl:{ResultType enum Priority}{TypedText Low} (32)
 // CHECK-CC1: ParmDecl:{ResultType enum Priority}{TypedText priority} (17)
 // CHECK-CC1: EnumConstantDecl:{ResultType enum Color}{TypedText Red} (32)
 // CHECK-CC1: NotImplemented:{ResultType size_t}{TypedText sizeof}{LeftParen 
(}{Placeholder expression-or-type}{RightParen )} (40)
 // CHECK-CC1: FunctionDecl:{ResultType enum Priority}{TypedText 
test1}{LeftParen (}{Placeholder enum Priority priority}{Comma , }{Placeholder 
enum Color color}{Comma , }{Placeholder int integer}{RightParen )} (25)
 // RUN: c-index-test -code-completion-at=%s:17:18 -Xclang 
-code-completion-patterns %s | FileCheck -check-prefix=CHECK-CC2 %s
 // CHECK-CC2: EnumConstantDecl:{ResultType enum Color}{TypedText Blue} (16)
-// CHECK-CC2: VarDecl:{ResultType enum Color}{TypedText c} (8)
 // CHECK-CC2: ParmDecl:{ResultType enum Color}{TypedText color} (8)
 // CHECK-CC2: FunctionDecl:{ResultType int}{TypedText func1}{LeftParen 
(}{Placeholder enum Color}{RightParen )} (25)
 // CHECK-CC2: FunctionDecl:{ResultType enum Priority}{TypedText 
func2}{LeftParen (}{Placeholder int}{RightParen )} (50)
Index: test/CodeCompletion/self-inits.cpp
===
--- /dev/null
+++ test/CodeCompletion/self-inits.cpp
@@ -0,0 +1,3 @@
+int foo = 10;
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:1:11 %s -o - | 
FileCheck --check-prefix=CC1 %s
+// CC1-NOT: foo
Index: test/CodeCompletion/ordinary-name.cpp
===
--- test/CodeCompletion/ordinary-name.cpp
+++ test/CodeCompletion/ordinary-name.cpp
@@ -171,7 +171,6 @@
   // CHECK-CC4-NEXT: COMPLETION: volatile
   // CHECK-CC4-NEXT: COMPLETION: wchar_t
   // CHECK-CC4-NEXT: COMPLETION: X : X
-  // CHECK-CC4-NEXT: COMPLETION: y : [#int#]y
   // CHECK-CC4-NEXT: COMPLETION: z : [#void#]z(<#int#>)
 
   // RUN: %clang_cc1 -fsyntax-only -fno-rtti -code-completion-patterns 
-code-completion-at=%s:6:14 -std=gnu++98 %s -o - | FileCheck 
-check-prefix=CHECK-NO-RTTI %s
Index: test/CodeCompletion/ordinary-name-cxx11.cpp
===
--- test/CodeCompletion/ordinary-name-cxx11.cpp
+++ test/CodeCompletion/ordinary-name-cxx11.cpp
@@ -197,7 +197,6 @@
   // CHECK-CC4-NEXT: COMPLETION: volatile
   // CHECK-CC4-NEXT: COMPLETION: wchar_t
   // CHECK-CC4-NEXT: COMPLETION: X : X
-  // CHECK-CC4-NEXT: COMPLETION: y : [#int#]y
   // CHECK-CC4-NEXT: COMPLETION: z : [#void#]z(<#int#>)
 
   // RUN: %clang_cc1 -fsyntax-only -fno-rtti -code-completion-patterns 
-code-completion-at=%s:6:14 -std=gnu++11 %s -o - | FileCheck 
-check-prefix=CHECK-NO-RTTI %s
Index: lib/Sema/SemaCodeComplete.cpp
===
--- lib/Sema/SemaCodeComplete.cpp
+++ lib/Sema/SemaCodeComplete.cpp
@@ -4747,7 +4747,12 @@
 return;
   }
 
-  CodeCompleteExpression(S, VD->getType());
+  CodeCompleteExpressionData Data;
+  Data.PreferredType = VD->getType();
+  // Ignore VD to avoid completing the variable itself, e.g. in 'int foo = ^'.
+  Data.IgnoreDecls.push_back(VD);
+
+  CodeCompleteExpression(S, Data);
 }
 
 void Sema::CodeCompleteReturn(Scope *S) {


Index: test/Index/complete-type-factors.m
===
--- test/Index/complete-type-factors.m
+++ test/Index/complete-type-factors.m
@@ -39,16 +39,14 @@
 // CHECK-CC1: FunctionDecl:{ResultType enum Priority}{TypedText func2}{LeftParen (}{Placeholder int}{RightParen )} (25)
 // CHECK-CC1: EnumConstantDecl:{ResultType enum Color}{TypedText Green} (32)
 // CHECK-CC1: EnumConstantDecl:{ResultType enum Priority}{TypedText High} (32)
-// CHECK-CC1: VarDecl:{ResultType int}{TypedText i} (8)
 // CHECK-CC1: ParmDecl:{ResultType int}{TypedText integer} (8)
 // CHECK-CC1: EnumConstantDecl:{ResultType enum Priority}{TypedText Low} (32)
 // CHECK-C

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

This feature seems like a good idea. I started writing it too some months ago, 
but then I changed tactic and worked on distributing the refactor over the 
network instead. As far as I know, your deduplication would not work with a 
distributed environment.

However, it seems that both features can exist.

You use a regex to parse the clang output. Why not use the 
already-machine-readable yaml output and de-duplicate based on that? I think 
the design would be something like:

- Run clang-tidy in a quiet mode which only exports yaml and does not issue 
diagnostics
- Read the yaml in your python script
- Add the entries to your already-seen cache
- For any entry which was not already there
  - Write the entries to a new yaml file
  - Use clang-apply-replacements --issue-diags the_new_file.yaml to actually 
cause the new diagnostics to be issued (they were omitted from the clang-tidy 
run).

This avoids fragile parsing of the output from clang, instead relying on the 
machine-readable format.

I think clang-apply-replacements already does de-duplication, so it's possible 
that could take more responsibility.

Also, I think your test content is too big. I suggest trying to write more 
contained tests for this.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54141



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

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> - The output of clang-tidy diagnostic is YAML, and YAML is not an 
> space-efficient format (just for human readability). If you want to save 
> space further, we might consider using some compressed formats, e.g. 
> llvm::bitcode. Given the reduced YAML result (5.4MB) is promising, this might 
> not matter.

The output were normal diagnostics written to stdout, deduplication happens 
from there (see the test-cases). The files i created were just through piping 
to filter some of the noise.
Without de-duplication its very hard to get something useful out of a run with 
many checks activated for bigger projects (e.g. Blender and OpenCV are useless 
to try, because they have some commonly used macros with a check-violation. The 
buildbot filled 30GB of RAM before it crashed and couldn't even finish the 
analysis of the project. Similar for LLVM)

> - clang-tidy itself doesn't do deduplication, and `run-clang-tidy.py` seems 
> an old way of running clang-tidy in parallel. The python script seems become 
> more complicated now.  We have `AllTUsToolExecutor` right now, which supports 
> running clang tools on a compilation database in parallel, so another option 
> would be to use `AllTUsToolExecutor` in clang-tidy, and we can do 
> deduplication inside clang-tidy binary (in reduce phase), which should be 
> faster than the python script (spawn new clang-tidy processes and do 
> round-trip of all the data through YAML-on-disk).

Yes, this patch came out of necessity because testing through all available 
clang-tidy checks for big projects and see if their transformations are 
incorrect or not was/is just impossible right now with the tools we have 
upstream.
I agree that `AllTUsToolExecutor` would be better instead of the python script, 
but i think getting this done takes longer, then just patching the script now. 
From the patch here (it is an by-default off option as well) it is easier to 
test all pieces of clang-tidy. From there we can easily migrate to something 
better then `run-clang-tidy.py´.
The deduplication within clang-tidy would be the best option! But for full 
deduplication the parallelization must happen first.

> The python script seems become more complicated now.

A bit, yes. The actual calling of clang-tidy and other parts are not touched. 
Just the parser adds additional complexity, which is covered in the unit tests. 
I don't think this solution lives for ever, but its fast and effective, and 
again its optional and by default off.

For context: This is more a spinoff of my attempts of getting statistics of 
clang-tidy results for a wide range of projects. This parser is the minimal 
version that can do de-duplication.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54141



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

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Thank you for the comment!

In https://reviews.llvm.org/D54141#1288809, @steveire wrote:

> This feature seems like a good idea. I started writing it too some months 
> ago, but then I changed tactic and worked on distributing the refactor over 
> the network instead. As far as I know, your deduplication would not work with 
> a distributed environment.


I agree that it would probably not work. It might enable a two-stage 
deduplication, but I don't know if that would be viable.

> However, it seems that both features can exist.
> 
> You use a regex to parse the clang output. Why not use the 
> already-machine-readable yaml output and de-duplicate based on that? I think 
> the design would be something like:
> 
> - Run clang-tidy in a quiet mode which only exports yaml and does not issue 
> diagnostics
> - Read the yaml in your python script
> - Add the entries to your already-seen cache
> - For any entry which was not already there
>   - Write the entries to a new yaml file
>   - Use clang-apply-replacements --issue-diags the_new_file.yaml to actually 
> cause the new diagnostics to be issued (they were omitted from the clang-tidy 
> run).
> 
> This avoids fragile parsing of the output from clang, instead relying on 
> the machine-readable format.

In principle this approach seems more robust and I am not claiming my approach 
is robust at all :)
The point hokein raised should be considered first in my opinion. If clang-tidy 
itself is already parallel we should definitely deduplicate there. This is 
something I would put more
effort in. The proposed solution is more a hack to get my buildbot running and 
find transformation bugs and provide real-world data for checks we implement. :)

> I think clang-apply-replacements already does de-duplication, so it's 
> possible that could take more responsibility.

Yes, the emitted fixes are deduplicated but i think we need something even if 
no fixes are involved.

> Also, I think your test content is too big. I suggest trying to write more 
> contained tests for this.

I wanted to have a mix of both real snippets and some unit-tests on short 
examples. Do you think its enough if i shorten the list of fields that the CSA 
output contains for the padding checker?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54141



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

[PATCH] D54152: [OpenCL] Fix diagnostic message about overload candidates

[Anastasia Stulova via Phabricator via cfe-commits]

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

LGTM! Thanks!


Repository:
  rC Clang

https://reviews.llvm.org/D54152



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

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In https://reviews.llvm.org/D54141#1288818, @JonasToth wrote:

> Thank you for the comment!
>
> In https://reviews.llvm.org/D54141#1288809, @steveire wrote:
>
> > This feature seems like a good idea. I started writing it too some months 
> > ago, but then I changed tactic and worked on distributing the refactor over 
> > the network instead. As far as I know, your deduplication would not work 
> > with a distributed environment.
>
>
> I agree that it would probably not work. It might enable a two-stage 
> deduplication, but I don't know if that would be viable.


Yes, I think the distributed refactoring would benefit from the design I 
outlined - issuing diagnostics from the yaml files.

> In principle this approach seems more robust and I am not claiming my 
> approach is robust at all :)
>  The point hokein raised should be considered first in my opinion. If 
> clang-tidy itself is already parallel we should definitely deduplicate there. 
> This is something I would put more
>  effort in. The proposed solution is more a hack to get my buildbot running 
> and find transformation bugs and provide real-world data for checks we 
> implement. :)

Yes, I think it makes sense to do something more robust. I understand you're 
yak shaving here a bit while trying to reach a higher goal.

The AllTUsToolExecutor idea is worth exploring - it would mean we could remove 
the threading from run-clang-tidy.py.

I don't think we should get a self-confessed hack in just because it's already 
written.

However, AllTUsToolExecutor seems to not create output replacement files at 
all, which is not distributed-refactoring-friendly.

>> I think clang-apply-replacements already does de-duplication, so it's 
>> possible that could take more responsibility.
> 
> Yes, the emitted fixes are deduplicated but i think we need something even if 
> no fixes are involved.

Maybe my suggestion was not clear. The yaml file generated by clang-tidy 
contains not only replacements, but all diagnostics, even without a fixit.

So, running `clang-apply-replacements --issue-diags the_new_file.yaml` would 
issue the warnings/fixit hints by processing the yaml and issuing the 
diagnostics the way clang-tidy would have done (See in my proposed design that 
we silence clang-tidy).

Note also that the `--issue-diags` option does not yet exist. I'm proposing 
adding it.

> 
> 
>> Also, I think your test content is too big. I suggest trying to write more 
>> contained tests for this.
> 
> I wanted to have a mix of both real snippets and some unit-tests on short 
> examples. Do you think its enough if i shorten the list of fields that the 
> CSA output contains for the padding checker?

It seems that the bulk of the testing part of this commit is parsing a 
real-world log that you made. I guess if you remove the parsing (by taking a 
machine-readable approach) that bulk will disappear anyway.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54141



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

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> Maybe my suggestion was not clear. The yaml file generated by clang-tidy 
> contains not only replacements, but all diagnostics, even without a fixit.
> 
> So, running `clang-apply-replacements --issue-diags the_new_file.yaml` would 
> issue the warnings/fixit hints by processing the yaml and issuing the 
> diagnostics the way clang-tidy would have done (See in my proposed design 
> that we silence clang-tidy).
> 
> Note also that the `--issue-diags` option does not yet exist. I'm proposing 
> adding it.

At the moment `clang-apply-replacements` is called at the end of an clang-tidy 
run in `run-clang-tidy.py` That means we produce ~GBs of Yaml first, to then 
emit ~10MBs worth of it.
I think just relying on `clang-apply-replacements` is not ok.
If we do a hybrid: on-the-fly deduplication within 
`clang-tidy`/`run-clang-tidy.py` and a potential final deduplication with 
`clang-apply-replacments` we get the best of both worlds. 
That fits the distributed use-case as well(? I don't use a distributed system 
for these things as my projects are too small), because the first stage is 
local, the second stage central after all local workers are done.

> It seems that the bulk of the testing part of this commit is parsing a 
> real-world log that you made. I guess if you remove the parsing (by taking a 
> machine-readable approach) that bulk will disappear anyway.

That is true. The lat bit you have to convince me is on-the-flight output to 
see whats going on. I personally usually just take the raw textual 
representation and grep/scroll through it to see whats going on. It might be a 
bit of a tension between large-scale and small/medium scale applications.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54141



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

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

[Guillaume Chatelet via Phabricator via cfe-commits]

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

- Addressing comments and enhancing diagnostics


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488

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

Index: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
===
--- test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
+++ test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
@@ -1,4 +1,5 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t -- --
+// -target x86_64-unknown-linux
 
 float ceil(float);
 namespace std {
@@ -9,12 +10,12 @@
 namespace floats {
 
 struct ConvertsToFloat {
-  operator float() const { return 0.5; }
+  operator float() const { return 0.5f; }
 };
 
-float operator "" _Pa(unsigned long long);
+float operator"" _float(unsigned long long);
 
-void not_ok(double d) {
+void narrow_double_to_int_not_ok(double d) {
   int i = 0;
   i = d;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
@@ -24,11 +25,11 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
   i = ConvertsToFloat();
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
-  i = 15_Pa;
+  i = 15_float;
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
 }
 
-void not_ok_binary_ops(double d) {
+void narrow_double_to_int_not_ok_binary_ops(double d) {
   int i = 0;
   i += 0.5;
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
@@ -38,7 +39,7 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
   // We warn on the following even though it's not dangerous because there is no
   // reason to use a double literal here.
-  // TODO(courbet): Provide an automatic fix.
+  // TODO: Provide an automatic fix if the number is exactly representable in the destination type.
   i += 2.0;
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
   i += 2.0f;
@@ -52,6 +53,234 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
 }
 
+double operator"" _double(unsigned long long);
+
+float narrow_double_to_float_return() {
+  return 0.5;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+}
+
+void narrow_double_to_float_not_ok(double d) {
+  float f;
+  f = d;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+  f = 0.5;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+  f = 15_double;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+}
+
+void narrow_double_to_float_not_ok_binary_ops(double d) {
+  float f;
+  f += 0.5;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+  f += d;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+  // We warn on the following even though it's not dangerous because there is no reason to use a double literal here.
+  // TODO: Provide an automatic fix if the number is exactly representable in the destination type.
+  f += 2.0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+
+  f *= 0.5;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+  f /= 0.5;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+  f += (double)0.5f;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+}
+
+void narrow_integer_to_floating() {
+  {
+long long ll; // 64 bits

[PATCH] D54157: [clangd] [NFC] Fix clang-tidy warnings.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54157

Files:
  clangd/index/FileIndex.cpp
  unittests/clangd/BackgroundIndexTests.cpp


Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -43,7 +43,7 @@
   void f_b() {
 (void)common;
   })cpp";
-  BackgroundIndex Idx(Context::empty(), "", FS, /*URISchmes=*/{"unittest"});
+  BackgroundIndex Idx(Context::empty(), "", FS, /*URISchemes=*/{"unittest"});
 
   tooling::CompileCommand Cmd;
   Cmd.Filename = testPath("root/A.cc");
Index: clangd/index/FileIndex.cpp
===
--- clangd/index/FileIndex.cpp
+++ clangd/index/FileIndex.cpp
@@ -126,7 +126,7 @@
   for (const auto &Sym : *Slab) {
 auto I = Merged.try_emplace(Sym.ID, Sym);
 if (!I.second)
-  I.first->second = mergeSymbol(std::move(I.first->second), Sym);
+  I.first->second = mergeSymbol(I.first->second, Sym);
   }
 }
 SymsStorage.reserve(Merged.size());


Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -43,7 +43,7 @@
   void f_b() {
 (void)common;
   })cpp";
-  BackgroundIndex Idx(Context::empty(), "", FS, /*URISchmes=*/{"unittest"});
+  BackgroundIndex Idx(Context::empty(), "", FS, /*URISchemes=*/{"unittest"});
 
   tooling::CompileCommand Cmd;
   Cmd.Filename = testPath("root/A.cc");
Index: clangd/index/FileIndex.cpp
===
--- clangd/index/FileIndex.cpp
+++ clangd/index/FileIndex.cpp
@@ -126,7 +126,7 @@
   for (const auto &Sym : *Slab) {
 auto I = Merged.try_emplace(Sym.ID, Sym);
 if (!I.second)
-  I.first->second = mergeSymbol(std::move(I.first->second), Sym);
+  I.first->second = mergeSymbol(I.first->second, Sym);
   }
 }
 SymsStorage.reserve(Merged.size());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54077: [clangd] Implemented DraftFileSystem

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In https://reviews.llvm.org/D54077#1288455, @LutsenkoDanil wrote:

> I already made a patch which introduces such behavior (not uploaded here 
> yet), and looks like it works well with draft fs: diagnostics updates for 
> depended files in 'real-time' on typing for opened files and no seen 
> performance glitches, in multi-threaded mode at least.


This does indeed sound exciting! I'm a little skeptical that it "just works", 
there are some tricky cases:

- TUs where the preamble takes multiple seconds to rebuild
- "thundering herd" problems if several TUs are active that include the edited 
file
- redundant background work does have costs (e.g. battery)

> If it helps (and the LSP allows it): In case A.h is edited, we flag it dirty. 
> If the user makes some file depending on it visible (or the current file), we 
> trigger a reparse for that. Not sure whether the LSP has a notion of current 
> or visible files...

It doesn't. Maybe there's a way to make this idea work anyway (nothing comes to 
mind).

> I suggest continue discussion when/if dependencies tracking will be 
> implemented and real performance reduce introduced by this patch can be 
> checked with real code.

Sounds reasonable. I think maybe the shortest path there is to implement on top 
of the LSP file change notifications, and have it push a "maybe rebuild" item 
on every TUScheduler worker.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54077



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

[PATCH] D54156: [CodeComplete] Do not complete self-initializations

[Sam McCall via Phabricator via cfe-commits]

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

:-)


Repository:
  rC Clang

https://reviews.llvm.org/D54156



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

[PATCH] D54157: [clangd] [NFC] Fix clang-tidy warnings.

[Jonas Toth via Phabricator via cfe-commits]

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

Fixing such warnings is ok without prior review.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54157



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

[PATCH] D52421: [Sema] Diagnose parameter names that shadow inherited field names

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

And now i'm having concerns.

  struct Base {
  void* f;
  };
  
  struct Inherit : Base {
  static void func(void* f) { // <- does 'f' *actually* shadow the 'f' in 
the 'Base'? You can't access that non-static member variable from static 
function.
  }
  };


https://reviews.llvm.org/D52421



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

[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I think the check is really close.
If the other reviewers want to take a look at it again, now is a good moment :)




Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:178
+  return;
+// Conversions to unsigned integer are well defined and follow modulo 2
+// arithmetic.

I am surprised by `following modulo 2 arithmetic` and think it's a bit 
misleading. Writing just `module arithmetic` is probably better, as `module 2` 
somewhat implies there a only 2 valid values (0, 1).

Is this the `int` -> `unsigned int` case path? That seems worth diagnosing too.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:224
+  if (ToType->isBooleanType())
+return; // conversion to bool value is well defined.
+

Nit: Please move these two comment above the `if` and make 
`s/conversion/Conversion/`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53488



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

[PATCH] D52421: [Sema] Diagnose parameter names that shadow inherited field names

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D52421#1288910, @lebedev.ri wrote:

> And now i'm having concerns.
>
>   struct Base {
>   void* f;
>   };
>  
>   struct Inherit : Base {
>   static void func(void* f) { // <- does 'f' *actually* shadow the 'f' in 
> the 'Base'? You can't access that non-static member variable from static 
> function.
>   }
>   };
>  
>


The diagnostic is correct here, IMO. Consider:

  struct Base {
  int* f;
  };
  
  struct Inherit : Base {
  static void func(void* f) { 
  decltype(f) ptr = 0;
  }
  };

If the parameter were originally named `frobble` (so there was no hiding), then 
the `decltype(f)` would have resulted in `int *`. When `frobble` gets 
refactored into `f`, the type of `ptr` changes to `void *`.


https://reviews.llvm.org/D52421



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

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In https://reviews.llvm.org/D54141#1288851, @JonasToth wrote:

> > So, running `clang-apply-replacements --issue-diags the_new_file.yaml` 
> > would issue the warnings/fixit hints by processing the yaml and issuing the 
> > diagnostics the way clang-tidy would have done (See in my proposed design 
> > that we silence clang-tidy).
> > 
> > Note also that the `--issue-diags` option does not yet exist. I'm proposing 
> > adding it.
>
> At the moment `clang-apply-replacements` is called at the end of an 
> clang-tidy run in `run-clang-tidy.py` That means we produce ~GBs of Yaml 
> first, to then emit ~10MBs worth of it.
>  I think just relying on `clang-apply-replacements` is not ok.


I think my proposal is still unclear to you. Sorry about that.

I am proposing on-the-fly de-duplication, but without regex parsing of the 
diagnostic output of clang-tidy.

My proposal is still the same as I wrote before, but maybe what I write below 
will be more clear. Sorry if this seems condescendingly detailed. I don't know 
where the misunderstanding is coming from, so I err on the side of detail:

Imagine you have two cpp files which both include the same header. Imagine also 
that all 3 files are missing 1 override keyword and you run the 
modernize-use-override check on the two translation units using 
run-clang-tidy.py.

Here is what I propose happens:

- First, assume that the two translation units are processed serially, just to 
simplify the process as described. You will see that parallelizing does not 
change anything.
- clang-tidy gets run on file1.cpp in a way that it does not write diagnostics 
(and fixes) to stdout/stderr, but only generates a yaml file representing the 
diagnostics (warnings and fixes).
- Two diagnostics are created - one for the missing override in file1.cpp and 
one for the missing override in shared_header.h
- the on-the-fly deduplication cache is empty, so both diagnostics get added to 
the on-the-fly deduplication cache
- Because both were added to the cache, both diagnostics get written to a 
temporary file foo.yaml
- `clang-appy-replacements --issue-diags foo.yaml` is run (some other tool 
could be used for this step, but CAR seems right to me)
- `clang-appy-replacements --issue-diags foo.yaml` causes the two diagnostics 
to be issued, exactly as they would have been issued by clang-tidy.
- `clang-appy-replacements --issue-diags foo.yaml` DOES NOT actually change the 
source code. It only emits diagnostics
- Next process file2.cpp
- Processing file2.cpp results in two diagnostics - one for the missing 
override in file2.cpp and one for the missing override in shared_header..h
- NOTE: The diagnostic for the missing override in shared_header.h is a 
duplicate
- NOTE: The diagnostic for the missing override in file2.cpp is NOT a duplicate
- Try to add both to the on-the-fly deduplication cache
- Discover that the diagnostic for file2.cpp IS NOT a duplicate. Add it to a 
tempoary bar.yaml (named not to conflict with any other temporary file! This is 
built into temporary file APIs).
- Discover that the diagnostic for shared_header.h IS a duplicate. DO NOT write 
it to the temporary file
- Run `clang-appy-replacements --issue-diags bar.yaml`
- Run `clang-appy-replacements --issue-diags bar.yaml` causes ONLY the 
diagnostic for file2.cpp to be issued because that is all that is in the file.
- At the end, you have a de-duplicated yaml structure in memory. Write it to 
the file provided to the --export-fixes parameter of `run-clang-tidy.py`.

Do you understand the proposal now?

This means that you get

- A deduplicated fixes file
- De-duplicated diagnostics issued - which means you can process them in your 
CI system etc.

> That fits the distributed use-case as well?

If the distributed system processes more than one file at a time on the remote 
computer.

I'm not aware of any distributed systems that work that way. I think they all 
process single files at a time.

However, when the resulting yaml files are sent back to your computer, your 
computer can deduplicate the diagnostics issued (in my design).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54141



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

Re: r342827 - Fix modules build with shared library.

[David Blaikie via cfe-commits]

Shuai - have you had a chance to look at this?

On Mon, Oct 22, 2018 at 4:43 PM Richard Smith  wrote:

> On Mon, 22 Oct 2018 at 14:57, David Blaikie via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Richard - any further thoughts here (re: layering/dependencies, etc)?
>> Would love to get the layering oddity fixed rather than having it get more
>> embedded over time.
>>
>
> Here's the intended current directory layout and layering as I understand
> it:
>
>  * include/clang/Analysis and lib/Analysis are the parts of the static
> analysis engine that are depended on by both Sema and StaticAnalyzer, and
> include common functionality / infrastructure
>  * include/clang/Analysis/Analyses and lib/Analysis/Analyses are the
> specific Sema analyses (that don't depend on the static analyzer);
> StaticAnalyzer should not need to depend on this
>  * the StaticAnalyzer library should contain all the pieces that are
> specific to the static analyzer
>  * Sema should not depend on Tooling, but it's fine for StaticAnalyzer to
> depend on Tooling
>
> Compared to where we are today, there are two differences:
>
> 1) The implementations of the headers in include/clang/Analysis are in
> lib/Analysis, not in lib/Analysis/Analyses
> 2) ExprMutationAnalyzer is in lib/Analysis despite not being part of the
> common infrastructure depended on by Sema and StaticAnalyzer
>
> For point 1, we should change the lib/Analysis directory layout to match
> the headers.
> For point 2, we should find a home for this ExprMutationAnalyzer code that
> makes sense. Given that the intent is to use it from the static analyzer,
> that seems like a reasonable home for it, but libTooling would also make
> some sense (perhaps a new subdirectory Tooling/Analysis), since it's only
> performing AST matching, not a flow-sensitive / path-sensitive static
> analysis.
>
> On Tue, Oct 2, 2018 at 2:44 PM Shuai Wang via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> On Mon, Oct 1, 2018 at 4:58 PM Richard Smith 
>>> wrote:
>>>
 On Mon, 1 Oct 2018 at 16:10, George Karpenkov via cfe-commits <
 cfe-commits@lists.llvm.org> wrote:

> Hi Richard,
>
> On Oct 1, 2018, at 2:50 PM, Richard Smith via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
> This looks like the wrong fix to me, but I don't really know enough
> about what's being done with ExprMutationAnalyzer to have an opinion on
> what the right fix is.
>
> Shuai, what is the goal here? Why is this code being moved to
> Analysis/?
>
>
> I’ve asked for this possibility, as I wanted to use it from the Clang
> static analyzer.
>
> Do you intend to call it from the compiler frontend at some point? I
> can see a code review for the move itself, but no discussion of a plan or
> overall direction being followed here. Did I miss it?
>
> We have historically decided to not use the tooling interfaces
> (ASTMatcher, ParentMap, etc) from the frontend,
>
>
> Clang analyzer uses ASTMatcher all over the place.
>
> because they're generally a poor fit for our goals (we aim for a
> largely-single-pass compilation with good locality, and the AST matchers
> make different design choices)
>
>
> That’s totally good for the analyzer though, right?
>

 Yes, that all seems fine for the static analyzer.


> In any case, in future it might make sense to move the analyzer out of
> Clang proper.
> But for know the only way to use clang-tidy utilities from the
> analyzer is to move them into Clang.
>

 Right. I think we should try to maintain the idea that all the Clang
 Static Analyzer-specific things are in lib/StaticAnalyzer, and lib/Analysis
 only contains things depended on by the frontend. (That is: the things a
 clang binary would use if we didn't link in the static analyzer)

 Given the above, I think the best approach would be to move this code
 out of lib/Analysis and into somewhere in lib/StaticAnalyzer. There
 shouldn't be any problem with clang-tidy using it from there, since it
 already depends on the static analyzer.

>>> I'm happy to do the move.
>>> Could you (or someone) help point out where exactly I should move it to
>>> though?
>>>
 . If you want to change that, we'll need to discuss that decision.
>
> If the idea is to move this code into clang proper so that it can be
> used by various different tooling clients, we'd need to discuss the right
> place for it. Perhaps lib/Tooling/Analysis would make sense?
>
>
> On Mon, 1 Oct 2018 at 13:13, David Blaikie via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> I can't really reproduce this - when I try to build clang/llvm with
>> LLVM_ENABLE_MODULES in CMake I'm still seeing an error I reported March 
>> on
>> a cfe-dev thread - something to do with unique_ptr insta

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> Do you understand the proposal now?

Yes better, I was under the impression that `clang-apply-replaments` is run on 
the end and the YAMLs are kept until then. Now its clear.
I assume `--issue-diags` produce the same result as the normal diagnostic 
engine. That could work, yes.

clang-tidy does not have a `quiet` mode though. It has the `-quiet` option 
which just does not emit how many warnings were created and suppressed.
Do you have these things already in the pipeline?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54141



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

[PATCH] D52421: [Sema] Diagnose parameter names that shadow inherited field names

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D52421#1288917, @aaron.ballman wrote:

> In https://reviews.llvm.org/D52421#1288910, @lebedev.ri wrote:
>
> > ...
>
>
> ...


Ok, thank you for reassuring me that it is working as intended.


https://reviews.llvm.org/D52421



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

Re: r342827 - Fix modules build with shared library.

[Shuai Wang via cfe-commits]

Sorry for the unresponsiveness, I've been fire fighting for other stuffs.
I'll look into this and try to get it fixed this week.

On Tue, Nov 6, 2018 at 9:52 AM David Blaikie  wrote:

> Shuai - have you had a chance to look at this?
>
> On Mon, Oct 22, 2018 at 4:43 PM Richard Smith 
> wrote:
>
>> On Mon, 22 Oct 2018 at 14:57, David Blaikie via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Richard - any further thoughts here (re: layering/dependencies, etc)?
>>> Would love to get the layering oddity fixed rather than having it get more
>>> embedded over time.
>>>
>>
>> Here's the intended current directory layout and layering as I understand
>> it:
>>
>>  * include/clang/Analysis and lib/Analysis are the parts of the static
>> analysis engine that are depended on by both Sema and StaticAnalyzer, and
>> include common functionality / infrastructure
>>  * include/clang/Analysis/Analyses and lib/Analysis/Analyses are the
>> specific Sema analyses (that don't depend on the static analyzer);
>> StaticAnalyzer should not need to depend on this
>>  * the StaticAnalyzer library should contain all the pieces that are
>> specific to the static analyzer
>>  * Sema should not depend on Tooling, but it's fine for StaticAnalyzer to
>> depend on Tooling
>>
>> Compared to where we are today, there are two differences:
>>
>> 1) The implementations of the headers in include/clang/Analysis are in
>> lib/Analysis, not in lib/Analysis/Analyses
>> 2) ExprMutationAnalyzer is in lib/Analysis despite not being part of the
>> common infrastructure depended on by Sema and StaticAnalyzer
>>
>> For point 1, we should change the lib/Analysis directory layout to match
>> the headers.
>> For point 2, we should find a home for this ExprMutationAnalyzer code
>> that makes sense. Given that the intent is to use it from the static
>> analyzer, that seems like a reasonable home for it, but libTooling would
>> also make some sense (perhaps a new subdirectory Tooling/Analysis), since
>> it's only performing AST matching, not a flow-sensitive / path-sensitive
>> static analysis.
>>
>> On Tue, Oct 2, 2018 at 2:44 PM Shuai Wang via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
 On Mon, Oct 1, 2018 at 4:58 PM Richard Smith 
 wrote:

> On Mon, 1 Oct 2018 at 16:10, George Karpenkov via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Hi Richard,
>>
>> On Oct 1, 2018, at 2:50 PM, Richard Smith via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>> This looks like the wrong fix to me, but I don't really know enough
>> about what's being done with ExprMutationAnalyzer to have an opinion on
>> what the right fix is.
>>
>> Shuai, what is the goal here? Why is this code being moved to
>> Analysis/?
>>
>>
>> I’ve asked for this possibility, as I wanted to use it from the Clang
>> static analyzer.
>>
>> Do you intend to call it from the compiler frontend at some point? I
>> can see a code review for the move itself, but no discussion of a plan or
>> overall direction being followed here. Did I miss it?
>>
>> We have historically decided to not use the tooling interfaces
>> (ASTMatcher, ParentMap, etc) from the frontend,
>>
>>
>> Clang analyzer uses ASTMatcher all over the place.
>>
>> because they're generally a poor fit for our goals (we aim for a
>> largely-single-pass compilation with good locality, and the AST matchers
>> make different design choices)
>>
>>
>> That’s totally good for the analyzer though, right?
>>
>
> Yes, that all seems fine for the static analyzer.
>
>
>> In any case, in future it might make sense to move the analyzer out
>> of Clang proper.
>> But for know the only way to use clang-tidy utilities from the
>> analyzer is to move them into Clang.
>>
>
> Right. I think we should try to maintain the idea that all the Clang
> Static Analyzer-specific things are in lib/StaticAnalyzer, and 
> lib/Analysis
> only contains things depended on by the frontend. (That is: the things a
> clang binary would use if we didn't link in the static analyzer)
>
> Given the above, I think the best approach would be to move this code
> out of lib/Analysis and into somewhere in lib/StaticAnalyzer. There
> shouldn't be any problem with clang-tidy using it from there, since it
> already depends on the static analyzer.
>
 I'm happy to do the move.
 Could you (or someone) help point out where exactly I should move it to
 though?

> . If you want to change that, we'll need to discuss that decision.
>>
>> If the idea is to move this code into clang proper so that it can be
>> used by various different tooling clients, we'd need to discuss the right
>> place for it. Perhaps lib/Tooling/Analysis would make sense?
>>
>>
>> On Mon, 1 Oct 2018 at 13:

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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D53738#1288372, @ebevhan wrote:

> In https://reviews.llvm.org/D53738#1288333, @rjmccall wrote:
>
> > In https://reviews.llvm.org/D53738#1287123, @ebevhan wrote:
> >
> > > In https://reviews.llvm.org/D53738#1284213, @rjmccall wrote:
> > >
> > > > Not out of line with other features that significantly break with 
> > > > what's expressible.  But the easy alternative to storing the 
> > > > intermediate "type" in the AST is to just provide a global function 
> > > > that can compute it on demand.
> > >
> > >
> > > Yeah, it might just be simpler to go the route of not storing the 
> > > computation semantic in the AST, at least for now. That's fairly similar 
> > > to the solution in the patch, so I feel a bit silly for just going around 
> > > in a circle.
> > >
> > > To make that work I think the patch needs some changes, though. There 
> > > should be a function in FixedPointSemantics to find the common 
> > > full-precision semantic between two semantics, and the 
> > > `getFixedPointSemantics` in ASTContext should be amended to take integer 
> > > types (or another method should be provided for this, but that one 
> > > already exists). I think that the `FixedPointConversions` method should 
> > > also be embedded into the rest of `UsualArithmeticConversions` as there 
> > > shouldn't be any need to have it separate. You still want to do the 
> > > rvalue conversion and other promotions, and the rules for fixed-point 
> > > still fit into the arithmetic conversions, even in the spec.
> > >
> > > `EmitFixedPointConversion` should be changed to take FixedPointSemantics 
> > > rather than QualType. This has a couple of downsides:
> > >
> > > - It can no longer handle floating point conversions. They would have to 
> > > be handled in EmitScalarConversion.
> > > - Conversion from integer is fine, but conversion to integer cannot be 
> > > generalized away with the fixed-point semantics as they are currently, as 
> > > that kind of conversion must round towards zero. This requires a rounding 
> > > step for negative numbers before downscaling, which the current code does 
> > > not do. Is there a better way of generalizing this?
> >
> >
> > `FixedPointSemantics` is free to do whatever is convenient for the 
> > representation; if it helps to make it able to explicitly represent an 
> > integral type — and then just assert that it's never in that form when it's 
> > used in certain places, I think that works.  Although you might consider 
> > making a `FixedPointOrIntegralSemantics` class and then making 
> > `FixedPointSemantics` a subclass which adds nothing to the representation 
> > but semantically asserts that it's representing a fixed-point type.
>
>
> It might just be simpler and a bit more general to add a 
> `DownscalingRoundsTowardZero` field to `FixedPointSemantics`, which specifies 
> that the source value should be rounded toward zero before downscaling. Then 
> conversion routines could handle the integer case explicitly. I'm not sure if 
> the field would be useful for anything else, though; it has a pretty specific 
> meaning.
>
> I think it's a bit odd to have `FixedPointSemantics` as a specialization of 
> something else, since technically integers are a specialization of 
> fixed-point values and not the other way around.


Well, it's not a specialization of "integer", it's a specialization of 
"fixed-point or integer".  It depends on how useful it ends up being to know 
that something statically isn't integral.


Repository:
  rC Clang

https://reviews.llvm.org/D53738



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

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Reducing log file size is good idea, but I think will be also good idea to 
count duplicates. This will allow to concentrate clean-up efforts on place 
where most of warnings originate.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54141



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

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

[Bruno Ricci via Phabricator via cfe-commits]

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

Move some data to the newly available space in the bit-fields
of `Stmt`. This cuts the size of `DeclRefExpr`, `UnaryOperator`,
`MemberExpr` and `BinaryOperator` by 1 pointer each.

Also run clang-format on each of these classes.


Repository:
  rC Clang

https://reviews.llvm.org/D54161

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

Index: lib/Serialization/ASTWriterStmt.cpp
===
--- lib/Serialization/ASTWriterStmt.cpp
+++ lib/Serialization/ASTWriterStmt.cpp
@@ -655,8 +655,8 @@
   if (E->hasQualifier())
 Record.AddNestedNameSpecifierLoc(E->getQualifierLoc());
 
-  Record.push_back(E->HasTemplateKWAndArgsInfo);
-  if (E->HasTemplateKWAndArgsInfo) {
+  Record.push_back(E->MemberExprBits.HasTemplateKWAndArgsInfo);
+  if (E->MemberExprBits.HasTemplateKWAndArgsInfo) {
 Record.AddSourceLocation(E->getTemplateKeywordLoc());
 unsigned NumTemplateArgs = E->getNumTemplateArgs();
 Record.push_back(NumTemplateArgs);
Index: lib/AST/Expr.cpp
===
--- lib/AST/Expr.cpp
+++ lib/AST/Expr.cpp
@@ -351,7 +351,8 @@
  const TemplateArgumentListInfo *TemplateArgs,
  QualType T, ExprValueKind VK)
   : Expr(DeclRefExprClass, T, VK, OK_Ordinary, false, false, false, false),
-D(D), Loc(NameInfo.getLoc()), DNLoc(NameInfo.getInfo()) {
+D(D), DNLoc(NameInfo.getInfo()) {
+  DeclRefExprBits.Loc = NameInfo.getLoc();
   DeclRefExprBits.HasQualifier = QualifierLoc ? 1 : 0;
   if (QualifierLoc) {
 new (getTrailingObjects())
@@ -1527,15 +1528,15 @@
  QualifierLoc.getNestedNameSpecifier()->isInstantiationDependent())
   E->setInstantiationDependent(true);
 
-E->HasQualifierOrFoundDecl = true;
+E->MemberExprBits.HasQualifierOrFoundDecl = true;
 
 MemberExprNameQualifier *NQ =
 E->getTrailingObjects();
 NQ->QualifierLoc = QualifierLoc;
 NQ->FoundDecl = founddecl;
   }
 
-  E->HasTemplateKWAndArgsInfo = (targs || TemplateKWLoc.isValid());
+  E->MemberExprBits.HasTemplateKWAndArgsInfo = (targs || TemplateKWLoc.isValid());
 
   if (targs) {
 bool Dependent = false;
Index: include/clang/AST/Stmt.h
===
--- include/clang/AST/Stmt.h
+++ include/clang/AST/Stmt.h
@@ -378,6 +378,58 @@
 unsigned HasFoundDecl : 1;
 unsigned HadMultipleCandidates : 1;
 unsigned RefersToEnclosingVariableOrCapture : 1;
+
+/// The location of the declaration name itself.
+SourceLocation Loc;
+  };
+
+  class UnaryOperatorBitfields {
+friend class UnaryOperator;
+
+unsigned : NumExprBits;
+
+/// The operator kind.
+unsigned OperatorKind : 5;
+
+unsigned CanOverflow : 1;
+
+/// The location of the operator.
+SourceLocation Loc;
+  };
+
+  class CallExprBitfields {
+friend class CallExpr;
+
+unsigned : NumExprBits;
+
+unsigned NumPreArgs : 1;
+  };
+
+  class MemberExprBitfields {
+friend class ASTStmtWriter;
+friend class MemberExpr;
+
+unsigned : NumExprBits;
+
+/// IsArrow - True if this is "X->F", false if this is "X.F".
+unsigned IsArrow : 1;
+
+/// True if this member expression used a nested-name-specifier to
+/// refer to the member, e.g., "x->Base::f", or found its member via a using
+/// declaration.  When true, a MemberExprNameQualifier
+/// structure is allocated immediately after the MemberExpr.
+unsigned HasQualifierOrFoundDecl : 1;
+
+/// True if this member expression specified a template keyword
+/// and/or a template argument list explicitly, e.g., x->f,
+/// x->template f, x->template f.
+/// When true, an ASTTemplateKWAndArgsInfo structure and its
+/// TemplateArguments (if any) are present.
+unsigned HasTemplateKWAndArgsInfo : 1;
+
+/// True if this member expression refers to a method that
+/// was resolved from an overloaded set having size greater than 1.
+unsigned HadMultipleCandidates : 1;
   };
 
   class CastExprBitfields {
@@ -391,12 +443,20 @@
 unsigned BasePathIsEmpty : 1;
   };
 
-  class CallExprBitfields {
-friend class CallExpr;
+  class BinaryOperatorBitfields {
+friend class BinaryOperator;
 
 unsigned : NumExprBits;
 
-unsigned NumPreArgs : 1;
+/// The kind of this binary operator.
+unsigned OperatorKind : 6;
+
+/// This is only meaningful for operations on floating
+/// point types and 0 otherwise.
+unsigned FPFeatures : 3;
+
+/// The location of the operator.
+SourceLocation OpLoc;
   };
 
   class ExprWithCleanupsBitfields {
@@ -502,8 +562,11 @@
 FloatingLiteralBitfields FloatingLiteralBits;
 UnaryExprOrTypeTraitExprBitfields Unar

[PATCH] D54162: OpenCL: Don't warn on v printf modifier

[Matt Arsenault via Phabricator via cfe-commits]

arsenm created this revision.
arsenm added a reviewer: Anastasia.
Herald added subscribers: yaxunl, wdng.

This avoids spurious warnings, but could use
a lot of work. For example the number of vector
elements is not verified, and the passed
value type is not checked.


https://reviews.llvm.org/D54162

Files:
  include/clang/AST/FormatString.h
  lib/AST/FormatString.cpp
  lib/AST/PrintfFormatString.cpp
  test/Sema/format-strings.c
  test/SemaOpenCL/printf-format-strings.cl

Index: test/SemaOpenCL/printf-format-strings.cl
===
--- /dev/null
+++ test/SemaOpenCL/printf-format-strings.cl
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -cl-std=CL1.2 -fsyntax-only -verify %s
+
+typedef __attribute__((ext_vector_type(2))) float float2;
+typedef __attribute__((ext_vector_type(4))) float float4;
+typedef __attribute__((ext_vector_type(4))) int int4;
+
+int printf(__constant const char* st, ...) __attribute__((format(printf, 1, 2)));
+
+kernel void format_v4f32(float4 arg)
+{
+printf("%v4f\n", arg); // expected-no-diagnostics
+}
+
+kernel void format_v4f32_wrong_num_elts(float2 arg)
+{
+printf("%v4f\n", arg); // expected-no-diagnostics
+}
+
+kernel void vector_precision_modifier_v4f32(float4 arg)
+{
+printf("%.2v4f\n", arg); // expected-no-diagnostics
+}
+
+// FIXME: This should warn
+kernel void format_missing_num_elts(float4 arg)
+{
+printf("%vf\n", arg); // expected-no-diagnostics
+}
+
+// FIXME: This should warn
+kernel void vector_precision_modifier_v4i32(int4 arg)
+{
+printf("%.2v4f\n", arg); // expected-no-diagnostics
+}
Index: test/Sema/format-strings.c
===
--- test/Sema/format-strings.c
+++ test/Sema/format-strings.c
@@ -613,6 +613,11 @@
   printf("%hhx", c);
 }
 
+void test_opencl_vector_format(int x) {
+  printf("%v4d", x); // expected-warning{{invalid conversion specifier 'v'}}
+  printf("%vd", x); // expected-warning{{invalid conversion specifier 'v'}}
+  printf("%0vd", x); // expected-warning{{invalid conversion specifier 'v'}}
+}
 
 // Test that we correctly merge the format in both orders.
 extern void test14_foo(const char *, const char *, ...)
Index: lib/AST/PrintfFormatString.cpp
===
--- lib/AST/PrintfFormatString.cpp
+++ lib/AST/PrintfFormatString.cpp
@@ -347,6 +347,12 @@
 case 'Z':
   if (Target.getTriple().isOSMSVCRT())
 k = ConversionSpecifier::ZArg;
+  break;
+// OpenCL specific.
+case 'v':
+  if (LO.OpenCL)
+k = ConversionSpecifier::VArg;
+  break;
   }
 
   // Check to see if we used the Objective-C modifier flags with
@@ -1008,6 +1014,7 @@
   case ConversionSpecifier::FreeBSDrArg:
   case ConversionSpecifier::FreeBSDyArg:
   case ConversionSpecifier::PArg:
+  case ConversionSpecifier::VArg:
 return true;
 
   default:
Index: lib/AST/FormatString.cpp
===
--- lib/AST/FormatString.cpp
+++ lib/AST/FormatString.cpp
@@ -618,6 +618,9 @@
 
   // MS specific specifiers.
   case ZArg: return "Z";
+
+ // OpenCL specific specifiers.
+  case VArg: return "v";
   }
   return nullptr;
 }
@@ -875,6 +878,8 @@
 case ConversionSpecifier::CArg:
 case ConversionSpecifier::SArg:
   return LangOpt.ObjC;
+case ConversionSpecifier::VArg:
+  return LangOpt.OpenCL;
 case ConversionSpecifier::InvalidSpecifier:
 case ConversionSpecifier::FreeBSDbArg:
 case ConversionSpecifier::FreeBSDDArg:
Index: include/clang/AST/FormatString.h
===
--- include/clang/AST/FormatString.h
+++ include/clang/AST/FormatString.h
@@ -166,6 +166,8 @@
 
 ZArg, // MS extension
 
+VArg, // OpenCL vectors
+
 // Objective-C specific specifiers.
 ObjCObjArg, // '@'
 ObjCBeg = ObjCObjArg,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54162: OpenCL: Don't warn on v printf modifier

[Anastasia Stulova via Phabricator via cfe-commits]

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

LGTM! Thanks! Seems like a reasonable start.


https://reviews.llvm.org/D54162



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

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

[Bruno Ricci via Phabricator via cfe-commits]

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

Use the newly available space in the bit-fields of `Stmt` and store the
string data in a trailing array of `char`s after the trailing array
of `SourceLocation`. This cuts the size of `StringLiteral` by 2 pointers.

Also refactor slightly `StringLiteral::Create` and `StringLiteral::CreateEmpty`
so that `StringLiteral::Create` is just responsible for the allocation, and the
constructor is responsible for doing all the initialization. This match what
is done for the other classes in general.

Two points I am wondering about:

The original version used type punning through an union. Here I am just
`reinterpret_cast`ing back and forth between `char *` and `uint16_t 
*`/`uint32_t *`.

There is a FIXME in `ASTWriterStmt.cpp` (see the end of the diff) which says 
that
storing the string past the `StringLiteral` would cause problems with 
abbreviations.
The note dates from 2009 and everything *seems* to be working just fine. However
I am not familiar with this and it is possible I am missing something here.


Repository:
  rC Clang

https://reviews.llvm.org/D54166

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

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

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In https://reviews.llvm.org/D54141#1288993, @Eugene.Zelenko wrote:

> Reducing log file size is good idea, but I think will be also good idea to 
> count duplicates. This will allow to concentrate clean-up efforts on place 
> where most of warnings originate.


Places that emit a lot of diagnostics, still do. I think the amount of 
duplicated warnings does not show an urgency with the unique warning.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54141



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

[PATCH] D54168: [clang-tidy] Zircon fbl::move -> std::move

[Julie Hockett via Phabricator via cfe-commits]

juliehockett created this revision.
juliehockett added reviewers: aaron.ballman, alexfh, hokein.
juliehockett added a project: clang-tools-extra.
Herald added subscribers: xazax.hun, mgorny.

Adds a check to convert fbl::move to std::move.

This check is part of a set of migration checks as we prepare to move Zircon 
user code to use the C++ standard library, and should prevent regressions after 
the migration is complete.


https://reviews.llvm.org/D54168

Files:
  clang-tools-extra/clang-tidy/zircon/CMakeLists.txt
  clang-tools-extra/clang-tidy/zircon/FblMoveCheck.cpp
  clang-tools-extra/clang-tidy/zircon/FblMoveCheck.h
  clang-tools-extra/clang-tidy/zircon/ZirconTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/zircon-fbl-move.rst
  clang-tools-extra/test/clang-tidy/zircon-fbl-move.cpp

Index: clang-tools-extra/test/clang-tidy/zircon-fbl-move.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/zircon-fbl-move.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s zircon-fbl-move %t
+
+namespace fbl {
+
+void move(int i) {}
+
+} // namespace fbl
+
+namespace std {
+
+void move(int i) {}
+
+} // namespace std
+
+int main() {
+  int i;
+  fbl::move(i);
+  // CHECK-NOTES: [[@LINE-1]]:3: warning: fbl::move is deprecated, use std::move instead
+  // CHECK-FIXES: #include 
+  // CHECK-FIXES: std::move
+
+  std::move(i);
+}
Index: clang-tools-extra/docs/clang-tidy/checks/zircon-fbl-move.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/zircon-fbl-move.rst
@@ -0,0 +1,11 @@
+.. title:: clang-tidy - zircon-fbl-move
+
+zircon-fbl-move
+===
+
+This check is part of the migration checks for moving Zircon user code to use
+the C++ standard library.
+
+It suggests converting uses of ``fbl::move`` to ``std::move``, and suggests
+inserting the  header.
+
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -254,4 +254,5 @@
readability-string-compare
readability-uniqueptr-delete-release
readability-uppercase-literal-suffix
+   zircon-fbl-move
zircon-temporary-objects
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -182,6 +182,13 @@
   ` check does not warn
   about calls inside macros anymore by default.
 
+- New :doc:`zircon-fbl-move
+  ` check.
+
+  This check is part of the migration checks for moving Zircon user code to use
+  the C++ standard library. It suggests converting uses of ``fbl::move`` to
+  ``std::move``, and suggests inserting the  header.
+
 Improvements to include-fixer
 -
 
Index: clang-tools-extra/clang-tidy/zircon/ZirconTidyModule.cpp
===
--- clang-tools-extra/clang-tidy/zircon/ZirconTidyModule.cpp
+++ clang-tools-extra/clang-tidy/zircon/ZirconTidyModule.cpp
@@ -10,6 +10,7 @@
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "FblMoveCheck.h"
 #include "TemporaryObjectsCheck.h"
 
 using namespace clang::ast_matchers;
@@ -22,6 +23,8 @@
 class ZirconModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+CheckFactories.registerCheck(
+"zircon-fbl-move");
 CheckFactories.registerCheck(
 "zircon-temporary-objects");
   }
Index: clang-tools-extra/clang-tidy/zircon/FblMoveCheck.h
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/zircon/FblMoveCheck.h
@@ -0,0 +1,45 @@
+//===--- FblMoveCheck.h - clang-tidy *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ZIRCON_FBLINCLUDENEWCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ZIRCON_FBLINCLUDENEWCHECK_H
+
+#include "../ClangTidy.h"
+#include "../utils/IncludeInserter.h"
+
+namespace clang {
+namespace tidy {
+namespace zircon {
+
+/// Replace instances of fbl::move with std::move, and adds the  header
+/// if a replacement occurs.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/zircon-fbl-move.html
+class FblMoveCheck : public ClangTidyCheck {
+public:
+  FblMoveCheck(StringRef Name, ClangTi

[PATCH] D54169: [clang-tidy] Zircon ->

[Julie Hockett via Phabricator via cfe-commits]

juliehockett created this revision.
juliehockett added reviewers: aaron.ballman, alexfh, hokein.
juliehockett added a project: clang-tools-extra.
Herald added subscribers: xazax.hun, mgorny.

Adds a check to convert  to std .

This check is part of a set of migration checks as we prepare to move Zircon 
user code to use the C++ standard library, and should prevent regressions after 
the migration is complete.


https://reviews.llvm.org/D54169

Files:
  clang-tools-extra/clang-tidy/zircon/CMakeLists.txt
  clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.cpp
  clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.h
  clang-tools-extra/clang-tidy/zircon/ZirconTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/zircon-fbl-limits.rst
  clang-tools-extra/test/clang-tidy/Inputs/zircon/fbl/limits.h
  clang-tools-extra/test/clang-tidy/Inputs/zircon/transitive.h
  clang-tools-extra/test/clang-tidy/zircon-fbl-limits-transitive.cpp
  clang-tools-extra/test/clang-tidy/zircon-fbl-limits.cpp

Index: clang-tools-extra/test/clang-tidy/zircon-fbl-limits.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/zircon-fbl-limits.cpp
@@ -0,0 +1,45 @@
+// RUN: %check_clang_tidy %s zircon-fbl-limits %t -- -- -isystem %S/Inputs/zircon
+
+#include 
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: including fbl/limits.h is deprecated
+// CHECK-FIXES-NOT: #include 
+// CHECK-FIXES: #include 
+
+namespace fbl {
+
+template 
+class numeric_limits {};
+
+#define SPECIALIZE_INT_FBL(type) \
+  template <>\
+  class numeric_limits {};
+
+SPECIALIZE_INT_FBL(short)
+
+numeric_limits lim;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use of fbl::numeric_limits is deprecated, use std::numeric_limits instead
+// CHECK-FIXES: std::numeric_limits lim;
+
+} // namespace fbl
+
+namespace std {
+
+template 
+class numeric_limits {};
+
+#define SPECIALIZE_INT_STD(type) \
+  template <>\
+  class numeric_limits {};
+
+SPECIALIZE_INT_STD(short)
+
+numeric_limits lim;
+
+} // namespace std
+
+int main() {
+  fbl::numeric_limits fbllim;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use of fbl::numeric_limits is deprecated, use std::numeric_limits instead
+  // CHECK-FIXES: std::numeric_limits fbllim;
+  std::numeric_limits stdlim;
+}
Index: clang-tools-extra/test/clang-tidy/zircon-fbl-limits-transitive.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/zircon-fbl-limits-transitive.cpp
@@ -0,0 +1,11 @@
+// RUN: rm -rf %T/Headers
+// RUN: mkdir %T/Headers
+// RUN: cp -r %S/Inputs/zircon %T/Headers/zircon
+// RUN: %check_clang_tidy %s zircon-fbl-limits %t -- -header-filter=.* -- -std=c++11 -I %T/Headers/zircon
+// RUN: FileCheck -input-file=%T/Headers/zircon/transitive.h %s -check-prefix=CHECK-FIXES
+// RUN: rm -rf %T/Headers
+
+// transitive.h includes 
+#include "transitive.h"
+// CHECK-NOTES: :2:1: warning: including fbl/limits.h is deprecated, transitively included from {{.*}}.
+// CHECK-FIXES-NOT: #include 
Index: clang-tools-extra/test/clang-tidy/Inputs/zircon/transitive.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/Inputs/zircon/transitive.h
@@ -0,0 +1,2 @@
+// Transitively included headers.
+#include 
Index: clang-tools-extra/docs/clang-tidy/checks/zircon-fbl-limits.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/zircon-fbl-limits.rst
@@ -0,0 +1,13 @@
+.. title:: clang-tidy - zircon-fbl-limits
+
+zircon-fbl-limits
+=
+
+This check is part of the migration checks for moving Zircon user code to use
+the C++ standard library.
+
+It suggests converting uses of ``fbl::numeric_limits`` to
+``std::numeric_limits``, and suggests inserting the  header. It
+also suggests the removal of all uses of the  header, as all
+declarations therein will be replaced by the check and the appropriate
+ replacement header recommended.
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -254,4 +254,5 @@
readability-string-compare
readability-uniqueptr-delete-release
readability-uppercase-literal-suffix
+   zircon-fbl-limits
zircon-temporary-objects
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -182,6 +182,13 @@
   ` check does not warn
   about calls inside macros anymore by default.
 
+- New :doc:`zircon-fbl-limits
+  ` check.
+
+  This check is part of the migratio

[PATCH] D54168: [clang-tidy] Zircon fbl::move -> std::move

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I think this check is ok in the current form, but does it really need to be in 
upstream clang-tidy? You said its for migration only, so it is not valuable for 
you for a long time either?




Comment at: clang-tools-extra/clang-tidy/zircon/FblMoveCheck.h:20
+
+/// Replace instances of fbl::move with std::move, and adds the  
header
+/// if a replacement occurs.

Please highlight code with quotes (single or backticks)



Comment at: clang-tools-extra/test/clang-tidy/zircon-fbl-move.cpp:5
+
+void move(int i) {}
+

please add test for aliases, function pointers and similar indirections.


https://reviews.llvm.org/D54168



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

[PATCH] D54171: [MS] Zero out ECX in __cpuid in intrin.h

[Reid Kleckner via Phabricator via cfe-commits]

rnk created this revision.
rnk added reviewers: craig.topper, hans.

Some CPUID leafs depend on the value of ECX as well as EAX, but we left
it uninitialized.

Originally reported as https://crbug.com/901547


https://reviews.llvm.org/D54171

Files:
  clang/lib/Headers/intrin.h
  clang/test/CodeGen/ms-intrinsics-cpuid.c


Index: clang/test/CodeGen/ms-intrinsics-cpuid.c
===
--- /dev/null
+++ clang/test/CodeGen/ms-intrinsics-cpuid.c
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility 
-fms-compatibility-version=17.00 \
+// RUN: -triple i686-windows-msvc -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility 
-fms-compatibility-version=17.00 \
+// RUN: -triple x86_64-windows-msvc -emit-llvm %s -o - | FileCheck %s
+
+// intrin.h needs size_t, but -ffreestanding prevents us from getting it from
+// stddef.h.  Work around it with this typedef.
+typedef __SIZE_TYPE__ size_t;
+
+#include 
+
+void test__cpuid(int *info, int level) {
+  __cpuid(info, level);
+}
+// CHECK-LABEL: define {{.*}} @test__cpuid(i32* %{{.*}}, i32 %{{.*}})
+// CHECK: call { i32, i32, i32, i32 } asm "cpuid",
+// CHECK-SAME:   
"={ax},={bx},={cx},={dx},{ax},{cx},~{dirflag},~{fpsr},~{flags}"
+// CHECK-SAME:   (i32 %{{.*}}, i32 0)
Index: clang/lib/Headers/intrin.h
===
--- clang/lib/Headers/intrin.h
+++ clang/lib/Headers/intrin.h
@@ -535,7 +535,7 @@
 static __inline__ void __DEFAULT_FN_ATTRS
 __cpuid(int __info[4], int __level) {
   __asm__ ("cpuid" : "=a"(__info[0]), "=b" (__info[1]), "=c"(__info[2]), 
"=d"(__info[3])
-   : "a"(__level));
+   : "a"(__level), "c"(0));
 }
 static __inline__ void __DEFAULT_FN_ATTRS
 __cpuidex(int __info[4], int __level, int __ecx) {


Index: clang/test/CodeGen/ms-intrinsics-cpuid.c
===
--- /dev/null
+++ clang/test/CodeGen/ms-intrinsics-cpuid.c
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility -fms-compatibility-version=17.00 \
+// RUN: -triple i686-windows-msvc -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility -fms-compatibility-version=17.00 \
+// RUN: -triple x86_64-windows-msvc -emit-llvm %s -o - | FileCheck %s
+
+// intrin.h needs size_t, but -ffreestanding prevents us from getting it from
+// stddef.h.  Work around it with this typedef.
+typedef __SIZE_TYPE__ size_t;
+
+#include 
+
+void test__cpuid(int *info, int level) {
+  __cpuid(info, level);
+}
+// CHECK-LABEL: define {{.*}} @test__cpuid(i32* %{{.*}}, i32 %{{.*}})
+// CHECK: call { i32, i32, i32, i32 } asm "cpuid",
+// CHECK-SAME:   "={ax},={bx},={cx},={dx},{ax},{cx},~{dirflag},~{fpsr},~{flags}"
+// CHECK-SAME:   (i32 %{{.*}}, i32 0)
Index: clang/lib/Headers/intrin.h
===
--- clang/lib/Headers/intrin.h
+++ clang/lib/Headers/intrin.h
@@ -535,7 +535,7 @@
 static __inline__ void __DEFAULT_FN_ATTRS
 __cpuid(int __info[4], int __level) {
   __asm__ ("cpuid" : "=a"(__info[0]), "=b" (__info[1]), "=c"(__info[2]), "=d"(__info[3])
-   : "a"(__level));
+   : "a"(__level), "c"(0));
 }
 static __inline__ void __DEFAULT_FN_ATTRS
 __cpuidex(int __info[4], int __level, int __ecx) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Bruno Ricci via Phabricator via cfe-commits]

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

Use the newly available space in the bit-fields of `Stmt`
to store some data from `ArraySubscriptExpr` and `CallExpr`.
This saves a pointer for each of them (which is a lot given
how frequently `CallExpr` is used).

Additionally for `ArraySubscriptExpr` store a bit indicating
whether the LHS is the base of the array subscript expression,
instead of checking if the type of the RHS is an integer type during
each access. This removes an indirection during each access.


Repository:
  rC Clang

https://reviews.llvm.org/D54172

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

Index: lib/Serialization/ASTWriterStmt.cpp
===
--- lib/Serialization/ASTWriterStmt.cpp
+++ lib/Serialization/ASTWriterStmt.cpp
@@ -638,6 +638,7 @@
   VisitExpr(E);
   Record.AddStmt(E->getLHS());
   Record.AddStmt(E->getRHS());
+  Record.push_back(E->lhsIsBase());
   Record.AddSourceLocation(E->getRBracketLoc());
   Code = serialization::EXPR_ARRAY_SUBSCRIPT;
 }
Index: lib/Serialization/ASTReaderStmt.cpp
===
--- lib/Serialization/ASTReaderStmt.cpp
+++ lib/Serialization/ASTReaderStmt.cpp
@@ -725,6 +725,7 @@
   VisitExpr(E);
   E->setLHS(Record.readSubExpr());
   E->setRHS(Record.readSubExpr());
+  E->setLhsIsBase(Record.readInt());
   E->setRBracketLoc(ReadSourceLocation());
 }
 
Index: lib/AST/Expr.cpp
===
--- lib/AST/Expr.cpp
+++ lib/AST/Expr.cpp
@@ -1225,23 +1225,26 @@
ExprValueKind VK, SourceLocation rparenloc)
 : Expr(SC, t, VK, OK_Ordinary, fn->isTypeDependent(),
fn->isValueDependent(), fn->isInstantiationDependent(),
-   fn->containsUnexpandedParameterPack()),
-  NumArgs(args.size()) {
+   fn->containsUnexpandedParameterPack()) {
+  unsigned NumArgs = args.size();
+  CallExprBits.NumArgs = NumArgs;
+  assert((getNumArgs() == NumArgs) && "NumArgs overflow!");
 
   unsigned NumPreArgs = preargs.size();
-  SubExprs = new (C) Stmt *[args.size()+PREARGS_START+NumPreArgs];
+  CallExprBits.NumPreArgs = NumPreArgs;
+
+  SubExprs = new (C) Stmt *[NumArgs+PREARGS_START+NumPreArgs];
   SubExprs[FN] = fn;
   for (unsigned i = 0; i != NumPreArgs; ++i) {
 updateDependenciesFromArg(preargs[i]);
 SubExprs[i+PREARGS_START] = preargs[i];
   }
-  for (unsigned i = 0; i != args.size(); ++i) {
+  for (unsigned i = 0; i != NumArgs; ++i) {
 updateDependenciesFromArg(args[i]);
 SubExprs[i+PREARGS_START+NumPreArgs] = args[i];
   }
 
-  CallExprBits.NumPreArgs = NumPreArgs;
-  RParenLoc = rparenloc;
+  setRParenLoc(rparenloc);
 }
 
 CallExpr::CallExpr(const ASTContext &C, StmtClass SC, Expr *fn,
@@ -1259,7 +1262,8 @@
 
 CallExpr::CallExpr(const ASTContext &C, StmtClass SC, unsigned NumPreArgs,
EmptyShell Empty)
-  : Expr(SC, Empty), SubExprs(nullptr), NumArgs(0) {
+  : Expr(SC, Empty), SubExprs(nullptr) {
+  CallExprBits.NumArgs = 0;
   // FIXME: Why do we allocate this?
   SubExprs = new (C) Stmt*[PREARGS_START+NumPreArgs]();
   CallExprBits.NumPreArgs = NumPreArgs;
@@ -1317,7 +1321,7 @@
 
   // If shrinking # arguments, just delete the extras and forgot them.
   if (NumArgs < getNumArgs()) {
-this->NumArgs = NumArgs;
+CallExprBits.NumArgs = NumArgs;
 return;
   }
 
@@ -1334,7 +1338,7 @@
 
   if (SubExprs) C.Deallocate(SubExprs);
   SubExprs = NewSubExprs;
-  this->NumArgs = NumArgs;
+  CallExprBits.NumArgs = NumArgs;
 }
 
 /// getBuiltinCallee - If this is a call to a builtin, return the builtin ID. If
Index: include/clang/AST/Stmt.h
===
--- include/clang/AST/Stmt.h
+++ include/clang/AST/Stmt.h
@@ -395,6 +395,22 @@
 unsigned IsType : 1; // true if operand is a type, false if an expression.
   };
 
+  class ArraySubscriptExprBitfields {
+friend class ArraySubscriptExpr;
+friend class ASTStmtReader;
+
+unsigned : NumExprBits;
+
+/// True if the expression is of the form A[4], that is
+/// getLHS() == getBase(). False if the the expression is of the
+/// form 4[A], that is getRHS() == getBase(). See the comment
+/// above ArraySubscriptExpr::lhsIsBase.
+unsigned LhsIsBase : 1;
+
+/// The location of the right bracket.
+SourceLocation RBracketLoc;
+  };
+
   class DeclRefExprBitfields {
 friend class ASTStmtReader; // deserialization
 friend class DeclRefExpr;
@@ -431,6 +447,9 @@
 unsigned : NumExprBits;
 
 unsigned NumPreArgs : 1;
+unsigned NumArgs : 14;
+
+SourceLocation RParenLoc;
   };
 
   class MemberExprBitfields {
@@ -590,6 +609,7 @@
 CharacterLiteralBitfields CharacterLiteralBit

[PATCH] D54169: [clang-tidy] Zircon ->

[Roland McGrath via Phabricator via cfe-commits]

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

lgtm


https://reviews.llvm.org/D54169



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

[PATCH] D49736: [Basic] Emit warning flag suggestion only in case there's existing flag *similar* to the unknown one

[Jan Korous via Phabricator via cfe-commits]

jkorous requested review of this revision.
jkorous added a comment.

I tried to move the getNearestOption() to it's only client - 
EmitUnknownDiagWarning() but it turned out to be a significant change because 
of clang/Basic/DiagnosticGroups.inc use in DiagnosticIDs.cpp.
I suggest we leave that for eventual future refactoring since it would blow up 
scope of this patch a lot.


Repository:
  rC Clang

https://reviews.llvm.org/D49736



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

[PATCH] D54171: [MS] Zero out ECX in __cpuid in intrin.h

[Craig Topper via Phabricator via cfe-commits]

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

LGTM


https://reviews.llvm.org/D54171



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

r346265 - [MS] Zero out ECX in __cpuid in intrin.h

[Reid Kleckner via cfe-commits]

Author: rnk
Date: Tue Nov  6 12:45:26 2018
New Revision: 346265

URL: http://llvm.org/viewvc/llvm-project?rev=346265&view=rev
Log:
[MS] Zero out ECX in __cpuid in intrin.h

Summary:
Some CPUID leafs depend on the value of ECX as well as EAX, but we left
it uninitialized.

Originally reported as https://crbug.com/901547

Reviewers: craig.topper, hans

Subscribers: cfe-commits

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

Added:
cfe/trunk/test/CodeGen/ms-intrinsics-cpuid.c
Modified:
cfe/trunk/lib/Headers/intrin.h

Modified: cfe/trunk/lib/Headers/intrin.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Headers/intrin.h?rev=346265&r1=346264&r2=346265&view=diff
==
--- cfe/trunk/lib/Headers/intrin.h (original)
+++ cfe/trunk/lib/Headers/intrin.h Tue Nov  6 12:45:26 2018
@@ -535,7 +535,7 @@ __stosq(unsigned __int64 *__dst, unsigne
 static __inline__ void __DEFAULT_FN_ATTRS
 __cpuid(int __info[4], int __level) {
   __asm__ ("cpuid" : "=a"(__info[0]), "=b" (__info[1]), "=c"(__info[2]), 
"=d"(__info[3])
-   : "a"(__level));
+   : "a"(__level), "c"(0));
 }
 static __inline__ void __DEFAULT_FN_ATTRS
 __cpuidex(int __info[4], int __level, int __ecx) {

Added: cfe/trunk/test/CodeGen/ms-intrinsics-cpuid.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/ms-intrinsics-cpuid.c?rev=346265&view=auto
==
--- cfe/trunk/test/CodeGen/ms-intrinsics-cpuid.c (added)
+++ cfe/trunk/test/CodeGen/ms-intrinsics-cpuid.c Tue Nov  6 12:45:26 2018
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility 
-fms-compatibility-version=17.00 \
+// RUN: -triple i686-windows-msvc -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility 
-fms-compatibility-version=17.00 \
+// RUN: -triple x86_64-windows-msvc -emit-llvm %s -o - | FileCheck %s
+
+// intrin.h needs size_t, but -ffreestanding prevents us from getting it from
+// stddef.h.  Work around it with this typedef.
+typedef __SIZE_TYPE__ size_t;
+
+#include 
+
+void test__cpuid(int *info, int level) {
+  __cpuid(info, level);
+}
+// CHECK-LABEL: define {{.*}} @test__cpuid(i32* %{{.*}}, i32 %{{.*}})
+// CHECK: call { i32, i32, i32, i32 } asm "cpuid",
+// CHECK-SAME:   
"={ax},={bx},={cx},={dx},{ax},{cx},~{dirflag},~{fpsr},~{flags}"
+// CHECK-SAME:   (i32 %{{.*}}, i32 0)


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

[PATCH] D54171: [MS] Zero out ECX in __cpuid in intrin.h

[Reid Kleckner via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL346265: [MS] Zero out ECX in __cpuid in intrin.h (authored 
by rnk, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54171?vs=172811&id=172831#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D54171

Files:
  cfe/trunk/lib/Headers/intrin.h
  cfe/trunk/test/CodeGen/ms-intrinsics-cpuid.c


Index: cfe/trunk/test/CodeGen/ms-intrinsics-cpuid.c
===
--- cfe/trunk/test/CodeGen/ms-intrinsics-cpuid.c
+++ cfe/trunk/test/CodeGen/ms-intrinsics-cpuid.c
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility 
-fms-compatibility-version=17.00 \
+// RUN: -triple i686-windows-msvc -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility 
-fms-compatibility-version=17.00 \
+// RUN: -triple x86_64-windows-msvc -emit-llvm %s -o - | FileCheck %s
+
+// intrin.h needs size_t, but -ffreestanding prevents us from getting it from
+// stddef.h.  Work around it with this typedef.
+typedef __SIZE_TYPE__ size_t;
+
+#include 
+
+void test__cpuid(int *info, int level) {
+  __cpuid(info, level);
+}
+// CHECK-LABEL: define {{.*}} @test__cpuid(i32* %{{.*}}, i32 %{{.*}})
+// CHECK: call { i32, i32, i32, i32 } asm "cpuid",
+// CHECK-SAME:   
"={ax},={bx},={cx},={dx},{ax},{cx},~{dirflag},~{fpsr},~{flags}"
+// CHECK-SAME:   (i32 %{{.*}}, i32 0)
Index: cfe/trunk/lib/Headers/intrin.h
===
--- cfe/trunk/lib/Headers/intrin.h
+++ cfe/trunk/lib/Headers/intrin.h
@@ -535,7 +535,7 @@
 static __inline__ void __DEFAULT_FN_ATTRS
 __cpuid(int __info[4], int __level) {
   __asm__ ("cpuid" : "=a"(__info[0]), "=b" (__info[1]), "=c"(__info[2]), 
"=d"(__info[3])
-   : "a"(__level));
+   : "a"(__level), "c"(0));
 }
 static __inline__ void __DEFAULT_FN_ATTRS
 __cpuidex(int __info[4], int __level, int __ecx) {


Index: cfe/trunk/test/CodeGen/ms-intrinsics-cpuid.c
===
--- cfe/trunk/test/CodeGen/ms-intrinsics-cpuid.c
+++ cfe/trunk/test/CodeGen/ms-intrinsics-cpuid.c
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility -fms-compatibility-version=17.00 \
+// RUN: -triple i686-windows-msvc -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility -fms-compatibility-version=17.00 \
+// RUN: -triple x86_64-windows-msvc -emit-llvm %s -o - | FileCheck %s
+
+// intrin.h needs size_t, but -ffreestanding prevents us from getting it from
+// stddef.h.  Work around it with this typedef.
+typedef __SIZE_TYPE__ size_t;
+
+#include 
+
+void test__cpuid(int *info, int level) {
+  __cpuid(info, level);
+}
+// CHECK-LABEL: define {{.*}} @test__cpuid(i32* %{{.*}}, i32 %{{.*}})
+// CHECK: call { i32, i32, i32, i32 } asm "cpuid",
+// CHECK-SAME:   "={ax},={bx},={cx},={dx},{ax},{cx},~{dirflag},~{fpsr},~{flags}"
+// CHECK-SAME:   (i32 %{{.*}}, i32 0)
Index: cfe/trunk/lib/Headers/intrin.h
===
--- cfe/trunk/lib/Headers/intrin.h
+++ cfe/trunk/lib/Headers/intrin.h
@@ -535,7 +535,7 @@
 static __inline__ void __DEFAULT_FN_ATTRS
 __cpuid(int __info[4], int __level) {
   __asm__ ("cpuid" : "=a"(__info[0]), "=b" (__info[1]), "=c"(__info[2]), "=d"(__info[3])
-   : "a"(__level));
+   : "a"(__level), "c"(0));
 }
 static __inline__ void __DEFAULT_FN_ATTRS
 __cpuidex(int __info[4], int __level, int __ecx) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In https://reviews.llvm.org/D54141#1288930, @JonasToth wrote:

> > Do you understand the proposal now?
>
> Yes better, I was under the impression that `clang-apply-replaments` is run 
> on the end and the YAMLs are kept until then. Now its clear.
>  I assume `--issue-diags` produce the same result as the normal diagnostic 
> engine. That could work, yes.


Great, glad we got that misunderstanding sorted out.

> clang-tidy does not have a `quiet` mode though. It has the `-quiet` option 
> which just does not emit how many warnings were created and suppressed.
>  Do you have these things already in the pipeline?

No, I have not started on these.

At least the clang-tidy quiet mode is trivial to implement. Maybe instead of 
`--quiet` we could have `--stdout=` where `output_format` can be 
one of `none`, `diag`, `yaml` and in the future possibly `json` (requested 
here: http://lists.llvm.org/pipermail/cfe-dev/2018-October/059944.html) or 
`cbor`, to address the binary output suggestion from @hokein.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54141



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

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In https://reviews.llvm.org/D54141#1288930, @JonasToth wrote:

> > Do you understand the proposal now?
>
> Yes better, I was under the impression that `clang-apply-replaments` is run 
> on the end and the YAMLs are kept until then. Now its clear.
>  I assume `--issue-diags` produce the same result as the normal diagnostic 
> engine. That could work, yes.
>
> clang-tidy does not have a `quiet` mode though. It has the `-quiet` option 
> which just does not emit how many warnings were created and suppressed.
>  Do you have these things already in the pipeline?


Please let me clarify a bit: `-export-fixes` _DOES_ emit all warnings to yaml, 
but clang-tidy still prints the diagnostics out, even in `-quiet` mode. So a 
useful deduplication would require changes to the clang-tidy itself if going 
the YAML route.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54141



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

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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In https://reviews.llvm.org/D53329#1270035, @yonghong-song wrote:

> Sure. Let me provide a little bit more context and what I want to achieve:
>
>   . I have a tool, called bcc (https://github.com/iovisor/bcc) which uses 
> clang CompilerInvocation interface and
> MCJIT to generates BPF code and load into kernel
>   . Several files (the main.c and a few others headers) are clang memory 
> mapped.
>   . The particular fix here is related to https://reviews.llvm.org/D53261, 
> getting source code into BTF, but
> before that, based on that particular implementation, the source code 
> needs to be in IR.
> What I found is that for the memory mapped /virtual/main.c file, there is 
> one DIFile entry in
> generated IR, the associated 'source' is empty, which actually caused a 
> seg fault later.
>   
> Not that this bug itself does not need https://reviews.llvm.org/D53261.
>   
>
> So without this fix, bcc tool will seg fault.
>  With this fix, bcc tool works properly and all DIFile entry has proper 
> source codes embedded, which
>  if coupled with IR->BTF or Dwarf->BTF should generate correct BTF debug info.


Thanks for the explanation/context.

Any chance of a test case? I'm not sure how the current virtual filesystem 
support is tested & whether it could be extended/used to cover this issue.


Repository:
  rC Clang

https://reviews.llvm.org/D53329



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

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> At least the clang-tidy quiet mode is trivial to implement. Maybe instead of 
> `--quiet` we could have `--stdout=` where `output_format` can 
> be one of `none`, `diag`, `yaml` and in the future possibly `json` (requested 
> here: http://lists.llvm.org/pipermail/cfe-dev/2018-October/059944.html) or 
> `cbor`, to address the binary output suggestion from @hokein.

Yes, it would slighlty duplicate `export-fixes` but i think that is not a big 
issue.
I think the first steps would be using parallel execution within clang-tidy 
itself.
After that we can extract the deduplication logic from apply-replacements (if 
it is actually suitable!) or deduplicate in the `diag()` emitting phase. A 
thing we have to keep in mind, that deduplication must happen to the whole 
`warning: blaaa\n note: blaa \n note: 'aslkdjad' here comes the only change 
between two different warnings` construct.
If we don't group the diagnostics properly we have a bad time.

That said, would you agree to have the parser-based deduplication as an 
developer-only optin solution for now? :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54141



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

r346266 - Don't use std::next() on an input iterator; NFC.

[Aaron Ballman via cfe-commits]

Author: aaronballman
Date: Tue Nov  6 13:12:44 2018
New Revision: 346266

URL: http://llvm.org/viewvc/llvm-project?rev=346266&view=rev
Log:
Don't use std::next() on an input iterator; NFC.

Instead, advance the old-fashioned way, as std::next() cannot be used on an 
input iterator until C++17.

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

Modified: cfe/trunk/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp?rev=346266&r1=346265&r2=346266&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp Tue Nov  6 13:12:44 
2018
@@ -82,25 +82,27 @@ static std::string fileNameToURI(StringR
 Ret += Twine("/" + Root).str();
   }
 
-  // Add the rest of the path components, encoding any reserved characters.
-  std::for_each(std::next(sys::path::begin(Filename)), 
sys::path::end(Filename),
-[&Ret](StringRef Component) {
-  // For reasons unknown to me, we may get a backslash with
-  // Windows native paths for the initial backslash following
-  // the drive component, which we need to ignore as a URI path
-  // part.
-  if (Component == "\\")
-return;
+  auto Iter = sys::path::begin(Filename), End = sys::path::end(Filename);
+  if (Iter != End) {
+// Add the rest of the path components, encoding any reserved characters;
+// we skip past the first path component, as it was handled it above.
+std::for_each(++Iter, End, [&Ret](StringRef Component) {
+  // For reasons unknown to me, we may get a backslash with Windows native
+  // paths for the initial backslash following the drive component, which
+  // we need to ignore as a URI path part.
+  if (Component == "\\")
+return;
 
-  // Add the separator between the previous path part and the
-  // one being currently processed.
-  Ret += "/";
+  // Add the separator between the previous path part and the one being
+  // currently processed.
+  Ret += "/";
 
-  // URI encode the part.
-  for (char C : Component) {
-Ret += percentEncodeURICharacter(C);
-  }
-});
+  // URI encode the part.
+  for (char C : Component) {
+Ret += percentEncodeURICharacter(C);
+  }
+});
+  }
 
   return Ret.str().str();
 }


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

[PATCH] D54169: [clang-tidy] Zircon ->

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.cpp:111
+  // Add in the  header, since we know this file uses it.
+  if (auto IncludeFixit = Inserter->CreateIncludeInsertion(
+  SM.getFileID(V->getLocation()), "limits",

Please don't use auto because type could not be deduced from statement.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:188
+
+  This check is part of the migration checks for moving Zircon user code to use
+  the C++ standard library. It suggests converting uses of declarations in the

I think first statement is not necessary. Second should start from 
//Suggests//. Will be good idea to synchronize it with documentation.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:190
+  the C++ standard library. It suggests converting uses of declarations in the
+   header to their std counterparts in .
+

Please highlight std with ``.


https://reviews.llvm.org/D54169



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

[PATCH] D54168: [clang-tidy] Zircon fbl::move -> std::move

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/zircon/FblMoveCheck.cpp:61
+// Add in the  header.
+if (auto IncludeFixit =
+Inserter->CreateIncludeInsertion(SM.getFileID(Start), "utility",

Please don't use auto when type could not be deduced from statement.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:188
+
+  This check is part of the migration checks for moving Zircon user code to use
+  the C++ standard library. It suggests converting uses of ``fbl::move`` to

I think first statement is not necessary. Second should start from 
//Suggests//. Will be good idea to synchronize it with documentation.


https://reviews.llvm.org/D54168



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

[PATCH] D54169: [clang-tidy] Zircon ->

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.cpp:47
+SrcMgr::CharacteristicKind FileType) {
+  if (FileName == "fbl/limits.h") {
+unsigned End = std::strcspn(SM.getCharacterData(HashLoc), "\n") + 1;

Does this also work on platforms where the path separator is `\` instead of 
`/`? What about case insensitive file systems where it may be spelled 
`LiMiTs.H`? Does this properly handle a case like:
```
#define LIMITS "fbl/limits.h"
#include LIMITS
```
(Should add test cases for all of these scenarios.)



Comment at: clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.cpp:70
+void FblLimitsCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(varDecl(hasType(cxxRecordDecl(allOf(
+ hasDeclContext(namespaceDecl(hasName("fbl"))),

Can users specialize `fbl::numeric_limits` for custom integral types like they 
can for `std::numeric_limits`? If so, that should maybe be covered with a 
checker as well.



Comment at: clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.cpp:71
+  Finder->addMatcher(varDecl(hasType(cxxRecordDecl(allOf(
+ hasDeclContext(namespaceDecl(hasName("fbl"))),
+ hasName("numeric_limits")

`::fbl` instead of `fbl` to cover a situation like:
```
namespace oops {
namespace fbl {
struct numeric_limits {};
}

void foo() {
  fbl::numeric_limits n;
}
}


https://reviews.llvm.org/D54169



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

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

[Scott Linder via Phabricator via cfe-commits]

scott.linder added a comment.

I don't believe that is currently the case (the unrestricted linking of OCL 
code to OCL code via a dynamic linker), but we do have the notion of a static 
link step, followed by dynamic linking at runtime. The static link step is 
currently via IR, but we plan to support linking object files. Maybe I 
misunderstand the distinction between linkage and visibility, but it seems 
reasonable that a user would want to have e.g. a non-kernel function 
participate in static linking, but not be preemptible in the final shared 
object. The intention with this patch is to allow this with something like 
`-fvisibility hidden` without disrupting kernel symbols, which must appear in 
the dynsym for the reasons mentioned earlier in the thread.


https://reviews.llvm.org/D53153



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

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In https://reviews.llvm.org/D54141#1289326, @JonasToth wrote:

>




> That said, would you agree to have the parser-based deduplication as an 
> developer-only optin solution for now? :)

If you're suggesting proceeding with this regex based solution, I don't think 
that's a good idea. Why commit a hack which people will object to ever 
removing? Just see if we can do the right thing instead.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54141



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