[PATCH] D44720: [clangd] Simplify fuzzy matcher (sequence alignment) by removing some condition checks.

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clangd/FuzzyMatch.cpp:93
+  for (int I = PatN; I <= WordN; I++)
+Best = std::max(Best, Scores[PatN][I][Match].Score);
   if (isAwful(Best))

sammccall wrote:
> this looks like a behavior change - why?
This is a behavior change. Instead of choosing between `Match/Miss` in the last 
position, we enumerate the last matching position in `Word`.

This saves `if (P < PatN - 1) {` check in the main loop at the cost of a for 
loop here (use sites of ending values)



Comment at: clangd/FuzzyMatch.cpp:241
 
-  auto MatchMissScore = PreMiss[Match].Score;
-  auto MissMissScore = PreMiss[Miss].Score;
-  if (P < PatN - 1) { // Skipping trailing characters is always free.
-MatchMissScore -= skipPenalty(W, Match);
-MissMissScore -= skipPenalty(W, Miss);
-  }
+  auto MatchMissScore = PreMiss[Match].Score + missScore(W, Match);
+  auto MissMissScore = PreMiss[Miss].Score + missScore(W, Miss);

sammccall wrote:
> adding the penalty unconditionally seems like a behavior change, why?
Because now we use a different method to calculate the final value. I believe 
this makes the loop simpler.

This was not regular because 

Scores[0][W + 1][Miss] = {Scores[0][W][Miss].Score + missScore(W, Miss),
  Miss};

This unconditionally added a trailing penalty but the main loop did not.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44720



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

[PATCH] D44559: [Sema] Wrong width of result of mul operation

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D44559#1044639, @rjmccall wrote:

> In https://reviews.llvm.org/D44559#1044186, @avt77 wrote:
>
> > >> In https://reviews.llvm.org/D44559#1040799, @rjmccall wrote:
> > >> 
> > >>> I think we're correct not to warn here and that GCC/ICC are being 
> > >>> noisy.  The existence of a temporary promotion to a wider type doesn't 
> > >>> justify warning on arithmetic between two operands that are the same 
> > >>> size as the ultimate result.  It is totally fair for users to think of 
> > >>> this operation as being "closed" on the original type.
> > >> 
> > >> 
> > >> Could you please clarify, are you saying that PR35409 
> > >>  is not a bug, and clang 
> > >> should continue to not warn in those cases?
> > > 
> > > Correct.
> >
> > Does it mean we should abandon this revision? On the other hand it's a real 
> > bug, isn't it?
>
>
> Not as I see it, no.


Do you see this code as having a bug when `a` is >= 182?

  short foo(unsigned char a) {
return a * a;
  }

(If you don't like seeing `unsigned char`  you can imagine it was spelled as 
`uint8_t`.)


https://reviews.llvm.org/D44559



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

[PATCH] D44720: [clangd] Simplify fuzzy matcher (sequence alignment) by removing some condition checks.

[Fangrui Song via Phabricator via cfe-commits]

MaskRay updated this revision to Diff 139314.
MaskRay edited the summary of this revision.
MaskRay added a comment.

Update summary


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44720

Files:
  clangd/FuzzyMatch.cpp
  clangd/FuzzyMatch.h

Index: clangd/FuzzyMatch.h
===
--- clangd/FuzzyMatch.h
+++ clangd/FuzzyMatch.h
@@ -58,8 +58,8 @@
   void buildGraph();
   void calculateRoles(const char *Text, CharRole *Out, int , int N);
   bool allowMatch(int P, int W) const;
-  int skipPenalty(int W, Action Last) const;
-  int matchBonus(int P, int W, Action Last) const;
+  int missScore(int W, Action Last) const;
+  int matchScore(int P, int W, Action Last) const;
 
   // Pattern data is initialized by the constructor, then constant.
   char Pat[MaxPat]; // Pattern data
