date:20201026

[PATCH] D90127: [clang][NFC] Rearrange Comment Token and Lexer fields to reduce padding

2020-10-26 Thread Dmitri Gribenko via Phabricator via cfe-commits

gribozavr2 added inline comments.



Comment at: clang/include/clang/AST/CommentLexer.h:244
+  /// command, including command marker.
+  SmallString<16> VerbatimBlockEndCommandName;
+

I'm not a fan of this change to `Lexer` because it breaks the grouping of 
fields: `VerbatimBlockEndCommandName` is no longer next to `State`.

There is only ever one `Lexer` class instance anyway, so any memory savings are 
not important I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90127

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

[PATCH] D90014: [clangd] Add support for multiple DecisionForest model experiments.

2020-10-26 Thread Utkarsh Saxena via Phabricator via cfe-commits

usaxena95 updated this revision to Diff 300582.
usaxena95 added a comment.

Added tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90014

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

Index: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
===
--- clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -194,6 +194,33 @@
   ElementsAre(Named("clangA"), Named("clangD")));
 }
 
+TEST(DecisionForestRankingModel, DecisionForestScorerCallbackTest) {
+  clangd::CodeCompleteOptions Opts;
+  constexpr float MagicNumber = 1234.5678;
+  Opts.RankingModel = CodeCompleteOptions::DecisionForest;
+  Opts.DecisionForestScorer = [](const SymbolQualitySignals &,
+ const SymbolRelevanceSignals &, float Base) {
+DecisionForestScores Scores;
+Scores.Total = MagicNumber;
+Scores.ExcludingName = MagicNumber;
+return Scores;
+  };
+  llvm::StringRef Code = "int func() { int xyz; xy^ }";
+  auto Results = completions(Code,
+ /*IndexSymbols=*/{}, Opts);
+  ASSERT_EQ(Results.Completions.size(), 1u);
+  EXPECT_EQ(Results.Completions[0].Score.Total, MagicNumber);
+  EXPECT_EQ(Results.Completions[0].Score.ExcludingName, MagicNumber);
+
+  // Do not use DecisionForestScorer for heuristics model.
+  Opts.RankingModel = CodeCompleteOptions::Heuristics;
+  Results = completions(Code,
+/*IndexSymbols=*/{}, Opts);
+  ASSERT_EQ(Results.Completions.size(), 1u);
+  EXPECT_NE(Results.Completions[0].Score.Total, MagicNumber);
+  EXPECT_NE(Results.Completions[0].Score.ExcludingName, MagicNumber);
+}
+
 TEST(CompletionTest, Limit) {
   clangd::CodeCompleteOptions Opts;
   Opts.Limit = 2;
Index: clang-tools-extra/clangd/Quality.h
===
--- clang-tools-extra/clangd/Quality.h
+++ clang-tools-extra/clangd/Quality.h
@@ -165,8 +165,18 @@
 /// Combine symbol quality and relevance into a single score.
 float evaluateSymbolAndRelevance(float SymbolQuality, float SymbolRelevance);
 
-float evaluateDecisionForest(const SymbolQualitySignals &Quality,
- const SymbolRelevanceSignals &Relevance);
+/// Same semantics as CodeComplete::Score. Quality score and Relevance score
+/// have been removed since DecisionForest cannot assign individual scores to
+/// Quality and Relevance signals.
+struct DecisionForestScores {
+  float Total = 0.f;
+  float ExcludingName = 0.f;
+};
+
+DecisionForestScores
+evaluateDecisionForest(const SymbolQualitySignals &Quality,
+   const SymbolRelevanceSignals &Relevance, float Base);
+
 /// TopN is a lossy container that preserves only the "best" N elements.
 template > class TopN {
 public:
Index: clang-tools-extra/clangd/Quality.cpp
===
--- clang-tools-extra/clangd/Quality.cpp
+++ clang-tools-extra/clangd/Quality.cpp
@@ -487,8 +487,9 @@
   return SymbolQuality * SymbolRelevance;
 }
 
-float evaluateDecisionForest(const SymbolQualitySignals &Quality,
- const SymbolRelevanceSignals &Relevance) {
+DecisionForestScores
+evaluateDecisionForest(const SymbolQualitySignals &Quality,
+   const SymbolRelevanceSignals &Relevance, float Base) {
   Example E;
   E.setIsDeprecated(Quality.Deprecated);
   E.setIsReservedName(Quality.ReservedName);
@@ -512,7 +513,19 @@
   E.setHadSymbolType(Relevance.HadSymbolType);
   E.setTypeMatchesPreferred(Relevance.TypeMatchesPreferred);
   E.setFilterLength(Relevance.FilterLength);
-  return Evaluate(E);
+
+  DecisionForestScores Scores;
+  // Exponentiating DecisionForest prediction makes the score of each tree a
+  // multiplciative boost (like NameMatch). This allows us to weigh the
+  // prediciton score and NameMatch appropriately.
+  Scores.ExcludingName = pow(Base, Evaluate(E));
+  // NeedsFixIts is not part of the DecisionForest as generating training
+  // data that needs fixits is not-feasible.
+  if (Relevance.NeedsFixIts)
+Scores.ExcludingName *= 0.5;
+  // NameMatch should be a multiplier on total score to support rescoring.
+  Scores.Total = Relevance.NameMatch * Scores.ExcludingName;
+  return Scores;
 }
 
 // Produces an integer that sorts in the same order as F.
Index: clang-tools-extra/clangd/CodeComplete.h
===
--- clang-tools-extra/clangd/CodeComplete.h
+++ clang-tools-extra/clangd/CodeComplete.h
@@ -154,14 +154,21 @@
 DecisionForest,
   } RankingModel = Heuristics;
 
+  /// Callback used to score

[PATCH] D88790: [libTooling] Recognize sccache as a compiler wrapper in compilation database commands

2020-10-26 Thread Nathan Ridge via Phabricator via cfe-commits

nridge added a comment.

Review ping :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88790

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

[PATCH] D90134: [clangd] Increase the TooMany limit for index-based textual navigation to 5

2020-10-26 Thread Nathan Ridge via Phabricator via cfe-commits

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

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90134

Files:
  clang-tools-extra/clangd/XRefs.cpp


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -488,8 +488,8 @@
   Located.Definition = *MaybeDefLoc;
 }
 
-if (ScoredResults.size() >= 3) {
-  // If we have more than 3 results, don't return anything,
+if (ScoredResults.size() >= 5) {
+  // If we have more than 5 results, don't return anything,
   // as confidence is too low.
   // FIXME: Alternatively, try a stricter query?
   TooMany = true;


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -488,8 +488,8 @@
   Located.Definition = *MaybeDefLoc;
 }
 
-if (ScoredResults.size() >= 3) {
-  // If we have more than 3 results, don't return anything,
+if (ScoredResults.size() >= 5) {
+  // If we have more than 5 results, don't return anything,
   // as confidence is too low.
   // FIXME: Alternatively, try a stricter query?
   TooMany = true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90134: [clangd] Increase the TooMany limit for index-based textual navigation to 5

2020-10-26 Thread Nathan Ridge via Phabricator via cfe-commits

nridge added a comment.

My experience using this feature in real codebases has been that we take this 
early-exit too often; the limit of 3 is fairly easy to hit due to things like 
layers of wrappers that repeat a function name. Bumping the limit to 5 has 
increased the usefulness for me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90134

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

[PATCH] D78784: [clangd] Add some logging to explain why textual fallback navigation failed

2020-10-26 Thread Nathan Ridge via Phabricator via cfe-commits

nridge abandoned this revision.
nridge added a comment.

Abandoning as per discussion. Running this locally has led me to propose D90134 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78784

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

[PATCH] D90133: clang-format: Allow fallback-style to take formatting arguments

2020-10-26 Thread Joel Grunbaum via Phabricator via cfe-commits

Chizi123 updated this revision to Diff 300586.
Chizi123 added a comment.

Accidentally added patch to commit


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

https://reviews.llvm.org/D90133

Files:
  clang/lib/Format/Format.cpp


Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -2844,8 +2844,15 @@
   FormatStyle Style = getLLVMStyle(guessLanguage(FileName, Code));
 
   FormatStyle FallbackStyle = getNoStyle();
-  if (!getPredefinedStyle(FallbackStyleName, Style.Language, &FallbackStyle))
-return make_string_error("Invalid fallback style \"" + FallbackStyleName);
+  if (FallbackStyleName.startswith("{")) {
+if (std::error_code ec = parseConfiguration(
+FallbackStyleName, &FallbackStyle, AllowUnknownOptions))
+  return make_string_error("Error parsing -fallback-style: " +
+   ec.message());
+  } else if (!getPredefinedStyle(FallbackStyleName, Style.Language,
+ &FallbackStyle)) {
+return make_string_error("Invalid fallback style \"" + FallbackStyleName + 
"\"");
+  }
 
   if (StyleName.startswith("{")) {
 // Parse YAML/JSON style from the command line.


Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -2844,8 +2844,15 @@
   FormatStyle Style = getLLVMStyle(guessLanguage(FileName, Code));
 
   FormatStyle FallbackStyle = getNoStyle();
-  if (!getPredefinedStyle(FallbackStyleName, Style.Language, &FallbackStyle))
-return make_string_error("Invalid fallback style \"" + FallbackStyleName);
+  if (FallbackStyleName.startswith("{")) {
+if (std::error_code ec = parseConfiguration(
+FallbackStyleName, &FallbackStyle, AllowUnknownOptions))
+  return make_string_error("Error parsing -fallback-style: " +
+   ec.message());
+  } else if (!getPredefinedStyle(FallbackStyleName, Style.Language,
+ &FallbackStyle)) {
+return make_string_error("Invalid fallback style \"" + FallbackStyleName + "\"");
+  }
 
   if (StyleName.startswith("{")) {
 // Parse YAML/JSON style from the command line.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90130: Add mips64 support in lib/tsan/go/buildgo.sh

2020-10-26 Thread Meng Zhuo via Phabricator via cfe-commits

mzh updated this revision to Diff 300587.
Herald added subscribers: llvm-commits, cfe-commits, hiraditya.
Herald added projects: clang, LLVM.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90130

Files:
  clang/test/CodeGen/X86/att-inline-asm-prefix.c
  llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
  llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h
  llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp

Index: llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp
===
--- llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp
+++ llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp
@@ -346,6 +346,21 @@
 O << "\trepne\t";
   else if (Flags & X86::IP_HAS_REPEAT)
 O << "\trep\t";
+
+  // These all require a pseudo prefix
+  if (Flags & X86::IP_USE_VEX)
+O << "\t{vex}";
+  else if (Flags & X86::IP_USE_VEX2)
+O << "\t{vex2}";
+  else if (Flags & X86::IP_USE_VEX3)
+O << "\t{vex3}";
+  else if (Flags & X86::IP_USE_EVEX)
+O << "\t{evex}";
+
+  if (Flags & X86::IP_USE_DISP8)
+O << "\t{disp8}";
+  else if (Flags & X86::IP_USE_DISP32)
+O << "\t{disp32}";
 }
 
 void X86InstPrinterCommon::printVKPair(const MCInst *MI, unsigned OpNo,
Index: llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h
===
--- llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h
+++ llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h
@@ -55,15 +55,18 @@
   /// The constants to describe instr prefixes if there are
   enum IPREFIXES {
 IP_NO_PREFIX = 0,
-IP_HAS_OP_SIZE = 1,
-IP_HAS_AD_SIZE = 2,
-IP_HAS_REPEAT_NE = 4,
-IP_HAS_REPEAT = 8,
-IP_HAS_LOCK = 16,
-IP_HAS_NOTRACK = 32,
-IP_USE_VEX3 = 64,
-IP_USE_DISP8 = 128,
-IP_USE_DISP32 = 256,
+IP_HAS_OP_SIZE =   1U << 0,
+IP_HAS_AD_SIZE =   1U << 1,
+IP_HAS_REPEAT_NE = 1U << 2,
+IP_HAS_REPEAT =1U << 3,
+IP_HAS_LOCK =  1U << 4,
+IP_HAS_NOTRACK =   1U << 5,
+IP_USE_VEX =   1U << 6,
+IP_USE_VEX2 =  1U << 7,
+IP_USE_VEX3 =  1U << 8,
+IP_USE_EVEX =  1U << 9,
+IP_USE_DISP8 = 1U << 10,
+IP_USE_DISP32 =1U << 11,
   };
 
   enum OperandType : unsigned {
Index: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
===
--- llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
+++ llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
@@ -83,6 +83,7 @@
   enum VEXEncoding {
 VEXEncoding_Default,
 VEXEncoding_VEX,
+VEXEncoding_VEX2,
 VEXEncoding_VEX3,
 VEXEncoding_EVEX,
   };
@@ -2818,8 +2819,10 @@
 return Error(Parser.getTok().getLoc(), "Expected '}'");
   Parser.Lex(); // Eat curly.
 
-  if (Prefix == "vex" || Prefix == "vex2")
+  if (Prefix == "vex")
 ForcedVEXEncoding = VEXEncoding_VEX;
+  else if (Prefix == "vex2")
+ForcedVEXEncoding = VEXEncoding_VEX2;
   else if (Prefix == "vex3")
 ForcedVEXEncoding = VEXEncoding_VEX3;
   else if (Prefix == "evex")
@@ -3837,6 +3840,7 @@
 return Match_Unsupported;
 
   if ((ForcedVEXEncoding == VEXEncoding_VEX ||
+   ForcedVEXEncoding == VEXEncoding_VEX2 ||
ForcedVEXEncoding == VEXEncoding_VEX3) &&
   (MCID.TSFlags & X86II::EncodingMask) != X86II::VEX)
 return Match_Unsupported;
@@ -3879,10 +3883,16 @@
 
   MCInst Inst;
 
-  // If VEX3 encoding is forced, we need to pass the USE_VEX3 flag to the
-  // encoder.
-  if (ForcedVEXEncoding == VEXEncoding_VEX3)
+  // If VEX/EVEX encoding is forced, we need to pass the USE_* flag to the
+  // encoder and printer.
+  if (ForcedVEXEncoding == VEXEncoding_VEX)
+Prefixes |= X86::IP_USE_VEX;
+  else if (ForcedVEXEncoding == VEXEncoding_VEX2)
+Prefixes |= X86::IP_USE_VEX2;
+  else if (ForcedVEXEncoding == VEXEncoding_VEX3)
 Prefixes |= X86::IP_USE_VEX3;
+  else if (ForcedVEXEncoding == VEXEncoding_EVEX)
+Prefixes |= X86::IP_USE_EVEX;
 
   // Set encoded flags for {disp8} and {disp32}.
   if (ForcedDispEncoding == DispEncoding_Disp8)
Index: clang/test/CodeGen/X86/att-inline-asm-prefix.c
===
--- /dev/null
+++ clang/test/CodeGen/X86/att-inline-asm-prefix.c
@@ -0,0 +1,25 @@
+// RUN:%clang_cc1 %s -ferror-limit 0 -triple=x86_64-pc -target-feature +avx512f -target-feature +avx2 -target-feature +avx512vl -S -o -  | FileCheck %s -check-prefix CHECK
+
+// This test is to check if the prefix in inline assembly is correctly
+// preserved.
+
+void check_inline_prefix(void) {
+  __asm__ (
+// CHECK: vcvtps2pd %xmm0, %xmm1
+// CHECK: {vex} vcvtps2pd %xmm0, %xmm1
+// CHECK: {vex2} vcvtps2pd %xmm0, %xmm1
+// CHECK: {vex3} vcvtps2pd %xmm0, %xmm1
+// CHECK: {evex} vcvtps2pd %xmm0, %xmm1
+// CHECK: movl $1, (%rax)
+// CHECK: {disp8}  movl $1, (%rax)
+// CHECK: {disp32} movl $1, (%rax)

[PATCH] D90130: Add mips64 support in lib/tsan/go/buildgo.sh

2020-10-26 Thread Meng Zhuo via Phabricator via cfe-commits

mzh marked 2 inline comments as done.
mzh added a comment.

Thanks @dvyukov

All done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90130

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

[PATCH] D90130: Add mips64 support in lib/tsan/go/buildgo.sh

2020-10-26 Thread Meng Zhuo via Phabricator via cfe-commits

mzh updated this revision to Diff 300588.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90130

Files:
  compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
  compiler-rt/lib/sanitizer_common/sanitizer_platform.h
  compiler-rt/lib/tsan/go/buildgo.sh
  compiler-rt/lib/tsan/rtl/tsan_platform.h

Index: compiler-rt/lib/tsan/rtl/tsan_platform.h
===
--- compiler-rt/lib/tsan/rtl/tsan_platform.h
+++ compiler-rt/lib/tsan/rtl/tsan_platform.h
@@ -488,6 +488,30 @@
 // Indicates the runtime will define the memory regions at runtime.
 #define TSAN_RUNTIME_VMA 1
 
+#elif SANITIZER_GO && defined(__mips64)
+/*
+Go on linux/mips64 (47-bit VMA)
+  1000 -  1000 : executable
+ 1000  - 00c0  : -
+00c0   - 00e0  : heap
+00e0   - 2000  : -
+2000   - 3000  : shadow
+3000   - 3000  : -
+3000   - 4000  : metainfo (memory blocks and sync objects)
+4000   - 6000  : -
+6000   - 6200  : traces
+6200   - 8000  : -
+*/
+struct Mapping {
+  static const uptr kMetaShadowBeg = 0x3000ull;
+  static const uptr kMetaShadowEnd = 0x4000ull;
+  static const uptr kTraceMemBeg = 0x6000ull;
+  static const uptr kTraceMemEnd = 0x6200ull;
+  static const uptr kShadowBeg = 0x2000ull;
+  static const uptr kShadowEnd = 0x3000ull;
+  static const uptr kAppMemBeg = 0x1000ull;
+  static const uptr kAppMemEnd = 0x00e0ull;
+};
 #else
 # error "Unknown platform"
 #endif
Index: compiler-rt/lib/tsan/go/buildgo.sh
===
--- compiler-rt/lib/tsan/go/buildgo.sh
+++ compiler-rt/lib/tsan/go/buildgo.sh
@@ -64,6 +64,14 @@
 	elif [ "`uname -a | grep aarch64`" != "" ]; then
 		SUFFIX="linux_arm64"
 		ARCHCFLAGS=""
+	elif [ "`uname -a | grep -i mips64`" != "" ]; then
+		if [ "`lscpu | grep -i Little`" != "" ]; then
+			SUFFIX="linux_mips64le"
+			ARCHCFLAGS="-mips64 -EL"
+		else
+			SUFFIX="linux_mips64"
+			ARCHCFLAGS="-mips64 -EB"
+		fi
 	fi
 elif [ "`uname -a | grep FreeBSD`" != "" ]; then
 	# The resulting object still depends on libc.
Index: compiler-rt/lib/sanitizer_common/sanitizer_platform.h
===
--- compiler-rt/lib/sanitizer_common/sanitizer_platform.h
+++ compiler-rt/lib/sanitizer_common/sanitizer_platform.h
@@ -238,7 +238,11 @@
 // FIXME: this value should be different on different platforms.  Larger values
 // will still work but will consume more memory for TwoLevelByteMap.
 #if defined(__mips__)
+#if SANITIZER_GO && defined(__mips64)
+#define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 47)
+#else
 # define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 40)
+#endif
 #elif SANITIZER_RISCV64
 #define SANITIZER_MMAP_RANGE_SIZE FIRST_32_SECOND_64(1ULL << 32, 1ULL << 38)
 #elif defined(__aarch64__)
Index: compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
===
--- compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
+++ compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
@@ -38,6 +38,14 @@
 #include 
 #include 
 #define stat kernel_stat
+#if SANITIZER_GO
+#undef st_atime
+#undef st_mtime
+#undef st_ctime
+#define st_atime st_atim
+#define st_mtime st_mtim
+#define st_ctime st_ctim
+#endif
 #include 
 #undef stat
 #endif
@@ -248,9 +256,11 @@
 // Undefine compatibility macros from 
 // so that they would not clash with the kernel_stat
 // st_[a|m|c]time fields
+#if !SANITIZER_GO
 #undef st_atime
 #undef st_mtime
 #undef st_ctime
+#endif
 #if defined(SANITIZER_ANDROID)
 // Bionic sys/stat.h defines additional macros
 // for compatibility with the old NDKs and
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90130: Add mips64 support in lib/tsan/go/buildgo.sh

2020-10-26 Thread Dmitry Vyukov via Phabricator via cfe-commits

dvyukov added a comment.

This now looks like a patch for some other change :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90130

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

[PATCH] D90130: Add mips64 support in lib/tsan/go/buildgo.sh

2020-10-26 Thread Meng Zhuo via Phabricator via cfe-commits

mzh added a comment.

In D90130#2352826 , @dvyukov wrote:

> This now looks like a patch for some other change :)

Sorry! It's my first time with llvm patch submission. 
I think it's ok now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90130

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

[PATCH] D89158: [NewPM] Run all EP callbacks under -O0

2020-10-26 Thread Nikita Popov via Phabricator via cfe-commits

nikic added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:1439
 if (CodeGenOpts.OptimizationLevel == 0) {
+  PB.runRegisteredEPCallbacks(MPM, Level, CodeGenOpts.DebugPassManager);
+

It should be possible to simplify this function a lot now. It currently 
duplicates nearly all logic (for sanitizers etc) between O0 and O1+. Or is that 
planned for a followup?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89158

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

[PATCH] D90130: Add mips64 support in lib/tsan/go/buildgo.sh

2020-10-26 Thread Dmitry Vyukov via Phabricator via cfe-commits

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

Looks good to me.
Do you want me to merge this change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90130

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

[PATCH] D89946: [clang] Suppress "follow-up" diagnostics on recovery call expressions.

2020-10-26 Thread Haojian Wu via Phabricator via cfe-commits

hokein updated this revision to Diff 300590.
hokein marked an inline comment as done.
hokein added a comment.

address comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89946

Files:
  clang/lib/Sema/SemaOverload.cpp
  clang/test/AST/ast-dump-recovery.cpp
  clang/test/SemaCXX/typo-correction-delayed.cpp


Index: clang/test/SemaCXX/typo-correction-delayed.cpp
===
--- clang/test/SemaCXX/typo-correction-delayed.cpp
+++ clang/test/SemaCXX/typo-correction-delayed.cpp
@@ -209,6 +209,15 @@
 // expected-error-re@-1 {{use of undeclared identifier 'N'{{$
 }
 
+namespace noSecondaryDiags {
+void abcc(); // expected-note {{'abcc' declared here}}
+
+void test() {
+  // Verify the secondary diagnostic ".. convertiable to 'bool'" is suppressed.
+  if (abc()) {} // expected-error {{use of undeclared identifier 'abc'; did 
you mean 'abcc'?}}
+}
+}
+
 // PR 23285. This test must be at the end of the file to avoid additional,
 // unwanted diagnostics.
 // expected-error-re@+2 {{use of undeclared identifier 'uintmax_t'{{$
Index: clang/test/AST/ast-dump-recovery.cpp
===
--- clang/test/AST/ast-dump-recovery.cpp
+++ clang/test/AST/ast-dump-recovery.cpp
@@ -271,3 +271,12 @@
   // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 2
   invalid() ? 1 : 2;
 }
+
+void abcc();
+void TypoCorrection() {
+  // CHECK:  RecoveryExpr {{.*}} ''
+  // CHECK-NEXT: `-CallExpr {{.*}} 'void'
+  // CHECK-NEXT:   `-ImplicitCastExpr
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'abcc'
+  abc();
+}
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -12725,8 +12725,6 @@
 }
 
 /// Attempts to recover from a call where no functions were found.
-///
-/// Returns true if new candidates were found.
 static ExprResult
 BuildRecoveryCallExpr(Sema &SemaRef, Scope *S, Expr *Fn,
   UnresolvedLookupExpr *ULE,
@@ -12783,7 +12781,7 @@
 return ExprError();
   }
 
-  // Build an implicit member call if appropriate.  Just drop the
+  // Build an implicit member access expression if appropriate. Just drop the
   // casts and such from the call, we don't really care.
   ExprResult NewFn = ExprError();
   if ((*R.begin())->isCXXClassMember())
@@ -12798,12 +12796,17 @@
   if (NewFn.isInvalid())
 return ExprError();
 
-  // This shouldn't cause an infinite loop because we're giving it
-  // an expression with viable lookup results, which should never
-  // end up here.
-  return SemaRef.BuildCallExpr(/*Scope*/ nullptr, NewFn.get(), LParenLoc,
-   MultiExprArg(Args.data(), Args.size()),
-   RParenLoc);
+  auto CallE =
+  SemaRef.BuildCallExpr(/*Scope*/ nullptr, NewFn.get(), LParenLoc,
+MultiExprArg(Args.data(), Args.size()), RParenLoc);
+  if (CallE.isInvalid())
+return ExprError();
+  // We now have recovered a callee. However, building a real call may lead to
+  // incorrect secondary diagnostics if our recovery wasn't correct.
+  // We keep the recovery behavior but suppress all following diagnostics by
+  // using RecoveryExpr.
+  return SemaRef.CreateRecoveryExpr(CallE.get()->getBeginLoc(),
+CallE.get()->getEndLoc(), {CallE.get()});
 }
 
 /// Constructs and populates an OverloadedCandidateSet from


Index: clang/test/SemaCXX/typo-correction-delayed.cpp
===
--- clang/test/SemaCXX/typo-correction-delayed.cpp
+++ clang/test/SemaCXX/typo-correction-delayed.cpp
@@ -209,6 +209,15 @@
 // expected-error-re@-1 {{use of undeclared identifier 'N'{{$
 }
 
+namespace noSecondaryDiags {
+void abcc(); // expected-note {{'abcc' declared here}}
+
+void test() {
+  // Verify the secondary diagnostic ".. convertiable to 'bool'" is suppressed.
+  if (abc()) {} // expected-error {{use of undeclared identifier 'abc'; did you mean 'abcc'?}}
+}
+}
+
 // PR 23285. This test must be at the end of the file to avoid additional,
 // unwanted diagnostics.
 // expected-error-re@+2 {{use of undeclared identifier 'uintmax_t'{{$
Index: clang/test/AST/ast-dump-recovery.cpp
===
--- clang/test/AST/ast-dump-recovery.cpp
+++ clang/test/AST/ast-dump-recovery.cpp
@@ -271,3 +271,12 @@
   // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 2
   invalid() ? 1 : 2;
 }
