date:20180915

Re: [PATCH] D52078: [clangd] Store preamble macros in dynamic index.

2018-09-15 Thread Sam McCall via cfe-commits

On Sat, Sep 15, 2018, 08:36 Eric Liu via Phabricator <
revi...@reviews.llvm.org> wrote:

> ioeric added inline comments.
>
>
> 
> Comment at: clangd/index/FileIndex.h:93
>  std::pair
>  indexAST(ASTContext &AST, std::shared_ptr PP,
> + bool IndexMacros = false,
> 
> sammccall wrote:
> > I'm concerned about the proliferation of parameters here. (Not new with
> this patch, it's been a continuous process - the first version had one
> input and one output!)
> > It's heading in the direction of being a catch-all "collect some data
> from an AST" function, at which point each caller has to think hard about
> every option and we're going to end up with bugs.
> > (For example: TestTU::index() passes "false" for IndexMacros. It seems
> to me maybe it should be "true". But it's really hard to tell.)
> > That's also pretty similar to the mission of SymbolCollector itself, so
> we're going to get some confusion there.
> >
> > As far as I can tell, there's now two types of indexing that we do:
> >   - preamble indexing: we look at all decls, and only index those
> outside the main file. We index macros. We don't collect references. Inputs
> are ASTContext and PP.
> >   - mainfile-style indexing: we look only at top-level decls in the main
> file. We don't index macros. We collect references from the main file.
> Inputs are ParsedAST.
> > This really looks like it should be 2 functions with 2 and 1 parameters,
> rather than 1 function with 4.
> > Then callers will have two styles of indexing (with names!) to choose
> between, rather than being required to design their own. And we can get rid
> of the "is this a main-file index?" hacks in the implementation.
> >
> > Sorry for jumping on you here, this change isn't any worse than the
> three previous patches that added knobs to this function.
> > This doesn't need to be addressed in this patch - could be before or
> after, and I'm happy to take this on myself. But I think we shouldn't kick
> this can down the road too much further, eventually we end up with APIs
> like clang :-)
> I'm definitely on board with this and happy to do the refactoring before
> landing this patch, to break API just once ;)  Just to clarify my
> understanding before doing that, do we also want to split
> `FileIndex::update` into `updatePreamble` and `updateMain`?  And the new
> `indexAST` will be `indexMain` and `indexPreamble`?
>
Basically yes, though it's a bit weird that each instance would use one
method or the other.

> I think if we're going to break the API, we should break it good :-)

FileIndex is pretty thin today logic-wise, all the opinions are in
FileSymbols, indexAST, and the DynamicIndex callers. I think it could stand
to take on more responsibility.
What if we merged DynamicIndex into it? So it'd have both updatePreamble
and updateMain, but they would update two separate FileSymbols. FileIndex
would maintain two SwapIndexes, and expose a merged index.

This would take the logic out of ClangdServer (where it's hard to test) and
also I think we have an out-of-tree DynamicIndex copy we could remove.
And I think the API would push us towards sensible decisions (e.g. I think
the answer for testTU::index() is it really wants to be a one-file
FileIndex with both indexing types)

Thoughts?


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

[PATCH] D52120: [analyzer] Treat std::{move, forward} as casts in ExprMutationAnalyzer.

2018-09-15 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added a comment.

In https://reviews.llvm.org/D52120#1235515, @lebedev.ri wrote:

> Though some of that is still false-positives (pointers, va_arg, callback 
> lambdas passed as templated function param), but i'll file further bugs i 
> guess.


Filed the ones that i can pinpoint, marked them as blockers of PR38890 



Repository:
  rC Clang

https://reviews.llvm.org/D52120



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

[PATCH] D52118: [Loopinfo] Remove one latch case in getLoopID. NFC.

2018-09-15 Thread Johannes Doerfert via Phabricator via cfe-commits

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

> save an iteration over the loop's basic blocks (which is what getLoopLatch 
> does)

I'm not sure this is true. getLoopLatch() in LoopInfoImpl.h 
only traverses the children of the header in the inverse graph.
That should, I think, be similar to predecessors(Header) in case
of the IR CFG.

That being said, the patch makes sense to me and it is a simple,
straightforward improvement. I don't see any downsides and
it simplifies the code.

LGTM


Repository:
  rC Clang

https://reviews.llvm.org/D52118



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

[PATCH] D52120: [analyzer] Treat std::{move, forward} as casts in ExprMutationAnalyzer.

2018-09-15 Thread Jonas Toth via Phabricator via cfe-commits

JonasToth added a comment.

Thank you very much for the good work from my side as well!




Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:387
   match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("std::move(x)"));
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+

I am thinking about the correctness of the change.

If we have this case:
```
std::string s1 = "Hello World!";
std::string s2 = std::move(s1);
```
`s1` gets mutated, but it looks like it would not be considered as mutation?

On the other hand
```
int i1 = 42;
int i2 = std::move(i1);
```
should resolve in a copy `i1` and therefor not be a mutation.

Could you please add tests demonstrating this difference and the correct 
diagnostic detection for that.

Potentially interesting:
- Builtin types should be Copy-Only?, builtin arrays as expensive to move types 
for the completeness please as well
- Types with deleted move operations, should resolve in copy too
- Move-Only types like unique_ptr
- Types with both move and copy constructor (vector-like)
- and something with everything defaulted

These thoughts are just thinking and writing, I just wondered that moving a 
variable with std::move is not considered as mutation here, even though there 
are cases were it is actually a mutation.


Repository:
  rC Clang

https://reviews.llvm.org/D52120



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

[PATCH] D52120: [analyzer] Treat std::{move, forward} as casts in ExprMutationAnalyzer.

2018-09-15 Thread Jonas Toth via Phabricator via cfe-commits

JonasToth added inline comments.



Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:387
   match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("std::move(x)"));
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+

JonasToth wrote:
> I am thinking about the correctness of the change.
> 
> If we have this case:
> ```
> std::string s1 = "Hello World!";
> std::string s2 = std::move(s1);
> ```
> `s1` gets mutated, but it looks like it would not be considered as mutation?
> 
> On the other hand
> ```
> int i1 = 42;
> int i2 = std::move(i1);
> ```
> should resolve in a copy `i1` and therefor not be a mutation.
> 
> Could you please add tests demonstrating this difference and the correct 
> diagnostic detection for that.
> 
> Potentially interesting:
> - Builtin types should be Copy-Only?, builtin arrays as expensive to move 
> types for the completeness please as well
> - Types with deleted move operations, should resolve in copy too
> - Move-Only types like unique_ptr
> - Types with both move and copy constructor (vector-like)
> - and something with everything defaulted
> 
> These thoughts are just thinking and writing, I just wondered that moving a 
> variable with std::move is not considered as mutation here, even though there 
> are cases were it is actually a mutation.
Related: 
https://clang.llvm.org/extra/clang-tidy/checks/performance-move-const-arg.html


Repository:
  rC Clang

https://reviews.llvm.org/D52120



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

r342322 - [OPENMP] Move OMPClauseReader/Writer classes to ASTReader/Writer (NFC)

2018-09-15 Thread Kelvin Li via cfe-commits

Author: kli
Date: Sat Sep 15 06:54:15 2018
New Revision: 342322

URL: http://llvm.org/viewvc/llvm-project?rev=342322&view=rev
Log:
[OPENMP] Move OMPClauseReader/Writer classes to ASTReader/Writer (NFC)

Move declarations for OMPClauseReader, OMPClauseWriter to ASTReader.h 
and ASTWriter.h and move implementation to ASTReader.cpp and 
ASTWriter.cpp. This change helps generalize the serialization of
OpenMP clauses and will be used in the future implementation of new 
OpenMP directives (e.g. requires).

Patch by Patrick Lyster

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

Modified:
cfe/trunk/include/clang/AST/OpenMPClause.h
cfe/trunk/include/clang/AST/StmtVisitor.h
cfe/trunk/include/clang/Serialization/ASTReader.h
cfe/trunk/include/clang/Serialization/ASTWriter.h
cfe/trunk/lib/Serialization/ASTReader.cpp
cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/lib/Serialization/ASTWriterStmt.cpp

Modified: cfe/trunk/include/clang/AST/OpenMPClause.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/OpenMPClause.h?rev=342322&r1=342321&r2=342322&view=diff
==
--- cfe/trunk/include/clang/AST/OpenMPClause.h (original)
+++ cfe/trunk/include/clang/AST/OpenMPClause.h Sat Sep 15 06:54:15 2018
@@ -5041,6 +5041,43 @@ public:
   }
 };
 
+/// This class implements a simple visitor for OMPClause
+/// subclasses.
+template class Ptr, typename RetTy>
+class OMPClauseVisitorBase {
+public:
+#define PTR(CLASS) typename Ptr::type
+#define DISPATCH(CLASS) \
+  return 
static_cast(this)->Visit##CLASS(static_cast(S))
+
+#define OPENMP_CLAUSE(Name, Class)  \
+  RetTy Visit ## Class (PTR(Class) S) { DISPATCH(Class); }
+#include "clang/Basic/OpenMPKinds.def"
+
+  RetTy Visit(PTR(OMPClause) S) {
+// Top switch clause: visit each OMPClause.
+switch (S->getClauseKind()) {
+default: llvm_unreachable("Unknown clause kind!");
+#define OPENMP_CLAUSE(Name, Class)  \
+case OMPC_ ## Name : return Visit ## Class(static_cast(S));
+#include "clang/Basic/OpenMPKinds.def"
+}
+  }
+  // Base case, ignore it. :)
+  RetTy VisitOMPClause(PTR(OMPClause) Node) { return RetTy(); }
+#undef PTR
+#undef DISPATCH
+};
+
+template 
+using const_ptr = typename std::add_pointer::type>;
+
+template
+class OMPClauseVisitor :
+  public OMPClauseVisitorBase  {};
+template
+class ConstOMPClauseVisitor :
+  public OMPClauseVisitorBase  {};
 } // namespace clang
 
 #endif // LLVM_CLANG_AST_OPENMPCLAUSE_H

Modified: cfe/trunk/include/clang/AST/StmtVisitor.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/StmtVisitor.h?rev=342322&r1=342321&r2=342322&view=diff
==
--- cfe/trunk/include/clang/AST/StmtVisitor.h (original)
+++ cfe/trunk/include/clang/AST/StmtVisitor.h Sat Sep 15 06:54:15 2018
@@ -195,41 +195,6 @@ template {};
 
