HazardyKnusperkeks added a comment. In D143546#4114341 <https://reviews.llvm.org/D143546#4114341>, @owenpan wrote:
> In D143546#4113721 <https://reviews.llvm.org/D143546#4113721>, @rymiel wrote: > >> In D143546#4112077 <https://reviews.llvm.org/D143546#4112077>, @owenpan >> wrote: >> >>> As this one is an invalid-code-generation bug, I wanted it fixed ASAP. >> >> Do you intend to backport it to the 16 release branch then? > > I don't know but will go with whatever you, @MyDeveloperDay and > @HazardyKnusperkeks prefer. I'd vote in favor, letting a code breaking bug knowingly in the most current (or to come? I don't follow that.) version would be let's say not nice. ================ Comment at: clang/lib/Format/TokenAnnotator.cpp:3884 if (Style.isCpp()) { + if (Right.is(tok::period) && Left.is(tok::numeric_constant)) + return true; ---------------- owenpan wrote: > rsmith wrote: > > owenpan wrote: > > > HazardyKnusperkeks wrote: > > > > Add a comment what that is? Without the bug report I'd not know what > > > > that sequence would be. > > > I could do that, but the github issue is linked in the summary above and > > > will be in the commit message. In general, I don't like unnecessary > > > comments littered in the source. They can become outdated, out of place, > > > misleading, and even wrong. How about giving an example as shown above? > > Does `clang-format` have any formatting modes where it would leave out > > spaces around `+` or `-`? The same issue arises with things like `0xe + n`, > > where removing the space between the `0xe` and the `+` results in a token > > splice. > No. I was aware of them and had made sure clang-format already handled them > correctly. > I could do that, but the github issue is linked in the summary above and will > be in the commit message. In general, I don't like unnecessary comments > littered in the source. They can become outdated, out of place, misleading, > and even wrong. How about giving an example as shown above? If you do a `git blame` you can come to the commit and thus to the bug, but if you are just reading, at least I don't blame all the lines. There I think a comment is nice, and your proposal is a really nice one. But I've accepted the patch as is, and so the decision is yours. :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143546/new/ https://reviews.llvm.org/D143546 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
