Page MenuHomePhabricator

clang-format: handle formatting on constexpr if
ClosedPublic

Authored by thegameg on Nov 21 2016, 7:20 PM.

Details

Summary

c++1z adds the following constructions to the language:

if constexpr (cond)
  statement1;
else if constexpr (cond)
  statement2;
else if constexpr (cond)
  statement3;
else
  statement4;

so clang-format needs to accept the constexpr keyword after the if
keyword.

  • lib/Format/TokenAnnotator.cpp: Handle Style.SpaceBeforeParens.
  • lib/Format/UnwrappedLineParser.cpp: Skip the token when parsing

ifThenElse.

  • unittests/Format/FormatTest.cpp: Tests.

Diff Detail

Event Timeline

thegameg updated this revision to Diff 78821.Nov 21 2016, 7:20 PM
thegameg retitled this revision from to clang-format: handle formatting on constexpr if.
thegameg updated this object.
thegameg added reviewers: djasper, mprobst, ioeric, klimek.
thegameg updated this object.
thegameg added a subscriber: cfe-commits.
ioeric removed a reviewer: ioeric.Nov 23 2016, 1:31 PM
ioeric added a subscriber: ioeric.

The actual problem is with that clang-format does not know anything about c++17 features, because it does not set it in lang options. Not sure if that will fix constexpr problem as well, but I could solve some other problems with

tools/clang                                                                                                                        ✓  13:45:12 
╰─ svn diff
Index: lib/Format/Format.cpp
===================================================================
--- lib/Format/Format.cpp	(revision 297506)
+++ lib/Format/Format.cpp	(working copy)
@@ -1893,6 +1893,7 @@
   LangOpts.CPlusPlus = 1;
   LangOpts.CPlusPlus11 = Style.Standard == FormatStyle::LS_Cpp03 ? 0 : 1;
   LangOpts.CPlusPlus14 = Style.Standard == FormatStyle::LS_Cpp03 ? 0 : 1;
+  LangOpts.CPlusPlus1z = Style.Standard == FormatStyle::LS_Cpp03 ? 0 : 1;
   LangOpts.LineComment = 1;
   bool AlternativeOperators = Style.IsCpp();
   LangOpts.CXXOperatorNames = AlternativeOperators ? 1 : 0;

any progress on this?

any progress on this?

It is getting tracked in mailing list http://lists.llvm.org/pipermail/cfe-dev/2017-April/053397.html

srhines added a subscriber: srhines.Apr 3 2017, 6:58 PM
thakis accepted this revision.May 10 2017, 8:27 AM
thakis added a subscriber: thakis.

This looks good to me, thanks. Sorry about the slow turnaround. Do you have commit access? If not, I can land it for you – but it also looks like you've contributed several patches by now, so you could also ask for commit access if you don't have it yet.

This revision is now accepted and ready to land.May 10 2017, 8:27 AM

This looks good to me, thanks. Sorry about the slow turnaround. Do you have commit access? If not, I can land it for you – but it also looks like you've contributed several patches by now, so you could also ask for commit access if you don't have it yet.

Thank you for the review. As @gnzlbg pointed out here (PR28867), I tested with AllowShortIfStatementsOnASingleLine and it does not seem to work as intended. I didn't have time to fix that yet, so I'll probably delay this for now. If anyone has time, feel free to fix + land this instead of me.

Hm, I probably should've searched first — but I just re-implemented this in D34330. Actually, I think my implementation solves the AllowShortIfStatementsOnASingleLine issue you were mentioning here 🎉

djasper closed this revision.Jun 19 2017, 12:42 AM
djasper edited edge metadata.

Submitted the other implementation of this as r305666.

Hm, I probably should've searched first — but I just re-implemented this in D34330. Actually, I think my implementation solves the AllowShortIfStatementsOnASingleLine issue you were mentioning here 🎉

No problem! Great!