[PATCH] D80950: [clang-format] [PR44542,38872] String << String always get a forced newline.

2020-06-04 Thread Krasimir Georgiev via Phabricator via cfe-commits
krasimir added a comment. Another //weak// argument about why the current rule makes sense (break before `<<` between two string literals) is: suppose you don't want such a break. Then you can //change your code// to concatenate the two string literals into a single one, avoiding the problem

[PATCH] D80950: [clang-format] [PR44542,38872] String << String always get a forced newline.

2020-06-04 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment. I couldn't quite parse the last comment about \n, but yes the endl handling (didn't realize it also covered \n!) is implemented separately but falls under the same "heuristic formatting" umbrella I think. > LOG_IF(DFATAL, a < b) << "Equality condition can never be

[PATCH] D80950: [clang-format] [PR44542,38872] String << String always get a forced newline.

2020-06-04 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment. In D80950#2072926 , @MyDeveloperDay wrote: > The same function with smaller variables isn't quite so readable > > LOG_IF(DFATAL, a < b) << "Equality condition can never be satisfied:" > << " foo=" << a

[PATCH] D80950: [clang-format] [PR44542,38872] String << String always get a forced newline.

2020-06-04 Thread MyDeveloperDay via Phabricator via cfe-commits
MyDeveloperDay added a comment. Just to add, I also think although because there are no tests to enforce it the primary reason is for when we see '\n', breaking the stream on endl but this is actually covered by another rule this also in mustBreak() if (Current.is(tok::lessless) &&

[PATCH] D80950: [clang-format] [PR44542,38872] String << String always get a forced newline.

2020-06-04 Thread MyDeveloperDay via Phabricator via cfe-commits
MyDeveloperDay added a comment. @sammccall in the example LOG_IF(DFATAL, exp_await_calls < num_await_calls_) << "Equality condition can never be satisfied:" << " exp_await_calls=" << exp_await_calls << " num_await_calls_=" << num_await_calls_; I agree this is more pleasing,

[PATCH] D80950: [clang-format] [PR44542,38872] String << String always get a forced newline.

2020-06-03 Thread MyDeveloperDay via Phabricator via cfe-commits
MyDeveloperDay added a comment. > IMO this *is* actually a good heuristic: `<<` between string literals > indicates some intent to break, and when the stream is entirely literals the > formatting is good. I take your point, how do you feel about an option even if the default was

[PATCH] D80950: [clang-format] [PR44542,38872] String << String always get a forced newline.

2020-06-03 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment. IMO this *is* actually a good heuristic: `<<` between string literals indicates some intent to break, and when the stream is entirely literals the formatting is good. (@krasimir threw this into our regression testing, and where the formatting was different, it was

[PATCH] D80950: [clang-format] [PR44542,38872] String << String always get a forced newline.

2020-06-03 Thread Sam McCall via Phabricator via cfe-commits
sammccall added a comment. (Oops, hit enter too soon here... will edit the rest in) Lack of tests is bad :-( This is http://github.com/llvm/llvm-project/commit/2603ee0dc6003 which dates back to clang-format early days, before there was good test coverage. Obviously we can add tests for the

[PATCH] D80950: [clang-format] [PR44542,38872] String << String always get a forced newline.

2020-06-03 Thread Marek Kurdej via Phabricator via cfe-commits
curdeius added a comment. I would have nothing against if you'd added this option and we kept current behaviour by default. The only drawback is the (bit of) complexity it adds bit that seems justified to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D80950: [clang-format] [PR44542,38872] String << String always get a forced newline.

2020-06-03 Thread MyDeveloperDay via Phabricator via cfe-commits
MyDeveloperDay added a comment. In D80950#2069733 , @curdeius wrote: > The change seems to me technically sound, but I'm not sure of the scope of > its effects. There might be users that rely on this behavior. On the other > hand, adding an option to

[PATCH] D80950: [clang-format] [PR44542,38872] String << String always get a forced newline.

2020-06-02 Thread Marek Kurdej via Phabricator via cfe-commits
curdeius added a comment. The change seems to me technically sound, but I'm not sure of the scope of its effects. There might be users that rely on this behaviour. On the other hand, adding an option to keep the old behaviour doesn't seem appropriate, and personally I consider the old

[PATCH] D80950: [clang-format] [PR44542,38872] String << String always get a forced newline.

2020-06-01 Thread MyDeveloperDay via Phabricator via cfe-commits
MyDeveloperDay created this revision. MyDeveloperDay added reviewers: krasimir, sammccall, klimek, curdeius, JakeMerdichAMD, Abpostelnicu. MyDeveloperDay added projects: clang, clang-format. Herald added a reviewer: bollu. https://bugs.llvm.org/show_bug.cgi?id=44542