-/// This class implements a simple visitor for OMPClause
-/// subclasses.
-template class Ptr, typename RetTy>
-class OMPClauseVisitorBase {
-public:
-#define PTR(CLASS) typename Ptr::type
-#define DISPATCH(CLASS) \
-  return 
static_cast(this)->Visit##CLASS(static_cast(S))
-
-#define OPENMP_CLAUSE(Name, Class)  \
-  RetTy Visit ## Class (PTR(Class) S) { DISPATCH(Class); }
-#include "clang/Basic/OpenMPKinds.def"
-
-  RetTy Visit(PTR(OMPClause) S) {
-// Top switch clause: visit each OMPClause.
-switch (S->getClauseKind()) {
-default: llvm_unreachable("Unknown clause kind!");
-#define OPENMP_CLAUSE(Name, Class)  \
-case OMPC_ ## Name : return Visit ## Class(static_cast(S));
-#include "clang/Basic/OpenMPKinds.def"
-}
-  }
-  // Base case, ignore it. :)
-  RetTy VisitOMPClause(PTR(OMPClause) Node) { return RetTy(); }
-#undef PTR
-#undef DISPATCH
-};
-
-template
-class OMPClauseVisitor :
-  public OMPClauseVisitorBase  {};
-template
-class ConstOMPClauseVisitor :
-  public OMPClauseVisitorBase  {};
-
 } // namespace clang
 
 #endif // LLVM_CLANG_AST_STMTVISITOR_H

Modified: cfe/trunk/include/clang/Serialization/ASTReader.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTReader.h?rev=342322&r1=342321&r2=342322&view=diff
==
--- cfe/trunk/include/clang/Serialization/ASTReader.h (original)
+++ cfe/trunk/include/clang/Serialization/ASTReader.h Sat Sep 15 06:54:15 2018
@@ -18,6 +18,7 @@
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclarationName.h"
 #include "clang/AST/NestedNameSpecifier.h"
+#include "clang/AST/OpenMPClause.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/TemplateName.h"
 #include "clang/AST/Type.h"
@@ -2677,6 +2678,21 @@ inline void PCHValidator::Error(const ch
   Reader.Error(

[PATCH] D52097: [OPENMP] Move OMPClauseReader/Writer classes to ASTReader/Writer - NFC

2018-09-15 Thread Phabricator via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL342322: [OPENMP] Move OMPClauseReader/Writer classes to 
ASTReader/Writer (NFC) (authored by kli, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D52097?vs=165508&id=165640#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D52097

Files:
  cfe/trunk/include/clang/AST/OpenMPClause.h
  cfe/trunk/include/clang/AST/StmtVisitor.h
  cfe/trunk/include/clang/Serialization/ASTReader.h
  cfe/trunk/include/clang/Serialization/ASTWriter.h
  cfe/trunk/lib/Serialization/ASTReader.cpp
  cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
  cfe/trunk/lib/Serialization/ASTWriter.cpp
  cfe/trunk/lib/Serialization/ASTWriterStmt.cpp

Index: cfe/trunk/include/clang/AST/StmtVisitor.h
===
--- cfe/trunk/include/clang/AST/StmtVisitor.h
+++ cfe/trunk/include/clang/AST/StmtVisitor.h
@@ -195,41 +195,6 @@
 class ConstStmtVisitor
  : public StmtVisitorBase {};
 
-/// This class implements a simple visitor for OMPClause
-/// subclasses.
-template class Ptr, typename RetTy>
-class OMPClauseVisitorBase {
-public:
-#define PTR(CLASS) typename Ptr::type
-#define DISPATCH(CLASS) \
-  return static_cast(this)->Visit##CLASS(static_cast(S))
-
-#define OPENMP_CLAUSE(Name, Class)  \
-  RetTy Visit ## Class (PTR(Class) S) { DISPATCH(Class); }
-#include "clang/Basic/OpenMPKinds.def"
-
-  RetTy Visit(PTR(OMPClause) S) {
-// Top switch clause: visit each OMPClause.
-switch (S->getClauseKind()) {
-default: llvm_unreachable("Unknown clause kind!");
-#define OPENMP_CLAUSE(Name, Class)  \
-case OMPC_ ## Name : return Visit ## Class(static_cast(S));
-#include "clang/Basic/OpenMPKinds.def"
-}
-  }
-  // Base case, ignore it. :)
-  RetTy VisitOMPClause(PTR(OMPClause) Node) { return RetTy(); }
-#undef PTR
-#undef DISPATCH
-};
-
-template
-class OMPClauseVisitor :
-  public OMPClauseVisitorBase  {};
-template
-class ConstOMPClauseVisitor :
-  public OMPClauseVisitorBase  {};
-
 } // namespace clang
 
 #endif // LLVM_CLANG_AST_STMTVISITOR_H
Index: cfe/trunk/include/clang/AST/OpenMPClause.h
===
--- cfe/trunk/include/clang/AST/OpenMPClause.h
+++ cfe/trunk/include/clang/AST/OpenMPClause.h
@@ -5041,6 +5041,43 @@
   }
 };
 
+/// This class implements a simple visitor for OMPClause
+/// subclasses.
+template class Ptr, typename RetTy>
+class OMPClauseVisitorBase {
+public:
+#define PTR(CLASS) typename Ptr::type
+#define DISPATCH(CLASS) \
+  return static_cast(this)->Visit##CLASS(static_cast(S))
+
+#define OPENMP_CLAUSE(Name, Class)  \
+  RetTy Visit ## Class (PTR(Class) S) { DISPATCH(Class); }
+#include "clang/Basic/OpenMPKinds.def"
+
+  RetTy Visit(PTR(OMPClause) S) {
+// Top switch clause: visit each OMPClause.
+switch (S->getClauseKind()) {
+default: llvm_unreachable("Unknown clause kind!");
+#define OPENMP_CLAUSE(Name, Class)  \
+case OMPC_ ## Name : return Visit ## Class(static_cast(S));
+#include "clang/Basic/OpenMPKinds.def"
+}
+  }
+  // Base case, ignore it. :)
+  RetTy VisitOMPClause(PTR(OMPClause) Node) { return RetTy(); }
+#undef PTR
+#undef DISPATCH
+};
+
+template 
+using const_ptr = typename std::add_pointer::type>;
+
+template
+class OMPClauseVisitor :
+  public OMPClauseVisitorBase  {};
+template
+class ConstOMPClauseVisitor :
+  public OMPClauseVisitorBase  {};
 } // namespace clang
 
 #endif // LLVM_CLANG_AST_OPENMPCLAUSE_H
Index: cfe/trunk/include/clang/Serialization/ASTReader.h
===
--- cfe/trunk/include/clang/Serialization/ASTReader.h
+++ cfe/trunk/include/clang/Serialization/ASTReader.h
@@ -18,6 +18,7 @@
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclarationName.h"
 #include "clang/AST/NestedNameSpecifier.h"
+#include "clang/AST/OpenMPClause.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/TemplateName.h"
 #include "clang/AST/Type.h"
@@ -2677,6 +2678,21 @@
   Reader.Error(Msg);
 }
 
+class OMPClauseReader : public OMPClauseVisitor {
+  ASTRecordReader &Record;
+  ASTContext &Context;
+
+public:
+  OMPClauseReader(ASTRecordReader &Record)
+  : Record(Record), Context(Record.getContext()) {}
+
+#define OPENMP_CLAUSE(Name, Class) void Visit##Class(Class *C);
+#include "clang/Basic/OpenMPKinds.def"
+  OMPClause *readClause();
+  void VisitOMPClauseWithPreInit(OMPClauseWithPreInit *C);
+  void VisitOMPClauseWithPostUpdate(OMPClauseWithPostUpdate *C);
+};
+
 } // namespace clang
 
 #endif // LLVM_CLANG_SERIALIZATION_ASTREADER_H
Index: cfe/trunk/include/clang/Serialization/ASTWriter.h
===
--- cfe/trunk/include/clang/Serialization/ASTWriter.h
+++ cfe/trunk/incl

[PATCH] D52135: Hello, i would like to suggest a fix for one of the checks in clang-tidy.The bug was reported in https://bugs.llvm.org/show_bug.cgi?id=32575 where you can find more information.

2018-09-15 Thread Idriss via Phabricator via cfe-commits

IdrissRio created this revision.
IdrissRio added reviewers: aaron.ballman, hokein, alexfh.
Herald added a subscriber: cfe-commits.

[Clang-Tidy: modernize] Fix for modernize-redundant-void-arg: complains about 
variable cast to void


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52135

Files:
  clang-tidy/modernize/RedundantVoidArgCheck.cpp
  test/clang-tidy/modernize-redundant-void-arg.cpp


Index: test/clang-tidy/modernize-redundant-void-arg.cpp
===
--- test/clang-tidy/modernize-redundant-void-arg.cpp
+++ test/clang-tidy/modernize-redundant-void-arg.cpp
@@ -488,3 +488,17 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: redundant void argument list in 
lambda expression [modernize-redundant-void-arg]
   // CHECK-FIXES: []() BODY;
 }
+
+template 
+struct S_template {
+   template 
+   void g_template() const {
+   int a_template;
+   (void)a_template;
+   }
+};
+
+void test_template() {
+   S_template().g_template();
+}
+
Index: clang-tidy/modernize/RedundantVoidArgCheck.cpp
===
--- clang-tidy/modernize/RedundantVoidArgCheck.cpp
+++ clang-tidy/modernize/RedundantVoidArgCheck.cpp
@@ -49,7 +49,7 @@
 return;
 
   Finder->addMatcher(functionDecl(parameterCountIs(0), unless(isImplicit()),
-  unless(isExternC()))
+  unless(isInstantiated()), 
unless(isExternC()))
  .bind(FunctionId),
  this);
   Finder->addMatcher(typedefNameDecl().bind(TypedefId), this);


Index: test/clang-tidy/modernize-redundant-void-arg.cpp
===
--- test/clang-tidy/modernize-redundant-void-arg.cpp
+++ test/clang-tidy/modernize-redundant-void-arg.cpp
@@ -488,3 +488,17 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: redundant void argument list in lambda expression [modernize-redundant-void-arg]
   // CHECK-FIXES: []() BODY;
 }
+
+template 
+struct S_template {
+	template 
+	void g_template() const {
+		int a_template;
+		(void)a_template;
+	}
+};
+
+void test_template() {
+	S_template().g_template();
+}
+
Index: clang-tidy/modernize/RedundantVoidArgCheck.cpp
===
--- clang-tidy/modernize/RedundantVoidArgCheck.cpp
+++ clang-tidy/modernize/RedundantVoidArgCheck.cpp
@@ -49,7 +49,7 @@
 return;
 
   Finder->addMatcher(functionDecl(parameterCountIs(0), unless(isImplicit()),
-  unless(isExternC()))
+  unless(isInstantiated()), unless(isExternC()))
  .bind(FunctionId),
  this);
   Finder->addMatcher(typedefNameDecl().bind(TypedefId), this);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52135: [Clang-Tidy: modernize] Fix for modernize-redundant-void-arg: complains about variable cast to void

2018-09-15 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added a comment.

This looks weird.
It will now completely skip all the explicitly instantiated functions, no?
I'd think the problem that needs to be fixed is that it considers the local 
variable `int a_template;` to be in the function argument list.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52135



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

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

2018-09-15 Thread Wojtek Gumuła via Phabricator via cfe-commits

