[PATCH] [MSVC] Improved __noop support (https://llvm.org/bugs/show_bug.cgi?id=14081)

[Ilia]

This is a kind reminder to review the patch.


================
Comment at: lib/Parse/ParseExpr.cpp:1459-1464
@@ +1458,8 @@
+          if (LHS.isUsable()) {
+            Expr *Fn = LHS.get()->IgnoreParens();
+            if (DeclRefExpr *DRE = dyn_cast_or_null<DeclRefExpr>(Fn)) {
+              FunctionDecl *FnDecl =
+                  dyn_cast_or_null<FunctionDecl>(DRE->getDecl());
+              BuiltinId = (FnDecl ? FnDecl->getBuiltinID() : 0);
+            }
+          }
----------------
rsmith wrote:
> Code in Parse shouldn't be inspecting the AST like this. If you really need different parse rules for the argument of __noop, maybe make it a keyword rather than a builtin?
Firstly I tried to make __noop as a separate keyword however __noop has a builtin behavior in 95% cases. For example __noop will be a normal variable after "int __noop;" declaration. I suppose that it will be inefficient to detect all these cases for a new key-word.

================
Comment at: lib/Parse/ParseExpr.cpp:1466-1467
@@ -1457,1 +1465,4 @@
+          }
+          Sema::InsideNoopRAII SubstIndex(Actions,
+                                          BuiltinId == Builtin::BI__noop);
           if (ParseExpressionList(ArgExprs, CommaLocs, [&] {
----------------
rsmith wrote:
> Can you enter an unevaluated context here rather than inventing a new sort of context for this case? Global state in Sema doesn't seem like the right way to handle this -- if you trigger an instantiation from within this context, you should not instantiate the template in noop mode (it might end up being used outside noop mode too).
Entering unevaluated context doesn’t prevent from template instantiation here. Global state points that we are inside __noop parsing now.

================
Comment at: lib/Sema/SemaExpr.cpp:13055
@@ +13054,3 @@
+  // This behaviour is the same as Microsoft compiler behaviour.
+  if (!InsideNoop || !isa<FunctionDecl>(E->getDecl()))
+    MarkExprReferenced(*this, E->getLocation(), E->getDecl(), E, OdrUse);
----------------
rnk wrote:
> rnk wrote:
> > Why do we need to mark non-FunctionDecls referenced? Surely they can trigger template instantiation of other function decls.
> Can this just be `if (InsideNoop) return;` at the beginning of the function?
Non-FunctionDecls are marked as referenced because this prevents warnings about unused variables

================
Comment at: test/Sema/builtin-ms-noop-errors.cpp:16
@@ +15,3 @@
+  int var1, var2, var3, var4, var5, var6, var7, var8;
+  ((__noop))(function2<void>()); // expected-note{{'__noop' is a builtin with type 'int ()'}}
+  __noop(Z(), Z(), (function2<int>()), Z1(function2<int>()));
----------------
chatur01 wrote:
> I find checking this note weird. I know `__noop` is defined as taking `...` from the Builtins.def file, but `ASTContext::GetBuiltinType` decides to ignore it and generate the empty argument list instead. 
This patch doesn’t influence on this. I simply added this case (error + note) in a test. Deleted them.

http://reviews.llvm.org/D9000

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/