This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] [PR43372] - clang-format shows replacements in DOS files when no replacement is needed
ClosedPublic

Authored by MyDeveloperDay on Sep 30 2019, 8:00 AM.

Details

Summary

This is a patch to fix PR43372 (https://bugs.llvm.org/show_bug.cgi?id=43372) - clang-format can't format file with includes, ( which really keep providing replacements for already sorted headers.)

A similar issue was addressed by @krasimir in D60199: [clang-format] Do not emit replacements while regrouping if Cpp includes are OK, however, this seemingly only prevented the issue when the files being formatted did not contain windows line endings (\r\n)

It's possible this is related to https://twitter.com/StephanTLavavej/status/1176722938243895296 given who @STL_MSFT works for!

As people often used the existence of replacements to determine if a file needs clang-formatting, this is probably pretty important for windows users

There may be a better way of comparing 2 strings and ignoring \r (which appear in both Results and Code), I couldn't choose between this idiom or the copy_if approach, but I'm happy to change it to whatever people consider more performant.

Diff Detail

Event Timeline

MyDeveloperDay created this revision.Sep 30 2019, 8:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2019, 8:00 AM
MyDeveloperDay edited the summary of this revision. (Show Details)Sep 30 2019, 8:01 AM
krasimir added inline comments.Sep 30 2019, 1:06 PM
clang/lib/Format/Format.cpp
1924

Here compartIgnoringCarriageReturns is a bit imprecise: there could be \r-s not followed by \n-s in the source code.
Since we have constructed result with newlines as \n-s in this code, I'd suggest to instead implement a function that replaces all occurrences of \r\n with \n in a string (something like "stripNewlineCRs") and have a comparison like result == stripNewlineCRs(...) here. That shouldn't be much harder than this approach and will be a bit more precise.
I've looked around the Format code for preexisting functionality we can reuse, but it seems most of the \r\n handling is concerned with outputting the right newline encoding and not with reading it.

Through experiments, I notice that both result and the Code contains "\r\n", because although we construct the results with "\n" the actual text of the include has I believe trailing "\r" or "\r\n", I notice this when I thought one fix I tried was to trim the include text.

`result += Imports[Index].Text.trim()``

However, this had the effect of dos2unixing the `result``` and the include section of a windows file

but I take your point about the "\r" without the "\r\n"

I'll change the code but I believe it will need to be

if (stripNewlineCRs(result) == stripNewlineCRs(...))

I'll send an updated review

Being specific about replacing "\r\n"'s with "\n"'s rather than removing just \r's

krasimir added inline comments.Oct 1 2019, 2:04 AM
clang/lib/Format/Format.cpp
1828

nit: missing trailing fullstop.

1832

The worst-case complexity of this code is quadratic, since std::string::replace is linear in the size of the resulting string.
If we have an include block of a few hundred lines, each a hundred or so characters, I could imagine that could easily add up.
I'd suggest constructing the result by appending to a fresh string to keep the overall complexity linear.
You can then take the argument by const reference.

1838

Maybe refine this comment: ignoring any \r, followed by \n in either strings.
Alternatively, since this function is just performing a comparison, consider directly inlining it in the if statements below (not too strong opinion).

MyDeveloperDay set the repository for this revision to rC Clang.
MyDeveloperDay added a project: Restricted Project.
MyDeveloperDay marked 4 inline comments as done.
krasimir accepted this revision.Oct 1 2019, 11:57 AM

Thank you!

This revision is now accepted and ready to land.Oct 1 2019, 11:57 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2019, 1:21 PM