wgml created this revision.
wgml added reviewers: alexfh, aaron.ballman, hokein.
Herald added subscribers: xazax.hun, mgorny.

Finds instances of namespaces concatenated using explicit syntax, such as 
`namespace a { namespace b { [...] }}` and offers fix to glue it to `namespace 
a::b { [...] }`.

Properly handles `inline` and unnamed namespaces. Also, detects empty blocks in 
nested namespaces and offers to remove them.

Test with common use cases included.
I ran the check against entire llvm repository. Except for expected `nested 
namespace definitions only available with -std=c++17 or -std=gnu++17` warnings 
I noticed no issues when the check was performed.

Example:

  namespace a { namespace b {
  void test();
  }}
  
  namespace c { namespace d { namespace e { }}}

can become

  namespace a::b {
  void test();
  }




Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52136

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
  clang-tidy/modernize/ConcatNestedNamespacesCheck.h
  clang-tidy/modernize/ModernizeTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
  test/clang-tidy/modernize-concat-nested-namespaces.cpp

Index: test/clang-tidy/modernize-concat-nested-namespaces.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-concat-nested-namespaces.cpp
@@ -0,0 +1,124 @@
+// RUN: %check_clang_tidy %s modernize-concat-nested-namespaces %t -- -- -std=c++17
+
+namespace n1 {}
+
+namespace n2 {
+namespace n3 {
+void t();
+}
+namespace n4 {
+void t();
+}
+} // namespace n2
+
+namespace n5 {
+inline namespace n6 { void t(); }
+} // namespace n5
+
+namespace n7 {
+void t();
+
+namespace n8 {
+void t();
+}
+} // namespace n7
+
+namespace n9 {
+namespace n10 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n9::n10
+void t();
+} // namespace n10
+} // namespace n9
+// CHECK-FIXES: }
+
+namespace n11 {
+namespace n12 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n11::n12
+namespace n13 {
+void t();
+}
+namespace n14 {
+void t();
+}
+} // namespace n12
+} // namespace n11
+// CHECK-FIXES: }
+
+namespace n15 {
+namespace n16 {
+void t();
+}
+
+inline namespace n17 { void t(); }
+
+namespace n18 {
+namespace n19 {
+namespace n20 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n18::n19::n20
+void t();
+} // namespace n20
+} // namespace n19
+} // namespace n18
+// CHECK-FIXES: }
+
+namespace n21 {
+void t();
+}
+} // namespace n15
+
+namespace n22 {
+namespace {
+void t();
+}
+} // namespace n22
+
+namespace n23 {
+namespace {
+namespace n24 {
+namespace n25 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n24::n25
+void t();
+} // namespace n25
+} // namespace n24
+// CHECK-FIXES: }
+} // namespace
+} // namespace n23
+
+namespace n26::n27 {
+namespace n28 {
+namespace n29::n30 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n26::n27::n28::n29::n30
+void t() {}
+} // namespace n29::n30
+} // namespace n28
+} // namespace n26::n27
+// CHECK-FIXES: }
+
+namespace n31 {
+namespace n32 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces are empty and can be removed [modernize-concat-nested-namespaces]
+} // namespace n31
+
+namespace n33 {
+namespace n34 {
+namespace n35 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces are empty and can be removed [modernize-concat-nested-namespaces]
+} // namespace n34
+namespace n36 {
+void t();
+}
+} // namespace n33
+
+namespace n37::n38 {
+void t();
+}
+
+int main() {
+  n26::n27::n28::n29::n30::t();
+  return 0;
+}
Index: docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
@@ -0,0 +1,47 @@
+.. title:: clang-tidy - modernize-concat-nested-namespaces
+
+modernize-concat-nested-namespaces
+==
+
+Checks for use of nested namespaces in a form of ``namespace a { namespace b { ... } }``
+and offers change to syntax introduced in C++17: ``namespace a::b { ... }``.
+Inlined namespaces are not modified.
+
+For example:
+
+.. code-block:: c++
+  namspace n1 {
+  namespace n2 {
+  void t();
+  }
+  }
+
+  namespace n3 {
+  namespace n4 {
+  namespace n5 {
+  void t();
+  }
+  }
+  namespace n6 {
+  namespace n7 {
+  void t();
+  }
+  }
+  }
+
+Will be modified to:
+
+..

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

2018-09-15 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added inline comments.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:73-74
+void ConcatNestedNamespacesCheck::registerMatchers(MatchFinder *Finder) {
+  if (getLangOpts().CPlusPlus)
+Finder->addMatcher(namespaceDecl().bind("namespace"), this);
+}

It should only get registered in C++17 or newer mode.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52136



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

[PATCH] D52135: [Clang-Tidy: modernize] Fix for modernize-redundant-void-arg: complains about variable cast to void

2018-09-15 Thread Idriss via Phabricator via cfe-commits

IdrissRio added a comment.

In https://reviews.llvm.org/D52135#1235950, @lebedev.ri wrote:

>

> It will now completely skip all the explicitly instantiated functions, no?

In my opinion, for this kind of check,  we don't have the necessity to take in 
consideration the function instantiation. We only have to consider the function 
definition/declaration.

> I'd think the problem that needs to be fixed is that it considers the local 
> variable `int a_template;` to be in the function argument list.

No the problem is that the ast_matcher find 2 occurrence of g_template: the 
first is the definition of g_template() and the second is the instantiation of 
g_template. The second is the one that create the problem since  it start to 
iterate all over the body of the function trying to fine  the the tokens '( 
void )' that in this case is this guiltless void cast : (void)a_template.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52135

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

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

2018-09-15 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added inline comments.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:42-45
+  if (Decl->getKind() != Decl::Kind::Namespace)
+return false;
+
+  auto const *NS = reinterpret_cast(Decl);

Use proper casting, `isa<>()`, `dyn_cast<>`, so on.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:79
+const MatchFinder::MatchResult &Result) {
+  NamespaceDecl const &ND = 
*Result.Nodes.getNodeAs("namespace");
+  SourceManager const &Sources = *Result.SourceManager;

Here and everywhere - the consts are on the wrong side.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:102-108
+  if (childrenCount(ND.decls()) == 0) {
+SourceRange Removal(Namespaces.front().Begin, Namespaces.front().RBrace);
+
+diag(Namespaces.front().Begin,
+ "Nested namespaces are empty and can be removed",
+ DiagnosticIDs::Warning)
+<< FixItHint::CreateRemoval(Removal);

1. This is unrelated to the main check, so it should be in a separate check.
2. This is really fragile i think. What about preprocessor-driven emptiness?



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.h:29-32
+  struct NamespaceContext {
+std::string Name;
+SourceLocation Begin, NameBegin, RBrace;
+  };

This looks like a pessimization.
I think you should just store `NamespaceDecl*`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52136



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

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

2018-09-15 Thread Eugene Zelenko via Phabricator via cfe-commits

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:37
+static bool singleNamedNamespaceChild(NamespaceDecl const &ND) {
+  auto const Decls = ND.decls();
+  if (childrenCount(Decls) != 1)

Type is not spelled in declaration, so please don't use auto.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:53
+LangOptions const &LangOpts) {
+  auto const TextRange =
+  Lexer::getAsCharRange(ReplacementRange, Sources, LangOpts);

Type is not spelled in declaration, so please don't use auto.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:55
+  Lexer::getAsCharRange(ReplacementRange, Sources, LangOpts);
+  auto const CurrentNamespacesText =
+  Lexer::getSourceText(TextRange, Sources, LangOpts);

Type is not spelled in declaration, so please don't use auto.



Comment at: docs/ReleaseNotes.rst:96
 
+  - New :doc:`modernize-concat-nested-namespaces
+  ` check.

Wrong indentation. See other entries.



Comment at: docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst:12
+
+.. code-block:: c++
+  namspace n1 {

Please add empty line after.



Comment at: docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst:34
+
+.. code-block:: c++
+  namspace n1::n2 {

Please add empty line after.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52136



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

[PATCH] D52137: Added warning for unary minus used with unsigned type

2018-09-15 Thread Dávid Bolvanský via Phabricator via cfe-commits

xbolva00 created this revision.
xbolva00 added a reviewer: rsmith.
Herald added a subscriber: cfe-commits.

Inspired by MSVC, which found some occurrences of this expression on our code 
base.

Fixes PR38950


Repository:
  rC Clang

https://reviews.llvm.org/D52137

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/Sema/unary-minus-unsigned.c


Index: test/Sema/unary-minus-unsigned.c
===
--- test/Sema/unary-minus-unsigned.c
+++ test/Sema/unary-minus-unsigned.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+
+void test1(void) {
+unsigned int a = 1;
+unsigned long b = 2;
+unsigned long long c = 1;
+ 
+unsigned int a2 = -a; // expected-error {{unary minus operator applied to 
type 'unsigned int', result value is still positive}}
+unsigned long b2 = -b; // expected-error {{unary minus operator applied to 
type 'unsigned long', result value is still positive}}
+unsigned long long c2 = -c; // expected-error {{unary minus operator 
applied to type 'unsigned long long', result value is still positive}}
+ }
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12646,8 +12646,12 @@
 resultType = Input.get()->getType();
 if (resultType->isDependentType())
   break;
