[cfe-commits] PATCH #2 for "[sema++] clang should give a fixit for */& mismatch"

[Douglas Gregor]

On Jul 22, 2011, at 11:41 AM, Anna Zaks wrote:

> Attached are 3 patches that add more support for adding */& mismatch fixits. In particular, C function parameters would get handled as well as assignments in general.
> 
> 1) In the first patch, I refactored most of the fixit generation functionality out of SemaOverload and provided a simple conversion checking function. This patch also includes a bug fix - the range for FixItHint::CreateRemoval was not specified correctly.
> <deref_addof_refactor.diff>

+  typedef bool (*TypeComparisonFuncTy) (const CanQualType FromTy,
+                                        const CanQualType ToTy,
+                                        Sema &S,
+                                        SourceLocation Loc,
+                                        ExprValueKind VK);

Could you name 'VK' something like 'FromVK', so we know which type the value kind refers to?

+  bool NeedParen = true;
+  if (isa<ParenExpr>(FullExpr) ||
+      isa<DeclRefExpr>(Expr) ||
+      isa<ArraySubscriptExpr>(Expr) ||
+      isa<CallExpr>(Expr) ||
+      isa<MemberExpr>(Expr))
+    NeedParen = false;
+  if (const UnaryOperator *UO = dyn_cast<UnaryOperator>(Expr))
+    if (UO->isPostfix())
+      NeedParen = false;

I'd prefer if you did an audit of the various expression kinds we have in Clang, so that you can white-list all of the ones that don't need to be parenthesized here. Otherwise, we're setting ourselves up for a long tail of QoI bugs where Clang suggests parentheses unnecessarily. (We don't need test cases for every expression kind noted here, of course).

Index: lib/Sema/SemaFixItUtils.cpp
===================================================================
--- lib/Sema/SemaFixItUtils.cpp	(revision 0)
+++ lib/Sema/SemaFixItUtils.cpp	(revision 0)
@@ -0,0 +1,145 @@
+//===--- SemaFixItUtils.cpp - Sema FixIts ---------------------------------===//

Please add this .cpp file to lib/Sema/CMakeLists.txt.

With those minor tweaks, please go ahead and commit this patch!

> 2) This one adds fixit generation to the  Sema::DiagnoseAssignmentResult().
> <deref_addof_assign.diff>

> 3) Test the fixits generated as a result of  patch #2.
> <deref_addof_assign_tests.diff>


These two look great, thanks!

	- Doug
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110727/1f1c87eb/attachment.html>