penagos marked an inline comment as done. penagos added inline comments.
================ Comment at: clang/unittests/Format/FormatTest.cpp:7652 + verifyFormat("test < a - 1 >> 1;"); verifyFormat("test >> a >> b;"); ---------------- Quuxplusone wrote: > Quuxplusone wrote: > > IMO you should use `"test < a | b >> c;"` as your test case here, to > > reassure the reader that it doesn't depend on the fact that `... 1;` is > > visibly not a variable declaration. > > Personally I'd also like to see `"test<test<a | b>> c;"` tested on the very > > next line, to show off the intended difference between the two. (Assuming > > that I understand the intent of this patch correctly.) > > (I also switched to a bitwise operator just for the heck of it; that makes > > this expression just a //very tiny bit// less implausible — but still > > highly implausible, to the point where I question why we're special-casing > > it.) > Btw, a much-bigger-scope way to fix this would be to teach clang-format about > "input encoding" versus "output encoding." The only time clang-format should > //ever// be inserting space in the middle of `>>` is if it's translating > C++11-encoded input into C++03-encoded output. If the input is known to > already be C++03-encoded, then breaking up an `>>` token into a pair of `> >` > tokens is //guaranteed// to introduce a bug. > Right now, my impression is that clang-format has a concept of "output > encoding" (i.e. "language mode") but has no way of knowing the "input > encoding." Thanks for the feedback. Your 2 test suggestions make sense to me; I've updated the patch diff. I hadn't considered teaching clang-format input encoding, but that does sound like the preferable long term solution. This patch is intended to be a lightweight fix to fix a very narrow use case. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100778/new/ https://reviews.llvm.org/D100778 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits