[PATCH] D17348: Add -Wcast-calling-convention to warn when casting away calling conventions

[Reid Kleckner via cfe-commits]

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