Index: clangd/FuzzyMatch.cpp
===
--- clangd/FuzzyMatch.cpp
+++ clangd/FuzzyMatch.cpp
@@ -58,6 +58,7 @@
 
 #include "FuzzyMatch.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Format.h"
 
 namespace clang {
@@ -67,7 +68,6 @@
 constexpr int FuzzyMatcher::MaxPat;
 constexpr int FuzzyMatcher::MaxWord;
 
-static char lower(char C) { return C >= 'A' && C <= 'Z' ? C + ('a' - 'A') : C; }
 // A "negative infinity" score that won't overflow.
 // We use this to mark unreachable states and forbidden solutions.
 // Score field is 15 bits wide, min value is -2^14, we use half of that.
@@ -80,25 +80,17 @@
   ScoreScale(PatN ? float{1} / (PerfectBonus * PatN) : 0), WordN(0) {
   std::copy(Pattern.begin(), Pattern.begin() + PatN, Pat);
   for (int I = 0; I < PatN; ++I)
-LowPat[I] = lower(Pat[I]);
-  Scores[0][0][Miss] = {0, Miss};
-  Scores[0][0][Match] = {AwfulScore, Miss};
-  for (int P = 0; P <= PatN; ++P)
-for (int W = 0; W < P; ++W)
-  for (Action A : {Miss, Match})
-Scores[P][W][A] = {AwfulScore, Miss};
-  if (PatN > 0)
-calculateRoles(Pat, PatRole, PatTypeSet, PatN);
+LowPat[I] = toLower(Pat[I]);
+  calculateRoles(Pat, PatRole, PatTypeSet, PatN);
 }
 
 Optional FuzzyMatcher::match(StringRef Word) {
   if (!(WordContainsPattern = init(Word)))
 return None;
-  if (!PatN)
-return 1;
   buildGraph();
-  auto Best = std::max(Scores[PatN][WordN][Miss].Score,
-   Scores[PatN][WordN][Match].Score);
+  int Best = AwfulScore;
+  for (int I = PatN; I <= WordN; I++)
+Best = std::max(Best, Scores[PatN][I][Match].Score);
   if (isAwful(Best))
 return None;
   return ScoreScale * std::min(PerfectBonus * PatN, std::max(0, Best));
@@ -179,7 +171,8 @@
 }
 void FuzzyMatcher::calculateRoles(const char *Text, CharRole *Out, int ,
   int N) {
-  assert(N > 0);
+  if (!N)
+return;
   CharType Type = packedLookup(CharTypes, Text[0]);
   TypeSet = 1 << Type;
   // Types holds a sliding window of (Prev, Curr, Next) types.
@@ -206,10 +199,8 @@
   if (PatN > WordN)
 return false;
   std::copy(NewWord.begin(), NewWord.begin() + WordN, Word);
-  if (PatN == 0)
-return true;
   for (int I = 0; I < WordN; ++I)
-LowWord[I] = lower(Word[I]);
+LowWord[I] = toLower(Word[I]);
 
   // Cheap subsequence check.
   for (int W = 0, P = 0; P != PatN; ++W) {
@@ -236,31 +227,29 @@
 // This range is not strict: we can apply larger bonuses/penalties, or penalize
 // non-matched characters.
 void FuzzyMatcher::buildGraph() {
+  Scores[0][0][Miss] = Scores[0][0][Match] = {0, Miss};
   for (int W = 0; W < WordN; ++W) {
-Scores[0][W + 1][Miss] = {Scores[0][W][Miss].Score - skipPenalty(W, Miss),
+Scores[0][W + 1][Miss] = {Scores[0][W][Miss].Score + missScore(W, Miss),
   Miss};
 Scores[0][W + 1][Match] = {AwfulScore, Miss};
   }
   for (int P = 0; P < PatN; ++P) {
+Scores[P + 1][P][Miss] = Scores[P + 1][P][Match] = {AwfulScore, Miss};
 for (int W = P; W < WordN; ++W) {
   auto  = Scores[P + 1][W + 1],  = Scores[P + 1][W];
 
-  auto MatchMissScore = PreMiss[Match].Score;
-  auto MissMissScore = PreMiss[Miss].Score;
-  if (P < PatN - 1) { // Skipping trailing characters is always free.
-MatchMissScore -= skipPenalty(W, Match);
-MissMissScore -= skipPenalty(W, Miss);
-  }
+  auto MatchMissScore = PreMiss[Match].Score + missScore(W, Match);
+  auto MissMissScore = PreMiss[Miss].Score + missScore(W, Miss);
   Score[Miss] = (MatchMissScore > MissMissScore)
 ? ScoreInfo{MatchMissScore, Match}
 : ScoreInfo{MissMissScore, Miss};
 
   if (!allowMatch(P, W)) {
 Score[Match] = {AwfulScore, Miss};
   } else {
 auto  = Scores[P][W];
-auto MatchMatchScore = PreMatch[Match].Score + matchBonus(P, W, Match);
-auto MissMatchScore = PreMatch[Miss].Score + matchBonus(P, W, Miss);
+auto MatchMatchScore = PreMatch[Match].Score + 

[PATCH] D44720: [clangd] Simplify fuzzy matcher (sequence alignment) by removing some condition checks.

[Fangrui Song via Phabricator via cfe-commits]

MaskRay updated this revision to Diff 139313.
MaskRay added a comment.

Update summary


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44720

Files:
  clangd/FuzzyMatch.cpp
  clangd/FuzzyMatch.h

Index: clangd/FuzzyMatch.h
===
--- clangd/FuzzyMatch.h
+++ clangd/FuzzyMatch.h
@@ -58,8 +58,8 @@
   void buildGraph();
   void calculateRoles(const char *Text, CharRole *Out, int , int N);
   bool allowMatch(int P, int W) const;
-  int skipPenalty(int W, Action Last) const;
-  int matchBonus(int P, int W, Action Last) const;
+  int missScore(int W, Action Last) const;
+  int matchScore(int P, int W, Action Last) const;
 
   // Pattern data is initialized by the constructor, then constant.
   char Pat[MaxPat]; // Pattern data
Index: clangd/FuzzyMatch.cpp
===
--- clangd/FuzzyMatch.cpp
+++ clangd/FuzzyMatch.cpp
@@ -58,6 +58,7 @@
 
 #include "FuzzyMatch.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Format.h"
 
 namespace clang {
@@ -67,7 +68,6 @@
 constexpr int FuzzyMatcher::MaxPat;
 constexpr int FuzzyMatcher::MaxWord;
 
-static char lower(char C) { return C >= 'A' && C <= 'Z' ? C + ('a' - 'A') : C; }
 // A "negative infinity" score that won't overflow.
 // We use this to mark unreachable states and forbidden solutions.
 // Score field is 15 bits wide, min value is -2^14, we use half of that.
@@ -80,25 +80,17 @@
   ScoreScale(PatN ? float{1} / (PerfectBonus * PatN) : 0), WordN(0) {
   std::copy(Pattern.begin(), Pattern.begin() + PatN, Pat);
   for (int I = 0; I < PatN; ++I)
-LowPat[I] = lower(Pat[I]);
-  Scores[0][0][Miss] = {0, Miss};
-  Scores[0][0][Match] = {AwfulScore, Miss};
-  for (int P = 0; P <= PatN; ++P)
-for (int W = 0; W < P; ++W)
-  for (Action A : {Miss, Match})
-Scores[P][W][A] = {AwfulScore, Miss};
-  if (PatN > 0)
-calculateRoles(Pat, PatRole, PatTypeSet, PatN);
+LowPat[I] = toLower(Pat[I]);
+  calculateRoles(Pat, PatRole, PatTypeSet, PatN);
 }
 
 Optional FuzzyMatcher::match(StringRef Word) {
   if (!(WordContainsPattern = init(Word)))
 return None;
-  if (!PatN)
-return 1;
   buildGraph();
-  auto Best = std::max(Scores[PatN][WordN][Miss].Score,
-   Scores[PatN][WordN][Match].Score);
+  int Best = AwfulScore;
+  for (int I = PatN; I <= WordN; I++)
+Best = std::max(Best, Scores[PatN][I][Match].Score);
   if (isAwful(Best))
 return None;
   return ScoreScale * std::min(PerfectBonus * PatN, std::max(0, Best));
@@ -179,7 +171,8 @@
 }
 void FuzzyMatcher::calculateRoles(const char *Text, CharRole *Out, int ,
   int N) {
-  assert(N > 0);
+  if (!N)
+return;
   CharType Type = packedLookup(CharTypes, Text[0]);
   TypeSet = 1 << Type;
   // Types holds a sliding window of (Prev, Curr, Next) types.
@@ -206,10 +199,8 @@
   if (PatN > WordN)
 return false;
   std::copy(NewWord.begin(), NewWord.begin() + WordN, Word);
-  if (PatN == 0)
-return true;
   for (int I = 0; I < WordN; ++I)
-LowWord[I] = lower(Word[I]);
+LowWord[I] = toLower(Word[I]);
 
   // Cheap subsequence check.
   for (int W = 0, P = 0; P != PatN; ++W) {
@@ -236,31 +227,29 @@
 // This range is not strict: we can apply larger bonuses/penalties, or penalize
 // non-matched characters.
 void FuzzyMatcher::buildGraph() {
+  Scores[0][0][Miss] = Scores[0][0][Match] = {0, Miss};
   for (int W = 0; W < WordN; ++W) {
-Scores[0][W + 1][Miss] = {Scores[0][W][Miss].Score - skipPenalty(W, Miss),
+Scores[0][W + 1][Miss] = {Scores[0][W][Miss].Score + missScore(W, Miss),
   Miss};
 Scores[0][W + 1][Match] = {AwfulScore, Miss};
   }
   for (int P = 0; P < PatN; ++P) {
+Scores[P + 1][P][Miss] = Scores[P + 1][P][Match] = {AwfulScore, Miss};
 for (int W = P; W < WordN; ++W) {
   auto  = Scores[P + 1][W + 1],  = Scores[P + 1][W];
 
-  auto MatchMissScore = PreMiss[Match].Score;
-  auto MissMissScore = PreMiss[Miss].Score;
-  if (P < PatN - 1) { // Skipping trailing characters is always free.
-MatchMissScore -= skipPenalty(W, Match);
-MissMissScore -= skipPenalty(W, Miss);
-  }
+  auto MatchMissScore = PreMiss[Match].Score + missScore(W, Match);
+  auto MissMissScore = PreMiss[Miss].Score + missScore(W, Miss);
   Score[Miss] = (MatchMissScore > MissMissScore)
 ? ScoreInfo{MatchMissScore, Match}
 : ScoreInfo{MissMissScore, Miss};
 
   if (!allowMatch(P, W)) {
 Score[Match] = {AwfulScore, Miss};
   } else {
 auto  = Scores[P][W];
-auto MatchMatchScore = PreMatch[Match].Score + matchBonus(P, W, Match);
-auto MissMatchScore = PreMatch[Miss].Score + matchBonus(P, W, Miss);
+auto MatchMatchScore = PreMatch[Match].Score + matchScore(P, W, Match);
+auto 

[PATCH] D44559: [Sema] Wrong width of result of mul operation

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D44559#1044186, @avt77 wrote:

> >> In https://reviews.llvm.org/D44559#1040799, @rjmccall wrote:
> >> 
> >>> I think we're correct not to warn here and that GCC/ICC are being noisy.  
> >>> The existence of a temporary promotion to a wider type doesn't justify 
> >>> warning on arithmetic between two operands that are the same size as the 
> >>> ultimate result.  It is totally fair for users to think of this operation 
> >>> as being "closed" on the original type.
> >> 
> >> 
> >> Could you please clarify, are you saying that PR35409 
> >>  is not a bug, and clang 
> >> should continue to not warn in those cases?
> > 
> > Correct.
>
> Does it mean we should abandon this revision? On the other hand it's a real 
> bug, isn't it?


Not as I see it, no.


https://reviews.llvm.org/D44559



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

[PATCH] D44724: [Fuchsia] Don't install libc++, libc++abi or libunwind on Darwin

[Petr Hosek via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL328114: [Fuchsia] Dont install libc++, libc++abi or 
libunwind on Darwin (authored by phosek, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D44724?vs=139311=139312#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D44724

Files:
  cfe/trunk/cmake/caches/Fuchsia-stage2.cmake


Index: cfe/trunk/cmake/caches/Fuchsia-stage2.cmake
===
--- cfe/trunk/cmake/caches/Fuchsia-stage2.cmake
+++ cfe/trunk/cmake/caches/Fuchsia-stage2.cmake
@@ -38,17 +38,28 @@
 set(LLVM_RUNTIME_TARGETS 
"default;x86_64-fuchsia;aarch64-fuchsia;x86_64-fuchsia-asan:x86_64-fuchsia;aarch64-fuchsia-asan:aarch64-fuchsia"
 CACHE STRING "")
 
 # Set the default target runtimes options.
-set(LIBUNWIND_ENABLE_SHARED OFF CACHE BOOL "")
-set(LIBUNWIND_USE_COMPILER_RT ON CACHE BOOL "")
-set(LIBUNWIND_INSTALL_LIBRARY OFF CACHE BOOL "")
-set(LIBCXXABI_USE_COMPILER_RT ON CACHE BOOL "")
-set(LIBCXXABI_ENABLE_SHARED OFF CACHE BOOL "")
-set(LIBCXXABI_USE_LLVM_UNWINDER ON CACHE BOOL "")
-set(LIBCXXABI_ENABLE_STATIC_UNWINDER ON CACHE BOOL "")
-set(LIBCXXABI_INSTALL_LIBRARY OFF CACHE BOOL "")
-set(LIBCXX_USE_COMPILER_RT ON CACHE BOOL "")
-set(LIBCXX_ENABLE_SHARED OFF CACHE BOOL "")
-set(LIBCXX_ENABLE_STATIC_ABI_LIBRARY ON CACHE BOOL "")
+if(APPLE)
+  # Disable installing libc++, libc++abi or libunwind on Darwin, since Clang
+  # driver doesn't know how to use libraries that are part of the toolchain.
+  set(LIBUNWIND_INSTALL_HEADERS OFF CACHE BOOL "")
+  set(LIBUNWIND_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXXABI_INSTALL_HEADERS OFF CACHE BOOL "")
+  set(LIBCXXABI_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXX_INSTALL_HEADERS OFF CACHE BOOL "")
+  set(LIBCXX_INSTALL_LIBRARY OFF CACHE BOOL "")
+else()
+  set(LIBUNWIND_ENABLE_SHARED OFF CACHE BOOL "")
+  set(LIBUNWIND_USE_COMPILER_RT ON CACHE BOOL "")
+  set(LIBUNWIND_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXXABI_USE_COMPILER_RT ON CACHE BOOL "")
+  set(LIBCXXABI_ENABLE_SHARED OFF CACHE BOOL "")
+  set(LIBCXXABI_USE_LLVM_UNWINDER ON CACHE BOOL "")
+  set(LIBCXXABI_ENABLE_STATIC_UNWINDER ON CACHE BOOL "")
+  set(LIBCXXABI_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXX_USE_COMPILER_RT ON CACHE BOOL "")
+  set(LIBCXX_ENABLE_SHARED OFF CACHE BOOL "")
+  set(LIBCXX_ENABLE_STATIC_ABI_LIBRARY ON CACHE BOOL "")
+endif()
 
 # Set the per-target runtimes options.
 foreach(target x86_64;aarch64)


Index: cfe/trunk/cmake/caches/Fuchsia-stage2.cmake
===
--- cfe/trunk/cmake/caches/Fuchsia-stage2.cmake
+++ cfe/trunk/cmake/caches/Fuchsia-stage2.cmake
@@ -38,17 +38,28 @@
 set(LLVM_RUNTIME_TARGETS "default;x86_64-fuchsia;aarch64-fuchsia;x86_64-fuchsia-asan:x86_64-fuchsia;aarch64-fuchsia-asan:aarch64-fuchsia" CACHE STRING "")
 
 # Set the default target runtimes options.
-set(LIBUNWIND_ENABLE_SHARED OFF CACHE BOOL "")
-set(LIBUNWIND_USE_COMPILER_RT ON CACHE BOOL "")
-set(LIBUNWIND_INSTALL_LIBRARY OFF CACHE BOOL "")
-set(LIBCXXABI_USE_COMPILER_RT ON CACHE BOOL "")
-set(LIBCXXABI_ENABLE_SHARED OFF CACHE BOOL "")
-set(LIBCXXABI_USE_LLVM_UNWINDER ON CACHE BOOL "")
-set(LIBCXXABI_ENABLE_STATIC_UNWINDER ON CACHE BOOL "")
-set(LIBCXXABI_INSTALL_LIBRARY OFF CACHE BOOL "")
-set(LIBCXX_USE_COMPILER_RT ON CACHE BOOL "")
-set(LIBCXX_ENABLE_SHARED OFF CACHE BOOL "")
-set(LIBCXX_ENABLE_STATIC_ABI_LIBRARY ON CACHE BOOL "")
+if(APPLE)
+  # Disable installing libc++, libc++abi or libunwind on Darwin, since Clang
+  # driver doesn't know how to use libraries that are part of the toolchain.
+  set(LIBUNWIND_INSTALL_HEADERS OFF CACHE BOOL "")
+  set(LIBUNWIND_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXXABI_INSTALL_HEADERS OFF CACHE BOOL "")
+  set(LIBCXXABI_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXX_INSTALL_HEADERS OFF CACHE BOOL "")
+  set(LIBCXX_INSTALL_LIBRARY OFF CACHE BOOL "")
+else()
+  set(LIBUNWIND_ENABLE_SHARED OFF CACHE BOOL "")
+  set(LIBUNWIND_USE_COMPILER_RT ON CACHE BOOL "")
+  set(LIBUNWIND_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXXABI_USE_COMPILER_RT ON CACHE BOOL "")
+  set(LIBCXXABI_ENABLE_SHARED OFF CACHE BOOL "")
+  set(LIBCXXABI_USE_LLVM_UNWINDER ON CACHE BOOL "")
+  set(LIBCXXABI_ENABLE_STATIC_UNWINDER ON CACHE BOOL "")
+  set(LIBCXXABI_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXX_USE_COMPILER_RT ON CACHE BOOL "")
+  set(LIBCXX_ENABLE_SHARED OFF CACHE BOOL "")
+  set(LIBCXX_ENABLE_STATIC_ABI_LIBRARY ON CACHE BOOL "")
+endif()
 
 # Set the per-target runtimes options.
 foreach(target x86_64;aarch64)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r328114 - [Fuchsia] Don't install libc++, libc++abi or libunwind on Darwin

[Petr Hosek via cfe-commits]

Author: phosek
Date: Wed Mar 21 09:48:26 2018
New Revision: 328114

URL: http://llvm.org/viewvc/llvm-project?rev=328114=rev
Log:
[Fuchsia] Don't install libc++, libc++abi or libunwind on Darwin

The Clang driver doesn't currently know how to use the libraries
that are shipped as part of the toolchain so there's no reason to
ship them at all.

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

Modified:
cfe/trunk/cmake/caches/Fuchsia-stage2.cmake

Modified: cfe/trunk/cmake/caches/Fuchsia-stage2.cmake
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/cmake/caches/Fuchsia-stage2.cmake?rev=328114=328113=328114=diff
==
--- cfe/trunk/cmake/caches/Fuchsia-stage2.cmake (original)
+++ cfe/trunk/cmake/caches/Fuchsia-stage2.cmake Wed Mar 21 09:48:26 2018
@@ -38,17 +38,28 @@ endforeach()
 set(LLVM_RUNTIME_TARGETS 
"default;x86_64-fuchsia;aarch64-fuchsia;x86_64-fuchsia-asan:x86_64-fuchsia;aarch64-fuchsia-asan:aarch64-fuchsia"
 CACHE STRING "")
 
 # Set the default target runtimes options.
-set(LIBUNWIND_ENABLE_SHARED OFF CACHE BOOL "")
-set(LIBUNWIND_USE_COMPILER_RT ON CACHE BOOL "")
-set(LIBUNWIND_INSTALL_LIBRARY OFF CACHE BOOL "")
-set(LIBCXXABI_USE_COMPILER_RT ON CACHE BOOL "")
-set(LIBCXXABI_ENABLE_SHARED OFF CACHE BOOL "")
-set(LIBCXXABI_USE_LLVM_UNWINDER ON CACHE BOOL "")
-set(LIBCXXABI_ENABLE_STATIC_UNWINDER ON CACHE BOOL "")
-set(LIBCXXABI_INSTALL_LIBRARY OFF CACHE BOOL "")
-set(LIBCXX_USE_COMPILER_RT ON CACHE BOOL "")
-set(LIBCXX_ENABLE_SHARED OFF CACHE BOOL "")
-set(LIBCXX_ENABLE_STATIC_ABI_LIBRARY ON CACHE BOOL "")
+if(APPLE)
+  # Disable installing libc++, libc++abi or libunwind on Darwin, since Clang
+  # driver doesn't know how to use libraries that are part of the toolchain.
+  set(LIBUNWIND_INSTALL_HEADERS OFF CACHE BOOL "")
+  set(LIBUNWIND_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXXABI_INSTALL_HEADERS OFF CACHE BOOL "")
+  set(LIBCXXABI_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXX_INSTALL_HEADERS OFF CACHE BOOL "")
+  set(LIBCXX_INSTALL_LIBRARY OFF CACHE BOOL "")
+else()
+  set(LIBUNWIND_ENABLE_SHARED OFF CACHE BOOL "")
+  set(LIBUNWIND_USE_COMPILER_RT ON CACHE BOOL "")
+  set(LIBUNWIND_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXXABI_USE_COMPILER_RT ON CACHE BOOL "")
+  set(LIBCXXABI_ENABLE_SHARED OFF CACHE BOOL "")
+  set(LIBCXXABI_USE_LLVM_UNWINDER ON CACHE BOOL "")
+  set(LIBCXXABI_ENABLE_STATIC_UNWINDER ON CACHE BOOL "")
+  set(LIBCXXABI_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXX_USE_COMPILER_RT ON CACHE BOOL "")
+  set(LIBCXX_ENABLE_SHARED OFF CACHE BOOL "")
+  set(LIBCXX_ENABLE_STATIC_ABI_LIBRARY ON CACHE BOOL "")
+endif()
 
 # Set the per-target runtimes options.
 foreach(target x86_64;aarch64)


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

[PATCH] D44272: [clangd] Support incremental document syncing

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/DraftStore.h:36
   /// Replace contents of the draft for \p File with \p Contents.
-  void updateDraft(PathRef File, StringRef Contents);
+  void addDraft(PathRef File, StringRef Contents);
+

simark wrote:
> ilya-biryukov wrote:
> > Could we add versions from LSP's `VersionedTextDocumentIdentifier` here and 
> > make the relevant sanity checks?
> > Applying diffs to the wrong version will cause everything to fall apart, so 
> > we should detect this error and signal it to the client as soon as possible.
> I agree that applying diffs to the wrong version will break basically 
> everything, but even if we detect a version mismatch, I don't see what we 
> could do, since we don't have a mean to report the error to the client.  The 
> only thing we could do is log it (which we already do.
If we couldn't apply a diff, we should return errors from all future operations 
on this file until it gets closed and reopened. Otherwise clangd and the editor 
would see inconsistent contents for the file and results provided by clangd 
would be unreliable.
The errors from any actions on the invalid file would actually be visible to 
the users.

The simplest way to achieve that is to remove the file from `DraftStore` and 
`ClangdServer` on errors from `updateDraft`.
This will give "calling action on non-tracked file" errors for future 
operations and the actual root cause can be figured out from the logs.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44272



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

[PATCH] D44724: [Fuchsia] Don't install libc++, libc++abi or libunwind on Darwin

[Petr Hosek via Phabricator via cfe-commits]

phosek updated this revision to Diff 139311.

https://reviews.llvm.org/D44724

Files:
  clang/cmake/caches/Fuchsia-stage2.cmake


Index: clang/cmake/caches/Fuchsia-stage2.cmake
===
--- clang/cmake/caches/Fuchsia-stage2.cmake
+++ clang/cmake/caches/Fuchsia-stage2.cmake
@@ -38,17 +38,28 @@
 set(LLVM_RUNTIME_TARGETS 
"default;x86_64-fuchsia;aarch64-fuchsia;x86_64-fuchsia-asan:x86_64-fuchsia;aarch64-fuchsia-asan:aarch64-fuchsia"
 CACHE STRING "")
 
 # Set the default target runtimes options.
-set(LIBUNWIND_ENABLE_SHARED OFF CACHE BOOL "")
-set(LIBUNWIND_USE_COMPILER_RT ON CACHE BOOL "")
-set(LIBUNWIND_INSTALL_LIBRARY OFF CACHE BOOL "")
-set(LIBCXXABI_USE_COMPILER_RT ON CACHE BOOL "")
-set(LIBCXXABI_ENABLE_SHARED OFF CACHE BOOL "")
-set(LIBCXXABI_USE_LLVM_UNWINDER ON CACHE BOOL "")
-set(LIBCXXABI_ENABLE_STATIC_UNWINDER ON CACHE BOOL "")
-set(LIBCXXABI_INSTALL_LIBRARY OFF CACHE BOOL "")
-set(LIBCXX_USE_COMPILER_RT ON CACHE BOOL "")
-set(LIBCXX_ENABLE_SHARED OFF CACHE BOOL "")
-set(LIBCXX_ENABLE_STATIC_ABI_LIBRARY ON CACHE BOOL "")
+if(APPLE)
+  # Disable installing libc++, libc++abi or libunwind on Darwin, since Clang
+  # driver doesn't know how to use libraries that are part of the toolchain.
+  set(LIBUNWIND_INSTALL_HEADERS OFF CACHE BOOL "")
+  set(LIBUNWIND_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXXABI_INSTALL_HEADERS OFF CACHE BOOL "")
+  set(LIBCXXABI_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXX_INSTALL_HEADERS OFF CACHE BOOL "")
+  set(LIBCXX_INSTALL_LIBRARY OFF CACHE BOOL "")
+else()
+  set(LIBUNWIND_ENABLE_SHARED OFF CACHE BOOL "")
+  set(LIBUNWIND_USE_COMPILER_RT ON CACHE BOOL "")
+  set(LIBUNWIND_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXXABI_USE_COMPILER_RT ON CACHE BOOL "")
+  set(LIBCXXABI_ENABLE_SHARED OFF CACHE BOOL "")
+  set(LIBCXXABI_USE_LLVM_UNWINDER ON CACHE BOOL "")
+  set(LIBCXXABI_ENABLE_STATIC_UNWINDER ON CACHE BOOL "")
+  set(LIBCXXABI_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXX_USE_COMPILER_RT ON CACHE BOOL "")
+  set(LIBCXX_ENABLE_SHARED OFF CACHE BOOL "")
+  set(LIBCXX_ENABLE_STATIC_ABI_LIBRARY ON CACHE BOOL "")
+endif()
 
 # Set the per-target runtimes options.
 foreach(target x86_64;aarch64)


Index: clang/cmake/caches/Fuchsia-stage2.cmake
===
--- clang/cmake/caches/Fuchsia-stage2.cmake
+++ clang/cmake/caches/Fuchsia-stage2.cmake
@@ -38,17 +38,28 @@
 set(LLVM_RUNTIME_TARGETS "default;x86_64-fuchsia;aarch64-fuchsia;x86_64-fuchsia-asan:x86_64-fuchsia;aarch64-fuchsia-asan:aarch64-fuchsia" CACHE STRING "")
 
 # Set the default target runtimes options.
-set(LIBUNWIND_ENABLE_SHARED OFF CACHE BOOL "")
-set(LIBUNWIND_USE_COMPILER_RT ON CACHE BOOL "")
-set(LIBUNWIND_INSTALL_LIBRARY OFF CACHE BOOL "")
-set(LIBCXXABI_USE_COMPILER_RT ON CACHE BOOL "")
-set(LIBCXXABI_ENABLE_SHARED OFF CACHE BOOL "")
-set(LIBCXXABI_USE_LLVM_UNWINDER ON CACHE BOOL "")
-set(LIBCXXABI_ENABLE_STATIC_UNWINDER ON CACHE BOOL "")
-set(LIBCXXABI_INSTALL_LIBRARY OFF CACHE BOOL "")
-set(LIBCXX_USE_COMPILER_RT ON CACHE BOOL "")
-set(LIBCXX_ENABLE_SHARED OFF CACHE BOOL "")
-set(LIBCXX_ENABLE_STATIC_ABI_LIBRARY ON CACHE BOOL "")
+if(APPLE)
+  # Disable installing libc++, libc++abi or libunwind on Darwin, since Clang
+  # driver doesn't know how to use libraries that are part of the toolchain.
+  set(LIBUNWIND_INSTALL_HEADERS OFF CACHE BOOL "")
+  set(LIBUNWIND_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXXABI_INSTALL_HEADERS OFF CACHE BOOL "")
+  set(LIBCXXABI_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXX_INSTALL_HEADERS OFF CACHE BOOL "")
+  set(LIBCXX_INSTALL_LIBRARY OFF CACHE BOOL "")
+else()
+  set(LIBUNWIND_ENABLE_SHARED OFF CACHE BOOL "")
+  set(LIBUNWIND_USE_COMPILER_RT ON CACHE BOOL "")
+  set(LIBUNWIND_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXXABI_USE_COMPILER_RT ON CACHE BOOL "")
+  set(LIBCXXABI_ENABLE_SHARED OFF CACHE BOOL "")
+  set(LIBCXXABI_USE_LLVM_UNWINDER ON CACHE BOOL "")
+  set(LIBCXXABI_ENABLE_STATIC_UNWINDER ON CACHE BOOL "")
+  set(LIBCXXABI_INSTALL_LIBRARY OFF CACHE BOOL "")
+  set(LIBCXX_USE_COMPILER_RT ON CACHE BOOL "")
+  set(LIBCXX_ENABLE_SHARED OFF CACHE BOOL "")
+  set(LIBCXX_ENABLE_STATIC_ABI_LIBRARY ON CACHE BOOL "")
+endif()
 
 # Set the per-target runtimes options.
 foreach(target x86_64;aarch64)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44272: [clangd] Support incremental document syncing

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 139310.
simark marked an inline comment as done.
simark added a comment.

Address review comments, except LSP version check


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44272

Files:
  clangd/ClangdLSPServer.cpp
  clangd/DraftStore.cpp
  clangd/DraftStore.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  test/clangd/initialize-params-invalid.test
  test/clangd/initialize-params.test
  unittests/clangd/CMakeLists.txt
  unittests/clangd/DraftStoreTests.cpp

Index: unittests/clangd/DraftStoreTests.cpp
===
--- /dev/null
+++ unittests/clangd/DraftStoreTests.cpp
@@ -0,0 +1,355 @@
+//===-- DraftStoreTests.cpp - Clangd unit tests -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Annotations.h"
+#include "DraftStore.h"
+#include "SourceCode.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using namespace llvm;
+
+struct IncrementalTestStep {
+  StringRef Src;
+  StringRef Contents;
+};
+
+static int rangeLength(StringRef Code, const Range ) {
+  llvm::Expected Start = positionToOffset(Code, Rng.start);
+  llvm::Expected End = positionToOffset(Code, Rng.end);
+  assert(Start);
+  assert(End);
+  return *End - *Start;
+}
+
+// Send the changes one by one to updateDraft, verify the intermediate results.
+
+static void stepByStep(llvm::ArrayRef Steps) {
+  DraftStore DS;
+  Annotations InitialSrc(Steps.front().Src);
+  const char Path[] = "/hello.cpp";
+
+  // Set the initial content.
+  DS.addDraft(Path, InitialSrc.code());
+
+  for (size_t i = 1; i < Steps.size(); i++) {
+Annotations SrcBefore(Steps[i - 1].Src);
+Annotations SrcAfter(Steps[i].Src);
+StringRef Contents = Steps[i - 1].Contents;
+TextDocumentContentChangeEvent Event{
+SrcBefore.range(),
+rangeLength(SrcBefore.code(), SrcBefore.range()),
+Contents.str(),
+};
+
+llvm::Expected Result = DS.updateDraft(Path, {Event});
+EXPECT_TRUE(!!Result);
+EXPECT_EQ(*Result, SrcAfter.code());
+EXPECT_EQ(*DS.getDraft(Path), SrcAfter.code());
+  }
+}
+
+// Send all the changes at once to updateDraft, check only the final result.
+
+static void allAtOnce(llvm::ArrayRef Steps) {
+  DraftStore DS;
+  Annotations InitialSrc(Steps.front().Src);
+  Annotations FinalSrc(Steps.back().Src);
+  const char Path[] = "/hello.cpp";
+  std::vector Changes;
+
+  for (size_t i = 0; i < Steps.size() - 1; i++) {
+Annotations Src(Steps[i].Src);
+StringRef Contents = Steps[i].Contents;
+
+Changes.push_back({
+Src.range(),
+rangeLength(Src.code(), Src.range()),
+Contents.str(),
+});
+  }
+
+  // Set the initial content.
+  DS.addDraft(Path, InitialSrc.code());
+
+  llvm::Expected Result = DS.updateDraft(Path, Changes);
+
+  EXPECT_TRUE(!!Result) << llvm::toString(Result.takeError());
+  EXPECT_EQ(*Result, FinalSrc.code());
+  EXPECT_EQ(*DS.getDraft(Path), FinalSrc.code());
+}
+
+TEST(DraftStoreIncrementalUpdateTest, Simple) {
+  // clang-format off
+  IncrementalTestStep Steps[] =
+{
+  // Replace a range
+  {
+R"cpp(static int
+hello[[World]]()
+{})cpp",
+"Universe"
+  },
+  // Delete a range
+  {
+R"cpp(static int
+hello[[Universe]]()
+{})cpp",
+""
+  },
+  // Add a range
+  {
+R"cpp(static int
+hello[[]]()
+{})cpp",
+"Monde"
+  },
+  {
+R"cpp(static int
+helloMonde()
+{})cpp",
+""
+  }
+};
+  // clang-format on
+
+  stepByStep(Steps);
+  allAtOnce(Steps);
+}
+
+TEST(DraftStoreIncrementalUpdateTest, MultiLine) {
+  // clang-format off
+  IncrementalTestStep Steps[] =
+{
+  // Replace a range
+  {
+R"cpp(static [[int
+helloWorld]]()
+{})cpp",
+R"cpp(char
+welcome)cpp"
+  },
+  // Delete a range
+  {
+R"cpp(static char[[
+welcome]]()
+{})cpp",
+""
+  },
+  // Add a range
+  {
+R"cpp(static char[[]]()
+{})cpp",
+R"cpp(
+cookies)cpp"
+  },
+  // Replace the whole file
+  {
+R"cpp([[static char
+cookies()
+{}]])cpp",
+R"cpp(#include 
+)cpp"
+  },
+  // Delete the whole file
+  {
+R"cpp([[#include 
+]])cpp",
+"",
+  },
+  // Add something to an empty file
+  {
+"[[]]",
+R"cpp(int main() {
+)cpp",
+  },
+  {
+R"cpp(int main() {
+)cpp",
+""
+  }
+};
+  // clang-format on
+
+  stepByStep(Steps);
+  allAtOnce(Steps);
+}
+
+TEST(DraftStoreIncrementalUpdateTest, WrongRangeLength) {
+  DraftStore DS;
+  Path File = "foo.cpp";
+
+  DS.addDraft(File, "int main() {}\n");
+
+  TextDocumentContentChangeEvent Change;
+  

[PATCH] D44589: [Sema] Make deprecation fix-it replace all multi-parameter ObjC method slots.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In https://reviews.llvm.org/D44589#1044350, @aaron.ballman wrote:

> This generally looks reasonable to me, but @rsmith should weigh in before you 
> commit because `MultiSourceLocation` is novel.


Thanks for the review, Aaron. I tried not to do anything stupid with 
`MultiSourceLocation` but another opinion will be definitely useful.


https://reviews.llvm.org/D44589



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

[PATCH] D42730: [clang-tidy]] Add check for use of types/classes/functions from header which are deprecated and removed in C++17

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:48-54
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("ptr_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("mem_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::mem_fun'";
+  }

