[PATCH] D65043: [Format] Add C++20 standard to style options

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Sorry for taking so long here, I've been swamped.

Unfortunately there's a couple of pain points still:

- this change is (mostly?) about being able to turn off c++20 parsing, so you preserve old desirable formatting of c++17 code. But the tests don't contain any such code, and I haven't seen any in the review. What does a real example look like?
- I misunderstood, while coroutines are newly added to c++20 mode, we've been shipping c++20 formatting like `<=>` since at least 2017. So the arguments for giving `Cpp11` a silly back-compat meaning probably sholud cover c++20 features too.

At this point, would we be better off just:

- adding `'Latest' -> LS_Latest`
- mapping `'Cpp11' -> LS_Latest` with a backward-compatibility comment
- adding `'c++11' -> LS_cxx11` with sensible semantics (name matches clang's LangStandards.def)
- adding `'c++14' -> LS_cxx14` etc
- keeping `Auto` as today



================
Comment at: clang/include/clang/Format/Format.h:1875
     LS_Cpp03,
-    /// Use features of C++11, C++14 and C++1z (e.g. ``A<A<int>>`` instead of
-    /// ``A<A<int> >``).
+    /// Use C++11-compatible syntax. For backwards-compatibility with older
+    /// versions of clang-format, this option implies C++14- and
----------------
If we're going to make "Cpp11" exclude Cpp2a, we should probably try to stop this problem occurring again, by including `LS_Latest`, I think.

But I'm not sure we should exclude 2a, see below...


================
Comment at: clang/lib/Format/Format.cpp:2386
   LangOpts.CPlusPlus14 = LexingStd >= FormatStyle::LS_Cpp11;
   LangOpts.CPlusPlus17 = LexingStd >= FormatStyle::LS_Cpp11;
+  LangOpts.CPlusPlus2a = LexingStd >= FormatStyle::LS_Cpp2a;
----------------
I think this should be `LexingStd >= Cpp17 || LexingStd == LS_Cpp11`. no need to have weird behavior for the new Cpp14 value.

Similarly the previous line is probably clearer (though equivalent) as `Std >= 14 || Std == 11`


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:2865
     return Right.is(TT_TemplateCloser) && Left.is(TT_TemplateCloser) &&
-           (Style.Standard < FormatStyle::LS_Cpp11 || Style.SpacesInAngles);
+           (Style.Standard == FormatStyle::LS_Cpp03 || Style.SpacesInAngles);
   }
----------------
<=


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:2884
     return (Left.is(TT_TemplateOpener) &&
-            Style.Standard < FormatStyle::LS_Cpp11) ||
+            Style.Standard == FormatStyle::LS_Cpp03) ||
            !(Left.isOneOf(tok::l_paren, tok::r_paren, tok::l_square,
----------------
<=


================
Comment at: clang/unittests/Format/FormatTest.cpp:13780
+  // In C++2a, it is interpreted as a prefix increment on 'i'.
+  verifyFormat("co_yield++ i;");
+  verifyFormat("co_yield ++i;", Cpp2a);
----------------
modocache wrote:
> Quuxplusone wrote:
> > My comment https://reviews.llvm.org/D65043#inline-582865 still stands. Please just write
> > 
> >     verifyFormat("f(co_yield - 1);");
> >     verifyFormat("f(co_yield -1);", Cpp2a);
> > 
> Unfortunately I think there's a separate issue here, that `co_yield -1` is formatted as `co_yield - 1` no matter which C++ standard is used. I'd like to address that in a separate commit, if that's alright. For now, I removed the test verifying the C++17 behavior, and instead only test the correct C++20 formatting of `co_yield ++it`.
So unfortunately as this stands, it doesn't seem to test that the 2a flag makes any difference. What exactly is the formatting of non-2a code you're trying to preserve? Can we test that?


================
Comment at: clang/unittests/Format/FormatTest.cpp:3811
       "if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
       "        aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) <=> 5) {\n"
+      "}", Cpp2a);
----------------
This test has been around for a couple of years, I guess we've been treating Cpp11 as triggering the 2a parser since then. This looks like a regression.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65043/new/

https://reviews.llvm.org/D65043