-if (resultType->isArithmeticType()) // C99 6.5.3.3p1
+if (resultType->isArithmeticType()) { // C99 6.5.3.3p1
+  if (Opc == UO_Minus && resultType->isUnsignedIntegerType())
+return ExprError(Diag(OpLoc, diag::err_unsignedtypecheck_unary_minus)
+ << resultType << Input.get()->getSourceRange());
   break;
+}
 else if (resultType->isVectorType() &&
  // The z vector extensions don't allow + or - with bool vectors.
  (!Context.getLangOpts().ZVector ||
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -5820,6 +5820,8 @@
   "taking the address of a destructor">;
 def err_typecheck_unary_expr : Error<
   "invalid argument type %0 to unary expression">;
+def err_unsignedtypecheck_unary_minus : Error<
+  "unary minus operator applied to type %0, result value is still positive">;
 def err_typecheck_indirection_requires_pointer : Error<
   "indirection requires pointer operand (%0 invalid)">;
 def ext_typecheck_indirection_through_void_pointer : ExtWarn<


Index: test/Sema/unary-minus-unsigned.c
===
--- test/Sema/unary-minus-unsigned.c
+++ test/Sema/unary-minus-unsigned.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+
+void test1(void) {
+unsigned int a = 1;
+unsigned long b = 2;
+unsigned long long c = 1;
+ 
+unsigned int a2 = -a; // expected-error {{unary minus operator applied to type 'unsigned int', result value is still positive}}
+unsigned long b2 = -b; // expected-error {{unary minus operator applied to type 'unsigned long', result value is still positive}}
+unsigned long long c2 = -c; // expected-error {{unary minus operator applied to type 'unsigned long long', result value is still positive}}
+ }
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12646,8 +12646,12 @@
 resultType = Input.get()->getType();
 if (resultType->isDependentType())
   break;
-if (resultType->isArithmeticType()) // C99 6.5.3.3p1
+if (resultType->isArithmeticType()) { // C99 6.5.3.3p1
+  if (Opc == UO_Minus && resultType->isUnsignedIntegerType())
+return ExprError(Diag(OpLoc, diag::err_unsignedtypecheck_unary_minus)
+ << resultType << Input.get()->getSourceRange());
   break;
+}
 else if (resultType->isVectorType() &&
  // The z vector extensions don't allow + or - with bool vectors.
  (!Context.getLangOpts().ZVector ||
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -5820,6 +5820,8 @@
   "taking the address of a destructor">;
 def err_typecheck_unary_expr : Error<
   "invalid argument type %0 to unary expression">;
+def err_unsignedtypecheck_unary_minus : Error<
+  "unary minus operator applied to type %0, result value is still positive">;
 def err_typecheck_indirection_requires_pointer : Error<
   "indirection requires pointer operand (%0 invalid)">;
 def ext_typecheck_indirection_through_void_pointer : ExtWarn<
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

2018-09-15 Thread Wojtek Gumuła via Phabricator via cfe-commits

wgml added inline comments.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:102-108
+  if (childrenCount(ND.decls()) == 0) {
+SourceRange Removal(Namespaces.front().Begin, Namespaces.front().RBrace);
+
+diag(Namespaces.front().Begin,
+ "Nested namespaces are empty and can be removed",
+ DiagnosticIDs::Warning)
+<< FixItHint::CreateRemoval(Removal);

lebedev.ri wrote:
> 1. This is unrelated to the main check, so it should be in a separate check.
> 2. This is really fragile i think. What about preprocessor-driven emptiness?
1. It seems to me that if I produce fixit to concat empty namespace (that is, 
execute the `else` branch), the fix is not applied as I intended - the 
namespace gets deleted.
I do not fully understand this behavior and did not manage to find bug in the 
code. I also checked what will happen if I try replace the entire namespace 
with `namespace {}` and it is not getting inserted but simply deleted. On the 
other hand, I if replace with `namespace {;}`, I see the expected output. So, 
either it is really sneaky off-by-one somewhere, or clang does it's own thing.
Truth be told, I added that `if` branch to handle such behavior explicitly 
instead of silently deleting namespace.

2. Can you provide an example when it will matter?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52136



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

[PATCH] D52137: Added warning for unary minus used with unsigned type

2018-09-15 Thread Dávid Bolvanský via Phabricator via cfe-commits

xbolva00 added a subscriber: RKSimon.
xbolva00 added a comment.

/home/xbolva00/LLVM/build/lib/clang/8.0.0/include/bmiintrin.h:312:16: error: 
unary minus operator applied to type 'unsigned long long', result value is 
still unsigned

  return __X & -__X;

@RKSimon what do you think?


Repository:
  rC Clang

https://reviews.llvm.org/D52137



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

[PATCH] D52137: Added warning for unary minus used with unsigned type

2018-09-15 Thread Dávid Bolvanský via Phabricator via cfe-commits

xbolva00 updated this revision to Diff 165651.

https://reviews.llvm.org/D52137

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/OpenMP/teams_thread_limit_messages.cpp
  test/Sema/unary-minus-unsigned.c


Index: test/Sema/unary-minus-unsigned.c
===
--- test/Sema/unary-minus-unsigned.c
+++ test/Sema/unary-minus-unsigned.c
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+
+int test1(void) {
+unsigned int a = 1;
+unsigned long b = 2;
+unsigned long long c = 1;
+ 
+unsigned int a2 = -a; // expected-error {{unary minus operator applied to 
type 'unsigned int', result value is still unsigned}}
+unsigned long b2 = -b; // expected-error {{unary minus operator applied to 
type 'unsigned long', result value is still unsigned}}
+unsigned long long c2 = -c; // expected-error {{unary minus operator 
applied to type 'unsigned long long', result value is still unsigned}}
+
+int a3 = -a; // expected-error {{unary minus operator applied to type 
'unsigned int', result value is still unsigned}}
+long b3 = -b; // expected-error {{unary minus operator applied to type 
'unsigned long', result value is still unsigned}}
+long long c3 = -c; // expected-error {{unary minus operator applied to 
type 'unsigned long long', result value is still unsigned}}
+ }
Index: test/OpenMP/teams_thread_limit_messages.cpp
===
--- test/OpenMP/teams_thread_limit_messages.cpp
+++ test/OpenMP/teams_thread_limit_messages.cpp
@@ -51,7 +51,7 @@
 #pragma omp teams thread_limit(-2) // expected-error {{argument to 
'thread_limit' clause must be a strictly positive integer value}}
   foo();
 #pragma omp target
-#pragma omp teams thread_limit(-10u)
+#pragma omp teams thread_limit(-10u) // expected-error {{unary minus operator 
applied to type 'unsigned int', result value is still unsigned}}
   foo();
 #pragma omp target
 #pragma omp teams thread_limit(3.14) // expected-error 2 {{expression must 
have integral or unscoped enumeration type, not 'double'}}
@@ -102,7 +102,7 @@
   foo();
 
 #pragma omp target
-#pragma omp teams thread_limit (-10u)
+#pragma omp teams thread_limit (-10u) // expected-error {{unary minus operator 
applied to type 'unsigned int', result value is still unsigned}}
   foo();
 
 #pragma omp target
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12646,8 +12646,12 @@
 resultType = Input.get()->getType();
 if (resultType->isDependentType())
   break;
-if (resultType->isArithmeticType()) // C99 6.5.3.3p1
+if (resultType->isArithmeticType()) { // C99 6.5.3.3p1
+  if (Opc == UO_Minus && resultType->isUnsignedIntegerType())
+return ExprError(Diag(OpLoc, diag::err_unsignedtypecheck_unary_minus)
+ << resultType << Input.get()->getSourceRange());
   break;
+}
 else if (resultType->isVectorType() &&
  // The z vector extensions don't allow + or - with bool vectors.
  (!Context.getLangOpts().ZVector ||
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -5820,6 +5820,8 @@
   "taking the address of a destructor">;
 def err_typecheck_unary_expr : Error<
   "invalid argument type %0 to unary expression">;
+def err_unsignedtypecheck_unary_minus : Error<
+  "unary minus operator applied to type %0, result value is still unsigned">;
 def err_typecheck_indirection_requires_pointer : Error<
   "indirection requires pointer operand (%0 invalid)">;
 def ext_typecheck_indirection_through_void_pointer : ExtWarn<


Index: test/Sema/unary-minus-unsigned.c
===
--- test/Sema/unary-minus-unsigned.c
+++ test/Sema/unary-minus-unsigned.c
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+
+int test1(void) {
+unsigned int a = 1;
+unsigned long b = 2;
+unsigned long long c = 1;
+ 
+unsigned int a2 = -a; // expected-error {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
+unsigned long b2 = -b; // expected-error {{unary minus operator applied to type 'unsigned long', result value is still unsigned}}
+unsigned long long c2 = -c; // expected-error {{unary minus operator applied to type 'unsigned long long', result value is still unsigned}}
+
+int a3 = -a; // expected-error {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
+long b3 = -b; // expected-error {{unary minus operator applied to type 'unsigned long', result value is still unsigned}}
+long long c3 = -c; // expected-error {{unary minus operator applied

[PATCH] D52137: Added warning for unary minus used with unsigned type

2018-09-15 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added a comment.

I don't think this makes much sense.

In https://reviews.llvm.org/D52137#1236011, @xbolva00 wrote:

> /home/xbolva00/LLVM/build/lib/clang/8.0.0/include/bmiintrin.h:312:16: error: 
> unary minus operator applied to type 'unsigned long long', result value is 
> still unsigned
>
>   return __X & -__X;
>   
>
> @RKSimon what do you think? valid?


https://godbolt.org/z/2n3lQp <- i'm not sure why the second one is better, how 
how the first one is broken.




Comment at: lib/Sema/SemaExpr.cpp:12651
+  if (Opc == UO_Minus && resultType->isUnsignedIntegerType())
+return ExprError(Diag(OpLoc, diag::err_unsignedtypecheck_unary_minus)
+ << resultType << Input.get()->getSourceRange());

Why is this an error?


https://reviews.llvm.org/D52137



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

[PATCH] D52137: Added warning for unary minus used with unsigned type

2018-09-15 Thread Dávid Bolvanský via Phabricator via cfe-commits

xbolva00 added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:12651
+  if (Opc == UO_Minus && resultType->isUnsignedIntegerType())
+return ExprError(Diag(OpLoc, diag::err_unsignedtypecheck_unary_minus)
+ << resultType << Input.get()->getSourceRange());

lebedev.ri wrote:
> Why is this an error?
Ok, I will switch to warning.


https://reviews.llvm.org/D52137



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

[PATCH] D52137: Added warning for unary minus used with unsigned type

2018-09-15 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added a comment.

In https://reviews.llvm.org/D52137#1236011, @xbolva00 wrote:

> /home/xbolva00/LLVM/build/lib/clang/8.0.0/include/bmiintrin.h:312:16: error: 
> unary minus operator applied to type 'unsigned long long', result value is 
> still unsigned
>
>   return __X & -__X;
>   
>
> @RKSimon what do you think? valid? Can we fix that header to return __X & 
> -(int)__X;?
>  @craig.topper

That is not correct transformation as far as i can see.
https://godbolt.org/z/qbQDSq
https://rise4fun.com/Alive/wsPn

https://reviews.llvm.org/D52137

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

[PATCH] D52137: Added warning for unary minus used with unsigned type

2018-09-15 Thread Dávid Bolvanský via Phabricator via cfe-commits

xbolva00 added a comment.

The first is not broken, but it is now detected by this new check. I think the 
second form is more explicit and better.


https://reviews.llvm.org/D52137



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

[PATCH] D52137: Added warning for unary minus used with unsigned type

2018-09-15 Thread Dávid Bolvanský via Phabricator via cfe-commits

xbolva00 added a comment.

@RKSimon are fine to rewrite X & -X to X & ((~X)+1) in bmiintrin.h?


https://reviews.llvm.org/D52137



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

[PATCH] D52137: Added warning for unary minus used with unsigned type

2018-09-15 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added a comment.

> found some occurrences of this expression on our code base.

Standard question: what is the SNR of this warning? What value it brings?
How many times it fired? How many of these are actual real bugs?


https://reviews.llvm.org/D52137



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

[PATCH] D52093: [ToolChains] Linux.cpp: Use the same style across all all multi-arch includes. NFC.

2018-09-15 Thread Kristina Brooks via Phabricator via cfe-commits

kristina added a comment.

Yeah but what about the rest of them, most are completely on their own line, 
some no newlines, mipsel64 was split across two lines while others weren't, and 
switch is meant to have default as the topmost case IIRC. I mean I'm if you 
don't think it's more readable that way I'll abandon it. Though IMHO reading 
wise it's much easier to read as a list down with separators than as a mix of 
various newline/no-newline styles.


Repository:
  rC Clang

https://reviews.llvm.org/D52093



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

[PATCH] D52137: Added warning for unary minus used with unsigned type

2018-09-15 Thread Dávid Bolvanský via Phabricator via cfe-commits

xbolva00 added a comment.

15-20 times / 10 000 lines. I checked and first two are bugs, I don't track 
other warnings yet.

Anyway, I think this pattern should be diagnosed, as confusing and potentially 
buggy.


https://reviews.llvm.org/D52137



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

[PATCH] D52137: Added warning for unary minus used with unsigned type

2018-09-15 Thread Dávid Bolvanský via Phabricator via cfe-commits

xbolva00 updated this revision to Diff 165654.
xbolva00 added a comment.

1. Error -> Warning


https://reviews.llvm.org/D52137

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/CXX/drs/dr6xx.cpp
  test/OpenMP/teams_distribute_thread_limit_messages.cpp
  test/OpenMP/teams_thread_limit_messages.cpp
  test/Sema/unary-minus-unsigned.c

Index: test/Sema/unary-minus-unsigned.c
===
--- test/Sema/unary-minus-unsigned.c
+++ test/Sema/unary-minus-unsigned.c
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+
+void test1(void) {
+unsigned int a = 1;
+unsigned long b = 2;
+unsigned long long c = 1;
+ 
+unsigned int a2 = -a; // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
+unsigned long b2 = -b; // expected-warning {{unary minus operator applied to type 'unsigned long', result value is still unsigned}}
+unsigned long long c2 = -c; // expected-warning {{unary minus operator applied to type 'unsigned long long', result value is still unsigned}}
+
+int a3 = -a; // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
+long b3 = -b; // expected-warning {{unary minus operator applied to type 'unsigned long', result value is still unsigned}}
+long long c3 = -c; // expected-warning {{unary minus operator applied to type 'unsigned long long', result value is still unsigned}}
+ }
Index: test/OpenMP/teams_thread_limit_messages.cpp
===
--- test/OpenMP/teams_thread_limit_messages.cpp
+++ test/OpenMP/teams_thread_limit_messages.cpp
@@ -51,7 +51,7 @@
 #pragma omp teams thread_limit(-2) // expected-error {{argument to 'thread_limit' clause must be a strictly positive integer value}}
   foo();
 #pragma omp target
-#pragma omp teams thread_limit(-10u)
+#pragma omp teams thread_limit(-10u) // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
   foo();
 #pragma omp target
 #pragma omp teams thread_limit(3.14) // expected-error 2 {{expression must have integral or unscoped enumeration type, not 'double'}}
@@ -102,7 +102,7 @@
   foo();
 
 #pragma omp target
-#pragma omp teams thread_limit (-10u)
+#pragma omp teams thread_limit (-10u) // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
   foo();
 
 #pragma omp target
Index: test/OpenMP/teams_distribute_thread_limit_messages.cpp
===
--- test/OpenMP/teams_distribute_thread_limit_messages.cpp
+++ test/OpenMP/teams_distribute_thread_limit_messages.cpp
@@ -51,7 +51,7 @@
 #pragma omp teams distribute thread_limit(-2) // expected-error {{argument to 'thread_limit' clause must be a strictly positive integer value}}
   for (int j=0; j<100; j++) foo();
 #pragma omp target
-#pragma omp teams distribute thread_limit(-10u)
+#pragma omp teams distribute thread_limit(-10u) // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
   for (int j=0; j<100; j++) foo();
 #pragma omp target
 #pragma omp teams distribute thread_limit(3.14) // expected-error 2 {{expression must have integral or unscoped enumeration type, not 'double'}}
@@ -102,7 +102,7 @@
   for (int j=0; j<100; j++) foo();
 
 #pragma omp target
-#pragma omp teams distribute thread_limit (-10u)
+#pragma omp teams distribute thread_limit (-10u) // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
   for (int j=0; j<100; j++) foo();
 
 #pragma omp target
Index: test/CXX/drs/dr6xx.cpp
===
--- test/CXX/drs/dr6xx.cpp
+++ test/CXX/drs/dr6xx.cpp
@@ -91,7 +91,8 @@
   struct D : B, C {};
 }
 
-int dr610[-0u == 0u ? 1 : -1]; // dr610: yes
+int dr610[-0u == 0u ? // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
+  1 : -1]; // dr610: yes
 
 namespace dr611 { // dr611: yes
   int k;
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12646,8 +12646,12 @@
 resultType = Input.get()->getType();
 if (resultType->isDependentType())
   break;
-if (resultType->isArithmeticType()) // C99 6.5.3.3p1
+if (resultType->isArithmeticType()) { // C99 6.5.3.3p1
+  if (Opc == UO_Minus && resultType->isUnsignedIntegerType())
+return Diag(OpLoc, diag::warn_unsignedtypecheck_unary_minus)
+   << resultType << Input.get()->getSourceRange();
   break;
+}
 else if (resultType->isVectorType() &&
  // The z vector extensions don't allow + or - with bool vectors.
  (!Context.getLangOpts().

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

2018-09-15 Thread Wojtek Gumuła via Phabricator via cfe-commits

wgml updated this revision to Diff 165658.
wgml marked 12 inline comments as done.
wgml added a comment.

Adjusted to review comments.


https://reviews.llvm.org/D52136

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
  clang-tidy/modernize/ConcatNestedNamespacesCheck.h
  clang-tidy/modernize/ModernizeTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
  test/clang-tidy/modernize-concat-nested-namespaces.cpp

Index: test/clang-tidy/modernize-concat-nested-namespaces.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-concat-nested-namespaces.cpp
@@ -0,0 +1,161 @@
+// RUN: %check_clang_tidy %s modernize-concat-nested-namespaces %t -- -- -std=c++17
+
+namespace n1 {}
+
+namespace n2 {
+namespace n3 {
+void t();
+}
+namespace n4 {
+void t();
+}
+} // namespace n2
+
+namespace n5 {
+inline namespace n6 {
+void t();
+}
+} // namespace n5
+
+namespace n7 {
+void t();
+
+namespace n8 {
+void t();
+}
+} // namespace n7
+
+namespace n9 {
+namespace n10 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n9::n10
+void t();
+} // namespace n10
+} // namespace n9
+// CHECK-FIXES: }
+
+namespace n11 {
+namespace n12 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n11::n12
+namespace n13 {
+void t();
+}
+namespace n14 {
+void t();
+}
+} // namespace n12
+} // namespace n11
+// CHECK-FIXES: }
+
+namespace n15 {
+namespace n16 {
+void t();
+}
+
+inline namespace n17 {
+void t();
+}
+
+namespace n18 {
+namespace n19 {
+namespace n20 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n18::n19::n20
+void t();
+} // namespace n20
+} // namespace n19
+} // namespace n18
+// CHECK-FIXES: }
+
+namespace n21 {
+void t();
+}
+} // namespace n15
+
+namespace n22 {
+namespace {
+void t();
+}
+} // namespace n22
+
+namespace n23 {
+namespace {
+namespace n24 {
+namespace n25 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n24::n25
+void t();
+} // namespace n25
+} // namespace n24
+// CHECK-FIXES: }
+} // namespace
+} // namespace n23
+
+namespace n26::n27 {
+namespace n28 {
+namespace n29::n30 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n26::n27::n28::n29::n30
+void t() {}
+} // namespace n29::n30
+} // namespace n28
+} // namespace n26::n27
+// CHECK-FIXES: }
+
+namespace n31 {
+namespace n32 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+} // namespace n31
+// CHECK-FIXES-EMPTY
+
+namespace n33 {
+namespace n34 {
+namespace n35 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+} // namespace n34
+// CHECK-FIXES-EMPTY
+namespace n36 {
+void t();
+}
+} // namespace n33
+
+namespace n37::n38 {
+void t();
+}
+
+#define IEXIST
+namespace n39 {
+namespace n40 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n39::n40
+#ifdef IEXIST
+void t() {}
+#endif
+} // namespace n40
+} // namespace n39
+// CHECK-FIXES: }
+
+namespace n41 {
+namespace n42 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n41::n42
+#ifdef IDONTEXIST
+void t() {}
+#endif
+} // namespace n42
+} // namespace n41
+// CHECK-FIXES: }
+
+int main() {
+  n26::n27::n28::n29::n30::t();
+#ifdef IEXIST
+  n39::n40::t();
+#endif
+
+#ifdef IDONTEXIST
+  n41::n42::t();
+#endif
+
+  return 0;
+}
Index: docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
@@ -0,0 +1,49 @@
+.. title:: clang-tidy - modernize-concat-nested-namespaces
+
+modernize-concat-nested-namespaces
+==
+
+Checks for use of nested namespaces in a form of ``namespace a { namespace b { ... } }``
+and offers change to syntax introduced in C++17: ``namespace a::b { ... }``.
+Inlined namespaces are not modified.
+
+For example:
+
+.. code-block:: c++
+
+  namspace n1 {
+  namespace n2 {
+  void t();
+  }
+  }
+
+  namespace n3 {
+  namespace n4 {
+  namespace n5 {
+  void t();
+  }
+  }
+  namespace n6 {
+  namespace n7 {
+  void t();
+  }
+  }
+  }
+
+Will be modified to:
+
+.. code-block:: c++
+
+  n

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

2018-09-15 Thread Wojtek Gumuła via Phabricator via cfe-commits