alexfh wrote:
> aaron.ballman wrote:
> > alexfh wrote:
> > > aaron.ballman wrote:
> > > > alexfh wrote:
> > > > > aaron.ballman wrote:
> > > > > > alexfh wrote:
> > > > > > > alexfh wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > massberg wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > I think that this code should be generalized (same with 
> > > > > > > > > > > the matchers) so that you match on `hasAnyName()` for the 
> > > > > > > > > > > function calls and use `CallExpr::getCalleeDecl()` to get 
> > > > > > > > > > > the declaration. e.g.,
> > > > > > > > > > > ```
> > > > > > > > > > > if (const auto *Call = 
> > > > > > > > > > > Result.Nodes.getNodeAs("blech")) {
> > > > > > > > > > >   if (const Decl *Callee = Call->getCalleeDecl())
> > > > > > > > > > > diag(Call->getLocStart(), Message) << Calleee;
> > > > > > > > > > > }
> > > > > > > > > > > ```
> > > > > > > > > > > This way you can add more named without having to add 
> > > > > > > > > > > extra logic for the diagnostics.
> > > > > > > > > > I generalized the code and the matcher.
> > > > > > > > > > When we use
> > > > > > > > > > ```
> > > > > > > > > > << cast(Callee);
> > > > > > > > > > ```
> > > > > > > > > > we get the template arguments in the name , e.g. 
> > > > > > > > > > `ptr_fun`, so I chose to use 
> > > > > > > > > > `getQualifiedNameAsString`.
> > > > > > > > > > If there is there a better way to get the function name 
> > > > > > > > > > without template arguments I appreciate any suggestions.
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > Nope, in that case, your code is correct. However, we 
> > > > > > > > > generally provide the template arguments in diagnostics. I 
> > > > > > > > > see @alexfh was asking for them to be removed as not being 
> > > > > > > > > useful, but I'm not certain I agree with his rationale. Yes, 
> > > > > > > > > all instances are deprecated and thus the template arguments 
> > > > > > > > > don't discern between good and bad cases, but showing the 
> > > > > > > > > template arguments is also consistent with the other 
> > > > > > > > > diagnostics we emit. For instance, other "deprecated" 
> > > > > > > > > diagnostics also emit the template arguments. I'm not certain 
> > > > > > > > > we should be inconsistent with the way we produce 
> > > > > > > > > diagnostics, but I'll defer to Alex if he still feels 
> > > > > > > > > strongly about leaving them off here.
> > > > > > > > Indeed, -Wdeprecated-declarations warnings print template 
> > > > > > > > arguments. Moreover, they seem to be issued only on 
> > > > > > > > instantiations, see https://godbolt.org/g/W563gw.
> > > > > > > > 
> > > > > > > > But I have a number of concerns with template arguments in the 
> > > > > > > > deprecation warnings:
> > > > > > > > 
> > > > > > > > 1. The note attached to the warning lies. Consider a warning 
> > > > > > > > from the test above:
> > > > > > > >   ...
> > > > > > > >   :11:1: warning: 'B' is deprecated: bbb 
> > > > > > > > [-Wdeprecated-declarations]
> > > > > > > >   B e;
> > > > > > > >   ^
> > > > > > > >   :7:10: note: 'B' has been explicitly marked 
> > > > > > > > deprecated here
> > > > > > > >   struct [[deprecated("bbb")]] B {};
> > > > > > > > 
> > > > > > > >  But `B` hasn't been explicitly marked deprecated, only 
> > > > > > > > the template definition of `B` has been. Template arguments are 
> > > > > > > > important in the case of the explicit template specialization 
> > > > > > > > `A` in the same example, but not in cases where the 
> > > > > > > > template definition was marked deprecated, since template 
> > > > > > > > arguments only add noise and no useful information there.
> > > > > > > > 2. Warnings can easily become unreadable: 
> > > > > > > > https://godbolt.org/g/AFdznH
> > > > > > > > 3. Certain coding patterns can result in numerous deprecation 
> > > > > > > > warnings differing only in template arguments: 
> > > > > > > > https://godbolt.org/g/U2JCbG. Clang-tidy can deduplicate 
> > > > > > > > warnings, if they have identical text and location, but adding 
> > > > > > > > template arguments to the message will prevent deduplication. 
> > > > > > > > I've seen a case where thousands of deprecation warnings were 
> > > > > > > > generated for a single line in a widely used header.
> > > > > > > > 
> > > > > > > > So yes, I feel strongly about leaving off template arguments in 
> > > > > > > > case the whole template was marked 

[clang-tools-extra] r328108 - [Fix] fix type deduction on ARM and MSVC

[Jonas Toth via cfe-commits]

Author: jonastoth
Date: Wed Mar 21 08:50:15 2018
New Revision: 328108

URL: http://llvm.org/viewvc/llvm-project?rev=328108=rev
Log:
[Fix] fix type deduction on ARM and MSVC

Modified:
clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp

Modified: clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp?rev=328108=328107=328108=diff
==
--- clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp Wed 
Mar 21 08:50:15 2018
@@ -167,7 +167,7 @@ void MultiwayPathsCoveredCheck::handleSw
   return twoPow(BitfieldDecl->getBitWidthValue(*Result.Context));
 }
 
-return 0ul;
+return static_cast(0);
   }();
 
   // FIXME: Transform the 'switch' into an 'if' for CaseCount == 1.


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

[PATCH] D43967: [ASTImporter] Add test helper Fixture

[Aleksei Sidorin via Phabricator via cfe-commits]

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

This looks good to me. Thank you!
@xazax.hun Gabor, could you please take a look?


Repository:
  rC Clang

https://reviews.llvm.org/D43967



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

[PATCH] D42730: [clang-tidy]] Add check for use of types/classes/functions from header which are deprecated and removed in C++17

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/modernize/DeprecatedFunctionalCheck.cpp:48-54
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("ptr_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::ptr_fun'";
+  } else if (const auto *const Call =
+ Result.Nodes.getNodeAs("mem_fun_call")) {
+diag(Call->getLocStart(), Message) << "'std::mem_fun'";
+  }

aaron.ballman wrote:
> alexfh wrote:
> > aaron.ballman wrote:
> > > alexfh wrote:
> > > > aaron.ballman wrote:
> > > > > alexfh wrote:
> > > > > > alexfh wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > massberg wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > I think that this code should be generalized (same with the 
> > > > > > > > > > matchers) so that you match on `hasAnyName()` for the 
> > > > > > > > > > function calls and use `CallExpr::getCalleeDecl()` to get 
> > > > > > > > > > the declaration. e.g.,
> > > > > > > > > > ```
> > > > > > > > > > if (const auto *Call = 
> > > > > > > > > > Result.Nodes.getNodeAs("blech")) {
> > > > > > > > > >   if (const Decl *Callee = Call->getCalleeDecl())
> > > > > > > > > > diag(Call->getLocStart(), Message) << Calleee;
> > > > > > > > > > }
> > > > > > > > > > ```
> > > > > > > > > > This way you can add more named without having to add extra 
> > > > > > > > > > logic for the diagnostics.
> > > > > > > > > I generalized the code and the matcher.
> > > > > > > > > When we use
> > > > > > > > > ```
> > > > > > > > > << cast(Callee);
> > > > > > > > > ```
> > > > > > > > > we get the template arguments in the name , e.g. 
> > > > > > > > > `ptr_fun`, so I chose to use 
> > > > > > > > > `getQualifiedNameAsString`.
> > > > > > > > > If there is there a better way to get the function name 
> > > > > > > > > without template arguments I appreciate any suggestions.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > Nope, in that case, your code is correct. However, we generally 
> > > > > > > > provide the template arguments in diagnostics. I see @alexfh 
> > > > > > > > was asking for them to be removed as not being useful, but I'm 
> > > > > > > > not certain I agree with his rationale. Yes, all instances are 
> > > > > > > > deprecated and thus the template arguments don't discern 
> > > > > > > > between good and bad cases, but showing the template arguments 
> > > > > > > > is also consistent with the other diagnostics we emit. For 
> > > > > > > > instance, other "deprecated" diagnostics also emit the template 
> > > > > > > > arguments. I'm not certain we should be inconsistent with the 
> > > > > > > > way we produce diagnostics, but I'll defer to Alex if he still 
> > > > > > > > feels strongly about leaving them off here.
> > > > > > > Indeed, -Wdeprecated-declarations warnings print template 
> > > > > > > arguments. Moreover, they seem to be issued only on 
> > > > > > > instantiations, see https://godbolt.org/g/W563gw.
> > > > > > > 
> > > > > > > But I have a number of concerns with template arguments in the 
> > > > > > > deprecation warnings:
> > > > > > > 
> > > > > > > 1. The note attached to the warning lies. Consider a warning from 
> > > > > > > the test above:
> > > > > > >   ...
> > > > > > >   :11:1: warning: 'B' is deprecated: bbb 
> > > > > > > [-Wdeprecated-declarations]
> > > > > > >   B e;
> > > > > > >   ^
> > > > > > >   :7:10: note: 'B' has been explicitly marked 
> > > > > > > deprecated here
> > > > > > >   struct [[deprecated("bbb")]] B {};
> > > > > > > 
> > > > > > >  But `B` hasn't been explicitly marked deprecated, only the 
> > > > > > > template definition of `B` has been. Template arguments are 
> > > > > > > important in the case of the explicit template specialization 
> > > > > > > `A` in the same example, but not in cases where the template 
> > > > > > > definition was marked deprecated, since template arguments only 
> > > > > > > add noise and no useful information there.
> > > > > > > 2. Warnings can easily become unreadable: 
> > > > > > > https://godbolt.org/g/AFdznH
> > > > > > > 3. Certain coding patterns can result in numerous deprecation 
> > > > > > > warnings differing only in template arguments: 
> > > > > > > https://godbolt.org/g/U2JCbG. Clang-tidy can deduplicate 
> > > > > > > warnings, if they have identical text and location, but adding 
> > > > > > > template arguments to the message will prevent deduplication. 
> > > > > > > I've seen a case where thousands of deprecation warnings were 
> > > > > > > generated for a single line in a widely used header.
> > > > > > > 
> > > > > > > So yes, I feel strongly about leaving off template arguments in 
> > > > > > > case the whole template was marked deprecated. I think it would 
> > > > > > > be the right thing to do for the -Wdeprecated-declarations 
> > > > > > > diagnostic as well.
> > > > > > s/leaving off/leaving out/
> > > > > > The note attached to the 

[PATCH] D44272: [clangd] Support incremental document syncing

[Simon Marchi via Phabricator via cfe-commits]

simark marked 34 inline comments as done.
simark added inline comments.



Comment at: clangd/DraftStore.h:36
   /// Replace contents of the draft for \p File with \p Contents.
-  void updateDraft(PathRef File, StringRef Contents);
+  void addDraft(PathRef File, StringRef Contents);
+

ilya-biryukov wrote:
> Could we add versions from LSP's `VersionedTextDocumentIdentifier` here and 
> make the relevant sanity checks?
> Applying diffs to the wrong version will cause everything to fall apart, so 
> we should detect this error and signal it to the client as soon as possible.
I agree that applying diffs to the wrong version will break basically 
everything, but even if we detect a version mismatch, I don't see what we could 
do, since we don't have a mean to report the error to the client.  The only 
thing we could do is log it (which we already do.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44272



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

[PATCH] D44729: [ASTMatchers] Add hasSubExpr() matcher.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

Thanks for the pointer. I missed the trampoline through 
GetSourceExpressionMatcher.


Repository:
  rC Clang

https://reviews.llvm.org/D44729



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

Re: r327959 - [ms] Parse #pragma optimize and ignore it behind its own flag

[Nico Weber via cfe-commits]

>From the bot changes, it seems that -Wunknown-pragma doesn't disable this
new warning. Shouldn't it do that?

On Tue, Mar 20, 2018, 9:55 AM Hans Wennborg via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: hans
> Date: Tue Mar 20 01:53:11 2018
> New Revision: 327959
>
> URL: http://llvm.org/viewvc/llvm-project?rev=327959=rev
> Log:
> [ms] Parse #pragma optimize and ignore it behind its own flag
>
> This allows users to turn off warnings about this pragma specifically,
> while still receiving warnings about other ignored pragmas.
>
> Differential Revision: https://reviews.llvm.org/D44630
>
> Modified:
> cfe/trunk/include/clang/Basic/DiagnosticGroups.td
> cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
> cfe/trunk/include/clang/Parse/Parser.h
> cfe/trunk/lib/Parse/ParsePragma.cpp
> cfe/trunk/test/Preprocessor/pragma_microsoft.c
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=327959=327958=327959=diff
>
> ==
> --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Tue Mar 20 01:53:11
> 2018
> @@ -515,8 +515,13 @@ def UninitializedStaticSelfInit : DiagGr
>  def Uninitialized  : DiagGroup<"uninitialized", [UninitializedSometimes,
>
> UninitializedStaticSelfInit]>;
>  def IgnoredPragmaIntrinsic : DiagGroup<"ignored-pragma-intrinsic">;
> +// #pragma optimize is often used to avoid to work around MSVC codegen
> bugs or
> +// to disable inlining. It's not completely clear what alternative to
> suggest
> +// (#pragma clang optimize, noinline) so suggest nothing for now.
> +def IgnoredPragmaOptimize : DiagGroup<"ignored-pragma-optimize">;
>  def UnknownPragmas : DiagGroup<"unknown-pragmas">;
> -def IgnoredPragmas : DiagGroup<"ignored-pragmas",
> [IgnoredPragmaIntrinsic]>;
> +def IgnoredPragmas : DiagGroup<"ignored-pragmas",
> +[IgnoredPragmaIntrinsic, IgnoredPragmaOptimize]>;
>  def PragmaClangAttribute : DiagGroup<"pragma-clang-attribute">;
>  def PragmaPackSuspiciousInclude :
> DiagGroup<"pragma-pack-suspicious-include">;
>  def PragmaPack : DiagGroup<"pragma-pack", [PragmaPackSuspiciousInclude]>;
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td?rev=327959=327958=327959=diff
>
> ==
> --- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td Tue Mar 20
> 01:53:11 2018
> @@ -895,6 +895,12 @@ def warn_pragma_expected_rparen : Warnin
>"missing ')' after '#pragma %0' - ignoring">, InGroup;
>  def warn_pragma_expected_identifier : Warning<
>"expected identifier in '#pragma %0' - ignored">,
> InGroup;
> +def warn_pragma_expected_string : Warning<
> +  "expected string literal in '#pragma %0' - ignoring">,
> InGroup;
> +def warn_pragma_missing_argument : Warning<
> +  "missing argument to '#pragma %0'%select{|; expected %2}1">,
> InGroup;
> +def warn_pragma_invalid_argument : Warning<
> +  "unexpected argument '%0' to '#pragma %1'%select{|; expected %3}2">,
> InGroup;
>
>  // '#pragma clang section' related errors
>  def err_pragma_expected_clang_section_name : Error<
> @@ -923,6 +929,8 @@ def warn_pragma_ms_struct : Warning<
>  def warn_pragma_extra_tokens_at_eol : Warning<
>"extra tokens at end of '#pragma %0' - ignored">,
>InGroup;
> +def warn_pragma_expected_comma : Warning<
> +  "expected ',' in '#pragma %0'">, InGroup;
>  def warn_pragma_expected_punc : Warning<
>"expected ')' or ',' in '#pragma %0'">, InGroup;
>  def warn_pragma_expected_non_wide_string : Warning<
> @@ -960,6 +968,10 @@ def warn_pragma_pack_malformed : Warning
>  def warn_pragma_intrinsic_builtin : Warning<
>"%0 is not a recognized builtin%select{|; consider including 
> to access non-builtin intrinsics}1">,
>InGroup;
> +// - #pragma optimize
> +def warn_pragma_optimize : Warning<
> +  "'#pragma optimize' is not supported">,
> +  InGroup;
>  // - #pragma unused
>  def warn_pragma_unused_expected_var : Warning<
>"expected '#pragma unused' argument to be a variable name">,
>
> Modified: cfe/trunk/include/clang/Parse/Parser.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Parse/Parser.h?rev=327959=327958=327959=diff
>
> ==
> --- cfe/trunk/include/clang/Parse/Parser.h (original)
> +++ cfe/trunk/include/clang/Parse/Parser.h Tue Mar 20 01:53:11 2018
> @@ -179,6 +179,7 @@ class Parser : public CodeCompletionHand
>std::unique_ptr MSSection;
>std::unique_ptr MSRuntimeChecks;
>std::unique_ptr MSIntrinsic;
> +  std::unique_ptr MSOptimize;
>std::unique_ptr 

[clang-tools-extra] r328107 - [clang-tidy] Resubmit hicpp-multiway-paths-covered without breaking test

[Jonas Toth via cfe-commits]

Author: jonastoth
Date: Wed Mar 21 08:34:15 2018
New Revision: 328107

URL: http://llvm.org/viewvc/llvm-project?rev=328107=rev
Log:
[clang-tidy] Resubmit hicpp-multiway-paths-covered without breaking test

The original check did break the green buildbot in the sanitizer build.
It took a while to redroduce and understand the issue.

There occured a stackoverflow while parsing the AST. The testcase with
256 case labels was the problem because each case label added another
stackframe. It seemed that the issue occured only in 'RelWithDebInfo' builds
and not in normal sanitizer builds.

To simplify the matchers the recognition for the different kinds of switch
statements has been moved into a seperate function and will not be done with
ASTMatchers. This is an attempt to reduce recursion and stacksize as well.

The new check removed this big testcase. Covering all possible values is still
implemented for bitfields and works there. The same logic on integer types
will lead to the issue.

Running it over LLVM gives the following results:


Differential: https://reviews.llvm.org/D40737

Added:
clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.h

clang-tools-extra/trunk/docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst

clang-tools-extra/trunk/test/clang-tidy/hicpp-multiway-paths-covered-else.cpp
clang-tools-extra/trunk/test/clang-tidy/hicpp-multiway-paths-covered.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/hicpp/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/hicpp/HICPPTidyModule.cpp
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

Modified: clang-tools-extra/trunk/clang-tidy/hicpp/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/hicpp/CMakeLists.txt?rev=328107=328106=328107=diff
==
--- clang-tools-extra/trunk/clang-tidy/hicpp/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/hicpp/CMakeLists.txt Wed Mar 21 08:34:15 
2018
@@ -2,6 +2,7 @@ set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyHICPPModule
   ExceptionBaseclassCheck.cpp
+  MultiwayPathsCoveredCheck.cpp
   NoAssemblerCheck.cpp
   HICPPTidyModule.cpp
   SignedBitwiseCheck.cpp

