Hi Michael, Thanks for the revert.
However, please also keep in mind that we have *very* limited Win32 development resources. Most Clang/LLVM developers have limited access to Windows systems, we have no buildbot coverage of VS 2010 (which isn't even released), etc. In general, until the platform is more strongly supported I recommend trying to resolve the problems, or let the developer know with some hints of what to fix, before resorting to reversion, to avoid excess churn on the tree. Cheers, - Daniel On Mon, Jul 26, 2010 at 9:51 PM, Michael Spencer <[email protected]> wrote: > I reverted this change in r109491 due to: > > 4>..\..\..\..\..\..\llvm\tools\clang\lib\Analysis\ScanfFormatString.cpp(221): > error C2065: 'ASTContext' : undeclared identifier > 4>..\..\..\..\..\..\llvm\tools\clang\lib\Analysis\ScanfFormatString.cpp(221): > error C2065: 'Ctx' : undeclared identifier > 4>..\..\..\..\..\..\llvm\tools\clang\lib\Analysis\ScanfFormatString.cpp(221): > error C2761: 'clang::analyze_format_string::ArgTypeResult > clang::analyze_scanf::ScanfSpecifier::getArgType(clang::ASTContext &) > const' : member function redeclaration not allowed > 4>..\..\..\..\..\..\llvm\tools\clang\lib\Analysis\ScanfFormatString.cpp(221): > error C2143: syntax error : missing ';' before 'const' > 4>..\..\..\..\..\..\llvm\tools\clang\lib\Analysis\ScanfFormatString.cpp(221): > error C2059: syntax error : 'const' > 4>..\..\..\..\..\..\llvm\tools\clang\lib\Analysis\ScanfFormatString.cpp(221): > error C2143: syntax error : missing ';' before '{' > 4>..\..\..\..\..\..\llvm\tools\clang\lib\Analysis\ScanfFormatString.cpp(221): > error C2447: '{' : missing function header (old-style formal list?) > > - Michael Spencer > > > On Mon, Jul 26, 2010 at 3:45 PM, Ted Kremenek <[email protected]> wrote: >> Author: kremenek >> Date: Mon Jul 26 14:45:54 2010 >> New Revision: 109428 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=109428&view=rev >> Log: >> Hoist argument type checking into CheckFormatHandler. This is prep for >> scanf format >> string argument type checking. >> >> Modified: >> cfe/trunk/include/clang/Analysis/Analyses/FormatString.h >> cfe/trunk/lib/Analysis/FormatString.cpp >> cfe/trunk/lib/Analysis/ScanfFormatString.cpp >> cfe/trunk/lib/Sema/SemaChecking.cpp >> >> Modified: cfe/trunk/include/clang/Analysis/Analyses/FormatString.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/Analyses/FormatString.h?rev=109428&r1=109427&r2=109428&view=diff >> ============================================================================== >> --- cfe/trunk/include/clang/Analysis/Analyses/FormatString.h (original) >> +++ cfe/trunk/include/clang/Analysis/Analyses/FormatString.h Mon Jul 26 >> 14:45:54 2010 >> @@ -305,6 +305,8 @@ >> public: >> FormatSpecifier(bool isPrintf) >> : CS(isPrintf), UsesPositionalArg(false), argIndex(0) {} >> + >> + virtual ~FormatSpecifier(); >> >> void setLengthModifier(LengthModifier lm) { >> LM = lm; >> @@ -339,6 +341,17 @@ >> bool usesPositionalArg() const { return UsesPositionalArg; } >> >> bool hasValidLengthModifier() const; >> + >> + /// \brief Returns the type that a data argument >> + /// paired with this format specifier should have. This method >> + /// will an invalid ArgTypeResult if the format specifier does not have >> + /// a matching data argument or the matching argument matches >> + /// more than one type. >> + virtual ArgTypeResult getArgType(ASTContext &Ctx) const = 0; >> + >> + const ConversionSpecifier &getConversionSpecifier() const { >> + return CS; >> + } >> }; >> >> } // end analyze_format_string namespace >> @@ -438,9 +451,9 @@ >> return getConversionSpecifier().consumesDataArgument(); >> } >> >> - /// \brief Returns the builtin type that a data argument >> + /// \brief Returns the type that a data argument >> /// paired with this format specifier should have. This method >> - /// will return null if the format specifier does not have >> + /// will an invalid ArgTypeResult if the format specifier does not have >> /// a matching data argument or the matching argument matches >> /// more than one type. >> ArgTypeResult getArgType(ASTContext &Ctx) const; >> @@ -468,6 +481,11 @@ >> >> bool hasValidPrecision() const; >> bool hasValidFieldWidth() const; >> + >> + static bool classof(const analyze_format_string::FormatSpecifier *FS) { >> + return FS->getConversionSpecifier().isPrintfKind(); >> + } >> + >> }; >> } // end analyze_printf namespace >> >> @@ -492,6 +510,7 @@ >> } >> }; >> >> +using analyze_format_string::ArgTypeResult; >> using analyze_format_string::LengthModifier; >> using analyze_format_string::OptionalAmount; >> using analyze_format_string::OptionalFlag; >> @@ -523,6 +542,13 @@ >> bool consumesDataArgument() const { >> return CS.consumesDataArgument() && !SuppressAssignment; >> } >> + >> + /// \brief Returns the type that a data argument >> + /// paired with this format specifier should have. This method >> + /// will an invalid ArgTypeResult if the format specifier does not have >> + /// a matching data argument or the matching argument matches >> + /// more than one type. >> + ArgTypeResult getArgType(ASTContext &Ctx) const; >> >> static ScanfSpecifier Parse(const char *beg, const char *end); >> }; >> >> Modified: cfe/trunk/lib/Analysis/FormatString.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/FormatString.cpp?rev=109428&r1=109427&r2=109428&view=diff >> ============================================================================== >> --- cfe/trunk/lib/Analysis/FormatString.cpp (original) >> +++ cfe/trunk/lib/Analysis/FormatString.cpp Mon Jul 26 14:45:54 2010 >> @@ -379,9 +379,11 @@ >> } >> >> //===----------------------------------------------------------------------===// >> -// Methods on ConversionSpecifier. >> +// Methods on FormatSpecifier. >> //===----------------------------------------------------------------------===// >> >> +FormatSpecifier::~FormatSpecifier() {} >> + >> bool FormatSpecifier::hasValidLengthModifier() const { >> switch (LM.getKind()) { >> case LengthModifier::None: >> >> Modified: cfe/trunk/lib/Analysis/ScanfFormatString.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/ScanfFormatString.cpp?rev=109428&r1=109427&r2=109428&view=diff >> ============================================================================== >> --- cfe/trunk/lib/Analysis/ScanfFormatString.cpp (original) >> +++ cfe/trunk/lib/Analysis/ScanfFormatString.cpp Mon Jul 26 14:45:54 2010 >> @@ -217,4 +217,10 @@ >> return false; >> } >> >> +ArgTypeResult ScanfSpecifier::getArgType(ASTContext &Ctx) const { >> + // FIXME: Fill in. >> + return ArgTypeResult(); >> +} >> + >> + >> >> >> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=109428&r1=109427&r2=109428&view=diff >> ============================================================================== >> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original) >> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Mon Jul 26 14:45:54 2010 >> @@ -1174,6 +1174,11 @@ >> const analyze_format_string::ConversionSpecifier &CS, >> const char *startSpecifier, unsigned specifierLen, >> unsigned argIndex); >> + >> + void CheckArgType(const analyze_format_string::FormatSpecifier &FS, >> + const analyze_format_string::ConversionSpecifier &CS, >> + const char *startSpecifier, unsigned specifierLen, >> + unsigned argIndex); >> }; >> } >> >> @@ -1299,6 +1304,52 @@ >> return true; >> } >> >> +void CheckFormatHandler::CheckArgType( >> + const analyze_format_string::FormatSpecifier &FS, >> + const analyze_format_string::ConversionSpecifier &CS, >> + const char *startSpecifier, unsigned specifierLen, unsigned argIndex) { >> + >> + const Expr *Ex = getDataArg(argIndex); >> + const analyze_format_string::ArgTypeResult &ATR = >> FS.getArgType(S.Context); >> + >> + if (ATR.isValid() && !ATR.matchesType(S.Context, Ex->getType())) { >> + // Check if we didn't match because of an implicit cast from a 'char' >> + // or 'short' to an 'int'. This is done because scanf/printf are >> varargs >> + // functions. >> + if (const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(Ex)) >> + if (ICE->getType() == S.Context.IntTy) >> + if (ATR.matchesType(S.Context, ICE->getSubExpr()->getType())) >> + return; >> + >> + if (const analyze_printf::PrintfSpecifier *PFS = >> + dyn_cast<analyze_printf::PrintfSpecifier>(&FS)) { >> + // We may be able to offer a FixItHint if it is a supported type. >> + analyze_printf::PrintfSpecifier fixedFS(*PFS); >> + if (fixedFS.fixType(Ex->getType())) { >> + // Get the fix string from the fixed format specifier >> + llvm::SmallString<128> buf; >> + llvm::raw_svector_ostream os(buf); >> + fixedFS.toString(os); >> + >> + S.Diag(getLocationOfByte(CS.getStart()), >> + diag::warn_printf_conversion_argument_type_mismatch) >> + << ATR.getRepresentativeType(S.Context) << Ex->getType() >> + << getSpecifierRange(startSpecifier, specifierLen) >> + << Ex->getSourceRange() >> + << FixItHint::CreateReplacement( >> + getSpecifierRange(startSpecifier, specifierLen), os.str()); >> + } >> + else { >> + S.Diag(getLocationOfByte(CS.getStart()), >> + diag::warn_printf_conversion_argument_type_mismatch) >> + << ATR.getRepresentativeType(S.Context) << Ex->getType() >> + << getSpecifierRange(startSpecifier, specifierLen) >> + << Ex->getSourceRange(); >> + } >> + } >> + } >> +} >> + >> //===--- CHECK: Printf format string checking >> ------------------------------===// >> >> namespace { >> @@ -1570,47 +1621,8 @@ >> if (!CheckNumArgs(FS, CS, startSpecifier, specifierLen, argIndex)) >> return false; >> >> - // Now type check the data expression that matches the >> - // format specifier. >> - const Expr *Ex = getDataArg(argIndex); >> - const analyze_printf::ArgTypeResult &ATR = FS.getArgType(S.Context); >> - if (ATR.isValid() && !ATR.matchesType(S.Context, Ex->getType())) { >> - // Check if we didn't match because of an implicit cast from a 'char' >> - // or 'short' to an 'int'. This is done because printf is a varargs >> - // function. >> - if (const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(Ex)) >> - if (ICE->getType() == S.Context.IntTy) >> - if (ATR.matchesType(S.Context, ICE->getSubExpr()->getType())) >> - return true; >> - >> - // We may be able to offer a FixItHint if it is a supported type. >> - PrintfSpecifier fixedFS = FS; >> - bool success = fixedFS.fixType(Ex->getType()); >> - >> - if (success) { >> - // Get the fix string from the fixed format specifier >> - llvm::SmallString<128> buf; >> - llvm::raw_svector_ostream os(buf); >> - fixedFS.toString(os); >> - >> - S.Diag(getLocationOfByte(CS.getStart()), >> - diag::warn_printf_conversion_argument_type_mismatch) >> - << ATR.getRepresentativeType(S.Context) << Ex->getType() >> - << getSpecifierRange(startSpecifier, specifierLen) >> - << Ex->getSourceRange() >> - << FixItHint::CreateReplacement( >> - getSpecifierRange(startSpecifier, specifierLen), >> - os.str()); >> - } >> - else { >> - S.Diag(getLocationOfByte(CS.getStart()), >> - diag::warn_printf_conversion_argument_type_mismatch) >> - << ATR.getRepresentativeType(S.Context) << Ex->getType() >> - << getSpecifierRange(startSpecifier, specifierLen) >> - << Ex->getSourceRange(); >> - } >> - } >> - >> + CheckArgType(FS, CS, startSpecifier, specifierLen, argIndex); >> + >> return true; >> } >> >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