wgml added inline comments.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:102-108
+  if (childrenCount(ND.decls()) == 0) {
+SourceRange Removal(Namespaces.front().Begin, Namespaces.front().RBrace);
+
+diag(Namespaces.front().Begin,
+ "Nested namespaces are empty and can be removed",
+ DiagnosticIDs::Warning)
+<< FixItHint::CreateRemoval(Removal);

wgml wrote:
> lebedev.ri wrote:
> > 1. This is unrelated to the main check, so it should be in a separate check.
> > 2. This is really fragile i think. What about preprocessor-driven emptiness?
> 1. It seems to me that if I produce fixit to concat empty namespace (that is, 
> execute the `else` branch), the fix is not applied as I intended - the 
> namespace gets deleted.
> I do not fully understand this behavior and did not manage to find bug in the 
> code. I also checked what will happen if I try replace the entire namespace 
> with `namespace {}` and it is not getting inserted but simply deleted. On the 
> other hand, I if replace with `namespace {;}`, I see the expected output. So, 
> either it is really sneaky off-by-one somewhere, or clang does it's own thing.
> Truth be told, I added that `if` branch to handle such behavior explicitly 
> instead of silently deleting namespace.
> 
> 2. Can you provide an example when it will matter?
I dropped the "support" for removing. However, empty namespaces are still being 
removed implicitly, by a fixit applying tool, I believe.

I added entries to the test to make sure preprocessor stuff is never removed.


https://reviews.llvm.org/D52136



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

[PATCH] D52137: Added warning for unary minus used with unsigned type

2018-09-15 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added a comment.

I think we can and should do better about false positives here. If you move 
this check to SemaChecking, you can produce the warning in a context where you 
know what the final type is -- I don't think there's any reason to warn if the 
final type is signed and no wider than the promoted type of the negation.

I think we should also not warn when the negation is an operand of a & or | 
operator whose width is no greater than the promoted type of the negation, 
because -power_of_2 is an idiomatic way to form a mask.


https://reviews.llvm.org/D52137



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

[libclc] r342337 - .travis: Use source whitelist alias for llvm-6 repository

2018-09-15 Thread Jan Vesely via cfe-commits

Author: jvesely
Date: Sat Sep 15 13:00:12 2018
New Revision: 342337

URL: http://llvm.org/viewvc/llvm-project?rev=342337&view=rev
Log:
.travis: Use source whitelist alias for llvm-6 repository

Fixes issue with unauthenticated packages.
Signed-off-by: Jan Vesely 
Reviewer: Aaron Watry

Modified:
libclc/trunk/.travis.yml

Modified: libclc/trunk/.travis.yml
URL: 
http://llvm.org/viewvc/llvm-project/libclc/trunk/.travis.yml?rev=342337&r1=342336&r2=342337&view=diff
==
--- libclc/trunk/.travis.yml (original)
+++ libclc/trunk/.travis.yml Sat Sep 15 13:00:12 2018
@@ -64,7 +64,7 @@ matrix:
   addons:
 apt:
   sources:
-- sourceline: 'deb http://apt.llvm.org/trusty/ 
llvm-toolchain-trusty-6.0 main'
+- llvm-toolchain-trusty-6.0
 - ubuntu-toolchain-r-test
   packages:
 - libedit-dev


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

[libclc] r342338 - .travis: Add llvm-7 build

2018-09-15 Thread Jan Vesely via cfe-commits

Author: jvesely
Date: Sat Sep 15 13:00:37 2018
New Revision: 342338

URL: http://llvm.org/viewvc/llvm-project?rev=342338&view=rev
Log:
.travis: Add llvm-7 build

Signed-off-by: Jan Vesely 
Reviewer: Aaron Watry

Modified:
libclc/trunk/.travis.yml

Modified: libclc/trunk/.travis.yml
URL: 
http://llvm.org/viewvc/llvm-project/libclc/trunk/.travis.yml?rev=342338&r1=342337&r2=342338&view=diff
==
--- libclc/trunk/.travis.yml (original)
+++ libclc/trunk/.travis.yml Sat Sep 15 13:00:37 2018
@@ -73,6 +73,26 @@ matrix:
 # From sources above
 - llvm-6.0-dev
 - clang-6.0
+- env:
+- LABEL="make gcc LLVM-7"
+- LLVM_VERSION=7
+- LLVM_CONFIG="llvm-config-${LLVM_VERSION}"
+- CHECK_FILES="barts-r600--.bc cayman-r600--.bc cedar-r600--.bc 
cypress-r600--.bc tahiti-amdgcn--.bc amdgcn--amdhsa.bc nvptx--nvidiacl.bc 
nvptx64--nvidiacl.bc"
+# llvm passes -Werror=date-time which is only supported in gcc-4.9+
+- MATRIX_EVAL="CC=gcc-4.9 && CXX=g++-4.9"
+  addons:
+apt:
+  sources:
+- sourceline: 'deb http://apt.llvm.org/trusty/ 
llvm-toolchain-trusty-7 main'
+  key_url: https://apt.llvm.org/llvm-snapshot.gpg.key
+- ubuntu-toolchain-r-test
+  packages:
+- libedit-dev
+# LLVM-7 needs libstdc++4.9
+- g++-4.9
+# From sources above
+- llvm-7-dev
+- clang-7
 
 before_install:
 - eval "${MATRIX_EVAL}"


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

[PATCH] D52137: Added warning for unary minus used with unsigned type

2018-09-15 Thread Arthur O'Dwyer via Phabricator via cfe-commits

Quuxplusone added a comment.

In https://reviews.llvm.org/D52137#1236053, @rsmith wrote:

> I think we can and should do better about false positives here. If you move 
> this check to SemaChecking, you can produce the warning in a context where 
> you know what the final type is -- I don't think there's any reason to warn 
> if the final type is signed and no wider than the promoted type of the 
> negation.

I share your general concern about false positives, but in the specific case 
you mentioned—

  void use(int16_t x)
  uint8_t u8 = 1;
  use(-u8);

—I think it'd be surprising to maybe 50% of average programmers that `x`'s 
received value wasn't `int16_t(255)` but rather `int16_t(-1)`. The only cases 
I'd personally consider "false positives" would be cases where `-u32` was being 
used as a convenient shorthand for `~u32 + 1` a.k.a. `(0u - u32)`... but maybe 
in those cases it wouldn't be much of a burden for the programmer to just 
accept a fixit to `(0u - u32)` and be done with it.

https://reviews.llvm.org/D52137

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

[PATCH] D52137: Added warning for unary minus used with unsigned type

2018-09-15 Thread Dávid Bolvanský via Phabricator via cfe-commits

xbolva00 added a comment.

I think we can emit fixit hint always, not even for -u32 case :)


https://reviews.llvm.org/D52137



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

[PATCH] D52093: [ToolChains] Linux.cpp: Use the same style across all all multi-arch includes. NFC.

2018-09-15 Thread Sam McCall via Phabricator via cfe-commits

sammccall added a comment.

I don't have an opinion on the style per se, but please use clang-format. If it 
doesn't produce the formatting you want, then change the code so it does. (e.g. 
adding a trailing , at the end of an init-list will force one-per-line).


Repository:
  rC Clang

https://reviews.llvm.org/D52093



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

r342340 - [NFC] cosmetic tweaks to ExprMutationAnalyzer to be more consistent

2018-09-15 Thread Shuai Wang via cfe-commits

Author: shuaiwang
Date: Sat Sep 15 14:38:18 2018
New Revision: 342340

URL: http://llvm.org/viewvc/llvm-project?rev=342340&view=rev
Log:
[NFC] cosmetic tweaks to ExprMutationAnalyzer to be more consistent
especially considering future changes.

Modified:
cfe/trunk/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
cfe/trunk/lib/Analysis/ExprMutationAnalyzer.cpp

Modified: cfe/trunk/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h?rev=342340&r1=342339&r2=342340&view=diff
==
--- cfe/trunk/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h (original)
+++ cfe/trunk/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h Sat Sep 15 
14:38:18 2018
@@ -26,12 +26,14 @@ public:
   ExprMutationAnalyzer(const Stmt &Stm, ASTContext &Context)
   : Stm(Stm), Context(Context) {}
 
-  bool isMutated(const Decl *Dec) { return findDeclMutation(Dec) != nullptr; }
   bool isMutated(const Expr *Exp) { return findMutation(Exp) != nullptr; }
+  bool isMutated(const Decl *Dec) { return findMutation(Dec) != nullptr; }
   const Stmt *findMutation(const Expr *Exp);
-  const Stmt *findDeclMutation(const Decl *Dec);
+  const Stmt *findMutation(const Decl *Dec);
 
 private:
+  using ResultMap = llvm::DenseMap;
+
   bool isUnevaluated(const Expr *Exp);
 
   const Stmt *findExprMutation(ArrayRef Matches);
@@ -50,7 +52,7 @@ private:
   llvm::DenseMap>
   FuncParmAnalyzer;
-  llvm::DenseMap Results;
+  ResultMap Results;
 };
 
 // A convenient wrapper around ExprMutationAnalyzer for analyzing function