Modified: clang-tools-extra/trunk/clang-tidy/hicpp/HICPPTidyModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/hicpp/HICPPTidyModule.cpp?rev=328107=328106=328107=diff
==
--- clang-tools-extra/trunk/clang-tidy/hicpp/HICPPTidyModule.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/hicpp/HICPPTidyModule.cpp Wed Mar 21 
08:34:15 2018
@@ -36,6 +36,7 @@
 #include "../readability/FunctionSizeCheck.h"
 #include "../readability/IdentifierNamingCheck.h"
 #include "ExceptionBaseclassCheck.h"
+#include "MultiwayPathsCoveredCheck.h"
 #include "NoAssemblerCheck.h"
 #include "SignedBitwiseCheck.h"
 
@@ -54,6 +55,8 @@ public:
 "hicpp-deprecated-headers");
 CheckFactories.registerCheck(
 "hicpp-exception-baseclass");
+CheckFactories.registerCheck(
+"hicpp-multiway-paths-covered");
 CheckFactories.registerCheck("hicpp-signed-bitwise");
 CheckFactories.registerCheck(
 "hicpp-explicit-conversions");

Added: clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp?rev=328107=auto
==
--- clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp 
(added)
+++ clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp Wed 
Mar 21 08:34:15 2018
@@ -0,0 +1,181 @@
+//===--- MultiwayPathsCoveredCheck.cpp - 
clang-tidy===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "MultiwayPathsCoveredCheck.h"
+#include "clang/AST/ASTContext.h"
+
+#include 
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace hicpp {
+
+void MultiwayPathsCoveredCheck::storeOptions(
+ClangTidyOptions::OptionMap ) {
+  Options.store(Opts, "WarnOnMissingElse", WarnOnMissingElse);
+}
+
+void MultiwayPathsCoveredCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+  switchStmt(
+  hasCondition(allOf(
+  // Match on switch statements that have either a bit-field or
+  // an integer condition. The ordering in 'anyOf()' is
+  // important because the last condition is the 

[PATCH] D44426: Fix llvm + clang build with Intel compiler

[Melanie Blower via Phabricator via cfe-commits]

mibintc added inline comments.



Comment at: include/llvm-c/Target.h:25
 
-#if defined(_MSC_VER) && !defined(inline)
+#if defined(_MSC_VER) && !defined(inline) && !defined(__INTEL_COMPILER)
 #define inline __inline

yvvan wrote:
> mibintc wrote:
> > I really think all the Intel-compiler bug workarounds should be version 
> > specific. For example, in the boost.org customizations we have checks like 
> > this:
> > #if defined(__INTEL_COMPILER) && (__INTEL_COMPILER >= 1500)
> > 
> > I believe you are using the most recent Intel compiler, 18.0, which has 
> > version 1800.  Let's assume the bug will either be fixed in our 18.0 
> > compiler or in our next compiler which would be numbered 1900, so the check 
> > could be like this:
> > #if defined(__INTEL_COMPILER) && (__INTEL_COMPILER < 1900) // employ the 
> > workaround for the Intel 18.0 compiler and older
> > 
> > This way the workaround will "disappear" when the bug gets fixed.
> > 
> > For that matter I wonder if it's still necessary to use the workaround for 
> > the Microsoft compiler?
> > 
> > Thanks for reporting the bug into the Intel forum. If we put a bug 
> > workaround into LLVM it would be very nice to have the bug reported to the 
> > offending compiler. 
> > 
> This one will probably not be fixed because I was answered that it's the 
> predicted behavior and that I need not to "#define inline __inline" if I use 
> intel compiler
> I agree though about versions which need to be taken into account
thanks



Comment at: include/llvm/ADT/BitmaskEnum.h:73
 
+#ifdef __INTEL_COMPILER
+template 

yvvan wrote:
> mibintc wrote:
> > what problem is being worked around here? I checked your post into the 
> > Intel compiler forum "enum members are not visible to template 
> > specialization" and I can observe the bug on our Windows compiler but not 
> > our Linux compiler. 
> it was recently accepted. the issues is that 
> E::LLVM_BITMASK_LARGEST_ENUMERATOR from line 80 here is always invalid with 
> intel compiler and the whole set of operator overloads is ignored
> at least on Windows :)
Thanks. I found the bug report for this one in the icc bug tracking database. 
It's CMPLRS-47898. With luck it will be fixed in the next update (ballpark 
based on previous release cadence, 3 months)



Comment at: include/llvm/Analysis/AliasAnalysis.h:254
   FMRB_OnlyAccessesInaccessibleOrArgMem = FMRL_InaccessibleMem |
-  FMRL_ArgumentPointees |
+  
static_cast(FMRL_ArgumentPointees) |
   static_cast(ModRefInfo::ModRef),

yvvan wrote:
> mibintc wrote:
> > is this a necessary workaround for the Intel compiler? It's not pretty.  
> > Suggest we add the #if around it? Long term i hope this would not be 
> > necessary.
> i'm not sure that adding #if around makes it easier to understand :)
> of course I can add it everywhere I append enums with static_cast
The modification for the Intel compiler is so ugly that I'd rather it get 
wrapped with the #if, and eventually the workaround could get discarded. 
Imagine yourself 20 years from now staring at that code and wondering why it is 
written that way.  The #if makes it stand out, and easier to find later, and 
easier to restore to pristine state, when you want to clean it up.  

I tried a small test case with the Intel compiler but i couldn't see the 
problem, must be too simple-minded.
typedef enum { a,b,c,d,e } letters;
char conv( letters l) {
  switch(l) {
  case a: return 'a';
  case a | b : return 'b';
  default : return 'z';
  }
}




Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:6131
 
+#ifndef __INTEL_COMPILER
 static_assert(((~(SIInstrFlags::S_NAN |

yvvan wrote:
> mibintc wrote:
> > this assesrts with Intel compiler?  Or the expression needs to be rewritten 
> > with static_cast as above?
> "needs to be rewritten with static_cast" - correct. it can be done but I was 
> a bit lazy and statioc_assert does not affect the outcome of build :)

ok, it's got the if intel so: lgtm


https://reviews.llvm.org/D44426



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

[PATCH] D40737: [clang-tidy] Resubmit hicpp-multiway-paths-covered without breaking test

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:119
+  if (!SwitchHasDefault && SwitchCaseCount == 0) {
+diag(Switch->getLocStart(), "degenerated switch without labels");
+return;

aaron.ballman wrote:
> JonasToth wrote:
> > aaron.ballman wrote:
> > > I think a better way to phrase this one is: "switch statement without 
> > > labels has no effect" and perhaps have a fix-it to replace the entire 
> > > switch construct with its predicate?
> > The fixit would only aim for the sideeffects the predicate has, right? I 
> > would consider such a switch as a bug or are there reasonable things to 
> > accomplish? Given its most likely unintended code i dont think a fixit is 
> > good.
> > 
> > Fixing the code removes the warning and might introduce a bug.
> This code pattern comes up with machine-generated code with some frequency, 
> so I was thinking it would be nice to automatically fix that code. However, I 
> think you're right that "fixing" the code may introduce bugs because you 
> don't want to keep around non-side effecting operations and that's 
> complicated. e.g.,
> ```
> switch (i) { // Don't replace this with i;
> }
> 
> switch (some_function_call()) { // Maybe replace this with 
> some_function_call()?
> }
> 
> switch (i = 12) { // Should definitely be replaced with i = 12;
> }
> ```
> Perhaps only diagnosing is the best option.
I will add another FIXME. The if-is-better pattern might be reasonable 
transformable and doing this allows addressing the issue again later.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40737



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

[PATCH] D40737: [clang-tidy] Resubmit hicpp-multiway-paths-covered without breaking test

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 139304.
JonasToth marked 3 inline comments as done.
JonasToth added a comment.

- add fixme for degenerated switch


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40737

Files:
  clang-tidy/hicpp/CMakeLists.txt
  clang-tidy/hicpp/HICPPTidyModule.cpp
  clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
  clang-tidy/hicpp/MultiwayPathsCoveredCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/hicpp-multiway-paths-covered-else.cpp
  test/clang-tidy/hicpp-multiway-paths-covered.cpp

Index: test/clang-tidy/hicpp-multiway-paths-covered.cpp
===
--- /dev/null
+++ test/clang-tidy/hicpp-multiway-paths-covered.cpp
@@ -0,0 +1,468 @@
+// RUN: %check_clang_tidy %s hicpp-multiway-paths-covered %t
+
+enum OS { Mac,
+  Windows,
+  Linux };
+
+struct Bitfields {
+  unsigned UInt : 3;
+  int SInt : 1;
+};
+
+int return_integer() { return 42; }
+
+void bad_switch(int i) {
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with only one case; use an if statement
+  case 0:
+break;
+  }
+  // No default in this switch
+  switch (i) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label
+  case 0:
+break;
+  case 1:
+break;
+  case 2:
+break;
+  }
+
+  // degenerate, maybe even warning
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch statement without labels has no effect
+  }
+
+  switch (int j = return_integer()) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label
+  case 0:
+  case 1:
+  case 2:
+break;
+  }
+
+  // Degenerated, only default case.
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch with default label only
+  default:
+break;
+  }
+
+  // Degenerated, only one case label and default case -> Better as if-stmt.
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch could be better written as an if/else statement
+  case 0:
+break;
+  default:
+break;
+  }
+
+  unsigned long long BigNumber = 0;
+  switch (BigNumber) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label
+  case 0:
+  case 1:
+break;
+  }
+
+  const int  = i;
+  switch (IntRef) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label
+  case 0:
+  case 1:
+break;
+  }
+
+  char C = 'A';
+  switch (C) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label
+  case 'A':
+break;
+  case 'B':
+break;
+  }
+
+  Bitfields Bf;
+  // UInt has 3 bits size.
+  switch (Bf.UInt) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label
+  case 0:
+  case 1:
+break;
+  }
+  // All paths explicitly covered.
+  switch (Bf.UInt) {
+  case 0:
+  case 1:
+  case 2:
+  case 3:
+  case 4:
+  case 5:
+  case 6:
+  case 7:
+break;
+  }
+  // SInt has 1 bit size, so this is somewhat degenerated.
+  switch (Bf.SInt) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with only one case; use an if statement
+  case 0:
+break;
+  }
+  // All paths explicitly covered.
+  switch (Bf.SInt) {
+  case 0:
+  case 1:
+break;
+  }
+
+  bool Flag = false;
+  switch (Flag) {
+// CHECK-MESSAGES:[[@LINE-1]]:3: warning: switch with only one case; use an if statement
+  case true:
+break;
+  }
+
+  switch (Flag) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch with default label only
+  default:
+break;
+  }
+
+  // This `switch` will create a frontend warning from '-Wswitch-bool' but is
+  // ok for this check.
+  switch (Flag) {
+  case true:
+break;
+  case false:
+break;
+  }
+}
+
+void unproblematic_switch(unsigned char c) {
+  //
+  switch (c) {
+  case 0:
+  case 1:
+  case 2:
+  case 3:
+  case 4:
+  case 5:
+  case 6:
+  case 7:
+  case 8:
+  case 9:
+  case 10:
+  case 11:
+  case 12:
+  case 13:
+  case 14:
+  case 15:
+  case 16:
+  case 17:
+  case 18:
+  case 19:
+  case 20:
+  case 21:
+  case 22:
+  case 23:
+  case 24:
+  case 25:
+  case 26:
+  case 27:
+  case 28:
+  case 29:
+  case 30:
+  case 31:
+  case 32:
+  case 33:
+  case 34:
+  case 35:
+  case 36:
+  case 37:
+  case 38:
+  case 39:
+  case 40:
+  case 41:
+  case 42:
+  case 43:
+  case 44:
+  case 45:
+  case 46:
+  case 47:
+  case 48:
+  case 49:
+  case 50:
+  case 51:
+  case 52:
+  case 53:
+  case 54:
+  case 55:
+  case 56:
+  case 57:
+  case 58:
+  case 59:
+  case 60:
+  case 61:
+  case 62:
+  case 63:
+  case 64:
+  case 65:
+  case 66:
+  case 67:
+  case 68:
+  case 69:
+  case 70:
+  case 71:
+  case 72:
+  case 73:
+  case 74:
+  case 75:
+  case 76:
+  case 77:
+  case 78:
+  case 79:
+  case 80:
+  case 81:
+  case 

[PATCH] D40737: [clang-tidy] Resubmit hicpp-multiway-paths-covered without breaking test

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




Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:119
+  if (!SwitchHasDefault && SwitchCaseCount == 0) {
+diag(Switch->getLocStart(), "degenerated switch without labels");
+return;

JonasToth wrote:
> aaron.ballman wrote:
> > I think a better way to phrase this one is: "switch statement without 
> > labels has no effect" and perhaps have a fix-it to replace the entire 
> > switch construct with its predicate?
> The fixit would only aim for the sideeffects the predicate has, right? I 
> would consider such a switch as a bug or are there reasonable things to 
> accomplish? Given its most likely unintended code i dont think a fixit is 
> good.
> 
> Fixing the code removes the warning and might introduce a bug.
This code pattern comes up with machine-generated code with some frequency, so 
I was thinking it would be nice to automatically fix that code. However, I 
think you're right that "fixing" the code may introduce bugs because you don't 
want to keep around non-side effecting operations and that's complicated. e.g.,
```
switch (i) { // Don't replace this with i;
}

switch (some_function_call()) { // Maybe replace this with some_function_call()?
}

switch (i = 12) { // Should definitely be replaced with i = 12;
}
```
Perhaps only diagnosing is the best option.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40737



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

[PATCH] D38798: [OpenMP] Support for implicit "declare target" functions - Sema patch

[George Rokos via Phabricator via cfe-commits]

grokos abandoned this revision.
grokos added a comment.

@ABataev came up with a much simpler solution to the implementation of `declare 
target`: https://reviews.llvm.org/rL327636

I am abandoning this obsolete revision.


Repository:
  rL LLVM

https://reviews.llvm.org/D38798



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

[PATCH] D43026: [OpenMP] CodeGen for the "declare target" directive - variables, functions, ctors/dtors

[George Rokos via Phabricator via cfe-commits]

grokos abandoned this revision.
grokos added a comment.

@ABataev came up with a much simpler solution to the implementation of `declare 
target`: https://reviews.llvm.org/rL327636

I am abandoning this obsolete revision.


Repository:
  rC Clang

https://reviews.llvm.org/D43026



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

[PATCH] D44632: [clang-format] Add a few more Core Graphics identifiers to ObjC heuristic

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.

Looks good.


Repository:
  rC Clang

https://reviews.llvm.org/D44632



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

[PATCH] D44692: [clang-format] Don't insert space between r_paren and 'new' in ObjC decl

[Daniel Jasper via Phabricator via cfe-commits]

djasper accepted this revision.
djasper added a comment.

Looks good.


Repository:
  rC Clang

https://reviews.llvm.org/D44692



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

[clang-tools-extra] r328100 - Make positionToOffset return llvm::Expected

[Simon Marchi via cfe-commits]

Author: simark
Date: Wed Mar 21 07:36:46 2018
New Revision: 328100

URL: http://llvm.org/viewvc/llvm-project?rev=328100=rev
Log:
Make positionToOffset return llvm::Expected

Summary:

To implement incremental document syncing, we want to verify that the
ranges provided by the front-end are valid.  Currently, positionToOffset
deals with invalid Positions by returning 0 or Code.size(), which are
two valid offsets.  Instead, return an llvm:Expected with an
error if the position is invalid.

According to the LSP, if the character value exceeds the number of
characters of the given line, it should default back to the end of the
line.  It makes sense in some contexts to have this behavior, and does
not in other contexts.  The AllowColumnsBeyondLineLength parameter
allows to decide what to do in that case, default back to the end of the
line, or return an error.

Reviewers: ilya-biryukov

Subscribers: klimek, ilya-biryukov, jkorous-apple, ioeric, cfe-commits

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

Modified:
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/SourceCode.cpp
clang-tools-extra/trunk/clangd/SourceCode.h
clang-tools-extra/trunk/unittests/clangd/Annotations.cpp
clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
clang-tools-extra/trunk/unittests/clangd/SourceCodeTests.cpp

Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=328100=328099=328100=diff
==
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Wed Mar 21 07:36:46 2018
@@ -190,9 +190,13 @@ void ClangdServer::signatureHelp(PathRef
 
 llvm::Expected
 ClangdServer::formatRange(StringRef Code, PathRef File, Range Rng) {
-  size_t Begin = positionToOffset(Code, Rng.start);
-  size_t Len = positionToOffset(Code, Rng.end) - Begin;
-  return formatCode(Code, File, {tooling::Range(Begin, Len)});
+  llvm::Expected Begin = positionToOffset(Code, Rng.start);
+  if (!Begin)
+return Begin.takeError();
+  llvm::Expected End = positionToOffset(Code, Rng.end);
+  if (!End)
+return End.takeError();
+  return formatCode(Code, File, {tooling::Range(*Begin, *End - *Begin)});
 }
 
 llvm::Expected ClangdServer::formatFile(StringRef Code,
@@ -205,11 +209,13 @@ llvm::Expected
 ClangdServer::formatOnType(StringRef Code, PathRef File, Position Pos) {
   // Look for the previous opening brace from the character position and
   // format starting from there.
-  size_t CursorPos = positionToOffset(Code, Pos);
-  size_t PreviousLBracePos = StringRef(Code).find_last_of('{', CursorPos);
+  llvm::Expected CursorPos = positionToOffset(Code, Pos);
+  if (!CursorPos)
+return CursorPos.takeError();
+  size_t PreviousLBracePos = StringRef(Code).find_last_of('{', *CursorPos);
   if (PreviousLBracePos == StringRef::npos)
-PreviousLBracePos = CursorPos;
-  size_t Len = CursorPos - PreviousLBracePos;
+PreviousLBracePos = *CursorPos;
+  size_t Len = *CursorPos - PreviousLBracePos;
 
   return formatCode(Code, File, {tooling::Range(PreviousLBracePos, Len)});
 }

Modified: clang-tools-extra/trunk/clangd/SourceCode.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/SourceCode.cpp?rev=328100=328099=328100=diff
==
--- clang-tools-extra/trunk/clangd/SourceCode.cpp (original)
+++ clang-tools-extra/trunk/clangd/SourceCode.cpp Wed Mar 21 07:36:46 2018
@@ -9,23 +9,43 @@
 #include "SourceCode.h"
 
 #include "clang/Basic/SourceManager.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
 
 namespace clang {
 namespace clangd {
 using namespace llvm;
 
-size_t positionToOffset(StringRef Code, Position P) {
+llvm::Expected positionToOffset(StringRef Code, Position P,
+bool AllowColumnsBeyondLineLength) {
   if (P.line < 0)
-return 0;
+return llvm::make_error(
+llvm::formatv("Line value can't be negative ({0})", P.line),
+llvm::errc::invalid_argument);
+  if (P.character < 0)
+return llvm::make_error(
+llvm::formatv("Character value can't be negative ({0})", P.character),
+llvm::errc::invalid_argument);
   size_t StartOfLine = 0;
   for (int I = 0; I != P.line; ++I) {
 size_t NextNL = Code.find('\n', StartOfLine);
 if (NextNL == StringRef::npos)
-  return Code.size();
+  return llvm::make_error(
+  llvm::formatv("Line value is out of range ({0})", P.line),
+  llvm::errc::invalid_argument);
 StartOfLine = NextNL + 1;
   }
+
+  size_t NextNL = Code.find('\n', StartOfLine);
+  if (NextNL == StringRef::npos)
+NextNL = Code.size();
+
+  if (StartOfLine + P.character > NextNL && !AllowColumnsBeyondLineLength)
+return llvm::make_error(
+  

[PATCH] D44231: [clang-tidy] Check for sizeof that call functions

[Alexander Kornienko via Phabricator via cfe-commits]

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

LG modulo other reviewers' open comments.


https://reviews.llvm.org/D44231



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

Re: [clang-tools-extra] r327833 - [clang-tidy] New check bugprone-unused-return-value

[Alexander Kornienko via cfe-commits]

On Mon, Mar 19, 2018 at 10:43 PM  wrote:

> This should be fixed by r327911.
>

Thanks!


>
>
> Douglas Yung
>
>
>
> *From:* cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] *On
> Behalf Of *Galina Kistanova via cfe-commits
> *Sent:* Monday, March 19, 2018 13:13
> *To:* Alexander Kornienko
> *Cc:* cfe-commits
> *Subject:* Re: [clang-tools-extra] r327833 - [clang-tidy] New check
> bugprone-unused-return-value
>
>
>
> Hello Alexander,
>
> This commit broke at least two of our builders:
>
>
> http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/26909
>
> http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast
>
> . . .
> Failing Tests (1):
> Clang Tools :: clang-tidy/bugprone-unused-return-value.cpp
>
> Please have a look?
>
> It is not good idea to keep the bot red for too long. This hides new
> problem which later hard to track down.
>
> Thanks
>
> Galina
>
>
>
> On Mon, Mar 19, 2018 at 6:02 AM, Alexander Kornienko via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
> Author: alexfh
> Date: Mon Mar 19 06:02:32 2018
> New Revision: 327833
>
> URL: http://llvm.org/viewvc/llvm-project?rev=327833=rev
> Log:
> [clang-tidy] New check bugprone-unused-return-value
>
> Summary:
> Detects function calls where the return value is unused.
>
> Checked functions can be configured.
>
> Reviewers: alexfh, aaron.ballman, ilya-biryukov, hokein
>
> Reviewed By: alexfh, aaron.ballman
>
> Subscribers: hintonda, JonasToth, Eugene.Zelenko, mgorny, xazax.hun,
> cfe-commits
>
> Tags: #clang-tools-extra
>
> Patch by Kalle Huttunen!
>
> Differential Revision: https://reviews.llvm.org/D41655
>
> Added:
> clang-tools-extra/trunk/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
> clang-tools-extra/trunk/clang-tidy/bugprone/UnusedReturnValueCheck.h
>
> clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-unused-return-value.rst
>
> clang-tools-extra/trunk/test/clang-tidy/bugprone-unused-return-value-custom.cpp
>
> clang-tools-extra/trunk/test/clang-tidy/bugprone-unused-return-value.cpp
> Modified:
> clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
> clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
> clang-tools-extra/trunk/docs/ReleaseNotes.rst
> clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
>
> Modified:
> clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp?rev=327833=327832=327833=diff
>
> ==
> --- clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
> (original)
> +++ clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp Mon
> Mar 19 06:02:32 2018
> @@ -43,6 +43,7 @@
>  #include "UndefinedMemoryManipulationCheck.h"
>  #include "UndelegatedConstructorCheck.h"
>  #include "UnusedRaiiCheck.h"
> +#include "UnusedReturnValueCheck.h"
>  #include "UseAfterMoveCheck.h"
>  #include "VirtualNearMissCheck.h"
>
> @@ -119,6 +120,8 @@ public:
>  "bugprone-undelegated-constructor");
>  CheckFactories.registerCheck(
>  "bugprone-unused-raii");
> +CheckFactories.registerCheck(
> +"bugprone-unused-return-value");
>  CheckFactories.registerCheck(
>  "bugprone-use-after-move");
>  CheckFactories.registerCheck(
>
> Modified: clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt?rev=327833=327832=327833=diff
>
> ==
> --- clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt (original)
> +++ clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt Mon Mar 19
> 06:02:32 2018
> @@ -35,6 +35,7 @@ add_clang_library(clangTidyBugproneModul
>UndefinedMemoryManipulationCheck.cpp
>UndelegatedConstructorCheck.cpp
>UnusedRaiiCheck.cpp
> +  UnusedReturnValueCheck.cpp
>UseAfterMoveCheck.cpp
>VirtualNearMissCheck.cpp
>
>
> Added:
> clang-tools-extra/trunk/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/bugprone/UnusedReturnValueCheck.cpp?rev=327833=auto
>
> ==
> --- clang-tools-extra/trunk/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
> (added)
> +++ clang-tools-extra/trunk/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
> Mon Mar 19 06:02:32 2018
> @@ -0,0 +1,82 @@
> +//===--- UnusedReturnValueCheck.cpp -
> clang-tidy---===//
> +//
> +// The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
>
> 

[PATCH] D44673: Make positionToOffset return llvm::Expected

[Simon Marchi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE328100: Make positionToOffset return 
llvm::Expectedsize_t (authored by simark, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D44673?vs=139292=139297#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D44673

Files:
  clangd/ClangdServer.cpp
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  unittests/clangd/Annotations.cpp
  unittests/clangd/CMakeLists.txt
  unittests/clangd/SourceCodeTests.cpp

Index: unittests/clangd/SourceCodeTests.cpp
===
--- unittests/clangd/SourceCodeTests.cpp
+++ unittests/clangd/SourceCodeTests.cpp
@@ -7,14 +7,19 @@
 //
 //===--===//
 #include "SourceCode.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/raw_os_ostream.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang{
 namespace clangd {
 namespace {
 
+using llvm::Failed;
+using llvm::HasValue;
+
 MATCHER_P2(Pos, Line, Col, "") {
   return arg.line == Line && arg.character == Col;
 }
@@ -33,30 +38,57 @@
 
 TEST(SourceCodeTests, PositionToOffset) {
   // line out of bounds
-  EXPECT_EQ(0u, positionToOffset(File, position(-1, 2)));
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(-1, 2)), Failed());
   // first line
-  EXPECT_EQ(0u, positionToOffset(File, position(0, -1))); // out of range
-  EXPECT_EQ(0u, positionToOffset(File, position(0, 0)));  // first character
-  EXPECT_EQ(3u, positionToOffset(File, position(0, 3)));  // middle character
-  EXPECT_EQ(6u, positionToOffset(File, position(0, 6)));  // last character
-  EXPECT_EQ(7u, positionToOffset(File, position(0, 7)));  // the newline itself
-  EXPECT_EQ(8u, positionToOffset(File, position(0, 8)));  // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, -1)),
+   Failed()); // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 0)),
+   HasValue(0)); // first character
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 3)),
+   HasValue(3)); // middle character
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 6)),
+   HasValue(6)); // last character
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 7)),
+   HasValue(7)); // the newline itself
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 7), false),
+   HasValue(7));
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 8)),
+   HasValue(7)); // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 8), false),
+   Failed()); // out of range
   // middle line
