[PATCH] D116859: [clang-format] Fix break being added to macro define with ColumnLimit: 0

[Owen Pan via Phabricator via cfe-commits]

owenpan accepted this revision.
owenpan added inline comments.


================
Comment at: clang/unittests/Format/FormatTest.cpp:4810
+
+  verifyFormat("#define A LOOOOOOOOOOOOOOOOOOONG() LOOOOOOOOOOOOOOOOOOONG()\n",
+               ZeroColumn);
----------------
Please remove the newline and re-run `FormatTests`.


================
Comment at: clang/unittests/Format/FormatTest.cpp:4810-4811
+
+  verifyFormat("#define STRINGIFY(t) #t\n"
+               "#define MAKEVERSIONSTRING(x, y, z, build) STRINGIFY(x) \".\" STRINGIFY(y) \".\" STRINGIFY(z) \".\" STRINGIFY(build)\n",
+               ZeroColumn);
----------------
curdeius wrote:
> HazardyKnusperkeks wrote:
> > curdeius wrote:
> > > Please test with something simpler.
> > Did this show the bugged behavior without the patch?
> > 
> > Regarding the `// clang-format off` there is the question what do we want in the tests? Show the long lines as the long lines, then we need to turn it off some times. Or do we want to keep the limit and break the strings, thus making it harder to read for the human?
> > Did this show the bugged behavior without the patch?
> Yes. I checked with a pretty fresh master.
> 
> > Regarding the `// clang-format off` there is the question what do we want in the tests? Show the long lines as the long lines, then we need to turn it off some times. Or do we want to keep the limit and break the strings, thus making it harder to read for the human?
> I'm pretty much against adding `// clang-format off` too, it hurts more than it helps IMO.
> Regarding the `// clang-format off` there is the question what do we want in the tests? Show the long lines as the long lines, then we need to turn it off some times. Or do we want to keep the limit and break the strings, thus making it harder to read for the human?

I prefer the latter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116859