[PATCH] D26882: Refactor how FunctionDecl handles constexpr:

[Nathan Wilson via cfe-commits]

nwilson added inline comments.


================
Comment at: include/clang/AST/Decl.h:1915
+    IsConstexprSpecified = IC;
+    IsConstexpr = IC;
+  }
----------------
hubert.reinterpretcast wrote:
> How is the `inline` property transmitted here? Why does the `setImplicitlyConstexpr` function need to call `setImplicitlyInline`?
`inline` isn't //really// transmitted here. When we create a `FunctionDecl` in `CreateNewFunctionDecl` we //could// use this rather than passing `isConstexpr` as a parameter to the constructor. `setImplicitlyConstexpr` is intended to be used downstream such as in `ActOnFunctionDeclarator` or `CheckExplicitlyDefaultedSpecialMember`.


================
Comment at: include/clang/AST/Decl.h:1919
+  /// Flag that this function as implicitly constexpr
+  /// C++11 [dcl.constexpr]p2: constexpr functions and constexpr constructors
+  /// are implicitly inline functions (7.1.2).
----------------
hubert.reinterpretcast wrote:
> The quote does not seem to be pertinent here. Maybe have it a few lines down?
Sorry, can you point to where you're thinking below?


================
Comment at: include/clang/AST/Decl.h:1923
+    IsConstexpr = IC;
+    setImplicitlyInline();
+  }
----------------
hubert.reinterpretcast wrote:
> I am quite sure this is not the right thing to do when `IC` is `false`.
Good point. I //could// do `if (IC) setImplicitlyInline();`, but that doesn't seem great with these smaller functions. Any suggestions by you or @rsmith would be great!


================
Comment at: lib/Sema/SemaDecl.cpp:8085
       // C++ Concepts TS [dcl.spec.concept]p2: Every concept definition is
       // implicity defined to be a constexpr declaration (implicitly inline)
+      NewFD->setImplicitlyConstexpr(true);
----------------
hubert.reinterpretcast wrote:
> The parenthetical here seems to be no longer needed.
I'll remove it.


================
Comment at: lib/Sema/SemaDecl.cpp:9107
       << FixItHint::CreateRemoval(DS.getConstexprSpecLoc());
-    FD->setConstexpr(false);
+    FD->setImplicitlyConstexpr(false);
   }
----------------
hubert.reinterpretcast wrote:
> This reads wrong to me. I get that the idea is that the function should not be semantically considered `constexpr`, but the choice of `setImplicitlyConstexpr` seems odd.
Hmm, I'm not sure if we should  keep around `setConstexpr` to handle cases either... Do you or @rsmith have any opinion?


================
Comment at: test/CXX/concepts-ts/dcl.dcl/dcl.spec/dcl.spec.concept/p2.cpp:21
+static_assert(FCSizeOfU4<unsigned int>(), "");
+static_assert(FCSizeOfU4<long>(), "size of argument not equal to expected (4)"); // expected-error {{static_assert failed "size of argument not equal to expected (4)"}}
+
----------------
hubert.reinterpretcast wrote:
> This does not strike me as being very portable (I may be mistaken about how `clang -cc1` works though). I think it would be much safer to use `char [8]` here (and in general for the size matching).
> 
Yeah, good point. I'll fix this.


https://reviews.llvm.org/D26882