-  EXPECT_EQ(8u, positionToOffset(File, position(1, -1))); // out of range
-  EXPECT_EQ(8u, positionToOffset(File, position(1, 0)));  // first character
-  EXPECT_EQ(11u, positionToOffset(File, position(1, 3))); // middle character
-  EXPECT_EQ(14u, positionToOffset(File, position(1, 6))); // last character
-  EXPECT_EQ(15u, positionToOffset(File, position(1, 7))); // the newline itself
-  EXPECT_EQ(16u, positionToOffset(File, position(1, 8))); // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, -1)),
+   Failed()); // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 0)),
+   HasValue(8)); // first character
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 3)),
+   HasValue(11)); // middle character
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 3), false),
+   HasValue(11));
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 6)),
+   HasValue(14)); // last character
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 7)),
+   HasValue(15)); // the newline itself
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 8)),
+   HasValue(15)); // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 8), false),
+   Failed()); // out of range
   // last line
-  EXPECT_EQ(16u, positionToOffset(File, position(2, -1))); // out of range
-  EXPECT_EQ(16u, positionToOffset(File, position(2, 0)));  // first character
-  EXPECT_EQ(19u, positionToOffset(File, position(2, 3)));  // middle character
-  EXPECT_EQ(23u, positionToOffset(File, position(2, 7)));  // last character
-  EXPECT_EQ(24u, positionToOffset(File, position(2, 8)));  // EOF
-  EXPECT_EQ(24u, positionToOffset(File, position(2, 9)));  // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, -1)),
+   Failed()); // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, 0)),
+   HasValue(16)); // first 

[PATCH] D43766: [clang-tidy][modernize-make-unique] Checks c++14 flag before using std::make_unique

[Alexander Kornienko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL328101: [clang-tidy][modernize-make-unique] Checks c++14 
flag before using std… (authored by alexfh, committed by ).
Herald added subscribers: llvm-commits, klimek.

Changed prior to commit:
  https://reviews.llvm.org/D43766?vs=138398=139298#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D43766

Files:
  clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp
  clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.h
  clang-tools-extra/trunk/clang-tidy/modernize/MakeUniqueCheck.cpp
  clang-tools-extra/trunk/clang-tidy/modernize/MakeUniqueCheck.h
  clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique-cxx11.cpp
  clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique-cxx14.cpp
  clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique-macros.cpp
  clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique.cpp

Index: clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.h
+++ clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.h
@@ -40,6 +40,9 @@
   /// in this class.
   virtual SmartPtrTypeMatcher getSmartPointerTypeMatcher() const = 0;
 
+  /// Returns whether the C++ version is compatible with current check.
+  virtual bool isLanguageVersionSupported(const LangOptions ) const;
+
   static const char PointerType[];
   static const char ConstructorCall[];
   static const char ResetCall[];
Index: clang-tools-extra/trunk/clang-tidy/modernize/MakeUniqueCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/MakeUniqueCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/modernize/MakeUniqueCheck.cpp
@@ -17,7 +17,8 @@
 
 MakeUniqueCheck::MakeUniqueCheck(StringRef Name,
  clang::tidy::ClangTidyContext *Context)
-: MakeSmartPtrCheck(Name, Context, "std::make_unique") {}
+: MakeSmartPtrCheck(Name, Context, "std::make_unique"),
+  RequireCPlusPlus14(Options.get("MakeSmartPtrFunction", "").empty()) {}
 
 MakeUniqueCheck::SmartPtrTypeMatcher
 MakeUniqueCheck::getSmartPointerTypeMatcher() const {
@@ -36,6 +37,11 @@
 equalsBoundNode(PointerType;
 }
 
+bool MakeUniqueCheck::isLanguageVersionSupported(
+const LangOptions ) const {
+  return RequireCPlusPlus14 ? LangOpts.CPlusPlus14 : LangOpts.CPlusPlus11;
+}
+
 } // namespace modernize
 } // namespace tidy
 } // namespace clang
Index: clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp
@@ -61,16 +61,21 @@
   Options.store(Opts, "IgnoreMacros", IgnoreMacros);
 }
 
+bool MakeSmartPtrCheck::isLanguageVersionSupported(
+const LangOptions ) const {
+  return LangOpts.CPlusPlus11;
+}
+
 void MakeSmartPtrCheck::registerPPCallbacks(CompilerInstance ) {
-  if (getLangOpts().CPlusPlus11) {
+  if (isLanguageVersionSupported(getLangOpts())) {
 Inserter.reset(new utils::IncludeInserter(
 Compiler.getSourceManager(), Compiler.getLangOpts(), IncludeStyle));
 Compiler.getPreprocessor().addPPCallbacks(Inserter->CreatePPCallbacks());
   }
 }
 
 void MakeSmartPtrCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
-  if (!getLangOpts().CPlusPlus11)
+  if (!isLanguageVersionSupported(getLangOpts()))
 return;
 
   // Calling make_smart_ptr from within a member function of a type with a
Index: clang-tools-extra/trunk/clang-tidy/modernize/MakeUniqueCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/modernize/MakeUniqueCheck.h
+++ clang-tools-extra/trunk/clang-tidy/modernize/MakeUniqueCheck.h
@@ -31,6 +31,11 @@
 
 protected:
   SmartPtrTypeMatcher getSmartPointerTypeMatcher() const override;
+
+  bool isLanguageVersionSupported(const LangOptions ) const override;
+
+private:
+  const bool RequireCPlusPlus14;
 };
 
 } // namespace modernize
Index: clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s modernize-make-unique %t -- -- -std=c++11 \
+// RUN: %check_clang_tidy %s modernize-make-unique %t -- -- -std=c++14 \
 // RUN:   -I%S/Inputs/modernize-smart-ptr
 
 #include "unique_ptr.h"
Index: clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique-cxx11.cpp
===
--- 

[PATCH] D44673: Make positionToOffset return llvm::Expected

[Simon Marchi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL328100: Make positionToOffset return 
llvm::Expectedsize_t (authored by simark, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D44673

Files:
  clang-tools-extra/trunk/clangd/ClangdServer.cpp
  clang-tools-extra/trunk/clangd/SourceCode.cpp
  clang-tools-extra/trunk/clangd/SourceCode.h
  clang-tools-extra/trunk/unittests/clangd/Annotations.cpp
  clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
  clang-tools-extra/trunk/unittests/clangd/SourceCodeTests.cpp

Index: clang-tools-extra/trunk/unittests/clangd/Annotations.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/Annotations.cpp
+++ clang-tools-extra/trunk/unittests/clangd/Annotations.cpp
@@ -86,7 +86,11 @@
 std::pair
 Annotations::offsetRange(llvm::StringRef Name) const {
   auto R = range(Name);
-  return {positionToOffset(Code, R.start), positionToOffset(Code, R.end)};
+  llvm::Expected Start = positionToOffset(Code, R.start);
+  llvm::Expected End = positionToOffset(Code, R.end);
+  assert(Start);
+  assert(End);
+  return {*Start, *End};
 }
 
 } // namespace clangd
Index: clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
===
--- clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
+++ clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
@@ -43,4 +43,5 @@
   clangTooling
   clangToolingCore
   LLVMSupport
+  LLVMTestingSupport
   )
Index: clang-tools-extra/trunk/unittests/clangd/SourceCodeTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/SourceCodeTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/SourceCodeTests.cpp
@@ -7,14 +7,19 @@
 //
 //===--===//
 #include "SourceCode.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/raw_os_ostream.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang{
 namespace clangd {
 namespace {
 
+using llvm::Failed;
+using llvm::HasValue;
+
 MATCHER_P2(Pos, Line, Col, "") {
   return arg.line == Line && arg.character == Col;
 }
@@ -33,30 +38,57 @@
 
 TEST(SourceCodeTests, PositionToOffset) {
   // line out of bounds
-  EXPECT_EQ(0u, positionToOffset(File, position(-1, 2)));
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(-1, 2)), Failed());
   // first line
-  EXPECT_EQ(0u, positionToOffset(File, position(0, -1))); // out of range
-  EXPECT_EQ(0u, positionToOffset(File, position(0, 0)));  // first character
-  EXPECT_EQ(3u, positionToOffset(File, position(0, 3)));  // middle character
-  EXPECT_EQ(6u, positionToOffset(File, position(0, 6)));  // last character
-  EXPECT_EQ(7u, positionToOffset(File, position(0, 7)));  // the newline itself
-  EXPECT_EQ(8u, positionToOffset(File, position(0, 8)));  // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, -1)),
+   Failed()); // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 0)),
+   HasValue(0)); // first character
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 3)),
+   HasValue(3)); // middle character
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 6)),
+   HasValue(6)); // last character
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 7)),
+   HasValue(7)); // the newline itself
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 7), false),
+   HasValue(7));
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 8)),
+   HasValue(7)); // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 8), false),
+   Failed()); // out of range
   // middle line
-  EXPECT_EQ(8u, positionToOffset(File, position(1, -1))); // out of range
-  EXPECT_EQ(8u, positionToOffset(File, position(1, 0)));  // first character
-  EXPECT_EQ(11u, positionToOffset(File, position(1, 3))); // middle character
-  EXPECT_EQ(14u, positionToOffset(File, position(1, 6))); // last character
-  EXPECT_EQ(15u, positionToOffset(File, position(1, 7))); // the newline itself
-  EXPECT_EQ(16u, positionToOffset(File, position(1, 8))); // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, -1)),
+   Failed()); // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 0)),
+   HasValue(8)); // first character
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 3)),
+   HasValue(11)); // middle character
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 3), false),
+   

[clang-tools-extra] r328101 - [clang-tidy][modernize-make-unique] Checks c++14 flag before using std::make_unique

[Alexander Kornienko via cfe-commits]

Author: alexfh
Date: Wed Mar 21 07:39:24 2018
New Revision: 328101

URL: http://llvm.org/viewvc/llvm-project?rev=328101=rev
Log:
[clang-tidy][modernize-make-unique] Checks c++14 flag before using 
std::make_unique

Summary: For a c++11 code, the clang-tidy rule "modernize-make-unique" should 
return immediately, as std::make_unique is not supported.

Reviewers: hokein, aaron.ballman, ilya-biryukov, alexfh

Reviewed By: hokein, aaron.ballman, alexfh

Subscribers: Quuxplusone, xazax.hun, cfe-commits

Tags: #clang-tools-extra

Patch by Frederic Tingaud!

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

Added:
clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique-cxx11.cpp
clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique-cxx14.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp
clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.h
clang-tools-extra/trunk/clang-tidy/modernize/MakeUniqueCheck.cpp
clang-tools-extra/trunk/clang-tidy/modernize/MakeUniqueCheck.h
clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique-macros.cpp
clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique.cpp

Modified: clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp?rev=328101=328100=328101=diff
==
--- clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp 
(original)
+++ clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp Wed Mar 
21 07:39:24 2018
@@ -61,8 +61,13 @@ void MakeSmartPtrCheck::storeOptions(Cla
   Options.store(Opts, "IgnoreMacros", IgnoreMacros);
 }
 