Modified: cfe/trunk/lib/Analysis/ExprMutationAnalyzer.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/ExprMutationAnalyzer.cpp?rev=342340&r1=342339&r2=342340&view=diff
==
--- cfe/trunk/lib/Analysis/ExprMutationAnalyzer.cpp (original)
+++ cfe/trunk/lib/Analysis/ExprMutationAnalyzer.cpp Sat Sep 15 14:38:18 2018
@@ -131,13 +131,13 @@ ExprMutationAnalyzer::findExprMutation(A
 const Stmt *
 ExprMutationAnalyzer::findDeclMutation(ArrayRef Matches) {
   for (const auto &DeclNodes : Matches) {
-if (const Stmt *S = findDeclMutation(DeclNodes.getNodeAs("decl")))
+if (const Stmt *S = findMutation(DeclNodes.getNodeAs("decl")))
   return S;
   }
   return nullptr;
 }
 
-const Stmt *ExprMutationAnalyzer::findDeclMutation(const Decl *Dec) {
+const Stmt *ExprMutationAnalyzer::findMutation(const Decl *Dec) {
   const auto Refs = match(
   findAll(declRefExpr(to(equalsNode(Dec))).bind("expr")), Stm, Context);
   for (const auto &RefNodes : Refs) {
@@ -280,15 +280,14 @@ const Stmt *ExprMutationAnalyzer::findRe
   // Follow non-const reference returned by `operator*()` of move-only classes.
   // These are typically smart pointers with unique ownership so we treat
   // mutation of pointee as mutation of the smart pointer itself.
-  const auto Ref = match(
-  findAll(cxxOperatorCallExpr(
-  hasOverloadedOperatorName("*"),
-  callee(cxxMethodDecl(ofClass(isMoveOnly()),
-   returns(hasUnqualifiedDesugaredType(
-   nonConstReferenceType(),
-  argumentCountIs(1), hasArgument(0, equalsNode(Exp)))
-  .bind("expr")),
-  Stm, Context);
+  const auto Ref =
+  match(findAll(cxxOperatorCallExpr(
+hasOverloadedOperatorName("*"),
+callee(cxxMethodDecl(ofClass(isMoveOnly()),
+ 
returns(nonConstReferenceType(,
+argumentCountIs(1), hasArgument(0, equalsNode(Exp)))
+.bind("expr")),
+Stm, Context);
   if (const Stmt *S = findExprMutation(Ref))
 return S;
 
@@ -370,7 +369,7 @@ FunctionParmMutationAnalyzer::FunctionPa
   for (const ParmVarDecl *Parm : Ctor->parameters()) {
 if (Results.find(Parm) != Results.end())
   continue;
-if (const Stmt *S = InitAnalyzer.findDeclMutation(Parm))
+if (const Stmt *S = InitAnalyzer.findMutation(Parm))
   Results[Parm] = S;
   }
 }
@@ -383,7 +382,7 @@ FunctionParmMutationAnalyzer::findMutati
   if (Memoized != Results.end())
 return Memoized->second;
 
-  if (const Stmt *S = BodyAnalyzer.findDeclMutation(Parm))
+  if (const Stmt *S = BodyAnalyzer.findMutation(Parm))
 return Results[Parm] = S;
 
   return Results[Parm] = nullptr;


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

[PATCH] D52141: Thread safety analysis: Run more tests with capability attributes [NFC]

2018-09-15 Thread Aaron Puchert via Phabricator via cfe-commits

aaronpuchert created this revision.
aaronpuchert added reviewers: aaron.ballman, delesley, grooverdan.
Herald added a subscriber: cfe-commits.

We run the tests for -Wthread-safety-{negative,verbose} with the new
attributes as well as the old ones. Also put the macros in a header so
that we don't have to copy them all around.

The warn-thread-safety-parsing.cpp test checks for warnings depending on
the actual attribute name, so it can't undergo the same treatment.

Together with https://reviews.llvm.org/D49275 this should fix PR33754.


Repository:
  rC Clang

https://reviews.llvm.org/D52141

Files:
  test/SemaCXX/thread-safety-annotations.h
  test/SemaCXX/warn-thread-safety-analysis.cpp
  test/SemaCXX/warn-thread-safety-negative.cpp
  test/SemaCXX/warn-thread-safety-verbose.cpp

Index: test/SemaCXX/warn-thread-safety-verbose.cpp
===
--- test/SemaCXX/warn-thread-safety-verbose.cpp
+++ test/SemaCXX/warn-thread-safety-verbose.cpp
@@ -1,37 +1,15 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wthread-safety-verbose -Wno-thread-safety-negative -fcxx-exceptions %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wthread-safety-verbose -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=0 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wthread-safety-verbose -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s
 
-#define LOCKABLE__attribute__ ((lockable))
-#define SCOPED_LOCKABLE __attribute__ ((scoped_lockable))
-#define GUARDED_BY(x)   __attribute__ ((guarded_by(x)))
-#define GUARDED_VAR __attribute__ ((guarded_var))
-#define PT_GUARDED_BY(x)__attribute__ ((pt_guarded_by(x)))
-#define PT_GUARDED_VAR  __attribute__ ((pt_guarded_var))
-#define ACQUIRED_AFTER(...) __attribute__ ((acquired_after(__VA_ARGS__)))
-#define ACQUIRED_BEFORE(...) __attribute__ ((acquired_before(__VA_ARGS__)))
-#define EXCLUSIVE_LOCK_FUNCTION(...)__attribute__ ((exclusive_lock_function(__VA_ARGS__)))
-#define SHARED_LOCK_FUNCTION(...)   __attribute__ ((shared_lock_function(__VA_ARGS__)))
-#define ASSERT_EXCLUSIVE_LOCK(...)  __attribute__ ((assert_exclusive_lock(__VA_ARGS__)))
-#define ASSERT_SHARED_LOCK(...) __attribute__ ((assert_shared_lock(__VA_ARGS__)))
-#define EXCLUSIVE_TRYLOCK_FUNCTION(...) __attribute__ ((exclusive_trylock_function(__VA_ARGS__)))
-#define SHARED_TRYLOCK_FUNCTION(...)__attribute__ ((shared_trylock_function(__VA_ARGS__)))
-#define UNLOCK_FUNCTION(...)__attribute__ ((unlock_function(__VA_ARGS__)))
-#define LOCK_RETURNED(x)__attribute__ ((lock_returned(x)))
-#define LOCKS_EXCLUDED(...) __attribute__ ((locks_excluded(__VA_ARGS__)))
-#define EXCLUSIVE_LOCKS_REQUIRED(...) \
-  __attribute__ ((exclusive_locks_required(__VA_ARGS__)))
-#define SHARED_LOCKS_REQUIRED(...) \
-  __attribute__ ((shared_locks_required(__VA_ARGS__)))
-#define NO_THREAD_SAFETY_ANALYSIS  __attribute__ ((no_thread_safety_analysis))
+#include "thread-safety-annotations.h"
 
-
-class  __attribute__((lockable)) Mutex {
+class LOCKABLE Mutex {
  public:
-  void Lock() __attribute__((exclusive_lock_function));
-  void ReaderLock() __attribute__((shared_lock_function));
-  void Unlock() __attribute__((unlock_function));
-  bool TryLock() __attribute__((exclusive_trylock_function(true)));
-  bool ReaderTryLock() __attribute__((shared_trylock_function(true)));
-  void LockWhen(const int &cond) __attribute__((exclusive_lock_function));
+  void Lock() EXCLUSIVE_LOCK_FUNCTION();
+  void ReaderLock() SHARED_LOCK_FUNCTION();
+  void Unlock() UNLOCK_FUNCTION();
+  bool TryLock() EXCLUSIVE_TRYLOCK_FUNCTION(true);
+  bool ReaderTryLock() SHARED_TRYLOCK_FUNCTION(true);
 
   // for negative capabilities
   const Mutex& operator!() const { return *this; }
Index: test/SemaCXX/warn-thread-safety-negative.cpp
===
--- test/SemaCXX/warn-thread-safety-negative.cpp
+++ test/SemaCXX/warn-thread-safety-negative.cpp
@@ -1,40 +1,18 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wthread-safety-negative -fcxx-exceptions %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wthread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=0 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wthread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s
 
 // FIXME: should also run  %clang_cc1 -fsyntax-only -verify -Wthread-safety -std=c++11 -Wc++98-compat %s
 // FIXME: should also run  %clang_cc1 -fsyntax-only -verify -Wthread-safety %s
 
-#define LOCKABLE__attribute__ ((lockable))
-#define SCOPED_LOCKABLE __attribute__ ((scoped_lockable))
-#define GUARDED_BY(x)   __attribute__ ((guarded_

[PATCH] D52137: Added warning for unary minus used with unsigned type

2018-09-15 Thread Dávid Bolvanský via Phabricator via cfe-commits

xbolva00 added a comment.

/home/xbolva00/LLVM/llvm/tools/clang/test/Sema/unary-minus-unsigned.c:13:15: 
warning: unary minus operator applied to type 'unsigned long', result value is 
still unsigned [-Wsign-conversion]

  long b3 = -b; // expected-warning {{unary minus operator applied to type 
'unsigned long', result value is still unsigned}}
   ^~
   0u -

Any advice how to get UnaryOperator operand as a string? To emit fixit hint 
like 0u - b

FixItHint::CreateReplacement(InputExpr->getBeginLoc(), "0u - " + ???) << 
Input.get()->getSourceRange();


https://reviews.llvm.org/D52137



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

[libclc] r342341 - configure: Rework support for gfx9+ devices that were added post LLVM 3.9

2018-09-15 Thread Jan Vesely via cfe-commits

Author: jvesely
Date: Sat Sep 15 15:02:01 2018
New Revision: 342341

URL: http://llvm.org/viewvc/llvm-project?rev=342341&view=rev
Log:
configure: Rework support for gfx9+ devices that were added post LLVM 3.9

v2: Fix reference to Vega12/20 enabling commit

Signed-off-by: Jan Vesely 
Reviewer: Aaron Watry

Modified:
libclc/trunk/configure.py

Modified: libclc/trunk/configure.py
URL: 
http://llvm.org/viewvc/llvm-project/libclc/trunk/configure.py?rev=342341&r1=342340&r2=342341&view=diff
==
--- libclc/trunk/configure.py (original)
+++ libclc/trunk/configure.py Sat Sep 15 15:02:01 2018
@@ -100,15 +100,25 @@ available_targets = {
 {'gpu' : 'barts',   'aliases' : ['turks', 'caicos'] },
 {'gpu' : 'cayman',  'aliases' : ['aruba']} ]},
   'amdgcn--': { 'devices' :
-[{'gpu' : 'tahiti', 'aliases' : ['pitcairn', 'verde', 'oland', 
'hainan', 'bonaire', 'kabini', 'kaveri', 'hawaii', 'mullins', 'tonga', 
'iceland', 'carrizo', 'fiji', 'stoney', 'polaris10', 'polaris11', 'gfx900']} ]},
+[{'gpu' : 'tahiti', 'aliases' : ['pitcairn', 'verde', 'oland', 
'hainan', 'bonaire', 'kabini', 'kaveri', 'hawaii', 'mullins', 'tonga', 
'iceland', 'carrizo', 'fiji', 'stoney', 'polaris10', 'polaris11']} ]},
   'amdgcn--amdhsa': { 'devices' :
-  [{'gpu' : '', 'aliases' : ['bonaire', 'kabini', 
'kaveri', 'hawaii', 'mullins', 'tonga', 'iceland', 'carrizo', 'fiji', 'stoney', 
'polaris10', 'polaris11', 'gfx900']} ]},
+  [{'gpu' : '', 'aliases' : ['bonaire', 'kabini', 
'kaveri', 'hawaii', 'mullins', 'tonga', 'iceland', 'carrizo', 'fiji', 'stoney', 
'polaris10', 'polaris11']} ]},
   'nvptx--'   : { 'devices' : [{'gpu' : '', 'aliases' : []} ]},
   'nvptx64--' : { 'devices' : [{'gpu' : '', 'aliases' : []} ]},
   'nvptx--nvidiacl'   : { 'devices' : [{'gpu' : '', 'aliases' : []} ]},
   'nvptx64--nvidiacl' : { 'devices' : [{'gpu' : '', 'aliases' : []} ]},
 }
 
+# Support for gfx9 was added in LLVM 5 (r295554)
+if llvm_int_version >= 500:
+available_targets['amdgcn--']['devices'][0]['aliases'] += ['gfx900', 
'gfx902']
+available_targets['amdgcn--amdhsa']['devices'][0]['aliases'] += ['gfx900', 
'gfx902']
+
+# Support for Vega12 and Vega20 was added in LLVM 7 (r331215)
+if llvm_int_version >= 700:
+available_targets['amdgcn--']['devices'][0]['aliases'] += ['gfx904', 
'gfx906']
+available_targets['amdgcn--amdhsa']['devices'][0]['aliases'] += ['gfx904', 
'gfx906']
+
 
 default_targets = ['nvptx--nvidiacl', 'nvptx64--nvidiacl', 'r600--', 
'amdgcn--', 'amdgcn--amdhsa']
 


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

[PATCH] D52137: Added warning for unary minus used with unsigned type

2018-09-15 Thread Dávid Bolvanský via Phabricator via cfe-commits

xbolva00 updated this revision to Diff 165663.
xbolva00 added a comment.

Added fixit hints


https://reviews.llvm.org/D52137

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/CXX/drs/dr6xx.cpp
  test/OpenMP/teams_distribute_thread_limit_messages.cpp
  test/OpenMP/teams_thread_limit_messages.cpp
  test/Sema/unary-minus-unsigned.c

Index: test/Sema/unary-minus-unsigned.c
===
--- test/Sema/unary-minus-unsigned.c
+++ test/Sema/unary-minus-unsigned.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+
+void test1(void) {
+unsigned int a = 1;
+unsigned long b = 1;
+unsigned long long c = 1;
+ 
+unsigned int a2 = -a; // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
+unsigned long b2 = -b; // expected-warning {{unary minus operator applied to type 'unsigned long', result value is still unsigned}}
+unsigned long long c2 = -c; // expected-warning {{unary minus operator applied to type 'unsigned long long', result value is still unsigned}}
+
+int a3 = -a; // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
+long b3 = -b; // expected-warning {{unary minus operator applied to type 'unsigned long', result value is still unsigned}}
+long long c3 = -c; // expected-warning {{unary minus operator applied to type 'unsigned long long', result value is still unsigned}}
+
+unsigned int a4 = -a;
+unsigned long b4 = -b;
+unsigned long long c4 = -c;
+ }
\ No newline at end of file
Index: test/OpenMP/teams_thread_limit_messages.cpp
===
--- test/OpenMP/teams_thread_limit_messages.cpp
+++ test/OpenMP/teams_thread_limit_messages.cpp
@@ -51,7 +51,7 @@
 #pragma omp teams thread_limit(-2) // expected-error {{argument to 'thread_limit' clause must be a strictly positive integer value}}
   foo();
 #pragma omp target
-#pragma omp teams thread_limit(-10u)
+#pragma omp teams thread_limit(-10u) // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
   foo();
 #pragma omp target
 #pragma omp teams thread_limit(3.14) // expected-error 2 {{expression must have integral or unscoped enumeration type, not 'double'}}
@@ -102,7 +102,7 @@
   foo();
 
 #pragma omp target
-#pragma omp teams thread_limit (-10u)
+#pragma omp teams thread_limit (-10u) // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
   foo();
 
 #pragma omp target
Index: test/OpenMP/teams_distribute_thread_limit_messages.cpp
===
--- test/OpenMP/teams_distribute_thread_limit_messages.cpp
+++ test/OpenMP/teams_distribute_thread_limit_messages.cpp
@@ -51,7 +51,7 @@
 #pragma omp teams distribute thread_limit(-2) // expected-error {{argument to 'thread_limit' clause must be a strictly positive integer value}}
   for (int j=0; j<100; j++) foo();
 #pragma omp target
-#pragma omp teams distribute thread_limit(-10u)
+#pragma omp teams distribute thread_limit(-10u) // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
   for (int j=0; j<100; j++) foo();
 #pragma omp target
 #pragma omp teams distribute thread_limit(3.14) // expected-error 2 {{expression must have integral or unscoped enumeration type, not 'double'}}
@@ -102,7 +102,7 @@
   for (int j=0; j<100; j++) foo();
 
 #pragma omp target
-#pragma omp teams distribute thread_limit (-10u)
+#pragma omp teams distribute thread_limit (-10u) // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
   for (int j=0; j<100; j++) foo();
 
 #pragma omp target
Index: test/CXX/drs/dr6xx.cpp
===
--- test/CXX/drs/dr6xx.cpp
+++ test/CXX/drs/dr6xx.cpp
@@ -91,7 +91,8 @@
   struct D : B, C {};
 }
 
-int dr610[-0u == 0u ? 1 : -1]; // dr610: yes
+int dr610[-0u == 0u ? // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
+  1 : -1]; // dr610: yes
 
 namespace dr611 { // dr611: yes
   int k;
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12646,13 +12646,18 @@
 resultType = Input.get()->getType();
 if (resultType->isDependentType())
   break;
-if (resultType->isArithmeticType()) // C99 6.5.3.3p1
+if (resultType->isArithmeticType()) { // C99 6.5.3.3p1
+  if (Opc == UO_Minus && resultType->isUnsignedIntegerType())
+return Diag(OpLoc, diag::warn_unsignedtypecheck_unary_minus)
+   << resultType << FixItHint::CreateRemoval(OpLoc)
+   << FixItHint::CreateInsertion(InputExpr->getBegi

[PATCH] D51597: [ASTImporter] Fix import of VarDecl init

2018-09-15 Thread Aleksei Sidorin via Phabricator via cfe-commits

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



Comment at: lib/AST/ASTImporter.cpp:1441
+  To->setInit(ToInit);
+  if (From->isInitKnownICE()) {
+EvaluatedStmt *Eval = To->ensureEvaluatedStmt();

martong wrote:
> a_sidorin wrote:
> > I see that this is only a code move but I realized that ASTReader and 
> > ASTImporter handle this case differently. ASTReader code says:
> > 
> > ```
> > if (Val > 1) { // IsInitKnownICE = 1, IsInitNotICE = 2, IsInitICE = 3
> >   EvaluatedStmt *Eval = VD->ensureEvaluatedStmt();
> >   Eval->CheckedICE = true;
> >   Eval->IsICE = Val == 3;
> > }
> > ```
> > but ASTimporter sets these bits only if `isInitKnownICE()` is `true`. This 
> > looks strange.
> The comment in ASTReader seems to be wrong and misleading.
> I checked the correspondent code in ASTWriter:
> ```
> Record.push_back(!D->isInitKnownICE() ? 1 : (D->isInitICE() ? 3 : 2));
> ```
> Thus, the comment in ASTReader should be:
> ```
> if (Val > 1) { // IsInitNOTKnownICE = 1, IsInitNotICE = 2, IsInitICE = 3
> ```
> So, `Val > 1` means that the original init expression written by the 
> ASTWriter had the ICE-ness already determined.
> Thus the ASTImporter code seems correct to me. 
Thank you for checking this!
The reason I was worrying about this code is that ASTReader/Writer are used in 
XTU as well so a mismatch between them and ASTImporter can cost us some 
annoying debug in the future.


Repository:
  rC Clang

https://reviews.llvm.org/D51597



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

[PATCH] D52137: Added warning for unary minus used with unsigned type

2018-09-15 Thread Dávid Bolvanský via Phabricator via cfe-commits

xbolva00 updated this revision to Diff 165664.

https://reviews.llvm.org/D52137

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/CXX/drs/dr6xx.cpp
  test/OpenMP/teams_distribute_thread_limit_messages.cpp
  test/OpenMP/teams_thread_limit_messages.cpp
  test/Sema/unary-minus-unsigned.c

Index: test/Sema/unary-minus-unsigned.c
===
--- test/Sema/unary-minus-unsigned.c
+++ test/Sema/unary-minus-unsigned.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+
+void test1(void) {
+  unsigned int a = 1;
+  unsigned long b = 1;
+  unsigned long long c = 1;
+
+  unsigned int a2 = -a;   // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
+  unsigned long b2 = -b;  // expected-warning {{unary minus operator applied to type 'unsigned long', result value is still unsigned}}
+  unsigned long long c2 = -c; // expected-warning {{unary minus operator applied to type 'unsigned long long', result value is still unsigned}}
+
+  int a3 = -a;   // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
+  long b3 = -b;  // expected-warning {{unary minus operator applied to type 'unsigned long', result value is still unsigned}}
+  long long c3 = -c; // expected-warning {{unary minus operator applied to type 'unsigned long long', result value is still unsigned}}
+
+  unsigned int a4 = -1u; // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
+  unsigned long b4 = -1ul;   // expected-warning {{unary minus operator applied to type 'unsigned long', result value is still unsigned}}
+  unsigned long long c4 = -1ull; // expected-warning {{unary minus operator applied to type 'unsigned long long', result value is still unsigned}}
+}
Index: test/OpenMP/teams_thread_limit_messages.cpp
===
--- test/OpenMP/teams_thread_limit_messages.cpp
+++ test/OpenMP/teams_thread_limit_messages.cpp
@@ -51,7 +51,7 @@
 #pragma omp teams thread_limit(-2) // expected-error {{argument to 'thread_limit' clause must be a strictly positive integer value}}
   foo();
 #pragma omp target
-#pragma omp teams thread_limit(-10u)
+#pragma omp teams thread_limit(-10u) // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
   foo();
 #pragma omp target
 #pragma omp teams thread_limit(3.14) // expected-error 2 {{expression must have integral or unscoped enumeration type, not 'double'}}
@@ -102,7 +102,7 @@
   foo();
 
 #pragma omp target
-#pragma omp teams thread_limit (-10u)
+#pragma omp teams thread_limit (-10u) // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
   foo();
 
 #pragma omp target
Index: test/OpenMP/teams_distribute_thread_limit_messages.cpp
===
--- test/OpenMP/teams_distribute_thread_limit_messages.cpp
+++ test/OpenMP/teams_distribute_thread_limit_messages.cpp
@@ -51,7 +51,7 @@
 #pragma omp teams distribute thread_limit(-2) // expected-error {{argument to 'thread_limit' clause must be a strictly positive integer value}}
   for (int j=0; j<100; j++) foo();
 #pragma omp target
-#pragma omp teams distribute thread_limit(-10u)
+#pragma omp teams distribute thread_limit(-10u) // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
   for (int j=0; j<100; j++) foo();
 #pragma omp target
 #pragma omp teams distribute thread_limit(3.14) // expected-error 2 {{expression must have integral or unscoped enumeration type, not 'double'}}
@@ -102,7 +102,7 @@
   for (int j=0; j<100; j++) foo();
 
 #pragma omp target
-#pragma omp teams distribute thread_limit (-10u)
+#pragma omp teams distribute thread_limit (-10u) // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
   for (int j=0; j<100; j++) foo();
 
 #pragma omp target
Index: test/CXX/drs/dr6xx.cpp
===
--- test/CXX/drs/dr6xx.cpp
+++ test/CXX/drs/dr6xx.cpp
@@ -91,7 +91,8 @@
   struct D : B, C {};
 }
 
-int dr610[-0u == 0u ? 1 : -1]; // dr610: yes
+int dr610[-0u == 0u ? // expected-warning {{unary minus operator applied to type 'unsigned int', result value is still unsigned}}
+  1 : -1]; // dr610: yes
 
 namespace dr611 { // dr611: yes
   int k;
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12646,13 +12646,18 @@
 resultType = Input.get()->getType();
 if (resultType->isDependentType())
   break;
-if (resultType->isArithmeticType()) // C99 6.5.3.3p1
+if (resultType->isArithmeticType()) { // C99 6.5

[PATCH] D52137: Added warning for unary minus used with unsigned type

2018-09-15 Thread Dávid Bolvanský via Phabricator via cfe-commits

xbolva00 added a comment.

Hm, isUnsignedIntegerType seems to ignore uintN_t types?

int f(void) {

  uint8_t u8 = 1;
  uint8_t b = -u8;

}

No warning now :/


https://reviews.llvm.org/D52137



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

[PATCH] D52089: [clangd] Get rid of AST matchers in SymbolCollector. NFC

2018-09-15 Thread Marc-Andre Laperle via Phabricator via cfe-commits

malaperle added a comment.

In https://reviews.llvm.org/D52089#1235851, @ioeric wrote:

> In https://reviews.llvm.org/D52089#1235777, @malaperle wrote:
>
> > why?
>
>
> I wanted to get some numbers and update the patch summary, but somehow 
> forgot. Sorry about that and thanks for asking!
>
> The AST matcher pops up in performance profile. Although not the most 
> expensive thing we could optimize, this still brings `indexAST` latency from 
> ~5.3s to ~4.4s for a large TU with big preamble.


Thanks! That's what I was looking for :) Sounds good to me.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52089



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

44 matches

Mail list logo