subject:"\[PATCH\] D61357\: SemaOverload\: Complete candidates before emitting the error, to ensure diagnostics emitted \(or suppressed\) during completion don't interfere with the overload notes"

[PATCH] D61357: SemaOverload: Complete candidates before emitting the error, to ensure diagnostics emitted (or suppressed) during completion don't interfere with the overload notes

2019-05-02 Thread David Blaikie via Phabricator via cfe-commits

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

In D61357#1488839 , @rsmith wrote:

> In D61357#1485581 , @dblaikie wrote:
>
> > Oh, @rsmith - if there's any better/different testing (if you can figure 
> > out how to reduce the test case down further, now that we know the cause - 
> > or if you'd like testing for other codepaths I've touched in this patch) 
> > I'm all ears. (also naming/API wrangling - my changes were just the minimal 
> > sort of "this works" based on what we discussed, but totally happy to do 
> > more involved/different things if there's such to be done)
>
>
> Perhaps we could add a diagnostic scope object of some kind, which would 
> assert (or reorder the diagnostics?) if a diagnostic is produced while 
> producing notes for a different diagnostic. That might help for new code and 
> for places where we're aware -- or suspect -- there is a problem, but we 
> don't know where the existing bugs are and that doesn't help us find them. :(


Yeah, I was thinking about something like that. Yeah, doesn't seem any reason 
we couldn't retrofit something on top of the diagnostics infratsructure - or a 
utility in it that connects notes to their diagnostic & either asserts, or 
delays emission, etc.

I was thinking about that in the context of cleaning up /everything/ to use 
that, but yeah - we could do it as an incremental migration/as needed sort of 
thing.




Comment at: lib/Sema/SemaOverload.cpp:3518-3519
   << false << From->getType() << From->getSourceRange() << ToType;
   } else
 return false;
+

rsmith wrote:
> Can we avoid calling `CompleteCandidates` on this path?
Yeah, I raised up the two if conditions to a pre-check:

if (!(OvResult == OR_Ambiguous ||
  (OvResult == OR_No_Viable_Function && !CandidateSet.empty(
  return false;

auto Cands = ...
if (...Ambiguous)
  ...
else { // No_Viable_Function && !empty()
  ...
NoteCandidates

(open to other ideas, couldn't quite think of anything that didn't involve some 
duplication - either duplicating the conditions or the CompleteCandidates call, 
etc)


Repository:
  rC Clang

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

https://reviews.llvm.org/D61357



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

[PATCH] D61357: SemaOverload: Complete candidates before emitting the error, to ensure diagnostics emitted (or suppressed) during completion don't interfere with the overload notes

2019-05-02 Thread David Blaikie via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rC359854: SemaOverload: Complete candidates before emitting 
the error, to ensure… (authored by dblaikie, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D61357?vs=197491=197901#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D61357

Files:
  include/clang/AST/TemplateName.h
  include/clang/Basic/PartialDiagnostic.h
  include/clang/Sema/Overload.h
  lib/AST/TemplateName.cpp
  lib/Sema/SemaCast.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaInit.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaStmt.cpp
  test/SemaCXX/overload-template.cpp

Index: lib/AST/TemplateName.cpp
===
--- lib/AST/TemplateName.cpp
+++ lib/AST/TemplateName.cpp
@@ -250,6 +250,20 @@
   return DB << NameStr;
 }
 
+const PartialDiagnostic::operator<<(const PartialDiagnostic ,
+   TemplateName N) {
+  std::string NameStr;
+  llvm::raw_string_ostream OS(NameStr);
+  LangOptions LO;
+  LO.CPlusPlus = true;
+  LO.Bool = true;
+  OS << '\'';
+  N.print(OS, PrintingPolicy(LO));
+  OS << '\'';
+  OS.flush();
+  return PD << NameStr;
+}
+
 void TemplateName::dump(raw_ostream ) const {
   LangOptions LO;  // FIXME!
   LO.CPlusPlus = true;
Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -2227,9 +2227,11 @@
   return Sema::FRS_Success;
 
 case Sema::FRS_NoViableFunction:
-  SemaRef.Diag(BeginRange->getBeginLoc(), diag::err_for_range_invalid)
-  << BeginRange->getType() << BEFFound;
-  CandidateSet->NoteCandidates(SemaRef, OCD_AllCandidates, BeginRange);
+  CandidateSet->NoteCandidates(
+  PartialDiagnosticAt(BeginRange->getBeginLoc(),
+  SemaRef.PDiag(diag::err_for_range_invalid)
+  << BeginRange->getType() << BEFFound),
+  SemaRef, OCD_AllCandidates, BeginRange);
   LLVM_FALLTHROUGH;
 
 case Sema::FRS_DiagnosticIssued:
@@ -2526,9 +2528,12 @@
   // Otherwise, emit diagnostics if we haven't already.
   if (RangeStatus == FRS_NoViableFunction) {
 Expr *Range = BEFFailure ? EndRangeRef.get() : BeginRangeRef.get();
-Diag(Range->getBeginLoc(), diag::err_for_range_invalid)
-<< RangeLoc << Range->getType() << BEFFailure;
-CandidateSet.NoteCandidates(*this, OCD_AllCandidates, Range);
+CandidateSet.NoteCandidates(
+PartialDiagnosticAt(Range->getBeginLoc(),
+PDiag(diag::err_for_range_invalid)
+<< RangeLoc << Range->getType()
+<< BEFFailure),
+*this, OCD_AllCandidates, Range);
   }
   // Return an error if no fix was discovered.
   if (RangeStatus != FRS_Success)
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -3505,18 +3505,25 @@
   OverloadingResult OvResult =
 IsUserDefinedConversion(*this, From, ToType, ICS.UserDefined,
 CandidateSet, false, false);
+
+  if (!(OvResult == OR_Ambiguous ||
+(OvResult == OR_No_Viable_Function && !CandidateSet.empty(
+return false;
+
+  auto Cands = CandidateSet.CompleteCandidates(*this, OCD_AllCandidates, From);
   if (OvResult == OR_Ambiguous)
 Diag(From->getBeginLoc(), diag::err_typecheck_ambiguous_condition)
 << From->getType() << ToType << From->getSourceRange();
-  else if (OvResult == OR_No_Viable_Function && !CandidateSet.empty()) {
+  else { // OR_No_Viable_Function && !CandidateSet.empty()
 if (!RequireCompleteType(From->getBeginLoc(), ToType,
  diag::err_typecheck_nonviable_condition_incomplete,
  From->getType(), From->getSourceRange()))
   Diag(From->getBeginLoc(), diag::err_typecheck_nonviable_condition)
   << false << From->getType() << From->getSourceRange() << ToType;
-  } else
-return false;
-  CandidateSet.NoteCandidates(*this, OCD_AllCandidates, From);
+  }
+
+  CandidateSet.NoteCandidates(
+  *this, From, Cands);
   return true;
 }
 
@@ -10745,11 +10752,9 @@
   }
 }
 
-/// When overload resolution fails, prints diagnostic messages containing the
-/// candidates in the candidate set.
-void OverloadCandidateSet::NoteCandidates(
+SmallVector OverloadCandidateSet::CompleteCandidates(
 Sema , OverloadCandidateDisplayKind OCD, ArrayRef Args,
-StringRef Opc, SourceLocation OpLoc,
+SourceLocation OpLoc,
 llvm::function_ref Filter) {
   // Sort the candidates by viability and position.  Sorting

[PATCH] D61357: SemaOverload: Complete candidates before emitting the error, to ensure diagnostics emitted (or suppressed) during completion don't interfere with the overload notes

2019-05-02 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added a comment.

In D61357#1485581 , @dblaikie wrote:

> Oh, @rsmith - if there's any better/different testing (if you can figure out 
> how to reduce the test case down further, now that we know the cause - or if 
> you'd like testing for other codepaths I've touched in this patch) I'm all 
> ears. (also naming/API wrangling - my changes were just the minimal sort of 
> "this works" based on what we discussed, but totally happy to do more 
> involved/different things if there's such to be done)

Perhaps we could add a diagnostic scope object of some kind, which would assert 
(or reorder the diagnostics?) if a diagnostic is produced while producing notes 
for a different diagnostic. That might help for new code and for places where 
we're aware -- or suspect -- there is a problem, but we don't know where the 
existing bugs are and that doesn't help us find them. :(

Repository:
  rC Clang

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

https://reviews.llvm.org/D61357

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

[PATCH] D61357: SemaOverload: Complete candidates before emitting the error, to ensure diagnostics emitted (or suppressed) during completion don't interfere with the overload notes

2019-05-02 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

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

LGTM, thanks!




Comment at: lib/Sema/SemaOverload.cpp:3518-3519
   << false << From->getType() << From->getSourceRange() << ToType;
   } else
 return false;
+

Can we avoid calling `CompleteCandidates` on this path?


Repository:
  rC Clang

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

https://reviews.llvm.org/D61357



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

[PATCH] D61357: SemaOverload: Complete candidates before emitting the error, to ensure diagnostics emitted (or suppressed) during completion don't interfere with the overload notes

2019-04-30 Thread David Blaikie via Phabricator via cfe-commits

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

Because diagnostics and their notes are not connected at the API level,
if the error message for an overload is emitted, then the overload
candidates are completed - if a diagnostic is emitted during that work,
the notes related to overload candidates would be attached to the latter
diagnostic, not the original error. Sort of worse, if the latter
diagnostic was disabled, the notes are disabled.


Repository:
  rC Clang

https://reviews.llvm.org/D61357

Files:
  include/clang/AST/TemplateName.h
  include/clang/Basic/PartialDiagnostic.h
  include/clang/Sema/Overload.h
  lib/AST/TemplateName.cpp
  lib/Sema/SemaCast.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaInit.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaStmt.cpp
  test/SemaCXX/overload-template.cpp

Index: test/SemaCXX/overload-template.cpp
===
--- /dev/null
+++ test/SemaCXX/overload-template.cpp
@@ -0,0 +1,35 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+enum copy_traits { movable = 1 };
+
+template 
+struct optional_ctor_base {};
+template 
+struct ctor_copy_traits {
+  // this would produce a c++98-compat warning, which would erroneously get the
+  // no-matching-function-call error's notes attached to it (or suppress those
+  // notes if this diagnostic was suppressed, as it is in this case)
+  static constexpr int traits = copy_traits::movable;
+};
+template 
+struct optional : optional_ctor_base::traits> {
+  template 
+  constexpr optional(U&& v);
+};
+struct A {};
+struct XA {
+  XA(const A&);
+};
+struct B {};
+struct XB {
+  XB(const B&);
+  XB(const optional&);
+};
+struct YB : XB {
+  using XB::XB;
+};
+void InsertRow(const XA&, const YB&); // expected-note {{candidate function not viable: no known conversion from 'int' to 'const XA' for 1st argument}}
+void ReproducesBugSimply() {
+  InsertRow(3, B{}); // expected-error {{no matching function for call to 'InsertRow'}}
+}
+
Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -2224,9 +2224,11 @@
   return Sema::FRS_Success;
 
 case Sema::FRS_NoViableFunction:
-  SemaRef.Diag(BeginRange->getBeginLoc(), diag::err_for_range_invalid)
-  << BeginRange->getType() << BEFFound;
-  CandidateSet->NoteCandidates(SemaRef, OCD_AllCandidates, BeginRange);
+  CandidateSet->NoteCandidates(
+  PartialDiagnosticAt(BeginRange->getBeginLoc(),
+  SemaRef.PDiag(diag::err_for_range_invalid)
+  << BeginRange->getType() << BEFFound),
+  SemaRef, OCD_AllCandidates, BeginRange);
   LLVM_FALLTHROUGH;
 
 case Sema::FRS_DiagnosticIssued:
@@ -2523,9 +2525,12 @@
   // Otherwise, emit diagnostics if we haven't already.
   if (RangeStatus == FRS_NoViableFunction) {
 Expr *Range = BEFFailure ? EndRangeRef.get() : BeginRangeRef.get();
-Diag(Range->getBeginLoc(), diag::err_for_range_invalid)
-<< RangeLoc << Range->getType() << BEFFailure;
-CandidateSet.NoteCandidates(*this, OCD_AllCandidates, Range);
+CandidateSet.NoteCandidates(
+PartialDiagnosticAt(Range->getBeginLoc(),
+PDiag(diag::err_for_range_invalid)
+<< RangeLoc << Range->getType()
+<< BEFFailure),
+*this, OCD_AllCandidates, Range);
   }
   // Return an error if no fix was discovered.
   if (RangeStatus != FRS_Success)
Index: lib/Sema/SemaOverload.cpp
===
--- lib/Sema/SemaOverload.cpp
+++ lib/Sema/SemaOverload.cpp
@@ -3504,6 +3504,8 @@
   OverloadingResult OvResult =
 IsUserDefinedConversion(*this, From, ToType, ICS.UserDefined,
 CandidateSet, false, false);
+
+  auto Cands = CandidateSet.CompleteCandidates(*this, OCD_AllCandidates, From);
   if (OvResult == OR_Ambiguous)
 Diag(From->getBeginLoc(), diag::err_typecheck_ambiguous_condition)
 << From->getType() << ToType << From->getSourceRange();
@@ -3515,7 +3517,9 @@
   << false << From->getType() << From->getSourceRange() << ToType;
   } else
 return false;
-  CandidateSet.NoteCandidates(*this, OCD_AllCandidates, From);
+
+  CandidateSet.NoteCandidates(
+  *this, From, Cands);
   return true;
 }
 
@@ -10744,11 +10748,9 @@
   }
 }
 
-/// When overload resolution fails, prints diagnostic messages containing the
-/// candidates in the candidate set.
-void OverloadCandidateSet::NoteCandidates(
+SmallVector OverloadCandidateSet::CompleteCandidates(
 Sema , OverloadCandidateDisplayKind OCD, ArrayRef Args,
-StringRef Opc,

[PATCH] D61357: SemaOverload: Complete candidates before emitting the error, to ensure diagnostics emitted (or suppressed) during completion don't interfere with the overload notes

2019-04-30 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

Oh, @rsmith - if there's any better/different testing (if you can figure out 
how to reduce the test case down further, now that we know the cause - or if 
you'd like testing for other codepaths I've touched in this patch) I'm all 
ears. (also naming/API wrangling - my changes were just the minimal sort of 
"this works" based on what we discussed, but totally happy to do more 
involved/different things if there's such to be done)


Repository:
  rC Clang

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

https://reviews.llvm.org/D61357



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

[PATCH] D61357: SemaOverload: Complete candidates before emitting the error, to ensure diagnostics emitted (or suppressed) during completion don't interfere with the overload notes

[PATCH] D61357: SemaOverload: Complete candidates before emitting the error, to ensure diagnostics emitted (or suppressed) during completion don't interfere with the overload notes

[PATCH] D61357: SemaOverload: Complete candidates before emitting the error, to ensure diagnostics emitted (or suppressed) during completion don't interfere with the overload notes

[PATCH] D61357: SemaOverload: Complete candidates before emitting the error, to ensure diagnostics emitted (or suppressed) during completion don't interfere with the overload notes

[PATCH] D61357: SemaOverload: Complete candidates before emitting the error, to ensure diagnostics emitted (or suppressed) during completion don't interfere with the overload notes

[PATCH] D61357: SemaOverload: Complete candidates before emitting the error, to ensure diagnostics emitted (or suppressed) during completion don't interfere with the overload notes

6 matches

Site Navigation

Mail list logo

Footer information