+bool MakeSmartPtrCheck::isLanguageVersionSupported(
+const LangOptions ) const {
+  return LangOpts.CPlusPlus11;
+}
+
 void MakeSmartPtrCheck::registerPPCallbacks(CompilerInstance ) {
-  if (getLangOpts().CPlusPlus11) {
+  if (isLanguageVersionSupported(getLangOpts())) {
 Inserter.reset(new utils::IncludeInserter(
 Compiler.getSourceManager(), Compiler.getLangOpts(), IncludeStyle));
 Compiler.getPreprocessor().addPPCallbacks(Inserter->CreatePPCallbacks());
@@ -70,7 +75,7 @@ void MakeSmartPtrCheck::registerPPCallba
 }
 
 void MakeSmartPtrCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
-  if (!getLangOpts().CPlusPlus11)
+  if (!isLanguageVersionSupported(getLangOpts()))
 return;
 
   // Calling make_smart_ptr from within a member function of a type with a

Modified: clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.h?rev=328101=328100=328101=diff
==
--- clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.h (original)
+++ clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.h Wed Mar 21 
07:39:24 2018
@@ -40,6 +40,9 @@ protected:
   /// in this class.
   virtual SmartPtrTypeMatcher getSmartPointerTypeMatcher() const = 0;
 
+  /// Returns whether the C++ version is compatible with current check.
+  virtual bool isLanguageVersionSupported(const LangOptions ) const;
+
   static const char PointerType[];
   static const char ConstructorCall[];
   static const char ResetCall[];

Modified: clang-tools-extra/trunk/clang-tidy/modernize/MakeUniqueCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/MakeUniqueCheck.cpp?rev=328101=328100=328101=diff
==
--- clang-tools-extra/trunk/clang-tidy/modernize/MakeUniqueCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/modernize/MakeUniqueCheck.cpp Wed Mar 21 
07:39:24 2018
@@ -17,7 +17,8 @@ namespace modernize {
 
 MakeUniqueCheck::MakeUniqueCheck(StringRef Name,
  clang::tidy::ClangTidyContext *Context)
-: MakeSmartPtrCheck(Name, Context, "std::make_unique") {}
+: MakeSmartPtrCheck(Name, Context, "std::make_unique"),
+  RequireCPlusPlus14(Options.get("MakeSmartPtrFunction", "").empty()) {}
 
 MakeUniqueCheck::SmartPtrTypeMatcher
 MakeUniqueCheck::getSmartPointerTypeMatcher() const {
@@ -36,6 +37,11 @@ MakeUniqueCheck::getSmartPointerTypeMatc
 
equalsBoundNode(PointerType;
 }
 
+bool MakeUniqueCheck::isLanguageVersionSupported(
+const LangOptions ) const {
+  return RequireCPlusPlus14 ? LangOpts.CPlusPlus14 : LangOpts.CPlusPlus11;
+}
+
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

Modified: clang-tools-extra/trunk/clang-tidy/modernize/MakeUniqueCheck.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/MakeUniqueCheck.h?rev=328101=328100=328101=diff

[PATCH] D44694: [clang-tidy] Use :doc: for check links in Release Notes

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh accepted this revision.
alexfh added a comment.

LG. Thanks!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44694



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

[PATCH] D44673: Make positionToOffset return llvm::Expected

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdServer.cpp:199
+return End.takeError();
+
+  return formatCode(Code, File, {tooling::Range(*Begin, *End - *Begin)});

simark wrote:
> ilya-biryukov wrote:
> > NIT: unnecessary empty line
> In general I like spacing out the different logical blocks of code a little 
> bit, especially after a "return", but I don't mind removing them.
We don't really have a style rule for that, but most of clangd code does not 
have that many empty lines.
I personally think it's fine to have reasonable style differences in new 
functions, but I prefer to stick to the original author's style when 
refactoring code. Hence my suggestions to remove the empty lines here.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44673



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

[PATCH] D43967: [ASTImporter] Add test helper Fixture

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: unittests/AST/ASTImporterTest.cpp:1169
+  MatchVerifier Verifier;
+  ASSERT_TRUE(Verifier.match(From, cxxRecordDecl(has(fieldDecl();
+  ASSERT_TRUE(Verifier.match(To, cxxRecordDecl(has(fieldDecl();

a.sidorin wrote:
> Tests with `has(fieldDecl())` duplicate tests with `hasFieldOrder()`. Is it 
> intentional?
Ok, I removed the redundant `has(fieldDecl())` checks.



Comment at: unittests/AST/ASTImporterTest.cpp:1171
+  ASSERT_TRUE(Verifier.match(To, cxxRecordDecl(has(fieldDecl();
+  ASSERT_TRUE(Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b"};
+  EXPECT_TRUE(Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b"};

a.sidorin wrote:
> Should this be `match(From, ...)` instead of `To`?

That's right, good catch.



Comment at: unittests/AST/ASTImporterTest.cpp:1177
+ASTImporterTestBase,
+
DISABLED_CXXRecordDeclFieldsShouldBeInCorrectOrderEvenWhenWeImportFirstTheLastDecl)
 {
+  Decl *From, *To;

a.sidorin wrote:
> This identifier is very long. How about renaming it to 
> DISABLED_CXXRecordDeclFieldOrderShouldNotDependOnImportOrder? It is 60 char 
> long instead of 82.
Ok, I like the shorter name, changed it.


Repository:
  rC Clang

https://reviews.llvm.org/D43967



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

[PATCH] D43967: [ASTImporter] Add test helper Fixture

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 139295.
martong marked 7 inline comments as done.
martong added a comment.

Add minor syntax changes and rename


Repository:
  rC Clang

https://reviews.llvm.org/D43967

Files:
  unittests/AST/ASTImporterTest.cpp
  unittests/AST/DeclMatcher.h

Index: unittests/AST/DeclMatcher.h
===
--- /dev/null
+++ unittests/AST/DeclMatcher.h
@@ -0,0 +1,68 @@
+//===- unittest/AST/DeclMatcher.h - AST unit test support ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_UNITTESTS_AST_DECLMATCHER_H
+#define LLVM_CLANG_UNITTESTS_AST_DECLMATCHER_H
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+namespace clang {
+namespace ast_matchers {
+
+enum class DeclMatcherKind { First, Last };
+
+// Matcher class to retrieve the first/last matched node under a given AST.
+template 
+class DeclMatcher : public MatchFinder::MatchCallback {
+  NodeType *Node = nullptr;
+  void run(const MatchFinder::MatchResult ) override {
+if ((MatcherKind == DeclMatcherKind::First && Node == nullptr) ||
+MatcherKind == DeclMatcherKind::Last) {
+  Node = const_cast(Result.Nodes.getNodeAs(""));
+}
+  }
+public:
+  // Returns the first/last matched node under the tree rooted in `D`.
+  template 
+  NodeType *match(const Decl *D, const MatcherType ) {
+MatchFinder Finder;
+Finder.addMatcher(AMatcher.bind(""), this);
+Finder.matchAST(D->getASTContext());
+assert(Node);
+return Node;
+  }
+};
+template 
+using LastDeclMatcher = DeclMatcher;
+template 
+using FirstDeclMatcher = DeclMatcher;
+
+template 
+class DeclCounter : public MatchFinder::MatchCallback {
+  unsigned Count = 0;
+  void run(const MatchFinder::MatchResult ) override {
+  if(Result.Nodes.getNodeAs("")) {
+++Count;
+  }
+  }
+public:
+  // Returns the number of matched nodes under the tree rooted in `D`.
+  template 
+  unsigned match(const Decl *D, const MatcherType ) {
+MatchFinder Finder;
+Finder.addMatcher(AMatcher.bind(""), this);
+Finder.matchAST(D->getASTContext());
+return Count;
+  }
+};
+
+} // end namespace ast_matchers
+} // end namespace clang
+
+#endif
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -17,6 +17,8 @@
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Tooling.h"
+
+#include "DeclMatcher.h"
 #include "gtest/gtest.h"
 
 namespace clang {
@@ -29,7 +31,7 @@
   return Lang == Lang_CXX || Lang == Lang_CXX11;
 }
 
-static RunOptions getRunOptionsForLanguage(Language Lang) {
+static ArgVector getBasicRunOptionsForLanguage(Language Lang) {
   ArgVector BasicArgs;
   // Test with basic arguments.
   switch (Lang) {
@@ -49,6 +51,11 @@
   case Lang_OBJCXX:
 llvm_unreachable("Not implemented yet!");
   }
+  return BasicArgs;
+}
+
+static RunOptions getRunOptionsForLanguage(Language Lang) {
+  ArgVector BasicArgs = getBasicRunOptionsForLanguage(Lang);
 
   // For C++, test with "-fdelayed-template-parsing" enabled to handle MSVC
   // default behaviour.
@@ -61,6 +68,19 @@
   return {BasicArgs};
 }
 
+// Creates a virtual file and assigns that to the context of given AST. If the
+// file already exists then the file will not be created again as a duplicate.
+static void createVirtualFile(ASTUnit *ToAST, StringRef FileName,
+  const std::string ) {
+  assert(ToAST);
+  ASTContext  = ToAST->getASTContext();
+  auto *OFS = static_cast(
+  ToCtx.getSourceManager().getFileManager().getVirtualFileSystem().get());
+  auto *MFS =
+  static_cast(OFS->overlays_begin()->get());
+  MFS->addFile(FileName, 0, llvm::MemoryBuffer::getMemBuffer(Code.c_str()));
+}
+
 template
 testing::AssertionResult
 testImport(const std::string , const ArgVector ,
@@ -79,11 +99,7 @@
 
   // Add input.cc to virtual file system so importer can 'find' it
   // while importing SourceLocations.
-  vfs::OverlayFileSystem *OFS = static_cast(
-ToCtx.getSourceManager().getFileManager().getVirtualFileSystem().get());
-  vfs::InMemoryFileSystem *MFS = static_cast(
-OFS->overlays_begin()->get());
-  MFS->addFile(InputFileName, 0, llvm::MemoryBuffer::getMemBuffer(FromCode));
+  createVirtualFile(ToAST.get(), InputFileName, FromCode);
 
   ASTImporter Importer(ToCtx, ToAST->getFileManager(),
FromCtx, FromAST->getFileManager(), false);
@@ -133,6 +149,167 @@
  Verifier, AMatcher));
 }
 
+const StringRef DeclToImportID = 

[PATCH] D40737: [clang-tidy] Resubmit hicpp-multiway-paths-covered without breaking test

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp:119
+  if (!SwitchHasDefault && SwitchCaseCount == 0) {
+diag(Switch->getLocStart(), "degenerated switch without labels");
+return;

aaron.ballman wrote:
> I think a better way to phrase this one is: "switch statement without labels 
> has no effect" and perhaps have a fix-it to replace the entire switch 
> construct with its predicate?
The fixit would only aim for the sideeffects the predicate has, right? I would 
consider such a switch as a bug or are there reasonable things to accomplish? 
Given its most likely unintended code i dont think a fixit is good.

Fixing the code removes the warning and might introduce a bug.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40737



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

[PATCH] D40737: [clang-tidy] Resubmit hicpp-multiway-paths-covered without breaking test

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 139293.
JonasToth marked 4 inline comments as done.
JonasToth added a comment.

- adress review comments


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40737

Files:
  clang-tidy/hicpp/CMakeLists.txt
  clang-tidy/hicpp/HICPPTidyModule.cpp
  clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
  clang-tidy/hicpp/MultiwayPathsCoveredCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/hicpp-multiway-paths-covered.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/hicpp-multiway-paths-covered-else.cpp
  test/clang-tidy/hicpp-multiway-paths-covered.cpp

Index: test/clang-tidy/hicpp-multiway-paths-covered.cpp
===
--- /dev/null
+++ test/clang-tidy/hicpp-multiway-paths-covered.cpp
@@ -0,0 +1,468 @@
+// RUN: %check_clang_tidy %s hicpp-multiway-paths-covered %t
+
+enum OS { Mac,
+  Windows,
+  Linux };
+
+struct Bitfields {
+  unsigned UInt : 3;
+  int SInt : 1;
+};
+
+int return_integer() { return 42; }
+
+void bad_switch(int i) {
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with only one case; use an if statement
+  case 0:
+break;
+  }
+  // No default in this switch
+  switch (i) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label
+  case 0:
+break;
+  case 1:
+break;
+  case 2:
+break;
+  }
+
+  // degenerate, maybe even warning
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch statement without labels has no effect
+  }
+
+  switch (int j = return_integer()) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label
+  case 0:
+  case 1:
+  case 2:
+break;
+  }
+
+  // Degenerated, only default case.
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch with default label only
+  default:
+break;
+  }
+
+  // Degenerated, only one case label and default case -> Better as if-stmt.
+  switch (i) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch could be better written as an if/else statement
+  case 0:
+break;
+  default:
+break;
+  }
+
+  unsigned long long BigNumber = 0;
+  switch (BigNumber) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label
+  case 0:
+  case 1:
+break;
+  }
+
+  const int  = i;
+  switch (IntRef) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label
+  case 0:
+  case 1:
+break;
+  }
+
+  char C = 'A';
+  switch (C) {
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label
+  case 'A':
+break;
+  case 'B':
+break;
+  }
+
+  Bitfields Bf;
+  // UInt has 3 bits size.
+  switch (Bf.UInt) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: potential uncovered code path; add a default label
+  case 0:
+  case 1:
+break;
+  }
+  // All paths explicitly covered.
+  switch (Bf.UInt) {
+  case 0:
+  case 1:
+  case 2:
+  case 3:
+  case 4:
+  case 5:
+  case 6:
+  case 7:
+break;
+  }
+  // SInt has 1 bit size, so this is somewhat degenerated.
+  switch (Bf.SInt) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: switch with only one case; use an if statement
+  case 0:
+break;
+  }
+  // All paths explicitly covered.
+  switch (Bf.SInt) {
+  case 0:
+  case 1:
+break;
+  }
+
+  bool Flag = false;
+  switch (Flag) {
+// CHECK-MESSAGES:[[@LINE-1]]:3: warning: switch with only one case; use an if statement
+  case true:
+break;
+  }
+
+  switch (Flag) {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: degenerated switch with default label only
+  default:
+break;
+  }
+
+  // This `switch` will create a frontend warning from '-Wswitch-bool' but is
+  // ok for this check.
+  switch (Flag) {
+  case true:
+break;
+  case false:
+break;
+  }
+}
+
+void unproblematic_switch(unsigned char c) {
+  //
+  switch (c) {
+  case 0:
+  case 1:
+  case 2:
+  case 3:
+  case 4:
+  case 5:
+  case 6:
+  case 7:
+  case 8:
+  case 9:
+  case 10:
+  case 11:
+  case 12:
+  case 13:
+  case 14:
+  case 15:
+  case 16:
+  case 17:
+  case 18:
+  case 19:
+  case 20:
+  case 21:
+  case 22:
+  case 23:
+  case 24:
+  case 25:
+  case 26:
+  case 27:
+  case 28:
+  case 29:
+  case 30:
+  case 31:
+  case 32:
+  case 33:
+  case 34:
+  case 35:
+  case 36:
+  case 37:
+  case 38:
+  case 39:
+  case 40:
+  case 41:
+  case 42:
+  case 43:
+  case 44:
+  case 45:
+  case 46:
+  case 47:
+  case 48:
+  case 49:
+  case 50:
+  case 51:
+  case 52:
+  case 53:
+  case 54:
+  case 55:
+  case 56:
+  case 57:
+  case 58:
+  case 59:
+  case 60:
+  case 61:
+  case 62:
+  case 63:
+  case 64:
+  case 65:
+  case 66:
+  case 67:
+  case 68:
+  case 69:
+  case 70:
+  case 71:
+  case 72:
+  case 73:
+  case 74:
+  case 75:
+  case 76:
+  case 77:
+  case 78:
+  case 79:
+  case 80:
+  case 81:
+  case 82:
+  

[PATCH] D44673: Make positionToOffset return llvm::Expected

[Simon Marchi via Phabricator via cfe-commits]

simark marked 6 inline comments as done.
simark added inline comments.



Comment at: clangd/ClangdServer.cpp:199
+return End.takeError();
+
+  return formatCode(Code, File, {tooling::Range(*Begin, *End - *Begin)});

ilya-biryukov wrote:
> NIT: unnecessary empty line
In general I like spacing out the different logical blocks of code a little 
bit, especially after a "return", but I don't mind removing them.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44673



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

[PATCH] D44673: Make positionToOffset return llvm::Expected

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 139292.
simark marked 3 inline comments as done.
simark added a comment.

Remove spaces, add fixmes


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44673

Files:
  clangd/ClangdServer.cpp
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  unittests/clangd/Annotations.cpp
  unittests/clangd/CMakeLists.txt
  unittests/clangd/SourceCodeTests.cpp

Index: unittests/clangd/SourceCodeTests.cpp
===
--- unittests/clangd/SourceCodeTests.cpp
+++ unittests/clangd/SourceCodeTests.cpp
@@ -7,14 +7,19 @@
 //
 //===--===//
 #include "SourceCode.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/raw_os_ostream.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang{
 namespace clangd {
 namespace {
 
+using llvm::Failed;
+using llvm::HasValue;
+
 MATCHER_P2(Pos, Line, Col, "") {
   return arg.line == Line && arg.character == Col;
 }
@@ -33,30 +38,57 @@
 
 TEST(SourceCodeTests, PositionToOffset) {
   // line out of bounds
-  EXPECT_EQ(0u, positionToOffset(File, position(-1, 2)));
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(-1, 2)), Failed());
   // first line
-  EXPECT_EQ(0u, positionToOffset(File, position(0, -1))); // out of range
-  EXPECT_EQ(0u, positionToOffset(File, position(0, 0)));  // first character
-  EXPECT_EQ(3u, positionToOffset(File, position(0, 3)));  // middle character
-  EXPECT_EQ(6u, positionToOffset(File, position(0, 6)));  // last character
-  EXPECT_EQ(7u, positionToOffset(File, position(0, 7)));  // the newline itself
-  EXPECT_EQ(8u, positionToOffset(File, position(0, 8)));  // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, -1)),
+   Failed()); // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 0)),
+   HasValue(0)); // first character
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 3)),
+   HasValue(3)); // middle character
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 6)),
+   HasValue(6)); // last character
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 7)),
+   HasValue(7)); // the newline itself
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 7), false),
+   HasValue(7));
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 8)),
+   HasValue(7)); // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(0, 8), false),
+   Failed()); // out of range
   // middle line
-  EXPECT_EQ(8u, positionToOffset(File, position(1, -1))); // out of range
-  EXPECT_EQ(8u, positionToOffset(File, position(1, 0)));  // first character
-  EXPECT_EQ(11u, positionToOffset(File, position(1, 3))); // middle character
-  EXPECT_EQ(14u, positionToOffset(File, position(1, 6))); // last character
-  EXPECT_EQ(15u, positionToOffset(File, position(1, 7))); // the newline itself
-  EXPECT_EQ(16u, positionToOffset(File, position(1, 8))); // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, -1)),
+   Failed()); // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 0)),
+   HasValue(8)); // first character
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 3)),
+   HasValue(11)); // middle character
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 3), false),
+   HasValue(11));
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 6)),
+   HasValue(14)); // last character
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 7)),
+   HasValue(15)); // the newline itself
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 8)),
+   HasValue(15)); // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(1, 8), false),
+   Failed()); // out of range
   // last line
-  EXPECT_EQ(16u, positionToOffset(File, position(2, -1))); // out of range
-  EXPECT_EQ(16u, positionToOffset(File, position(2, 0)));  // first character
-  EXPECT_EQ(19u, positionToOffset(File, position(2, 3)));  // middle character
-  EXPECT_EQ(23u, positionToOffset(File, position(2, 7)));  // last character
-  EXPECT_EQ(24u, positionToOffset(File, position(2, 8)));  // EOF
-  EXPECT_EQ(24u, positionToOffset(File, position(2, 9)));  // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, -1)),
+   Failed()); // out of range
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, 0)),
+   HasValue(16)); // first character
+  EXPECT_THAT_EXPECTED(positionToOffset(File, position(2, 3)),
+   HasValue(19)); // 

[PATCH] D44732: [clangd] Set CLANGD_EDITOR environment variable in vscode extension.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: sammccall.
Herald added subscribers: ioeric, jkorous-apple, ilya-biryukov, klimek.

This would allow us to log the editor using custom-built clangd.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44732

Files:
  clangd/clients/clangd-vscode/package.json
  clangd/clients/clangd-vscode/src/extension.ts


Index: clangd/clients/clangd-vscode/src/extension.ts
===
--- clangd/clients/clangd-vscode/src/extension.ts
+++ clangd/clients/clangd-vscode/src/extension.ts
@@ -23,6 +23,9 @@
 command: getConfig('path'),
 args: getConfig('arguments')
 };
