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

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

baloghadamsoftware accepted this revision.
baloghadamsoftware added a comment.

LGTM!


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

https://reviews.llvm.org/D57922



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

[PATCH] D53701: [Analyzer] Instead of recording comparisons in interator checkers do an eager state split

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

baloghadamsoftware added a comment.

Ping!


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

https://reviews.llvm.org/D53701



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

[PATCH] D55007: [Analyzer] Constraint Manager - Calculate Effective Range for Differences

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

baloghadamsoftware added a comment.
Herald added a subscriber: Charusso.

Ping!


Repository:
  rC Clang

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

https://reviews.llvm.org/D55007



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

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

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

baloghadamsoftware added a comment.

In D57860#1432728 , @dcoughlin wrote:

> I think it would be better if the default for compatibility mode were 'true'. 
> That way this change will be backwards compatible and clients who want to 
> enforce stricter checking could enable it. Setting compatibility mode to be 
> true in the driver is not sufficient since many build systems call the 
> frontend directly.


How long must compatibility be kept?


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

https://reviews.llvm.org/D57860



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

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

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D55170#1431854 , @MyDeveloperDay 
wrote:

> > I'm sorry for not having a positive answer here, but I'm not in favor of 
> > this change. The style rule looks like it introduces arbitrary complexity 
> > at a point where I don't understand at all  how it matters. We cannot 
> > possibly support all style guides that come up with random variance. I do 
> > not think this option pulls its weight.
>
> So is this a personal opinion or based on a technical reason (other than the 
> normal mantra about risk & cost),


I don't think risk is what matters - in the end it's about cost, and cost is a 
very technical reason.

>   Could we give @reuk some positive feedback about what he needs to change in 
> this review to get this accepted? He has the other boxes checked about public 
> style etc..
>
> 
> It may not be your preference but its not without some merit for those of us 
> who like to fine tune our style, if that can be done with minimal impact to 
> other styles and is well covered by tests, then can we at least have some 
> discussion first

I'm fine having a discussion - my main question is why this matters. This seems 
a lot more arbitrary than other things we have.

> @reuk please lets continue to fix this revision so it builds as I want to 
> pull it into
> 
> https://github.com/mydeveloperday/clang-experimental
> 
> As an aside, Wouldn't it be great if JUCE was a public style so you could 
> just say
> 
> BasedOnStyle: JUCE
> 
> Where that style defines what JUCE uses? I've always found this odd, I've 
> never understood why we only have Google,WebKit,Mozilla,LLVM, I would have 
> thought it would have been nice to have other big guns Microsoft,Qt generally 
> its only going to be a single function setting the defaults.
> 
> I actually think a change like that would really help people so they don't 
> have to work out all the individual styles, it would help keep .clang-format 
> files small and neat and not full of a bunch of different options.
> 
> Even if JUCE was based on another style say "Google" the code should really 
> be saying
> 
> if (Style.isGoogleDerivedStyle())
> 
>   ..
>
> 
> and not
> 
> Style == FormatStyle::Google
> 
> When its specific to JUCE it would say
> 
> if (Style.isJUCEDerivedStyle())
> 
>   ..
>
> 
> I know when using LLVM, that being able to have a .clang-format file that 
> just says
> 
> BasedOnStyle: LLVM
> 
> is really nice, I don't have to go work out what the LLVM style is, I just 
> get what the project defines, I think it would be great that anything that 
> passes the public style test for submissions should really be able to add 
> that kind of configuration.




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

https://reviews.llvm.org/D55170



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

[PATCH] D59407: [clangd] Add RelationSlab

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clangd/index/Index.h:59
+return sizeof(*this) + Arena.getTotalMemory() +
+   sizeof(value_type) * Relations.size();
+  }

nridge wrote:
> kadircet wrote:
> > use capacity instead of size
> Note, `RefSlab::bytes()` (which I where I copied this from) uses `size()` as 
> well. Should I change that too?
I'd say yes -- in a separate patch though.  Thanks for catching it!



Comment at: clang-tools-extra/clangd/index/Relation.h:1
+//===--- Ref.h ---*- 
C++-*-===//
+//

"--- Relation.h "



Comment at: clang-tools-extra/clangd/index/Relation.h:41
+public:
+  // Key.Symbol is Key.Kind of every symbol in Value.
+  // For example, if Key.Kind == SymbolRole::RelationChildOf,

Three slashes for doc comments, please.



