This is an archive of the discontinued LLVM Phabricator instance.

Refactor how FunctionDecl handles constexpr:
AcceptedPublic

Authored by nwilson on Nov 18 2016, 7:59 PM.

Details

Summary
  • This distinguishes between whether a function has constexpr specified and if a function is implicitly constexpr (implicitly inline).
  • This also gives the ability to mark a function concept as implicitly constexpr (and implicitly inline)

Event Timeline

nwilson updated this revision to Diff 78616.Nov 18 2016, 7:59 PM
nwilson retitled this revision from to Refactor how FunctionDecl handles constexpr:.
nwilson updated this object.
include/clang/AST/Decl.h
1916

How is the inline property transmitted here? Why does the setImplicitlyConstexpr function need to call setImplicitlyInline?

1920

The quote does not seem to be pertinent here. Maybe have it a few lines down?

1924

I am quite sure this is not the right thing to do when IC is false.

lib/Sema/SemaDecl.cpp
8084–8085

The parenthetical here seems to be no longer needed.

9107

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.

test/CXX/concepts-ts/dcl.dcl/dcl.spec/dcl.spec.concept/p2.cpp
21

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).

nwilson added inline comments.Nov 22 2016, 8:24 PM
include/clang/AST/Decl.h
1916

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.

1920

Sorry, can you point to where you're thinking below?

1924

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!

lib/Sema/SemaDecl.cpp
8084–8085

I'll remove it.

9107

Hmm, I'm not sure if we should keep around setConstexpr to handle cases either... Do you or @rsmith have any opinion?

test/CXX/concepts-ts/dcl.dcl/dcl.spec/dcl.spec.concept/p2.cpp
21

Yeah, good point. I'll fix this.

nwilson updated this revision to Diff 80845.EditedDec 8 2016, 4:31 PM
  • Initialize IsConstexprSpecified
  • Remove unnecessary parenthetical in comment
  • Fix non-portable test
This comment was removed by nwilson.
rsmith added inline comments.Dec 20 2016, 4:47 PM
include/clang/AST/Decl.h
1924

Doing something automatic here with the inline specifier is turning out to be too complex. Instead, let's just remove the setImplicitlyInline() call from here and call it explicitly when handling a concept.

nwilson updated this revision to Diff 82194.Dec 20 2016, 7:37 PM
  • Remove the call to setImplicitlyInline() within setImplicitlyConstexpr() and call setImplicitlyInline() directly for function concepts.

Small Ping. @rsmith - did you have anymore thoughts about this patch?

rsmith accepted this revision.Dec 28 2016, 5:39 PM
rsmith edited edge metadata.
This revision is now accepted and ready to land.Dec 28 2016, 5:39 PM