[cfe-commits] r127425 - /cfe/trunk/lib/Sema/SemaChecking.cpp

[Chris Lattner]

On Mar 10, 2011, at 1:22 PM, Ted Kremenek wrote:

> The spelling location is checked because the diagnostic gets pruned if:
> 
> 1) The diagnostic was instantiated from a macro
> 
> AND
> 
> 2) That macro was from a system header.
> 
> #2 is the reason getSpellingLoc() is called.

Right, I get that... but why is this to right policy?

-Chris

> 
> On Mar 10, 2011, at 1:09 PM, Chris Lattner wrote:
> 
>> 
>> On Mar 10, 2011, at 12:03 PM, Ted Kremenek wrote:
>> 
>>> Author: kremenek
>>> Date: Thu Mar 10 14:03:42 2011
>>> New Revision: 127425
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=127425&view=rev
>>> Log:
>>> Profiling showed that 'CheckImplicitConversions' was very slow because of the call to getSpellingLoc().  On 'aes.c'
>>> in the LLVM test suite, this function was consuming 7.4% of -fsyntax-only time.  This change fixes this issue
>>> by delaying the check that the warning would be issued within a system macro by as long as possible.  The
>>> main negative of this change is now the logic for this check is done in multiple places in this function instead
>>> of just in one place up front.
>> 
>> Nice catch Ted.  Why are we checking the spelling location in any case?  Whether a token came from a system header doesn't sound that interesting to me, and *is* very expensive to compute.
>> 
>> -Chris
>> 
>>> 
>>> Modified:
>>>  cfe/trunk/lib/Sema/SemaChecking.cpp
>>> 
>>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=127425&r1=127424&r2=127425&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Mar 10 14:03:42 2011
>>> @@ -2762,6 +2762,11 @@
>>> return ValueInRange.toString(10);
>>> }
>>> 
>>> +static bool isFromSystemMacro(Sema &S, SourceLocation loc) {
>>> +  SourceManager &smgr = S.Context.getSourceManager();
>>> +  return loc.isMacroID() && smgr.isInSystemHeader(smgr.getSpellingLoc(loc));
>>> +}
>>> +  
>>> void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
>>>                            SourceLocation CC, bool *ICContext = 0) {
>>> if (E->isTypeDependent() || E->isValueDependent()) return;
>>> @@ -2771,11 +2776,12 @@
>>> if (Source == Target) return;
>>> if (Target->isDependentType()) return;
>>> 
>>> -  // If the conversion context location is invalid or instantiated
>>> -  // from a system macro, don't complain.
>>> -  if (CC.isInvalid() ||
>>> -      (CC.isMacroID() && S.Context.getSourceManager().isInSystemHeader(
>>> -                           S.Context.getSourceManager().getSpellingLoc(CC))))
>>> +  // If the conversion context location is invalid don't complain.
>>> +  // We also don't want to emit a warning if the issue occurs from the
>>> +  // instantiation of a system macro.  The problem is that 'getSpellingLoc()'
>>> +  // is slow, so we delay this check as long as possible.  Once we detect
>>> +  // we are in that scenario, we just return.
>>> +  if (CC.isInvalid())
>>>   return;
>>> 
>>> // Never diagnose implicit casts to bool.
>>> @@ -2784,8 +2790,11 @@
>>> 
>>> // Strip vector types.
>>> if (isa<VectorType>(Source)) {
>>> -    if (!isa<VectorType>(Target))
>>> +    if (!isa<VectorType>(Target)) {
>>> +      if (isFromSystemMacro(S, CC))
>>> +        return;
>>>     return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_vector_scalar);
>>> +    }
>>> 
>>>   Source = cast<VectorType>(Source)->getElementType().getTypePtr();
>>>   Target = cast<VectorType>(Target)->getElementType().getTypePtr();
>>> @@ -2793,8 +2802,12 @@
>>> 
>>> // Strip complex types.
>>> if (isa<ComplexType>(Source)) {
>>> -    if (!isa<ComplexType>(Target))
>>> +    if (!isa<ComplexType>(Target)) {
>>> +      if (isFromSystemMacro(S, CC))
>>> +        return;
>>> +
>>>     return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_complex_scalar);
>>> +    }
>>> 
>>>   Source = cast<ComplexType>(Source)->getElementType().getTypePtr();
>>>   Target = cast<ComplexType>(Target)->getElementType().getTypePtr();
>>> @@ -2822,13 +2835,19 @@
>>>           return;
>>>       }
>>> 
>>> +        if (isFromSystemMacro(S, CC))
>>> +          return;
>>> +
>>>       DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_float_precision);
>>>     }
>>>     return;
>>>   }
>>> 
>>> -    // If the target is integral, always warn.
>>> +    // If the target is integral, always warn.    
>>>   if ((TargetBT && TargetBT->isInteger())) {
>>> +      if (isFromSystemMacro(S, CC))
>>> +        return;
>>> +      
>>>     Expr *InnerE = E->IgnoreParenImpCasts();
>>>     if (FloatingLiteral *LiteralExpr = dyn_cast<FloatingLiteral>(InnerE)) {
>>>       DiagnoseImpCast(S, LiteralExpr, T, CC,
>>> @@ -2852,6 +2871,9 @@
>>>   // TODO: this should happen for bitfield stores, too.
>>>   llvm::APSInt Value(32);
>>>   if (E->isIntegerConstantExpr(Value, S.Context)) {
>>> +      if (isFromSystemMacro(S, CC))
>>> +        return;
>>> +
>>>     std::string PrettySourceValue = Value.toString(10);
>>>     std::string PrettyTargetValue = PrettyPrintInRange(Value, TargetRange);
>>> 
>>> @@ -2863,6 +2885,10 @@
>>> 
>>>   // People want to build with -Wshorten-64-to-32 and not -Wconversion
>>>   // and by god we'll let them.
>>> +    
>>> +    if (isFromSystemMacro(S, CC))
>>> +      return;
>>> +    
>>>   if (SourceRange.Width == 64 && TargetRange.Width == 32)
>>>     return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_integer_64_32);
>>>   return DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_integer_precision);
>>> @@ -2871,6 +2897,10 @@
>>> if ((TargetRange.NonNegative && !SourceRange.NonNegative) ||
>>>     (!TargetRange.NonNegative && SourceRange.NonNegative &&
>>>      SourceRange.Width == TargetRange.Width)) {
>>> +        
>>> +    if (isFromSystemMacro(S, CC))
>>> +      return;
>>> +
>>>   unsigned DiagID = diag::warn_impcast_integer_sign;
>>> 
>>>   // Traditionally, gcc has warned about this under -Wsign-compare.
>>> @@ -2893,9 +2923,13 @@
>>>          SourceEnum->getDecl()->getTypedefForAnonDecl()) &&
>>>         (TargetEnum->getDecl()->getIdentifier() ||
>>>          TargetEnum->getDecl()->getTypedefForAnonDecl()) &&
>>> -          SourceEnum != TargetEnum)
>>> +          SourceEnum != TargetEnum) {
>>> +        if (isFromSystemMacro(S, CC))
>>> +          return;
>>> +
>>>       return DiagnoseImpCast(S, E, T, CC, 
>>>                              diag::warn_impcast_different_enum_types);
>>> +      }
>>> 
>>> return;
>>> }
>>> 
>>> 
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> 
>