Comment at: clang-tools-extra/clangd/index/Relation.h:45
+  // in Value are the base classes of Key.Symbol).
+  struct Relation {
+RelationKey Key;

Lift it up into the `clang::clangd` namespace?  (like `Symbol` and `Ref`)



Comment at: clang-tools-extra/clangd/index/Relation.h:68
+
+  // RelationSlab::Builder is a mutable container that can 'freeze' to
+  // RelationSlab.

No need to repeat the type name being documented.  "A mutable container that 
can ..."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59407



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

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

[Reuben Thomas via Phabricator via cfe-commits]

reuk added a comment.

@klimek I agree that the rule is somewhat arbitrary. However, it's the style 
rule of an established codebase with many users (I don't have a concrete 
number, but the project has 1400 stars on github 
). I've found this patch useful when 
contributing to JUCE and I thought others might too.


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

https://reviews.llvm.org/D55170



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

[PATCH] D59481: [clangd] Count number of references while merging RefSlabs inside FileIndex

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

For counting number of references clangd was relying on merging every
duplication of a symbol. Unfortunately this does not apply to FileIndex(and one
of its users' BackgroundIndex), since we get rid of duplication by simply
dropping symbols coming from non-canonical locations. So only one or two(coming
from canonical declaration header and defined source file, if exists)
replications of the same symbol reaches merging step.

This patch changes reference counting logic to rather count number of different
RefSlabs a given SymbolID exists.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59481

Files:
  clang-tools-extra/clangd/index/FileIndex.cpp
  clang-tools-extra/unittests/clangd/BackgroundIndexTests.cpp

Index: clang-tools-extra/unittests/clangd/BackgroundIndexTests.cpp
===
--- clang-tools-extra/unittests/clangd/BackgroundIndexTests.cpp
+++ clang-tools-extra/unittests/clangd/BackgroundIndexTests.cpp
@@ -32,6 +32,7 @@
   return !arg.IsTU && !arg.URI.empty() && arg.Digest == FileDigest{{0}} &&
  arg.DirectIncludes.empty();
 }
+MATCHER_P(NumReferences, N, "") { return arg.References == N; }
 
 class MemoryShardStorage : public BackgroundIndexStorage {
   mutable std::mutex StorageMu;
@@ -127,20 +128,25 @@
   CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
 
   ASSERT_TRUE(Idx.blockUntilIdleForTest());
-  EXPECT_THAT(
-  runFuzzyFind(Idx, ""),
-  UnorderedElementsAre(Named("common"), Named("A_CC"), Named("g"),
-   AllOf(Named("f_b"), Declared(), Not(Defined();
+  EXPECT_THAT(runFuzzyFind(Idx, ""),
+  UnorderedElementsAre(AllOf(Named("common"), NumReferences(2U)),
+   AllOf(Named("A_CC"), NumReferences(1U)),
+   AllOf(Named("g"), NumReferences(0U)),
+   AllOf(Named("f_b"), Declared(),
+ Not(Defined()), NumReferences(1U;
 
   Cmd.Filename = testPath("root/B.cc");
   Cmd.CommandLine = {"clang++", Cmd.Filename};
-  CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+  CDB.setCompileCommand(testPath("root/B.cc"), Cmd);
 
   ASSERT_TRUE(Idx.blockUntilIdleForTest());
   // B_CC is dropped as we don't collect symbols from A.h in this compilation.
   EXPECT_THAT(runFuzzyFind(Idx, ""),
-  UnorderedElementsAre(Named("common"), Named("A_CC"), Named("g"),
-   AllOf(Named("f_b"), Declared(), Defined(;
+  UnorderedElementsAre(AllOf(Named("common"), NumReferences(3U)),
+   AllOf(Named("A_CC"), NumReferences(1U)),
+   AllOf(Named("g"), NumReferences(0U)),
+   AllOf(Named("f_b"), Declared(), Defined(),
+ NumReferences(2U;
 
   auto Syms = runFuzzyFind(Idx, "common");
   EXPECT_THAT(Syms, UnorderedElementsAre(Named("common")));
Index: clang-tools-extra/clangd/index/FileIndex.cpp
===
--- clang-tools-extra/clangd/index/FileIndex.cpp
+++ clang-tools-extra/clangd/index/FileIndex.cpp
@@ -27,6 +27,47 @@
 
 namespace clang {
 namespace clangd {
+namespace {
+// Puts all refs for the same symbol into RefsStorage in a contigious way.
+// If MergedSyms is provided, increments a symbols Reference count once for
+// every file it was mentioned in.
+void mergeRefs(llvm::ArrayRef> RefSlabs,
+   std::vector &RefsStorage,
+   llvm::DenseMap> &AllRefs,
+   llvm::DenseMap *MergedSyms = nullptr) {
+  llvm::DenseMap> MergedRefs;
+  size_t Count = 0;
+  for (const auto &RefSlab : RefSlabs) {
+for (const auto &Sym : *RefSlab) {
+  MergedRefs[Sym.first].append(Sym.second.begin(), Sym.second.end());
+  Count += Sym.second.size();
+  if (MergedSyms) {
+auto It = MergedSyms->find(Sym.first);
+assert(It != MergedSyms->end() && "Reference to unknown symbol!");
+// Note that we increment References per each file mentioning the
+// symbol, including headers. This only happens once for each file since
+// FileIndex keeps only oneslab per file.
+// This contradicts with behavior inside clangd-indexer, it only counts
+// translation units since it processes each header multiple times.
+It->getSecond().References++;
+  }
+}
+  }
+  RefsStorage.reserve(Count);
+  AllRefs.reserve(MergedRefs.size());
+  for (auto &Sym : MergedRefs) {
+auto &SymRefs = Sym.second;
+// Sorting isn't required, but yields more stable results over rebuilds.
+llvm::sort(SymRefs);
+llvm::copy(SymRefs, back_inserter(Ref

[PATCH] D59415: Do not resolve directory junctions for `-fdiagnostics-absolute-paths` on Windows.

[Hans Wennborg via Phabricator via cfe-commits]

hans added inline comments.



Comment at: test/Frontend/absolute-paths-windows.test:4
+// RUN: mkdir -p %t.dir\real
+// RUN: cmd /c mklink /j %t.dir\junc %t.dir\real
+// RUN: echo "wrong code" > %t.dir\real\foo.cpp

This requires (a recent version of) Win 10 or later to run without running as 
Admin, right?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59415



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6390
+/// Given OpenMP directive, matches the first clause (out of all specified),
+/// that matches InnerMatcher.
+///

Other comments usually don't explain the interactions with inner matchers, 
unless the semantics are unusual.

"Matches any clause in an OpenMP directive."



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6400
+/// \endcode
+AST_MATCHER_P(OMPExecutableDirective, hasClause, internal::Matcher,
+  InnerMatcher) {

Looking at other similar matches, they usually follow the naming pattern 
`hasAny~`, WDYT?

(`hasAnyDeclaration`, `hasAnyConstructorInitializer` etc.)



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6411
+///
+/// Given, `ompDefaultClause()`:
+///

Please follow the existing comment style.  Either:

```
Given

\code

\endcode

 matches "".
```

or

```
Example:  matches "" in 

\code

\endcode   
```

For example:

```
Given

 \code
   #pragma omp parallel default(none)
   #pragma omp parallel default(shared)
   #pragma omp parallel
 \endcode

``ompDefaultClause()`` matches ``default(none)` and ``default(shared)``.
```

Similarly for other comments in this patch.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6433
+  return Node.getDefaultKind() == OMPC_DEFAULT_none;
+}
+

Also add `isSharedKind`?  It is weird that we have one but not the other.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6449
+///
+/// NOTE: while the matcher takes the *matcher* for the OpenMP clause,
+///   it does *NOT* actually match that matcher. It only fetches the

"while this matcher"



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

Why not make `isAllowedToContainClause` take an `OpenMPClauseKind` enum value?

I don't see right now advantages for taking a matcher.  (For example, it can't 
be a more complex matcher with inner matchers, it can't be a disjunction of 
matchers etc.)



Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+

I'm not sure if breaking out the source code into the "SourceX" variables 
improves readability.  WDYT about inlining the code into the EXPECT_TRUE code 
like in other tests in this file?

If you want to break it out, I'd suggest to drop "`void x() {`" down to the 
next line, so that all code lines start at the same column.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked 4 inline comments as done.
lebedev.ri added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

gribozavr wrote:
> Why not make `isAllowedToContainClause` take an `OpenMPClauseKind` enum value?
> 
> I don't see right now advantages for taking a matcher.  (For example, it 
> can't be a more complex matcher with inner matchers, it can't be a 
> disjunction of matchers etc.)
I don't feel like it, it's uglier.
The matcher is documented, `OpenMPClauseKind` is not documented.
Also, how will passing some random enum work with e.g. clang-query?




Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+

gribozavr wrote:
> I'm not sure if breaking out the source code into the "SourceX" variables 
> improves readability.  WDYT about inlining the code into the EXPECT_TRUE code 
> like in other tests in this file?
> 
> If you want to break it out, I'd suggest to drop "`void x() {`" down to the 
> next line, so that all code lines start at the same column.
> I'm not sure if breaking out the source code into the "SourceX" variables 
> improves readability

It's not about readability. Inlining will break the build, rC354201.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D59481: [clangd] Count number of references while merging RefSlabs inside FileIndex

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

I'm not sure if FileIndex is the correct layer to make decision about how 
References is calculated. Currently, we have two use cases in clangd 1) one 
slab per TU and 2) one slab per file. It seems to me that we would want 
different strategies for the two use cases, so it might make sense to make this 
configurable in FileIndex.  And in one case, we have `References` ~= # of TUs, 
and in the other case, we would have `References` ~= # of files. This can 
over-count `References`  in the second case. It might be fine as an 
approximation (if there is no better solution), but we should definitely 
document it (e.g. in `BackgroundIndex`).




Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:50
+// FileIndex keeps only oneslab per file.
+// This contradicts with behavior inside clangd-indexer, it only counts
+// translation units since it processes each header multiple times.

this is a bit of a code smell. FileIndex is not supposed to know anything about 
clangd-indexer etc.



Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:170
   I.first->second = mergeSymbol(I.first->second, Sym);
+// We re-count number of references while merging refs from scratch.
+I.first->getSecond().References = 0;

note that references might not always exist. SymbolCollector can be set up to 
not collect references.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59481



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

lebedev.ri wrote:
> gribozavr wrote:
> > Why not make `isAllowedToContainClause` take an `OpenMPClauseKind` enum 
> > value?
> > 
> > I don't see right now advantages for taking a matcher.  (For example, it 
> > can't be a more complex matcher with inner matchers, it can't be a 
> > disjunction of matchers etc.)
> I don't feel like it, it's uglier.
> The matcher is documented, `OpenMPClauseKind` is not documented.
> Also, how will passing some random enum work with e.g. clang-query?
> 
There are dozens of clauses in `OpenMPClauseKind`.  We would have to replicate 
them all as matchers to provide a useful API.

> Also, how will passing some random enum work with e.g. clang-query?

See `llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h`.



Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+

lebedev.ri wrote:
> gribozavr wrote:
> > I'm not sure if breaking out the source code into the "SourceX" variables 
> > improves readability.  WDYT about inlining the code into the EXPECT_TRUE 
> > code like in other tests in this file?
> > 
> > If you want to break it out, I'd suggest to drop "`void x() {`" down to the 
> > next line, so that all code lines start at the same column.
> > I'm not sure if breaking out the source code into the "SourceX" variables 
> > improves readability
> 
> It's not about readability. Inlining will break the build, rC354201.
Other tests in this file use string concatenation, see 
`TEST(DeclarationMatcher, MatchHasRecursiveAllOf)` for example.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D59463: [ASTMatchers][OpenMP] OpenMP Structured-block-related matchers

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6389
 
+/// Given OpenMP directive, matches if it is an OpenMP standalone directive,
+/// i.e. an executable directive with no structured block.

"Matches standalone OpenMP directives, i.e., directives that can't have a 
structured block."



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6400
+///   #pragma omp taskyield  // <- match, is standalone
+/// \endcode
+AST_MATCHER(OMPExecutableDirective, isStandaloneDirective) {

Please follow the existing comment style. Either:

```
Given

\code

\endcode

 matches "".
```

or

```
Example:  matches "" in 

\code

\endcode
For example:

Given

 \code
   #pragma omp parallel default(none)
   #pragma omp parallel default(shared)
   #pragma omp parallel
 \endcode

``ompDefaultClause()`` matches ``default(none)` and ``default(shared)``.
```

Similarly for other comments in this patch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59463



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked 5 inline comments as done.
lebedev.ri added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

gribozavr wrote:
> lebedev.ri wrote:
> > gribozavr wrote:
> > > Why not make `isAllowedToContainClause` take an `OpenMPClauseKind` enum 
> > > value?
> > > 
> > > I don't see right now advantages for taking a matcher.  (For example, it 
> > > can't be a more complex matcher with inner matchers, it can't be a 
> > > disjunction of matchers etc.)
> > I don't feel like it, it's uglier.
> > The matcher is documented, `OpenMPClauseKind` is not documented.
> > Also, how will passing some random enum work with e.g. clang-query?
> > 
> There are dozens of clauses in `OpenMPClauseKind`.  We would have to 
> replicate them all as matchers to provide a useful API.
> 
> > Also, how will passing some random enum work with e.g. clang-query?
> 
> See `llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h`.
True. Also, but there's dosens of Stmt types, and there is no overload that 
takes `StmtClass` enum.



Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+

gribozavr wrote:
> lebedev.ri wrote:
> > gribozavr wrote:
> > > I'm not sure if breaking out the source code into the "SourceX" variables 
> > > improves readability.  WDYT about inlining the code into the EXPECT_TRUE 
> > > code like in other tests in this file?
> > > 
> > > If you want to break it out, I'd suggest to drop "`void x() {`" down to 
> > > the next line, so that all code lines start at the same column.
> > > I'm not sure if breaking out the source code into the "SourceX" variables 
> > > improves readability
> > 
> > It's not about readability. Inlining will break the build, rC354201.
> Other tests in this file use string concatenation, see 
> `TEST(DeclarationMatcher, MatchHasRecursiveAllOf)` for example.
I'm sorry, but i fail to see how that is relevant?
I'm using multiline raw string literals, and inlining it will break the build, 
like i linked.
You are pointing at the code that is not using multiline raw string literals.
You only suggested inlining, not switching away from multiline raw string 
literals, i believe?

Not using multiline raw string literals looked even worse, because then you 
need to manually add "\n"


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

lebedev.ri wrote:
> gribozavr wrote:
> > lebedev.ri wrote:
> > > gribozavr wrote:
> > > > Why not make `isAllowedToContainClause` take an `OpenMPClauseKind` enum 
> > > > value?
> > > > 
> > > > I don't see right now advantages for taking a matcher.  (For example, 
> > > > it can't be a more complex matcher with inner matchers, it can't be a 
> > > > disjunction of matchers etc.)
> > > I don't feel like it, it's uglier.
> > > The matcher is documented, `OpenMPClauseKind` is not documented.
> > > Also, how will passing some random enum work with e.g. clang-query?
> > > 
> > There are dozens of clauses in `OpenMPClauseKind`.  We would have to 
> > replicate them all as matchers to provide a useful API.
> > 
> > > Also, how will passing some random enum work with e.g. clang-query?
> > 
> > See `llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h`.
> True. Also, but there's dosens of Stmt types, and there is no overload that 
> takes `StmtClass` enum.
For Stmts, we do have dozens of individual matchers for them.

The point of your work is to add ASTMatchers for OpenMP, right?  However, if 
there are no matchers for a reasonable amount of AST surface, it is as good as 
if the matchers are not there, because prospective users won't be able to use 
them.

I don't particularly care how exactly this is achieved, through individual 
matchers or through a matcher that takes an enum.  However, I want to make sure 
that if you're going through all this trouble to add matchers, the resulting 
API should cover a good amount of AST.

The reason why I suggested to pass the enum to the matcher is simply because it 
is less code duplication, less work, and more reliable code (since there will 
be only one matcher to review, test, and maintain, instead of combinations of 
matchers).

Another reason to not use an inner matcher here is the peculiar semantics of 
this function -- it does not evaluate the matcher, and it does not accept a 
matcher expression of any shape.



Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+

lebedev.ri wrote:
> gribozavr wrote:
> > lebedev.ri wrote:
> > > gribozavr wrote:
> > > > I'm not sure if breaking out the source code into the "SourceX" 
> > > > variables improves readability.  WDYT about inlining the code into the 
> > > > EXPECT_TRUE code like in other tests in this file?
> > > > 
> > > > If you want to break it out, I'd suggest to drop "`void x() {`" down to 
> > > > the next line, so that all code lines start at the same column.
> > > > I'm not sure if breaking out the source code into the "SourceX" 
> > > > variables improves readability
> > > 
> > > It's not about readability. Inlining will break the build, rC354201.
> > Other tests in this file use string concatenation, see 
> > `TEST(DeclarationMatcher, MatchHasRecursiveAllOf)` for example.
> I'm sorry, but i fail to see how that is relevant?
> I'm using multiline raw string literals, and inlining it will break the 
> build, like i linked.
> You are pointing at the code that is not using multiline raw string literals.
> You only suggested inlining, not switching away from multiline raw string 
> literals, i believe?
> 
> Not using multiline raw string literals looked even worse, because then you 
> need to manually add "\n"
> You only suggested inlining, not switching away from multiline raw string 
> literals, i believe?

I am now suggesting to switch away from raw string literals.

> Not using multiline raw string literals looked even worse, because then you 
> need to manually add "\n"

I believe that adding "\n" manually is better than having lots of 
similarly-named SourceX variables, which can easily cause copy-paste mistakes 
(define a SourceX variable, use SourceY in the EXPECT_TRUE line).

However, this is a minor point, up to you.  I only wanted to explain my 
reasoning why I prefer inline code snippets.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked 3 inline comments as done.
lebedev.ri added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

gribozavr wrote:
> lebedev.ri wrote:
> > gribozavr wrote:
> > > lebedev.ri wrote:
> > > > gribozavr wrote:
> > > > > Why not make `isAllowedToContainClause` take an `OpenMPClauseKind` 
> > > > > enum value?
> > > > > 
> > > > > I don't see right now advantages for taking a matcher.  (For example, 
> > > > > it can't be a more complex matcher with inner matchers, it can't be a 
> > > > > disjunction of matchers etc.)
> > > > I don't feel like it, it's uglier.
> > > > The matcher is documented, `OpenMPClauseKind` is not documented.
> > > > Also, how will passing some random enum work with e.g. clang-query?
> > > > 
> > > There are dozens of clauses in `OpenMPClauseKind`.  We would have to 
> > > replicate them all as matchers to provide a useful API.
> > > 
> > > > Also, how will passing some random enum work with e.g. clang-query?
> > > 
> > > See `llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h`.
> > True. Also, but there's dosens of Stmt types, and there is no overload that 
> > takes `StmtClass` enum.
> For Stmts, we do have dozens of individual matchers for them.
> 
> The point of your work is to add ASTMatchers for OpenMP, right?  However, if 
> there are no matchers for a reasonable amount of AST surface, it is as good 
> as if the matchers are not there, because prospective users won't be able to 
> use them.
> 
> I don't particularly care how exactly this is achieved, through individual 
> matchers or through a matcher that takes an enum.  However, I want to make 
> sure that if you're going through all this trouble to add matchers, the 
> resulting API should cover a good amount of AST.
> 
> The reason why I suggested to pass the enum to the matcher is simply because 
> it is less code duplication, less work, and more reliable code (since there 
> will be only one matcher to review, test, and maintain, instead of 
> combinations of matchers).
> 
> Another reason to not use an inner matcher here is the peculiar semantics of 
> this function -- it does not evaluate the matcher, and it does not accept a 
> matcher expression of any shape.
> The point of your work is to add ASTMatchers for OpenMP, right?

Absolutely not.
D57113 + D59466 is the one and only point, to address the bugs i have 
personally encountered.
The whole reason why i have started off with NOT adding these matchers to the 
`ASTMatchers.h`,
but keeping them at least initially internal to the checks was to avoid all 
this bikeshedding.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/AST/Stmt.h:102
+/// This bit is set only for the Stmt's that are the structured-block
+/// of OpeMP [non-standalone] executable directives.
+/// I.e. those returned by OMPExecutableDirective::getStructuredBlock().

"non-standalone OpenMP executable directives"



Comment at: test/PCH/stmt-openmp_structured_block-bit.cpp:8
+
+// expected-no-diagnostics
+

Maybe try imitating a more recently written test, like 
`llvm/tools/clang/test/PCH/cxx2a-compare.cpp` ?  It has both header and user 
written in the same file, for improved readability.



Comment at: test/PCH/stmt-openmp_structured_block-bit.h:2
+// Test this without pch.
+// RUN: %clang_cc1 -include %S/cxx_exprs.h -std=c++11 -fsyntax-only -verify %s 
-ast-dump | FileCheck %s
+

Is cxx_exprs.h needed?

I don't think `-std=c++11` is needed either.

Same in RUN lines below.

Maybe try imitating a more recently written test, like 
`llvm/tools/clang/test/PCH/cxx2a-compare.cpp` ?



Comment at: test/PCH/stmt-openmp_structured_block-bit.h:5
+// Test with pch. Use '-ast-dump' to force deserialization of function bodies.
+// RUN: %clang_cc1 -x c++-header -std=c++11 -emit-pch -o %t %S/cxx_exprs.h
+// RUN: %clang_cc1 -std=c++11 -include-pch %t -fsyntax-only -verify %s 
-ast-dump-all | FileCheck %s

The point of PCH tests is to test the serialization of ASTs into PCH.  In this 
line the cxx_exprs.h header is being initialized, it does not contain any 
OpenMP constructs, so I'm not sure what this test is supposed to test.



Comment at: test/PCH/stmt-openmp_structured_block-bit.h:6
+// RUN: %clang_cc1 -x c++-header -std=c++11 -emit-pch -o %t %S/cxx_exprs.h
+// RUN: %clang_cc1 -std=c++11 -include-pch %t -fsyntax-only -verify %s 
-ast-dump-all | FileCheck %s
+

I don't see any CHECK lines for FileCheck.  (FileCheck fails the test if there 
are no CHECK lines.)



Comment at: unittests/AST/OMPStructuredBlockTest.cpp:9
+//
+// Fine-grained tests for IsOMPStructuredBlock bit of Stmt's.
+//

"of Stmt"



Comment at: unittests/AST/OMPStructuredBlockTest.cpp:27
+
+AST_MATCHER(Stmt, isOMPStructuredBlock) { return Node.isOMPStructuredBlock(); }
+

Eventually you'll move it to `ASTMatchers.h`, right?



Comment at: unittests/AST/OMPStructuredBlockTest.cpp:41
+
+StatementMatcher OMPStandaloneDirectivekMatcher() {
+  return stmt(ompExecutableDirective(isStandaloneDirective())).bind("id");

extra "k" before "Matcher"



Comment at: unittests/AST/PrintTest.h:15
+
+using namespace clang;
+using namespace ast_matchers;

Please don't write `using namespace` in headers.  Instead, put the code below 
into the `clang` namespace.



Comment at: unittests/AST/PrintTest.h:19
+
+namespace {
+

No need for this, make `PrintStmt()` static.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214



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

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.



> I don't think risk is what matters - in the end it's about cost, and cost is 
> a very technical reason

I'm really sorry I'm not trying to be difficult its just over the years I keep 
seeing this being said in reviews? but what is the cost? I assume you don't 
mean a financial cost? could you define cost as a problem so we can address it? 
what would it take to make `cost` not an issue?


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

https://reviews.llvm.org/D55170



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

lebedev.ri wrote:
> gribozavr wrote:
> > lebedev.ri wrote:
> > > gribozavr wrote:
> > > > lebedev.ri wrote:
> > > > > gribozavr wrote:
> > > > > > Why not make `isAllowedToContainClause` take an `OpenMPClauseKind` 
> > > > > > enum value?
> > > > > > 
> > > > > > I don't see right now advantages for taking a matcher.  (For 
> > > > > > example, it can't be a more complex matcher with inner matchers, it 
> > > > > > can't be a disjunction of matchers etc.)
> > > > > I don't feel like it, it's uglier.
> > > > > The matcher is documented, `OpenMPClauseKind` is not documented.
> > > > > Also, how will passing some random enum work with e.g. clang-query?
> > > > > 
> > > > There are dozens of clauses in `OpenMPClauseKind`.  We would have to 
> > > > replicate them all as matchers to provide a useful API.
> > > > 
> > > > > Also, how will passing some random enum work with e.g. clang-query?
> > > > 
> > > > See `llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h`.
> > > True. Also, but there's dosens of Stmt types, and there is no overload 
> > > that takes `StmtClass` enum.
> > For Stmts, we do have dozens of individual matchers for them.
> > 
> > The point of your work is to add ASTMatchers for OpenMP, right?  However, 
> > if there are no matchers for a reasonable amount of AST surface, it is as 
> > good as if the matchers are not there, because prospective users won't be 
> > able to use them.
> > 
> > I don't particularly care how exactly this is achieved, through individual 
> > matchers or through a matcher that takes an enum.  However, I want to make 
> > sure that if you're going through all this trouble to add matchers, the 
> > resulting API should cover a good amount of AST.
> > 
> > The reason why I suggested to pass the enum to the matcher is simply 
> > because it is less code duplication, less work, and more reliable code 
> > (since there will be only one matcher to review, test, and maintain, 
> > instead of combinations of matchers).
> > 
> > Another reason to not use an inner matcher here is the peculiar semantics 
> > of this function -- it does not evaluate the matcher, and it does not 
> > accept a matcher expression of any shape.
> > The point of your work is to add ASTMatchers for OpenMP, right?
> 
> Absolutely not.
> D57113 + D59466 is the one and only point, to address the bugs i have 
> personally encountered.
> The whole reason why i have started off with NOT adding these matchers to the 
> `ASTMatchers.h`,
> but keeping them at least initially internal to the checks was to avoid all 
> this bikeshedding.
However, I do care about the AST matchers being usable by other clients.

I also care about the API following existing patterns:

> Another reason to not use an inner matcher here is the peculiar semantics of 
> this function -- it does not evaluate the matcher, and it does not accept a 
> matcher expression of any shape.




Repository:
  rC Clang

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

https://reviews.llvm.org/D57112



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

[PATCH] D59415: Do not resolve directory junctions for `-fdiagnostics-absolute-paths` on Windows.

[Igor Kudrin via Phabricator via cfe-commits]

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



Comment at: test/Frontend/absolute-paths-windows.test:4
+// RUN: mkdir -p %t.dir\real
+// RUN: cmd /c mklink /j %t.dir\junc %t.dir\real
+// RUN: echo "wrong code" > %t.dir\real\foo.cpp

hans wrote:
> This requires (a recent version of) Win 10 or later to run without running as 
> Admin, right?
As far as I know, creating a directory junction, in contrast to a symbolic 
link, does not require escalated privileges. But, honestly, I do not have any 
old system to validate that.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59415



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

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6400
+/// \endcode
+AST_MATCHER_P(OMPExecutableDirective, hasClause, internal::Matcher,
+  InnerMatcher) {

gribozavr wrote:
> Looking at other similar matches, they usually follow the naming pattern 
> `hasAny~`, WDYT?
> 
> (`hasAnyDeclaration`, `hasAnyConstructorInitializer` etc.)
Yeah, I think this would be a good change to make. Good catch!



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6433
+  return Node.getDefaultKind() == OMPC_DEFAULT_none;
+}
+

gribozavr wrote:
> Also add `isSharedKind`?  It is weird that we have one but not the other.
Given that there's only two options, it's not strictly required to have them 
both. But I don't see a reason not to add it, either.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456
+  // Note that we have recieved a *matcher* for the clause, not the
+  // OpenMPClauseKind. We now need to extract the 'return' type of said 
matcher,
+  // and convert it to the OpenMPClauseKind, so we can finally use that.

gribozavr wrote:
> lebedev.ri wrote:
> > gribozavr wrote:
> > > lebedev.ri wrote:
> > > > gribozavr wrote:
> > > > > lebedev.ri wrote:
> > > > > > gribozavr wrote:
> > > > > > > Why not make `isAllowedToContainClause` take an 
> > > > > > > `OpenMPClauseKind` enum value?
> > > > > > > 
> > > > > > > I don't see right now advantages for taking a matcher.  (For 
> > > > > > > example, it can't be a more complex matcher with inner matchers, 
> > > > > > > it can't be a disjunction of matchers etc.)
> > > > > > I don't feel like it, it's uglier.
> > > > > > The matcher is documented, `OpenMPClauseKind` is not documented.
> > > > > > Also, how will passing some random enum work with e.g. clang-query?
> > > > > > 
> > > > > There are dozens of clauses in `OpenMPClauseKind`.  We would have to 
> > > > > replicate them all as matchers to provide a useful API.
> > > > > 
> > > > > > Also, how will passing some random enum work with e.g. clang-query?
> > > > > 
> > > > > See `llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h`.
> > > > True. Also, but there's dosens of Stmt types, and there is no overload 
> > > > that takes `StmtClass` enum.
> > > For Stmts, we do have dozens of individual matchers for them.
> > > 
> > > The point of your work is to add ASTMatchers for OpenMP, right?  However, 
> > > if there are no matchers for a reasonable amount of AST surface, it is as 
> > > good as if the matchers are not there, because prospective users won't be 
> > > able to use them.
> > > 
> > > I don't particularly care how exactly this is achieved, through 
> > > individual matchers or through a matcher that takes an enum.  However, I 
> > > want to make sure that if you're going through all this trouble to add 
> > > matchers, the resulting API should cover a good amount of AST.
> > > 
> > > The reason why I suggested to pass the enum to the matcher is simply 
> > > because it is less code duplication, less work, and more reliable code 
> > > (since there will be only one matcher to review, test, and maintain, 
> > > instead of combinations of matchers).
> > > 
> > > Another reason to not use an inner matcher here is the peculiar semantics 
> > > of this function -- it does not evaluate the matcher, and it does not 
> > > accept a matcher expression of any shape.
> > > The point of your work is to add ASTMatchers for OpenMP, right?
> > 
> > Absolutely not.
> > D57113 + D59466 is the one and only point, to address the bugs i have 
> > personally encountered.
> > The whole reason why i have started off with NOT adding these matchers to 
> > the `ASTMatchers.h`,
> > but keeping them at least initially internal to the checks was to avoid all 
> > this bikeshedding.
> However, I do care about the AST matchers being usable by other clients.
> 
> I also care about the API following existing patterns:
> 
> > Another reason to not use an inner matcher here is the peculiar semantics 
> > of this function -- it does not evaluate the matcher, and it does not 
> > accept a matcher expression of any shape.
> 
> 
>> Also, how will passing some random enum work with e.g. clang-query?
> See llvm/tools/clang/lib/ASTMatchers/Dynamic/Marshallers.h.

That doesn't mean it works super well, though. String literals more easily 
contain silent typos, don't have autocomplete support, etc. I can definitely 
sympathize with not wanting to use an enum here.

However, I see that there are 50+ enumerations in this list -- that seems like 
too many matchers to want to expose. I think an enum will be the better, more 
maintainable option. The current approach won't scale well.



Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:2283
+})";
+  EXPECT_TRUE(notMatchesWithOpenMP(Source0, Matcher));
+

gribozavr wrote:
> lebedev.ri wrote:
> > gribozav

[PATCH] D59485: [ASTImporter] Allow adding a shim to the ASTImporter

[Raphael Isemann via Phabricator via cfe-commits]

teemperor created this revision.
teemperor added reviewers: martong, balazske, a.sidorin, shafik.
Herald added subscribers: cfe-commits, rnkovacs.
Herald added a project: clang.

We are currently implementing support in LLDB that reconstructs the STL 
templates from
the target program in the expression evaluator. This reconstruction happens 
during the
import process from our debug info AST into the expression evaluation AST[1], 
which means
we need a way to intercept the ASTImporter import process in some controlled 
fashion.

This patch adds a shim to the ASTImporter that allows us to intercept any 
Import calls an
ASTImporter may make (either externally or while importing internally). We then 
provide
a shim in LLDB that does do our template reconstruction.

The shim is on purpose not reusing some ASTImporter functionalities like the 
map of already
imported decls because we want to keep it flexible. If a Shim wants to use this 
functionality
it needs to update/check the map itself.

[1] The reason for this is that we can't reliable import templates with the 
ASTImporter, so
actually reconstructing the template in the debug info AST and then importing 
it doesn't work.
It would also be much slower.


Repository:
  rC Clang

https://reviews.llvm.org/D59485

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -316,11 +316,13 @@
 std::unique_ptr Unit;
 TranslationUnitDecl *TUDecl = nullptr;
 std::unique_ptr Importer;
-TU(StringRef Code, StringRef FileName, ArgVector Args)
+ImportShim *Shim;
+TU(StringRef Code, StringRef FileName, ArgVector Args,
+   ImportShim *Shim = nullptr)
 : Code(Code), FileName(FileName),
   Unit(tooling::buildASTFromCodeWithArgs(this->Code, Args,
  this->FileName)),
-  TUDecl(Unit->getASTContext().getTranslationUnitDecl()) {
+  TUDecl(Unit->getASTContext().getTranslationUnitDecl()), Shim(Shim) {
   Unit->enableSourceFileDiagnostics();
 }
 
@@ -331,6 +333,7 @@
 new ASTImporter(ToAST->getASTContext(), ToAST->getFileManager(),
 Unit->getASTContext(), Unit->getFileManager(),
 false, &LookupTable));
+Importer->setShim(Shim);
   }
   assert(&ToAST->getASTContext() == &Importer->getToContext());
   createVirtualFileIfNeeded(ToAST, FileName, Code);
@@ -401,11 +404,12 @@
   // Must not be called more than once within the same test.
   std::tuple
   getImportedDecl(StringRef FromSrcCode, Language FromLang, StringRef ToSrcCode,
-  Language ToLang, StringRef Identifier = DeclToImportID) {
+  Language ToLang, StringRef Identifier = DeclToImportID,
+  ImportShim *Shim = nullptr) {
 ArgVector FromArgs = getArgVectorForLanguage(FromLang),
   ToArgs = getArgVectorForLanguage(ToLang);
 
-FromTUs.emplace_back(FromSrcCode, InputFileName, FromArgs);
+FromTUs.emplace_back(FromSrcCode, InputFileName, FromArgs, Shim);
 TU &FromTU = FromTUs.back();
 
 assert(!ToAST);
@@ -455,6 +459,12 @@
 return ToAST->getASTContext().getTranslationUnitDecl();
   }
 
+  ASTImporter &getImporter(Decl *From, Language ToLang) {
+lazyInitToAST(ToLang, "", OutputFileName);
+TU *FromTU = findFromTU(From);
+return *FromTU->Importer;
+  }
+
   // Import the given Decl into the ToCtx.
   // May be called several times in a given test.
   // The different instances of the param From may have different ASTContext.
@@ -544,6 +554,81 @@
   EXPECT_THAT(RedeclsD1, ::testing::ContainerEq(RedeclsD2));
 }
 
+namespace {
+class RedirectShim : public ImportShim {
+  llvm::Optional Import(ASTImporter &Importer, Decl *FromD) override {
+auto *ND = dyn_cast(FromD);
+if (!ND)
+  return {};
+if (ND->getName() != "shouldNotBeImported")
+  return {};
+for (Decl *D : Importer.getToContext().getTranslationUnitDecl()->decls())
+  if (auto ND = dyn_cast(D))
+if (ND->getName() == "realDecl")
+  return ND;
+return {};
+  }
+};
+} // namespace
+
+struct ShimTestCase : ASTImporterOptionSpecificTestBase {};
+
+// Test that the shim can intercept an import call.
+TEST_P(ShimTestCase, InterceptImport) {
+  RedirectShim Shim;
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl("class shouldNotBeImported {};",
+   Lang_CXX, "class realDecl {};", Lang_CXX,
+   "shouldNotBeImported", &Shim);
+  auto *Imported = cast(To);
+  EXPECT_EQ(Imported->getQualifiedNameAsString(), "realDecl");
+
+  // Make sure the Shim prevented the importing of the decl.
+  auto *ToTU = Imported->ge

[PATCH] D59486: [OpenCL] Improve testing of default header in C++ mode

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added a reviewer: bader.
Herald added subscribers: ebevhan, yaxunl.

Moved testing of the default header into the Driver test and also added a test 
line for C++ mode in printf test.

Follow up from https://reviews.llvm.org/D59219


https://reviews.llvm.org/D59486

Files:
  test/Driver/include-default-header.cl
  test/SemaOpenCL/builtin.cl
  test/SemaOpenCL/extensions.cl


Index: test/SemaOpenCL/extensions.cl
===
--- test/SemaOpenCL/extensions.cl
+++ test/SemaOpenCL/extensions.cl
@@ -28,7 +28,7 @@
 // enabled by default with -cl-std=CL2.0).
 //
 // RUN: %clang_cc1 %s -triple amdgcn-unknown-unknown -verify -pedantic 
-fsyntax-only -cl-std=CL2.0 -finclude-default-header
-// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic 
-fsyntax-only -cl-std=c++ -finclude-default-header
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic 
-fsyntax-only -cl-std=c++
 
 #ifdef _OPENCL_H_
 // expected-no-diagnostics
Index: test/SemaOpenCL/builtin.cl
===
--- test/SemaOpenCL/builtin.cl
+++ test/SemaOpenCL/builtin.cl
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL1.2
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=c++
 
 // expected-no-diagnostics
 
Index: test/Driver/include-default-header.cl
===
--- test/Driver/include-default-header.cl
+++ test/Driver/include-default-header.cl
@@ -1,5 +1,7 @@
-// RUN: %clang -save-temps -x cl -Xclang -cl-std=CL2.0 -Xclang 
-finclude-default-header -emit-llvm -S -### %s
-// CHECK-NOT: finclude-default-header
+// RUN: %clang -save-temps -x cl -Xclang -cl-std=CL2.0 -Xclang 
-finclude-default-header -emit-llvm -S -### %s 2>&1 | FileCheck %s
+// RUN: %clang -save-temps -x cl -Xclang -cl-std=c++ -Xclang 
-finclude-default-header -emit-llvm -S -### %s 2>&1 | FileCheck %s
+
+// CHECK-LABEL: finclude-default-header
 // Make sure we don't pass -finclude-default-header to any commands other than 
the driver.
 
 void test() {}


Index: test/SemaOpenCL/extensions.cl
===
--- test/SemaOpenCL/extensions.cl
+++ test/SemaOpenCL/extensions.cl
@@ -28,7 +28,7 @@
 // enabled by default with -cl-std=CL2.0).
 //
 // RUN: %clang_cc1 %s -triple amdgcn-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=CL2.0 -finclude-default-header
-// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=c++ -finclude-default-header
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=c++
 
 #ifdef _OPENCL_H_
 // expected-no-diagnostics
Index: test/SemaOpenCL/builtin.cl
===
--- test/SemaOpenCL/builtin.cl
+++ test/SemaOpenCL/builtin.cl
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL1.2
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=c++
 
 // expected-no-diagnostics
 
Index: test/Driver/include-default-header.cl
===
--- test/Driver/include-default-header.cl
+++ test/Driver/include-default-header.cl
@@ -1,5 +1,7 @@
-// RUN: %clang -save-temps -x cl -Xclang -cl-std=CL2.0 -Xclang -finclude-default-header -emit-llvm -S -### %s
-// CHECK-NOT: finclude-default-header
+// RUN: %clang -save-temps -x cl -Xclang -cl-std=CL2.0 -Xclang -finclude-default-header -emit-llvm -S -### %s 2>&1 | FileCheck %s
+// RUN: %clang -save-temps -x cl -Xclang -cl-std=c++ -Xclang -finclude-default-header -emit-llvm -S -### %s 2>&1 | FileCheck %s
+
+// CHECK-LABEL: finclude-default-header
 // Make sure we don't pass -finclude-default-header to any commands other than the driver.
 
 void test() {}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59296: [pp-trace] Delete -ignore and add generalized -callbacks

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM aside from some small nits. Feel free to fix and commit without further 
review if you'd like.




Comment at: docs/ReleaseNotes.rst:141
+
+- New `-callbacks` option to filter PP callbacks.

How about: Added a new option, `-callbacks`, to filter preprocessor callbacks; 
replaces the `-ignore` option.



Comment at: pp-trace/PPTrace.cpp:168
+  for (StringRef Pattern : Patterns) {
+bool Enabled = !Pattern.consume_front("-");
+if (Expected Pat = GlobPattern::create(Pattern))

Do you want to trim the pattern, in case the user specifies a pattern with 
whitespace around the commas? e.g., `-callbacks 'foo, bar,baz'` (You should 
add a test for this as well.)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59296



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

[PATCH] D57571: [clang-tidy] A new OpenMP module

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

While I'm okay with this, I would mildly prefer it if landed with at least one 
check rather than entirely empty (IIRC, that's how we'd added new modules in 
the past).


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57571



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

[libunwind] r356365 - Creating release candidate final from release_800 branch

[Hans Wennborg via cfe-commits]

Author: hans
Date: Mon Mar 18 05:53:32 2019
New Revision: 356365

URL: http://llvm.org/viewvc/llvm-project?rev=356365&view=rev
Log:
Creating release candidate final from release_800 branch

Added:
libunwind/tags/RELEASE_800/final/
  - copied from r356364, libunwind/branches/release_80/

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

[libclc] r356365 - Creating release candidate final from release_800 branch

[Hans Wennborg via cfe-commits]

Author: hans
Date: Mon Mar 18 05:53:32 2019
New Revision: 356365

URL: http://llvm.org/viewvc/llvm-project?rev=356365&view=rev
Log:
Creating release candidate final from release_800 branch

Added:
libclc/tags/RELEASE_800/final/
  - copied from r356364, libclc/branches/release_80/

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

[PATCH] D59487: [Tooling] Add more scope specifiers until spelling is not ambiguous.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added reviewers: kadircet, gribozavr.
Herald added subscribers: cfe-commits, jdoerfert.
Herald added a project: clang.

Previously, when the renamed spelling is ambiguous, we simply use the
full-qualfied name (with leading "::"). This patch makes it try adding
additional specifiers one at a time until name is no longer ambiguous,
which allows us to find better disambuguated spelling.


Repository:
  rC Clang

https://reviews.llvm.org/D59487

Files:
  lib/Tooling/Core/Lookup.cpp
  unittests/Tooling/LookupTest.cpp

Index: unittests/Tooling/LookupTest.cpp
===
--- unittests/Tooling/LookupTest.cpp
+++ unittests/Tooling/LookupTest.cpp
@@ -129,20 +129,38 @@
 
   // If the shortest name is ambiguous, we need to add more qualifiers.
   Visitor.OnCall = [&](CallExpr *Expr) {
-EXPECT_EQ("::a::y::bar", replaceCallExpr(Expr, "::a::y::bar"));
+EXPECT_EQ("a::y::bar", replaceCallExpr(Expr, "::a::y::bar"));
   };
   Visitor.runOver(R"(
 namespace a {
-namespace b {
-namespace x { void foo() {} }
-namespace y { void foo() {} }
-}
+ namespace b {
+  namespace x { void foo() {} }
+  namespace y { void foo() {} }
+ }
 }
 
 namespace a {
-namespace b {
-void f() { x::foo(); }
+ namespace b {
+  void f() { x::foo(); }
+ }
+})");
+
+  Visitor.OnCall = [&](CallExpr *Expr) {
+// y::bar would be ambiguous due to "a::b::y".
+EXPECT_EQ("::y::bar", replaceCallExpr(Expr, "::y::bar"));
+  };
+  Visitor.runOver(R"(
+namespace a {
+ namespace b {
+  void foo() {}
+  namespace y { }
+ }
 }
+
+namespace a {
+ namespace b {
+  void f() { foo(); }
+ }
 })");
 
   Visitor.OnCall = [&](CallExpr *Expr) {
Index: lib/Tooling/Core/Lookup.cpp
===
--- lib/Tooling/Core/Lookup.cpp
+++ lib/Tooling/Core/Lookup.cpp
@@ -14,6 +14,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclarationName.h"
+#include "llvm/ADT/SmallVector.h"
 using namespace clang;
 using namespace clang::tooling;
 
@@ -114,35 +115,60 @@
   return false;
 }
 
-// Returns true if spelling symbol \p QName as \p Spelling in \p UseContext is
-// ambiguous. For example, if QName is "::y::bar" and the spelling is "y::bar"
-// in `UseContext` "a" that contains a nested namespace "a::y", then "y::bar"
-// can be resolved to ::a::y::bar, which can cause compile error.
+// Adds more scope specifier to the spelled name until the spelling is not
+// ambiguous. A spelling is ambiguous if the resolution of the symbol is
+// ambiguous. For example, if QName is "::y::bar", the spelling is "y::bar", and
+// context contains a nested namespace "a::y", then "y::bar" can be resolved to
+// ::a::y::bar in the context, which can cause compile error.
 // FIXME: consider using namespaces.
-static bool isAmbiguousNameInScope(StringRef Spelling, StringRef QName,
-   const DeclContext &UseContext) {
+static std::string disambiguateSpellingInScope(StringRef Spelling,
+   StringRef QName,
+   const DeclContext &UseContext) {
   assert(QName.startswith("::"));
+  assert(QName.endswith(Spelling));
   if (Spelling.startswith("::"))
-return false;
+return Spelling;
 
-  // Lookup the first component of Spelling in all enclosing namespaces and
-  // check if there is any existing symbols with the same name but in different
-  // scope.
-  StringRef Head = Spelling.split("::").first;
+  auto UnspelledSpecifier = QName.drop_back(Spelling.size());
+  llvm::SmallVector UnspelledScopes;
+  UnspelledSpecifier.split(UnspelledScopes, "::", /*MaxSplit=*/-1,
+   /*KeepEmpty=*/false);
 
-  llvm::SmallVector UseNamespaces =
+  llvm::SmallVector EnclosingNamespaces =
   getAllNamedNamespaces(&UseContext);
   auto &AST = UseContext.getParentASTContext();
   StringRef TrimmedQName = QName.substr(2);
-  for (const auto *NS : UseNamespaces) {
-auto LookupRes = NS->lookup(DeclarationName(&AST.Idents.get(Head)));
-if (!LookupRes.empty()) {
-  for (const NamedDecl *Res : LookupRes)
-if (!TrimmedQName.startswith(Res->getQualifiedNameAsString()))
-  return true;
+
+  auto IsAmbiguousSpelling = [&EnclosingNamespaces, &AST, &TrimmedQName](
+ const llvm::StringRef CurSpelling) {
+if (CurSpelling.startswith("::"))
+  return false;
+// Lookup the first component of Spelling in all enclosing namespaces
+// and check if there is any existing symbols with the same name but in
+// different scope.
+StringRef Head = CurSpelling.split("::").first;
+for (const auto *NS : EnclosingNamespaces) {
+  auto LookupRes = NS->lookup(DeclarationName(&AST.Idents.get(Head)));
+  if (!LookupRe

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D59467#1432675 , @Tyker wrote:

> if likely/unlikely can be applied to any statement or label what should be 
> generated when it is applied to a statement that don't have any conditional 
> branch ? should it be ignored  ?


That's the semantics we need to figure out. The standard talks about paths of 
execution including the attribute 
(http://eel.is/c++draft/dcl.attr.likelihood#2.sentence-1), so I imagine this 
means we should be supporting code like:

  void func(bool some_condition) {
if (some_condition) {
  [[likely]];
} else {
  [[unlikely]];
}
  }

While that may seem a bit silly (because you can add the attribute to the 
compound-statement), there are situations where you cannot do that, but the 
feature still has use. Consider:

  void func() {
try {
  some_operation();
} catch (some_exception &e) {
  [[likely]];
  recover_without_killing_everyone_on_the_elevator();
}
  }

In some safety-critical situations, you may not care how fast the normal 
operation works but still need the failure mode to be highly optimized to avoid 
loss of life.

We have other fun questions to figure out as well, such as whether to diagnose 
this code and what the semantics should be:

  void func(bool foo) {
if (foo) {
  [[likely]];
  [[unlikely]];
}
  }
  
  void func2(bool foo) {
if (foo) {
  [[likely]];
} else {
  [[likely]];
}
  }

We don't have to solve it all up front, but having an idea as to what the 
backend is going to do with this information may help us design the appropriate 
frontend behavior where the standard gives us this much latitude. Do you know 
how this is expected to hook in to LLVM?


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

https://reviews.llvm.org/D59467



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

[PATCH] D59485: [ASTImporter] Allow adding a shim to the ASTImporter

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

balazske added a comment.

I do not know if "Shim" is the correct name for this, maybe "Strategy" is 
better?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59485



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

[clang-tools-extra] r356366 - [pp-trace] Delete -ignore and add a new option -callbacks

[Fangrui Song via cfe-commits]

Author: maskray
Date: Mon Mar 18 06:30:17 2019
New Revision: 356366

URL: http://llvm.org/viewvc/llvm-project?rev=356366&view=rev
Log:
[pp-trace] Delete -ignore and add a new option -callbacks

Summary:
-ignore specifies a list of PP callbacks to ignore. It cannot express a
whitelist, which may be more useful than a blacklist.
Add a new option -callbacks to replace it.

-ignore= (default) => -callbacks='*' (default)
-ignore=FileChanged,FileSkipped => -callbacks='*,-FileChanged,-FileSkipped'

-callbacks='Macro*' : print only MacroDefined,MacroExpands,MacroUndefined,...

Reviewers: juliehockett, aaron.ballman, alexfh, ioeric

Reviewed By: aaron.ballman

Subscribers: nemanjai, kbarton, jsji, cfe-commits

Tags: #clang

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

Added:
clang-tools-extra/trunk/test/pp-trace/pp-trace-filter.cpp
Modified:
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/pp-trace.rst
clang-tools-extra/trunk/pp-trace/PPCallbacksTracker.cpp
clang-tools-extra/trunk/pp-trace/PPCallbacksTracker.h
clang-tools-extra/trunk/pp-trace/PPTrace.cpp
clang-tools-extra/trunk/test/pp-trace/pp-trace-conditional.cpp
clang-tools-extra/trunk/test/pp-trace/pp-trace-ident.cpp
clang-tools-extra/trunk/test/pp-trace/pp-trace-macro.cpp
clang-tools-extra/trunk/test/pp-trace/pp-trace-modules.cpp
clang-tools-extra/trunk/test/pp-trace/pp-trace-pragma-general.cpp
clang-tools-extra/trunk/test/pp-trace/pp-trace-pragma-ms.cpp
clang-tools-extra/trunk/test/pp-trace/pp-trace-pragma-opencl.cpp

Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=356366&r1=356365&r2=356366&view=diff
==
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Mon Mar 18 06:30:17 2019
@@ -132,3 +132,9 @@ Improvements to modularize
 --
 
 The improvements are...
+
+Improvements to pp-trace
+
+
+- Added a new option `-callbacks` to filter preprocessor callbacks. It replaces
+  the `-ignore` option.

Modified: clang-tools-extra/trunk/docs/pp-trace.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/pp-trace.rst?rev=356366&r1=356365&r2=356366&view=diff
==
--- clang-tools-extra/trunk/docs/pp-trace.rst (original)
+++ clang-tools-extra/trunk/docs/pp-trace.rst Mon Mar 18 06:30:17 2019
@@ -40,12 +40,12 @@ which must follow the .
 Command Line Options
 
 
-.. option:: -ignore 
+.. option:: -callbacks 
 
-  This option specifies a comma-separated list of names of callbacks
-  that shouldn't be traced. It can be used to eliminate unwanted
-  trace output. The callback names are the name of the actual
-  callback function names in the PPCallbacks class:
+  This option specifies a comma-separated list of globs describing the list of
+  callbacks that should be traced. Globs are processed in order of appearance.
+  Positive globs add matched callbacks to the set, netative globs (those with
+  the '-' prefix) remove callacks from the set.
 
   * FileChanged
   * FileSkipped

Modified: clang-tools-extra/trunk/pp-trace/PPCallbacksTracker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/pp-trace/PPCallbacksTracker.cpp?rev=356366&r1=356365&r2=356366&view=diff
==
--- clang-tools-extra/trunk/pp-trace/PPCallbacksTracker.cpp (original)
+++ clang-tools-extra/trunk/pp-trace/PPCallbacksTracker.cpp Mon Mar 18 06:30:17 
2019
@@ -88,10 +88,10 @@ static const char *const MappingStrings[
 
 // PPCallbacksTracker functions.
 
-PPCallbacksTracker::PPCallbacksTracker(llvm::SmallSet &Ignore,
+PPCallbacksTracker::PPCallbacksTracker(const FilterType &Filters,
std::vector 
&CallbackCalls,
clang::Preprocessor &PP)
-: CallbackCalls(CallbackCalls), Ignore(Ignore), PP(PP) {}
+: CallbackCalls(CallbackCalls), Filters(Filters), PP(PP) {}
 
 PPCallbacksTracker::~PPCallbacksTracker() {}
 
@@ -425,7 +425,14 @@ void PPCallbacksTracker::Endif(clang::So
 
 // Start a new callback.
 void PPCallbacksTracker::beginCallback(const char *Name) {
-  DisableTrace = Ignore.count(std::string(Name));
+  auto R = CallbackIsEnabled.try_emplace(Name, false);
+  if (R.second) {
+llvm::StringRef N(Name);
+for (const std::pair &Filter : Filters)
+  if (Filter.first.match(N))
+R.first->second = Filter.second;
+  }
+  DisableTrace = !R.first->second;
   if (DisableTrace)
 return;
   CallbackCalls.push_back(CallbackCall(Name));

Modified: clang-tools-extra/trunk/pp-trace/PPCallbacksTracker.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-ex

[PATCH] D59296: [pp-trace] Delete -ignore and add a new option -callbacks

[Fangrui Song via Phabricator via cfe-commits]

MaskRay updated this revision to Diff 191075.
MaskRay retitled this revision from "[pp-trace] Delete -ignore and add 
generalized -callbacks" to "[pp-trace] Delete -ignore and add a new option 
-callbacks".
MaskRay edited the summary of this revision.
MaskRay added a comment.

Add a test with spaces around patterns


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59296

Files:
  docs/ReleaseNotes.rst
  docs/pp-trace.rst
  pp-trace/PPCallbacksTracker.cpp
  pp-trace/PPCallbacksTracker.h
  pp-trace/PPTrace.cpp
  test/pp-trace/pp-trace-conditional.cpp
  test/pp-trace/pp-trace-filter.cpp
  test/pp-trace/pp-trace-ident.cpp
  test/pp-trace/pp-trace-macro.cpp
  test/pp-trace/pp-trace-modules.cpp
  test/pp-trace/pp-trace-pragma-general.cpp
  test/pp-trace/pp-trace-pragma-ms.cpp
  test/pp-trace/pp-trace-pragma-opencl.cpp

Index: test/pp-trace/pp-trace-pragma-opencl.cpp
===
--- test/pp-trace/pp-trace-pragma-opencl.cpp
+++ test/pp-trace/pp-trace-pragma-opencl.cpp
@@ -1,4 +1,4 @@
-// RUN: pp-trace -ignore FileChanged,MacroDefined %s -x cl | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s -x cl | FileCheck --strict-whitespace %s
 
 #pragma OPENCL EXTENSION all : disable
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : disable
Index: test/pp-trace/pp-trace-pragma-ms.cpp
===
--- test/pp-trace/pp-trace-pragma-ms.cpp
+++ test/pp-trace/pp-trace-pragma-ms.cpp
@@ -1,4 +1,4 @@
-// RUN: pp-trace -ignore FileChanged,MacroDefined %s -target x86_64-unknown-windows-msvc -fms-extensions -w | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s -target x86_64-unknown-windows-msvc -fms-extensions -w | FileCheck --strict-whitespace %s
 
 #pragma comment(compiler, "compiler comment")
 #pragma comment(exestr, "exestr comment")
Index: test/pp-trace/pp-trace-pragma-general.cpp
===
--- test/pp-trace/pp-trace-pragma-general.cpp
+++ test/pp-trace/pp-trace-pragma-general.cpp
@@ -1,4 +1,4 @@
-// RUN: pp-trace -ignore FileChanged,MacroDefined %s | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s | FileCheck --strict-whitespace %s
 
 #pragma clang diagnostic push
 #pragma clang diagnostic pop
Index: test/pp-trace/pp-trace-modules.cpp
===
--- test/pp-trace/pp-trace-modules.cpp
+++ test/pp-trace/pp-trace-modules.cpp
@@ -1,5 +1,5 @@
 // RUN: rm -rf %t
-// RUN: pp-trace -ignore FileChanged,MacroDefined %s -x objective-c++ -undef -target x86_64 -std=c++11 -fmodules -fcxx-modules -fmodules-cache-path=%t -I%S -I%S/Input | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s -x objective-c++ -undef -target x86_64 -std=c++11 -fmodules -fcxx-modules -fmodules-cache-path=%t -I%S -I%S/Input | FileCheck --strict-whitespace %s
 
 // CHECK: ---
 
Index: test/pp-trace/pp-trace-macro.cpp
===
--- test/pp-trace/pp-trace-macro.cpp
+++ test/pp-trace/pp-trace-macro.cpp
@@ -1,4 +1,4 @@
-// RUN: pp-trace -ignore FileChanged %s -undef -target x86_64 -std=c++11 | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged' %s -undef -target x86_64 -std=c++11 | FileCheck --strict-whitespace %s
 
 #define MACRO 1
 int i = MACRO;
Index: test/pp-trace/pp-trace-ident.cpp
===
--- test/pp-trace/pp-trace-ident.cpp
+++ test/pp-trace/pp-trace-ident.cpp
@@ -1,4 +1,4 @@
-// RUN: pp-trace -ignore FileChanged,MacroDefined %s -undef -target x86_64 -std=c++11 | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s -undef -target x86_64 -std=c++11 | FileCheck --strict-whitespace %s
 
 #ident "$Id$"
 
Index: test/pp-trace/pp-trace-filter.cpp
===
--- /dev/null
+++ test/pp-trace/pp-trace-filter.cpp
@@ -0,0 +1,17 @@
+// RUN: pp-trace -callbacks 'File*,Macro*,-MacroUndefined' %s | FileCheck %s
+// RUN: pp-trace -callbacks ' File* , Macro* , -MacroUndefined ' %s | FileCheck %s
+// RUN: not pp-trace -callbacks '[' %s 2>&1 | FileCheck --check-prefix=INVALID %s
+
+#define M 1
+int i = M;
+#undef M
+
+// CHECK:  ---
+// CHECK:  - Callback: FileChanged
+// CHECK:  - Callback: MacroDefined
+// CHECK:  - Callback: MacroExpands
+// CHECK-NOT:  - Callback: MacroUndefined
+// CHECK-NOT:  - Callback: EndOfMainFile
+// CHECK:  ...
+
+// INVALID: error: invalid glob pattern: [
Index: test/pp-trace/pp-trace-conditional.cpp
===
--- test/pp-trace/pp-trace-conditional

[PATCH] D59296: [pp-trace] Delete -ignore and add a new option -callbacks

[Fangrui Song via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL356366: [pp-trace] Delete -ignore and add a new option 
-callbacks (authored by MaskRay, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59296

Files:
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/pp-trace.rst
  clang-tools-extra/trunk/pp-trace/PPCallbacksTracker.cpp
  clang-tools-extra/trunk/pp-trace/PPCallbacksTracker.h
  clang-tools-extra/trunk/pp-trace/PPTrace.cpp
  clang-tools-extra/trunk/test/pp-trace/pp-trace-conditional.cpp
  clang-tools-extra/trunk/test/pp-trace/pp-trace-filter.cpp
  clang-tools-extra/trunk/test/pp-trace/pp-trace-ident.cpp
  clang-tools-extra/trunk/test/pp-trace/pp-trace-macro.cpp
  clang-tools-extra/trunk/test/pp-trace/pp-trace-modules.cpp
  clang-tools-extra/trunk/test/pp-trace/pp-trace-pragma-general.cpp
  clang-tools-extra/trunk/test/pp-trace/pp-trace-pragma-ms.cpp
  clang-tools-extra/trunk/test/pp-trace/pp-trace-pragma-opencl.cpp

Index: clang-tools-extra/trunk/test/pp-trace/pp-trace-pragma-opencl.cpp
===
--- clang-tools-extra/trunk/test/pp-trace/pp-trace-pragma-opencl.cpp
+++ clang-tools-extra/trunk/test/pp-trace/pp-trace-pragma-opencl.cpp
@@ -1,4 +1,4 @@
-// RUN: pp-trace -ignore FileChanged,MacroDefined %s -x cl | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s -x cl | FileCheck --strict-whitespace %s
 
 #pragma OPENCL EXTENSION all : disable
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : disable
Index: clang-tools-extra/trunk/test/pp-trace/pp-trace-filter.cpp
===
--- clang-tools-extra/trunk/test/pp-trace/pp-trace-filter.cpp
+++ clang-tools-extra/trunk/test/pp-trace/pp-trace-filter.cpp
@@ -0,0 +1,17 @@
+// RUN: pp-trace -callbacks 'File*,Macro*,-MacroUndefined' %s | FileCheck %s
+// RUN: pp-trace -callbacks ' File* , Macro* , -MacroUndefined ' %s | FileCheck %s
+// RUN: not pp-trace -callbacks '[' %s 2>&1 | FileCheck --check-prefix=INVALID %s
+
+#define M 1
+int i = M;
+#undef M
+
+// CHECK:  ---
+// CHECK:  - Callback: FileChanged
+// CHECK:  - Callback: MacroDefined
+// CHECK:  - Callback: MacroExpands
+// CHECK-NOT:  - Callback: MacroUndefined
+// CHECK-NOT:  - Callback: EndOfMainFile
+// CHECK:  ...
+
+// INVALID: error: invalid glob pattern: [
Index: clang-tools-extra/trunk/test/pp-trace/pp-trace-pragma-ms.cpp
===
--- clang-tools-extra/trunk/test/pp-trace/pp-trace-pragma-ms.cpp
+++ clang-tools-extra/trunk/test/pp-trace/pp-trace-pragma-ms.cpp
@@ -1,4 +1,4 @@
-// RUN: pp-trace -ignore FileChanged,MacroDefined %s -target x86_64-unknown-windows-msvc -fms-extensions -w | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s -target x86_64-unknown-windows-msvc -fms-extensions -w | FileCheck --strict-whitespace %s
 
 #pragma comment(compiler, "compiler comment")
 #pragma comment(exestr, "exestr comment")
Index: clang-tools-extra/trunk/test/pp-trace/pp-trace-ident.cpp
===
--- clang-tools-extra/trunk/test/pp-trace/pp-trace-ident.cpp
+++ clang-tools-extra/trunk/test/pp-trace/pp-trace-ident.cpp
@@ -1,4 +1,4 @@
-// RUN: pp-trace -ignore FileChanged,MacroDefined %s -undef -target x86_64 -std=c++11 | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s -undef -target x86_64 -std=c++11 | FileCheck --strict-whitespace %s
 
 #ident "$Id$"
 
Index: clang-tools-extra/trunk/test/pp-trace/pp-trace-conditional.cpp
===
--- clang-tools-extra/trunk/test/pp-trace/pp-trace-conditional.cpp
+++ clang-tools-extra/trunk/test/pp-trace/pp-trace-conditional.cpp
@@ -1,4 +1,4 @@
-// RUN: pp-trace -ignore FileChanged %s -undef -target x86_64 -std=c++11 | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged' %s -undef -target x86_64 -std=c++11 | FileCheck --strict-whitespace %s
 
 #if 1
 #endif
Index: clang-tools-extra/trunk/test/pp-trace/pp-trace-pragma-general.cpp
===
--- clang-tools-extra/trunk/test/pp-trace/pp-trace-pragma-general.cpp
+++ clang-tools-extra/trunk/test/pp-trace/pp-trace-pragma-general.cpp
@@ -1,4 +1,4 @@
-// RUN: pp-trace -ignore FileChanged,MacroDefined %s | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s | FileCheck --strict-whitespace %s
 
 #pragma clang diagnostic push
 #pragma clang diagnostic pop
Index: clang-tools-extra/trunk/test/pp-trace/pp-trace-macro.cpp
=

[PATCH] D59485: [ASTImporter] Allow adding a shim to the ASTImporter

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

balazske added a comment.

The replace of `Import` calls may work if the internal state of the 
`ASTImporter` is updated correctly from the 'shim' object (especially if not 
every Decl is handled by the shim). For this to work some of the state of 
`ASTImporter` that was intended to be internal must be made public, for example 
map of imported decls and lookup table. The shim may corrupt the `ASTImporter` 
state for example when the lookup table is not updated correctly. (Probably for 
the LLDB special use cases this is not a problem?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D59485



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

[PATCH] D45978: dllexport const variables must have external linkage.

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

In D45978#1431057 , @aaron.ballman 
wrote:

> LGTM!


Would you mind committing the changes please? Thanks.


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

https://reviews.llvm.org/D45978



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

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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Before i do address any further bike^W review notes in other
reviews in this path stack, i want to finish with this review.
If **this** change doesn't stick, rest is pointless waste of everyone's time.

@ABataev please do tell if there is any outstanding issues here.
I think i have addressed everything..


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214



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

[PATCH] D59486: [OpenCL] Improve testing of default header in C++ mode

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

I see seven OpenCL C tests using -finclude-default-header option and AFAIK, 
only test/Driver/include-default-header.cl doesn't parse it. Could you also 
update OpenCL C tests?
I think clang/test/Headers/opencl-c-header.cl and 
test/Driver/include-default-header.cl fully cover this feature. 
All other tests seems to use this option to simplify the test, but with 
additional cost on parsing this header.

grep -r include-default-header clang/test/

clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple 
spir-unknown-unknown -emit-llvm -o - -finclude-default-header -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash 
-ftime-report %s 2>&1 | FileCheck --check-prefix=CHECK --check-prefix=CHECK-MOD 
%s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple 
spir-unknown-unknown -emit-llvm -o - -finclude-default-header -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash 
-ftime-report %s 2>&1 | FileCheck --check-prefix=CHECK --check-prefix=CHECK-MOD 
%s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple 
spir-unknown-unknown -O0 -emit-llvm -o - -cl-std=CL2.0 -finclude-default-header 
-fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash 
-ftime-report %s 2>&1 | FileCheck --check-prefix=CHECK20 
--check-prefix=CHECK-MOD %s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple 
spir-unknown-unknown -O0 -emit-llvm -o - -cl-std=CL2.0 -finclude-default-header 
-fmodules -fimplicit-module-maps -fmodules-cache-path=%t -fdisable-module-hash 
-ftime-report %s 2>&1 | FileCheck --check-prefix=CHECK20 
--check-prefix=CHECK-MOD %s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple 
spir64-unknown-unknown -emit-llvm -o - -cl-std=CL1.2 -finclude-default-header 
-fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ftime-report %s 2>&1 
| FileCheck --check-prefix=CHECK --check-prefix=CHECK-MOD %s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple amdgcn--amdhsa 
-O0 -emit-llvm -o - -cl-std=CL2.0 -finclude-default-header -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t -ftime-report %s 2>&1 | 
FileCheck --check-prefix=CHECK20 --check-prefix=CHECK-MOD %s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple 
spir64-unknown-unknown -emit-llvm -o - -cl-std=CL1.2 -finclude-default-header 
-fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ftime-report %s 2>&1 
| FileCheck --check-prefix=CHECK --check-prefix=CHECK-MOD %s
clang/test/Headers/opencl-c-header.cl:// RUN: %clang_cc1 -triple amdgcn--amdhsa 
-O0 -emit-llvm -o - -cl-std=CL2.0 -finclude-default-header -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t -ftime-report %s 2>&1 | 
FileCheck --check-prefix=CHECK20 --check-prefix=CHECK-MOD %s
clang/test/Driver/include-default-header.cl:// RUN: %clang -save-temps -x cl 
-Xclang -cl-std=CL2.0 -Xclang -finclude-default-header -emit-llvm -S -### %s
clang/test/Driver/include-default-header.cl:// CHECK-NOT: 
finclude-default-header
clang/test/Driver/include-default-header.cl:// Make sure we don't pass 
-finclude-default-header to any commands other than the driver.
clang/test/SemaOpenCL/as_type.cl:// RUN: %clang_cc1 %s -emit-llvm -triple 
spir-unknown-unknown -finclude-default-header -o - -verify -fsyntax-only
clang/test/SemaOpenCL/printf-format-string-warnings.cl:// RUN: %clang_cc1 %s 
-verify -pedantic -fsyntax-only -cl-std=CL2.0 -finclude-default-header
clang/test/SemaOpenCL/extensions.cl:// Test with -finclude-default-header, 
which includes opencl-c.h. opencl-c.h
clang/test/SemaOpenCL/extensions.cl:// RUN: %clang_cc1 %s -triple 
amdgcn-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=CL2.0 
-finclude-default-header
clang/test/SemaOpenCL/extensions.cl:// RUN: %clang_cc1 %s -triple 
spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=c++ 
-finclude-default-header
clang/test/CodeGenOpenCL/builtins.cl:// RUN: %clang_cc1 %s 
-finclude-default-header -cl-std=CL2.0 -O0 -emit-llvm -o - -triple 
"spir-unknown-unknown" | FileCheck %s
clang/test/CodeGenOpenCL/size_t.cl:// RUN: %clang_cc1 %s -cl-std=CL2.0 
-finclude-default-header -emit-llvm -O0 -triple spir-unknown-unknown -o - | 
FileCheck --check-prefix=SZ32 %s
clang/test/CodeGenOpenCL/size_t.cl:// RUN: %clang_cc1 %s -cl-std=CL2.0 
-finclude-default-header -emit-llvm -O0 -triple spir64-unknown-unknown -o - | 
FileCheck --check-prefix=SZ64 --check-prefix=SZ64ONLY %s
clang/test/CodeGenOpenCL/size_t.cl:// RUN: %clang_cc1 %s -cl-std=CL2.0 
-finclude-default-header -emit-llvm -O0 -triple amdgcn -o - | FileCheck 
--check-prefix=SZ64 --check-prefix=AMDGCN %s
clang/test/CodeGenOpenCL/size_t.cl:// RUN: %clang_cc1 %s -cl-std=CL2.0 
-finclude-default-header -emit-llvm -O0 -triple amdgcn---opencl -o - | 
FileCheck --check-prefix=SZ64 --check-prefix=AMDGCN %s




Comment at: test/Driver/include-default-header.cl:2
+// RUN: %clang -save-temps -x cl -Xclang -cl-std=

[PATCH] D57108: [clang-tidy] diagnose possibiltiy to add 'noexcept' in modernize-use-noexcept

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

baloghadamsoftware added a comment.

Ping! Any news regarding this patch?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57108



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

[PATCH] D57855: [analyzer] Reimplement checker options

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

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

Aside from little comment it seems OK.




Comment at: lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:200
+#undef PACKAGE_OPTION
+#undef GET_PACKAGE_OPTIONS
 

Everywhere 1. package 2. checker, please!


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

https://reviews.llvm.org/D57855



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

[PATCH] D58367: [analyzer] NFC: Improve upon the concept of BugReporterVisitor.

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

baloghadamsoftware added a comment.

In D58367#1425922 , @Szelethus wrote:

> As I understand it, this solution could be used to entirely get rid of the 
> current bugreporter visitor structure (at least for checkers), right? The 
> discussion seems to conclude that this is just as general, far easier to 
> understand, far easier to implement, and is basically better in every regard 
> without an (//edit: significant//) hit to performance? Because if so, I'm 
> definitely against supporting two concurrent implementations of the same 
> functionality -- in fact, we should even just forbid checkers to add custom 
> visitors.


I am not sure we could get rid of all the checker-specific visitors, but most 
probably many of them. However, there are cases where we should find something 
"last" which is best done bottom-up.


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

https://reviews.llvm.org/D58367



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

[PATCH] D57108: [clang-tidy] diagnose possibiltiy to add 'noexcept' in modernize-use-noexcept

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D57108#1433087 , 
@baloghadamsoftware wrote:

> Ping! Any news regarding this patch?


Its on the todolist, but i currently have many things to do in my real life, so 
no time for clang-tidy development :(


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57108



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

[PATCH] D59492: [OpenCL] Allow variadic macros as Clang feature

[Anastasia Stulova via Phabricator via cfe-commits]

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

Better wording


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

https://reviews.llvm.org/D59492

Files:
  docs/UsersManual.rst
  include/clang/Basic/DiagnosticLexKinds.td
  lib/Lex/PPDirectives.cpp
  test/Preprocessor/macro_variadic.cl
  test/SemaOpenCL/func.cl


Index: test/SemaOpenCL/func.cl
===
--- test/SemaOpenCL/func.cl
+++ test/SemaOpenCL/func.cl
@@ -1,5 +1,10 @@
 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -triple 
spir-unknown-unknown
 
+//Variadic macro
+#define NO_VAR_FUNC(...)  5
+#define VAR_FUNC(...) func(__VA_ARGS__);
+#define VAR_PRINTF(str, ...) printf(str, __VA_ARGS__);
+
 // Variadic functions
 void vararg_f(int, ...);// expected-error {{invalid 
prototype, variadic arguments are not allowed in OpenCL}}
 void __vararg_f(int, ...);
@@ -33,4 +38,8 @@
 
   // just calling a function is correct
   foo(0);
+
+  NO_VAR_FUNC(1, 2, 3);
+  VAR_FUNC(1, 2, 3); //expected-error{{implicit declaration of function 'func' 
is invalid in OpenCL}}
+  VAR_PRINTF("%i", 1);
 }
Index: test/Preprocessor/macro_variadic.cl
===
--- test/Preprocessor/macro_variadic.cl
+++ /dev/null
@@ -1,3 +0,0 @@
-// RUN: %clang_cc1 -verify %s
-
-#define X(...) 1 // expected-error {{variadic macros not supported in OpenCL}}
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -2233,12 +2233,6 @@
  diag::warn_cxx98_compat_variadic_macro :
  diag::ext_variadic_macro);
 
-  // OpenCL v1.2 s6.9.e: variadic macros are not supported.
-  if (LangOpts.OpenCL) {
-Diag(Tok, diag::err_pp_opencl_variadic_macros);
-return true;
-  }
-
   // Lex the token after the identifier.
   LexUnexpandedToken(Tok);
   if (Tok.isNot(tok::r_paren)) {
Index: include/clang/Basic/DiagnosticLexKinds.td
===
--- include/clang/Basic/DiagnosticLexKinds.td
+++ include/clang/Basic/DiagnosticLexKinds.td
@@ -393,9 +393,6 @@
 def note_macro_here : Note<"macro %0 defined here">;
 def note_macro_expansion_here : Note<"expansion of macro %0 requested here">;
 
-def err_pp_opencl_variadic_macros :
-  Error<"variadic macros not supported in OpenCL">;
-
 def err_pp_invalid_directive : Error<"invalid preprocessing directive">;
 def err_pp_directive_required : Error<
   "%0 must be used within a preprocessing directive">;
Index: docs/UsersManual.rst
===
--- docs/UsersManual.rst
+++ docs/UsersManual.rst
@@ -2734,6 +2734,14 @@
   enqueue query functions from `section 6.13.17.5
   `_.
 
+Differences from various standard modes
+---
+
+All OpenCL standards:
+
+- Clang accepts variadic macros.
+
+
 .. _target_features:
 
 Target-Specific Features and Limitations


Index: test/SemaOpenCL/func.cl
===
--- test/SemaOpenCL/func.cl
+++ test/SemaOpenCL/func.cl
@@ -1,5 +1,10 @@
 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -triple spir-unknown-unknown
 
+//Variadic macro
+#define NO_VAR_FUNC(...)  5
+#define VAR_FUNC(...) func(__VA_ARGS__);
+#define VAR_PRINTF(str, ...) printf(str, __VA_ARGS__);
+
 // Variadic functions
 void vararg_f(int, ...);// expected-error {{invalid prototype, variadic arguments are not allowed in OpenCL}}
 void __vararg_f(int, ...);
@@ -33,4 +38,8 @@
 
   // just calling a function is correct
   foo(0);
+
+  NO_VAR_FUNC(1, 2, 3);
+  VAR_FUNC(1, 2, 3); //expected-error{{implicit declaration of function 'func' is invalid in OpenCL}}
+  VAR_PRINTF("%i", 1);
 }
Index: test/Preprocessor/macro_variadic.cl
===
--- test/Preprocessor/macro_variadic.cl
+++ /dev/null
@@ -1,3 +0,0 @@
-// RUN: %clang_cc1 -verify %s
-
-#define X(...) 1 // expected-error {{variadic macros not supported in OpenCL}}
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -2233,12 +2233,6 @@
  diag::warn_cxx98_compat_variadic_macro :
  diag::ext_variadic_macro);
 
-  // OpenCL v1.2 s6.9.e: variadic macros are not supported.
-  if (LangOpts.OpenCL) {
-Diag(Tok, diag::err_pp_opencl_variadic_macros);
-return true;
-  }
-
   // Lex the token after the identifier.
   LexUnexpandedToken(Tok);
   if (Tok.isNot(tok::r_paren)) {
Index: include/clang/Basic/DiagnosticLexKinds.td
===

[PATCH] D59492: [OpenCL] Allow variadic macros as Clang feature

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added a reviewer: bader.
Herald added subscribers: ebevhan, yaxunl.

Variadic macros are used for debugging and therefore it is desirable to accept 
the code that uses them.

There was a discussion about this in OpenCL spec bugs: 
https://github.com/KhronosGroup/OpenCL-Docs/issues/50

There was no disagreement on relaxing this , but however we didn't find a way 
to version this yet.

For now we are allowing this as Clang feature (difference to a standard).

I assume it's not a separate feature and therefore not worth to start Clang 
extension for this. So I just modified the Clang manual for now. However, I 
could also start a new section in the extension document: 
https://clang.llvm.org/docs/LanguageExtensions.html


https://reviews.llvm.org/D59492

Files:
  docs/UsersManual.rst
  include/clang/Basic/DiagnosticLexKinds.td
  lib/Lex/PPDirectives.cpp
  test/Preprocessor/macro_variadic.cl
  test/SemaOpenCL/func.cl


Index: test/SemaOpenCL/func.cl
===
--- test/SemaOpenCL/func.cl
+++ test/SemaOpenCL/func.cl
@@ -1,5 +1,10 @@
 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -triple 
spir-unknown-unknown
 
+//Variadic macro
+#define NO_VAR_FUNC(...)  5
+#define VAR_FUNC(...) func(__VA_ARGS__);
+#define VAR_PRINTF(str, ...) printf(str, __VA_ARGS__);
+
 // Variadic functions
 void vararg_f(int, ...);// expected-error {{invalid 
prototype, variadic arguments are not allowed in OpenCL}}
 void __vararg_f(int, ...);
@@ -33,4 +38,8 @@
 
   // just calling a function is correct
   foo(0);
+
+  NO_VAR_FUNC(1, 2, 3);
+  VAR_FUNC(1, 2, 3); //expected-error{{implicit declaration of function 'func' 
is invalid in OpenCL}}
+  VAR_PRINTF("%i", 1);
 }
Index: test/Preprocessor/macro_variadic.cl
===
--- test/Preprocessor/macro_variadic.cl
+++ /dev/null
@@ -1,3 +0,0 @@
-// RUN: %clang_cc1 -verify %s
-
-#define X(...) 1 // expected-error {{variadic macros not supported in OpenCL}}
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -2233,12 +2233,6 @@
  diag::warn_cxx98_compat_variadic_macro :
  diag::ext_variadic_macro);
 
-  // OpenCL v1.2 s6.9.e: variadic macros are not supported.
-  if (LangOpts.OpenCL) {
-Diag(Tok, diag::err_pp_opencl_variadic_macros);
-return true;
-  }
-
   // Lex the token after the identifier.
   LexUnexpandedToken(Tok);
   if (Tok.isNot(tok::r_paren)) {
Index: include/clang/Basic/DiagnosticLexKinds.td
===
--- include/clang/Basic/DiagnosticLexKinds.td
+++ include/clang/Basic/DiagnosticLexKinds.td
@@ -393,9 +393,6 @@
 def note_macro_here : Note<"macro %0 defined here">;
 def note_macro_expansion_here : Note<"expansion of macro %0 requested here">;
 
-def err_pp_opencl_variadic_macros :
-  Error<"variadic macros not supported in OpenCL">;
-
 def err_pp_invalid_directive : Error<"invalid preprocessing directive">;
 def err_pp_directive_required : Error<
   "%0 must be used within a preprocessing directive">;
Index: docs/UsersManual.rst
===
--- docs/UsersManual.rst
+++ docs/UsersManual.rst
@@ -2734,6 +2734,14 @@
   enqueue query functions from `section 6.13.17.5
   `_.
 
+Differences between various standard modes
+--
+
+All OpenCL standards:
+
+- Variadic macros are supported.
+
+
 .. _target_features:
 
 Target-Specific Features and Limitations


Index: test/SemaOpenCL/func.cl
===
--- test/SemaOpenCL/func.cl
+++ test/SemaOpenCL/func.cl
@@ -1,5 +1,10 @@
 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -triple spir-unknown-unknown
 
+//Variadic macro
+#define NO_VAR_FUNC(...)  5
+#define VAR_FUNC(...) func(__VA_ARGS__);
+#define VAR_PRINTF(str, ...) printf(str, __VA_ARGS__);
+
 // Variadic functions
 void vararg_f(int, ...);// expected-error {{invalid prototype, variadic arguments are not allowed in OpenCL}}
 void __vararg_f(int, ...);
@@ -33,4 +38,8 @@
 
   // just calling a function is correct
   foo(0);
+
+  NO_VAR_FUNC(1, 2, 3);
+  VAR_FUNC(1, 2, 3); //expected-error{{implicit declaration of function 'func' is invalid in OpenCL}}
+  VAR_PRINTF("%i", 1);
 }
Index: test/Preprocessor/macro_variadic.cl
===
--- test/Preprocessor/macro_variadic.cl
+++ /dev/null
@@ -1,3 +0,0 @@
-// RUN: %clang_cc1 -verify %s
-
-#define X(...) 1 // expected-error {{variadic macros not supported in OpenCL}}
Index: lib/Lex/PPDirectives.cpp
==

[PATCH] D59492: [OpenCL] Allow variadic macros as Clang feature

[Anastasia Stulova via Phabricator via cfe-commits]

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

Fixed comment


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

https://reviews.llvm.org/D59492

Files:
  docs/UsersManual.rst
  include/clang/Basic/DiagnosticLexKinds.td
  lib/Lex/PPDirectives.cpp
  test/Preprocessor/macro_variadic.cl
  test/SemaOpenCL/func.cl


Index: test/SemaOpenCL/func.cl
===
--- test/SemaOpenCL/func.cl
+++ test/SemaOpenCL/func.cl
@@ -1,5 +1,10 @@
 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -triple 
spir-unknown-unknown
 
+// Variadic macros
+#define NO_VAR_FUNC(...)  5
+#define VAR_FUNC(...) func(__VA_ARGS__);
+#define VAR_PRINTF(str, ...) printf(str, __VA_ARGS__);
+
 // Variadic functions
 void vararg_f(int, ...);// expected-error {{invalid 
prototype, variadic arguments are not allowed in OpenCL}}
 void __vararg_f(int, ...);
@@ -33,4 +38,8 @@
 
   // just calling a function is correct
   foo(0);
+
+  NO_VAR_FUNC(1, 2, 3);
+  VAR_FUNC(1, 2, 3); //expected-error{{implicit declaration of function 'func' 
is invalid in OpenCL}}
+  VAR_PRINTF("%i", 1);
 }
Index: test/Preprocessor/macro_variadic.cl
===
--- test/Preprocessor/macro_variadic.cl
+++ /dev/null
@@ -1,3 +0,0 @@
-// RUN: %clang_cc1 -verify %s
-
-#define X(...) 1 // expected-error {{variadic macros not supported in OpenCL}}
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -2233,12 +2233,6 @@
  diag::warn_cxx98_compat_variadic_macro :
  diag::ext_variadic_macro);
 
-  // OpenCL v1.2 s6.9.e: variadic macros are not supported.
-  if (LangOpts.OpenCL) {
-Diag(Tok, diag::err_pp_opencl_variadic_macros);
-return true;
-  }
-
   // Lex the token after the identifier.
   LexUnexpandedToken(Tok);
   if (Tok.isNot(tok::r_paren)) {
Index: include/clang/Basic/DiagnosticLexKinds.td
===
--- include/clang/Basic/DiagnosticLexKinds.td
+++ include/clang/Basic/DiagnosticLexKinds.td
@@ -393,9 +393,6 @@
 def note_macro_here : Note<"macro %0 defined here">;
 def note_macro_expansion_here : Note<"expansion of macro %0 requested here">;
 
-def err_pp_opencl_variadic_macros :
-  Error<"variadic macros not supported in OpenCL">;
-
 def err_pp_invalid_directive : Error<"invalid preprocessing directive">;
 def err_pp_directive_required : Error<
   "%0 must be used within a preprocessing directive">;
Index: docs/UsersManual.rst
===
--- docs/UsersManual.rst
+++ docs/UsersManual.rst
@@ -2734,6 +2734,14 @@
   enqueue query functions from `section 6.13.17.5
   `_.
 
+Differences from various standard modes
+---
+
+All OpenCL standards:
+
+- Clang accepts variadic macros.
+
+
 .. _target_features:
 
 Target-Specific Features and Limitations


Index: test/SemaOpenCL/func.cl
===
--- test/SemaOpenCL/func.cl
+++ test/SemaOpenCL/func.cl
@@ -1,5 +1,10 @@
 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -triple spir-unknown-unknown
 
+// Variadic macros
+#define NO_VAR_FUNC(...)  5
+#define VAR_FUNC(...) func(__VA_ARGS__);
+#define VAR_PRINTF(str, ...) printf(str, __VA_ARGS__);
+
 // Variadic functions
 void vararg_f(int, ...);// expected-error {{invalid prototype, variadic arguments are not allowed in OpenCL}}
 void __vararg_f(int, ...);
@@ -33,4 +38,8 @@
 
   // just calling a function is correct
   foo(0);
+
+  NO_VAR_FUNC(1, 2, 3);
+  VAR_FUNC(1, 2, 3); //expected-error{{implicit declaration of function 'func' is invalid in OpenCL}}
+  VAR_PRINTF("%i", 1);
 }
Index: test/Preprocessor/macro_variadic.cl
===
--- test/Preprocessor/macro_variadic.cl
+++ /dev/null
@@ -1,3 +0,0 @@
-// RUN: %clang_cc1 -verify %s
-
-#define X(...) 1 // expected-error {{variadic macros not supported in OpenCL}}
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -2233,12 +2233,6 @@
  diag::warn_cxx98_compat_variadic_macro :
  diag::ext_variadic_macro);
 
-  // OpenCL v1.2 s6.9.e: variadic macros are not supported.
-  if (LangOpts.OpenCL) {
-Diag(Tok, diag::err_pp_opencl_variadic_macros);
-return true;
-  }
-
   // Lex the token after the identifier.
   LexUnexpandedToken(Tok);
   if (Tok.isNot(tok::r_paren)) {
Index: include/clang/Basic/DiagnosticLexKinds.td
=

[PATCH] D59457: [analyzer][NFC] Use capital variable names in CheckerRegistry

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

baloghadamsoftware added a comment.

Please rename the patch. Its name does not really express its content.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59457



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

[PATCH] D54978: Move the SMT API to LLVM

[Mikhail Ramalho via Phabricator via cfe-commits]

mikhail.ramalho updated this revision to Diff 191095.
mikhail.ramalho added a comment.

Fixes:

- `FindZ3.cmake` format .
- Wrong `SMTExpr` namespace in `SMTConstraintManager.h`.


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

https://reviews.llvm.org/D54978

Files:
  clang/CMakeLists.txt
  clang/cmake/modules/FindZ3.cmake
  clang/include/clang/Config/config.h.cmake
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTAPI.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/CMakeLists.txt
  clang/lib/StaticAnalyzer/Core/SMTConstraintManager.cpp
  clang/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
  clang/test/CMakeLists.txt
  clang/test/lit.site.cfg.py.in
  llvm/CMakeLists.txt
  llvm/cmake/modules/FindZ3.cmake
  llvm/cmake/modules/LLVMConfig.cmake.in
  llvm/include/llvm/Config/config.h.cmake
  llvm/include/llvm/Support/SMTAPI.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/Z3Solver.cpp

Index: llvm/lib/Support/Z3Solver.cpp
===
--- llvm/lib/Support/Z3Solver.cpp
+++ llvm/lib/Support/Z3Solver.cpp
@@ -1,4 +1,4 @@
-//== Z3ConstraintManager.cpp *- C++ -*--==//
+//== Z3Solver.cpp ---*- C++ -*--==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -6,18 +6,14 @@
 //
 //===--===//
 
-#include "clang/Basic/TargetInfo.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h"
+#include "llvm/ADT/Twine.h"
+#include "llvm/Config/config.h"
+#include "llvm/Support/SMTAPI.h"
+#include 
 
-#include "clang/Config/config.h"
+using namespace llvm;
 
-using namespace clang;
-using namespace ento;
-
-#if CLANG_ANALYZER_WITH_Z3
+#if LLVM_WITH_Z3
 
 #include 
 
@@ -818,18 +814,13 @@
 
 #endif
 
-SMTSolverRef clang::ento::CreateZ3Solver() {
-#if CLANG_ANALYZER_WITH_Z3
+llvm::SMTSolverRef llvm::CreateZ3Solver() {
+#if LLVM_WITH_Z3
   return llvm::make_unique();
 #else
   llvm::report_fatal_error("Clang was not compiled with Z3 support, rebuild "
-   "with -DCLANG_ANALYZER_ENABLE_Z3_SOLVER=ON",
+   "with -DLLVM_ENABLE_Z3_SOLVER=ON",
false);
   return nullptr;
 #endif
 }
-
-std::unique_ptr
-ento::CreateZ3ConstraintManager(ProgramStateManager &StMgr, SubEngine *Eng) {
-  return llvm::make_unique(Eng, StMgr.getSValBuilder());
-}
Index: llvm/lib/Support/CMakeLists.txt
===
--- llvm/lib/Support/CMakeLists.txt
+++ llvm/lib/Support/CMakeLists.txt
@@ -44,6 +44,13 @@
   set (delayload_flags delayimp -delayload:shell32.dll -delayload:ole32.dll)
 endif()
 
+# Link Z3 if the user wants to build it.
+if(LLVM_WITH_Z3)
+  set(Z3_LINK_FILES ${Z3_LIBRARIES})
+else()
+  set(Z3_LINK_FILES "")
+endif()
+
 add_llvm_library(LLVMSupport
   AArch64TargetParser.cpp
   ARMTargetParser.cpp
@@ -152,6 +159,7 @@
   regfree.c
   regstrlcpy.c
   xxhash.cpp
+  Z3Solver.cpp
 
 # System
   Atomic.cpp
@@ -177,7 +185,14 @@
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/ADT
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/Support
   ${Backtrace_INCLUDE_DIRS}
-  LINK_LIBS ${system_libs} ${delayload_flags}
+  LINK_LIBS ${system_libs} ${delayload_flags} ${Z3_LINK_FILES}
   )
 
 set_property(TARGET LLVMSupport PROPERTY LLVM_SYSTEM_LIBS "${system_libs}")
+
+if(LLVM_WITH_Z3)
+  target_include_directories(LLVMSupport SYSTEM
+PRIVATE
+${Z3_INCLUDE_DIR}
+)
+endif()
Index: llvm/include/llvm/Support/SMTAPI.h
===
--- llvm/include/llvm/Support/SMTAPI.h
+++ llvm/include/llvm/Support/SMTAPI.h
@@ -11,15 +11,16 @@
 //
 //===--===//
 
-#ifndef LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_SMTSOLVER_H
-#define LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_SMTSOLVER_H
+#ifndef LLVM_SUPPORT_SMTAPI_H
+#define LLVM_SUPPORT_SMTAPI_H
 
-#include "clang/Basic/TargetInfo.h"
+#include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/FoldingSet.h"
+#include "llvm/Support/raw_ostream.h"
+#include 
 
-namespace clang {
-namespace ento {
+namespace llvm {
 
 /// Generic base class for SMT sorts
 class SMTSort {
@@ -399,7 +400,6 @@
 /// Convenience method to create and Z3Solver object
 SMTSolverRef CreateZ3Solver();
 
-} // namespace ento
-} // namespace clang
+} // namespace llvm
 
 #endif
Index: llvm/inclu

[PATCH] D59458: [analyzer][NFC] Clang-format CheckerRegistry

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

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

No patch should be committed without clang-formatting anyway...


Repository:
  rC Clang

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

https://reviews.llvm.org/D59458



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

[PATCH] D59492: [OpenCL] Allow variadic macros as Clang feature

[Alexey Bader via Phabricator via cfe-commits]

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

LGTM. Thanks!


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

https://reviews.llvm.org/D59492



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

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

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

As far as I'm concerned, I don't have any major revision requests.  I haven't 
reviewed the unit tests (I'm planning to), but for non-test code, I don't have 
any further comments.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214



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

[PATCH] D59493: [OpenCL] Generate 'unroll.enable' metadata for __attribute__((opencl_unroll_hint))

[Dmitry Sidorov via Phabricator via cfe-commits]

sidorovd created this revision.
sidorovd added reviewers: Anastasia, yaxunl.
Herald added subscribers: cfe-commits, jdoerfert, dmgreen, zzheng.
Herald added a project: clang.

[OpenCL] Generate 'unroll.enable' metadata for  
__attribute__((opencl_unroll_hint))

  

For both !{!"llvm.loop.unroll.enable"} and !{!"llvm.loop.unroll.full"} the 
unroller
will try to fully unroll a loop unless the trip count is not known at compile 
time.
In that case for '.full' metadata no unrolling will be processed, while for 
'.enable'
the loop will be partially unrolled with a heuristically chosen unroll factor.

  

See: docs/LanguageExtensions.rst

  

>From 
>https://www.khronos.org/registry/OpenCL/sdk/2.0/docs/man/xhtml/attributes-loopUnroll.html
--

  __attribute__((opencl_unroll_hint))
  for (int i=0; i<2; i++)
  {
  ...
  }


In the example above, the compiler will determine how much to unroll the loop.
--

Before the patch for  __attribute__((opencl_unroll_hint)) was generated metadata
!{!"llvm.loop.unroll.full"}, which limits ability of loop unroller to decide, 
how
much to unroll the loop.


Repository:
  rC Clang

https://reviews.llvm.org/D59493

Files:
  lib/CodeGen/CGLoopInfo.cpp
  test/CodeGenOpenCL/unroll-hint.cl


Index: test/CodeGenOpenCL/unroll-hint.cl
===
--- test/CodeGenOpenCL/unroll-hint.cl
+++ test/CodeGenOpenCL/unroll-hint.cl
@@ -18,12 +18,12 @@
 // CHECK: br label %{{.*}}, !llvm.loop ![[FOR_DISABLE:.*]]
 }
 
-void for_full()
+void for_enable()
 {
-// CHECK-LABEL: for_full
+// CHECK-LABEL: for_enable
 __attribute__((opencl_unroll_hint))
 for( int i = 0; i < 1000; ++i);
-// CHECK: br label %{{.*}}, !llvm.loop ![[FOR_FULL:.*]]
+// CHECK: br label %{{.*}}, !llvm.loop ![[FOR_ENABLE:.*]]
 }
 
 /*** while ***/
@@ -45,13 +45,13 @@
 // CHECK: br label %{{.*}}, !llvm.loop ![[WHILE_DISABLE:.*]]
 }
 
-void while_full()
+void while_enable()
 {
-// CHECK-LABEL: while_full
+// CHECK-LABEL: while_enable
 int i = 1000;
 __attribute__((opencl_unroll_hint))
 while(i-->0);
-// CHECK: br label %{{.*}}, !llvm.loop ![[WHILE_FULL:.*]]
+// CHECK: br label %{{.*}}, !llvm.loop ![[WHILE_ENABLE:.*]]
 }
 
 /*** do ***/
@@ -73,13 +73,13 @@
 // CHECK: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}, !llvm.loop 
![[DO_DISABLE:.*]]
 }
 
-void do_full()
+void do_enable()
 {
-// CHECK-LABEL: do_full
+// CHECK-LABEL: do_enable
 int i = 1000;
 __attribute__((opencl_unroll_hint))
 do {} while(i--> 0);
-// CHECK: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}, !llvm.loop 
![[DO_FULL:.*]]
+// CHECK: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}, !llvm.loop 
![[DO_ENABLE:.*]]
 }
 
 
@@ -87,11 +87,11 @@
 // CHECK: ![[COUNT]] =  !{!"llvm.loop.unroll.count", i32 8}
 // CHECK: ![[FOR_DISABLE]]   =  distinct !{![[FOR_DISABLE]],  ![[DISABLE:.*]]}
 // CHECK: ![[DISABLE]]   =  !{!"llvm.loop.unroll.disable"}
-// CHECK: ![[FOR_FULL]]  =  distinct !{![[FOR_FULL]],  ![[FULL:.*]]}
-// CHECK: ![[FULL]]  =  !{!"llvm.loop.unroll.full"}
+// CHECK: ![[FOR_ENABLE]]  =  distinct !{![[FOR_ENABLE]],  ![[ENABLE:.*]]}
+// CHECK: ![[ENABLE]]  =  !{!"llvm.loop.unroll.enable"}
 // CHECK: ![[WHILE_COUNT]]   =  distinct !{![[WHILE_COUNT]],![[COUNT]]}
 // CHECK: ![[WHILE_DISABLE]] =  distinct !{![[WHILE_DISABLE]],  ![[DISABLE]]}
-// CHECK: ![[WHILE_FULL]]=  distinct !{![[WHILE_FULL]], ![[FULL]]}
+// CHECK: ![[WHILE_ENABLE]]=  distinct !{![[WHILE_ENABLE]], 
![[ENABLE]]}
 // CHECK: ![[DO_COUNT]]  =  distinct !{![[DO_COUNT]],   ![[COUNT]]}
 // CHECK: ![[DO_DISABLE]]=  distinct !{![[DO_DISABLE]], ![[DISABLE]]}
-// CHECK: ![[DO_FULL]]   =  distinct !{![[DO_FULL]],![[FULL]]}
+// CHECK: ![[DO_ENABLE]]   =  distinct !{![[DO_ENABLE]],
![[ENABLE]]}
Index: lib/CodeGen/CGLoopInfo.cpp
===
--- lib/CodeGen/CGLoopInfo.cpp
+++ lib/CodeGen/CGLoopInfo.cpp
@@ -208,13 +208,13 @@
 // Translate opencl_unroll_hint attribute argument to
 // equivalent LoopHintAttr enums.
 // OpenCL v2.0 s6.11.5:
-// 0 - full unroll (no argument).
+// 0 - enable unroll (no argument).
 // 1 - disable unroll.
 // other positive integer n - unroll by n.
 if (OpenCLHint) {
   ValueInt = OpenCLHint->getUnrollHint();
   if (ValueInt == 0) {
-State = LoopHintAttr::Full;
+State = LoopHintAttr::Enable;
   } else if (ValueInt != 1) {
 Option = LoopHintAttr::UnrollCount;
 State = LoopHintAttr::Numeric;


Index: test/CodeGenOpenCL/unroll-hint.cl
===
--- test/CodeGenOpenCL/unroll-hint.cl
+++ test/CodeGenOpenCL/unroll-hint.cl
@@ -18,12 +18,12 @@
 // CHECK: br label %

[PATCH] D59494: AMDGPU: Add support for cross address space synchronization scopes

[Konstantin Zhuravlyov via Phabricator via cfe-commits]

kzhuravl added a comment.

Backend change will be posted later today. I am currently working on adding 
backend tests.


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

https://reviews.llvm.org/D59494



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

[PATCH] D59494: AMDGPU: Add support for cross address space synchronization scopes

[Konstantin Zhuravlyov via Phabricator via cfe-commits]

kzhuravl created this revision.
kzhuravl added reviewers: b-sumner, rampitec, t-tye.
Herald added subscribers: jfb, tpr, dstuttard, yaxunl, wdng.
kzhuravl added a comment.

Backend change will be posted later today. I am currently working on adding 
backend tests.


https://reviews.llvm.org/D59494

Files:
  lib/CodeGen/CGAtomic.cpp
  lib/CodeGen/TargetInfo.cpp
  lib/CodeGen/TargetInfo.h
  test/CodeGenOpenCL/atomic-ops.cl

Index: test/CodeGenOpenCL/atomic-ops.cl
===
--- test/CodeGenOpenCL/atomic-ops.cl
+++ test/CodeGenOpenCL/atomic-ops.cl
@@ -83,7 +83,7 @@
 
 bool fi4(atomic_int *i) {
   // CHECK-LABEL: @fi4(
-  // CHECK: [[PAIR:%[.0-9A-Z_a-z]+]] = cmpxchg i32* [[PTR:%[.0-9A-Z_a-z]+]], i32 [[EXPECTED:%[.0-9A-Z_a-z]+]], i32 [[DESIRED:%[.0-9A-Z_a-z]+]] syncscope("workgroup") acquire acquire
+  // CHECK: [[PAIR:%[.0-9A-Z_a-z]+]] = cmpxchg i32* [[PTR:%[.0-9A-Z_a-z]+]], i32 [[EXPECTED:%[.0-9A-Z_a-z]+]], i32 [[DESIRED:%[.0-9A-Z_a-z]+]] syncscope("workgroup-one-as") acquire acquire
   // CHECK: [[OLD:%[.0-9A-Z_a-z]+]] = extractvalue { i32, i1 } [[PAIR]], 0
   // CHECK: [[CMP:%[.0-9A-Z_a-z]+]] = extractvalue { i32, i1 } [[PAIR]], 1
   // CHECK: br i1 [[CMP]], label %[[STORE_EXPECTED:[.0-9A-Z_a-z]+]], label %[[CONTINUE:[.0-9A-Z_a-z]+]]
@@ -141,21 +141,21 @@
   // CHECK-NEXT: i32 4, label %[[SEQ_SUB:.*]]
   // CHECK-NEXT: ]
   // CHECK: [[MON_WG]]:
-  // CHECK: load atomic i32, i32* %{{.*}} syncscope("workgroup") monotonic
+  // CHECK: load atomic i32, i32* %{{.*}} syncscope("workgroup-one-as") monotonic
   // CHECK: [[MON_DEV]]:
-  // CHECK: load atomic i32, i32* %{{.*}} syncscope("agent") monotonic
+  // CHECK: load atomic i32, i32* %{{.*}} syncscope("agent-one-as") monotonic
   // CHECK: [[MON_ALL]]:
   // CHECK: load atomic i32, i32* %{{.*}} monotonic
   // CHECK: [[MON_SUB]]:
-  // CHECK: load atomic i32, i32* %{{.*}} syncscope("wavefront") monotonic
+  // CHECK: load atomic i32, i32* %{{.*}} syncscope("wavefront-one-as") monotonic
   // CHECK: [[ACQ_WG]]:
-  // CHECK: load atomic i32, i32* %{{.*}} syncscope("workgroup") acquire
+  // CHECK: load atomic i32, i32* %{{.*}} syncscope("workgroup-one-as") acquire
   // CHECK: [[ACQ_DEV]]:
-  // CHECK: load atomic i32, i32* %{{.*}} syncscope("agent") acquire
+  // CHECK: load atomic i32, i32* %{{.*}} syncscope("agent-one-as") acquire
   // CHECK: [[ACQ_ALL]]:
   // CHECK: load atomic i32, i32* %{{.*}} acquire
   // CHECK: [[ACQ_SUB]]:
-  // CHECK: load atomic i32, i32* %{{.*}} syncscope("wavefront") acquire
+  // CHECK: load atomic i32, i32* %{{.*}} syncscope("wavefront-one-as") acquire
   // CHECK: [[SEQ_WG]]:
   // CHECK: load atomic i32, i32* %{{.*}} syncscope("workgroup") seq_cst
   // CHECK: [[SEQ_DEV]]:
@@ -169,13 +169,13 @@
 
 float ff1(global atomic_float *d) {
   // CHECK-LABEL: @ff1
-  // CHECK: load atomic i32, i32 addrspace(1)* {{.*}} syncscope("workgroup") monotonic
+  // CHECK: load atomic i32, i32 addrspace(1)* {{.*}} syncscope("workgroup-one-as") monotonic
   return __opencl_atomic_load(d, memory_order_relaxed, memory_scope_work_group);
 }
 
 void ff2(atomic_float *d) {
   // CHECK-LABEL: @ff2
-  // CHECK: store atomic i32 {{.*}} syncscope("workgroup") release
+  // CHECK: store atomic i32 {{.*}} syncscope("workgroup-one-as") release
   __opencl_atomic_store(d, 1, memory_order_release, memory_scope_work_group);
 }
 
@@ -198,7 +198,7 @@
 
 // CHECK-LABEL: @failureOrder
 void failureOrder(atomic_int *ptr, int *ptr2) {
-  // CHECK: cmpxchg i32* {{%[0-9A-Za-z._]+}}, i32 {{%[0-9A-Za-z._]+}}, i32 {{%[0-9A-Za-z_.]+}} syncscope("workgroup") acquire monotonic
+  // CHECK: cmpxchg i32* {{%[0-9A-Za-z._]+}}, i32 {{%[0-9A-Za-z._]+}}, i32 {{%[0-9A-Za-z_.]+}} syncscope("workgroup-one-as") acquire monotonic
   __opencl_atomic_compare_exchange_strong(ptr, ptr2, 43, memory_order_acquire, memory_order_relaxed, memory_scope_work_group);
 
   // CHECK: cmpxchg weak i32* {{%[0-9A-Za-z._]+}}, i32 {{%[0-9A-Za-z._]+}}, i32 {{%[0-9A-Za-z_.]+}} syncscope("workgroup") seq_cst acquire
Index: lib/CodeGen/TargetInfo.h
===
--- lib/CodeGen/TargetInfo.h
+++ lib/CodeGen/TargetInfo.h
@@ -262,8 +262,10 @@
llvm::Type *DestTy) const;
 
   /// Get the syncscope used in LLVM IR.
-  virtual llvm::SyncScope::ID getLLVMSyncScopeID(SyncScope S,
- llvm::LLVMContext &C) const;
+  virtual llvm::SyncScope::ID getLLVMSyncScopeID(const LangOptions &LangOpts,
+ SyncScope Scope,
+ llvm::AtomicOrdering Ordering,
+ llvm::LLVMContext &Ctx) const;
 
   /// Interface class for filling custom fields of a block literal for OpenCL.
   class TargetOpenCLBlockHelper {
Index: lib/CodeGen/TargetInfo.cpp

[PATCH] D59493: [OpenCL] Generate 'unroll.enable' metadata for __attribute__((opencl_unroll_hint))

[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!

The wording in spec is confusing btw because it says:

> This attribute qualifier can be used to specify full unrolling or partial 
> unrolling by a specified amount.

May be we should fix this.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59493



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

[PATCH] D54978: Move the SMT API to LLVM

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

In D54978#1432178 , @ddcc wrote:

> In D54978#1431935 , @delcypher wrote:
>
> > Would one of you be able to file a bug against Z3 to fix this? I am no 
> > longer in a position to contribute to Z3 so I can't do this.
>
>
> I've opened https://github.com/Z3Prover/z3/issues/2184 .


Thanks.

> In D54978#1431936 , @delcypher wrote:
> 
>> This output means you built Z3 from source that was not in a git repository. 
>> In this case the header file should look the same for both Z3's CMake and 
>> Python build systems.
> 
> 
> That's strange, I have been building from a git repository.

Hmm I misread the python code. I inferred that because the extra text from

  def get_full_version_string(major, minor, build, revision):
  global GIT_HASH, GIT_DESCRIBE
  res = "Z3 %s.%s.%s.%s" % (major, minor, build, revision)
  if GIT_HASH:
  res += " " + GIT_HASH
  if GIT_DESCRIBE:
  branch = check_output(['git', 'rev-parse', '--abbrev-ref', 'HEAD'])
  res += " " + branch + " " + check_output(['git', 'describe'])
  return '"' + res + '"'

wasn't in your output. However, I didn't actually check to see how `GIT_HASH` 
and `GIT_DESCRIBE` are set. It looks like you need to pass `--githash` and 
`--gitdescribe` respectively to the Python build system configure scripts to 
get those. They are false by default.
The Z3 CMake build system is different. It will add the git hash and 
description by default if it detects you're using git ( see 
https://github.com/Z3Prover/z3/blob/057151c7a80dac44d610f5286799ad7b727b5d2c/CMakeLists.txt#L67
 ). You can switch this off by passing
`-DINCLUDE_GIT_HASH=OFF -DINCLUDE_GIT_DESCRIBE=OFF` to the CMake invocation you 
use when configuring Z3.

> In D54978#1431430 , @mikhail.ramalho 
> wrote:
> 
>> 2. Instead of parsing `Z3_FULL_VERSION`, we can parse `Z3_MAJOR_VERSION`, 
>> `Z3_MINOR_VERSION` and `Z3_BUILD_NUMBER` which are also available in the 
>> same header.
> 
> 
> Since the differences in version string depending on the build system are 
> intended behavior, it seems (2) would be preferable?

Yeah, that's the way I would go.


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

https://reviews.llvm.org/D54978



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

[PATCH] D59493: [OpenCL] Generate 'unroll.enable' metadata for __attribute__((opencl_unroll_hint))

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

PS, I am just saying that I guess leaving this to the compiler is more helpful 
than explicitly requiring the full unroll. However, spec contradicts itself by 
first mentioning the full unroll and then stating that compiler will determines 
the unrolling factor.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59493



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

[PATCH] D59486: [OpenCL] Improve testing of default header in C++ mode

[Anastasia Stulova via Phabricator via cfe-commits]

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

Moved testing of default header in C++ mode to test/Headers/opencl-c-header.cl


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

https://reviews.llvm.org/D59486

Files:
  test/Driver/include-default-header.cl
  test/Headers/opencl-c-header.cl
  test/SemaOpenCL/builtin.cl
  test/SemaOpenCL/extensions.cl


Index: test/SemaOpenCL/extensions.cl
===
--- test/SemaOpenCL/extensions.cl
+++ test/SemaOpenCL/extensions.cl
@@ -28,7 +28,7 @@
 // enabled by default with -cl-std=CL2.0).
 //
 // RUN: %clang_cc1 %s -triple amdgcn-unknown-unknown -verify -pedantic 
-fsyntax-only -cl-std=CL2.0 -finclude-default-header
-// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic 
-fsyntax-only -cl-std=c++ -finclude-default-header
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic 
-fsyntax-only -cl-std=c++
 
 #ifdef _OPENCL_H_
 // expected-no-diagnostics
Index: test/SemaOpenCL/builtin.cl
===
--- test/SemaOpenCL/builtin.cl
+++ test/SemaOpenCL/builtin.cl
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL1.2
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=c++
 
 // expected-no-diagnostics
 
Index: test/Headers/opencl-c-header.cl
===
--- test/Headers/opencl-c-header.cl
+++ test/Headers/opencl-c-header.cl
@@ -1,6 +1,7 @@
 // RUN: %clang_cc1 -O0 -triple spir-unknown-unknown -internal-isystem 
../../lib/Headers -include opencl-c.h -emit-llvm -o - %s -verify | FileCheck %s
-// RUN: %clang_cc1 -O0 -triple spir-unknown-unknown -internal-isystem 
../../lib/Headers -include opencl-c.h -emit-llvm -o - %s -verify -cl-std=CL1.1| 
FileCheck %s
-// RUN: %clang_cc1 -O0 -triple spir-unknown-unknown -internal-isystem 
../../lib/Headers -include opencl-c.h -emit-llvm -o - %s -verify -cl-std=CL1.2| 
FileCheck %s
+// RUN: %clang_cc1 -O0 -triple spir-unknown-unknown -internal-isystem 
../../lib/Headers -include opencl-c.h -emit-llvm -o - %s -verify -cl-std=CL1.1 
| FileCheck %s
+// RUN: %clang_cc1 -O0 -triple spir-unknown-unknown -internal-isystem 
../../lib/Headers -include opencl-c.h -emit-llvm -o - %s -verify -cl-std=CL1.2 
| FileCheck %s
+// RUN: %clang_cc1 -O0 -triple spir-unknown-unknown -internal-isystem 
../../lib/Headers -include opencl-c.h -emit-llvm -o - %s -verify -cl-std=c++ | 
FileCheck %s --check-prefix=CHECK20
 
 // Test including the default header as a module.
 // The module should be compiled only once and loaded from cache afterwards.
@@ -54,7 +55,7 @@
 // CHECK20: _Z3ctzc
 // CHECK20-NOT: _Z16convert_char_rtec
 char f(char x) {
-#if __OPENCL_C_VERSION__ != CL_VERSION_2_0
+#if !defined(__OPENCL_CPP_VERSION__) && (__OPENCL_C_VERSION__ != 
CL_VERSION_2_0)
   return convert_char_rte(x);
 
 #else //__OPENCL_C_VERSION__
@@ -67,7 +68,7 @@
 // from OpenCL 2.0 onwards.
 
 // CHECK20: _Z12write_imagef14ocl_image3d_wo
-#if __OPENCL_C_VERSION__ >= CL_VERSION_2_0
+#if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_2_0)
 void test_image3dwo(write_only image3d_t img) {
   write_imagef(img, (0), (0.0f));
 }
@@ -75,7 +76,7 @@
 
 // Verify that non-builtin cl_intel_planar_yuv extension is defined from
 // OpenCL 1.2 onwards.
-#if (__OPENCL_C_VERSION__ >= CL_VERSION_1_2)
+#if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ >= CL_VERSION_1_2)
 // expected-no-diagnostics
 #ifndef cl_intel_planar_yuv
 #error "Missing cl_intel_planar_yuv define"
Index: test/Driver/include-default-header.cl
===
--- test/Driver/include-default-header.cl
+++ test/Driver/include-default-header.cl
@@ -1,5 +1,6 @@
-// RUN: %clang -save-temps -x cl -Xclang -cl-std=CL2.0 -Xclang 
-finclude-default-header -emit-llvm -S -### %s
-// CHECK-NOT: finclude-default-header
+// RUN: %clang -save-temps -x cl -Xclang -cl-std=CL2.0 -Xclang 
-finclude-default-header -emit-llvm -S -### %s 2>&1 | FileCheck %s
+
+// CHECK-LABEL: finclude-default-header
 // Make sure we don't pass -finclude-default-header to any commands other than 
the driver.
 
 void test() {}


Index: test/SemaOpenCL/extensions.cl
===
--- test/SemaOpenCL/extensions.cl
+++ test/SemaOpenCL/extensions.cl
@@ -28,7 +28,7 @@
 // enabled by default with -cl-std=CL2.0).
 //
 // RUN: %clang_cc1 %s -triple amdgcn-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=CL2.0 -finclude-default-header
-// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=c++ -finclude-default-header
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only -cl-std=c++
 
 #ifdef _OPENCL_H_
 // expected-no-diagnostics
Index: test/SemaOpenCL/builtin.cl
==

[PATCH] D59388: Basic: Return a reference from FileManager::getVirtualFileSystem, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D59388#1431314 , @jkorous wrote:

> Hi Duncan, thanks for working on better interfaces in clang!
>
> I am just wondering - is it safe to have the lifetime of a single object on 
> heap managed by two different `IntrusiveRefCntPtr` instances?


Yes, it's safe.  The reference count is "intrusive", meaning it's stored in the 
object itself (via inheritance from `RefCountedBase`).  As a result, all the 
instances of `IntrusiveRefCntPtr` that reference at the same object will 
implicitly share their count.


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

https://reviews.llvm.org/D59388



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

[PATCH] D59486: [OpenCL] Improve testing of default header in C++ mode

[Anastasia Stulova via Phabricator via cfe-commits]

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



Comment at: test/Driver/include-default-header.cl:2
+// RUN: %clang -save-temps -x cl -Xclang -cl-std=CL2.0 -Xclang 
-finclude-default-header -emit-llvm -S -### %s 2>&1 | FileCheck %s
+// RUN: %clang -save-temps -x cl -Xclang -cl-std=c++ -Xclang 
-finclude-default-header -emit-llvm -S -### %s 2>&1 | FileCheck %s
+

bader wrote:
> According to my understanding test/Driver tests covers only driver component, 
> i.e. all other components like Parser/Sema are not covered by these tests. 
> AFAIK "-###" prints the commands generated by driver, but do not run them.
> Do we want to test parsing/Sema for OpenCL C++ mode or do you think that 
> testing OpenCL C mode is enough?
I think testing Clang actions in C++ mode with default include is pointless 
since lang mode doesn't affect anything there. But testing in Sema is indeed 
what we should do. I have updated the patch now.

However, I am still fixing this test because it wasn't testing anything before.

Thanks for spotting this!


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

https://reviews.llvm.org/D59486



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

[PATCH] D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith updated this revision to Diff 191104.
dexonsmith added a comment.

Document the default VFS used by the `FileManager`.


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

https://reviews.llvm.org/D59377

Files:
  clang-tools-extra/clangd/Compiler.cpp
  clang/include/clang/Basic/FileManager.h
  clang/include/clang/Frontend/CompilerInstance.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
@@ -671,7 +671,7 @@
   }
 
   // Make sure clang uses the same VFS as LLDB.
-  instance->setVirtualFileSystem(FileSystem::Instance().GetVirtualFileSystem());
+  instance->createFileManager(FileSystem::Instance().GetVirtualFileSystem());
   instance->setDiagnostics(diagnostics_engine.get());
   instance->setInvocation(invocation);
 
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -309,8 +309,7 @@
   }
 
   // Make sure clang uses the same VFS as LLDB.
-  m_compiler->setVirtualFileSystem(
-  FileSystem::Instance().GetVirtualFileSystem());
+  m_compiler->createFileManager(FileSystem::Instance().GetVirtualFileSystem());
 
   lldb::LanguageType frame_lang =
   expr.Language(); // defaults to lldb::eLanguageTypeUnknown
Index: clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
===
--- clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
+++ clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
@@ -82,8 +82,6 @@
 
   Instance.getDiagnostics().setSourceManager(&SM);
 
-  Instance.setVirtualFileSystem(&CI.getVirtualFileSystem());
-
   // The instance wants to take ownership, however DisableFree frontend option
   // is set to true to avoid double free issues
   Instance.setFileManager(&CI.getFileManager());
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -89,10 +89,6 @@
 
 void CompilerInstance::setFileManager(FileManager *Value) {
   FileMgr = Value;
-  if (Value)
-VirtualFileSystem = Value->getVirtualFileSystem();
-  else
-VirtualFileSystem.reset();
 }
 
 void CompilerInstance::setSourceManager(SourceManager *Value) {
@@ -297,13 +293,14 @@
 
 // File Manager
 
-FileManager *CompilerInstance::createFileManager() {
-  if (!hasVirtualFileSystem()) {
-IntrusiveRefCntPtr VFS =
-createVFSFromCompilerInvocation(getInvocation(), getDiagnostics());
-setVirtualFileSystem(VFS);
-  }
-  FileMgr = new FileManager(getFileSystemOpts(), VirtualFileSystem);
+FileManager *CompilerInstance::createFileManager(
+IntrusiveRefCntPtr VFS) {
+  if (!VFS)
+VFS = FileMgr ? FileMgr->getVirtualFileSystem()
+  : createVFSFromCompilerInvocation(getInvocation(),
+getDiagnostics());
+  assert(VFS && "FileManager has no VFS?");
+  FileMgr = new FileManager(getFileSystemOpts(), std::move(VFS));
   return FileMgr.get();
 }
 
@@ -1101,8 +1098,6 @@
ImportingInstance.getDiagnosticClient()),
  /*ShouldOwnClient=*/true);
 
-  Instance.setVirtualFileSystem(&ImportingInstance.getVirtualFileSystem());
-
   // Note that this module is part of the module build stack, so that we
   // can detect cycles in the module graph.
   Instance.setFileManager(&ImportingInstance.getFileManager());
Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -1078,28 +1078,29 @@
   if (!Invocation)
 return true;
 
+  if (VFS && FileMgr)
+assert(VFS == FileMgr->getVirtualFileSystem() &&
+   "VFS passed to Parse and VFS in FileMgr are different");
+
   auto CCInvocation = std::make_shared(*Invocation);
   if (OverrideMainBuffer) {
 assert(Preamble &&
"No preamble was built, but OverrideMainBuffer is not null");
-IntrusiveRefCntPtr OldVFS = VFS;
 Preamble->AddImplicitPreamble(*CCInvocation, VFS, OverrideMainBuffer.get());
-if (OldVFS != VFS && FileMgr) {
-  assert(OldVFS == FileMgr->getVirtualFileSystem() &&
- "VFS passed to Parse and VFS in FileMgr are diff

[PATCH] D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D59377#1431203 , @jkorous wrote:

> In D59377#1430002 , @dexonsmith 
> wrote:
>
> > ... since I noticed that FileManager ...
>
>
> This kind of implies that we should move the comment from `FileManager` 
> constructor implementation to the header thus making it an explicit part of 
> the interface contract.
>
>   // If the caller doesn't provide a virtual file system, just grab the real
>   // file system.
>   
>
> Maybe https://reviews.llvm.org/D59388 would be the right place to do it.


Good call; since I'm relying on it in this patch I've updated this patch to 
document it.


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

https://reviews.llvm.org/D59377



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

[PATCH] D59487: [Tooling] Add more scope specifiers until spelling is not ambiguous.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

LGTM




Comment at: lib/Tooling/Core/Lookup.cpp:165
+if (UnspelledScopes.empty()) {
+  Disambiguated = "::" + Disambiguated;
+} else {

maybe return or break afterwards?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59487



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

[PATCH] D59388: Basic: Return a reference from FileManager::getVirtualFileSystem, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith updated this revision to Diff 191105.
dexonsmith added a comment.

Rebase.


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

https://reviews.llvm.org/D59388

Files:
  clang-tools-extra/clang-move/ClangMove.cpp
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clangd/SourceCode.cpp
  clang/include/clang/Basic/FileManager.h
  clang/include/clang/Frontend/CompilerInstance.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/lib/Lex/PPLexerChange.cpp
  clang/lib/Sema/SemaCodeComplete.cpp
  clang/lib/Tooling/Tooling.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -44,7 +44,7 @@
   assert(ToAST);
   ASTContext &ToCtx = ToAST->getASTContext();
   auto *OFS = static_cast(
-  ToCtx.getSourceManager().getFileManager().getVirtualFileSystem().get());
+  &ToCtx.getSourceManager().getFileManager().getVirtualFileSystem());
   auto *MFS = static_cast(
   OFS->overlays_begin()->get());
   MFS->addFile(FileName, 0, std::move(Buffer));
Index: clang/lib/Tooling/Tooling.cpp
===
--- clang/lib/Tooling/Tooling.cpp
+++ clang/lib/Tooling/Tooling.cpp
@@ -301,7 +301,7 @@
   DiagConsumer ? DiagConsumer : &DiagnosticPrinter, false);
 
   const std::unique_ptr Driver(
-  newDriver(&Diagnostics, BinaryName, Files->getVirtualFileSystem()));
+  newDriver(&Diagnostics, BinaryName, &Files->getVirtualFileSystem()));
   // The "input file not found" diagnostics from the driver are useful.
   // The driver is only aware of the VFS working directory, but some clients
   // change this at the FileManager level instead.
Index: clang/lib/Sema/SemaCodeComplete.cpp
===
--- clang/lib/Sema/SemaCodeComplete.cpp
+++ clang/lib/Sema/SemaCodeComplete.cpp
@@ -8378,7 +8378,8 @@
   // We need the native slashes for the actual file system interactions.
   SmallString<128> NativeRelDir = StringRef(RelDir);
   llvm::sys::path::native(NativeRelDir);
-  auto FS = getSourceManager().getFileManager().getVirtualFileSystem();
+  llvm::vfs::FileSystem &FS =
+  getSourceManager().getFileManager().getVirtualFileSystem();
 
   ResultBuilder Results(*this, CodeCompleter->getAllocator(),
 CodeCompleter->getCodeCompletionTUInfo(),
@@ -8424,7 +8425,7 @@
 
 std::error_code EC;
 unsigned Count = 0;
-for (auto It = FS->dir_begin(Dir, EC);
+for (auto It = FS.dir_begin(Dir, EC);
  !EC && It != llvm::vfs::directory_iterator(); It.increment(EC)) {
   if (++Count == 2500) // If we happen to hit a huge directory,
 break; // bail out early so we're not too slow.
Index: clang/lib/Lex/PPLexerChange.cpp
===
--- clang/lib/Lex/PPLexerChange.cpp
+++ clang/lib/Lex/PPLexerChange.cpp
@@ -270,7 +270,7 @@
 
   ModuleMap &ModMap = getHeaderSearchInfo().getModuleMap();
   const DirectoryEntry *Dir = Mod.getUmbrellaDir().Entry;
-  llvm::vfs::FileSystem &FS = *FileMgr.getVirtualFileSystem();
+  llvm::vfs::FileSystem &FS = FileMgr.getVirtualFileSystem();
   std::error_code EC;
   for (llvm::vfs::recursive_directory_iterator Entry(FS, Dir->getName(), EC),
End;
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -1021,7 +1021,7 @@
 = StringRef(FrameworkDir->getName());
   llvm::sys::path::append(SubframeworksDirName, "Frameworks");
   llvm::sys::path::native(SubframeworksDirName);
-  llvm::vfs::FileSystem &FS = *FileMgr.getVirtualFileSystem();
+  llvm::vfs::FileSystem &FS = FileMgr.getVirtualFileSystem();
   for (llvm::vfs::directory_iterator
Dir = FS.dir_begin(SubframeworksDirName, EC),
DirEnd;
@@ -2397,7 +2397,7 @@
 std::error_code EC;
 SmallVector Headers;
 llvm::vfs::FileSystem &FS =
-*SourceMgr.getFileManager().getVirtualFileSystem();
+SourceMgr.getFileManager().getVirtualFileSystem();
 for (llvm::vfs::recursive_directory_iterator I(FS, Dir->getName(), EC), E;
  I != E && !EC; I.increment(EC)) {
   if (const FileEntry *FE = SourceMgr.getFileManager().getFile(I->path())) {
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -1579,7 +1579,7 @@
 DirNative);
 
 // Search each of the ".framework" directories to load them as modules.
-llvm::vfs::FileSystem &FS = *FileMgr.getVi

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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

The OpenMP part looks good.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59214



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

[PATCH] D59487: [Tooling] Add more scope specifiers until spelling is not ambiguous.

[Eric Liu via Phabricator via cfe-commits]

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



Comment at: lib/Tooling/Core/Lookup.cpp:165
+if (UnspelledScopes.empty()) {
+  Disambiguated = "::" + Disambiguated;
+} else {

kadircet wrote:
> maybe return or break afterwards?
I also struggled a bit here. But I decided to let `IsAmbiguousSpelling` handle 
it because it seemed a bit more natural. Otherwise, we would somewhat duplicate 
the logic that spelling with leading "::" is not ambiguous.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59487



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

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

[Gauthier via Phabricator via cfe-commits]

Tyker updated this revision to Diff 191115.
Tyker added a comment.

i think we are suppose to hook likely/unlikely on builtin_expected, for if, 
for, while, switch. but i have no idea how we could hook it if we need to 
support catch.

i added a revision with likely/unlikely and the correct semantic (i think).

i also added the required checks. i didn't find a way to detect when we are in 
a catch block. but it should perhaps be allowded.


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

https://reviews.llvm.org/D59467

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/SemaCXX/cxx2a-likelihood-attr.cpp

Index: clang/test/SemaCXX/cxx2a-likelihood-attr.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/cxx2a-likelihood-attr.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++2a
+
+int f(int i) {
+  if (i == 1) [[unlikely]]
+{
+  return 0;
+}
+  else if (i == 2) [[likely]]
+return 1;
+  return 3;
+}
+
+[[likely]] typedef int n1; // expected-error {{'likely' attribute cannot be applied to a declaration}}
+typedef int [[likely]] n2; // expected-error {{'likely' attribute cannot be applied to types}}
+typedef int n3 [[likely]]; // expected-error {{'likely' attribute cannot be applied to a declaration}}
+
+enum [[likely]] E { // expected-error {{'likely' attribute cannot be applied to a declaration}}
+  One
+};
+
+[[likely]] // expected-error {{'likely' attribute cannot be applied to a declaration}}
+
+void test(int i) {
+  [[likely]] // expected-error {{'likely' attribute can only appear in if, while, switch and for}}
+if (1) [[likely, likely]] {
+  // expected-error@-1 {{there can only be one likely attribue in any attribute list}}
+  // expected-note@-2 {{previously used likely attribue}}
+  [[unlikely]] return ; // expected-error {{'unlikely' attribute can only appear in if, while, switch and for}}
+}
+  else [[unlikely]] if (1) {
+  while (1) [[likely]] {
+  switch (i) {
+[[likely]] case 1:
+  default: [[likely]]
+return ;
+  }
+}
+  for (;;) [[likely, unlikely]]
+  // expected-error@-1 {{unlikely and likely are mutually exclusive}}
+  // expected-note@-2 {{previously used likely attribue}}
+[[likely]] return ;
+  // expected-error@-1 {{likely and unlikely are mutually exclusive}}
+  // expected-note@-5 {{previously used unlikely attribue}}
+}
+  try { // expected-error {{cannot use 'try' with exceptions disabled}}
+[[likely]]; // expected-error {{'likely' attribute can only appear in if, while, switch and for}}
+  } catch (int) {
+  [[likely]] test: // expected-error {{'likely' attribute cannot be applied to a declaration}}
+  [[unlikely]] return ;
+  }
+}
\ No newline at end of file
Index: clang/lib/Sema/SemaStmtAttr.cpp
===
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -51,6 +51,31 @@
   return ::new (S.Context) auto(Attr);
 }
 
+static Attr *handleLikelihoodAttr(Sema &S, Stmt *St, const ParsedAttr &A,
+   SourceRange Range) {
+  LikelihoodAttr Attr(A.getRange(), S.Context,
+   A.getAttributeSpellingListIndex());
+
+  Scope* scope = S.getCurScope();
+  Scope* previousScope = nullptr;
+
+  if (scope)
+previousScope = scope->getParent();
+
+  //check that ths attribute is used in an if, while, for, switch or catch
+  if (!previousScope || 
+  !(previousScope->getFlags() & Scope::ControlScope) ||
+   previousScope->getFlags() & Scope::SEHExceptScope ||
+   previousScope->getFlags() & Scope::SEHTryScope ||
+   previousScope->getFlags() & Scope::FnTryCatchScope)
+ S.Diag(A.getLoc(), diag::err_likelihood_outside_control_scope) << A.getName();
+
+  if (!S.getLangOpts().CPlusPlus2a)
+S.Diag(A.getLoc(), diag::ext_cxx2a_attr) << A.getName();
+
+  return ::new (S.Context) auto(Attr);
+}
+
 static Attr *handleSuppressAttr(Sema &S, Stmt *St, const ParsedAttr &A,
 SourceRange Range) {
   if (A.getNumArgs() < 1) {
@@ -201,7 +226,21 @@
   } HintAttrs[] = {{nullptr, nullptr}, {nullptr, nullptr}, {nullptr, nullptr},
{nullptr, nullptr}, {nullptr, nullptr}, {nullptr, nullptr}};
 
+  //there can be only one likelyhood attribute
+  const Attr* likelihoodAttr = nullptr;
+
   for (const auto *I : Attrs) {
+if (llvm::isa(I)) {
+  if (likelihoodAttr) {
+if (std::strcmp(I->getSpelling(), likelihoodAttr->getSpelling()))
+  S.Diag(I->getLocation(), diag::err_mutuably_exclusive_likelihood) << I->getSpelling() << likelihoodAttr->getSpelling();
+else
+  S.Diag(I->getLocation(), diag::err_multiple_likelihood) << I->getSpelling();
+

[PATCH] D59494: AMDGPU: Add support for cross address space synchronization scopes

[Stanislav Mekhanoshin via Phabricator via cfe-commits]

rampitec added inline comments.



Comment at: lib/CodeGen/TargetInfo.cpp:7973
+  if (Ordering != llvm::AtomicOrdering::SequentiallyConsistent) {
+if (Scope != SyncScope::OpenCLAllSVMDevices)
+  Name = Twine(Twine(Name) + Twine("-")).str();

if (!Name.empty())



Comment at: lib/CodeGen/TargetInfo.cpp:7976
+
+Name = Twine(Twine(Name) + Twine("one-as")).str();
+  }

I think subgroup is in the single address space even if sequentially consistent.


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

https://reviews.llvm.org/D59494



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

[PATCH] D59474: [OpenMP 5.0] Codegen support for user-defined mappers

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Could you provide a little bit more comments what are you doing in this patch? 
It would be good t have a detailed description of the codegen scheme.




Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:2390
   }
+  case OMPRTL__tgt_target_data_mapper: {
+// Build void __tgt_target_data_mapper(int64_t device_id, int32_t arg_num,

You need to implement these mapper functions first.



Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:6769-6773
+  const OMPExecutableDirective *CurDir = nullptr;
+
+  /// The mapper directive from where the map clauses were extracted. One and
+  /// only one of CurDir and CurMapperDir is valid.
+  const OMPDeclareMapperDecl *CurMapperDir = nullptr;

Use `llvm::PointerUnion` to 
save the space.



Comment at: lib/CodeGen/CGOpenMPRuntime.h:351
+  /// is the asynchronous version.
+  llvm::DenseMap>

You should be very careful with this map. If the mapper is declared in the 
function context, it must be removed from this map as soon as the function 
processing is completed. All local declarations are removed after this and 
their address might be used again.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59474



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

[PATCH] D58160: MS ABI: adding template static member in the linker directive section to make sure init function can be called before main.

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

Apparently I wrote this comment long ago and never hit send.




Comment at: lib/CodeGen/CGDeclCXX.cpp:484
 AddGlobalCtor(Fn, 65535, COMDATKey);
+if (getTarget().getCXXABI().isMicrosoft()) {
+  // In The MS C++, MS add template static data member in the linker

We might really want isOSBinFormatCOFF, since I think the same issue affects 
Windows Itanium with -OPT:REF and LLD.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58160



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

[PATCH] D59440: add steps to preprocess file and reduce command line args

[Amy Huang via Phabricator via cfe-commits]

akhuang updated this revision to Diff 191128.
akhuang added a comment.

fix some typos


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

https://reviews.llvm.org/D59440

Files:
  clang/utils/creduce-clang-crash.py

Index: clang/utils/creduce-clang-crash.py
===
--- clang/utils/creduce-clang-crash.py
+++ clang/utils/creduce-clang-crash.py
@@ -1,7 +1,5 @@
 #!/usr/bin/env python
 """Calls C-Reduce to create a minimal reproducer for clang crashes.
-
-Requires C-Reduce and not (part of LLVM utils) to be installed.
 """
 
 from argparse import ArgumentParser
@@ -11,103 +9,243 @@
 import sys
 import subprocess
 import pipes
+import shlex
+import tempfile
+import shutil
 from distutils.spawn import find_executable
 
-def create_test(build_script, llvm_not):
+verbose = False
+llvm_bin = None
+creduce_cmd = None
+not_cmd = None
+
+def check_file(fname):
+  fname = os.path.abspath(fname)
+  if not os.path.isfile(fname):
+sys.exit("ERROR: %s does not exist" % (fname))
+  return fname
+
+def check_cmd(cmd_name, cmd_dir, cmd_path=None):
   """
-  Create an interestingness test from the crash output.
-  Return as a string.
+  Returns absolute path to cmd_path if it is given,
+  or absolute path to cmd_dir/cmd_name.
   """
-  # Get clang call from build script
-  # Assumes the call is the last line of the script
-  with open(build_script) as f:
-cmd = f.readlines()[-1].rstrip('\n\r')
+  if cmd_path:
+cmd = find_executable(cmd_path)
+if cmd:
+  return cmd
+sys.exit("ERROR: executable %s not found" % (cmd_path))
+
+  cmd = find_executable(cmd_name, path=cmd_dir)
+  if cmd:
+return cmd
+  sys.exit("ERROR: %s not found in %s" % (cmd_name, cmd_dir))
+
+def quote_cmd(cmd):
+  return ' '.join(pipes.quote(s) for s in cmd)
+
+def get_crash_cmd(crash_script):
+  with open(crash_script) as f:
+# Assume clang call is on the last line of the script
+line = f.readlines()[-1]
+cmd = shlex.split(line)
 
-  # Get crash output
-  p = subprocess.Popen(build_script,
+# Overwrite the script's clang with the user's clang path
+clang_name = os.path.basename(cmd[0])
+new_clang = check_cmd(clang_name, llvm_bin)
+cmd[0] = pipes.quote(new_clang)
+return cmd
+
+def has_expected_output(crash_cmd, expected_output):
+  p = subprocess.Popen(crash_cmd,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT)
   crash_output, _ = p.communicate()
+  for msg in expected_output:
+if msg not in crash_output:
+  return False
+  return True
 
-  output = ['#!/bin/bash']
-  output.append('%s --crash %s >& t.log || exit 1' % (pipes.quote(llvm_not),
-  cmd))
+def get_expected_output(crash_cmd):
+  p = subprocess.Popen(crash_cmd,
+   stdout=subprocess.PIPE,
+   stderr=subprocess.STDOUT)
+  crash_output, _ = p.communicate()
 
-  # Add messages from crash output to the test
-  # If there is an Assertion failure, use that; otherwise use the
-  # last five stack trace functions
+  # If there is an assertion failure, use that;
+  # otherwise use the last five stack trace functions
   assertion_re = r'Assertion `([^\']+)\' failed'
   assertion_match = re.search(assertion_re, crash_output)
   if assertion_match:
-msg = assertion_match.group(1)
-output.append('grep %s t.log || exit 1' % pipes.quote(msg))
+return [assertion_match.group(1)]
   else:
 stacktrace_re = r'#[0-9]+\s+0[xX][0-9a-fA-F]+\s*([^(]+)\('
 matches = re.findall(stacktrace_re, crash_output)
-del matches[:-5]
-output += ['grep %s t.log || exit 1' % pipes.quote(msg) for msg in matches]
+return matches[-5:]
+
+def write_interestingness_test(testfile, crash_cmd, expected_output):
+  output = ['#!/bin/bash']
+  output.append('%s --crash %s >& t.log || exit 1' % (pipes.quote(not_cmd),
+  quote_cmd(crash_cmd)))
+
+  for msg in expected_output:
+output.append('grep %s t.log || exit 1' % pipes.quote(msg))
+
+  with open(testfile, 'w') as f:
+f.write('\n'.join(output))
+  os.chmod(testfile, os.stat(testfile).st_mode | stat.S_IEXEC)
+
+def check_interestingness(testfile, file_to_reduce):
+  # Check that the test considers the original file interesting
+  with open(os.devnull, 'w') as devnull:
+p = subprocess.Popen(testfile, stdout=devnull)
+p.communicate()
+  if p.returncode:
+sys.exit("The interestingness test does not pass for the original file.")
+
+  # Check that an empty file is not interesting
+  # file_to_reduce is hardcoded into the test, so this is a roundabout
+  # way to run it on an empty file
+  _, tmpfile = tempfile.mkstemp()
+  _, empty_file = tempfile.mkstemp()
+  shutil.copy(file_to_reduce, tmpfile)
+  shutil.copy(empty_file, file_to_reduce)
+  with open(os.devnull, 'w') as devnull:
+p = subprocess.Popen(testfile, stdo

r356385 - [AMDGPU] Add the missing clang change of the experimental buffer fat pointer

[Michael Liao via cfe-commits]

Author: hliao
Date: Mon Mar 18 11:11:37 2019
New Revision: 356385

URL: http://llvm.org/viewvc/llvm-project?rev=356385&view=rev
Log:
[AMDGPU] Add the missing clang change of the experimental buffer fat pointer

Modified:
cfe/trunk/lib/Basic/Targets/AMDGPU.cpp
cfe/trunk/test/CodeGen/target-data.c
cfe/trunk/test/CodeGenOpenCL/amdgpu-env-amdgcn.cl

Modified: cfe/trunk/lib/Basic/Targets/AMDGPU.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets/AMDGPU.cpp?rev=356385&r1=356384&r2=356385&view=diff
==
--- cfe/trunk/lib/Basic/Targets/AMDGPU.cpp (original)
+++ cfe/trunk/lib/Basic/Targets/AMDGPU.cpp Mon Mar 18 11:11:37 2019
@@ -34,7 +34,8 @@ static const char *const DataLayoutStrin
 static const char *const DataLayoutStringAMDGCN =
 "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32"
 "-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128"
-"-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5";
+"-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5"
+"-ni:7";
 
 const LangASMap AMDGPUTargetInfo::AMDGPUDefIsGenMap = {
 Generic,  // Default

Modified: cfe/trunk/test/CodeGen/target-data.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/target-data.c?rev=356385&r1=356384&r2=356385&view=diff
==
--- cfe/trunk/test/CodeGen/target-data.c (original)
+++ cfe/trunk/test/CodeGen/target-data.c Mon Mar 18 11:11:37 2019
@@ -152,12 +152,12 @@
 
 // RUN: %clang_cc1 -triple amdgcn-unknown -target-cpu hawaii -o - -emit-llvm 
%s \
 // RUN: | FileCheck %s -check-prefix=R600SI
-// R600SI: target datalayout = 
"e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5"
+// R600SI: target datalayout = 
"e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-ni:7"
 
 // Test default -target-cpu
 // RUN: %clang_cc1 -triple amdgcn-unknown -o - -emit-llvm %s \
 // RUN: | FileCheck %s -check-prefix=R600SIDefault
-// R600SIDefault: target datalayout = 
"e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5"
+// R600SIDefault: target datalayout = 
"e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-ni:7"
 
 // RUN: %clang_cc1 -triple arm64-unknown -o - -emit-llvm %s | \
 // RUN: FileCheck %s -check-prefix=AARCH64

Modified: cfe/trunk/test/CodeGenOpenCL/amdgpu-env-amdgcn.cl
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenOpenCL/amdgpu-env-amdgcn.cl?rev=356385&r1=356384&r2=356385&view=diff
==
--- cfe/trunk/test/CodeGenOpenCL/amdgpu-env-amdgcn.cl (original)
+++ cfe/trunk/test/CodeGenOpenCL/amdgpu-env-amdgcn.cl Mon Mar 18 11:11:37 2019
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 %s -O0 -triple amdgcn -emit-llvm -o - | FileCheck %s
 // RUN: %clang_cc1 %s -O0 -triple amdgcn---opencl -emit-llvm -o - | FileCheck 
%s
 
-// CHECK: target datalayout = 
"e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5"
+// CHECK: target datalayout = 
"e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-ni:7"
 void foo(void) {}


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

[PATCH] D59486: [OpenCL] Improve testing of default header in C++ mode

[Alexey Bader via Phabricator via cfe-commits]

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

OpenCL C++ part looks good. Thanks!




Comment at: test/Headers/opencl-c-header.cl:57-65
 char f(char x) {
-#if __OPENCL_C_VERSION__ != CL_VERSION_2_0
+#if !defined(__OPENCL_CPP_VERSION__) && (__OPENCL_C_VERSION__ != 
CL_VERSION_2_0)
   return convert_char_rte(x);
 
 #else //__OPENCL_C_VERSION__
   ndrange_t t;
   return ctz(x);

This test looks strange.
It checks that convert_char_rte is compiled if OpenCL version is not 2.0, but 
AFAIK it's not deprecated in OpenCL C 2.0, so I don't know why test is written 
in this way (i.e. ifdef-else-endif).

I think checks should look like this:
```
char f(char x) {
// Check check OpenCL C 2.0 and OpenCL C++ functionality 
#if defined(__OPENCL_CPP_VERSION__) || (__OPENCL_C_VERSION__ == CL_VERSION_2_0)
  ndrange_t t;
  x = ctz(x);
#endif
  return convert_char_rte(x);
}
```

Probably it's better to fix separately.


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

https://reviews.llvm.org/D59486



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

[PATCH] D58160: MS ABI: adding template static member in the linker directive section to make sure init function can be called before main.

[Reid Kleckner via Phabricator via cfe-commits]

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

lgtm




Comment at: lib/CodeGen/CGDeclCXX.cpp:484
 AddGlobalCtor(Fn, 65535, COMDATKey);
+if (getTarget().getCXXABI().isMicrosoft()) {
+  // In The MS C++, MS add template static data member in the linker

rnk wrote:
> We might really want isOSBinFormatCOFF, since I think the same issue affects 
> Windows Itanium with -OPT:REF and LLD.
On reflection, it's probably best not to mess with mingw for now. Let's just 
commit this as is.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58160



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

r356388 - [OPENMP] Set scheduling for doacross loops as schedule, 1.

[Alexey Bataev via cfe-commits]

Author: abataev
Date: Mon Mar 18 11:40:00 2019
New Revision: 356388

URL: http://llvm.org/viewvc/llvm-project?rev=356388&view=rev
Log:
[OPENMP] Set scheduling for doacross loops as schedule, 1.

The default scheduling for doacross loops is changed from static to
static, 1.

Modified:
cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
cfe/trunk/lib/CodeGen/CGOpenMPRuntime.h
cfe/trunk/test/OpenMP/ordered_doacross_codegen.c
cfe/trunk/test/OpenMP/ordered_doacross_codegen.cpp

Modified: cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp?rev=356388&r1=356387&r2=356388&view=diff
==
--- cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp Mon Mar 18 11:40:00 2019
@@ -3270,6 +3270,24 @@ unsigned CGOpenMPRuntime::getDefaultFlag
   return Flags;
 }
 
+void CGOpenMPRuntime::getDefaultScheduleAndChunk(
+CodeGenFunction &CGF, const OMPLoopDirective &S,
+OpenMPScheduleClauseKind &ScheduleKind, const Expr *&ChunkExpr) const {
+  // Check if the loop directive is actually a doacross loop directive. In this
+  // case choose static, 1 schedule.
+  if (llvm::any_of(
+  S.getClausesOfKind(),
+  [](const OMPOrderedClause *C) { return C->getNumForLoops(); })) {
+ScheduleKind = OMPC_SCHEDULE_static;
+// Chunk size is 1 in this case.
+llvm::APInt ChunkSize(32, 1);
+ChunkExpr = IntegerLiteral::Create(
+CGF.getContext(), ChunkSize,
+CGF.getContext().getIntTypeForBitwidth(32, /*Signed=*/0),
+SourceLocation());
+  }
+}
+
 void CGOpenMPRuntime::emitBarrierCall(CodeGenFunction &CGF, SourceLocation Loc,
   OpenMPDirectiveKind Kind, bool 
EmitChecks,
   bool ForceSimpleCall) {

Modified: cfe/trunk/lib/CodeGen/CGOpenMPRuntime.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGOpenMPRuntime.h?rev=356388&r1=356387&r2=356388&view=diff
==
--- cfe/trunk/lib/CodeGen/CGOpenMPRuntime.h (original)
+++ cfe/trunk/lib/CodeGen/CGOpenMPRuntime.h Mon Mar 18 11:40:00 2019
@@ -1565,7 +1565,7 @@ public:
   /// schedule clause.
   virtual void getDefaultScheduleAndChunk(CodeGenFunction &CGF,
   const OMPLoopDirective &S, OpenMPScheduleClauseKind &ScheduleKind,
-  const Expr *&ChunkExpr) const {}
+  const Expr *&ChunkExpr) const;
 
   /// Emits call of the outlined function with the provided arguments,
   /// translating these arguments to correct target-specific arguments.

Modified: cfe/trunk/test/OpenMP/ordered_doacross_codegen.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/ordered_doacross_codegen.c?rev=356388&r1=356387&r2=356388&view=diff
==
--- cfe/trunk/test/OpenMP/ordered_doacross_codegen.c (original)
+++ cfe/trunk/test/OpenMP/ordered_doacross_codegen.c Mon Mar 18 11:40:00 2019
@@ -33,7 +33,7 @@ int main() {
 // CHECK: [[DIM:%.+]] = getelementptr inbounds [1 x [[KMP_DIM]]], [1 x 
[[KMP_DIM]]]* [[DIMS]], i64 0, i64 0
 // CHECK: [[CAST:%.+]] = bitcast [[KMP_DIM]]* [[DIM]] to i8*
 // CHECK: call void @__kmpc_doacross_init([[IDENT]], i32 [[GTID]], i32 1, i8* 
[[CAST]])
-// CHECK: call void @__kmpc_for_static_init_4(
+// CHECK: call void @__kmpc_for_static_init_4(%struct.ident_t* @{{.+}}, i32 
[[GTID]], i32 33, i32* %{{.+}}, i32* %{{.+}}, i32* %{{.+}}, i32* %{{.+}}, i32 
1, i32 1)
 #pragma omp for ordered(1)
   for (i = 0; i < n; ++i) {
 a[i] = b[i] + 1;

Modified: cfe/trunk/test/OpenMP/ordered_doacross_codegen.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/ordered_doacross_codegen.cpp?rev=356388&r1=356387&r2=356388&view=diff
==
--- cfe/trunk/test/OpenMP/ordered_doacross_codegen.cpp (original)
+++ cfe/trunk/test/OpenMP/ordered_doacross_codegen.cpp Mon Mar 18 11:40:00 2019
@@ -44,7 +44,7 @@ int main() {
 // CHECK: [[DIM:%.+]] = getelementptr inbounds [1 x [[KMP_DIM]]], [1 x 
[[KMP_DIM]]]* [[DIMS]], i64 0, i64 0
 // CHECK: [[CAST:%.+]] = bitcast [[KMP_DIM]]* [[DIM]] to i8*
 // CHECK: call void @__kmpc_doacross_init([[IDENT]], i32 [[GTID]], i32 1, i8* 
[[CAST]])
-// CHECK: call void @__kmpc_for_static_init_4(
+// CHECK: call void @__kmpc_for_static_init_4(%struct.ident_t* @{{.+}}, i32 
[[GTID]], i32 33, i32* %{{.+}}, i32* %{{.+}}, i32* %{{.+}}, i32* %{{.+}}, i32 
1, i32 1)
 #pragma omp for ordered(1)
   for (int i = 0; i < n; ++i) {
 a[i] = b[i] + 1;
@@ -113,7 +113,7 @@ struct TestStruct {
 // CHECK: [[DIM:%.+]] = getelementptr inbounds [2 x [[KMP_DIM]]], [2 x 
[[KMP_DIM]]]* [[DIMS]], i64 0, i64 0
 // CHECK: [[CAST:%.+]] = bitcast [[KMP_DIM]]* [[DIM]] to i8*
 // CHECK: call void @__kmpc_doacross_init([[IDENT]], i32 [[GTID]], 

Re: r311065 - Further refactoring of the constant emitter. NFC.

[Don Hinton via cfe-commits]

It looks like this change introduced a small bug;  Specifically, the
following cast test:

-  if (auto PT = dyn_cast(DestTy)) {
...
+  // If we're producing a pointer, this is easy.
+  if (auto destPtrTy = cast(destTy)) {

Since the cast can fail, shouldn't you prefer dyn_cast<>(), which can
return nullptr, over cast<>(), which will assert?


On Wed, Aug 16, 2017 at 11:04 PM John McCall via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: rjmccall
> Date: Wed Aug 16 22:03:55 2017
> New Revision: 311065
>
> URL: http://llvm.org/viewvc/llvm-project?rev=311065&view=rev
> Log:
> Further refactoring of the constant emitter.  NFC.
>
> Modified:
> cfe/trunk/lib/CodeGen/CGExprConstant.cpp
>
> Modified: cfe/trunk/lib/CodeGen/CGExprConstant.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprConstant.cpp?rev=311065&r1=311064&r2=311065&view=diff
>
> ==
> --- cfe/trunk/lib/CodeGen/CGExprConstant.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGExprConstant.cpp Wed Aug 16 22:03:55 2017
> @@ -1044,120 +1044,6 @@ public:
>llvm::Type *ConvertType(QualType T) {
>  return CGM.getTypes().ConvertType(T);
>}
> -
> -public:
> -  ConstantAddress EmitLValue(APValue::LValueBase LVBase) {
> -if (const ValueDecl *Decl = LVBase.dyn_cast()) {
> -  if (Decl->hasAttr())
> -return CGM.GetWeakRefReference(Decl);
> -  if (const FunctionDecl *FD = dyn_cast(Decl))
> -return ConstantAddress(CGM.GetAddrOfFunction(FD),
> CharUnits::One());
> -  if (const VarDecl* VD = dyn_cast(Decl)) {
> -// We can never refer to a variable with local storage.
> -if (!VD->hasLocalStorage()) {
> -  CharUnits Align = CGM.getContext().getDeclAlign(VD);
> -  if (VD->isFileVarDecl() || VD->hasExternalStorage())
> -return ConstantAddress(CGM.GetAddrOfGlobalVar(VD), Align);
> -  else if (VD->isLocalVarDecl()) {
> -auto Ptr = CGM.getOrCreateStaticVarDecl(
> -*VD, CGM.getLLVMLinkageVarDefinition(VD,
> /*isConstant=*/false));
> -return ConstantAddress(Ptr, Align);
> -  }
> -}
> -  }
> -  return ConstantAddress::invalid();
> -}
> -
> -Expr *E = const_cast(LVBase.get());
> -switch (E->getStmtClass()) {
> -default: break;
> -case Expr::CompoundLiteralExprClass:
> -  return tryEmitGlobalCompoundLiteral(CGM, Emitter.CGF,
> -  cast(E));
> -case Expr::StringLiteralClass:
> -  return
> CGM.GetAddrOfConstantStringFromLiteral(cast(E));
> -case Expr::ObjCEncodeExprClass:
> -  return
> CGM.GetAddrOfConstantStringFromObjCEncode(cast(E));
> -case Expr::ObjCStringLiteralClass: {
> -  ObjCStringLiteral* SL = cast(E);
> -  ConstantAddress C =
> -  CGM.getObjCRuntime().GenerateConstantString(SL->getString());
> -  return C.getElementBitCast(ConvertType(E->getType()));
> -}
> -case Expr::PredefinedExprClass: {
> -  unsigned Type = cast(E)->getIdentType();
> -  if (auto CGF = Emitter.CGF) {
> -LValue Res = CGF->EmitPredefinedLValue(cast(E));
> -return cast(Res.getAddress());
> -  } else if (Type == PredefinedExpr::PrettyFunction) {
> -return CGM.GetAddrOfConstantCString("top level", ".tmp");
> -  }
> -
> -  return CGM.GetAddrOfConstantCString("", ".tmp");
> -}
> -case Expr::AddrLabelExprClass: {
> -  assert(Emitter.CGF &&
> - "Invalid address of label expression outside function.");
> -  llvm::Constant *Ptr =
> -Emitter.CGF->GetAddrOfLabel(cast(E)->getLabel());
> -  Ptr = llvm::ConstantExpr::getBitCast(Ptr,
> ConvertType(E->getType()));
> -  return ConstantAddress(Ptr, CharUnits::One());
> -}
> -case Expr::CallExprClass: {
> -  CallExpr* CE = cast(E);
> -  unsigned builtin = CE->getBuiltinCallee();
> -  if (builtin !=
> -Builtin::BI__builtin___CFStringMakeConstantString &&
> -  builtin !=
> -Builtin::BI__builtin___NSStringMakeConstantString)
> -break;
> -  const Expr *Arg = CE->getArg(0)->IgnoreParenCasts();
> -  const StringLiteral *Literal = cast(Arg);
> -  if (builtin ==
> -Builtin::BI__builtin___NSStringMakeConstantString) {
> -return CGM.getObjCRuntime().GenerateConstantString(Literal);
> -  }
> -  // FIXME: need to deal with UCN conversion issues.
> -  return CGM.GetAddrOfConstantCFString(Literal);
> -}
> -case Expr::BlockExprClass: {
> -  StringRef FunctionName;
> -  if (auto CGF = Emitter.CGF)
> -FunctionName = CGF->CurFn->getName();
> -  else
> -FunctionName = "global";
> -
> -  // This is not really an l-value.
> -  llvm::Constant *Ptr =
> -CGM.GetAddrOfGlobalBlock(cast(E), FunctionName);
> -  return ConstantAddress(Ptr, CGM.getPointerAlign());
> -}
> -

[PATCH] D59394: [Sema] De-duplicate some availability checking logic

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington requested review of this revision.
erik.pilkington added a comment.

In D59394#1430221 , @ldionne wrote:

> I can confirm this fixed my original problem. It's also close to the patch I 
> attempted writing myself earlier this week -- but this one is better of 
> course.


Okay, thanks for testing this out! @rsmith/@aaron.ballman would you mind 
looking at this one too?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59394



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

[PATCH] D59440: add steps to preprocess file and reduce command line args

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: clang/utils/creduce-clang-crash.py:106-117
+  # Check that an empty file is not interesting
+  # file_to_reduce is hardcoded into the test, so this is a roundabout
+  # way to run it on an empty file
+  _, tmpfile = tempfile.mkstemp()
+  _, empty_file = tempfile.mkstemp()
+  shutil.copy(file_to_reduce, tmpfile)
+  shutil.copy(empty_file, file_to_reduce)

Another way to do this without the complexity of swapping the files around is 
to construct the interestingness test to take the path to the file as a 
parameter (`$1`). CReduce always passes the path to the file being reduced as 
the first argument.


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

https://reviews.llvm.org/D59440



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

Re: r311065 - Further refactoring of the constant emitter. NFC.

[John McCall via cfe-commits]

On 18 Mar 2019, at 14:39, Don Hinton wrote:

It looks like this change introduced a small bug;  Specifically, the
following cast test:

-  if (auto PT = dyn_cast(DestTy)) {
...
+  // If we're producing a pointer, this is easy.
+  if (auto destPtrTy = cast(destTy)) {

Since the cast can fail, shouldn't you prefer dyn_cast<>(), which can
return nullptr, over cast<>(), which will assert?


Yes, although if it hasn't caused a problem in the last year and a half, 
maybe we should just change the code to be non-conditional.


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

r356397 - [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Erik Pilkington via cfe-commits]

Author: epilk
Date: Mon Mar 18 12:23:45 2019
New Revision: 356397

URL: http://llvm.org/viewvc/llvm-project?rev=356397&view=rev
Log:
[Sema] Add some compile time _FORTIFY_SOURCE diagnostics

These diagnose overflowing calls to subset of fortifiable functions. Some
functions, like sprintf or strcpy aren't supported right not, but we should
probably support these in the future. We previously supported this kind of
functionality with -Wbuiltin-memcpy-chk-size, but that diagnostic doesn't work
with _FORTIFY implementations that use wrapper functions. Also unlike that
diagnostic, we emit these warnings regardless of whether _FORTIFY_SOURCE is
actually enabled, which is nice for programs that don't enable the runtime
checks.

Why not just use diagnose_if, like Bionic does? We can get better diagnostics in
the compiler (i.e. mention the sizes), and we have the potential to diagnose
sprintf and strcpy which is impossible with diagnose_if (at least, in languages
that don't support C++14 constexpr). This approach also saves standard libraries
from having to add diagnose_if.

rdar://48006655

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

Added:
cfe/trunk/test/Sema/warn-fortify-source.c
Modified:
cfe/trunk/include/clang/AST/Decl.h
cfe/trunk/include/clang/Basic/DiagnosticGroups.td
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/AST/Decl.cpp
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/lib/Sema/SemaExpr.cpp
cfe/trunk/test/Analysis/bstring.c
cfe/trunk/test/Analysis/null-deref-ps-region.c
cfe/trunk/test/Analysis/pr22954.c
cfe/trunk/test/Analysis/string.c
cfe/trunk/test/Sema/builtin-object-size.c
cfe/trunk/test/Sema/builtins.c
cfe/trunk/test/Sema/transpose-memset.c
cfe/trunk/test/Sema/warn-strncat-size.c

Modified: cfe/trunk/include/clang/AST/Decl.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=356397&r1=356396&r2=356397&view=diff
==
--- cfe/trunk/include/clang/AST/Decl.h (original)
+++ cfe/trunk/include/clang/AST/Decl.h Mon Mar 18 12:23:45 2019
@@ -2255,7 +2255,7 @@ public:
 return const_cast(this)->getCanonicalDecl();
   }
 
-  unsigned getBuiltinID() const;
+  unsigned getBuiltinID(bool ConsiderWrapperFunctions = false) const;
 
   // ArrayRef interface to parameters.
   ArrayRef parameters() const {

Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=356397&r1=356396&r2=356397&view=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Mon Mar 18 12:23:45 2019
@@ -1054,3 +1054,5 @@ def NoDeref : DiagGroup<"noderef">;
 def CrossTU : DiagGroup<"ctu">;
 
 def CTADMaybeUnsupported : DiagGroup<"ctad-maybe-unsupported">;
+
+def FortifySource : DiagGroup<"fortify-source">;

Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=356397&r1=356396&r2=356397&view=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Mar 18 12:23:45 
2019
@@ -672,11 +672,17 @@ def warn_assume_side_effects : Warning<
   "the argument to %0 has side effects that will be discarded">,
   InGroup>;
 
-def warn_memcpy_chk_overflow : Warning<
+def warn_builtin_chk_overflow : Warning<
   "'%0' will always overflow; destination buffer has size %1,"
   " but size argument is %2">,
   InGroup>;
 
+def warn_fortify_source_overflow
+  : Warning, InGroup;
+def warn_fortify_source_size_mismatch : Warning<
+  "'%0' size argument is too large; destination buffer has size %1,"
+  " but size argument is %2">, InGroup;
+
 /// main()
 // static main() is not an error in C, just in C++.
 def warn_static_main : Warning<"'main' should not be declared static">,

Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=356397&r1=356396&r2=356397&view=diff
==
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Mon Mar 18 12:23:45 2019
@@ -10677,6 +10677,7 @@ private:
 
   ExprResult CheckBuiltinFunctionCall(FunctionDecl *FDecl,
   unsigned BuiltinID, CallExpr *TheCall);
+  void checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, CallExpr 
*TheCall);
 
   bool CheckARMBuiltinExclusiveCall(unsigned BuiltinID, CallExpr *TheCall,
 unsigned MaxWidth);

Modified: cfe/trunk/li

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC356397: [Sema] Add some compile time _FORTIFY_SOURCE 
diagnostics (authored by epilk, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D58797?vs=190549&id=191142#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D58797

Files:
  include/clang/AST/Decl.h
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/AST/Decl.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaExpr.cpp
  test/Analysis/bstring.c
  test/Analysis/null-deref-ps-region.c
  test/Analysis/pr22954.c
  test/Analysis/string.c
  test/Sema/builtin-object-size.c
  test/Sema/builtins.c
  test/Sema/transpose-memset.c
  test/Sema/warn-fortify-source.c
  test/Sema/warn-strncat-size.c

Index: include/clang/AST/Decl.h
===
--- include/clang/AST/Decl.h
+++ include/clang/AST/Decl.h
@@ -2255,7 +2255,7 @@
 return const_cast(this)->getCanonicalDecl();
   }
 
-  unsigned getBuiltinID() const;
+  unsigned getBuiltinID(bool ConsiderWrapperFunctions = false) const;
 
   // ArrayRef interface to parameters.
   ArrayRef parameters() const {
Index: include/clang/Sema/Sema.h
===
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -10677,6 +10677,7 @@
 
   ExprResult CheckBuiltinFunctionCall(FunctionDecl *FDecl,
   unsigned BuiltinID, CallExpr *TheCall);
+  void checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD, CallExpr *TheCall);
 
   bool CheckARMBuiltinExclusiveCall(unsigned BuiltinID, CallExpr *TheCall,
 unsigned MaxWidth);
Index: include/clang/Basic/DiagnosticGroups.td
===
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -1054,3 +1054,5 @@
 def CrossTU : DiagGroup<"ctu">;
 
 def CTADMaybeUnsupported : DiagGroup<"ctad-maybe-unsupported">;
+
+def FortifySource : DiagGroup<"fortify-source">;
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -672,11 +672,17 @@
   "the argument to %0 has side effects that will be discarded">,
   InGroup>;
 
-def warn_memcpy_chk_overflow : Warning<
+def warn_builtin_chk_overflow : Warning<
   "'%0' will always overflow; destination buffer has size %1,"
   " but size argument is %2">,
   InGroup>;
 
+def warn_fortify_source_overflow
+  : Warning, InGroup;
+def warn_fortify_source_size_mismatch : Warning<
+  "'%0' size argument is too large; destination buffer has size %1,"
+  " but size argument is %2">, InGroup;
+
 /// main()
 // static main() is not an error in C, just in C++.
 def warn_static_main : Warning<"'main' should not be declared static">,
Index: test/Sema/builtins.c
===
--- test/Sema/builtins.c
+++ test/Sema/builtins.c
@@ -230,14 +230,14 @@
 // expected-note {{change size argument to be the size of the destination}}
 __builtin___strlcpy_chk(buf, b, sizeof(b), __builtin_object_size(buf, 0)); // expected-warning {{size argument in '__builtin___strlcpy_chk' call appears to be size of the source; expected the size of the destination}} \
 // expected-note {{change size argument to be the size of the destination}} \
-// expected-warning {{'__builtin___strlcpy_chk' will always overflow; destination buffer has size 20, but size argument is 40}}
+// expected-warning {{'strlcpy' will always overflow; destination buffer has size 20, but size argument is 40}}
 
 strlcat(buf, b, sizeof(b)); // expected-warning {{size argument in 'strlcat' call appears to be size of the source; expected the size of the destination}} \
 // expected-note {{change size argument to be the size of the destination}}
 
 __builtin___strlcat_chk(buf, b, sizeof(b), __builtin_object_size(buf, 0)); // expected-warning {{size argument in '__builtin___strlcat_chk' call appears to be size of the source; expected the size of the destination}} \
// expected-note {{change size argument to be the size of the destination}} \
-   // expected-warning {{'__builtin___strlcat_chk' will always overflow; destination buffer has size 20, but size argument is 40}}
+   // expected-warning {{'strlcat' will always overflow; destination buffer has size 20, but size a

Re: r311065 - Further refactoring of the constant emitter. NFC.

[Don Hinton via cfe-commits]

Hi John:

I found this investigating the cast assert noted here:
http://lists.llvm.org/pipermail/cfe-dev/2019-March/061685.html

I subsequently did quick grep and found a number of cases in clang+llvm
(didn't find any in other projects) .  I'm happy to fix these in mass,
i.e., s/cast/dyn_cast/, but as you mentioned, some might require additional
refactoring, e.g., removal of the conditional.

In any case, here's what I've found -- perhaps a new llvm clang-tidy
checker would be useful:

$ find ./clang ./llvm -type f \( -name "*.h" -o -name "*.cpp" \) -exec grep
-Hn "if *(auto.* *= *cast *<" \{\} \;
./clang/lib/Sema/SemaOpenMP.cpp:10904:  else if (auto *DRD =
cast(D))
./clang/lib/CodeGen/CGExprConstant.cpp:1701:  if (auto destPtrTy =
cast(destTy)) {
./clang/lib/CodeGen/MicrosoftCXXABI.cpp:738:  if (auto *Fn =
cast(Throw.getCallee()))
./clang/lib/AST/ASTImporter.cpp:8463:  if (auto *FromDC =
cast(From)) {
./clang/lib/AST/DeclBase.cpp:1182:if (auto *Def =
cast(this)->getDefinition())
./clang/lib/AST/DeclBase.cpp:1187:if (auto *Def =
cast(this)->getDefinition())
./llvm/tools/llvm-objdump/llvm-objdump.cpp:802:  if (auto *Elf64BEObj =
cast(Obj))
./llvm/tools/llvm-objdump/llvm-objdump.cpp:846:  else if (auto *Elf64BEObj
= cast(Obj))
./llvm/lib/Target/X86/AsmParser/X86Operand.h:102:if (auto Imm =
cast(Val)->getValue())
./llvm/lib/Transforms/IPO/CalledValuePropagation.cpp:139:  } else if
(auto *F = cast(Key.getPointer()))
./llvm/lib/Transforms/Coroutines/CoroEarly.cpp:183:if (auto *CII =
cast(&I)) {

thanks...
don

On Mon, Mar 18, 2019 at 12:07 PM John McCall  wrote:

> On 18 Mar 2019, at 14:39, Don Hinton wrote:
> > It looks like this change introduced a small bug;  Specifically, the
> > following cast test:
> >
> > -  if (auto PT = dyn_cast(DestTy)) {
> > ...
> > +  // If we're producing a pointer, this is easy.
> > +  if (auto destPtrTy = cast(destTy)) {
> >
> > Since the cast can fail, shouldn't you prefer dyn_cast<>(), which can
> > return nullptr, over cast<>(), which will assert?
>
> Yes, although if it hasn't caused a problem in the last year and a half,
> maybe we should just change the code to be non-conditional.
>
> John.
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58548: IR: Support parsing numeric block ids, and emit them in textual output.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Sorry, forgot to re-ping this in a timely manner. :)

The discussion on mailing list concluded positively, so waiting for an LG here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58548



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

[PATCH] D59346: [X86] Add gcc rotate intrinsics to ia32intrin.h

[James Y Knight via Phabricator via cfe-commits]

jyknight accepted this revision.
jyknight added a comment.

Looks good.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59346



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

[PATCH] D59448: [WebAssembly] Change wasm.throw's first argument to an immediate

[Derek Schuff via Phabricator via cfe-commits]

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

LGTM; I wonder if it makes sense to have predefined macro for the C++ tag (or 
perhaps just a regular macro for use in libcxxabi?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D59448



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

[PATCH] D59448: [WebAssembly] Change wasm.throw's first argument to an immediate

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

Shouldn't the definition in BuiltinsWebAssembly.def be updated to include an 
'I' in the type string so that this will be properly diagnosed in the frontend?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59448



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

[PATCH] D59455: Thread safety analysis: Add note for unlock kind mismatch

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM aside from a small nit.




Comment at: include/clang/Analysis/Analyses/ThreadSafety.h:127
+ SourceLocation LocLocked,
  SourceLocation Loc) {}
 

Can you rename this one to `LocUnlocked` to mirror the new parameter?



Comment at: lib/Sema/AnalysisBasedWarnings.cpp:1690
+ SourceLocation LocLocked,
  SourceLocation Loc) override {
 if (Loc.isInvalid())

Same suggestion here.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59455



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

[PATCH] D54978: Move the SMT API to LLVM

[Mikhail Ramalho via Phabricator via cfe-commits]

mikhail.ramalho updated this revision to Diff 191152.
mikhail.ramalho added a comment.

Updated script to parse Z3's headers and changed the workflow to handle cross 
compilation cases.


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

https://reviews.llvm.org/D54978

Files:
  clang/CMakeLists.txt
  clang/cmake/modules/FindZ3.cmake
  clang/include/clang/Config/config.h.cmake
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTAPI.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/CMakeLists.txt
  clang/lib/StaticAnalyzer/Core/SMTConstraintManager.cpp
  clang/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
  clang/test/CMakeLists.txt
  clang/test/lit.site.cfg.py.in
  llvm/CMakeLists.txt
  llvm/cmake/modules/FindZ3.cmake
  llvm/cmake/modules/LLVMConfig.cmake.in
  llvm/include/llvm/Config/config.h.cmake
  llvm/include/llvm/Support/SMTAPI.h
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/Z3Solver.cpp

Index: llvm/lib/Support/Z3Solver.cpp
===
--- llvm/lib/Support/Z3Solver.cpp
+++ llvm/lib/Support/Z3Solver.cpp
@@ -1,4 +1,4 @@
-//== Z3ConstraintManager.cpp *- C++ -*--==//
+//== Z3Solver.cpp ---*- C++ -*--==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -6,18 +6,14 @@
 //
 //===--===//
 
-#include "clang/Basic/TargetInfo.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h"
+#include "llvm/ADT/Twine.h"
+#include "llvm/Config/config.h"
+#include "llvm/Support/SMTAPI.h"
+#include 
 
-#include "clang/Config/config.h"
+using namespace llvm;
 
-using namespace clang;
-using namespace ento;
-
-#if CLANG_ANALYZER_WITH_Z3
+#if LLVM_WITH_Z3
 
 #include 
 
@@ -818,18 +814,13 @@
 
 #endif
 
-SMTSolverRef clang::ento::CreateZ3Solver() {
-#if CLANG_ANALYZER_WITH_Z3
+llvm::SMTSolverRef llvm::CreateZ3Solver() {
+#if LLVM_WITH_Z3
   return llvm::make_unique();
 #else
   llvm::report_fatal_error("Clang was not compiled with Z3 support, rebuild "
-   "with -DCLANG_ANALYZER_ENABLE_Z3_SOLVER=ON",
+   "with -DLLVM_ENABLE_Z3_SOLVER=ON",
false);
   return nullptr;
 #endif
 }
-
-std::unique_ptr
-ento::CreateZ3ConstraintManager(ProgramStateManager &StMgr, SubEngine *Eng) {
-  return llvm::make_unique(Eng, StMgr.getSValBuilder());
-}
Index: llvm/lib/Support/CMakeLists.txt
===
--- llvm/lib/Support/CMakeLists.txt
+++ llvm/lib/Support/CMakeLists.txt
@@ -44,6 +44,13 @@
   set (delayload_flags delayimp -delayload:shell32.dll -delayload:ole32.dll)
 endif()
 
+# Link Z3 if the user wants to build it.
+if(LLVM_WITH_Z3)
+  set(Z3_LINK_FILES ${Z3_LIBRARIES})
+else()
+  set(Z3_LINK_FILES "")
+endif()
+
 add_llvm_library(LLVMSupport
   AArch64TargetParser.cpp
   ARMTargetParser.cpp
@@ -152,6 +159,7 @@
   regfree.c
   regstrlcpy.c
   xxhash.cpp
+  Z3Solver.cpp
 
 # System
   Atomic.cpp
@@ -177,7 +185,14 @@
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/ADT
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/Support
   ${Backtrace_INCLUDE_DIRS}
-  LINK_LIBS ${system_libs} ${delayload_flags}
+  LINK_LIBS ${system_libs} ${delayload_flags} ${Z3_LINK_FILES}
   )
 
 set_property(TARGET LLVMSupport PROPERTY LLVM_SYSTEM_LIBS "${system_libs}")
+
+if(LLVM_WITH_Z3)
+  target_include_directories(LLVMSupport SYSTEM
+PRIVATE
+${Z3_INCLUDE_DIR}
+)
+endif()
Index: llvm/include/llvm/Support/SMTAPI.h
===
--- llvm/include/llvm/Support/SMTAPI.h
+++ llvm/include/llvm/Support/SMTAPI.h
@@ -11,15 +11,16 @@
 //
 //===--===//
 
-#ifndef LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_SMTSOLVER_H
-#define LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_SMTSOLVER_H
+#ifndef LLVM_SUPPORT_SMTAPI_H
+#define LLVM_SUPPORT_SMTAPI_H
 
-#include "clang/Basic/TargetInfo.h"
+#include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/FoldingSet.h"
+#include "llvm/Support/raw_ostream.h"
+#include 
 
-namespace clang {
-namespace ento {
+namespace llvm {
 
 /// Generic base class for SMT sorts
 class SMTSort {
@@ -399,7 +400,6 @@
 /// Convenience method to create and Z3Solver object
 SMTSolverRef CreateZ3Solver();
 
-} // namespace ento
-} // namespace clang
+} // namespace llvm
 
 #endif
Index: llv

[PATCH] D54978: Move the SMT API to LLVM

[Dominic Chen via Phabricator via cfe-commits]

ddcc added inline comments.



Comment at: llvm/cmake/modules/FindZ3.cmake:92
+
+  set(Z3_VERSION_STRING ${Z3_MAJOR}.${Z3_MINOR}.${Z3_MAJOR})
+  unset(z3_version_str)

Should be ${Z3_MAJOR}.${Z3_MINOR}.${Z3_BUILD_NUMBER}


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

https://reviews.llvm.org/D54978



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

[PATCH] D54978: Move the SMT API to LLVM

[Mikhail Ramalho via Phabricator via cfe-commits]

mikhail.ramalho added a comment.

Hi all,

I just updated the CMake script to get the version by parsing the header, 
however, I just found a potential issue in the previous approach: if we have 
incompatible libs in the path (32-bits libs when compiling in 64-bit mode) the 
CMake config will succeed but the compilation will fail.

To fix that, I changed the script slightly: we first try to dinamically get the 
Z3's version, if we fail and we are cross compiling, then we try to parse the 
headers. Right now, it should just work but I'd like to add a message to warn 
the user that we found the lib but we don't know if it'll link.

Let me know your thoughts.


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

https://reviews.llvm.org/D54978



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

[PATCH] D59509: Make static constructors + destructors minsize + cold (except for in -O0)

[Jessica Paquette via Phabricator via cfe-commits]

paquette created this revision.
paquette added reviewers: rsmith, jyknight.
Herald added a project: clang.

Since these are only ever run once, they can be marked as cold. On top of that, 
since they're only ever run once, they might as well be minsize.

I observed a 0.21% code size improvement in a standard release build of clang 
for x86 with this change.


Repository:
  rC Clang

https://reviews.llvm.org/D59509

Files:
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/test/CodeGen/address-safety-attr.cpp
  clang/test/CodeGen/static-attr.cpp

Index: clang/test/CodeGen/static-attr.cpp
===
--- /dev/null
+++ clang/test/CodeGen/static-attr.cpp
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -disable-O0-optnone -emit-llvm %s -o - | FileCheck -check-prefix=WITH %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -emit-llvm %s -o - | FileCheck -check-prefix=WITHOUT %s
+
+// WITHOUT-NOT: cold minsize noinline
+// WITH: define internal void @__cxx_global_var_init() [[ATTR:#[0-9]]]
+
+extern int baz(void);
+
+int foo(void) {
+  return baz();
+}
+
+static struct StaticInitializer {
+  StaticInitializer() {
+foo();
+  }
+} I;
+
+int bar() {
+  StaticInitializer I;
+  return 0;
+}
+
+// WITH: attributes [[ATTR]] = { {{.*}}cold minsize noinline nounwind optsize{{.*}} }
Index: clang/test/CodeGen/address-safety-attr.cpp
===
--- clang/test/CodeGen/address-safety-attr.cpp
+++ clang/test/CodeGen/address-safety-attr.cpp
@@ -26,17 +26,16 @@
 
 // Check that functions generated for global in different source file are
 // not blacklisted.
-// WITHOUT: @__cxx_global_var_init{{.*}}[[NOATTR:#[0-9]+]]
-// WITHOUT: @__cxx_global_array_dtor{{.*}}[[NOATTR]]
-// BLFILE: @__cxx_global_var_init{{.*}}[[WITH:#[0-9]+]]
-// BLFILE: @__cxx_global_array_dtor{{.*}}[[WITH]]
-// BLFUNC: @__cxx_global_var_init{{.*}}[[WITH:#[0-9]+]]
-// BLFUNC: @__cxx_global_array_dtor{{.*}}[[WITH]]
-// ASAN: @__cxx_global_var_init{{.*}}[[WITH:#[0-9]+]]
-// ASAN: @__cxx_global_array_dtor{{.*}}[[WITH]]
-
-
-// WITHOUT:  NoAddressSafety1{{.*}}) [[NOATTR]]
+// WITHOUT: @__cxx_global_var_init{{.*}}[[NOATTR_STATIC:#[0-9]+]]
+// WITHOUT: @__cxx_global_array_dtor{{.*}}[[NOATTR_STATIC]]
+// BLFILE: @__cxx_global_var_init{{.*}}[[WITH_STATIC:#[0-9]+]]
+// BLFILE: @__cxx_global_array_dtor{{.*}}[[WITH_STATIC]]
+// BLFUNC: @__cxx_global_var_init{{.*}}[[WITH_STATIC:#[0-9]+]]
+// BLFUNC: @__cxx_global_array_dtor{{.*}}[[WITH_STATIC]]
+// ASAN: @__cxx_global_var_init{{.*}}[[WITH_STATIC:#[0-9]+]]
+// ASAN: @__cxx_global_array_dtor{{.*}}[[WITH_STATIC]]
+
+// WITHOUT:  NoAddressSafety1{{.*}}) [[NOATTR:#[0-9]+]]
 // BLFILE:  NoAddressSafety1{{.*}}) [[NOATTR:#[0-9]+]]
 // BLFUNC:  NoAddressSafety1{{.*}}) [[NOATTR:#[0-9]+]]
 // ASAN:  NoAddressSafety1{{.*}}) [[NOATTR:#[0-9]+]]
@@ -138,18 +137,23 @@
 // Check that __cxx_global_var_init* get the sanitize_address attribute.
 int global1 = 0;
 int global2 = *(int*)((char*)&global1+1);
-// WITHOUT: @__cxx_global_var_init{{.*}}[[NOATTR]]
-// BLFILE: @__cxx_global_var_init{{.*}}[[NOATTR:#[0-9]+]]
-// BLFUNC: @__cxx_global_var_init{{.*}}[[WITH]]
-// ASAN: @__cxx_global_var_init{{.*}}[[WITH]]
+// WITHOUT: @__cxx_global_var_init{{.*}}[[NOATTR_STATIC]]
+// BLFILE: @__cxx_global_var_init{{.*}}[[NOATTR_STATIC:#[0-9]+]]
+// BLFUNC: @__cxx_global_var_init{{.*}}[[WITH_STATIC]]
+// ASAN: @__cxx_global_var_init{{.*}}[[WITH_STATIC]]
 
 // WITHOUT: attributes [[NOATTR]] = { noinline nounwind{{.*}} }
+// WITHOUT: attributes [[NOATTR_STATIC]] = { cold minsize noinline nounwind optsize{{.*}} }
 
 // BLFILE: attributes [[WITH]] = { noinline nounwind sanitize_address{{.*}} }
+// BLFILE: attributes [[WITH_STATIC]] = { cold minsize noinline nounwind optsize sanitize_address{{.*}} }
 // BLFILE: attributes [[NOATTR]] = { noinline nounwind{{.*}} }
+// BLFILE: attributes [[NOATTR_STATIC]] = { cold minsize noinline nounwind optsize{{.*}} }
 
 // BLFUNC: attributes [[WITH]] = { noinline nounwind sanitize_address{{.*}} }
+// BLFUNC: attributes [[WITH_STATIC]] = { cold minsize noinline nounwind optsize sanitize_address{{.*}} }
 // BLFUNC: attributes [[NOATTR]] = { noinline nounwind{{.*}} }
 
 // ASAN: attributes [[WITH]] = { noinline nounwind sanitize_address{{.*}} }
+// ASAN: attributes [[WITH_STATIC]] = { cold minsize noinline nounwind optsize sanitize_address{{.*}} }
 // ASAN: attributes [[NOATTR]] = { noinline nounwind{{.*}} }
Index: clang/lib/CodeGen/CGDeclCXX.cpp
===
--- clang/lib/CodeGen/CGDeclCXX.cpp
+++ clang/lib/CodeGen/CGDeclCXX.cpp
@@ -391,6 +391,15 @@
   if (getCodeGenOpts().BranchTargetEnforcement)
 Fn->addFnAttr("branch-target-enforcement");
 
+  // If we're optimizing, then we can make these small, since they're only ever
+  // run once.
+  if (getCodeGenOpts().DisableO0ImplyOptNone ||
+  getCodeGenOpts().OptimizationLevel != 0) {
+Fn->addFnAt

[PATCH] D59455: Thread safety analysis: Add note for unlock kind mismatch

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: include/clang/Analysis/Analyses/ThreadSafety.h:127
+ SourceLocation LocLocked,
  SourceLocation Loc) {}
 

aaron.ballman wrote:
> Can you rename this one to `LocUnlocked` to mirror the new parameter?
This was supposed to mirror the following function (handleDoubleLock), where 
similarly `Loc` is the place of the issue itself and `LocLocked` indicates 
where the lock is coming from. If we want to change this, I would suggest to 
use present tense here: `LocUnlock`. The lock from `LocLocked` "happened in the 
past" (meaning earlier in the code), but the unlock "happens now" (meaning at 
the current location).


Repository:
  rC Clang

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

https://reviews.llvm.org/D59455



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

[PATCH] D57645: [C++2a] Fix PR40576: Turn destroying delete off prior to C++2a. Add -fdestroying-delete

[Eric Fiselier via Phabricator via cfe-commits]

EricWF abandoned this revision.
EricWF added a comment.
Herald added a subscriber: jdoerfert.

This is the wrong approach. Abandoning.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57645



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

[PATCH] D59509: Make static constructors + destructors minsize + cold (except for in -O0)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang/test/CodeGen/static-attr.cpp:4
+
+// WITHOUT-NOT: cold minsize noinline
+// WITH: define internal void @__cxx_global_var_init() [[ATTR:#[0-9]]]

This is fragile, it may have false-negative if they appear in other order.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59509



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

[PATCH] D58827: [Sema][NFCI] Don't allocate storage for the various CorrectionCandidateCallback unless we are going to do some typo correction

[Reid Kleckner via Phabricator via cfe-commits]

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

I think this is worth the complexity of the repeated clone methods. lgtm




Comment at: lib/Sema/SemaType.cpp:5911
   ExprResult AddrSpace = S.ActOnIdExpression(
-  S.getCurScope(), SS, TemplateKWLoc, id, false, false);
+  S.getCurScope(), SS, TemplateKWLoc, id, /*HasTrailingLParen=*/false,
+  /*IsAddressOfOperand=*/false);

These changes are unrelated, but good.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58827



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

[PATCH] D59492: [OpenCL] Allow variadic macros as Clang feature

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

Should it be downgraded to a warning about an extension instead of just 
removing it?


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

https://reviews.llvm.org/D59492



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

[PATCH] D58668: [ASTImporter] Fix redecl failures of FunctionTemplateSpec

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik accepted this revision.
shafik added a comment.

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D58668



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

[PATCH] D59457: [analyzer][NFC] Use capital variable names in CheckerRegistry

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

xazax.hun added a comment.

I did not check the patch yet but wanted to point out that we might not want to 
rush about renaming all the variables until the community decides on the coding 
guideline, see https://reviews.llvm.org/D59251


Repository:
  rC Clang

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

https://reviews.llvm.org/D59457



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

[PATCH] D59455: Thread safety analysis: Add note for unlock kind mismatch

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/Analysis/Analyses/ThreadSafety.h:127
+ SourceLocation LocLocked,
  SourceLocation Loc) {}
 

aaronpuchert wrote:
> aaron.ballman wrote:
> > Can you rename this one to `LocUnlocked` to mirror the new parameter?
> This was supposed to mirror the following function (handleDoubleLock), where 
> similarly `Loc` is the place of the issue itself and `LocLocked` indicates 
> where the lock is coming from. If we want to change this, I would suggest to 
> use present tense here: `LocUnlock`. The lock from `LocLocked` "happened in 
> the past" (meaning earlier in the code), but the unlock "happens now" 
> (meaning at the current location).
Ah, good call on the tense of the identifier! Given that it's following a 
pattern, I'd say you can commit as-is. If you're similarly bothered by the 
name, then you could have a follow-up NFC commit that renames consistently 
across APIs.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59455



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

Re: r311065 - Further refactoring of the constant emitter. NFC.

[John McCall via cfe-commits]

On 18 Mar 2019, at 15:38, Don Hinton wrote:

Hi John:

I found this investigating the cast assert noted here:
http://lists.llvm.org/pipermail/cfe-dev/2019-March/061685.html

I subsequently did quick grep and found a number of cases in 
clang+llvm

(didn't find any in other projects) .  I'm happy to fix these in mass,
i.e., s/cast/dyn_cast/, but as you mentioned, some might require 
additional

refactoring, e.g., removal of the conditional.


They probably all ought to be considered individually.  Tagging Richard 
in case he has opinions about the AST ones.



In any case, here's what I've found -- perhaps a new llvm clang-tidy
checker would be useful:


Yeah, it would be great if something like this got run automatically.

John.

$ find ./clang ./llvm -type f \( -name "*.h" -o -name "*.cpp" \) -exec 
grep

-Hn "if *(auto.* *= *cast *<" \{\} \;
./clang/lib/Sema/SemaOpenMP.cpp:10904:  else if (auto *DRD =
cast(D))
./clang/lib/CodeGen/CGExprConstant.cpp:1701:  if (auto destPtrTy =
cast(destTy)) {
./clang/lib/CodeGen/MicrosoftCXXABI.cpp:738:  if (auto *Fn =
cast(Throw.getCallee()))
./clang/lib/AST/ASTImporter.cpp:8463:  if (auto *FromDC =
cast(From)) {
./clang/lib/AST/DeclBase.cpp:1182:if (auto *Def =
cast(this)->getDefinition())
./clang/lib/AST/DeclBase.cpp:1187:if (auto *Def =
cast(this)->getDefinition())
./llvm/tools/llvm-objdump/llvm-objdump.cpp:802:  if (auto *Elf64BEObj 
=

cast(Obj))
./llvm/tools/llvm-objdump/llvm-objdump.cpp:846:  else if (auto 
*Elf64BEObj

= cast(Obj))
./llvm/lib/Target/X86/AsmParser/X86Operand.h:102:if (auto Imm 
=

cast(Val)->getValue())
./llvm/lib/Transforms/IPO/CalledValuePropagation.cpp:139:  } else 
if

(auto *F = cast(Key.getPointer()))
./llvm/lib/Transforms/Coroutines/CoroEarly.cpp:183:if (auto 
*CII =

cast(&I)) {

thanks...
don

On Mon, Mar 18, 2019 at 12:07 PM John McCall  
wrote:



On 18 Mar 2019, at 14:39, Don Hinton wrote:

It looks like this change introduced a small bug;  Specifically, the
following cast test:

-  if (auto PT = dyn_cast(DestTy)) {
...
+  // If we're producing a pointer, this is easy.
+  if (auto destPtrTy = cast(destTy)) {

Since the cast can fail, shouldn't you prefer dyn_cast<>(), which 
can

return nullptr, over cast<>(), which will assert?


Yes, although if it hasn't caused a problem in the last year and a 
half,

maybe we should just change the code to be non-conditional.

John.




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

r356423 - [X86] Add gcc rotate intrinsics to ia32intrin.h

[Craig Topper via cfe-commits]

Author: ctopper
Date: Mon Mar 18 15:25:57 2019
New Revision: 356423

URL: http://llvm.org/viewvc/llvm-project?rev=356423&view=rev
Log:
[X86] Add gcc rotate intrinsics to ia32intrin.h

This is another attempt at what Erich Keane tried to do in r355322.

This adds rolb, rolw, rold, rolq and their ror equivalent as always_inline 
wrappers around __builtin_rotate* which will lower to funnel shift intrinsics 
in IR.

Additionally, when _MSC_VER is not defined we will define _rotl, _lrotl, _rotr, 
_lrotr as macros to one of the always_inline intrinsics mentioned above. Making 
sure that _lrotl/_lrotr use either 32 or 64 bit based on the size of long. 
These need to be macros because we have builtins with the same name for MS 
compatibility, but _MSC_VER isn't always defined when those builtins are 
enabled.

We also define _rotwl and _rotwr as macros aliasing to rolw/rorw just like gcc 
to complete the set. These don't need to be gated with _MSC_VER because these 
aren't MS builtins.

I've added tests both for non-MS and -ms-extensions with and without _MSC_VER 
being defined.

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

Added:
cfe/trunk/test/CodeGen/rot-intrinsics.c   (with props)
Modified:
cfe/trunk/lib/Headers/ia32intrin.h

Modified: cfe/trunk/lib/Headers/ia32intrin.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Headers/ia32intrin.h?rev=356423&r1=356422&r2=356423&view=diff
==
--- cfe/trunk/lib/Headers/ia32intrin.h (original)
+++ cfe/trunk/lib/Headers/ia32intrin.h Mon Mar 18 15:25:57 2019
@@ -75,4 +75,64 @@ _wbinvd(void) {
   __builtin_ia32_wbinvd();
 }
 
+static __inline__ unsigned char __attribute__((__always_inline__, __nodebug__))
+__rolb(unsigned char __X, int __C) {
+  return __builtin_rotateleft8(__X, __C);
+}
+
+static __inline__ unsigned char __attribute__((__always_inline__, __nodebug__))
+__rorb(unsigned char __X, int __C) {
+  return __builtin_rotateright8(__X, __C);
+}
+
+static __inline__ unsigned short __attribute__((__always_inline__, 
__nodebug__))
+__rolw(unsigned short __X, int __C) {
+  return __builtin_rotateleft16(__X, __C);
+}
+
+static __inline__ unsigned short __attribute__((__always_inline__, 
__nodebug__))
+__rorw(unsigned short __X, int __C) {
+  return __builtin_rotateright16(__X, __C);
+}
+
+static __inline__ unsigned int __attribute__((__always_inline__, __nodebug__))
+__rold(unsigned int __X, int __C) {
+  return __builtin_rotateleft32(__X, __C);
+}
+
+static __inline__ unsigned int __attribute__((__always_inline__, __nodebug__))
+__rord(unsigned int __X, int __C) {
+  return __builtin_rotateright32(__X, __C);
+}
+
+#ifdef __x86_64__
+static __inline__ unsigned long long __attribute__((__always_inline__, 
__nodebug__))
+__rolq(unsigned long long __X, int __C) {
+  return __builtin_rotateleft64(__X, __C);
+}
+
+static __inline__ unsigned long long __attribute__((__always_inline__, 
__nodebug__))
+__rorq(unsigned long long __X, int __C) {
+  return __builtin_rotateright64(__X, __C);
+}
+#endif /* __x86_64__ */
+
+#ifndef _MSC_VER
+/* These are already provided as builtins for MSVC. */
+/* Select the correct function based on the size of long. */
+#ifdef __LP64__
+#define _lrotl(a,b) __rolq((a), (b))
+#define _lrotr(a,b) __rorq((a), (b))
+#else
+#define _lrotl(a,b) __rold((a), (b))
+#define _lrotr(a,b) __rord((a), (b))
+#endif
+#define _rotl(a,b) __rold((a), (b))
+#define _rotr(a,b) __rord((a), (b))
+#endif // _MSC_VER
+
+/* These are not builtins so need to be provided in all modes. */
+#define _rotwl(a,b) __rolw((a), (b))
+#define _rotwr(a,b) __rorw((a), (b))
+
 #endif /* __IA32INTRIN_H */

Added: cfe/trunk/test/CodeGen/rot-intrinsics.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/rot-intrinsics.c?rev=356423&view=auto
==
--- cfe/trunk/test/CodeGen/rot-intrinsics.c (added)
+++ cfe/trunk/test/CodeGen/rot-intrinsics.c Mon Mar 18 15:25:57 2019
@@ -0,0 +1,120 @@
+// RUN: %clang_cc1 -ffreestanding -triple i686--linux -emit-llvm %s -o - | 
FileCheck %s --check-prefixes CHECK,CHECK-32BIT-LONG
+// RUN: %clang_cc1 -ffreestanding -triple x86_64--linux -emit-llvm %s -o - | 
FileCheck %s --check-prefixes CHECK,CHECK-64BIT-LONG
+// RUN: %clang_cc1 -fms-extensions -fms-compatibility -ffreestanding %s 
-triple=i686-windows-msvc -target-feature +sse2 -emit-llvm -o - -Wall -Werror | 
FileCheck %s --check-prefixes CHECK,CHECK-32BIT-LONG
+// RUN: %clang_cc1 -fms-extensions -fms-compatibility -ffreestanding %s 
-triple=x86_64-windows-msvc -target-feature +sse2 -emit-llvm -o - -Wall -Werror 
| FileCheck %s --check-prefixes CHECK,CHECK-32BIT-LONG
+// RUN: %clang_cc1 -fms-extensions -fms-compatibility 
-fms-compatibility-version=17.00 -ffreestanding %s -triple=i686-windows-msvc 
-target-feature +sse2 -emit-llvm -o - -Wall -Werror | FileCheck %s 
--check-prefixes CHECK,CHECK-32BIT-LONG
+// RUN: %clang_cc1