+
+void abcc();
+void TypoCorrection() {
+  // CHECK:  RecoveryExpr {{.*}} ''
+  // CHECK-NEXT: `-CallExpr {{.*}} 'void'
+  // CHECK-NEXT:   `-ImplicitCastExpr
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'abcc'
+  abc();
+}
Index: clang/lib/Sema/SemaOverload.cpp

[PATCH] D89946: [clang] Suppress "follow-up" diagnostics on recovery call expressions.

2020-10-26 Thread Haojian Wu via Phabricator via cfe-commits

hokein added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:12814
   // end up here.
   return SemaRef.BuildCallExpr(/*Scope*/ nullptr, NewFn.get(), LParenLoc,
MultiExprArg(Args.data(), Args.size()),

sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > This results in (IIUC):
> > > 
> > > ```
> > > CallExpr
> > > >RecoveryExpr (indirection we inserted)
> > > >>DeclRefExpr (callee)
> > > >Arg1
> > > ```
> > > 
> > > Whereas when overload resolution fails, we create:
> > > ```
> > > RecoveryExpr (call)
> > > > OverloadExpr (callee)
> > > > Arg1
> > > ```
> > > 
> > > I can see advantages for either, but is there a reason not to be 
> > > consistent?
> > > (Which I guess means emitting the first one here)
> > > (Which I guess means emitting the first one here)
> > 
> > yes, exactly. 
> > 
> > reasons:
> > - BuildCallExpr could emit diagnostics if we pass a non-dependent callee to 
> > it, this is something we want to avoid;
> > - we'd save some cost -- `BuildCallExpr` has dedicated code path to handle 
> > dependent expr; 
> Sorry, I meant emitting the second one here. (Or changing the callexpr 
> recovery to use the first form, but that's a larger patch).
> 
> I understand we save some code, but not very much (the second form is easy to 
> produce), and we produce quite different ASTs for essentially the same 
> pattern (the only difference is whether we attempted recovery or not). This 
> doesn't seem like a good trade...
Changed to the second form for consistency. And it doesn't break any existing 
tests (neither improvements nor regressions).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89946

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

[PATCH] D88790: [libTooling] Recognize sccache as a compiler wrapper in compilation database commands

2020-10-26 Thread Sam McCall via Phabricator via cfe-commits

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

Thanks! Sorry about missing this...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88790

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

[PATCH] D89946: [clang] Suppress "follow-up" diagnostics on recovery call expressions.

2020-10-26 Thread Sam McCall via Phabricator via cfe-commits

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

Thanks! LG with comment nit




Comment at: clang/lib/Sema/SemaOverload.cpp:12808
+  // using RecoveryExpr.
+  return SemaRef.CreateRecoveryExpr(CallE.get()->getBeginLoc(),
+CallE.get()->getEndLoc(), {CallE.get()});

is it a deliberate decision to drop the return type of the recovery function 
here too? If so, mention it in the comment (currentyl you only talk about not 
preserving the real call node)



Comment at: clang/test/AST/ast-dump-recovery.cpp:277
+void TypoCorrection() {
+  // CHECK:  RecoveryExpr {{.*}} ''
+  // CHECK-NEXT: `-CallExpr {{.*}} 'void'

(so this could be void. It will still trigger some follow-on diagnostics though)



Comment at: clang/test/SemaCXX/typo-correction-delayed.cpp:216
+void test() {
+  // Verify the secondary diagnostic ".. convertiable to 'bool'" is suppressed.
+  if (abc()) {} // expected-error {{use of undeclared identifier 'abc'; did 
you mean 'abcc'?}}

nit: convertible


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89946

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

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

2020-10-26 Thread Krasimir Georgiev via Phabricator via cfe-commits

krasimir added a comment.

This looks reasonable.

I'm curious about DiagnosticsEngine: previously we were using the defaults for 
`client` and `ShouldOwnClient` ctor params (`nullptr` and `true`). If this 
usage leads to crashes, isn't the issue in `DiagnosticsEngine` itself?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90121

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

[PATCH] D90130: Add mips64 support in lib/tsan/go/buildgo.sh

2020-10-26 Thread Meng Zhuo via Phabricator via cfe-commits

mzh added a comment.

In D90130#2352831 , @dvyukov wrote:

> Looks good to me.
> Do you want me to merge this change?

Sure, Thanks :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90130

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

[PATCH] D90134: [clangd] Increase the TooMany limit for index-based textual navigation to 5

2020-10-26 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet added a comment.

> My experience using this feature in real codebases has been that we take this 
> early-exit too often; the limit of 3 is fairly easy to hit due to things like 
> layers of wrappers that repeat a function name. Bumping the limit to 5 has 
> increased the usefulness for me.

What about counting number of different files, rather than number of symbols ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90134

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

[PATCH] D80531: [clang-tidy]: Added modernize-replace-disallow-copy-and-assign-macro

2020-10-26 Thread Konrad Kleine via Phabricator via cfe-commits

kkleine added a comment.

@dblaikie sorry for the late feedback. The `LLVM_ENABLE_WERROR:BOOL` will "Stop 
and fail the build, if a compiler warning is triggered. Defaults to OFF." I 
wonder if any other test fails from clang tidy because. My test explicitly 
checks that a warning is issued (e.g. `// CHECK-MESSAGES-DEFAULT: 
:[[@LINE-2]]:3: warning: prefer deleting`) and when the `LLVM_ENABLE_WERROR` 
propagates to this piece, it will indeed fail the build. But I wonder why this 
doesn't happen to the other clang-tidy checks. @njames93  @Eugene.Zelenko any 
ideas?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80531

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

[PATCH] D89942: Disable LTO and LLD for bootstrap builds on systems unsupported by LLD

2020-10-26 Thread serge via Phabricator via cfe-commits

serge-sans-paille added inline comments.



Comment at: clang/CMakeLists.txt:668
+message(STATUS "Using system linker for stage3 builds on Apple")
+set(BOOTSTRAP_LLVM_ENABLE_LLD OFF CACHE BOOL "")
+  else()

Should you also disable LTO here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89942

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

[PATCH] D89946: [clang] Suppress "follow-up" diagnostics on recovery call expressions.

2020-10-26 Thread Haojian Wu via Phabricator via cfe-commits

hokein updated this revision to Diff 300601.
hokein marked an inline comment as done.
hokein added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89946

Files:
  clang/lib/Sema/SemaOverload.cpp
  clang/test/AST/ast-dump-recovery.cpp
  clang/test/SemaCXX/typo-correction-delayed.cpp


Index: clang/test/SemaCXX/typo-correction-delayed.cpp
===
--- clang/test/SemaCXX/typo-correction-delayed.cpp
+++ clang/test/SemaCXX/typo-correction-delayed.cpp
@@ -209,6 +209,15 @@
 // expected-error-re@-1 {{use of undeclared identifier 'N'{{$
 }
 
+namespace noSecondaryDiags {
+void abcc(); // expected-note {{'abcc' declared here}}
+
+void test() {
+  // Verify the secondary diagnostic ".. convertible to 'bool'" is suppressed.
+  if (abc()) {} // expected-error {{use of undeclared identifier 'abc'; did 
you mean 'abcc'?}}
+}
+}
+
 // PR 23285. This test must be at the end of the file to avoid additional,
 // unwanted diagnostics.
 // expected-error-re@+2 {{use of undeclared identifier 'uintmax_t'{{$
Index: clang/test/AST/ast-dump-recovery.cpp
===
--- clang/test/AST/ast-dump-recovery.cpp
+++ clang/test/AST/ast-dump-recovery.cpp
@@ -271,3 +271,14 @@
   // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 2
   invalid() ? 1 : 2;
 }
+
+void abcc();
+void TypoCorrection() {
+  // RecoveryExpr is always dependent-type in this case in order to suppress
+  // following diagnostics.
+  // CHECK:  RecoveryExpr {{.*}} ''
+  // CHECK-NEXT: `-CallExpr {{.*}} 'void'
+  // CHECK-NEXT:   `-ImplicitCastExpr
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'abcc'
+  abc();
+}
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -12735,8 +12735,6 @@
 }
 
 /// Attempts to recover from a call where no functions were found.
-///
-/// Returns true if new candidates were found.
 static ExprResult
 BuildRecoveryCallExpr(Sema &SemaRef, Scope *S, Expr *Fn,
   UnresolvedLookupExpr *ULE,
@@ -12793,7 +12791,7 @@
 return ExprError();
   }
 
-  // Build an implicit member call if appropriate.  Just drop the
+  // Build an implicit member access expression if appropriate. Just drop the
   // casts and such from the call, we don't really care.
   ExprResult NewFn = ExprError();
   if ((*R.begin())->isCXXClassMember())
@@ -12808,12 +12806,19 @@
   if (NewFn.isInvalid())
 return ExprError();
 
-  // This shouldn't cause an infinite loop because we're giving it
-  // an expression with viable lookup results, which should never
-  // end up here.
-  return SemaRef.BuildCallExpr(/*Scope*/ nullptr, NewFn.get(), LParenLoc,
-   MultiExprArg(Args.data(), Args.size()),
-   RParenLoc);
+  auto CallE =
+  SemaRef.BuildCallExpr(/*Scope*/ nullptr, NewFn.get(), LParenLoc,
+MultiExprArg(Args.data(), Args.size()), RParenLoc);
+  if (CallE.isInvalid())
+return ExprError();
+  // We now have recovered a callee. However, building a real call may lead to
+  // incorrect secondary diagnostics if our recovery wasn't correct.
+  // We keep the recovery behavior but suppress all following diagnostics by
+  // using RecoveryExpr. We deliberately drop the return type of the recovery
+  // function, and rely on clang's dependent mechanism to suppress following
+  // diagnostics.
+  return SemaRef.CreateRecoveryExpr(CallE.get()->getBeginLoc(),
+CallE.get()->getEndLoc(), {CallE.get()});
 }
 
 /// Constructs and populates an OverloadedCandidateSet from


Index: clang/test/SemaCXX/typo-correction-delayed.cpp
===
--- clang/test/SemaCXX/typo-correction-delayed.cpp
+++ clang/test/SemaCXX/typo-correction-delayed.cpp
@@ -209,6 +209,15 @@
 // expected-error-re@-1 {{use of undeclared identifier 'N'{{$
 }
 
+namespace noSecondaryDiags {
+void abcc(); // expected-note {{'abcc' declared here}}
+
+void test() {
+  // Verify the secondary diagnostic ".. convertible to 'bool'" is suppressed.
+  if (abc()) {} // expected-error {{use of undeclared identifier 'abc'; did you mean 'abcc'?}}
+}
+}
+
 // PR 23285. This test must be at the end of the file to avoid additional,
 // unwanted diagnostics.
 // expected-error-re@+2 {{use of undeclared identifier 'uintmax_t'{{$
Index: clang/test/AST/ast-dump-recovery.cpp
===
--- clang/test/AST/ast-dump-recovery.cpp
+++ clang/test/AST/ast-dump-recovery.cpp
@@ -271,3 +271,14 @@
   // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 2
   invalid() ? 1 : 2;
 }
+
+void abcc();
+void TypoCorrection() {
+  // RecoveryExpr is always

[PATCH] D89946: [clang] Suppress "follow-up" diagnostics on recovery call expressions.

2020-10-26 Thread Haojian Wu via Phabricator via cfe-commits

hokein added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:12808
+  // using RecoveryExpr.
+  return SemaRef.CreateRecoveryExpr(CallE.get()->getBeginLoc(),
+CallE.get()->getEndLoc(), {CallE.get()});

sammccall wrote:
> is it a deliberate decision to drop the return type of the recovery function 
> here too? If so, mention it in the comment (currentyl you only talk about not 
> preserving the real call node)
yes, we deliberately use a dependent-type for recovery-expr to suppress 
diagnostics (rely on clang's dependent mechanism).



Comment at: clang/test/AST/ast-dump-recovery.cpp:277
+void TypoCorrection() {
+  // CHECK:  RecoveryExpr {{.*}} ''
+  // CHECK-NEXT: `-CallExpr {{.*}} 'void'

sammccall wrote:
> (so this could be void. It will still trigger some follow-on diagnostics 
> though)
As described in the previous comment, the type should always be dependent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89946

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

[clang] d3205bb - [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-26 Thread via cfe-commits


Author: Tyker
Date: 2020-10-26T10:50:05+01:00
New Revision: d3205bbca3e0002d76282878986993e7e7994779

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

LOG: [Annotation] Allows annotation to carry some additional constant arguments.

This allows using annotation in a much more contexts than it currently has.
especially when annotation with template or constexpr.

Reviewed By: aaron.ballman

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

Added: 
clang/test/CodeGenCXX/attr-annotate.cpp
clang/test/CodeGenCXX/attr-annotate2.cpp
clang/test/SemaCXX/attr-annotate.cpp

Modified: 
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/ParsedAttr.h
clang/include/clang/Sema/Sema.h
clang/lib/CodeGen/CGBuiltin.cpp
clang/lib/CodeGen/CodeGenFunction.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/lib/CodeGen/CodeGenModule.cpp
clang/lib/CodeGen/CodeGenModule.h
clang/lib/Sema/SemaDeclAttr.cpp
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
clang/test/AST/ast-dump-attr.cpp
clang/test/CodeGen/annotations-field.c
clang/test/CodeGen/annotations-global.c
clang/test/CodeGen/annotations-loc.c
clang/test/CodeGen/annotations-var.c
clang/test/Misc/pragma-attribute-cxx.cpp
clang/test/Misc/pragma-attribute-objc.m
clang/test/Parser/access-spec-attrs.cpp
clang/test/Parser/objc-implementation-attrs.m
clang/test/Sema/annotate.c
clang/test/Sema/pragma-attribute.c
llvm/include/llvm/IR/Intrinsics.td
llvm/test/Analysis/CostModel/X86/free-intrinsics.ll
llvm/test/Analysis/CostModel/free-intrinsics-datalayout.ll
llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
llvm/test/CodeGen/Generic/ptr-annotate.ll
llvm/test/Transforms/InstCombine/assume_inevitable.ll

Removed: 




diff  --git a/clang/include/clang/Basic/Attr.td 
b/clang/include/clang/Basic/Attr.td
index 56079db4d9c5..2b51a3ffcc6e 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -737,7 +737,7 @@ def AnalyzerNoReturn : InheritableAttr {
 
 def Annotate : InheritableParamAttr {
   let Spellings = [Clang<"annotate">];
-  let Args = [StringArgument<"Annotation">];
+  let Args = [StringArgument<"Annotation">, VariadicExprArgument<"Args">];
   // Ensure that the annotate attribute can be used with
   // '#pragma clang attribute' even though it has no subject list.
   let PragmaAttributeSupport = 1;

diff  --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 1aa6064d2210..e1d20fbcb433 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2860,7 +2860,7 @@ def err_attribute_sizeless_type : Error<
   "%0 attribute cannot be applied to sizeless type %1">;
 def err_attribute_argument_n_type : Error<
   "%0 attribute requires parameter %1 to be %select{int or bool|an integer "
-  "constant|a string|an identifier}2">;
+  "constant|a string|an identifier|a constant expression}2">;
 def err_attribute_argument_type : Error<
   "%0 attribute requires %select{int or bool|an integer "
   "constant|a string|an identifier}1">;

diff  --git a/clang/include/clang/Sema/ParsedAttr.h 
b/clang/include/clang/Sema/ParsedAttr.h
index 45b0f85857f6..43c21faaece9 100644
--- a/clang/include/clang/Sema/ParsedAttr.h
+++ b/clang/include/clang/Sema/ParsedAttr.h
@@ -1023,7 +1023,8 @@ enum AttributeArgumentNType {
   AANT_ArgumentIntOrBool,
   AANT_ArgumentIntegerConstant,
   AANT_ArgumentString,
-  AANT_ArgumentIdentifier
+  AANT_ArgumentIdentifier,
+  AANT_ArgumentConstantExpr,
 };
 
 /// These constants match the enumerated choices of
@@ -1058,6 +1059,31 @@ inline const StreamingDiagnostic &operator<<(const 
StreamingDiagnostic &DB,
   return DB;
 }
 
+/// AttributeCommonInfo has a non-explicit constructor which takes an
+/// SourceRange as its only argument, this constructor has many uses so making
+/// it explicit is hard. This constructor causes ambiguity with
+/// DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB, SourceRange R).
+/// We use SFINAE to disable any conversion and remove any ambiguity.
+template ::value, int> = 0>
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
+   const ACI &CI) {
+  DB.AddTaggedVal(reinterpret_cast(CI.getAttrName()),
+  DiagnosticsEngine::ak_identifierinfo);
+  return DB;
+}
+
+template ::value, int> = 0>
+inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
+   const ACI* CI) {
+  DB.AddTaggedVal(reinterpret_cast(CI->getAttrName()),
+  DiagnosticsEn

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-26 Thread Tyker via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd3205bbca3e0: [Annotation] Allows annotation to carry some 
additional constant arguments. (authored by Tyker).

Changed prior to commit:
  https://reviews.llvm.org/D88645?vs=300213&id=300608#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88645

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/ParsedAttr.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGen/annotations-field.c
  clang/test/CodeGen/annotations-global.c
  clang/test/CodeGen/annotations-loc.c
  clang/test/CodeGen/annotations-var.c
  clang/test/CodeGenCXX/attr-annotate.cpp
  clang/test/CodeGenCXX/attr-annotate2.cpp
  clang/test/Misc/pragma-attribute-cxx.cpp
  clang/test/Misc/pragma-attribute-objc.m
  clang/test/Parser/access-spec-attrs.cpp
  clang/test/Parser/objc-implementation-attrs.m
  clang/test/Sema/annotate.c
  clang/test/Sema/pragma-attribute.c
  clang/test/SemaCXX/attr-annotate.cpp
  llvm/include/llvm/IR/Intrinsics.td
  llvm/test/Analysis/CostModel/X86/free-intrinsics.ll
  llvm/test/Analysis/CostModel/free-intrinsics-datalayout.ll
  llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
  llvm/test/CodeGen/Generic/ptr-annotate.ll
  llvm/test/Transforms/InstCombine/assume_inevitable.ll

Index: llvm/test/Transforms/InstCombine/assume_inevitable.ll
===
--- llvm/test/Transforms/InstCombine/assume_inevitable.ll
+++ llvm/test/Transforms/InstCombine/assume_inevitable.ll
@@ -15,7 +15,7 @@
 ; CHECK-NEXT:[[DUMMY_EQ:%.*]] = icmp ugt i32 [[LOADRES]], 42
 ; CHECK-NEXT:tail call void @llvm.assume(i1 [[DUMMY_EQ]])
 ; CHECK-NEXT:[[M_I8:%.*]] = bitcast i64* [[M]] to i8*
-; CHECK-NEXT:[[M_A:%.*]] = call i8* @llvm.ptr.annotation.p0i8(i8* nonnull [[M_I8]], i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i64 0, i64 0), i32 2)
+; CHECK-NEXT:[[M_A:%.*]] = call i8* @llvm.ptr.annotation.p0i8(i8* nonnull [[M_I8]], i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i64 0, i64 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i64 0, i64 0), i32 2, i8* null)
 ; CHECK-NEXT:[[M_X:%.*]] = bitcast i8* [[M_A]] to i64*
 ; CHECK-NEXT:[[OBJSZ:%.*]] = call i64 @llvm.objectsize.i64.p0i8(i8* [[C:%.*]], i1 false, i1 false, i1 false)
 ; CHECK-NEXT:store i64 [[OBJSZ]], i64* [[M_X]], align 4
@@ -44,7 +44,7 @@
   call void @llvm.lifetime.end.p0i8(i64 1, i8* %dummy)
 
   %m_i8 = bitcast i64* %m to i8*
-  %m_a = call i8* @llvm.ptr.annotation.p0i8(i8* %m_i8, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2)
+  %m_a = call i8* @llvm.ptr.annotation.p0i8(i8* %m_i8, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2, i8* null)
   %m_x = bitcast i8* %m_a to i64*
   %objsz = call i64 @llvm.objectsize.i64.p0i8(i8* %c, i1 false)
   store i64 %objsz, i64* %m_x
@@ -64,7 +64,7 @@
 
 declare i64 @llvm.objectsize.i64.p0i8(i8*, i1)
 declare i32 @llvm.annotation.i32(i32, i8*, i8*, i32)
-declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32)
+declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32, i8*)
 
 declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture)
 declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture)
Index: llvm/test/CodeGen/Generic/ptr-annotate.ll
===
--- llvm/test/CodeGen/Generic/ptr-annotate.ll
+++ llvm/test/CodeGen/Generic/ptr-annotate.ll
@@ -10,9 +10,9 @@
 define void @foo() {
 entry:
   %m = alloca i8, align 4
-  %0 = call i8* @llvm.ptr.annotation.p0i8(i8* %m, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2)
+  %0 = call i8* @llvm.ptr.annotation.p0i8(i8* %m, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str, i32 0, i32 0), i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str1, i32 0, i32 0), i32 2, i8* null)
   store i8 1, i8* %0, align 4
   ret void
 }
 
-declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32) #1
+declare i8* @llvm.ptr.annotation.p0i8(i8*, i8*, i8*, i32, i8*) #1
Index: llvm/test/Analysis/CostModel/free-intrinsics-no_info.ll
===
--- llvm/tes

[PATCH] D90140: [clang][RecoveryExpr] Add tests for ObjectiveC.

2020-10-26 Thread Haojian Wu via Phabricator via cfe-commits

hokein created this revision.
hokein added a reviewer: sammccall.
Herald added a project: clang.
hokein requested review of this revision.

to demonstrate it works for some cases.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90140

Files:
  clang/test/AST/ast-dump-recovery.m


Index: clang/test/AST/ast-dump-recovery.m
===
--- /dev/null
+++ clang/test/AST/ast-dump-recovery.m
@@ -0,0 +1,18 @@
+// RUN: not %clang_cc1 -triple x86_64-unknown-unknown -frecovery-ast 
-frecovery-ast-type -ast-dump %s | FileCheck -strict-whitespace %s
+
+@interface Foo
+- (void)method:(int)n;
+@end
+
+void k(Foo *foo) {
+  // CHECK:   ObjCMessageExpr {{.*}} 'void' contains-errors
+  // CHECK-CHECK:  |-ImplicitCastExpr {{.*}} 'Foo *' 
+  // CHECK-CHECK:  | `-DeclRefExpr {{.*}} 'foo'
+  // CHECK-CHECK:  `-RecoveryExpr {{.*}}
+  [foo method:undef];
+
+  // CHECK:  ImplicitCastExpr {{.*}} '' contains-errors
+  // CHECK-NEXT: `-RecoveryExpr {{.*}} '' contains-errors
+  // CHECK-NEXT:   `-DeclRefExpr {{.*}} 'foo'
+  foo.undef;
+}


Index: clang/test/AST/ast-dump-recovery.m
===
--- /dev/null
+++ clang/test/AST/ast-dump-recovery.m
@@ -0,0 +1,18 @@
+// RUN: not %clang_cc1 -triple x86_64-unknown-unknown -frecovery-ast -frecovery-ast-type -ast-dump %s | FileCheck -strict-whitespace %s
+
+@interface Foo
+- (void)method:(int)n;
+@end
+
+void k(Foo *foo) {
+  // CHECK:   ObjCMessageExpr {{.*}} 'void' contains-errors
+  // CHECK-CHECK:  |-ImplicitCastExpr {{.*}} 'Foo *' 
+  // CHECK-CHECK:  | `-DeclRefExpr {{.*}} 'foo'
+  // CHECK-CHECK:  `-RecoveryExpr {{.*}}
+  [foo method:undef];
+
+  // CHECK:  ImplicitCastExpr {{.*}} '' contains-errors
+  // CHECK-NEXT: `-RecoveryExpr {{.*}} '' contains-errors
+  // CHECK-NEXT:   `-DeclRefExpr {{.*}} 'foo'
+  foo.undef;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89936: clang-tidy: adding "--clang-tidy-config=" to specify custom config file

2020-10-26 Thread Dmitry Polukhin via Phabricator via cfe-commits

DmitryPolukhin added a comment.

I'm not sure that we need additional option to read configuration from file 
but, if we do need, I think this diff needs some improvements + test for new 
option.




Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp:324
+llvm::sys::fs::is_symlink_file(Twine(AbsoluteFilePath), IsLink);
+if (!(IsFile || IsLink)) {
+  std::string Msg;

Is it actually required to check absolute path, link it or not, etc.? Why not 
just try reading file with provided filename and report error if it fails?



Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp:408
+FileOptionsProvider::tryReadConfigFile(StringRef Path, bool IsFile) {
+  // llvm::outs() << "tryReadConfigFile IsFile<" <<
+  // OverrideOptions.ClangTidyConfig << ">\n";

It seems like some debug prints.



Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:68
+  /// Clang-tidy-config
+  llvm::Optional ClangTidyConfig;
+

I'm not sure that we need it here. I would reuse code path for `--config` 
options as much as possible and implement new option as a simple wrapper that 
reads content of the file and interpret it as `--config` option. Moreover I 
think it should not be possible to specify both options in command line.



Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:233
   llvm::Optional tryReadConfigFile(llvm::StringRef Directory);
+  llvm::Optional tryReadConfigFile(llvm::StringRef Path,
+  bool IsFile);

It looks like the second argument was added only for overload resolution. But I 
think it is better to rename this function. After all it is not "try" anymore 
because it reports fatal error in case of errors.



Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:76
 
+static cl::opt ClangTidyConfig("clang-tidy-config", cl::desc(R"(
+Specify full path of .clang-tidy config file.

I would call it something like `--config-file`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89936

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

[PATCH] D90116: [clangd] Escape Unicode characters to fix Windows builds

2020-10-26 Thread Sam McCall via Phabricator via cfe-commits

sammccall added a comment.

This isn't the only place where we use such characters. While I'm not sure 
spelling these literally is a hill we want to die on, I think u8 literals are 
so poorly understood that trying to do this consistently isn't going to be that 
simple.

As noted, clang is always reading as utf-8. gcc is more complicated but we have 
no reports of this problem. So I think we should assume this is msvc-only and 
if we can find a CMake workaround we should take it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90116

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

[PATCH] D90127: [clang][NFC] Rearrange Comment Token and Lexer fields to reduce padding

2020-10-26 Thread Nathan James via Phabricator via cfe-commits

njames93 updated this revision to Diff 300622.
njames93 added a comment.

Keep VerbatimBlockEndCommandName after LexerState while preserving smaller size


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90127

Files:
  clang/include/clang/AST/CommentLexer.h
  clang/lib/AST/CommentLexer.cpp


Index: clang/lib/AST/CommentLexer.cpp
===
--- clang/lib/AST/CommentLexer.cpp
+++ clang/lib/AST/CommentLexer.cpp
@@ -740,11 +740,10 @@
 
 Lexer::Lexer(llvm::BumpPtrAllocator &Allocator, DiagnosticsEngine &Diags,
  const CommandTraits &Traits, SourceLocation FileLoc,
- const char *BufferStart, const char *BufferEnd,
- bool ParseCommands)
+ const char *BufferStart, const char *BufferEnd, bool 
ParseCommands)
 : Allocator(Allocator), Diags(Diags), Traits(Traits),
-  BufferStart(BufferStart), BufferEnd(BufferEnd), FileLoc(FileLoc),
-  BufferPtr(BufferStart), CommentState(LCS_BeforeComment), 
State(LS_Normal),
+  BufferStart(BufferStart), BufferEnd(BufferEnd), BufferPtr(BufferStart),
+  FileLoc(FileLoc), CommentState(LCS_BeforeComment), State(LS_Normal),
   ParseCommands(ParseCommands) {}
 
 void Lexer::lex(Token &T) {
Index: clang/include/clang/AST/CommentLexer.h
===
--- clang/include/clang/AST/CommentLexer.h
+++ clang/include/clang/AST/CommentLexer.h
@@ -66,9 +66,6 @@
   /// tokens.
   unsigned Length;
 
-  /// Contains text value associated with a token.
-  const char *TextPtr;
-
   /// Integer value associated with a token.
   ///
   /// If the token is a known command, contains command ID and TextPtr is
@@ -76,6 +73,9 @@
   /// contains the length of the string that starts at TextPtr.
   unsigned IntVal;
 
+  /// Contains text value associated with a token.
+  const char *TextPtr;
+
 public:
   SourceLocation getLocation() const LLVM_READONLY { return Loc; }
   void setLocation(SourceLocation SL) { Loc = SL; }
@@ -232,7 +232,6 @@
 
   const char *const BufferStart;
   const char *const BufferEnd;
-  SourceLocation FileLoc;
 
   const char *BufferPtr;
 
@@ -240,7 +239,14 @@
   /// to newline or BufferEnd, for C comments points to star in '*/'.
   const char *CommentEnd;
 
-  enum LexerCommentState {
+  SourceLocation FileLoc;
+
+  /// If true, the commands, html tags, etc will be parsed and reported as
+  /// separate tokens inside the comment body. If false, the comment text will
+  /// be parsed into text and newline tokens.
+  bool ParseCommands;
+
+  enum LexerCommentState : uint8_t {
 LCS_BeforeComment,
 LCS_InsideBCPLComment,
 LCS_InsideCComment,
@@ -250,7 +256,7 @@
   /// Low-level lexer state, track if we are inside or outside of comment.
   LexerCommentState CommentState;
 
-  enum LexerState {
+  enum LexerState : uint8_t {
 /// Lexing normal comment text
 LS_Normal,
 
@@ -280,11 +286,6 @@
   /// command, including command marker.
   SmallString<16> VerbatimBlockEndCommandName;
 
-  /// If true, the commands, html tags, etc will be parsed and reported as
-  /// separate tokens inside the comment body. If false, the comment text will
-  /// be parsed into text and newline tokens.
-  bool ParseCommands;
-
   /// Given a character reference name (e.g., "lt"), return the character that
   /// it stands for (e.g., "<").
   StringRef resolveHTMLNamedCharacterReference(StringRef Name) const;


Index: clang/lib/AST/CommentLexer.cpp
===
--- clang/lib/AST/CommentLexer.cpp
+++ clang/lib/AST/CommentLexer.cpp
@@ -740,11 +740,10 @@
 
 Lexer::Lexer(llvm::BumpPtrAllocator &Allocator, DiagnosticsEngine &Diags,
  const CommandTraits &Traits, SourceLocation FileLoc,
- const char *BufferStart, const char *BufferEnd,
- bool ParseCommands)
+ const char *BufferStart, const char *BufferEnd, bool ParseCommands)
 : Allocator(Allocator), Diags(Diags), Traits(Traits),
-  BufferStart(BufferStart), BufferEnd(BufferEnd), FileLoc(FileLoc),
-  BufferPtr(BufferStart), CommentState(LCS_BeforeComment), State(LS_Normal),
+  BufferStart(BufferStart), BufferEnd(BufferEnd), BufferPtr(BufferStart),
+  FileLoc(FileLoc), CommentState(LCS_BeforeComment), State(LS_Normal),
   ParseCommands(ParseCommands) {}
 
 void Lexer::lex(Token &T) {
Index: clang/include/clang/AST/CommentLexer.h
===
--- clang/include/clang/AST/CommentLexer.h
+++ clang/include/clang/AST/CommentLexer.h
@@ -66,9 +66,6 @@
   /// tokens.
   unsigned Length;
 
-  /// Contains text value associated with a token.
-  const char *TextPtr;
-
   /// Integer value associated with a token.
   ///
   /// If the token is a known command, contains command ID and TextPtr is
@@ -76,6 +73,9 @@
   /// contains

[PATCH] D88645: [Annotation] Allows annotation to carry some additional constant arguments.

2020-10-26 Thread Nico Weber via Phabricator via cfe-commits

thakis added a comment.

Looks like this broke tests: http://45.33.8.238/linux/31159/step_12.txt

Please take a look, and revert for now if it takes a while to fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88645

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

[PATCH] D90127: [clang][NFC] Rearrange Comment Token and Lexer fields to reduce padding

2020-10-26 Thread Nathan James via Phabricator via cfe-commits

njames93 added inline comments.



Comment at: clang/include/clang/AST/CommentLexer.h:244
+  /// command, including command marker.
+  SmallString<16> VerbatimBlockEndCommandName;
+

gribozavr2 wrote:
> I'm not a fan of this change to `Lexer` because it breaks the grouping of 
> fields: `VerbatimBlockEndCommandName` is no longer next to `State`.
> 
> There is only ever one `Lexer` class instance anyway, so any memory savings 
> are not important I think.
Fair point, I've fixed that grouping of fields while keeping the smaller size, 
but if you think its still not worth it, I'll revert that change.
The token class size change would be worth it as they appear in vectors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90127

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

[PATCH] D90127: [clang][NFC] Rearrange Comment Token and Lexer fields to reduce padding

2020-10-26 Thread Nathan James via Phabricator via cfe-commits

njames93 updated this revision to Diff 300623.
njames93 added a comment.

Fix field initialisation order in constructor


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90127

Files:
  clang/include/clang/AST/CommentLexer.h
  clang/lib/AST/CommentLexer.cpp


Index: clang/lib/AST/CommentLexer.cpp
===
--- clang/lib/AST/CommentLexer.cpp
+++ clang/lib/AST/CommentLexer.cpp
@@ -740,12 +740,11 @@
 
 Lexer::Lexer(llvm::BumpPtrAllocator &Allocator, DiagnosticsEngine &Diags,
  const CommandTraits &Traits, SourceLocation FileLoc,
- const char *BufferStart, const char *BufferEnd,
- bool ParseCommands)
+ const char *BufferStart, const char *BufferEnd, bool 
ParseCommands)
 : Allocator(Allocator), Diags(Diags), Traits(Traits),
-  BufferStart(BufferStart), BufferEnd(BufferEnd), FileLoc(FileLoc),
-  BufferPtr(BufferStart), CommentState(LCS_BeforeComment), 
State(LS_Normal),
-  ParseCommands(ParseCommands) {}
+  BufferStart(BufferStart), BufferEnd(BufferEnd), BufferPtr(BufferStart),
+  FileLoc(FileLoc), ParseCommands(ParseCommands),
+  CommentState(LCS_BeforeComment), State(LS_Normal) {}
 
 void Lexer::lex(Token &T) {
 again:
Index: clang/include/clang/AST/CommentLexer.h
===
--- clang/include/clang/AST/CommentLexer.h
+++ clang/include/clang/AST/CommentLexer.h
@@ -66,9 +66,6 @@
   /// tokens.
   unsigned Length;
 
-  /// Contains text value associated with a token.
-  const char *TextPtr;
-
   /// Integer value associated with a token.
   ///
   /// If the token is a known command, contains command ID and TextPtr is
@@ -76,6 +73,9 @@
   /// contains the length of the string that starts at TextPtr.
   unsigned IntVal;
 
+  /// Contains text value associated with a token.
+  const char *TextPtr;
+
 public:
   SourceLocation getLocation() const LLVM_READONLY { return Loc; }
   void setLocation(SourceLocation SL) { Loc = SL; }
@@ -232,7 +232,6 @@
 
   const char *const BufferStart;
   const char *const BufferEnd;
-  SourceLocation FileLoc;
 
   const char *BufferPtr;
 
@@ -240,7 +239,14 @@
   /// to newline or BufferEnd, for C comments points to star in '*/'.
   const char *CommentEnd;
 
-  enum LexerCommentState {
+  SourceLocation FileLoc;
+
+  /// If true, the commands, html tags, etc will be parsed and reported as
+  /// separate tokens inside the comment body. If false, the comment text will
+  /// be parsed into text and newline tokens.
+  bool ParseCommands;
+
+  enum LexerCommentState : uint8_t {
 LCS_BeforeComment,
 LCS_InsideBCPLComment,
 LCS_InsideCComment,
@@ -250,7 +256,7 @@
   /// Low-level lexer state, track if we are inside or outside of comment.
   LexerCommentState CommentState;
 
-  enum LexerState {
+  enum LexerState : uint8_t {
 /// Lexing normal comment text
 LS_Normal,
 
@@ -280,11 +286,6 @@
   /// command, including command marker.
   SmallString<16> VerbatimBlockEndCommandName;
 
-  /// If true, the commands, html tags, etc will be parsed and reported as
-  /// separate tokens inside the comment body. If false, the comment text will
-  /// be parsed into text and newline tokens.
-  bool ParseCommands;
-
   /// Given a character reference name (e.g., "lt"), return the character that
   /// it stands for (e.g., "<").
   StringRef resolveHTMLNamedCharacterReference(StringRef Name) const;


Index: clang/lib/AST/CommentLexer.cpp
===
--- clang/lib/AST/CommentLexer.cpp
+++ clang/lib/AST/CommentLexer.cpp
@@ -740,12 +740,11 @@
 
 Lexer::Lexer(llvm::BumpPtrAllocator &Allocator, DiagnosticsEngine &Diags,
  const CommandTraits &Traits, SourceLocation FileLoc,
- const char *BufferStart, const char *BufferEnd,
- bool ParseCommands)
+ const char *BufferStart, const char *BufferEnd, bool ParseCommands)
 : Allocator(Allocator), Diags(Diags), Traits(Traits),
-  BufferStart(BufferStart), BufferEnd(BufferEnd), FileLoc(FileLoc),
-  BufferPtr(BufferStart), CommentState(LCS_BeforeComment), State(LS_Normal),
-  ParseCommands(ParseCommands) {}
+  BufferStart(BufferStart), BufferEnd(BufferEnd), BufferPtr(BufferStart),
+  FileLoc(FileLoc), ParseCommands(ParseCommands),
+  CommentState(LCS_BeforeComment), State(LS_Normal) {}
 
 void Lexer::lex(Token &T) {
 again:
Index: clang/include/clang/AST/CommentLexer.h
===
--- clang/include/clang/AST/CommentLexer.h
+++ clang/include/clang/AST/CommentLexer.h
@@ -66,9 +66,6 @@
   /// tokens.
   unsigned Length;
 
-  /// Contains text value associated with a token.
-  const char *TextPtr;
-
   /// Integer value associated with a token.
   ///
   /// If the token is a known command, contains

[PATCH] D89748: SourceManager: Use the same fake SLocEntry whenever it fails to load

2020-10-26 Thread Raphael Isemann via Phabricator via cfe-commits

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

LGTM.


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

https://reviews.llvm.org/D89748

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

[PATCH] D80499: Remove obsolete ignore*() matcher uses

2020-10-26 Thread Alexander Kornienko via Phabricator via cfe-commits

alexfh added a comment.

In D80499#2352339 , @steveire wrote:

> @alexfh This change is based on the behavior of AST Matchers being changed to 
> ignore invisible/implicit AST nodes by default. As the default was not 
> changed in the end, this patch would need to be updated to add 
> `traverse(TK_IgnoreUnlessSpelledInSource)` wrapping around the matchers.
>
> Before I update the patch to add that, do you have any feedback?

If the default is not being changed, clear improvement will be noticeable only 
where multiple `ignore.*` matchers are being replaced with a 
`traverse(TK_IgnoreUnlessSpelledInSource)`. However, in many cases the behavior 
of the checks will change (and I'm not sure we have good enough test coverage 
especially where implicit conversions and similar are present). I believe, 
we're going to hit a number of corner cases and uncover a ton of bugs with this 
change. (Not that it would be different, if the default was changed to ignore 
implicit stuff ;)

You should be ready for back and forth with this change, if users hit 
widespread issues not caught by tests. Maybe splitting it into separate pieces 
and involving check authors where practical may be reasonable.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80499

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

[PATCH] D80961: Ignore template instantiations if not in AsIs mode

2020-10-26 Thread Stephen Kelly via Phabricator via cfe-commits

steveire updated this revision to Diff 300629.
steveire added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80961

Files:
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/lib/AST/ASTDumper.cpp
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp
  clang/unittests/AST/ASTTraverserTest.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -1064,6 +1064,70 @@
   EXPECT_EQ(ErrorCount, 0);
 }
 
+TEST_F(TransformerTest, TemplateInstantiation) {
+
+  std::string NonTemplatesInput = R"cpp(
+struct S {
+  int m_i;
+};
+)cpp";
+  std::string NonTemplatesExpected = R"cpp(
+struct S {
+  safe_int m_i;
+};
+)cpp";
+
+  std::string TemplatesInput = R"cpp(
+template
+struct TemplStruct {
+  TemplStruct() {}
+  ~TemplStruct() {}
+
+private:
+  T m_t;
+};
+
+void instantiate()
+{
+  TemplStruct ti;
+}
+)cpp";
+
+  auto MatchedField = fieldDecl(hasType(asString("int"))).bind("theField");
+
+  // Changes the 'int' in 'S', but not the 'T' in 'TemplStruct':
+  testRule(makeRule(traverse(TK_IgnoreUnlessSpelledInSource, MatchedField),
+changeTo(cat("safe_int ", name("theField",
+   NonTemplatesInput + TemplatesInput,
+   NonTemplatesExpected + TemplatesInput);
+
+  // In AsIs mode, template instantiations are modified, which is
+  // often not desired:
+
+  std::string IncorrectTemplatesExpected = R"cpp(
+template
+struct TemplStruct {
+  TemplStruct() {}
+  ~TemplStruct() {}
+
+private:
+  safe_int m_t;
+};
+
+void instantiate()
+{
+  TemplStruct ti;
+}
+)cpp";
+
+  // Changes the 'int' in 'S', and (incorrectly) the 'T' in 'TemplStruct':
+  testRule(makeRule(traverse(TK_AsIs, MatchedField),
+changeTo(cat("safe_int ", name("theField",
+
+   NonTemplatesInput + TemplatesInput,
+   NonTemplatesExpected + IncorrectTemplatesExpected);
+}
+
 // Transformation of macro source text when the change encompasses the entirety
 // of the expanded text.
 TEST_F(TransformerTest, SimpleMacro) {
Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -2085,9 +2085,17 @@
   traverse(TK_AsIs,
staticAssertDecl(has(implicitCastExpr(has(
substNonTypeTemplateParmExpr(has(integerLiteral());
+  EXPECT_TRUE(matches(
+  Code, traverse(TK_IgnoreUnlessSpelledInSource,
+ staticAssertDecl(has(declRefExpr(
+ to(nonTypeTemplateParmDecl(hasName("alignment"))),
+ hasType(asString("unsigned int";
 
-  EXPECT_TRUE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource,
- staticAssertDecl(has(integerLiteral());
+  EXPECT_TRUE(matches(Code, traverse(TK_AsIs, staticAssertDecl(hasDescendant(
+  integerLiteral());
+  EXPECT_FALSE(matches(
+  Code, traverse(TK_IgnoreUnlessSpelledInSource,
+ staticAssertDecl(hasDescendant(integerLiteral());
 
   Code = R"cpp(
 
Index: clang/unittests/AST/ASTTraverserTest.cpp
===
--- clang/unittests/AST/ASTTraverserTest.cpp
+++ clang/unittests/AST/ASTTraverserTest.cpp
@@ -68,6 +68,14 @@
   void Visit(const TemplateArgument &A, SourceRange R = {},
  const Decl *From = nullptr, const char *Label = nullptr) {
 OS << "TemplateArgument";
+switch (A.getKind()) {
+case TemplateArgument::Type: {
+  OS << " type " << A.getAsType().getAsString();
+  break;
+}
+default:
+  break;
+}
   }
 
   template  void Visit(T...) {}
@@ -243,7 +251,7 @@
 
   verifyWithDynNode(TA,
 R"cpp(
-TemplateArgument
+TemplateArgument type int
 `-BuiltinType
 )cpp");
 
@@ -1042,4 +1050,145 @@
   }
 }
 
+TEST(Traverse, IgnoreUnlessSpelledInSourceTemplateInstantiations) {
+
+  auto AST = buildASTFromCode(R"cpp(
+template
+struct TemplStruct {
+  TemplStruct() {}
+  ~TemplStruct() {}
+
+private:
+  T m_t;
+};
+
+template
+T timesTwo(T input)
+{
+  return input * 2;
+}
+
+void instantiate()
+{
+  TemplStruct ti;
+  TemplStruct td;
+  (void)timesTwo(2);
+  (void)timesTwo(2);
+}
+)cpp");
+  {
+auto BN = ast_matchers::match(
+classTemplateDecl(hasName("TemplStruct")).bind("rec"),
+AST->getASTConte

[PATCH] D80961: Ignore template instantiations if not in AsIs mode

2020-10-26 Thread Stephen Kelly via Phabricator via cfe-commits

steveire updated this revision to Diff 300630.
steveire added a comment.

Update


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80961

Files:
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/lib/AST/ASTDumper.cpp
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp
  clang/unittests/AST/ASTTraverserTest.cpp
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -1064,6 +1064,70 @@
   EXPECT_EQ(ErrorCount, 0);
 }
 
+TEST_F(TransformerTest, TemplateInstantiation) {
+
+  std::string NonTemplatesInput = R"cpp(
+struct S {
+  int m_i;
+};
+)cpp";
+  std::string NonTemplatesExpected = R"cpp(
+struct S {
+  safe_int m_i;
+};
+)cpp";
+
+  std::string TemplatesInput = R"cpp(
+template
+struct TemplStruct {
+  TemplStruct() {}
+  ~TemplStruct() {}
+
+private:
+  T m_t;
+};
+
+void instantiate()
+{
+  TemplStruct ti;
+}
+)cpp";
+
+  auto MatchedField = fieldDecl(hasType(asString("int"))).bind("theField");
+
+  // Changes the 'int' in 'S', but not the 'T' in 'TemplStruct':
+  testRule(makeRule(traverse(TK_IgnoreUnlessSpelledInSource, MatchedField),
+changeTo(cat("safe_int ", name("theField",
+   NonTemplatesInput + TemplatesInput,
+   NonTemplatesExpected + TemplatesInput);
+
+  // In AsIs mode, template instantiations are modified, which is
+  // often not desired:
+
+  std::string IncorrectTemplatesExpected = R"cpp(
+template
+struct TemplStruct {
+  TemplStruct() {}
+  ~TemplStruct() {}
+
+private:
+  safe_int m_t;
+};
+
+void instantiate()
+{
+  TemplStruct ti;
+}
+)cpp";
+
+  // Changes the 'int' in 'S', and (incorrectly) the 'T' in 'TemplStruct':
+  testRule(makeRule(traverse(TK_AsIs, MatchedField),
+changeTo(cat("safe_int ", name("theField",
+
+   NonTemplatesInput + TemplatesInput,
+   NonTemplatesExpected + IncorrectTemplatesExpected);
+}
+
 // Transformation of macro source text when the change encompasses the entirety
 // of the expanded text.
 TEST_F(TransformerTest, SimpleMacro) {
Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -2085,9 +2085,17 @@
   traverse(TK_AsIs,
staticAssertDecl(has(implicitCastExpr(has(
substNonTypeTemplateParmExpr(has(integerLiteral());
+  EXPECT_TRUE(matches(
+  Code, traverse(TK_IgnoreUnlessSpelledInSource,
+ staticAssertDecl(has(declRefExpr(
+ to(nonTypeTemplateParmDecl(hasName("alignment"))),
+ hasType(asString("unsigned int";
 
-  EXPECT_TRUE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource,
- staticAssertDecl(has(integerLiteral());
+  EXPECT_TRUE(matches(Code, traverse(TK_AsIs, staticAssertDecl(hasDescendant(
+  integerLiteral());
+  EXPECT_FALSE(matches(
+  Code, traverse(TK_IgnoreUnlessSpelledInSource,
+ staticAssertDecl(hasDescendant(integerLiteral());
 
   Code = R"cpp(
 
Index: clang/unittests/AST/ASTTraverserTest.cpp
===
--- clang/unittests/AST/ASTTraverserTest.cpp
+++ clang/unittests/AST/ASTTraverserTest.cpp
@@ -68,6 +68,14 @@
   void Visit(const TemplateArgument &A, SourceRange R = {},
  const Decl *From = nullptr, const char *Label = nullptr) {
 OS << "TemplateArgument";
+switch (A.getKind()) {
+case TemplateArgument::Type: {
+  OS << " type " << A.getAsType().getAsString();
+  break;
+}
+default:
+  break;
+}
   }
 
   template  void Visit(T...) {}
@@ -243,7 +251,7 @@
 
   verifyWithDynNode(TA,
 R"cpp(
-TemplateArgument
+TemplateArgument type int
 `-BuiltinType
 )cpp");
 
@@ -1042,4 +1050,145 @@
   }
 }
 
+TEST(Traverse, IgnoreUnlessSpelledInSourceTemplateInstantiations) {
+
+  auto AST = buildASTFromCode(R"cpp(
+template
+struct TemplStruct {
+  TemplStruct() {}
+  ~TemplStruct() {}
+
+private:
+  T m_t;
+};
+
+template
+T timesTwo(T input)
+{
+  return input * 2;
+}
+
+void instantiate()
+{
+  TemplStruct ti;
+  TemplStruct td;
+  (void)timesTwo(2);
+  (void)timesTwo(2);
+}
+)cpp");
+  {
+auto BN = ast_matchers::match(
+classTemplateDecl(hasName("TemplStruct")).bind("rec"),
+AST->getASTConte

[PATCH] D90130: Add mips64 support in lib/tsan/go/buildgo.sh

2020-10-26 Thread Dmitry Vyukov via Phabricator via cfe-commits

dvyukov closed this revision.
dvyukov added a comment.

Landed in 
https://github.com/llvm/llvm-project/commit/5cad535ccfebf9b41a57cf2788d8de7a765f7f35


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90130

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

[PATCH] D89946: [clang] Suppress "follow-up" diagnostics on recovery call expressions.

2020-10-26 Thread Haojian Wu via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGefa9aaad703e: [clang] Suppress "follow-up" 
diagnostics on recovery call expressions. (authored by hokein).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89946

Files:
  clang/lib/Sema/SemaOverload.cpp
  clang/test/AST/ast-dump-recovery.cpp
  clang/test/SemaCXX/typo-correction-delayed.cpp


Index: clang/test/SemaCXX/typo-correction-delayed.cpp
===
--- clang/test/SemaCXX/typo-correction-delayed.cpp
+++ clang/test/SemaCXX/typo-correction-delayed.cpp
@@ -209,6 +209,15 @@
 // expected-error-re@-1 {{use of undeclared identifier 'N'{{$
 }
 
+namespace noSecondaryDiags {
+void abcc(); // expected-note {{'abcc' declared here}}
+
+void test() {
+  // Verify the secondary diagnostic ".. convertible to 'bool'" is suppressed.
+  if (abc()) {} // expected-error {{use of undeclared identifier 'abc'; did 
you mean 'abcc'?}}
+}
+}
+
 // PR 23285. This test must be at the end of the file to avoid additional,
 // unwanted diagnostics.
 // expected-error-re@+2 {{use of undeclared identifier 'uintmax_t'{{$
Index: clang/test/AST/ast-dump-recovery.cpp
===
--- clang/test/AST/ast-dump-recovery.cpp
+++ clang/test/AST/ast-dump-recovery.cpp
@@ -271,3 +271,14 @@
   // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 2
   invalid() ? 1 : 2;
 }
+
+void abcc();
+void TypoCorrection() {
+  // RecoveryExpr is always dependent-type in this case in order to suppress
+  // following diagnostics.
+  // CHECK:  RecoveryExpr {{.*}} ''
+  // CHECK-NEXT: `-CallExpr {{.*}} 'void'
+  // CHECK-NEXT:   `-ImplicitCastExpr
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'abcc'
+  abc();
+}
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -12735,8 +12735,6 @@
 }
 
 /// Attempts to recover from a call where no functions were found.
-///
-/// Returns true if new candidates were found.
 static ExprResult
 BuildRecoveryCallExpr(Sema &SemaRef, Scope *S, Expr *Fn,
   UnresolvedLookupExpr *ULE,
@@ -12793,7 +12791,7 @@
 return ExprError();
   }
 
-  // Build an implicit member call if appropriate.  Just drop the
+  // Build an implicit member access expression if appropriate. Just drop the
   // casts and such from the call, we don't really care.
   ExprResult NewFn = ExprError();
   if ((*R.begin())->isCXXClassMember())
@@ -12808,12 +12806,19 @@
   if (NewFn.isInvalid())
 return ExprError();
 
-  // This shouldn't cause an infinite loop because we're giving it
-  // an expression with viable lookup results, which should never
-  // end up here.
-  return SemaRef.BuildCallExpr(/*Scope*/ nullptr, NewFn.get(), LParenLoc,
-   MultiExprArg(Args.data(), Args.size()),
-   RParenLoc);
+  auto CallE =
+  SemaRef.BuildCallExpr(/*Scope*/ nullptr, NewFn.get(), LParenLoc,
+MultiExprArg(Args.data(), Args.size()), RParenLoc);
+  if (CallE.isInvalid())
+return ExprError();
+  // We now have recovered a callee. However, building a real call may lead to
+  // incorrect secondary diagnostics if our recovery wasn't correct.
+  // We keep the recovery behavior but suppress all following diagnostics by
+  // using RecoveryExpr. We deliberately drop the return type of the recovery
+  // function, and rely on clang's dependent mechanism to suppress following
+  // diagnostics.
+  return SemaRef.CreateRecoveryExpr(CallE.get()->getBeginLoc(),
+CallE.get()->getEndLoc(), {CallE.get()});
 }
 
 /// Constructs and populates an OverloadedCandidateSet from


Index: clang/test/SemaCXX/typo-correction-delayed.cpp
===
--- clang/test/SemaCXX/typo-correction-delayed.cpp
+++ clang/test/SemaCXX/typo-correction-delayed.cpp
@@ -209,6 +209,15 @@
 // expected-error-re@-1 {{use of undeclared identifier 'N'{{$
 }
 
+namespace noSecondaryDiags {
+void abcc(); // expected-note {{'abcc' declared here}}
+
+void test() {
+  // Verify the secondary diagnostic ".. convertible to 'bool'" is suppressed.
+  if (abc()) {} // expected-error {{use of undeclared identifier 'abc'; did you mean 'abcc'?}}
+}
+}
+
 // PR 23285. This test must be at the end of the file to avoid additional,
 // unwanted diagnostics.
 // expected-error-re@+2 {{use of undeclared identifier 'uintmax_t'{{$
Index: clang/test/AST/ast-dump-recovery.cpp
===
--- clang/test/AST/ast-dump-recovery.cpp
+++ clang/test/AST/ast-dump-recovery.cpp
@@ -271,3 +271,14 @@
   // CHECK-NEXT:

[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

2020-10-26 Thread Kirill Dmitrenko via Phabricator via cfe-commits

dmikis added a comment.

> If this usage leads to crashes, isn't the issue in DiagnosticsEngine itself?

I don't know design intent behind `DiagnosticsEngine`. As far as I can 
understand from source code it doesn't try to create any consumers by itself 
yet requires one to be provided (there're asserts that client is not 
`nullptr`). Reasonable thing to do may be to remove default for `client` 
parameter of `DiagnosticsEngine` constructor? I'm strugling to build whole LLVM 
under MSVC 19.27.29112 due to some weird problems with STL. I'll check later 
whether it's feasable to simply remove default.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90121

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

[clang] efa9aaa - [clang] Suppress "follow-up" diagnostics on recovery call expressions.

2020-10-26 Thread Haojian Wu via cfe-commits


Author: Haojian Wu
Date: 2020-10-26T12:40:00+01:00
New Revision: efa9aaad703e6b150980ed1a74b4e7c9da7d85a2

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

LOG: [clang] Suppress "follow-up" diagnostics on recovery call expressions.

Because of typo-correction, the AST can be transformed, and the transformed
AST is marginally useful for diagnostics purpose, the following
diagnostics usually do harm than good (easily cause confusions).

Given the following code:

```
void abcc();
void test() {
  if (abc());
  // diagnostic 1 (for the typo-correction): the typo is correct to `abcc()`, 
so the code is treate as `if (abcc())` in AST perspective;
  // diagnostic 2 (for mismatch type): we perform an type-analysis on `if`, 
discover the type is not match
}
```

The secondary diagnostic "convertable to bool" is likely bogus to users.

The idea is to use RecoveryExpr (clang's dependent mechanism) to preserve the
recovery behavior but suppress all follow-up diagnostics.

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

Added: 


Modified: 
clang/lib/Sema/SemaOverload.cpp
clang/test/AST/ast-dump-recovery.cpp
clang/test/SemaCXX/typo-correction-delayed.cpp

Removed: 




diff  --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index d7b985a7b329..ae106ff4557a 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -12735,8 +12735,6 @@ class BuildRecoveryCallExprRAII {
 }
 
 /// Attempts to recover from a call where no functions were found.
-///
-/// Returns true if new candidates were found.
 static ExprResult
 BuildRecoveryCallExpr(Sema &SemaRef, Scope *S, Expr *Fn,
   UnresolvedLookupExpr *ULE,
@@ -12793,7 +12791,7 @@ BuildRecoveryCallExpr(Sema &SemaRef, Scope *S, Expr *Fn,
 return ExprError();
   }
 
-  // Build an implicit member call if appropriate.  Just drop the
+  // Build an implicit member access expression if appropriate. Just drop the
   // casts and such from the call, we don't really care.
   ExprResult NewFn = ExprError();
   if ((*R.begin())->isCXXClassMember())
@@ -12808,12 +12806,19 @@ BuildRecoveryCallExpr(Sema &SemaRef, Scope *S, Expr 
*Fn,
   if (NewFn.isInvalid())
 return ExprError();
 
-  // This shouldn't cause an infinite loop because we're giving it
-  // an expression with viable lookup results, which should never
-  // end up here.
-  return SemaRef.BuildCallExpr(/*Scope*/ nullptr, NewFn.get(), LParenLoc,
-   MultiExprArg(Args.data(), Args.size()),
-   RParenLoc);
+  auto CallE =
+  SemaRef.BuildCallExpr(/*Scope*/ nullptr, NewFn.get(), LParenLoc,
+MultiExprArg(Args.data(), Args.size()), RParenLoc);
+  if (CallE.isInvalid())
+return ExprError();
+  // We now have recovered a callee. However, building a real call may lead to
+  // incorrect secondary diagnostics if our recovery wasn't correct.
+  // We keep the recovery behavior but suppress all following diagnostics by
+  // using RecoveryExpr. We deliberately drop the return type of the recovery
+  // function, and rely on clang's dependent mechanism to suppress following
+  // diagnostics.
+  return SemaRef.CreateRecoveryExpr(CallE.get()->getBeginLoc(),
+CallE.get()->getEndLoc(), {CallE.get()});
 }
 
 /// Constructs and populates an OverloadedCandidateSet from

diff  --git a/clang/test/AST/ast-dump-recovery.cpp 
b/clang/test/AST/ast-dump-recovery.cpp
index 69d5f80427cb..366b3bfd9e07 100644
--- a/clang/test/AST/ast-dump-recovery.cpp
+++ b/clang/test/AST/ast-dump-recovery.cpp
@@ -271,3 +271,14 @@ void InvalidCondition() {
   // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 2
   invalid() ? 1 : 2;
 }
+
+void abcc();
+void TypoCorrection() {
+  // RecoveryExpr is always dependent-type in this case in order to suppress
+  // following diagnostics.
+  // CHECK:  RecoveryExpr {{.*}} ''
+  // CHECK-NEXT: `-CallExpr {{.*}} 'void'
+  // CHECK-NEXT:   `-ImplicitCastExpr
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'abcc'
+  abc();
+}

diff  --git a/clang/test/SemaCXX/typo-correction-delayed.cpp 
b/clang/test/SemaCXX/typo-correction-delayed.cpp
index 66d19daf66fd..aa136a08be4f 100644
--- a/clang/test/SemaCXX/typo-correction-delayed.cpp
+++ b/clang/test/SemaCXX/typo-correction-delayed.cpp
@@ -209,6 +209,15 @@ int z = 1 ? N : ;  // expected-error {{expected 
expression}}
 // expected-error-re@-1 {{use of undeclared identifier 'N'{{$
 }
 
+namespace noSecondaryDiags {
+void abcc(); // expected-note {{'abcc' declared here}}
+
+void test() {
+  // Verify the secondary diagnostic ".. convertible to 'bool'" is suppressed.
+  if (abc()) {} // expected-error {{use of undeclared identifier 'abc'; did 
you mean 'a

[PATCH] D80531: [clang-tidy]: Added modernize-replace-disallow-copy-and-assign-macro

2020-10-26 Thread Nathan James via Phabricator via cfe-commits

njames93 added a comment.

In D80531#2352944 , @kkleine wrote:

> @dblaikie sorry for the late feedback. The `LLVM_ENABLE_WERROR:BOOL` will 
> "Stop and fail the build, if a compiler warning is triggered. Defaults to 
> OFF." I wonder if any other test fails from clang tidy because. My test 
> explicitly checks that a warning is issued (e.g. `// CHECK-MESSAGES-DEFAULT: 
> :[[@LINE-2]]:3: warning: prefer deleting`) and when the `LLVM_ENABLE_WERROR` 
> propagates to this piece, it will indeed fail the build. But I wonder why 
> this doesn't happen to the other clang-tidy checks. @njames93  
> @Eugene.Zelenko any ideas?

The reason WERROR causes this to fail is the tests are generating a warning 
`Wextra-semi` for the `;` that appear after each macro usage in the class. 
clang-tidy would ignore this diagnostic as its set-up to ignore all warnings 
from clang.
However when Werror is set, that warning is promoted to an error, and 
clang-tidy will emit all errors, regardless of their origin.
This causes there to be extra diagnostics in the tests, though only in the 
tests where you invoke clang-tidy directly. Not sure why that obeys 
`LLVM_ENABLE_WERROR` likewise not sure why check_clang_tidy doesn't obey it.
Anyway the simple fix is to just remove the `;` after the macro usages in the 
class

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80531

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

[PATCH] D80531: [clang-tidy]: Added modernize-replace-disallow-copy-and-assign-macro

2020-10-26 Thread Konrad Kleine via Phabricator via cfe-commits

kkleine added a comment.

@njames93 I could do that but the original Macros had were defined without a 
semicolon at the end and one had to add it manually. See this revision in which 
I replaced some occurrences of `DISALLOW_COPY_AND_ASSIGN`: 
eaebcbc67926a18befaa297f1778edde63baec9b 
. What do 
you suggest? Keep the semicolon to more closely match the original macros or 
remove it to make the test happy?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80531

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

[clang] e499186 - [clang-fuzzer] CreateAndRunJITFunc - fix use after move static analyzer warning.

2020-10-26 Thread Simon Pilgrim via cfe-commits


Author: Simon Pilgrim
Date: 2020-10-26T12:24:18Z
New Revision: e4991867fb5ace434640bfacfd28720ad031d33c

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

LOG: [clang-fuzzer] CreateAndRunJITFunc - fix use after move static analyzer 
warning.

We were using the unique_ptr M to determine the triple after it had been moved 
in the EngineBuilder constructor.

Added: 


Modified: 
clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp

Removed: 




diff  --git a/clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp 
b/clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp
index d57515b1ac13..17ce40b5b1b6 100644
--- a/clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp
+++ b/clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp
@@ -158,6 +158,8 @@ static void CreateAndRunJITFunc(const std::string &IR, 
CodeGenOpt::Level OLvl) {
 ErrorAndExit("Function not found in module");
 
   std::string ErrorMsg;
+  Triple ModuleTriple(M->getTargetTriple());
+
   EngineBuilder builder(std::move(M));
   builder.setMArch(codegen::getMArch());
   builder.setMCPU(codegen::getCPUStr());
@@ -166,8 +168,6 @@ static void CreateAndRunJITFunc(const std::string &IR, 
CodeGenOpt::Level OLvl) {
   builder.setEngineKind(EngineKind::JIT);
   builder.setMCJITMemoryManager(std::make_unique());
   builder.setOptLevel(OLvl);
-
-  Triple ModuleTriple(M->getTargetTriple());
   builder.setTargetOptions(
   codegen::InitTargetOptionsFromCodeGenFlags(ModuleTriple));
 



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

[PATCH] D80531: [clang-tidy]: Added modernize-replace-disallow-copy-and-assign-macro

2020-10-26 Thread Nathan James via Phabricator via cfe-commits

njames93 added a comment.

In D80531#2353314 , @kkleine wrote:

> @njames93 I could do that but the original Macros had were defined without a 
> semicolon at the end and one had to add it manually. See this revision in 
> which I replaced some occurrences of `DISALLOW_COPY_AND_ASSIGN`: 
> eaebcbc67926a18befaa297f1778edde63baec9b 
> . What 
> do you suggest? Keep the semicolon to more closely match the original macros 
> or remove it to make the test happy?

Or add `-Wno-extra-semi` in the tests where you invoke clang-tidy directly, 
That'll silence the warnings and leave the rest of the test as-is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80531

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

[PATCH] D90009: [X86] VEX/EVEX prefix doesn't work for inline assembly.

2020-10-26 Thread Kiran Chandramohan via Phabricator via cfe-commits

kiranchandramohan added a comment.

This change probably requires the X86 target.
// REQUIRES: x86-registered-target

Builds which target AArch64 only have been failing due to this change.
http://lab.llvm.org:8011/#/builders/32/builds/291


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90009

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

[PATCH] D33029: [clang-format] add option for dangling parenthesis

2020-10-26 Thread Andrew Somerville via Phabricator via cfe-commits

catskul added a comment.

@MyDeveloperDay I'm going to upload a re-based version of this. Should I rebase 
it off the top of master? Tip of 11? and/or create a new diff/review for each?


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

https://reviews.llvm.org/D33029

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

[PATCH] D33029: [clang-format] add option for dangling parenthesis

2020-10-26 Thread Andrew Somerville via Phabricator via cfe-commits

catskul added inline comments.



Comment at: include/clang/Format/Format.h:793
+  /// \endcode
+  bool DanglingParenthesis;
+

stringham wrote:
> djasper wrote:
> > stringham wrote:
> > > djasper wrote:
> > > > I don't think this is a name that anyone will intuitively understand. I 
> > > > understand that the naming is hard here. One thing I am wondering is 
> > > > whether this might ever make sense unless AlignAfterOpenBracket is set 
> > > > to AlwaysBreak?
> > > > 
> > > > Unless that option is set, we could have both in the same file:
> > > > 
> > > >   someFunction(,
> > > >);
> > > > 
> > > > and
> > > > 
> > > >   someFunction(
> > > >   , 
> > > >   );
> > > > 
> > > > Is that intended, i.e. are you actively using that? Answering this is 
> > > > important, because it influence whether or not we actually need to add 
> > > > another style option and even how to implement this.
> > > The name was based on the configuration option that scalafmt has for 
> > > their automatic scala formatter, they also have an option to have the 
> > > closing paren on its own line and they call it `danglingParentheses`. I 
> > > don't love the name and am open to other options.
> > > 
> > > That's a good point about AlignAfterOpenBracket being set to AlwaysBreak. 
> > > In our usage we have that option set, and I'm also unsure if it makes 
> > > sense without AlwaysBreak.
> > Hm. I am not sure either. What do you think of extending the 
> > BracketAlignmentStyle enum with an AlwaysBreakWithDanglingParenthesis? Or 
> > AlwaysBreakAndWrapRParen?
> Sorry for the delay in the reply!
> 
> That seems like a reasonable solution to me. I believe the structure of the 
> patch would be the same, just changing out the configuration option.
> 
> Can you point me to where I should look to figure out how to run the unit 
> tests and let me know if there are other changes you would recommend besides 
> updating configuration options?
@djasper I made the changes to @stringham's diff to implement your request to 
replace the `bool` with new item of `BracketAlignmentStyle` `enum`, and 
wondered if it might be backing us into a corner. 

As strange as some of these may be I've seen a few similar to:

```
return_t myfunc(int x,
int y,
int z
);
```
```
return_t myfunc(int x,
int somelongy,
int z );
```
```
return_t myfunc(
 int x,
 int y,
 int z
 );
```
and even my least favorite
```
return_t myfunc(
int x,
int y,
int z
);
```

Without advocating for supporting all such styles it would seem desireable to 
avoid an NxM enum of two, at least theoretically, independent style parameters. 
With that in mind should I instead create a different parameter 
`AlignClosingBracket` with a respective `enum` which includes 
`AfterFinalParameter` by default, and `NextLineWhenMultiline` to handle the 
variations this diff was opened for?

On the other hand, I can just stick with adding a new variation to 
`BracketAlignmentStyle` and deal with potential variations in the future if 
they're requested.



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

https://reviews.llvm.org/D33029

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

[PATCH] D88913: [FPEnv] Use strictfp metadata in casting nodes

2020-10-26 Thread Serge Pavlov via Phabricator via cfe-commits

sepavloff added a comment.

Generally the patch looks good. But the need to expect incorrect values in 
tests is a concern. Maybe this is a consequence of storing exception behavior 
in a separate field of CGFPOptionsRAII. This misbehavior should be fixed.




Comment at: clang/test/CodeGen/aarch64-v8.2a-neon-intrinsics-constrained.c:26
+// metadata from the AST instead of the global default from the command line.
+// FIXME: All cases of "fpexcept.maytrap" in this test are wrong.
+

kpn wrote:
> kpn wrote:
> > sepavloff wrote:
> > > kpn wrote:
> > > > sepavloff wrote:
> > > > > Why they are wrong?
> > > > Because the #pragma covers the entire file and sets exception handling 
> > > > to "strict". Thus all constrained intrinsic calls should be "strict", 
> > > > and if they are "maytrap" or "ignore" then we have a bug.
> > > What is the reason for that? Does `#pragma float_control` work 
> > > incorrectly? Why  in `clang/test/CodeGen/complex-math-strictfp.c` 
> > > exception handling is correct?
> > The #pragma works just fine. The problem is that we need to get the 
> > strictfp bits from the AST to the IRBuilder, and we haven't finished 
> > implementing that. So sometimes it works, like in complex-math-strictfp.c, 
> > and sometimes it doesn't. As you can see in the tests in this patch.
> I forgot to mention that complex-math-strictfp.c gets a correct BinOpInfo 
> passed down to get the IRBuilder set correctly. But there are other places 
> that don't correctly set BinOpInfo and so we get inconsistent behavior 
> despite the call to the IRBuilder being wrapped in FPOptsRAII. And _that's_ 
> why I structured this patch the way I did.
> The problem is that we need to get the strictfp bits from the AST to the 
> IRBuilder

What is the status of this problem? Is it on track?


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

https://reviews.llvm.org/D88913

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

[clang-tools-extra] 58d0ef2 - [clangd] Fix remote index build failures due to lack of proto dependency

2020-10-26 Thread Kirill Bobyrev via cfe-commits


Author: Kirill Bobyrev
Date: 2020-10-26T14:14:47+01:00
New Revision: 58d0ef2d0466a893ab400f6a9829057b9d851038

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

LOG: [clangd] Fix remote index build failures due to lack of proto dependency

Previous attempt (15f6bad6d74a993e366c8fc93a9c91f213ac6bc3) introduced
add_dependencies but unfortunately it does not actually add a dependency
between RemoteIndexProto and RemoteIndexServiceProto. This is likely due
to some requirements of it that clang_add_library violates.

As a workaround, we will link RemoteIndexProto library to
RemoteIndexServiceProto which is logical because the library can not be
without linking to RemoteIndexProto anyway.

Added: 


Modified: 
clang-tools-extra/clangd/index/remote/CMakeLists.txt

Removed: 




diff  --git a/clang-tools-extra/clangd/index/remote/CMakeLists.txt 
b/clang-tools-extra/clangd/index/remote/CMakeLists.txt
index e3782d9701c7..5a9f49a41d1f 100644
--- a/clang-tools-extra/clangd/index/remote/CMakeLists.txt
+++ b/clang-tools-extra/clangd/index/remote/CMakeLists.txt
@@ -1,7 +1,11 @@
 if (CLANGD_ENABLE_REMOTE)
   generate_protos(RemoteIndexServiceProto "Service.proto" GRPC)
   generate_protos(RemoteIndexProto "Index.proto")
-  add_dependencies(RemoteIndexServiceProto RemoteIndexProto)
+  target_link_libraries(RemoteIndexServiceProto
+
+PRIVATE
+RemoteIndexProto
+)
   include_directories(${CMAKE_CURRENT_BINARY_DIR})
   include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../../)
 



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

[PATCH] D80961: Ignore template instantiations if not in AsIs mode

2020-10-26 Thread Stephen Kelly via Phabricator via cfe-commits

steveire added a comment.

Many of the changes which were part of a previous iteration of this change were 
related to the change of default behavior of matchers. As the default is no 
longer changed, those changes fell away.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80961

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

[PATCH] D69844: [clang][Basic] Integrate SourceLocation with FoldingSet, NFCI

2020-10-26 Thread Mikhail Maltsev via Phabricator via cfe-commits

miyuki added inline comments.



Comment at: clang/lib/Analysis/PathDiagnostic.cpp:1088
+  ID.Add(Range.getEnd());
+  ID.Add(static_cast(Loc));
 }

dexonsmith wrote:
> I'm surprised you need this `static_cast`, can you explain why it was 
> necessary?
`Loc` is an object of type `FullSourceLoc`, a class derived from 
`SourceLocation`. When the `FoldingSetNodeID::Add` template method is called 
with `Loc` it tries to instantiate `FoldingSetTrait` and fails 
(because there is no explicit specialization for `FullSourceLoc` and the 
default one relies on the method `FullSourceLoc::Profile` which does not 
exist). `FullSourceLoc` does not contain any additional information about the 
location itself (it just holds a pointer to the associated `SourceManager`), so 
there is no need to specialize `FoldingSetTrait` for it, and casting to 
`SourceLocation` will make `FoldingSetNodeID::Add` use the correct 
`FoldingSetTrait` specialization.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69844

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

[PATCH] D90127: [clang][NFC] Rearrange Comment Token and Lexer fields to reduce padding

2020-10-26 Thread Dmitri Gribenko via Phabricator via cfe-commits

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



Comment at: clang/include/clang/AST/CommentLexer.h:74
   /// contains the length of the string that starts at TextPtr.
   unsigned IntVal;
 

Could you also swap Length and IntVal? It would be nice to keep Length and 
TextPtr next to each other.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90127

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

[PATCH] D90116: [clangd] Escape Unicode characters to fix Windows builds

2020-10-26 Thread Kirill Bobyrev via Phabricator via cfe-commits

kbobyrev added a comment.

Hmm, I see. From the looks of it, the solution for several projects would be

  add_compile_options("$<$:/utf-8>")
  add_compile_options("$<$:/utf-8>")

But I'm not sure if it makes sense in our case and I don't see many 
`add_compile_options` in LLVM. Also, I don't have a way to test it out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90116

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

[PATCH] D89935: [clangd] NFC: Update FIXME comment regarding lack of c/dtor support

2020-10-26 Thread Kirill Bobyrev via Phabricator via cfe-commits

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

Only drop the bits about "they're all considered methods", update XRefs.cpp.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89935

Files:
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/XRefs.cpp


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -1284,8 +1284,8 @@
   SM, Lexer::getLocForEndOfToken(NameLoc, 0, SM, Ctx.getLangOpts()));
 
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
-  // FIXME: this is not classifying constructors, destructors and operators
-  //correctly (they're all "methods").
+  // FIXME: This is not classifying constructors, destructors and operators
+  // correctly.
   SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
   TypeHierarchyItem THI;
Index: clang-tools-extra/clangd/FindSymbols.cpp
===
--- clang-tools-extra/clangd/FindSymbols.cpp
+++ clang-tools-extra/clangd/FindSymbols.cpp
@@ -179,8 +179,8 @@
 return llvm::None;
 
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
-  // FIXME: this is not classifying constructors, destructors and operators
-  //correctly (they're all "methods").
+  // FIXME: This is not classifying constructors, destructors and operators
+  // correctly.
   SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
   DocumentSymbol SI;


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -1284,8 +1284,8 @@
   SM, Lexer::getLocForEndOfToken(NameLoc, 0, SM, Ctx.getLangOpts()));
 
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
-  // FIXME: this is not classifying constructors, destructors and operators
-  //correctly (they're all "methods").
+  // FIXME: This is not classifying constructors, destructors and operators
+  // correctly.
   SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
   TypeHierarchyItem THI;
Index: clang-tools-extra/clangd/FindSymbols.cpp
===
--- clang-tools-extra/clangd/FindSymbols.cpp
+++ clang-tools-extra/clangd/FindSymbols.cpp
@@ -179,8 +179,8 @@
 return llvm::None;
 
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
-  // FIXME: this is not classifying constructors, destructors and operators
-  //correctly (they're all "methods").
+  // FIXME: This is not classifying constructors, destructors and operators
+  // correctly.
   SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
   DocumentSymbol SI;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89935: [clangd] NFC: Update FIXME comment regarding lack of c/dtor support

2020-10-26 Thread Kirill Bobyrev via Phabricator via cfe-commits

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

Rebase on top of master.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89935

Files:
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/XRefs.cpp


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -1284,8 +1284,8 @@
   SM, Lexer::getLocForEndOfToken(NameLoc, 0, SM, Ctx.getLangOpts()));
 
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
-  // FIXME: this is not classifying constructors, destructors and operators
-  //correctly (they're all "methods").
+  // FIXME: This is not classifying constructors, destructors and operators
+  // correctly.
   SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
   TypeHierarchyItem THI;
Index: clang-tools-extra/clangd/FindSymbols.cpp
===
--- clang-tools-extra/clangd/FindSymbols.cpp
+++ clang-tools-extra/clangd/FindSymbols.cpp
@@ -179,8 +179,8 @@
 return llvm::None;
 
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
-  // FIXME: this is not classifying constructors, destructors and operators
-  //correctly (they're all "methods").
+  // FIXME: This is not classifying constructors, destructors and operators
+  // correctly.
   SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
   DocumentSymbol SI;


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -1284,8 +1284,8 @@
   SM, Lexer::getLocForEndOfToken(NameLoc, 0, SM, Ctx.getLangOpts()));
 
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
-  // FIXME: this is not classifying constructors, destructors and operators
-  //correctly (they're all "methods").
+  // FIXME: This is not classifying constructors, destructors and operators
+  // correctly.
   SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
   TypeHierarchyItem THI;
Index: clang-tools-extra/clangd/FindSymbols.cpp
===
--- clang-tools-extra/clangd/FindSymbols.cpp
+++ clang-tools-extra/clangd/FindSymbols.cpp
@@ -179,8 +179,8 @@
 return llvm::None;
 
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
-  // FIXME: this is not classifying constructors, destructors and operators
-  //correctly (they're all "methods").
+  // FIXME: This is not classifying constructors, destructors and operators
+  // correctly.
   SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
   DocumentSymbol SI;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] 1704704 - [clangd] NFC: Update FIXME comment regarding lack of c/dtor support

2020-10-26 Thread Kirill Bobyrev via cfe-commits


Author: Kirill Bobyrev
Date: 2020-10-26T15:31:59+01:00
New Revision: 1704704e762f232e827849ee881ebe74b5db7ef1

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

LOG: [clangd] NFC: Update FIXME comment regarding lack of c/dtor support

Both `SymbolKind` and `indexSymbolKindToSymbolKind` support constructors and
separate them into a different category from regular methods.

Reviewed By: kadircet

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

Added: 


Modified: 
clang-tools-extra/clangd/FindSymbols.cpp
clang-tools-extra/clangd/XRefs.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/FindSymbols.cpp 
b/clang-tools-extra/clangd/FindSymbols.cpp
index abd03ecb0464..d6908f7ab5fb 100644
--- a/clang-tools-extra/clangd/FindSymbols.cpp
+++ b/clang-tools-extra/clangd/FindSymbols.cpp
@@ -179,8 +179,8 @@ llvm::Optional declToSym(ASTContext &Ctx, 
const NamedDecl &ND) {
 return llvm::None;
 
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
-  // FIXME: this is not classifying constructors, destructors and operators
-  //correctly (they're all "methods").
+  // FIXME: This is not classifying constructors, destructors and operators
+  // correctly.
   SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
   DocumentSymbol SI;

diff  --git a/clang-tools-extra/clangd/XRefs.cpp 
b/clang-tools-extra/clangd/XRefs.cpp
index 9469ab46c9fc..4c049803a5f9 100644
--- a/clang-tools-extra/clangd/XRefs.cpp
+++ b/clang-tools-extra/clangd/XRefs.cpp
@@ -1284,8 +1284,8 @@ declToTypeHierarchyItem(ASTContext &Ctx, const NamedDecl 
&ND) {
   SM, Lexer::getLocForEndOfToken(NameLoc, 0, SM, Ctx.getLangOpts()));
 
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
-  // FIXME: this is not classifying constructors, destructors and operators
-  //correctly (they're all "methods").
+  // FIXME: This is not classifying constructors, destructors and operators
+  // correctly.
   SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
   TypeHierarchyItem THI;



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

[PATCH] D89935: [clangd] NFC: Update FIXME comment regarding lack of c/dtor support

2020-10-26 Thread Kirill Bobyrev via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG1704704e762f: [clangd] NFC: Update FIXME comment regarding 
lack of c/dtor support (authored by kbobyrev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89935

Files:
  clang-tools-extra/clangd/FindSymbols.cpp
  clang-tools-extra/clangd/XRefs.cpp


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -1284,8 +1284,8 @@
   SM, Lexer::getLocForEndOfToken(NameLoc, 0, SM, Ctx.getLangOpts()));
 
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
-  // FIXME: this is not classifying constructors, destructors and operators
-  //correctly (they're all "methods").
+  // FIXME: This is not classifying constructors, destructors and operators
+  // correctly.
   SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
   TypeHierarchyItem THI;
Index: clang-tools-extra/clangd/FindSymbols.cpp
===
--- clang-tools-extra/clangd/FindSymbols.cpp
+++ clang-tools-extra/clangd/FindSymbols.cpp
@@ -179,8 +179,8 @@
 return llvm::None;
 
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
-  // FIXME: this is not classifying constructors, destructors and operators
-  //correctly (they're all "methods").
+  // FIXME: This is not classifying constructors, destructors and operators
+  // correctly.
   SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
   DocumentSymbol SI;


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -1284,8 +1284,8 @@
   SM, Lexer::getLocForEndOfToken(NameLoc, 0, SM, Ctx.getLangOpts()));
 
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
-  // FIXME: this is not classifying constructors, destructors and operators
-  //correctly (they're all "methods").
+  // FIXME: This is not classifying constructors, destructors and operators
+  // correctly.
   SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
   TypeHierarchyItem THI;
Index: clang-tools-extra/clangd/FindSymbols.cpp
===
--- clang-tools-extra/clangd/FindSymbols.cpp
+++ clang-tools-extra/clangd/FindSymbols.cpp
@@ -179,8 +179,8 @@
 return llvm::None;
 
   index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
-  // FIXME: this is not classifying constructors, destructors and operators
-  //correctly (they're all "methods").
+  // FIXME: This is not classifying constructors, destructors and operators
+  // correctly.
   SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
 
   DocumentSymbol SI;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90140: [clang][RecoveryExpr] Add tests for ObjectiveC.

2020-10-26 Thread Sam McCall via Phabricator via cfe-commits

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



Comment at: clang/test/AST/ast-dump-recovery.m:8
+void k(Foo *foo) {
+  // CHECK:   ObjCMessageExpr {{.*}} 'void' contains-errors
+  // CHECK-CHECK:  |-ImplicitCastExpr {{.*}} 'Foo *' 

this contains the resolved selector so go-to-def on `method` will work, right? 
Really cool :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90140

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

[PATCH] D89946: [clang] Suppress "follow-up" diagnostics on recovery call expressions.

2020-10-26 Thread Sam McCall via Phabricator via cfe-commits

sammccall added a comment.

Still LG thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89946

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

[PATCH] D89986: [AIX] do not emit visibility attribute into IR when there is -mignore-xcoff-visibility

2020-10-26 Thread Jason Liu via Phabricator via cfe-commits

jasonliu added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:520
   Options.DataSections = CodeGenOpts.DataSections;
-  Options.IgnoreXCOFFVisibility = CodeGenOpts.IgnoreXCOFFVisibility;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;

Instead of just removing this line, should this get replaced with the new 
LangOpts option?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986

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

[PATCH] D90023: [Syntax] Add iterators over children of syntax trees.

2020-10-26 Thread Sam McCall via Phabricator via cfe-commits

sammccall updated this revision to Diff 300674.
sammccall marked an inline comment as done.
sammccall added a comment.

Style changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90023

Files:
  clang/include/clang/Tooling/Syntax/Tree.h
  clang/lib/Tooling/Syntax/Tree.cpp
  clang/unittests/Tooling/Syntax/TreeTest.cpp
  clang/unittests/Tooling/Syntax/TreeTestBase.h

Index: clang/unittests/Tooling/Syntax/TreeTestBase.h
===
--- clang/unittests/Tooling/Syntax/TreeTestBase.h
+++ clang/unittests/Tooling/Syntax/TreeTestBase.h
@@ -20,7 +20,9 @@
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "clang/Tooling/Syntax/Tree.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Testing/Support/Annotations.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
@@ -53,6 +55,14 @@
 };
 
 std::vector allTestClangConfigs();
+
+MATCHER_P(role, R, "") {
+  if (arg.getRole() == R)
+return true;
+  *result_listener << "role is " << llvm::to_string(arg.getRole());
+  return false;
+}
+
 } // namespace syntax
 } // namespace clang
 #endif // LLVM_CLANG_UNITTESTS_TOOLING_SYNTAX_TREETESTBASE_H
Index: clang/unittests/Tooling/Syntax/TreeTest.cpp
===
--- clang/unittests/Tooling/Syntax/TreeTest.cpp
+++ clang/unittests/Tooling/Syntax/TreeTest.cpp
@@ -8,6 +8,7 @@
 
 #include "clang/Tooling/Syntax/Tree.h"
 #include "TreeTestBase.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Syntax/BuildTree.h"
 #include "clang/Tooling/Syntax/Nodes.h"
 #include "llvm/ADT/STLExtras.h"
@@ -17,6 +18,7 @@
 using namespace clang::syntax;
 
 namespace {
+using testing::ElementsAre;
 
 class TreeTest : public SyntaxTreeTest {
 private:
@@ -124,6 +126,56 @@
   }
 }
 
+TEST_F(TreeTest, Iterators) {
+  buildTree("", allTestClangConfigs().front());
+  std::vector Children = {createLeaf(*Arena, tok::identifier, "a"),
+  createLeaf(*Arena, tok::identifier, "b"),
+  createLeaf(*Arena, tok::identifier, "c")};
+  auto *Tree = syntax::createTree(*Arena,
+  {{Children[0], NodeRole::LeftHandSide},
+   {Children[1], NodeRole::OperatorToken},
+   {Children[2], NodeRole::RightHandSide}},
+  NodeKind::TranslationUnit);
+  const auto *ConstTree = Tree;
+
+  auto Range = Tree->getChildren();
+  EXPECT_THAT(Range, ElementsAre(role(NodeRole::LeftHandSide),
+ role(NodeRole::OperatorToken),
+ role(NodeRole::RightHandSide)));
+
+  auto ConstRange = ConstTree->getChildren();
+  EXPECT_THAT(ConstRange, ElementsAre(role(NodeRole::LeftHandSide),
+  role(NodeRole::OperatorToken),
+  role(NodeRole::RightHandSide)));
+
+  // FIXME: mutate and observe no invalidation. Mutations are private for now...
+  auto It = Range.begin();
+  auto CIt = ConstRange.begin();
+  static_assert(std::is_same::value,
+"mutable range");
+  static_assert(std::is_same::value,
+"const range");
+
+  for (unsigned I = 0; I < 3; ++I) {
+EXPECT_EQ(It, CIt);
+EXPECT_TRUE(It);
+EXPECT_TRUE(CIt);
+EXPECT_EQ(It.asPointer(), Children[I]);
+EXPECT_EQ(CIt.asPointer(), Children[I]);
+EXPECT_EQ(&*It, Children[I]);
+EXPECT_EQ(&*CIt, Children[I]);
+++It;
+++CIt;
+  }
+  EXPECT_EQ(It, CIt);
+  EXPECT_EQ(It, Tree::ChildIterator());
+  EXPECT_EQ(CIt, Tree::ConstChildIterator());
+  EXPECT_FALSE(It);
+  EXPECT_FALSE(CIt);
+  EXPECT_EQ(nullptr, It.asPointer());
+  EXPECT_EQ(nullptr, CIt.asPointer());
+}
+
 class ListTest : public SyntaxTreeTest {
 private:
   std::string dumpQuotedTokensOrNull(const Node *N) {
Index: clang/lib/Tooling/Syntax/Tree.cpp
===
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -19,8 +19,8 @@
 static void traverse(const syntax::Node *N,
  llvm::function_ref Visit) {
   if (auto *T = dyn_cast(N)) {
-for (const auto *C = T->getFirstChild(); C; C = C->getNextSibling())
-  traverse(C, Visit);
+for (const syntax::Node &C : T->getChildren())
+  traverse(&C, Visit);
   }
   Visit(N);
 }
@@ -194,21 +194,21 @@
   DumpExtraInfo(N);
   OS << "\n";
 
-  for (const auto *It = T->getFirstChild(); It; It = It->getNextSibling()) {
+  for (const syntax::Node &It : T->getChildren()) {
 for (bool Filled : IndentMask) {
   if (Filled)
 OS << "| ";
   else
 OS << "  ";
 }
-if (!It->getNextSibling()) {
+if (!It.getNextSibling()) {
   OS << "`-";
   IndentMask.push_back(fa

[PATCH] D90023: [Syntax] Add iterators over children of syntax trees.

2020-10-26 Thread Sam McCall via Phabricator via cfe-commits

sammccall added inline comments.

Comment at: clang/include/clang/Tooling/Syntax/Tree.h:157-184
+  /// Iterator over children (common base for const/non-const).
+  /// Not invalidated by tree mutations (holds a stable node pointer).
+  template 
+  class child_iterator_base
+  : public llvm::iterator_facade_base {
+  protected:

eduucaldas wrote:
> I think we should only have an iterator through `Node`s, not a general one 
> like this.
> 
> In general `Tree` has *heterogeneous* children:
> For instance, `1+2` yields the syntax tree:
> ```
> BinaryOperatorExpression
> |-  IntegerLiteralExpression
> |-  Leaf
> `- IntegerLiteralExpression
> ```
> 
> Very rarely will syntax trees have all their children of the same kind, so I 
> don't think it makes sense to try an provide an iterator template. That makes 
> sense for lists , because generally they *have* the same kind. But in that 
> case we provide special iterators for lists only.
> 
This does only iterate over nodes, note this class is private.
This is a common pattern when implementing iterators: you need `iterator` and 
`const_iterator`, they have almost identical implementations but operate on 
different types (`const T` vs `T`). Thus a private template.

Sometimes it's used directly (`using iterator = iterator_base`) but that 
means the template needs to be visible and also leads to worse compiler 
diagnostics when the canonical type is printed. Inheritance lets us prevent 
direct use of the base class and give the iterator classes distinct names.

---

That said, in future, I think we should (later) have a facility to iterate over 
children of a certain type. This would explicitly be a filtering operation, 
ignoring nodes not of that type.
Providing these typed operations for concrete subclasses of List makes sense, 
but in clangd outside of specific refactorings we've found generic facilities 
to often be more useful. When we designed syntax trees, providing a simple and 
generic data structure that could be used directly was an explicit goal.

Comment at: clang/include/clang/Tooling/Syntax/Tree.h:202-219
+  /// child_iterator is not invalidated by mutations.
+  struct child_iterator : child_iterator_base {
+using Base::Base;
+  };
+  struct const_child_iterator
+  : child_iterator_base {
+using Base::Base;

eduucaldas wrote:
> TL;DR:
> `child_iterator` -> `ChildIterator`
> `const_child_iterator` -> `ConstChildIterator`
> `children` -> `getChildren`
> 
> I see you followed the naming convention of the ClangAST, which makes loads 
> of sense. 
> 
> In syntax trees we follow the naming conventions of the [conding 
> standard](https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly)
>  -- just because we want consistency and we had to pick a guideline to follow 
> -- according to that, functions should be verb like and names should be camel 
> case.
> 
> If like us you chose `const_child_iterator` and `children` kind of "just 
> because", could you change it to follow `ConstChildIterator` and 
> `getChildren` respectively. 
> In syntax trees we follow the naming conventions of the conding standard

I see that's been used in some of the newer classes, and that you changed this 
file to follow that style in 4c14ee61b73746b314d83e7c52e03d6527b78105. However 
it's not the original style we used here and elsewhere in libSyntax.
It's a bit frustrating to hear the argument about consistency, because we were 
quite careful and deliberate about this style, which was IMO fairly consistent 
prior to that change.

I'm willing to change these to match the local style in this file. However 
`*_iterator`, `*_begin/end/range()` is more common than FooIterator etc in LLVM 
overall (I think for consistency with `iterator` etc, which is endorsed by the 
style guide). So I don't think this change is particularly an improvement, even 
in terms of consistency.

Comment at: clang/lib/Tooling/Syntax/Tree.cpp:22-23
   if (auto *T = dyn_cast(N)) {
-for (const auto *C = T->getFirstChild(); C; C = C->getNextSibling())
-  traverse(C, Visit);
+for (const syntax::Node &C : T->children())
+  traverse(&C, Visit);
   }

eduucaldas wrote:
> Hmm... 
> That looks funny, if the user uses a range-based for loop and forgets the 
> `&`, then we would be copying the Node???
> 
> Also in many places we had to get the address to the node. That is not really 
> consistent with how the syntax trees API's were designed, they generally take 
> pointers instead of references. (that said we could obviously reconsider 
> those design choices)
> That looks funny, if the user uses a range-based for loop and forgets the &, 
> then we would be copying the Node???

It doesn't look funny to me - foreach usually iterates over values rather than 
pointers. This raises a good point - it looks like Node is copyable and 
shouldn't be. (The

[PATCH] D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way

2020-10-26 Thread Denys Petrov via Phabricator via cfe-commits

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

Refactor `SValBuilder::evalCast` function. Make the function clear and get rid 
of redundant and repetitive code. Unite `SValBuilder::evalCast`, 
`SimpleSValBuilder::dispatchCast`, `SimpleSValBuilder::evalCastFromNonLoc` and 
`SimpleSValBuilder::evalCastFromLoc` functions into single 
`SValBuilder::evalCast`.

P.S. Inspired by D89055 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90157

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -530,108 +530,178 @@
   return evalCast(val, castTy, originalTy);
 }
 
-// FIXME: should rewrite according to the cast kind.
-SVal SValBuilder::evalCast(SVal val, QualType castTy, QualType originalTy) {
-  castTy = Context.getCanonicalType(castTy);
-  originalTy = Context.getCanonicalType(originalTy);
-  if (val.isUnknownOrUndef() || castTy == originalTy)
-return val;
-
-  if (castTy->isBooleanType()) {
-if (val.isUnknownOrUndef())
-  return val;
-if (val.isConstant())
-  return makeTruthVal(!val.isZeroConstant(), castTy);
-if (!Loc::isLocType(originalTy) &&
-!originalTy->isIntegralOrEnumerationType() &&
-!originalTy->isMemberPointerType())
-  return UnknownVal();
-if (SymbolRef Sym = val.getAsSymbol(true)) {
-  BasicValueFactory &BVF = getBasicValueFactory();
-  // FIXME: If we had a state here, we could see if the symbol is known to
-  // be zero, but we don't.
-  return makeNonLoc(Sym, BO_NE, BVF.getValue(0, Sym->getType()), castTy);
-}
-// Loc values are not always true, they could be weakly linked functions.
-if (Optional L = val.getAs())
-  return evalCastFromLoc(*L, castTy);
+//===--===//
+// Cast methods.
+// `evalCast` is the main method
+// `evalCastKind` and `evalCastSubKind` are helpers
+//===--===//
 
-Loc L = val.castAs().getLoc();
-return evalCastFromLoc(L, castTy);
+SVal SValBuilder::evalCast(SVal V, QualType CastTy, QualType OriginalTy) {
+  // FIXME: Move this check to the most appropriate evalCastKind/evalCastSubKind
+  // function.
+  // For const casts, casts to void, just propagate the value.
+  if (!CastTy->isVariableArrayType() && !OriginalTy->isVariableArrayType())
+if (shouldBeModeledWithNoOp(Context, Context.getPointerType(CastTy),
+Context.getPointerType(OriginalTy)))
+  return V;
+
+  // Cast SVal according to kinds.
+  switch (V.getBaseKind()) {
+  case SVal::UndefinedValKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::UnknownValKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::LocKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  case SVal::NonLocKind:
+return evalCastKind(V.castAs(), CastTy, OriginalTy);
+  default:
+llvm_unreachable("Unknown SVal kind");
   }
+}
 
-  // For const casts, casts to void, just propagate the value.
-  if (!castTy->isVariableArrayType() && !originalTy->isVariableArrayType())
-if (shouldBeModeledWithNoOp(Context, Context.getPointerType(castTy),
- Context.getPointerType(originalTy)))
-  return val;
+SVal SValBuilder::evalCastKind(UndefinedVal V, QualType CastTy,
+   QualType OriginalTy) {
+  return V;
+}
 
-  // Check for casts from pointers to integers.
-  if (castTy->isIntegralOrEnumerationType() && Loc::isLocType(originalTy))
-return evalCastFromLoc(val.castAs(), castTy);
-
-  // Check for casts from integers to pointers.
-  if (Loc::isLocType(castTy) && originalTy->isIntegralOrEnumerationType()) {
-if (Optional LV = val.getAs()) {
-  if (const MemRegion *R = LV->getLoc().getAsRegion()) {
-StoreManager &storeMgr = StateMgr.getStoreManager();
-R = storeMgr.castRegion(R, castTy);
-return R ? SVal(loc::MemRegionVal(R)) : UnknownVal();
-  }
-  return LV->getLoc();
-}
-return dispatchCast(val, castTy);
+SVal SValBuilder::evalCastKind(UnknownVal V, QualType CastTy,
+   QualType OriginalTy) {
+  return V;
+}
+
+SVal SValBuilder::evalCastKind(Loc V, QualType CastTy, QualType OriginalTy) {
+  switch (V.getSubKind()) {
+  case loc::ConcreteInt

[PATCH] D89986: [AIX] do not emit visibility attribute into IR when there is -mignore-xcoff-visibility

2020-10-26 Thread Digger via Phabricator via cfe-commits

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



Comment at: clang/lib/CodeGen/BackendUtil.cpp:520
   Options.DataSections = CodeGenOpts.DataSections;
-  Options.IgnoreXCOFFVisibility = CodeGenOpts.IgnoreXCOFFVisibility;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;

jasonliu wrote:
> Instead of just removing this line, should this get replaced with the new 
> LangOpts option?
I do not think we need a CodeGenOp of ignore-xcoff-visibility in clang, we only 
need the LangOpt of the ignore-xcoff-visilbity to control whether we will  
generate the visibility in the IR,  when the LangOpt of ignore-xcoff-visibility 
do not generate the visibility attribute of GV in the IR. it do not need 
CodeGenOp of ignore-xcoff-visibility any more for the clang .

we have still CodeGen ignore-xcoff-visibility op in  llc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986

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

[PATCH] D89055: [analyzer] Wrong type cast occures during pointer dereferencing after type punning

2020-10-26 Thread Denys Petrov via Phabricator via cfe-commits

ASDenysPetrov added a comment.

In D89055#2336709 , @NoQ wrote:

> Ugh, sorry, no, that's `evalCast()`. Like `evalBinOp()` etc. My bad. Can we 
> also use `evalCast()`?

I dived into `evalCast()`. Initially I had to figure it out and rework it to 
find the right place to put the fix in. You are welcome to review D89055 
 as a parent revision.

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

https://reviews.llvm.org/D89055

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

[clang] 32efb81 - [analyzer] [NFC] Simplify SVal::getAsLocSymbol function using existing functions

2020-10-26 Thread Denys Petrov via cfe-commits


Author: Denys Petrov
Date: 2020-10-26T17:00:29+02:00
New Revision: 32efb81ea60a9e99571923bf9308598f6cd341f2

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

LOG: [analyzer] [NFC] Simplify SVal::getAsLocSymbol function using existing 
functions

Summary: Method of obtaining MemRegion from LocAsInteger/MemRegionVal already 
exists in SVal::getAsRegion function. Replace repetitive conditions in 
SVal::getAsLocSymbol with SVal::getAsRegion function.

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

Added: 


Modified: 
clang/lib/StaticAnalyzer/Core/SVals.cpp

Removed: 




diff  --git a/clang/lib/StaticAnalyzer/Core/SVals.cpp 
b/clang/lib/StaticAnalyzer/Core/SVals.cpp
index c4d5377f3bae..252596887e4f 100644
--- a/clang/lib/StaticAnalyzer/Core/SVals.cpp
+++ b/clang/lib/StaticAnalyzer/Core/SVals.cpp
@@ -84,16 +84,12 @@ const FunctionDecl *SVal::getAsFunctionDecl() const {
 /// the first symbolic parent region is returned.
 SymbolRef SVal::getAsLocSymbol(bool IncludeBaseRegions) const {
   // FIXME: should we consider SymbolRef wrapped in CodeTextRegion?
-  if (Optional X = getAs())
-return X->getLoc().getAsLocSymbol(IncludeBaseRegions);
-
-  if (Optional X = getAs()) {
-const MemRegion *R = X->getRegion();
-if (const SymbolicRegion *SymR = IncludeBaseRegions ?
-  R->getSymbolicBase() :
-  
dyn_cast(R->StripCasts()))
+  if (const MemRegion *R = getAsRegion())
+if (const SymbolicRegion *SymR =
+IncludeBaseRegions ? R->getSymbolicBase()
+   : dyn_cast(R->StripCasts()))
   return SymR->getSymbol();
-  }
+
   return nullptr;
 }
 



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

[PATCH] D89982: [analyzer] [NFC] Simplify SVal::getAsLocSymbol function using existing functions

2020-10-26 Thread Denys Petrov via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG32efb81ea60a: [analyzer] [NFC] Simplify SVal::getAsLocSymbol 
function using existing functions (authored by ASDenysPetrov).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89982

Files:
  clang/lib/StaticAnalyzer/Core/SVals.cpp


Index: clang/lib/StaticAnalyzer/Core/SVals.cpp
===
--- clang/lib/StaticAnalyzer/Core/SVals.cpp
+++ clang/lib/StaticAnalyzer/Core/SVals.cpp
@@ -84,16 +84,12 @@
 /// the first symbolic parent region is returned.
 SymbolRef SVal::getAsLocSymbol(bool IncludeBaseRegions) const {
   // FIXME: should we consider SymbolRef wrapped in CodeTextRegion?
-  if (Optional X = getAs())
-return X->getLoc().getAsLocSymbol(IncludeBaseRegions);
-
-  if (Optional X = getAs()) {
-const MemRegion *R = X->getRegion();
-if (const SymbolicRegion *SymR = IncludeBaseRegions ?
-  R->getSymbolicBase() :
-  
dyn_cast(R->StripCasts()))
+  if (const MemRegion *R = getAsRegion())
+if (const SymbolicRegion *SymR =
+IncludeBaseRegions ? R->getSymbolicBase()
+   : dyn_cast(R->StripCasts()))
   return SymR->getSymbol();
-  }
+
   return nullptr;
 }
 


Index: clang/lib/StaticAnalyzer/Core/SVals.cpp
===
--- clang/lib/StaticAnalyzer/Core/SVals.cpp
+++ clang/lib/StaticAnalyzer/Core/SVals.cpp
@@ -84,16 +84,12 @@
 /// the first symbolic parent region is returned.
 SymbolRef SVal::getAsLocSymbol(bool IncludeBaseRegions) const {
   // FIXME: should we consider SymbolRef wrapped in CodeTextRegion?
-  if (Optional X = getAs())
-return X->getLoc().getAsLocSymbol(IncludeBaseRegions);
-
-  if (Optional X = getAs()) {
-const MemRegion *R = X->getRegion();
-if (const SymbolicRegion *SymR = IncludeBaseRegions ?
-  R->getSymbolicBase() :
-  dyn_cast(R->StripCasts()))
+  if (const MemRegion *R = getAsRegion())
+if (const SymbolicRegion *SymR =
+IncludeBaseRegions ? R->getSymbolicBase()
+   : dyn_cast(R->StripCasts()))
   return SymR->getSymbol();
-  }
+
   return nullptr;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89749: SourceManager: Don't allocate an SLocEntry until it's loaded

2020-10-26 Thread Raphael Isemann via Phabricator via cfe-commits

teemperor added a subscriber: v.g.vassilev.
teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

Maybe arc didn't correctly apply the patch, but this causes a few LLDB tests to 
fail on my machine:

  lldb-api :: 
commands/expression/import-std-module/queue/TestQueueFromStdModule.py
  lldb-api :: 
commands/expression/import-std-module/list/TestListFromStdModule.py
  lldb-api :: 
commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py

From the backtrace this seems like the ASTImporter needs to be updated:

  * frame #0: 0x725f1bb6 
_lldb.so`clang::ASTImporter::Import(clang::FileID, bool) + 566
frame #1: 0x725ecc56 
_lldb.so`clang::ASTImporter::Import(clang::SourceLocation) + 182
frame #2: 0x725bbaed _lldb.so`clang::SourceLocation 
clang::ASTNodeImporter::importChecked(llvm::Error&, 
clang::SourceLocation const&) + 61
frame #3: 0x725c34e8 
_lldb.so`clang::ASTNodeImporter::VisitFunctionDecl(clang::FunctionDecl*) + 2968
frame #4: 0x725ebb31 
_lldb.so`clang::declvisitor::Base >::Visit(clang::Decl*) + 81
frame #5: 0x725ebac2 
_lldb.so`clang::ASTImporter::ImportImpl(clang::Decl*) + 34
frame #6: 0x70df9cb1 
_lldb.so`lldb_private::ClangASTImporter::ASTImporterDelegate::ImportImpl(clang::Decl*)
 + 1345
frame #7: 0x725ed3f4 
_lldb.so`clang::ASTImporter::Import(clang::Decl*) + 532
frame #8: 0x725aeb93 _lldb.so`llvm::Expected 
clang::ASTNodeImport

The tests are just loading a Clang module and then trying to import some Decls 
into another ASTContext, so I don't think the error here is specific to LLDB. 
We just don't have any ASTImporter tests that exercise modules.

FWIW, if you quickly want to see if your SourceManager changes break LLDB, then 
you can just set the LIT_FILTER to "Modules" and run `check-lldb` as LLDB's use 
of Clang modules are the only place where we have any valid SourceLocations 
within LLDB.

Regarding the patch itself:

1. The `I - 1` everywhere when translating `SLocEntryLoaded` values to a 
`LoadedSLocEntryTable` index is a bit cryptic. We already do similar magic in 
other places, but here it's really just about expressing an optional integer. 
Could we maybe have a wrapper function that turns this into a 
`llvm::Optional` (that is `llvm::None` when it's not loaded). 
Something like this maybe (which hopefully optimizes to something similar to 
the original code):

  llvm::Optional getLoadedSLocEntryTableIndex(size_t Index) const {
unsigned result = SLocEntryLoaded[Index];
if (result == /*sentinel value*/0)
  return llvm::None;
return result - 1U;
  }



2. If we have this as an Optional, we might as well make the sentinel value 
`std::numeric_limits::max()` instead of 0 in `SLocEntryLoaded`. Then 
we can just all values as-is without translating them to an index.

3. Now that the primary purpose of `SLocEntryLoaded` is storing indices and not 
the loaded status, maybe something like `SLocEntryIndices` would be a better 
name?

4. Do you still have your benchmarking values (or some estimates) for this? I'm 
just curious how much memory this actually saves.

Otherwise I think this looks good.


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

https://reviews.llvm.org/D89749

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

[PATCH] D90161: [SyntaxTree] Provide iterators for Lists

2020-10-26 Thread Eduardo Caldas via Phabricator via cfe-commits

eduucaldas created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
eduucaldas requested review of this revision.

Provide an iterator for `List` that iterates through elements and delimiters on 
pairs. This iterator is robust to missing elements. For instance, in the 
following function call:
`f(a b, , )`
The iterator would emit the following elements and delimiters:
`[("a", null), ("b", ","), (null, ","), (null, null) ]`

This iterator abstracts out many corner cases when dealing with lists, thus 
providing a cleaner interface for mutation and traversal of `List`s.

Note: This iterator has the memory footprint of one pointer.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90161

Files:
  clang/include/clang/Tooling/Syntax/Tree.h
  clang/lib/Tooling/Syntax/Nodes.cpp
  clang/lib/Tooling/Syntax/Tree.cpp
  clang/unittests/Tooling/Syntax/TreeTest.cpp

Index: clang/unittests/Tooling/Syntax/TreeTest.cpp
===
--- clang/unittests/Tooling/Syntax/TreeTest.cpp
+++ clang/unittests/Tooling/Syntax/TreeTest.cpp
@@ -11,6 +11,7 @@
 #include "clang/Tooling/Syntax/BuildTree.h"
 #include "clang/Tooling/Syntax/Nodes.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest.h"
 
 using namespace clang;
@@ -125,7 +126,7 @@
 }
 
 class ListTest : public SyntaxTreeTest {
-private:
+protected:
   std::string dumpQuotedTokensOrNull(const Node *N) {
 return N ? "'" +
StringRef(N->dumpTokens(Arena->getSourceManager()))
@@ -135,7 +136,15 @@
  : "null";
   }
 
-protected:
+  std::string
+  dumpElementAndDelimiter(const List::ElementAndDelimiter ED) {
+std::string Storage;
+llvm::raw_string_ostream OS(Storage);
+OS << "(" << dumpQuotedTokensOrNull(ED.element) << ", "
+   << dumpQuotedTokensOrNull(ED.delimiter) << ")";
+return OS.str();
+  }
+
   std::string
   dumpElementsAndDelimiters(ArrayRef> EDs) {
 std::string Storage;
@@ -145,8 +154,7 @@
 
 llvm::interleaveComma(
 EDs, OS, [&OS, this](const List::ElementAndDelimiter &ED) {
-  OS << "(" << dumpQuotedTokensOrNull(ED.element) << ", "
- << dumpQuotedTokensOrNull(ED.delimiter) << ")";
+  OS << dumpElementAndDelimiter(ED);
 });
 
 OS << "]";
@@ -351,4 +359,143 @@
   EXPECT_EQ(dumpNodes(List->getElementsAsNodes()), "['a', 'b', 'c']");
 }
 
+TEST_P(ListTest, List_Terminated_Iterator_Parent) {
+  if (!GetParam().isCXX()) {
+return;
+  }
+  buildTree("", GetParam());
+
+  // "a   b::  :: c"
+  auto *List = dyn_cast(syntax::createTree(
+  *Arena,
+  {
+  {createLeaf(*Arena, tok::identifier, "a"), NodeRole::ListElement},
+  {createLeaf(*Arena, tok::identifier, "b"), NodeRole::ListElement},
+  {createLeaf(*Arena, tok::coloncolon), NodeRole::ListDelimiter},
+  {createLeaf(*Arena, tok::coloncolon), NodeRole::ListDelimiter},
+  {createLeaf(*Arena, tok::identifier, "c"), NodeRole::ListElement},
+  },
+  NodeKind::NestedNameSpecifier));
+
+  EXPECT_EQ(List->getElementsAsNodesAndDelimitersBeforeBegin().getParent(),
+List);
+
+  for (auto It = List->getElementsAsNodesAndDelimitersBegin(),
+End = List->getElementsAsNodesAndDelimitersEnd();
+   It != End; ++It)
+EXPECT_EQ(It.getParent(), List);
+
+  EXPECT_EQ(List->getElementsAsNodesAndDelimitersEnd().getParent(), List);
+}
+
+TEST_P(ListTest, List_Terminated_Iterator_Equality) {
+  if (!GetParam().isCXX()) {
+return;
+  }
+  buildTree("", GetParam());
+
+  // "a   b::  :: c"
+  auto *List = dyn_cast(syntax::createTree(
+  *Arena,
+  {
+  {createLeaf(*Arena, tok::identifier, "a"), NodeRole::ListElement},
+  {createLeaf(*Arena, tok::identifier, "b"), NodeRole::ListElement},
+  {createLeaf(*Arena, tok::coloncolon), NodeRole::ListDelimiter},
+  {createLeaf(*Arena, tok::coloncolon), NodeRole::ListDelimiter},
+  {createLeaf(*Arena, tok::identifier, "c"), NodeRole::ListElement},
+  },
+  NodeKind::NestedNameSpecifier));
+
+  // different iterators are different.
+  for (auto BeforeIt = List->getElementsAsNodesAndDelimitersBeforeBegin(),
+End = List->getElementsAsNodesAndDelimitersEnd();
+   BeforeIt != End; ++BeforeIt) {
+auto It = BeforeIt;
+++It;
+for (; It != End; ++It) {
+  EXPECT_NE(BeforeIt, It);
+}
+  }
+
+  // Equal iterators are equal.
+  for (auto It = List->getElementsAsNodesAndDelimitersBegin(),
+It2 = List->getElementsAsNodesAndDelimitersBegin(),
+End = List->getElementsAsNodesAndDelimitersEnd();
+   It != End && It2 != End;) {
+EXPECT_EQ(++It, ++It2);
+  }
+
+  // Different sentinel iterators are different.
+  EXPECT_NE(List->getElementsAsNodesAndDelimitersBeforeBegin(),
+List->getElementsAsNodesAndDelimitersEnd());
+}
+
+TEST_P(ListTest, List_Separa

[PATCH] D89749: SourceManager: Don't allocate an SLocEntry until it's loaded

2020-10-26 Thread Vassil Vassilev via Phabricator via cfe-commits

v.g.vassilev added a comment.

Thanks for the patch!! This is a super hot place for us (mostly due to boost). 
I will try it on our end and let you know!


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

https://reviews.llvm.org/D89749

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

[PATCH] D89832: [CUDA] Extract CUDA version from cuda.h if version.txt is not found

2020-10-26 Thread Evgeny Mankov via Phabricator via cfe-commits

emankov added a comment.

D89832  eliminates 47332 
 on Windows as well. Tested 
against the same CUDA versions as for D89752 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89832

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

[PATCH] D90023: [Syntax] Add iterators over children of syntax trees.

2020-10-26 Thread Dmitri Gribenko via Phabricator via cfe-commits

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



Comment at: clang/include/clang/Tooling/Syntax/Tree.h:182
+/// The element, or nullptr if past-the-end.
+NodeT *asPointer() const { return N; }
+  };

"nodePointer()" ?



Comment at: clang/include/clang/Tooling/Syntax/Tree.h:202-219
+  /// child_iterator is not invalidated by mutations.
+  struct child_iterator : child_iterator_base {
+using Base::Base;
+  };
+  struct const_child_iterator
+  : child_iterator_base {
+using Base::Base;

sammccall wrote:
> eduucaldas wrote:
> > TL;DR:
> > `child_iterator` -> `ChildIterator`
> > `const_child_iterator` -> `ConstChildIterator`
> > `children` -> `getChildren`
> > 
> > I see you followed the naming convention of the ClangAST, which makes loads 
> > of sense. 
> > 
> > In syntax trees we follow the naming conventions of the [conding 
> > standard](https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly)
> >  -- just because we want consistency and we had to pick a guideline to 
> > follow -- according to that, functions should be verb like and names should 
> > be camel case.
> > 
> > If like us you chose `const_child_iterator` and `children` kind of "just 
> > because", could you change it to follow `ConstChildIterator` and 
> > `getChildren` respectively. 
> > In syntax trees we follow the naming conventions of the conding standard
> 
> I see that's been used in some of the newer classes, and that you changed 
> this file to follow that style in 4c14ee61b73746b314d83e7c52e03d6527b78105. 
> However it's not the original style we used here and elsewhere in libSyntax.
> It's a bit frustrating to hear the argument about consistency, because we 
> were quite careful and deliberate about this style, which was IMO fairly 
> consistent prior to that change.
> 
> I'm willing to change these to match the local style in this file. However 
> `*_iterator`, `*_begin/end/range()` is more common than FooIterator etc in 
> LLVM overall (I think for consistency with `iterator` etc, which is endorsed 
> by the style guide). So I don't think this change is particularly an 
> improvement, even in terms of consistency.
It can go either way:

> As an exception, classes that mimic STL classes can have member names in 
> STL’s style of lower-case words separated by underscores




Comment at: clang/include/clang/Tooling/Syntax/Tree.h:210
+const_child_iterator(const child_iterator &I)
+: Base(I == child_iterator() ? nullptr : &*I) {}
+  };





Comment at: clang/lib/Tooling/Syntax/Tree.cpp:22-23
   if (auto *T = dyn_cast(N)) {
-for (const auto *C = T->getFirstChild(); C; C = C->getNextSibling())
-  traverse(C, Visit);
+for (const syntax::Node &C : T->children())
+  traverse(&C, Visit);
   }

sammccall wrote:
> eduucaldas wrote:
> > Hmm... 
> > That looks funny, if the user uses a range-based for loop and forgets the 
> > `&`, then we would be copying the Node???
> > 
> > Also in many places we had to get the address to the node. That is not 
> > really consistent with how the syntax trees API's were designed, they 
> > generally take pointers instead of references. (that said we could 
> > obviously reconsider those design choices)
> > That looks funny, if the user uses a range-based for loop and forgets the 
> > &, then we would be copying the Node???
> 
> It doesn't look funny to me - foreach usually iterates over values rather 
> than pointers. This raises a good point - it looks like Node is copyable and 
> shouldn't be. (The default copy operation violates the tree invariants, e.g. 
> the copied node will not be a child of its parent). I'll send a patch...
> 
> (That said this is always the case with copyable objects in range-based for 
> loops, and indeed using references - that doesn't usually mean we avoid using 
> them)
> 
> > Also in many places we had to get the address to the node. That is not 
> > really consistent with how the syntax trees API's were designed, they 
> > generally take pointers instead of references
> 
> I'm not convinced that taking the address is itself a burden, any more than 
> using `->` instead of `.` is a burden.
> Sometimes the use of pointer vs reference intends to convey something about 
> address stability, but here this goes with the type as all nodes are 
> allocated on the arena. (IMO this is a part of the AST design that works 
> well).
> The other thing it conveys is nullability, and this seems valuable enough 
> that IMO APIs that take *non-nullable* nodes by pointer should be 
> reconsidered.
> 
> For the iterators specifically, the main alternative here I guess is having 
> Tree::children() act as a container of *pointers to* children, instead of the 
> children themselves. This *is* highly unconventiona

[PATCH] D69844: [clang][Basic] Integrate SourceLocation with FoldingSet, NFCI

2020-10-26 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits

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

LGTM.




Comment at: clang/lib/Analysis/PathDiagnostic.cpp:1088
+  ID.Add(Range.getEnd());
+  ID.Add(static_cast(Loc));
 }

miyuki wrote:
> dexonsmith wrote:
> > I'm surprised you need this `static_cast`, can you explain why it was 
> > necessary?
> `Loc` is an object of type `FullSourceLoc`, a class derived from 
> `SourceLocation`. When the `FoldingSetNodeID::Add` template method is called 
> with `Loc` it tries to instantiate `FoldingSetTrait` and fails 
> (because there is no explicit specialization for `FullSourceLoc` and the 
> default one relies on the method `FullSourceLoc::Profile` which does not 
> exist). `FullSourceLoc` does not contain any additional information about the 
> location itself (it just holds a pointer to the associated `SourceManager`), 
> so there is no need to specialize `FoldingSetTrait` for it, and casting to 
> `SourceLocation` will make `FoldingSetNodeID::Add` use the correct 
> `FoldingSetTrait` specialization.
Right, that makes sense. Thanks for explaining!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69844

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

[PATCH] D90163: [Syntax] Disallow invalid Node operations

2020-10-26 Thread Sam McCall via Phabricator via cfe-commits

sammccall created this revision.
sammccall added reviewers: gribozavr, eduucaldas.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
sammccall requested review of this revision.

Copy/move break invariants (move could be fixed).
Node/Tree should have no public constructors, they're abstract.
Destructor is private to enforce arena allocation.

(Making the constructor of all subclasses private doesn't seem worthwhile)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90163

Files:
  clang/include/clang/Tooling/Syntax/Tree.h


Index: clang/include/clang/Tooling/Syntax/Tree.h
===
--- clang/include/clang/Tooling/Syntax/Tree.h
+++ clang/include/clang/Tooling/Syntax/Tree.h
@@ -76,10 +76,20 @@
 /// A node in a syntax tree. Each node is either a Leaf (representing tokens) 
or
 /// a Tree (representing language constructrs).
 class Node {
-public:
+protected:
   /// Newly created nodes are detached from a tree, parent and sibling links 
are
   /// set when the node is added as a child to another one.
   Node(NodeKind Kind);
+  /// Nodes are allocated on Arenas; the destructor is never called.
+  ~Node() = default;
+
+public:
+  /// Nodes cannot simply be copied without violating tree invariants.
+  Node(const Node &) = delete;
+  Node &operator=(const Node &) = delete;
+  /// Idiomatically, nodes are allocated on an Arena and never moved.
+  Node(Node &&) = delete;
+  Node &operator=(Node &&) = delete;
 
   NodeKind getKind() const { return static_cast(Kind); }
   NodeRole getRole() const { return static_cast(Role); }
@@ -153,7 +163,6 @@
 /// A node that has children and represents a syntactic language construct.
 class Tree : public Node {
 public:
-  using Node::Node;
   static bool classof(const Node *N);
 
   Node *getFirstChild() { return FirstChild; }
@@ -170,6 +179,7 @@
   }
 
 protected:
+  using Node::Node;
   /// Find the first node with a corresponding role.
   Node *findChild(NodeRole R);
 


Index: clang/include/clang/Tooling/Syntax/Tree.h
===
--- clang/include/clang/Tooling/Syntax/Tree.h
+++ clang/include/clang/Tooling/Syntax/Tree.h
@@ -76,10 +76,20 @@
 /// A node in a syntax tree. Each node is either a Leaf (representing tokens) or
 /// a Tree (representing language constructrs).
 class Node {
-public:
+protected:
   /// Newly created nodes are detached from a tree, parent and sibling links are
   /// set when the node is added as a child to another one.
   Node(NodeKind Kind);
+  /// Nodes are allocated on Arenas; the destructor is never called.
+  ~Node() = default;
+
+public:
+  /// Nodes cannot simply be copied without violating tree invariants.
+  Node(const Node &) = delete;
+  Node &operator=(const Node &) = delete;
+  /// Idiomatically, nodes are allocated on an Arena and never moved.
+  Node(Node &&) = delete;
+  Node &operator=(Node &&) = delete;
 
   NodeKind getKind() const { return static_cast(Kind); }
   NodeRole getRole() const { return static_cast(Role); }
@@ -153,7 +163,6 @@
 /// A node that has children and represents a syntactic language construct.
 class Tree : public Node {
 public:
-  using Node::Node;
   static bool classof(const Node *N);
 
   Node *getFirstChild() { return FirstChild; }
@@ -170,6 +179,7 @@
   }
 
 protected:
+  using Node::Node;
   /// Find the first node with a corresponding role.
   Node *findChild(NodeRole R);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90165: Clang is crashing after generating the right diagnostic for a re-declaration of a friend method - Fix for PR47544.

2020-10-26 Thread Zahira Ammarguellat via Phabricator via cfe-commits

zahiraam created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
zahiraam requested review of this revision.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90165

Files:
  clang/lib/Parse/ParseCXXInlineMethods.cpp
  clang/test/SemaCXX/invalid-decl.cpp


Index: clang/test/SemaCXX/invalid-decl.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/invalid-decl.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+class test1 {
+  template friend int bar(bool = true) {} // expected-note 
{{previous declaration is here}}
+  template friend int bar(bool); // expected-error {{friend 
declaration specifying a default argument must be the only declaration}}
+};
+
+class test2 {
+  friend int bar(bool = true) {} // expected-note {{previous declaration is 
here}}
+  friend int bar(bool); // expected-error{{friend declaration specifying a 
default argument must be the only declaration}}
+};
Index: clang/lib/Parse/ParseCXXInlineMethods.cpp
===
--- clang/lib/Parse/ParseCXXInlineMethods.cpp
+++ clang/lib/Parse/ParseCXXInlineMethods.cpp
@@ -405,14 +405,22 @@
 ConsumeAnyToken();
 } else if (HasUnparsed) {
   assert(Param->hasInheritedDefaultArg());
-  FunctionDecl *Old = cast(LM.Method)->getPreviousDecl();
-  ParmVarDecl *OldParam = Old->getParamDecl(I);
-  assert (!OldParam->hasUnparsedDefaultArg());
-  if (OldParam->hasUninstantiatedDefaultArg())
-Param->setUninstantiatedDefaultArg(
-OldParam->getUninstantiatedDefaultArg());
+  const FunctionDecl *Old;
+  if (const auto *FunTmpl =
+  dyn_cast(LM.Method))
+Old =
+cast(FunTmpl->getTemplatedDecl())->getPreviousDecl();
   else
-Param->setDefaultArg(OldParam->getInit());
+Old = cast(LM.Method)->getPreviousDecl();
+  if (Old) {
+ParmVarDecl *OldParam = const_cast(Old->getParamDecl(I));
+assert(!OldParam->hasUnparsedDefaultArg());
+if (OldParam->hasUninstantiatedDefaultArg())
+  Param->setUninstantiatedDefaultArg(
+  OldParam->getUninstantiatedDefaultArg());
+else
+  Param->setDefaultArg(OldParam->getInit());
+  }
 }
   }
 


Index: clang/test/SemaCXX/invalid-decl.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/invalid-decl.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+class test1 {
+  template friend int bar(bool = true) {} // expected-note {{previous declaration is here}}
+  template friend int bar(bool); // expected-error {{friend declaration specifying a default argument must be the only declaration}}
+};
+
+class test2 {
+  friend int bar(bool = true) {} // expected-note {{previous declaration is here}}
+  friend int bar(bool); // expected-error{{friend declaration specifying a default argument must be the only declaration}}
+};
Index: clang/lib/Parse/ParseCXXInlineMethods.cpp
===
--- clang/lib/Parse/ParseCXXInlineMethods.cpp
+++ clang/lib/Parse/ParseCXXInlineMethods.cpp
@@ -405,14 +405,22 @@
 ConsumeAnyToken();
 } else if (HasUnparsed) {
   assert(Param->hasInheritedDefaultArg());
-  FunctionDecl *Old = cast(LM.Method)->getPreviousDecl();
-  ParmVarDecl *OldParam = Old->getParamDecl(I);
-  assert (!OldParam->hasUnparsedDefaultArg());
-  if (OldParam->hasUninstantiatedDefaultArg())
-Param->setUninstantiatedDefaultArg(
-OldParam->getUninstantiatedDefaultArg());
+  const FunctionDecl *Old;
+  if (const auto *FunTmpl =
+  dyn_cast(LM.Method))
+Old =
+cast(FunTmpl->getTemplatedDecl())->getPreviousDecl();
   else
-Param->setDefaultArg(OldParam->getInit());
+Old = cast(LM.Method)->getPreviousDecl();
+  if (Old) {
+ParmVarDecl *OldParam = const_cast(Old->getParamDecl(I));
+assert(!OldParam->hasUnparsedDefaultArg());
+if (OldParam->hasUninstantiatedDefaultArg())
+  Param->setUninstantiatedDefaultArg(
+  OldParam->getUninstantiatedDefaultArg());
+else
+  Param->setDefaultArg(OldParam->getInit());
+  }
 }
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D90161: [SyntaxTree] Provide iterators for Lists

2020-10-26 Thread Dmitri Gribenko via Phabricator via cfe-commits

gribozavr2 added a comment.

Could you add tests that verify the pairing of elements and delimiters?




Comment at: clang/include/clang/Tooling/Syntax/Tree.h:214
+  /// elements and delimiters are represented as null pointers. Below we give
+  /// examples of what this iteration would looks like.
   ///




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90161

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

[PATCH] D90023: [Syntax] Add iterators over children of syntax trees.

2020-10-26 Thread Sam McCall via Phabricator via cfe-commits

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



Comment at: clang/include/clang/Tooling/Syntax/Tree.h:182
+/// The element, or nullptr if past-the-end.
+NodeT *asPointer() const { return N; }
+  };

gribozavr2 wrote:
> "nodePointer()" ?
I find this a little confusing; it suggests to me there's also something 
*other* than the node here.
What aspect does it help with?

I can live with it, though currently would prefer `pointer()`, `getPointer()` 
or `asPointer()`.



Comment at: clang/include/clang/Tooling/Syntax/Tree.h:202-219
+  /// child_iterator is not invalidated by mutations.
+  struct child_iterator : child_iterator_base {
+using Base::Base;
+  };
+  struct const_child_iterator
+  : child_iterator_base {
+using Base::Base;

gribozavr2 wrote:
> sammccall wrote:
> > eduucaldas wrote:
> > > TL;DR:
> > > `child_iterator` -> `ChildIterator`
> > > `const_child_iterator` -> `ConstChildIterator`
> > > `children` -> `getChildren`
> > > 
> > > I see you followed the naming convention of the ClangAST, which makes 
> > > loads of sense. 
> > > 
> > > In syntax trees we follow the naming conventions of the [conding 
> > > standard](https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly)
> > >  -- just because we want consistency and we had to pick a guideline to 
> > > follow -- according to that, functions should be verb like and names 
> > > should be camel case.
> > > 
> > > If like us you chose `const_child_iterator` and `children` kind of "just 
> > > because", could you change it to follow `ConstChildIterator` and 
> > > `getChildren` respectively. 
> > > In syntax trees we follow the naming conventions of the conding standard
> > 
> > I see that's been used in some of the newer classes, and that you changed 
> > this file to follow that style in 4c14ee61b73746b314d83e7c52e03d6527b78105. 
> > However it's not the original style we used here and elsewhere in libSyntax.
> > It's a bit frustrating to hear the argument about consistency, because we 
> > were quite careful and deliberate about this style, which was IMO fairly 
> > consistent prior to that change.
> > 
> > I'm willing to change these to match the local style in this file. However 
> > `*_iterator`, `*_begin/end/range()` is more common than FooIterator etc in 
> > LLVM overall (I think for consistency with `iterator` etc, which is 
> > endorsed by the style guide). So I don't think this change is particularly 
> > an improvement, even in terms of consistency.
> It can go either way:
> 
> > As an exception, classes that mimic STL classes can have member names in 
> > STL’s style of lower-case words separated by underscores
> 
Yeah, though read narrowly that doesn't apply (Tree doesn't really mimic STL, 
and the rule applies to Tree::ChildIterator's members rather than the class 
itself).

I think it's rare enough to spell out iterator class names that those don't 
matter at all, and getChildren() is necessary given getParent() etc.



Comment at: clang/lib/Tooling/Syntax/Tree.cpp:22-23
   if (auto *T = dyn_cast(N)) {
-for (const auto *C = T->getFirstChild(); C; C = C->getNextSibling())
-  traverse(C, Visit);
+for (const syntax::Node &C : T->children())
+  traverse(&C, Visit);
   }

gribozavr2 wrote:
> sammccall wrote:
> > eduucaldas wrote:
> > > Hmm... 
> > > That looks funny, if the user uses a range-based for loop and forgets the 
> > > `&`, then we would be copying the Node???
> > > 
> > > Also in many places we had to get the address to the node. That is not 
> > > really consistent with how the syntax trees API's were designed, they 
> > > generally take pointers instead of references. (that said we could 
> > > obviously reconsider those design choices)
> > > That looks funny, if the user uses a range-based for loop and forgets the 
> > > &, then we would be copying the Node???
> > 
> > It doesn't look funny to me - foreach usually iterates over values rather 
> > than pointers. This raises a good point - it looks like Node is copyable 
> > and shouldn't be. (The default copy operation violates the tree invariants, 
> > e.g. the copied node will not be a child of its parent). I'll send a 
> > patch...
> > 
> > (That said this is always the case with copyable objects in range-based for 
> > loops, and indeed using references - that doesn't usually mean we avoid 
> > using them)
> > 
> > > Also in many places we had to get the address to the node. That is not 
> > > really consistent with how the syntax trees API's were designed, they 
> > > generally take pointers instead of references
> > 
> > I'm not convinced that taking the address is itself a burden, any more than 
> > using `->` instead of `.` is a burden.
> > Sometimes the use of pointer vs reference intends to convey something about 
> > address stability, but here this goes with t

[PATCH] D90023: [Syntax] Add iterators over children of syntax trees.

2020-10-26 Thread Sam McCall via Phabricator via cfe-commits

sammccall updated this revision to Diff 300691.
sammccall marked an inline comment as done.
sammccall added a comment.

Simplify ConstChildIterator cross-constructor


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90023

Files:
  clang/include/clang/Tooling/Syntax/Tree.h
  clang/lib/Tooling/Syntax/Tree.cpp
  clang/unittests/Tooling/Syntax/TreeTest.cpp
  clang/unittests/Tooling/Syntax/TreeTestBase.h

Index: clang/unittests/Tooling/Syntax/TreeTestBase.h
===
--- clang/unittests/Tooling/Syntax/TreeTestBase.h
+++ clang/unittests/Tooling/Syntax/TreeTestBase.h
@@ -20,7 +20,9 @@
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "clang/Tooling/Syntax/Tree.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Testing/Support/Annotations.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 namespace clang {
@@ -53,6 +55,14 @@
 };
 
 std::vector allTestClangConfigs();
+
+MATCHER_P(role, R, "") {
+  if (arg.getRole() == R)
+return true;
+  *result_listener << "role is " << llvm::to_string(arg.getRole());
+  return false;
+}
+
 } // namespace syntax
 } // namespace clang
 #endif // LLVM_CLANG_UNITTESTS_TOOLING_SYNTAX_TREETESTBASE_H
Index: clang/unittests/Tooling/Syntax/TreeTest.cpp
===
--- clang/unittests/Tooling/Syntax/TreeTest.cpp
+++ clang/unittests/Tooling/Syntax/TreeTest.cpp
@@ -8,6 +8,7 @@
 
 #include "clang/Tooling/Syntax/Tree.h"
 #include "TreeTestBase.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Syntax/BuildTree.h"
 #include "clang/Tooling/Syntax/Nodes.h"
 #include "llvm/ADT/STLExtras.h"
@@ -17,6 +18,7 @@
 using namespace clang::syntax;
 
 namespace {
+using testing::ElementsAre;
 
 class TreeTest : public SyntaxTreeTest {
 private:
@@ -124,6 +126,56 @@
   }
 }
 
+TEST_F(TreeTest, Iterators) {
+  buildTree("", allTestClangConfigs().front());
+  std::vector Children = {createLeaf(*Arena, tok::identifier, "a"),
+  createLeaf(*Arena, tok::identifier, "b"),
+  createLeaf(*Arena, tok::identifier, "c")};
+  auto *Tree = syntax::createTree(*Arena,
+  {{Children[0], NodeRole::LeftHandSide},
+   {Children[1], NodeRole::OperatorToken},
+   {Children[2], NodeRole::RightHandSide}},
+  NodeKind::TranslationUnit);
+  const auto *ConstTree = Tree;
+
+  auto Range = Tree->getChildren();
+  EXPECT_THAT(Range, ElementsAre(role(NodeRole::LeftHandSide),
+ role(NodeRole::OperatorToken),
+ role(NodeRole::RightHandSide)));
+
+  auto ConstRange = ConstTree->getChildren();
+  EXPECT_THAT(ConstRange, ElementsAre(role(NodeRole::LeftHandSide),
+  role(NodeRole::OperatorToken),
+  role(NodeRole::RightHandSide)));
+
+  // FIXME: mutate and observe no invalidation. Mutations are private for now...
+  auto It = Range.begin();
+  auto CIt = ConstRange.begin();
+  static_assert(std::is_same::value,
+"mutable range");
+  static_assert(std::is_same::value,
+"const range");
+
+  for (unsigned I = 0; I < 3; ++I) {
+EXPECT_EQ(It, CIt);
+EXPECT_TRUE(It);
+EXPECT_TRUE(CIt);
+EXPECT_EQ(It.asPointer(), Children[I]);
+EXPECT_EQ(CIt.asPointer(), Children[I]);
+EXPECT_EQ(&*It, Children[I]);
+EXPECT_EQ(&*CIt, Children[I]);
+++It;
+++CIt;
+  }
+  EXPECT_EQ(It, CIt);
+  EXPECT_EQ(It, Tree::ChildIterator());
+  EXPECT_EQ(CIt, Tree::ConstChildIterator());
+  EXPECT_FALSE(It);
+  EXPECT_FALSE(CIt);
+  EXPECT_EQ(nullptr, It.asPointer());
+  EXPECT_EQ(nullptr, CIt.asPointer());
+}
+
 class ListTest : public SyntaxTreeTest {
 private:
   std::string dumpQuotedTokensOrNull(const Node *N) {
Index: clang/lib/Tooling/Syntax/Tree.cpp
===
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -19,8 +19,8 @@
 static void traverse(const syntax::Node *N,
  llvm::function_ref Visit) {
   if (auto *T = dyn_cast(N)) {
-for (const auto *C = T->getFirstChild(); C; C = C->getNextSibling())
-  traverse(C, Visit);
+for (const syntax::Node &C : T->getChildren())
+  traverse(&C, Visit);
   }
   Visit(N);
 }
@@ -194,21 +194,21 @@
   DumpExtraInfo(N);
   OS << "\n";
 
-  for (const auto *It = T->getFirstChild(); It; It = It->getNextSibling()) {
+  for (const syntax::Node &It : T->getChildren()) {
 for (bool Filled : IndentMask) {
   if (Filled)
 OS << "| ";
   else
 OS << "  ";
 }
-if (!It->getNextSibling()) {
+if (!It.getNextSibling()) {
   OS << "`-"

[PATCH] D89749: SourceManager: Don't allocate an SLocEntry until it's loaded

2020-10-26 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits

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

In D89749#2353651 , @v.g.vassilev 
wrote:

> Thanks for the patch!! This is a super hot place for us (mostly due to 
> boost). I will try it on our end and let you know!

Great!

In D89749#2353602 , @teemperor wrote:

> Maybe arc didn't correctly apply the patch, but this causes a few LLDB tests 
> to fail on my machine:
>
>   lldb-api :: 
> commands/expression/import-std-module/queue/TestQueueFromStdModule.py
>   lldb-api :: 
> commands/expression/import-std-module/list/TestListFromStdModule.py
>   lldb-api :: 
> commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py
>
> From the backtrace this seems like the ASTImporter needs to be updated:

Thanks for doing that legwork, I'll take a look.

> The tests are just loading a Clang module and then trying to import some 
> Decls into another ASTContext, so I don't think the error here is specific to 
> LLDB. We just don't have any ASTImporter tests that exercise modules.

I'll add one.

> FWIW, if you quickly want to see if your SourceManager changes break LLDB, 
> then you can just set the LIT_FILTER to "Modules" and run `check-lldb` as 
> LLDB's use of Clang modules are the only place where we have any valid 
> SourceLocations within LLDB.

This is helpful, thanks. I also need to deal with whatever entitlements 
`check-lldb` needs (new machine since the last time and I remember it being 
non-trivial) but I'll figure that out.

> 1. The `I - 1` everywhere when translating `SLocEntryLoaded` values to a 
> `LoadedSLocEntryTable` index is a bit cryptic. [...]

Great comment; I think the way I'll do this is to create a simple wrapper type 
that degrades to `Optional`.

> 2. If we have this as an Optional, we might as well make the sentinel value 
> `std::numeric_limits::max()` instead of 0 in `SLocEntryLoaded`. 
> Then we can just all values as-is without translating them to an index.

Could do, but there's a tradeoff since then a `resize` operation can't/doesn't 
use zero-initialization. As a result I have a (weak) preference for a 
0-sentinel (probably my next patch will keep that semantic), but I'm open to 
changing it.

> 3. Now that the primary purpose of `SLocEntryLoaded` is storing indices and 
> not the loaded status, maybe something like `SLocEntryIndices` would be a 
> better name?

Good idea.

> 4. Do you still have your benchmarking values (or some estimates) for this? 
> I'm just curious how much memory this actually saves.

I'll find some numbers if Vassil hasn't shared his by the time I've fixed the 
patch.


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

https://reviews.llvm.org/D89749

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

[PATCH] D90023: [Syntax] Add iterators over children of syntax trees.

2020-10-26 Thread Dmitri Gribenko via Phabricator via cfe-commits

gribozavr2 added inline comments.



Comment at: clang/include/clang/Tooling/Syntax/Tree.h:182
+/// The element, or nullptr if past-the-end.
+NodeT *asPointer() const { return N; }
+  };

sammccall wrote:
> gribozavr2 wrote:
> > "nodePointer()" ?
> I find this a little confusing; it suggests to me there's also something 
> *other* than the node here.
> What aspect does it help with?
> 
> I can live with it, though currently would prefer `pointer()`, `getPointer()` 
> or `asPointer()`.
> it suggests to me there's also something *other* than the node here.

Not in this case, but there often are other pointers -- LLVM types often have 
an "opaque pointer" representation, and that was the first thing that I thought 
about when I read `asPointer()`. However, it is strongly typed, so one can 
figure it out. Up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90023

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

[PATCH] D89158: [NewPM] Run all EP callbacks under -O0

2020-10-26 Thread Arthur Eubanks via Phabricator via cfe-commits

aeubanks added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:1439
 if (CodeGenOpts.OptimizationLevel == 0) {
+  PB.runRegisteredEPCallbacks(MPM, Level, CodeGenOpts.DebugPassManager);
+

nikic wrote:
> It should be possible to simplify this function a lot now. It currently 
> duplicates nearly all logic (for sanitizers etc) between O0 and O1+. Or is 
> that planned for a followup?
Yup, I have that ready to go after this :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89158

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

[PATCH] D90023: [Syntax] Add iterators over children of syntax trees.

2020-10-26 Thread Eduardo Caldas via Phabricator via cfe-commits

eduucaldas accepted this revision.
eduucaldas added a comment.

Thanks for the instructive replies


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90023

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

[PATCH] D90161: [SyntaxTree] Provide iterators for Lists

2020-10-26 Thread Eduardo Caldas via Phabricator via cfe-commits

eduucaldas marked an inline comment as done.
eduucaldas added a comment.

In D90161#2353736 , @gribozavr2 wrote:

> Could you add tests that verify the pairing of elements and delimiters?

Those are surfaced through the previous tests, via 
`getElementsAsNodesAndDelimiters`




Comment at: clang/include/clang/Tooling/Syntax/Tree.h:442
+  ElementAndDelimiterIterator
+  getElementsAsNodesAndDelimitersBeforeBegin();
+  ElementAndDelimiterIterator getElementsAsNodesAndDelimitersBegin();

What do you think?

I didn't change it, because we had agreed on `ElementsAsNodesAndDelimiters`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90161

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

[PATCH] D89832: [CUDA] Extract CUDA version from cuda.h if version.txt is not found

2020-10-26 Thread Artem Belevich via Phabricator via cfe-commits

tra closed this revision.
tra added a comment.

Landed in  rGe7fe125b776b 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89832

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

[PATCH] D90161: [SyntaxTree] Provide iterators for Lists

2020-10-26 Thread Eduardo Caldas via Phabricator via cfe-commits

eduucaldas updated this revision to Diff 300696.
eduucaldas added a comment.

Fix comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90161

Files:
  clang/include/clang/Tooling/Syntax/Tree.h
  clang/unittests/Tooling/Syntax/TreeTest.cpp

Index: clang/unittests/Tooling/Syntax/TreeTest.cpp
===
--- clang/unittests/Tooling/Syntax/TreeTest.cpp
+++ clang/unittests/Tooling/Syntax/TreeTest.cpp
@@ -359,42 +359,143 @@
   EXPECT_EQ(dumpNodes(List->getElementsAsNodes()), "['a', 'b', 'c']");
 }
 
-TEST_P(ListTest, List_Iterator_StableDereference) {
+TEST_P(ListTest, List_Terminated_Iterator_Parent) {
   if (!GetParam().isCXX()) {
 return;
   }
   buildTree("", GetParam());
 
-  // "a:: b:: c"
-  auto *List = dyn_cast(syntax::createTree(
+  // "a   b::  :: c"
+  auto *List = dyn_cast(syntax::createTree(
   *Arena,
   {
   {createLeaf(*Arena, tok::identifier, "a"), NodeRole::ListElement},
+  {createLeaf(*Arena, tok::identifier, "b"), NodeRole::ListElement},
+  {createLeaf(*Arena, tok::coloncolon), NodeRole::ListDelimiter},
   {createLeaf(*Arena, tok::coloncolon), NodeRole::ListDelimiter},
+  {createLeaf(*Arena, tok::identifier, "c"), NodeRole::ListElement},
+  },
+  NodeKind::NestedNameSpecifier));
+
+  EXPECT_EQ(List->getElementsAsNodesAndDelimitersBeforeBegin().getParent(),
+List);
+
+  for (auto It = List->getElementsAsNodesAndDelimitersBegin(),
+End = List->getElementsAsNodesAndDelimitersEnd();
+   It != End; ++It)
+EXPECT_EQ(It.getParent(), List);
+
+  EXPECT_EQ(List->getElementsAsNodesAndDelimitersEnd().getParent(), List);
+}
+
+TEST_P(ListTest, List_Terminated_Iterator_Equality) {
+  if (!GetParam().isCXX()) {
+return;
+  }
+  buildTree("", GetParam());
+
+  // "a   b::  :: c"
+  auto *List = dyn_cast(syntax::createTree(
+  *Arena,
+  {
+  {createLeaf(*Arena, tok::identifier, "a"), NodeRole::ListElement},
   {createLeaf(*Arena, tok::identifier, "b"), NodeRole::ListElement},
   {createLeaf(*Arena, tok::coloncolon), NodeRole::ListDelimiter},
+  {createLeaf(*Arena, tok::coloncolon), NodeRole::ListDelimiter},
   {createLeaf(*Arena, tok::identifier, "c"), NodeRole::ListElement},
   },
   NodeKind::NestedNameSpecifier));
 
-  auto It = List->getElementsAsNodesAndDelimitersBegin();
-  syntax::List::ElementAndDelimiter First = {It.getElement(),
-   It.getDelimiter()};
-  auto ItAssign = It;
+  // different iterators are different.
+  for (auto BeforeIt = List->getElementsAsNodesAndDelimitersBeforeBegin(),
+End = List->getElementsAsNodesAndDelimitersEnd();
+   BeforeIt != End; ++BeforeIt) {
+auto It = BeforeIt;
+++It;
+for (; It != End; ++It) {
+  EXPECT_NE(BeforeIt, It);
+}
+  }
 
-  EXPECT_EQ(dumpElementAndDelimiter(First), "('a', '::')");
-  ++It;
-  EXPECT_EQ(dumpElementAndDelimiter(First), "('a', '::')");
+  // Equal iterators are equal.
+  for (auto It = List->getElementsAsNodesAndDelimitersBegin(),
+It2 = List->getElementsAsNodesAndDelimitersBegin(),
+End = List->getElementsAsNodesAndDelimitersEnd();
+   It != End && It2 != End;) {
+EXPECT_EQ(++It, ++It2);
+  }
 
-  EXPECT_EQ(ItAssign.getElement(), First.element);
+  // Different sentinel iterators are different.
+  EXPECT_NE(List->getElementsAsNodesAndDelimitersBeforeBegin(),
+List->getElementsAsNodesAndDelimitersEnd());
+}
 
-  auto It2 = ++(++List->getElementsAsNodesAndDelimitersBeforeBegin());
-  EXPECT_EQ(It, It2);
-  EXPECT_EQ(It.getElement(), It2.getElement());
-  EXPECT_EQ(It.getDelimiter(), It2.getDelimiter());
+TEST_P(ListTest, List_Separated_Iterator_Parent) {
+  if (!GetParam().isCXX()) {
+return;
+  }
+  buildTree("", GetParam());
+
+  // "a  b,  ,"
+  auto *List = dyn_cast(syntax::createTree(
+  *Arena,
+  {
+  {createLeaf(*Arena, tok::identifier, "a"), NodeRole::ListElement},
+  {createLeaf(*Arena, tok::identifier, "b"), NodeRole::ListElement},
+  {createLeaf(*Arena, tok::comma), NodeRole::ListDelimiter},
+  {createLeaf(*Arena, tok::comma), NodeRole::ListDelimiter},
+  },
+  NodeKind::CallArguments));
+
+  EXPECT_EQ(List->getElementsAsNodesAndDelimitersBeforeBegin().getParent(),
+List);
+
+  for (auto It = List->getElementsAsNodesAndDelimitersBegin(),
+End = List->getElementsAsNodesAndDelimitersEnd();
+   It != End; ++It)
+EXPECT_EQ(It.getParent(), List);
+
+  EXPECT_EQ(List->getElementsAsNodesAndDelimitersEnd().getParent(), List);
+}
+
+TEST_P(ListTest, List_Separated_Iterator_Equality) {
+  if (!GetParam().isCXX()) {
+return;
+  }
+  buildTree("", GetParam());
+
+  // "a  b,  ,"
+  auto *List = dyn_cast(syntax::createTree(
+  *Arena

[PATCH] D88106: [SyntaxTree] Provide iterator-like functions for Lists

2020-10-26 Thread Eduardo Caldas via Phabricator via cfe-commits

eduucaldas abandoned this revision.
eduucaldas added inline comments.

Comment at: clang/include/clang/Tooling/Syntax/Tree.h:252
+  Kind = IteratorKind::NotSentinel;
+  if (isElement(Begin))
+Current = getWithDelimiter(cast(Begin));

Since `NodeRole` is forward declared, we cannot access `NodeRole::ListElement` 
within the header, this is my work-around.

Another possibility would be to not forward declare and put `NodeRole` 
definition here. I think this is a bad choice because most roles are linked to 
syntax.

I declare `isElement` as a static private member function of `List` from lack 
of better place, I accept suggestions.

Comment at: clang/include/clang/Tooling/Syntax/Tree.h:390
+
+  ElementAndDelimiterIterator getBeforeBeginNode();
+  ElementAndDelimiterIterator getBeginNode();

I have a hard time finding a name that is expressive enough and concise enough. 
The current one seems to imply that we're getting a node...

Alternatives:
* `getBeforeBeginWithElementAsNode`
* `getBeforeBeginWithNodes`
* `getBeforeBeginBase`
* `getBeforeBeginNodeAndDelimiter`

Comment at: clang/include/clang/Tooling/Syntax/Tree.h:241-242
   /// "a; b; c"  <=> [("a" , ";"), ("b" , ";" ), ("c" , null)]
+  template 
+  class ElementAndDelimiterIterator
+  : public llvm::iterator_facade_base<

eduucaldas wrote:
> Since we're gonna provide strongly-typed iterators, I make the Iterator a 
> class template.
> 
> I keep the base functions as they were, `getElementAndDelimiterAfter...`, but 
> I cast their result using `castToElementType`.
> 
> Another option would be to make the base functions also templated and not 
> perform any casting.
> 
> We'll use `castToElementType` to provide the strongly-typed iterators as well.
Ignore comment above

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88106

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

[PATCH] D90161: [SyntaxTree] Provide iterators for Lists

2020-10-26 Thread Sam McCall via Phabricator via cfe-commits

sammccall added a comment.

(I started to review this, but now the current diff has only your changes 
against the first diff, rather than against master - can you upload a new diff?)




Comment at: clang/include/clang/Tooling/Syntax/Tree.h:231
+  /// `ElementAndDelimiter` acts as one.
+  template  class ElementAndDelimiterIterator {
+  public:

this name is bulky and hard to read, leads to more unwieldy names later (e.g. 
`getElementAndDelimiterBeforeBegin` which despite its length is unclear that 
it's even an iterator).

Would there be actual confusion in calling this `ElementIterator`, and 
providing the associated delimiters in addition to the elements?



Comment at: clang/include/clang/Tooling/Syntax/Tree.h:282
+/// Returns what you would expect from `It->element`.
+ElementType *getElement() {
+  auto *ElementOrDelimiter = getElementOrDelimiter();

nit: const (and getDelimiter)

iterators, like pointers, don't propagate const to their pointees.

(This rarely matters, but it sometimes comes up with template goop)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90161

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

[PATCH] D89488: FileManager: Shrink FileEntryRef to the size of a pointer

2020-10-26 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: clang/lib/Basic/FileManager.cpp:217
 FileEntry *FE;
-if (LLVM_LIKELY(FE = Value.dyn_cast()))
-  return FileEntryRef(SeenFileInsertResult.first->first(), *FE);
-return getFileRef(*Value.get(), openFile, CacheFailure);
+if (LLVM_LIKELY(FE = Value.V.dyn_cast()))
+  return FileEntryRef(*SeenFileInsertResult.first);

Can you use `isa` here instead of `dyn_cast`?



Comment at: clang/lib/Basic/FileManager.cpp:219
+  return FileEntryRef(*SeenFileInsertResult.first);
+return FileEntryRef(*Value.V.get());
   }

It looks like this accounts for one level of redirections, but the previous 
implementation could support multiple levels of redirections. Can multiple 
levels of redirections still be supported here too?



Comment at: clang/lib/Basic/FileManager.cpp:309
 // adopt FileEntryRef.
-UFE.Name = InterndFileName;
-
-return FileEntryRef(InterndFileName, UFE);
+return *(UFE.LastRef = FileEntryRef(*NamedFileEnt));
   }

Can you rewrite the FIXME above, and also the assignment return makes it less 
readable. Maybe separate out the return onto the next line?


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

https://reviews.llvm.org/D89488

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

[PATCH] D89498: HeaderSearch: Simplify use of FileEntryRef in HeaderSearch::LookupFile, NFC

2020-10-26 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits

dexonsmith added a comment.

I committed this on Friday as 74910cbbd8d1df824ab1d5e742c50641d0fee907 
.


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

https://reviews.llvm.org/D89498

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

[PATCH] D89749: SourceManager: Don't allocate an SLocEntry until it's loaded

2020-10-26 Thread Raphael Isemann via Phabricator via cfe-commits

teemperor added a comment.

>> The tests are just loading a Clang module and then trying to import some 
>> Decls into another ASTContext, so I don't think the error here is specific 
>> to LLDB. We just don't have any ASTImporter tests that exercise modules.
>
> I'll add one.

I'm actually not even sure if we have a nice framework where we could test 
modules + ASTImporter, so don't feel obligated to solve this as part of this 
patch.

>> FWIW, if you quickly want to see if your SourceManager changes break LLDB, 
>> then you can just set the LIT_FILTER to "Modules" and run `check-lldb` as 
>> LLDB's use of Clang modules are the only place where we have any valid 
>> SourceLocations within LLDB.
>
> This is helpful, thanks. I also need to deal with whatever entitlements 
> `check-lldb` needs (new machine since the last time and I remember it being 
> non-trivial) but I'll figure that out.
>
>> 1. The `I - 1` everywhere when translating `SLocEntryLoaded` values to a 
>> `LoadedSLocEntryTable` index is a bit cryptic. [...]
>
> Great comment; I think the way I'll do this is to create a simple wrapper 
> type that degrades to `Optional`.

Thanks!

>> 2. If we have this as an Optional, we might as well make the sentinel value 
>> `std::numeric_limits::max()` instead of 0 in `SLocEntryLoaded`. 
>> Then we can just all values as-is without translating them to an index.
>
> Could do, but there's a tradeoff since then a `resize` operation 
> can't/doesn't use zero-initialization. As a result I have a (weak) preference 
> for a 0-sentinel (probably my next patch will keep that semantic), but I'm 
> open to changing it.

True. Let's keep the 0 sentinel value for now then, I don't think the small 
advantage of using 2^32 is worth that trouble.

>> 3. Now that the primary purpose of `SLocEntryLoaded` is storing indices and 
>> not the loaded status, maybe something like `SLocEntryIndices` would be a 
>> better name?
>
> Good idea.
>
>> 4. Do you still have your benchmarking values (or some estimates) for this? 
>> I'm just curious how much memory this actually saves.
>
> I'll find some numbers if Vassil hasn't shared his by the time I've fixed the 
> patch.




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

https://reviews.llvm.org/D89749

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

[PATCH] D90161: [SyntaxTree] Provide iterators for Lists

2020-10-26 Thread Eduardo Caldas via Phabricator via cfe-commits

eduucaldas updated this revision to Diff 300702.
eduucaldas added a comment.

Diff against master, sorry about that


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90161

Files:
  clang/include/clang/Tooling/Syntax/Tree.h
  clang/lib/Tooling/Syntax/Nodes.cpp
  clang/lib/Tooling/Syntax/Tree.cpp
  clang/unittests/Tooling/Syntax/TreeTest.cpp

Index: clang/unittests/Tooling/Syntax/TreeTest.cpp
===
--- clang/unittests/Tooling/Syntax/TreeTest.cpp
+++ clang/unittests/Tooling/Syntax/TreeTest.cpp
@@ -11,6 +11,7 @@
 #include "clang/Tooling/Syntax/BuildTree.h"
 #include "clang/Tooling/Syntax/Nodes.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest.h"
 
 using namespace clang;
@@ -125,7 +126,7 @@
 }
 
 class ListTest : public SyntaxTreeTest {
-private:
+protected:
   std::string dumpQuotedTokensOrNull(const Node *N) {
 return N ? "'" +
StringRef(N->dumpTokens(Arena->getSourceManager()))
@@ -135,7 +136,15 @@
  : "null";
   }
 
-protected:
+  std::string
+  dumpElementAndDelimiter(const List::ElementAndDelimiter ED) {
+std::string Storage;
+llvm::raw_string_ostream OS(Storage);
+OS << "(" << dumpQuotedTokensOrNull(ED.element) << ", "
+   << dumpQuotedTokensOrNull(ED.delimiter) << ")";
+return OS.str();
+  }
+
   std::string
   dumpElementsAndDelimiters(ArrayRef> EDs) {
 std::string Storage;
@@ -145,8 +154,7 @@
 
 llvm::interleaveComma(
 EDs, OS, [&OS, this](const List::ElementAndDelimiter &ED) {
-  OS << "(" << dumpQuotedTokensOrNull(ED.element) << ", "
- << dumpQuotedTokensOrNull(ED.delimiter) << ")";
+  OS << dumpElementAndDelimiter(ED);
 });
 
 OS << "]";
@@ -351,4 +359,143 @@
   EXPECT_EQ(dumpNodes(List->getElementsAsNodes()), "['a', 'b', 'c']");
 }
 
+TEST_P(ListTest, List_Terminated_Iterator_Parent) {
+  if (!GetParam().isCXX()) {
+return;
+  }
+  buildTree("", GetParam());
+
+  // "a   b::  :: c"
+  auto *List = dyn_cast(syntax::createTree(
+  *Arena,
+  {
+  {createLeaf(*Arena, tok::identifier, "a"), NodeRole::ListElement},
+  {createLeaf(*Arena, tok::identifier, "b"), NodeRole::ListElement},
+  {createLeaf(*Arena, tok::coloncolon), NodeRole::ListDelimiter},
+  {createLeaf(*Arena, tok::coloncolon), NodeRole::ListDelimiter},
+  {createLeaf(*Arena, tok::identifier, "c"), NodeRole::ListElement},
+  },
+  NodeKind::NestedNameSpecifier));
+
+  EXPECT_EQ(List->getElementsAsNodesAndDelimitersBeforeBegin().getParent(),
+List);
+
+  for (auto It = List->getElementsAsNodesAndDelimitersBegin(),
+End = List->getElementsAsNodesAndDelimitersEnd();
+   It != End; ++It)
+EXPECT_EQ(It.getParent(), List);
+
+  EXPECT_EQ(List->getElementsAsNodesAndDelimitersEnd().getParent(), List);
+}
+
+TEST_P(ListTest, List_Terminated_Iterator_Equality) {
+  if (!GetParam().isCXX()) {
+return;
+  }
+  buildTree("", GetParam());
+
+  // "a   b::  :: c"
+  auto *List = dyn_cast(syntax::createTree(
+  *Arena,
+  {
+  {createLeaf(*Arena, tok::identifier, "a"), NodeRole::ListElement},
+  {createLeaf(*Arena, tok::identifier, "b"), NodeRole::ListElement},
+  {createLeaf(*Arena, tok::coloncolon), NodeRole::ListDelimiter},
+  {createLeaf(*Arena, tok::coloncolon), NodeRole::ListDelimiter},
+  {createLeaf(*Arena, tok::identifier, "c"), NodeRole::ListElement},
+  },
+  NodeKind::NestedNameSpecifier));
+
+  // different iterators are different.
+  for (auto BeforeIt = List->getElementsAsNodesAndDelimitersBeforeBegin(),
+End = List->getElementsAsNodesAndDelimitersEnd();
+   BeforeIt != End; ++BeforeIt) {
+auto It = BeforeIt;
+++It;
+for (; It != End; ++It) {
+  EXPECT_NE(BeforeIt, It);
+}
+  }
+
+  // Equal iterators are equal.
+  for (auto It = List->getElementsAsNodesAndDelimitersBegin(),
+It2 = List->getElementsAsNodesAndDelimitersBegin(),
+End = List->getElementsAsNodesAndDelimitersEnd();
+   It != End && It2 != End;) {
+EXPECT_EQ(++It, ++It2);
+  }
+
+  // Different sentinel iterators are different.
+  EXPECT_NE(List->getElementsAsNodesAndDelimitersBeforeBegin(),
+List->getElementsAsNodesAndDelimitersEnd());
+}
+
+TEST_P(ListTest, List_Separated_Iterator_Parent) {
+  if (!GetParam().isCXX()) {
+return;
+  }
+  buildTree("", GetParam());
+
+  // "a  b,  ,"
+  auto *List = dyn_cast(syntax::createTree(
+  *Arena,
+  {
+  {createLeaf(*Arena, tok::identifier, "a"), NodeRole::ListElement},
+  {createLeaf(*Arena, tok::identifier, "b"), NodeRole::ListElement},
+  {createLeaf(*Arena, tok::comma), NodeRole::ListDelimiter},
+  {createLeaf(*Arena, tok::comma), NodeRole::ListDelimiter},
+

[PATCH] D90079: Avoid unnecessary uses of `MDNode::getTemporary`, NFC

2020-10-26 Thread Adrian Prantl via Phabricator via cfe-commits

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

The `push_back(nullptr)` reads weird to me, but it is an accurate reflection of 
what the IR will look like.


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

https://reviews.llvm.org/D90079

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

[PATCH] D90054: Added remotely ran compiler-rt tests.

2020-10-26 Thread Alex Orlov via Phabricator via cfe-commits

aorlov updated this revision to Diff 300708.
aorlov added a comment.

Disable compiler-rt crt tests for cross ARM builders since the tests are broken.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90054

Files:
  clang/cmake/caches/CrossWinToARMLinux.cmake


Index: clang/cmake/caches/CrossWinToARMLinux.cmake
===
--- clang/cmake/caches/CrossWinToARMLinux.cmake
+++ clang/cmake/caches/CrossWinToARMLinux.cmake
@@ -71,7 +71,9 @@
 set(COMPILER_RT_BUILD_XRAY  OFF CACHE BOOL "")
 set(COMPILER_RT_BUILD_LIBFUZZER OFF CACHE BOOL "")
 set(COMPILER_RT_BUILD_PROFILE   OFF CACHE BOOL "")
+set(COMPILER_RT_BUILD_CRT   OFF CACHE BOOL "")
 set(COMPILER_RT_DEFAULT_TARGET_ONLY ON CACHE BOOL "")
+set(COMPILER_RT_INCLUDE_TESTS   ON CACHE BOOL "")
 
 set(LIBUNWIND_USE_COMPILER_RT   ON CACHE BOOL "")
 set(LIBUNWIND_TARGET_TRIPLE "${CMAKE_C_COMPILER_TARGET}" CACHE 
STRING "")
@@ -109,6 +111,9 @@
   set(DEFAULT_TEST_TARGET_INFO  
"libcxx.test.target_info.LinuxRemoteTI")
 
   # Allow override with the custom values.
+  if(NOT DEFINED COMPILER_RT_EMULATOR)
+set(COMPILER_RT_EMULATOR"\\\"${Python3_EXECUTABLE}\\\" 
\\\"${LLVM_PROJECT_DIR}/llvm/utils/remote-exec.py\\\" --execdir %%T 
--exec-pattern='.*\\.c.*\\.tmp.*' 
--host='${REMOTE_TEST_USER}@${REMOTE_TEST_HOST}'" CACHE STRING "")
+  endif()
   if(NOT DEFINED LIBUNWIND_TARGET_INFO)
 set(LIBUNWIND_TARGET_INFO   "${DEFAULT_TEST_TARGET_INFO}" 
CACHE STRING "")
   endif()


Index: clang/cmake/caches/CrossWinToARMLinux.cmake
===
--- clang/cmake/caches/CrossWinToARMLinux.cmake
+++ clang/cmake/caches/CrossWinToARMLinux.cmake
@@ -71,7 +71,9 @@
 set(COMPILER_RT_BUILD_XRAY  OFF CACHE BOOL "")
 set(COMPILER_RT_BUILD_LIBFUZZER OFF CACHE BOOL "")
 set(COMPILER_RT_BUILD_PROFILE   OFF CACHE BOOL "")
+set(COMPILER_RT_BUILD_CRT   OFF CACHE BOOL "")
 set(COMPILER_RT_DEFAULT_TARGET_ONLY ON CACHE BOOL "")
+set(COMPILER_RT_INCLUDE_TESTS   ON CACHE BOOL "")
 
 set(LIBUNWIND_USE_COMPILER_RT   ON CACHE BOOL "")
 set(LIBUNWIND_TARGET_TRIPLE "${CMAKE_C_COMPILER_TARGET}" CACHE STRING "")
@@ -109,6 +111,9 @@
   set(DEFAULT_TEST_TARGET_INFO  "libcxx.test.target_info.LinuxRemoteTI")
 
   # Allow override with the custom values.
+  if(NOT DEFINED COMPILER_RT_EMULATOR)
+set(COMPILER_RT_EMULATOR"\\\"${Python3_EXECUTABLE}\\\" \\\"${LLVM_PROJECT_DIR}/llvm/utils/remote-exec.py\\\" --execdir %%T --exec-pattern='.*\\.c.*\\.tmp.*' --host='${REMOTE_TEST_USER}@${REMOTE_TEST_HOST}'" CACHE STRING "")
+  endif()
   if(NOT DEFINED LIBUNWIND_TARGET_INFO)
 set(LIBUNWIND_TARGET_INFO   "${DEFAULT_TEST_TARGET_INFO}" CACHE STRING "")
   endif()
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] c686dfd - Unconfuse gcc5.3 after 2e204e23911b1f / D87528

2020-10-26 Thread Nico Weber via cfe-commits


Author: Nico Weber
Date: 2020-10-26T12:55:38-04:00
New Revision: c686dfd61705f33419875b22eb08bd197a72cd18

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

LOG: Unconfuse gcc5.3 after 2e204e23911b1f / D87528

The local variable CmpResult added in that change shadowed the
type CmpResult, which confused an older gcc. Rename the variable
CmpResult to APFloatCmpResult.

Added: 


Modified: 
clang/lib/AST/ExprConstant.cpp

Removed: 




diff  --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index b8891055ab61..fa4026b865fb 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -12561,16 +12561,16 @@ EvaluateComparisonBinaryOperator(EvalInfo &Info, 
const BinaryOperator *E,
   return false;
 
 assert(E->isComparisonOp() && "Invalid binary operator!");
-llvm::APFloatBase::cmpResult CmpResult = LHS.compare(RHS);
+llvm::APFloatBase::cmpResult APFloatCmpResult = LHS.compare(RHS);
 if (!Info.InConstantContext &&
-CmpResult == APFloat::cmpUnordered &&
+APFloatCmpResult == APFloat::cmpUnordered &&
 E->getFPFeaturesInEffect(Info.Ctx.getLangOpts()).isFPConstrained()) {
   // Note: Compares may raise invalid in some cases involving NaN or sNaN.
   Info.FFDiag(E, diag::note_constexpr_float_arithmetic_strict);
   return false;
 }
 auto GetCmpRes = [&]() {
-  switch (CmpResult) {
+  switch (APFloatCmpResult) {
   case APFloat::cmpEqual:
 return CmpResult::Equal;
   case APFloat::cmpLessThan:



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

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

2020-10-26 Thread Anastasia Stulova via Phabricator via cfe-commits

Anastasia added a comment.

In the RFC it has been discussed to either use target address spaces or perhaps 
to introduce a new attribute to reflect a semantic needed for SYCL, but it 
seems to me in this change you are building on top of the existing language 
address space attribute with new entries for SYCL.

http://lists.llvm.org/pipermail/cfe-dev/2020-August/066632.html

Has there been any other discussion regarding the appraoch which is not 
captured in the cfe-dev thread I have linked?




Comment at: clang/lib/CodeGen/CGExpr.cpp:4554
+
+  // Language rules define if it is legal to cast from one address space
+  // to another, and which address space we should use as a "common

What language rules?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909

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

[PATCH] D90161: [SyntaxTree] Provide iterators for Lists

2020-10-26 Thread Sam McCall via Phabricator via cfe-commits

sammccall added a comment.

This seems like more implementation and API complexity than is justified by the 
problems it's solving.
I do think it's possible to drastically simplify it (I've left some suggestions 
here) but if not, I think we should move it to its own header.




Comment at: clang/include/clang/Tooling/Syntax/Tree.h:231
+  /// `ElementAndDelimiter` acts as one.
+  template  class ElementAndDelimiterIterator {
+  public:

sammccall wrote:
> this name is bulky and hard to read, leads to more unwieldy names later (e.g. 
> `getElementAndDelimiterBeforeBegin` which despite its length is unclear that 
> it's even an iterator).
> 
> Would there be actual confusion in calling this `ElementIterator`, and 
> providing the associated delimiters in addition to the elements?
overall, this design seems to be focused on the size being a single pointer, at 
the cost of complexity of most of the operations.

Why is this important to optimize for, compared to doing something like:
```
private:
  Node *Elt, *Delim, *Next;
  void advance() {
// read one element
// set Elt and/or Delim
// always advances Next by at least one
  }
public:
  Iterator(Node *Start) : Elt(nullptr), Delim(nullptr), Next(Start) {}
  operator++() { advance(); }
  Leaf *getDelimiter() { return Delim; }
  Node *getElement() { return Elt; }
```

(Performance isn't the biggest concern here, but I'd usually expect two extra 
pointers on the stack to be preferable to all the branches in the current impl)



Comment at: clang/include/clang/Tooling/Syntax/Tree.h:231
+  /// `ElementAndDelimiter` acts as one.
+  template  class ElementAndDelimiterIterator {
+  public:

sammccall wrote:
> sammccall wrote:
> > this name is bulky and hard to read, leads to more unwieldy names later 
> > (e.g. `getElementAndDelimiterBeforeBegin` which despite its length is 
> > unclear that it's even an iterator).
> > 
> > Would there be actual confusion in calling this `ElementIterator`, and 
> > providing the associated delimiters in addition to the elements?
> overall, this design seems to be focused on the size being a single pointer, 
> at the cost of complexity of most of the operations.
> 
> Why is this important to optimize for, compared to doing something like:
> ```
> private:
>   Node *Elt, *Delim, *Next;
>   void advance() {
> // read one element
> // set Elt and/or Delim
> // always advances Next by at least one
>   }
> public:
>   Iterator(Node *Start) : Elt(nullptr), Delim(nullptr), Next(Start) {}
>   operator++() { advance(); }
>   Leaf *getDelimiter() { return Delim; }
>   Node *getElement() { return Elt; }
> ```
> 
> (Performance isn't the biggest concern here, but I'd usually expect two extra 
> pointers on the stack to be preferable to all the branches in the current 
> impl)
how sure are we that the template and strong typing is worthwhile on the 
iterators? Can we try without it for a while and see how painful it is?



Comment at: clang/include/clang/Tooling/Syntax/Tree.h:235
+  switch (getKind()) {
+  case IteratorKind::BeforeBegin: {
+if (auto *Begin = getParent()->getFirstChild())

Are the before-begin iterators needed immediately?

My understanding is that before-begin iterators are used with nodes due to the 
single-linked-list implementation, but that's going away. Can we avoid adding 
more of these by sequencing those changes differently?



Comment at: clang/include/clang/Tooling/Syntax/Tree.h:242
+  }
+  case IteratorKind::NotSentinel: {
+auto *DelimiterOrElement = getDelimiterOrElement();

NotSentinel -> Element?

Logically, the iterator is pointing at an element in the list. (If the element 
happens to be missing, fine...)



Comment at: clang/include/clang/Tooling/Syntax/Tree.h:272
+
+// An `operator*` would return an `ElementAndDelimiter`, but it would 
return
+// it as a value instead of the expected reference, since this

If we're not willing for this to be an InputIterator (I'm still not clear *why* 
not) or a stashing iterator (ditto), then maybe we might as well define 
`operator*` to refer to `Element` so people only have to use explicit iterators 
if they care about delimiters?



Comment at: clang/include/clang/Tooling/Syntax/Tree.h:285
+  if (ElementOrDelimiter && isElement(ElementOrDelimiter))
+return cast(ElementOrDelimiter);
+  return nullptr;

this is a crash if the list contains elements that have a type that doesn't 
match the container's expected type.

If we really need the strongly-typed version of this for constrained lists, we 
should define behavior here - probably return nullptr, just like an accessor on 
Tree for a strongly-typed single child whose type doesn't match what's expected.



Comment at: clang/include/clang/Tooling/

[clang] dd7095f - [clang][unittest] Don't hardcode the string "Assertion"

2020-10-26 Thread Benjamin Kramer via cfe-commits


Author: Benjamin Kramer
Date: 2020-10-26T18:10:56+01:00
New Revision: dd7095f52bda36e0f3cd37574a1cb97c7a46cffe

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

LOG: [clang][unittest] Don't hardcode the string "Assertion"

This depends on the libc implementation. Use the string from the
assertion message instead. Overly specific, but so is this entire test.

Added: 


Modified: 
clang/unittests/Basic/LineOffsetMappingTest.cpp

Removed: 




diff  --git a/clang/unittests/Basic/LineOffsetMappingTest.cpp 
b/clang/unittests/Basic/LineOffsetMappingTest.cpp
index e04081378283..b50374a449c8 100644
--- a/clang/unittests/Basic/LineOffsetMappingTest.cpp
+++ b/clang/unittests/Basic/LineOffsetMappingTest.cpp
@@ -20,7 +20,7 @@ TEST(LineOffsetMappingTest, empty) {
   EXPECT_FALSE(Mapping);
 
 #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
-  EXPECT_DEATH((void)Mapping.getLines(), "Assertion");
+  EXPECT_DEATH((void)Mapping.getLines(), "Storage");
 #endif
 }
 
@@ -34,7 +34,7 @@ TEST(LineOffsetMappingTest, construct) {
   EXPECT_EQ(20u, Mapping[2]);
 
 #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
-  EXPECT_DEATH((void)Mapping[3], "Assertion");
+  EXPECT_DEATH((void)Mapping[3], "Invalid index");
 #endif
 }
 



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

[PATCH] D89986: [AIX] do not emit visibility attribute into IR when there is -mignore-xcoff-visibility

2020-10-26 Thread Jason Liu via Phabricator via cfe-commits

jasonliu added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:520
   Options.DataSections = CodeGenOpts.DataSections;
-  Options.IgnoreXCOFFVisibility = CodeGenOpts.IgnoreXCOFFVisibility;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;

DiggerLin wrote:
> jasonliu wrote:
> > Instead of just removing this line, should this get replaced with the new 
> > LangOpts option?
> I do not think we need a CodeGenOp of ignore-xcoff-visibility in clang, we 
> only need the LangOpt of the ignore-xcoff-visilbity to control whether we 
> will  generate the visibility in the IR,  when the LangOpt of 
> ignore-xcoff-visibility do not generate the visibility attribute of GV in the 
> IR. it do not need CodeGenOp of ignore-xcoff-visibility any more for the 
> clang .
> 
> we have still CodeGen ignore-xcoff-visibility op in  llc.
We removed the visibility from IR level with this patch. But there is also 
visibility settings coming from CodeGen part of clang, which needs to get 
ignore when we are doing the code gen in llc. So I think you still need to set 
the options correct for llc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89986

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

1 2 3 >

1 - 100 of 239 matches

Mail list logo