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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
