rnk added inline comments. ================ Comment at: include/clang/Basic/DiagnosticGroups.td:294 @@ -293,2 +293,3 @@ def SelTypeCast : DiagGroup<"cast-of-sel-type">; +def CastCallingConvention : DiagGroup<"cast-calling-convention">; def FunctionDefInObjCContainer : DiagGroup<"function-def-in-objc-container">; ---------------- thakis wrote: > nit: since this isn't referenced in this file, consider using an unnamed > inline `InGroup<DiagGroup<"cast-calling-convention">` in the other file > instead Sure. I dono, I never really liked that style since it makes it harder to add it to a group later.
================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6516 @@ +6515,3 @@ + "cast between incompatible calling conventions '%0' and '%1'">, + InGroup<CastCallingConvention>, DefaultIgnore; +def note_change_calling_conv_fixit : Note< ---------------- thakis wrote: > I'd build a large codebase of your choice with this enabled and see how it > does. If it does well I'd land it default-on. (I spent some time enabling > DefaultIgnore warnings that nobody turned on after they landed recently, and > I'd prefer to not having to do this for this warning too :-) ) My attempt to bait you into doing this work has failed. :) I'd still like to land it as ignored-by-default so I can run this warning across Chromium with a try job instead of doing it locally on my workstation. Sound good? ================ Comment at: lib/Sema/SemaCast.cpp:1752-1753 @@ +1751,4 @@ + Expr *Src = SrcExpr.get()->IgnoreParenImpCasts(); + if (auto *AddrOf = dyn_cast<UnaryOperator>(Src)) + Src = AddrOf->getSubExpr()->IgnoreParenImpCasts(); + auto *DRE = dyn_cast<DeclRefExpr>(Src); ---------------- rtrieu wrote: > Which UnaryOperator are you attempting to strip off here? This one will > strip off every UnaryOperator. Oops, added a test for that. ================ Comment at: lib/Sema/SemaCast.cpp:1762 @@ +1761,3 @@ + + // The source expression is a pointer to a known function defined in this TU. + // Diagnose this cast, as it is probably bad. ---------------- thakis wrote: > If it's an inline function in a header, would we want to warn? Should this > check if the body's SourceLocation is from the main file? Yeah, we do want to warn even if it's from a header. If it were defined elsewhere with a different convention, that'd be an ODR violation. ================ Comment at: lib/Sema/SemaCast.cpp:1775 @@ +1774,3 @@ + Self.Diag(NameLoc, diag::note_change_calling_conv_fixit) + << FD << DstCCName << FixItHint::CreateInsertion(NameLoc, CCAttrText); +} ---------------- thakis wrote: > Consider doing something like r164858 did to see if there's a define for the > calling convention (WINAPI or what have you) and suggest that instead Done, that was fun. :) http://reviews.llvm.org/D17348 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
