cjdb accepted this revision. cjdb added a comment. This revision is now accepted and ready to land.
In D130894#3709431 <https://reviews.llvm.org/D130894#3709431>, @aaron.ballman wrote: > In D130894#3709027 <https://reviews.llvm.org/D130894#3709027>, @tbaeder wrote: > >> I don't really want to bike-shed about the presentation for too long... > > I understand the desire, but at the same time, the whole goal of this patch > is to improve the presentation of the diagnostic. You can't invite us all to > a bikeshed painting party and then ask us not to use our brushes, that's just > cruel! :-D Agreed. Getting the presentation of diagnostics right is critical. >> I'm fine with just removing the parens, since we present it like that in the >> error message as well anyway: >> >> ./assert.cpp:6:1: error: static assertion failed due to requirement ''c' >> == 'a'' >> static_assert('c' == 'a') > > That's still my preference as well. Splitting across multiple lines with hard > line breaks has some issues for some IDEs that want to display diagnostic > information in a listbox (IIRC), so I'd rather we avoid ad hoc use of line > breaks in diagnostics for the moment (it'd be easier/more appropriate if we > were making a diagnostic policy for using multiple lines, but that'd involve > an RFC and is more effort than I think is needed for this patch). Fair enough, no further objections on my part. >>> Any tests with macros? >> >> I can add some, but they should be handled transparently, with the usual >> "expanded from macro" note. > > I'm fine either way. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130894/new/ https://reviews.llvm.org/D130894 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
