[cfe-commits] r131260 - in /cfe/trunk: include/clang/Sema/Sema.h lib/Sema/SemaDeclCXX.cpp lib/Sema/SemaExprCXX.cpp lib/Sema/SemaInit.cpp lib/Serialization/ASTReaderDecl.cpp lib/Serialization/ASTWriterDecl.cpp

[Chandler Carruth]

Hate to dig up an old commit...

On Thu, May 12, 2011 at 3:46 PM, Sean Hunt <scshunt at csclub.uwaterloo.ca>wrote:

> Modified: cfe/trunk/lib/Sema/SemaExprCXX.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprCXX.cpp?rev=131260&r1=131259&r2=131260&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaExprCXX.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaExprCXX.cpp Thu May 12 17:46:29 2011
> @@ -1327,11 +1327,12 @@
>  bool Sema::FindAllocationOverload(SourceLocation StartLoc, SourceRange
> Range,
>                                   DeclarationName Name, Expr** Args,
>                                   unsigned NumArgs, DeclContext *Ctx,
> -                                  bool AllowMissing, FunctionDecl
> *&Operator) {
> +                                  bool AllowMissing, FunctionDecl
> *&Operator,
> +                                  bool Diagnose) {
>

I'm now thinking this pattern (the Diagnose bool) is more than annoying,
it's actively harmful. Why can't we use one of the scoped diagnostic
suppression utilities in Sema instead? That has quite a few advantages:
1) It doesn't require threading parameters everywhere
2) It is more localized to the area of the code that needs to suppress
diagnostics
3) It doesn't suffer from programmer errors such as:


>   case OR_No_Viable_Function:
> -    Diag(StartLoc, diag::err_ovl_no_viable_function_in_call)
> -      << Name << Range;
> +    if (Diagnose)
> +      Diag(StartLoc, diag::err_ovl_no_viable_function_in_call)
> +        << Name << Range;
>     Candidates.NoteCandidates(*this, OCD_AllCandidates, Args, NumArgs);
>     return true;
>

The notes aren't conditioned here, and so are emitted and attached to
whatever diagnostic happened to precede it. I know you fixed a bunch of
these, but I just fixed more in r132746.

I understand the disadvantage of the scoped object too -- it makes it easy
to forget to short-circuit any expensive operations performed only to
produce diagnostics which are then suppressed. However, we could make this
easier, potentially providing a way to more directly attach a callback for
generating the (expensive) notes to a particular diagnostic, or just making
easy query APIs for Sema-level suppression state and paying attention to
profiles.

Anyways, wanted to bring this up to see if there is a better pattern to
follow, and as a reminder that you may want to audit some of the other
places you're using this pattern currently for similar bugs.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110608/e4c43431/attachment.html>