+// Set "CLANGD_EDITOR" environment variable allowing us to log which editor
+// uses clangd.
+process.env['CLANGD_EDITOR'] = 'vscode';
 const traceFile = getConfig('trace');
 if (!!traceFile) {
 const trace = { CLANGD_TRACE: traceFile };
Index: clangd/clients/clangd-vscode/package.json
===
--- clangd/clients/clangd-vscode/package.json
+++ clangd/clients/clangd-vscode/package.json
@@ -2,7 +2,7 @@
 "name": "vscode-clangd",
 "displayName": "vscode-clangd",
 "description": "Clang Language Server",
-"version": "0.0.5",
+"version": "0.0.6",
 "publisher": "llvm-vs-code-extensions",
 "homepage": "https://clang.llvm.org/extra/clangd.html;,
 "engines": {


Index: clangd/clients/clangd-vscode/src/extension.ts
===
--- clangd/clients/clangd-vscode/src/extension.ts
+++ clangd/clients/clangd-vscode/src/extension.ts
@@ -23,6 +23,9 @@
 command: getConfig('path'),
 args: getConfig('arguments')
 };
+// Set "CLANGD_EDITOR" environment variable allowing us to log which editor
+// uses clangd.
+process.env['CLANGD_EDITOR'] = 'vscode';
 const traceFile = getConfig('trace');
 if (!!traceFile) {
 const trace = { CLANGD_TRACE: traceFile };
Index: clangd/clients/clangd-vscode/package.json
===
--- clangd/clients/clangd-vscode/package.json
+++ clangd/clients/clangd-vscode/package.json
@@ -2,7 +2,7 @@
 "name": "vscode-clangd",
 "displayName": "vscode-clangd",
 "description": "Clang Language Server",
-"version": "0.0.5",
+"version": "0.0.6",
 "publisher": "llvm-vs-code-extensions",
 "homepage": "https://clang.llvm.org/extra/clangd.html;,
 "engines": {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r328092 - clang-interpreter example cmake fix

[Luke Cheeseman via cfe-commits]

Author: lukecheeseman
Date: Wed Mar 21 05:05:19 2018
New Revision: 328092

URL: http://llvm.org/viewvc/llvm-project?rev=328092=rev
Log:
clang-interpreter example cmake fix

Add in a space when appending the export to the linker options. Without
the space the export is appended onto whatever the last link option
was, which might be a file.


Modified:
cfe/trunk/examples/clang-interpreter/CMakeLists.txt

Modified: cfe/trunk/examples/clang-interpreter/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/examples/clang-interpreter/CMakeLists.txt?rev=328092=328091=328092=diff
==
--- cfe/trunk/examples/clang-interpreter/CMakeLists.txt (original)
+++ cfe/trunk/examples/clang-interpreter/CMakeLists.txt Wed Mar 21 05:05:19 2018
@@ -34,7 +34,7 @@ if (MSVC)
   # Is this a CMake bug that even with export_executable_symbols, Windows
   # needs to explictly export the type_info vtable
   set_property(TARGET clang-interpreter
-   APPEND_STRING PROPERTY LINK_FLAGS /EXPORT:??_7type_info@@6B@)
+   APPEND_STRING PROPERTY LINK_FLAGS " /EXPORT:??_7type_info@@6B@")
 endif()
 
 function(clang_enable_exceptions TARGET)


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

[PATCH] D44673: Make positionToOffset return llvm::Expected

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/SourceCode.h:41
 /// Turn an offset in Code into a [line, column] pair.
 Position offsetToPosition(llvm::StringRef Code, size_t Offset);
 

We should be consistent for all functions inside this fail. They may all fail 
for the same reasons, and it doesn't make sense for them to have a different 
interfaces.
Could you add a FIXME that we should update other functions to report errors?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44673



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

[PATCH] D44589: [Sema] Make deprecation fix-it replace all multi-parameter ObjC method slots.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

This generally looks reasonable to me, but @rsmith should weigh in before you 
commit because `MultiSourceLocation` is novel.


https://reviews.llvm.org/D44589



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

[PATCH] D44729: [ASTMatchers] Add hasSubExpr() matcher.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think `hasSourceExpression()` already does what you're looking for.

  void f() {
int i = 1.0f;
  }

clang-query> match implicitCastExpr(hasSourceExpression(floatLiteral()))

Match #1:

C:\Users\aballman.GRAMMATECH\Desktop\test.c:2:11: note: "root" binds here

  int i = 1.0f;
  ^~~~

1 match.


Repository:
  rC Clang

https://reviews.llvm.org/D44729



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

[PATCH] D43957: Fixing issue where a space was not added before a global namespace variable when SpacesInParentheses is set

[Daniel Jasper via Phabricator via cfe-commits]

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

Please generally upload diffs with the full file as context so that Phabricator 
can properly expand the context where necessary.

This looks good, though.


Repository:
  rC Clang

https://reviews.llvm.org/D43957



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

[PATCH] D43967: [ASTImporter] Add test helper Fixture

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

Hi Gabor! This looks much better.




Comment at: unittests/AST/ASTImporterTest.cpp:1000
+  R"(
+  template
+  struct X {};

In C++, names started with underscore are reserved for implementation so it's 
undesirable to use them.



Comment at: unittests/AST/ASTImporterTest.cpp:1003
+
+  void declToImport(int y,X ){}
+

I still can see a number of style violations in inline code. Some examples: 
lack of space between arguments and parameters, no space before '{' in `g(){`, 
etc. Could you please fix them?



Comment at: unittests/AST/ASTImporterTest.cpp:1169
+  MatchVerifier Verifier;
+  ASSERT_TRUE(Verifier.match(From, cxxRecordDecl(has(fieldDecl();
+  ASSERT_TRUE(Verifier.match(To, cxxRecordDecl(has(fieldDecl();

Tests with `has(fieldDecl())` duplicate tests with `hasFieldOrder()`. Is it 
intentional?



Comment at: unittests/AST/ASTImporterTest.cpp:1171
+  ASSERT_TRUE(Verifier.match(To, cxxRecordDecl(has(fieldDecl();
+  ASSERT_TRUE(Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b"};
+  EXPECT_TRUE(Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b"};

Should this be `match(From, ...)` instead of `To`?



Comment at: unittests/AST/ASTImporterTest.cpp:1177
+ASTImporterTestBase,
+
DISABLED_CXXRecordDeclFieldsShouldBeInCorrectOrderEvenWhenWeImportFirstTheLastDecl)
 {
+  Decl *From, *To;

This identifier is very long. How about renaming it to 
DISABLED_CXXRecordDeclFieldOrderShouldNotDependOnImportOrder? It is 60 char 
long instead of 82.



Comment at: unittests/AST/ASTImporterTest.cpp:1194
+  ASSERT_TRUE(Verifier.match(To, cxxRecordDecl(has(fieldDecl();
+  ASSERT_TRUE(
+  Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b", "c"};

Same as upper.



Comment at: unittests/AST/ASTImporterTest.cpp:1211
+  MatchVerifier Verifier;
+  // matches the implicit decl
+  auto Matcher = classTemplateDecl(has(cxxRecordDecl(has(cxxRecordDecl();

Please make it a complete sentence.



Comment at: unittests/AST/ASTImporterTest.cpp:1258
+  Lang_CXX);
+  Decl* From = LastDeclMatcher{}.match(FromTU, functionDecl());
+  const Decl* To = Import(From, Lang_CXX);

Please stick `*` to the name (below too).


Repository:
  rC Clang

https://reviews.llvm.org/D43967



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

[PATCH] D44555: clang-interpreter example cmake fix

[Luke Cheeseman via Phabricator via cfe-commits]

LukeCheeseman closed this revision.
LukeCheeseman added a comment.

closed by https://reviews.llvm.org/rC328092


Repository:
  rC Clang

https://reviews.llvm.org/D44555



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

[PATCH] D44632: [clang-format] Add a few more Core Graphics identifiers to ObjC heuristic

[Jacek Olesiak via Phabricator via cfe-commits]

jolesiak added inline comments.



Comment at: lib/Format/Format.cpp:1450
 // Keep this array sorted, since we are binary searching over it.
 static constexpr llvm::StringLiteral FoundationIdentifiers[] = {
 "CGFloat",

krasimir wrote:
> jolesiak wrote:
> > benhamilton wrote:
> > > djasper wrote:
> > > > benhamilton wrote:
> > > > > jolesiak wrote:
> > > > > > djasper wrote:
> > > > > > > I have concerns about this growing lists of things. Specifically:
> > > > > > > - Keeping it sorted is a maintenance concern.
> > > > > > > - Doing binary search for basically every identifier in a header 
> > > > > > > seems an unnecessary waste.
> > > > > > > 
> > > > > > > Could we just create a hash set of these?
> > > > > > It was a hash set initially: D42135
> > > > > > Changed in: D42189
> > > > > Happy to do whatever folks recommend; I assume @krasimir's concern in 
> > > > > D42189 was the startup cost of creating the (read-only) hash set.
> > > > > 
> > > > > We can automate keeping this sorted with an `arc lint` check, they're 
> > > > > quite easy to write:
> > > > > 
> > > > > https://secure.phabricator.com/book/phabricator/article/arcanist_lint/
> > > > Krasimir clarified this to me offline. I have no concerns staying with 
> > > > binary search here and for this patch so long as someone builds in an 
> > > > assert that warns us when the strings here are not in the right order 
> > > > at some point.
> > > Good call, added `assert(std::is_sorted(...))`.
> > > 
> > > I tried `static_assert`, but `std::is_sorted()` is not `constexpr` until 
> > > c++2x.
> > Checking this type of constraints in `arc lint` sounds rather weird. I like 
> > this assert as testing private methods is painful.
> I now think a hash set here is better. Sent https://reviews.llvm.org/D44695 
> to replace the array with that.
> 
> Sorry for wasting everybody's time.
Then assertion is no longer needed.


Repository:
  rC Clang

https://reviews.llvm.org/D44632



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

[PATCH] D44673: Make positionToOffset return llvm::Expected

[Ilya Biryukov via Phabricator via cfe-commits]

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

LGTM. See the empty line NITS, though.




Comment at: clangd/ClangdServer.cpp:199
+return End.takeError();
+
+  return formatCode(Code, File, {tooling::Range(*Begin, *End - *Begin)});

NIT: unnecessary empty line



Comment at: clangd/ClangdServer.cpp:216
+return CursorPos.takeError();
+
+  size_t PreviousLBracePos = StringRef(Code).find_last_of('{', *CursorPos);

NIT: unnecessary empty line





Comment at: clangd/SourceCode.cpp:29
+llvm::errc::invalid_argument);
+
   size_t StartOfLine = 0;

NIT: unnecessary empty line





Comment at: clangd/SourceCode.cpp:48
+llvm::errc::invalid_argument);
+
   // FIXME: officially P.character counts UTF-16 code units, not UTF-8 bytes!

NIT: unnecessary empty line





Comment at: unittests/clangd/Annotations.cpp:91
+  llvm::Expected End = positionToOffset(Code, R.end);
+
+  assert(Start);

NIT: unnecessary empty line





Comment at: unittests/clangd/Annotations.cpp:94
+  assert(End);
+
+  return {*Start, *End};

NIT: unnecessary empty line




Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44673



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

[PATCH] D41808: Rename clang link from clang-X.Y to clang-X

[Dimitry Andric via Phabricator via cfe-commits]

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

LGTM.


https://reviews.llvm.org/D41808



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

r328087 - [ASTMatchers] Remove extra qualifier for consistency (LibASTMatchersReference.html)

[Clement Courbet via cfe-commits]

Author: courbet
Date: Wed Mar 21 03:54:29 2018
New Revision: 328087

URL: http://llvm.org/viewvc/llvm-project?rev=328087=rev
Log:
[ASTMatchers] Remove extra qualifier for consistency 
(LibASTMatchersReference.html)

+ Regenerate doc.

Modified:
cfe/trunk/docs/LibASTMatchersReference.html
cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h

Modified: cfe/trunk/docs/LibASTMatchersReference.html
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/LibASTMatchersReference.html?rev=328087=328086=328087=diff
==
--- cfe/trunk/docs/LibASTMatchersReference.html (original)
+++ cfe/trunk/docs/LibASTMatchersReference.html Wed Mar 21 03:54:29 2018
@@ -5152,7 +5152,7 @@ only match the declarations for b, c, an
 
 
 
-Matcherhttp://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprignoringImplicitast_matchers::Matcherhttp://clang.llvm.org/doxygen/classclang_1_1Expr.html;>Expr 
InnerMatcher
+Matcherhttp://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprignoringImplicitMatcherhttp://clang.llvm.org/doxygen/classclang_1_1Expr.html;>Expr 
InnerMatcher
 Matches 
expressions that match InnerMatcher after any implicit AST
 nodes are stripped off.
 

Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h?rev=328087=328086=328087=diff
==
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h (original)
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h Wed Mar 21 03:54:29 2018
@@ -693,7 +693,7 @@ AST_POLYMORPHIC_MATCHER_P(
 ///varDecl(hasInitializer(cxxConstructExpr()))
 /// \endcode
 /// only match the declarations for b and c.
-AST_MATCHER_P(Expr, ignoringImplicit, ast_matchers::internal::Matcher,
+AST_MATCHER_P(Expr, ignoringImplicit, internal::Matcher,
   InnerMatcher) {
   return InnerMatcher.matches(*Node.IgnoreImplicit(), Finder, Builder);
 }


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

[PATCH] D44729: [ASTMatchers] Add hasSubExpr() matcher.

[Clement Courbet via Phabricator via cfe-commits]

courbet created this revision.
courbet added reviewers: aaron.ballman, alexfh.
Herald added a subscriber: klimek.

This is needed to implement more checks in https://reviews.llvm.org/D38455.


Repository:
  rC Clang

https://reviews.llvm.org/D44729

Files:
  docs/LibASTMatchersReference.html
  include/clang/ASTMatchers/ASTMatchers.h
  unittests/ASTMatchers/ASTMatchersTraversalTest.cpp


Index: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -1232,6 +1232,13 @@
 cxxMethodDecl(ofClass(hasName("X"))), true, "-std=gnu++98"));
 }
 
+TEST(HasSubExpr, MatchesSimpleCase) {
+  EXPECT_TRUE(matches("int i = 0.0;",
+  implicitCastExpr(hasSubExpr(floatLiteral();
+  EXPECT_TRUE(notMatches("int i = '0';",
+  implicitCastExpr(hasSubExpr(floatLiteral();
+}
+
 TEST(HasDestinationType, MatchesSimpleCase) {
   EXPECT_TRUE(matches("char* p = static_cast(0);",
   cxxStaticCastExpr(hasDestinationType(
Index: include/clang/ASTMatchers/ASTMatchers.h
===
--- include/clang/ASTMatchers/ASTMatchers.h
+++ include/clang/ASTMatchers/ASTMatchers.h
@@ -4109,6 +4109,21 @@
   return InnerMatcher.matches(Node.getType(), Finder, Builder);
 }
 
+/// \brief Matches casts whose subexpr matches a given matcher.
+///
+/// Example matches the first cast
+/// (matcher = implicitCastExpr(hasSubExpr(floatLiteral())
+///   int i = 0.0;
+///   int j = '0';
+AST_POLYMORPHIC_MATCHER_P(
+hasSubExpr,
+AST_POLYMORPHIC_SUPPORTED_TYPES(CastExpr),
+internal::Matcher, InnerMatcher) {
+  const Expr *const SubExpr = Node.getSubExpr();
+  return (SubExpr != nullptr &&
+  InnerMatcher.matches(*SubExpr, Finder, Builder));
+}
+
 /// \brief Matches RecordDecl object that are spelled with "struct."
 ///
 /// Example matches S, but not C or U.
Index: docs/LibASTMatchersReference.html
===
--- docs/LibASTMatchersReference.html
+++ docs/LibASTMatchersReference.html
@@ -4781,6 +4781,16 @@
 
 
 
+Matcherhttp://clang.llvm.org/doxygen/classclang_1_1CastExpr.html;>CastExprhasSubExprMatcherhttp://clang.llvm.org/doxygen/classclang_1_1Expr.html;>Expr 
InnerMatcher
+Matches casts whose 
subexpr matches a given matcher.
+
+Example matches the first cast
+(matcher = implicitCastExpr(hasSubExpr(floatLiteral())
+  int i = 0.0;
+  int j = '0';
+
+
+
 Matcherhttp://clang.llvm.org/doxygen/classclang_1_1ClassTemplateSpecializationDecl.html;>ClassTemplateSpecializationDeclhasAnyTemplateArgumentMatcherhttp://clang.llvm.org/doxygen/classclang_1_1TemplateArgument.html;>TemplateArgument
 InnerMatcher
 Matches 
classTemplateSpecializations, templateSpecializationType and
 functionDecl that have at least one TemplateArgument matching the given


Index: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -1232,6 +1232,13 @@
 cxxMethodDecl(ofClass(hasName("X"))), true, "-std=gnu++98"));
 }
 
+TEST(HasSubExpr, MatchesSimpleCase) {
+  EXPECT_TRUE(matches("int i = 0.0;",
+  implicitCastExpr(hasSubExpr(floatLiteral();
+  EXPECT_TRUE(notMatches("int i = '0';",
+  implicitCastExpr(hasSubExpr(floatLiteral();
+}
+
 TEST(HasDestinationType, MatchesSimpleCase) {
   EXPECT_TRUE(matches("char* p = static_cast(0);",
   cxxStaticCastExpr(hasDestinationType(
Index: include/clang/ASTMatchers/ASTMatchers.h
===
--- include/clang/ASTMatchers/ASTMatchers.h
+++ include/clang/ASTMatchers/ASTMatchers.h
@@ -4109,6 +4109,21 @@
   return InnerMatcher.matches(Node.getType(), Finder, Builder);
 }
 
+/// \brief Matches casts whose subexpr matches a given matcher.
+///
+/// Example matches the first cast
+/// (matcher = implicitCastExpr(hasSubExpr(floatLiteral())
+///   int i = 0.0;
+///   int j = '0';
+AST_POLYMORPHIC_MATCHER_P(
+hasSubExpr,
+AST_POLYMORPHIC_SUPPORTED_TYPES(CastExpr),
+internal::Matcher, InnerMatcher) {
+  const Expr *const SubExpr = Node.getSubExpr();
+  return (SubExpr != nullptr &&
+  InnerMatcher.matches(*SubExpr, Finder, Builder));
+}
+
 /// \brief Matches RecordDecl object that are spelled with "struct."
 ///
 /// Example matches S, but not C or U.
Index: docs/LibASTMatchersReference.html
===
--- docs/LibASTMatchersReference.html
+++ docs/LibASTMatchersReference.html
@@ -4781,6 +4781,16 @@
 
 
 

r328086 - [ASTMatchers] Regenerate doc.

[Clement Courbet via cfe-commits]

Author: courbet
Date: Wed Mar 21 03:48:00 2018
New Revision: 328086

URL: http://llvm.org/viewvc/llvm-project?rev=328086=rev
Log:
[ASTMatchers] Regenerate doc.

Modified:
cfe/trunk/docs/LibASTMatchersReference.html

Modified: cfe/trunk/docs/LibASTMatchersReference.html
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/LibASTMatchersReference.html?rev=328086=328085=328086=diff
==
--- cfe/trunk/docs/LibASTMatchersReference.html (original)
+++ cfe/trunk/docs/LibASTMatchersReference.html Wed Mar 21 03:48:00 2018
@@ -4353,7 +4353,7 @@ with hasAnyArgument(...)
 
 For ObjectiveC, given
   @interface I - (void) f:(int) y; @end
-  void foo(I *i) { [i f:12] }
+  void foo(I *i) { [i f:12]; }
 objcMessageExpr(hasAnyArgument(integerLiteral(equals(12
   matches [i f:12]
 
@@ -4706,7 +4706,7 @@ with hasAnyArgument(...)
 
 For ObjectiveC, given
   @interface I - (void) f:(int) y; @end
-  void foo(I *i) { [i f:12] }
+  void foo(I *i) { [i f:12]; }
 objcMessageExpr(hasAnyArgument(integerLiteral(equals(12
   matches [i f:12]
 
@@ -5659,7 +5659,7 @@ with hasAnyArgument(...)
 
 For ObjectiveC, given
   @interface I - (void) f:(int) y; @end
-  void foo(I *i) { [i f:12] }
+  void foo(I *i) { [i f:12]; }
 objcMessageExpr(hasAnyArgument(integerLiteral(equals(12
   matches [i f:12]
 


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

[PATCH] D44559: [Sema] Wrong width of result of mul operation

[Andrew V. Tischenko via Phabricator via cfe-commits]

avt77 updated this revision to Diff 139271.
avt77 added a comment.

I removed the dependence on arch and updated the tests.


https://reviews.llvm.org/D44559

Files:
  lib/Sema/SemaChecking.cpp
  test/Sema/conversion.c
  test/SemaCXX/conversion.cpp


Index: test/SemaCXX/conversion.cpp
===
--- test/SemaCXX/conversion.cpp
+++ test/SemaCXX/conversion.cpp
@@ -298,3 +298,15 @@
   conditional_run_13(NULL);
 }
 }
+
+template
+T mul_t(T a, T b) {
+  int c = a * b;
+  T d1 = c; // expected-warning{{implicit conversion loses integer precision: 
'int' to 'char'}}
+  T d2 = a * b; // expected-warning{{implicit conversion loses integer 
precision: 'int' to 'char'}}
+  return d1 + d2;
+}
+
+char mul_t_test (char a, char b) {
+return mul_t(a, b); // expected-note{{in instantiation of function 
template specialization 'mul_t' requested here}}
+}
Index: test/Sema/conversion.c
===
--- test/Sema/conversion.c
+++ test/Sema/conversion.c
@@ -302,7 +302,7 @@
 }
 
 void test15(char c) {
-  c = c + 1 + c * 2;
+  c = c + 1 + c * 2; // expected-warning {{implicit conversion loses integer 
precision: 'int'}}
   c = (short) c + 1 + c * 2; // expected-warning {{implicit conversion loses 
integer precision}}
 }
 
@@ -448,3 +448,11 @@
   b -= a; // expected-warning {{implicit conversion when assigning computation 
result loses floating-point precision: 'double' to 'float'}}
   return b;
 }
+
+unsigned short foo1(unsigned char a) {
+  return a * a; // expected-warning {{implicit conversion loses integer 
precision: 'int' to 'unsigned short'}}
+}
+
+signed short bar1(signed char a) {
+  return a * a; // expected-warning {{implicit conversion loses integer 
precision: 'int' to 'short'}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -8536,8 +8536,13 @@
   return meet;
 }
 
+case BO_Mul: {
+  // The result width should be calculated in advance
+  unsigned opWidth = C.getIntWidth(GetExprType(E));
+  return IntRange(opWidth, false);
+}
+
 // The default behavior is okay for these.
-case BO_Mul:
 case BO_Add:
 case BO_Xor:
 case BO_Or:


Index: test/SemaCXX/conversion.cpp
===
--- test/SemaCXX/conversion.cpp
+++ test/SemaCXX/conversion.cpp
@@ -298,3 +298,15 @@
   conditional_run_13(NULL);
 }
 }
+
+template
+T mul_t(T a, T b) {
+  int c = a * b;
+  T d1 = c; // expected-warning{{implicit conversion loses integer precision: 'int' to 'char'}}
+  T d2 = a * b; // expected-warning{{implicit conversion loses integer precision: 'int' to 'char'}}
+  return d1 + d2;
+}
+
+char mul_t_test (char a, char b) {
+return mul_t(a, b); // expected-note{{in instantiation of function template specialization 'mul_t' requested here}}
+}
Index: test/Sema/conversion.c
===
--- test/Sema/conversion.c
+++ test/Sema/conversion.c
@@ -302,7 +302,7 @@
 }
 
 void test15(char c) {
-  c = c + 1 + c * 2;
+  c = c + 1 + c * 2; // expected-warning {{implicit conversion loses integer precision: 'int'}}
   c = (short) c + 1 + c * 2; // expected-warning {{implicit conversion loses integer precision}}
 }
 
@@ -448,3 +448,11 @@
   b -= a; // expected-warning {{implicit conversion when assigning computation result loses floating-point precision: 'double' to 'float'}}
   return b;
 }
+
+unsigned short foo1(unsigned char a) {
+  return a * a; // expected-warning {{implicit conversion loses integer precision: 'int' to 'unsigned short'}}
+}
+
+signed short bar1(signed char a) {
+  return a * a; // expected-warning {{implicit conversion loses integer precision: 'int' to 'short'}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -8536,8 +8536,13 @@
   return meet;
 }
 
+case BO_Mul: {
+  // The result width should be calculated in advance
+  unsigned opWidth = C.getIntWidth(GetExprType(E));
+  return IntRange(opWidth, false);
+}
+
 // The default behavior is okay for these.
-case BO_Mul:
 case BO_Add:
 case BO_Xor:
 case BO_Or:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet added inline comments.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:26
+  binaryOperator(
+  anyOf(hasOperatorName("+="), hasOperatorName("-=")),
+  hasLHS(hasType(isInteger())),

aaron.ballman wrote:
> courbet wrote:
> > aaron.ballman wrote:
> > > Why only these two operators? This does not match the behavior from the 
> > > C++ Core Guideline check itself 
> > > (https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-narrowing).
> > These provided the best signal to noise ratio. Also they are the most 
> > dangerous (in a loop, you might end up losing one unit per iteration). I'll 
> > add other operators later if that's fine with you.
> If the check doesn't cover anything listed in the C++ Core Guideline examples 
> or enforcement wording, I have a hard time accepting it as the initial commit 
> for such a check.
> 
> Of special interest to me is the request from the C++ Core Guideline authors 
> themselves: 
> ```
>   - flag all floating-point to integer conversions (maybe only float->char 
> and double->int. Here be dragons! we need data)
>   - flag all long->char (I suspect int->char is very common. Here be dragons! 
> we need data)
> ```
> I think we need data as well -- writing the check and then testing it over 
> several million lines of source could give us some of that data, which we can 
> then feed back to the guideline authors, so that we come up with a check 
> that's realistic.
Quoting the guideline:
"A good analyzer can detect all narrowing conversions. However, flagging all 
narrowing conversions will lead to a lot of false positives." Then follows a 
list of suggestions. While +=/-= are not in the list of suggestions, they are a 
subset of "flag all floating-point to integer conversions" that I've found to 
be useful (very low rate of false positives) by running on our codebase.
I agree that more data would be useful, so I'll do an analysis of flagging all 
(non-ceil/floor float/double)->integral conversions.






Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



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

Re: [clang-tools-extra] r328069 - Revert "[lit] Adding config initialization to lit tests in clang-tools-extra"

[Roman Lebedev via cfe-commits]

On Wed, Mar 21, 2018 at 5:28 AM, Petr Hosek via cfe-commits
 wrote:
> Author: phosek
> Date: Tue Mar 20 19:28:22 2018
> New Revision: 328069
>
> URL: http://llvm.org/viewvc/llvm-project?rev=328069=rev
> Log:
> Revert "[lit] Adding config initialization to lit tests in clang-tools-extra"
>
> This reverts commit r328060 because a test that was inteded to run on
> Windows but never ran before due to the missing config initialization
> is now being executed and is failing.
Only one test?
Would it be possible to just completely disable that test for now?
Since it has never run, it's not like it is crucial for the CI...

This revision is the only thing that is blocking https://reviews.llvm.org/D41102

> Modified:
> clang-tools-extra/trunk/test/lit.site.cfg.in
>
> Modified: clang-tools-extra/trunk/test/lit.site.cfg.in
> URL: 
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/lit.site.cfg.in?rev=328069=328068=328069=diff
> ==
> --- clang-tools-extra/trunk/test/lit.site.cfg.in (original)
> +++ clang-tools-extra/trunk/test/lit.site.cfg.in Tue Mar 20 19:28:22 2018
> @@ -23,7 +23,5 @@ except KeyError:
>  key, = e.args
>  lit_config.fatal("unable to find %r parameter, use '--param=%s=VALUE'" % 
> (key,key))
>
> -@LIT_SITE_CFG_IN_FOOTER@
> -
>  # Let the main config do the real work.
>  lit_config.load_config(config, "@CLANG_TOOLS_SOURCE_DIR@/test/lit.cfg")

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

[PATCH] D41537: Optionally add code completion results for arrow instead of dot

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added inline comments.



Comment at: include/clang/Sema/CodeCompleteConsumer.h:565
+  /// \brief For this completion result correction is required.
+  Optional Corr = None;
+

ilya-biryukov wrote:
> Having a string replacement without an actual range to replace still moves a 
> lot of responsibility onto the clients. We should probably model corrections 
> after the fix-its for diagnostics:
> - replacements need to provide a range to be replaced, alongside with a text 
> for the replacement,
> - we should provide a list of edits to allow corrections that touch two 
> separate pieces of code.
> 
> For the fix-its in the diagnostics, see 
> [[https://reviews.llvm.org/source/clang/browse/cfe/trunk/tools/libclang/CXLoadedDiagnostic.h;327861$84
>  | CXLoadedDiagnostic.h]] for an interface exported via libclang and 
> [[https://reviews.llvm.org/source/clang/browse/cfe/trunk/include/clang/Basic/Diagnostic.h;327861$947|Diagnostic.h]]
>  for an interface exported in C++ API.
> WDYT?
I thought that fixits always exist separately from diagnostics. I will check 
this out, thanks.


https://reviews.llvm.org/D41537



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

[PATCH] D44426: Fix llvm + clang build with Intel compiler

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added a comment.

In https://reviews.llvm.org/D44426#1042162, @mibintc wrote:

> I added some inline comments. You are using the Intel 18.0 compiler on 
> Windows - what version of Visual Studio is in the environment?


Yes, I'm using 18.0




Comment at: include/llvm-c/Target.h:25
 
-#if defined(_MSC_VER) && !defined(inline)
+#if defined(_MSC_VER) && !defined(inline) && !defined(__INTEL_COMPILER)
 #define inline __inline

mibintc wrote:
> I really think all the Intel-compiler bug workarounds should be version 
> specific. For example, in the boost.org customizations we have checks like 
> this:
> #if defined(__INTEL_COMPILER) && (__INTEL_COMPILER >= 1500)
> 
> I believe you are using the most recent Intel compiler, 18.0, which has 
> version 1800.  Let's assume the bug will either be fixed in our 18.0 compiler 
> or in our next compiler which would be numbered 1900, so the check could be 
> like this:
> #if defined(__INTEL_COMPILER) && (__INTEL_COMPILER < 1900) // employ the 
> workaround for the Intel 18.0 compiler and older
> 
> This way the workaround will "disappear" when the bug gets fixed.
> 
> For that matter I wonder if it's still necessary to use the workaround for 
> the Microsoft compiler?
> 
> Thanks for reporting the bug into the Intel forum. If we put a bug workaround 
> into LLVM it would be very nice to have the bug reported to the offending 
> compiler. 
> 
This one will probably not be fixed because I was answered that it's the 
predicted behavior and that I need not to "#define inline __inline" if I use 
intel compiler
I agree though about versions which need to be taken into account



Comment at: include/llvm/ADT/BitmaskEnum.h:73
 
+#ifdef __INTEL_COMPILER
+template 

mibintc wrote:
> what problem is being worked around here? I checked your post into the Intel 
> compiler forum "enum members are not visible to template specialization" and 
> I can observe the bug on our Windows compiler but not our Linux compiler. 
it was recently accepted. the issues is that E::LLVM_BITMASK_LARGEST_ENUMERATOR 
from line 80 here is always invalid with intel compiler and the whole set of 
operator overloads is ignored
at least on Windows :)



Comment at: include/llvm/Analysis/AliasAnalysis.h:254
   FMRB_OnlyAccessesInaccessibleOrArgMem = FMRL_InaccessibleMem |
-  FMRL_ArgumentPointees |
+  
static_cast(FMRL_ArgumentPointees) |
   static_cast(ModRefInfo::ModRef),

mibintc wrote:
> is this a necessary workaround for the Intel compiler? It's not pretty.  
> Suggest we add the #if around it? Long term i hope this would not be 
> necessary.
i'm not sure that adding #if around makes it easier to understand :)
of course I can add it everywhere I append enums with static_cast



Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:6131
 
+#ifndef __INTEL_COMPILER
 static_assert(((~(SIInstrFlags::S_NAN |

mibintc wrote:
> this assesrts with Intel compiler?  Or the expression needs to be rewritten 
> with static_cast as above?
"needs to be rewritten with static_cast" - correct. it can be done but I was a 
bit lazy and statioc_assert does not affect the outcome of build :)


https://reviews.llvm.org/D44426



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

[PATCH] D44559: [Sema] Wrong width of result of mul operation

[Andrew V. Tischenko via Phabricator via cfe-commits]

avt77 added a comment.

>> In https://reviews.llvm.org/D44559#1040799, @rjmccall wrote:
>> 
>>> I think we're correct not to warn here and that GCC/ICC are being noisy.  
>>> The existence of a temporary promotion to a wider type doesn't justify 
>>> warning on arithmetic between two operands that are the same size as the 
>>> ultimate result.  It is totally fair for users to think of this operation 
>>> as being "closed" on the original type.
>> 
>> 
>> Could you please clarify, are you saying that PR35409 
>>  is not a bug, and clang should 
>> continue to not warn in those cases?
> 
> Correct.

Does it mean we should abandon this revision? On the other hand it's a real 
bug, isn't it?


Repository:
  rC Clang

https://reviews.llvm.org/D44559



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

[PATCH] D44720: [clangd] Simplify fuzzy matcher (sequence alignment) by removing some condition checks.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

(Sorry if I sound gruff - I'm sure there are improvements to be had here. But 
since the code is a bit dense (my fault) I have trouble inferring them from the 
deltas.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44720



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

[PATCH] D44720: [clangd] Simplify fuzzy matcher (sequence alignment) by removing some condition checks.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Can you elaborate on what this patch is improving, and how?
There are some stylistic changes, and also what look like subtle logic changes, 
and some rearrangement of the algorithm - to what end?

Canonical way to run all tests is `ninja check-clang-tools`, the way you 
suggested is the right thing for rapid iteration with unit tests.
Personally, I don't use a debugger.




Comment at: clangd/FuzzyMatch.cpp:93
+  for (int I = PatN; I <= WordN; I++)
+Best = std::max(Best, Scores[PatN][I][Match].Score);
   if (isAwful(Best))

this looks like a behavior change - why?



Comment at: clangd/FuzzyMatch.cpp:241
 
-  auto MatchMissScore = PreMiss[Match].Score;
-  auto MissMissScore = PreMiss[Miss].Score;
-  if (P < PatN - 1) { // Skipping trailing characters is always free.
-MatchMissScore -= skipPenalty(W, Match);
-MissMissScore -= skipPenalty(W, Miss);
-  }
+  auto MatchMissScore = PreMiss[Match].Score + missScore(W, Match);
+  auto MissMissScore = PreMiss[Miss].Score + missScore(W, Miss);

adding the penalty unconditionally seems like a behavior change, why?



Comment at: clangd/FuzzyMatch.h:61
   bool allowMatch(int P, int W) const;
-  int skipPenalty(int W, Action Last) const;
-  int matchBonus(int P, int W, Action Last) const;
+  int missScore(int W, Action Last) const;
+  int matchScore(int P, int W, Action Last) const;

FWIW, I don't think this is an improvement - I think the clarity of purpose in 
names is more useful than having consistent signs in this case.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44720



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

[PATCH] D44189: [RISCV] Verify the input value of -march=

[Kito Cheng via Phabricator via cfe-commits]

kito-cheng marked 4 inline comments as done.
kito-cheng added a comment.

Hi Alex:

Thanks for your input, check for repeated letter was missed in my last patch :)




Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:34
+
+// The canonical order specified in ISA manual.
+StringRef StdExts = "mafdc";

asb wrote:
> I'd reference Table 22.1 in RISC-V User-Level ISA V2.2 for anyone who wants 
> to verify this.
Done.



Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:37-38
+
+bool hasF = false, hasD = false;
+char baseline = MArch[4];
+

asb wrote:
> Should be HasF, HasD, and Baseline to conform to standard LLVM naming 
> conventions.
Fixed.



Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:65
+for (char c : Exts) {
+  // Check march is satisfied the canonical order.
+  while (StdExtsItr != StdExts.end() && *StdExtsItr != c)

asb wrote:
> I'd phrase this as "Check ISA extensions are specified in the canonical 
> order."
Done.



Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:99
+
+// Dependency check
+if (hasD && !hasF)

asb wrote:
> I'd be tempted to give a bit more explanation a bit more "It's illegal to 
> specify the 'd' (double-precision floating point) extension without also 
> specifying the 'f' (single precision floating-point) extension".
Done.


Repository:
  rC Clang

https://reviews.llvm.org/D44189



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

[PATCH] D44189: [RISCV] Verify the input value of -march=

[Kito Cheng via Phabricator via cfe-commits]

kito-cheng updated this revision to Diff 139256.
kito-cheng added a comment.

Update revision according Alex's review.

Changes:

- Add testcase for uppercase of -march string.
- Add testcase for repeated letter in -march.
- Add more comment.
- Add several TODO item for diagnostic message improvement.
- Fix coding style issue.


Repository:
  rC Clang

https://reviews.llvm.org/D44189

Files:
  lib/Driver/ToolChains/Arch/RISCV.cpp
  test/Driver/riscv-arch.c

Index: test/Driver/riscv-arch.c
===
--- /dev/null
+++ test/Driver/riscv-arch.c
@@ -0,0 +1,89 @@
+// RUN: %clang -target riscv32-unknown-elf -march=rv32i -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32im -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32ima -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32imaf -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32imafd -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv32ic -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32imc -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32imac -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32imafc -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32imafdc -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv32ia -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32iaf -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32iafd -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv32iac -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32iafc -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32iafdc -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv32g -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-unknown-elf -march=rv32gc -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// RUN: %clang -target riscv64-unknown-elf -march=rv64i -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64im -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64ima -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64imaf -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64imafd -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// RUN: %clang -target riscv64-unknown-elf -march=rv64ic -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64imc -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64imac -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64imafc -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64imafdc -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// RUN: %clang -target riscv64-unknown-elf -march=rv64ia -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64iaf -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64iafd -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// RUN: %clang -target riscv64-unknown-elf -march=rv64iac -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64iafc -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64iafdc -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// RUN: %clang -target riscv64-unknown-elf -march=rv64g -### %s -fsyntax-only 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64gc -### %s -fsyntax-only 2>&1 | FileCheck %s
+
+// CHECK-NOT: error: invalid arch name '
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv32 -### %s -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32 %s
+// RV32: error: invalid arch name 'rv32'
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv32m -### %s -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32M %s
+// RV32M: error: invalid arch name 'rv32m'
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv32id -### %s -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32ID %s
+// RV32ID: error: invalid arch name 'rv32id'
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv32l -### %s -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32L %s
+// RV32L: error: invalid arch name 'rv32l'
+
+// 

[PATCH] D44724: [Fuchsia] Don't install libc++, libc++abi or libunwind on Darwin

[Roland McGrath via Phabricator via cfe-commits]

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

Lgtm with some more comments


Repository:
  rC Clang

https://reviews.llvm.org/D44724



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